From f07298c77ebc99918c93b15d3460f0cb9530c541 Mon Sep 17 00:00:00 2001 From: James Liu Date: Mon, 16 Sep 2024 16:52:26 +1000 Subject: [PATCH] housekeeping: Check nil interface using reflect Housekeeping tasks can execute with or without transactions enabled. `maybeStartTransaction()` checks whether transactions are enabled by checking if the node manager is available, and short-circuits the transaction handling if it's not. Prior to b0d13e05, the partition manager provided to the housekeeping manager in serve.go would've been nil unless transactions were enabled, or pending WAL entries may be suspected. The `partitionMgr` we create here (https://gitlab.com/gitlab-org/gitaly/-/commit/b0d13e051daee7749190ca330b7109044763c994#b8c0783add550ab0382189efdcaf870f14006aa1_480_481) is a shadowed variable due to the :=. In b0d13e05, `partitionMgr` was replaced by `node`, and instead of correctly shadowing the outer variable, we overwrote it here (https://gitlab.com/gitlab-org/gitaly/-/commit/b0d13e051daee7749190ca330b7109044763c994#b8c0783add550ab0382189efdcaf870f14006aa1_482_481). In https://gitlab.com/gitlab-org/gitaly/-/merge_requests/7263, we fixed this problem by renaming `node` to `nodeMgr` which effectively restores the old behaviour. Unfortunately, b0d13e05 also made the node field in the housekeeping manager an interface type (https://gitlab.com/gitlab-org/gitaly/-/commit/b0d13e051daee7749190ca330b7109044763c994#ac92eb5fd5b43ef5972598346ed9dd0c3c8a19c2_221_219) where previously we used a struct pointer. This means that our nil check to short circuit transaction handling wasn't working as expected; we would continue to execute the rest of the function body and then panic when we attempted to call `tx.Commit`, since `tx` would've also been nil. Our tests didn't catch this because they explicitly provide the `nil` value to the housekeeping manager (https://gitlab.com/gitlab-org/gitaly/blob/b0d13e051daee7749190ca330b7109044763c994/internal/git/housekeeping/manager/testhelper_test.go#L124-L127) whereas we provide a nil struct in production. Update the nil check using reflection so we can handle both situations. --- internal/git/housekeeping/manager/optimize_repository.go | 3 ++- .../git/housekeeping/manager/optimize_repository_test.go | 3 ++- internal/git/housekeeping/manager/testhelper_test.go | 5 ++++- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/internal/git/housekeeping/manager/optimize_repository.go b/internal/git/housekeeping/manager/optimize_repository.go index 838ad454e4..095aee5431 100644 --- a/internal/git/housekeeping/manager/optimize_repository.go +++ b/internal/git/housekeeping/manager/optimize_repository.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "os" + "reflect" "gitlab.com/gitlab-org/gitaly/v16/internal/git/gitcmd" "gitlab.com/gitlab-org/gitaly/v16/internal/git/housekeeping" @@ -98,7 +99,7 @@ func (m *RepositoryManager) OptimizeRepository( } func (m *RepositoryManager) maybeStartTransaction(ctx context.Context, repo *localrepo.Repo, run func(context.Context, storage.Transaction, *localrepo.Repo) error) error { - if m.node == nil { + if m.node == nil || reflect.ValueOf(m.node).IsNil() { return run(ctx, nil, repo) } diff --git a/internal/git/housekeeping/manager/optimize_repository_test.go b/internal/git/housekeeping/manager/optimize_repository_test.go index 1cd9c94246..7663d04fb5 100644 --- a/internal/git/housekeeping/manager/optimize_repository_test.go +++ b/internal/git/housekeeping/manager/optimize_repository_test.go @@ -7,6 +7,7 @@ import ( "fmt" "os" "path/filepath" + "reflect" "runtime" "sync" "testing" @@ -1089,7 +1090,7 @@ func TestOptimizeRepository(t *testing.T) { require.Equal(t, setup.expectedErr, err) expectedMetrics := setup.expectedMetrics - if node != nil && setup.expectedMetricsForTransaction != nil { + if node != nil && !reflect.ValueOf(node).IsNil() && setup.expectedMetricsForTransaction != nil { expectedMetrics = setup.expectedMetricsForTransaction } // If object pool is enabled, asserting transaction's metrics using diff --git a/internal/git/housekeeping/manager/testhelper_test.go b/internal/git/housekeeping/manager/testhelper_test.go index 74317abd78..c91448bdba 100644 --- a/internal/git/housekeeping/manager/testhelper_test.go +++ b/internal/git/housekeeping/manager/testhelper_test.go @@ -122,8 +122,11 @@ func testWithAndWithoutTransaction(t *testing.T, desc string, testFunc func(*tes }) t.Run("without transaction", func(t *testing.T) { + // Make sure to pass a nil struct rather than an explicit `nil` value to the housekeeping + // manager, as this exercises our nil interface check using reflection. + var node *nodeimpl.Manager cfg := testcfg.Build(t) - testFunc(t, cfg, nil) + testFunc(t, cfg, node) }) }) } -- GitLab