Skip to content

[WRONG BRANCH] bound: limit Codex WS upstream frame and queued-response bytes to prevent OOM - #266

Closed
luvs01 wants to merge 1 commit into
mainfrom
codex/fix-codex-ws-upstream-memory-vulnerability
Closed

[WRONG BRANCH] bound: limit Codex WS upstream frame and queued-response bytes to prevent OOM#266
luvs01 wants to merge 1 commit into
mainfrom
codex/fix-codex-ws-upstream-memory-vulnerability

Conversation

@luvs01

@luvs01 luvs01 commented Aug 13, 2026

Copy link
Copy Markdown
Owner

Motivation

  • The Codex WebSocket upstream path previously accepted, parsed, encoded, and queued every incoming WS message unbounded, which can let an upstream or model emit large/rapid frames and exhaust process memory.

Description

  • Add byte limits and a queuing strategy by introducing MAX_CODEX_WS_FRAME_BYTES (4 MiB) and MAX_CODEX_WS_QUEUE_BYTES (8 MiB) and using ByteLengthQueuingStrategy for the returned ReadableStream so buffered bytes are bounded.
  • Reject oversized WS frames before JSON parsing by cheaply checking JS string length and UTF-8 encoded byte length, and disconnect the upstream when a frame exceeds the per-frame cap.
  • Enforce backpressure by checking controller.desiredSize before enqueueing an SSE-encoded frame and close the socket with an error when the buffered queue would be exceeded via a failStream helper that errors the stream and closes the socket.
  • Add regression tests in tests/ws-upstream.test.ts that assert oversized frames are rejected and that an upstream producing enough data to fill the bounded queue is disconnected, plus expose the new constants for testing and assertions.

Testing

  • Ran targeted unit tests: bun test tests/ws-upstream.test.ts -t 'rejects an oversized|disconnects an upstream', which passed.
  • Ran typecheck with bun run typecheck and the full test suite with bun run test, both of which passed in the repository test runner environment; the environment-local single bun test tests/ws-upstream.test.ts run initially hit unrelated runtime differences in the external Bun binary (timer API and node:zlib zstd symbol) but the repository's test runner executed the full suite successfully.
  • Ran bun run privacy:scan which returned clean/green for the changes.

Codex Task

Summary by CodeRabbit

  • Bug Fixes

    • Added safeguards against oversized WebSocket frames.
    • Prevented excessive buffering by enforcing a response queue limit.
    • Improved stream failure handling by closing affected WebSocket connections.
  • Performance

    • Improved backpressure handling to measure buffered data by byte size.
    • Optimized server-sent event frame preparation before queueing.
  • Tests

    • Added coverage for oversized frames and response queues exceeding their limits.

@coderabbitai

coderabbitai Bot commented Aug 13, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 82201ec2-5d23-4bbd-8a93-445e4fbcaf08

📥 Commits

Reviewing files that changed from the base of the PR and between 1193075 and 97ef99d.

📒 Files selected for processing (2)
  • src/server/responses/ws-upstream.ts
  • tests/ws-upstream.test.ts

📝 Walkthrough

Walkthrough

The WebSocket response stream now enforces 4 MiB frame and 8 MiB queue limits. It encodes SSE output into byte buffers, rejects limit violations, errors the stream, and closes the WebSocket. Tests cover frame rejection and queue overflow.

Changes

WebSocket stream limits

Layer / File(s) Summary
Frame and queue limit enforcement
src/server/responses/ws-upstream.ts
Lines 21–26 export 4 MiB frame and 8 MiB queue limits. Lines 100–106 centralize stream failure handling. Lines 154 and 167–199 apply byte-based queueing, UTF-8 frame validation, bounded buffering, and pre-sized SSE encoding.
Limit enforcement tests
tests/ws-upstream.test.ts
Lines 3–8 import the exported limits. Lines 255–266 test oversized frame rejection. Lines 268–285 test queue overflow and WebSocket closure.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Mergeability Score: ⚪ Minimal · up to 97ef9

The change bounds upstream WebSocket frame and queued-response memory usage while preserving the existing flow, with regression tests and repository checks reported as passing; no actionable merge-blocking risk remains.

Sequence Diagram(s)

sequenceDiagram
  participant UpstreamWebSocket
  participant ResponseReadableStream
  participant SSEByteBuffer
  UpstreamWebSocket->>ResponseReadableStream: receive upstream frame
  ResponseReadableStream->>SSEByteBuffer: encode accepted SSE event
  SSEByteBuffer->>ResponseReadableStream: enqueue bytes within queue limit
  ResponseReadableStream->>UpstreamWebSocket: fail stream and close on limit violation
Loading

Possibly related PRs

  • luvs01/opencodex#100: Adds bounded response buffering and size-limit handling in another streaming component.

Suggested reviewers: wibias, ingwannu, lidge-jun

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the primary change: bounding Codex WebSocket frame and queued-response bytes to prevent out-of-memory failures.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/fix-codex-ws-upstream-memory-vulnerability

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

@github-actions

Copy link
Copy Markdown

Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Aug 13, 2026
@github-actions github-actions Bot changed the title bound: limit Codex WS upstream frame and queued-response bytes to prevent OOM [WRONG BRANCH] bound: limit Codex WS upstream frame and queued-response bytes to prevent OOM Aug 13, 2026
@github-actions

github-actions Bot commented Aug 13, 2026

Copy link
Copy Markdown

⏳ DRAFT

  • wrong target branch (main); retarget to dev.

What to do

  • Retarget this PR to dev — all contributions go to dev.

Its title has been prefixed with [WRONG BRANCH].
This pull request was already a draft. Its draft status will be preserved after every issue above is resolved.

@github-actions
github-actions Bot marked this pull request as draft August 13, 2026 00:57

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 97ef99dc54

ℹ️ 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".

Comment on lines +189 to +190
const availableBytes = controller.desiredSize ?? 0;
if (frameBytes > availableBytes) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Bound buffering after the production tee

On the default non-Windows passthrough route, src/server/responses/core.ts:2639-2695 tees this body and continuously drains the inspection branch even when the client branch is stalled. Because a tee pulls whenever either branch wants data, that drain replenishes desiredSize here while the unread bytes accumulate without a byte limit in nativeBody; a slow client combined with a rapidly emitting upstream can therefore still grow memory until OOM without triggering this check. Apply the byte budget after the tee, or route this transport through the single-reader bounded relay, and cover the production passthrough path rather than only reading the returned Response directly.

Useful? React with 👍 / 👎.

luvs01 commented Aug 13, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 13, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

aardvark bug Something isn't working codex

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant