fix(tests): widen _wait_pidfile's window and name what it saw (#595) - #758
Open
Masashi-Ono0611 wants to merge 1 commit into
Open
fix(tests): widen _wait_pidfile's window and name what it saw (#595)#758Masashi-Ono0611 wants to merge 1 commit into
Masashi-Ono0611 wants to merge 1 commit into
Conversation
…e#595) A watcher relaunch is a real fork + lock-acquire + SIGTERM-the-predecessor + self-write before the pidfile reflects it, and the 3s this polled for could lose that race on a loaded CI runner -- the flake fujibee#595 caught on PR fujibee#436's macos-latest 4/4 shard, at _wait_pidfile "$pf" "$w2". Of fujibee#595's four sites, three are already fixed independently: marker-gc and codex-monitor by fujibee#606, the launcher re-registration race by fujibee#615. This is the fourth and last: _wait_pidfile in test_watch.bats itself, still on its original budget. - Widened to 10s, matching the launcher suite's wait_for_child_count budget (test_codex_bridge_launcher.bats) rather than inventing a new number. - On timeout, reports the pidfile path, the wanted pid, and what it last read instead -- fujibee#595 asked for a message that can tell "never arrived" from "arrived as something else"; a bare assertion failure at this line couldn't. Not a hoist into a shared helper: fujibee#595 suggested one, but the launcher suite's wait_for_child_count is a different predicate shape (process count, not pidfile content) with no shared caller today, and unifying that pattern is a design call for whoever wants it, not implied by fixing this flake. Verified: full tests/test_watch.bats (27/27) before and after; the target test individually; and the timeout path's message format tested standalone against a forced-unreachable pid.
This was referenced Aug 14, 2026
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.
Closes the last of #595's four sites.
What this fixes
_wait_pidfileintests/test_watch.batspolled up to 3s for a relaunchedwatcher's pidfile to record the new pid. A relaunch is a real fork +
lock-acquire + SIGTERM-the-predecessor + self-write before the pidfile
reflects it, and 3s could lose that race on a loaded CI runner — the flake
#595 caught on PR #436's
macos-latest 4/4shard, at_wait_pidfile "$pf" "$w2".Of #595's four sites, three turned out to already be fixed independently
since I filed it: marker-gc and codex-monitor by #606, the launcher
re-registration race by #615. This PR is the fourth and last —
_wait_pidfileitself, still on its original budget.
What changed
wait_for_child_countbudget (test_codex_bridge_launcher.bats) ratherthan inventing a new number.
what it last read instead of that — bats suite flakes across the process-lifecycle tests: main itself fails ~half its runs, a different test each time #595 asked for a failure message that
can tell "never arrived" from "arrived as something else"; a bare
assertion failure at that line couldn't.
What this deliberately doesn't do
#595 also suggested hoisting a shared predicate-wait helper across all four
sites. I didn't do that here:
wait_for_child_countis a differentpredicate shape (process count, not pidfile content) with no shared caller
today, and unifying the pattern is a design call rather than something this
one-site fix implies. Happy to take that separately if it's still wanted now
that three of the four sites are gone.
Verification
bats tests/test_watch.bats: 27/27 pass, before and after.-unreachable pid — confirmed it reports the actual stale value read from
the pidfile, not a generic failure.