[go: up one dir, main page]

housekeeping: Check nil interface using reflect

Fixes this broken pipeline: gitlab!166025 (closed)

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 (b0d13e05) 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 (b0d13e05).

In !7263 (merged), 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 (b0d13e05) 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.

Edited by James Liu

Merge request reports

Loading