fix(codex): bound watch-once by its lifetime, not by its polling (#558) - #560
Merged
Conversation
The deadline was taken after startup, so real wall time was startup plus TIMEOUT. The bridge force-kills the child at timeout + interval + 10 from spawn, so once startup exceeds interval + 10 the child dies at 124 before reaching its own clean exit 2 — every re-arm, not intermittently. Three of those and the bridge self-destructs, the launcher restarts it, and orphan watch-once processes pile up (the retention half of #149). Startup is ~0.2s here and ~29s under MSYS fork emulation, which is why this only ever appeared on Windows. Reported with measurements and this fix by 東リ屋 (#558). A slow startup now eats into the polling window instead of overrunning the ceiling. It cannot skip the check: the loop queries before testing the deadline, so a deadline already past still yields one full inbox check. Tests delay awk, which startup uses to resolve pairs while the poll goes through sqlite3, so startup slows without touching the query. One test pins the lifetime ceiling and fails without the fix (8s against a 4s timeout); the other pins that an already-past deadline still checks, which guards the fix rather than the bug.
The awk shim is the premise, not scaffolding: without the delay an unfixed watch-once finishes in about TIMEOUT and satisfies the ceiling, so the test would go green against the bug it exists for. That happens silently the day startup stops routing through awk. Assert the marker the shim writes, so a vanished seam fails loudly and names what to do. Verified by disabling the shim: the guard fires with "the awk shim never fired ... re-pick the seam" instead of passing.
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.
Fixes bug 1 of #558, reported with root-cause analysis, this fix, and measurements by 東リ屋 (@8CEVSmSRMT32119).
The bug
watch-once.shtook its deadline after the startup work, so its real wall time wasstartup + TIMEOUT. The bridge force-kills the child at(timeout + interval + 10)seconds measured from spawn. Once startup exceedsinterval + 10— about 12s at defaults — the child is killed before it can reach its own deadline and the cleanexit 2, so it exits 124 on every re-arm rather than intermittently. Three of those and the bridge self-destructs; the launcher restarts it; orphanwatch-onceprocesses accumulate, which is the retention half of #149.Startup is ~0.2s on Linux and ~29s under MSYS fork emulation, which is why this only ever appeared on Windows.
Confirmed independently against
mainbefore implementing: the deadline atwatch-once.shwas computed after four library sources plus project and subscription-pair resolution, andcodex-bridge.jssetstimeoutMs: (this.opts.timeout + this.opts.interval + 10) * 1000on the spawn.The fix
Take the timestamp before any startup work and base the deadline on it. Two lines, as the reporter proposed.
A slow startup now eats into the polling window instead of overrunning the ceiling. It cannot degrade into skipping the work: the loop queries the inbox before testing the deadline, so even a deadline that is already past yields one full check first.
Testing the Linux-invisible
The difference is ~startup, which is 0.2s here — too small to assert. So the tests make startup slow on purpose: an
awkshim delays once, and startup resolves subscription pairs throughawkwhile the polling loop queries throughsqlite3, so the delay lengthens startup without touching the poll. The shim sleeps on first call only, so the delay is exactly N regardless of how manyawkcalls startup makes.Two tests, and they are not the same kind of evidence:
total lifetime stays within the timeout even when startup is slow— pins the ceiling the bridge depends on. Verified load-bearing: with the fix reverted and the test kept, it fails withlifetime 8s exceeded the 4s timeout by more than slack.a deadline already past still performs one inbox check— startup longer than the whole timeout, message pending, must still exit 0. This one passes without the fix too, so it does not catch the bug; it guards against a fix that bounds the lifetime by skipping the check. Recorded as such rather than presented as a second regression test.The assertion is a ceiling with slack (6s against a 4s timeout) rather than a precise duration, so it separates the two behaviours without being tight enough to flake on a loaded runner.
bats tests/test_watch_once.bats— 8/8, exit 0.Not in this PR
Bugs 2 and 3 from #558 (codex join skipping the seat record;
identities.shcost) are recorded on that issue. Bug 2 touches the join/seat-record contract and wants a decision rather than a patch. TheAGMSG_WATCH_ONCE_TIMEOUTdefault and the re-arm-period-vs-poll-interval documentation the reporter raises are also left for that discussion.