[4.1.2 Backport] CBG-5594: fix BackgroundManager.Stop transition - #8632
Conversation
There was a problem hiding this comment.
Pull request overview
Backport of the upstream fix for a multi-node BackgroundManager.Stop() convergence bug where non-initiating nodes could incorrectly transition their local state to completed and overwrite the correct stopped cluster status.
Changes:
- Replace the
errBackgroundManagerStatusNotRunningsentinel with a typed error carrying the observed cluster state, and update call sites to use type-based matching. - Ensure poller nodes adopt the cluster’s terminal/stopping state locally before terminating, and only default to
completedwhen the local state is stillrunningat process completion. - Update existing unit tests to assert the new error type via
require.ErrorAs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
db/background_mgr.go |
Fixes multi-node stop-state convergence by adopting cluster terminal state locally; refactors not-running error into a typed error; adjusts completion/state persistence logic and logging. |
db/background_mgr_test.go |
Updates assertions to accommodate the typed errBackgroundManagerStatusNotRunning error. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This doesn't seem like good justification for not porting over the tests and this has cropped up repeatedly now. The backport skills is just making the wrong choice here IMO. Either we backport the testing packages to make the backports simpler to cherry pick - or we special case this and make the backport skill adapt the tests (which should only require minimal changes to make work with testify?) |
Pull Request is not mergeable
Pull Request is not mergeable
ec13fe6 to
95f9d1a
Compare
Pull Request is not mergeable
Pull Request is not mergeable
Cherry-picked from 4ec1dd2 (CBG-5521, #8468). db/background_mgr.go applies identically to upstream. One adaptation in db/background_mgr_test.go: upstream's new tests are typed against MockProcessOptions, introduced by CBG-5653 (#8490, "switch backgroundmanager options to strong types"), which is not on 4.1.2. Applied against BackgroundManager[map[string]any] with map[string]any{} options in six places, matching the rest of the file on this branch. No assertions changed. The import block keeps 4.1.2's github.com/stretchr/testify/{assert,require} rather than the testing/{assert,require} wrappers, which the branch predates; this commit's own import change, adding testing/sgtest, was applied. Everything else applied unchanged, including the rename of TestBackgroundManagerMultiNodePollingAvoidsOverwrite to TestBackgroundManagerMultiNodePollingConverges with its CBG-5660 skip, the errBackgroundManagerStatusNotRunning sentinel-to-type change with its errors.AsType call sites, and TestBackgroundManagerConcurrentStopStartRace. Co-authored-by: Tor Colvin <tor.colvin@couchbase.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CBG-5594
Unclean cherry pick of #8468 (CBG-5521) to 4.1.2
Stacked on #8631 (CBG-5743), which sits on #8660 (CBG-5752) and #8635 (CBG-5744).
Changes from main commit:
db/background_mgr_test.go— upstream's new tests are typed againstMockProcessOptions, added by CBG-5653 (CBG-5653 switch backgroundmanager options to strong types #8490) which is not on 4.1.2. Applied againstBackgroundManager[map[string]any]in six places, matching the rest of the file on this branch; no assertions changed.db/background_mgr.gois identical to upstream.Integration Tests
🤖 Opened with the
sync-gateway-backportskill in Claude Code