Repository navigation
[mem] fix(memory): release consumed buffers, abandoned waits, and deleted diff views - #30650
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the
Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Merge Risk: 🔵 Low · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 25.71% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 35 functions across 27 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title is concise and clearly summarizes the main memory-retention fixes, including consumed buffers, abandoned waits, and deleted diff views. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description check The description is detailed and covers the required change summary, rationale, testing, review risks, compatibility notes, visual impact, and checklist. It does not link an actual issue and does not u… Full details: Description check
-
Explanation
The description is detailed and covers the required change summary, rationale, testing, review risks, compatibility notes, visual impact, and checklist. It does not link an actual issue and does not use the exact
N/Awording required for visual proof, but these are limited omissions in an otherwise complete description.
-
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments: 1
src/shared/ndjson-retained-chunk-memory.test.ts: 1
55-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe duplicated
paused = false; framer.resume()pair is redundant.Lines 53-54 already perform this sequence. Repeat the call only if a second resume is needed to drain a queued record after a re-pause. In that case, add a comment that says so. Otherwise, remove the duplicate.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
5c3a3adf-2915-41ac-9930-0c780b5de9af
📒 Files selected for processing: 19
src/main/daemon/daemon-stream-binary-framing.tssrc/main/daemon/daemon-stream-binary-retained-memory.test.tssrc/main/runtime/mobile-cloud-relay/relay-readiness-retention.test.tssrc/main/runtime/mobile-cloud-relay/relay-readiness.tssrc/renderer/src/components/editor/combined-diff/CombinedDiffViewer.tsxsrc/renderer/src/components/editor/combined-diff/remember-view/combined-diff-view-memory.tssrc/renderer/src/components/editor/combined-diff/remember-view/use-combined-diff-view-persist.tssrc/renderer/src/components/editor/combined-diff/remember-view/use-combined-diff-view-restore.test.tsxsrc/renderer/src/components/terminal-pane/pty-preconnect-input-buffer.tssrc/renderer/src/components/terminal-pane/pty-preconnect-input-retention.test.tssrc/renderer/src/store/slices/removed-worktree-diff-memory.test.tssrc/renderer/src/store/slices/worktrees/teardown/remove-worktree-store-cleanup.tssrc/renderer/src/store/slices/worktrees/teardown/worktree-purge-state.tssrc/shared/abort-signal-reason-memory.test.tssrc/shared/abort-signal-reason.test.tssrc/shared/abort-signal-reason.tssrc/shared/main-process-ndjson-framer.tssrc/shared/ndjson-retained-chunk-memory.test.tssrc/shared/promise-settlement-waiters.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
53e114e1-bdc6-4214-a17b-6da036c0496f
📒 Files selected for processing: 4
src/renderer/src/components/editor/combined-diff/remember-view/combined-diff-view-cache.tssrc/renderer/src/components/editor/combined-diff/remember-view/combined-diff-view-memory.tssrc/renderer/src/store/slices/worktrees/teardown/remove-worktree-store-cleanup.tssrc/renderer/src/store/slices/worktrees/teardown/worktree-purge-state.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.
| /** A deleted workspace cannot restore its cached file bodies. Ordinary tab closes keep them. */ | ||
| export function forgetCombinedDiffViewsForWorktrees(worktreeIds: Iterable<string>): void { | ||
| const removed = new Set(worktreeIds) | ||
| for (const [key, view] of combinedDiffViewStateCache) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,180p' src/renderer/src/components/editor/combined-diff/remember-view/combined-diff-view-cache.ts
sed -n '1,240p' src/renderer/src/components/editor/combined-diff/remember-view/combined-diff-view-memory.ts
rg -n 'invalidateCombinedDiffCachesForRelativePath|combinedDiffScroll(Top|Anchor)Cache|forgetCombinedDiffViewsForWorktrees' src/renderer/src/components/editor/combined-diff src/renderer/src/store/slices/worktrees/teardownRepository: stablyai/orca
Length of output: 8923
🏁 Script executed:
set -o pipefail
rg -n -F --glob '*.ts' --glob '*.tsx' -e 'viewStateKey' -e 'combinedDiffViewStateCache' -e 'combinedDiffScrollTopCache' -e 'combinedDiffScrollAnchorCache' src/renderer/src/components/editor/combined-diff src/renderer/src/store/slices/worktrees/teardown
printf '\n--- restore ---\n'
sed -n '1,230p' src/renderer/src/components/editor/combined-diff/remember-view/use-combined-diff-view-restore.ts
printf '\n--- persist ---\n'
sed -n '1,180p' src/renderer/src/components/editor/combined-diff/remember-view/use-combined-diff-view-persist.ts
printf '\n--- anchors ---\n'
sed -n '1,190p' src/renderer/src/components/editor/combined-diff/scroll-viewport/use-combined-diff-scroll-anchors.ts
printf '\n--- related cache consumers ---\n'
sed -n '1,180p' src/renderer/src/components/editor/combined-diff/remember-view/use-combined-diff-view-memory.ts 2>/dev/null || true
rg -n -F --glob '*.ts' --glob '*.tsx' -- 'getCombinedDiffViewStateKey' 'combined-diff-view-state-key' 'createCombinedDiff' src/renderer/src/components/editor/combined-diffRepository: stablyai/orca
Length of output: 29780
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- viewer and key callers ---'
sed -n '1,290p' src/renderer/src/components/editor/combined-diff/CombinedDiffViewer.tsx
rg -n -F --glob '*.ts' --glob '*.tsx' -e 'viewStateKey=' -e 'viewStateKey:' -e 'CombinedDiffViewer' src/renderer/src/components/editor/combined-diff src/renderer/src/components/editor | head -160
printf '%s\n' '--- teardown cleanup ---'
sed -n '1,130p' src/renderer/src/store/slices/worktrees/teardown/remove-worktree-store-cleanup.ts
sed -n '1,130p' src/renderer/src/store/slices/worktrees/teardown/worktree-purge-state.ts
printf '%s\n' '--- cache invalidation and direct cache deletion ---'
rg -n -F --glob '*.ts' --glob '*.tsx' -e 'invalidateViewStateCache' -e 'combinedDiffViewStateCache.delete' -e 'combinedDiffScrollTopCache.delete' -e 'combinedDiffScrollAnchorCache.delete' src/renderer/src/components/editor/combined-diff src/renderer/src/storeRepository: stablyai/orca
Length of output: 41517
🏁 Script executed:
set -o pipefail
rg -n -F --glob '*.ts' --glob '*.tsx' \
-e 'diffViewStateKey' \
-e 'combinedDiffViewKey' \
-e 'combinedDiff' \
src/renderer/src/components/editor/EditorContent.tsx \
src/renderer/src/components/editor/EditorEditFileSurface.tsx \
src/renderer/src/components/editor 2>/dev/null | head -220
printf '%s\n' '--- candidate caller context ---'
for f in src/renderer/src/components/editor/EditorContent.tsx src/renderer/src/components/editor/EditorEditFileSurface.tsx; do
if test -f "$f"; then
grep -n -C 8 -E 'CombinedDiffViewer|diffViewStateKey|viewStateKey' "$f" || true
fi
doneRepository: stablyai/orca
Length of output: 23197
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- OpenFile identity and construction ---'
rg -n -F --glob '*.ts' --glob '*.tsx' \
-e 'type OpenFile' -e 'interface OpenFile' -e 'OpenFile =' -e 'id:.*worktreeId' \
-e 'worktreeId.*filePath' -e 'filePath.*worktreeId' \
src/renderer/src/store/slices/editor src/renderer/src | head -240
printf '%s\n' '--- direct scroll-only cleanup outside cache ownership helper ---'
rg -n -F --glob '*.ts' --glob '*.tsx' \
-e 'combinedDiffScrollTopCache.clear' \
-e 'combinedDiffScrollAnchorCache.clear' \
-e 'combinedDiffScrollTopCache.delete' \
-e 'combinedDiffScrollAnchorCache.delete' \
-e 'combinedDiffViewStateCache.delete' \
src/renderer/src | head -160Repository: stablyai/orca
Length of output: 12138
🏁 Script executed:
set -o pipefail
sed -n '1,115p' src/renderer/src/store/slices/editor/types/open-file.ts
printf '%s\n' '--- combined diff constructors ---'
sed -n '1,245p' src/renderer/src/store/slices/editor/actions/open-combined-diff.ts
printf '%s\n' '--- file id helpers ---'
rg -n -F --glob '*.ts' --glob '*.tsx' -e 'buildOwnedEditorFileId' -e 'buildEditorFileId' -e 'file.id =' -e 'id: crypto' src/renderer/src/store/slices/editor src/renderer/src/shared shared 2>/dev/null | head -180Repository: stablyai/orca
Length of output: 21005
Clear scroll entries when invalidating a view state.
invalidateCombinedDiffCachesForRelativePath deletes only the view-state entry. Delete the matching scroll-top and scroll-anchor entries in the same branch. Otherwise, a recreated worktree with the same combined-diff key can restore stale scroll state.
🐛 Suggested fix
-import { combinedDiffViewStateCache } from './combined-diff-view-cache'
+import {
+ combinedDiffScrollAnchorCache,
+ combinedDiffScrollTopCache,
+ combinedDiffViewStateCache
+} from './combined-diff-view-cache'
...
if (cached.sections.some((section) => section.path === relativePath)) {
combinedDiffViewStateCache.delete(key)
+ combinedDiffScrollTopCache.delete(key)
+ combinedDiffScrollAnchorCache.delete(key)
}
OrcaWin
changed the title
…t re-create cached diff entries Combined-diff view state, scroll offset and scroll anchor now live in one owner-tagged entry per view key. A mounted viewer claims its key; writes only update claimed entries, so the late unmount write that follows a workspace removal is a no-op instead of re-adding orphan scroll entries.
- Combined diff: writes go through a per-mount lease. Forgetting a workspace revokes its leases, so a late unmount write cannot resurrect entries, while an LRU-evicted entry of a still-mounted viewer is re-created on its next write. Drops the duplicated view scrollTop. - NDJSON framer: copy a retained tail only when it is a strict slice of its chunk, copy an unfinished line once, and copy every rejected-line prefix. - Binary frame reader: single exit path for the end-of-feed leftover copy. - Relay readiness reuses waitForPromiseWithSignal instead of re-implementing it. - PTY preconnect input: pass the listener-removing settle into createInput instead of mutating the input afterwards.
ELI5
Long sessions could keep old terminal output, cancelled requests, paste text and deleted workspaces' diffs alive. Release those objects when their owner is finished so memory can return to the collector.
What Changed
Fix five retention areas, plus the relay's related deadline wait:
waitForPromiseWithSignal.Why
These are ownership fixes. They preserve content, input order, history, existing cache policies and wire formats; they add no caps or truncation. Copying only surviving fragments avoids retaining consumed data, and using the existing waiter implementation keeps cancellation ownership explicit.
The forced-GC regressions failed against the original production code. On macOS / Node 24, with 32 readers or cancelled callers:
An alternating five-sample local microbenchmark processed the same 163,840 records per sample: JSON median 57.05 → 55.59 ms; binary median 56.47 → 56.85 ms. Both stayed within 3% of baseline. Electron's JavaScript runtime independently reproduced the JSON retention drop (32.0–32.2 MiB → 0.15–2.03 MiB); the same throughput samples showed no slowdown there.
Linked Issue
User-requested OOM audit; no existing issue linked.
Visual Proof
No visual change. Validated in a background-launched Electron build of this branch (CDP, isolated profile, 20-file repo):
Testing
pnpm tc— passed.pnpm run check:code-quality:changed— passed; commit hooks also passed lint, React Doctor and formatting.pnpm run build:electron-vite— passed, including renderer boot-graph verification.verifyaggregate (PR checks passed) passed forb49cd7f8db208b66babc644b24962a24630d590a, including all unit shards, relay integration, Linux/Windows packaging and static analysis. Earlier commitb76926ddhit a Codex dispatch test that was red on main at the time (fixed by test(codex): pin the stopped-send receipt rule where #30403 moved it (fix red main) #30865). No native Windows/Linux or live SSH session was exercised locally.Review
Shared-cancellation ordering trade-off (reviewed independently): all callers waiting on the same promise now settle in the first caller's reaction turn. A later caller can therefore resume one step before an unrelated
.thenattached between it and the first caller, and an abort fired from such a reaction no longer wins. Exact old ordering needs one reaction per caller, which is the leak. No current caller has that shape (shared-promise sites audited: orcad artifact materializer, Jira image cache, relay auth, PTY dialect probe, primary state writes, worktree preparation).Risk assessment: low. The full PR workflow passed on the current commit
b49cd7f8; independent reviews of the diff-cache lease and shared-cancellation ordering found no blocking issues. The most sensitive change is shared cancellation timing; tests cover cancellation before cached delivery, surviving callers, late failures and releasing stalled callers before the underlying operation finishes. Parser tests verify reconstruction and Unicode behavior. Existing limits and paired-client wire contracts are unchanged. Content invalidation continues to preserve scroll restoration. Scroll-only entries (no saved body) are owned too, so workspace removal releases them as well.Agent skill upstream boundary
Notes
All changes use existing cross-platform JavaScript/Node facilities. Execution-host authority is unchanged. Deleted-workspace cache cleanup applies to folder workspaces and preserves a same-ID workspace still published by another host.
Checklist
OOM audit PR list
[mem] fix(memory): release consumed buffers, abandoned waits, and deleted diff views(this PR).[mem] Backpressure worker subprocess output without dropping bytes.[mem] Stream saved transcripts and stage large search writes.The two follow-ups are independent branches based directly on this PR. Together they address four additional reproduced OOM mechanisms.