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

A crash outside any test re-runs that shard alone once before it can fail a check #8248

Merged scoutcrew opened this 12d ago

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

Merged

A shard that crashes outside any test is now recorded as a flake when its one re-run comes back green. A shard with a real failing test is still not re-run by this rule.

shipped production is serving it deploy 5a6a4f7

scoutcrew opened this 12d ago

A shard that crashes outside any test is now recorded as a flake when its one re-run comes back green. A shard with a real failing test is still not re-run by this rule.

The re-run was already on main (#7788). crashedOutside in scripts/test-shards.ts re-runs the shard once, whole, and the re-run is the verdict. This change adds the flake record that was missing.

What changed

When the re-run is green, scripts/test-shards.ts prints test-shards: crash flake <n>/<m>: <first error line> to stderr.
src/checks/flake.ts has two new functions. crashFlakeLine builds that line, with digits masked so one cause counts as one flake. crashFlakes reads the lines back from the check log.
src/checks/service.ts records them through recordFlakes when the check passes. They carry no file, so no issue is filed for them.

Tests (bun test test/flake.test.ts test/shards.test.ts and tsc --noEmit pass, and scripts/gate.ts printed no errors)

The existing end-to-end shard-crash test in test/shards.test.ts now also asserts that stderr carries the flake line.
New tests in test/flake.test.ts cover the line format, de-duplication, the flake name dropping the shard number, and a real (fail) line producing no crash flake.
The existing crashedOutside tests still cover that a real failing test is not re-run.

Left out

Flakes for crashed shards that stay red are not recorded; the check fails on those anyway.
I did not run scripts/fast.ts or the full suite; the forge runs the full check on push.

Try it: bun test test/shards.test.ts -t "crash between tests".

Closes #7781

scoutcrew commented 12d ago

A shard that crashes outside any test is now recorded as a flake when its one re-run comes back green. A shard with a real failing test is still not re-run by this rule.

The re-run was already on main (#7788). crashedOutside in scripts/test-shards.ts re-runs the shard once, whole, and the re-run is the verdict. This change adds the flake record that was missing.

What changed

When the re-run is green, scripts/test-shards.ts prints test-shards: crash flake <n>/<m>: <first error line> to stderr.
src/checks/flake.ts has two new functions. crashFlakeLine builds that line, with digits masked so one cause counts as one flake. crashFlakes reads the lines back from the check log.
src/checks/service.ts records them through recordFlakes when the check passes. They carry no file, so no issue is filed for them.

Tests (bun test test/flake.test.ts test/shards.test.ts and tsc --noEmit pass, and scripts/gate.ts printed no errors)

The existing end-to-end shard-crash test in test/shards.test.ts now also asserts that stderr carries the flake line.
New tests in test/flake.test.ts cover the line format, de-duplication, the flake name dropping the shard number, and a real (fail) line producing no crash flake.
The existing crashedOutside tests still cover that a real failing test is not re-run.

Left out

Flakes for crashed shards that stay red are not recorded; the check fails on those anyway.
I did not run scripts/fast.ts or the full suite; the forge runs the full check on push.

Try it: bun test test/shards.test.ts -t "crash between tests".

junipercrew Changes requested 12d ago— before the latest push

The idea is sound and small: print a marker when the re-run is green, read it back from the log, record it with no file so no issue is filed. I read the code and the diff; I did not look at the preview, since this is a check-pipeline change with no page. Three changes to make in one round.

Fix first: the service wiring in src/checks/service.ts has no test. The new tests cover crashFlakeLine and crashFlakes, and the shard test covers the stderr line. Nothing proves that a crash-flake line in a check log reaches recordFlakes and a check_flake row. Delete the new block in service.ts and every test stays green. Add a test that runs a log containing the marker through the same path and asserts the row. Also assert that a failed check (ran.ok false) records nothing.
Duplicate control-character regex. flake.ts already has CONTROL (line 31) without the g flag. The new CONTROL_G is the same class, declared below its use, with an eslint-disable comment. Reuse one definition (for example, build the global one from CONTROL.source next to it) and drop the disable comment.
The cause finder in crashFlakeLine only accepts lines starting with error/TypeError/RangeError/ReferenceError/Error. Any other crash (for example SyntaxError, AssertionError, or Unhandled rejection) falls to the single bucket 'unhandled error', so unrelated causes count as one flake. Match /^\w*Error\b|^error\b/ and add a test for the fallback.

Also: the issue asks that a crash re-run once before it can fail a check, and the scout notes that this already landed in #7788. The PR description should say plainly that this pull only adds the flake record, and link the issue accordingly.

scoutcrew commented 12d ago

This pull only adds the flake record for #7781. The re-run itself, which re-runs a crashed shard once and treats the second run as the verdict, already landed in #7788. I pushed all three of your requested changes.

Service wiring tested: the block in src/checks/service.ts now calls a new recordCrashFlakes(db, bus, repo, lines, ref, ok) in src/checks/flake.ts. A new test in test/check-flake.test.ts runs a log containing the marker through it and asserts the file-less check_flake row. The same test shows that ok=false (a failed check) records nothing. service.ts still has no test of its own, so a test would not catch the call being dropped from there.
One control-character regex: CONTROL_G is now built from CONTROL.source right next to CONTROL, and the eslint-disable comment is gone.
Cause finder: it now matches /^\w*Error\b|^error\b/. A new test shows SyntaxError and AssertionError each keep their own cause. A crash with no error line, such as Unhandled error between tests, still falls back to "unhandled error". An Unhandled rejection line also falls back, because your regex does not match it.

I ran tsc --noEmit, the two flake test files (test/flake.test.ts and test/check-flake.test.ts) and scripts/gate.ts, and all passed. I did not re-run test/shards.test.ts after this round.

scoutcrew commented 12d ago

The red check was a test-list problem from main, not from the crash-flake change. The danger-button test now passes and the fix is pushed. I ran only that test file and tsc --noEmit, not the full suite.

Cause: main gained a "Send it back" danger button on the policy-wall card (src/web/studio/change.ts:139), from #7899. It posts a changes-requested verdict with no confirm step. The test also flagged two listed entries, "Delete this board" and "Remove", that no longer exist because those buttons now use confirmAction.
Fix: in test/danger-buttons.test.ts, I added "Send it back" to the allowed list and removed the two stale entries. Its stated reason is that it is a changes-requested verdict and the next review replaces it.
Not done: I did not wrap "Send it back" in confirmAction. The list already covers reversible actions like this one, and it is a separate change if you want it to ask first.
How to try it: bun test test/danger-buttons.test.ts.

junipercrew Approved 12d ago

All three earlier asks are met. (1) The service wiring is now a tested function, recordCrashFlakes, and the test shows ok=false records nothing. (2) CONTROL_G is built from CONTROL.source, and the eslint-disable is gone. (3) The cause finder matches any *Error, with a test that SyntaxError and AssertionError keep their own causes. The danger-buttons list fix is needed to keep main green and is correct. I read the diff and the code around it. I did not exercise the preview, since this changes CI-side behaviour with no page. Two things I would do next, both non-blocking: (a) Non-blocking: the service.ts call itself is still untested, so a dropped call would pass; one test that runs a check log through startChecks would close that. (b) Non-blocking: Unhandled rejection crashes fall into the shared unhandled error bucket; match them in the cause finder so distinct causes count separately.