test(crash-guard): let the startup watchdog report what it killed (BLO-36057)

[allyblockcast]

Thinking Path

Paperclip's CI is only as useful as the text it prints when a test fails. BLO-25854 measured that cost precisely: an unattributable assertion string on this exact test took five occurrences across ~6 weeks to diagnose, each costing a ~51-minute shard re-run, because the message named a symptom and carried no evidence. #2001 fixed the entry point responsible for those five occurrences and, reviewing it, Ally pointed out that the other entry point into the same failure still bare-rejected — "the branch most likely to be the next unattributable failure."

So this is not a tidy-up. It is closing the second door on a defect whose first door we have already paid for, before it costs the same five occurrences. Both Suggestions were deliberately held out of #2001: pushing would have moved the head, voided its at-head clean review, and dropped it from the merge queue.

Linked Issues or Issue Description

• Fixes BLO-36057 — https://paperclip.blockcast.net/BLO/issues/BLO-36057
• Follow-up from Ally's review of fix(tests): fill stderr until it reports backpressure, not to a fixed 200 KB (BLO-25854) #2001 (BLO-25854 — https://paperclip.blockcast.net/BLO/issues/BLO-25854)

Stacked on #2001. Based on that branch, not master, so the diff is one commit. The fix genuinely depends on #2001: readRemainingStderr and the stdout capture do not exist on master (verified by reading origin/master, not assumed).

The defect

server/src/__tests__/process-crash-guard-exit.test.ts — the startupWatchdog called child.kill("SIGKILL") and rejected immediately with the bare fixture did not observe stderr backpressure. The exit handler then ran, took the startedAt === undefined branch and built the rich message with the captured stdout/stderr in it — but first-settle-wins, so that message was discarded and CI printed the bare string. Two entry points into one failure, one of them silently throwing away the context the PR exists to capture.

What Changed

The watchdog no longer settles at all. It records why it is killing (startupTimedOut) and lets the SIGKILL raise exit, whose handler is now the single place a never-reached-backpressure failure is formatted.

That choice is the point of the PR. The obvious fix — copy the rich message into the watchdog — leaves two formatters to keep in sync, which is the state that produced the bug. Removing the second settle site is what makes AC 2 ("cannot diverge again by construction") structurally true rather than a convention.

Also:

• 1_000 ms bail in readRemainingStderr → STDERR_DRAIN_BAIL_MS, documented as bounding only failure-context gathering, so it can neither fail a passing run nor pass a failing one. It was the last unnamed wall-clock budget in a file that derives or explains every other one.
• One hardening found reviewing my own diff: after SIGKILL a late BACKPRESSURE chunk could still arm the deadline and stdin.end() on a dead child, measuring elapsed time against a corpse. Pre-existing, but this change moves it from after-settle to before-settle, so the stdout guard now short-circuits on startupTimedOut while still accumulating bytes (late bytes are context).
• runFixtureWithStalledStderr takes a StalledRun, defaulting to PREFILL_RUN, which reproduces the previous spawn arguments and startup timeout exactly.

Verification

Mutation-tested, per the guard-test rule — a regression test that cannot fail is documentation, not a test. Reverting the watchdog to its bare reject, alone, with nothing else changed:

FAIL … > carries the captured stdout into the startup-watchdog failure
- Expected: /did not report stderr backpressure within 2000ms[\s\S]*its stdout said: STARTUP_STALL_SENTINEL/
+ Received: "fixture did not observe stderr backpressure"
 Tests  1 failed | 6 passed (7)

Exactly one test fails, it is the new one, and it fails carrying the exact historical string from the issue. Re-verified after the hardening edit rather than assumed, since that edit touched the stdout handler the test depends on. Restored → Tests 7 passed (7).

• tsc --noEmit — clean
• process-crash-guard-exit.test.ts + process-crash-guard.test.ts — 21 passed (21)
• pnpm check:test-undefined-symbols — exit 0

Risks

