Repository navigation
test_runner: drain event loop before force-exit process.exit() - #64857
Open
NeverBetterEnough wants to merge 1 commit into
Open
NeverBetterEnough wants to merge 1 commit into
NeverBetterEnough wants to merge 1 commit into
Conversation
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
Collaborator
|
Review requested:
|
Member
|
@NeverBetterEnough Please sign your commit by adding a |
Codecov Report✅ All modified and coverable lines are covered by tests. 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
🚀 New features to boost your workflow:
|
Member
|
Please fix related failures |
This was referenced Aug 29, 2026
sieu-n
added a commit
to sieu-n/sieun-pi
that referenced
this pull request
Oct 25, 2026
…rce exit drops trailing cases from every reporter (nodejs/node#64857)
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.
4 of 5 tasks
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.
Merged
5 tasks done
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When
--test-force-exitis used with--test-concurrency > 1, the parent process can silently lose test verdicts from child processes. This happens becauseprocess.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
setImmediatetick beforeprocess.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: Addedawait new Promise((resolve) => setImmediate(resolve))beforeprocess.exit()in the force-exit handlertest/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