Skip to content

fix(request): linear SSE buffering with pre-append size check - #418

Merged
ndycode merged 2 commits into
mainfrom
fix/sse-buffer-linear
Apr 18, 2026
Merged

fix(request): linear SSE buffering with pre-append size check#418
ndycode merged 2 commits into
mainfrom
fix/sse-buffer-linear

Conversation

@ndycode

@ndycode ndycode commented Apr 17, 2026

Copy link
Copy Markdown
Owner

Summary

Addresses deep-audit finding REQ-HIGH-03: convertSseToJson in lib/request/response-handler.ts buffered the entire SSE stream via repeated fullText += decoder.decode(...). That is O(n) per append on V8, producing O(n^2) total work for large streams, and the MAX_SSE_SIZE (10 MB) cap was enforced AFTER the append, so peak memory briefly held chunk + 10 MB before throwing.

Fix

  • Accumulate decoded chunks in a string[] and join('') once at the end (linear total work).
  • Track a running totalSize counter.
  • Check totalSize + decoded.length > MAX_SSE_SIZE BEFORE appending each chunk, so the cap is enforced pre-allocation and peak memory is bounded to the cap rather than cap + chunk.

Tests

New dedicated regression file test/response-handler-sse-buffer.test.ts (3 tests), isolated from the existing 860-line response-handler.test.ts to avoid whole-file reformatting noise in the diff:

  • Accumulates many ~128 KB chunks summing just under the 10 MB cap and parses successfully (exercises the multi-chunk accumulation path).
  • Asserts the throw fires on the FIRST chunk that would push past the cap (pre-append), and that no further reads happen after the throw.
  • Asserts a single oversize first chunk is rejected before it is retained in any buffer.

Verification

  • npm test -- response-handler — 56 passed (was 53; +3 regression tests)
  • npm run typecheck — clean
  • npm run lint — clean

Constraints observed

  • No as any, @ts-ignore, or @ts-expect-error.
  • Changes are confined to lib/request/response-handler.ts and a new co-located response-handler-scoped test file.
  • No amend, no force-push.

note: greptile review for oc-chatgpt-multi-auth. cite files like lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.

Greptile Summary

the pr correctly replaces O(n²) string concatenation with an array-then-join pattern and enforces the 10 MB cap before each append. the follow-up commit (afd1fff) also switched from decoded.length (utf-16 code units) to Buffer.byteLength(decoded, "utf8"), addressing the prior review comment — though value.byteLength would be simpler, avoids re-encoding, and is slightly tighter at multi-byte chunk boundaries.

Confidence Score: 5/5

safe to merge; both remaining findings are P2 style suggestions with no correctness impact

the core fix is correct — linear accumulation, pre-append guard, and proper utf-8 byte counting all work. the Buffer.byteLength vs value.byteLength difference is a minor efficiency/elegance nit, not a bug; totals are equivalent over the full stream. four targeted regression tests cover all three original scenarios plus the utf-8 edge case. no security, data-loss, or concurrency issues.

no files require special attention

Important Files Changed

Filename Overview
lib/request/response-handler.ts linear accumulation and pre-append size guard are correct; Buffer.byteLength(decoded, "utf8") re-encodes needlessly — value.byteLength is simpler and tighter at chunk boundaries
test/response-handler-sse-buffer.test.ts 4 targeted regression tests covering accumulation, pre-append throw, single-oversized-chunk, and utf-8 byte counting; the ascii-rationale comment in buildChunkedReader is stale after the utf-8 fix

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[reader.read] --> B{done?}
    B -- yes --> G[chunks.join]
    B -- no --> C[decoder.decode value stream:true]
    C --> D[decodedBytes = Buffer.byteLength decoded utf8]
    D --> E{totalSize + decodedBytes > MAX_SSE_SIZE?}
    E -- yes --> F[throw + reader.cancel]
    E -- no --> H[chunks.push decoded\ntotalSize += decodedBytes]
    H --> A
    G --> I[parseSseStream fullText]
    I --> J{finalResponse?}
    J -- no --> K[return plain text Response]
    J -- yes --> L[return JSON Response]
