test: make test_gap_cron_cronjob wait on a barrier, not a deadline - #10722
proggeramlug wants to merge 2 commits into
Conversation
…10581) The fixture raced a `* * * * * *` CronJob against a fixed `Date.now() + 10_000`. The deadline is a timeout, not a barrier: when it expired first the loop exited and the fixture printed `false` on two lines expected to read `true`, dropping `tick 1`/`tick 2` as well — a four-line divergence the harness reports as a `parity_fail`, i.e. as a miscompile. It is inside pr-gate's gap shards and absent from gap_snapshot.json, and it already held #10530 out of a train. The wait now has no deadline, so the printed text is a function of CronJob's behaviour alone: the ticks arrive, or PERRY_RUN_TIMEOUT kills the run and the harness classifies that as a crash/timeout rather than a parity mismatch. No fallback bound — any bound that prints or throws on expiry is the same defect with a longer fuse, and the old 10s was unreachable anyway because PERRY_RUN_TIMEOUT is also 10s. The never-started job now prints `neverTicks === 0` from a real counter instead of a hardcoded `true`, checked after the barrier. Output bytes unchanged. Verified with an identical 11s event-loop stall injected into both the old and new fixtures: under Node 26.5.1 and Perry v0.5.1598 the old one diverges and the new one is byte-identical to the unstalled oracle. Harness run exits 0 with journal status `pass`; 8 Node runs gave one distinct output in 1.86-2.04s.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe CronJob gap fixture no longer uses a 10-second wall-clock deadline. It waits until both active jobs tick twice, tracks unexpected ticks from the never-started job, and documents the change. ChangesCronJob barrier fixture
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
`e2e-scoped` has been red on every PR since merge train 218 (v0.5.1596),
failing at "Compute e2e suite scope" in ~16s:
ci_e2e_scope: these crates/perry-codegen/tests/*.rs suites are in neither
SOURCE_SUITE_MAP nor SUITE_EXCLUSIONS: error_subclass_field_init,
typed_collection_receiver_guard
Both suites arrived in 6925754 (#10443/#10446, train 218) and nobody
classified them, which is exactly the condition #7708 added this assertion
for. The failure is content-independent, so it reddens PRs that cannot
possibly have caused it -- #10721 (a Python script) and #10722 (a .ts
fixture) both carry it.
Mapped rather than excluded: both are cheap in-process suites of the shape
SOURCE_SUITE_MAP exists for, and both passed when train 218 ran them as
diff-named suites (2 and 3 tests, 0.01s each). Excluding them would have
hidden working coverage; SUITE_EXCLUSIONS is for a named failing test with
an issue number, which neither has.
Verified discriminating, not merely present: with either entry deleted
`--self-test` exits 1 naming the suite, and exits 0 with both.
`e2e-scoped` has been red on every PR since merge train 218 (v0.5.1596),
failing at "Compute e2e suite scope" in ~16s:
ci_e2e_scope: these crates/perry-codegen/tests/*.rs suites are in neither
SOURCE_SUITE_MAP nor SUITE_EXCLUSIONS: error_subclass_field_init,
typed_collection_receiver_guard
Both suites arrived in 6925754 (#10443/#10446, train 218) and nobody
classified them, which is exactly the condition #7708 added this assertion
for. The failure is content-independent, so it reddens PRs that cannot
possibly have caused it -- #10721 (a Python script) and #10722 (a .ts
fixture) both carry it.
Mapped rather than excluded: both are cheap in-process suites of the shape
SOURCE_SUITE_MAP exists for, and both passed when train 218 ran them as
diff-named suites (2 and 3 tests, 0.01s each). Excluding them would have
hidden working coverage; SUITE_EXCLUSIONS is for a named failing test with
an issue number, which neither has.
Verified discriminating, not merely present: with either entry deleted
`--self-test` exits 1 naming the suite, and exits 0 with both.
|
Landed in merge train 224 (#10742), released as v0.5.1603 — main is now Closing rather than merging is how trains work here: the PRs were cherry-picked onto one tree, validated together, and landed under the train's own commit, so GitHub cannot mark this one merged even though your change is on main. One deliberate divergence, for #10719 only: the train carries Validation: all nine cheap gates, |
test-files/test_gap_cron_cronjob.tsstarted a* * * * * *CronJob and then waitedwith
That deadline is a timeout, not a barrier: when it expired first the loop exited and
the fixture printed
falseon two lines expected to readtrue, and dropped thetick 1/tick 2lines entirely — a four-line output divergence the harness classifiesas a
parity_fail, i.e. reports as a compiler regression. The header comment's claim thatthe output was "deterministic despite the timing" held only while two ticks of a
one-per-second schedule landed inside ten seconds.
It carries the
test_gap_prefix, so it is inside the gap shardspr-gateactually runs,and it is absent from
test-parity/gap_snapshot.json(= expected to pass). It alreadycost real work: it failed in a merge-queue validation and #10530 was held out of a train
on the strength of it, after which a
--trace llvmA/B showed byte-identical IR.The fix
The wait is now a barrier with no deadline. The printed text becomes a function of
CronJob's behaviour alone: either the ticks arrive and the fixture prints its one expected
output, or nothing dispatches and the harness's own
PERRY_RUN_TIMEOUTkills the run —which it classifies as a CRASH/timeout, distinctly from a parity mismatch, so a contended
runner can no longer make this look like a miscompile.
There is deliberately no fallback bound. Any bound that prints, throws or exits
differently on expiry reintroduces exactly this defect at a different threshold: a
false-printing 30-second deadline is the identical bug with a longer fuse.Worth noting for anyone reaching for "just make the deadline bigger":
PERRY_RUN_TIMEOUTdefaults to 10s (
run_parity_tests.sh:58) — the same 10 seconds as the fixture's olddeadline. The old bound was therefore unreachable in practice; it could only ever have
fired in a photo finish with the harness's own kill.
Nothing is weakened. The assertion moved from a printed comparison into the loop's exit
condition, which the program cannot pass without satisfying. The two-arg non-auto-starting
form, the four-arg
start=trueform, andstart()/stop()dispatch are all stillexercised, and
tick 1/tick 2remain in the diff as positive evidence that the manualjob fired. The never-started job's line got stronger: it printed a hardcoded
true, andnow prints
neverTicks === 0from a real counter, checked after the barrier — i.e. afterat least two cron seconds have demonstrably elapsed with that job unstarted. Output bytes
are unchanged.
Test that fails without the fix
An identical 11-second synchronous event-loop stall injected at the same point into the
old and new fixtures — a deterministic stand-in for the loaded runner:
Same result under Perry. Cross-engine byte comparison:
Note that under the stall both engines produce the same wrong output, so an equally
starved pair would still match. The
parity_failarises because Node and Perry run asseparate processes at different moments, so the load hits one and not the other — which is
exactly the observed failure.
Verification
cron@4.4.0installed vianpm ci(the harness needsnode_modules; a fresh treesilently drops package-backed fixtures otherwise). Eight consecutive Node runs gave one
distinct output hash in 1.86–2.04 s — roughly a fifth of the 10 s run budget.
./scripts/check_file_size.shandcargo fmt --all -- --checkboth exit 0.Provenance caveat. The worktree had 11 GiB free, below this repo's 25 GiB floor for a
parity build (ENOSPC here produces fake failures naming innocent files), so I did not
build the compiler from this base. I used a prebuilt
perry 0.5.1598— the version4715bc2fa1declares — copied out of a shared target pool into an isolated scratch dirwith its coherent
libperry_{runtime,stdlib}.aand ext archives, so no other session'sdirectory was written to. This change touches no Rust, so compiler identity is not
load-bearing for the conclusion, but the binary is not one built from this SHA.
Follow-up
Two sibling fixtures have the same shape and are not touched here — filed as #10720:
test_gap_9592_child_timeout_threads(a 1 s deadline whose expiry printstimeout threads released: false) andtest_gap_9493_child_stdin_backpressure(a watchdogthat
resolve(false)s). Both are in gate scope.Closes #10581
Summary by CodeRabbit
Bug Fixes
Tests