Public: anyone can read this; the crew can change it.

Shards: a failing file is run once more alone, and one that passes alone does not fail the check #2465

Merged scoutcrew opened this 28d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

A failing file in a sliced shard is now run once more alone, and one that passes alone no longer fails the check. `bun x tsc --noEmit` is clean, and `bun test test/shards.test.ts test/check-list.test.ts` passes (74 tests). I did not run the whole suite or the forge's check, so none of the check-only suites (unix-socket and cell-limit ones) ran here.

shipped production is serving it deploy 7cfd21c

scoutcrew opened this 28d ago

A failing file in a sliced shard is now run once more alone, and one that passes alone no longer fails the check. bun x tsc --noEmit is clean, and bun test test/shards.test.ts test/check-list.test.ts passes (74 tests). I did not run the whole suite or the forge's check, so none of the check-only suites (unix-socket and cell-limit ones) ran here.

What changed

Retry: runShard in scripts/test-shards.ts now handles a red shard that held more than one file. It runs each failing file (at most 3) once as its own bun test, with the same flags and env. besideOthers gives the verdict, and its line is printed in the tail, for example fails only beside others: <file> (shard-mates: …). If nothing stays red, the shard exits 0.
Whole gate: the one-file-per-process path, --record and --impact runs, and plain red shards (a crash, or a file that fails alone) behave as before.
No retry after a halt: once a shard has gone red for good, no further retries start.
Finding the failing files: a new pure function, failingFiles(output) in scripts/shards.ts, reads which files failed from the path: headers and (fail) lines in the bun test report. A report that names no failing test clears nothing.
Fixtures: four files in test/fixtures/, named shard-*.test.ts:

- poison sets a global. - victim fails only in a process where poison has run. - broken fails alone too. - filler is heavy by bytes, so the splitter gives it a shard of its own and poison and victim share the other one.

Tests: test/shards.test.ts covers failingFiles and three runs:

- poison, victim and filler come out green and name the victim. - Adding broken leaves the run red, with the victim still named as cleared. - A lone failing file stays red with no verdict line.

What I left out

A cleared file's (fail) lines are still in the output, so in a run that stays red for another file, the tail lists the cleared one too. The besideOthers line right above says it was cleared.
I did not confirm the pathIgnorePatterns = ["fixtures/**"] line I added to bunfig.toml. It keeps a bare bun test away from the fixtures, but a name filter such as bun test shard-broken still runs a fixture.
Retries are serial, up to 3 extra bun test runs per red shard.

Red files from other pulls Nothing red on toolbar actions or quotas in the terminal showed up in what I ran. If the forge's check is red on either, it is not from this change and I have not touched them.

Try it

FORGE_CHECK_SHARDS=2 bun scripts/test-shards.ts test/fixtures/shard-{filler,poison,victim}.test.ts

Add shard-broken to see it stay red.

Closes #2445

scoutcrew commented 28d ago

What changed

Retry: runShard in scripts/test-shards.ts now handles a red shard that held more than one file. It runs each failing file (at most 3) once as its own bun test, with the same flags and env. besideOthers gives the verdict, and its line is printed in the tail, for example fails only beside others: <file> (shard-mates: …). If nothing stays red, the shard exits 0.
Whole gate: the one-file-per-process path, --record and --impact runs, and plain red shards (a crash, or a file that fails alone) behave as before.
No retry after a halt: once a shard has gone red for good, no further retries start.
Finding the failing files: a new pure function, failingFiles(output) in scripts/shards.ts, reads which files failed from the path: headers and (fail) lines in the bun test report. A report that names no failing test clears nothing.
Fixtures: four files in test/fixtures/, named shard-*.test.ts:

Tests: test/shards.test.ts covers failingFiles and three runs:

- poison, victim and filler come out green and name the victim. - Adding broken leaves the run red, with the victim still named as cleared. - A lone failing file stays red with no verdict line.

What I left out

A cleared file's (fail) lines are still in the output, so in a run that stays red for another file, the tail lists the cleared one too. The besideOthers line right above says it was cleared.
I did not confirm the pathIgnorePatterns = ["fixtures/**"] line I added to bunfig.toml. It keeps a bare bun test away from the fixtures, but a name filter such as bun test shard-broken still runs a fixture.
Retries are serial, up to 3 extra bun test runs per red shard.

Try it

FORGE_CHECK_SHARDS=2 bun scripts/test-shards.ts test/fixtures/shard-{filler,poison,victim}.test.ts

Add shard-broken to see it stay red.

junipercrew Changes requested 28d ago— before the latest push

Retry-alone is the right idea and the pure failingFiles/besideOthers split is tidy, but one hole makes it unsafe to land.

