From d57f94a330ea2c206abe42983fb6bfcfbaeafd15 Mon Sep 17 00:00:00 2001 From: Jack Lau <72348727+jackulau@users.noreply.github.com> Date: Thu, 13 Aug 2026 01:16:54 -0500 Subject: [PATCH] fix(desktop): publish gateway, profile, and connection descriptor atomically on a profile switch ensureGatewayProfile used to activate the target gateway and set $activeGatewayProfile while the connection descriptor fetch was still in flight, so during that window $gateway already targeted the new backend while $connection still described the previous one, and any request or plugin mode-listener firing then announced the wrong mode to the new backend. A failed descriptor fetch made the mismatch permanent. prepareGatewayForProfile (new gateway-store seam) opens the socket and returns a synchronous activation thunk without publishing anything; ensureGatewayForProfile now delegates to it. The switch resolves the descriptor and opens the socket first, then flips the active gateway, the profile atom, and $connection in one synchronous frame. A descriptor failure aborts the switch as a unit: nothing is published and every atom still consistently describes the previous profile. The deferred-descriptor test holds the fetch open and asserts the public atoms never disagree, then releases it and asserts all three flipped together; the failure test asserts no partial publication. --- apps/desktop/src/store/gateway.ts | 53 +++++++++++++----- apps/desktop/src/store/profile.test.ts | 57 ++++++++++++++++++-- apps/desktop/src/store/profile.ts | 74 ++++++++++++++++---------- 3 files changed, 137 insertions(+), 47 deletions(-) diff --git a/apps/desktop/src/store/gateway.ts b/apps/desktop/src/store/gateway.ts index 2966b93d6d..c01269867b 100644 --- a/apps/desktop/src/store/gateway.ts +++ b/apps/desktop/src/store/gateway.ts @@ -717,27 +717,34 @@ export async function ensureGatewayForAgent(connectionId: null | string, profile return activated } -// Make `profile` the active gateway, lazily opening its socket if needed. The -// primary is a no-op fast path. Background sockets are never closed here. -export async function ensureGatewayForProfile(profile: string): Promise { +// Open `profile`'s socket if needed and hand back a synchronous activation +// thunk — the publication seam for atomic profile switches. The caller invokes +// the thunk in the same synchronous frame as its own atom writes (profile +// pointer, connection descriptor), so no subscriber can observe the active +// gateway pointing at one backend while companion state still describes +// another. Nothing is published until the thunk runs. +export async function prepareGatewayForProfile(profile: string): Promise<() => void> { const key = normKey(profile) const activationEpoch = beginGatewayActivation() if (key === g.primaryProfile) { - applyActive(key, activationEpoch) - - return + return () => { + applyActive(key, activationEpoch) + } } // Global-remote share (routing case 3): one remote host serves every // profile through the PRIMARY socket, scoped per request. Activate the // primary instead of dialing a doomed duplicate socket at the same - // descriptor — $activeGatewayProfile still moves to `key`, so request - // scoping and profile-aware surfaces behave identically. + // descriptor - $activeGatewayProfile still moves to `key`, so request + // scoping and profile-aware surfaces behave identically. Checked BEFORE + // createSecondary so a shared-remote profile never mints a secondary + // entry, and returned as a thunk like every other path here so this + // switch publishes as atomically as a dedicated-socket one. if (await sharedPrimaryRoute(key)) { - applyActive(g.primaryProfile, activationEpoch) - - return + return () => { + applyActive(g.primaryProfile, activationEpoch) + } } let entry = g.secondaries.get(key) @@ -760,11 +767,31 @@ export async function ensureGatewayForProfile(profile: string): Promise { } } - if (entry.wantOpen && g.secondaries.get(key) === entry && applyActive(key, activationEpoch) && entry.connection) { - publishActiveConnection(entry.connection) + // Bind the entry the await settled on. `g.secondaries.get(key)` can be a + // DIFFERENT object by the time the thunk runs (a teardown + redial between + // prepare and publish), and publishing that one's descriptor would be the + // very mismatch this seam exists to prevent, so the identity re-check below + // compares against this exact entry. + const prepared = entry + + return () => { + if ( + prepared.wantOpen && + g.secondaries.get(key) === prepared && + applyActive(key, activationEpoch) && + prepared.connection + ) { + publishActiveConnection(prepared.connection) + } } } +// Make `profile` the active gateway, lazily opening its socket if needed. The +// primary is a no-op fast path. Background sockets are never closed here. +export async function ensureGatewayForProfile(profile: string): Promise { + ;(await prepareGatewayForProfile(profile))() +} + // Reconnect the active gateway after a transient request failure. Primary // reconnects are owned by use-gateway-boot, so we only drive secondaries here. export async function ensureActiveGatewayOpen(): Promise { diff --git a/apps/desktop/src/store/profile.test.ts b/apps/desktop/src/store/profile.test.ts index c2e7cf36c0..95b9f6957f 100644 --- a/apps/desktop/src/store/profile.test.ts +++ b/apps/desktop/src/store/profile.test.ts @@ -6,13 +6,21 @@ import type { ProfileInfo } from '@/types/hermes' // Keep profile.ts's side-effecting imports inert: the gateway socket layer and // the REST query client must not run for real in a unit test. +const activateGateway = vi.fn() const ensureGatewayForProfile = vi.fn(async () => undefined) const ensureGatewayForAgent = vi.fn(async () => undefined) +const prepareGatewayForProfile = vi.fn(async (_profile: string) => activateGateway) const openGatewayForProfile = vi.fn(async (_profile: string) => undefined) const $gateway = atom({ id: 'live-socket' }) const resetStarmapGraph = vi.fn() -vi.mock('@/store/gateway', () => ({ $gateway, ensureGatewayForAgent, ensureGatewayForProfile, openGatewayForProfile })) +vi.mock('@/store/gateway', () => ({ + $gateway, + ensureGatewayForAgent, + ensureGatewayForProfile, + openGatewayForProfile, + prepareGatewayForProfile +})) vi.mock('@/hermes', () => ({ getProfiles: vi.fn(async () => ({ profiles: [] })), setApiRequestProfile: vi.fn() @@ -53,7 +61,9 @@ const getConnection = vi.fn<(profile?: string | null) => Promise { getConnection.mockReset() + activateGateway.mockClear() ensureGatewayForProfile.mockClear() + prepareGatewayForProfile.mockClear() openGatewayForProfile.mockClear() $gateway.set({ id: 'live-socket' }) $activeGatewayProfile.set('default') @@ -79,7 +89,8 @@ describe('ensureGatewayProfile → $connection sync (#46651)', () => { await ensureGatewayProfile('vps-remote') - expect(ensureGatewayForProfile).toHaveBeenCalledWith('vps-remote') + expect(prepareGatewayForProfile).toHaveBeenCalledWith('vps-remote') + expect(activateGateway).toHaveBeenCalledTimes(1) expect(getConnection).toHaveBeenCalledWith('vps-remote') expect($connection.get()?.mode).toBe('remote') expect($connection.get()?.profile).toBe('vps-remote') @@ -96,15 +107,51 @@ describe('ensureGatewayProfile → $connection sync (#46651)', () => { expect($connection.get()?.mode).toBe('local') }) - it('leaves the prior connection intact when the descriptor fetch fails', async () => { + it('fails as a unit when the descriptor fetch fails — no mixed state', async () => { + // Previously the gateway was activated and $activeGatewayProfile set even + // when the descriptor lookup failed, leaving $gateway on the new backend + // while $connection kept describing the old one for the rest of the + // session. Now nothing is published: every atom still consistently + // describes the previous profile and the user can retry. getConnection.mockRejectedValue(new Error('backend unreachable')) await ensureGatewayProfile('vps-remote') - // Best-effort: boot/reconnect resyncs later; we must not null it out here. + expect(activateGateway).not.toHaveBeenCalled() + expect($activeGatewayProfile.get()).toBe('default') expect($connection.get()?.mode).toBe('local') }) + it('never publishes the new gateway before its connection descriptor', async () => { + // The exact mixed-state window from the follow-up review: a slow + // descriptor fetch must not leave $gateway/$activeGatewayProfile on the + // remote backend while $connection still says local. All three flip + // together only once the descriptor is in hand. + let resolveDescriptor: (conn: HermesConnection) => void = () => undefined + getConnection.mockReturnValue( + new Promise(resolve => { + resolveDescriptor = resolve + }) + ) + + const switching = ensureGatewayProfile('vps-remote') + // Let the socket-open half of the switch settle; the descriptor is still + // deliberately pending. + await Promise.resolve() + await Promise.resolve() + + expect(activateGateway).not.toHaveBeenCalled() + expect($activeGatewayProfile.get()).toBe('default') + expect($connection.get()?.mode).toBe('local') + + resolveDescriptor(remoteConn()) + await switching + + expect(activateGateway).toHaveBeenCalledTimes(1) + expect($activeGatewayProfile.get()).toBe('vps-remote') + expect($connection.get()?.mode).toBe('remote') + }) + it('does not churn $connection when the target is already the active profile', async () => { $activeGatewayProfile.set('vps-remote') $connection.set(remoteConn()) @@ -112,7 +159,7 @@ describe('ensureGatewayProfile → $connection sync (#46651)', () => { await ensureGatewayProfile('vps-remote') expect(getConnection).not.toHaveBeenCalled() - expect(ensureGatewayForProfile).not.toHaveBeenCalled() + expect(prepareGatewayForProfile).not.toHaveBeenCalled() expect($connection.get()?.mode).toBe('remote') }) }) diff --git a/apps/desktop/src/store/profile.ts b/apps/desktop/src/store/profile.ts index 68f0574bfb..f1b7c0ad39 100644 --- a/apps/desktop/src/store/profile.ts +++ b/apps/desktop/src/store/profile.ts @@ -12,7 +12,12 @@ import { storedStringRecord } from '@/lib/storage' import { invalidateCronModelImpactScopeState } from '@/store/cron-model-impact-scope' -import { $gateway, ensureGatewayForAgent, ensureGatewayForProfile, openGatewayForProfile } from '@/store/gateway' +import { + $gateway, + ensureGatewayForAgent, + openGatewayForProfile, + prepareGatewayForProfile +} from '@/store/gateway' import { setConnection } from '@/store/session' import { resetStarmapGraph } from '@/store/starmap' import type { ProfileInfo } from '@/types/hermes' @@ -269,30 +274,24 @@ export function prewarmProfileBackend(name: string): void { let gatewaySwitch: Promise | null = null -// Keep the renderer's $connection (mode / baseUrl / profile) in lockstep with -// the profile the live gateway is now on. $connection seeds from the PRIMARY +// The target profile's connection descriptor (mode / baseUrl / …), fetched +// BEFORE activation so the switch can publish it in the same synchronous frame +// as the gateway and profile pointer. $connection seeds from the PRIMARY // (window) backend at boot and otherwise only refreshes on a sleep/wake -// reconnect — so activating a *background* profile left $connection describing -// the primary, with the wrong `mode` for everything that branches on -// local-vs-remote. Headline symptom: with a local primary and a remote pool -// profile active, image attachments went out via the path-based `image.attach` -// instead of `image.attach_bytes`, handing the remote gateway a client-only -// path it can't resolve ("image not found: C:\…"), while the /api/fs/* file -// browser and /api/media fetches targeted the wrong machine (#46651). -// Best-effort: a failed descriptor fetch leaves the prior connection intact for -// boot/reconnect to resync. -async function syncConnectionToActiveProfile(profile: string): Promise { +// reconnect — so activating a *background* profile without this left +// $connection describing the primary, with the wrong `mode` for everything +// that branches on local-vs-remote (#46651: path-based `image.attach` against +// a remote gateway, /api/fs/* and /api/media on the wrong machine). +// +// Null means "no desktop bridge" (plain browser) — there is no descriptor to +// sync then. A bridge REJECTION propagates: the caller aborts the whole switch +// rather than activating a backend whose descriptor (and thus mode) is +// unknown, which previously left $gateway on the new backend while +// $connection kept describing the old one for the rest of the session. +async function resolveConnectionForProfile(profile: string) { const getConnection = window.hermesDesktop?.getConnection - if (!getConnection) { - return - } - - try { - setConnection(await getConnection(profile)) - } catch { - // Leave the prior connection in place; boot/reconnect resyncs it later. - } + return getConnection ? getConnection(profile) : null } // Make `profile`'s backend the active gateway, lazily opening its socket if it @@ -331,14 +330,31 @@ export async function ensureGatewayProfile(profile: string | null | undefined): $gatewaySwapTarget.set(target) gatewaySwitch = (async () => { - // ensureGatewayForProfile opens (or reuses) the target's socket and points - // the active gateway at it — without closing the profile you came from. - await ensureGatewayForProfile(target) + // Resolve the target's connection descriptor and open (or reuse) its + // socket BEFORE anything is published — without closing the profile you + // came from. The gateway used to be activated (and the profile atom set) + // while the descriptor fetch was still in flight, so during that window + // $gateway already targeted the new backend while $connection still + // described the previous one — and any request or plugin mode-listener + // firing then announced the WRONG mode to the new backend. + const [connection, activate] = await Promise.all([ + resolveConnectionForProfile(target), + prepareGatewayForProfile(target) + ]) + + // One synchronous publication frame — no awaits from here down, so the + // active gateway, $activeGatewayProfile, and $connection flip together. + activate() $activeGatewayProfile.set(target) - // The active backend just changed; resync $connection so remote-aware - // paths (image.attach_bytes vs image.attach, /api/fs/*, /api/media) follow. - await syncConnectionToActiveProfile(target) - })() + + if (connection) { + setConnection(connection) + } + })().catch(() => { + // Descriptor lookup failed: the switch fails as a unit. Nothing was + // published, so every atom still consistently describes the previous + // profile; the user can retry the switch. + }) try { await gatewaySwitch