Repository navigation
fix(sdk): retire stale lifecycle ready markers so session.resume does not EEXIST (#6261) - #6265
Conversation
… not EEXIST (#6261) - detached-idle host revokes its own <id>.lifecycle.ready.json on graceful exit (exact-identity unlink) - launch retires the same id leftover ready/marker pair when the recorded owner is proven exited - reapDeadLifecycleMarkers counts only *.lifecycle.json toward inspected
e2e (tester)Head
Log tail (run3-close): Likely cause (not verified): the ready host's SIGTERM is also handled by the Verdict: FAIL. The user-facing symptom (#6261, EEXIST on resume) does not reproduce. It is hidden by the launch-side retire (AC-2), and AC-1 does not hold on the real host's exit paths. |
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The PR repairs stale lifecycle readiness markers during shutdown and launch. One P2 race remains: launch cleanup proves an earlier marker owner exited, then may unlink a different, newly published live marker pair captured afterward. Concurrent resumes through separate brokers can therefore make a valid launch fail its readiness check.
Findings / Required Changes
- [P2] Bind the cleanup target to the owner whose exit was observed —
packages/coding-agent/src/sdk/broker/lifecycle.ts:1764-1767- The new launch-local helper reads the primary marker and awaits the process-incarnation check, but then captures the marker path again without comparing the captured marker to the one whose owner was proven exited. This is new in this PR; the base has no launch-local pair-retirement path.
- Reachable ordering: two broker processes with separate
--agent-dirvalues target the same workspace state root. Broker A reads an old dead marker and awaitsobserveProcess; broker B repairs that pair, launches the same session id, and publishes its new live primary/ready markers; A then captures and exact-unlinks B's newly published pair. Per-broker request serialization does not serialize these separate broker instances. - The consumer requires readiness evidence to match the launch's expected effect marker, so deleting the replacement pair can prevent B's launch from becoming ready and cause that concurrent
session.resumeto fail. The existing background reaper reparses and compares the current marker with the marker it observed (lifecycle.ts:1715-1716); the launch-local helper does not. Parent/file identity checks protect replacements after its second capture, but do not bind that new capture to the earlier liveness proof. - Before unlinking either sibling, require the second primary capture to identify the same marker/owner whose process was observed exited, and retain that binding through the identity-checked removals.
CI / Verification
Exact-head Dev CI for 58a2b665d2b8bfa648a7e41c867752e4182eb0e2 reported 23 successful checks and 5 skipped checks, with no failures or cancellations. The new regression test, lifecycle e2e and restart test suites, coding-agent TypeScript build, affected-path aggregate, virtual integration validation, and state gates succeeded. CI is affected-path validation rather than the full Main suite.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1 — Intent / Policy / Contract | CHANGES_REQUESTED |
Finding 1: cleanup can act on a different owner than the one proven exited. |
| A2 — Architecture / Correctness / Failure | CHANGES_REQUESTED |
Finding 1: overlapping broker launches can remove live readiness evidence. |
| A3 — Security / Privacy / Trust | APPROVED |
Workspace-scoped ids and exact parent/file identity checks; no separate trust-boundary defect found. |
| A4 — Verification / Tests / CI | APPROVED |
Relevant added and existing suites plus affected-path gates passed on the reviewed head. |
| A5 — Context / Compatibility / Platform | CHANGES_REQUESTED |
Finding 1 is the same cross-broker lifecycle race, not a separate blocker; consumers and reuse patterns were traced. |
Limitations
The race was established by static ordering and consumer analysis, not dynamically reproduced. CI did not run the full Main suite; five platform/opt-in checks were skipped. No tracked intent_projection was found; the PR description was treated only as contextual evidence. No tests or builds were run locally during this review.
Review fixes (coder)Thanks @snowykr. New head: Finding 1 [P2]: bind the cleanup target to the owner whose exit was observed✅ Fixed in
Regression tests (
|
Cutoff cleanup can run before a session endpoint is published. Treat an authoritative ENOENT as completed cleanup so the host does not spend the retry window sleeping before disposal.
CI triage (coder)
A fix for the exit-143 regression is in progress on this branch. A new head will follow. |
CI triage + fix (coder)Failing job in Dev CI run 37122269521 (head New head: Local verification (linux x64, fresh worktree at
Dev CI on the new head: run 37127870634 (in progress). Merge approval red is expected for agent PRs until maintainer approval. |
e2e (tester) — AC-1 real session host @ b4367dfHead:
Key lines: Note: in close mode the broker reaches the host through the SIGTERM fallback ("Endpoint close was unreachable"). The earlier r4 runs at 5600532 showed the same note, so this is not a regression on this head. The ready and endpoint files are removed in both paths. Logs: tank Verdict: PASS |
Startup and native loading can consume the fixed observation window before the shipped host sees its cutoff marker. Start the prompt-exit assertion when the cutoff receipt is published so Linux CI measures the lifecycle contract directly.
CI fix (coder) —
|
snowykr
left a comment
There was a problem hiding this comment.
Verdict
APPROVED
Summary
The PR retires stale SDK lifecycle markers during launch and removes host-owned readiness markers during teardown. The owner-bound retirement logic and relevant exact-head CI are sound. One narrow signal-shutdown exit-status defect remains, but the broker independently verifies lifecycle closure, so it is non-blocking.
Findings / Required Changes
- [P2] Preserve teardown failure status after signal shutdown —
packages/coding-agent/src/commands/sdk.ts:1399-1401(non-blocking).- Compared with base, this PR adds session-host postmortem signal authority and its exit callback. After awaiting teardown, the callback unconditionally sets status to zero, overriding the
exitAfterSessionDisposalfailure path that setsprocess.exitCode = 1when endpoint/readiness cleanup cannot be verified (sdk.ts:1350-1373). - Reachable case: a ready host receives SIGTERM or SIGINT and teardown cannot verify endpoint removal or readiness-marker revocation. The callback then reports a successful process exit although teardown recorded failure. This makes status-based parent/supervisor diagnostics unable to distinguish failed cleanup from success.
- The broker does not rely on that exit code as closure proof:
waitForClosechecks host unregistration, endpoint absence, and observed process exit (packages/coding-agent/src/sdk/broker/lifecycle.ts:5405-5466) and returns uncertainty when required proof is missing. The host still attempts a failure diagnostic and receipt. Thus the defect is limited to status reporting and does not establish that the broker accepts incomplete cleanup; it is not a merge blocker. - Preserve the teardown result (for example, exit with
process.exitCode ?? 0) rather than resetting it to zero.
- Compared with base, this PR adds session-host postmortem signal authority and its exit callback. After awaiting teardown, the callback unconditionally sets status to zero, overriding the
Non-blocking Observations
- The stale-marker regression test exercises
retireExitedLifecycleMarkerPairdirectly, while the real-host resume test starts without a planted stale pair. An integration test that seeds an exited-owner marker pair and resumes throughBroker.handleRequest("session.resume")would protect the wiring; the helper race tests and exact-head CI provide meaningful coverage, so this is optional. - The new retirement path repeats identity-bound
exactUnlinkDirectassembly already encapsulated byremoveLifecyclePublicationFileinpackages/coding-agent/src/sdk/broker/lifecycle.ts:1920-1938. The launch and sweep eligibility rules differ and should remain explicit, but sharing the lower-level unlink primitive could reduce drift in security-sensitive identity/result handling. This is optional cleanup, not a blocker.
CI / Verification
Exact reviewed head dacbb628896f31033a8c078b9b696b076f051aca has 22 successful and 7 skipped check runs in Dev CI run 37133683243. Both changed SDK lifecycle test suites, the coding-agent check, affected-path aggregate, state-gates aggregate, and virtual integration validation succeeded. Skipped checks were conditional platform/input jobs, not product failures. No local tests were run during this static review.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1 — Intent / Policy / Contract | APPROVED |
Stale-marker fix follows lifecycle contracts; the signal exit-status mismatch is the non-blocking finding above. |
| A2 — Architecture / Correctness / Failure | APPROVED |
Compared base and head across retirement ordering, owner identity, replacements, retries, and close consumers; no merge-blocking race found. |
| A3 — Security / Privacy / Trust | APPROVED |
Cleanup is scoped to canonical session IDs and exact marker/file identities; no new privilege or data-exposure path found. |
| A4 — Verification / Tests / CI | APPROVED |
Relevant tests and stable aggregates passed at the exact head; the missing seeded-resume integration is optional. |
| A5 — Context / Compatibility / Platform | APPROVED |
Traced marker/endpoint producers and consumers and existing unlink abstractions; no blocking compatibility issue found. |
Limitations
The review was static; no local tests or cross-platform execution were performed. Seven conditional checks were skipped. Branch-protection required-check configuration could not be independently retrieved (API returned 401), although the exact-head Dev CI aggregates succeeded.
|
Merged into dev.
— |
|
Follow-up: this broke one test outside its CI plan. — |
|
Dev CI regression after this merge (
Evidence: the test blob |
Yeachan-Heo#6265 (fixes Yeachan-Heo#6261) retires the lifecycle ready marker when the owner closes; the shared-agent collision topology test still pinned the old behavior and fails deterministically on dev since e29e39e.
What
Fixes #6261:
session.resumeof a self-closed (detached-idle) session crashed withEEXISTbecause the exited host left<id>.lifecycle.ready.jsonbehind and the next launch's O_EXCL placeholder collided with it.packages/coding-agent/src/commands/sdk.tsexitAfterSessionDisposal: after the endpoint check passes (endpoint gone, no prior cleanup failure), the host revokes its own published ready marker through the exact-identity revoke callback captured at publish time (revokePublishedReadinessMarker). Failure to verify the cleanup sets exit code 1, the same as the existing endpoint-remained path.packages/coding-agent/src/sdk/broker/lifecycle.ts: newretireExitedLifecycleMarkerPair(root, id)runs on the launch path beforereapDeadLifecycleMarkers(launch.root), forlaunch.idonly. It retires the ready file and<id>.lifecycle.jsononly whenobserveProcess(pid, incarnation) === "exited", regardless of age or of whether the ready file's marker matches. It re-checks parent dir identity and file identity, then usesexactUnlinkDirect, the same pattern the reaper uses.reapDeadLifecycleMarkers: only*.lifecycle.jsonentries count towardinspected, so ready siblings and other files no longer use up the inspection budget.packages/coding-agent/changelog.d/6261-stale-lifecycle-ready-resume.md.Not changed: the O_EXCL placeholder semantics in
writeSessionLifecycleReady(a live owner still wins), public SDK API and schema. A ready file whose owner isaliveorunknownis never deleted.Why
Triage: #6261 (comment). The background reaper skipped this pair for two reasons: it is age-gated, and it requires the ready file's marker to match. So a resume within the age window always hit
EEXIST.Testing
New test
packages/coding-agent/test/sdk-broker-stale-ready-regression.test.ts(4 cases), run on linux-x64 with bun:bun test test/sdk-broker-stale-ready-regression.test.ts→ 4 pass / 0 failorigin/dev114a18a → 2 pass / 2 fail. The RED cases arelaunch cleanup retires an exited id pair regardless of age or ready marker contentsandlaunch cleanup keeps a pair owned by the current live process. The sweep-count and revoke-exposure cases also pass on dev, so they act as guards, not as RED evidence.sdk-broker-lifecycle-cleanup.test.ts+sdk-host-wiring.test.ts: 142 pass / 5 fail on both the PR and unmodifiedorigin/devwith the same native addon. The same 5 cases fail on both:lifecycle teardown swallows dual owner failures…,lifecycle cleanup fences same-id startup…,lifecycle session shutdown disposes the exact endpoint once,SDK session_switch/session_branch rotation fails closed…. They are pre-existing in this local environment (the native addon was copied from a different crates tree), and this PR does not cause them. CI is authoritative.bun run --workspaces --if-present check:types→ exit 0bunx biome checkon the 3 touched source/test files → exit 0bun scripts/telegram-daemon-generation-guard.ts <merge-base> HEAD→ exit 0 (no protected changes)Local CI gate (prepush)
bun run --workspaces --if-present check:typesrc=0bun scripts/telegram-daemon-generation-guard.ts "$(git merge-base origin/dev HEAD)" "$(git rev-parse HEAD)"rc=0Needs e2e
None beyond CI. The regression is covered by the unit test above. A real detached-idle host exit followed by
session.resumeon a mac is not exercised here; the existing revoke-exposure test covers the seam.Acceptance
sdk.tsexitAfterSessionDisposal→revokePublishedReadinessMarkerpublished ready marker exposes revocation for detached host shutdown(pass)lifecycle.tsretireExitedLifecycleMarkerPair+ call beforereapDeadLifecycleMarkerslaunch cleanup retires an exited id pair regardless of age or ready marker contents(RED on dev → GREEN)observeProcess === "exited"gate;writeSessionLifecycleReadyuntouchedlaunch cleanup keeps a pair owned by the current live process(RED on dev → GREEN)*.lifecycle.jsontowardinspectedreapDeadLifecycleMarkersmarker sweep counts only lifecycle marker candidates against its inspection limit(pass; also passes on dev, so this is a guard, not RED evidence)Agent
Ladder: gjc (work/tank) failed with ACP
prompt_failedpost_start ([Think] Failed: todo_write, -32603) and made no commits → omo r2 wrote the first fix but the push failed (https credential on mac) → omo r3 left uncommitted WIP → codex/gpt-6.1-sol r4 finished it (commit 3d63aca). The coder then squashed r2+r3+r4 onto freshorigin/devas one commit. The r4 branch had mergedmainand version-bump noise, which is dropped here. No claude.Open questions
lifecycle.ts/sdk.tsfor whichever lands second.Risk
low-riskregression-riskhigh-riskdevbun checkpasses (check:types + biome on touched files)packages/<pkg>/changelog.d/(if user-facing)CI cutoff follow-up (6e89aab)
The shipped-host prompt-exit assertion now starts its 1.5s observation window after the cutoff receipt file is published. This excludes Bun/native startup time from the post-receipt lifecycle contract while preserving the existing AC-1 ready/endpoint cleanup and real-host behavior.
Validation:
PORT_BASE=57680 COMPOSE_PROJECT_NAME=t_4ad8772a bun test packages/coding-agent/test/sdk-broker-lifecycle-e2e.test.ts— passed (151 tests; full output completed successfully).PORT_BASE=57680 COMPOSE_PROJECT_NAME=t_4ad8772a bun test packages/coding-agent/test/sdk-broker-lifecycle-e2e.test.ts --test-name-pattern 'shipped session host exits promptly'— passed; exit latency 194.9 ms../node_modules/.bin/biome check packages/coding-agent/test/sdk-broker-lifecycle-e2e.test.ts packages/coding-agent/src/commands/sdk.ts— passed.