fix(#565): captain re-adoption after cmux restart — role restoration + no-death-by-absence fail-safe - #568
Merged
Merged
Conversation
reconcileLiveness's runtime/agent branch treated an incoming record with an older startedAt than the stored one as stale and discarded it outright — unless lastState was "end". That's correct when prev is alive (guards against a stale duplicate session clobbering a live one, #527), but wrong when prev is already stopped/gone: once a captain's liveness entry is wrongly marked dead, no future runtime signal for that same still-running session can ever win, because its startedAt never changes and will keep losing the ">=" comparison forever. The daemon then never re-adopts the captain, permanently pausing delivery. Reproduced against the live incident data (packages/core/src/liveness.ts) and added a regression test alongside a guard test for the #527 case.
…store role via sessionId Root cause (corrected — the earlier reconcileLiveness startedAt theory in this branch was wrong and does not explain the live incident): cmux's own hook-sessions record for the live bet2fun-app captain degraded to a bare launchCommand (`["claude"]`, no --append-system-prompt-file) after a crash/ reattach, so runLivenessTick's role-from-template classification read it as role:"unknown". Filtered out of the captain group, the project fell out of `seen`, and the very next tick markEnded() it — "stopped" forever, crews reaped, delivery permanently paused, even though the pid was alive the whole time (verified against the live cmux store data pulled from the running daemon). Two changes: 1. Root cause: sessionId identity now outranks a degraded/unclassifiable launchCommand. A record whose sessionId matches an already-known captain for its project is treated as that captain regardless of what roleFromTemplate currently reads. 2. Fail-safe (stands independently of #1): a captain missing from the runtime snapshot is no longer marked ended on absence alone. markEnded now requires positive evidence — the tracked pid confirmed dead via isPidAlive. If the pid is still alive, or unknown/null, the entry is left alone and a log line is emitted instead. This also gates the #324 reaper, which fires off the same derived state. Updated the two existing tests that encoded the old (buggy) behavior — "absent from snapshot marks ended/reaps even with isPidAlive() => true" — to require genuine death, and added regression tests reproducing the exact incident for both the role-restoration path and the fail-safe.
…act change The daemon-direct reap test encoded the OLD contract: "captain absent from the runtime snapshot" alone was sufficient to reap a project's orphaned crews. That contract is what caused #565 — a single snapshot glitch (cmux degrading a session's launchCommand) silently reaped 3 live crews that were actively committing code. The previous commit changed the contract on purpose: reaping now requires positive evidence the tracked pid is actually dead, not just absent from one read. - Updated the existing test's fixture to simulate a genuine close (the snapshot goes empty AND the tracked pid is confirmed dead) — this is what actually happens when a user closes a captain workspace, so the real reap path is preserved and still covered. - Added the inverse case as its own test, reproducing the actual incident: captain absent from the snapshot but its pid still alive → the crew must NOT be reaped and its task stays "working".
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.
Summary
Correction: this PR originally claimed the fix was a
reconcileLivenessstartedAt-ordering change. That theory was wrong (confirmed by the reporter against live daemon/cmux data) and does not explain the incident. It's kept as a defensive hardening only — see "Kept as hardening" below — but it is not what fixes #565.The actual root cause and fix, verified against the live incident data pulled from the running daemon and the real
~/.cmuxterm/claude-hook-sessions.json:cmux's own hook-sessions record for the still-alive
bet2fun-appcaptain (pid alive 7h49m+) degraded to a barelaunchCommand(["claude"], missing--append-system-prompt-file) after a crash/reattach.runLivenessTick'sroleFromTemplateclassification depends entirely on that flag, so the record readrole:"unknown", was filtered out of the captain group, and the project fell out ofseenon the very next tick — triggeringmarkEnded()→"stopped"forever, crews reaped, delivery permanently paused. The pid was alive the entire time; nothing ever re-checked that before inferring death.Two changes, both in
packages/core/src/daemon/delivery-loop.ts(runLivenessTick):launchCommand. A record whosesessionIdmatches an already-known captain for its project is treated as that captain regardless of whatroleFromTemplatecurrently reads.markEndednow requires positive evidence: the tracked pid must be confirmed dead viaisPidAlive. If the pid is still alive, or unknown/null, the entry is left alone (a log line is emitted instead). This also gates the Detect user-closed captain workspace → clean up (deregister relay, reap orphaned crews) + set explicit 'stopped' status #324 reaper, which fires off the same derived state — so live crews can no longer be reaped from an inferred signal.This is a deliberate breaking change to the reap contract, not a side effect
Reaping is destructive — it terminalizes live, working crew tasks. The old contract ("absent from one runtime snapshot read → captain is stopped → reap its crews") is exactly what turned a cosmetic cmux store glitch into 3 cancelled crews that were actively committing code the morning of this incident.
packages/cli/src/__tests__/squadrantd-daemon-direct.test.tshad a test that encoded the old contract directly (isPidAlive: () => trueyet still asserting the crew got reaped on snapshot absence). Rather than loosen the fix to keep that test green, the test was updated to reflect the new, intentional contract:"working".Kept as hardening (not the fix)
packages/core/src/liveness.ts'sreconcileLiveness: a live pid now outranks a startedAt-ordering comparison onceprevis already dead, so a captain can't get stuck ifreconcileLivenessever sees a live reopen with an olderstartedAtfor some other reason. Doesn't apply whenprevis still alive (preserves the existing #527 stale-duplicate guard). Independently reasonable, doesn't fix #565 on its own.Test plan
role:"unknown"but a known-captainsessionIdis still treated as the captain, (b) a captain absent from the snapshot with a still-alive pid is NOT marked ended and NOT reaped (core-level + CLI integration-level), (c) same with an unknown/null pidpackages/core(unit) andpackages/cli(integration) — to require genuine death instead, and added the inverse case alongside eachnpx tsc -bonpackages/shared/packages/core/packages/agents/packages/workspaces(leaf deps, needed to unblock local test resolution in this fresh worktree) +npx vitest runinpackages/core(42 files/656 tests) andpackages/cli(58/59 files, 479/480 tests — the one failing file is an unrelated pre-existing@squadrant/webunbuilt-dist issue in this worktree) — all greennpx tsc --noEmitclean in bothpackages/coreandpackages/cli