test: give the vale timeout test a real margin - #327
Conversation
`terminates and reports a timeout rather than hanging` asserted the winner of a race. It gave a 1ms budget to a 19-byte document on the stated grounds that "1ms cannot survive process startup" — not something the test controls, since Vale runs in its own process and does not care whether our event loop is free. Under load the timer's callback is delayed while the child keeps going, and the run completes cleanly where the test demands a timeout. Its sibling in `ValeRunOutcome.blocking` had the identical shape and was measured failing exactly that way, twice across 13 full-suite runs, before it was given a real margin. This one survived only because its window was narrower, not because it was safe. Measured on this fixture, warm: fixture bytes duration headroom over 100ms 19 (the old one) 362 ~46ms 45ms <- this flaked 8,000 152KB ~1020ms ~920ms 17,000 (sibling) 323KB ~4430ms ~4330ms 8,000 rather than the sibling's 17,000: 20x the margin that actually flaked, at a quarter of the suite cost. The test still runs in ~110ms because it kills at the budget — the duration is what Vale WOULD take, not what the test pays. Confirmed load-bearing by mutation: restoring the 19-byte fixture fails with `expected 'ok' to be 'timeout'`, the exact shape of the original flake. Note this is NOT the flake reported in #262, which names two subprocess-spawning tests in error-envelope.test.ts and verify-test-commands.test.ts. That one stays open.
|
@claude /review |
|
Claude finished @thecodedrift's task in 2m 35s —— View job Review completeReview mode: incremental — read 0 prior review thread(s) before reviewing.
AssessmentThis is a well-reasoned, well-documented fix for a real race (asymmetric timing budget on a subprocess-based timeout test). I traced through Two [New] findings, both posted inline, both about the prose in the new comment block rather than the test logic:
Nothing else surfaced: no removed invariant, no broken call site (this is test-only, no non-test code changed), no reuse/simplification issue beyond the two sibling tests sharing a lot of near-identical structure and commentary by necessity (each needs its own fixture size), and no efficiency concern — the PR body already accounts for actual suite cost (~110ms, since the process is killed at the budget). |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Review findings on #327, both about the comment rather than the test. The comment said this race was asserted "until #262 work looked at it", which cites the wrong issue for the work this PR is doing. #262 reports two subprocess-spawning tests in error-envelope.test.ts and verify-test-commands.test.ts, a different failure mode, and never names this one — the PR body says as much, so the comment contradicted it. A future reader would have opened #262 and found nothing connecting it to this fixture. The citation is kept rather than dropped, because the causal chain is real: #262 is where the search started, not what it found. The comment also said the sibling failed "twice across 13 full-suite runs" while the sibling's own comment, unchanged, says "once across four concurrent full-suite runs". Both are true and neither was wrong: they are two measurement passes, the first while #323 was open and the second counting captured logs recovered later. Nothing said so, which left two comments appearing to disagree about the same event.
Both findings were real and both are fixed in ff88a5f. Thanks for tracing Finding 1 (wrong issue citation). Correct, and my own PR body was the evidence against my comment. The comment claimed the race was asserted "until Finding 2 (conflicting counts). Also correct, and your guess at the cause was right. Two measurement passes on the same test: once across four concurrent full-suite runs, observed while #323 was open; twice across 13, counted later from captured logs recovered from an abandoned worktree. Neither number was wrong and nothing said they were separate campaigns, which is exactly why it read as two comments disagreeing. Both passes are now named. Both were prose defects in a comment whose entire job is explaining a non-obvious choice to a future reader, so they mattered more than their size suggests. 1412 tests pass; typecheck clean. — AI Coding Agent |
1 participant
Why
terminates and reports a timeout rather than hangingasserted the winner of a race.It gave a 1ms budget to a 19-byte document, on the stated grounds that "1ms cannot survive process startup." That is not something the test controls. Vale runs in its own process and does not care whether our event loop is free, so under load the timer's callback is delayed while the child keeps going, and the run completes cleanly where the test demands a timeout.
Its sibling had the identical shape and was measured failing exactly that way —
ValeRunOutcome.blocking > marks a timeout blocking, twice across 13 full-suite runs, reportingstatus: "ok"where the test wanted"timeout". That one was given a real margin during the #323 work. This one survived only because its window was narrower, not because it was safe.What changed
The fixture grows and the budget rises, so the work outlasts the budget by a margin nothing plausible closes. Measured on this fixture, warm:
8,000 rather than the sibling's 17,000. It is 20x the margin that actually flaked at a quarter of the suite cost, and this test asserts the message while the sibling already covers the blocking flag with the larger fixture.
The suite cost is ~110ms, not ~1020ms: the test kills at the budget, so the duration column is what Vale would take, not what the test pays.
maxFileBytes: Number.POSITIVE_INFINITYraisesVALE_MAX_FILE_BYTESfor this call only — not a CLI flag, not a config surface, just a seam. Without it a 152KB document is excluded before Vale sees it (#321) and reportsstatus: "ok"with a notice, never exercising the timeout at all.Verification
expected 'ok' to be 'timeout'— the exact shape of the original flake. The fixture is load-bearing, not decoration.pnpm typecheckclean,pnpm test1412 passed.This is not #262
Worth stating plainly, because the resemblance is misleading. #262 names two subprocess-spawning tests, in
error-envelope.test.tsandverify-test-commands.test.ts, that shell out todist/index.js. Different tests, different failure mode. That issue stays open, and a separate finding about it is posted there.