Repository navigation
gateway: leaving a session waits for model-gateway to detach the run - #37
Merged
Merged
Conversation
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. |
GatewaySession.__aexit__ closed its side of the Attach stream and cancelled it at once, while model-gateway detaches the run only when it reads that close. For a moment after the session was left, the run's keys were still accepted, and a call then reached the backend before failing closed for want of an ack. That made test_without_an_attached_stream_the_backend_is_never_called fail now and then under load. Leaving now waits, up to 10 s, for the gateway to end the stream, which it does after detaching; a stream that ends while leaving is not an error. The guard delays the gateway's detach so the window is always open. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
jackeydou
force-pushed
the
gateway-detach-race
branch
from
October 8, 2026 00:30
309bd52 to
0a7f867
Compare
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.
Summary
test_without_an_attached_stream_the_backend_is_never_calledfailed now and then undermise run check: the scripted backend got a call nobody had queued a reply for. It passes on its own. This came up on #36, but it is a race in code that PR does not touch.Root cause.
GatewaySession.__aexit__closes its side of theAttachstream and cancels the stream at once. model-gateway detaches the run only when it reads that close, so for a moment after the session is left, the run's keys are still accepted. A call made in that window reaches the backend, then fails closed because no ack comes.The fail-closed rule held throughout: no response went unrecorded. The cost was a wasted backend call, plus the flaky test.
Fix. Leaving the session waits, up to 10 s, for the gateway to end the stream. The gateway ends it only after it has detached the run. A stream that ends while the session is leaving is not an error. If the gateway takes longer than 10 s, a warning is logged and the session leaves anyway, as before.
Also in this PR: a
BUGFIX.mdentry, aCHANGELOG.mdentry under Fixed, and a note indocs/services/model-gateway.md.Test plan
test_leaving_a_session_returns_after_model_gateway_detached_the_run. It delays the gateway's detach by 0.2 s so the window is always open. It fails without the fix and passes with it.mise run checkmise run test:docker: 149 passed.Notes
The
test:dockerrun also had one failure:test_takeover.py::test_a_live_worker_keeps_its_run_past_many_leases. It is another timing-sensitive test, with 0.3 s leases. It passed 6 of 6 runs on its own and does not go through the gateway session, so it is not part of this PR.🤖 Generated with Claude Code