perf(orchestration): bound in-memory read model and client per-thread state#4176
perf(orchestration): bound in-memory read model and client per-thread state#4176RusiruSadathana wants to merge 1 commit into
Conversation
github-actions
Bot
added
vouch:unvouched
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review 1 blocking correctness issue found. Cross-cutting performance refactor affecting memory management in orchestration engine and multiple client stores. Unresolved review comments include a high-severity issue where the preview index keys may still grow unbounded despite the eviction changes. You can customize Macroscope's approvability policy. Learn more. |
7b31cf8 to
e30b77f
Compare
… state (Issue pingdotgg#4178) Replays PR pingdotgg#4176 on current main and preserves the connection catch-up path from pingdotgg#4177. Replace the array-backed command model with HashMap/HashSet state, evict deleted thread bodies, prune released VCS and browser caches, and clear per-thread client state. Adds focused coverage for read-model invariants, deletion behavior, client cleanup, and VCS cache eviction after the final subscriber releases. Co-authored-by: codex <codex@users.noreply.github.com>
e30b77f to
56b6615
Compare
There was a problem hiding this comment.
🟡 Medium
t3code/apps/web/src/browser/browserSurfaceStore.ts
Lines 104 to 120 in 56b6615
release deletes the entry from byTabId, but presentContent is owner-independent and recreates a { owner: null } entry whenever the entry is missing. If HostedBrowserWebview fires a scheduled layout or scroll update after the lease has released, presentContent resurrects the deleted entry, so released tabs still accumulate in byTabId — defeating the bounded-state goal of this change. Consider guarding presentContent so it no-ops (or requires a valid owner) when no entry exists, instead of creating a new owner: null entry.
| presentContent: (tabId, content) => | |
| set((state) => { | |
| const current = state.byTabId[tabId]; | |
| if (!current) return state; |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/browser/browserSurfaceStore.ts around lines 104-120:
`release` deletes the entry from `byTabId`, but `presentContent` is owner-independent and recreates a `{ owner: null }` entry whenever the entry is missing. If `HostedBrowserWebview` fires a scheduled layout or scroll update after the lease has released, `presentContent` resurrects the deleted entry, so released tabs still accumulate in `byTabId` — defeating the bounded-state goal of this change. Consider guarding `presentContent` so it no-ops (or requires a valid owner) when no entry exists, instead of creating a new `owner: null` entry.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 56b6615. Configure here.
| export const previewStateAtom = Atom.family((threadKey: string) => | ||
| Atom.make<ThreadPreviewState>(EMPTY_THREAD_PREVIEW_STATE).pipe( | ||
| Atom.keepAlive, | ||
| Atom.setIdleTTL(PREVIEW_STATE_IDLE_TTL_MS), |
There was a problem hiding this comment.
Idle preview TTL drops tab suppressions
Medium Severity
Introducing Atom.setIdleTTL on previewStateAtom causes client-only suppressedTabIds to be lost when a thread's preview state is evicted. This results in preview tabs that a user previously closed reappearing from the server list.
Reviewed by Cursor Bugbot for commit 56b6615. Configure here.
| export const previewStateAtom = Atom.family((threadKey: string) => | ||
| Atom.make<ThreadPreviewState>(EMPTY_THREAD_PREVIEW_STATE).pipe( | ||
| Atom.keepAlive, | ||
| Atom.setIdleTTL(PREVIEW_STATE_IDLE_TTL_MS), |
There was a problem hiding this comment.
Preview index keys never pruned
High Severity
activePreviewThreadKeysAtom is keepAlive and only updated via syncActivePreviewThread on explicit preview writes. When previewStateAtom idle TTL evicts a thread that still had sessions, the index entry is never removed, so the key set grows without bound and activePreviewSessionsAtom keeps scanning stale keys.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 56b6615. Configure here.
1 participant
What Changed
Server: replaced the array-backed in-memory orchestration command read model with a HashMap-keyed
CommandReadModel. Thread and project lookups and updates are now O(1) instead of O(N) linear scans plus full-array copies on every event, and deleted threads are evicted from the in-memory model (archived threads are retained because unarchive needs them and the projection cannot rehydrate them). The wireOrchestrationReadModelcontract is unchanged; it is served by the DB-backed projection, which is untouched.Web: wired the existing but previously uncalled per-thread cleanup into the thread deletion flow (preview state, right panel, diff panel, ui state), switched
previewStateAtomfromkeepAliveto an idle TTL so idle preview atoms are collected, and made a released browser surface delete its entry instead of leaving a tombstone.VCS: the
VcsStatusBroadcasterper-cwd status cache is now pruned when the last poller subscriber releases, instead of growing for the process lifetime.Added unit and integration coverage for the read model, projector eviction, decider behavior against a deleted thread, and the client store cleanup.
Why
After long uptime the server RSS and CPU climbed and the web UI became laggy. The cause was unbounded in-memory state on the single orchestration worker fiber (every event did O(N) work over a collection that never shed deleted or archived threads) compounded by client stores that accumulated one entry per thread ever visited and were never pruned on deletion. Making the hot-path structures O(1), evicting dead threads, and pruning the client and VCS caches removes the growth without changing any external contract or the projection that serves reads.
UI Changes
No visible UI changes. The web portion only prunes per-thread client state on deletion and adjusts atom retention; sidebar and panel behavior are unchanged.
Checklist
Related Issues
Closes #4178.
Note
Medium Risk
Orchestration command-path behavior changes (deleted-thread eviction and id retention) affect invariant checks and decider/projector semantics; web/VCS changes are localized leak fixes with tests.
Overview
Addresses unbounded memory growth on long-running server and web sessions by changing how hot-path state is stored and when it is dropped.
Server orchestration introduces an internal
CommandReadModel(HashMap/HashSet) for the serial command worker, replacing array scans on every event.thread.deletedevicts thread bodies from memory whiledeletedThreadIdspreserves the “cannot create twice” invariant across restarts; boot seeds viafromWireReadModelwithdropDeletedThreads: true. The wireOrchestrationReadModeland DB projection are unchanged.Web wires per-thread cleanup into thread delete (preview, right panel, diff panel, UI state in localStorage), switches
previewStateAtomfromkeepAliveto a 5-minute idle TTL, and removes browser surface entries on release instead of tombstones.VCS evicts per-
cwdcache when the last remote poller releases, fixing a race where concurrent retain/release could leave stale entries forever.Reviewed by Cursor Bugbot for commit 56b6615. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Bound in-memory orchestration read model and client per-thread state to prevent unbounded growth
OrchestrationReadModelwith aCommandReadModelbacked byHashMapfor O(1) lookups of threads and projects in commandReadModel.ts.HashSet, preventing memory growth and blocking re-creation of deleted thread IDs.VcsStatusBroadcasternow atomically evicts cached VCS status when the last subscriber releases a poller, closing a race condition.Atom.setIdleTTLinstead ofAtom.keepAlive.visible=false.Macroscope summarized 56b6615.