From 8e4d472f72de89f50cb1b13ced522284b8c711e8 Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Mon, 31 Aug 2026 22:45:29 +0200 Subject: [PATCH] Back off from a link that dies as soon as it comes up MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A bind that returns is not proof the link works: an unreadable stream is only found afterwards, so resetting the delay there handed a link that died on arrival a fresh minDelay every cycle — one connect and bind per second forever, which is how an account gets blocked for bind flooding. The loop resets only once a link has outlasted maxDelay. Also covers the unreadable stream that has no loop to retry it. --- AGENTS.md | 21 ++++++++++------- README.md | 2 +- src/pdu-transport.ts | 2 +- src/reconnect-loop.ts | 13 +++++++++- test/session-extras.test.ts | 7 +++--- test/session.test.ts | 47 +++++++++++++++++++++++++++++++++++++ 6 files changed, 78 insertions(+), 14 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index a6ecdfd..4e6633a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -296,19 +296,24 @@ exactly 140. that loop before tearing down, so every deliberate shutdown emits `close`. Without the split an application that opens a replacement client on `close` ends up holding two binds on one account. +- **Only a link that outlasted `maxDelay` resets the backoff.** Coming up is not proof it works: an + unreadable stream is found after the bind returns, so resetting there gave a link that died on + arrival a fresh `minDelay` every cycle — one TCP connect and bind per second, forever, which is how + an account gets blocked for bind flooding. `ReconnectLoop` records when it brought the owner up and + resets only if the link then lasted longer than the longest wait it would ever schedule. A drop + after a healthy link still retries at `minDelay`. + - **A stream this library cannot read is a dead link, not a dead session.** Maintainer's call, 2026-08-31: a framing or codec error tears the link down through `teardown()`, so the reconnect loop retries it on a fresh socket with a fresh framer — which is what a desynced stream needs, and the common cause. `sessionError` still carries every failure, so a peer that only ever sends - garbage is visible in the log rather than silent, and the backoff caps the retries at one per - `maxDelay`. Ending the session outright was inherited from when reconnect was opt-in, where the - distinction could not arise. + garbage is visible in the log rather than silent, and the backoff grows to one attempt per + `maxDelay`. -- **`disconnected` counts failed links, not outages.** A retry that opens a socket and then loses its - bind re-enters `attach()` and so emits again, which makes it deliberately not one-to-one with - `reconnected`: each emission is a link that went down, and suppressing the later ones would leave a - failed rebind with nothing but a log line. The README says so, because the pairing is what a reader - would otherwise assume. +- **`disconnected` counts failed links, not outages.** A retry that opens a socket and then loses it + clears `closed` through `attach()`, so the next `teardown()` emits again: each emission is a link + that went down, which makes the event deliberately not one-to-one with `reconnected`. Suppressing + the later ones would leave a failed rebind with nothing but a log line. - **`session.sock` is a getter over `PduTransport`, and stays public.** Maintainer's call, 2026-08-31: `session.ts` had reached its line cap, so the socket-to-PDU seam todo.md named was opened — diff --git a/README.md b/README.md index 3c9f8ea..264f756 100644 --- a/README.md +++ b/README.md @@ -104,7 +104,7 @@ Every one is optional. | `shutdownTimeout` | `5000` | How long `close()` and `unbind()` wait for the requests this end already sent; `0` waits forever. | | `maxOutstanding` | `10` | Requests allowed on the wire at once; further sends queue. | | `smsIdFormat` | — | The notation the SMSC writes message ids in, per place it writes them: `{ receipt: 'decimal', submitResp: 'hex' }`. Only needed where the two disagree. | -| `reconnect` | on | Re-binds after a drop or an idle timeout, backing off from `minDelay` 1 s to `maxDelay` 30 s. `{ minDelay, maxDelay }` retunes it; `false` turns it off, so a drop ends the session. | +| `reconnect` | on | Re-binds after a drop, an idle timeout, or a stream the library cannot read, backing off from `minDelay` 1 s to `maxDelay` 30 s. `{ minDelay, maxDelay }` retunes it; `false` turns it off, so a drop ends the session. | | `log` | silent | Any object with `debug`, `error`, `info`, `verbose` and `warn` methods — see [Logging](#logging). | | `signal` | — | An `AbortSignal` that cancels connecting and tears the session down. | diff --git a/src/pdu-transport.ts b/src/pdu-transport.ts index d27a823..b1f74c0 100644 --- a/src/pdu-transport.ts +++ b/src/pdu-transport.ts @@ -14,7 +14,7 @@ export type PduTransportOptions = { /** A complete PDU, before it is parsed. */ onFramed: (pdu: Buffer) => void; onPdu: (pduObj: PduObject) => void; - /** Nothing further can be read off this stream; the socket has to go. */ + /** Nothing further can be read off this stream, whatever the socket does next. */ onUnreadable: (err: Error) => void; }; diff --git a/src/reconnect-loop.ts b/src/reconnect-loop.ts index 0c6ec13..efa5306 100644 --- a/src/reconnect-loop.ts +++ b/src/reconnect-loop.ts @@ -7,19 +7,23 @@ export type ReconnectLoopOptions = { log: SmppLog; maxDelay: number; minDelay: number; + now?: (() => number) | undefined; /** Brings the owner back up on a freshly opened socket. An err means try again. */ onConnected: (sock: Socket) => Promise; }; /** Reopens a dropped connection, backing off between attempts until it is told to stop. */ export class ReconnectLoop { + private readonly now: () => number; private readonly options: ReconnectLoopOptions; private attempting = false; private delay: number; private halted = false; private timer: NodeJS.Timeout | undefined; + private upAt: number | undefined; constructor(options: ReconnectLoopOptions) { + this.now = options.now ?? Date.now; this.options = options; this.delay = options.minDelay; } @@ -32,6 +36,13 @@ export class ReconnectLoop { schedule(): void { if (this.timer || this.attempting || this.isStopped()) return; + // Coming up is not proof: a stream we cannot read is only found once the link is bound. + if (this.upAt !== undefined && this.now() - this.upAt >= this.options.maxDelay) { + this.delay = this.options.minDelay; + } + + this.upAt = undefined; + const delay = this.delay; this.options.log.info('reconnect - retrying after a drop', { delay }); @@ -98,7 +109,7 @@ export class ReconnectLoop { return true; } - this.delay = this.options.minDelay; + this.upAt = this.now(); return false; } diff --git a/test/session-extras.test.ts b/test/session-extras.test.ts index d1da3af..fe709eb 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -344,7 +344,7 @@ describe('reconnect', () => { test('reports a drop it will retry as disconnected, keeping close for the end', async t => { const smpp = await startServer(t); - const { session } = await connect(t, smpp); + const { session } = await connect(t, smpp, { reconnect: { maxDelay: 100, minDelay: 20 } }); assert.ok(session); @@ -391,7 +391,7 @@ describe('reconnect', () => { test('re-binds after a stream it cannot read, rather than ending the session', async t => { const smpp = await startServer(t); - const { session } = await connect(t, smpp); + const { session } = await connect(t, smpp, { reconnect: { maxDelay: 100, minDelay: 20 } }); assert.ok(session); @@ -448,7 +448,8 @@ describe('reconnect', () => { const closed = once(resolve => { session.on('close', () => { resolve(true); }); }); - await peerOf(smpp).close(); + // A cmd_length below the 16-octet header: unreadable, and nothing left to retry it. + peerOf(smpp).sock.write(Buffer.from([0, 0, 0, 4, 0, 0, 0, 4, 0, 0, 0, 0, 0, 0, 0, 1])); await closed; assert.equal(disconnects, 0); diff --git a/test/session.test.ts b/test/session.test.ts index 5165d1e..3579c4b 100644 --- a/test/session.test.ts +++ b/test/session.test.ts @@ -4,6 +4,7 @@ import test, { describe } from 'node:test'; import type { Dlr } from '../src/dlr.ts'; import type { PduObject, PduObjectInput } from '../src/pdu.ts'; import type { Sms } from '../src/sms.ts'; +import type { SmppLog } from '../src/log.ts'; import type { SmppServer } from '../src/server.ts'; import type { TestContext } from 'node:test'; import type { VoidResult } from '../src/result.ts'; @@ -1372,6 +1373,52 @@ describe('application hooks that throw or reject', () => { assert.ok(retried, 'a throwing connect should be retried, not left for the process to die on'); }); + test('keeps backing off when every link dies as soon as it comes up', async t => { + // A stream we cannot read is found after the bind, so a bind alone must not prove the link. + const clock = { now: 0 }; + const delays: number[] = []; + const noop = (): void => undefined; + const log: SmppLog = { + debug: noop, + error: noop, + info: (msg, metadata) => { + if (msg === 'reconnect - retrying after a drop') delays.push(Number(metadata?.delay)); + }, + verbose: noop, + warn: noop, + }; + let up = 0; + const loop = new ReconnectLoop({ + connect: () => Promise.resolve({ sock: new net.Socket() }), + log, + maxDelay: 80, + minDelay: 10, + now: () => clock.now, + onConnected: () => { + up++; + + return Promise.resolve({}); + }, + }); + + t.after(() => { loop.stop(); }); + + for (let died = 0; died < 4; died++) { + loop.schedule(); + await waitFor(() => up === died + 1); + await delay(5); + } + + assert.deepEqual(delays, [10, 20, 40, 80]); + + // A link that outlasted the longest wait earned a fresh start. + clock.now += 80; + loop.schedule(); + await waitFor(() => delays.length === 5); + + assert.deepEqual(delays, [10, 20, 40, 80, 10]); + }); + test('starts only one reconnect attempt at a time', async t => { let attempts = 0; let finish: (() => void) | undefined;