diff --git a/CHANGELOG.md b/CHANGELOG.md index 765b013..c7c4e4c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,18 +8,19 @@ While pre-1.0, the public API may change between 0.x releases. ## [Unreleased] -### Fixed - -- **Rejection `code` now survives tx-dedup replay (#21).** The dedup record - persisted only the rejection message, so a client retrying the same `txId` - got the reason with no machine-readable `code` — breaking code-based error - handling on exactly the retry path it exists for. `_sync_seen_tx` gains an - `error_code` column (added in place on wake for already-deployed DOs), and - the replayed `rejected` frame is now shaped identically to the original: - `{ code, message }` when a code was recorded, `{ message }` otherwise. - ### Changed +- **Transport reconnect policy (ADR-0016; fixes #25, #26).** One option, + `reconnectDelay?: number | ((attempt, closeCode?, closeReason?) => number | null)` + (`null` = stop), replaces `reconnectDelayMs` (**breaking**, pre-1.0: a + number keeps the old meaning — the default policy's base delay). The + default (`defaultReconnectDelay`) replaces the fixed interval with capped + exponential backoff + full jitter (base 250 ms; cap 30 s; attempt counter + resets on a successful open) and treats application close codes 4000-4999 + as terminal, so an accept-then-close auth rejection (e.g. 4403) no longer + retries forever. A terminal stop surfaces through the new + `onClosed(code, reason)` hook, making auth closes distinguishable from + transient drops. - **Cohosting docs moved to `recipes/cohosting.md` and re-grounded.** The README's cohosting section shrinks to the pitch, the code sample, and a pointer; the recipe carries the rules, an honest account of what is verified @@ -34,6 +35,13 @@ While pre-1.0, the public API may change between 0.x releases. ### Fixed +- **Rejection `code` now survives tx-dedup replay (#21).** The dedup record + persisted only the rejection message, so a client retrying the same `txId` + got the reason with no machine-readable `code` — breaking code-based error + handling on exactly the retry path it exists for. `_sync_seen_tx` gains an + `error_code` column (added in place on wake for already-deployed DOs), and + the replayed `rejected` frame is now shaped identically to the original: + `{ code, message }` when a code was recorded, `{ message }` otherwise. - **BLOB columns no longer corrupt to `{}` over the wire (ADR-0017, [#27](https://github.com/grrowl/tanstack-durable-object-sync/issues/27)).** workerd's `SqlStorage` returns BLOB values as bare `ArrayBuffer`, which the diff --git a/docs/adr/0016-reconnect-policy.md b/docs/adr/0016-reconnect-policy.md new file mode 100644 index 0000000..3d40202 --- /dev/null +++ b/docs/adr/0016-reconnect-policy.md @@ -0,0 +1,138 @@ +# 0016 — Transport reconnect policy: injectable delay function, terminal 4xxx, jittered backoff + +**Status:** Accepted. Fixes issues #25 and #26. + +## Context + +`WebSocketTransport` auto-reconnects from two sites — the socket close handler +and the connect-failure catch — and both were blind to *why* the connection +went away: a fixed `reconnectDelayMs` (default 250 ms) rescheduled forever, +never inspecting `CloseEvent.code`. Two real failures fall out: + +- **Terminal rejections retry forever (#25).** The DO's only client-readable + auth-rejection pattern is accept-then-close with an application code (e.g. + 4403) — a failed HTTP handshake status is invisible to a browser WebSocket. + The transport turned exactly that deliberate rejection into an infinite + 250 ms retry loop against the DO, with no way for the app to even observe + the code. +- **Fixed-interval thundering herd (#26).** During a DO outage every client + retries on the same fixed interval indefinitely; when the DO returns, they + all arrive in lockstep. The option's own doc comment admitted it: + "production should layer exponential backoff + jitter." + +## Decision + +### 1. The policy is a function, not a bag of flags + +One option subsumes the whole space: + +```ts +reconnectDelay?: number | ((attempt: number, closeCode?: number, closeReason?: string) => number | null) +``` + +The function form is called once per attempt with a 1-based counter; it +returns the delay in ms before the next attempt, or `null` to stop +reconnecting. The alternative — config knobs (`maxRetries`, `backoffFactor`, +`capMs`, `terminalCodes`, …) — grows a flag per policy nuance and still can't +express app-specific shapes ("retry 4408 rate-limit closes after the reason's +hinted delay"). A function is the minimal complete surface; the knobs become a +*default implementation* of it. The number form is the one sanctioned +shorthand: the default policy's base delay. It **replaces** `reconnectDelayMs` +(removed — pre-1.0, one option per concern; a fixed delay is spelled +`() => ms`). + +`closeCode` stays `number` and `closeReason` stays `string` deliberately: the +4xxx range is *private-use* vocabulary that each application defines (this +library emits no close codes of its own), the reason is app-authored prose, +and a closed union at a network boundary is a promise the wire doesn't keep. +The closed set in this signature is the *return* — `number | null`, the +policy's actual decision. + +Both reconnect sites route through one `scheduleReconnect(code?, reason?)`. +The connect-failure path has no close frame, so the policy (and `onClosed`) +see `undefined` there — honest, and distinguishable from a server-sent close. + +### 2. Default: application closes (4000-4999) are terminal + +The 4xxx range is reserved for application meaning: the server closed *on +purpose* and said why. Retrying a deliberate rejection cannot succeed and is +indistinguishable from an attack pattern from the DO's perspective. The +default policy returns `null` for any 4000-4999 code; everything else — +1000/1001 (hibernation, redeploy), 1006 (network drop), or a failed open — +stays transient and retries. Apps that use a 4xxx code as "please reconnect +later" opt out with a custom policy. + +A terminal stop must be observable, or an auth rejection looks like a silent +hang. New hook: + +```ts +onClosed?: (code: number | undefined, reason: string | undefined) => void +``` + +Called exactly when the policy returns `null` — never for an intentional +`close()`, never while retries continue. This replaces the previous workaround +(attaching a close listener inside a custom `open()` and calling +`transport.close()` before the transport's own handler ran — a +listener-ordering hack). + +### 3. Default delay: capped exponential backoff, full jitter, reset on open + +`defaultReconnectDelay(baseMs, capMs = 30_000)` returns a uniform delay in +`[0, min(cap, base·2^(attempt−1))]` — AWS-style *full* jitter, which +desynchronizes a fleet of clients that all lost the same DO at the same +moment. The cap never drops below `baseMs`, so a caller who set +`reconnectDelay: 60_000` is not silently truncated. The attempt counter +resets on a successful *open* (the demand-driven reconnect window makes +"first server frame" needlessly stricter — an open that immediately drops +re-enters backoff on the very next close anyway, one attempt later). + +### Invariants preserved + +- **`reconnecting` is set at SCHEDULING time**, not in the timer — the + demand-driven connect window (a mutation racing the timer) must run the + resubscribe path (pre-existing bug, ADR-0011 grill). `scheduleReconnect` + sets it before consulting the policy; a *terminal* close also sets it, so an + app-driven `connect()` after re-auth still resubscribes from the cursor. +- **No idle timers.** At most one reconnect timer exists, and only while a + retry is pending: the handle is tracked and cancelled on a successful open, + on a terminal stop, on `close()`, and when a newer drop supersedes it. + Without this (adversarial-review catch), a stale timer from a transient + drop could fire after a later terminal 4xxx close and resurrect the + connection — retrying past "stop" and double-firing `onClosed`. + +## Alternatives considered + +- **Flag-based config** (`maxRetries`/`backoffFactor`/`terminalCodes`). Rejected + above — a function is smaller and complete. +- **Rejecting `connect()` for a post-handshake 4xxx close.** The close arrives + *after* `open()` resolved, so connect() has already succeeded by the time + the code is known; faking a rejection would need a "wait for first frame" + handshake phase. `onClosed` reports the same fact without restructuring the + connect path. +- **Terminal = give up on any close code the app didn't allowlist.** Inverts + the safe default: transient network drops (1006) are the overwhelmingly + common case and must retry out of the box. + +## Consequences + +- **Behavior change:** a 4xxx server close no longer auto-reconnects. Code + that relied on retry-after-4xxx must pass a custom `reconnectDelay`. +- **Breaking (pre-1.0):** `reconnectDelayMs: n` becomes `reconnectDelay: n` + (same meaning: the default policy's base). +- Retry delays are now randomized and growing; tests that need a fast + deterministic retry inject `reconnectDelay: () => smallMs` (the migrated + `reconnectDelay: 20` fixtures stay valid — jitter only shortens them; a + fixture that needs the timer to NEVER fire uses a fixed policy, since a + jittered 60 s base can land near 0). +- The `open()`-listener workaround for observing close codes is obsolete; + `onClosed` is the supported surface. +- **Future unification (deliberate deferral).** The three race guards that + adversarial review accreted — the tracked reconnect timer, the + stale-socket close-event guard (`this.ws !== ws`), and the close-epoch + check after `await open()` — could collapse into one generation counter: + *a continuation is only valid in the generation it was created in* (bump on + close / install / schedule; check at each continuation head). That demotes + a forgotten cleanup from "resurrected socket" to "one late no-op". Not done + here: the current shape is pinned by the race tests and has passed two + adversarial reviews; unify when this file is next touched, with those same + tests as the harness. diff --git a/docs/adr/README.md b/docs/adr/README.md index 2660e95..c1dd131 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -22,4 +22,5 @@ explains the displacement. | [0013](./0013-predicate-floor-one-evaluator.md) | Filtered-subscription membership: one evaluator is the source of truth; the floor is the verified-agreeing set | Accepted | | [0014](./0014-object-sync-schema.md) | `defineSync`: one schema value, mutations on the collection, commands on the connection | Accepted (supersedes 0001 D11 builder; closes 0010 manifest) | | [0015](./0015-syncable-mixin.md) | `Syncable` mixin: the sync core as a mixin over any DO base | Accepted (reframes 0001 D13; Actor unsupported) | +| [0016](./0016-reconnect-policy.md) | Transport reconnect policy: injectable delay function, terminal 4xxx, jittered backoff | Accepted | | [0017](./0017-blob-wire-normalization.md) | BLOB wire normalization: bare ArrayBuffer becomes Uint8Array at emission | Accepted | diff --git a/src/client/index.ts b/src/client/index.ts index f8b004c..c23ac67 100644 --- a/src/client/index.ts +++ b/src/client/index.ts @@ -9,9 +9,10 @@ // server-filtered by a `where` predicate. export { + defaultReconnectDelay, MutationRejectedError, WebSocketTransport, } from "./transport.ts" -export type { SubHandler, TransportOptions, WebSocketLike } from "./transport.ts" +export type { ReconnectDelayFn, SubHandler, TransportOptions, WebSocketLike } from "./transport.ts" export { doCollectionOptions, WriteOutsideSubError } from "./do-collection.ts" export type { CollectionName, DoApiCollectionOptions, RowOf } from "./do-collection.ts" diff --git a/src/client/transport.ts b/src/client/transport.ts index 1d3b693..6f37468 100644 --- a/src/client/transport.ts +++ b/src/client/transport.ts @@ -45,6 +45,27 @@ export class MutationRejectedError extends Error { } } +/** Reconnect delay policy (ADR-0016). Called once per reconnect attempt with + * the 1-based attempt number (reset to 1 after each successful open) and, + * when the drop came from a socket close, that close's code/reason (a failed + * `open()` has no close frame, so both are undefined). Return the delay in ms + * before the next attempt, or `null` to stop reconnecting (terminal — the + * transport surfaces it via `onClosed`). */ +export type ReconnectDelayFn = (attempt: number, closeCode?: number, closeReason?: string) => number | null + +/** The default reconnect policy: capped exponential backoff with full jitter — + * a uniform delay in [0, min(cap, base·2^(attempt−1))], cap 30 s (never below + * the base) — and application close codes (4000-4999) are terminal: the + * server closed deliberately (e.g. an accept-then-close 4403 auth rejection), + * so retrying cannot succeed. */ +export function defaultReconnectDelay(baseMs: number, capMs = 30_000): ReconnectDelayFn { + const cap = Math.max(capMs, baseMs) + return (attempt, closeCode) => { + if (closeCode !== undefined && closeCode >= 4000 && closeCode <= 4999) return null + return Math.random() * Math.min(cap, baseMs * 2 ** (attempt - 1)) + } +} + export interface TransportOptions { url: string /** Returns a CONNECTED socket. Default opens `new WebSocket(url)` and resolves @@ -53,9 +74,17 @@ export interface TransportOptions { codec?: FrameCodec /** Confirmation/await timeout in ms. */ timeoutMs?: number - /** Delay before an auto-reconnect attempt after an unexpected drop (ms). - * Fixed for now; production should layer exponential backoff + jitter. */ - reconnectDelayMs?: number + /** Reconnect pacing. A number is the base delay (ms) for the default + * jittered-backoff policy (`defaultReconnectDelay`) — the attempt-1 jitter + * ceiling. A function is the full policy; return `null` to stop + * reconnecting. Default: `defaultReconnectDelay(250)`. A truly fixed delay + * is a policy too: `() => 500`. */ + reconnectDelay?: number | ReconnectDelayFn + /** Called when an unexpected drop is TERMINAL — the policy returned `null` + * (default: any 4000-4999 application close, e.g. an auth rejection) — so + * the app can tell "re-auth needed" from a transient blip the transport is + * still retrying. Never called for an intentional `close()`. */ + onClosed?: (code: number | undefined, reason: string | undefined) => void } interface SeqWaiter { @@ -116,12 +145,30 @@ export class WebSocketTransport { private intentionallyClosed = false /** True while reconnecting, so connect() resubscribes on success. */ private reconnecting = false - private readonly reconnectDelayMs: number + private readonly reconnectDelay: ReconnectDelayFn + private readonly onClosed?: (code: number | undefined, reason: string | undefined) => void + /** Consecutive reconnect attempts since the last successful open; the + * 1-based value passed to the policy. */ + private reconnectAttempt = 0 + /** The one pending reconnect timer, so a successful open, a terminal stop, + * or close() can cancel it — a stale timer from an earlier transient drop + * must never fire a further attempt after any of those. */ + private reconnectTimer: ReturnType | null = null + /** Bumped by close(). A connect() body captures it before awaiting open(); + * a mismatch after the await means close() ran mid-flight — the resolved + * socket must be discarded, not installed. (close() can cancel the pending + * timer, but not a connect() body already parked on a slow open() — without + * this, that body resurrects a live, resubscribed socket after teardown. + * An epoch rather than checking intentionallyClosed, because an explicit + * connect() AFTER close() is allowed and must still install its socket.) */ + private closeEpoch = 0 constructor(opts: TransportOptions) { this.codec = opts.codec ?? createFrameCodec() this.timeoutMs = opts.timeoutMs ?? 5000 - this.reconnectDelayMs = opts.reconnectDelayMs ?? 250 + this.reconnectDelay = + typeof opts.reconnectDelay === "function" ? opts.reconnectDelay : defaultReconnectDelay(opts.reconnectDelay ?? 250) + this.onClosed = opts.onClosed this.open = opts.open ?? (() => @@ -141,7 +188,18 @@ export class WebSocketTransport { if (this.ws) return if (this.connectPromise) return this.connectPromise this.connectPromise = (async () => { + const epoch = this.closeEpoch const ws = await this.open() + if (epoch !== this.closeEpoch) { + // close() ran while open() was in flight: the transport is torn down. + // Discard the orphan instead of installing it. + try { + ws.close() + } catch { + /* ignore */ + } + return + } // Browsers default WebSocket.binaryType to "blob"; force "arraybuffer" so // binary frames arrive as ArrayBuffer (workerd already does). Without this // the codec can't decode and every server frame is silently dropped. @@ -151,26 +209,25 @@ export class WebSocketTransport { /* some socket impls don't expose binaryType; codec handles AB/Uint8Array */ } ws.addEventListener("message", (ev) => this.onMessage(ev.data)) - ws.addEventListener("close", () => { + ws.addEventListener("close", (ev) => { + // Only the CURRENT socket's close may detach/reconnect. A stale + // socket's late close (delivered after close()+connect() installed a + // fresh socket) must not null the live connection (codex review). + if (this.ws !== ws) return this.ws = null this.connectPromise = null // Auto-reconnect on an unexpected drop while subscriptions are active. if (!this.intentionallyClosed && this.handlers.size > 0) { - // The flag is set at SCHEDULING time, not in the timer: a demand- - // driven connect() (a mutation inside the reconnect window) may - // establish the fresh socket first, and it must run the resubscribe - // path too — or every subscription is silently dead on the new - // socket and the late timer wedges the flag (pre-existing bug, found - // in the ADR-0011 grill). - this.reconnecting = true - setTimeout(() => { - void this.connect().catch(() => { - /* next attempt retries on the following close */ - }) - }, this.reconnectDelayMs) + const { code, reason } = ev as { code?: number; reason?: string } + this.scheduleReconnect(code, reason) } }) this.ws = ws + // A successful open resets the backoff: a later blip starts from the + // policy's first-attempt delay again, not the accumulated one. It also + // supersedes any pending timer (a demand-driven connect beat it). + this.reconnectAttempt = 0 + this.clearReconnectTimer() // On a reconnect, re-establish every subscription from our single applied // cursor so the server serves a windowed catch-up rather than a snapshot. if (this.reconnecting) { @@ -185,17 +242,49 @@ export class WebSocketTransport { this.connectPromise.catch(() => { this.connectPromise = null if (!this.intentionallyClosed && this.handlers.size > 0) { - this.reconnecting = true - setTimeout(() => { - void this.connect().catch(() => { - /* next attempt retries on the following close */ - }) - }, this.reconnectDelayMs) + // A socket that never opened has no close frame — the policy sees an + // undefined code (backoff, under the default). + this.scheduleReconnect() } }) return this.connectPromise } + /** Consult the policy and either arm the next reconnect attempt or stop. */ + private scheduleReconnect(closeCode?: number, closeReason?: string): void { + // The flag is set at SCHEDULING time, not in the timer: a demand-driven + // connect() (a mutation inside the reconnect window) may establish the + // fresh socket first, and it must run the resubscribe path too — or every + // subscription is silently dead on the new socket and the late timer + // wedges the flag (pre-existing bug, found in the ADR-0011 grill). It is + // also set on a TERMINAL close, so an app-driven connect() after e.g. + // re-auth still resubscribes from the cursor. + this.reconnecting = true + // At most one pending attempt: a newer drop supersedes an older timer. + this.clearReconnectTimer() + const delay = this.reconnectDelay(++this.reconnectAttempt, closeCode, closeReason) + if (delay === null) { + // Terminal (default: application close 4000-4999, e.g. an + // accept-then-close auth rejection): retrying cannot help — surface the + // close to the app instead of looping against the DO. + this.onClosed?.(closeCode, closeReason) + return + } + this.reconnectTimer = setTimeout(() => { + this.reconnectTimer = null + void this.connect().catch(() => { + /* next attempt retries via the connect-failure path above */ + }) + }, delay) + } + + private clearReconnectTimer(): void { + if (this.reconnectTimer !== null) { + clearTimeout(this.reconnectTimer) + this.reconnectTimer = null + } + } + /** Re-send a `sub` for every registered subscription, carrying `since`. */ private resubscribeAll(): void { const since = this.appliedCursor @@ -214,6 +303,8 @@ export class WebSocketTransport { close(): void { this.intentionallyClosed = true + this.closeEpoch++ + this.clearReconnectTimer() for (const w of this.seqWaiters.splice(0)) { clearTimeout(w.timer) w.reject(new Error("transport closed")) diff --git a/tests/auth-path.test.ts b/tests/auth-path.test.ts new file mode 100644 index 0000000..ae4fbd6 --- /dev/null +++ b/tests/auth-path.test.ts @@ -0,0 +1,216 @@ +// Auth on the sync upgrade path — where each kind of rejection surfaces. +// +// WHY: auth has exactly two client-visible surfaces, and confusing them breaks +// real apps. (1) `parseAttachment` runs on the HTTP upgrade: a thrown Response +// is returned verbatim, any other throw is HTTP 401 — a browser WebSocket +// cannot read those statuses, so this surface is for trusted-header setups +// behind a Worker. (2) Client-readable rejection is accept-then-close with an +// application code (4403): a subclass fetch override can do async authz BEFORE +// super.fetch without touching sync socket bookkeeping, and the transport must +// treat that close as TERMINAL (no retry loop — issue #25, ADR-0016). Also +// pins that a bare SyncDurableObject claims the upgrade on ANY path, so +// Workers need no URL rewrite. + +import { env, runInDurableObject, SELF } from "cloudflare:test" +import { describe, expect, it } from "vitest" +import { WebSocketTransport, type SubHandler, type WebSocketLike } from "../src/client/transport.ts" +import { createFrameCodec } from "../src/wire/frame-codec.ts" +import type { ClientFrame, ServerFrame } from "../src/wire/frames.ts" + +const codec = createFrameCodec() + +async function upgrade(path: string, headers: Record = {}): Promise { + return SELF.fetch(`https://example.com${path}`, { + headers: { Upgrade: "websocket", ...headers }, + }) +} + +async function openWs(path: string, headers: Record = {}): Promise { + const res = await upgrade(path, headers) + expect(res.status).toBe(101) + const ws = res.webSocket + if (!ws) throw new Error("no webSocket on 101 response") + ws.accept() + return ws +} + +function collectUntil(ws: WebSocket, done: (f: ServerFrame) => boolean, timeoutMs = 2000): Promise> { + return new Promise((resolve, reject) => { + const out: Array = [] + const timer = setTimeout(() => reject(new Error(`timeout; got [${out.map((f) => f.t).join(",")}]`)), timeoutMs) + const onMsg = (e: MessageEvent): void => { + const f = codec.decode(e.data as ArrayBuffer) as ServerFrame + out.push(f) + if (done(f)) { + clearTimeout(timer) + ws.removeEventListener("message", onMsg) + resolve(out) + } + } + ws.addEventListener("message", onMsg) + }) +} + +async function waitFor(pred: () => boolean, timeoutMs = 3000): Promise { + const start = Date.now() + while (!pred()) { + if (Date.now() - start > timeoutMs) throw new Error("waitFor timeout") + await new Promise((r) => setTimeout(r, 5)) + } +} + +/** Installs a fetch override in the same dispatch position a subclass's + * `async fetch()` would occupy (own property beats the mixin prototype): + * async authz that rejects non-alice upgrades with accept-then-close(4403) + * BEFORE super.fetch — the rejected socket never reaches ctx.acceptWebSocket. */ +function installAuthzOverride(room: string): Promise { + const stub = env.SYNC_DO.get(env.SYNC_DO.idFromName(room)) + return runInDurableObject(stub, (instance) => { + const inst = instance as unknown as { fetch: (req: Request) => Promise } + const superFetch = inst.fetch.bind(instance) + inst.fetch = async (req: Request): Promise => { + if (req.headers.get("Upgrade") === "websocket") { + await new Promise((r) => setTimeout(r, 5)) // genuine async authz hop + if (req.headers.get("x-user") !== "alice") { + const pair = new WebSocketPair() + pair[1].accept() // plain accept — NOT ctx.acceptWebSocket, no SYNC_TAG + pair[1].close(4403, "org access denied") + return new Response(null, { status: 101, webSocket: pair[0] }) + } + } + return superFetch(req) + } + }) +} + +describe("auth path: upgrade statuses and application close codes", () => { + it("bare SyncDurableObject accepts the upgrade on ANY path — no URL rewrite needed", async () => { + // The Worker forwards the ORIGINAL request; the DO sees /sync/sessions/abc123 + // (nothing like /_sync) and still upgrades + syncs. + const ws = await openWs("/sync/sessions/abc123", { "x-user": "alice" }) + ws.send(codec.encode({ t: "sub", subId: "s1", collection: "messages" } satisfies ClientFrame)) + const frames = await collectUntil(ws, (f) => f.t === "snap-end") + expect(frames.at(-1)!.t).toBe("snap-end") + ws.close() + }) + + it("parseAttachment throw -> HTTP status on the upgrade response, not a WS close code", async () => { + const room = "authpath-parse-throw" + // Prime the instance, then swap the auth hook via the public sync facade — + // exactly what a subclass's parseAttachment override feeds through. + const stub = env.SYNC_DO.get(env.SYNC_DO.idFromName(room)) + await runInDurableObject(stub, (instance) => { + ;(instance as unknown as { sync: { configure: (o: object) => void } }).sync.configure({ + parseAttachment: (req: Request) => { + const u = req.headers.get("x-user") + if (u === "response-throw") throw new Response("forbidden", { status: 403 }) + if (u === "error-throw") throw new Error("nope") + return { userId: u ?? "anon" } + }, + }) + }) + + const resp403 = await upgrade(`/sync/${room}`, { "x-user": "response-throw" }) + expect(resp403.status).toBe(403) + expect(resp403.webSocket).toBeNull() + + const resp401 = await upgrade(`/sync/${room}`, { "x-user": "error-throw" }) + expect(resp401.status).toBe(401) + expect(await resp401.text()).toBe("unauthorized") + expect(resp401.webSocket).toBeNull() + + // parseAttachment is upgrade-only: a plain GET never reaches it (426 first). + const plain = await SELF.fetch(`https://example.com/sync/${room}`, { + headers: { "x-user": "response-throw" }, + }) + expect(plain.status).toBe(426) + }) + + it("accept-then-close(4403) before super.fetch: client sees 4403; later authorized connect syncs", async () => { + const room = "authpath-4403" + const stub = env.SYNC_DO.get(env.SYNC_DO.idFromName(room)) + await installAuthzOverride(room) + + // Unauthorized connect: handshake succeeds, then the app close code arrives. + const badRes = await upgrade(`/sync/${room}`, { "x-user": "intruder" }) + expect(badRes.status).toBe(101) + const badWs = badRes.webSocket! + const closed = new Promise<{ code: number; reason: string }>((resolve, reject) => { + const timer = setTimeout(() => reject(new Error("no close event")), 2000) + badWs.addEventListener("close", (e: CloseEvent) => { + clearTimeout(timer) + resolve({ code: e.code, reason: e.reason }) + }) + }) + badWs.accept() + const closeEvt = await closed + expect(closeEvt.code).toBe(4403) + expect(closeEvt.reason).toBe("org access denied") + + // Sync socket bookkeeping untouched: no hibernatable socket was registered. + await runInDurableObject(stub, (_i, state) => { + expect(state.getWebSockets().length).toBe(0) + }) + + // Authorized connect on the SAME instance still syncs end to end. + const ws = await openWs(`/sync/${room}`, { "x-user": "alice" }) + ws.send(codec.encode({ t: "sub", subId: "s1", collection: "messages" } satisfies ClientFrame)) + await collectUntil(ws, (f) => f.t === "snap-end") + + const mut: ClientFrame = { + t: "mut", + txId: "tx-authpath-1", + collection: "messages", + ops: [{ type: "insert", key: "m1", cols: { id: "m1", body: "hello" } }], + } + ws.send(codec.encode(mut)) + const frames = await collectUntil(ws, (f) => f.t === "committed") + expect(frames.some((f) => f.t === "d" && f.op === "insert" && f.key === "m1")).toBe(true) + + // Exactly one hibernatable (tagged) sync socket exists now. + await runInDurableObject(stub, (_i, state) => { + expect(state.getWebSockets().length).toBe(1) + expect(state.getWebSockets()[0]!.deserializeAttachment()).toEqual({ userId: "alice" }) + }) + ws.close() + }) + + it("the transport treats the 4403 rejection as terminal: no retry loop, onClosed surfaces it", async () => { + const room = "authpath-4403-transport" + await installAuthzOverride(room) + + let opens = 0 + const closed: Array<[number | undefined, string | undefined]> = [] + const t = new WebSocketTransport({ + url: `https://example.com/sync/${room}`, + reconnectDelay: 5, + onClosed: (code, reason) => closed.push([code, reason]), + open: async () => { + opens++ + const res = await upgrade(`/sync/${room}`, { "x-user": "intruder" }) + const ws = res.webSocket + if (!ws) throw new Error("no webSocket") + ws.accept() + return ws as unknown as WebSocketLike + }, + }) + const handler: SubHandler = { + onSnap: () => {}, + onSnapEnd: () => {}, + onDelta: () => {}, + onUptodate: () => {}, + onReset: () => {}, + } + // The server closes immediately after accepting, so the sub frame may race + // the close — a send on the dead socket rejecting is fine here. + await t.subscribe("s1", "messages", handler).catch(() => {}) + + await waitFor(() => closed.length === 1) + // The app learns WHY (auth), instead of the transport looping forever + // against the DO on a rejection that can never succeed. + expect(closed[0]).toEqual([4403, "org access denied"]) + + await new Promise((r) => setTimeout(r, 150)) + expect(opens).toBe(1) + }) +}) diff --git a/tests/reconnect-policy.test.ts b/tests/reconnect-policy.test.ts new file mode 100644 index 0000000..6f041c4 --- /dev/null +++ b/tests/reconnect-policy.test.ts @@ -0,0 +1,390 @@ +import { env, runInDurableObject, SELF } from "cloudflare:test" +import { afterEach, describe, expect, it, vi } from "vitest" +import { + defaultReconnectDelay, + type SubHandler, + WebSocketTransport, + type WebSocketLike, +} from "../src/client/transport.ts" + +// WHY (ADR-0016, issues #25/#26): the DO's client-readable auth-rejection +// pattern is accept-then-close with an application code (4xxx) — a failed HTTP +// handshake status is invisible to a browser WebSocket. A transport that +// blindly reconnects on ANY close turns that rejection into an infinite retry +// loop against the DO, and a fixed retry interval turns a DO outage into a +// thundering herd. So the reconnect delay is a POLICY function: the default +// treats 4000-4999 as terminal (surfaced via onClosed) and otherwise backs off +// exponentially with full jitter, resetting the attempt counter on a +// successful open. + +function noopHandler(): SubHandler { + return { onSnap: () => {}, onSnapEnd: () => {}, onDelta: () => {}, onUptodate: () => {}, onReset: () => {} } +} + +async function waitFor(pred: () => boolean, timeoutMs = 3000): Promise { + const start = Date.now() + while (!pred()) { + if (Date.now() - start > timeoutMs) throw new Error("waitFor timeout") + await new Promise((r) => setTimeout(r, 5)) + } +} + +async function openSocket(room: string): Promise { + const res = await SELF.fetch(`https://example.com/sync/${room}`, { headers: { Upgrade: "websocket" } }) + const ws = res.webSocket + if (!ws) throw new Error("no webSocket") + ws.accept() + return ws as unknown as WebSocketLike +} + +function serverDrop(room: string, code: number, reason: string): Promise { + return runInDurableObject(env.SYNC_DO.get(env.SYNC_DO.idFromName(room)), (_i, state) => { + for (const sock of state.getWebSockets()) sock.close(code, reason) + }) +} + +describe("defaultReconnectDelay policy", () => { + afterEach(() => { + vi.restoreAllMocks() + }) + + it("application close codes 4000-4999 are terminal (null); everything else retries", () => { + const policy = defaultReconnectDelay(250) + expect(policy(1, 4000)).toBeNull() + expect(policy(1, 4403, "org access denied")).toBeNull() + expect(policy(5, 4999)).toBeNull() + expect(policy(1, 1000)).not.toBeNull() // normal closure: transient (hibernation, redeploy) + expect(policy(1, 1006)).not.toBeNull() // abnormal drop: transient + expect(policy(1)).not.toBeNull() // connect failure: no close frame at all + }) + + it("backs off exponentially from the base and caps at 30s", () => { + vi.spyOn(Math, "random").mockReturnValue(1) // pin jitter at the ceiling + const policy = defaultReconnectDelay(250) + expect(policy(1)).toBe(250) + expect(policy(2)).toBe(500) + expect(policy(3)).toBe(1000) + expect(policy(8)).toBe(30_000) // 250·2^7 = 32_000 → capped + expect(policy(1000)).toBe(30_000) // huge attempt counts must not overflow past the cap + }) + + it("full jitter: the delay is uniform in [0, ceiling] — attempts desynchronize", () => { + vi.spyOn(Math, "random").mockReturnValue(0) + expect(defaultReconnectDelay(250)(3)).toBe(0) + vi.spyOn(Math, "random").mockReturnValue(0.5) + expect(defaultReconnectDelay(250)(3)).toBe(500) + }) + + it("a base above the default cap is not truncated below itself", () => { + vi.spyOn(Math, "random").mockReturnValue(1) + expect(defaultReconnectDelay(60_000)(1)).toBe(60_000) + }) +}) + +describe("transport reconnect policy (ADR-0016)", () => { + it("a 4xxx application close is terminal: no reconnect; onClosed surfaces code+reason", async () => { + const room = "rcpol-4403-terminal" + let opens = 0 + const closed: Array<[number | undefined, string | undefined]> = [] + const t = new WebSocketTransport({ + url: `https://example.com/sync/${room}`, + reconnectDelay: 5, + onClosed: (code, reason) => closed.push([code, reason]), + open: async () => { + opens++ + return openSocket(room) + }, + }) + await t.subscribe("s1", "messages", noopHandler()) + expect(opens).toBe(1) + + await serverDrop(room, 4403, "org access denied") + await waitFor(() => closed.length === 1) + // The app can distinguish this auth rejection from a transient drop. + expect(closed[0]).toEqual([4403, "org access denied"]) + + // Ample time for a (wrongly) scheduled reconnect to have fired. + await new Promise((r) => setTimeout(r, 150)) + expect(opens).toBe(1) + }) + + it("a normal-closure drop (1000) still auto-reconnects; onClosed stays silent", async () => { + const room = "rcpol-1000-transient" + let opens = 0 + const closed: Array<[number | undefined, string | undefined]> = [] + const t = new WebSocketTransport({ + url: `https://example.com/sync/${room}`, + reconnectDelay: 5, + onClosed: (code, reason) => closed.push([code, reason]), + open: async () => { + opens++ + return openSocket(room) + }, + }) + await t.subscribe("s1", "messages", noopHandler()) + await serverDrop(room, 1000, "drop") + await waitFor(() => opens >= 2) + expect(closed.length).toBe(0) + t.close() + }) + + it("intentional close() is permanent: connect() works again but drops no longer auto-reconnect", async () => { + const room = "rcpol-close-permanent" + let opens = 0 + const t = new WebSocketTransport({ + url: `https://example.com/sync/${room}`, + reconnectDelay: 5, + open: async () => { + opens++ + return openSocket(room) + }, + }) + await t.subscribe("s1", "messages", noopHandler()) + expect(opens).toBe(1) + t.close() + + // connect() after close(): a new socket IS opened (close() nulled ws), but + // intentionallyClosed stays true… + await t.connect() + expect(opens).toBe(2) + // …so a subsequent unexpected drop performs NO auto-reconnect on this instance. + await serverDrop(room, 1000, "drop") + await new Promise((r) => setTimeout(r, 150)) + expect(opens).toBe(2) + }) +}) + +// --- Fake-socket tests: deterministic attempt-counter semantics ------------ + +interface Fake { + ws: WebSocketLike + emit: (type: string, ev: object) => void +} + +function makeFake(): Fake { + const listeners = new Map void>>() + return { + emit: (type, ev) => { + for (const l of listeners.get(type) ?? []) l(ev) + }, + ws: { + send: () => {}, + close: () => {}, + addEventListener: (type, l) => { + const arr = listeners.get(type) ?? [] + arr.push(l as (ev: object) => void) + listeners.set(type, arr) + }, + removeEventListener: () => {}, + }, + } +} + +describe("reconnect attempt counter (drives the backoff)", () => { + it("grows across consecutive failed opens and resets after a successful open", async () => { + const attempts: Array = [] + const fakes: Array = [] + let failNext = 0 + const t = new WebSocketTransport({ + url: "wss://fake-backoff", + // Deterministic policy: record the attempt number the transport passes, + // retry almost immediately so the test never sleeps for real backoff. + reconnectDelay: (attempt) => { + attempts.push(attempt) + return 1 + }, + open: async () => { + if (failNext > 0) { + failNext-- + throw new Error("server unreachable") + } + const f = makeFake() + fakes.push(f) + return f.ws + }, + }) + await t.subscribe("s1", "messages", noopHandler()) + expect(fakes.length).toBe(1) + + // Outage: the next two open() attempts fail, the third succeeds. The + // policy must see a GROWING attempt number across the failures — this is + // what makes the default backoff exponential rather than fixed-interval. + failNext = 2 + fakes[0]!.emit("close", { code: 1006 }) + await waitFor(() => fakes.length === 2) + expect(attempts).toEqual([1, 2, 3]) + + // A drop after the successful open starts over at attempt 1 — otherwise a + // long-lived client would pay maximum backoff for every later blip. + fakes[1]!.emit("close", { code: 1006 }) + await waitFor(() => fakes.length === 3) + expect(attempts).toEqual([1, 2, 3, 1]) + t.close() + }) + + it("a custom policy returning null stops retrying; onClosed carries no code for a connect failure", async () => { + const closed: Array<[number | undefined, string | undefined]> = [] + const fakes: Array = [] + let opens = 0 + let failOpens = false + const t = new WebSocketTransport({ + url: "wss://fake-stop", + reconnectDelay: (attempt) => (attempt >= 2 ? null : 1), + onClosed: (code, reason) => closed.push([code, reason]), + open: async () => { + opens++ + if (failOpens) throw new Error("server unreachable") + const f = makeFake() + fakes.push(f) + return f.ws + }, + }) + await t.subscribe("s1", "messages", noopHandler()) + expect(opens).toBe(1) + + // Drop, then the reconnect attempt fails (attempt 1 → retry), and the + // policy gives up on attempt 2. The connect-failure path has no close + // frame, so onClosed reports (undefined, undefined) — distinguishable + // from a server-sent application close. + failOpens = true + fakes[0]!.emit("close", { code: 1006, reason: "abnormal" }) + await waitFor(() => closed.length === 1) + expect(closed[0]).toEqual([undefined, undefined]) + + const opensAtStop = opens + await new Promise((r) => setTimeout(r, 100)) + expect(opens).toBe(opensAtStop) // no further attempts after the policy said stop + t.close() + }) + + it("a stale transient-drop timer cannot resurrect the transport after a terminal close", async () => { + // Codex-review scenario: transient drop arms a delayed retry; a + // demand-driven connect() beats the timer, and THAT socket is closed + // terminally (4403). "Terminal means stop" must win — the earlier timer + // must not fire a further attempt (or a duplicate onClosed) later. + const closed: Array<[number | undefined, string | undefined]> = [] + const fakes: Array = [] + let opens = 0 + const t = new WebSocketTransport({ + url: "wss://fake-stale-timer", + reconnectDelay: (_attempt, code) => (code !== undefined && code >= 4000 && code <= 4999 ? null : 50), + onClosed: (code, reason) => closed.push([code, reason]), + open: async () => { + opens++ + const f = makeFake() + fakes.push(f) + return f.ws + }, + }) + await t.subscribe("s1", "messages", noopHandler()) + + fakes[0]!.emit("close", { code: 1006 }) // transient: retry timer armed for +50ms + await t.connect() // demand-driven connect wins the race + expect(opens).toBe(2) + fakes[1]!.emit("close", { code: 4403, reason: "forbidden" }) // terminal + await waitFor(() => closed.length === 1) + + await new Promise((r) => setTimeout(r, 150)) // well past the stale timer + expect(opens).toBe(2) // the stale timer did not reconnect… + expect(closed).toEqual([[4403, "forbidden"]]) // …and onClosed fired exactly once + t.close() + }) + + it("close() during the reconnect window cancels the pending retry", async () => { + const fakes: Array = [] + let opens = 0 + const t = new WebSocketTransport({ + url: "wss://fake-close-cancels", + reconnectDelay: () => 30, + open: async () => { + opens++ + const f = makeFake() + fakes.push(f) + return f.ws + }, + }) + await t.subscribe("s1", "messages", noopHandler()) + fakes[0]!.emit("close", { code: 1006 }) // retry armed for +30ms + t.close() // the app shut the transport down before the timer fired + + await new Promise((r) => setTimeout(r, 100)) + expect(opens).toBe(1) // an intentional close() leaves nothing running + }) + + it("a stale socket's late close event cannot detach the live socket", async () => { + // close() then an immediate connect(): the OLD socket's close event can be + // delivered after the new socket is installed. It must be ignored — + // otherwise it nulls the live connection and the next demand-driven + // connect() opens a needless third socket. + const fakes: Array = [] + let opens = 0 + const t = new WebSocketTransport({ + url: "wss://fake-stale-close", + reconnectDelay: () => 1, + open: async () => { + opens++ + const f = makeFake() + fakes.push(f) + return f.ws + }, + }) + await t.subscribe("s1", "messages", noopHandler()) + t.close() // old socket told to close; its event is not delivered yet + await t.connect() // fresh socket on the same instance + expect(opens).toBe(2) + + fakes[0]!.emit("close", { code: 1005 }) // the old socket's close finally lands + await t.connect() // must be a no-op: the live socket is still attached + expect(opens).toBe(2) + t.close() + }) + + it("close() during an in-flight reconnect open() cannot resurrect a socket after teardown", async () => { + // Post-PR codex adversarial pass: a transient drop arms the retry timer; + // the timer fires and open() is SLOW (TLS / Worker cold start). The app + // tears down with close() while that open() is still pending — close() + // can cancel the timer but not the already-running connect() body. When + // open() finally resolves, the socket must NOT be installed: otherwise a + // live, message-handling, resubscribed socket outlives teardown and + // "close() is permanent" is a lie. The orphan must be closed instead. + const fakes: Array = [] + let opens = 0 + let releaseOpen: ((ws: WebSocketLike) => void) | null = null + const t = new WebSocketTransport({ + url: "wss://fake-teardown-race", + reconnectDelay: () => 10, + open: async () => { + opens++ + const f = makeFake() + fakes.push(f) + if (opens === 1) return f.ws + // The reconnect attempt: hold open() unresolved until the test says so. + return new Promise((resolve) => { + releaseOpen = resolve + }) + }, + }) + await t.subscribe("s1", "messages", noopHandler()) + + fakes[0]!.emit("close", { code: 1006 }) // transient: retry armed for +10ms + await waitFor(() => releaseOpen !== null) // timer fired; open() is in flight + + let orphanClosed = false + let orphanFrames = 0 + const orphan = fakes[1]! + orphan.ws.close = () => { + orphanClosed = true + } + orphan.ws.send = () => { + orphanFrames++ + } + + t.close() // teardown while the reconnect open() is still pending + releaseOpen!(orphan.ws) // the slow open finally resolves, after teardown + await waitFor(() => orphanClosed) // the orphan socket is closed, not installed + expect(orphanFrames).toBe(0) // and never resubscribed to the server + + await new Promise((r) => setTimeout(r, 50)) + expect(opens).toBe(2) // nothing kept retrying after close() + }) +}) diff --git a/tests/reconnect-recovery.test.ts b/tests/reconnect-recovery.test.ts index 5bc5b7e..84cfdad 100644 --- a/tests/reconnect-recovery.test.ts +++ b/tests/reconnect-recovery.test.ts @@ -7,7 +7,7 @@ import { type SubHandler, WebSocketTransport, type WebSocketLike } from "../src/ // the cached connectPromise stays a rejected promise forever: every subsequent // connect() returns the same stale rejection, no close event ever fires (the // socket never opened), and no further retry is ever scheduled. One outage -// longer than reconnectDelayMs permanently kills all subscriptions until reload. +// longer than the reconnect delay permanently kills all subscriptions until reload. interface Recorder { events: Array<[string, ...Array]> @@ -43,7 +43,7 @@ describe("transport reconnect recovery — failed open must not wedge the client const t = new WebSocketTransport({ url: `https://example.com/sync/${room}`, - reconnectDelayMs: 20, + reconnectDelay: 20, open: async () => { if (failNext > 0) { failNext-- @@ -106,7 +106,7 @@ describe("transport reconnect recovery — failed open must not wedge the client const t = new WebSocketTransport({ url: "wss://fake-recovery", - reconnectDelayMs: 20, + reconnectDelay: 20, open: async () => { if (!allowOpen) throw new Error("server unreachable") // Return a fake WebSocketLike that does nothing — we just need the diff --git a/tests/reconnect-window.test.ts b/tests/reconnect-window.test.ts index 70d8892..8d115d2 100644 --- a/tests/reconnect-window.test.ts +++ b/tests/reconnect-window.test.ts @@ -7,7 +7,7 @@ import type { ClientFrame, ServerFrame } from "../src/wire/frames.ts" // design (the bug itself is in the plain reconnect path, present on this // branch; the forced-reconnect machinery is not). The `reconnecting` flag was // set inside the reconnect TIMER, so a connect() triggered on demand — a -// mutation fired within reconnectDelayMs of a drop — established the fresh +// mutation fired within the reconnect delay of a drop — established the fresh // socket with the flag still false: NO resubscribeAll, every subscription // silently dead (the server has no subs for the new socket), and the late // timer's connect() early-returned, wedging the flag. The flag must be set @@ -56,7 +56,7 @@ describe("reconnect window (drop → demand-driven connect before the timer)", ( const fakes: Array = [] const t = new WebSocketTransport({ url: "wss://fake", - reconnectDelayMs: 60_000, // the timer must NOT be what saves us + reconnectDelay: () => 60_000, // FIXED policy: the timer must NOT be what saves us (jitter could fire early) open: () => { const f = makeFake() fakes.push(f) diff --git a/tests/reconnect.test.ts b/tests/reconnect.test.ts index 26583cd..d44a3b1 100644 --- a/tests/reconnect.test.ts +++ b/tests/reconnect.test.ts @@ -40,7 +40,7 @@ describe("transport auto-reconnect + resubscribe (M7)", () => { const sockets: Array void }> = [] const t = new WebSocketTransport({ url: `https://example.com/sync/${room}`, - reconnectDelayMs: 20, + reconnectDelay: 20, open: async () => { const res = await SELF.fetch(`https://example.com/sync/${room}`, { headers: { Upgrade: "websocket" } }) const ws = res.webSocket diff --git a/tests/reinsert-catchup.test.ts b/tests/reinsert-catchup.test.ts index b21355d..06ac2b6 100644 --- a/tests/reinsert-catchup.test.ts +++ b/tests/reinsert-catchup.test.ts @@ -18,7 +18,7 @@ import type { TestApi } from "./test-worker.ts" function makeTransport(room: string): WebSocketTransport { return new WebSocketTransport({ url: `https://example.com/sync/${room}`, - reconnectDelayMs: 20, + reconnectDelay: 20, open: async () => { const res = await SELF.fetch(`https://example.com/sync/${room}`, { headers: { Upgrade: "websocket" } }) const ws = res.webSocket