fix(responses): close passthrough streams at terminal events - #942
fix(responses): close passthrough streams at terminal events#942L14nY1Wang wants to merge 1 commit into
Conversation
|
✅ PR quality gates passed This pull request now targets The title was left unchanged. The pull request has been marked ready for review again. |
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughWindows passthrough SSE now uses terminal-aware eager relaying. Relays detect protocol terminals, emit ChangesSSE terminal handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ResponsesCore
participant Relay
participant SSEBoundary
participant Client
ResponsesCore->>Relay: select terminal-aware SSE relay
Relay->>SSEBoundary: inspect streamed chunks
SSEBoundary-->>Relay: detect response.completed
Relay->>Client: emit terminal event and data: [DONE]
Relay->>ResponsesCore: cancel upstream reader
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
4f34411 to
5254e45
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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-eager.ts`:
- Around line 235-256: Make both relay emission paths frame-aware: in
src/server/relay-eager.ts lines 235-256 and src/server/relay.ts lines 117-139,
buffer complete SSE blocks, emit only through the first terminal event block,
and discard trailing frames from the same chunk. Append the [DONE] sentinel only
when the terminal-boundary output lacks an actual [DONE] data event, not based
on substring matching. Add focused coverage in tests/relay-eager.test.ts and
tests/sse-failed-tail.test.ts for terminal-plus-trailing data and non-sentinel
content containing the sentinel text.
🪄 Autofix (Beta)
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: 68108577-8de9-4f3a-b5cc-66a5b4806bad
📒 Files selected for processing (9)
src/lib/bun-stream-caps.tssrc/server/index.tssrc/server/relay-eager.tssrc/server/relay.tssrc/server/responses/core.tsstructure/04_transports-and-sidecars.mdtests/passthrough-abort.test.tstests/relay-eager.test.tstests/sse-failed-tail.test.ts
5254e45 to
eed07d2
Compare
|
Addressed the review feedback in eed07d2. Both relays now share a frame-aware SSE output boundary that emits only complete blocks through the first Responses terminal, drops coalesced post-terminal frames, and recognizes |
![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: 5254e45073
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
eed07d2 to
d0ab275
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/responses/core.ts`:
- Around line 1878-1880: Extend tests in relay-eager.test.ts with a regression
case that creates the inspector without onTerminal, cancels the client, and
delivers a terminal event while the upstream remains open. Assert onClientCancel
is not called, no synthetic terminal is emitted, and onDone occurs before the
drain timeout, exercising the sawTerminal callback passed from the response
flow.
🪄 Autofix (Beta)
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: 96e181ad-23ac-44b7-88b1-c11ecd6dc28e
📒 Files selected for processing (9)
src/lib/bun-stream-caps.tssrc/server/index.tssrc/server/relay-eager.tssrc/server/relay.tssrc/server/responses/core.tsstructure/04_transports-and-sidecars.mdtests/passthrough-abort.test.tstests/relay-eager.test.tstests/sse-failed-tail.test.ts
d0ab275 to
88d60c3
Compare
|
Carried onto the review stack as #953 (stack 3/3), unmodified. Your commits were taken with Verified on the stack: This PR stays open until #953 lands. If a maintainer prefers to take yours directly instead, that path is unaffected — the stack commits get dropped and this one merges. Once #953 merges I'll close this as carried, with the credit already in the commit history rather than in a comment. Stack: #951 (plan, base Thanks for the fix. |
2 participants
Summary
response.completed,response.failed, orresponse.incompleteevent arrives, instead of waiting for the upstream HTTP connection to reach EOF.data: [DONE]sentinel when a compatible gateway omits it, then cancel the retained upstream reader so Codex leaves its thinking state promptly.Verification
bun test tests/sse-failed-tail.test.ts tests/passthrough-abort.test.ts tests/consume-for-inspection-cancel.test.ts tests/relay-eager.test.ts tests/bun-stream-caps.test.ts— 84 passed, 0 failed.bun run typecheck— passed.bun run privacy:scan— passed.git diff --check— passed.Bun.serve({ port: 0 })failed with environment-levelEADDRINUSE. No focused assertion failed.Checklist
Summary by CodeRabbit
data: [DONE]completion markers when terminal responses omit them.