Skip to content

fix(desktop): route each session RPC by its target session, not the focused tile - #93301

Closed
teknium1 wants to merge 1 commit into
mainfrom
fix/route-rpc-by-target-session
Closed

teknium1 wants to merge 1 commit into
mainfrom
fix/route-rpc-by-target-session

Conversation

@teknium1

Copy link
Copy Markdown
Collaborator

The bug

Bot Mode: messaging any bot except the launch profile silently runs on the default backend (served via ?profile= from the default's state.db), or fails with "session not found" (4001) when the default backend doesn't hold the runtime session. Verified on a live install — logs show the bot's turn executing in the default backend's log while the bot's own per-profile backend sits idle.

Root cause

requestGateway in apps/desktop/src/app/contrib/wiring.tsx is one shared closure used for every session-scoped RPC in the window, but it derived the owning profile from the globally-focused tile:

const routingSessionId = $focusedStoredSessionId.get() ?? selectedStoredSessionIdRef.current

A bot chat is a background tile while another pane is active. So its prompt.submit carried the bot's own session_id in params, but was routed by the focused tile's owner (the default chat) → default backend. This is downstream of the tile-ownerRoute fix (#92956): that made bot chats carry their owner, but the router still read the wrong session's owner.

Fix

Route by the session the RPC targets (params.session_id), not the focused tile. session_id is a runtime id while tiles/rows key on the stored id, so translate via the state cache, then a reverse scan of the stored→runtime map (the same ladder use-session-tile-delegate's storedSessionIdForRuntime uses). An unresolved id is already a stored id (several RPCs pass stored ids directly). RPCs with no session_id (ambient/config calls) keep the focused→selected fallback.

Pure helpers (findStoredIdForRuntimeId, resolveRoutingSessionId) extracted to wiring-routing.ts so they're unit-testable without importing the React controller (there was no wiring.tsx test before — that gap is why this class of bug went uncaught).

Verification

  • tsc --noEmit: 0 errors.
  • 6 new tests in wiring-routing.test.ts: target-vs-focused routing (the misroute), stored-id passthrough on runtime miss, and the no-session focused→selected→null fallback.
  • CI runs the vitest UI suite.
  • Needs a desktop rebuild to go live; acceptance test is messaging an idle background bot and getting a reply on its own backend (no 4001, no wrong-backend execution), with a normal session still working.

Credit

Root cause + the target-vs-focused routing approach were diagnosed collaboratively; this PR packages it with the runtime→stored translation, extracted testable helpers, and coverage.

…ocused tile

The Bot Mode 'session not found' / bot-runs-on-wrong-backend bug. wiring's
requestGateway is ONE shared closure for every session-scoped RPC in the
window, but it derived the owning profile from the globally-FOCUSED tile
($focusedStoredSessionId). A bot chat is a background tile while another pane
is active, so its prompt.submit carried the bot's own session_id yet was
dispatched on the FOCUSED tile's backend — the default backend served the bot
via ?profile= from the default's state.db, or answered 4001 'session not
found' when it didn't hold the runtime session.

Route by the session the RPC TARGETS (params.session_id) instead. session_id
is a RUNTIME id while tiles/rows key on the STORED id, so translate via the
state cache then a reverse scan of the stored->runtime map (the same ladder
use-session-tile-delegate's storedSessionIdForRuntime uses); an unresolved id
is already a stored id (several RPCs pass stored ids directly). RPCs with no
session_id (ambient/config) keep the focused->selected fallback.

Pure helpers extracted to wiring-routing.ts so they're unit-testable without
importing the React controller; 6 tests cover the target-vs-focused routing,
the stored-id passthrough, and the no-session fallback. tsc 0 errors.

Diagnosis verified on a live install: the fix was present in source but the
running build still misrouted, and logs showed the bot's turn executing on the
default backend while its own per-profile backend sat idle.
@github-actions

github-actions Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

૮ >ﻌ< ა ci review

ran on fbd4f0c — fix(desktop): route each session RPC by its target session,

⚠️ Warnings

OSV vulnerability scan · View job

7 known vulnerabilities found in pinned dependencies.

How to fix:

Review the findings in the Security tab. Update the affected dependencies if a patched version is available.


debug info

CI timings

CI timings · View report · View job

Wall time 2m34s vs 3m27s (-25.6%). 1 job(s) slower, 2 faster, 3 unchanged.

  • JS & TS checks / JS & TS checks: -33.0s
  • OSV scan / Emit review status: -4.0s
  • OSV scan / Scan lockfiles / osv-scan: +3.0s

@teknium1

Copy link
Copy Markdown
Collaborator Author

Superseded by #93296 (merged), which lands the same fix — route session-scoped RPCs by params.session_id (runtime→stored translated) instead of $focusedStoredSessionId. Kshitij's version has the cleaner home for the runtime→stored helper (storedSessionIdForRuntimeId() in session-states.ts, where tiles already carry both identities, with stored-id claims winning over runtime bindings so a stale tile can't hijack a live one) plus broader regression coverage. Closing this as a duplicate. Thanks!

@teknium1 teknium1 closed this Aug 23, 2026
@teknium1
teknium1 deleted the fix/route-rpc-by-target-session branch August 23, 2026 23:17
@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 duplicate This issue or pull request already exists labels Aug 23, 2026
@alt-glitch

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Duplicate of merged #93296: current main already contains the target-session RPC routing repair.

@kshitijk4poor

Copy link
Copy Markdown
Contributor

Reviewed this against main — the diagnosis is exactly right (it's the same root cause I implemented in #93296 from your traced repro, which merged first and is why this branch now conflicts: both PRs rewrite the same requestGateway region).

Rather than close it as superseded, I salvaged it: #93311 carries this commit verbatim (your authorship preserved via cherry-pick) with the conflict resolved by unifying the two implementations — and the unified version keeps YOUR semantics on the two points where they're better than what main has:

  1. Unresolved-id passthroughstoredIdForRuntime(id) ?? id treats a translation miss as an already-stored id, which rememberedSessionProfile can still resolve. Main's merged version falls back to the focused tile on that edge, which is a residual misroute for RPCs that pass stored ids directly. Yours is strictly better; the salvage adopts it.
  2. Pure helpers in wiring-routing.ts — the DI seam makes the routing decision unit-testable without the React controller. Your 6 tests come along unchanged.

The one thing the salvage adds to your ladder: main's storedSessionIdForRuntimeId() (persisted tile map) as the final lookup rung — tiles survive window reloads when both in-memory caches are cold. Full ladder: state cache → binding-map reverse scan → persisted tile map → passthrough.

Validation on the salvage: tsc clean, your test file + main's routing tests green (81), full contrib+store sweep 1163 passed.

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/*) duplicate This issue or pull request already exists 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.

3 participants