Conversation
github-actions
Bot
added
the
maintainer:e2e:ok
Contributor
📝 WalkthroughWalkthroughThe CLI now detects Bun and skips ineffective Node heap arguments. The footer omits Bun heap-limit denominators and refreshes Node heap limits on each call. Tests use Bun-compatible utilities and cover runtime-specific memory behavior. ChangesBun memory behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
WalkthroughBefore this PR, the CLI treated memory/relaunch and footer heap display as Node-only assumptions. In bootstrap, the startup relaunch path unconditionally used v8 heap statistics and os.totalmem() to compute a larger Node heap, so under Bun it could try to relaunch with --max-old-space-size even though Bun does not expose the same Node heap ceiling. In the sandbox path, computeSandboxMemoryArgsFromEnv still flowed through the same Node-centric logic, and the footer always read v8.getHeapStatistics().heap_size_limit to show a denominator like 8.0GB. After this PR, the code detects when it is running under Bun, skips the Node-specific relaunch/sandbox memory-arg computation in those paths, and stops showing a fabricated Bun heap ceiling by omitting the denominator from the footer heap line. The result is that Bun users no longer see bogus relaunch attempts or an inaccurate heap-limit display, while Node behavior remains unchanged. Release Notes
Changes
Sequence DiagramsequenceDiagram
User->>CLI: Run command
CLI->>Bootstrap: maybeRelaunchForMemory
Bootstrap->>Bootstrap: Detect Bun runtime
alt Bun runtime
Bootstrap-->>CLI: Skip Node heap relaunch
else Node runtime
Bootstrap->>CLI: Return memory relaunch args
end
CLI->>Sandbox: maybeHopIntoSandbox
Sandbox->>Bootstrap: computeSandboxMemoryArgsFromEnv
Bootstrap->>Bootstrap: Detect Bun runtime
alt Bun runtime
Bootstrap-->>Sandbox: Return empty memory args
else Node runtime
Bootstrap->>Sandbox: Return --max-old-space-size args
end
Sandbox->>CLI: Start sandbox process
CLI->>Footer: Render footer
Footer->>Footer: Detect Bun runtime
alt Bun runtime
Footer->>Footer: Omit heap limit denominator
else Node runtime
Footer->>Footer: Include heap limit denominator
end
Magnitude🎯 1 (S) Related
Pre-merge Checks
Walkthrough generated by LLxprt PR Review. Planner issue: #2256 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/utils/bootstrap.test.ts`:
- Around line 35-39: Update the afterEach cleanup to remove process.versions.bun
when originalBunDescriptor is undefined, while restoring the saved descriptor
when it exists. Ensure cleanup leaves process.versions matching its state before
each test so later runtime detection is unaffected.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d48d5311-75c6-4a57-8f44-8c573069831c
⛔ Files ignored due to path filters (2)
project-plans/issue3112/plan/00a-preflight-verification.mdis excluded by!project-plans/**project-plans/issue3112/specification.mdis excluded by!project-plans/**
📒 Files selected for processing (5)
packages/cli/src/cliSandbox.tspackages/cli/src/ui/components/Footer.test.tsxpackages/cli/src/ui/components/Footer.tsxpackages/cli/src/utils/bootstrap.test.tspackages/cli/src/utils/bootstrap.ts
Contributor
OpenCodeReview — PR #3123
|
acoliver
added a commit
that referenced
this pull request
Aug 7, 2026
Long-running sessions grew memory with uptime rather than activity. Most of the umbrella issue has now landed as separate PRs: #3117 coalesced thinking blocks in `StreamOutputAccumulator` (the primary retention site), #3123 made the Bun memory readout honest, #3125 removed the history deep-clone and #3124 bounded error reports. This lands the one attributed retention defect still outstanding, and the measurement that shows the combined result holds. `Turn.cleanupStreamResources` aborted the timeout controller *before* calling `closeIteratorBounded`. Because that helper early-returns when the signal it is handed is already aborted, the abort raced ahead of its own cleanup and the branch that awaits `iterator.return()` was unreachable on every turn. A cooperative provider iterator therefore never finished unwinding, so the generator scope capturing the turn's stream state stayed alive. Closure is now awaited first and the controller is aborted after. The cleanup signal is omitted deliberately: passing the turn-owned signal is what defeated the wait. A noncooperative iterator is still bounded by `closeIteratorBounded`'s own one-second timeout, so this cannot hang. There is no added cancellation latency. `onParentAbort` already aborts the timeout controller from the parent signal, so on user cancel the controller is aborted before cleanup runs; the reordering only affects normal completion and early consumer exit. Both new cases fail against the previous implementation and pass against this one. Proof extends `scripts/issue-2852-memory-runner.ts` rather than adding a parallel harness. A new `reasoning` mode drives the real `StreamOutputAccumulator` with 200 full-so-far thinking deltas per turn against a 30 KB final thought, the shape Anthropic actually streams. Each prefix is copied through a Buffer so the workload allocates distinct string backing stores instead of engine-dependent substring views, and the turn asserts the span collapses to exactly one block carrying the full final text, stream id, status and signature -- so the target fails loudly if that coalescing ever regresses. The plateau gate now evaluates JSC heap, `process.memoryUsage().external` and dirty WebKit Malloc from `vmmap` independently, and passes only if all three settle. External is the metric that matters here: the vmmap investigation attributed 32.9 GB to it against 0.5 GB of ArrayBuffers, and under Bun it tracks string backing stores at roughly a byte per character. Gating on JSC heap alone would have declared victory while the strings were still resident. Measured over four turns: JSC heap +0.5%, external +2.3%, dirty WebKit Malloc +0.0%. `docs/sandbox.md` still described the Node heap limit as automatically derived regardless of runtime, which #3123 made untrue when it stopped passing `--max-old-space-size` under Bun. Corrected so the documentation matches the shipped behavior. No bound discards content. The reasoning span is retained in full; only the duplicate partial copies of it are not.
acoliver
added a commit
that referenced
this pull request
Aug 8, 2026
…) (#3127) * Release per-turn stream state and prove the memory plateau (Fixes #3114) Long-running sessions grew memory with uptime rather than activity. Most of the umbrella issue has now landed as separate PRs: #3117 coalesced thinking blocks in `StreamOutputAccumulator` (the primary retention site), #3123 made the Bun memory readout honest, #3125 removed the history deep-clone and #3124 bounded error reports. This lands the one attributed retention defect still outstanding, and the measurement that shows the combined result holds. `Turn.cleanupStreamResources` aborted the timeout controller *before* calling `closeIteratorBounded`. Because that helper early-returns when the signal it is handed is already aborted, the abort raced ahead of its own cleanup and the branch that awaits `iterator.return()` was unreachable on every turn. A cooperative provider iterator therefore never finished unwinding, so the generator scope capturing the turn's stream state stayed alive. Closure is now awaited first and the controller is aborted after. The cleanup signal is omitted deliberately: passing the turn-owned signal is what defeated the wait. A noncooperative iterator is still bounded by `closeIteratorBounded`'s own one-second timeout, so this cannot hang. There is no added cancellation latency. `onParentAbort` already aborts the timeout controller from the parent signal, so on user cancel the controller is aborted before cleanup runs; the reordering only affects normal completion and early consumer exit. Both new cases fail against the previous implementation and pass against this one. Proof extends `scripts/issue-2852-memory-runner.ts` rather than adding a parallel harness. A new `reasoning` mode drives the real `StreamOutputAccumulator` with 200 full-so-far thinking deltas per turn against a 30 KB final thought, the shape Anthropic actually streams. Each prefix is copied through a Buffer so the workload allocates distinct string backing stores instead of engine-dependent substring views, and the turn asserts the span collapses to exactly one block carrying the full final text, stream id, status and signature -- so the target fails loudly if that coalescing ever regresses. The plateau gate now evaluates JSC heap, `process.memoryUsage().external` and dirty WebKit Malloc from `vmmap` independently, and passes only if all three settle. External is the metric that matters here: the vmmap investigation attributed 32.9 GB to it against 0.5 GB of ArrayBuffers, and under Bun it tracks string backing stores at roughly a byte per character. Gating on JSC heap alone would have declared victory while the strings were still resident. Measured over four turns: JSC heap +0.5%, external +2.3%, dirty WebKit Malloc +0.0%. `docs/sandbox.md` still described the Node heap limit as automatically derived regardless of runtime, which #3123 made untrue when it stopped passing `--max-old-space-size` under Bun. Corrected so the documentation matches the shipped behavior. No bound discards content. The reasoning span is retained in full; only the duplicate partial copies of it are not. * Address review: tighten cleanup-bound assertion and name malformed checkpoints CodeRabbit: the noncooperative-cleanup assertion allowed 5s against a documented 1s bound, so a regression adding a whole extra second per turn would have passed. Tightened to 2.5s, which still absorbs CI scheduling jitter. The plateau test promised a growth-ratio assertion in its name but only checked the baseline and verdict; growthRatio and maxBytes are now asserted too. OCR: a malformed checkpoint line surfaced as a bare SyntaxError naming neither the file nor the line, across artifacts with hundreds of records. Both readers now share readPostGcRecords, which reports the offending path and line number and re-throws — a corrupt artifact still fails the run rather than being tolerated. Sharing the reader also removes the duplicated read/split/filter/parse chain, and readPostGcHeapBytes now reuses requireMetric instead of carrying its own copy of the same check. * Report the malformed checkpoint line as it appears in the file The line-number reporting added in the previous commit was wrong: blank lines were filtered out before the index was taken, so the reported number counted only non-blank lines. On a file with blank lines it pointed at the wrong record, which defeats the entire purpose of naming the line. Line numbers are now captured before blanks are dropped. The parser moved to issue-2852-memory-benchmark.ts, alongside the other artifact parsers, so it takes its contents as an argument and can be tested directly; the runner still owns reading the file. Three behavioral tests cover it, including one whose malformed record sits on physical line 4 behind two blank lines and would have been misreported as line 2 by the previous implementation. * Treat whitespace-only checkpoint lines as blank A line holding only spaces or a tab is not a record, but it was reaching JSON.parse and failing the run on an artifact that is structurally fine. The blank-line filter now trims before measuring, and the test feeds both a spaces-only and a tab-only line to prove they are ignored.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TLDR
Stop treating Bun's compatibility V8 heap statistic as a real memory ceiling. Bun now shows current heap usage without a fabricated denominator and skips both host and sandbox relaunch arguments that Bun ignores. The meaningful Node behavior remains unchanged, including a live heap-limit denominator that is refreshed rather than memoized.
Dive Deeper
Review findings were classified as Blocker-Fix, In-scope-Fix, Reject, or Defer. DeepThinker and local Open Code Review completed; all valid in-scope findings were resolved. No public API, dependency, workflow, lint-rule, or memory-retention change was introduced.
Reviewer Test Plan
Run the focused behavioral tests:
Expected: 82 tests pass.
Start the CLI with memory display enabled under Bun and confirm the footer resembles:
It must not contain a slash denominator. At wide width it should also show External and ArrayBuffers.
Review the Node-like test cases to confirm the heap denominator remains and changes after the two-second refresh when the mocked V8 statistic changes.
Review the bootstrap Bun cases to confirm both memory-argument functions return an empty array before host/V8 calculations, while the inherited Node calculation cases remain green.
Local evidence on macOS:
Repository-wide local caveats, reported without weakening gates or expanding this issue:
Testing Matrix
Linked issues / bugs
Fixes #3112
Related to #3108
Summary by CodeRabbit
Bug Fixes
Documentation