Skip to content

fix(desktop): close cross-session leak windows in composer + session refs (#59305) - #70610

Closed
JoaoMarcos44 wants to merge 3 commits into
NousResearch:mainfrom
JoaoMarcos44:fix/59305-session-composer-scope-leak
Closed

fix(desktop): close cross-session leak windows in composer + session refs (#59305)#70610
JoaoMarcos44 wants to merge 3 commits into
NousResearch:mainfrom
JoaoMarcos44:fix/59305-session-composer-scope-leak

Conversation

@JoaoMarcos44

@JoaoMarcos44 JoaoMarcos44 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Summary

Root-cause fix for #59305 ("[Desktop] Chat tab messages leak across sessions — cross-tab content mixing"). Full root-cause writeup in #70609.

Two independent React passive-effect race windows let a fast session switch land content in the wrong chat, plus a third gap where nothing checked that the composer and the session-side refs agreed with each other at the instant of send:

  1. use-session-state-cache.tsactiveSessionIdRef / selectedStoredSessionIdRef were mirrored from props via useEffect, which fires one commit after paint. A synchronous read in that window (a stable callback firing against the just-painted new session) still observed the outgoing session's ids.
  2. use-composer-draft.ts — the per-thread attachment-scope-swap effect (attachmentScope.$attachments.set(...) on session switch) was the same kind of useEffect. In the race window, $composerAttachments still held the outgoing session's chips — a submit fired in that window would ship them into the new session's turn. This is the actual content leak, not just a visual glitch.
  3. submit.ts / session-context-drift.ts — even with 1–2 fixed, the composer (ChatBar) and the session state cache live in separate React subtrees and had no cross-check against each other at send time.

Fix

  • use-session-state-cache.ts: session refs now mirror their prop synchronously during render instead of via useEffect. Guarded to fire only when the prop itself actually changed (replicating a useEffect([dep]) dependency array, just synchronously) — an unconditional resync was tried first and would have silently clobbered an intentional imperative ref pin (submit.ts pins a freshly-resumed runtime id into the ref without touching the source atom, by design) on the next unrelated re-render. Caught by a dedicated regression test before it shipped.
  • use-composer-draft.ts: the scope-swap effect is now a useLayoutEffect — runs before paint, closing the window. Also added a redacted [composer-rehydrate] diagnostic log (counts/kinds/scope only — never raw refs/URLs/paths) to help triage any future report in this class.
  • session-context-drift.ts: new third drift prong comparing the composer's loaded scope (SubmitTextOptions.composerScope, threaded from use-composer-submit.ts) against the submit target. Compared against the target resolved into the same durable lineage-root domain the composer itself uses (resolveComposerSessionKey), not the raw tip id — comparing against the raw tip would have false-positive-aborted every submit into any session that has ever gone through auto-compression. Verified with a dedicated regression test that reproduces the false-positive against the naive comparison before fixing it.
  • routes.ts / chat/index.tsx: the primary composer's durable scope key (primaryRouteSelectedSessionId) now prefers the route over a possibly momentarily-stale store selection during a switch.
  • chat-runtime.ts: attachmentId() normalizes the value (URL trailing slash, \ vs / paths) before hashing, so a re-attach of the same resource dedupes instead of appearing as a second attachment.

Test plan

  • New deterministic regression tests per fix (layout-phase probes, no sleep/timing-dependent repros) — each proven to fail against the pre-fix code, then pass after.
  • Regression test specifically proving the lineage-root vs live-tip false-positive is closed.
  • Regression test specifically proving the synchronous ref mirror does not clobber an imperative ref pin.
  • Full suite across the 17 touched/dependent test files: 286 passed.
  • tsc --noEmit: no new errors in any touched file.
  • Full project test:ui: no new failures introduced (pre-existing unrelated locale/timing failures confirmed identical via git stash comparison against the same base).

Closes #70609. Related to #59305.


PR #59305: Desktop Cross-Session Leak Window Closure

JoaoMarcos44 and others added 2 commits July 24, 2026 03:59
…refs (NousResearch#59305)

Two React passive-effect timing bugs let a session switch land in the wrong
chat: activeSessionIdRef/selectedStoredSessionIdRef (use-session-state-cache)
and the composer's attachment-scope swap (use-composer-draft) both mirrored
their source props via useEffect, which fires one commit AFTER the new
session's view has already painted — a synchronous read/submit in that window
observed the outgoing session's ids/attachments.

- use-session-state-cache.ts: mirror the session refs synchronously during
  render instead of a useEffect, guarded to fire only when the prop itself
  changed (not unconditionally) so an imperative pin from submit.ts /
  use-session-actions (e.g. a freshly resumed runtime id, intentionally not
  synced to the source atom) survives an unrelated re-render.
- use-composer-draft.ts: the per-thread attachment-scope-swap effect is now a
  useLayoutEffect, closing the window before paint.
- submit.ts / session-context-drift.ts: add a 3rd drift prong comparing the
  composer's loaded scope (SubmitTextOptions.composerScope) against the
  submit target, resolved into the same lineage-root domain
  (resolveComposerSessionKey) the composer itself uses — comparing against
  the raw tip id would false-positive-abort every submit into any session
  that has ever auto-compressed.
- routes.ts / chat/index.tsx: the primary composer's durable scope key now
  prefers the route over a possibly-stale store selection
  (primaryRouteSelectedSessionId).
- use-composer-draft.ts: redacted [composer-rehydrate] diagnostic log
  (counts/kinds/scope only, never raw refs) for future reports in this class.
- chat-runtime.ts: normalize attachment id values (url/path) before hashing
  so a re-attach with a trailing slash or backslash path dedupes correctly.

16 files, 286 tests across the touched/dependent suites (17 files) green,
including new regression coverage for each fix.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@alt-glitch alt-glitch added type/bug Something isn't working P1 High — major feature broken, no workaround comp/desktop Electron desktop app (apps/desktop/*) sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Jul 24, 2026
….test.tsx

CI's check:lint failed on two perfectionist rule violations introduced by the
new test file: type import ordering and missing blank line between the
parent-relative and same-directory import groups. No behavior change.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@teknium1

Copy link
Copy Markdown
Contributor

Merged via #70986 with your authorship preserved on both fix commits — outstanding work on this one: the #70609 root-cause analysis (two passive-effect race windows + missing submit coherence check) made the salvage mechanical, and all 141 tests + typecheck shipped clean. Only your infographic asset commit was dropped. Fixes #59305 and #70609.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/desktop Electron desktop app (apps/desktop/*) P1 High — major feature broken, no workaround 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.

[Desktop] Root cause of #59305: two passive-effect race windows + missing composer/session coherence check on submit

3 participants