fix: resilient reconnect — preserve state on fetch fail, pause when offline (#1308)

* fix: preserve state when reconnect-time fetches fail

Several client API methods swallowed fetch/SDK errors and returned an
empty value (`[]`, `{}`), which was indistinguishable from a successful
"server says nothing here" response. Reconnect resync paths trusted that
empty result as authoritative and deleted local state — so after a
network blip (sleep/wake, wifi reconnect, tunnel switch), the UI could
show:

- sessions stuck on the "running" indicator (status never cleared)
- pending permission prompts disappearing from the UI
- pending question prompts disappearing from the UI

and only a page reload would recover. A related case: `listAgents`
silently returning `[]` defeated the 3-attempt retry loop in
`useAgentsStore` because the loop never saw an error.

The systematic fix:

- `getSessionStatusForDirectory` now returns `null` on fetch failure
  (vs the previous `{}`); the reconnect resync treats only a non-null
  response as authoritative — candidates missing from the response are
  written as `{type: "idle"}`, candidates after a failure are left
  untouched.
- `listPendingPermissions`, `listPendingQuestions`, and `listAgents`
  now throw on SDK/network failure. The pre-existing outer try/catch
  blocks in `resyncBlockingRequestsForDirectory` and the retry loop in
  `useAgentsStore` were already in the right shape — they just never
  fired because no exception was thrown. A small `formatSdkError`
  helper renders the SDK `{data, error}` shape into the thrown message.
- `permissionStore.setSessionAutoAccept` catches the new throw and
  falls back to whatever sync-store snapshots provide; the next SSE
  event or reconnect resync will catch up anything missed.

AGENTS.md gets a new "Distinguish fetch failure from empty success"
subsection documenting the principle (throw vs `T | null` patterns,
when to pick which, the retry-loop trap) so this doesn't regress.

Adds 3 regression tests covering the resync paths: existing
questions/permissions are preserved when the corresponding `list*`
method throws, and a permission-fetch failure does not block the
question block from running (verifies per-block try/catch isolation).

* fix: pause reconnect loop when offline or hidden

The SSE/WebSocket reconnect loop retried indefinitely with no awareness
of whether the browser was online or whether the tab was even visible.
Three issues compounded:

- No `online`/`offline` event handling. With a foreground tab on a dead
  network, we'd hit the server every ~5s forever, and on network
  recovery we'd wait up to ~5s for the next probe instead of reacting
  to the `online` event.
- No visibility awareness. A backgrounded PWA on a flaky link kept
  probing at the same rate as a foreground tab. The browser does
  throttle hidden-tab timers, but the intent wasn't expressed in code.
- The "exponential backoff" math
  `min(5000, max(retryDelayMs, 250) * (failures <= 1 ? 1 : 2))`
  re-initialized `retryDelayMs` to 250 every iteration, so the cap of
  5s was never reached — we waited 500ms forever after the second
  failure. Not actually exponential.

Now:

- `online` event aborts the current attempt (if disconnected) and
  cuts inter-attempt waits short. `offline` event aborts so the loop
  enters the slow-probe path immediately.
- `computeRetryDelay` returns the long cap (60s) when `navigator.onLine`
  is false or the tab is hidden; the short cap (5s) when foreground +
  online. The `online` event is the expected recovery path; the 60s cap
  is a fallback for browsers that miss the event.
- Real exponential growth: `BASE * 2^min(failures-1, 8)`, clamped.
- New `waitForRetry` helper interrupts on `online`,
  visibility-becomes-visible, and abort signal — so visibility/network
  recovery doesn't wait out the rest of the current sleep.

AGENTS.md gets a "Reconnect-loop pacing" subsection alongside the
fetch-failure rule, since they're the same family of resilience
concerns.

One regression test: simulates offline + failed first attempt + `online`
event after the failure; verifies the next attempt fires within seconds
instead of waiting the full 60s offline cap.

* fix: long-cap backoff for permanent 4xx server errors

Before this commit the reconnect loop didn't distinguish HTTP error
types. A stuck-path client (wrong URL after server upgrade) or an
expired-auth client (stale token) would hit the server at the normal
5-second cap forever — ~12 reqs/min, indefinitely, with no path to
recovery besides the user reloading.

Now the catch block extracts an HTTP status (looking on `error.status`
and `error.response.status` — the SDK exposes both depending on the
code path) and overrides the backoff:

- 4xx other than 408/429 → use the long cap (60s) immediately.
  Blind retries won't fix wrong path / bad auth / forbidden, so don't
  pound the server. waitForRetry's `online` / visibility-visible
  interrupters still apply — when an operator fixes the server-side
  config and the client comes back to foreground, recovery is prompt.
- 408 (Request Timeout) and 429 (Too Many Requests) → normal
  exponential path. Those are retryable in spirit.
- 5xx / network / unknown → normal exponential path. Unchanged.

AGENTS.md gets a new bullet under "Reconnect-loop pacing" covering
this — the rule fits naturally alongside the existing `navigator.onLine`
and visibility signals.

Two regression tests:
- A 404-throwing SDK doesn't fire a second attempt within 250ms (proves
  we left the exponential path). After `online` interrupts the wait,
  subsequent attempts fire promptly — proves the override doesn't break
  recovery once the underlying problem is fixed.
