fix(lifecycle-poc): publish the live run before releasing its workers - #2834
Open
devarismeroxa wants to merge 1 commit into
Open
fix(lifecycle-poc): publish the live run before releasing its workers#2834devarismeroxa wants to merge 1 commit into
devarismeroxa wants to merge 1 commit into
Conversation
Fixes #2833. Tier 1 (data-path/shutdown ordering). runPipeline ordered close(registered) -> runningPipelines.Set(...) -> UpdateStatus(StatusRunning). Between the first two statements, this run's worker goroutines are already released and free to do observable work (read, process, ack), while runningPipelines[id] still points at the PREVIOUS run. On a recovery restart the previous entry is deliberately left in place until the swap (see recoverPipeline), so the window is real and reachable: a Stop landing in it resolves the DEAD run. Kill lands on an already-dead tomb (a no-op - tomb.v2 keeps only the first death reason) and the actually-running pipeline is left completely unsupervised - invariant 7, silently. This is #2746/#2806's failure mode again, shrunk to two adjacent statements with no I/O between them. #2812 fixed the *2746* window (ordering Set before UpdateStatus in the sibling pkg/lifecycle package) but never touched the close(registered)-vs-Set order, so it narrowed this window rather than closing it, exactly as #2833 diagnosed. The fix: move close(registered) to run AFTER runningPipelines.Set. Verified safe against the barrier's own stated constraint (see the comment moved alongside it) - registered only needs to be closed before UpdateStatus, which is potentially slow; every t.Go registration it guards against already completes earlier in the same function, synchronously, regardless of where close(registered) itself lands relative to Set. Regression test: TestServiceLifecycle_Recovery_ForceStopDuringWorkerReleaseWindow. Adds a production test seam, Service.testWorkersReleased, matching this package's existing statusRecorder.onUpdate shape (wrap/hook instead of racing a sleep) but one call earlier, because this window closes before UpdateStatus - and therefore PipelineService - is ever invoked, so hooking UpdateStatus (as the #2746 test does) cannot observe it. Nil in production; a single unlocked nil-check with no behavior change when unset. The test holds runPipeline paused at the exact instant under test during a recovery restart, issues a real concurrent Stop(force=true), and asserts which run's tomb it killed - not just map identity. force=true is deliberate: that branch is a single unconditional rp.t.Kill with no worker interaction, so the assertion reduces to a synchronous tomb.Alive() check with no drain-timing nondeterminism of its own. Perturbation-proved: reverting only the production reorder (keeping the seam and test as-is) fails the test deterministically 200/200 under `-race -shuffle=on` (1.7s, no hangs - the test's own safety-net Kill guarantees cleanup regardless of outcome). Reapplying the fix passes 500/500 under the same flags (3.2s). Full pkg/lifecycle-poc/... suite green under -race (16.4s). tests/chaos (no-infra suite) green under -race (120s). golangci-lint and gofumpt clean. Adversarial self-review: the pre-recovery run's dead-tomb identity (deadRp) is captured immediately after Start returns, before the transient failure fires - the same acknowledged non-atomic timing this package's existing #2746 sibling test already relies on (LiveEntryPublishedBeforeRunningStatus), not a new risk. Confirmed to hold empirically across 500+ -race -shuffle=on iterations here. Invariants: 7 (shutdown is graceful by default) is the one this closes - the misdirected Stop was the failure. 1 and 3 are implicated as a consequence, not directly: if an operator's Stop/StopAndWait/ApplyPlanLive believes a pipeline has stopped while the live run keeps consuming and acking records, any invariant-1/3 guarantee that depends on "the pipeline actually stopped when told to" (e.g. a caller proceeding to tear down shared infrastructure) is only as good as invariant 7 holding - this fix restores that precondition rather than touching ack/position logic itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016xE861dwb3MLgqEdWRECLY
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
1 participant
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.
Closes #2833. Tier 1 — production shutdown/recovery ordering in the engine that becomes the
default in v0.21.
The bug
runPipelineorderedclose(registered)->runningPipelines.Set(...)->UpdateStatus. Betweenthe first two statements, worker goroutines are released and can read, process and ack records while
runningPipelines[id]still points at the dead run on a recovery restart. AStoplanding thereresolves the dead run and returns success while the live run keeps consuming, unsupervised.
#2746/#2812 fixed the wider window (Set before UpdateStatus) but left this narrower one — two
adjacent statements instead of spanning the status write.
The fix
Publish the run before releasing its workers. The barrier's real constraint is that
close(registered)must happen before the potentially-slowUpdateStatus, not before theSet—every
t.Goregistration it protects completes earlier in the function, synchronously. The mapwrite is fast and non-blocking, so moving it ahead costs nothing.
Invariant-7 comment added at the enforcement site.
Regression test
TestServiceLifecycle_Recovery_ForceStopDuringWorkerReleaseWindowexercises the recovery restartpath specifically — the initial-start path has no such window, because
ls.Startreturns only afterthe status write.
It needs a production seam to hold a two-statement window open deterministically:
Service.testWorkersReleased func(rp *runnablePipeline), called synchronously right afterclose(registered). The existingstatusRecorder.onUpdatehook cannot observe this window, becauseit closes before
PipelineServiceis ever invoked. The seam is nil in production (NewServicenever sets it), guarded by a single unlocked nil-check, called on the same goroutine rather than a
spawned one, and set only in
service_test.go. Zero behaviour change when unset.Perturbation proof
Verified independently of the authoring agent, by reverting only the production reorder (moving
close(registered)and the seam call back ahead of theSet) and leaving the test untouched:-race -count=20Stop(force=true) ... did not kill the live run (tomb still alive) - it landed on the dead pre-recovery run instead-race -shuffle=on -count=50Full
pkg/lifecycle-poc/...green under-race. The authoring agent additionally reports 200/200FAIL before and 500/500 PASS after, and
tests/chaos(no-infra suite) green under-race.A first perturbation attempt was discarded as invalid: it moved the two statements but left the
seam call in place, so the test's hold point landed after the
Setand the test passed. The proofabove moves the seam with
close(registered), which is the faithful pre-fix state.Failure-mode analysis
What this could break. The reorder makes the map write happen while worker goroutines are still
blocked on
<-registered. IfrunningPipelines.Setcould block on something only a released workercan satisfy, this would deadlock at startup. It cannot:
Setis a guarded map write with nodependency on worker progress, and no worker holds that lock (they are parked on the channel).
Which metric or alert would show it. A pipeline stuck in
StatusStarting/pre-Runningwith norecords flowing — the startup path never reaching
UpdateStatus(StatusRunning).tests/chaos (race, x3)and thelifecycle-pocsuite both exercise startup and recovery and would fail loudly.How to roll back. Revert the commit. The seam is inert, so reverting restores exactly the prior
behaviour with no migration or state concern. Nothing is serialized and no format changes.
Invariants. Closes an invariant 7 (graceful shutdown) hole. 1 and 3 are implicated
only downstream: nothing in the ack/position path changes, but a caller who believes a drain
completed may tear down shared infrastructure while the live run is still writing — so an
invariant-1/3 guarantee a caller depends on is only as strong as invariant 7 holding.
Adversarial self-review
deadRpis captured immediately afterStart()returns, with a small window before the inducedfailure could race ahead of that read. This is the same non-atomic timing pattern the sibling #2746
test already relies on — not newly introduced here — and it held across 500+
-race -shuffle=oniterations.
Merges cleanly with #2832 (disjoint edit regions in
service_test.go), confirmed viagit merge-tree --write-tree.Roadmap: v0.20, arch-v2 hardening ahead of the v0.21 default flip.