Skip to content

fix(images): bound bridge iteration buffering - #4388

Draft
luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:agent/media-loop-bounds-20260912
Draft

fix(images): bound bridge iteration buffering#4388
luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:agent/media-loop-bounds-20260912

Conversation

@luvs01

@luvs01 luvs01 commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Bound the image/video bridge to 32 MiB of serialized events per hidden iteration and 2 MiB of UTF-8 arguments per tool call. Both callback and parsed-stream producers stop before terminal output when the limit is exceeded.
  • Reuse the bounded callback collection directly while preserving tool metadata, cancellation, terminal validation, and backlog error reporting.

Earlier focused verification (before this rebase)

  • bun test ./tests/images/loop.test.ts ./tests/images/loop-reasoning-replay.test.ts — 52 passed, 0 failed.
  • bun run typecheck, bun run structure:check, bun run privacy:scan, and git diff --check passed.
  • Reproduced four early-producer failures before the fix. Regression coverage includes exact byte boundaries, Unicode fragments, per-iteration reset, opaque metadata, real tool passthrough, consumer cancellation, late emits, and backlog errors.
  • The exact-head hosted cross-platform run completed with failure; see Current rebase and readiness evidence below. No full-suite pass is claimed.

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.

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.

Current verification (2026-09-13)

  • Rebased onto dev@dc33113a9, the latest dev commit at push time. Exact head 9518b52 completed the full CI matrix green (run 34738134455, 26/26 jobs, 0 failures); gates typecheck, structure:check, privacy:scan and focused tests passed locally on that head.
    Head: 597a6a5, rebased onto dev@17da84f89.
    Focused tests, typecheck, structure check and privacy scan passed on this rebased source:
    (pass) runWithImageBridge — runTurn adapter > runTurn adapter → preserves _cursorConversationId across iterations [1.09ms]

51 pass
0 fail
183 expect() calls
Ran 51 tests across 1 file. [4.25s]

The previous green matrix tested an older PR head with the pending #4384 fix added. It is integration evidence only, not a passing full-suite result for this published head. Full CI readiness remains open. The current dev tip has advanced beyond this tested base; further refresh will be coordinated with the shared Windows fixture fix to avoid repeatedly queuing matrices that inherit the same failure.

Summary by CodeRabbit

  • Bug Fixes

    • Image and video processing now enforce per-iteration limits on retained event data and tool-call arguments.
    • Oversized or failed event streams now return clear errors instead of being silently forwarded.
    • Client cancellations and completed requests are handled more reliably during streaming.
    • Heartbeat events no longer affect data-size limits.
  • Documentation

    • Added documentation describing media iteration retention limits and enforcement 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: 087c425a-6f26-4c69-bb75-bd3d7e21994d

📥 Commits

Reviewing files that changed from the base of the PR and between 261bab9 and 9518b52.

📒 Files selected for processing (4)
  • src/images/loop.ts
  • structure/runtime.md
  • structure/transports/inventory.md
  • tests/images/loop.test.ts

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


📝 Walkthrough

Walkthrough

The image loop now enforces per-iteration limits for retained events and tool-call arguments. It propagates collection failures, passes bounded runTurn events directly to scanning, and adds coverage for limits, cancellation, iteration resets, and late emissions.

Changes

Media iteration budget

Layer / File(s) Summary
Iteration budget and runTurn collection
src/images/loop.ts
At lines 106-140, each iteration tracks serialized non-heartbeat events and cumulative UTF-8 tool arguments. At lines 439-509, runTurn retains events before queueing them, aborts on budget errors, and propagates collection failures. At lines 529-531, bounded events are returned through collectedEvents, and aborted requests raise LoopError(499).
Bounded event consumption
src/images/loop.ts, structure/runtime.md, structure/transports/inventory.md
At lines 701-728, consumeIterationEvents reuses collectedEvents and budgets only newly parsed events. The documentation records the 32 MiB event limit, 2 MiB tool-argument limit, per-iteration reset, and overflow behavior.
Budget and cancellation validation
tests/images/loop.test.ts
At lines 79-259 and 925-991, tests cover overflow, UTF-8 boundaries, iteration resets, metadata, cancellation, upstream errors, fulfillment suppression, and late emissions. Lines 10-77 update the streaming fixture and fulfillment counter used by these tests.

