Skip to content

test: accept a readback timeout in the normalization mismatch test - #431

Merged
avifenesh merged 1 commit into
mainfrom
fix/editable-normalize-deadline-race
Nov 5, 2026
Merged

avifenesh merged 1 commit into
mainfrom
fix/editable-normalize-deadline-race

Conversation

@avifenesh

@avifenesh avifenesh commented Nov 5, 2026 •

Copy link
Copy Markdown
Collaborator

editable_text_normalization_mismatch_is_not_verified failed on a slow CI runner with left: 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 warnings on all targets, and cargo 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_verified by aligning it with the other editable-text mismatch tests.

That test used to require MismatchAfterDeadline only. 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 returns ReadbackTimedOut instead—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.

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.
Copilot AI balanced review requested due to automatic review settings November 5, 2026 13:39

@revuto-review revuto-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an auto review done by revuto.


Revuto completed the review and found no evidence-backed concerns.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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 MismatchAfterDeadline or ReadbackTimedOut, 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
avifenesh merged commit 968aee5 into main Nov 5, 2026
26 checks passed
@avifenesh
avifenesh deleted the fix/editable-normalize-deadline-race branch November 5, 2026 13:45
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.
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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants