From 92a0dd3774609ca5c3a4b93ea2d8abbb1064833d Mon Sep 17 00:00:00 2001 From: ygd58 Date: Sat, 22 Aug 2026 13:46:00 +0000 Subject: [PATCH] fix(desktop): require an open socket before publishing a secondary gateway route Fixes #92265 (proposed fix #2; #1 and #4 are separate follow-ups, see below). ensureGatewayForAgent() and ensureGatewayForProfile() both decided whether a secondary activation "succeeded" by checking Boolean(entry.connection) alone. entry.connection is set in openSecondary() BEFORE the WebSocket dial completes (`entry.connection = conn` happens ahead of `await entry.gateway.connect(wsUrl)`), so a transient first-dial failure -- caught by the surrounding try/catch and left for scheduleReconnect's backoff retry -- still left entry.connection truthy. Both functions then treated this as a successful activation: applyActive() switched g.activeKey and published $gateway to the closed socket, and publishActiveConnection() pushed the connection descriptor to the UI. The next chat RPC then failed with "Hermes gateway is not connected" against a route the user/desktop believed was live. Added an isOpen(entry.gateway) check alongside the existing Boolean(entry.connection) check in both functions' activation/publish conditions, gating BOTH applyActive() (which switches g.activeKey and publishes $gateway) and publishActiveConnection() (which pushes the connection descriptor) on the socket having actually reached 'open'. A failed first dial now correctly returns false / leaves the previous active route untouched, matching option 3 from the issue's own proposed fix ("if both bounded attempts fail, keep the existing active route") -- the existing scheduleReconnect backoff still owns recovery for that entry going forward. Not implemented in this PR (separate, lower-priority follow-ups): - Proposed fix #1 (one immediate bounded reconnect attempt before returning activation status) -- a larger behavioral change with its own retry/timing tradeoffs; left to a separate PR. - Proposed fix #4 (Bot Mode's own connection-ID-only guard in plugins/hermes-bots/plugin.js) -- host.ensureAgent() calls into the now-fixed gateway.ts functions, so this class of bug is already closed at the root; Bot Mode's own additional profile/state verification may still be worth adding but is a separate, narrower hardening pass on top of this fix. Found and fixed a genuine test-suite inconsistency while verifying: the existing "refreshes the active connection after a pooled profile reconnect succeeds" test in gateway-shared-remote.test.ts asserted setConnection was called once after a SINGLE ensureGatewayForProfile() call whose first dial failed -- i.e. it encoded the exact bug this issue reports as the EXPECTED, correct behavior. Rewrote it to assert the corrected contract: the failed first attempt does not call setConnection at all, and a realistic retry (calling ensureGatewayForProfile() again, since g.activeKey correctly never left the primary after the failed attempt -- ensureActiveGatewayOpen() is for reconnecting an already-active gateway that went stale, not retrying an activation that never succeeded) succeeds and publishes once the second dial goes through. Added a new test file (gateway-secondary-open-check.test.ts) following the established mocking pattern from gateway-agent-scope.test.ts, covering both ensureGatewayForAgent and ensureGatewayForProfile: a transient first-dial failure does not activate/publish (the exact reported symptom), and a successful dial still activates/publishes normally (sanity, no regression to the happy path). Verified as genuine regressions by reverting both isOpen() checks and confirming 2 of 4 new tests fail with exactly the reported symptom (activated resolves true / the primary gets replaced despite the failed dial). 44/44 pass across all 9 gateway-related test files (no regression). --- .../gateway-secondary-open-check.test.ts | 159 ++++++++++++++++++ .../src/store/gateway-shared-remote.test.ts | 16 +- apps/desktop/src/store/gateway.ts | 19 ++- 3 files changed, 186 insertions(+), 8 deletions(-) create mode 100644 apps/desktop/src/store/gateway-secondary-open-check.test.ts diff --git a/apps/desktop/src/store/gateway-secondary-open-check.test.ts b/apps/desktop/src/store/gateway-secondary-open-check.test.ts new file mode 100644 index 0000000000000..237faacbfd4d6 --- /dev/null +++ b/apps/desktop/src/store/gateway-secondary-open-check.test.ts @@ -0,0 +1,159 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +// Regression for issue #92265: a transient first-dial WebSocket failure +// (e.g. ECONNRESET before the socket reaches `open`) must not let Desktop +// publish the closed gateway as the active route. entry.connection is set +// BEFORE the dial completes in openSecondary(), so checking only its +// truthiness previously let a failed activation still "succeed" and +// publish -- the next chat RPC then failed with "Hermes gateway is not +// connected" even though the UI had already switched to that route. + +const gatewayMocks = vi.hoisted(() => ({ + connect: vi.fn(async (_wsUrl: string): Promise => undefined), + setConnection: vi.fn(), + setGatewayState: vi.fn() +})) + +vi.mock('@/hermes', () => ({ + setApiRequestConnection: vi.fn(), + HermesGateway: class { + connectionState = 'closed' + connect = async (wsUrl: string): Promise => { + // Unlike gateway-agent-scope.test.ts's always-succeeds mock, this + // one propagates gatewayMocks.connect's outcome -- letting tests + // below simulate a rejected first dial without flipping + // connectionState to 'open'. + await gatewayMocks.connect(wsUrl) + this.connectionState = 'open' + } + close = (): void => { + this.connectionState = 'closed' + } + onEvent = vi.fn(() => () => {}) + onState = vi.fn(() => () => {}) + } +})) +vi.mock('@/store/session', () => ({ + setConnection: gatewayMocks.setConnection, + setGatewayState: gatewayMocks.setGatewayState +})) +vi.mock('@/store/notify-baseline', () => ({ markNativeNotifyBaseline: vi.fn() })) + +const { + $gateway, + activeGateway, + closeSecondaryGateways, + configureGatewayRegistry, + ensureGatewayForAgent, + ensureGatewayForProfile, + isActivePrimary, + setPrimaryGateway +} = await import('./gateway') + +interface DesktopStub { + getConnection: ReturnType + getConnectionFor: ReturnType +} + +function installDesktop(stub: DesktopStub): void { + ;(window as unknown as { hermesDesktop: unknown }).hermesDesktop = stub +} + +function makePrimary(): { connectionState: string } { + return { connectionState: 'open' } +} + +const agentConn = { + authMode: 'token', + baseUrl: 'https://homelab.invalid', + mode: 'remote', + profile: 'research', + token: 'fake-test-token', + wsUrl: 'wss://homelab.invalid/api/ws?token=fake-test-token' +} + +function installAgentDesktop(): DesktopStub { + const stub: DesktopStub = { + getConnection: vi.fn(async () => agentConn), + getConnectionFor: vi.fn(async () => agentConn) + } + + installDesktop(stub) + + return stub +} + +beforeEach(() => { + configureGatewayRegistry({ onEvent: vi.fn() }) +}) + +afterEach(() => { + closeSecondaryGateways() + vi.clearAllMocks() + delete (window as unknown as { hermesDesktop?: unknown }).hermesDesktop +}) + +describe('secondary activation requires an open socket, not just a connection descriptor (issue #92265)', () => { + it('ensureGatewayForAgent: a transient first-dial failure does not activate or publish the closed gateway', async () => { + const primary = makePrimary() + setPrimaryGateway(primary as never, 'default') + await ensureGatewayForProfile('default') + const publishedPrimary = $gateway.get() + installAgentDesktop() + + gatewayMocks.connect.mockRejectedValueOnce(new Error('ECONNRESET')) + + const activated = await ensureGatewayForAgent('homelab', 'research') + + expect(activated).toBe(false) + // The exact reported symptom: the UI must NOT have switched away from + // the primary onto the closed secondary. + expect(isActivePrimary()).toBe(true) + expect(activeGateway()).toBe(primary) + expect($gateway.get()).toBe(publishedPrimary) + }) + + it('ensureGatewayForAgent: a successful dial still activates and publishes normally', async () => { + const primary = makePrimary() + setPrimaryGateway(primary as never, 'default') + installAgentDesktop() + + const activated = await ensureGatewayForAgent('homelab', 'research') + + expect(activated).toBe(true) + expect(isActivePrimary()).toBe(false) + expect(activeGateway()).not.toBe(primary) + expect($gateway.get()).not.toBe(primary) + }) + + it('ensureGatewayForProfile: a transient first-dial failure does not publish the closed gateway', async () => { + const primary = makePrimary() + setPrimaryGateway(primary as never, 'default') + await ensureGatewayForProfile('default') + const publishedPrimary = $gateway.get() + installDesktop({ + getConnection: vi.fn(async () => agentConn), + getConnectionFor: vi.fn(async () => agentConn) + }) + + gatewayMocks.connect.mockRejectedValueOnce(new Error('ECONNRESET')) + + await ensureGatewayForProfile('research') + + // Must still be on the primary -- the closed secondary was never + // published as the active route. + expect(isActivePrimary()).toBe(true) + expect($gateway.get()).toBe(publishedPrimary) + }) + + it('ensureGatewayForProfile: a successful dial still activates and publishes normally', async () => { + const primary = makePrimary() + setPrimaryGateway(primary as never, 'default') + installAgentDesktop() + + await ensureGatewayForProfile('research') + + expect(isActivePrimary()).toBe(false) + expect($gateway.get()).not.toBe(primary) + }) +}) diff --git a/apps/desktop/src/store/gateway-shared-remote.test.ts b/apps/desktop/src/store/gateway-shared-remote.test.ts index 690b9667436e3..aee4d3eead613 100644 --- a/apps/desktop/src/store/gateway-shared-remote.test.ts +++ b/apps/desktop/src/store/gateway-shared-remote.test.ts @@ -41,7 +41,6 @@ const { $gateway, closeSecondaryGateways, configureGatewayRegistry, - ensureActiveGatewayOpen, ensureGatewayForProfile, setPrimaryGateway } = await import('./gateway') @@ -113,7 +112,7 @@ describe('ensureGatewayForProfile under a shared global remote', () => { expect($gateway.get()).not.toBe(primary) }) - it('refreshes the active connection after a pooled profile reconnect succeeds', async () => { + it('does not publish on a failed first dial, but a retried activation succeeds once the reconnect works (issue #92265)', async () => { const connection = { authMode: 'token', baseUrl: 'https://worker.invalid', @@ -130,14 +129,19 @@ describe('ensureGatewayForProfile under a shared global remote', () => { gatewayMocks.connect.mockRejectedValueOnce(new Error('temporarily offline')).mockResolvedValueOnce(undefined) + // First attempt: the dial fails transiently. Per the fix, this must + // NOT publish the closed gateway as the active connection -- the + // prior behavior here (publish immediately, closed socket and all) + // is exactly the bug #92265 reports. await ensureGatewayForProfile('worker') - expect(gatewayMocks.setConnection).toHaveBeenCalledOnce() - expect(gatewayMocks.setConnection).toHaveBeenLastCalledWith(connection) + expect(gatewayMocks.setConnection).not.toHaveBeenCalled() - await ensureActiveGatewayOpen() + // Retrying the activation (e.g. the user selects the profile again, + // or an app-level retry) succeeds on the second dial and publishes. + await ensureGatewayForProfile('worker') - expect(gatewayMocks.setConnection).toHaveBeenCalledTimes(2) + expect(gatewayMocks.setConnection).toHaveBeenCalledOnce() expect(gatewayMocks.setConnection).toHaveBeenLastCalledWith(connection) }) }) diff --git a/apps/desktop/src/store/gateway.ts b/apps/desktop/src/store/gateway.ts index f9b9eb2ca2713..40e1c8d40b068 100644 --- a/apps/desktop/src/store/gateway.ts +++ b/apps/desktop/src/store/gateway.ts @@ -749,11 +749,17 @@ export async function ensureGatewayForAgent(connectionId: null | string, profile entry.activationLeaseUntil = 0 // A source edit/remove may dispose this entry while its dial is still in - // flight. Only the still-registered, still-owned activation may publish. + // flight. Only the still-registered, still-owned activation may publish -- + // and only when the WebSocket actually reached open: entry.connection is + // set BEFORE the dial completes in openSecondary, so a transient first-dial + // failure (caught above, left for scheduleReconnect) must not count as a + // successful activation just because a connection descriptor exists + // (issue #92265). const activated = entry.wantOpen && g.secondaries.get(scope) === entry && Boolean(entry.connection) && + isOpen(entry.gateway) && applyActive(scope, activationEpoch) if (activated && entry.connection) { @@ -812,7 +818,16 @@ export async function ensureGatewayForProfile(profile: string): Promise { // The activation is settling either way — release the prune lease. entry.activationLeaseUntil = 0 - if (entry.wantOpen && g.secondaries.get(key) === entry && applyActive(key, activationEpoch) && entry.connection) { + // Only publish when the WebSocket actually reached open -- entry.connection + // is set before the dial completes, so a transient first-dial failure must + // not count as a successful activation (issue #92265). + if ( + entry.wantOpen && + g.secondaries.get(key) === entry && + isOpen(entry.gateway) && + applyActive(key, activationEpoch) && + entry.connection + ) { publishActiveConnection(entry.connection) } }