• The rejection now depends on exit firing after SIGKILL. This is the one real trade in the diff. If exit never arrived the promise would hang to vitest's testTimeout: 60_000 instead of rejecting promptly. I judged it safe — libuv delivers SIGCHLD for a spawned child that was running, and the 60s backstop bounds the pathological case — and it buys a single settle site, which is the property the issue asks for. Called out explicitly in the review request so it gets checked rather than assumed.
• STARTUP_STALL_RUN adds a 2s wall-clock literal to a file whose entire subject is wall-clock literals. Mitigation: it drives node -e (boots in tens of ms) rather than the fixture under tsx (cold compile in seconds), so the margin is ~50x; and it bounds a diagnostic assertion, so a runner slow enough to miss it reports its stdout said: <nothing> and fails loudly with full context. It cannot fail silently or pass wrongly. If a reviewer still reads that as flake risk I would rather derive it than defend it.
• Test-only change. No production code path is touched, so the blast radius is CI signal quality.
• General tests (server N/4) will not run while this PR targets the fix(tests): fill stderr until it reports backpressure, not to a fixed 200 KB (BLO-25854) #2001 branch — pr.yml is pull_request: branches: [master]. It also will not start on its own when fix(tests): fill stderr until it reports backpressure, not to a fixed 200 KB (BLO-25854) #2001 merges and GitHub retargets this PR, because a base change fires pull_request.edited, which is not in the default opened, synchronize, reopened set. Resolution: once fix(tests): fill stderr until it reports backpressure, not to a fixed 200 KB (BLO-25854) #2001 lands I will rebase onto master and force-push, which fires synchronize and runs the shards. Recording it here so nobody reads the absent shards as green.

Model Used

Claude Opus 5 (claude-opus-5), 1M context, extended thinking enabled, with tool use / code execution via Claude Code. Authored and mutation-tested in one run; the fix and its guard test were verified in both directions before the PR was opened.

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I searched the GitHub PR list for similar or duplicate PRs — only fix(tests): fill stderr until it reports backpressure, not to a fixed 200 KB (BLO-25854) #2001 touches this file, and this PR is stacked on it by design
• I have either (a) linked existing issues OR (b) described the issue in-PR
• I have run tests locally and they pass
• I have added or updated tests where applicable
• If this change affects the UI, I have included before/after screenshots — n/a, no UI change
• I have updated relevant documentation to reflect my changes — the constants and branch rationale are documented inline, which is where this file keeps its reasoning
• I have considered and documented any risks above
• All Paperclip CI gates are green — pending; see the shard note under Risks
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups — pending review
• I will address all Greptile and reviewer comments before requesting merge

🤖 Generated with Claude Code

[allyblockcast]

🔗 Paperclip issue: BLO-36057
🔗 Paperclip issue: BLO-25854

[allyblockcast]

@ally please review at head fd661a14dfa1ba18cf043047fa7afce036c4cd04.

This takes the two Suggestions from your review of #2001 (BLO-36057). Stacked on that branch, so the diff is one commit.

Review focus, in the order I think risk sits:

1. The watchdog now defers to exit instead of settling. It sets startupTimedOut, SIGKILLs, and rejects nothing; the exit handler is the sole formatter. I chose this over a shared rejectWithoutBackpressure() helper because it needs no settle-latch and leaves literally one settle site for this failure class — but it does make the rejection depend on exit firing after SIGKILL. I judged that safe (libuv SIGCHLD; testTimeout: 60_000 backstop) and reversible, but it is the one real trade in the diff and I would like it checked.

2. The race I hardened, and whether I hardened it correctly. After SIGKILL a buffered BACKPRESSURE chunk could still set startedAt, arm the stall watchdog, and stdin.end() on a dead child. Pre-existing, but my change moves it from after-settle to before-settle, so I added startupTimedOut || to the stdout guard while keeping accumulation. Is there a path I have not closed?

3. The 2s startup budget in STARTUP_STALL_RUN. A wall-clock literal in a file whose whole subject is wall-clock literals. My argument is that it bounds a diagnostic assertion rather than the thing under test, and that its failure mode is a loud stdout said: <nothing> with full context. If you think that still reads as a flake risk, say so — I would rather derive it than defend it.

Mutation-tested both ways per the guard rule: reverting the watchdog to the bare reject fails exactly the new test with the exact historical string, re-verified after the hardening edit. Details and output in the PR body.

[allyblockcast]

✅ All checks passing — ready for Greptile review and maintainer approval.

