CBG-5496: Mark background process state as error on synchronous start failure#8454
CBG-5496: Mark background process state as error on synchronous start failure#8454RIT3shSapata wants to merge 1 commit into
Conversation
- Mark the process state as Error when Process.Init fails synchronously, instead of leaving it stuck on Running. - Mark the process state as Error when the post-Init cluster status persist fails, since Process.Run is already running in the background by that point. - Add StopFromErrorState and route Stop() to it when in the Error state, so it transitions to Stopped instead of being a no-op. - Reset h.db.State from DBResyncing back to DBOffline when stopping a resync that never got past Init, since Run's own reset never fires in that case. - Add unit tests covering Init failure across all three BackgroundManager modes (local, single-node, multi-node) and Stop from that state. - Add a unit test covering the case where the initial status persist fails after Process.Run has already started.
There was a problem hiding this comment.
Pull request overview
This PR fixes a resync background-process lifecycle bug where synchronous start failures (e.g. invalid collection filters) could leave the resync process/state stuck as “running”, blocking subsequent resync operations until manual cleanup. It extends BackgroundManager error-state handling and adds test coverage for the affected failure modes.
Changes:
- Mark
BackgroundManagerasErrorwhenProcess.Initfails synchronously, and also when initial cluster-status persistence fails afterRunhas already started. - Add an
Error-recovery stop path (StopFromErrorState) so_resync?action=stopcan reliably clear errored processes. - Add unit/integration tests covering init failures and cluster-status persistence failures for resync/background manager.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
db/background_mgr.go |
Updates start/stop state transitions to correctly enter Error on synchronous init/persist failures and adds stop recovery from Error. |
rest/api.go |
Adjusts _resync?action=stop handler to reset DB state when stopping an error-state resync that never reached Run(). |
db/background_mgr_test.go |
Adds unit tests for init-error behavior across manager modes and for persist-failure-after-run-start behavior. |
rest/adminapitest/resync_test.go |
Adds admin API test verifying _resync behavior when started with invalid collection names and subsequent stop/restart flow. |
| initMode, err := b.Process.Init(ctx, options, processClusterStatus) | ||
| if err != nil { | ||
| // markStart already set state to Running before Init ran, so without this the manager would be | ||
| // stuck reporting Running even though it never actually started. Mirrors the Process.Run error | ||
| // handling below. | ||
| b.SetError(err) | ||
| return err | ||
| } |
There was a problem hiding this comment.
I think something like this might be true - I think you can write a test for this scenario.
I propose changing this to something like:
// markStart already set state to Running (and, for single-node mode, registered a heartbeat)
// before Init ran, but the process never actually started - Process.Run is never invoked in this
// path. Reset back to the uninitialized status (as if Start had never been called) and report the
// failure directly to the caller, rather than leaving the manager in the Error state where it
// would need an explicit Stop to become startable again.
b.Terminate()
b.clearStatus()
if b.mode() == backgroundManagerModeSingleNode {
_ = b.clusterAwareOptions.metadataStore.Delete(ctx, b.clusterAwareOptions.HeartbeatDocID())
}
where:
// clearStatus resets the in-memory status back to its uninitialized zero value, e.g. to undo an aborted start attempt.
func (b *BackgroundManager[O]) clearStatus() {
b.statusLock.Lock()
defer b.statusLock.Unlock()
b.status = BackgroundManagerStatus{}
}
The idea comes from the comment below where I don't think we want to require stop to be called or change any of hte start code. I think with this change, all of the other changed code around Stop and api.go can be dropped.
| // h.db.State is normally reset from DBResyncing back to DBOffline by a deferred call inside | ||
| // ResyncManagerDCP.Run once it returns - but Run is never invoked when Init failed (leaving the | ||
| // process in the Error state), so that reset never happens. Capture the status here, before | ||
| // Stop() changes it, so we can tell afterward whether we're recovering from that case and need to | ||
| // do the DBState reset ourselves. |
There was a problem hiding this comment.
This comment raised the idea that the original idea I had was wrong for how to fix this, I do not like this being the way thing works.
I think a better way of things working would be.
- /db/_resync?action=start with invalid collections -> (optimally this would return 400, but this can be out of scope, I'm not sure that all situations warrant returning 400 if Init fails, we could pull this into a separate PR)
- /db/_resync?action=start with invalid collections runs
It would be non intuitive that you have to stop a resync that was never running in the first place.
CBG-5496
Fixes a bug where passing invalid collection names to
POST /db/_resync?action=startleft the background process permanently stuck reportingrunning, even though it had actually failed. Since the process was never marked as errored, subsequent_resynccalls (start or otherwise) kept getting rejected with "already in progress" until the underlying heartbeat/status state was manually cleared.BackgroundManagernow transitions to theErrorstate whenProcess.Initfails synchronously (e.g. invalid collections), instead of being left stuck on theRunningstate thatmarkStarthad already set beforeInitran.BackgroundManageralso transitions toErrorif the initial cluster status persist fails right afterInitsucceeds - by that pointProcess.Runis already executing in its own goroutine, so without this the caller would see an error while the process kept running unbeknownst to them.Stop()had no concept of anErrorstate to recover from —markStoptreatedErrorthe same as any other terminal state (Completed/Stopped) and returned early as a no-op, so a process stuck inErrorhad no path back toStoppedat all; only a freshStart()(whichmarkStartdoes allow fromError) could move it anywhere.StopFromErrorState, andStop()now routes into it whenever the process is in theErrorstate. It exists because the normalRunning→Stopping→Stoppedprogression only happens afterProcess.Runreturns (handled in the wrapper goroutine instart()) — but several error paths (likeInitfailing) meanRunis never invoked in the first place, so that progression never has a chance to fire.StopFromErrorStatefills that gap by explicitly moving the process straight toStopped, updating the cluster-aware status doc so other nodes see the change, and cleaning up the heartbeat doc so a subsequentStart()isn't blocked.Errornever resolved toStoppedon its own (that transition normally only happens afterProcess.Runreturns, which never happens ifInitfailed), a resync that ends up in theErrorstate now must be explicitly stopped via_resync?action=stopbefore a new_resync?action=startwill succeed. Previously this same restriction existed too, just as an unintended side effect of the stuck state rather than a designed recovery path.rest/api.go's resync stop handler now captures the status before callingStop()and resets the database-levelDBResyncingstate back toDBOfflineif that prior status wasError, since the normal reset (a deferred call insideResyncManagerDCP.Run) never fires whenRunwas never invoked.db/background_mgr_test.go:Initfailing across all threeBackgroundManagermodes (local, single-node cluster-aware, multi-node cluster-aware), followed byStop.Process.Runhas already been launched.Pre-review checklist
fmt.Print,log.Print, ...)base.UD(docID),base.MD(dbName))docs/apiDependencies (if applicable)
Integration Tests