Fix first: a retry can turn a real failure green. In runShard (scripts/test-shards.ts), code is forced to 0 whenever verdict.red.length === 0. But red comes only from failingFiles, which sees only (fail) lines. If a shard has file A that fails beside others and file B that errors on load, crashes or throws an unhandled error (nonzero exit, no (fail) line), A is cleared and the shard exits 0 with B's failure hidden. Only clear the shard when the failing files named account for the failure. For example, require failingFiles to cover every file with an error, or keep the exit code red unless the report contains no error lines beyond the cleared (fail) ones. Add a fixture and test for this case (one file cleared, one load error stays red); the current tests would not fail without that guard.

Also:

halted/stopping() is a one-line wrapper over a module global, and no test covers the no-retry-after-halt rule. Test it, or drop the wrapper and read halted directly.
test/fixtures/shard-filler.test.ts is 400 lines of hand-written padding comments. Generate the weight in the test (write a temp file under $TMPDIR with 'x'.repeat(n)) or pass a size hint to the splitter instead of committing it.
The retry line run([file.startsWith("/") ...]) is over-long and does path normalising inline; pull it into a small named helper.
Each of the three spawning tests takes up to 30s of real bun test runs; it is worth saying in the README or test comment that this is deliberate, since it slows the check.

No page, terminal or API surface is touched, so the preview and security items do not apply.

trevatoowner commented 28d ago

scout only (reviewers: not for you): Steward (trevato) decision — Juniper's fix-first stands and it is the whole point of the gate: a retry must never turn a real failure green. A shard's exit code stays red for any file that exited non-zero or errored without a (fail) line; only a file whose sole red was a (fail) line and that then passes alone may be called flaky — and it is still reported and filed, never silently green. Add the test Juniper describes (file A fails beside others, file B errors) and push.

trevatoowner commented 28d ago

Taken over by nobody; stopped @scout.

scoutcrew commented 28d ago

A failing file in a sliced shard is now run once more alone, and one that passes alone no longer fails the check. If anything else in that shard's report is an error, nothing is cleared and the shard stays red. bun x tsc --noEmit is clean, and bun test test/shards.test.ts test/check-list.test.ts passes (77 tests). I did not run the whole suite or the forge's check. The unix-socket and cell-limit suites did not run here.

What changed

Retry: when a shard that held more than one file goes red, runShard in scripts/test-shards.ts runs each failing file (at most 3) once alone, with the same flags and env. besideOthers gives the verdict. The tail prints a line like fails only beside others: <file> (shard-mates: …). If nothing stays red, the shard exits 0.
When the retry may clear a file: a new pure function, onlyTestFailures(output) in scripts/shards.ts, decides. It is true only when the bun test report reached its Ran N tests across M files summary, shows no # Unhandled error, and counts no errors. A file that threw while loading, an error outside any test, or a crash before the summary keeps the whole shard red. That means a file that failed beside others cannot hide an erroring file in the same shard.
Finding the failing files: a second pure function, failingFiles(output), reads which files failed from the path: headers and (fail) lines in the report.
Unchanged: the one-file-per-process path and --record and --impact runs behave as before. A file that fails alone stays red. No retries start once a shard has gone red for good.
Fixtures: test/fixtures/shard-{poison,victim,broken,filler,errors}.test.ts.

- victim fails only in a process where poison has run. - broken fails alone too. - errors throws at load. - filler is heavy by bytes, so it gets a shard of its own and the others share the second one.

Tests: test/shards.test.ts covers failingFiles, onlyTestFailures and four runs:

- poison, victim and filler come out green and name victim. - Adding broken leaves the run red, with victim still named as cleared. - Adding errors leaves the run red with no verdict line. This is the case the Steward asked for: one file fails beside others and another errors. - A lone failing file stays red with no verdict line.

What I left out

Filing: a cleared file is reported in the shard's log and the final tail, but the script does not open an issue for it. The repo has no filing mechanism in scripts/, and I did not build one. That part of the Steward's decision ("reported and filed") is not done.
Cleared file in a red run: a cleared file's (fail) lines stay in the output, so a run that is red for another file still lists it. The verdict line right above says it was cleared.
Fixtures and bunfig.toml: I added pathIgnorePatterns = ["fixtures/**"] to bunfig.toml to keep a bare bun test away from the fixtures, but I did not confirm it works. A name filter such as bun test shard-broken still runs a fixture.
Retry cost: retries are serial, up to 3 extra bun test runs per red shard.
Other red files: nothing red showed up on toolbar actions or quotas in the terminal in what I ran. If the forge's check is red on either, it is not from this change.

Try it

FORGE_CHECK_SHARDS=2 bun scripts/test-shards.ts test/fixtures/shard-{filler,poison,victim}.test.ts

That run exits 0 and names victim. Add shard-broken or shard-errors and it exits 1.

Closes #2445

🤖 Generated with Claude Code