- A 429-throwing SDK recovers within 2s — proves 429 still hits the
  fast exponential path and isn't caught by the permanent-error branch.

---------

Co-authored-by: vhqtvn <8930337+vhqtvn@users.noreply.github.com>
This commit is contained in:
vhqtvn
2026-05-18 17:47:20 +03:00
committed by GitHub
co-authored by vhqtvn
parent d5cdf464fa
commit ff35f40b43
8 changed files with 598 additions and 52 deletions
+25
View File
@@ -210,6 +210,31 @@ All scripts are in `package.json`.
- Prefer per-item results, rollback paths, or resumable cleanup over all-or-nothing assumptions. - Prefer per-item results, rollback paths, or resumable cleanup over all-or-nothing assumptions.
- Never leave optimistic state or local caches stranded after failure. - 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) ## CLI Parity and Safety Policy (MANDATORY)
### Principle: policy-first, UX-second ### Principle: policy-first, UX-second
+65 -25
View File
@@ -26,6 +26,25 @@ import {
// Use relative path by default (works with both dev and nginx proxy server) // 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 // Can be overridden with VITE_OPENCODE_URL for absolute URLs in special deployments
const DEFAULT_BASE_URL = import.meta.env.VITE_OPENCODE_URL || "/api"; 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 ABSOLUTE_URL_PATTERN = /^[a-zA-Z][a-zA-Z\d+\-.]*:\/\//;
const ID_RANDOM_CHARS = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"; const ID_RANDOM_CHARS = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
const ID_RANDOM_LENGTH = 14; const ID_RANDOM_LENGTH = 14;
@@ -951,12 +970,20 @@ class OpencodeService {
async getSessionStatus(): Promise< async getSessionStatus(): Promise<
Record<string, { type: "idle" | "busy" | "retry"; attempt?: number; message?: string; next?: number }> Record<string, { type: "idle" | "busy" | "retry"; attempt?: number; message?: string; next?: number }>
> { > {
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( async getSessionStatusForDirectory(
directory: string | null | undefined directory: string | null | undefined
): Promise<Record<string, { type: "idle" | "busy" | "retry"; attempt?: number; message?: string; next?: number }>> { ): Promise<Record<string, { type: "idle" | "busy" | "retry"; attempt?: number; message?: string; next?: number }> | null> {
try { try {
const base = this.baseUrl.replace(/\/$/, ""); const base = this.baseUrl.replace(/\/$/, "");
const url = new URL(`${base}/session/status`); const url = new URL(`${base}/session/status`);
@@ -974,12 +1001,12 @@ class OpencodeService {
}); });
if (!response.ok) { if (!response.ok) {
return {}; return null;
} }
const data = await response.json().catch(() => null); const data = await response.json().catch(() => null);
if (!data || typeof data !== "object") { if (!data || typeof data !== "object") {
return {}; return null;
} }
return data as Record< return data as Record<
@@ -987,14 +1014,14 @@ class OpencodeService {
{ type: "idle" | "busy" | "retry"; attempt?: number; message?: string; next?: number } { type: "idle" | "busy" | "retry"; attempt?: number; message?: string; next?: number }
>; >;
} catch { } catch {
return {}; return null;
} }
} }
async getGlobalSessionStatus(): Promise< async getGlobalSessionStatus(): Promise<
Record<string, { type: "idle" | "busy" | "retry"; attempt?: number; message?: string; next?: number }> Record<string, { type: "idle" | "busy" | "retry"; attempt?: number; message?: string; next?: number }>
> { > {
return this.getSessionStatusForDirectory(null); return (await this.getSessionStatusForDirectory(null)) ?? {};
} }
/** /**
@@ -1059,17 +1086,22 @@ class OpencodeService {
return result.data || false; 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<string | null | undefined> }): Promise<PermissionRequest[]> { async listPendingPermissions(options?: { directories?: Array<string | null | undefined> }): Promise<PermissionRequest[]> {
const fetches: Array<Promise<PermissionRequest[]>> = []; const fetches: Array<Promise<PermissionRequest[]>> = [];
const fetchForDirectory = async (directory?: string | null): Promise<PermissionRequest[]> => { const fetchForDirectory = async (directory?: string | null): Promise<PermissionRequest[]> => {
try { const trimmed = typeof directory === 'string' ? directory.trim() : '';
const trimmed = typeof directory === 'string' ? directory.trim() : ''; const result = await this.client.permission.list(trimmed ? { directory: trimmed } : undefined);
const result = await this.client.permission.list(trimmed ? { directory: trimmed } : undefined); if (result.error) {
return (result.data || []) as unknown as PermissionRequest[]; throw new Error(`permission.list failed: ${formatSdkError(result.error)}`);
} catch {
return [];
} }
return (result.data || []) as unknown as PermissionRequest[];
}; };
// Try unscoped first (server may return global pending items). // Try unscoped first (server may return global pending items).
@@ -1133,17 +1165,21 @@ class OpencodeService {
return result.data || false; 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<string | null | undefined> }): Promise<QuestionRequest[]> { async listPendingQuestions(options?: { directories?: Array<string | null | undefined> }): Promise<QuestionRequest[]> {
const fetches: Array<Promise<QuestionRequest[]>> = []; const fetches: Array<Promise<QuestionRequest[]>> = [];
const fetchForDirectory = async (directory?: string | null): Promise<QuestionRequest[]> => { const fetchForDirectory = async (directory?: string | null): Promise<QuestionRequest[]> => {
try { const trimmed = typeof directory === 'string' ? directory.trim() : '';
const trimmed = typeof directory === 'string' ? directory.trim() : ''; const result = await this.client.question.list(trimmed ? { directory: trimmed } : undefined);
const result = await this.client.question.list(trimmed ? { directory: trimmed } : undefined); if (result.error) {
return (result.data || []) as unknown as QuestionRequest[]; throw new Error(`question.list failed: ${formatSdkError(result.error)}`);
} catch {
return [];
} }
return (result.data || []) as unknown as QuestionRequest[];
}; };
// Try unscoped first (server may return global pending items). // Try unscoped first (server may return global pending items).
@@ -1257,15 +1293,19 @@ class OpencodeService {
} }
// Agent Management // 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<Agent[]> { async listAgents(): Promise<Agent[]> {
try { const response = await this.client.app.agents(
const response = await this.client.app.agents( this.currentDirectory ? { directory: this.currentDirectory } : undefined
this.currentDirectory ? { directory: this.currentDirectory } : undefined );
); if (response.error) {
return response.data || []; throw new Error(`app.agents failed: ${formatSdkError(response.error)}`);
} catch {
return [];
} }
return response.data || [];
} }
// SSE infrastructure removed — EventPipeline in sync/event-pipeline.ts handles // SSE infrastructure removed — EventPipeline in sync/event-pipeline.ts handles
+6 -1
View File
@@ -267,7 +267,12 @@ export const usePermissionStore = create<PermissionStore>()(
const directoryList = Array.from(directories); const directoryList = Array.from(directories);
const pendingFromStores = collectPendingFromSyncStores(); 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<string, { id: string; sessionID: string }>(); const mergedPending = new Map<string, { id: string; sessionID: string }>();
for (const permission of pendingFromStores) { for (const permission of pendingFromStores) {
@@ -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);
});
});
@@ -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);
});
});
@@ -6,15 +6,19 @@ const listPendingQuestionsCalls: Array<{ directories?: Array<string | null | und
const listPendingPermissionsCalls: Array<{ directories?: Array<string | null | undefined> }> = [] const listPendingPermissionsCalls: Array<{ directories?: Array<string | null | undefined> }> = []
let pendingQuestionsResponse: QuestionRequest[] = [] let pendingQuestionsResponse: QuestionRequest[] = []
let pendingPermissionsResponse: PermissionRequest[] = [] let pendingPermissionsResponse: PermissionRequest[] = []
let pendingQuestionsShouldThrow = false
let pendingPermissionsShouldThrow = false
mock.module("@/lib/opencode/client", () => ({ mock.module("@/lib/opencode/client", () => ({
opencodeClient: { opencodeClient: {
listPendingQuestions: mock(async (opts?: { directories?: Array<string | null | undefined> }) => { listPendingQuestions: mock(async (opts?: { directories?: Array<string | null | undefined> }) => {
listPendingQuestionsCalls.push(opts ?? {}) listPendingQuestionsCalls.push(opts ?? {})
if (pendingQuestionsShouldThrow) throw new Error("question.list failed: simulated")
return pendingQuestionsResponse return pendingQuestionsResponse
}), }),
listPendingPermissions: mock(async (opts?: { directories?: Array<string | null | undefined> }) => { listPendingPermissions: mock(async (opts?: { directories?: Array<string | null | undefined> }) => {
listPendingPermissionsCalls.push(opts ?? {}) listPendingPermissionsCalls.push(opts ?? {})
if (pendingPermissionsShouldThrow) throw new Error("permission.list failed: simulated")
return pendingPermissionsResponse return pendingPermissionsResponse
}), }),
getDirectory: () => "/repo", getDirectory: () => "/repo",
@@ -85,6 +89,8 @@ describe("resyncBlockingRequestsForDirectory", () => {
listPendingPermissionsCalls.length = 0 listPendingPermissionsCalls.length = 0
pendingQuestionsResponse = [] pendingQuestionsResponse = []
pendingPermissionsResponse = [] pendingPermissionsResponse = []
pendingQuestionsShouldThrow = false
pendingPermissionsShouldThrow = false
}) })
test("calls listPendingQuestions and listPendingPermissions exactly once for the directory", async () => { test("calls listPendingQuestions and listPendingPermissions exactly once for the directory", async () => {
@@ -156,4 +162,47 @@ describe("resyncBlockingRequestsForDirectory", () => {
expect(listPendingQuestionsCalls).toHaveLength(0) expect(listPendingQuestionsCalls).toHaveLength(0)
expect(listPendingPermissionsCalls).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)
})
}) })
+138 -4
View File
@@ -31,6 +31,16 @@ const DEFAULT_RECONNECT_DELAY_MS = 250
const DEFAULT_HEARTBEAT_TIMEOUT_MS = 30_000 const DEFAULT_HEARTBEAT_TIMEOUT_MS = 30_000
const WS_FALLBACK_WINDOW_MS = 60_000 const WS_FALLBACK_WINDOW_MS = 60_000
const DEFAULT_WS_READY_TIMEOUT_MS = 2_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+\-.]*:\/\// const ABSOLUTE_URL_PATTERN = /^[a-zA-Z][a-zA-Z\d+\-.]*:\/\//
export type EventPipelineInput = { export type EventPipelineInput = {
@@ -261,6 +271,93 @@ export function createEventPipeline(input: EventPipelineInput) {
error instanceof DOMException && error.name === "AbortError" || error instanceof DOMException && error.name === "AbortError" ||
(typeof error === "object" && error !== null && (error as { name?: string }).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<void>((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<typeof setTimeout> | 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 streamErrorLogged = false
let attempt: AbortController | undefined let attempt: AbortController | undefined
let lastEventAt = Date.now() let lastEventAt = Date.now()
@@ -600,9 +697,24 @@ export function createEventPipeline(input: EventPipelineInput) {
: `${currentTransport}_error:unknown` : `${currentTransport}_error:unknown`
notifyDisconnected(reason) notifyDisconnected(reason)
// Backoff so a hard-down server doesn't spin the browser event loop. // Exponential backoff so a hard-down server / dead network doesn't
// Cap at 5s; reset occurs in markConnected(). // spin the event loop. Caps lower (5s) when the user is foreground
retryDelayMs = Math.min(5_000, Math.max(retryDelayMs, 250) * (consecutiveFailures <= 1 ? 1 : 2)) // 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 { } finally {
abort.signal.removeEventListener("abort", onAbort) abort.signal.removeEventListener("abort", onAbort)
@@ -617,7 +729,7 @@ export function createEventPipeline(input: EventPipelineInput) {
attemptAbortReason = null attemptAbortReason = null
} }
if (retryDelayMs > 0) { if (retryDelayMs > 0) {
await wait(retryDelayMs) await waitForRetry(retryDelayMs)
} }
} }
})().finally(flushAll) })().finally(flushAll)
@@ -642,6 +754,24 @@ export function createEventPipeline(input: EventPipelineInput) {
attempt?.abort() 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") { if (typeof document !== "undefined") {
document.addEventListener("visibilitychange", onVisibility) document.addEventListener("visibilitychange", onVisibility)
window.addEventListener("pageshow", onPageShow) window.addEventListener("pageshow", onPageShow)
@@ -651,6 +781,8 @@ export function createEventPipeline(input: EventPipelineInput) {
// test environments can replace globalThis.window with a stub. // test environments can replace globalThis.window with a stub.
if (typeof globalThis.window !== "undefined") { if (typeof globalThis.window !== "undefined") {
globalThis.window.addEventListener("openchamber:system-resume", onSystemResume) globalThis.window.addEventListener("openchamber:system-resume", onSystemResume)
globalThis.window.addEventListener("online", onOnline)
globalThis.window.addEventListener("offline", onOffline)
} }
const cleanup = () => { const cleanup = () => {
@@ -660,6 +792,8 @@ export function createEventPipeline(input: EventPipelineInput) {
} }
if (typeof globalThis.window !== "undefined") { if (typeof globalThis.window !== "undefined") {
globalThis.window.removeEventListener("openchamber:system-resume", onSystemResume) globalThis.window.removeEventListener("openchamber:system-resume", onSystemResume)
globalThis.window.removeEventListener("online", onOnline)
globalThis.window.removeEventListener("offline", onOffline)
} }
abort.abort() abort.abort()
flushAll() flushAll()
+22 -22
View File
@@ -889,33 +889,33 @@ async function resyncDirectoryAfterReconnect(
if (candidateSessionIds.length === 0) return if (candidateSessionIds.length === 0) return
const nextStatuses = await opencodeClient.getSessionStatusForDirectory(directory) const nextStatuses = await opencodeClient.getSessionStatusForDirectory(directory)
const relevantStatuses: Record<string, SessionStatus> = {} // null = fetch failed; preserve existing state. {} or populated = authoritative
// snapshot of active sessions — candidates not listed are idle now.
for (const sessionId of candidateSessionIds) { if (nextStatuses !== null) {
const nextStatus = toSessionStatus(nextStatuses[sessionId]) const relevantStatuses: Record<string, SessionStatus> = {}
if (nextStatus) { for (const sessionId of candidateSessionIds) {
relevantStatuses[sessionId] = nextStatus relevantStatuses[sessionId] = toSessionStatus(nextStatuses[sessionId]) ?? { type: "idle" }
} }
}
if (Object.keys(relevantStatuses).length > 0) { if (Object.keys(relevantStatuses).length > 0) {
store.setState((state: DirectoryStore) => { store.setState((state: DirectoryStore) => {
let changed = false let changed = false
for (const [sessionId, nextStatus] of Object.entries(relevantStatuses)) { for (const [sessionId, nextStatus] of Object.entries(relevantStatuses)) {
if (!haveEquivalentSyncSnapshots(state.session_status?.[sessionId], nextStatus)) { if (!haveEquivalentSyncSnapshots(state.session_status?.[sessionId], nextStatus)) {
changed = true changed = true
break break
}
} }
}
if (!changed) { if (!changed) {
return state return state
} }
return { return {
session_status: { ...state.session_status, ...relevantStatuses }, session_status: { ...state.session_status, ...relevantStatuses },
} }
}) })
}
} }
const scopedClient = opencodeClient.getScopedSdkClient(directory) const scopedClient = opencodeClient.getScopedSdkClient(directory)