Repository navigation
fix(appproto): free the passthrough slot before an answer's last frame - #1709
Conversation
The race job sometimes failed two api tests on PRs that never touch them. TestReadingTheConfigIsAllowedForAnyRole gathered chunks after api.head and before api.end, so under load the body came back empty. It now waits for api.end first and checks the byte count. TestAViewerMayStillRead got E_UNAVAILABLE busy: it sent each read while the last answer was still streaming. It now waits for api.end, as the app does. Behind that, the box freed its one-request slot only after sending the last frame, so a client asking again on api.end could still be told busy. The slot is now freed just before api.end or a refusal goes out. A new test hands the box the next request from inside that Send; it fails every time on master. The waitIdle workaround in the appproto tests is gone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: adf1c35b79
ℹ️ 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".
| // middle. The app is committed to the status by then, so the only honest | ||
| // thing left to say is that this is not the whole of it. | ||
| truncated := w.truncated || w.ctx.Err() != nil || w.panicked | ||
| w.free() |
There was a problem hiding this comment.
When BenchmarkAPIWriterLargeResponse calls finish() at passthrough_test.go:413, its directly constructed apiWriter never initializes free. This unconditional call therefore panics with a nil-function dereference instead of producing benchmark results; initialize the callback in that literal or make the callback safe when omitted.
Useful? React with 👍 / 👎.
1 participant
The race job ("core (Go race detector)") sometimes failed two
go/internal/apitests on PRs that never touch them. It blocked #1703 (run 39045010631) and failed onclaude/box-page-cars(run 39021378697). This fixes both tests and the box-side cause behind the second one.What failed, and why
TestReadingTheConfigIsAllowedForAnyRolegatheredapi.chunkframes as soon asapi.headarrived and only then waited forapi.end. Under load the chunks came after the snapshot, so the body was empty (unexpected end of JSON input ("")). The test now waits forapi.endfirst, gathers the chunks after it, and checks that their size matches what the end reports. No other passthrough test gathers chunks this way.call()inapi_app_link_session_test.goand the appproto tests already wait for the end first.TestAViewerMayStillReadgotE_UNAVAILABLEwithreason: busy, not a timeout or a size limit. The whole test ran in 0.01 s. The passthrough serves one request per session at a time. The test sent each next read as soon as the head of the last one arrived, while that answer was still being sent./api/system/infofailed most often because it follows/api/modes, the largest of the four answers. I also saw/api/modeand/api/modesfail. The test now waits for each answer'sapi.endbefore it asks again, the way the webapp does (session.tssends one call at a time and waits for each to finish).That alone would leave a smaller race. The box freed its slot only after it had sent the answer's last frame (
defer h.apiBusy.Store(false)aroundserveAPI). A client that asks again the momentapi.endor a refusal arrives could still be told busy. The appproto tests already worked around this with awaitIdlehelper that polled the private flag. Its comment said: "The frame lands just before the serving goroutine clears the flag".The box change
go/internal/appproto/passthrough.go: the slot is freed just before the last frame goes out (api.end, a gate refusal, or the "unavailable" error after a timeout or panic), once the handler has returned. Async.Oncearound the release keeps the end of the goroutine as a fallback for other paths: a revoke, a failed send, or a media-type refusal sent while the handler still runs. It cannot free a later request's slot. Depth one is unchanged.Would a box hit this? Rarely. Over a relay or WebRTC, the network round trip is far longer than the gap between sending
api.endand freeing the slot. But the webapp asks again as soon as an answer ends, and losing that race costs a view the 30 s busy backoff thatsession.tsdescribes. In process, as in these tests, it happens. So this is a timing slip, not slow work: system/info answered in milliseconds.The same fix covers the sweeps that send refusal after refusal in one session (
TestAViewerIsRefusedAtEveryWrite,TestNoSecretBearingReadCrossesTheSession). They had the same small race.Tests
TestTheNextRequestIsServedOnceAnAnswerEnds(appproto). A sender with no delay hands the box the next request from inside theSendthat carries the last frame, for both an answer and a refusal. It fails every time on master (E_UNAVAILABLE busy) and passes on this branch (300 of 300 with-race).waitIdleis gone. Waiting for the frame is now enough.Evidence
Same machine and load for both: three full copies of the api package with
-racein parallel, and the two tests withGOMAXPROCS=2 -count=2000:TestAViewerMayStillReadTestReadingTheConfigIsAllowedForAnyRole/api/mode,/api/modes,/api/system/info)go test -race -run <name> -count=200for each test with the full package running beside it: 200 of 200 passed.go test -race -count=3 ./internal/api/ ./internal/appproto/: ok.make verify: green (76 packages, vet and build).No change in
go/internal/control. Patch changeset included.Follow-ups (not in this PR)
Other tests that fail now and then in master's race job:
TestTheBoxOpensAnExchangeBesideItsUplinkTestForecastRetentionLargeOverflowWithinScoringBudgetTestAPlugInReachesAStreamingPhoneAsAPlan🤖 Generated with Claude Code