* 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>
187 lines
5.8 KiB
JavaScript
187 lines
5.8 KiB
JavaScript
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);
|
|
});
|
|
});
|