fix(ui): bound renderer memory and keep large sessions responsive - #626
fix(ui): bound renderer memory and keep large sessions responsive#626pascalandr wants to merge 14 commits into
Conversation
Keep inactive workspace and subagent histories in a paged IndexedDB cache while enforcing a global byte-aware hot-memory budget. Visible, streaming, compacting, and input-blocked sessions retain authority, and nested task sessions load only while their owning pane is visible. Bound tool, diff, diagnostic, speech, search, and nested-task rendering so large outputs cannot create unbounded parser or DOM work. Full large inputs and patches remain available through lazy copy actions. Harden HTTP, SSE, revert, deletion, and instance-reopen ordering with revision and lifecycle fences. Coalesce cache writes without retaining queued snapshots, preserve orphan deltas through an authoritative reload, and version cache invalidations across shared workspace keys. Validated with 395 UI tests, UI typecheck, production build, and independent memory, cache-concurrency, and rendering gatekeepers.
pascalandr
left a comment
pascalandr
left a comment
There was a problem hiding this comment.
Gatekeeper verdict: DO NOT SHIP
Reviewed commit dd3b818e against upstream/dev. The platform tests and builds pass, but the following correctness and memory-safety blockers remain.
-
P1: normal workspace refreshes invalidate lifecycle fences.
instances.ts:353-356,765-766,927-933replaces every existingInstanceobject during SSE reconnect reconciliation. Identity checks insession-api.ts:926,session-actions.ts:468,494, andsession-message-cache.ts:134,290then treat an unchanged workspace as removed/reopened. A successful server delete can skip local cleanup, and valid queued cache work can be dropped. Use a stable occurrence token rather than object identity, and change it only on actual remove/reopen. -
P1: an idle session can evict active optimistic work.
session-memory.ts:23-29,52-64checks only pending input when metadata saysidle, ignoring normalizedsendingandstreamingrecords. Send a prompt, switch away before the working-status event, then trigger memory pressure: the optimistic transcript is cleared while the request remains active. Protect all active normalized work regardless of metadata status. -
P1: orphan deltas are unbounded and recovery can lose them.
delta-buffer.ts:27-32,108-115re-holds failed deltas with no byte cap, entry cap, TTL, timer, or budget accounting.session-api.ts:1259-1264,1307-1310clears held deltas before the replacement HTTP load succeeds; failure/cancellation loses streamed output. The||=also stops evaluating later cleanup calls once true. Replace retained payloads with a bounded dirty-session marker plus a coalesced authoritative reload, or retain deltas until successful reconciliation. -
P1: same-ID reopen reuses stale HTTP and SSE authority.
session-state.ts:157-170,1217-1253deletes search authority so request IDs restart; an old search can match the replacement instance and write old sessions throughsession-api.ts:595-646.sse-manager.ts:164-170gates only on current ID membership, so buffered events from the old occurrence are accepted after reopen, while valid startup events are dropped before the instance list loads. Fence both transports with one stable instance-occurrence generation. -
P1: persistent cache metadata misrepresents truncated history and can restore stale snapshots.
session-message-cache.ts:94-121first truncates to an approximately 8 MiB suffix.lib/session-message-cache.ts:212-260receives only that suffix and recordsstartIndex: 0,totalCount: suffix.length, andcomplete: true. If a delayed writer later skips because the store was evicted orsnapshotSessionreturns null (session-message-cache.ts:124-140), the older IndexedDB snapshot is left eligible for restore. Preserve original total/start metadata and explicitly invalidate obsolete snapshots when a replacement cannot be written. -
P1: large structured tool output becomes inaccessible.
tool-call/utils.ts:161-171replaces oversized non-string output for rendering and returnsnullfor copy.tool-call/renderers/default.tsx:8-27consequently removes the copy action, and long errors are truncated without recovery intool-call.tsx:483-489. This contradicts the full-copy behavior described by the PR. Keep full content behind a lazy copy path without eagerly stringifying it. -
P2: cache fencing maps grow for process lifetime.
session-message-cache.ts:296-325,345-359retains generation/version keys for every invalidated or destroyed session. This is monotonic memory growth in the feature intended to bound memory. Delete settled keys or scope generations to an instance occurrence.
Validation
- All application CI tests and cross-platform builds passed.
git diff --checkpassed and the worktree is clean.- The red
commentcheck is unrelated: the artifact-comment workflow received a GitHub API504while paginating runs. - Missing coverage: actual IndexedDB transactions/failures, idle-plus-sending eviction, orphan-delta bounds/recovery failure, same-ID reopen, and cache/HTTP reconciliation.
The authenticated account owns this PR, so GitHub does not permit a self-authored REQUEST_CHANGES; this COMMENT review carries the gatekeeper rejection.
Fence asynchronous session, workspace, permission, question, command, and stream updates to the runtime that started them. Rotate stream authority across workspace restarts and preserve connecting transitions through the Tauri transport. Make persistent message restoration fail closed across incomplete reads, duplicate workspace owners, mutations, invalidation failures, and destructive client-state retries. Bound delta recovery, derived caches, search, diagnostics, task scans, and tool-output formatting by retained memory and CPU work. Preserve prompts and attachments across definite failures without replaying ambiguous destructive requests, and add focused UI, server, and Rust regressions. Validated with UI 435/435, Tauri 85/85, typechecks, and production build; the Windows worktree fixture remains branch-name dependent.
pascalandr
left a comment
pascalandr
left a comment
There was a problem hiding this comment.
Gatekeeper verdict: SHIP
Re-reviewed the updated head at c10cab6 after the correction loop. The runtime-authority, persistent-cache, bounded-memory/search, prompt recovery, SSE stream identity, and Tauri coalescing findings are resolved. The final independent gate found no release blocker.
Validation:
- UI: 435/435 tests passed
- Tauri: 85/85 tests passed
- UI, Electron, and server typechecks passed
- UI production build passed
- Server: 248 passed, 4 skipped; the only local failure is the unchanged Windows git-worktrees fixture that assumes the root branch is named main
Merge upstream dev at 1d4f778, retaining the shared LOOPBACK_HOST and instance client factory introduced by NeuralNomadsAI#630 while preserving per-runtime stream IDs, restart fencing, bounded SSE parsing, and reader cleanup from NeuralNomadsAI#626. Validated with the server typecheck and focused instance-client, background-process, and instance-event tests (13 passing).
Replace AbortSignal.timeout with a managed AbortController timer so a stuck loopback request keeps the Node event loop alive until it is aborted. Preserve caller-provided signals and clear the timer when fetch settles. Validated with the instance-client timeout test and server typecheck; the full server suite passes except for the known local branch-name fixture.
Keep the experimental persistent message cache disabled in desktop restore flows. The bounded cache could expose only a transcript suffix while progressive IndexedDB hydration repeatedly copied and measured large histories on the renderer thread, preventing responsive session changes. Tab restoration and in-memory session eviction remain enabled; selected sessions now use the established authoritative HTTP load path. Add a regression check and validate request authority, memory eviction, UI typechecking, and the production build.
Abort root and subagent message requests when selection or workspace visibility moves away, preventing late large histories from parsing and hydrating behind the active session. Derive request ownership from authoritative session ancestry so parent and selected-child paths share one cancellation boundary. Keep persistent transcript caching disabled, preserve desktop startup restoration, and make disabled-cache invalidation inert. Defer protected-session memory measurement to idle work while conservatively accounting for unknown size, then refresh measurements as sessions grow. Cover root, child, selected-child, workspace-switch, AbortSignal, memory-growth, global-budget, and authoritative empty-part scenarios. Validated with 444 UI tests, 118 Electron tests, typechecks, adversarial review, and a packaged Windows build from this worktree.
Run the Windows Tauri crate tests with a single test thread so process-election and registration-timeout tests do not contend for scheduler time and fail nondeterministically on shared runners. The same 85-test suite passes locally in serial mode, including 20 repeated isolated executions of the previously flaky simultaneous-startup election case. Linux test parallelism is unchanged.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/30692196593 Artifacts expire in 7 days.
|
pascalandr
changed the title
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/30693097142 Artifacts expire in 7 days.
|
Keep PR NeuralNomadsAI#626 focused on renderer memory safety by deleting the disabled IndexedDB transcript cache, its progressive restore path, cache mutation hooks, and cache-specific tests. Evicted sessions now always reload their complete authoritative history from OpenCode, preserving CodeNomad's existing search, timeline, usage, revert, and scroll semantics. Retain whole-session byte-budget eviction, bounded renderer work, runtime and request fencing, state purge, and desktop UI-state restoration. Keep revision retries cancellable while a session is hidden and suppress stale workspace-resolution failures before they can repopulate or report against a replaced runtime. Validated with 202 targeted UI tests, 118 Electron native tests, UI and Electron typechecks, the production UI build, and a focused hidden-retry regression test.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/30739857895 Artifacts expire in 7 days.
|
pascalandr
left a comment
pascalandr
left a comment
There was a problem hiding this comment.
Gate verdict: REQUEST CHANGES
Reviewed latest head 59b415c01c0e5d19040b8bfe0963781b6ca08f35, all eight PR commits, issue #553, and the complete diff from the true merge base/current dev base 1d4f77869e9d7d2c6820bc3f4515dddabab19758.
Merge blockers
-
P1: complete-session search is now silently incomplete.
packages/ui/src/lib/session-search.ts:34-39,180-214searches only the newest 10,000 messages, 20,000 parts, 5,000,000 characters, and 1,000 matches. It then scans the retained window oldest-first, so early large parts can exhaust the character budget before newer messages. A fully hydrated long session can therefore report no match for content that is resident and visible, with no partial-results indication. This directly contradicts the PR's complete-session search claim. Scan the authoritative resident transcript incrementally with cancellation/yielding; bound stored/rendered matches and previews, not the searchable corpus, or expose continuation/truncation explicitly. -
P1: scroll restoration can commit against a partial post-eviction suffix.
packages/ui/src/components/message-section.tsx:881-927no longer waits forprops.loading; that guard was explicitly removed by cleanup commit59b415c0. If an evicted session receives one or more SSE messages and is reopened, restoration runs while the complete HTTP hydration is still in flight. An older anchor is absent from the suffix, so fallback scrolls to bottom and setsdidRestoreScroll; the subsequent full seed never reapplies the saved anchor. Restore only after loading is false. -
P1: reconnect does not reconcile permissions or questions missed during the SSE gap.
packages/ui/src/stores/instances.ts:276-286refreshes sessions and one active resident transcript, but never calls the existing authoritativesyncPendingPermissions/syncPendingQuestionspaths atpackages/ui/src/stores/instances.ts:501-619. A request created during disconnection never appears; one answered elsewhere remains actionable and can keep its session protected indefinitely. Run both snapshot reconciliations during the trailing resync, fenced against both runtime replacement and newer accepted interruption events. -
P1: revision-conflict recovery has no global or per-session work bound. After three conflicted full-history loads,
packages/ui/src/stores/session-api.ts:1265-1283re-enters delta recovery. The active recovery atpackages/ui/src/stores/session-events.ts:487-509is marked dirty, treats the nested load as successful, resets failures to zero, and immediately repeats. A live session whose revision changes during each HTTP parse can continuously download and parse its complete history. Return an explicit conflict result and use one bounded, backoff-enabled recovery scheduled after streaming settles, with global concurrency limits. -
P1: the Tauri event path still permits unbounded frame and delta allocations.
packages/tauri-app/src-tauri/src/desktop_event_transport/stream.rs:161-189usesBufRead::read_lineand accumulatesdata_lineswithout a line/frame byte limit; a valid CR-only stream or a peer that withholds LF can grow one string indefinitely.packages/tauri-app/src-tauri/src/desktop_event_transport.rs:448-462also repeatedly copies coalesced deltas without a byte cap before the UI's 64 KiB delta limit is reached. This leaves the affected Tauri path outside the PR's SSE/delta-bound claim. Parse CR/LF/CRLF incrementally with strict line/frame limits and cap/drop-to-authoritative-reload before concatenation. -
P1: stream rotation does not rotate server-side auto-accept authority.
packages/server/src/permissions/auto-accept-manager.ts:113-117changesstreamIdbut clears only queued hydration events. Old runtime session ancestry, pending permissions, reply attempts, and in-flight replies survive; completions atpackages/server/src/permissions/auto-accept-manager.ts:401-415are not generation-fenced. After a workspace PID/runtime replacement, an old permission can be sent to the replacement instance, or a late old reply can publishyolo.autoAcceptedand clear current UI state. Advance the instance generation on stream rotation, clear runtime-scoped pending state, rehydrate persisted root settings, and ignore late reply completions from older generations. -
P1: the common tool-output DOM cap is bypassed by always-mounted titles and task summaries. Collapsed tools render the complete
state.title/ renderer title atpackages/ui/src/components/tool-call.tsx:950-999,1120-1124. Legacy nested-task rows retain up to 200 entries but render each unbounded title/description atpackages/ui/src/components/tool-call/renderers/task.tsx:430-443,499-523. A single multi-megabyte title, or 200 large summary descriptions, recreates the oversized DOM #553 is meant to prevent even with output collapsed. Apply a small common header/row text limit while preserving full data only behind explicit lazy copy/access.
Non-blocking follow-ups
-
P2: input-blocked payload memory is omitted from the 64 MiB accounting.
packages/ui/src/stores/message-v2/instance-store.ts:372-396measures session/message/usage data, then protects sessions with permission/question entries without including those canonical queue objects in the size. Large interruption payloads can exceed the advertised budget invisibly. Include each session-associated request once in retained-size accounting or cap those queues separately. -
P2: command and shell failures are all classified as delivery-ambiguous.
packages/ui/src/stores/session-actions.ts:274-366wraps even definite HTTP validation failures withsuppressPromptRecovery;packages/ui/src/components/prompt-input.tsx:491-509,541-570has already cleared the text. A definite 4xx loses recoverable shell/command input. Preserve input for definite failures and retain a separate delivery-unknown record for genuinely ambiguous outcomes instead of silently discarding it. -
P2: the loopback client replaces caller cancellation and times only response headers. The installed SDK invokes custom fetch as
fetch(Request), butpackages/server/src/workspaces/instance-client.ts:56-62reads onlyinit.signal, replaces the signal carried by the Request, and clears its timer when fetch returns headers. Caller cancellation is lost and a stalled body can hang indefinitely. Combine the Request signal with the timeout and keep timeout authority through body completion, or narrow the documented guarantee and preserve caller cancellation. -
P3: cleanup leaves earlier PR-artifact transcript data orphaned. Current source has no stale transcript-cache imports or runtime hooks, and startup correctly restores UI/session metadata before authoritative HTTP hydration. However,
packages/ui/src/stores/client-state.ts:232-236clears only native client state; users who ran earlier #626 artifacts can retain the deletedcodenomad-session-messagesIndexedDB database and unsafe marker. Keep a one-time deletion migration, then remove it after affected artifacts age out.
Better implementation assessment
Removing the experimental persistent transcript cache was the right simplification: OpenCode remains authoritative and the normal desktop tab/draft/scroll restore path is intact. Whole-session eviction is also simpler and safer than partial transcript restoration. The remaining design is not merge-ready because bounds are fragmented across renderers, recovery is recursive instead of scheduled, and runtime authority is not one end-to-end generation across server, transport, and UI.
Validation assessment
All current GitHub checks pass at 59b415c0, including Linux/macOS/Windows Electron and Tauri builds, the UI/server/Rust test jobs, and the serialized Windows Tauri suite. git diff --check passes and the review worktree is clean. CI success is insufficient here: there is no WebKit soak, native oversized-frame test, evict/SSE/reopen scroll integration, reconnect interruption reconciliation, sustained revision-conflict recovery test, or auto-accept runtime-rotation test. The manually enumerated UI CI list at .github/workflows/pr-build.yml:104-145 also omits the new packages/ui/src/components/prompt-input/submitPrompt.test.ts.
GitHub does not permit a self-authored REQUEST_CHANGES review on this PR, so this is published as a COMMENT review with an exact gate verdict of REQUEST CHANGES.
Preserve complete-session search, scroll restoration, prompts, interruptions, and reconnect behavior while keeping renderer work and retained state bounded. Fence reconnect and revision recovery by runtime generation and abort authority, reconcile complete snapshots safely, make search cancellable and resumable, and keep delayed permission and question events from reviving resolved work. Cancel stale auto-accept and SDK operations across runtime rotation, bound native SSE parsing and batching without rejecting server-valid events, and cover the recovered races in UI, server, and Rust tests. Validated with UI and server typechecks, UI production builds, targeted CI-equivalent UI/server suites, Electron native tests, and the locked Tauri test suite.
Make reconnect, search, hydration, revert, delta, and generation recovery share explicit runtime and mutation authority so stale responses cannot delete, prune, or resurrect session state. Measure all resident transcripts incrementally, preserve recoverable ambiguous drafts and complete tool history access, and keep hidden sessions evictable under sustained updates. Harden auto-accept persistence and queued topology across rotation, scope native stream starts and stops with persistent leases, and preserve ordered status and delta events within strict byte bounds. Extend CI coverage for native reconnects, task transcript remounts, workspace synchronization, diagnostics, and prompt-display eviction. Validated with UI/server typechecks, production UI build, 219 standard UI tests, 178 browser UI tests, 72 server tests, 99 Rust tests, and 118 Electron tests.
pascalandr
left a comment
pascalandr
left a comment
There was a problem hiding this comment.
Gatekeeper verdict: no findings
Reviewed the complete PR range from base \1d4f77869e9d7d2c6820bc3f4515dddabab19758\ through head \2c3dd0a8\ after the remediation passes.
No reproducible high- or medium-severity merge blockers remain in session authority, reconnect/revert handling, revision and delta recovery, memory eviction, full-session search/access, ambiguous prompt recovery, auto-accept rotation, or native desktop event ordering and bounds.
Validated locally with:
- 219 standard UI tests
- 178 browser-condition UI tests
- 72 affected server tests
- 99 locked Rust tests
- 118 Electron native tests
- UI, Electron, and server typechecks
- production UI build
- clean \git diff --check\
Residual risk: native webview reload behavior is covered at the UI/Rust unit boundary rather than by a packaged end-to-end Tauri test.
Create the rejected session-list result when the reconnect code invokes the mock instead of during test setup. This prevents Node's test runner from reporting an unhandled rejection before the production recovery path attaches its handler under slower Linux CI scheduling. Validated with the reconnect resync test file and the two previously failing serialized Windows cross-host tests.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/30771101257 Artifacts expire in 7 days.
|
Bring PR NeuralNomadsAI#626 onto upstream/dev at c16cc00, including the merged workspace configuration, failed prompt history, desktop, picker, and right-panel updates. Resolve the selective CI workflow conflict by retaining both the branch global-cache test and upstream launch-error test coverage.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/30800083351 Artifacts expire in 7 days.
|
Prevent the global memory sweep from clearing message stores that are still consumed by cached SessionView panes. Cached panes now hold visibility leases until unmounted, while inactive instances release their panes and leases so memory can still be reclaimed. Remove eager cache-trim eviction and cover the six-session boundary plus inactive-instance cleanup with a focused regression test. Validated with UI typechecking, a release Tauri build, and a monitored manual reproduction that exercised repeated evictions without stale Solid reads or Virtua failures.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/30804146427 Artifacts expire in 7 days.
|
Wait for healthy initial session hydration before reconnect resync claims commit authority. This prevents reconnect startup events from invalidating the hydration that restores idle and interrupted markers, while retaining the bounded timeout path for stalled hydration. Add a regression test covering reconnect during healthy initial hydration and keep the stalled-hydration test's timeout explicit. Validated with targeted Bun tests, UI typecheck, and a packaged Tauri restart preserving an idle marker.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/30826423962 Artifacts expire in 7 days.
|
1 participant
Problem addressed
This PR closes #553 and addresses the linked Discord report.
What changes for users
Scope
This PR does not add persistent or offline transcript caching. OpenCode remains the single authority for message history, and evicted sessions reload their complete history over HTTP.
Existing desktop restoration of workspace tabs, selected sessions, drafts, scroll positions, and layout remains unchanged. Clearing saved startup state does not delete project files or OpenCode conversations.
Validation
Current head:
59b415c0A 24-hour WebKit memory soak and the Discord reporter's exact five-workspace concurrent stress case have not been run. Simultaneously active or streaming sessions are intentionally protected from eviction, so resident message memory may temporarily exceed 64 MiB while work is running; their rendered output remains bounded, and they become eviction candidates once idle. A single visible session also remains complete by design.
Closes #553