diff --git a/AGENTS.md b/AGENTS.md index 35dd9de9..4b36e01b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -210,6 +210,31 @@ All scripts are in `package.json`. - Prefer per-item results, rollback paths, or resumable cleanup over all-or-nothing assumptions. - Never leave optimistic state or local caches stranded after failure. +### Distinguish fetch failure from empty success + +Client API methods that feed authoritative state (bootstrap, reconnect resync, retry loops) **must signal fetch failure distinctly from a successful-but-empty server response.** A method that swallows errors and returns `[]`/`{}`/`null` lets the caller delete or overwrite legitimate state on a transient network blip, indistinguishable from "the server says nothing here." + +- **Decide which methods are authoritative.** A method is authoritative if any caller uses its result to delete, clear, or replace persisted/sync state. UI-display-only methods (autocomplete, dropdowns, settings pages) can keep silent-empty fallback because the user's next action refreshes them. +- **For authoritative methods, pick one of two patterns** — both already exist in the codebase, do not invent a third: + - **Throw on failure** (e.g. `listPendingPermissions`, `listPendingQuestions`, `listAgents`, the `unwrap()` helper in `packages/ui/src/sync/bootstrap.ts`). Use this when the caller has an outer `try/catch` per logical block — the throw skips the block and preserves prior state. + - **Return `T | null` on failure, where `null` strictly means "fetch failed"** (e.g. `getSessionStatusForDirectory`, the `.catch(() => null)` + early-return-on-null pattern at the per-session reconnect loop in `sync-context.tsx`). Use this when the caller has follow-up work that should still run when one fetch fails. +- **Never swallow inside the method while returning the same type as success.** The SDK's `{data, error}` shape already does this silently — wrap with `if (result.error) throw …` so the failure can't be lost. +- **Verify the caller actually preserves state on failure.** Adding the throw is only half the fix; the consumer must not run the "delete missing" / "overwrite" branch unless it knows the fetch succeeded. The relevant outer `try/catch` is often already there but dormant. +- **Retry loops require a failure signal.** A `for (let attempt = 0; attempt < 3; …)` retry around a method that swallows to `[]` will run exactly once — the loop never sees an error. + +This rule is the API-layer counterpart of "Use live server/session state for live activity. Do not let historical anomalies masquerade as current execution." A fetch failure is the same kind of anomaly — don't let it masquerade as authoritative server state. + +### Reconnect-loop pacing + +The SSE/WebSocket reconnect loop in `packages/ui/src/sync/event-pipeline.ts` retries indefinitely. To avoid burning battery and server load on dead/idle connections, the loop's pacing must respect three signals: + +- **`navigator.onLine`**: when the browser reports offline, use the long backoff cap (~60s) instead of the short one (~5s). The expected recovery path is the `online` event, not the next probe. +- **`document.visibilityState`**: when hidden, use the long cap too. A backgrounded PWA shouldn't hammer the network at 1/5s; the browser may also throttle our timers, but state the intent in code rather than relying on it. +- **HTTP status of the last failure**: permanent 4xx errors (401, 403, 404, …) don't recover from blind retry. Jump straight to the long cap instead of running the normal exponential path; otherwise a stale-path or expired-token client would put ~12 reqs/min on the server log forever. 408 (Request Timeout) and 429 (Too Many Requests) are retryable in spirit — let them go through normal backoff. +- **Consecutive failures**: real exponential growth (`base * 2^failures`, clamped), not constant 500ms. A hard-down server should see geometrically fewer probes per minute over time. + +The inter-attempt wait must be interruptible by `online`, visibility-becomes-visible, and the pipeline's abort signal — otherwise recovery is delayed by however long the current sleep had left to run. + ## CLI Parity and Safety Policy (MANDATORY) ### Principle: policy-first, UX-second diff --git a/packages/ui/src/lib/opencode/client.ts b/packages/ui/src/lib/opencode/client.ts index c1dbfe8e..f642acaa 100644 --- a/packages/ui/src/lib/opencode/client.ts +++ b/packages/ui/src/lib/opencode/client.ts @@ -26,6 +26,25 @@ import { // Use relative path by default (works with both dev and nginx proxy server) // Can be overridden with VITE_OPENCODE_URL for absolute URLs in special deployments const DEFAULT_BASE_URL = import.meta.env.VITE_OPENCODE_URL || "/api"; + +/** + * Render an SDK error payload into a short string for Error messages. + * The SDK returns `{data, error}` shape without throwing on non-2xx; methods + * that need to signal failure (so callers can preserve state instead of + * conflating failure with an empty success) wrap the error with this helper. + */ +function formatSdkError(error: unknown): string { + if (error instanceof Error) return error.message; + if (typeof error === "string") return error; + if (error && typeof error === "object" && "message" in error && typeof (error as { message: unknown }).message === "string") { + return (error as { message: string }).message; + } + try { + return JSON.stringify(error); + } catch { + return String(error); + } +} const ABSOLUTE_URL_PATTERN = /^[a-zA-Z][a-zA-Z\d+\-.]*:\/\//; const ID_RANDOM_CHARS = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"; const ID_RANDOM_LENGTH = 14; @@ -951,12 +970,20 @@ class OpencodeService { async getSessionStatus(): Promise< Record > { - return this.getSessionStatusForDirectory(this.currentDirectory ?? null); + return (await this.getSessionStatusForDirectory(this.currentDirectory ?? null)) ?? {}; } + /** + * Returns the upstream `/session/status` map, or `null` if the fetch failed. + * + * `null` vs `{}` matters for reconnect resync: the server omits idle sessions + * from the response, so an empty `{}` means "everything is idle" and a candidate + * missing from the response is authoritatively idle. A network/HTTP failure must + * not be conflated with that — return `null` so the caller can preserve state. + */ async getSessionStatusForDirectory( directory: string | null | undefined - ): Promise> { + ): Promise | null> { try { const base = this.baseUrl.replace(/\/$/, ""); const url = new URL(`${base}/session/status`); @@ -974,12 +1001,12 @@ class OpencodeService { }); if (!response.ok) { - return {}; + return null; } const data = await response.json().catch(() => null); if (!data || typeof data !== "object") { - return {}; + return null; } return data as Record< @@ -987,14 +1014,14 @@ class OpencodeService { { type: "idle" | "busy" | "retry"; attempt?: number; message?: string; next?: number } >; } catch { - return {}; + return null; } } async getGlobalSessionStatus(): Promise< Record > { - return this.getSessionStatusForDirectory(null); + return (await this.getSessionStatusForDirectory(null)) ?? {}; } /** @@ -1059,17 +1086,22 @@ class OpencodeService { return result.data || false; } + /** + * Throws on fetch/SDK failure. Callers that drive authoritative state from + * the result (e.g. reconnect resync) must let the throw propagate so they + * can preserve existing state instead of conflating "fetch failed" with + * "server returned no pending permissions". + */ async listPendingPermissions(options?: { directories?: Array }): Promise { const fetches: Array> = []; const fetchForDirectory = async (directory?: string | null): Promise => { - try { - const trimmed = typeof directory === 'string' ? directory.trim() : ''; - const result = await this.client.permission.list(trimmed ? { directory: trimmed } : undefined); - return (result.data || []) as unknown as PermissionRequest[]; - } catch { - return []; + const trimmed = typeof directory === 'string' ? directory.trim() : ''; + const result = await this.client.permission.list(trimmed ? { directory: trimmed } : undefined); + if (result.error) { + throw new Error(`permission.list failed: ${formatSdkError(result.error)}`); } + return (result.data || []) as unknown as PermissionRequest[]; }; // Try unscoped first (server may return global pending items). @@ -1133,17 +1165,21 @@ class OpencodeService { return result.data || false; } + /** + * Throws on fetch/SDK failure. See {@link listPendingPermissions} for + * rationale — resync paths preserve state on throw via outer try/catch + * instead of conflating failure with an empty server response. + */ async listPendingQuestions(options?: { directories?: Array }): Promise { const fetches: Array> = []; const fetchForDirectory = async (directory?: string | null): Promise => { - try { - const trimmed = typeof directory === 'string' ? directory.trim() : ''; - const result = await this.client.question.list(trimmed ? { directory: trimmed } : undefined); - return (result.data || []) as unknown as QuestionRequest[]; - } catch { - return []; + const trimmed = typeof directory === 'string' ? directory.trim() : ''; + const result = await this.client.question.list(trimmed ? { directory: trimmed } : undefined); + if (result.error) { + throw new Error(`question.list failed: ${formatSdkError(result.error)}`); } + return (result.data || []) as unknown as QuestionRequest[]; }; // Try unscoped first (server may return global pending items). @@ -1257,15 +1293,19 @@ class OpencodeService { } // Agent Management + /** + * Throws on fetch/SDK failure so caller-side retry loops (see + * useAgentsStore) can observe failure and retry; silently returning an + * empty list would defeat retries and clear the cached agent list. + */ async listAgents(): Promise { - try { - const response = await this.client.app.agents( - this.currentDirectory ? { directory: this.currentDirectory } : undefined - ); - return response.data || []; - } catch { - return []; + const response = await this.client.app.agents( + this.currentDirectory ? { directory: this.currentDirectory } : undefined + ); + if (response.error) { + throw new Error(`app.agents failed: ${formatSdkError(response.error)}`); } + return response.data || []; } // SSE infrastructure removed — EventPipeline in sync/event-pipeline.ts handles diff --git a/packages/ui/src/stores/permissionStore.ts b/packages/ui/src/stores/permissionStore.ts index b4444855..3a12e811 100644 --- a/packages/ui/src/stores/permissionStore.ts +++ b/packages/ui/src/stores/permissionStore.ts @@ -267,7 +267,12 @@ export const usePermissionStore = create()( const directoryList = Array.from(directories); const pendingFromStores = collectPendingFromSyncStores(); - const pendingFromApi = await opencodeClient.listPendingPermissions({ directories: Array.from(directories) }); + // Best-effort: if listPendingPermissions throws (transient fetch failure), + // proceed with whatever sync-store snapshots gave us. The next SSE event + // or reconnect resync will auto-accept anything we missed. + const pendingFromApi = await opencodeClient + .listPendingPermissions({ directories: Array.from(directories) }) + .catch(() => []); const mergedPending = new Map(); for (const permission of pendingFromStores) { diff --git a/packages/ui/src/sync/__tests__/event-pipeline-online.test.js b/packages/ui/src/sync/__tests__/event-pipeline-online.test.js new file mode 100644 index 00000000..aa5f4b14 --- /dev/null +++ b/packages/ui/src/sync/__tests__/event-pipeline-online.test.js @@ -0,0 +1,107 @@ +import { afterEach, describe, expect, it } from 'bun:test'; +import { createEventPipeline } from '../event-pipeline'; + +const savedDocument = globalThis.document; +const savedWindow = globalThis.window; +const savedNavigator = globalThis.navigator; + +afterEach(() => { + globalThis.document = savedDocument; + globalThis.window = savedWindow; + globalThis.navigator = savedNavigator; +}); + +// Multi-listener event-target stub. The simpler single-slot stub used in +// event-pipeline-resume.test.js would break here because waitForRetry and the +// top-level onOnline handler both register for `online`. +function createEventTarget(extras = {}) { + const listeners = new Map(); + return { + ...extras, + addEventListener(event, handler) { + const list = listeners.get(event); + if (list) list.add(handler); + else listeners.set(event, new Set([handler])); + }, + removeEventListener(event, handler) { + listeners.get(event)?.delete(handler); + }, + dispatch(event) { + const list = listeners.get(event); + if (!list) return; + for (const handler of Array.from(list)) { + handler(); + } + }, + }; +} + +describe('createEventPipeline — online event', () => { + it('cuts the inter-attempt wait short when `online` fires after disconnect', async () => { + globalThis.document = createEventTarget({ visibilityState: 'visible' }); + globalThis.window = createEventTarget({ + location: { href: 'http://127.0.0.1:3000/', origin: 'http://127.0.0.1:3000' }, + }); + globalThis.navigator = { onLine: false }; + + let sdkCallIndex = 0; + const sdk = { + global: { + event: async () => { + const idx = sdkCallIndex++; + if (idx === 0) { + // Force a real failure so the loop enters the offline backoff path + // (computeRetryDelay returns the long cap because navigator.onLine + // is false). Without our `online` interrupt this would wait the + // full hidden/offline cap of 60s and the test would time out. + throw new Error('simulated network error'); + } + return { + stream: (async function* () { + yield { + payload: { + type: 'session.status', + properties: { sessionID: 's1', status: { type: 'idle' } }, + }, + }; + await new Promise(() => {}); + })(), + }; + }, + }, + }; + + const startedAt = Date.now(); + const elapsed = await new Promise((resolve) => { + let connects = 0; + const { cleanup } = createEventPipeline({ + sdk, + transport: 'sse', + heartbeatTimeoutMs: 60_000, + reconnectDelayMs: 60_000, + onEvent: () => {}, + onDisconnect: () => { + // We're now inside waitForRetry on the long offline cap. + // Flip the browser back online and fire the event; waitForRetry + // should resolve early and the next attempt should fire. + setTimeout(() => { + globalThis.navigator = { onLine: true }; + globalThis.window.dispatch('online'); + }, 30); + }, + onReconnect: () => { + connects += 1; + if (connects === 1) { + cleanup(); + resolve(Date.now() - startedAt); + } + }, + }); + }); + + // Two attempts: the failed one + the recovery one. If the `online` + // interrupt didn't fire, the test would have hung on the 60s offline cap. + expect(sdkCallIndex).toBe(2); + expect(elapsed).toBeLessThan(2_000); + }); +}); diff --git a/packages/ui/src/sync/__tests__/event-pipeline-permanent-error.test.js b/packages/ui/src/sync/__tests__/event-pipeline-permanent-error.test.js new file mode 100644 index 00000000..8055b469 --- /dev/null +++ b/packages/ui/src/sync/__tests__/event-pipeline-permanent-error.test.js @@ -0,0 +1,186 @@ +import { afterEach, describe, expect, it } from 'bun:test'; +import { createEventPipeline } from '../event-pipeline'; + +const savedDocument = globalThis.document; +const savedWindow = globalThis.window; +const savedNavigator = globalThis.navigator; + +afterEach(() => { + globalThis.document = savedDocument; + globalThis.window = savedWindow; + globalThis.navigator = savedNavigator; +}); + +function createEventTarget(extras = {}) { + const listeners = new Map(); + return { + ...extras, + addEventListener(event, handler) { + const list = listeners.get(event); + if (list) list.add(handler); + else listeners.set(event, new Set([handler])); + }, + removeEventListener(event, handler) { + listeners.get(event)?.delete(handler); + }, + dispatch(event) { + const list = listeners.get(event); + if (!list) return; + for (const handler of Array.from(list)) { + handler(); + } + }, + }; +} + +describe('createEventPipeline — permanent server errors', () => { + it('uses the long backoff cap for 4xx so we do not hammer at 5s intervals', async () => { + globalThis.document = createEventTarget({ visibilityState: 'visible' }); + globalThis.window = createEventTarget({ + location: { href: 'http://127.0.0.1:3000/', origin: 'http://127.0.0.1:3000' }, + }); + globalThis.navigator = { onLine: true }; + + let sdkCallIndex = 0; + const sdk = { + global: { + event: async () => { + const idx = sdkCallIndex++; + if (idx <= 1) { + // First two attempts: permanent 404. Under the old code these + // would have entered the exponential path and the second retry + // would fire after ~250-500ms. With the permanent-error override + // both go to the long (60s) cap, so the test should observe + // exactly one retry (after `online` interrupts) within its + // observation window. + const error = new Error('Not Found'); + error.status = 404; + throw error; + } + return { + stream: (async function* () { + yield { + payload: { + type: 'session.status', + properties: { sessionID: 's1', status: { type: 'idle' } }, + }, + }; + await new Promise(() => {}); + })(), + }; + }, + }, + }; + + const startedAt = Date.now(); + let cleanupFn = () => {}; + + // Phase 1: let the first 404 fire and verify the loop is NOT spinning. + // If the permanent-error override is broken, the loop would retry every + // 250-500ms and sdkCallIndex would climb past 1. + await new Promise((resolve) => { + const { cleanup } = createEventPipeline({ + sdk, + transport: 'sse', + heartbeatTimeoutMs: 60_000, + reconnectDelayMs: 60_000, + onEvent: () => {}, + onDisconnect: () => { + // Wait 250ms after disconnect — long enough that the broken + // exponential path would have retried at least once. If our + // override works, sdkCallIndex stays at 1. + setTimeout(resolve, 250); + }, + }); + cleanupFn = cleanup; + }); + + expect(sdkCallIndex).toBe(1); + + // Phase 2: fire `online` to interrupt the long wait. Loop should fire + // the second attempt (still 404) immediately, then the third attempt + // which succeeds. + const recovered = new Promise((resolve) => { + // Trigger an `online` event; waitForRetry's interrupter resolves and + // the next attempt fires. That attempt is also a 404 (idx=1), then + // another `online` advances us to the success path (idx=2). + const advance = () => { + globalThis.window.dispatch('online'); + }; + advance(); + const t = setInterval(() => { + if (sdkCallIndex >= 3) { + clearInterval(t); + resolve(); + } else { + advance(); + } + }, 50); + }); + + await recovered; + cleanupFn(); + + expect(sdkCallIndex).toBeGreaterThanOrEqual(3); + // Total elapsed should be < 2s — well under the 60s cap that proves the + // interrupters work for permanent-error retries too. + expect(Date.now() - startedAt).toBeLessThan(5_000); + }); + + it('retries 408 and 429 on the normal exponential path (not the permanent cap)', async () => { + globalThis.document = createEventTarget({ visibilityState: 'visible' }); + globalThis.window = createEventTarget({ + location: { href: 'http://127.0.0.1:3000/', origin: 'http://127.0.0.1:3000' }, + }); + globalThis.navigator = { onLine: true }; + + let sdkCallIndex = 0; + const sdk = { + global: { + event: async () => { + const idx = sdkCallIndex++; + if (idx === 0) { + const error = new Error('Rate limited'); + error.status = 429; + throw error; + } + return { + stream: (async function* () { + yield { + payload: { + type: 'session.status', + properties: { sessionID: 's1', status: { type: 'idle' } }, + }, + }; + await new Promise(() => {}); + })(), + }; + }, + }, + }; + + const startedAt = Date.now(); + const elapsed = await new Promise((resolve) => { + let connects = 0; + const { cleanup } = createEventPipeline({ + sdk, + transport: 'sse', + heartbeatTimeoutMs: 60_000, + reconnectDelayMs: 60_000, + onEvent: () => {}, + onReconnect: () => { + connects += 1; + if (connects === 1) { + cleanup(); + resolve(Date.now() - startedAt); + } + }, + }); + }); + + // 429 went through computeRetryDelay (consecutiveFailures=1) -> 250ms, + // not the 60s permanent cap. Recovery should be sub-second. + expect(sdkCallIndex).toBe(2); + expect(elapsed).toBeLessThan(2_000); + }); +}); diff --git a/packages/ui/src/sync/__tests__/session-switch-resync.test.ts b/packages/ui/src/sync/__tests__/session-switch-resync.test.ts index 2c2008de..7962067d 100644 --- a/packages/ui/src/sync/__tests__/session-switch-resync.test.ts +++ b/packages/ui/src/sync/__tests__/session-switch-resync.test.ts @@ -6,15 +6,19 @@ const listPendingQuestionsCalls: Array<{ directories?: Array }> = [] let pendingQuestionsResponse: QuestionRequest[] = [] let pendingPermissionsResponse: PermissionRequest[] = [] +let pendingQuestionsShouldThrow = false +let pendingPermissionsShouldThrow = false mock.module("@/lib/opencode/client", () => ({ opencodeClient: { listPendingQuestions: mock(async (opts?: { directories?: Array }) => { listPendingQuestionsCalls.push(opts ?? {}) + if (pendingQuestionsShouldThrow) throw new Error("question.list failed: simulated") return pendingQuestionsResponse }), listPendingPermissions: mock(async (opts?: { directories?: Array }) => { listPendingPermissionsCalls.push(opts ?? {}) + if (pendingPermissionsShouldThrow) throw new Error("permission.list failed: simulated") return pendingPermissionsResponse }), getDirectory: () => "/repo", @@ -85,6 +89,8 @@ describe("resyncBlockingRequestsForDirectory", () => { listPendingPermissionsCalls.length = 0 pendingQuestionsResponse = [] pendingPermissionsResponse = [] + pendingQuestionsShouldThrow = false + pendingPermissionsShouldThrow = false }) test("calls listPendingQuestions and listPendingPermissions exactly once for the directory", async () => { @@ -156,4 +162,47 @@ describe("resyncBlockingRequestsForDirectory", () => { expect(listPendingQuestionsCalls).toHaveLength(0) expect(listPendingPermissionsCalls).toHaveLength(0) }) + + // Regression: prior to the fix, listPendingQuestions silently returned [] on + // fetch failure, indistinguishable from a successful empty server response. + // The resync then walked the candidate set and deleted any question that + // wasn't in the (empty) result — wiping legitimate in-flight prompts on a + // transient network blip. The client method now throws on failure and the + // outer try/catch preserves existing state. + test("preserves existing questions when listPendingQuestions throws (transient fetch failure)", async () => { + const store = createDirectoryStore({ + question: { ses_a: [{ ...buildQuestion(), id: "que_in_flight" }] }, + }) + pendingQuestionsShouldThrow = true + + await resyncBlockingRequestsForDirectory("/repo", store) + + expect(store.getState().question["ses_a"]).toHaveLength(1) + expect(store.getState().question["ses_a"]?.[0]?.id).toBe("que_in_flight") + }) + + test("preserves existing permissions when listPendingPermissions throws (transient fetch failure)", async () => { + const store = createDirectoryStore({ + permission: { ses_a: [{ ...buildPermission(), id: "perm_in_flight" }] }, + }) + pendingPermissionsShouldThrow = true + + await resyncBlockingRequestsForDirectory("/repo", store) + + expect(store.getState().permission["ses_a"]).toHaveLength(1) + expect(store.getState().permission["ses_a"]?.[0]?.id).toBe("perm_in_flight") + }) + + test("permission fetch failure does not block question resync (and vice versa)", async () => { + const store = createDirectoryStore({}) + pendingQuestionsResponse = [buildQuestion()] + pendingPermissionsShouldThrow = true + + await resyncBlockingRequestsForDirectory("/repo", store) + + // Question block ran successfully despite permission block failing. + expect(store.getState().question["ses_a"]).toHaveLength(1) + expect(store.getState().question["ses_a"]?.[0]?.id).toBe("que_1") + expect(listPendingPermissionsCalls).toHaveLength(1) + }) }) diff --git a/packages/ui/src/sync/event-pipeline.ts b/packages/ui/src/sync/event-pipeline.ts index 7e8282a8..cef03ece 100644 --- a/packages/ui/src/sync/event-pipeline.ts +++ b/packages/ui/src/sync/event-pipeline.ts @@ -31,6 +31,16 @@ const DEFAULT_RECONNECT_DELAY_MS = 250 const DEFAULT_HEARTBEAT_TIMEOUT_MS = 30_000 const WS_FALLBACK_WINDOW_MS = 60_000 const DEFAULT_WS_READY_TIMEOUT_MS = 2_000 +// Retry pacing. Visible+online tabs probe quickly so the user sees connection +// recovery in under a second of real outage; hidden/offline tabs back off +// further so a backgrounded PWA on a flaky link doesn't burn battery probing +// a dead network every few seconds. The browser would throttle hidden-tab +// timers anyway, but this keeps the intent explicit and shrinks server load +// from idle tabs. +const RETRY_BACKOFF_BASE_MS = 250 +const RETRY_BACKOFF_CAP_VISIBLE_MS = 5_000 +const RETRY_BACKOFF_CAP_HIDDEN_OR_OFFLINE_MS = 60_000 +const RETRY_BACKOFF_MAX_EXPONENT = 8 const ABSOLUTE_URL_PATTERN = /^[a-zA-Z][a-zA-Z\d+\-.]*:\/\// export type EventPipelineInput = { @@ -261,6 +271,93 @@ export function createEventPipeline(input: EventPipelineInput) { error instanceof DOMException && error.name === "AbortError" || (typeof error === "object" && error !== null && (error as { name?: string }).name === "AbortError") + const isOffline = (): boolean => + typeof navigator === "object" && navigator !== null && navigator.onLine === false + + const isHidden = (): boolean => + typeof document !== "undefined" && document.visibilityState !== "visible" + + // Extract an HTTP status code from anywhere it might be hiding on the + // error object. The SDK's unwrap pattern stashes it on `.status`; raw + // fetch failures may carry `.response.status`; some SDKs also use `.code`. + const extractStatus = (error: unknown): number | undefined => { + if (!error || typeof error !== "object") return undefined + const direct = (error as { status?: unknown }).status + if (typeof direct === "number") return direct + const fromResponse = (error as { response?: { status?: unknown } }).response?.status + if (typeof fromResponse === "number") return fromResponse + return undefined + } + + // 4xx errors don't recover from blind retry — wrong path, expired auth, + // bad request body. Keep retrying anyway (a remote reconfigure or reauth + // can fix the underlying problem) but at the long cap so we're not + // hammering the server at 5s intervals indefinitely. 408 (timeout) and + // 429 (rate limit) are retryable in spirit — let them through to the + // normal exponential path. + const isPermanentHttpStatus = (status: number): boolean => { + if (status < 400 || status >= 500) return false + if (status === 408 || status === 429) return false + return true + } + + /** + * Wait between reconnect attempts. Resolves early when: + * - the browser fires `online` (network came back — probe immediately), + * - the tab becomes visible (user came back — probe immediately), + * - the pipeline is being torn down (cleanup aborts). + * Otherwise resolves after `ms` like a plain timer. + */ + const waitForRetry = (ms: number) => new Promise((resolve) => { + if (ms <= 0 || abort.signal.aborted) { + resolve() + return + } + + const cleanup = () => { + if (timer !== undefined) { + clearTimeout(timer) + timer = undefined + } + if (typeof globalThis.window !== "undefined") { + globalThis.window.removeEventListener("online", onInterrupt) + } + if (typeof document !== "undefined") { + document.removeEventListener("visibilitychange", onVisibilityInterrupt) + } + abort.signal.removeEventListener("abort", onInterrupt) + } + const onInterrupt = () => { + cleanup() + resolve() + } + const onVisibilityInterrupt = () => { + if (typeof document !== "undefined" && document.visibilityState === "visible") { + onInterrupt() + } + } + + let timer: ReturnType | undefined = setTimeout(onInterrupt, ms) + if (typeof globalThis.window !== "undefined") { + globalThis.window.addEventListener("online", onInterrupt, { once: true }) + } + if (typeof document !== "undefined") { + document.addEventListener("visibilitychange", onVisibilityInterrupt) + } + abort.signal.addEventListener("abort", onInterrupt, { once: true }) + }) + + const computeRetryDelay = (failures: number): number => { + if (failures <= 0) return 0 + // Offline: don't spin probing a dead network. Use the long cap and rely on + // waitForRetry to resolve early when the `online` event fires. The cap is + // also a fallback for browsers that miss `online`. + if (isOffline()) return RETRY_BACKOFF_CAP_HIDDEN_OR_OFFLINE_MS + const cap = isHidden() ? RETRY_BACKOFF_CAP_HIDDEN_OR_OFFLINE_MS : RETRY_BACKOFF_CAP_VISIBLE_MS + const exponent = Math.min(failures - 1, RETRY_BACKOFF_MAX_EXPONENT) + return Math.min(cap, RETRY_BACKOFF_BASE_MS * 2 ** exponent) + } + let streamErrorLogged = false let attempt: AbortController | undefined let lastEventAt = Date.now() @@ -600,9 +697,24 @@ export function createEventPipeline(input: EventPipelineInput) { : `${currentTransport}_error:unknown` notifyDisconnected(reason) - // Backoff so a hard-down server doesn't spin the browser event loop. - // Cap at 5s; reset occurs in markConnected(). - retryDelayMs = Math.min(5_000, Math.max(retryDelayMs, 250) * (consecutiveFailures <= 1 ? 1 : 2)) + // Exponential backoff so a hard-down server / dead network doesn't + // spin the event loop. Caps lower (5s) when the user is foreground + // and the browser thinks it's online; caps higher (60s) when hidden + // or offline so a backgrounded PWA on a flaky link doesn't burn + // battery. waitForRetry below resolves early on `online` or + // visibility-visible so recovery is still under a second. + // + // Override for permanent 4xx errors: stuck-path / bad-auth scenarios + // won't recover from blind retry. Use the long cap immediately so + // the client doesn't pound the server log at 12 reqs/min. The + // waitForRetry interrupters still apply, so a fix on the other end + // followed by `online`/visibility recovery probes promptly. + const status = extractStatus(error) + if (status !== undefined && isPermanentHttpStatus(status)) { + retryDelayMs = RETRY_BACKOFF_CAP_HIDDEN_OR_OFFLINE_MS + } else { + retryDelayMs = computeRetryDelay(consecutiveFailures) + } } } finally { abort.signal.removeEventListener("abort", onAbort) @@ -617,7 +729,7 @@ export function createEventPipeline(input: EventPipelineInput) { attemptAbortReason = null } if (retryDelayMs > 0) { - await wait(retryDelayMs) + await waitForRetry(retryDelayMs) } } })().finally(flushAll) @@ -642,6 +754,24 @@ export function createEventPipeline(input: EventPipelineInput) { attempt?.abort() } + // Browser told us the network is back. If we're already in a disconnected + // cycle, abort the (stale) attempt and let the loop probe immediately; + // waitForRetry also resolves early on `online`, so any inter-attempt sleep + // ends now. Guard on `disconnected` so a spurious `online` from the browser + // doesn't disrupt a healthy connection. + const onOnline = () => { + if (!disconnected) return + attempt?.abort() + } + + // Browser told us we're offline. Abort the current attempt — its socket / + // fetch will throw soon anyway, this just stops sooner. computeRetryDelay + // then returns the long cap so we wait for `online` instead of hammering + // a dead network. + const onOffline = () => { + attempt?.abort() + } + if (typeof document !== "undefined") { document.addEventListener("visibilitychange", onVisibility) window.addEventListener("pageshow", onPageShow) @@ -651,6 +781,8 @@ export function createEventPipeline(input: EventPipelineInput) { // test environments can replace globalThis.window with a stub. if (typeof globalThis.window !== "undefined") { globalThis.window.addEventListener("openchamber:system-resume", onSystemResume) + globalThis.window.addEventListener("online", onOnline) + globalThis.window.addEventListener("offline", onOffline) } const cleanup = () => { @@ -660,6 +792,8 @@ export function createEventPipeline(input: EventPipelineInput) { } if (typeof globalThis.window !== "undefined") { globalThis.window.removeEventListener("openchamber:system-resume", onSystemResume) + globalThis.window.removeEventListener("online", onOnline) + globalThis.window.removeEventListener("offline", onOffline) } abort.abort() flushAll() diff --git a/packages/ui/src/sync/sync-context.tsx b/packages/ui/src/sync/sync-context.tsx index a68814a4..d35273ca 100644 --- a/packages/ui/src/sync/sync-context.tsx +++ b/packages/ui/src/sync/sync-context.tsx @@ -889,33 +889,33 @@ async function resyncDirectoryAfterReconnect( if (candidateSessionIds.length === 0) return const nextStatuses = await opencodeClient.getSessionStatusForDirectory(directory) - const relevantStatuses: Record = {} - - for (const sessionId of candidateSessionIds) { - const nextStatus = toSessionStatus(nextStatuses[sessionId]) - if (nextStatus) { - relevantStatuses[sessionId] = nextStatus + // null = fetch failed; preserve existing state. {} or populated = authoritative + // snapshot of active sessions — candidates not listed are idle now. + if (nextStatuses !== null) { + const relevantStatuses: Record = {} + for (const sessionId of candidateSessionIds) { + relevantStatuses[sessionId] = toSessionStatus(nextStatuses[sessionId]) ?? { type: "idle" } } - } - if (Object.keys(relevantStatuses).length > 0) { - store.setState((state: DirectoryStore) => { - let changed = false - for (const [sessionId, nextStatus] of Object.entries(relevantStatuses)) { - if (!haveEquivalentSyncSnapshots(state.session_status?.[sessionId], nextStatus)) { - changed = true - break + if (Object.keys(relevantStatuses).length > 0) { + store.setState((state: DirectoryStore) => { + let changed = false + for (const [sessionId, nextStatus] of Object.entries(relevantStatuses)) { + if (!haveEquivalentSyncSnapshots(state.session_status?.[sessionId], nextStatus)) { + changed = true + break + } } - } - if (!changed) { - return state - } + if (!changed) { + return state + } - return { - session_status: { ...state.session_status, ...relevantStatuses }, - } - }) + return { + session_status: { ...state.session_status, ...relevantStatuses }, + } + }) + } } const scopedClient = opencodeClient.getScopedSdkClient(directory)