Skip to content

fix(desktop): invalidate stale runtimes before profile reopen - #94927

Closed
Tomenatore wants to merge 1 commit into
NousResearch:mainfrom
Tomenatore:fix/desktop-stale-runtime-after-backend-exit
Closed

Tomenatore wants to merge 1 commit into
NousResearch:mainfrom
Tomenatore:fix/desktop-stale-runtime-after-backend-exit

Conversation

@Tomenatore

Copy link
Copy Markdown

Summary

  • remember secondary gateway scopes across renderer pool pruning
  • invalidate process-local stored-session → runtime bindings before a later socket can publish open
  • keep busy-state reconciliation post-open and best-effort
  • add a regression test for the prune → fresh entry → direct reopen path

Problem

A profile backend can be evicted after its renderer-side Secondary entry is pruned while the renderer still holds that backend generation's runtime ID. The next profile action creates a fresh Secondary, respawns/reopens the backend, and can immediately issue a session-scoped RPC with the old process-local ID. The new backend rejects it as session not found / no active session; reloading the window happens to recover only because it clears the renderer bindings.

The existing cleanup ran after reconnectSecondary(). It did not cover a direct openSecondary() caused by the next routed user action, and post-connect cleanup also left a race in which eager route/submit work could observe open first.

This is complementary to #94656: that PR improves owner continuity and foreground retention; this change covers the backend-generation boundary when a secondary was nevertheless disposed or restarted.

Approach

  • Track scopes that have successfully opened during the current renderer generation, independently of an individual pool entry.
  • On any later open of that scope, await the existing scoped runtime invalidation before dialing the socket.
  • Add the scope to the ledger only after a successful connection.
  • Clear the ledger with the full secondary-gateway teardown.
  • Preserve the previous post-open, best-effort busy-state reconciliation semantics.

Verification

  • Regression test fails on pre-fix upstream/main with zero runtime-invalidation calls.
  • npx vitest run --project ui on six affected gateway/session files: 102 passed.
  • npm run typecheck: passed.
  • ESLint on both changed files with --max-warnings 0: passed.
  • Prettier check on both changed files: passed.
  • npm run build: passed, including renderer, Electron main/preload bundles, and native dependency staging.
  • Full npm run test:ui: 5,718 passed / 12 failed on this Windows/de-DE host. Four locale-sensitive assertions reproduce identically on an unchanged upstream/main worktree; the remaining full-load timeouts passed when the same baseline files were rerun in isolation. No changed-file or affected gateway/session test failed.

Risk

Initial secondary opens are unchanged. Reopens add one local dynamic-import microtask before transport publication and conservatively re-resume bindings for only the reconnected scope. If UI-state reconciliation itself throws, the already-open transport remains successful as before.

Related to #93937, #94828, and #94872.

Remember secondary scopes across renderer pool pruning and clear their process-local runtime bindings before a later socket can publish open. This prevents eager session RPCs from racing a respawned profile backend with an ID minted by the previous process.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/desktop Electron desktop app (apps/desktop/*) area/sessions Session lifecycle, resume, persistence, history sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Aug 25, 2026
@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference, author can ignore or act on any point.

Moving the invalidation from reconnectSecondary's post-open fire-and-forget into openSecondary itself, and splitting it into "reset bindings BEFORE connect / reconcile busy flags AFTER connect" (gateway.ts:349-378, 400-407), closes the actual hole: a direct reopen after pruneSecondaryGateways no longer gives an eager route effect a window to send a runtime id the respawned backend never minted. The test pinning resetTileRuntimeBindings invocation order before the second connect is the right regression contract, and the generation ledger (openedSecondaryScopes, gateway.ts:109, 160-163) with the HMR-tolerant lazy accessor is thoughtful — surviving one prune is precisely the scenario that motivated this.

A few observations:

  1. First-open false positives are cheap but real. reopening is entry.openedOnce || entry.connection !== null || openedScopes.has(entry.scope) (gateway.ts:362). Any path that assigns entry.connection before the first connect (or any HMR-shaped state handoff) makes a first-ever dial take the invalidation branch — a wasted dynamic import plus a no-op reset and a busy-reconcile against an empty store. Harmless, but it means the branches guarding "best effort" catches will also run on cold starts, which slightly muddies the semantics of the flag. A short comment that false positives are safe-by-design would prevent a future "optimization" from narrowing the condition and re-opening the race.

  2. Reset-then-failed-connect state. If connect throws after bindings were reset (gateway.ts:399), tiles now show cleared runtime state with no backend — and the backoff retry loops back through openSecondary, resetting again. That's consistent, but it changes the failure-mode UX compared to before (bindings used to survive a failed re-dial until success). If any tile renders "session gone" rather than "reconnecting" after a reset, a failed period may flash the wrong message. Worth a manual check on the tile rendering of cleared bindings while connectPromise is pending.

  3. Ledger eviction. openedSecondaryScopes is only cleared in closeSecondaryGateways (gateway.ts:1168). Growth is bounded by scopes actually dialed in this renderer generation, so fine — just noting the intent is "never forget within a generation", and nothing granular should be added later without rethinking the reopen contract.

Well-scoped fix with a genuinely useful ordering assertion in the test.

@teknium1

Copy link
Copy Markdown
Collaborator

Merged via #95080 at 682a34a — your commit was cherry-picked into the consolidated salvage PR with authorship preserved, so this work carries your name in git history.

Thanks for invalidating stale runtimes before profile reopen — that stale-runtime window was a subtle one to spot.

Closing this original PR now that the consolidated branch has landed on main. Thanks for contributing to the desktop multi-gateway campaign (tracker: #94724).

@teknium1 teknium1 closed this Aug 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/desktop Electron desktop app (apps/desktop/*) P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants