From d9db60aad5b9839d8ed8e3276dddd648b1a3af4b Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 9 Sep 2026 19:11:52 -0700 Subject: [PATCH 1/2] test(cli): give the vale timeout test a real margin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- packages/cli/test/vale-run.test.ts | 43 +++++++++++++++++++++++++++--- 1 file changed, 39 insertions(+), 4 deletions(-) diff --git a/packages/cli/test/vale-run.test.ts b/packages/cli/test/vale-run.test.ts index 39a28031..c8520921 100644 --- a/packages/cli/test/vale-run.test.ts +++ b/packages/cli/test/vale-run.test.ts @@ -258,15 +258,50 @@ withVale("runVale against the real binary", () => { }); it("terminates and reports a timeout rather than hanging", async () => { + // THE BUDGET AND THE INPUT ARE BOTH LOAD-BEARING. This asserted the winner + // of a race until taskless/cli#262 work looked at it: a 1ms budget against + // a one-line 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 demanded a timeout. + // + // Its sibling in `ValeRunOutcome.blocking` had the identical shape and was + // MEASURED failing that way — twice across 13 full-suite runs, reporting + // `status: "ok"` — before it was given a real margin. This test survived + // only because its window was narrower, not because it was safe. + // + // The metric that matters is the ABSOLUTE margin (duration minus budget), + // not a ratio: what has to happen is the child finishing before a delayed + // timer callback runs. 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 is chosen over the sibling's 17,000 deliberately: it is 20x the + // margin that actually flaked while costing a quarter of the suite time, + // and this test asserts the message rather than the blocking flag, which + // the sibling already covers with the larger fixture. + // + // `maxFileBytes` raises `VALE_MAX_FILE_BYTES` for 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 (taskless/cli#321) and reports + // `status: "ok"` with a notice, never exercising the timeout at all. const cwd = makeProject( `${header}\n[*.md]\nno-simply.no-simply = YES\n`, { "no-simply": existenceRule("simply", "Avoid 'simply'") }, - { "doc.md": "Just simply do it.\n" } + { "doc.md": `${"Just simply do it. ".repeat(8000)}\n` } ); - // 1ms cannot survive process startup, so this exercises the kill path - // without needing a pathological corpus to provoke it. - const outcome = await runVale({ cwd, paths: ["doc.md"], timeoutMs: 1 }); + const outcome = await runVale({ + cwd, + paths: ["doc.md"], + timeoutMs: 100, + maxFileBytes: Number.POSITIVE_INFINITY, + }); expect(outcome.status).toBe("timeout"); if (outcome.status !== "timeout") return; expect(outcome.message).toContain("terminated"); From ff88a5f09f86bfbb310a4e0931d78e207bd88d36 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 9 Sep 2026 20:06:17 -0700 Subject: [PATCH 2/2] docs(cli): correct the issue citation and reconcile the flake counts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings on #327, both about the comment rather than the test. The comment said this race was asserted "until taskless/cli#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. --- packages/cli/test/vale-run.test.ts | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/packages/cli/test/vale-run.test.ts b/packages/cli/test/vale-run.test.ts index c8520921..291b025e 100644 --- a/packages/cli/test/vale-run.test.ts +++ b/packages/cli/test/vale-run.test.ts @@ -259,17 +259,29 @@ withVale("runVale against the real binary", () => { it("terminates and reports a timeout rather than hanging", async () => { // THE BUDGET AND THE INPUT ARE BOTH LOAD-BEARING. This asserted the winner - // of a race until taskless/cli#262 work looked at it: a 1ms budget against - // a one-line 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 demanded a timeout. + // of a race until taskless/cli#327: a 1ms budget against a one-line + // 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 demanded a timeout. + // + // NO ISSUE EVER FLAGGED THIS TEST. It was found while investigating + // taskless/cli#262, which reports a different flake entirely — two + // SUBPROCESS-SPAWNING tests in `error-envelope.test.ts` and + // `verify-test-commands.test.ts` — and does not name this one. #262 is + // where the search started, not what it found, and it remains open. // // Its sibling in `ValeRunOutcome.blocking` had the identical shape and was - // MEASURED failing that way — twice across 13 full-suite runs, reporting - // `status: "ok"` — before it was given a real margin. This test survived - // only because its window was narrower, not because it was safe. + // MEASURED failing that way, reporting `status: "ok"`, before it was given + // a real margin. This test survived only because its window was narrower, + // not because it was safe. + // + // TWO SEPARATE MEASUREMENT PASSES COUNTED THAT SIBLING, which is why the + // numbers here and in its own comment below differ and neither is wrong. + // The first, while #323 was open, saw it lose ONCE ACROSS FOUR concurrent + // full-suite runs. The second, counting a set of captured logs recovered + // later, saw TWICE ACROSS 13. Same test, same failure, different samples. // // The metric that matters is the ABSOLUTE margin (duration minus budget), // not a ratio: what has to happen is the child finishing before a delayed