From 4d49702b6035e882934572dab4b32defd1508a41 Mon Sep 17 00:00:00 2001 From: Diego Mello Date: Wed, 12 Aug 2026 20:44:21 -0300 Subject: [PATCH 1/4] refactor: one Deadline primitive behind every bounded wait in the connection lifecycle --- CONTEXT.md | 4 +- ...ne-deadline-primitive-for-bounded-waits.md | 60 +++++++ lib/drivers/__tests__/ddp.liveness.spec.ts | 33 ++++ lib/drivers/ddp.ts | 162 ++++++++---------- package-lock.json | 46 +---- 5 files changed, 169 insertions(+), 136 deletions(-) create mode 100644 docs/adr/0005-one-deadline-primitive-for-bounded-waits.md diff --git a/CONTEXT.md b/CONTEXT.md index 5e4ede48..5bf616f4 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -78,11 +78,11 @@ A retry scheduled after a connection drops, waited out before a new Socket is bu _Avoid_: Reconnect (unqualified — say which of the two), retry **Liveness chain**: -The repeating ping and its pong, the only thing that decides whether an apparently-open Socket is actually alive. A Socket the server has stopped answering still reads as open to the transport. +The repeating ping and its pong, the only thing that decides whether an apparently-open Socket is actually alive. One Probe per interval, repeated for as long as the server answers, and a Reopen the moment it stops. A Socket the server has stopped answering still reads as open to the transport. _Avoid_: Heartbeat, keepalive **Probe**: -A single bounded liveness check on a Socket that looks open, asked for on demand rather than on the chain's schedule. +A single bounded liveness check on a Socket that looks open. The chain runs one per interval; a caller can also ask for one on demand, off that schedule. _Avoid_: Health check, ping (that is one message of the chain) **Deadline**: diff --git a/docs/adr/0005-one-deadline-primitive-for-bounded-waits.md b/docs/adr/0005-one-deadline-primitive-for-bounded-waits.md new file mode 100644 index 00000000..aca740af --- /dev/null +++ b/docs/adr/0005-one-deadline-primitive-for-bounded-waits.md @@ -0,0 +1,60 @@ +# ADR-0005: Every bounded wait in the connection lifecycle goes through one Deadline primitive + +**Status:** Accepted + +## Context + +The Socket in `lib/drivers/ddp.ts` holds four waits it must be able to give up +on: `reopenNow` waits for `open`, `probe` waits for `pong`, `waitForOpen` waits +for `open`, and the Liveness chain waits for the answer to its ping. + +Each one wrote the same sequence by hand. Register a `once` listener. Arm a +`setTimeout`. Guard a `settled` boolean so the two racers cannot both settle the +promise. Then remove the listener and clear the timer, whichever arrived first. + +That sequence is not incidental. ADR-0002 names these same four waits as the +reason `SDKEventEmitter` had to replace `off` and `emit`: an `off` that misses +removes an unrelated listener, and a listener left attached after its wait ends +is a leak on a socket that lives for the length of the session. The pairing of +`once` with the matching `off` is load-bearing, and it was written out four +times, so a fifth wait meant getting it right a fifth time. + +The two liveness waits had already drifted. A Probe and one turn of the Liveness +chain are the same act — write a ping frame, wait for the pong, give up on a +Deadline — but the chain raced a promise against a hand-built timer while the +Probe listened for an event with a timer of its own. Each needed its own fix for +the same class of fault. + +## Decision + +The Socket has one private bounded wait, `awaitEvent(event, deadlineMs, start)`. +It listens for the event, gives up on the Deadline, and detaches the listener and +clears the timer on either outcome. It resolves true when the event arrived and +false when the Deadline expired. + +- `start` runs with the listener already attached, so a server that answers in + the same tick as the write cannot be missed. Returning false from `start` + abandons the wait, leaving neither timer nor listener behind. This is how a + Probe reports a transport that refused the write. +- `reopenNow`, `probe` and `waitForOpen` are each expressed in terms of it. +- The Liveness chain is expressed as a repeated Probe: probe on the interval, + schedule the next turn when the server answers, Reopen when it does not. The + Deadline stays on the Probe rather than moving into `send`, so no other caller + inherits a reply timeout. + +## Consequences + +- The `once`/`off` pairing ADR-0002 protects now exists in one place. A fifth + wait gets it by construction. +- A Probe and a turn of the chain cannot drift apart again, because there is only + one of them. +- The chain's ping is written straight to the transport rather than through + `send`. The frame on the wire is unchanged — `send` assigns no id to a ping — + but the chain no longer consumes an id from the `sent` counter and no longer + registers the `disconnected` listener `send` attaches. Nothing reads the + counter for anything but uniqueness. +- A chain turn on a transport that refuses the write now reopens at once instead + of waiting for a reply that cannot come. +- The `settled` booleans are gone. Settling is idempotent because the primitive + removes the listener and clears the timer together, so neither racer can reach + the promise twice. diff --git a/lib/drivers/__tests__/ddp.liveness.spec.ts b/lib/drivers/__tests__/ddp.liveness.spec.ts index 4af25020..ef075768 100644 --- a/lib/drivers/__tests__/ddp.liveness.spec.ts +++ b/lib/drivers/__tests__/ddp.liveness.spec.ts @@ -168,6 +168,17 @@ describe('Socket liveness', () => { expect(jest.getTimerCount()).toBe(1) }) + it('abandons its deadline when the transport refuses the ping write', async () => { + transport.sendError = new Error('socket closed under the write') + + await expect(socket.probe(2000)).resolves.toBe(false) + + // Only the ping chain's timer: the probe left neither a deadline nor a + // listener behind on the way out. + expect(jest.getTimerCount()).toBe(1) + expect(socket.removeAllListeners('pong')).toHaveLength(0) + }) + it('succeeds when the pong lands after the clock has moved', async () => { // The millisecond advance is the whole test: without it this passes for the // wrong reason, resolving false on the timeout instead of true on the pong. @@ -214,6 +225,28 @@ describe('Socket liveness', () => { } }) + it('leaves no pong listener behind, however many turns it runs', async () => { + // The chain is a probe per turn, and a probe listens for one pong. A turn + // that failed to detach would pile listeners up on a long-lived socket. + for (let turn = 1; turn <= 5; turn += 1) { + await tickWithPong() + } + + expect(socket.removeAllListeners('pong')).toHaveLength(0) + }) + + it('reopens without waiting out the deadline when the transport refuses the ping write', async () => { + transport.sendError = new Error('socket closed under the write') + + await tickWithoutPong() + + // The reopen is already scheduled, with no time advanced past the ping + // interval — a socket that cannot even be written to is dead now, not at + // the deadline. + expect(socket.openTimeout).toBeDefined() + expect(fakeSockets).toHaveLength(1) + }) + it('reconnects when one pong is withheld', async () => { // This test used to pin the opposite: the chain died for good, because the // ping's send went out while `connected` was still true, waited forever on diff --git a/lib/drivers/ddp.ts b/lib/drivers/ddp.ts index 552f7e48..88752763 100644 --- a/lib/drivers/ddp.ts +++ b/lib/drivers/ddp.ts @@ -34,6 +34,8 @@ import { hostToWS } from '../util' import { sha256 } from 'js-sha256' const userDisconnectCloseCode = 4000; +const reopenNowDeadline = 10000; +const probeDeadline = 2000; /** Websocket handler class, manages connections and subscriptions by DDP */ export class Socket extends SDKEventEmitter { @@ -257,6 +259,40 @@ export class Socket extends SDKEventEmitter { }, this.config.reopen); } + /** + * The one bounded wait every other wait in the connection lifecycle is built + * from: listen for `event`, give up on the Deadline, and detach the listener + * and clear the timer whichever arrives first. Resolves true when the event + * arrived and false when the Deadline expired. + * + * `start` runs with the listener already attached, so an event answered in the + * same tick as the write cannot be missed; returning false from it abandons + * the wait without leaving the timer or the listener behind. Pairing `once` + * with the matching `off` is what ADR-0002 hardened the emitter for, and this + * is the only place the SDK now writes that pairing. + */ + private awaitEvent = ( + event: string, + deadlineMs: number, + start: () => boolean = () => true + ): Promise => { + return new Promise(resolve => { + const settle = (arrived: boolean) => { + this.off(event, onArrival) + clearTimeout(deadline as any) + resolve(arrived) + } + + const onArrival = () => settle(true) + + const deadline = setTimeout(() => settle(false), deadlineMs) + + this.once(event, onArrival) + + if (!start()) settle(false) + }) + } + /** * Force an immediate reconnect. Shared across concurrent callers so only one * new WebSocket is created. Emits 'disconnected' to unblock in-flight sends, @@ -269,68 +305,37 @@ export class Socket extends SDKEventEmitter { return this.reopenPromise } - this.reopenPromise = new Promise(resolve => { - this.openTimeout && clearTimeout(this.openTimeout as any) - this.lastPing = 0 - this.emit('disconnected') - - let settled = false - const cleanup = () => { - if (settled) return - settled = true - this.off('open', cleanup) - if (timeout) clearTimeout(timeout as any) - delete this.reopenPromise - resolve() - } - - this.once('open', cleanup) + this.openTimeout && clearTimeout(this.openTimeout as any) + this.lastPing = 0 + this.emit('disconnected') + this.reopenPromise = this.awaitEvent('open', reopenNowDeadline, () => { this.createConnection().catch(() => {}) - - const timeout = setTimeout(() => cleanup(), 10000) + return true + }).then(() => { + delete this.reopenPromise }) return this.reopenPromise } + /** Write a bare ping frame, reporting whether the transport took it. */ + private writePing = (): boolean => { + if (!this.connection || this.connection.readyState !== 1) return false + try { + this.connection.send(JSON.stringify({ msg: 'ping' })) + return true + } catch { + return false + } + } + /** * Bounded liveness check for a socket in the gray zone. Returns true only if * the socket is open and the server answers the ping within the deadline. */ - probe = (timeoutMs = 2000): Promise => { - return new Promise(resolve => { - if (!this.connection || this.connection.readyState !== 1) { - return resolve(false) - } - - let settled = false - const cleanup = () => { - if (settled) return - settled = true - this.off('pong', onPong) - if (timeout) clearTimeout(timeout as any) - } - - const onPong = () => { - cleanup() - resolve(true) - } - - this.once('pong', onPong) - - const timeout = setTimeout(() => { - cleanup() - resolve(false) - }, timeoutMs) - - try { - this.connection.send(JSON.stringify({ msg: 'ping' })) - } catch { - cleanup() - resolve(false) - } - }) + probe = (deadlineMs = probeDeadline): Promise => { + return this.awaitEvent('pong', deadlineMs, this.writePing) } /** Check if websocket connected and ready. */ @@ -356,25 +361,11 @@ export class Socket extends SDKEventEmitter { * *schedules* the retry at that interval, so a deadline of exactly `reopen` * expires as the reconnect begins and every send issued at a drop fails. */ - private waitForOpen = (timeoutMs = this.config.reopen * 2): Promise => { - return new Promise((resolve, reject) => { - const cleanup = () => { - this.off('open', onOpen) - clearTimeout(timeout as any) - } - - const onOpen = () => { - cleanup() - resolve() - } - - this.once('open', onOpen) - - const timeout = setTimeout(() => { - cleanup() - reject(new Error('[ddp] timed out waiting for the connection to open')) - }, timeoutMs) - }) + private waitForOpen = async (deadlineMs = this.config.reopen * 2): Promise => { + const opened = await this.awaitEvent('open', deadlineMs) + if (!opened) { + throw new Error('[ddp] timed out waiting for the connection to open') + } } /** @@ -434,27 +425,20 @@ export class Socket extends SDKEventEmitter { }) } - /** Send ping, record time, re-open if nothing comes back, repeat */ + /** + * The Liveness chain: one Probe per interval, repeated for as long as the + * server keeps answering, and a Reopen the moment it stops. + * + * The Probe carries the Deadline the chain depends on. Without one the chain + * waits on a pong a dead socket never sends, stops there, and never reaches + * `reopen`. The Deadline belongs to the Probe rather than to `send`, so no + * other caller inherits a reply timeout. + */ ping = async () => { this.pingTimeout && clearTimeout(this.pingTimeout as any) - this.pingTimeout = setTimeout(() => { - // The ping goes out while `connected` is still true, so its send never - // waits on `open` — it waits on a pong reply that a dead socket never - // sends, and without a deadline of its own the chain stops here and - // `reopen` is never reached. The deadline lives in `ping` rather than in - // `send`, so no other caller inherits a reply timeout. - let deadline: NodeJS.Timer | number | undefined - const answered = new Promise((_, expire) => { - deadline = setTimeout( - () => expire(new Error('[ddp] ping went unanswered')), - this.config.ping - ) - }) - - Promise.race([this.send({ msg: 'ping' }), answered]) - .then(() => this.ping()) - .catch(() => this.reopen()) - .finally(() => clearTimeout(deadline as any)) + this.pingTimeout = setTimeout(async () => { + if (await this.probe(this.config.ping)) return this.ping() + this.reopen() }, this.config.ping) } diff --git a/package-lock.json b/package-lock.json index 10271bdb..458b9e45 100644 --- a/package-lock.json +++ b/package-lock.json @@ -10,7 +10,6 @@ "license": "MIT", "dependencies": { "js-sha256": "^0.9.0", - "mem": "^4.0.0", "tiny-events": "^1.0.1", "universal-websocket-client": "^1.0.2" }, @@ -6649,32 +6648,6 @@ "tmpl": "1.0.5" } }, - "node_modules/map-age-cleaner": { - "version": "0.1.3", - "resolved": "https://registry.npmjs.org/map-age-cleaner/-/map-age-cleaner-0.1.3.tgz", - "integrity": "sha512-bJzx6nMoP6PDLPBFmg7+xRKeFZvFboMrGlxmNj9ClvX53KrmvM5bXFXEWjbz4cz1AFn+jWJ9z/DJSz7hrs0w3w==", - "license": "MIT", - "dependencies": { - "p-defer": "^1.0.0" - }, - "engines": { - "node": ">=6" - } - }, - "node_modules/mem": { - "version": "4.3.0", - "resolved": "https://registry.npmjs.org/mem/-/mem-4.3.0.tgz", - "integrity": "sha512-qX2bG48pTqYRVmDB37rn/6PT7LcR8T7oAX3bf99u1Tt1nzxYfxkgqDwUwolPlXweM0XzBOBFzSx4kfp7KP1s/w==", - "license": "MIT", - "dependencies": { - "map-age-cleaner": "^0.1.1", - "mimic-fn": "^2.0.0", - "p-is-promise": "^2.0.0" - }, - "engines": { - "node": ">=6" - } - }, "node_modules/merge-stream": { "version": "2.0.0", "resolved": "https://registry.npmjs.org/merge-stream/-/merge-stream-2.0.0.tgz", @@ -6686,6 +6659,7 @@ "version": "2.1.0", "resolved": "https://registry.npmjs.org/mimic-fn/-/mimic-fn-2.1.0.tgz", "integrity": "sha512-OqbOk5oEQeAZ8WXWydlu9HJjz9WVdEIvamMCcXmuqUYjTknH/sqsWvhQ3vgwKFRR1HpjvNBKQ37nbJgYzGqGcg==", + "dev": true, "license": "MIT", "engines": { "node": ">=6" @@ -6876,24 +6850,6 @@ } } }, - "node_modules/p-defer": { - "version": "1.0.0", - "resolved": "https://registry.npmjs.org/p-defer/-/p-defer-1.0.0.tgz", - "integrity": "sha512-wB3wfAxZpk2AzOfUMJNL+d36xothRSyj8EXOa4f6GMqYDN9BJaaSISbsk+wS9abmnebVw95C2Kb5t85UmpCxuw==", - "license": "MIT", - "engines": { - "node": ">=4" - } - }, - "node_modules/p-is-promise": { - "version": "2.1.0", - "resolved": "https://registry.npmjs.org/p-is-promise/-/p-is-promise-2.1.0.tgz", - "integrity": "sha512-Y3W0wlRPK8ZMRbNq97l4M5otioeA5lm1z7bkNkxCka8HSPjR0xRWmpCmc9utiaLP9Jb1eD8BgeIxTW4AIF45Pg==", - "license": "MIT", - "engines": { - "node": ">=6" - } - }, "node_modules/p-limit": { "version": "3.1.0", "resolved": "https://registry.npmjs.org/p-limit/-/p-limit-3.1.0.tgz", From b1e124957af5505e87a0237a4e8c61c1ed3cf724 Mon Sep 17 00:00:00 2001 From: Diego Mello Date: Wed, 12 Aug 2026 20:45:58 -0300 Subject: [PATCH 2/4] chore: restore the lockfile an npm run rewrote --- package-lock.json | 46 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/package-lock.json b/package-lock.json index 458b9e45..10271bdb 100644 --- a/package-lock.json +++ b/package-lock.json @@ -10,6 +10,7 @@ "license": "MIT", "dependencies": { "js-sha256": "^0.9.0", + "mem": "^4.0.0", "tiny-events": "^1.0.1", "universal-websocket-client": "^1.0.2" }, @@ -6648,6 +6649,32 @@ "tmpl": "1.0.5" } }, + "node_modules/map-age-cleaner": { + "version": "0.1.3", + "resolved": "https://registry.npmjs.org/map-age-cleaner/-/map-age-cleaner-0.1.3.tgz", + "integrity": "sha512-bJzx6nMoP6PDLPBFmg7+xRKeFZvFboMrGlxmNj9ClvX53KrmvM5bXFXEWjbz4cz1AFn+jWJ9z/DJSz7hrs0w3w==", + "license": "MIT", + "dependencies": { + "p-defer": "^1.0.0" + }, + "engines": { + "node": ">=6" + } + }, + "node_modules/mem": { + "version": "4.3.0", + "resolved": "https://registry.npmjs.org/mem/-/mem-4.3.0.tgz", + "integrity": "sha512-qX2bG48pTqYRVmDB37rn/6PT7LcR8T7oAX3bf99u1Tt1nzxYfxkgqDwUwolPlXweM0XzBOBFzSx4kfp7KP1s/w==", + "license": "MIT", + "dependencies": { + "map-age-cleaner": "^0.1.1", + "mimic-fn": "^2.0.0", + "p-is-promise": "^2.0.0" + }, + "engines": { + "node": ">=6" + } + }, "node_modules/merge-stream": { "version": "2.0.0", "resolved": "https://registry.npmjs.org/merge-stream/-/merge-stream-2.0.0.tgz", @@ -6659,7 +6686,6 @@ "version": "2.1.0", "resolved": "https://registry.npmjs.org/mimic-fn/-/mimic-fn-2.1.0.tgz", "integrity": "sha512-OqbOk5oEQeAZ8WXWydlu9HJjz9WVdEIvamMCcXmuqUYjTknH/sqsWvhQ3vgwKFRR1HpjvNBKQ37nbJgYzGqGcg==", - "dev": true, "license": "MIT", "engines": { "node": ">=6" @@ -6850,6 +6876,24 @@ } } }, + "node_modules/p-defer": { + "version": "1.0.0", + "resolved": "https://registry.npmjs.org/p-defer/-/p-defer-1.0.0.tgz", + "integrity": "sha512-wB3wfAxZpk2AzOfUMJNL+d36xothRSyj8EXOa4f6GMqYDN9BJaaSISbsk+wS9abmnebVw95C2Kb5t85UmpCxuw==", + "license": "MIT", + "engines": { + "node": ">=4" + } + }, + "node_modules/p-is-promise": { + "version": "2.1.0", + "resolved": "https://registry.npmjs.org/p-is-promise/-/p-is-promise-2.1.0.tgz", + "integrity": "sha512-Y3W0wlRPK8ZMRbNq97l4M5otioeA5lm1z7bkNkxCka8HSPjR0xRWmpCmc9utiaLP9Jb1eD8BgeIxTW4AIF45Pg==", + "license": "MIT", + "engines": { + "node": ">=6" + } + }, "node_modules/p-limit": { "version": "3.1.0", "resolved": "https://registry.npmjs.org/p-limit/-/p-limit-3.1.0.tgz", From a9abe5327c28f066033b5806d2967c1e15dfee80 Mon Sep 17 00:00:00 2001 From: Diego Mello Date: Wed, 12 Aug 2026 20:50:01 -0300 Subject: [PATCH 3/4] =?UTF-8?q?refactor:=20address=20review=20=E2=80=94=20?= =?UTF-8?q?flag=20the=20ADR=20amendment,=20disclose=20the=20abandoned-wait?= =?UTF-8?q?=20paths,=20drop=20restated=20comments?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- ...ne-deadline-primitive-for-bounded-waits.md | 28 +++++++++++++++---- lib/drivers/__tests__/ddp.liveness.spec.ts | 11 ++++++++ lib/drivers/ddp.ts | 19 ++++--------- 3 files changed, 40 insertions(+), 18 deletions(-) diff --git a/docs/adr/0005-one-deadline-primitive-for-bounded-waits.md b/docs/adr/0005-one-deadline-primitive-for-bounded-waits.md index aca740af..f95b9904 100644 --- a/docs/adr/0005-one-deadline-primitive-for-bounded-waits.md +++ b/docs/adr/0005-one-deadline-primitive-for-bounded-waits.md @@ -2,6 +2,8 @@ **Status:** Accepted +**Amends:** ADR-0003 + ## Context The Socket in `lib/drivers/ddp.ts` holds four waits it must be able to give up @@ -42,6 +44,12 @@ false when the Deadline expired. Deadline stays on the Probe rather than moving into `send`, so no other caller inherits a reply timeout. +This amends ADR-0003, whose Decision reads "`ping` races its send against a +Deadline of `config.ping`". The chain no longer races a send. It awaits a Probe, +and the Probe carries that same Deadline. Everything ADR-0003 decided about where +the Deadline lives, and about `config.ping` being the one bound the SDK does not +choose for the consuming app, still holds. Only the mechanism moved. + ## Consequences - The `once`/`off` pairing ADR-0002 protects now exists in one place. A fifth @@ -53,8 +61,18 @@ false when the Deadline expired. but the chain no longer consumes an id from the `sent` counter and no longer registers the `disconnected` listener `send` attaches. Nothing reads the counter for anything but uniqueness. -- A chain turn on a transport that refuses the write now reopens at once instead - of waiting for a reply that cannot come. -- The `settled` booleans are gone. Settling is idempotent because the primitive - removes the listener and clears the timer together, so neither racer can reach - the promise twice. +- A chain turn reopens at once, rather than waiting for a reply that cannot come, + whenever the transport will not take the ping — because it threw, or because it + is no longer open. The second case used to reach `waitForOpen` through `send` + and wait up to twice the Reopen interval before failing into `reopen`. +- The `settled` booleans are gone. Settling removes the listener and clears the + timer together, so neither racer reaches the promise a second time. Where a + wait is abandoned in the same tick as its event arrives, the second `off` is a + miss — harmless only because ADR-0002 made a missed `off` a no-op. This + primitive depends on that guarantee; read ADR-0002 before replacing the + emitter. +- The reopen promise is now cleared a microtask after the `open` arrives rather + than synchronously inside the emit. A caller that asks for an immediate + reconnect from inside an `open` listener therefore joins the reopen that is + settling instead of starting a new one. That was already true of any listener + registered ahead of the old cleanup; it is now true of all of them. diff --git a/lib/drivers/__tests__/ddp.liveness.spec.ts b/lib/drivers/__tests__/ddp.liveness.spec.ts index ef075768..1fa4bf35 100644 --- a/lib/drivers/__tests__/ddp.liveness.spec.ts +++ b/lib/drivers/__tests__/ddp.liveness.spec.ts @@ -247,6 +247,17 @@ describe('Socket liveness', () => { expect(fakeSockets).toHaveLength(1) }) + it('reopens without waiting out the deadline when the socket is no longer open', async () => { + // This turn used to go out through `send`, which waited on `open` for + // twice the reopen interval before failing into the reopen. A socket that + // cannot be written to is dead now, not two reopen intervals from now. + transport.readyState = CLOSED + + await tickWithoutPong() + + expect(socket.openTimeout).toBeDefined() + }) + it('reconnects when one pong is withheld', async () => { // This test used to pin the opposite: the chain died for good, because the // ping's send went out while `connected` was still true, waited forever on diff --git a/lib/drivers/ddp.ts b/lib/drivers/ddp.ts index 88752763..ae8aeeca 100644 --- a/lib/drivers/ddp.ts +++ b/lib/drivers/ddp.ts @@ -261,15 +261,12 @@ export class Socket extends SDKEventEmitter { /** * The one bounded wait every other wait in the connection lifecycle is built - * from: listen for `event`, give up on the Deadline, and detach the listener - * and clear the timer whichever arrives first. Resolves true when the event - * arrived and false when the Deadline expired. + * from. Resolves true when the event arrived, false when the Deadline expired. * * `start` runs with the listener already attached, so an event answered in the * same tick as the write cannot be missed; returning false from it abandons - * the wait without leaving the timer or the listener behind. Pairing `once` - * with the matching `off` is what ADR-0002 hardened the emitter for, and this - * is the only place the SDK now writes that pairing. + * the wait. Pairing `once` with the matching `off` is what ADR-0002 hardened + * the emitter for, and this is the only place the SDK now writes that pairing. */ private awaitEvent = ( event: string, @@ -319,7 +316,6 @@ export class Socket extends SDKEventEmitter { return this.reopenPromise } - /** Write a bare ping frame, reporting whether the transport took it. */ private writePing = (): boolean => { if (!this.connection || this.connection.readyState !== 1) return false try { @@ -427,12 +423,9 @@ export class Socket extends SDKEventEmitter { /** * The Liveness chain: one Probe per interval, repeated for as long as the - * server keeps answering, and a Reopen the moment it stops. - * - * The Probe carries the Deadline the chain depends on. Without one the chain - * waits on a pong a dead socket never sends, stops there, and never reaches - * `reopen`. The Deadline belongs to the Probe rather than to `send`, so no - * other caller inherits a reply timeout. + * server keeps answering, and a Reopen the moment it stops. The Deadline the + * chain depends on belongs to the Probe rather than to `send`, so no other + * caller inherits a reply timeout — see ADR-0003 and ADR-0005. */ ping = async () => { this.pingTimeout && clearTimeout(this.pingTimeout as any) From a2b7b58c70692f8ef839b5e20a34c2d4288abbe0 Mon Sep 17 00:00:00 2001 From: Diego Mello Date: Thu, 13 Aug 2026 12:09:06 -0300 Subject: [PATCH 4/4] refactor: sharpen the Deadline primitive and pin what the ADR claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The wait now answers one question — did the event arrive in time — and only a refused write is a signal, so a `start` with nothing to report returns nothing. `reopenNow` holds the same promise it returns by construction rather than by statement order, and the Deadline timer is declared before the code that clears it. Two consequences the ADR stated were unenforced: a chain turn waiting out its Deadline when the connection drops after the ping is written, and a reconnect staying joinable from an `open` listener. Both are pinned, and both fail on the change that would undo them. `SDKEventEmitter` gains `listenerCount`, so the tests that check a wait left no listener behind stop counting by deleting. --- ...ne-deadline-primitive-for-bounded-waits.md | 20 ++++++-- lib/__tests__/emitter.spec.ts | 32 +++++++++++++ lib/drivers/__tests__/ddp.connection.spec.ts | 22 +++++++++ lib/drivers/__tests__/ddp.liveness.spec.ts | 47 ++++++++++++------- lib/drivers/ddp.ts | 35 +++++++------- lib/emitter.ts | 5 ++ 6 files changed, 120 insertions(+), 41 deletions(-) diff --git a/docs/adr/0005-one-deadline-primitive-for-bounded-waits.md b/docs/adr/0005-one-deadline-primitive-for-bounded-waits.md index f95b9904..4c1c0ac4 100644 --- a/docs/adr/0005-one-deadline-primitive-for-bounded-waits.md +++ b/docs/adr/0005-one-deadline-primitive-for-bounded-waits.md @@ -31,13 +31,17 @@ the same class of fault. The Socket has one private bounded wait, `awaitEvent(event, deadlineMs, start)`. It listens for the event, gives up on the Deadline, and detaches the listener and -clears the timer on either outcome. It resolves true when the event arrived and -false when the Deadline expired. +clears the timer on either outcome. It answers one question — did the event +arrive in time — as true or false, and nothing else. - `start` runs with the listener already attached, so a server that answers in the same tick as the write cannot be missed. Returning false from `start` - abandons the wait, leaving neither timer nor listener behind. This is how a - Probe reports a transport that refused the write. + abandons the wait, leaving neither timer nor listener behind, and short + circuits to that same false answer. This is how a Probe reports a transport + that refused the write: no caller distinguishes a refused write from an + expired Deadline, because both mean the Socket cannot be trusted and both + lead to a Reopen. Only a refusal is a signal, so a `start` with nothing to + report returns nothing. - `reopenNow`, `probe` and `waitForOpen` are each expressed in terms of it. - The Liveness chain is expressed as a repeated Probe: probe on the interval, schedule the next turn when the server answers, Reopen when it does not. The @@ -65,6 +69,10 @@ choose for the consuming app, still holds. Only the mechanism moved. whenever the transport will not take the ping — because it threw, or because it is no longer open. The second case used to reach `waitForOpen` through `send` and wait up to twice the Reopen interval before failing into `reopen`. +- The mirror of that is a connection that drops *after* the ping was written. The + turn holds a Deadline and a pong listener, and nothing that watches for the + drop, so it waits the Deadline out before reopening where `send`'s + `disconnected` listener would have failed it immediately. - The `settled` booleans are gone. Settling removes the listener and clears the timer together, so neither racer reaches the promise a second time. Where a wait is abandoned in the same tick as its event arrives, the second `off` is a @@ -75,4 +83,6 @@ choose for the consuming app, still holds. Only the mechanism moved. than synchronously inside the emit. A caller that asks for an immediate reconnect from inside an `open` listener therefore joins the reopen that is settling instead of starting a new one. That was already true of any listener - registered ahead of the old cleanup; it is now true of all of them. + registered ahead of the old cleanup; it is now true of all of them. The field + holds the same object `reopenNow` returns, by construction rather than by + statement order, so a joiner can be identified by identity. diff --git a/lib/__tests__/emitter.spec.ts b/lib/__tests__/emitter.spec.ts index ff5f80f5..4295038c 100644 --- a/lib/__tests__/emitter.spec.ts +++ b/lib/__tests__/emitter.spec.ts @@ -79,6 +79,38 @@ describe('SDKEventEmitter.removeAllListeners', () => { }) }) +describe('SDKEventEmitter.listenerCount', () => { + let emitter: SDKEventEmitter + + beforeEach(() => { + emitter = new SDKEventEmitter() + }) + + it('counts the listeners of one event, an unfired `once` among them', () => { + emitter.on('greeting', jest.fn()) + emitter.once('greeting', jest.fn()) + emitter.on('unrelated', jest.fn()) + + expect(emitter.listenerCount('greeting')).toBe(2) + }) + + it('returns 0 for an event that was never registered', () => { + expect(emitter.listenerCount('never-registered')).toBe(0) + }) + + it('can be asked twice, then follows a real removal down to 0', () => { + const listener = jest.fn() + emitter.on('greeting', listener) + + expect(emitter.listenerCount('greeting')).toBe(1) + expect(emitter.listenerCount('greeting')).toBe(1) + + emitter.off('greeting', listener) + + expect(emitter.listenerCount('greeting')).toBe(0) + }) +}) + /** * `off` and `emit` are overridden for the same reason: `tiny-events` mutates the * listener array by index, and both of its index bugs are silent — a listener diff --git a/lib/drivers/__tests__/ddp.connection.spec.ts b/lib/drivers/__tests__/ddp.connection.spec.ts index 1887646e..fbbbf631 100644 --- a/lib/drivers/__tests__/ddp.connection.spec.ts +++ b/lib/drivers/__tests__/ddp.connection.spec.ts @@ -188,6 +188,28 @@ describe('Socket connection lifecycle', () => { await expect(reopening).resolves.toBeUndefined() expect(socket.reopenPromise).toBeUndefined() }) + + it('is still joinable from an open listener that runs after the wait settles', async () => { + const reopening = socket.reopenNow() + + // Registered after `reopenNow`, so it runs after the listener the wait + // itself attached — the worst case for a cleanup that ran inside the emit. + let joined: Promise | undefined + let socketsWhenNotified = 0 + socket.on('open', () => { + joined = socket.reopenNow() + socketsWhenNotified = fakeSockets.length + }) + + await driveToHandshake(fakeSockets[1]) + + // Identity, not equality: two distinct promises serialise the same, so + // only `toBe` catches a second Reopen being started here. + expect(joined).toBe(reopening) + expect(socketsWhenNotified).toBe(2) + + await reopening + }) }) describe('a transport constructor that throws', () => { diff --git a/lib/drivers/__tests__/ddp.liveness.spec.ts b/lib/drivers/__tests__/ddp.liveness.spec.ts index 1fa4bf35..6b01c6be 100644 --- a/lib/drivers/__tests__/ddp.liveness.spec.ts +++ b/lib/drivers/__tests__/ddp.liveness.spec.ts @@ -173,10 +173,9 @@ describe('Socket liveness', () => { await expect(socket.probe(2000)).resolves.toBe(false) - // Only the ping chain's timer: the probe left neither a deadline nor a - // listener behind on the way out. + // Only the ping chain's timer. expect(jest.getTimerCount()).toBe(1) - expect(socket.removeAllListeners('pong')).toHaveLength(0) + expect(socket.listenerCount('pong')).toBe(0) }) it('succeeds when the pong lands after the clock has moved', async () => { @@ -226,13 +225,11 @@ describe('Socket liveness', () => { }) it('leaves no pong listener behind, however many turns it runs', async () => { - // The chain is a probe per turn, and a probe listens for one pong. A turn - // that failed to detach would pile listeners up on a long-lived socket. for (let turn = 1; turn <= 5; turn += 1) { await tickWithPong() } - expect(socket.removeAllListeners('pong')).toHaveLength(0) + expect(socket.listenerCount('pong')).toBe(0) }) it('reopens without waiting out the deadline when the transport refuses the ping write', async () => { @@ -240,17 +237,13 @@ describe('Socket liveness', () => { await tickWithoutPong() - // The reopen is already scheduled, with no time advanced past the ping - // interval — a socket that cannot even be written to is dead now, not at - // the deadline. + // No time has advanced past the ping interval, so the scheduled reopen is + // the whole assertion. expect(socket.openTimeout).toBeDefined() expect(fakeSockets).toHaveLength(1) }) it('reopens without waiting out the deadline when the socket is no longer open', async () => { - // This turn used to go out through `send`, which waited on `open` for - // twice the reopen interval before failing into the reopen. A socket that - // cannot be written to is dead now, not two reopen intervals from now. transport.readyState = CLOSED await tickWithoutPong() @@ -258,18 +251,36 @@ describe('Socket liveness', () => { expect(socket.openTimeout).toBeDefined() }) + it('waits out the deadline when the connection drops after the ping is written', async () => { + // The mirror of the two above. The write landed, so the turn holds a + // Deadline and nothing else: a Probe listens for its pong, not for the + // 'disconnected' a send would have listened for. + await jest.advanceTimersToNextTimerAsync() + expect(transport.lastSent()).toEqual({ msg: 'ping' }) + + transport.readyState = CLOSED + socket.emit('disconnected') + await jest.advanceTimersByTimeAsync(0) + + // The drop alone reopens nothing: a send would have rejected on it here. + expect(socket.openTimeout).toBeUndefined() + + await jest.advanceTimersByTimeAsync(PING_INTERVAL - 1) + + expect(socket.openTimeout).toBeUndefined() + + await jest.advanceTimersByTimeAsync(1) + + expect(socket.openTimeout).toBeDefined() + }) + it('reconnects when one pong is withheld', async () => { - // This test used to pin the opposite: the chain died for good, because the - // ping's send went out while `connected` was still true, waited forever on - // a pong reply, and the `.catch(() => this.reopen())` behind it never ran. - // `ping` now races its send against a deadline of its own, so the withheld - // pong is what triggers the reconnect rather than what prevents it. await tickWithPong() await tickWithPong() await tickWithoutPong() - // The ping's own deadline, the one timer the unanswered send leaves behind. + // The Probe's Deadline, the one timer an unanswered ping leaves behind. expect(jest.getTimerCount()).toBe(1) // One millisecond past the deadline, which is also one past the aliveness diff --git a/lib/drivers/ddp.ts b/lib/drivers/ddp.ts index ae8aeeca..30f57763 100644 --- a/lib/drivers/ddp.ts +++ b/lib/drivers/ddp.ts @@ -144,7 +144,7 @@ export class Socket extends SDKEventEmitter { support: ['1', 'pre2', 'pre1'] }) this.session = connected.session - this.ping().catch((err) => this.logger.error(`[ddp] Unable to ping server: ${err.message}`)) + this.ping() this.emit('open') return callback(this.connection) } @@ -261,19 +261,19 @@ export class Socket extends SDKEventEmitter { /** * The one bounded wait every other wait in the connection lifecycle is built - * from. Resolves true when the event arrived, false when the Deadline expired. - * - * `start` runs with the listener already attached, so an event answered in the - * same tick as the write cannot be missed; returning false from it abandons - * the wait. Pairing `once` with the matching `off` is what ADR-0002 hardened - * the emitter for, and this is the only place the SDK now writes that pairing. + * from. `start` runs with the listener already attached, so an event answered + * in the same tick as the write cannot be missed, and returning false from it + * abandons the wait. The `once`/`off` pairing this writes is what ADR-0002 + * hardened the emitter for. */ private awaitEvent = ( event: string, deadlineMs: number, - start: () => boolean = () => true + start: () => boolean | void = () => {} ): Promise => { return new Promise(resolve => { + let deadline: NodeJS.Timer | number + const settle = (arrived: boolean) => { this.off(event, onArrival) clearTimeout(deadline as any) @@ -282,11 +282,11 @@ export class Socket extends SDKEventEmitter { const onArrival = () => settle(true) - const deadline = setTimeout(() => settle(false), deadlineMs) + deadline = setTimeout(() => settle(false), deadlineMs) this.once(event, onArrival) - if (!start()) settle(false) + if (start() === false) settle(false) }) } @@ -306,14 +306,14 @@ export class Socket extends SDKEventEmitter { this.lastPing = 0 this.emit('disconnected') - this.reopenPromise = this.awaitEvent('open', reopenNowDeadline, () => { + const reopening = this.awaitEvent('open', reopenNowDeadline, () => { this.createConnection().catch(() => {}) - return true }).then(() => { delete this.reopenPromise }) + this.reopenPromise = reopening - return this.reopenPromise + return reopening } private writePing = (): boolean => { @@ -422,12 +422,11 @@ export class Socket extends SDKEventEmitter { } /** - * The Liveness chain: one Probe per interval, repeated for as long as the - * server keeps answering, and a Reopen the moment it stops. The Deadline the - * chain depends on belongs to the Probe rather than to `send`, so no other - * caller inherits a reply timeout — see ADR-0003 and ADR-0005. + * The Liveness chain. The Deadline it depends on belongs to the Probe rather + * than to `send`, so no other caller inherits a reply timeout — see ADR-0003 + * and ADR-0005. */ - ping = async () => { + ping = () => { this.pingTimeout && clearTimeout(this.pingTimeout as any) this.pingTimeout = setTimeout(async () => { if (await this.probe(this.config.ping)) return this.ping() diff --git a/lib/emitter.ts b/lib/emitter.ts index 670352da..eb553907 100644 --- a/lib/emitter.ts +++ b/lib/emitter.ts @@ -71,6 +71,11 @@ export class SDKEventEmitter extends EventEmitter { return this } + /** How many listeners the event holds, an unfired `once` counting as one. */ + listenerCount (event: string): number { + return (this._listeners[event] || []).length + } + /** Drop the listeners for one event, or for every event, and return them. */ removeAllListeners (event?: string): Function[] { if (event) {