Skip to content

fix(deploy): make bot restarts fail closed - #2138

Merged
justinhelmer merged 1 commit into
mainfrom
plan/fix-issue-2112-the-four-d7bec7/u1
Sep 21, 2026
Merged

justinhelmer merged 1 commit into
mainfrom
plan/fix-issue-2112-the-four-d7bec7/u1

Conversation

@coreplane-switchboard

@coreplane-switchboard coreplane-switchboard Bot commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

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

  1. Production config floor State-backed production config must include durable history, providers and grants before the runtime can start. ⚠ A requirement that is too strict can block an otherwise recoverable production boot.
  2. Refusal-only health mode A bad base config serves only a 503 health explanation; no Slack, ingress, command or model route opens. ⚠ Readiness decides whether a broken generation can replace a healthy one.
  3. Restart generation fence The CLI compares source time and digest with the durable base, refuses stale state, and names the base vN it will load. ⚠ A false current verdict restarts onto stale config; a false stale verdict blocks recovery.
  4. Deployment-side restart grant The Worker authenticates the bearer and checks the profile-rendered deployer subject before touching the container. ⚠ This recovery authorization boundary must never consult runtime grants.
  5. Dispatch-bound child admission Spawn acquires its drain permit after every asynchronous read and releases it only when registration can hold the child. ⚠ A missed boundary can admit work that dies with the old generation.
  6. Drain admission hold Pending child admissions keep the full drain deadline alive until a registry row replaces the permit. ⚠ Dropping this hold lets the process exit before a raced child registers.
  7. Restart behavior contract The spec binds config freshness, generation reporting and deployment-side authorization into one fail-closed restart.

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)
  • Use timestamps and digests together. Timestamp-only checks can miss coarse or preserved mtimes; digest-only checks miss a changed-then-restored source. Path mtime or GitHub Last-Modified catches the latter when available, and sha256 always catches different content.
  • Keep recovery authorization on the Worker. The runtime grants document is the object a restart may need to repair. Comparing the authenticated token subject with a deployment-profile Worker var preserves a fail-closed recovery route without widening runtime permissions.
  • Serve health but nothing else on config failure. Exiting hid the startup cause from /healthz, while loading defaults served a broken bot. A refusal-only server exposes the validator sentence and keeps every operational route closed.
  • Fence dispatch and repeat handoff. A check before spawn reads leaves a SIGTERM race. The final synchronous permit prevents new dispatch and holds the drain through registration; repeated ledger handoff then catches children that become resumable after the first pass.
Validation (8 criteria)
Criterion Proof
Original focused behavior set passed before the review fix npx vitest run <13 touched test files> at the prior reviewed head → 13 files passed; 505 passed, 1 skipped.
Coordinator admission and existing route behavior pass at the rebased head npx vitest run src/channels/adminCoordinator.test.ts → 1 file passed; 114 tests passed.
Root TypeScript remains sound at the rebased head NODE_OPTIONS=--max-old-space-size=6144 npx tsc --noEmit -p tsconfig.json → exit 0.
Cloudflare Worker TypeScript passed before the review fix 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.
Changed fix-round files match repository formatting npx prettier --check src/channels/adminCoordinator.ts src/channels/adminCoordinator.test.ts src/index.ts docs/reference/specs/agent-ship.md → all matched.
Public-tree hygiene and proof bindings remain clean npm run hygiene:check → ok; npm run specs:check → 50 specs and 5,576 proof references checked.
Changed sources remain covered without silent test removal npm run specs:coverage -- --changed origin/main...HEAD --test-guard --require → every source covered; test-guard ok.
The PR title is a valid changelog line 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 until runStarted. No full suite or npm run verify ran locally; CI owns those gates. The production boot regression remains src/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

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
@coreplane-switchboard
coreplane-switchboard Bot force-pushed the plan/fix-issue-2112-the-four-d7bec7/u1 branch from 44da3d1 to b3a9fc0 Compare September 21, 2026 17:09

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auto-approved: coreplane-switchboard[bot] reviewed this PR and posted an LGTM verdict (see its review). This repository opted in through its REVIEW_BOT_LOGIN and REVIEW_BOT_ID variables.

@justinhelmer
justinhelmer merged commit e392e08 into main Sep 21, 2026
30 checks passed
@justinhelmer
justinhelmer deleted the plan/fix-issue-2112-the-four-d7bec7/u1 branch September 21, 2026 17:16
@justinhelmer

Copy link
Copy Markdown
Contributor

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 [bot-preflight] preflight ok: container application settled with the non-blocking note 4 run(s) in flight — handed to the next generation on SIGTERM (run-history item 39); they continue there under their own cards; version uploaded 20:35:28Z, bot: live (commit 95c0153; 126s after the upload) at 20:37:35Z, deploy.step.bot span 137,673 ms status ok. The four in-flight pipelines resumed on the new generation (receipt on #2116). Positive path only: no refusal was exercised this deploy. Receipt runnable when (refusal path): a bot deploy attempted over a rollout in progress or a bare config — the preflight must refuse and nothing deploys.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant