diff --git a/CHANGELOG.md b/CHANGELOG.md index b55d30d..382f16b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,6 +51,16 @@ While pre-1.0, the public API may change between 0.x releases. applied cursor. ID-scoped receipts (`committed`/`rejected`/`page`) still settle their waiters from a stale socket — they are not re-covered by any replay — but never advance the cursor. +- **`WebSocketTransport.close()` is now revivable (ADR-0020).** A later + `connect()` clears the intentional-close latch, restoring auto-reconnect and + `onClosed` delivery on a reused transport. Previously `close()` was permanent + across a `connect()`: a revived transport (connection pools reuse instances) + silently lost both. Code that relied on `close()` being permanently terminal + must not re-`connect()` the same instance. +- **The injectable `open` gains an optional `AbortSignal` (ADR-0020).** + Additive and backward-compatible — existing openers compile and run + unchanged — but a custom opener should honour it (close its socket, reject) + so `close()` during an in-flight handshake frees a still-CONNECTING socket. ### Fixed @@ -61,6 +71,20 @@ While pre-1.0, the public API may change between 0.x releases. a benign no-op — the socket's `webSocketClose` already tore down its subs. Covers the `Broadcaster` egress path too (it routes through the same `#send`). Outbound-only; no state impact, no behavior change for OPEN sockets (issue #40). +- **`connect()` never resolves disconnected (ADR-0020, issue #37).** The + close-epoch discard previously `return`ed, resolving `connect()` with no + socket adopted; the awaiting `subscribe`/`sendMut`/`fetch` then sent on a + null socket and threw, floating an unhandled rejection and leaving the + collection silently empty. `connect()` now re-dials a revived transport or + rejects the new typed `TransportClosedError`; a racing `subscribe` either + lands its frame on the next connection or rejects typed (routed to + `markError`, so `preload()` fails loud and recovers on retry). +- **`close()` can abort a socket whose `open()` is still in flight (ADR-0020, + issue #38).** Previously `close()` disposed through `this.ws`, which is + `null` during the handshake, so a slow or never-completing handshake leaked a + CONNECTING socket forever. `close()` now aborts the in-flight `open()` via the + `AbortSignal`; the default browser opener honours it, and the close-epoch + discard remains the backstop for signal-ignoring openers. - `unsubscribe` during an in-flight `subscribe`'s connect no longer sends the sub after the socket opens — previously the server persisted a ghost subscription (ADR-0019) with no local consumer until the socket dropped. diff --git a/README.md b/README.md index 9481042..72c1dc8 100644 --- a/README.md +++ b/README.md @@ -267,6 +267,45 @@ same socket: `transport.call.(args)` (typed sugar) or the low-level `transport.sendCall("clearRoom", undefined)` — both mint the txId for you and resolve with the command's result on `committed`. +#### Transport lifecycle & reconnect + +The transport auto-reconnects on an unexpected drop and resubscribes every +collection from its single applied cursor (a windowed catch-up, not a +re-snapshot). Tune the pacing with `reconnectDelay` (a base-delay `number` for +the default jittered backoff, or a full `(attempt, closeCode?, closeReason?) => +number | null` policy — return `null` to stop). Application close codes +(4000-4999) are terminal by default; wire `onClosed(code, reason)` to observe a +deliberate server close (e.g. an auth rejection) — see +[ADR-0016](./docs/adr/0016-reconnect-policy.md). + +`close()` disposes the transport, and `connect()` **revives** it: a later +`connect()` clears the intentional-close latch, so a reused transport +auto-reconnects and delivers `onClosed` again. A `connect()` (and any +`subscribe`/`sendMut`/`fetch` awaiting it) never resolves disconnected — it +rejects `TransportClosedError` if the transport was closed and stays closed +([ADR-0020](./docs/adr/0020-connect-contract-abortable-open.md)). + +The socket opener is injectable (`open`, for non-browser runtimes or tests). It +receives an optional `AbortSignal` that `close()` aborts while the handshake is +still in flight — honour it (close the socket, reject) so disposing a transport +before its socket finishes connecting never leaks a CONNECTING socket: + +```ts +new WebSocketTransport({ + url, + open: (signal) => + new Promise((resolve, reject) => { + const ws = new WebSocket(url) + signal?.addEventListener("abort", () => { ws.close(); reject(new Error("aborted")) }, { once: true }) + ws.addEventListener("open", () => resolve(ws)) + ws.addEventListener("error", () => reject(new Error("ws error"))) + }), +}) +``` + +An opener that ignores the signal still works (the transport closes the socket +once `open()` resolves), but a handshake that never resolves then leaks. + ### 4. SSR (experimental) Built on TanStack DB's SSR support (`DbClient` `dehydrate()`/`hydrate()` and diff --git a/docs/adr/0020-connect-contract-abortable-open.md b/docs/adr/0020-connect-contract-abortable-open.md new file mode 100644 index 0000000..3a6ed5e --- /dev/null +++ b/docs/adr/0020-connect-contract-abortable-open.md @@ -0,0 +1,151 @@ +# 0020 — connect() never resolves disconnected; open() is abortable via AbortSignal + +**Status:** Accepted. Fixes issues #37 and #38. Amends ADR-0016 (reconnect +policy) and ADR-0011's `Transport` seam; does not supersede either. + +## Context + +`WebSocketTransport.connect()` and `close()` share one race-prone window: the +in-flight `open()` between "start dialing" and "socket adopted". Two real +defects lived there, both observed against 0.6.0 in a shipping product and +verified on `main` post-#36. + +### #37 — connect() could resolve having adopted no socket + +The close-epoch guard (ADR-0016's third race guard) discards a socket whose +`open()` was in flight when `close()` landed, then **`return`ed** — resolving +the connect promise with `this.ws === null`. `subscribe`/`sendMut`/`fetch` all +`await connect()` and then send; the send threw `transport not connected`, the +subscription registered with no frame ever on the wire, and the collection's +`preload()` promise floated as an unhandled rejection (measured: 9 per cold +page load). The natural React shape — `createCollection(...)` + `preload()` in +one render pass, StrictMode's mount→unmount→remount — hit it deterministically. + +Two residues shared the same surface: + +- **`intentionallyClosed` latched with no reset.** Set `true` by `close()` and + never cleared — not even by a later `connect()`. A transport revived after + `close()` (connection pools reuse instances) lost auto-reconnect forever + (measured: a 1006 on a revived transport never re-dialed). +- **The latch also silenced `onClosed`.** The unexpected-close handler returns + early when `intentionallyClosed` is set, skipping both `scheduleReconnect` + *and* the terminal `onClosed` — so an app wiring `onClosed` to learn about a + server's deliberate 4xxx close heard nothing on any revived transport. + +### #38 — close() could not abort an in-flight open() + +`connect()` assigns `this.ws` only *after* the handshake resolves; `close()` +disposes through that field (`this.ws?.close()`). While `open()` is in flight +`this.ws` is `null`, so `close()` is a no-op on the socket. The only path that +disposed the in-flight socket was the close-epoch discard — which runs **only +when `open()` resolves**. A handshake that is slow or never completes (a dev +proxy that doesn't upgrade, a hung upgrade, a half-open LB connection) leaked a +socket stuck in CONNECTING forever. Deterministic on any dispose-before-open; +benign in prod (handshakes complete in ms), user-visible in dev and a real +leak wherever a handshake can hang. + +## Decision + +### 1. `connect()` never resolves disconnected + +`await connect()` resolves only with a live socket adopted, and never resolves +with `this.ws === null`. At the close-epoch discard it now either: + +- **re-dials** — if a later `connect()` revived the transport (see §3), it + defers to whichever dial now owns the transport (`return this.connect()`), + so the original awaiter rides the socket that dial installs; or +- **rejects** `TransportClosedError` — a new typed, catchable error — if the + transport was closed and *stays* closed. + +This is the contract issue #37 asked for: fix `connect()`, not add a queue in +`subscribe`. `subscribe`/`sendMut`/`fetch` inherit it for free — a racing +`subscribe` now either lands its frame on the next connection or rejects typed; +`do-collection` already routes that rejection to `markError`, so `preload()` +fails loud (and a retried `preload()` / the policy-driven reconnect recovers) +rather than resolving a silently-empty collection. + +Only the dial that still **owns** `connectPromise` drives failure recovery (the +`catch` guards on `this.connectPromise === p`): a superseded dial that re-dialed +via the epoch branch must not null a newer attempt's promise or double-schedule +its reconnect. + +### 2. `open()` takes an `AbortSignal`; `close()` aborts the in-flight handshake + +The pluggable opener's contract widens (backward-compatibly): + +```ts +open?: (signal?: AbortSignal) => WebSocketLike | Promise +``` + +The transport creates one `AbortController` per dial, holds it in `openAbort` +for exactly as long as a dial is parked on `await open()`, and `close()` aborts +it. The default browser `open()` honours the signal: on abort it closes its +still-CONNECTING socket and rejects `TransportClosedError`. This is the one +path that disposes a handshake that *never resolves* — the acceptance criterion +the epoch discard could not meet. + +**Backward compatibility is preserved.** An existing `open()` that takes no +argument (or ignores the signal) still works: `close()` cannot abort it +mid-flight, but the epoch guard still closes the orphan the instant `open()` +resolves — the pre-fix behaviour, minus the never-resolves leak. A custom +opener that wants dispose-before-open to free its socket immediately (including +the never-resolves case) **must** honour the signal. This is the *only* +residual gap and it is opt-in: the default opener closes it, and a custom +opener closes it in the ~15 lines the reporting consumer already wrote at this +seam. `openAbort` tracks the current in-flight dial only; a pathological +close→connect→close chain can leave an earlier signal-ignoring dial's socket to +the epoch discard, same as today. + +`openAbort` is released in a `finally` the instant `open()` settles — before +the epoch check and install — so a `close()` arriving during install-processing +never aborts the socket about to be adopted; it bumps `closeEpoch` instead, and +the epoch guard handles it. + +### 3. Dialing clears the intentional-close latch + +`connect()` sets `intentionallyClosed = false` at its head. Dialing is the +clearest possible statement of intent to be connected, so a revived transport +regains **both** auto-reconnect and `onClosed` delivery — the two residues fall +out of one line. This is orthogonal to the close-epoch guard: the epoch still +discards a socket opened before *this* dial's `close()`, while the latch governs +whether a *future* unexpected drop reconnects. + +**Behaviour change (deliberate).** `close()` is no longer permanent across a +later `connect()`. ADR-0016's `intentionallyClosed` still governs auto-reconnect +suppression; only its clearing point moves — from "never" to "the next +`connect()`". The `reconnect-policy` test that pinned the old permanence is +rewritten to pin revival (issue #37's acceptance). + +## Alternatives considered + +- **A re-subscribe queue in `subscribe`.** Rejected by #37 itself: the defect + is `connect()`'s contract, and `sendMut`/`fetch` share it. Fixing one caller + leaves the others broken. +- **`open()` returns `{ socket, whenOpen }`** so the transport owns the handle + from creation. Strictly more capable, but a *breaking* signature change for + every existing opener; an `AbortSignal` is additive and the default opener + implements the whole contract. If a future need forces transport-owned socket + creation, that is a separate ADR. +- **The full generation-counter unification** ADR-0016 deferred (collapse the + tracked timer, the `this.ws !== ws` close guard, and the close-epoch check + into one counter). Not adopted: it does not *fall out* of this fix, the diff + is already surgical, and the existing race tests (its intended harness) stay + green. The deferral stands; unify when the fragility actually bites. + +## Consequences + +- **New export:** `TransportClosedError`. `connect()`/`subscribe`/`sendMut`/ + `fetch` can reject with it around a close race — catchable and distinct from + `MutationRejectedError`. +- **`open` gains an optional `AbortSignal`.** Additive; existing openers compile + and run unchanged. Custom openers should honour it to be abortable-before-open. +- **`close()` is revivable.** A later `connect()` restores auto-reconnect and + `onClosed`. Code that relied on `close()` being permanently terminal must not + re-`connect()` the same instance (or must track terminality itself). +- No new timers, no idle work: `openAbort` exists only while a dial is parked on + `open()`; the no-idle-timers invariant (ADR-0016) is preserved. + +## Out of scope + +Issue #39 (in-flight `pendingTx` settlement on an unexpected close) is a +separate follow-up and is untouched here. diff --git a/docs/adr/README.md b/docs/adr/README.md index cd3f6fe..c28b1ea 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -27,3 +27,4 @@ explains the displacement. | [0017](./0017-blob-wire-normalization.md) | BLOB wire normalization: bare ArrayBuffer becomes Uint8Array at emission | Accepted | | [0018](./0018-oversize-frames.md) | Oversize frames: client pre-send guard is the rejection surface; outbound is warn-only | Accepted | | [0019](./0019-subscription-persistence-across-hibernation.md) | Subscriptions persist in SQLite and restore on hibernation wake | Accepted | +| [0020](./0020-connect-contract-abortable-open.md) | connect() never resolves disconnected; open() is abortable via AbortSignal | Accepted (amends 0016 + 0011 seam) | diff --git a/src/client/transport.ts b/src/client/transport.ts index efe70a1..ae90c6d 100644 --- a/src/client/transport.ts +++ b/src/client/transport.ts @@ -91,6 +91,22 @@ export class MutationRejectedError extends Error { } } +/** Thrown by `connect()` — and so by any `await connect()` in `subscribe` / + * `sendMut` / `fetch` — when the transport was closed while a dial was in + * flight and was NOT revived by a later `connect()`. `connect()` never + * resolves having adopted no socket (issue #37): it re-dials when revived, or + * rejects with this typed, catchable error so a collection fails its ready + * gate loud (do-collection routes it to `markError`) instead of resolving a + * silently-empty collection or floating an unhandled rejection. The default + * `open()` also rejects with it when `close()` aborts an in-flight handshake + * (issue #38 / ADR-0020). */ +export class TransportClosedError extends Error { + constructor(message = "transport closed during connect") { + super(message) + this.name = "TransportClosedError" + } +} + /** 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 @@ -115,8 +131,16 @@ export function defaultReconnectDelay(baseMs: number, capMs = 30_000): Reconnect export interface TransportOptions { url: string /** Returns a CONNECTED socket. Default opens `new WebSocket(url)` and resolves - * on its `open` event. Tests/other runtimes inject a ready socket. */ - open?: () => WebSocketLike | Promise + * on its `open` event. Tests/other runtimes inject a ready socket. + * + * The optional `AbortSignal` is aborted by `close()` when the handshake is + * still in flight (issue #38 / ADR-0020): an `open` that honours it must + * close its in-flight socket and reject, so a still-CONNECTING socket is + * never leaked — including a handshake that never resolves on its own. + * Ignoring the signal stays backward-compatible (the transport still + * discards and closes the socket if `open()` ever resolves), but a + * never-resolving handshake then leaks; custom openers SHOULD honour it. */ + open?: (signal?: AbortSignal) => WebSocketLike | Promise codec?: FrameCodec /** Confirmation/await timeout in ms. */ timeoutMs?: number @@ -174,7 +198,11 @@ export class WebSocketTransport { private connectPromise: Promise | null = null private readonly codec: FrameCodec private readonly timeoutMs: number - private readonly open: () => WebSocketLike | Promise + private readonly open: (signal?: AbortSignal) => WebSocketLike | Promise + /** The in-flight handshake's abort controller, so close() can tear down a + * socket whose open() has not yet resolved (issue #38 / ADR-0020). Null + * whenever no dial is parked on `await open()`. */ + private openAbort: AbortController | null = null private readonly handlers = new Map< string, @@ -217,9 +245,24 @@ export class WebSocketTransport { this.onClosed = opts.onClosed this.open = opts.open ?? - (() => + ((signal?: AbortSignal) => new Promise((resolve, reject) => { const ws = new (globalThis as unknown as { WebSocket: new (u: string) => WebSocketLike }).WebSocket(opts.url) + const onAbort = (): void => { + // close() landed mid-handshake: abort the still-CONNECTING socket so + // it is never leaked in CONNECTING forever (issue #38), and reject + // typed so the parked connect() unwinds instead of hanging. + try { + ws.close() + } catch { + /* already dead */ + } + reject(new TransportClosedError()) + } + if (signal) { + if (signal.aborted) return onAbort() + signal.addEventListener("abort", onAbort, { once: true }) + } ws.addEventListener("open", () => resolve(ws)) ws.addEventListener("error", () => reject(new Error("websocket error"))) })) @@ -294,18 +337,40 @@ export class WebSocketTransport { async connect(): Promise { if (this.ws) return if (this.connectPromise) return this.connectPromise - this.connectPromise = (async () => { + // Dialing is the clearest statement of intent to be connected: clear the + // intentional-close latch so a transport revived after close() (connection + // pools do this) auto-reconnects on a later drop AND delivers onClosed + // again (issue #37). This is orthogonal to the close-epoch guard below, + // which still discards any socket opened before THIS dial's close. + this.intentionallyClosed = false + const p = (async (): Promise => { const epoch = this.closeEpoch - const ws = await this.open() + const controller = new AbortController() + this.openAbort = controller + let ws: WebSocketLike + try { + ws = await this.open(controller.signal) + } finally { + // Release our handle the instant open() settles — before the epoch + // check / install — so a close() during install-processing never aborts + // the socket we are about to adopt (it bumps closeEpoch instead). + if (this.openAbort === controller) this.openAbort = null + } if (epoch !== this.closeEpoch) { - // close() ran while open() was in flight: the transport is torn down. - // Discard the orphan instead of installing it. + // close() ran while open() was in flight: the transport was torn down. + // Discard the orphan instead of installing it — it must never speak for + // the stream. try { ws.close() } catch { /* ignore */ } - return + // connect() must never RESOLVE disconnected (issue #37). If the + // transport was closed and stays closed, reject typed. If a later + // connect() revived it (the latch is clear), defer to whichever dial + // now owns the transport so our awaiters ride the socket it installs. + if (this.intentionallyClosed) throw new TransportClosedError() + return this.connect() } // Browsers default WebSocket.binaryType to "blob"; force "arraybuffer" so // binary frames arrive as ArrayBuffer (workerd already does). Without this @@ -355,11 +420,17 @@ export class WebSocketTransport { this.resubscribeAll() } })() + this.connectPromise = p // A socket that never OPENED fires no close event, so the close-handler // recovery path (above) can't run. Clear the cached rejection so the next // connect() starts fresh, and re-arm the timer while subscriptions are // live — otherwise one unreachable attempt wedges the transport forever. - this.connectPromise.catch(() => { + // Only the dial that still OWNS connectPromise drives recovery: a superseded + // dial (a revived transport's earlier attempt that re-dialed via the epoch + // branch above) must not null a newer attempt's promise or double-schedule + // its reconnect — that newer attempt owns its own recovery. + p.catch(() => { + if (this.connectPromise !== p) return this.connectPromise = null if (!this.intentionallyClosed && this.handlers.size > 0) { // A socket that never opened has no close frame — the policy sees an @@ -367,7 +438,7 @@ export class WebSocketTransport { this.scheduleReconnect() } }) - return this.connectPromise + return p } /** Consult the policy and either arm the next reconnect attempt or stop. */ @@ -424,6 +495,12 @@ export class WebSocketTransport { close(): void { this.intentionallyClosed = true this.closeEpoch++ + // Abort an in-flight handshake so a still-CONNECTING socket is closed now, + // not leaked until (or unless) its open() eventually resolves (issue #38). + // A signal-honouring open() closes its socket and rejects; the epoch guard + // in connect() is the backstop for one that ignores the signal. + this.openAbort?.abort() + this.openAbort = null this.clearReconnectTimer() for (const w of this.seqWaiters.splice(0)) { clearTimeout(w.timer) diff --git a/tests/connect-contract.test.ts b/tests/connect-contract.test.ts new file mode 100644 index 0000000..ffd0b4d --- /dev/null +++ b/tests/connect-contract.test.ts @@ -0,0 +1,283 @@ +import { describe, expect, it } from "vitest" +import { + type SubHandler, + TransportClosedError, + WebSocketTransport, + type WebSocketLike, +} from "../src/client/transport.ts" +import { createFrameCodec } from "../src/wire/frame-codec.ts" +import type { ClientFrame } from "../src/wire/frames.ts" + +// WHY (issues #37 / #38, ADR-0020): the connect/close boundary is the client's +// most race-prone seam. Two defects live in the same in-flight-open window: +// +// #37 — connect() could RESOLVE having adopted no socket (the epoch-discard +// `return`). Its caller (subscribe/sendMut/fetch) then sent on a null +// socket and threw, floating an unhandled rejection and leaving the +// collection silently empty. And `intentionallyClosed` latched forever: +// a transport revived by a later connect() (connection pools do this) +// lost auto-reconnect AND onClosed delivery. +// +// #38 — close() could not abort a socket whose open() was still in flight: +// `this.ws` was null, so `this.ws?.close()` was a no-op, and a handshake +// that never resolved leaked a CONNECTING socket forever. +// +// The contract these pin: connect() never resolves disconnected (it re-dials +// under revival or rejects TransportClosedError), dialing clears the latch, and +// close() aborts an in-flight handshake via the AbortSignal handed to open(). + +const codec = createFrameCodec() + +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)) + } +} + +interface Fake { + ws: WebSocketLike + sent: Array + emit: (type: string, ev: { data?: unknown; code?: number; reason?: string }) => void +} + +function makeFake(): Fake { + const listeners = new Map void>>() + const fake: Fake = { + sent: [], + emit: (type, ev) => { + for (const l of listeners.get(type) ?? []) l(ev) + }, + ws: { + send: (data) => fake.sent.push(codec.decode(data as ArrayBuffer | string) as ClientFrame), + close: () => {}, + addEventListener: (type, l) => { + const arr = listeners.get(type) ?? [] + arr.push(l) + listeners.set(type, arr) + }, + removeEventListener: () => {}, + }, + } + return fake +} + +describe("connect() contract — never resolves disconnected (#37)", () => { + it("subscribe before the handshake completes still lands the sub frame once connected", async () => { + const fake = makeFake() + let release: (() => void) | null = null + const t = new WebSocketTransport({ + url: "wss://x", + open: () => new Promise((res) => (release = () => res(fake.ws))), + }) + + const sub = t.subscribe("s1", "messages", noopHandler()) + await waitFor(() => release !== null) + // Handshake still in flight: no frame has escaped. + expect(fake.sent.length).toBe(0) + + release!() // the socket finally opens + await sub + // The frame LANDS on the socket that finally opened — not thrown into the void. + expect(fake.sent.some((f) => f.t === "sub" && (f as { subId?: string }).subId === "s1")).toBe(true) + t.close() + }) + + it("close() during the handshake rejects the in-flight connect() typed — never resolves disconnected", async () => { + const fake = makeFake() + let aborted = false + let resolved = false + const t = new WebSocketTransport({ + url: "wss://x", + // A never-resolving open that HONOURS the abort signal (the documented + // contract): close() must be able to tear it down. + open: (signal) => + new Promise((_res, rej) => { + signal?.addEventListener( + "abort", + () => { + aborted = true + rej(new TransportClosedError()) + }, + { once: true }, + ) + }), + }) + + const c = t.connect() + const outcome = c.then( + () => { + resolved = true + return "resolved" + }, + (e) => e, + ) + t.close() // tears down the in-flight handshake + const r = await outcome + expect(aborted).toBe(true) + expect(resolved).toBe(false) // it did NOT resolve disconnected… + expect(r).toBeInstanceOf(TransportClosedError) // …it rejected typed + void fake + }) + + it("close() during the first subscribe's handshake, then a fresh connect() revives and lands the sub", async () => { + const fakes: Array = [] + const releases: Array<(() => void) | undefined> = [] + const t = new WebSocketTransport({ + url: "wss://x", + open: (signal) => + new Promise((res, rej) => { + const f = makeFake() + fakes.push(f) + const idx = fakes.length - 1 + releases[idx] = () => res(f.ws) + signal?.addEventListener("abort", () => rej(new TransportClosedError()), { once: true }) + }), + }) + + const sub1 = t.subscribe("s1", "messages", noopHandler()) + const sub1Outcome = sub1.then(() => "ok", (e) => e) // capture the rejection — no unhandled + await waitFor(() => fakes.length === 1) + + t.close() // aborts the in-flight open → sub1 must reject typed + expect(await sub1Outcome).toBeInstanceOf(TransportClosedError) + + // Revive: a fresh subscribe re-dials. The frame must land on the NEW socket. + const sub2 = t.subscribe("s2", "messages", noopHandler()) + await waitFor(() => releases[1] !== undefined) + releases[1]!() + await sub2 + expect(fakes[1]!.sent.some((f) => f.t === "sub" && (f as { subId?: string }).subId === "s2")).toBe(true) + t.close() + }) +}) + +describe("revived transport — dialing clears the intentional-close latch (#37)", () => { + it("auto-reconnects on a 1006 drop after a connect() revives a closed transport", async () => { + const fakes: Array = [] + let opens = 0 + const t = new WebSocketTransport({ + url: "wss://x", + reconnectDelay: () => 5, + open: () => { + opens++ + const f = makeFake() + fakes.push(f) + return f.ws + }, + }) + await t.subscribe("s1", "messages", noopHandler()) + expect(opens).toBe(1) + + t.close() + await t.connect() // revive: dialing is intent, the latch must clear + expect(opens).toBe(2) + + // An unexpected drop on the revived socket MUST auto-reconnect now — the + // pre-fix latch silenced this forever. + fakes[1]!.emit("close", { code: 1006 }) + await waitFor(() => opens >= 3) + t.close() + }) + + it("delivers onClosed on a terminal 4xxx close after a connect() revives a closed transport", async () => { + const fakes: Array = [] + const closed: Array<[number | undefined, string | undefined]> = [] + const t = new WebSocketTransport({ + url: "wss://x", + reconnectDelay: 5, + onClosed: (code, reason) => closed.push([code, reason]), + open: () => { + const f = makeFake() + fakes.push(f) + return f.ws + }, + }) + await t.subscribe("s1", "messages", noopHandler()) + + t.close() + await t.connect() // revive: onClosed delivery must be restored too + fakes[1]!.emit("close", { code: 4403, reason: "removed from this workspace" }) + await waitFor(() => closed.length === 1) + expect(closed[0]).toEqual([4403, "removed from this workspace"]) + t.close() + }) +}) + +describe("close() aborts an in-flight handshake — no leaked CONNECTING socket (#38)", () => { + it("close() during a never-resolving open() aborts (closes) the still-CONNECTING socket", async () => { + let socketClosed = false + let opened = false + const t = new WebSocketTransport({ + url: "wss://x", + open: (signal) => + new Promise((_res, rej) => { + opened = true + const ws: WebSocketLike = { + send: () => {}, + close: () => { + socketClosed = true + }, + addEventListener: () => {}, + removeEventListener: () => {}, + } + // The default browser open() closes its socket on abort; a custom one + // that honours the signal does the same. This never resolves — only + // the abort can free the socket. + signal?.addEventListener( + "abort", + () => { + ws.close() + rej(new TransportClosedError()) + }, + { once: true }, + ) + }), + }) + + const c = t.connect() + const outcome = c.then(() => "ok", (e) => e) + await waitFor(() => opened) + t.close() + expect(socketClosed).toBe(true) // the CONNECTING socket was aborted, not leaked + expect(await outcome).toBeInstanceOf(TransportClosedError) + t.close() + }) + + it("a custom open() that IGNORES the signal still has its late socket discarded and closed (backward-compat)", async () => { + let socketClosed = false + let release: (() => void) | null = null + const t = new WebSocketTransport({ + url: "wss://x", + // Legacy open contract: takes no signal, resolves whenever. close() cannot + // abort it mid-flight, but the epoch-discard must still close the orphan + // the moment open() resolves — no socket installed, no leak. + open: () => + new Promise((res) => { + const ws: WebSocketLike = { + send: () => {}, + close: () => { + socketClosed = true + }, + addEventListener: () => {}, + removeEventListener: () => {}, + } + release = () => res(ws) + }), + }) + + const c = t.connect() + const outcome = c.then(() => "ok", (e) => e) + await waitFor(() => release !== null) + t.close() // cannot abort a signal-ignoring open… + release!() // …but when it resolves, the orphan is closed and connect() rejects + await waitFor(() => socketClosed) + expect(await outcome).toBeInstanceOf(TransportClosedError) + t.close() + }) +}) diff --git a/tests/reconnect-policy.test.ts b/tests/reconnect-policy.test.ts index 6f041c4..158b0bd 100644 --- a/tests/reconnect-policy.test.ts +++ b/tests/reconnect-policy.test.ts @@ -128,8 +128,14 @@ describe("transport reconnect policy (ADR-0016)", () => { t.close() }) - it("intentional close() is permanent: connect() works again but drops no longer auto-reconnect", async () => { - const room = "rcpol-close-permanent" + it("close() suppresses auto-reconnect until a later connect() REVIVES the transport (#37)", async () => { + // Contract change (issue #37): close() suppresses auto-reconnect, but a + // later connect() is an explicit statement of intent that CLEARS the latch. + // A revived transport (connection pools do this) must auto-reconnect again — + // the pre-fix latch never reset, silencing reconnect forever on any revived + // transport. ADR-0016's `intentionallyClosed` still governs; only its + // clearing point moves (from "never" to "on the next connect()"). + const room = "rcpol-close-revive" let opens = 0 const t = new WebSocketTransport({ url: `https://example.com/sync/${room}`, @@ -141,16 +147,16 @@ describe("transport reconnect policy (ADR-0016)", () => { }) 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… + // While closed, a drop must NOT reconnect (the latch is set). + t.close() + // Revive: dialing clears the latch, so a subsequent unexpected drop DOES + // auto-reconnect on this same instance. 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) + await waitFor(() => opens >= 3) // auto-reconnect restored by the revive + t.close() }) })