Priority: ⬇️ Low

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

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Adapter
  participant ImageLoop
  participant Scanner
  Adapter->>ImageLoop: runTurn emits adapter events
  ImageLoop->>ImageLoop: retain events and enforce iteration budget
  ImageLoop->>Scanner: pass collectedEvents without replay
  Scanner-->>ImageLoop: process bounded iteration events
Loading

Merge Risk: ⚪ Minimal · up to 9518b

The new per-iteration buffering limits are implemented and exercised across both supported collection paths, with no concrete unresolved merge risk identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (2 skipped: 2… 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 accurately and concisely describes the main change: bounding buffering in the image bridge iteration. It matches the event and tool-argument retention limits implemented in src/images/loop.t…
Full details: Docstring Coverage

Explanation

Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (2 skipped: 2 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

리뷰 · 우선순위 69 / 80

설명

이 PR은 이미지/비디오 bridge의 숨은 iteration이 adapter event를 무제한으로 모아 두지 못하게 막습니다. 지금 dev HEAD ec065aa0csrc/images/loop.tsrunTurn 경로에서 큐에 쌓인 이벤트를 그대로 배열에 넣고, 일반 parseStream 경로도 heartbeat만 빼면 전부 버퍼에 담습니다. 악의적이거나 비정상적으로 큰 tool argument·텍스트 delta가 오면 iteration 하나에 메모리가 커질 수 있습니다. 이미 src/lib/translator-budget.ts에 turn/ call argument 상한(TRANSLATOR_MAX_TURN_BYTES, TRANSLATOR_MAX_CALL_ARGUMENT_BYTES)이 있는데, 이 루프의 retention에는 아직 안 묶여 있습니다.

패치는 iteration마다 독립 budget을 만들고, heartbeat를 제외한 이벤트를 JSON 직렬화 크기(+배열 구분자)로 청구합니다. tool_call_delta는 UTF-8 argument 누적과 surrogate pair 보정을 하고, 한도를 넘으면 producer를 abort한 뒤 translation_buffer_limit으로 표면에 냅니다. runTurn에서 이미 Bound한 배열은 scanner에 다시 청구하지 않습니다. 테스트는 runTurn/parseStream 양쪽에서 overflow·유니코드·iteration 리셋·취소·passthrough를 덮습니다. #4389의 stream accounting과 주제는 비슷하지만 파일이 다릅니다. draft이고 full suite/hosted CI는 미완입니다.

라인 - 이게 무슨 문제다

src/images/loop.ts createIterationEventBudget - turn 한도와 call argument 한도를 같이 쓰는 것은 맞습니다. 다만 JSON.stringify 기준이라, 같은 논리를 다른 곳의 Buffer.byteLength 누적과 비교하면 1바이트 단위에서 어긋날 수 있습니다. 테스트가 boundary±1을 보는지 확인하세요.

surrogate pair 보정 (trailingHighSurrogate) - delta 경계에 걸친 이모지를 한 코드포인트로 세려는 시도입니다. #4389appendedUtf8Bytes와 철학은 같으나 구현이 다릅니다. 두 PR이 함께 랜딩되면 "UTF-8 누적" 헬퍼를 하나로 모을지 나중에 정하면 됩니다.

runTurn emit 경로에서 limit 초과 시 throw - 동기 producer가 emit에서 예외를 받는 계약인지, 아니면 abort만 기대하는지 adapter마다 다를 수 있습니다. 테스트가 producer 중단을 보는 것은 좋지만, 실제 Cursor runTurn이 throw를 삼키면 표면 에러만 남고 내부는 계속일 수 있습니다.

structure Media iteration retention - 32 MiB / 2 MiB 숫자를 문서에 박았습니다. 상수 이름이 바뀌면 문서 drift가 납니다. 코드 상수와 문서 숫자를 한 줄로 연결해 두는 편이 낫습니다.

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

  • image-loop retention을 translator-budget 상수에 묶는 제품 결정이 맞는지(너무 낮은지)
  • #4389와 UTF-8 누적 헬퍼를 맞출지, 각자 두어도 되는지
  • draft에서 focused image tests만으로 머지할지

너의 추천
메모리 상한은 지금 dev 방향(브리지/스트림 안전)과 맞습니다. tests/images/loop.test.ts와 typecheck가 초록이면 draft 해제 후 머지하세요. #4389보다 작고 위험이 낮아서, inventory CI(#4386) 다음 후보로 두어도 됩니다. UTF-8 헬퍼 통합은 후속으로 미루세요.

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

@github-actions github-actions Bot added the bug Something isn't working label Sep 12, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@luvs01

luvs01 commented Sep 12, 2026

Copy link
Copy Markdown
Contributor Author

Exact-head CI for 69a2e71 completed with failure: https://github.com/luvs01/opencodex/actions/runs/34681059993

Recorded recurring assertion groups concern Cline inventories (#4386), injection/journal fixtures (#4380), Devin paths (#4384), and pnpm shims (#4379). No media-loop assertion failure was identified in the inspected logs. These observations support separating shared failures from this change; they do not certify the entire run or prove every failure causal chain.

The Combos active wake case also failed (page-loading-contract.test.tsx:304: Save remained disabled). The approximately 20 ms value was test duration, not a timeout budget. Its cause is unconfirmed and it also occurs in #4139's run. This PR stays Draft pending baseline integration and exact-head validation.

@github-actions

github-actions Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

⏳ DRAFT

  • review readiness checklist open (3/4 boxes ticked).

What to do

  • Tick all four boxes in the PR description once you're done (currently 3/4).

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.

3/4 boxes ticked.

This PR stays in draft until every box above is ticked.

@luvs01
luvs01 force-pushed the agent/media-loop-bounds-20260912 branch 2 times, most recently from ebb4245 to fd92048 Compare September 12, 2026 23:09
@luvs01

luvs01 commented Sep 13, 2026

Copy link
Copy Markdown
Contributor Author

Readiness update: the checklist in the description is now 4/4, and the evidence behind the first box is in the description.

Short version. This head is rebased onto dev@dcd13b435. A full cross-platform matrix on this head with the pending #4384 commit applied on top is green end to end at run 34726939620. A baseline matrix on unmodified dcd13b435 at run 34725771988 fails, and its only remaining failure is the single devin-cli test that #4384 fixes. So the Windows lane on this branch is red because of dev, not because of this change.

The first attempt of the matrix above failed the macos 1/2 shard in the Reserve listener suite while eleven matrices were competing for runners on this fork; re-running only the failed jobs on an idle queue returned them green, while the same treatment left the baseline failure reproducing.

One thing I cannot do from my side: GitHub refuses markPullRequestReadyForReview on this PR with FORBIDDEN even though I am the author, and it accepts the same call on my other PRs in this repo. The PR therefore still shows as draft despite the completed checklist. If that blocks triage, undrafting it from your side should be enough; nothing else is outstanding.

@luvs01
luvs01 force-pushed the agent/media-loop-bounds-20260912 branch from fd92048 to 597a6a5 Compare September 13, 2026 03:08
@luvs01
luvs01 force-pushed the agent/media-loop-bounds-20260912 branch from 597a6a5 to 9518b52 Compare September 13, 2026 04:32
@luvs01
luvs01 marked this pull request as ready for review September 13, 2026 05:48
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 13, 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
📝 Code Review Completed 2026-09-13T05:52:52.368466Z 9518b52 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.

@github-actions
github-actions Bot marked this pull request as draft September 13, 2026 05:48

@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: 9518b52816

ℹ️ 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 thread src/images/loop.ts
if (!accepting || signal.aborted) return;
try {
// Check at emission, before even a synchronous producer can fill the queue.
iterationBudget.retain(event);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Charge the queue's coalesced events instead of discarded deltas

When a runTurn producer emits many small adjacent text or thinking deltas before the consumer is scheduled, createAdapterEventQueue coalesces them into chunks of up to 64 KiB, but this line charges every original event—including its JSON envelope—before queue.push performs that coalescing. For example, roughly 1,016,801 one-character text deltas occupy about 1 MiB in the retained, coalesced queue yet are charged over 32 MiB, causing a spurious translation_buffer_limit for output far below the documented limit. Charge the resulting coalesced representation, or otherwise keep the budget accounting synchronized with the queue's replacements.

Useful? React with 👍 / 👎.

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

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants