Repository navigation
test: accept a readback timeout in the normalization mismatch test - #431
Merged
Merged
Conversation
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. A slow CI runner hit this. The two other mismatch tests already accept both outcomes, since both leave the write unverified and neither allows a replay.
There was a problem hiding this comment.
This is an auto review done by revuto.
Revuto completed the review and found no evidence-backed concerns.
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues block the test-only adjustment to accept both valid deadline outcomes.
No open issues 🎉
What changed in this PR
Reduces CI flakiness in the editable-text normalization mismatch test by accepting both valid deadline outcomes. Production behavior is unchanged.
Changes:
- Accepts either
MismatchAfterDeadlineorReadbackTimedOut, consistent with the other mismatch tests. - Documents that both outcomes leave the write unverified and do not permit replay.
| File | Description |
|---|---|
src/atspi_tree.rs |
Broadens the test assertion and explains the deadline race. |
CHANGELOG.md |
Adds an Unreleased note describing the test-only fix. |
Review effort: Balanced
avifenesh
added a commit
to avifenesh/codex-desktop-linux
that referenced
this pull request
Nov 5, 2026
- 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.
avifenesh
added a commit
to avifenesh/codex-desktop-linux
that referenced
this pull request
Nov 5, 2026
- 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.
This was referenced Nov 5, 2026
Merged
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.
editable_text_normalization_mismatch_is_not_verifiedfailed on a slow CI runner withleft: ReadbackTimedOut, right: MismatchAfterDeadline(the embedded copy's rust job in ilysenko/codex-desktop-linux#1670).The readback polls every 50 ms until a one-second deadline. When the last poll starts just before the deadline and its reads are still running when it passes, the result is
ReadbackTimedOut. The two other mismatch tests already accept both outcomes, with the comment that both leave the write unverified and neither allows a replay. This test now does the same. No product code changes.Validation:
cargo fmt --check, clippy-D warningson all targets, andcargo test --locked --no-fail-fast: 755 passed, 0 failed. The race did not reproduce locally in 144 stressed runs (96 at 24 in parallel, 48 at 12 in parallel limited to one CPU): the window is the part of the last 50 ms interval shorter than one poll's reads.Note
Low Risk
Test-only assertion and changelog update; editable-text verification behavior in the server is unchanged.
Overview
Fixes flaky CI in
editable_text_normalization_mismatch_is_not_verifiedby aligning it with the other editable-text mismatch tests.That test used to require
MismatchAfterDeadlineonly. When the last 50 ms readback poll starts just before the one-second verification deadline and D-Bus reads are still in flight when time expires, verification returnsReadbackTimedOutinstead—common on slow runners. The assertion now accepts either outcome, with a comment that both mean an unverified write and neither allows replay.No production code changes—only the test assertion and an [Unreleased] changelog note.
Reviewed by Cursor Bugbot for commit 968aee5. Configure here.