Skip to content

fix(desktop): keep new-chat binding stable during attachment sync - #64759

Closed
DESXIE wants to merge 7 commits into
NousResearch:mainfrom
DESXIE:fix/desktop-binding-attachment-race
Closed

fix(desktop): keep new-chat binding stable during attachment sync#64759
DESXIE wants to merge 7 commits into
NousResearch:mainfrom
DESXIE:fix/desktop-binding-attachment-race

Conversation

@DESXIE

@DESXIE DESXIE commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Problem

After #63624, a new text-only chat sends correctly, but the first send can still bounce back to the composer when it includes an attachment.

Session creation schedules navigation to the newly persisted chat. If React Router commits that navigation while attachment staging is awaiting, the submit guard can mistake the delayed app-initiated route update for a user session switch and abort before prompt.submit.

Fix

Return the exact runtime, stored-session, and target-route binding created by createBackendSessionForSend:

{ routeToken: string | null; runtimeSessionId: string; storedSessionId: string | null }

The submit pipeline now:

  • validates that binding instead of inferring ownership from live UI state;
  • accepts the expected delayed route transition exactly once;
  • rejects unrelated route-only navigation, stored-session mismatches, and runtime ABA rebinding;
  • preserves session.resume recovery for the newly created stored session when the first submit times out or reports session not found;
  • keeps user-driven cancellation during async creation silent; and
  • associates optimistic seed/rewrite/drop updates with the created stored session.

The prompt and slash callers also reject a stale binding if the user switches context during optional post-create setup.

Verification

  • npm run test:ui -- src/app/session/hooks/use-prompt-actions/index.test.tsx src/app/session/hooks/use-session-actions.test.tsx72/72 passed (53 prompt-action + 19 session-action tests)
  • npm run typecheck — passed
  • focused ESLint on the six changed Desktop files — 0 errors (one pre-existing react-hooks/exhaustive-deps warning in the test harness)
  • git diff --check — passed
  • manual packaged Windows Desktop verification of the reported race before the follow-up hardening changes: a new chat with image + text sent on the first click

Regression coverage includes delayed attachment-route commit, unrelated route-only navigation, real and partial session switches, runtime ABA rebinding, created-session timeout recovery, silent create cancellation, optimistic stored-session association, and post-create async setup.

