Repository navigation
fix(renderer): the infinite loop due to which the app ui stays frozen - #5965
Closed
snehendu098 wants to merge 5 commits into
Closed
snehendu098 wants to merge 5 commits into
snehendu098 wants to merge 5 commits into
Conversation
…nderer `localStorageCollectionOptions` re-serializes the whole collection on every mutation and rethrows QuotaExceededError. The library treats that throw as a failed transaction and rolls the optimistic write back, which makes the live query re-emit and the queuing effect retry — a synchronous loop that pegs the renderer and survives restarts, because the full store is on disk. Guard the storage boundary instead of one call site: on quota error, reclaim orphaned terminal snapshots, retry once, then return without throwing. No throw means no rollback, so the loop cannot form at any of the ~15 sites that write to these collections. Reclaim runs on the quota path rather than at startup, so a profile left wedged by a previous session recovers on next launch.
Contributor
📝 WalkthroughWalkthroughThis change adds shared terminal snapshot keys, parked-terminal registry tracking, orphaned snapshot reclamation, quota-safe localStorage persistence, quota notifications, collection integration, and tests covering these behaviors. ChangesTerminal snapshot quota handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Collection
participant QuotaGuard
participant SnapshotStorage
participant TerminalRegistry
participant LocalStorage
Collection->>QuotaGuard: persist collection value
QuotaGuard->>LocalStorage: attempt setItem
QuotaGuard->>TerminalRegistry: getRegisteredTerminalIds
QuotaGuard->>SnapshotStorage: reclaim orphaned snapshots
SnapshotStorage->>LocalStorage: remove orphaned keys
QuotaGuard->>LocalStorage: retry setItem
QuotaGuard-->>Collection: complete write or report failure
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
|
Ready to review this PR? Stage has broken it down into 5 individual chapters for you: 
Chapters generated by Stage for commit ac46d41 on Jul 27, 2026 8:55pm UTC. |
![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: 2
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/withQuotaGuard.test.ts (1)
53-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid wall-clock settling in this test.
setTimeout(resolve, 0)makes transaction settlement timing-dependent. Use the existing deterministic transaction-flush mechanism or an event-loop-friendly alternative that does not rely on real time. Based on learnings, test code should avoid real wall-clock timers.🤖 Prompt for 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. In `@apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/withQuotaGuard.test.ts` around lines 53 - 55, Replace the wall-clock setTimeout-based implementation of settleTransactions with the repository’s existing deterministic transaction-flush mechanism or a non-time-based event-loop yielding alternative. Keep settleTransactions responsible for waiting until mutation rollback transactions have settled without relying on real timer timing.Source: Learnings
🤖 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 `@apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.ts`:
- Around line 128-130: Update the terminal runtime registry around
getRegisteredTerminalIds, release(), eviction, and disposeEntry() to track
persisted snapshot reachability independently from live entryKeysByTerminalId.
Keep released or evicted terminals included for quota/orphan checks until
explicit dispose() removes their snapshot tracking, and add a regression test
covering release/eviction followed by the quota guard to ensure scrollback and
dimensions remain preserved.
In
`@apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/notifyQuotaExhausted.ts`:
- Around line 45-47: Update describeClearedSnapshots to label the count as
cleared terminal snapshots or entries, with accurate singular/plural wording
instead of terminals. In
apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/notifyQuotaExhausted.ts
lines 45-47, change the message while preserving the zero-count text; update the
singular and plural assertions in
apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/notifyQuotaExhausted.test.ts
lines 120-127 to match.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/withQuotaGuard.test.ts`:
- Around line 53-55: Replace the wall-clock setTimeout-based implementation of
settleTransactions with the repository’s existing deterministic
transaction-flush mechanism or a non-time-based event-loop yielding alternative.
Keep settleTransactions responsible for waiting until mutation rollback
transactions have settled without relying on real timer timing.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0294c9cf-8224-4959-b196-a015b54f4c3e
📒 Files selected for processing (13)
apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.test.tsapps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.tsapps/desktop/src/renderer/lib/terminal/terminal-runtime.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/notifyQuotaExhausted.test.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/notifyQuotaExhausted.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/selectOrphanedTerminalSnapshots.test.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/selectOrphanedTerminalSnapshots.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/terminalSnapshotStorage.test.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/terminalSnapshotStorage.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/withQuotaGuard.test.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/withQuotaGuard.tsapps/desktop/src/shared/constants.ts
Release and eviction persist a terminal's buffer and then drop its registry entry, deliberately leaving the snapshot on disk for the next mount to restore. Reclaim read that missing entry as "orphaned" and could delete the scrollback of a pane the user still had open. Track the terminals whose snapshot outlived their entry, mirroring what disposeRuntime does with it: "preserve" keeps the snapshot, anything else has already cleared it. dispose() clears the record, since that is what removes the snapshot for real. Also report cleared snapshots as storage entries rather than terminals — each terminal persists a buffer and a dimensions key, so the old wording claimed double the real count — and write the entry-key separator in tests as \u0000 instead of a literal NUL byte.
snehendu098
force-pushed
the
fix/quotaexceederror-loop
branch
from
July 26, 2026 09:09
947b26b to
d270573
Compare
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 `@apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.test.ts`:
- Around line 539-548: Update the test “stops reporting a terminal once dispose
clears its snapshot” to seed the terminal ID through the registry’s preserved
identity state, such as preservedSnapshotTerminalIds, rather than only
fakeStorage. Assert getRegisteredTerminalIds() contains terminalId before
calling dispose, then retain the existing assertion that it no longer contains
the ID afterward.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d6134950-33e7-4fc5-b5c1-eabc39382f48
📒 Files selected for processing (13)
apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.test.tsapps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.tsapps/desktop/src/renderer/lib/terminal/terminal-runtime.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/notifyQuotaExhausted.test.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/notifyQuotaExhausted.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/selectOrphanedTerminalSnapshots.test.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/selectOrphanedTerminalSnapshots.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/terminalSnapshotStorage.test.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/terminalSnapshotStorage.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/withQuotaGuard.test.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/withQuotaGuard.tsapps/desktop/src/shared/constants.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/selectOrphanedTerminalSnapshots.test.ts
- apps/desktop/src/shared/constants.ts
- apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/selectOrphanedTerminalSnapshots.ts
- apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/terminalSnapshotStorage.ts
- apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.ts
- apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/notifyQuotaExhausted.ts
- apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/notifyQuotaExhausted.test.ts
- apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/terminalSnapshotStorage.test.ts
- apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/withQuotaGuard.ts
- apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/withQuotaGuard.test.ts
- apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts
- apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts
…ecord The test seeded fakeStorage, but getRegisteredTerminalIds reads the registry maps rather than storage — so the id was absent before dispose ran and the assertion held either way. Seed the preserved record and assert it is reported before disposal, so removing dispose's bookkeeping fails the test.
Contributor
Author
|
Addressed the review feedback:
Two things I want to flag myself: A gap in my own change. Reachability comes from registry state, which is empty after a restart. Panes mount lazily, so a quota error early in startup could treat a not-yet-mounted pane's snapshot as orphaned. Bounded (scroll history, not work, and the app was frozen outright before this) but unintended. The robust signal looks like the persisted pane layout: On the remaining issue item. Capping |
Member
|
Thanks for this — the diagnosis of the rollback→retry loop was exactly right, and we're keeping your
Closing in favor of #6038, which credits you as co-author. The remaining growth contributors you flagged (uncapped |
Kitenite
added a commit
that referenced
this pull request
Jul 29, 2026
…ollback loops (#6038) Reworked from PR #5965 by @snehendu098 (refs #5496). When localStorage is full, @tanstack/db's localStorageCollectionOptions rethrows QuotaExceededError on every write; the library rolls the optimistic write back, the live query re-emits, the queuing effect retries - an infinite loop (~40x/s) that pegs the renderer and survives restarts. withQuotaGuard wraps every localStorage-backed collection's storage so setItem absorbs quota failures: reclaim space, retry once, then drop the write instead of throwing. No throw means no rollback, so the loop cannot form at any write site. Changes from the original PR: - Reclaim is driven by the terminal GC's persisted-at index (24h pressure TTL, terminal-buffer-gc.ts) instead of registry membership plus an in-memory preserved-set that reset on relaunch and could delete restorable snapshots of not-yet-opened workspaces. - Key prefixes stay canonical in terminal-buffer-gc.ts; the shared/constants.ts copies and the registry additions (getRegisteredTerminalIds, preservedSnapshotTerminalIds) are dropped. - QUOTA_NOTICE_MODE ships as offer-reclaim: genuine exhaustion surfaces a toast with a working "Free up space" action instead of a silent console warning. Co-authored-by: snehendu098 <snehendu098@users.noreply.github.com>
3 tasks done
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
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.
What & why
Refs #5496 — fixes the freeze, but does not close the issue (remaining items listed below).
When
localStorageis full, the desktop app becomes permanently unusable: UI frozen, renderer pegged at 100%+ CPU, RSS climbing past 5 GB. Restarting the app or the machine doesn't help, because the full store is on disk. The only escape today is manually deleting~/Library/Application Support/Superset/Partitions/superset/Local Storage.localStorageCollectionOptionsre-serializes the whole collection on every mutation and rethrowsQuotaExceededError. The library treats that throw as a failed transaction and rolls the optimistic write back → the live query re-emits → the effect that queued the write retries → repeat, ~40x a second, with zero user interaction.The fix guards the storage boundary rather than one call site. On quota error: reclaim orphaned terminal snapshots, retry once, then return without throwing. No throw means no rollback, so the loop cannot form at any of the ~15 sites that write to these collections — not just
usePlaceLocalWorktreesInSidebar.It also reclaims orphaned
terminal-buffer:*/terminal-dims:*keys — snapshots for terminals that no longer exist. Eviction leaves those behind by design (terminal-runtime-registry.ts) and nothing ever collected them; the reporter found 528 such keys, ~8.5 MB. Reclaim runs on the quota path rather than at startup, so a profile left wedged by a previous session recovers on next launch instead of needing Finder.Follows existing conventions in this directory:
withQuotaGuardis a sibling ofwithReadHeal, and the pure-policy/side-effect split mirrorsselectRuntimesToEvict. No library patch needed —localStorageCollectionOptionsalready accepts an injectablestorage.Deliberately out of scope (from the issue's other defects):
terminal-buffer:*for terminals still open (issue suggestis, so reclaim skips them — with enough live terminals there's nothing tofree and writes fail permanently. Degraded rather than frozen, but still permanent. Happy to take this as an immediate follow-up.viewedFiles, base64initialFilesin `paner growth contributors, untouched.existing SQLite helper is built for Electric-synced collections rather t
User-facing notice on genuine exhaustion is behind
QUOTA_NOTICE_MODEi— currentlysilent(console warning only, matching howterminal-runtime-registryalready handles this error).notifyandoffer-reclaim` are implemented alongside it, so switching is a one-word change if you'd prefer atoast.
How I tested it
Reproduced on unmodified
mainfirst, then replayed the identical six:Repro: local stack via
setup.local.sh, throwaway repo imported as a project, 12 worktree workspaces,localStoragefilled to the real ceiling, then let sidebar placement run. Measured over CDP against this workspace's matched renderer. Beforeeenshotand evenRuntime.enable` timed out. Killing every process andrelaunching cold re-wedged within a minute — confirming the "restart doesn't help" report.One correction to the issue: the quota was ~49 MB on Electron 40, not ~10 MB. Different threshold, identical mechanism.
Tests: 27 new unit tests across the four new modules. The regression test was verified to fail against unguarded code (the rollback is asynchronous, so asserting synchronously passes even when broken). Mutation-tested six deliberate bskip reclaim, swallow non-quota errors, break warn dedupe, ignore mode,leak the live Set — each killed a test.
clearAllTerminalSnapshotshas an explicit test asserting it never touches collection data.Full desktop suite: 2293 pass. The 8 failures on my machine are pre-existing and environmental (
runTeardown+getProcessEnvWithShellPathfail when$SHELLis nushell,since
getCommandShellArgsfalls back to-lc) — verified identical wiling separately.Checklist
type(scope): subject)bun run lintandbun run typecheckpass (CI fails on lint warnings too)Summary by cubic
Fixes the freeze caused by an infinite retry loop when
localStoragehits quota. Writes now fail soft: we reclaim safe terminal snapshots, retry once, then drop the write so the UI stays responsive and data stays in memory for the session.withQuotaGuard: onQuotaExceededError, reclaim, retry once, then return (no throw → no rollback loop across writers).TERMINAL_BUFFER_KEY_PREFIX/TERMINAL_DIMS_KEY_PREFIX; parked and released-but-restorable terminals are preserved via the registry’sgetRegisteredTerminalIds, anddisposeclears preserved records.notifyQuotaExhaustedbehindQUOTA_NOTICE_MODE(silentby default); “offer-reclaim” shows a toast with “Free up space” that clears saved scrollback and reports cleared entries (entries, not terminals).Written for commit ac46d41. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
User Experience