perf(opencode): throttle shell tool metadata streaming - #1049
Conversation
Every decoded chunk previously fired a full ctx.metadata push, which re-renders the tool part and publishes message.part.updated. For chatty commands (installs, builds, test runners) this is the dominant per-call cost even though the emitted preview is already capped at 30k chars. Coalesce the pushes through a single serialized channel (shell-metadata-throttle.ts): emit the first non-empty chunk immediately, then at most once per 150 ms or once accumulated input crosses 4 KiB, whichever comes first. Force a flush on spill-to-tempfile, and drain the consumer + push a final preview inside the process scope on exit/abort/timeout so no tail is dropped. Serialized emits prevent a stale preview from overwriting a newer one downstream. Refs #1038 (PR 3 of the bash -> shell migration).
Astro-Han
added
enhancement
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR adds an Effect-based metadata throttler (makeMetadataThrottle) that coalesces shell output metadata by time and byte thresholds, integrates it into ShellTool.run to replace per-chunk metadata writes (with explicit spill/final flushes), and expands tests to cover threshold, timer, empty-chunk, and emit-defect scenarios. ChangesMetadata Throttling Feature
Sequence DiagramsequenceDiagram
participant Chunk as Chunk Stream
participant Throttle as Metadata Throttle
participant Emit as ctx.metadata()
participant File as Spill File
Chunk->>Throttle: onChunk(size)
alt First non-empty chunk
Throttle->>Emit: emit() sync
end
alt Bytes exceed threshold
Throttle->>Emit: emit() and reset
end
alt Output exceeds limit
Chunk->>File: truncate to file
Chunk->>Throttle: flush("spill")
Throttle->>Emit: emit() immediately
end
alt Interval timer fires
Throttle->>Emit: emit() if dirty
end
Chunk->>Throttle: flush("final") on scope exit
Throttle->>Emit: emit() pending tail if dirty
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a metadata throttling mechanism (makeMetadataThrottle) to coalesce streaming metadata pushes from shell commands, reducing the overhead of frequent updates. It integrates this throttle into the shell tool execution path and adds comprehensive unit tests. The review feedback highlights a potential issue where errors during metadata emission could propagate and crash the stream consumer fiber, potentially causing the spawned process to hang. To address this, the reviewer suggested catching and ignoring emission errors and optimizing lock acquisition using a double-checked locking pattern.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…timeout Address review feedback on #1049: - Wrap the throttle's emit in Effect.catchDefect so a defect in the metadata channel cannot fail the stream consumer (which would stop reading stdout and could hang the process) or orDie the final flush. Interrupts and typed errors still propagate. emit is typed Effect<void> (E = never), so the prior suggestion of Effect.ignore would have been a no-op against the defect that is the actual hazard. Added a defect-emit test covering the first-chunk, timer, and final flush paths. - Name the post-exit consumer drain timeout (CONSUMER_DRAIN_TIMEOUT) and document its fall-through-to-scope-cleanup semantics. Refs #1038.
A leading empty decode chunk (TextDecoder can emit "" on a partial multibyte boundary) would burn the first-flush slot, delaying the first real output past the timer/threshold. Guard size <= 0 in onChunk.
1 participant
Summary
Coalesce the shell tool's per-chunk metadata streaming. Previously every decoded chunk fired a full
ctx.metadatapush, which re-renders the tool part, publishesmessage.part.updated, drives frontend reconcile, and pushes an ACP in-progress update. For chatty commands (installs, builds, test runners) this is the dominant per-call cost, even though the emitted preview is already capped at 30k chars bypreview().This is PR 3 of the #1038 bash to shell migration (PR 1 = #1046 split, PR 2 = #1048 rename). The throttle plan and a codex design consult live on issue #1038.
Changes
packages/opencode/src/tool/shell-metadata-throttle.ts: a single serialized emit channel. Emits the first non-empty chunk immediately; subsequent chunks coalesce on a 150 ms timer or once accumulated input crosses 4 KiB, whichever fires first.flush("spill")forces an emit;flush("final")is dirty-gated.shell.tsrun(): the stream consumer now feeds the throttle instead of callingctx.metadataper chunk. After the exit/abort/timeout race it joins the consumer fiber and does a final flush inside the process scope, so the throttle's forked timer is still alive and gets interrupted on scope exit. Spill-to-tempfile routes throughflush("spill").test/tool/shell-metadata-throttle.test.ts: 7 deterministicit.effecttests underTestClock(no real sleeps) — first-chunk-sync, byte-threshold, progressive timer flushes, final tail flush, spill force-flush, no-op timer when clean, no-op final when already emitted.Design (codex consult adopted)
ctx.metadata. Every emit ships the full preview, so out-of-order writes could let a stale preview overwrite a newer one downstream (the frontend reducer overwrites with the last-arrived full part).dirty/byte counters before awaiting emit so chunks arriving mid-emit are not silently dropped from the next flush.shell.test.ts"preserves output when aborted") depend on seeing the first chunk's metadata synchronously.Behavior preserved
bashtool id, schema name, and permission key unchanged.completeToolCall).Out of scope (deferred)
bashtoshellid flip (Refactor Bash tool toward upstream shell-aware structure #1038 PR 4, gated on a plugin-compat strategy).Verification
bun run typecheck— pass.bun test test/tool/ test/permission/— 656 pass, 1 skip, 0 fail across 34 files.shell.test.ts) — green.Refs #1038.
Summary by CodeRabbit
Refactor
Tests