fix: finish passthrough SSE after terminal events - #1055
Conversation
|
✅ Deterministic PR hygiene checks passed. |
Review readiness checklistThis PR is kept in draft until every requirement below is fulfilled. The tickable checklist has been added to your PR description — tick all four boxes there.
✅ 4/4 boxes ticked. ✅ All four boxes are ticked. |
|
✅ PR quality gates passed This pull request now targets The title was left unchanged. Its existing draft status has been preserved. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe SSE relay now tracks upstream ChangesSSE completion handling
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant UpstreamSSE
participant relaySseWithHeartbeat
participant ClientStream
UpstreamSSE->>relaySseWithHeartbeat: Send response.completed
relaySseWithHeartbeat->>ClientStream: Forward terminal event
relaySseWithHeartbeat->>ClientStream: Append data: [DONE] if missing
relaySseWithHeartbeat->>UpstreamSSE: Cancel upstream reading
relaySseWithHeartbeat->>ClientStream: Close stream
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
58f0307 to
5a69355
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/relay.ts`:
- Around line 590-598: The relay stream handling around inspectChunk must return
the byte boundary of the terminal SSE block, then enqueue only the input through
that boundary and discard any following bytes before emitting the synthetic
doneSentinel. Update terminal detection so doneSeen reflects the inspected
terminal block even when a later [DONE] block is split across the same read, and
add a split-chunk regression covering this case in passthrough-abort.test.ts.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 67b81ed9-0aac-459c-9b7c-9974f87fa702
📒 Files selected for processing (2)
src/server/relay.tstests/passthrough-abort.test.ts
| inspectChunk(value); | ||
| controller.enqueue(value); | ||
| if (terminalReported && !doneSeen) { | ||
| controller.enqueue(doneSentinel); | ||
| doneSeen = true; | ||
| cleanup(); | ||
| controller.close(); | ||
| reader.cancel("Responses terminal event received").catch(() => {}); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not forward bytes after the terminal SSE block.
Line 590 can leave an incomplete later block in buffer, but Line 591 forwards the entire value. If one upstream read contains a complete response.completed block followed by the first bytes of a real data: [DONE] block, doneSeen remains false. Lines 592-598 then append a second sentinel and cancel before the next read can complete the original block. The client receives adjacent data: [DONE] lines, which an SSE parser reads as one payload instead of [DONE].
Return the terminal block boundary from the inspector and enqueue only bytes through that boundary. Discard post-terminal bytes before emitting the synthetic sentinel. Add a split-chunk regression in tests/passthrough-abort.test.ts around Lines 224-238.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/relay.ts` around lines 590 - 598, The relay stream handling around
inspectChunk must return the byte boundary of the terminal SSE block, then
enqueue only the input through that boundary and discard any following bytes
before emitting the synthetic doneSentinel. Update terminal detection so
doneSeen reflects the inspected terminal block even when a later [DONE] block is
split across the same read, and add a split-chunk regression covering this case
in passthrough-abort.test.ts.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58f0307ca1
ℹ️ 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".
| } | ||
| inspectChunk(value); | ||
| controller.enqueue(value); | ||
| if (terminalReported && !doneSeen) { |
There was a problem hiding this comment.
When an upstream batches a Responses terminal event and data: [DONE] in the same chunk but keeps the HTTP connection open, inspectChunk() sets both terminalReported and doneSeen, so this new branch is skipped and the relay never runs cleanup() or controller.close(). That leaves the heartbeat timer active and onDone uncalled despite the terminal event already being forwarded; only the synthetic DONE enqueue should be guarded by !doneSeen, while close/cleanup should still happen whenever a terminal was reported.
Useful? React with 👍 / 👎.
| if (terminalReported && !doneSeen) { | ||
| controller.enqueue(doneSentinel); |
There was a problem hiding this comment.
If the upstream closes after a terminal frame that lacks the final blank SSE delimiter, the relay already forwarded those raw bytes during the previous pull and this EOF path then appends data: [DONE]\n\n directly after them. That turns the terminal payload and [DONE] into one malformed SSE block (data: <json>\ndata: [DONE]) instead of dispatching the terminal event followed by a separate DONE sentinel; insert a delimiter before the synthetic sentinel or use the frame-aware terminal boundary for this path.
Useful? React with 👍 / 👎.
| const relayed = relaySseWithHeartbeat(streamFromChunks([ | ||
| enc.encode('event: response.completed\ndata: {"type":"response.completed","response":{"id":"r1","status":"completed"}}\n\n'), | ||
| ]), ac, 15_000, status => terminals.push(status))!; |
There was a problem hiding this comment.
This regression constructs relaySseWithHeartbeat() directly, but this same test file asserts that the real native Responses passthrough branch does not call relaySseWithHeartbeat() and production now routes through relaySseWithFailedTail() instead. As a result, the new test can pass while the described passthrough path still hangs or formats DONE incorrectly; cover relaySseWithFailedTail() or a server-level passthrough stream so the focused regression protects the changed behavior.
AGENTS.md reference: AGENTS.md:L228-L230
Useful? React with 👍 / 👎.
| doneSeen = true; | ||
| cleanup(); | ||
| controller.close(); | ||
| reader.cancel("Responses terminal event received").catch(() => {}); |
There was a problem hiding this comment.
If a gateway writes response.completed and another SSE block in the same upstream chunk, this branch closes only after controller.enqueue(value) has already forwarded the whole chunk, so post-terminal deltas or metadata can reach the client before the synthetic [DONE]. The terminal-aware relays elsewhere use an SSE frame boundary to forward through the terminal block and drop later blocks; this path should do the same before cancelling the reader.
Useful? React with 👍 / 👎.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Closing this as superseded: upstream |
Problem
When a provider (e.g. DeepSeek Flash v4) ends a Responses passthrough stream with a terminal event such as
response.completed/response.failed/response.incompletebut does not senddata: [DONE], the relay keeps the client SSE stream open indefinitely. The client shows the assistant "thinking" forever and never receives the final response.Fix
In
src/server/relay.ts, after forwarding a terminal event, emit[DONE]and close the client stream so the client can finalize the turn.Tests
tests/passthrough-abort.test.tscoverage for a stream that ends withresponse.completedand no[DONE].bun test tests/passthrough-abort.test.tspasses.bun run typecheckpass.Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I fixed all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
Bug Fixes
Tests