codex-bridge: one wake, one turn — attribute mid-start turn-end signals by turn id - #889
Draft
ainyan03 wants to merge 4 commits into
Draft
codex-bridge: one wake, one turn — attribute mid-start turn-end signals by turn id#889ainyan03 wants to merge 4 commits into
ainyan03 wants to merge 4 commits into
Conversation
… app-server Three orderings around an in-flight turn/start request, replayed against a scripted app-server: - duplicate-turn injection: a wake deferred behind a running turn is delivered from onTurnEnded() when turn/completed arrives, and while the resulting turn/start request is still unanswered the app-server independently emits thread/status idle for that SAME previous turn. The second turn-end re-enters tryStartTurn() with the same wake and starts a second turn whose whole prompt is inbox.sh literal "No new messages." output, the first read having already consumed the rows. Observed live: one wakeup, two consecutive started-turn lines, an injected empty turn in the TUI. - pre-ACK completion: the dual ordering. A fast turn is fully notified (turn/started, then turn/completed) before the turn/start request is ACKed; the end must still be processed promptly instead of waiting out the idle watchdog. - stale-idle attribution: the composition. The new turn is seen starting before the ACK, and only THEN does the previous turn idle straggle in; the running turn must not be ended early. All three are red at this commit; the following commits turn them green one mechanism at a time. The duplicate-turn test also pins that the duplicate wake is never spent at all (no empty re-read aborting behind a quiet inbox), so the case stays red until the actual re-entry is closed, not merely its visible prompt. Assertions use grep -q in non-last positions per the existing note in this file: on bash 3.2, which is what macOS CI runs, a false [[ ]] there reports ok. Negated checks are count comparisons for the same reason.
…/start A wake deferred behind a running turn is delivered from onTurnEnded() when turn/completed arrives. While the resulting turn/start request is still in flight, the app-server independently emits thread/status idle for that SAME previous turn; both signals funnel into onTurnEnded(). Because the wake claim (pendingWake) was only cleared after the request resolved, the second turn-end reset turnActive/threadIdle and re-entered tryStartTurn() with the same wake -- starting a second turn whose entire prompt was inbox.sh literal "No new messages." output, the first read having already consumed the rows. Observed live: one wakeup, two consecutive started-turn lines, and an injected empty turn in the TUI. Two mechanisms, both required to close the re-entry (either alone happens to cover the replayed ordering, so the tests pin the invariant "the duplicate wake is never spent", which needs both): - tryStartTurn() claims the wake BEFORE the request goes out and restores it on failure, so no concurrent path can spend the same wake twice. - startInFlight scopes the in-flight window: onTurnEnded() drops every turn-end signal that lands inside it, and the idle handler does not flip threadIdle under the start it no longer owns. The duplicate-turn and stale-idle orderings go green here. The pre-ACK ordering is red AT THIS COMMIT, deliberately: the gate is a blanket drop, and a fast turn fully notified (started AND completed) before its ACK now waits out the idle watchdog. That is why this cannot ship without the next commit, which attributes mid-start end signals by turn id instead of dropping them wholesale.
Mid-start events are legal in BOTH directions -- the previous turn's tail, and the NEW turn's own lifecycle, which the app-server may notify before it ACKs turn/start -- and a stale tail can land AFTER the new turn was seen starting, so neither a blanket ignore (the previous commit) nor a phase flag can attribute them. Identity can: - turn/started (now filtered to our thread, which also stops another thread's turns from flipping our state) records the in-flight turn id. - While the request is unanswered, onTurnCompleted defers the end ONLY when its turn id matches that in-flight id; tryStartTurn() then runs one onTurnEnded after the ACK, so a fast turn neither waits out the idle watchdog nor skips the maxWakes accounting. A different id, an id-less completion, or a thread/status idle is unattributable mid-start and is dropped -- the idle watchdog closes a genuinely-ended turn (fujibee#41). This turns the pre-ACK completion ordering green (it hung until the watchdog under the blanket gate) while keeping the stale-idle ordering green: a stale idle straggling in after the new turn was seen starting carries no matching id and is dropped instead of ending the running turn. All three replayed orderings pass from this commit on.
…r become a prompt readInboxForPrompt() passes --quiet to inbox.sh (an existing flag), so an empty inbox reads back as EMPTY and the human-facing "No new messages." line can never again pass tryStartTurn's emptiness check and become the entire prompt of a turn. Defense in depth, not the fix: with the two previous commits in place a duplicate wake is never spent, so this path is not reachable from the replayed orderings -- reverting only this commit leaves all three tests green. It hardens the prompt boundary against any OTHER path that might reach an empty read, and the duplicate-turn test pins the distinction by asserting the re-read itself never happens rather than merely that no sentinel prompt was seen.
ainyan03
marked this pull request as draft
August 19, 2026 01:05
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
1 participant
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 #870.
One wake could start two turns: a turn-end signal landing while the
turn/startrequest was still unanswered re-entered
tryStartTurn()with the same wake. ThisPR closes the re-entry (the wake is claimed before the request goes out, and
turn-end signals are gated while it is in flight) and attributes mid-start end
signals by turn id so a fast turn still ends promptly. Every claim below is a
local test run at a specific commit of this branch; the one open decision is
whether to keep the wake claim as defense in depth (question at the end).
The series is the verification matrix
You asked for each half reverted individually, with exactly its case going red.
Measured, the halves don't decompose that way (next section), so the PR is
structured as a cumulative series instead — each commit flips exactly the tests
its mechanism owns. Rows 1–4 are
git checkoutof the Nth commit of thisbranch; the last two rows are described below the table.
--quiet(full series)test_codex_bridgesuite 45/45--quietonly (identical tree to row 3)"full − wake claim only" = on top of row 4, move
this.pendingWake = falseback to after the ACK and drop the restore in the
catch.Row 2 is red on pre-ACK completion on purpose: there the gate is a blanket
drop, and a fast turn fully notified before its ACK waits out the idle watchdog
(exit 124 under the test runner). That is the measured reason the gate cannot
ship without the id attribution.
Why the independent-revert matrix doesn't hold
gate already drops every turn-end signal that could re-enter
tryStartTurn()while the request is unanswered, so the claim's timing is unobservable from
events. It guards the invariant "a wake is spent at most once" at the state
level; it is not an independently testable half.
stale-idle: removing it also removes the deferral, so the mid-start
completion is acted on immediately and the bridge shuts down (
--max-wakes)before the ACK. The stale-idle ordering is instead covered by the step from
row 1 to row 2.
Which test covers which ordering
a turn fully notified before its turn/start ACK still ends promptly: the scripted app-server sendsturn/started+turn/completed(matching turn id) before ACKingturn/start; the testasserts no hang and both lifecycle lines. This is the only test that is red
at row 2.
a stale idle landing after the new turn was seen starting does not end the running turn:turn/started(new id), thenthe previous turn's
thread/statusidle, then the ACK, then the realcompletion 2 s later; the test asserts by log-line order that neither the
second turn nor the bridge's own exit precedes the real completion.
--quietRemoving only
--quietfrom the full series leaves everything green — matchingyour expectation that nothing may ride on it. It suppresses the symptom (the
sentinel line can never become a prompt), not the cause. The duplicate-turn
test pins that distinction: it asserts the duplicate wake is never spent at
all, not even on an empty re-read that aborts, so
--quietcannot mask aregression of the actual fix.
Assertions use
grep -qin non-last positions, per the existing note in thistest file about
[[ ]]under bash 3.2.The table is from local macOS runs; CI covers both OSes on the PR, so no
manual runs should be needed on your side.
One question: keep the wake claim (early
pendingWake = false+ restore onfailure) as defense in depth, or drop it for minimality? The series stays green
either way — happy to amend.