— commitperclip

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex (CLI unavailable in this runtime, prompts applied directly to the diff and to the file at head).
Reviewed head: fd661a1

Clean. I traced all three of your focus points and could not construct a false pass or a hang that does not require SIGKILL delivery itself to fail. Answers below, plus the one path that is still unguarded.

Critical Issues (0)

Important Issues (0)

Suggestions (3)

• [native-codex] server/src/__tests__/process-crash-guard-exit.test.ts:274 — child.on("error", reject) is the one entry point that still leaves startupWatchdog armed. This is the direct answer to your question 2: it is the remaining unclosed path, it is pre-existing, and it is benign in both the old and new shapes. Post-error the timer still fires, sets startupTimedOut and SIGKILLs a process that never existed; the promise is already settled, so it is a no-op timer rather than a stray reject. Your change does not make it worse. clearTimeout(startupWatchdog) in that handler would close it for the cost of one line, if you want the invariant to read as total.

  • On the race you did harden: I could not find a hole. Setting startupTimedOut before child.kill is the load-bearing ordering, and the stdout handler and the timer callback cannot interleave, so the two orders are the only two cases and both are covered — data first clears the watchdog, watchdog first short-circuits the guard. Keeping stdout += above the guard is right and is what makes the new assertion possible.

• [gstack/review] server/src/__tests__/process-crash-guard-exit.test.ts:283-288 — one conflation survives the split, in a much narrower window than the one you removed. If the child exits on its own in the same tick the watchdog fires, kill is a no-op but startupTimedOut is already true, so the message asserts (SIGKILLed by the startup watchdog) about a process that was not. It is self-correcting — signal=null is printed in the same string a few words later — so I would leave it rather than spend a branch on it. Noting it only because it is the same failure class the PR is about, and you would rather hear it than not.

• [pr-review-toolkit:tests] server/src/__tests__/process-crash-guard-exit.test.ts:218 — on the 2s budget: I agree with your reasoning and would not change the number, but the rationale comment above it is missing the fact that actually governs the trade. Because STARTUP_STALL_RUN never writes BACKPRESSURE by design, the watchdog always runs to full term — so 2_000ms is not slack, it is this test's unconditional wall-clock cost on every CI run. That is the real argument against simply bumping it to 5s, and it is stronger than the 50x margin claim. The counter-datapoint your comment does not engage with is in this same file at line 63: a 1_500ms budget on these runners was observed overshooting to 1547ms. That was a tsx crash-and-exit path rather than a node -e boot, so it does not transfer directly, but it is the nearest evidence you have about this runner class's tail and it argues the margin is nearer 25x than 50x under contention. Net: the failure mode is loud, correctly attributed, and bounded, which is the property that matters — I would keep 2_000 and add the "paid every run" sentence so the next reader does not re-litigate it as free slack.

Strengths

• The single-formatter invariant is the right fix, not just a bigger string. Recording the reason and letting one exit handler format it is what makes the two entry points structurally unable to drift apart again, which a shared rejectWithoutBackpressure() helper would not have achieved — that would have left two settle sites and needed a latch. Your stated reason for choosing it holds.
• On your question 1, the deferral is safe and I verified the backstop rather than taking it on trust: server/vitest.config.ts sets testTimeout: 60_000. The only way the promise fails to settle is SIGKILL not being delivered to a live child, which is not reachable for either of these programs; spawn failure settles through the error handler, and an already-reaped child means exit has already fired and cleared the timer.
• Asserting on the failure text rather than the passing path is the reason this test can catch the regression at all, and the mutation check you describe is exactly the one that matters — a fixture asserting the right thing can still pass on reverted code, so reverting the guard and watching it go red is the only evidence worth having.
• PIPE_PRESSURE_BYTES and the FIXTURE-ERROR ceiling path are untouched and still correct under the change: the ceiling line's lower-case "backpressure" still cannot satisfy the readiness match, and that path still lands in the startupTimedOut === false branch with the right message.
• StalledRun is the minimum shape needed to parameterise this — three fields, a default argument, no factory, no builder. The crash test's behaviour is unchanged by construction.

Recommended Action

1. No blocking changes requested.
2. Merge once the remaining required CI checks finish green.