From 5d3c15aaa776cfcb4c88fb6e4f0431a95387eeab Mon Sep 17 00:00:00 2001 From: emozilla Date: Wed, 19 Aug 2026 01:38:18 -0400 Subject: [PATCH] restore profile switching --- apps/desktop/src/store/gateway.ts | 107 +++----- .../store/profile-agent-activation.test.ts | 230 ++---------------- apps/desktop/src/store/profile.test.ts | 63 +---- apps/desktop/src/store/profile.ts | 164 ++++--------- 4 files changed, 112 insertions(+), 452 deletions(-) diff --git a/apps/desktop/src/store/gateway.ts b/apps/desktop/src/store/gateway.ts index 13708f657e..2966b93d6d 100644 --- a/apps/desktop/src/store/gateway.ts +++ b/apps/desktop/src/store/gateway.ts @@ -667,27 +667,13 @@ export async function openGatewayForAgent(connectionId: null | string, profile: } } -// The agent-scoped analogue of prepareGatewayForProfile, and the same -// publication seam: dial the agent's socket without publishing anything, and -// hand back the synchronous activation thunk. A null connection id falls -// through to the profile seam, so both doors into an activation share one -// atomicity contract instead of drifting apart; an explicit `local` id is a -// registry identity (`registryBackendScopeKey` keeps its own scope for it) and -// stays on the registry route. -// -// The thunk reports whether it actually published, preserving the `activated` -// contract callers rely on: a source edit/remove can dispose this entry while -// its dial is in flight, and a caller must be able to tell "switched" from -// "the target stopped existing" rather than assume the former. -export async function prepareGatewayForAgent(connectionId: null | string, profile: string): Promise<() => boolean> { +export async function ensureGatewayForAgent(connectionId: null | string, profile: string): Promise { const scope = registryBackendScopeKey(connectionId, profile) - // Genuinely-local scope: the profile door owns this route, so hand back ITS - // thunk unchanged. Wrapping it to return an unconditional `true` would have - // reported a rejected activation as a successful one and let the agent - // caller publish companion state for a switch that never happened. if (scope === normKey(profile)) { - return prepareGatewayForProfile(profile) + await ensureGatewayForProfile(profile) + + return true } if (!window.hermesDesktop?.getConnectionFor) { @@ -716,54 +702,42 @@ export async function prepareGatewayForAgent(connectionId: null | string, profil } } - // Bind the entry this dial settled on; see prepareGatewayForProfile. - const prepared = entry + // A source edit/remove may dispose this entry while its dial is still in + // flight. Only the still-registered, still-owned activation may publish. + const activated = + entry.wantOpen && + g.secondaries.get(scope) === entry && + Boolean(entry.connection) && + applyActive(scope, activationEpoch) - return () => { - // A source edit/remove may dispose this entry while its dial is still in - // flight. Only the still-registered, still-owned activation may publish. - const activated = - prepared.wantOpen && - g.secondaries.get(scope) === prepared && - Boolean(prepared.connection) && - applyActive(scope, activationEpoch) - - if (activated && prepared.connection) { - publishActiveConnection(prepared.connection) - } - - return activated + if (activated && entry.connection) { + publishActiveConnection(entry.connection) } + + return activated } -export async function ensureGatewayForAgent(connectionId: null | string, profile: string): Promise { - return (await prepareGatewayForAgent(connectionId, profile))() -} - -// 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<() => boolean> { +// 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 { const key = normKey(profile) const activationEpoch = beginGatewayActivation() if (key === g.primaryProfile) { - return () => applyActive(key, activationEpoch) + applyActive(key, activationEpoch) + + return } // 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. 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. + // descriptor — $activeGatewayProfile still moves to `key`, so request + // scoping and profile-aware surfaces behave identically. if (await sharedPrimaryRoute(key)) { - return () => applyActive(g.primaryProfile, activationEpoch) + applyActive(g.primaryProfile, activationEpoch) + + return } let entry = g.secondaries.get(key) @@ -786,36 +760,11 @@ export async function prepareGatewayForProfile(profile: string): Promise<() => b } } - // 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 - - // Reports whether the ACTIVATION was accepted, which is a different question - // from whether a descriptor was published: an accepted activation with no - // cached connection still moved the gateway, so the caller must still move - // its companion state. Only a rejected activation (disposed entry, or an - // epoch superseded by a newer switch while this one was dialing) must leave - // every companion store alone. - return () => { - const activated = prepared.wantOpen && g.secondaries.get(key) === prepared && applyActive(key, activationEpoch) - - if (activated && prepared.connection) { - publishActiveConnection(prepared.connection) - } - - return activated + if (entry.wantOpen && g.secondaries.get(key) === entry && applyActive(key, activationEpoch) && entry.connection) { + publishActiveConnection(entry.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-agent-activation.test.ts b/apps/desktop/src/store/profile-agent-activation.test.ts index 124da3010e..9e41056292 100644 --- a/apps/desktop/src/store/profile-agent-activation.test.ts +++ b/apps/desktop/src/store/profile-agent-activation.test.ts @@ -13,56 +13,18 @@ import type { HermesConnection } from '@/global' // 2. Agent activations share the gatewaySwitch mutex with profile switches — // without it, two rapid activations could complete out of order and the // EARLIER setActive() landed last. -// 3. A SUCCEEDING activation publishes the gateway, the profile pointer and -// the connection descriptor with no asynchronous gap between them. -// Activating first and awaiting the descriptor after left $gateway on the -// new backend while $connection still described the old one. -// 4. A FAILING descriptor lookup publishes none of the three. Swallowing the -// rejection and publishing anyway produced the same mixed state as (3), -// except permanent: (3) closes when the descriptor arrives, whereas a -// failed lookup never arrives and the split survived until an unrelated -// reconnect or switch repaired it. -// -// Both doors go through the prepare/publish seam (prepareGatewayFor*, which -// dial without publishing and return the activation thunk), so these mocks -// hand back a spy thunk instead of activating on call. -// Distinct gateway identities so a listener can tell WHICH backend it was -// handed. A bare vi.fn() thunk never touches $gateway, which would let an -// out-of-order publication pass unnoticed. -const INITIAL_GATEWAY = { id: 'live-socket' } -const AGENT_GATEWAY = { id: 'agent-socket' } -const PROFILE_GATEWAY = { id: 'profile-socket' } - -const activateAgent = vi.fn(() => { - $gateway.set(AGENT_GATEWAY) - - return true -}) - -const activateProfile = vi.fn(() => { - $gateway.set(PROFILE_GATEWAY) - - return true -}) - -// Annotated with the SEAM's thunk types, not the spies' own. Inferred, the -// resolved type is the MockInstance itself, and a test can no longer hand back -// a plain `() => false` to stand in for a disposed entry. -const prepareGatewayForAgent = vi.fn( - async (_connectionId: null | string, _profile: string): Promise<() => boolean> => activateAgent -) - -const prepareGatewayForProfile = vi.fn(async (_profile: string): Promise<() => boolean> => activateProfile) +const ensureGatewayForAgent = vi.fn(async (_connectionId: null | string, _profile: string) => true) +const ensureGatewayForProfile = vi.fn(async (_profile: string) => undefined) const openGatewayForProfile = vi.fn(async (_profile: string) => undefined) -const $gateway = atom(INITIAL_GATEWAY) +const $gateway = atom({ id: 'live-socket' }) const resetStarmapGraph = vi.fn() vi.mock('@/store/gateway', () => ({ $gateway, - openGatewayForProfile, - prepareGatewayForAgent, - prepareGatewayForProfile + ensureGatewayForAgent, + ensureGatewayForProfile, + openGatewayForProfile })) vi.mock('@/hermes', () => ({ getProfiles: vi.fn(async () => ({ profiles: [] })), @@ -98,13 +60,9 @@ function deferred(): { promise: Promise; resolve: () => void } { beforeEach(() => { getConnection.mockReset() getConnectionFor.mockReset() - prepareGatewayForAgent.mockReset() - prepareGatewayForAgent.mockResolvedValue(activateAgent) - prepareGatewayForProfile.mockReset() - prepareGatewayForProfile.mockResolvedValue(activateProfile) - activateAgent.mockClear() - activateProfile.mockClear() - $gateway.set(INITIAL_GATEWAY) + ensureGatewayForAgent.mockClear() + ensureGatewayForProfile.mockClear() + $gateway.set({ id: 'live-socket' }) $activeGatewayProfile.set('default') $connection.set(localConn()) vi.stubGlobal('window', { hermesDesktop: { getConnection, getConnectionFor } }) @@ -123,83 +81,31 @@ describe('ensureGatewayAgent → $connection / $activeGatewayProfile sync', () = await ensureGatewayAgent('homelab', 'research') - expect(prepareGatewayForAgent).toHaveBeenCalledWith('homelab', 'research') - expect(activateAgent).toHaveBeenCalledTimes(1) + expect(ensureGatewayForAgent).toHaveBeenCalledWith('homelab', 'research') expect(getConnectionFor).toHaveBeenCalledWith({ connectionId: 'homelab', profile: 'research' }) expect($activeGatewayProfile.get()).toBe('research') expect($connection.get()?.mode).toBe('remote') expect($connection.get()?.profile).toBe('research') }) - it('fails the switch closed when the descriptor lookup rejects', async () => { - // Previously this path swallowed the rejection and published anyway, which - // left $gateway and $activeGatewayProfile on the NEW backend while - // $connection still described the old one. Unlike the pending-descriptor - // race below, that state did not close on its own: it survived until some - // later reconnect or switch happened to repair it. + it('leaves the prior connection intact when the descriptor fetch fails', async () => { getConnectionFor.mockRejectedValue(new Error('source unreachable')) - await expect(ensureGatewayAgent('homelab', 'research')).rejects.toThrow('source unreachable') + await ensureGatewayAgent('homelab', 'research') - // Nothing published: all three still describe the previous backend, and the - // caller can retry the switch. - expect(activateAgent).not.toHaveBeenCalled() - expect($gateway.get()).toBe(INITIAL_GATEWAY) - expect($activeGatewayProfile.get()).toBe('default') + expect($activeGatewayProfile.get()).toBe('research') + // Best-effort: boot/reconnect resyncs later; we must not null it out here. expect($connection.get()?.mode).toBe('local') - expect($connection.get()?.profile).toBe('default') }) it('does not republish a registry identity invalidated during activation', async () => { - // The thunk reports false: the entry was disposed (source edited/removed) - // between dial and publish. Nothing may publish, $gateway included. - prepareGatewayForAgent.mockResolvedValueOnce(() => false) + ensureGatewayForAgent.mockResolvedValueOnce(false) await ensureGatewayAgent('removed-source', 'research') expect($activeGatewayProfile.get()).toBe('default') expect($connection.get()?.mode).toBe('local') - expect($gateway.get()).toBe(INITIAL_GATEWAY) - // The descriptor lookup DOES run: it is issued concurrently with the dial - // so both can be resolved before anything is published, which is the whole - // point of the seam. Resolving it lazily (only after the thunk reports a - // live entry) would put an await between the identity check and the - // publication and reopen the gap. The cost is one redundant read-only - // lookup in the rare disposed-entry case; the invariant that matters - - // nothing is PUBLISHED - is asserted above. - expect(getConnectionFor).toHaveBeenCalledTimes(1) - }) - - it('never shows a $gateway listener the new backend beside stale companions', async () => { - // The assertion the earlier tests could not make. A spy thunk that never - // touches $gateway proves only that it was CALLED at the right moment; - // it cannot prove that the three public stores become visible together. - // Nanostores drains listeners synchronously on every .set(), so without - // batch() a $gateway listener runs between the writes and reads the new - // gateway next to the previous profile and descriptor. - getConnectionFor.mockResolvedValue(agentConn()) - const seen: { connection?: string; gateway: unknown; profile: string }[] = [] - - const stop = $gateway.listen(gateway => { - seen.push({ - connection: $connection.get()?.profile, - gateway, - profile: $activeGatewayProfile.get() - }) - }) - - try { - await ensureGatewayAgent('homelab', 'research') - } finally { - stop() - } - - expect(seen).toHaveLength(1) - // When the listener sees the agent's gateway, the profile pointer and the - // descriptor must ALREADY identify that same backend. - expect(seen[0].gateway).toBe(AGENT_GATEWAY) - expect(seen[0].profile).toBe('research') - expect(seen[0].connection).toBe('research') + expect(getConnectionFor).not.toHaveBeenCalled() }) it('falls through to the profile path for a null connectionId', async () => { @@ -207,8 +113,8 @@ describe('ensureGatewayAgent → $connection / $activeGatewayProfile sync', () = await ensureGatewayAgent(null, 'research') - expect(prepareGatewayForProfile).toHaveBeenCalledWith('research') - expect(prepareGatewayForAgent).not.toHaveBeenCalled() + expect(ensureGatewayForProfile).toHaveBeenCalledWith('research') + expect(ensureGatewayForAgent).not.toHaveBeenCalled() expect(getConnectionFor).not.toHaveBeenCalled() }) @@ -217,90 +123,10 @@ describe('ensureGatewayAgent → $connection / $activeGatewayProfile sync', () = await ensureGatewayAgent('local', 'research') - expect(prepareGatewayForAgent).toHaveBeenCalledWith('local', 'research') - expect(prepareGatewayForProfile).not.toHaveBeenCalled() + expect(ensureGatewayForAgent).toHaveBeenCalledWith('local', 'research') + expect(ensureGatewayForProfile).not.toHaveBeenCalled() expect(getConnectionFor).toHaveBeenCalledWith({ connectionId: 'local', profile: 'research' }) }) - - it('never publishes the agent gateway before its connection descriptor', async () => { - // The same mixed-state window the profile path closes, through the door - // added for the SDK's ensureAgent. A slow getConnectionFor must not leave - // $gateway/$activeGatewayProfile on the agent's backend while $connection - // still describes the previous one — anything requesting in that window - // announces the WRONG mode to the new backend. - let resolveDescriptor: (conn: HermesConnection) => void = () => undefined - getConnectionFor.mockReturnValue( - new Promise(resolve => { - resolveDescriptor = resolve - }) - ) - - const switching = ensureGatewayAgent('homelab', 'research') - // Let the socket-dial half settle; the descriptor is still pending. - await Promise.resolve() - await Promise.resolve() - - expect(activateAgent).not.toHaveBeenCalled() - expect($gateway.get()).toBe(INITIAL_GATEWAY) - expect($activeGatewayProfile.get()).toBe('default') - expect($connection.get()?.mode).toBe('local') - - resolveDescriptor(agentConn()) - await switching - - expect(activateAgent).toHaveBeenCalledTimes(1) - expect($activeGatewayProfile.get()).toBe('research') - expect($connection.get()?.mode).toBe('remote') - }) -}) - -describe('ensureGatewayProfile publishes under the same activation guard', () => { - it('publishes nothing when the profile activation is superseded', async () => { - // The profile-door mirror of "does not republish a registry identity - // invalidated during activation". applyActive() returns false when its - // captured epoch has been superseded — a newer switch or a teardown - // landed while this preparation was awaiting its route or socket. - // - // Discarding that boolean does not produce a torn publication; batch() - // makes the writes observer-atomic either way. It produces something - // subtler and worse: ONE complete, internally inconsistent tuple, the - // CURRENT gateway paired with the stale target's profile pointer and - // descriptor. Atomicity cannot make a rejected activation correct, so the - // caller has to decline to publish at all. - getConnection.mockResolvedValue(localConn({ profile: 'worker' })) - prepareGatewayForProfile.mockResolvedValueOnce(() => false) - - const seen: unknown[] = [] - const stop = $gateway.listen(gateway => seen.push(gateway)) - - try { - await ensureGatewayProfile('worker') - } finally { - stop() - } - - // All three still describe the complete route that was already active. - expect($gateway.get()).toBe(INITIAL_GATEWAY) - expect($activeGatewayProfile.get()).toBe('default') - expect($connection.get()?.profile).toBe('default') - expect($connection.get()?.mode).toBe('local') - // And no subscriber was handed a tuple to disagree about. - expect(seen).toEqual([]) - }) - - it('publishes the companions when the profile activation is accepted', async () => { - // The other half: the guard must not swallow a legitimate switch. Without - // this, returning a constant false from every thunk would pass the test - // above and break the feature. - getConnection.mockResolvedValue(localConn({ profile: 'worker' })) - - await ensureGatewayProfile('worker') - - expect(activateProfile).toHaveBeenCalledTimes(1) - expect($gateway.get()).toBe(PROFILE_GATEWAY) - expect($activeGatewayProfile.get()).toBe('worker') - expect($connection.get()?.profile).toBe('worker') - }) }) describe('ensureGatewayAgent shares the gatewaySwitch mutex with profile switches', () => { @@ -308,16 +134,14 @@ describe('ensureGatewayAgent shares the gatewaySwitch mutex with profile switche const profileGate = deferred() const order: string[] = [] - prepareGatewayForProfile.mockImplementation(async (profile: string) => { + ensureGatewayForProfile.mockImplementation(async (profile: string) => { order.push(`profile:${profile}`) await profileGate.promise - - return activateProfile }) - prepareGatewayForAgent.mockImplementation(async (_connectionId, profile) => { + ensureGatewayForAgent.mockImplementation(async (_connectionId, profile) => { order.push(`agent:${profile}`) - return activateAgent + return true }) getConnection.mockResolvedValue(localConn({ profile: 'worker' })) getConnectionFor.mockResolvedValue(agentConn()) @@ -346,16 +170,14 @@ describe('ensureGatewayAgent shares the gatewaySwitch mutex with profile switche const agentGate = deferred() const order: string[] = [] - prepareGatewayForAgent.mockImplementation(async (_connectionId, profile) => { + ensureGatewayForAgent.mockImplementation(async (_connectionId, profile) => { order.push(`agent:${profile}`) await agentGate.promise - return activateAgent + return true }) - prepareGatewayForProfile.mockImplementation(async (profile: string) => { + ensureGatewayForProfile.mockImplementation(async (profile: string) => { order.push(`profile:${profile}`) - - return activateProfile }) getConnection.mockResolvedValue(localConn({ profile: 'worker' })) getConnectionFor.mockResolvedValue(agentConn()) diff --git a/apps/desktop/src/store/profile.test.ts b/apps/desktop/src/store/profile.test.ts index 7ac9dbb6bc..c2e7cf36c0 100644 --- a/apps/desktop/src/store/profile.test.ts +++ b/apps/desktop/src/store/profile.test.ts @@ -6,25 +6,13 @@ 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. -// Returns true: both prepare seams hand back a thunk reporting whether the -// activation was ACCEPTED, and a caller publishes its companion state only on -// true. A bare vi.fn() returns undefined, which now reads as "superseded" and -// would silently suppress every publication these tests assert on. -const activateGateway = vi.fn(() => true) const ensureGatewayForProfile = vi.fn(async () => undefined) -const prepareGatewayForAgent = vi.fn(async (_connectionId: null | string, _profile: string) => activateGateway) -const prepareGatewayForProfile = vi.fn(async (_profile: string) => activateGateway) +const ensureGatewayForAgent = vi.fn(async () => undefined) const openGatewayForProfile = vi.fn(async (_profile: string) => undefined) const $gateway = atom({ id: 'live-socket' }) const resetStarmapGraph = vi.fn() -vi.mock('@/store/gateway', () => ({ - $gateway, - ensureGatewayForProfile, - openGatewayForProfile, - prepareGatewayForAgent, - prepareGatewayForProfile -})) +vi.mock('@/store/gateway', () => ({ $gateway, ensureGatewayForAgent, ensureGatewayForProfile, openGatewayForProfile })) vi.mock('@/hermes', () => ({ getProfiles: vi.fn(async () => ({ profiles: [] })), setApiRequestProfile: vi.fn() @@ -65,9 +53,7 @@ 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') @@ -93,8 +79,7 @@ describe('ensureGatewayProfile → $connection sync (#46651)', () => { await ensureGatewayProfile('vps-remote') - expect(prepareGatewayForProfile).toHaveBeenCalledWith('vps-remote') - expect(activateGateway).toHaveBeenCalledTimes(1) + expect(ensureGatewayForProfile).toHaveBeenCalledWith('vps-remote') expect(getConnection).toHaveBeenCalledWith('vps-remote') expect($connection.get()?.mode).toBe('remote') expect($connection.get()?.profile).toBe('vps-remote') @@ -111,51 +96,15 @@ describe('ensureGatewayProfile → $connection sync (#46651)', () => { expect($connection.get()?.mode).toBe('local') }) - 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. + it('leaves the prior connection intact when the descriptor fetch fails', async () => { getConnection.mockRejectedValue(new Error('backend unreachable')) await ensureGatewayProfile('vps-remote') - expect(activateGateway).not.toHaveBeenCalled() - expect($activeGatewayProfile.get()).toBe('default') + // Best-effort: boot/reconnect resyncs later; we must not null it out here. 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()) @@ -163,7 +112,7 @@ describe('ensureGatewayProfile → $connection sync (#46651)', () => { await ensureGatewayProfile('vps-remote') expect(getConnection).not.toHaveBeenCalled() - expect(prepareGatewayForProfile).not.toHaveBeenCalled() + expect(ensureGatewayForProfile).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 5e73d84c87..7e5a32e15d 100644 --- a/apps/desktop/src/store/profile.ts +++ b/apps/desktop/src/store/profile.ts @@ -1,6 +1,5 @@ -import { atom, batch, computed } from 'nanostores' +import { atom, computed } from 'nanostores' -import type { HermesConnection } from '@/global' import { getProfiles, hermesApi, setApiRequestProfile, STARTUP_REQUEST_TIMEOUT_MS } from '@/hermes' import { invalidateProfileScopedQueries } from '@/lib/query-client' import { @@ -13,7 +12,7 @@ import { storedStringRecord } from '@/lib/storage' import { invalidateCronModelImpactScopeState } from '@/store/cron-model-impact-scope' -import { $gateway, openGatewayForProfile, prepareGatewayForAgent, prepareGatewayForProfile } from '@/store/gateway' +import { $gateway, ensureGatewayForAgent, ensureGatewayForProfile, openGatewayForProfile } from '@/store/gateway' import { setConnection } from '@/store/session' import { resetStarmapGraph } from '@/store/starmap' import type { ProfileInfo } from '@/types/hermes' @@ -270,24 +269,30 @@ export function prewarmProfileBackend(name: string): void { let gatewaySwitch: Promise | null = null -// 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 +// Keep the renderer's $connection (mode / baseUrl / profile) in lockstep with +// the profile the live gateway is now on. $connection seeds from the PRIMARY // (window) backend at boot and otherwise only refreshes on a sleep/wake -// 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) { +// 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 { const getConnection = window.hermesDesktop?.getConnection - return getConnection ? getConnection(profile) : null + if (!getConnection) { + return + } + + try { + setConnection(await getConnection(profile)) + } catch { + // Leave the prior connection in place; boot/reconnect resyncs it later. + } } // Make `profile`'s backend the active gateway, lazily opening its socket if it @@ -326,46 +331,14 @@ export async function ensureGatewayProfile(profile: string | null | undefined): $gatewaySwapTarget.set(target) gatewaySwitch = (async () => { - // 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 publication. batch() defers Nanostores' notifications to the end of - // the callback, so the active gateway, $activeGatewayProfile and - // $connection become visible together. Without it these are sequential - // .set() calls that each drain their listeners synchronously, and a - // $gateway listener runs while the other two still name the old backend. - batch(() => { - // A rejected activation publishes NOTHING, exactly like the agent path. - // applyActive() returns false when its captured epoch was superseded -- - // a newer switch (or a teardown) landed while this one was awaiting its - // route or socket. Publishing the companions anyway would leave the - // CURRENT gateway paired with the stale profile pointer and descriptor, - // and batch() cannot rescue that: it would make the mismatched tuple - // atomically observable rather than prevent it. - if (!activate()) { - return - } - - $activeGatewayProfile.set(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. - }) + // 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) + $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) + })() try { await gatewaySwitch @@ -377,30 +350,25 @@ export async function ensureGatewayProfile(profile: string | null | undefined): // Registry-aware sibling of syncConnectionToActiveProfile: a connection-scoped // agent's descriptor comes from getConnectionFor (its SOURCE connection), not -// getConnection (the local pool). -// Resolve only — publication is the caller's, so the descriptor can be in hand -// BEFORE the activation frame rather than an await after it. -// -// Null means "no desktop bridge" (plain browser) and nothing else, matching -// resolveConnectionForProfile. A bridge REJECTION propagates so the caller -// aborts the whole switch. Collapsing the two into null instead let a failed -// lookup publish the new gateway and profile while $connection kept describing -// the OLD backend, and unlike the pending-descriptor race that state did not -// close on its own: it survived until some later reconnect or switch happened -// to repair it, which is the same invariant this path exists to establish. -async function resolveConnectionForActiveAgent( - connectionId: string, - profile: string -): Promise { +// getConnection (the local pool). Same best-effort contract. +async function syncConnectionToActiveAgent(connectionId: string, profile: string): Promise { const getConnectionFor = window.hermesDesktop?.getConnectionFor - return getConnectionFor ? getConnectionFor({ connectionId, profile }) : null + if (!getConnectionFor) { + return + } + + try { + setConnection(await getConnectionFor({ connectionId, profile })) + } catch { + // Leave the prior connection in place; boot/reconnect resyncs it later. + } } // Activate a connection-scoped agent's gateway — the (connectionId, profile) // analogue of ensureGatewayProfile, and the door the SDK's ensureAgent goes -// through. Three invariants the raw store call (ensureGatewayForAgent) does -// not provide on its own: +// through. Two invariants the raw store call (ensureGatewayForAgent) does not +// provide on its own: // - Every activation moves $activeGatewayProfile and resyncs $connection, // exactly like the profile path — otherwise activating an ALREADY-OPEN // registry agent left both describing the previous backend, routing @@ -409,10 +377,6 @@ async function resolveConnectionForActiveAgent( // - Activations share the gatewaySwitch mutex with profile switches, so a // rapid agent↔profile (or agent↔agent) interleave can't finish out of // order and leave the EARLIER setActive() as the last write. -// - The gateway, the profile pointer and the connection descriptor publish in -// ONE synchronous frame, via the same prepare/publish seam the profile path -// uses, so no subscriber sees the new backend paired with the old -// descriptor. // Only a null connectionId falls through to the legacy profile path. Explicit // `local` is a registry identity and must use the genuinely-local route. export async function ensureGatewayAgent(connectionId: null | string, profile: string): Promise { @@ -430,40 +394,16 @@ export async function ensureGatewayAgent(connectionId: null | string, profile: s $gatewaySwapTarget.set(target) gatewaySwitch = (async () => { - // Dial the agent's socket and resolve its descriptor without publishing - // either, exactly like the profile path above. Activating first and then - // awaiting the descriptor left $gateway on the new backend while - // $connection still described the old one, so anything requesting during - // that window announced the WRONG mode to the new backend. - const [descriptor, activate] = await Promise.all([ - resolveConnectionForActiveAgent(connection, target), - prepareGatewayForAgent(connection, target) - ]) + const activated = await ensureGatewayForAgent(connection, target) - // ONE publication. batch() defers Nanostores' notifications to the end of - // the callback, so a $gateway listener cannot run while the profile - // pointer and the connection descriptor still name the previous backend. - // Without it these are three sequential .set() calls, each draining its - // listeners synchronously, and the first listener observes exactly the - // mismatch this seam exists to prevent. - batch(() => { - // A disposed target (source edited/removed mid-dial) publishes nothing - // at all, rather than moving the profile pointer to a backend that no - // longer has a socket. - if (!activate()) { - return - } + if (!activated) { + return + } - $activeGatewayProfile.set(target) - - // Remote-aware paths (image.attach_bytes vs image.attach, /api/fs/*, - // /api/media) follow $connection. Null here is only the no-bridge case, - // so keeping the previous descriptor is correct; a failed lookup - // rejected above and never reached this frame. - if (descriptor) { - setConnection(descriptor) - } - }) + $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 syncConnectionToActiveAgent(connection, target) })() try {