Skip to content

fix(desktop): release reconnect-orphaned warm transcripts once their authoritative state settles - #95338

Closed
BrunoBza wants to merge 1 commit into
NousResearch:mainfrom
BrunoBza:fix/95189-evict-orphaned-session-transcripts
Closed

BrunoBza wants to merge 1 commit into
NousResearch:mainfrom
BrunoBza:fix/95189-evict-orphaned-session-transcripts

Conversation

@BrunoBza

Copy link
Copy Markdown
Contributor

What

Adds an optional liveness probe to SessionStateCache so warm transcripts orphaned by a mid-turn connection death stop being pinned forever by frozen busy/awaitingResponse flags.

Partially addresses #95189 (the renderer-side retention guardrail; see scope note below).

The leak

When a gateway connection dies mid-turn, reconnect reconciliation (reconcileBusyStatesOnReconnect) correctly settles the authoritative $sessionStates record. But the per-session transcript cache keeps its own snapshot under the old runtime id - and the respawned backend re-mints runtime ids, so no terminal publish ever reaches that snapshot again. Its cached busy: true is frozen forever.

#isWarmSettled treated those frozen flags as live work, so the orphaned cache entry was never evictable: every reconnect cycle stranded another full warm transcript (~5MB) until app restart. Under the ~2-minute restart loop reported in #95189 that churn compounds into the renderer OOM the reporter measured (~5GB/day growth).

Fix

SessionStateCache now accepts an optional isAuthoritativelyActive(runtimeId) callback. When wired:

  • in-flight flags block eviction only while the authoritative store still claims work for the same runtime id;
  • once reconciliation settles that record (or wipes it), the frozen flags stop pinning the transcript and the entry drains through the normal LRU path - ownership cleanup included;
  • eviction stays gated on needsInput, pending drafts/in-flight messages, and active references, so a genuinely running turn (which re-asserts busy on every publish) is never a casualty.

Without the probe the behavior is byte-for-byte identical to today - existing constructions keep the always-block semantics.

The only production wiring is useSessionStateCache, which reads the same $sessionStates store it already imports.

Regression coverage (behavior-contract)

All new tests verified RED on pristine origin/main (71d804d) before the fix and GREEN after:

  1. orphaned busy/awaitingResponse transcripts are released once the authoritative record settles (probe wired);
  2. in-flight transcript whose authoritative record was wiped entirely (soft gateway-mode apply) is released;
  3. in-flight transcript stays pinned while the authoritative record still claims work;
  4. without the probe, in-flight transcripts are still never evicted (legacy contract pinned);
  5. hook-level integration: under the production budget (24 entries), reconnect churn drains the orphaned entry first while a genuinely running turn survives untouched, reverse ownership map cleaned.
  • Target suites: 30/30 passed
  • Neighbor regression batch (session-states x7 files incl. reconnect/watchdog/eviction/scopes/runtime-map, session-watchdog, gateway-switch, both session-state-cache suites): 129/129 passed

tsc -p . --noEmit: no diagnostics in touched files (only the pre-existing @tabler/icons-react declaration errors on unrelated files, reproduced on pristine main). ESLint and Prettier clean.

Scope note

This covers the guardrail half of #95189 ("mark stalled sessions evictable"). The unclean-exit restart loop itself is WSL2-environmental and needs lifecycle-sentinel forensics (boot-id instrumentation) - the direction already sketched by @Finn763 in #95237 (comment), which explicitly reserved "renderer-side retention guard ... worth its own issue+PR pair". This PR is that pair's renderer half; it does not touch the pool-eviction code reviewed there.


Bruno Bza (@BrunoBza)

…authoritative state settles

A gateway connection that dies mid-turn leaves cached session snapshots
whose busy/awaitingResponse flags can never settle: the respawned
backend re-mints runtime ids, so no terminal publish ever reaches the
orphaned snapshot again. #isWarmSettled treated those frozen flags as
live work, so every orphan pinned its full warm transcript until app
restart — roughly 5MB per reconnect cycle, which turned the restart
loop in NousResearch#95189 into renderer OOM.

SessionStateCache now accepts an optional isAuthoritativelyActive
probe. When wired, in-flight flags only block eviction while the
authoritative $sessionStates record still claims work for the same
runtime id; without the probe the legacy always-block behavior is
preserved byte-for-byte. Eviction remains gated on needsInput, pending
drafts, and active references, so a genuinely running turn (which
re-asserts busy on every publish) is never a casualty.

The useSessionStateCache hook wires the probe to the store it already
imports. Reconnect reconciliation (reconcileBusyStatesOnReconnect)
settles the authoritative record, and the next prune drains the
orphaned cache entry through the normal LRU path, ownership included.
@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 26, 2026
@Halldrix

Copy link
Copy Markdown
Contributor

This lands exactly right, and for full transparency: it's the deterministic criterion laid out in #95276 (comment) — trust frozen in-flight flags only while the authoritative $sessionStates record still claims work for the same runtime id. Good to see the seam traced independently and implemented faithfully.

What I checked against source, all clean:

  • The leak mechanism is correctly addressed at its rootreconcileBusyStatesOnReconnect (src/store/session-states.ts:404-420) heals the atom but not the hook-mounted cache copy; the probe closes precisely that gap rather than papering over it with a timer.
  • Legacy safety: without the probe wired, behavior is byte-identical to today (isAuthoritativelyActive?.() !== false keeps always-block semantics) — and test 4 pins that contract explicitly. Existing constructions can't regress.
  • The other eviction gates stay intact: needsInput, draft/in-flight messages, and reference checks all still block; only the stale busy/awaitingResponse pin is relaxed, and only when authority agrees.
  • Ownership hygiene: eviction cleans runtimeIdByStoredSessionIdRef via the existing onEvict path — the reverse-map staleness class is handled.
  • Tests are the right shape: RED-on-pristine-main claims match what I'd expect (the orphaned-busy entry survives prune() today), and the hook-level churn test exercises the production budget with a genuinely running turn surviving the drain.

One non-blocking observation for the record: there's a small theoretical window during a transient blip on a live socket — reconciliation clears the turn's busy in the atom, and if prune() runs before that turn's next event re-asserts busy, a still-live background transcript can be evicted mid-turn. This is acceptable because (a) it's the same contract already accepted upstream ("a live turn re-asserts busy on its next event", session-states.ts:395-398), (b) exposure is bounded — prune only drains under budget pressure, in LRU order, and referenced/foreground states are spared — and (c) recovery is cheap: resume reloads the transcript from the backend. Worth a line in the callback docs someday, not a blocker.

Residual gap (fine as follow-up): snapshots whose scope outlived the socket and were never reconciled — e.g. preserved needsInput states — aren't touched by this probe. The bounded-window idea from #95276 composes cleanly there if that case ever shows up in practice.

Nice work — this is the renderer half of #95189 done properly.

@teknium1

Copy link
Copy Markdown
Collaborator

Merged via wave-2 Phase B salvage PR #95495 (b455abe) — your #isWarmSettled unpinning landed with your authorship, releasing reconnect-orphaned warm transcripts (the renderer half of #95189). Clean, narrow fix — thanks. Tracker #94724.

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