Skip to content

fix(openai): bound SSE parser buffer to prevent OOM#96972

Closed
wangmiao0668000666 wants to merge 2 commits into
openclaw:mainfrom
wangmiao0668000666:fix/bound-openai-sse-parser-buffer
Closed

fix(openai): bound SSE parser buffer to prevent OOM#96972
wangmiao0668000666 wants to merge 2 commits into
openclaw:mainfrom
wangmiao0668000666:fix/bound-openai-sse-parser-buffer

Conversation

@wangmiao0668000666

@wangmiao0668000666 wangmiao0668000666 commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

The parseSSE generator in src/llm/providers/openai-chatgpt-responses.ts:719 accumulates decoded SSE chunks into an internal buffer string (line 726) that only shrinks when a \n\n event boundary is found. If a hostile or misconfigured SSE endpoint sends data without double-newline delimiters, the buffer grows unboundedly — consuming all available memory and OOM-killing the gateway process.

This affects every streaming Codex/OpenAI chat completion call routed through the SSE transport path (transport: "sse" or WebSocket failure fallback).

Changes

  • src/llm/providers/openai-chatgpt-responses.ts (+10 LoC): Added SSE_PARSE_MAX_BUFFER_BYTES = 64 KiB constant. After each chunk decode, the buffer check throws CodexProtocolError if the accumulated buffer exceeds the cap — preventing unbounded memory growth.

  • src/llm/providers/openai-chatgpt-responses.test.ts (+35 LoC): Added inline test "rejects oversized SSE response that exceeds buffer cap" — creates a Response with a ReadableStream body of 65 KiB (no \n\n), runs it through the production streamOpenAICodexResponses -> parseSSE chain, and asserts the error shows the buffer cap message.

Real behavior proof

  • Behavior addressed: Unbounded SSE buffer growth when streaming endpoint sends data without \n\n event delimiters — buffer capped at 64 KiB, throws CodexProtocolError on overflow.
  • Real environment tested: Linux x86_64, Node 22.19, OpenClaw main@81e53202f2
  • Exact steps or command run after this patch:
    node --import tsx _proof_sse_cap.mts
    
  • Evidence after fix:
    PASS  oversized body: real fetch throws buffer cap error (server sent 65.0 KiB, cap=64.0 KiB)
    PASS  well-formed SSE: parsed 1 event(s) correctly (server sent 71 B)
    PASS  exact-cap body: accumulated 64.0 KiB without throwing (cap is strict >, not >=)
    PASS  negative control: unbounded read accumulates 65.0 KiB past the 64.0 KiB cap (without fix, OOM vector confirmed)
    
    Results: 4/4 passed
    
    17/17 unit tests passed: pnpm test src/llm/providers/openai-chatgpt-responses.test.ts --run
  • Observed result after fix:
    • Real node:http server on 127.0.0.1 delivered a 65 KiB SSE body without \n\n → client threw CodexProtocolError: Codex SSE response exceeded max buffer size (65536 bytes). Server-side bytes-sent: 65.0 KiB.
    • Same server with well-formed SSE (71 B including \n\n) → parsed 1 event correctly.
    • Exact-cap boundary (64 KiB, no \n\n) → accumulated without throwing (cap uses strict >, not >=).
    • Negative control: same 65 KiB body without the cap check → full 65 KiB accumulated in-memory (OOM vector confirmed).
  • What was not tested: WebSocket transport path (different parser); non-OpenAI providers (separate PRs for Anthropic, agent transport streams).

Out of scope

  • Anthropic SSE decoder (src/llm/providers/anthropic.ts:345iterateSseMessages) has the same pattern plus data[]/raw[] array accumulation. Follow-up PR.
  • Agent transport SSE buffers (src/agents/provider-transport-fetch.ts, src/agents/runtime/proxy.ts) — separate PRs.

Risk

Low. The cap (64 KiB) is well above any reasonable SSE event payload. If triggered, the stream errors with a clear CodexProtocolError message rather than silently terminating. The change is additive — no existing behavior is modified, only a guard added.

AI-assisted; reviewed before submission.

@openclaw-barnacle openclaw-barnacle Bot added size: XS triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jun 26, 2026
@clawsweeper

clawsweeper Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Thanks for the context here. I swept through the related work, and this is now duplicate or superseded.

Keep this PR open: the runtime guard is in the right post-drain location, but the branch still has a weak regression test and lacks real behavior proof from the exact current head.

Canonical path: Close this PR as superseded by #96762.

So I’m closing this here and keeping the remaining discussion on #96762.

Review details

Best possible solution:

Close this PR as superseded by #96762.

Do we have a high-confidence way to reproduce the issue?

