perf(chat): reduce stream allocations and bound native Chat completion - #4485
Conversation
(cherry picked from commit 324008b)
(cherry picked from commit 4eafb3f)
(cherry picked from commit 930c916)
(cherry picked from commit b7d1330)
(cherry picked from commit 6e16274)
(cherry picked from commit d7f27e3)
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> (cherry picked from commit 176cbbd)
… doc [skip ci] Carry of #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>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Deterministic PR hygiene checks passed. |
리뷰 · 우선순위 69 / 80설명 하는 일은 크게 두 갈래입니다. 첫째, 요청·응답·스트림 버퍼 크기를 잴 때마다 UTF-8 전체 복사본을 새로 만들지 않게 바꿉니다. 현재 tip과의 관계는 이렇습니다. tip에는 이미 #4468 거절(refusal) 장부 수정과 #4471 단위 마감 문서가 들어 있고, 방금 #4484로 문서 쪽도 중요합니다. tip의 검증은 이 변경과 부모가 만지는 열한 파일 위주 경로/심볼 - 베이스가 아직 열린 #4473이라 tip에 단독 머지하면 스택이 꼬입니다. #4473 머지 후 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e2343326a
ℹ️ 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".
| return new TextEncoder().encode(value).byteLength; | ||
| // Keep TextEncoder's runtime coercion for legacy callers outside the string-typed contract. | ||
| // Template coercion rejects Symbols; String(value) would silently accept them. | ||
| return Buffer.byteLength(typeof value === "string" ? value : value === undefined ? "" : `${value}`, "utf8"); |
There was a problem hiding this comment.
Preserve TextEncoder sizing on older Bun runtimes
When the proxy runs under Bun 1.2.14, Buffer.byteLength does not match TextEncoder for unpaired UTF-16 surrogates: for example, "\ud800x\udc00" measures as 5 bytes here instead of the 7 bytes actually emitted as UTF-8, causing the newly added retained UTF-8 sizing test to fail. User-controlled JSON can produce these strings through escaped surrogates, so retainedUtf8Bytes can undercount admission limits and truncateRetainedUtf8 can return values exceeding its byte cap; use an allocation-free code-unit counter or a compatible fallback instead of delegating this contract to Bun's Buffer.byteLength.
AGENTS.md reference: src/AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
|
Landed on dev as part of lane C. Closing manually: this branch's head is a verified ancestor of origin/dev after tip #4487 merged as 55bb9f3, but GitHub did not close it automatically because the chain was retargeted at merge time. Tip-only CI record, owner-authorized for this batch: Cross-platform CI run 34744712476 concluded success on 9b30902, the exact merged head, and the lane is cumulative so that run executed this branch's content as a strict subset. This pull request's own ci check never ran; its head commit carries [skip ci] by design. |
…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.
Summary
Carry of #4389 by @olddonkey onto current
dev. Stacked on #4473; this PR targets that branch, so its own diff is just this change.Repeated byte measurements allocated full UTF-8 copies of requests and growing stream buffers, which amplified allocation pressure during concurrent turns. Native Chat could also wait indefinitely after response headers, return partial success after cancellation, or collect CRLF responses as empty output.
data.For 1,024 fragments of 1 KiB, compaction measurement arrays fall from a cumulative 1,025 MiB to zero, and native SSE and rewrite encoding volume now equals delivered output. Those are synthetic allocation measurements, not measured production RSS savings, and no live provider load test was run here.
Conflicts resolved
The source branch was 266 commits behind
dev, so three resolutions were judged against current behavior rather than the branch's original base.src/chat/outbound.ts—collectChatCompletion.devadded service-tier capture to the same loop this change rewrites fromreplaceRetainedtoappendRetainedwith aretainedBytesledger. The rewrite is taken wholesale anddev's three service-tier deltas are re-applied onto it: theserviceTierdeclaration, theparsed.service_tiercapture besideparsed.usage, and theservice_tierspread on the returned completion. Verified by diffing the result against both sides — the only delta against the source branch isdev's own.dev's refusal-ledger change from fix(chat): stop refusal bookkeeping from failing refusal-free turns #4468 is in the streaming function and merged without conflict.src/adapters/openai-chat.ts. Positional only:devinserted theparallel_tool_callsblock directly above, so git anchored the hunk onto a duplicate of the debug block.dev's code is kept and the one real change,new TextEncoder().encode(bodyJson).lengthtoBuffer.byteLength(bodyJson, "utf8"), is applied to the live site. The other two byte-counting sites in the file merged cleanly.devadded an intro paragraph at the same anchor (both sides kept,devfirst), ordevadded a table row above a row this change rewrites (row kept, rewrite applied).The 600-line structure budget
structure/transports/responses.mdis exactly at its 600-line budget ondev, so this change could not add its two new owning sections there at all —structure:checkfailed at 630 lines.structure/AGENTS.mdsays an over-budget doc is split along a topic boundary with its own manifest entry, and that agrace.oversizeDocsentry is only for a split already planned, so this takes the split. Request-copy and stream-buffer accounting are one topic and now live instructure/transports/byte-accounting.md;responses.mdis byte-identical todevagain. Both documents sit instructure/transports/, so the seventeen cross-references keep their relative prefix and change only the file name.structure/gui-and-management-api.mdhit 602 lines for the same reason, and 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.Maintainers should know this affects other lanes:
responses.mdhaving zero headroom ondevmeans any branch adding a line to it failsstructure:check.Attribution, per
AGENTS.mdandCREDITS.md— the landing commit must carry this trailer, and the squash message must not drop it:This is the middle link of lane C in
devlog/_plan/260913_contributor_carry_train/, stacked on #4473 with #4457 above it. Retarget todevonce #4473 lands.Verification
Source head
176cbbd2ec1f492c74174eecc54435af2d7cb8dbcarried onto #4473's head6645ccb06.bun teston the eleven files this change and its parent touch — 536 pass, 0 fail:tests/adapters/bridge-nonstreaming-terminal,tests/adapters/translator-budget,tests/lib/debug,tests/responses/chat-completions-endpoint,tests/responses/openai-responses-passthrough,tests/responses/sse-payload-rewrite,tests/usage/request-decompress,tests/responses/chat-conversation-affinity,tests/lab/core-lab-boundary,tests/ci-workflows/structure-ssot,tests/adapters/openai/openai-chat-system-order.bun run typecheck— clean.bun run structure:check— passes after the split above.bun run structure:indexregeneratedINDEX.md.bun run privacy:scan— passes.[skip ci]on its head commit, so its owncicheck does not run and the lane tip run covers it.Because this is a performance change, the question worth reviewing is whether any allocation removal changes observable behavior on a truncated or folded stream. It does not. The behavior that does change is the lifecycle and correctness policy this PR advertises, and the two are worth separating.
The allocation removals are behavior-preserving. The shared block scanner delimits on the same four blank-line shapes as
dev'snextSseBlockregex (\n\n,\r\n\r\n,\r\n\n,\n\r\n), and CR-only still never delimits on either side; byte-split events, several events per chunk, Unicode and surrogate pairs split acrossappendall produce identical blocks and leftovers. The incremental byte subtraction is exact, because SSE delimiters are CR/LF only, so.lengthequals their UTF-8 size; remaining bytes matchedBuffer.byteLength(tail)after every consume. Admission overlap is unchanged, so a buffer already atmaxTurnBytesstill cannot consume — fail-closed as before. An unterminated final event is still not forwarded: the HTTP rewrite flushes the raw tail exactly asdevdid, and native Chat still fails it closed asupstream_sse_unterminated. Incremental compaction counting matchesdev's full recount, including a surrogate pair split across two deltas; a 14,641-sequence sweep over empty, ASCII, CJK, emoji, lone high/low surrogates and NFD input found zero divergences.snapshot || doneText || deltasprecedence and terminal ownership are untouched.Buffer.byteLengthandTextEncoderdiffer only on non-string input, where the former throws and the latter coerces. Every changed call site is already guarded byJSON.stringify,typeof === "string", orTextDecoder.decode, so that hazard is not reachable.Three real behavior changes, all intentional. Each follows from a bullet above rather than from removing an allocation:
devread the next block before checking the abort, so a second event in the same chunk could still reach a client that had gone away.data:with an empty value is forwarded instead of failing.devranJSON.parse("")on it and killed the stream withupstream_sse_invalid.One leniency is given up.
data:lines within a single event are now joined per the EventSource contract, which is what makes CRLF and a JSON payload split acrossdata:lines work. A colonlessdataline in the same event as a JSON line used to be ignored and the JSON parsed; joined, it no longer parses. This is spec-conformant and uncommon on Chat wires, and it is deliberate.Scope check on the collector.
collectChatCompletionchanged framing too, but it is not a user-visible surface: both production call sites feed it opencodex's own generated SSE —nativeChatSseoutput inchat-native.tsandresponsesSseToChatCompletionsSseoutput inchat-completions.ts— never third-party SSE. Verified against a cleanorigin/devcheckout: a malformed upstream block returns the identical502 upstream returned malformed SSE JSONon both, because the relay sits in front of the collector anddev's relay already joineddata:lines.Audit method: three independent read-only reviewers, one per area, each comparing this branch against
origin/devandrefs/carry/4389with executable probes rather than reading alone. Two furthercollectChatCompletiontest files outside the source PR's focused set were found and run:tests/responses/sse-unspaced-data-fields.test.tsandtests/responses/chat-json-sse-fallback.test.ts, plustests/responses/compaction-progress.test.ts— 58 pass, 0 fail.Checklist
privacy:scanpasses.