Repository navigation
ci: the clock-shift job runs every test of a file, or fails (#848) - #1094
Conversation
|
Independent adversarial review (R94b), posted by the coordinator. VERDICT: PASS @c542ea595a823dcc66e877e6aaddd4d1a814c8db (adversarial) Reviewer R94b, own detached worktree at the head, Node 22.23.3, TMPDIR under /var/tmp. CI was still pending when I finished, including "unit tests on a shifted clock" and the test shards. Read CI before merging. Findings
The
Surviving mutants (follow-ups, not FAILs)measured: each mutant left the #848 and #774 tests at 2 pass, 0 fail.
What I ranI built with |
|
Independent final check (R94a), posted by the coordinator. VERDICT: FAIL @c542ea595a823dcc66e877e6aaddd4d1a814c8db (final check) Findings
What I ran
Follow-ups
|
--test-force-exit ended a file's process once the tests it knew of were done, so a file that registers tests after a top-level await (test/slot-dir-writes.test.ts) lost them silently: 18 fewer tests at +400d, still 0 fail. Both test steps drop the flag. --test-timeout already bounds a file whose process doesn't exit (a handle left open): it is cancelled and named, and the step fails, where before it passed. Test (fails without): test/clock-shift.test.ts runs both steps' command lines from the workflow on fixtures. A test registered after an await must run (before: never ran, exit 0), and a file left with an open handle must fail, named, at the step's timeout (before: exit 0). Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
) Round 1 dropped --test-force-exit and relied on --test-timeout to end a file whose process doesn't exit. That holds on Node 22 only: on Node 24, which the job uses, a file with a handle left open is never ended, so the step would wait for its 75-minute limit with no summary (#774 again; the new open-handle test failed on the Node 24 shard). scripts/ci/clock-shift-run.mjs runs every test file in a `node --test` of its own, without force-exit, as many at once as node --test does. A file passes only when it exits 0 with a summary; one still running after --file-timeout (25 minutes in the job) is killed with its process group and named, and the run fails. The run ends with totals and the failed files. The tests read both steps from the workflow and require their exact command lines (no `|| true`, no continue-on-error on any step or the job), then run each step on fixtures: a test registered after an await runs; a file with an open handle, a failing test and a hanging test each fail the step, named; no test file fails it. Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
c542ea5 to
dfe21e9
Compare
…t.ts requires (#848) test/isolate.test.ts requires a JavaScript runner to declare PRELOAD as ['--import', './dist/test/isolate.js'] and pass ...PRELOAD right before '--test'. Each file's run now does, from the repo root (its working directory, as in npm test), with the file given by its absolute path. The check failed on both Node shards at dfe21e9. Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
|
Independent final check (R94c), posted by the coordinator. VERDICT: FAIL @44b2b8b61fd43edc10600dc2628be5c9135e6fef (final check) Reviewer R94c: own detached worktree, Node 22.23.3 and a scratch Node v24.22.1 (checksum checked, kept under TMPDIR). Head re-checked at the end: unchanged. Why FAIL: one false claim in the PR body (bar a). The code passes.The Evidence section says: "Round 1's run, still with force-exit, reported 5795 tests per shift." That is false (measured). Run 40237889161 is a Findings (all measured unless labelled)
Weakening flags
Follow-ups
|
|
Independent adversarial review (R94d), posted by the coordinator. VERDICT: PASS @44b2b8b61fd43edc10600dc2628be5c9135e6fef (adversarial) Reviewer R94d. I used my own detached worktree at the head ( Findings
Surviving mutants (follow-ups)measured, each one left the suite green:
Suggested fixes: assert no Weakening flagsOutput of
Ran
|
|
Independent final check (R94e), posted by the coordinator. VERDICT: PASS @44b2b8b61fd43edc10600dc2628be5c9135e6fef (final check) Reviewer R94e, re-checking R94c after a body-only edit. The head is unchanged at 44b2b8b (measured: Body re-check (measured against R94c's findings)
Weakening flags (carried over from R94c)
Nits (no action needed for this PR)
|
|
Weakening scan decision for head The owner's instruction (verbatim): "Stp asking me quesgtions and just take recommended option each time". Both independent reviews of this exact head passed:
Both classified the scan's flags the same way. The weakening scan reports 2 reasons (measured, R94c and R94d):
The 7 failures in the optional shifted-clock job predate this PR and are tracked in #1107. Decision: proceed (recommended option). This covers only this head; any new push needs a fresh scan and review. |
What changed (round 2, head 44b2b8b, on main e80bac6)
scripts/ci/clock-shift-run.mjs. It gives each test file under the given directories anode --testof its own, with no--test-force-exit. It runs as many files at once asnode --testdoes (availableParallelism() - 1).--file-timeoutis killed together with its process group, named, and fails the run....PRELOAD, '--test', from the repo root, which is the formtest/isolate.test.tsrequires. That check failed on both Node shards at dfe21e9 and was fixed in 44b2b8b..github/workflows/clock-shift.yml. Both steps (+45d and +400d) run the script:node scripts/ci/clock-shift-run.mjs --test-timeout=600000 --file-timeout=1500000 dist/test. That is 10 minutes per test and 25 minutes per file. The clock-shiftNODE_OPTIONSpreload, the step limits (75 min) and the job limit (170 min) are unchanged.test/clock-shift.test.ts. It reads both steps from the workflow and checks:|| trueor wrapper),shell: bash, and nocontinue-on-erroron any step or on the job;changelog.d/848-clock-shift-all-tests.md: reworded (R94a, R94b).Closes #848
Why this design (round 1 failed, R94a)
--test-force-exitdropped tests registered after a top-level await, and the run still said 0 fail: 18 ofslot-dir-writes' 55 tests at +400d, per the issue.--test-timeoutto end a file whose process doesn't exit. That holds on Node 22 only. On Node 24 (the job's version), a file with an open handle is never ended, so the step would wait for its 75-minute limit with no summary (Tests: fix the budget-health second-boundary flake; weekly optional shifted-clock run; lint on clock-reading helpers (#687) [PROTECTED: owner merges] #774). Measured below, and by R94a on the Node 24 shard.node --testper file bounds each file on any Node version, from outside the process. It doesn't need Node's own file timeout. (measured on 22 and 24, below)Evidence
Local runs used Node 22.23.3 and a scratch Node v24.22.1. The Node 24 tarball was checked against nodejs.org's SHASUMS256 and lives under this builder's TMPDIR; nothing was installed.
The old command lines, on fixtures at +400d (measured):
--test-force-exit) on a file with a test after an await:# tests 1, exit 0, on both 22 and 24. The late test never runs.--test-timeout=3000) on a file with an opensetInterval, capped withtimeout 30: exit 1 on Node 22, and exit 124 on Node 24, meaning it hung until the cap.# tests 2 # pass 2, exit 0; open handle (--file-timeout=5000) exit 1, named in "# failed files". Both results on 22 and 24.Fail-before / pass-after (measured):
node --import ./dist/test/isolate.js --test --test-timeout=180000 dist/test/clock-shift.test.js.--test-force-exitadded to the per-file run: the Shifted-clock job: --test-force-exit drops tests silently (18 from slot-dir-writes at +400d, still 0 fail); make the job fail closed #848 test fails on 22 and 24 ("the test registered after the await never ran").Other checks (measured):
npm run typecheck: exit 0.npm run check:denylist: "1669 files clean".scripts/check-dco.mjs origin/main HEAD: "all commits signed off".clock-shift,no-wall-clock-durations,changelogandci-affectedtests: 49 of 49 pass on 22 and on 24.npm testunder the shared lock, at 44b2b8b: exit 1. The run had 5895 tests: 5769 pass, 17 fail, 69 skipped. The 17 are the same host-only failures as round 1 (merge-vacuity-*,machine-sudoers-*,coordinator), minusdesktop-removed. None is in a file this PR touches. The vacuity backstop names the symlinkednode_modulesas the cause (measured). That the others also come from this host is inferred.CI at 44b2b8b (measured,
gh pr checks 1094): 27 checks pass, 1 fails.test (ubuntu-latest, 22)andtest (ubuntu-latest, 24)roll-ups.unit tests on a shifted clock(job 121334474723, Node 24), which fails on the 7 files from before this PR (next section). From its log:# clock-shift run: 656 files # tests 5822 # pass 5710 # fail 7 # cancelled 0, followed by the 7 failed files by name.slot-dir-writesreported# tests 55at both +45d and +400d. (Round 1's run did not use--test-force-exiteither, so its 5795-test count is not a force-exit comparison; corrected by the coordinator per R94c.)--file-timeout.The clock-shift job's other failures predate this PR
R94a measured run 40237889161 of this optional job at round 1's head: 8 failures at each shift, one of them the round-1 open-handle test. The other 7 predate this PR:
script-host-toolstest: add-project.test.ts may reach the real sudo since #904 (stand in as_user_argv; host stand-ins first on PATH) #991 round 3watchdogreal-systemctl testengine-updatefrozen-guardmachine-helperseparate-usersR94a ran 3 of them (
script-host-tools,watchdogandfrozen-guard) locally at +45d. They fail the same way with and without--test-force-exit(measured by R94a). That the other 4 also predate this PR is inferred: this workflow's last green run was Oct 19, before #991. This PR's own run at 44b2b8b failed on exactly these 7 files at both shifts, and on no other file:engine-update,frozen-guard,machine-helper,isolate(the #312 regression),script-host-tools,separate-usersandwatchdog. So this job will still be red on main after merge, until those 7 are fixed. None of them is an open-handle timeout. This PR doesn't fix them; they are tracked in #1107.Critical set
Yes. The PR changes a CI workflow and adds a CI script (
scripts/ci/), so it needs two independent reviews. No engine-guards test changes. No required check is added or renamed: the job stays optional, and its name, triggers and limits are unchanged.Not done / follow-ups
process.exit(0)partway through still passes silently, as on main. The new script does fail a run with no test file at all. Checking each file's registered count against what ran would close theprocess.exitcase, but it needs the file's own plan, which the runner can't see from outside. Left as a follow-up.test/slot-dir-writes.test.tsis unchanged (engine-guards). It needs no change: 55 of 55 tests run without force-exit (measured in round 1).src/merge-policy-vacuity.tsaccepts--test-force-exitin a project'srunner.one. The same drop may affect vacuity coverage runs (guess).