fix(images): bound bridge iteration buffering - #4478
Conversation
Carry #4388 from 9518b52 onto the lane B chain above the #4381 carry. A hidden image/video iteration collected adapter events into an unbounded array. Both the runTurn emit path and ordinary stream collection kept every non-heartbeat event until the scanner ran, so an oversized tool argument or a long text stream grew one iteration without limit even though src/lib/translator-budget.ts already defines the turn and call-argument ceilings. Each iteration now owns an independent budget: 32 MiB of serialized retained events including array framing, and 2 MiB of UTF-8 arguments per open call, with surrogate pairs that straddle two deltas counted once. Overflow aborts the producer and surfaces translation_buffer_limit instead of a truncated success. Folded in the Codex review finding left on the source head, which was not addressed there. createAdapterEventQueue merges adjacent text and thinking deltas into chunks of up to 64 KiB while no reader is waiting, so a synchronous producer's token-granular deltas survive as a handful of strings. Charging each pre-merge envelope billed about 31 bytes for every one-character delta, so roughly 1 MiB of retained output crossed the 32 MiB turn limit and aborted a healthy turn. push now reports whether it merged the event into its buffered tail, and a merged delta is charged only for the payload it appended. The return value is additive: every other caller passes push as a void callback and is unaffected. That accounting is deliberately conservative in one place. JSON escaping is per character, so a merged delta costs its quoted form minus the quotes; a surrogate pair split across two deltas over-counts by eight bytes and never under-counts. Verification on this carry: bun test tests/images/loop.test.ts tests/adapters/run-turn-queue.test.ts (74 pass, 0 fail), bun run typecheck, bun run structure:check, bun run privacy:scan. The new coalescing regression was driven red against the unfixed emit path first, where it reproduced the spurious translation_buffer_limit response. Local full suite: NOT RUN. Hosted CI on the lane tip is the suite proof. Co-authored-by: luvs01 <27862058+luvs01@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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1453a3838
ℹ️ 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".
| // Check at emission, before even a synchronous producer can fill the queue. push | ||
| // reports whether it merged this delta into the buffered tail, which is what the | ||
| // iteration actually retains once the consumer drains it. | ||
| iterationBudget.retain(event, queue.push(event)); |
There was a problem hiding this comment.
When a runTurn adapter emits a single oversized event, queue.push(event) makes that event queue-visible before retain detects that it exceeds the 32 MiB limit. The catch then closes rather than clears the queue, so queue.stream() drains the rejected event into events before collectionError is thrown; an arbitrarily large tool-argument or text event can therefore remain retained and exhaust memory despite this change's bound. Make admission transactional or pre-charge the event before inserting it, adjusting the charge afterward if the queue coalesces it.
Useful? React with 👍 / 👎.
리뷰 · 우선순위 73 / 80설명 고침은 두 층이다. 우선순위 73은 DoS급 메모리 경로를 막고, 변경이 media loop+queue로 좁으며 tip과 독립이기 때문이다. types/config 분할과 무관하다.
심볼 원본 #4388 / 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
…-availability Lane B of the contributor carry train: truncated-terminal search failure (#4381 by luvs01), bounded bridge iteration buffering (#4388 by luvs01), and distinguishing model availability from auth failure (#4460 by AgenticLab-SH). Tip-only CI by owner authorization for this batch. Cross-platform CI run 34744793611 concluded success on bd39d3b, the exact head merged here, and it covers every link because the lane is cumulative. #4472 and #4478 carry no ci check of their own. Lane S (#4477, carrying #4447) is deliberately not part of this lane and is held for maintainer security review.
|
Merged into dev as part of lane B, under the tip-only CI economy the repository owner authorized for this batch.
Landed via merge commit 2af30c2, verified as an ancestor of origin/dev. |
1 participant
Summary
Carries #4388 by @luvs01 (source head
9518b5281, fork branchagent/media-loop-bounds-20260912) and folds in the Codex review finding that was still open on that head.A hidden image or video iteration collected adapter events into an unbounded array. Both the
runTurnemit path and ordinary stream collection retained every non-heartbeat event until the scanner ran, so one oversized tool argument or a long text stream could grow a single iteration without limit — even thoughsrc/lib/translator-budget.tsalready defines the turn and per-call ceilings the rest of the translator honours. Each iteration now owns an independent budget of 32 MiB of serialized retained events (including array framing) and 2 MiB of UTF-8 arguments per open call, counting a surrogate pair that straddles two deltas once. Overflow aborts the producer and surfacestranslation_buffer_limitinstead of a truncated success.The folded review finding
Codex flagged
src/images/loop.tsfor charging events the queue then threw away, and that finding was never addressed on the source branch.createAdapterEventQueuemerges adjacent text and thinking deltas into chunks of up to 64 KiB whenever no reader is waiting, which is the normal case for a synchronous producer. Charging every pre-merge envelope billed roughly 31 bytes for each one-character delta, so about 1 MiB of retained output crossed the 32 MiB turn limit and aborted a healthy turn — the failure this PR exists to prevent, arriving about thirty times too early.pushnow returns whether it merged the event into its buffered tail, and a merged delta is charged only for the payload it appended. The return value is purely additive:core.ts,compact.ts,collaboration.tsandencrypted-payload.tsall passpushas avoidcallback and are unaffected.The accounting is deliberately conservative in one spot. JSON escaping is per character, so a merged delta costs its quoted form minus the quotes; a surrogate pair split across two deltas over-counts by eight bytes and never under-counts.
tests/images/loop.test.tsgains a regression that emits 1.2 M one-character deltas, whose envelopes exceed the turn limit while their merged form is about 1 MiB. It was driven red against the unfixed emit path first, where it reproduced exactly the spurioustranslation_buffer_limitresponse.This is the middle link of lane B in the contributor carry train, based on the #4381 carry. The lane tip is the #4460 carry and its CI run is this lane's suite proof; this branch's head carries
[skip ci]deliberately.Verification
bun test tests/images/loop.test.ts tests/adapters/run-turn-queue.test.ts— 74 pass, 0 fail, 2242 assertions.bun run typecheck— passed.bun run structure:check— passed.bun run privacy:scan— passed.Checklist
Co-authored-by: luvs01 27862058+luvs01@users.noreply.github.com