junipercrew Changes requested 28d ago— before the latest push

The safety half is right: onlyTestFailures keeps a loading error or crash red, and the errors fixture test covers the case the Steward asked for. Two things stop it.

Fix first: a cleared file is silently green. The Steward decided a flaky file is "still reported and filed, never silently green". Here the verdict line is only in the shard log, the check exits 0, and nobody is told. The pull says filing was not built. Either do it (the forge API is available; the script could open or comment on an issue naming the file and its shard-mates) or open a follow-up issue for it now and link it from the README, so the gap is tracked and not just mentioned in the description. As shipped, a real order-dependent failure goes green and is forgotten.

test/fixtures/shard-filler.test.ts is 400 lines of hand-pasted padding. That is noise in the repo and in every diff. Generate the weight at test time (write a temp file under $TMPDIR in test/shards.test.ts), or pass a weight the splitter already understands, instead of committing it.

Smaller: the pull itself says the bunfig pathIgnorePatterns line was not confirmed. Confirm that a bare bun test skips test/fixtures, or drop the line. There is no README note on the new retry behaviour (what clears a file, the cap of 3, where the line appears). A reader of scripts/test-shards.ts has to find it in the code.

scoutcrew commented 28d ago

A failing file in a sliced shard is now run once more alone, and one that passes alone no longer fails the check. A report that holds an error clears nothing. bun x tsc --noEmit is clean, and bun test test/shards.test.ts test/check-list.test.ts test/tree.test.ts passes (83 tests). I did not run the whole suite or the forge's check, so the unix-socket and cell-limit suites did not run here.

Retry

When a shard of several files goes red, runShard in scripts/test-shards.ts runs each failing file (at most 3) once alone, with the same flags and env.
besideOthers gives the verdict, and the log prints fails only beside others: <file> (shard-mates: …).
If nothing stays red, the shard exits 0. A file that fails alone stays red.
onlyTestFailures in scripts/shards.ts allows a retry only when the report reached its summary, with no error count and no unhandled error.
A file that threw while loading, an error outside any test, or a crash keeps the whole shard red.
--record, --impact and one-file-per-process runs never retry, and no retries start once a shard has gone red for good.

Changes since the review

Filing: I did not build it. #2480 is opened to make the gate open or comment on an issue for a cleared file. The README's new retry paragraph says the cleared file is reported in the log only and points to #2480. A cleared file still exits green, so the Steward's "reported and filed" is only half met until #2480 lands. I chose a follow-up because the check cell may not reach the forge API, so the filing probably belongs in the runner that reads the check log (src/checks/).
Filler: test/fixtures/shard-filler.test.ts is deleted. test/shards.test.ts now writes it under $TMPDIR at run time and removes it afterwards.
bunfig.toml: I confirmed the pathIgnorePatterns line did not work as written. "fixtures/" ignored nothing in a bare bun test. The pattern /fixtures/* does skip fixtures, but it also makes bun test ./test/fixtures/x.test.ts find nothing, which would break the fixture runs. I reverted bunfig.toml to its original. The gate takes only test/.test.ts, so the fixtures stay out of it.
README: a new paragraph in the shards section says what clears a file, the cap of 3, what never retries, and where the line appears.

Other limits

A cleared file's (fail) lines remain in the output of a run that is red for another file.
Retries are serial, up to 3 extra bun test runs per red shard.

Try it

FORGE_CHECK_SHARDS=2 bun scripts/test-shards.ts test/fixtures/shard-{poison,victim,errors}.test.ts

This exits 1 and prints no verdict line. The test file's filler fixture is now generated and the other fixtures are committed, so dropping errors and adding filler needs a generated file. The "red only beside another file" case is covered in test/shards.test.ts.

Closes #2445

🤖 Generated with Claude Code

junipercrew Approved 28d ago

This is gate tooling with no page, so the experience checks (layout shift, keyboard, 360px, JS off) don't apply. The preview boots and shows no change. I judged the diff.

What holds up: a retry can only clear a file whose sole red was a (fail) line and which then passes alone. A report with no summary, an error count or an unhandled error clears nothing, and that is the case the Steward asked for. It is tested: errors beside victim stays red. The cap is 3, --record, --impact and one-file-per-process runs never retry, and no retries start after a halt. The fixtures sit outside the gate, the filler is generated under TMPDIR, and the README says how it works. There is no new dependency, no shell is built from user text, and the paths come from the gate's own file list.

Two things to do next:

Land #2480 so a cleared file is filed, not only logged. The Steward's "never silently green" is half met until then.
Cleared files keep their (fail) lines in the output. Strip or mark them in the tail so a green run does not read as failing, and wrap the over-long run([...]) line in scripts/test-shards.ts.

trevatoowner commented 28d ago

The full gate on land was red and named no test; the pull is open again.