Isolate the transport so one recipient cannot hold it against another - #19
Merged
Merged
Conversation
Criterion 8 asks for bounded connection wait, retry and error isolation, so a busy or failing parent cannot starve a serviceable one. PR #9 delivered the scheduler half and its own operations.md says so in as many words: fairness there is scheduling, not transport concurrency. This is the other half. The defect is not an unbounded hang. One send is three sequential RPCs, each bounded by the bridge's own wait_for, so the window is roughly 3 x timeout + connect. What was missing is isolation. _Transport._worker awaited one submission at a time, and _submit's caller budget expiring did not release the worker - the abandoned coroutine kept running to the end of its chain while the next submission, from a different parent, waited in the inbox behind it. So B paid A's whole chain rather than A's caller timeout. The scheduler's own isolation only starts once a send returns, which is exactly what was being delayed. Submissions are dispatched as tasks now. Two bounds keep that from becoming a different problem. One mutation at a time per recipient. _guarded_send reads the thread, resumes it and starts a turn across separate awaits, so two concurrent sends to one thread could both pass the idle check and both start a turn - request-id idempotency does not catch that, because the two requests are genuinely different. A second send to a busy recipient is reported immediately rather than queued behind a turn that may run for minutes; waiting would only turn a fast, accurate answer into a slow one, and waiters are precisely what would accumulate without bound. Nothing is sent on that path, so it is reported as a busy recipient rather than an unknown outcome, which keeps it retry-safe instead of parking the delivery as possibly delivered. And a deadline on executing work, because rpc.py awaits ws.send() outside its own response timeout - a write that never drains would otherwise hold its recipient forever. Cancellation reaches _guarded_send, which records its own outcome_unknown receipt before re-raising. Shutdown is ordered and finite. Closing resources used to be ordinary queued work, which under concurrent dispatch could close the ledger while a send still needed to save its receipt. It now stops accepting, answers what is still queued without running it, drains for a bounded interval, cancels what remains while the ledger is still open, and only then closes. The module docstring claimed asyncio's cross-thread wakeup never arrives here, measured on CPython 3.13 and 3.14. Re-measured against the same shape before designing any of this: it completes and overlaps on 3.13.14 and 3.14.4. The queue handover stays, because it depends on nothing but the loop's own timer, but the note is corrected rather than left to send the next reader down a road that is no longer closed. CALLER_SLACK_SECONDS names the +10 that was written inline. It is also the bound on how long a submission may outlive its caller, and a bound nobody can set is a bound nobody can test. Separately, enqueue() now deletes the delivery_intent row in the same transaction that inserts the delivery. They were two commits, and a crash between them left an intent for an event that is already queued - nothing lost and nothing sent twice, but no pass removes it either, because the queries that would find it filter on having no delivery. Five transport tests drive the real BridgeHostAdapter and its real worker thread, with the stall injected at the RPC boundary through _build's app_server_factory seam; fakehost.py cannot reach _Transport, being a separate implementation over its own state. The isolation assertion is causal rather than a stopwatch: B is asserted to complete while A is still held at its barrier. All five fail against the shipped transport from dev, and the two intent tests fail against the two-commit enqueue.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
The withheld receipt shares deferred_busy with a host-reported busy, and the frozen attempt schema is why: retrySafe is only accepted alongside thread/read or thread/resume, so a locally withheld send cannot carry failedOperation "transport" without also claiming it is unsafe to retry - which would be the wrong answer for a send that was never made. So the distinction lives in the error text, which is what the diagnostics criterion asks for: the specific cause must not be hidden behind the generic state. The test now asserts the text names the relay and says nothing was sent, and that the record it produces validates against the frozen schema.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Both are in the concurrency the previous two commits introduced, and review found both. close() cannot set _stopping and queue the sentinel in one indivisible step, so an idle worker that woke between those statements left through the empty-queue branch: _shut_down never ran, the connection and the ledger stayed open, and the sentinel close() blocks on was never settled. The flag no longer ends the worker. The sentinel is the only way out. The per-recipient bound exists to stop a second turn being started for one thread. A replay starts nothing - it reads a receipt - but it was still refused as a busy recipient whenever any other send to that thread happened to be in flight, turning a settled outcome back into a retry, which is the one thing a request id exists to prevent. The ledger is now asked before the recipient lock, and a lookup that raises on an id reused with different arguments still raises. _send_identity holds the ledger's operation name and argument fingerprint in one place, because the precheck and the send itself have to ask the same question or the precheck quietly stops matching and replays go back to being refused. Both regressions were confirmed failing against the previous commit.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Checking that the transport is still accepting and putting the work on the queue were two steps, and close() fit between them. A submission that passed the check and queued after the drain had already run left a future nobody would ever settle: its caller waited out the full budget and read a timeout. That is the one report this layer must not produce for work that was never started, because a timeout is read as an unknown outcome and an unknown outcome is not retry-safe - while the truth was that nothing had been sent at all. The check and the enqueue now happen under the lock close() takes to end acceptance, so a submission is either in the queue before the drain and answered by it, or refused outright. Confirmed failing against the previous commit: the caller came back with a TimeoutError. The interleaving is a few bytecodes wide, so the test drives it rather than racing it - the submitter is held at the moment it is about to enqueue, close() is started behind it, and then the submitter is let go.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
operations.md and the recorded-limits table in invariants.md both still said the adapter serialises on one worker and a stalled call blocks the one behind it. That came from #9 (23972b7) and was true when it was written. It is the exact limitation this branch exists to remove, so shipping it unchanged would have left the documents asserting that the transport half of the fairness criterion is still unmet. Both now describe the bound that actually holds - one send in flight per recipient, dispatched as its own task, so a stall reaches that recipient and no other - and the limits table carries the residual limit in its place rather than dropping the row: an abandoned send keeps holding its own recipient until the transport's deadline of four times the RPC timeout, because rpc.py awaits the websocket write outside its response timeout and cancelling a send mid-flight is what produces an unknown outcome instead of a real one.
StateDirectory did not pin HOME, so resolving a socket's directory read the siblings next to the real one and opened the live relay databases under ~/.local/state/codex-session-relay. Measured before and after a full suite run: only the sqlite shared-memory index was touched, never a database file or its WAL. It is still not something a unit test should reach. It now pins HOME the way the Precedence class directly below it already did. A full suite run leaves the real state directory byte-identical. This predates the branch and the same fix is on the operations-doc branch, which is where it was found. It is here as well because this branch's own isolation evidence depends on it and the two land independently.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…t-isolation # Conflicts: # packages/codex-session-relay/tests/test_store.py
_shut_down cancels work that outlived the drain window, and asyncio.CancelledError inherits from BaseException. Handed across the thread boundary unchanged it walked straight past the except Exception that DeliveryService.attempt wraps the send in. That matters because of where it lands. attempt() claims the delivery and inserts its attempt row in one transaction, then sends. An exception it cannot catch unwinds the tick between those two points, leaving the delivery leased, in sending, with an attempt row nothing ever settles - a claim a restart cannot account for. The ledger receipt on the bridge side bounds the damage; it does not settle the relay's own row. Cancellation is now translated at the settle boundary into an ordinary exception that says the outcome is unknown, which is the truth: _shut_down only cancels work that has already been in flight past the drain, so the write may have reached the host before the cancel landed. The original is kept as __cause__ with its trimmed traceback, so why a send ended is still readable. Everything that is already an Exception passes through untouched. Two regressions, both confirmed failing against the merge commit. The first asserts the caller receives an Exception at all. The second drives DeliveryService.attempt with the exception a real transport cancellation actually produces - captured from a real BridgeHostAdapter shutting down on a stalled send, not a hand-written stand-in - and asserts the attempt row is settled and a fresh service over the same store finds the delivery in held_uncertain with no lease.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a3725dcc4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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.
Closes acceptance criterion 8's transport half. PR #9 delivered the scheduler half, and its own
docs/operations.mdscopes the guarantee to scheduling rather than transport concurrency; the transport was not changed by #8 or #9.The defect, stated precisely
Not an unbounded hang. One send is three sequential RPCs, each bounded by
asyncio.wait_forinside the bridge'srpc.py, so the window is roughly 3 × timeout + connect — about 80 s at the default 20 s.What was missing is isolation.
_Transport._workerawaited one submission at a time:_submitgives the callertimeout + 10and then abandons — but abandonment did not release the worker. The abandoned coroutine ran on to the end of its chain while the next submission, from a different parent, waited in the inbox behind a send whose caller had already given up. So parent B paid parent A's whole chain rather than A's caller timeout. The scheduler's isolation (attempt→outcome_unknown→HELD_UNCERTAIN→struggling) only begins once a send returns, which is exactly what was being delayed.An independent audit reproduced it against the real adapter before any code changed:
What changed
Submissions are dispatched as tasks rather than awaited inline. Two bounds keep that from becoming a different problem.
One mutation at a time per recipient.
_guarded_sendreads the thread, resumes it and starts a turn across separate awaits, so two concurrent sends to one thread could both pass the idle check and both start a turn. Request-id idempotency does not catch that — the two requests are genuinely different. The audit reproduced this against the real_guarded_sendwith an in-memory ledger. A second send to a busy recipient is reported immediately rather than queued behind a turn that may run for minutes: waiting would only convert a fast, accurate answer into a slow one, and waiters are exactly what would accumulate without bound. Nothing is sent on that path, so it is classified as a busy recipient (DEFERRED_BUSY,sendAttempted: no, retry-safe) rather than an unknown outcome, which would park the delivery as possibly delivered and block a clean retry.A deadline on executing work, because
rpc.pyawaitsws.send()outside its response timeout — a write that never drains would otherwise hold its recipient forever. Cancellation reaches_guarded_send, which records its ownoutcome_unknownreceipt before re-raising.Ordered, finite shutdown. Closing resources used to be ordinary queued work, which under concurrent dispatch could close the ledger while a send still needed
ledger.save(). It now stops accepting, answers what is still queued without running it, drains for a bounded interval, cancels what remains while the ledger is still open, and only then closes RPC and ledger.CALLER_SLACK_SECONDSnames the+ 10that was written inline. It is also the bound on how long a submission may outlive its caller, and a bound nobody can set is a bound nobody can test.Re-measured before designing
The module docstring claimed asyncio's cross-thread wakeup never arrives here — "a coroutine submitted with
run_coroutine_threadsafeto arun_foreverloop in another thread never completes, measured on both CPython 3.13 and 3.14 here." Re-measured against the same shape, on both interpreters it names:run_coroutine_threadsafeThe auditor confirmed this independently. The queue handover stays — it depends on nothing but the loop's own timer, and the sqlite thread-affinity constraint that motivates the worker thread is unchanged — but the note is corrected rather than left to send the next reader down a road that is no longer closed.
Carried follow-up: the stranded intent
enqueue()now deletes thedelivery_intentrow in the same transaction that inserts the delivery, and the idempotent early return clears a stranded one too. They were two commits, and a crash between them left an intent for an event that is already queued — no lost event, no duplicate send, but nothing removes it either, because the queries that would find it filter on having no delivery._requeue_missing's separateclear_intentcall is gone, sinceenqueuenow owns it.Validation
Five transport tests in
TransportIsolationdrive the realBridgeHostAdapterand its real worker thread, with the stall injected at the RPC boundary through_build'sapp_server_factoryseam.fakehost.pycannot reach_Transport— it is a separate implementation over its own in-memory state — so these are the only tests that can establish this.The isolation assertion is causal rather than a stopwatch: B is asserted to complete while A is still held at its barrier, and that A has not started a turn.
All five fail against the shipped transport from
dev. The two intent tests fail against the two-commitenqueue.Relay suite 826 passed / 1 skipped with the bridge importable, 792 / 35 skipped without it.
validate.py,contracts.py,check_operations_contract.py,unittest discover -s scripts/ci/tests,secrets.sh,git diff --checkall exit 0.CRW_PACKAGES_TMPDIR=/var/tmp packages.pyexits 0 — bridge 119 tests, relay 859.Scope and limits
bridge_adapter.pyanddelivery.pyinside the relay package;packages/codex-thread-bridge/is untouched. The unguardedawait ws.send(...)inrpc.pyis the bridge package and out of scope — this PR bounds its effect rather than fixing it.These are RPC-seam tests with in-process fakes; neither they nor the probes establish live App Server behaviour. What this establishes is isolation from an outstanding asynchronous operation on a serviceable connection, not independent connections or zero scheduler delay.
AppServer._connect_lockremains a shared bottleneck at connect time.