Skip to content

test_runner: drain event loop before force-exit process.exit() - #64857

Open
NeverBetterEnough wants to merge 1 commit into
nodejs:mainfrom
NeverBetterEnough:fix/test-force-exit-concurrency
Open

NeverBetterEnough wants to merge 1 commit into
nodejs:mainfrom
NeverBetterEnough:fix/test-force-exit-concurrency

Conversation

@NeverBetterEnough

Copy link
Copy Markdown

When --test-force-exit is used with --test-concurrency > 1, the parent process can silently lose test verdicts from child processes. This happens because process.exit() fires immediately after reporter destinations close, before the event loop has a chance to drain pending write operations on child-process report streams.

The previous fix (#55099) ensured reporter destination files are flushed before exit, but did not extend the flush guarantee to the child-to-parent report stream layer. With process isolation and concurrency, V8-serialized test-event frames from children can be truncated or dropped before the parent's FileTest parser consumes them.

This change adds a setImmediate tick before process.exit() to allow the event loop one full iteration to drain any remaining write operations on reporter destinations (including child-process stdout pipes).

Changes

  • lib/internal/test_runner/test.js: Added await new Promise((resolve) => setImmediate(resolve)) before process.exit() in the force-exit handler
  • test/parallel/test-runner-force-exit-concurrency.mjs: New regression test (8 files × 10 tests, run 5 times to catch the race)

Reproduction

The issue requires --test-force-exit + --test-concurrency > 1 (process isolation). With many fast test files, the parent silently loses a fraction of test verdicts — both built-in and custom reporters receive fewer events than tests that actually ran, yet the run still exits 0.

Impact

Silent partial results defeat the purpose of a test run. On a production suite (Node v24.18.0, 1351 tests), the parent reported as few as 1168. CI stays green while a meaningful fraction of the suite is unaccounted for.

Fixes: #64833
Refs: #54327, #55099

When --test-force-exit is used with --test-concurrency > 1, the
parent process can silently lose test verdicts from child processes.
This happens because process.exit() fires immediately after reporter
destinations close, before the event loop has a chance to drain
pending write operations on child-process report streams.

The previous fix (nodejs#55099) ensured reporter destination files are
flushed before exit, but did not extend the flush guarantee to
the child-to-parent report stream layer. With process isolation
and concurrency, V8-serialized test-event frames from children
can be truncated or dropped.

Add a setImmediate tick before process.exit() to allow the event
loop to drain any remaining write operations on reporter
destinations (including child-process stdout pipes).

Fixes: nodejs#64833
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Jul 31, 2026
@avivkeller

Copy link
Copy Markdown
Member

@NeverBetterEnough Please sign your commit by adding a Signed-off-by: trailer

@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.15%. Comparing base (c8e2a82) to head (db862f5).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #64857      +/-   ##
==========================================
- Coverage   90.16%   90.15%   -0.01%     
==========================================
  Files         746      746              
  Lines      242760   242766       +6     
  Branches    45765    45757       -8     
==========================================
- Hits       218875   218863      -12     
- Misses      15375    15402      +27     
+ Partials     8510     8501       -9     
Files with missing lines Coverage Δ
lib/internal/test_runner/test.js 97.93% <100.00%> (+<0.01%) ⬆️

... and 34 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@MoLow

MoLow commented Aug 2, 2026

Copy link
Copy Markdown
Member

Please fix related failures

sieu-n added a commit to sieu-n/sieun-pi that referenced this pull request Oct 25, 2026
avifenesh added a commit to avifenesh/codex-desktop-linux that referenced this pull request Nov 6, 2026
--test-force-exit can drop finished tests from the report and still exit 0
(nodejs/node#64833, open; its fix PRs nodejs/node#64857 and #64875 are
unmerged). On Node 26.12 and 24.21, runs of the 2088-test suite with the flag
reported 2046 to 2088 tests, 6 of 8 runs short; without it, 8 of 8 reported
2088 in the same time. Every test file now exits on its own (91 files, each
run alone without the flag), and the 300 s watchdog still bounds a leaked
handle.
avifenesh added a commit to avifenesh/codex-desktop-linux that referenced this pull request Nov 6, 2026
--test-force-exit can drop finished tests from the report and still exit 0
(nodejs/node#64833, open; its fix PRs nodejs/node#64857 and #64875 are
unmerged). With the flag, 8 of 11 local full-suite runs on Node 26.12 and
24.21 reported up to 47 fewer tests than ran; without it, 11 of 11 reported
every test in the same time. Each of the 94 test files exits on its own when
run alone without the flag, and the timeout watchdog still bounds a leaked
handle.
avifenesh added a commit to avifenesh/codex-desktop-linux that referenced this pull request Nov 6, 2026
--test-force-exit can drop finished tests from the report and still exit 0
(nodejs/node#64833, open; its fix PRs nodejs/node#64857 and #64875 are
unmerged). With the flag, 8 of 11 local full-suite runs on Node 26.12 and
24.21 reported up to 47 fewer tests than ran; without it, 11 of 11 reported
every test in the same time. Each of the 96 test files exits on its own when
run alone without the flag, and the timeout watchdog still bounds a leaked
handle.
ilysenko pushed a commit to ilysenko/codex-desktop-linux that referenced this pull request Nov 7, 2026
* test(computer-use): accept a readback timeout in the normalization mismatch test

Port of standalone computer-use-linux #431 (968aee5). The last poll of the
editable text readback can start just before the one-second deadline and
still be reading when it passes, which reports ReadbackTimedOut instead of
MismatchAfterDeadline. The two other mismatch tests already accept both
outcomes, since both leave the write unverified and neither allows a
replay. The embedded src/atspi_tree.rs now equals standalone's at 968aee5.
The crate stays 0.7.47.

* ci: run the Node tests through the TMPDIR guard

CI's Node step ran its own list of test files and never used the guard in
scripts/ci/run-node-checks.sh, and the guard's glob missed three test.js
files CI ran: the upstream package watchdog test and the quit-confirmation
and shell-env core patch tests.

The Node step now runs run-node-checks.sh all: a syntax check of every
tracked .js file, then every *.test.js and test.js in a fresh TMPDIR, failing
when a run leaves anything there. The glob takes every test.js, so CI runs
exactly what the guard checks, including the new socket helper's unit test
and suites the old list skipped. github-actions-workflows.test.js checks
that the Node step is the guarded runner and that the runner's glob finds
every tracked test file.

* fix(appshots): check XInput 2 before the bare modifier stream starts

The monitor started xinput test-xi2 and, 0.1 s later, printed
permission-denied if the stream had already exited, else ready. test-xi2
fails at once when the X server is unreachable or has no XInput 2, but
under load that exit can come after 0.1 s, and the monitor then printed
ready and stopped.

The monitor now runs xinput --version first and waits for it. That command
opens the display and reads the server's XInput version, the same
preconditions, so the answer no longer depends on timing. A missing or
pre-2 server version gives permission-denied. The 0.1 s check stays for a
stream that dies after a good preflight.

Tests: a slow xinput that cannot reach the server and a server with
XInput 1.5 must give permission-denied; the old monitor printed ready for
both. A stream that dies after the preflight still leaves no stdbuf
directory, whichever way the short check goes.

* fix(global-dictation): stop the watcher's subshells before killing their children

The release watcher's cleanup ran pkill -TERM -P on each helper subshell and
then killed the subshell. A subshell that forked between the two, such as
the watchdog before its sleep or the parent check between two sleep 1, left
that child running after the watcher, up to the watchdog's 1800 s. A
reviewer saw one such sleep once under heavy load.

Cleanup now sends SIGSTOP to every subshell first. A stopped process cannot
fork, so pkill -P sees every child it will ever have; the SIGTERM then waits
until SIGCONT. A fork already in progress when the stop arrives completes
before the stop takes effect, so its child is listed too.

The race did not reproduce with the real watcher in 1200 parallel runs. A
model that copies the watcher's cleanup() verbatim, with one subshell that
forks 0.3 s late and a pkill that takes 0.5 s, leaves the late child running
in 10 of 10 runs with the old cleanup and in 0 of 10 with this one.

* test: keep the last test fixtures in TMPDIR and inside their try

- Six shared-app-server-socket tests bound their socket before the try that
  removes the fixture, so a failed bind left it in TMPDIR. With listenUnix
  made to fail, the old file leaves 6 fixtures and this one none.
- The GNOME dictation tests kept their scratch under
  ~/.cache/codex-desktop-dev, outside TMPDIR, so the guard could not see a
  leftover. They now use os.tmpdir(). The private-bus test's scratch holds
  a dbus-daemon socket, so it is placed with makeSocketTempDir under
  dbus-daemon's 99-byte limit; it passes with a 48 and a 92-byte TMPDIR.

* docs(changelog): describe the final test and monitor fixes

* fix: address review of the final TMPDIR and monitor fixes

- Release watcher: cleanup ignores INT, TERM and HUP while it runs, and
  stops, signals and continues each helper subshell in one pass. A second
  signal during cleanup used to run the exit trap at once; on main that left
  the helpers running, and with the first fix it left them stopped. With a
  slowed pkill and a second SIGTERM 0.4 s after the first, main leaves 3
  processes, the first fix 4 (three of them stopped), and this none.
- A regression test for the cleanup race uses the real watcher: its parent
  check's sleep ignores SIGTERM and lasts 0.4 s, and pkill takes 1 s, so the
  loop forks again after pkill -P has listed the children. On main the late
  sleep outlives the watcher in 3 of 3 runs; here it never does.
- shared-app-server-socket: a seventh test that bound before its try now
  binds inside it.
- AppShots monitor: the xinput --version preflight runs under timeout 5
  when timeout exists, so a server that never answers ends in
  permission-denied instead of silence. The startup test is renamed for
  what it tests, and the post-preflight test checks the exit status.
- The workflow test finds the runner's test glob by its own line.
- The changelog says run-node-checks.sh all also syntax-checks every .js
  file, and covers the second-signal fix.

* fix: address the final review of the monitors and fixtures

- Release watcher: cleanup ends each stopped subshell with SIGKILL instead
  of SIGTERM and SIGCONT. bash blocks SIGTERM from just before a fork until
  it returns, so a subshell stopped in that window resumed on SIGCONT,
  finished the fork and only then died, leaving the new child. With a shim
  that delays fork() in the helpers, main and the SIGTERM/SIGCONT cleanup
  left the watchdog's sleep and a sleep 1 in 3 of 3 runs; this leaves
  nothing. The subshells hold no state to clean up.
- The race test now reports a parent-check sleep that outlived its helper
  instead of looking for it after the watcher exits, so it fails on main in
  3 of 3 runs. It and a new test for a second SIGTERM during cleanup find a
  run's processes by the run's TMPDIR in their environment, and kill only
  those. The second-signal test fails on main in 3 of 3 runs.
- AppShots monitor: the xinput --version preflight has no time limit again.
  An Xwayland started on demand can take longer than any fixed limit, and
  the stream itself waited for the server before.
- package-common's permission tests keep their fixture under TMPDIR, where
  the guard sees it, instead of ~/.cache/codex-desktop-dev.
- shared-app-server-socket: the stale-inode test closes its server in its
  finally block too.
- The GNOME private-bus test comments no longer say the bus is on disk.
- AGENTS.md and CONTRIBUTING.md name `bash scripts/ci/run-node-checks.sh all`
  among the validation commands. The changelog links
  agent-sh/computer-use-linux#431 and describes the SIGKILL cleanup.

* fix(appshots): end a waiting preflight with the monitor

The xinput --version preflight ran before the monitor installed its traps
and in a command substitution, so while an X server did not answer,
SIGTERM killed the monitor and left xinput running, and the monitor never
noticed that its parent had exited. The traps and cleanup now come first,
and the preflight runs in a process substitution that the monitor reads
with a one-second timeout, checking its parent each time. Cleanup kills
the preflight. It still has no time limit of its own. A coprocess would
not do: bash drops a coprocess's descriptor once it exits, and a fast
answer can exit before the read.

Two tests cover a preflight that never answers: one sends SIGTERM, one
lets the parent exit. Both fail on the previous commit and pass here.

The release watcher tests also find processes whose TMPDIR is a directory
under the run's, such as the stream's stdbuf directory, so a failing run
cannot leave those behind.

* fix(appshots): treat a stream gone before its first read as dead

Bash unsets a coprocess's pid and descriptor variables once it exits. A
stream that fails at once can exit before the monitor reads them, and set
-u then stopped the monitor with an unbound-variable error and no output.
The monitor now reads both with empty defaults and reports
permission-denied when either is gone. With the monitor held 100 ms after
each fork, the previous commit failed this way in 3 of 3 runs, as main
does; this commit reports permission-denied in 3 of 3.

The watcher test stubs exec their final sleep 10, so the watcher's kill of
a stub ends that sleep too. The changelog says SIGTERM instead of "a stop
signal".

* fix(ci): run Node tests without --test-force-exit

--test-force-exit can drop finished tests from the report and still exit 0
(nodejs/node#64833, open; its fix PRs nodejs/node#64857 and #64875 are
unmerged). With the flag, 8 of 11 local full-suite runs on Node 26.12 and
24.21 reported up to 47 fewer tests than ran; without it, 11 of 11 reported
every test in the same time. Each of the 96 test files exits on its own when
run alone without the flag, and the timeout watchdog still bounds a leaked
handle.

* test: end the processes the bridge and Dock lock tests start

The host-service tests removed their fixture while the detached bridge was
still shutting down: one bridge outlived the file in 3 of 3 runs. They now
take the socket path before the launch, await the owner's unregister, and
wait up to 3 s for the bridge named in the lock to exit (checking its start
time), then kill it; 0 of 3 after.

The Dock icon lock test killed only flock, whose child keeps the lock and
its sleep, so the file waited about 2.1 s for it after the test ended. The
holder now reads its stdin and keeps the lock until the test closes it or
dies: the wait is 0.15 s, node's own exit, and a SIGKILLed run leaves no
holder 1 s later (3 of 3).

The CHANGELOG folds the --test-force-exit removal into the Node test entry,
since no release shipped the flag, and states when the watchdog applies.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test_runner: --test-force-exit with concurrency silently loses test verdicts (parent reports fewer tests than ran, exit 0)

4 participants