Skip to content

fix(desktop): prevent prompt.submit targeting stale runtime session when identity is already split - #64814

Closed
Apaarmeet wants to merge 0 commit into
NousResearch:mainfrom
Apaarmeet:main
Closed

Apaarmeet wants to merge 0 commit into
NousResearch:mainfrom
Apaarmeet:main

Conversation

@Apaarmeet

Copy link
Copy Markdown

What does this PR do?

Fixes a race condition in the Desktop app where prompt.submit could send a message to a stale runtime session when the three session identities (runtime session, selected stored session, route token) were already split at submit entry. The existing #54527 drift guard only detects stored/route changes during the async pipeline, not a split that already exists at entry (e.g. runtime=A, stored=B, route=B). Adds an entry-time consistency check that verifies the runtime session belongs to the selected stored session before accepting it as the submit target, redirecting to the resume path when they don't match.

Related Issue

Fixes #64789

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • apps/desktop/src/app/session/hooks/use-prompt-actions/submit.ts — Added getStoredSessionIdForRuntimeId to SubmitPromptDeps interface, destructured from deps, added to useCallback dep array. Added entry-time consistency check after line 231 that nulls out the runtime session ID when it doesn't belong to the selected stored session, causing the existing resume path to pick up the correct session.
  • apps/desktop/src/app/session/hooks/use-prompt-actions/index.ts — Added getStoredSessionIdForRuntimeId to PromptActionsOptions interface, threaded through to useSubmitPrompt.
  • apps/desktop/src/app/desktop-controller.tsx — Wired getStoredSessionIdForRuntimeId using storedSessionIdForNotification wrapper over the existing runtimeIdByStoredSessionIdRef.
  • apps/desktop/src/app/session/hooks/use-prompt-actions/index.test.tsx — Added getStoredSessionIdForRuntimeId to Harness component (optional prop with default). Added regression test covering the A/B/B split scenario (runtime A / stored B / route B at entry, no changes during pipeline).

How to Test

  1. Open Desktop with conversation A active (runtime session = A, stored session = A)
  2. Navigate to conversation B in the sidebar — route updates to /B, stored session updates to B, but if the runtime session A hasn't been reaped, activeSessionId remains A
  3. Type a message and submit
  4. Before fix: prompt.submit({ session_id: "rt-session-a" }) — message lands in A, permanently lost from B
  5. After fix: the consistency check detects runtime A doesn't belong to stored B, nulls sessionId, the resume path resumes stored B, and prompt.submit targets the correct runtime session for B

Unit test: npx vitest run src/app/session/hooks/use-prompt-actions/index.test.tsx — 46 tests pass including the new #64789 regression test.

Checklist

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(desktop): prevent prompt.submit targeting stale runtime when identity is already split)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run the test suite — 141 tests pass across 15 session test files, 46 tests in prompt-actions including new test
  • I've added tests for my changes
  • I've tested on my platform: macOS 26.5.1

Documentation & Housekeeping

  • N/A — no config keys, documentation, or architecture changes
  • N/A — renderer-only TypeScript change, no cross-platform impact

Copilot AI review requested due to automatic review settings July 15, 2026 07:23

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a Desktop submit-path race where prompt.submit could target a stale runtime session even though the UI’s stored session selection and route token already pointed at a different conversation at submit entry time (the pre-existing split state wasn’t caught by the existing “drift during async pipeline” guard). It adds an entry-time ownership check so the submit path falls back to the resume flow when the runtime session does not belong to the selected stored session, and includes a regression test for the A/B/B split scenario.

Changes:

  • Adds getStoredSessionIdForRuntimeId plumbing and an entry-time consistency check in useSubmitPrompt() to invalidate mismatched runtime session IDs.
  • Threads getStoredSessionIdForRuntimeId through usePromptActions() and wires it up from DesktopController using the runtime↔stored session cache.
  • Adds a Vitest regression test covering the “runtime A / stored B / route B at entry” misrouting scenario (#64789).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
apps/desktop/src/app/session/hooks/use-prompt-actions/submit.ts Adds an entry-time runtime→stored ownership check to avoid submitting to a stale runtime session when identities are already split.
apps/desktop/src/app/session/hooks/use-prompt-actions/index.ts Threads the new dependency through usePromptActions into the submit hook.
apps/desktop/src/app/desktop-controller.tsx Provides the runtime→stored lookup function from the session cache mapping.
apps/desktop/src/app/session/hooks/use-prompt-actions/index.test.tsx Adds a regression test for the pre-existing A/B/B split at submit entry (#64789).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread apps/desktop/src/app/session/hooks/use-prompt-actions/submit.ts Outdated
Comment thread apps/desktop/src/app/desktop-controller.tsx Outdated
@alt-glitch alt-glitch added type/bug Something isn't working 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 labels Jul 15, 2026
@Rika-xie

Copy link
Copy Markdown

Thanks for picking this up. The root-cause diagnosis is correct: the existing drift guard does not catch an A/B/B split that already exists at submit entry.

I checked the current PR merge ref and reproduced one remaining unsafe path.

1. Reverse-map misses still fail open

DesktopController wires getStoredSessionIdForRuntimeId through storedSessionIdForNotification(). On a map miss, that helper returns the input runtime ID rather than null.

This means the current condition can accept an unverified runtime:

runtime = A
selected stored = B
route = B
reverse lookup for A misses
storedSessionIdForNotification(A, map) returns A
ownerStoredId === sessionId
sessionId remains A
prompt.submit(session_id=A)

A temporary regression test reproduced this exact result:

AssertionError:
expected prompt.submit(session_id="rt-session-a-unknown")
to be undefined

The committed regression test only covers the cache-hit case because it pre-populates:

stored-project-a -> rt-session-a

2. Resume failure can silently retarget to a new session

When the ownership check does reject A, session.resume(B) can fail and the existing fallback then creates a new session C. That changes the outcome from “send to B” to “send to C” rather than failing closed.

Suggested minimum repair

  • Use a dedicated runtime-owner lookup whose miss result is null; do not reuse the notification fallback helper.
  • Accept the runtime only when it is explicitly proven to belong to the selected stored session.
  • If B must be resumed and resume fails or returns no runtime ID, abort the submit rather than creating C.
  • Add tests for reverse-map miss, stale/cross mapping, same-value IDs, and resume failure.

The current patch fixes the known cache-hit instance, but does not yet close the full #64789 bug class.

@Apaarmeet

Apaarmeet commented Jul 15, 2026 •

Copy link
Copy Markdown
Author

Thankyou for reviewing my changes, I have addressed all the remaining issues in my current patch and edit the tests for that and all 50 test passed

This branch was previously deployed

1 inactive deployment
github-pages — b6c11a35 Deployed Jul 15, 2026 by teknium1 via deploy-docs #1351
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/*) 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.

[Desktop] prompt.submit can target stale runtime A when route and stored session already point to B

5 participants