fix(lifecycle): publish the live run before announcing StatusRunning (#2806) - #2812
Merged
Conversation
Fixes #2806. Between UpdateStatus(StatusRunning) and Start's runningPipelines.Set — which happened only after runPipeline returned — the map still pointed at the PREVIOUS run. On a recovery restart the old entry is deliberately left in place until that swap, so the window is real and reachable. Every public entry point resolves through that map: Stop, StopAll, WaitPipeline, StopAndWait (and so provisioning.ApplyPlanLive), plus StartWithBackoff's pointer guard. During the window WaitPipeline joined the dead tomb and returned the pre-recovery error, and Stop stopped the dead run and reported success while the recovered run kept going — connectors never torn down, persister never quiesced. A drain reported complete that never happened: invariant 7, silently. The fix publishes rp inside runPipeline, immediately before the status write. What it deliberately does NOT do is reorder the cleanup-goroutine registration to match pkg/lifecycle-poc's shape. Adversarial review of the plan found that doing so reintroduces the same bug class deterministically: in a recovery chain the cleanup goroutine runs the nested recoverPipeline -> StartWithBackoff -> Start synchronously, so a transient status-write failure in the NESTED run propagates back into the OUTER cleanup, which then deletes by key — erasing a newer run whose nodes are alive. Registering cleanup earlier would also let a fast-failing pipeline's terminal status be stomped by the still-pending Running write, since UpdateStatus is last-writer-wins with no versioning. Today's ordering is load-bearing and is kept. So instead: roll back the publication explicitly on the UpdateStatus error path, and make every delete a compare-and-delete (deleteRunningPipelineIfCurrent) so a superseded run can never erase a newer run's entry. csync.Map has no CAS primitive, so that helper is a Get-compare-Delete and its doc says plainly that it narrows the window rather than closing it — it is not atomic and must not be read as such. Tests hold the window open via a PipelineService wrapper that blocks inside the StatusRunning update, rather than racing it: a pre-fix -count=60 sweep of the equivalent v2 package passed while that bug was live, so repeat runs are not evidence. Verified against the exact pre-fix service.go from main: LiveEntryPublishedBeforeRunningStatus and StopDuringWindowTargetsLiveRun both FAIL without the change and pass with it. StopAll_Recovering, which binds an old run before the failure and legitimately expects the pre-recovery error, still passes — that behavior is by design and is untouched. Out of scope, filed separately as #2809: UpdateStatus mutates the shared in-memory pipeline status before the store write and never rolls it back, so a failed status write leaves the instance reporting Running and Start's own precondition rejects the retry. That is why the failed-status test asserts only map state and not that a later Start succeeds.
…iew disproved Adversarial review of e48830a returned "merge with changes". Three of them were merge gates. F1 — the compare-and-delete was closeable and I left it leaky. The helper's doc admitted a residual TOCTOU as if it were unavoidable; it is not. csync.Map has no CAS, but Service has only one other writer, so publishMu now serializes publication against compare-and-delete and the read-compare-delete becomes atomic with respect to it. The review demonstrated the gap rather than arguing it: 4 erasures of a live entry in 200,000 races, dropping to 0 with the lock. And it is reachable without any recovery chain — an operator Stop leaves the status UserStopped, which admits a concurrent Start whose Set can land inside a departing cleanup's window. F2 — my failure-mode analysis was wrong about how the bug presents. I wrote that Stop "reported success" against the dead run. It does not: SourceNode.Stop returns "source node is not running" deterministically (a dead run's source is already stopped), so StopAndWait surfaces an error to ApplyPlanLive and the drain is reported FAILED, not complete. The invariant-7 violation is real but arrives via StopAll, which swallows that error into a log warning, after which ls.Wait resolves instantly off the dead tomb and shutdown quiesces the persister and closes the DB while the recovered run is still live. Corrected here, in the commit message of e48830a's description, and in issue #2806. F3 — the invariant comment overclaimed. It asserted that the map always tracks the live run; this change only establishes that at the publication window. The recovery-backoff window holds a dead run for 1s..10m while Stop admits StatusRecovering, which is a far larger hole and entirely pre-existing. The comment is now scoped, and that window is filed as #2810 rather than being implied covered. F5 — the rollback test was one-sided. Asserting only that no entry remains would pass equally against code that never published at all, so it would not have noticed #2806 being reintroduced wholesale. It now asserts, from inside the status hook, that the entry IS published before the status write, and gone after it fails. Verified: removing the publication makes it fail. Also filed, not fixed here: #2811, pkg/lifecycle-poc still deletes by key in its recovery-failed arm, which is the delete-side half of this same bug in v2. F6 (StopAll holds the map's read lock across stopGraceful) and the named test coverage gaps are follow-ups, not merge gates.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2806. Tier 1 — data path, the engine that ships by default.
Process, as requested: plan → adversarial review of the plan (which overturned the recommendation) → implement → adversarial review of the commit (which returned merge with changes, all applied) → merge on green.
The bug
Between
UpdateStatus(StatusRunning)andStart'srunningPipelines.Set— which only happened afterrunPipelinereturned — the map still pointed at the previous run. On a recovery restart the old entry is deliberately left in place until that swap, so the window is real and reachable.WaitPipelinejoined the dead tomb and returned the pre-recovery error;StopAllthen swallowed the resulting error at shutdown and let the runtime quiesce the persister and close the DB while the recovered run was still live. Invariant 7, silently.The fix
Publication moves into
runPipeline, immediately before the status write, with an explicit rollback on the error path, and every delete becomes a compare-and-delete serialized by a newpublishMu.What it deliberately does not do is reorder the cleanup-goroutine registration to match v2's shape. Registering cleanup before the status write lets a fast-failing pipeline's terminal status be stomped by the still-pending
Runningwrite, sinceUpdateStatusis last-writer-wins with no versioning. Today's ordering is load-bearing.Two things the reviews caught that I had wrong
My recommended design was wrong. The plan review rejected it: reordering cleanup registration reintroduces the same bug class through the recovery chain, where the cleanup goroutine runs the nested restart synchronously and a transient status-write failure in the nested run unwinds into the outer cleanup's delete.
My failure-mode analysis was wrong. I wrote that
Stop"reports success" against the dead run. It doesn't —SourceNode.Stopreturns"source node is not running"deterministically, soStopAndWaitsurfaces an error andApplyPlanLivedoes not proceed. The harm arrives viaStopAllinstead. Corrected here, in the commit messages, and in issue #2806 itself.The commit review also caught that my compare-and-delete was leaky when it didn't need to be — demonstrated, not argued: 4 erasures of a live entry in 200,000 races, 0 with
publishMu. And reachable without a recovery chain at all, since an operatorStopleaves the statusUserStopped, which admits a concurrentStart.Evidence
Verified against the exact pre-fix
service.gofrommain, keeping the new tests:Recovery_LiveEntryPublishedBeforeRunningStatusRecovery_StopDuringWindowTargetsLiveRunRunPipeline_UpdateStatusRunningFails_RollsBackPublicationStopAll_Recovering(binds an old run pre-failure by design)Worth recording: my first perturbation attempt reported zero failures, which would have meant the tests were vacuous. It was my error — the edit didn't compile, so
go testprinted a build error and mygrep '^--- FAIL'matched nothing. A build failure looked exactly like a passing perturbation. Redone with a git-exact revert.Tests hold the window open via a
PipelineServicewrapper that blocks inside the status update rather than racing it — a pre-fix-count=60sweep of the equivalent v2 package passed while that bug was live, so repeat runs are not evidence.go test ./pkg/lifecycle/ -race -count=3 -shuffle=ongreen (65s),golangci-lint0 issues.Filed, deliberately not fixed here
UpdateStatusmutates the shared in-memory status before the store write and never rolls it back. This is why the rollback test asserts only map state, not that a laterStartsucceeds.StopadmitsStatusRecovering, and a stop during backoff doesn't prevent the restart. Far larger than the window this PR closes, entirely pre-existing.lifecycle-pocstill deletes by key in its recovery-failed arm: the delete-side half of this bug in v2.Follow-ups, not merge gates:
StopAllholds the map's read lock acrossstopGraceful, and named coverage gaps aroundStopAll/reconfigureduring the window.