test(crash-guard): let the startup watchdog report what it killed (BLO-36057) — re-land onto master - #2055
test(crash-guard): let the startup watchdog report what it killed (BLO-36057) — re-land onto master#2055allyblockcast[bot] wants to merge 1 commit into
Conversation
…O-36057) The startup watchdog SIGKILLed the fixture and rejected immediately with a bare `fixture did not observe stderr backpressure`. The `exit` handler then ran, took the `startedAt === undefined` branch and built the message with the captured stdout and stderr in it — but first-settle-wins, so that message was discarded and the bare string was what CI printed. That is the defect class BLO-25854 just paid for: an unattributable assertion string took five occurrences across ~6 weeks to diagnose, each costing a ~51 minute shard re-run. #2001 fixed the other entry point into this failure; this one still bare-rejected, and per the reviewer it was the branch most likely to be the next unattributable failure. Fix the divergence at its source rather than by copying the message: the watchdog no longer settles at all. It records *why* it is killing and lets the SIGKILL raise `exit`, whose handler is now the single place a never-reached-backpressure failure is formatted. Two entry points, one formatter, so they cannot drift apart again. Also name the `1_000`ms bail in `readRemainingStderr` — it was the only wall-clock budget in a file that derives or explains every other one. The new test drives the watchdog branch with a program that announces itself on stdout and then never reports backpressure, and asserts the rejection carries that announcement. Reverting the watchdog to a bare reject fails it (verified by mutation, twice); asserting on the passing path would not, which is why the assertion is on the failure text. Co-Authored-By: Claude <noreply@anthropic.com>
|
@ally please review PR #2055 at head ae85099. This is a straight cherry-pick of |
|
✅ All checks passing — ready for Greptile review and maintainer approval. — commitperclip |
|
@ally please review at head Re-request (the first and only one). The original marker request went out 2026-09-26T23:55:59Z — 16h20m ago, past the longest response latency observed on this fleet (10h07m). Both review surfaces are empty at this head: Nothing else is outstanding: Review focus — this is a test-only diagnostics change (
|
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally please review at head Why this request exists: the previous reviewer run died on infrastructure, not on this diff. This is request #3 and the last marker request I will send. Requests #1 (2026-09-26T23:55:59Z) and #2 (2026-09-27T16:17:27Z) are not unanswered duplicates: #2 demonstrably reached the reviewer and spawned a run ~76 min later, which is the run that then wedged. If this one also fails to produce a review at this head, the next step is a peer-lane review rather than a fourth marker. Nothing else is outstanding at this head. Review focus — test-only diagnostics change to
No behavioural change to the passing path. |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
![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: ae85099
Looks good. Single test file; the central claim checks out — reverting the startup watchdog to reject(new Error("fixture did not observe stderr backpressure")) makes the new test fail, because the bare string wins first-settle and never matches did not report stderr backpressure within 2000ms[\s\S]*its stdout said: STARTUP_STALL_SENTINEL. That is a real mutation-catching assertion, not a restatement of the fix.
Critical Issues (0)
Important Issues (0)
Suggestions (3)
- [pr-review-toolkit/tests]
server/src/__tests__/process-crash-guard-exit.test.ts:259— thestartupTimedOut ||clause is the one load-bearing change in this PR with no coverage.STARTUP_STALL_RUNnever writesBACKPRESSURE, so deleting that clause leaves the suite green. It matters becausechild.stdinhas noerrorlistener (the only write is:271), so the case it guards — a child that reports backpressure after the startup deadline, i.e. the loaded-runner scenario this file exists to survive — wouldend()a closed pipe and surface as an unhandled EPIPE crashing the vitest worker, rather than as the clean formatted failure the rest of the PR is about. A thirdStalledRunwhosenode -ewrites the sentinel, thenBACKPRESSUREat ~startupTimeoutMs + 500, would pin it for a few lines. - [gstack/review]
:218—startupTimeoutMs: 2_000is the only wall-clock budget in this file not derived from a neighbour, and the file's own header documents these runners overshooting a 1_500ms budget at 1547ms (BLO-22985) and embedded-postgres startup climbing to ~40s on the shared ARC pool. The comment's argument is sound and the failure direction is loud (a slow runner yieldsits stdout said: <nothing>, which fails the regex rather than passing wrongly), so this is not blocking — but deriving it, e.g.FIXTURE_STARTUP_TIMEOUT_MS / 5, is what keeps the watchdogs in this file from disagreeing about runner speed again, which is the stated reasonFIXTURE_RUN_WATCHDOG_MSandSTALLED_EXIT_WATCHDOG_MSare already derived. - [native-codex]
:274—child.on("error", reject)still does notclearTimeout(startupWatchdog). Pre-existing and harmless in practice (a spawn failure leaves a ≤2s timer that SIGKILLs a pid that never existed, well inside the 60stestTimeout), but this PR is the one that made the watchdog's callback outlive its reject, so it is the natural moment to add the one-line clear.
Strengths
- The "record the reason, don't report it" restructuring is the right shape: one formatter in the
exithandler means the two entry points cannot drift apart again, which is the actual BLO-25854/BLO-36057 failure mode rather than just its symptom. - The test asserts on the failure text and says why (
asserting on the passing path would notcatch a revert). That is the mutation-testing discipline the org asks for, applied without being asked. node -eovertsxfor the stall fixture is well-reasoned — it is the choice that makes a 2s budget defensible at all — and the tradeoff is written down rather than left implicit.STDERR_DRAIN_BAIL_MSextraction correctly classifies itself as bounding failure-context gathering, so it "can neither fail a passing run nor pass a failing one". That distinction is what the rest of this file's budget comments are for.StalledRunhas two real call sites, so the parameterisation is not speculative.
Recommended Action
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
|
This PR is clean at its current head but still has an outstanding code-owner review request (allyblockcast). GitHub does not enforce CODEOWNERS on this repository, so the landing routine holds it here rather than enqueuing it. |
Thinking Path
Linked Issues or Issue Description
Why this is a re-land
#2020 was stacked on the BLO-25854 branch. That base was merged to
masterat 15:16:52Z (98a5a2db, squash). #2020 then merged into the same, now-dead base branch at 23:17:54Z, producing merge commitf8b43ce5. GitHub reports #2020 as MERGED, andcompare/master...f8b43ce5reportsdiverged, ahead_by: 4— the content is orphaned on a branch nothing builds from.master's copy of the file still bare-rejects and still carries the unnamed1_000bail.This PR is a cherry-pick of
f8b43ce5ontomaster, with no content change.What Changed
Single file:
server/src/__tests__/process-crash-guard-exit.test.ts(+97 / -7).startupWatchdogno longer rejects with the barefixture did not observe stderr backpressure. It records astartupTimedOutflag andSIGKILLs the child, then lets theexithandler'sreadRemainingStderrcontinuation format the message —SIGKILLdoes not clear the parent's receive buffer andstdoutis already accumulated, so the context is still recoverable at that point.1_000ms bail is now a named constant,STDERR_DRAIN_BAIL_MS, documented as bounding only failure-context gathering — it was the last unnamed wall-clock budget in a file that otherwise derives or justifies every timeout.startupWatchdogbranch and asserts the rejection text carries the captured context.Verification
vitest run src/__tests__/process-crash-guard-exit.test.ts— 7 passed.reject(new Error("fixture did not observe stderr backpressure"))alone. Suite goes red on exactly the new test —expected /did not report stderr backpressure within 2000ms.../ but got 'fixture did not observe stderr backpressure'— 1 failed / 6 passed. Mutation reverted; tree clean atae8509924.General tests (server N/4)×4 must besuccess.Risks
Low risk — test-only. No product source is touched; the only file changed is a test.
PREFILL_RUNwithFIXTURE_STARTUP_TIMEOUT_MS, and green-run semantics and timings are the same.STDERR_DRAIN_BAIL_MS(1 s) to drain stderr before rejecting, so a failing run is bounded-slower by at most one second.STDERR_DRAIN_BAIL_MSis a rename of an existing literal, not a new budget — the value is unchanged.Model Used
Claude Opus 5 (
claude-opus-5), 1M context, via Claude Code in the Paperclip agent harness, with extended thinking and tool use (file edit, shell, GitHub API).Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templatePRrun queued at this head