Loading

Fix All in Codex

Prompt To Fix All With AI
This is a comment left during a code review.
Path: lib/request/response-handler.ts
Line: 792-800

Comment:
**unnecessary re-encode; use `value.byteLength` directly**

`Buffer.byteLength(decoded, "utf8")` re-encodes the already-decoded string back to UTF-8 just to count bytes. `value` is a `Uint8Array` whose `.byteLength` is exactly the raw wire-byte count — no extra allocation or encoding pass needed. additionally, when `TextDecoder` running in streaming mode buffers 1–3 trailing bytes of a split multi-byte sequence, `Buffer.byteLength(decoded, "utf8")` excludes those bytes from the current chunk (they appear in the next `decoded` call instead); `value.byteLength` counts them immediately, making the guard slightly tighter at chunk boundaries.

```suggestion
			const decodedBytes = value.byteLength;
			// Pre-append size check: reject before allocating/retaining the chunk
			// alongside the accumulated buffer. This bounds peak memory to the
			// cap rather than cap + chunk.
			if (totalSize + decodedBytes > MAX_SSE_SIZE) {
				throw new Error(`SSE response exceeds ${MAX_SSE_SIZE} bytes limit`);
			}
			chunks.push(decoded);
			totalSize += decodedBytes;
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: test/response-handler-sse-buffer.test.ts
Line: 28-30

Comment:
**stale comment rationale after switching to `Buffer.byteLength`**

the comment says ascii is chosen because `decoder.decode()` returns a string of the same length as the byte buffer — that was relevant when tracking `decoded.length`. now that the implementation uses `Buffer.byteLength(decoded, "utf8")` (or ideally `value.byteLength`), the byte count is exact for all inputs, so ascii is no longer required for determinism. the 4th test in this file already uses emoji. the comment is now misleading; consider trimming or updating it to match the actual accounting.

How can I resolve this? If you propose a fix, please make it concise.

Reviews (2): Last reviewed commit: "fix(request): count utf-8 bytes in SSE s..." | Re-trigger Greptile

Addresses REQ-HIGH-03 deep-audit finding.

Previous: fullText += decoder.decode(...) caused O(n^2) string concatenation in convertSseToJson; MAX_SSE_SIZE check ran AFTER append so memory briefly held chunk + 10MB before throwing.

Fix: accumulate chunks in string[] array; track running size; check size BEFORE append; final join() once at end. Linear time, bounded memory, size-check enforced pre-allocation.

Test asserts pre-append throw when next chunk would exceed cap.
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@coderabbitai

coderabbitai Bot commented Apr 17, 2026

Copy link
Copy Markdown
Contributor

Warning

Rate limit exceeded

@ndycode has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 56 minutes and 28 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 56 minutes and 28 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 648bddd6-5632-454a-ada5-187e748e1959

📥 Commits

Reviewing files that changed from the base of the PR and between 3f1c1fe and afd1fff.

📒 Files selected for processing (2)
  • lib/request/response-handler.ts
  • test/response-handler-sse-buffer.test.ts
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/sse-buffer-linear
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch fix/sse-buffer-linear

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Comment thread lib/request/response-handler.ts Outdated
Responds to PR #418 review feedback.

The pre-append SSE size guard used decoded.length, which counts UTF-16 code
units rather than bytes. Multi-byte UTF-8 chunks (emoji, many CJK chars) could
therefore exceed MAX_SSE_SIZE without tripping the guard at the right point.

Use Buffer.byteLength(decoded, 'utf8') for both the pre-append check and the
running total, and add a regression test covering a multi-byte payload.
@ndycode
ndycode merged commit e0a8a34 into main Apr 18, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant