fix(server): bound client-facing SSE frame retention - #1241
Conversation
📝 WalkthroughWalkthroughThe PR adds bounded byte-based SSE framing. The shared framer handles fragmented delimiters, frame limits, finalization, and disposal. HTTP relay inspection and WebSocket bridging use it for terminal-event processing and oversized-frame handling. ChangesBounded SSE framing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant UpstreamSSE
participant BoundedSseFrameBuffer
participant RelayOrWebSocketBridge
UpstreamSSE->>BoundedSseFrameBuffer: send raw byte chunks
BoundedSseFrameBuffer->>RelayOrWebSocketBridge: emit complete bounded frames
RelayOrWebSocketBridge->>RelayOrWebSocketBridge: decode and process terminal events
UpstreamSSE->>BoundedSseFrameBuffer: complete stream
BoundedSseFrameBuffer->>RelayOrWebSocketBridge: return final tail
RelayOrWebSocketBridge->>BoundedSseFrameBuffer: dispose framing state
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/relay.ts (1)
129-140: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winThe
[DONE]forwarding is correct but the split push obscures it. Consolidate the decision.I traced both branches and the byte output is right:
- If
responsesTerminalisfalseand the payload is[DONE], Line 131 pushes the frame and Line 134 does not.- If
responsesTerminalistrueand the payload is[DONE], Line 131 skips and Line 134 pushes.So
[DONE]is emitted exactly once in both orders, and non-[DONE]frames after the terminal are dropped. That matches the contract stated on Lines 112-114.The problem is that two
output.push(frame.block, frame.delimiter)calls sit under inverted conditions inside the same loop. A reader scanning this will reasonably suspect a duplicate emission, and a future edit to either condition can introduce one. Express the forwarding rule once.Note that the decoder use here is sound:
frame.blockalways holds complete frame bytes, so the one-shotdecoder.decodeon Line 130 cannot split a multibyte sequence. That is the property the byte framer buys, and it is worth keeping intact in any rewrite.♻️ Proposed consolidation
for (const frame of frames) { const payload = sseDataPayload(decoder.decode(frame.block)); - if (!responsesTerminal) output.push(frame.block, frame.delimiter); - if (payload === "[DONE]") { - done = true; - if (responsesTerminal) output.push(frame.block, frame.delimiter); - continue; - } - if (!responsesTerminal && payload && terminalStatusFromSsePayload(payload)) { - responsesTerminal = true; - } + const isDone = payload === "[DONE]"; + // Relay every frame through the first Responses terminal. After that + // terminal, relay only the [DONE] sentinel and drop everything else. + if (!responsesTerminal || isDone) output.push(frame.block, frame.delimiter); + if (isDone) { + done = true; + continue; + } + if (!responsesTerminal && payload && terminalStatusFromSsePayload(payload)) { + responsesTerminal = true; + } }🤖 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 129 - 140, Consolidate the two conditional output.push calls in the frames loop into one forwarding decision: emit the current frame when responsesTerminal is false or payload is "[DONE]". Preserve the existing terminal-state transition, dropping non-"[DONE]" frames after terminal status, and keep the one-shot decoder.decode(frame.block) behavior unchanged.
🤖 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 153-156: Make the boundary’s finish() method total by handling
SseFrameTooLargeError from framer.finish() and returning an empty Uint8Array,
while preserving the existing disposed/terminal guard and successful output.
Update the finish() symbol in the relay boundary; do not modify ws-bridge.ts
because its existing try/catch already handles the exception.
In `@src/server/sse-frame-buffer.ts`:
- Around line 18-22: Update delimiterLengthAt to return number | undefined
instead of the redundant number | 0 | undefined annotation, and add a doc
comment documenting the three runtime states: positive delimiter length, 0 when
no delimiter starts at the index, and undefined when more bytes are required to
decide.
- Around line 89-102: Update ensureCapacity to explicitly reject requiredBytes
values greater than this.maxFrameBytes before entering the growth loop, failing
loudly rather than allowing an unbounded loop. Preserve the existing
capacity-growth behavior for valid requests.
- Around line 117-123: Update takeCandidate() to retain this.candidate after
slicing and reset only candidateBytes, allowing the geometrically grown buffer
to be reused across frames. Preserve clear() and dispose() behavior so they
continue releasing the buffer at end of stream.
- Around line 147-175: Add a per-call frame-count cap to the feed loop in the
SSE frame buffer, deriving it from maxFrameBytes so delimiter-only input cannot
produce unbounded frames; preserve existing empty-frame passthrough behavior and
ensure the cap is enforced before adding another frame. In the delimiter
handling around takeCandidate and copyRange, reuse shared constants for the
common delimiter byte sequences instead of allocating identical delimiter arrays
for every frame.
In `@src/server/ws-bridge.ts`:
- Around line 275-284: Update the cleanup in the finally block of the
reader-processing flow to call reader.cancel() before clearing ws.data.cancel,
ensuring exceptions from framer.feed or framer.finish release the upstream
stream. Preserve the existing cancellation behavior and cleanup for all other
termination paths.
---
Outside diff comments:
In `@src/server/relay.ts`:
- Around line 129-140: Consolidate the two conditional output.push calls in the
frames loop into one forwarding decision: emit the current frame when
responsesTerminal is false or payload is "[DONE]". Preserve the existing
terminal-state transition, dropping non-"[DONE]" frames after terminal status,
and keep the one-shot decoder.decode(frame.block) behavior unchanged.
🪄 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: 2a4d65d0-e434-41a6-a1b0-0045aa49b012
📒 Files selected for processing (4)
src/server/relay.tssrc/server/sse-frame-buffer.tssrc/server/ws-bridge.tstests/sse-client-frame-bounds.test.ts
![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: 7ba0225c8d
ℹ️ 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".
|
@coderabbitai review |
|
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Maintainer takeover review complete on final head Audited the superseding path for framing correctness, memory/resource bounds, delimiter and UTF-8 fragmentation, terminal ordering, failure-tail behavior, WebSocket cleanup/cancellation, hostile delimiter amplification, and reviewer/bot findings. Valid findings are fixed with focused regressions; the intentional per-frame candidate release remains as a bounded-memory trade-off and CodeRabbit withdrew that allocation-reuse suggestion. Closeout also documents the 4 MiB client-facing Responses SSE frame cap and HTTP/WebSocket failure semantics in the canonical proxy reference plus ja/ko/ru/zh-cn locale copies. All inline CodeRabbit/Codex threads are resolved. Final gates on this head: React Doctor ✅, Cross-platform CI ✅, CodeRabbit status ✅. PR is mergeable and remains open for the requested human reviews. |
1 participant
Summary
Maintainer follow-up from the full audit of #1095.
#1095's DeepSeek-specific terminal-repair state machine is no longer the right implementation:
devalready restored progressive DeepSeek Responses streaming in0b8e608c06a4a81ba676019ee99b10b6e201dcd1by relying on the existing terminal-event boundary instead of synthesizing delayed success.During the takeover review, that replacement path exposed one independent availability/security gap: client-facing HTTP and WebSocket SSE framing could retain an arbitrarily large unterminated upstream frame.
This PR:
BoundedSseFrameBufferwith a 4 MiB hard per-frame cap;Security / resource rationale
A malicious or broken upstream could previously keep sending bytes without an SSE frame delimiter and grow the client-output buffer without a hard bound. Inspection already had a 4 MiB frame cap, but the output fanout did not. This aligns the client-facing paths with that existing memory envelope.
The frame-count guard also prevents delimiter-only input from amplifying a bounded byte chunk into an unbounded number of frame objects. Candidate storage is intentionally released after each completed frame rather than reused, so a rare multi-MiB frame cannot pin its peak allocation for the rest of a long-lived response.
Relation to #1095
dev:0b8e608c06a4a81ba676019ee99b10b6e201dcd1.Verification