fix(deploy): make bot restarts fail closed - #2138
Conversation
There was a problem hiding this comment.
Changes requested: The SIGTERM admission guard has a race that can still start and lose a child after draining begins.
Warning
Changes requested · head 44da3d1 · 1 finding: 1 major
| Severity | Finding | Where |
|---|---|---|
| major | F1 Fence child admission across the SIGTERM boundary | src/channels/adminCoordinator.ts:738 |
Full review
F1 (major, high confidence, src/channels/adminCoordinator.ts:738): The drain check occurs before several asynchronous reads. A spawn can pass it, block while loading the instance, unit, history, or brief, then call dispatch() after SIGTERM has set draining. If the first handoff sees no registered child and nothing else holds the drain, the process can exit before that child registers durably, losing the run. Fence admission at dispatch/registration time, or recheck immediately before dispatch while ensuring the drain waits for in-progress admissions.
Co-Authored-By: coreplane-switchboard[bot] <318072483+coreplane-switchboard[bot]@users.noreply.github.com>
44da3d1 to
b3a9fc0
Compare
There was a problem hiding this comment.
LGTM: The SIGTERM child-admission race is fixed and the prior finding is resolved.
Note
Approved · head b3a9fc0 · no findings
Full review
F1 is resolved. The admission permit now closes the SIGTERM race through child registration or dispatch failure, with regression coverage for both blocked reads and drain-held registration.
|
Live receipt 2026-09-21 (1.260.2 deploy, run 35650904486, the first bot deploy with #2138 in the deploying CLI): the bot step ran its fail-closed preflight before touching the container — log 20:35:19Z |
Production restarts now require a valid, current base config and authorize recovery from deployment state. Draining generations fence child admission through registration and repeatedly hand resumable work to their replacement.
Why: #2112 showed a restart could boot without production config and strand work. This restores the release/deploy and ship guarantees without relying on broken runtime state.
Where to look
Feedback wanted: Please judge the production-config floor, the timestamp-plus-digest fence, and whether the dispatch permit closes every SIGTERM admission race without delaying handoff.
Risk: This is 1,025 changed lines across 31 files. Splitting startup, restart and drain was considered, but would separate one rollout safety contract. A bad fence can block recovery; roll back the commit to restore prior behavior.
Verified: Rebased-head coordinator tests (114), root tsc, Prettier, hygiene, specs and coverage passed. Full verification and production restart remain CI/human-gated.
Decisions (4)
Validation (8 criteria)
npx vitest run <13 touched test files>at the prior reviewed head → 13 files passed; 505 passed, 1 skipped.npx vitest run src/channels/adminCoordinator.test.ts→ 1 file passed; 114 tests passed.NODE_OPTIONS=--max-old-space-size=6144 npx tsc --noEmit -p tsconfig.json→ exit 0.NODE_OPTIONS=--max-old-space-size=6144 npx tsc --noEmit -p deploy/cloudflare/tsconfig.json→ exit 0 at the prior reviewed head; fix did not touch Worker code.npx prettier --check src/channels/adminCoordinator.ts src/channels/adminCoordinator.test.ts src/index.ts docs/reference/specs/agent-ship.md→ all matched.npm run hygiene:check→ ok;npm run specs:check→ 50 specs and 5,576 proof references checked.npm run specs:coverage -- --changed origin/main...HEAD --test-guard --require→ every source covered; test-guard ok.npm run check:pr-title -- "fix(deploy): make bot restarts fail closed"→ check:pr-title ok.For agents
Rebased onto origin/main immediately before the force-push; pushed head is b3a9fc0. F1's red proof blocks
instances.get, flips draining, then resumes the read; the old head dispatched and returned 200, while this head returns queued without dispatch. The admission test also observes one pending permit untilrunStarted. No full suite ornpm run verifyran locally; CI owns those gates. The production boot regression remainssrc/index.boot.test.ts, which starts the entrypoint against a fake state Worker with no base document and reads the 503 refusal.Requested by @justinhelmer in slack:C0BRRHKFLCB
🤖 Generated with Claude Code