Related

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/desktop Electron desktop app (apps/desktop/*) sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Jul 15, 2026
@DESXIE
DESXIE force-pushed the fix/desktop-binding-attachment-race branch from 0921628 to 282546c Compare July 15, 2026 05:29
@DESXIE DESXIE changed the title fix(desktop): preserve created session binding through attachment sync fix(desktop): keep new-chat binding stable during attachment sync Jul 15, 2026
@DESXIE
DESXIE force-pushed the fix/desktop-binding-attachment-race branch from 282546c to ab8a1d3 Compare July 16, 2026 00:53
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused follow-up. The premise is present on current main: apps/desktop/src/app/session/hooks/use-prompt-actions/submit.ts:337 snapshots the newly created route before awaiting attachment staging at :342-345; a delayed Router update then triggers the drift abort at :347-349 before prompt.submit.

The explicit runtime/stored/route binding in this PR addresses that interval while retaining the existing session-identity checks. It also matches the live route-token representation in apps/desktop/src/app/contrib/wiring.tsx:142-145. The PR base (3f2a389c) is an ancestor of current main, and the two subsequent main commits are unrelated skill-parser fixes, so this appears mechanically salvageable.

No blocking issue identified by static review. This is an automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 16, 2026
@ildunari

Copy link
Copy Markdown
Contributor

Independent confirmation from a real macOS Desktop repro:

  • A text-only first send from / succeeds.
  • A first send with image attachments creates and title-seeds the session, then drops the optimistic message before prompt.submit; the rejected draft is restored into the new-chat scope, so the next Cmd+N inherits the previous text and images.
  • Instrumentation/local red-first coverage confirms the route commit lands while image.attach is awaited. The post-sync drift check then mistakes the app's own delayed navigation for a user switch.

I checked out this PR at ab8a1d3e4 and ran:

npx vitest run --project ui \
  src/app/session/hooks/use-prompt-actions/index.test.tsx \
  src/app/session/hooks/use-session-actions.test.tsx

Result: 72/72 passed. The explicit {runtimeSessionId, storedSessionId, routeToken} binding here is stronger than the minimal two-token workaround I initially prototyped: it preserves the intended switch/ABA protections while accepting only the created session's delayed route transition. This matches and fixes both user-visible symptoms from the MacBook repro.

@ethernet8023

Copy link
Copy Markdown
Collaborator

I believe that #65890 will fix this. @DESXIE can you take a look?

@alt-glitch alt-glitch added the needs-decision Awaiting maintainer decision before any implementation label Jul 16, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #65708 and #64327 are open alternate fixes for the same desktop submit-drift family. This binding-triple approach specifically covers the delayed attachment-sync route transition.

@DESXIE

DESXIE commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

I believe that #65890 will fix this. @DESXIE can you take a look?

Thanks — I checked #65890. Disabling Router transitions is a useful improvement and should reduce the window in which the created-chat route commit is delayed.

However, it does not fully replace this PR's guard. The attachment path still crosses async boundaries, and correctness cannot rely on the route commit always winning that race. This PR carries the explicit runtime/stored-session/route binding through attachment staging, accepts only the created session's delayed route transition, and retains the existing protections for unrelated route changes, runtime rebinding, and created-session recovery.

I'm rebasing this on the newer routed-session/queued-submit work now and will update the branch once the combined regression coverage is green.

@alt-glitch alt-glitch removed the sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages label Jul 17, 2026
@DESXIE
DESXIE force-pushed the fix/desktop-binding-attachment-race branch from ab8a1d3 to b8d46db Compare July 17, 2026 02:42
@DESXIE
DESXIE force-pushed the fix/desktop-binding-attachment-race branch from fb9a134 to 5bd44b1 Compare July 20, 2026 00:47
DESXIE added 7 commits July 22, 2026 17:11
Keep the exact runtime, stored-session, and target-route binding from new-chat creation so a delayed React Router commit during image attachment staging is accepted once without weakening genuine session-switch or ABA protections.
Reject unrelated route-only navigation after session creation, preserve resume recovery for the newly created stored session, and keep user-driven create cancellation silent.
Provide the structured created-session binding required by the shared submit hook for tile-scoped sessions.
Keep queue drains scoped to their durable target while retaining created-session binding checks.
@DESXIE
DESXIE force-pushed the fix/desktop-binding-attachment-race branch from 5bd44b1 to f19bbf7 Compare July 22, 2026 09:19
@DESXIE

DESXIE commented Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

Closing as superseded by upstream work that landed on main after this PR.

Why this is no longer the right merge path

This PR explored an explicit created-session binding:

{ runtimeSessionId, storedSessionId, routeToken }

so the first new-chat send with attachments would not treat the app's own delayed React Router commit as a user switch.

That class of false-positive drift aborts is now covered on current main by:

  • fix(desktop): scope submit drift guard to genuine session switches #69578fix(desktop): scope submit drift guard to genuine session switches
    • shared session-context-drift helper
    • only aborts on a real move to a different chat
    • treats moves onto the submit's own target (create/resume re-home) as non-drift
    • ignores selection null-resets and search/hash-only route churn
  • first-send regression pins now on main:
    • intentional new-chat route transition delivery
    • late React Router route commit still delivers the first prompt
    • genuine post-create switch during attachment sync still aborts

I verified on current main (760112adb):

session-context-drift.test.ts — 16/16 passed
use-prompt-actions new-chat first-send delivery suite — 4/4 passed

Decision

Forcing another rebase of this branch would reintroduce a competing create-return contract against the newer drift helper and keep generating conflicts without a remaining independent production gap.

Thanks to everyone who reviewed and reproduced the original bounce, including the earlier #63624 path and the alternate first-send attempts. Closing this as superseded rather than continuing to fight the current main architecture.

@DESXIE DESXIE closed this Jul 25, 2026
@DESXIE
DESXIE deleted the fix/desktop-binding-attachment-race branch July 25, 2026 07:08
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/*) needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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.

5 participants