fix(browser): guest-retention polish — LRU across kill-switch, release veto on interrupted downloads - #12232
Conversation
…nload veto on interrupt Two robustness follow-ups from the #12194 safety-review loop (pass 3), staged by the reviewer before its session ended; both fail toward extra retention only: - recency bookkeeping now runs while the retention kill switch is off, so re-enabling evicts in true LRU order instead of worktree-list order - interrupted downloads release the eviction veto instead of pinning the worktree as downloading forever
📝 WalkthroughWalkthroughBrowser guest recency is updated for active worktrees before retention-budget eviction checks. Download tracking now records per-download active state, handles interruption and resumption events, ignores invalid progress events, and removes progress listeners during cleanup. Tests cover these state transitions, duplicate and unknown events, balanced activity counts, and listener removal. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 422022dc-e39b-4eac-9e51-694e7541da06
📒 Files selected for processing (3)
src/renderer/src/components/Terminal.tsxsrc/renderer/src/components/browser-pane/browser-page-download-activity.test.tssrc/renderer/src/components/browser-pane/browser-page-download-activity.ts
| const removeListener = (): void => { | ||
| removedListenerCount += 1 | ||
| } | ||
| vi.stubGlobal('window', { | ||
| api: { | ||
| browser: { | ||
| onDownloadRequested: (callback: DownloadRequestedCallback) => { | ||
| requestedCallbacks.push(callback) | ||
| return () => { | ||
| removedRequested = true | ||
| } | ||
| return removeListener | ||
| }, | ||
| onDownloadProgress: (callback: DownloadProgressCallback) => { | ||
| progressCallbacks.push(callback) | ||
| return removeListener | ||
| }, | ||
| onDownloadFinished: (callback: DownloadFinishedCallback) => { | ||
| finishedCallbacks.push(callback) | ||
| return () => { | ||
| removedFinished = true | ||
| } | ||
| return removeListener |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Track each listener removal separately.
The shared removeListener makes removedListenerCount === 3 pass if cleanup invokes one unsubscribe callback three times. Return distinct removal spies for onDownloadRequested, onDownloadProgress, and onDownloadFinished. Assert that each spy is called once.
Also applies to: 130-130
1 participant
Summary
Two robustness follow-ups to #12194's browser-guest retention budget, staged by the safety-review loop's third pass before its session ended (I committed its staged work verbatim; provenance noted in the commit body). Neither is release-gating: both edge cases fail toward retaining extra memory — the pre-#12194 status quo — never toward destroying something in use.
onDownloadProgresshandling forinterrupted/null states), so a stuck download entry can't pin a worktree as never-evictable forever.Screenshots
No visual change.
Testing
pnpm lintpnpm typecheckpnpm testpnpm buildbrowser-page-download-activity.test.tsextended for the progress/interrupted listener lifecycle (listener add/remove accounting included). PR CI runs the full suite; the delta is 6 files / ~74 lines originally reviewed inside the #12194 until-clean loop.AI Review Report
These changes ARE the output of the #12194 adversarial review loop (pass 3), which had already produced two merged fixes (surface-stays-mounted eviction; download veto + kill switch) in that PR. Risks checked there: eviction must never destroy an in-use guest (this PR widens the protection to interrupted downloads) and kill-switch toggling must not corrupt LRU order. Cross-platform: renderer-only logic, no paths/shortcuts/shell/Electron-main surface.
Security Audit
No new inputs, IPC, command execution, or secrets. The new listener consumes the existing preload
onDownloadProgressbridge events only.Notes
Not needed for the v1.4.165 cherry-picks; rides the next release train. If the rc.1 bake shows any retention oddity, this PR plus the in-code kill switch are the levers.