Skip to content

fix(tests): widen _wait_pidfile's window and name what it saw (#595) - #758

Open
Masashi-Ono0611 wants to merge 1 commit into
fujibee:mainfrom
Masashi-Ono0611:fix/595-wait-pidfile-timeout
Open

fix(tests): widen _wait_pidfile's window and name what it saw (#595)#758
Masashi-Ono0611 wants to merge 1 commit into
fujibee:mainfrom
Masashi-Ono0611:fix/595-wait-pidfile-timeout

Conversation

@Masashi-Ono0611

Copy link
Copy Markdown
Contributor

Closes the last of #595's four sites.

What this fixes

_wait_pidfile in tests/test_watch.bats polled up to 3s for a relaunched
watcher'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/4 shard, 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_pidfile
itself, still on its original budget.

What changed

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_count is a different
predicate 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.
  • The target test individually: pass.
  • The timeout path's message format tested standalone against a forced
    -unreachable pid — confirmed it reports the actual stale value read from
    the pidfile, not a generic failure.

…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.
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.

1 participant