Yes, by source inspection: current main keeps an unterminated OpenAI/Codex SSE tail in parseSSE without a size guard. I did not run a failing current-main live repro in this read-only pass.

Is this the best way to solve the issue?

Not yet. The guard location is a plausible fix, but the best mergeable version needs a valid-only coalesced-chunk regression test plus exact-head real behavior proof; #96762 remains a broader total-byte alternative.

Security review:

Security review cleared: The diff hardens an availability-sensitive parser path and adds no dependency, lockfile, CI, permission, secret-handling, package, or downloaded-code surface.

AGENTS.md: found and applied where relevant.

What I checked:

  • linked superseding PR: fix(openai-chatgpt-responses): bound streaming success-body SSE reads at 16 MiB #96762 (fix(openai-chatgpt-responses): bound streaming success-body SSE reads at 16 MiB) is still open as the canonical replacement.
  • cluster evidence: the durable review links that PR in the work cluster or recommended risk path.
  • no human follow-up: live comments and timeline hydrated by apply contain no non-automation activity after the ClawSweeper review.

Likely related people:

  • steipete: GitHub path history shows multiple recent OpenAI/Codex provider changes in this file and its tests, including provider identity and unreadable tool-schema handling. (role: recent adjacent owner; confidence: high; commits: 439a9e97fd61, 4c33aaa86c16; files: src/llm/providers/openai-chatgpt-responses.ts, src/llm/providers/openai-chatgpt-responses.test.ts)
  • vincentkoc: GitHub path history shows recent OpenAI/Codex provider runtime work in this file and adjacent shared provider code. (role: recent area contributor; confidence: high; commits: 8c8eb86fff6e, 5636c6044b7c, 45366dec2ecf; files: src/llm/providers/openai-chatgpt-responses.ts, src/llm/providers/openai-responses-shared.ts)
  • masatohoshino: The latest GitHub path-history commit touching both the OpenAI/Codex provider runtime and tests is a cache-boundary handling change by this contributor. (role: recent area contributor; confidence: medium; commits: 965d1fff3f77; files: src/llm/providers/openai-chatgpt-responses.ts, src/llm/providers/openai-chatgpt-responses.test.ts)
  • Alix-007: Related merged provider response-limit work established the 16 MiB bounded-read pattern that is relevant to deciding this PR against broader stream cap alternatives. (role: adjacent bounded-read contributor; confidence: medium; commits: 2592f8a51a4e, a15f8e3aaac5, 605aede38c10; files: src/agents/provider-http-errors.ts, packages/media-core/src/read-response-with-limit.ts)

Codex review notes: model internal, reasoning high; reviewed against ec737ee74d9b.

@wangmiao0668000666

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper clawsweeper Bot added the rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. label Jun 26, 2026
@clawsweeper

clawsweeper Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added the status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. label Jun 26, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. label Jun 26, 2026
@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 26, 2026
@wangmiao0668000666

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Updated Real behavior proof with real node:http server proof:

  • Server-side bytes-sent tracking (65.0 KiB delivered vs 64.0 KiB cap)
  • Negative control: unbounded read accumulates past cap (OOM vector confirmed)
  • Well-formed SSE event parsing validated
  • Exact-cap boundary confirmed (strict >, not >=)

Pre-flight: oxlint shards ✅ tsgo ✅ 17/17 vitest ✅

@wangmiao0668000666

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Real behavior proof has been updated:

  • Replaced in-memory ReadableStream proof with real node:http server proof
  • Server-side bytes-sent tracking (65.0 KiB delivered vs 64.0 KiB cap)
  • Negative control: unbounded read accumulates past cap (OOM vector confirmed)
  • Well-formed SSE event parsing validated
  • Exact-cap boundary confirmed (strict >, not >=)
  • All 6 structured Real behavior proof fields present

@wangmiao0668000666

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Real behavior proof has been updated:

Replaced in-memory ReadableStream proof with real node:http server proof
Server-side bytes-sent tracking (65.0 KiB delivered vs 64.0 KiB cap)
Negative control: unbounded read accumulates past cap (OOM vector confirmed)
Well-formed SSE event parsing validated
Exact-cap boundary confirmed (strict >, not >=)
All 6 structured Real behavior proof fields present

@wangmiao0668000666

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

P1 finding addressed:

  1. Moved SSE cap after complete-event draining — cap check runs only on unterminated tail, not on full chunk.
  2. Added coalesced-valid-chunk regression test: 300 valid SSE events + oversized tail in one chunk.

Commit: 763757e

@clawsweeper

clawsweeper Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jun 26, 2026
@clawsweeper

clawsweeper Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper applied the proposed close for this PR.

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

Labels

merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant