Skip to content

Reduce stream memory allocations and fix native Chat completion handling - #4389

Closed
olddonkey wants to merge 7 commits into
lidge-jun:devfrom
olddonkey:codex/memory-stream-optimizations
Closed

Reduce stream memory allocations and fix native Chat completion handling#4389
olddonkey wants to merge 7 commits into
lidge-jun:devfrom
olddonkey:codex/memory-stream-optimizations

Conversation

@olddonkey

@olddonkey olddonkey commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Summary

Repeated byte measurements allocate full UTF-8 copies of requests and growing stream buffers, amplifying allocation pressure during concurrent turns. Native Chat can also wait indefinitely after response headers, return partial success after cancellation, or collect CRLF responses as empty output.

  • Count request/response sizes without measurement arrays; accumulate compaction and Chat text sizes incrementally while preserving Unicode coercion, admission limits, and ownership.
  • Scan SSE input incrementally and encode only delivered output after admission. Release reservations on failed enqueue and stop rewrites after cancellation/disposal.
  • Apply the configured native Chat stall timeout to meaningful progress, pausing it under downstream backpressure. Return typed stall errors and cancellation status instead of partial success; preserve accepted terminals and support LF/CRLF/multiline data.
  • Update architecture notes and server configuration documentation in English and seven translations. Add focused regressions for allocation volume, Unicode, size boundaries, cancellation, terminal precedence, and cleanup.

For 1,024 fragments of 1 KiB, compaction measurement arrays fall from a cumulative 1,025 MiB to zero. Native SSE and rewrite encoding volume now equals delivered output. These are synthetic allocation measurements, not measured production RSS savings.

Verification

Final head: 176cbbd2ec1f492c74174eecc54435af2d7cb8db, rebased onto dev at 01e7d746d22e5a12aa35950c4035a3c5dee0bae6 (merge of #4422).

  • Rebase: the six source commits apply unchanged (git range-diff reports = for all five code patches); the docs commit resolved one textual conflict in structure/data-planes/inbound-compat.md where dev added "Chat conversation identity forwarding" at the same position, keeping both sections.
  • Added test(chat): pin caller abort precedence over a pending native stall in tests/responses/chat-completions-endpoint.test.ts, answering the review question about stall vs. cancel precedence: an abort during the stall wait is the only outcome, onCancel fires once, upstream is cancelled once, and no upstream_stall_timeout terminal surfaces after the deadline elapses.
  • Typecheck, privacy scan, and structure:check pass on the final head.
  • Final-head focused suite: 517 passed, 0 failed across the ten files below (the eight from the previous head plus tests/responses/chat-conversation-affinity.test.ts for the neighbouring dev change), then 13 passed for the stall|abort|cancel name pattern including the new regression.
./node_modules/.bin/bun run scripts/test.ts --parallel=1 \
  tests/adapters/bridge-nonstreaming-terminal.test.ts \
  tests/adapters/translator-budget.test.ts tests/lib/debug.test.ts \
  tests/responses/chat-completions-endpoint.test.ts \
  tests/responses/openai-responses-passthrough.test.ts \
  tests/responses/sse-payload-rewrite.test.ts \
  tests/usage/request-decompress.test.ts \
  tests/responses/chat-conversation-affinity.test.ts \
  tests/lab/core-lab-boundary.test.ts tests/ci-workflows/structure-ssot.test.ts
  • Import-connected coverage on the final head (--changed=upstream/dev, 951 of 1213 files): 20,380 passed, 0 failed, 36 skipped in a single --parallel=1 run. The earlier help/registry 14-vs-15 baseline failures no longer reproduce on current dev.
  • Bun 1.4.2 still segfaults in tests/providers/command-code-workspace-cache.test.ts under grouped --parallel=2 execution (Segmentation fault at address 0x10, reported by Bun as its own bug). That file was excluded from the grouped run and executed alone: 4 passed, 0 failed. Every other selected file ran in the grouped run, so no file was omitted.
  • Hosted cross-platform CI remains pending on this head. No live provider load/RSS test was performed.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults. Scope review found no authentication, credential-routing, workflow, dependency, or release changes; privacy scanning passes.

Review readiness checklist

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

  • All CI tests are green on my local testing.

  • I pushed my PR to the latest dev commit.

  • I resolved all correct Codex and CodeRabbit findings.

  • My PR is ready for review.

Summary by CodeRabbit

  • New Features

    • Native Chat now supports configurable stall timeouts based on meaningful upstream progress.
    • Stall timeouts report upstream_stall_timeout errors, while cancelled requests return cancellation errors instead of successful partial responses.
    • Native Chat streaming supports LF, CRLF, multiline SSE data, and progress from reasoning, refusals, tool updates, and completion events.
  • Documentation

    • Updated server configuration guidance and lifecycle details across supported languages.
    • Added documentation for request-copy and stream-buffer accounting behavior.

@coderabbitai

coderabbitai Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview 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: Advanced

Run ID: 620ab7d7-5bb3-4b43-8974-a88ac9ebf046

📥 Commits

Reviewing files that changed from the base of the PR and between 01e7d74 and 176cbbd.

📒 Files selected for processing (44)
  • docs-site/src/content/docs/fr/reference/configuration/server.md
  • docs-site/src/content/docs/ja/reference/configuration/server.md
  • docs-site/src/content/docs/ko/reference/configuration/server.md
  • docs-site/src/content/docs/reference/configuration/server.md
  • docs-site/src/content/docs/ru/reference/configuration/server.md
  • docs-site/src/content/docs/tr/reference/configuration/server.md
  • docs-site/src/content/docs/zh-cn/reference/configuration/server.md
  • docs-site/src/content/docs/zh-tw/reference/configuration/server.md
  • src/adapters/anthropic.ts
  • src/adapters/google.ts
  • src/adapters/openai-chat.ts
  • src/adapters/openai-responses.ts
  • src/chat/outbound.ts
  • src/lib/admission.ts
  • src/lib/translator-budget.ts
  • src/server/chat-completions.ts
  • src/server/chat-native-sse.ts
  • src/server/chat-native.ts
  • src/server/request-decompress.ts
  • src/server/sse-payload-rewrite.ts
  • structure/adapters/registry.md
  • structure/catalog.md
  • structure/clients/claude-desktop.md
  • structure/clients/integrations.md
  • structure/data-planes/images.md
  • structure/data-planes/inbound-compat.md
  • structure/gui-and-management-api.md
  • structure/ops/docs-and-release.md
  • structure/ops/service-and-sidecars.md
  • structure/overview.md
  • structure/providers/chat-compat.md
  • structure/providers/cursor.md
  • structure/providers/xai-grok.md
  • structure/runtime.md
  • structure/subagents.md
  • structure/transports/inventory.md
  • structure/transports/responses.md
  • structure/transports/streaming-health.md
  • tests/adapters/translator-budget.test.ts
  • tests/lib/debug.test.ts
  • tests/responses/chat-completions-endpoint.test.ts
  • tests/responses/openai-responses-passthrough.test.ts
  • tests/responses/sse-payload-rewrite.test.ts
  • tests/usage/request-decompress.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

Changes

Streaming accounting and native Chat lifecycle

Layer / File(s) Summary
Incremental UTF-8 and budget accounting
src/adapters/*, src/lib/*, src/server/request-decompress.ts, src/server/chat-completions.ts, src/adapters/openai-responses.ts, tests/adapters/*, tests/lib/*, tests/usage/*, tests/responses/openai-responses-passthrough.test.ts, structure/transports/responses.md
UTF-8 byte measurements now use Buffer.byteLength. Responses parsing tracks incremental collector sizes, surrogate pairs, request copies, and event batches without repeatedly encoding complete strings.
Shared SSE block buffering and collection
src/server/sse-payload-rewrite.ts, src/chat/outbound.ts, tests/responses/sse-payload-rewrite.test.ts
SSE processing now preserves partial frames and delimiters, supports bare data fields, reserves budget before output, and releases buffered state during completion, errors, and cancellation.
Native Chat stall and cancellation flow
src/server/chat-native-sse.ts, src/server/chat-native.ts, tests/responses/chat-completions-endpoint.test.ts, structure/data-planes/inbound-compat.md, structure/transports/streaming-health.md
Native Chat now applies stallTimeoutSec to meaningful upstream progress, pauses the deadline during downstream backpressure, reports upstream_stall_timeout, and returns cancellation errors before a terminal result.
Configuration and transport documentation
docs-site/src/content/docs/*/reference/configuration/server.md, structure/**/*.md
Localized configuration pages and structure documents describe the request-copy, stream-buffer, native Chat timeout, SSE, and cancellation contracts.

Priority: ➖ Normal

Estimated code review effort: 5 (Critical) | ~90 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant NativeChat
  participant NativeChatSSE
  participant Upstream
  Client->>NativeChat: send Chat request
  NativeChat->>NativeChatSSE: configure stallTimeoutSec
  NativeChatSSE->>Upstream: read SSE blocks
  Upstream-->>NativeChatSSE: meaningful progress or terminal event
  NativeChatSSE-->>Client: stream normalized Chat frames
  NativeChatSSE-->>NativeChat: report stall or cancellation status
  NativeChat-->>Client: return 502, 499, or completed response
Loading

Merge Risk: ⚪ Minimal · up to 176cb

No concrete current-head issue remains from the finalized review; the change is mergeable after normal checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 40.74% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 27 functions across 18 files. (26 skipped… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the two primary changes: reducing stream memory allocations and fixing native Chat completion handling.
Full details: Docstring Coverage

Explanation

Docstring coverage is 40.74% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 27 functions across 18 files. (26 skipped: 26 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 66 / 80

설명

이 PR은 두 줄을 한번에 들고 옵니다. 첫째, 요청/스트림 크기 계산이 TextEncoder().encode(...)로 전체 복사본을 만들지 않게 줄입니다. 둘째, native Chat이 헤더만 받고 본문이 멈추거나, 취소 후 부분 성공을 내보내거나, CRLF SSE를 빈 결과로 읽던 구멍을 막습니다. 지금 dev HEAD ec065aa0csrc/adapters/openai-responses.ts compaction 누적, src/chat/outbound.ts collect 경로, src/server/chat-native-sse.ts / chat-native.ts는 아직 “의미 있는 progress” 기준의 stall과 취소 우선 종료가 약합니다. #4341이 방금 올린 truncated tool incomplete 계약과는 이웃하지만, 이번 변경은 accounting·Chat 완료 쪽에 가깝습니다.

코드 요지는 Buffer.byteLength / appendedUtf8Bytes로 증분 청구하고, SSE scan은 전달된 출력만 encode하며, enqueue 실패 시 reservation을 풀고, 취소/dispose 뒤에는 rewrite를 멈춥니다. native Chat은 stallTimeoutSec를 본문 progress에 적용하고, downstream backpressure 중에는 타이머를 일시 정지합니다. stall은 typed upstream_stall_timeout(stream 에러 또는 non-stream 502)이고, 터미널 전 취소는 부분 성공 대신 cancellation입니다. LF/CRLF/multiline data도 받습니다. 문서·structure·테스트가 큽니다(+1453/-268). 본문은 focused 511 pass와, baseline으로 help 14 vs registry 15 실패가 이 PR과 무관하다고 적습니다. 그 baseline은 #4386이 고칩니다. draft이고 full suite/hosted CI는 미완입니다.

라인 - 이게 무슨 문제다

src/adapters/openai-responses.ts appendedUtf8Bytes - surrogate pair를 끊지 않고 증분 청구하는 방향이 맞습니다. #4388 image-loop의 argument 누적과 비슷한 문제를 다른 헬퍼로 풉니다. 두 PR이 같이 들어가면 공통 UTF-8 append 헬퍼를 후속으로 모을지 정하세요.

src/server/chat-native-sse.ts / chat-native.ts stall + cancel - “keepalive/role-only/usage-only는 progress가 아니다”는 계약이 문서와 맞습니다. slow-client backpressure에서 타이머를 멈추는 것도 좋습니다. 다만 stall과 cancel이 동시에 나면 어느 쪽이 이기는지 테스트가 고정하는지 확인하세요. 본문은 terminal precedence를 테스트에 넣었다고 하니, 그 케이스 이름을 CI 로그에서 보면 됩니다.

src/server/sse-payload-rewrite.ts / src/chat/outbound.ts - 프레임 소유권을 rewrite 버퍼와 collector가 나눠 갖습니다. 취소 후 rewrite 중단·failed enqueue release는 메모리 누수 방지에 중요합니다. 실패 경로에서 reservation이 항상 release되는지는 테스트가 강제하는지 한 번 더 보세요.

structure 다수 파일 + 8개 언어 server.md - 계약 문장을 넓게 복제합니다. #4387/#4388과 동시에 올리면 conflict 납니다. 동작 PR과 문서 PR을 나누는 편이 랜딩이 쉽습니다.

본문이 인정한 한계 - full suite/hosted CI 미완, live RSS 측정 없음, baseline 14/15 실패는 이 PR 밖. synthetic allocation 숫자(1025 MiB → 0)는 실험실 수치이지 생산 RSS 절감이 아닙니다.

메인테이너의 판단이 필요한 지점

  • 큰 accounting+Chat 패치를 한 번에 머지할지, stall/cancel 계약과 encoder 증분을 두 PR로 나눌지
  • structure/번역 문서를 같은 열차에 둘지, 후속으로 미룰지
  • #4386을 먼저 넣어 baseline을 가린 뒤 이 PR CI를 읽을지
  • draft 해제 전 full suite를 필수로 볼지, focused+hosted로 충분할지

너의 추천
방향은 dev의 스트림 안전·메모리 절약과 맞습니다. 다만 지금 상태에서는 #4386을 먼저 머지해 help/registry baseline을 없애고, 이 PR은 hosted CI와 chat-native/sse/outbound focused 테스트가 초록일 때 draft 해제하세요. structure 복제가 부담이면 문서 파일은 follow-up으로 빼도 됩니다. #4341 incomplete-tool 계약과 충돌하는 주장은 본문에 없으니, 랜딩 순서만 주의하면 됩니다.

이 댓글은 grok-bot이 작성했습니다

@github-actions

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@github-actions

github-actions Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

✅ READY

  • all PR quality gates passed; the review readiness checklist is complete.

Review readiness checklist

  • ✅ All CI tests are green on my local testing.
  • ✅ I pushed my PR to the latest dev commit.
  • ✅ I resolved all correct Codex and CodeRabbit findings.
  • ✅ My PR is ready for review.

4/4 boxes ticked.

This pull request is already Ready for Review.
The review-ready label marks this PR as ready; review automation runs independently.
Maintainers: @lidge-jun @Ingwannu

olddonkey and others added 7 commits September 12, 2026 09:16
A caller abort that lands while the native Chat stall clock is waiting on
upstream silence must be the only reported outcome. The stream closes as a
cancellation, onCancel fires once, upstream is cancelled once, and no
upstream_stall_timeout terminal surfaces even after the deadline it was
racing has elapsed.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@olddonkey
olddonkey force-pushed the codex/memory-stream-optimizations branch from 08f7824 to 176cbbd Compare September 12, 2026 16:29
@olddonkey
olddonkey marked this pull request as ready for review September 12, 2026 16:32
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 12, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
🔒 Security Review Completed 2026-09-12T16:39:30.441702Z 176cbbd Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@lidge-jun

Copy link
Copy Markdown
Owner

Closing as landed: reducing stream memory allocations and fixing native Chat completion handling is on dev, merged inside lane C's cumulative tip #4487 (merge commit 55bb9f3, verified as an ancestor of origin/dev).

Your authorship is preserved by a Co-authored-by trailer in the landed commit itself rather than only in the pull request body, so it counts on your contributor graph.

The carry folded the review findings already on this pull request and added regression coverage where the lane found a gap. If you think something from this branch did not make it to dev, say so and I will reopen.

@lidge-jun lidge-jun closed this Sep 13, 2026
S0RYUASUKA pushed a commit to S0RYUASUKA/opencodex that referenced this pull request Sep 13, 2026
… doc [skip ci]

Carry of lidge-jun#4389 by olddonkey onto current dev.

structure/transports/responses.md is exactly at its 600-line budget on dev, so
the two new owning sections this change needs could not be added there at all:
bun run structure:check failed at 630 lines. structure/AGENTS.md says an
over-budget doc is split along a topic boundary with its own manifest entry, and
that a grace.oversizeDocs entry is only for a split already planned — so this
takes the split rather than parking a promise nobody would keep.

Request-copy and stream-buffer accounting are one topic and now live in
structure/transports/byte-accounting.md with its own manifest entry.
responses.md is byte-identical to dev again. Both documents sit in
structure/transports/, so the seventeen cross-references this change adds keep
their relative prefix and only change file name and are otherwise untouched.

structure/gui-and-management-api.md was 602 lines for the same reason. Its
cross-reference is dropped instead: the dashboard and management API own neither
the request-decompression nor the SSE-rewrite path, which makes it the least
load-bearing of the seventeen. The other sixteen are unchanged.

Co-authored-by: Olddonkey <22208754+olddonkey@users.noreply.github.com>
S0RYUASUKA pushed a commit to S0RYUASUKA/opencodex that referenced this pull request Sep 13, 2026
…evin-restore-tool-names

Lane C of the contributor carry train: OCG DeepSeek timeline system instructions (lidge-jun#4438 by Yongzhaooo), stream allocation reduction and native Chat completion handling (lidge-jun#4389 by olddonkey), and restored namespaced Devin tool identities (lidge-jun#4457 by jeongjin0).

Cross-platform CI run 34744712476 concluded success on 9b30902, the exact head merged here, and it covers every link because the lane is cumulative. lidge-jun#4473 and lidge-jun#4485 carry no ci check of their own; their head commits carry [skip ci] by design, under the owner-authorized tip-only CI economy for this batch.

All three source authors are credited by Co-authored-by trailers in the landed commits.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants