From 809654d488e7c3c1f9bd038bf52fb01a01dffff4 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Wed, 9 Sep 2026 17:10:09 +0530 Subject: [PATCH] refactor(desktop): keep main's boot classification and share the ws-URL guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the renderer-dial retry: - Drop the `!connectionDescriptorResolved` gate so `bootFailureIsRetryable()` still decides every failure main can see (#82679 contract preserved for post-descriptor mint failures); the renderer-owned dial is OR'd in as the one failure main cannot classify. The budget check now short-circuits before the IPC, as on main. - Four booleans → one `stage` variable; the `isGatewayReauthRequired` term is dropped (reauth is raised before the dial stage, so it was unreachable). - `isGatewayWebSocketUrl` moves to `apps/shared/src/json-rpc-gateway.ts` and `JsonRpcGatewayClient.connect()` uses it, so the hook's "valid dial" predicate cannot drift from what `connect()` accepts. - Tests: keep the three that bind behaviour (first dial retries under a stale ready snapshot; invalid URL terminal; post-connect failure terminal), drop the four that pinned the removed gate or duplicated the existing #82679 bound test. 46/46 green; reverting the hook to main makes the retry test fail. --- .../gateway/hooks/use-gateway-boot.test.tsx | 139 ++---------------- .../src/app/gateway/hooks/use-gateway-boot.ts | 81 ++++------ apps/shared/src/index.ts | 1 + apps/shared/src/json-rpc-gateway.ts | 27 ++-- 4 files changed, 57 insertions(+), 191 deletions(-) diff --git a/apps/desktop/src/app/gateway/hooks/use-gateway-boot.test.tsx b/apps/desktop/src/app/gateway/hooks/use-gateway-boot.test.tsx index a6f01b4e67..d20e0cb89c 100644 --- a/apps/desktop/src/app/gateway/hooks/use-gateway-boot.test.tsx +++ b/apps/desktop/src/app/gateway/hooks/use-gateway-boot.test.tsx @@ -1,4 +1,3 @@ -import { GatewayReauthRequiredError } from '@hermes/shared' import { act, cleanup, render } from '@testing-library/react' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' @@ -1666,71 +1665,18 @@ describe('useGatewayBoot remote reconnect loop (real hook, fake socket)', () => expect($desktopBoot.get().error).toBeNull() }) - it('RETRY CONTRACT: a local descriptor never enters the renderer-side remote retry branch', async () => { - const desktop = fakeDesktop() - desktop.getConnection.mockImplementation(async () => ({ ...remotePrimaryConn, mode: 'local' as const }) as never) - desktop.getBootProgress = vi.fn(async () => ({ - error: 'contradictory main snapshot', - fakeMode: false, - message: 'Desktop boot failed', - phase: 'backend.error', - progress: 24, - retryable: true, - running: false, - timestamp: 1 - })) - ;(window as { hermesDesktop?: unknown }).hermesDesktop = desktop - FakeWebSocket.mode = 'fail' - - render() - await flushAsync() - - expect($desktopBoot.get().error).toBeTruthy() - expect(desktop.getConnection).toHaveBeenCalledTimes(1) - await advanceBackoff() - expect(desktop.getConnection).toHaveBeenCalledTimes(1) - }) - - it('RETRY CONTRACT: a confirmed remote reauth rejection remains terminal despite contradictory retryable progress', async () => { - const desktop = fakeDesktop() - desktop.getConnection = vi.fn(async () => ({ ...remotePrimaryConn, authMode: 'oauth' as const })) - desktop.getGatewayWsUrl = vi.fn(async () => { - throw new GatewayReauthRequiredError('Sign in again') - }) - desktop.getBootProgress = vi.fn(async () => ({ - error: 'contradictory main snapshot', - fakeMode: false, - message: 'Desktop boot failed', - phase: 'backend.error', - progress: 24, - retryable: true, - running: false, - timestamp: 1 - })) - ;(window as { hermesDesktop?: unknown }).hermesDesktop = desktop - - render() - await flushAsync() - - expect($desktopBoot.get().error).toBeTruthy() - expect(desktop.getConnection).toHaveBeenCalledTimes(1) - expect(FakeWebSocket.instances).toHaveLength(0) - await advanceBackoff() - expect(desktop.getConnection).toHaveBeenCalledTimes(1) - }) - - it('RETRY CONTRACT: invalid remote WebSocket URLs remain terminal despite contradictory retryable progress', async () => { + it('RETRY CONTRACT: an invalid remote WebSocket URL is not a dial failure — it stays terminal under the stale ready snapshot', async () => { const desktop = fakeDesktop() desktop.getConnection = vi.fn(async () => ({ ...remotePrimaryConn, wsUrl: 'not a WebSocket URL' })) desktop.getGatewayWsUrl = vi.fn(async () => 'not a WebSocket URL') desktop.getBootProgress = vi.fn(async () => ({ - error: 'contradictory main snapshot', + error: null, fakeMode: false, - message: 'Desktop boot failed', - phase: 'backend.error', - progress: 24, - retryable: true, - running: false, + message: 'Hermes is ready', + phase: 'backend.ready', + progress: 100, + retryable: false, + running: true, timestamp: 1 })) ;(window as { hermesDesktop?: unknown }).hermesDesktop = desktop @@ -1745,43 +1691,17 @@ describe('useGatewayBoot remote reconnect loop (real hook, fake socket)', () => expect(desktop.getConnection).toHaveBeenCalledTimes(1) }) - it('RETRY CONTRACT: missing OAuth ticket capability remains terminal despite contradictory retryable progress', async () => { - const desktop = fakeDesktop() - desktop.getConnection = vi.fn(async () => ({ ...remotePrimaryConn, authMode: 'oauth' as const })) - Reflect.deleteProperty(desktop, 'getGatewayWsUrl') - desktop.getBootProgress = vi.fn(async () => ({ - error: 'contradictory main snapshot', - fakeMode: false, - message: 'Desktop boot failed', - phase: 'backend.error', - progress: 24, - retryable: true, - running: false, - timestamp: 1 - })) - ;(window as { hermesDesktop?: unknown }).hermesDesktop = desktop - - render() - await flushAsync() - - expect($desktopBoot.get().error).toMatch(/cannot refresh OAuth WebSocket tickets/i) - expect(desktop.getConnection).toHaveBeenCalledTimes(1) - expect(FakeWebSocket.instances).toHaveLength(0) - await advanceBackoff() - expect(desktop.getConnection).toHaveBeenCalledTimes(1) - }) - - it('RETRY CONTRACT: a post-connect failure stays terminal even when its socket closes before boot catches it', async () => { + it('RETRY CONTRACT: a post-connect failure stays terminal even when its socket closes before boot catches it — a closed socket after a good dial is not a dial failure', async () => { const desktop = fakeDesktop() desktop.getConnection = vi.fn(async () => remotePrimaryConn) desktop.getBootProgress = vi.fn(async () => ({ - error: 'contradictory main snapshot', + error: null, fakeMode: false, - message: 'Desktop boot failed', - phase: 'backend.error', - progress: 24, - retryable: true, - running: false, + message: 'Hermes is ready', + phase: 'backend.ready', + progress: 100, + retryable: false, + running: true, timestamp: 1 })) ;(window as { hermesDesktop?: unknown }).hermesDesktop = desktop @@ -1801,37 +1721,6 @@ describe('useGatewayBoot remote reconnect loop (real hook, fake socket)', () => expect(desktop.getConnection).toHaveBeenCalledTimes(1) }) - it('RETRY CONTRACT: a persistent renderer-side remote dial outage consumes the existing five-attempt bound', async () => { - const desktop = fakeDesktop() - desktop.getConnection = vi.fn(async () => remotePrimaryConn) - desktop.getBootProgress = vi.fn(async () => ({ - error: null, - fakeMode: false, - message: 'Hermes is ready', - phase: 'backend.ready', - progress: 100, - retryable: false, - running: true, - timestamp: 1 - })) - ;(window as { hermesDesktop?: unknown }).hermesDesktop = desktop - FakeWebSocket.mode = 'fail' - - render() - await flushAsync() - - for (let i = 0; i < 7; i += 1) { - await advanceBackoff() - } - - expect(desktop.getConnection).toHaveBeenCalledTimes(6) - expect(FakeWebSocket.instances).toHaveLength(6) - expect($desktopBoot.get().error).toBeTruthy() - await advanceBackoff() - expect(desktop.getConnection).toHaveBeenCalledTimes(6) - expect(FakeWebSocket.instances).toHaveLength(6) - }) - it('FIX #82679: boot retries are BOUNDED — a persistently dead remote ends in the recovery overlay, not a spinner', async () => { const desktop = fakeDesktop() desktop.getConnection = vi.fn(async () => { diff --git a/apps/desktop/src/app/gateway/hooks/use-gateway-boot.ts b/apps/desktop/src/app/gateway/hooks/use-gateway-boot.ts index 0f42cd2dd3..1856dc97e4 100644 --- a/apps/desktop/src/app/gateway/hooks/use-gateway-boot.ts +++ b/apps/desktop/src/app/gateway/hooks/use-gateway-boot.ts @@ -1,4 +1,4 @@ -import { isGatewayReauthRequired, JsonRpcGatewayError, resolveGatewayWsUrl } from '@hermes/shared' +import { isGatewayReauthRequired, isGatewayWebSocketUrl, JsonRpcGatewayError, resolveGatewayWsUrl } from '@hermes/shared' import { useEffect, useRef } from 'react' import { shouldApplyPostBootProgressError } from '@/components/boot-failure-reauth' @@ -108,13 +108,13 @@ const RECONNECT_ESCALATE_AFTER_MS = 300_000 // only a STREAK of unanswered pings rebuilds the transport. const GATEWAY_LIVENESS_PROBE_TIMEOUT_MS = 5_000 -// Bounded self-heal for a failed REMOTE boot (#82679): main classifies faults -// before a descriptor exists; the renderer also classifies a valid remote -// WebSocket dial that fails before becoming usable. The renderer re-attempts -// the whole boot with the same full-jitter backoff the post-boot reconnect loop -// uses, up to this many attempts. Retries are bounded and end in the real -// recovery affordance (the boot-failure overlay with Retry / Settings), never -// an infinite spinner. Local failures, mint/capability failures, and confirmed +// Bounded self-heal for a failed REMOTE boot (#82679): main classifies every +// fault it can see (via getBootProgress().retryable); the renderer adds the one +// it cannot — a valid remote WebSocket dial that fails before becoming usable. +// The renderer re-attempts the whole boot with the same full-jitter backoff the +// post-boot reconnect loop uses, up to this many attempts. Retries are bounded +// and end in the real recovery affordance (the boot-failure overlay with +// Retry / Settings), never an infinite spinner. Local failures and confirmed // reauth rejections never enter this loop. const BOOT_RETRY_MAX_ATTEMPTS = 5 // Base delay for boot retries. Deliberately slower than the socket reconnect @@ -139,16 +139,6 @@ export function primaryRuntimeConnectionId(connection: Pick void handleGatewayEvent: (event: RpcEvent) => void @@ -1026,13 +1016,10 @@ export function useGatewayBoot({ }) async function boot() { - // These are historical facts for one boot attempt, not a late read of + // Where this boot attempt got to — a historical fact, not a late read of // gateway.connectionState. A socket can close after a successful dial; // later initialization errors must not be reclassified as boot dials. - let connectionDescriptorResolved = false - let resolvedRemoteConnection = false - let remoteGatewayConnectAttempted = false - let gatewayConnectSucceeded = false + let stage: 'resolving' | 'minting' | 'dialing' | 'connected' = 'resolving' try { // A profile-pinned helper window (the HUD) dials its target profile's @@ -1052,8 +1039,7 @@ export function useGatewayBoot({ return } - connectionDescriptorResolved = true - resolvedRemoteConnection = conn.mode === 'remote' + stage = 'minting' setDesktopBootStep({ phase: 'renderer.gateway.connect', @@ -1079,23 +1065,21 @@ export function useGatewayBoot({ // Mint a fresh WS URL right before connecting. For OAuth gateways the // ticket is single-use with a short TTL, so the ticket baked into // conn.wsUrl is stale; resolveGatewayWsUrl() re-mints it rather than - // connecting with a dead ticket. Auth rejection asks for sign-in; the - // Electron mint path has its own bounded transport retries. This await - // is bounded like the reconnect path (#93454) so a wedged mint reaches - // the recovery affordance instead of hanging "Starting Hermes…". + // connecting with a dead ticket. Auth rejection asks for sign-in. This + // await is bounded like the reconnect path (#93454) so a wedged mint + // reaches the recovery affordance instead of hanging "Starting Hermes…". const wsUrl = await withTimeout( resolveGatewayWsUrl(desktop, conn), RECONNECT_ATTEMPT_TIMEOUT_MS, 'Timed out minting the gateway WebSocket URL' ) - // The complementary retry classification is deliberately narrower - // than every remote pre-open error: only a valid WebSocket dial after - // a resolved remote descriptor is transient here. URL and capability - // failures stay terminal at their own boundaries. - remoteGatewayConnectAttempted = resolvedRemoteConnection && isGatewayWebSocketUrl(wsUrl) + // Only a valid WebSocket dial against a remote descriptor counts as a + // transient renderer-side failure; URL and capability failures stay + // terminal at their own boundaries. + if (conn.mode === 'remote' && isGatewayWebSocketUrl(wsUrl)) {stage = 'dialing'} await gateway.connect(wsUrl) - gatewayConnectSucceeded = true + stage = 'connected' if (cancelled) { return @@ -1142,25 +1126,16 @@ export function useGatewayBoot({ if (!cancelled) { const message = err instanceof Error ? err.message : String(err) - // Preserve the main-process classification for failures before a - // descriptor is available. Once this renderer has a descriptor, the - // only complementary retry is a transient, valid remote WebSocket - // dial which never became usable. A confirmed reauth rejection, - // invalid URL/capability failure, local descriptor, and anything - // after a successful dial retain the terminal recovery surface. - const retryableBeforeDescriptor = !connectionDescriptorResolved && (await bootFailureIsRetryable()) + // Main's classification (#82679) still decides every failure it can + // see. The one it cannot see is the renderer-owned WebSocket dial: + // after a renderer reload main serves its cached descriptor with a + // stale `backend.ready / retryable:false` snapshot, so a remote dial + // that never became usable is retryable on its own. Anything after a + // successful dial keeps the terminal recovery surface. + const canRetry = bootRetryAttempt < BOOT_RETRY_MAX_ATTEMPTS + const retryable = canRetry && (stage === 'dialing' || (await bootFailureIsRetryable())) - const retryableRemoteGatewayDial = - resolvedRemoteConnection && - remoteGatewayConnectAttempted && - !gatewayConnectSucceeded && - !isGatewayReauthRequired(err) - - if ( - bootRetryAttempt < BOOT_RETRY_MAX_ATTEMPTS && - (retryableBeforeDescriptor || retryableRemoteGatewayDial) && - !cancelled - ) { + if (retryable && !cancelled) { const delay = reconnectBackoffDelayMs(bootRetryAttempt, { baseDelayMs: BOOT_RETRY_BASE_DELAY_MS }) bootRetryAttempt += 1 resumeDesktopBootForRetry(translateNow('boot.steps.retryingRemoteBackend')) diff --git a/apps/shared/src/index.ts b/apps/shared/src/index.ts index 51ec33fb6a..e563361fc5 100644 --- a/apps/shared/src/index.ts +++ b/apps/shared/src/index.ts @@ -52,6 +52,7 @@ export { type GatewayEvent, type GatewayEventName, type GatewayRequestId, + isGatewayWebSocketUrl, type JsonRpcErrorPayload, type JsonRpcFrame, JsonRpcGatewayClient, diff --git a/apps/shared/src/json-rpc-gateway.ts b/apps/shared/src/json-rpc-gateway.ts index 29f6f69e2e..420555fd59 100644 --- a/apps/shared/src/json-rpc-gateway.ts +++ b/apps/shared/src/json-rpc-gateway.ts @@ -100,6 +100,19 @@ const DEFAULT_HEARTBEAT_DEADLINE_MS = 45_000 // handshake doesn't land in this window, fail to 'error' so callers can retry. const DEFAULT_CONNECT_TIMEOUT_MS = 15_000 +/** True for a `ws://` / `wss://` URL string — the only thing `JsonRpcGatewayClient.connect()` will dial. */ +export function isGatewayWebSocketUrl(value: unknown): value is string { + if (typeof value !== 'string') {return false} + + try { + const protocol = new URL(value).protocol + + return protocol === 'ws:' || protocol === 'wss:' + } catch { + return false + } +} + export class JsonRpcGatewayClient { private nextId = 0 private pending = new Map() @@ -162,19 +175,7 @@ export class JsonRpcGatewayClient { return new Error(`gateway connect() requires a ws:// or wss:// URL string, got ${got}`) } - if (typeof wsUrl !== 'string') { - throw invalidUrl() - } - - let url: URL - - try { - url = new URL(wsUrl) - } catch { - throw invalidUrl() - } - - if (url.protocol !== 'ws:' && url.protocol !== 'wss:') { + if (!isGatewayWebSocketUrl(wsUrl)) { throw invalidUrl() }