[codex] guard the worker against unhandled async crashes (BLO-20618) - #949
Conversation
…618) PR A of the BLO-19722 split — the converged half of #891, carrying only acceptance criteria 1 and 4. The crash-time run marking (AC 2/3) and the postgres driver patch (AC 5) stay on BLO-19722. Death path 1 — an unhandled async throw killed the worker with a bare stack. `paperclip-0` terminated `exitCode: 1` on 2026-07-31 from a TypeError raised inside a postgres.js `setImmediate` callback (porsager/postgres#1154), which no `try`/`catch` of ours can intercept. There was no `process.on("uncaughtException")` anywhere in `server/src`, so one transient null socket took down the supervisor for every run it was tracking. `process-crash-guard.ts` installs both handlers, serialises the full `.cause` chain (hardened against throwing getters, self-referential causes and non-Error throwables), writes a synchronous stderr breadcrumb so the reason survives into `kubectl logs --previous`, then exits non-zero deliberately. Crash bookkeeping is optional, raced against a timeout and re-entrancy guarded, so a crash while handling a crash exits immediately rather than hanging. It is installed under `isMainModule`, not inside `startServer()`. `startServer()` is exported and called 16 times in-process by `server-startup-feedback-export.test.ts`, so installing there both leaks a listener pair per call and arms a real `process.exit(1)` inside the test runner, where one late-handled rejection would kill the worker mid-suite as an unrelated flake. The entrypoint also covers strictly more of startup. Death path 2 — `timeoutSeconds: 5 / failureThreshold: 3` let a ~90s event-loop stall convince kubelet the worker was dead. `/healthz` is declared by no handler in `server/src`; it falls through to the SPA catch-all, so it measures only "can the event loop answer right now" and cannot distinguish a busy heartbeat pass from a wedged process. An API replica is stateless and cheap to kill early; the worker is a singleton whose false kill orphans every in-flight run. Widened to 10s/6 (~180s) with that rationale recorded inline. The guard imports only `shutdown-log.js` — no edge to the heartbeat service; crash-time marking arrives later as an injected `onCrash`.
…20618) Ally's round-6 review is correct, and the root cause is older than this PR: `shutdown-log.ts` documented the exact opposite of Node's stream semantics. Node's writes are synchronous for files and POSIX TTYs but ASYNCHRONOUS for pipes and sockets on POSIX. Container stderr under kubelet is a pipe, so the module header's claim — "when stderr is piped … libuv issues the write synchronously" — named the one case that is actually async. `process.exit()` then discards pending stdout/stderr I/O, so the crash guard's breadcrumbs could die with the process while having already suppressed Node's default crash printer: precisely the undiagnosable failure the guard exists to stop. Why it survived two review rounds: libuv tries an eager `uv_try_write`, so a short line into an empty pipe does land. Only once the pipe backs up — a crashing worker spewing a stack — is the remainder queued and dropped. Measured on Node v24.16.0 with stderr piped: of ~200 KB across three breadcrumbs, the first landed and the trailing `exiting 1 after uncaughtException` line was lost (146,176 of 200,092 bytes captured). Breadcrumbs now go through `fs.writeSync` on the raw stderr fd, looping partial writes and retrying EAGAIN/EINTR (stderr can be non-blocking) under a 100ms deadline. The deadline is deliberate: this is the fatal path, and a breadcrumb that blocked forever on a full pipe would wedge the crash handler, turning a fast crash-and-restart into a silent hang — worse than the bug. On give-up it falls back to the async stream, no worse than before. Tests, both falsified against pre-fix source: - `process-crash-guard-exit.test.ts` — out-of-process, per review. Spawns a real child that really crashes with real piped stderr and asserts the breadcrumb, stack and exit code survive a real `process.exit`. Pre-fix, the two pipe-pressure cases fail; the unpadded ones pass, pinning the asymmetry that hid this. - `graceful-shutdown.test.ts` — its stderr stub was not just stale, it was incapable of catching this: a stub returns synchronously regardless of the real write, so "writes synchronously" passed for years against an async write. Now redirects the real fd to a temp file and asserts the degraded stream path is unused, so both synchronicity tests fail pre-fix. 29/29 green post-fix; tsc unchanged at the pre-existing 38 errors, none in changed files.
Co-Authored-By: Paperclip <noreply@paperclip.ing>
Co-Authored-By: Paperclip <noreply@paperclip.ing>
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
/test |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 6e7101e
Looks good. No Critical or Important issues remain on this head.
Strengths
- The crash guard guarantees deliberate non-zero termination while bounding both bookkeeping and pipe-backed stderr flushing.
- Re-entrancy, hostile error getters, cyclic cause chains, stalled stderr, and real child-process exit behavior receive focused coverage.
- The graceful-shutdown path awaits its final breadcrumb before forced exit, preserving diagnostics under stderr backpressure.
- The worker probe tradeoff is explicit: readiness remains responsive while liveness tolerates transient singleton event-loop stalls.
Recommended Action
- Approve this exact head.
allyblockcast
left a comment
allyblockcast
left a comment
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 6e7101e
Looks good. No Critical or Important issues remain on this head.
Strengths
- The crash guard guarantees deliberate non-zero termination while bounding both bookkeeping and pipe-backed stderr flushing.
- Re-entrancy, hostile error getters, cyclic cause chains, stalled stderr, and real child-process exit behavior receive focused coverage.
- The graceful-shutdown path awaits its final breadcrumb before forced exit, preserving diagnostics under stderr backpressure.
- The worker probe tradeoff is explicit: readiness remains responsive while liveness tolerates transient singleton event-loop stalls.
Recommended Action
- Approve this exact head.
Resolve Ally review-consistency guard: duplicate operative Ally approval for this head; keeping the later user approval.
|
/ally review Please review exact head |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 35942c4
Important Issues (1)
- [native-codex]
server/src/index.ts:1775— A fatal error during graceful shutdown can be reported as a clean exit. The crash guard setsprocess.exitCode = 1and begins bounded asynchronous flushing, but the SIGINT/SIGTERM handler now yields at this awaited breadcrumb and later callsprocess.exit(0)unconditionally at line 1788. If an uncaught exception or rejection arrives while shutdown is in progress, whichever continuation exits first wins; the shutdown continuation can therefore overwrite the fatal status, truncate crash diagnostics, and make Kubernetes classify the crash as clean.- Coordinate the shutdown and crash paths, or make the shutdown exit preserve a non-zero
process.exitCode(for example,process.exit(process.exitCode ?? 0)). Add a real child-process test that starts SIGTERM shutdown, triggers a fatal error while the final stderr write is pending, and asserts exit code 1 plus the crash breadcrumb.
- Coordinate the shutdown and crash paths, or make the shutdown exit preserve a non-zero
Suggestions (1)
- [native-codex]
server/src/process-crash-guard.ts:287— Consider coveringNODE_OPTIONS=--unhandled-rejections=strict; Node emits bothuncaughtException(..., "unhandledRejection")andunhandledRejectionfor one rejection in that mode, so the second event currently takes the immediate re-entry path. The repository does not currently configure strict mode, so this is not blocking this deployment.
Strengths
- The real child-process tests exercise actual exit behavior and pipe-backed stderr pressure rather than relying only on mocked streams.
- Cause-chain serialization is bounded and hardened against cycles and hostile getters.
- Separating worker readiness from the wider liveness budget is operationally well motivated and documented.
Recommended Action
- Address the Important shutdown/crash race before merge.
- Consider the strict-rejection coverage opportunistically.
Signed-off-by: Omar Ramadan <omar@blockcast.net>
Signed-off-by: Omar Ramadan <omar@blockcast.net>
Signed-off-by: Omar Ramadan <omar@blockcast.net>
There was a problem hiding this comment.
Exact-head review
Reviewed head: 0dc6a17
Important
server/src/index.ts:1792 still allows graceful shutdown to preempt the crash guard's bounded diagnostic flush. If a fatal event arrives after the graceful handler's final shutdown breadcrumb has drained, especially while shutdownInstrumentation() is pending, the guard sets process.exitCode = 1 and starts its asynchronous initial/final breadcrumb sequence. The graceful continuation can then call process.exit(process.exitCode ?? 0) first. That preserves the non-zero status, but it can discard the crash stack or the final exiting 1 after ... breadcrumb.
Please coordinate exit ownership when the crash guard is active, or otherwise await its bounded completion before the graceful path force-exits. Extend the real-process race test to trigger the fatal event after the graceful breadcrumb and assert the complete crash record, including [shutdown] exiting 1 after uncaughtException.
Non-blocking test hardening
The child-process helpers in process-crash-guard-exit.test.ts and the first graceful-shutdown-exit.test.ts case have no watchdog/cleanup path. A regression where the child does not exit can leave open stdio handles after Vitest's timeout. Please add bounded child cleanup while touching these tests.
CI is not terminal green yet; multiple required jobs remain queued. I did not approve this head.
Signed-off-by: Omar Ramadan <omar@blockcast.net>
|
Addressed the exact-head review on
Verification: 3 focused files / 22 tests passed; @ally please re-review exact head |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: e9c1773
Looks good. No Critical or Important issues remain on this head.
Prior Findings Dispositioned (1)
- prior:35942c4 important 1 — fixed —
server/src/index.ts:1793— graceful shutdown now observes the active crash path, awaits its bounded completion, and returns without issuing its own exit; the exact-head child-process test atserver/src/__tests__/graceful-shutdown-exit.test.ts:84verifies the crash record survives through the finalexiting 1 after uncaughtExceptionbreadcrumb.
Suggestions (2)
- [error handling]
server/src/process-crash-guard.ts:283— When the futureonCrashbookkeeping hook is wired, consider keeping its timeout referenced untilfinish()starts. With no other referenced handles, the currentunref()permits a natural code-1 exit before the timeout emits the final crash breadcrumb. - [error handling]
server/src/process-crash-guard.ts:293— Consider formatting a rejected bookkeeping value through the same non-throwing serializer used for the original crash; a hostilemessagegetter or string coercion could otherwise take the re-entry path and truncate diagnostics.
Strengths
- Exit ownership is now explicit: once fatal handling starts, graceful shutdown cannot preempt or truncate it.
- The real-process race fires after the final graceful breadcrumb and asserts both code 1 and the complete fatal record.
- Child-process helpers now have bounded watchdogs, and the strict unhandled-rejection pair is covered without duplicate crash handling.
- Pipe-backed stderr writes are deadline-bounded while regular-file writes preserve synchronous durability.
Recommended Action
- Approve this exact head.
allyblockcast
left a comment
allyblockcast
left a comment
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: e9c1773
Looks good. No Critical or Important issues remain on this head.
Prior Findings Dispositioned (1)
- prior:35942c4 important 1 — fixed —
server/src/index.ts:1793— graceful shutdown now observes the active crash path, awaits its bounded completion, and returns without issuing its own exit; the exact-head child-process test atserver/src/__tests__/graceful-shutdown-exit.test.ts:84verifies the crash record survives through the finalexiting 1 after uncaughtExceptionbreadcrumb.
Suggestions (2)
- [error handling]
server/src/process-crash-guard.ts:283— When the futureonCrashbookkeeping hook is wired, consider keeping its timeout referenced untilfinish()starts. With no other referenced handles, the currentunref()permits a natural code-1 exit before the timeout emits the final crash breadcrumb. - [error handling]
server/src/process-crash-guard.ts:293— Consider formatting a rejected bookkeeping value through the same non-throwing serializer used for the original crash; a hostilemessagegetter or string coercion could otherwise take the re-entry path and truncate diagnostics.
Strengths
- Exit ownership is now explicit: once fatal handling starts, graceful shutdown cannot preempt or truncate it.
- The real-process race fires after the final graceful breadcrumb and asserts both code 1 and the complete fatal record.
- Child-process helpers now have bounded watchdogs, and the strict unhandled-rejection pair is covered without duplicate crash handling.
- Pipe-backed stderr writes are deadline-bounded while regular-file writes preserve synchronous durability.
Recommended Action
- Approve this exact head.
Dismiss duplicate Ally bot approval; keeping operative Ally user approval 4839735105 for head e9c1773.
|
/ally review Please review exact head |
|
/ally review Please review exact current head |
|
The branch was updated onto current @ally please replace the dismissed approval with an exact-head review for |
|
/ally review Please review exact current head |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: ddf1dd6
Looks good. No Critical or Important issues remain on this head.
Suggestions (2)
- [error handling]
server/src/process-crash-guard.ts:283— When the futureonCrashbookkeeping hook is wired, consider keeping its timeout referenced untilfinish()starts; with no other referenced handles, the currentunref()can permit natural process exit before the final crash breadcrumb. - [error handling]
server/src/process-crash-guard.ts:293— Consider formatting a rejected bookkeeping value through a non-throwing serializer; a hostilemessagegetter or string coercion could otherwise take the re-entry path and truncate diagnostics.
Strengths
- Graceful shutdown now yields exit ownership to an active crash guard, preserving the non-zero status and complete fatal record.
- A real child-process regression test triggers the fatal event after the final graceful breadcrumb and verifies code 1 plus the final crash breadcrumb under stderr backpressure.
- Re-entrancy, strict unhandled-rejection pairing, hostile original-error getters, cyclic cause chains, and stalled stderr all receive focused coverage.
- The only change since the previously reviewed clean tree is an unrelated merged-master test timeout adjustment; the crash-guard implementation is unchanged.
Recommended Action
- Approve this exact head.
allyblockcast
left a comment
allyblockcast
left a comment
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: ddf1dd6
Looks good. No Critical or Important issues remain on this head.
Suggestions (2)
- [error handling]
server/src/process-crash-guard.ts:283— When the futureonCrashbookkeeping hook is wired, consider keeping its timeout referenced untilfinish()starts; with no other referenced handles, the currentunref()can permit natural process exit before the final crash breadcrumb. - [error handling]
server/src/process-crash-guard.ts:293— Consider formatting a rejected bookkeeping value through a non-throwing serializer; a hostilemessagegetter or string coercion could otherwise take the re-entry path and truncate diagnostics.
Strengths
- Graceful shutdown now yields exit ownership to an active crash guard, preserving the non-zero status and complete fatal record.
- A real child-process regression test triggers the fatal event after the final graceful breadcrumb and verifies code 1 plus the final crash breadcrumb under stderr backpressure.
- Re-entrancy, strict unhandled-rejection pairing, hostile original-error getters, cyclic cause chains, and stalled stderr all receive focused coverage.
- The only change since the previously reviewed clean tree is an unrelated merged-master test timeout adjustment; the crash-guard implementation is unchanged.
Recommended Action
- Approve this exact head.
Dismiss duplicate exact-head Ally approval; keeping the user-side allyblockcast approval 4839914566 for head ddf1dd6.
… (BLO-19722 AC 2/3) `markRunsInterruptedByWorkerCrash` had no production caller. The reconciler only processes rows already marked `interrupted / worker_crashed`, so a real worker death still fell through to being rediscovered minutes later as `job_missing` — latching the agent to `error` with a reason naming the symptom rather than the crash. This is the wiring commit the #891 split deferred until PR A (`installProcessCrashGuard`, #949) had merged; it now has. The guard is installed at the process entrypoint, before `startServer()` has built the heartbeat service, so the marker is registered once that service exists and resolved at crash time. `crash-run-marking.ts` is a separate module on purpose. The entrypoint install site sits under `isMainModule`, which no test executes, so an `onCrash` passed inline there could only ever be verified by reading the file — and "verified by reading the file" is exactly how this hook stayed disconnected through three review rounds while its own unit tests passed. `installWorkerCrashGuard()` gives that link a seam a test can hold. Marking failures are deliberately not swallowed: the guard catches them and appends `crash bookkeeping failed: …` to the crash record, so a failed mark stays distinguishable from "nothing to mark".
3 participants
Thinking Path
job_missing.app/allyblockcast.kkroobranch, merged with currentmaster, so the normal review gate can attest the exact head.Linked Issues or Issue Description
What Changed
master.Verification
PATH=/opt/homebrew/bin:$PATH pnpm --filter @paperclipai/server typecheckPATH=/opt/homebrew/bin:$PATH pnpm exec vitest run server/src/__tests__/process-crash-guard.test.ts server/src/__tests__/process-crash-guard-exit.test.ts server/src/__tests__/graceful-shutdown-exit.test.ts server/src/__tests__/server-startup-feedback-export.test.ts— 37 passedPATH=/opt/homebrew/bin:$PATH node deploy/helm/paperclip/tests/probes.test.mjs— 2 passedgit diff --checkRisks
Model Used
Checklist