From 18177053bcbb93773bec4c983dbdd63c955ea96a Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Mon, 31 Aug 2026 20:18:29 +0200 Subject: [PATCH 01/19] Re-bind a dropped client link by default, and take false to turn it off Surviving a drop is most of what the session layer is for, and behind an opt-in an application that never read the options table got none of it. `reconnect` still takes `{ minDelay, maxDelay }` to retune the backoff; `false` is the one spelling for off. --- AGENTS.md | 8 ++++++++ README.md | 8 ++++---- src/client.ts | 28 ++++++++++++++++------------ test/session-extras.test.ts | 37 +++++++++++++++++++++++++++++++++++++ todo.md | 4 ---- 5 files changed, 65 insertions(+), 20 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 1ebe11f..af3fa53 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -288,6 +288,14 @@ exactly 140. and fail on every developer machine, and a committed key leaks in a public repository. Valid while the dev image has no openssl. +- **A client re-binds after a drop unless it is told not to.** Maintainer's call, 2026-08-31: + surviving a dropped link is most of what the session layer is for, and behind an opt-in an + application that never read the options table got none of it. `reconnect` takes + `{ minDelay, maxDelay }` to retune the backoff and `false` to turn it off, so absent means on and + there is one spelling for each. Only `client()` reconnects — a `server()` session is a connection + the peer opened, and nothing at this end can reopen it. The retry timer is `unref()`'d, so a + process with nothing else left to do still exits between attempts. + - **The notation a peer writes message ids in is named per place, and normalisation never reaches inside a `-` id.** An SMSC may answer `submit_sm_resp` in hex and write the receipt's `id:` in decimal, so one transform over both sides cannot make them equal — `smsIdFormat` names diff --git a/README.md b/README.md index ddb8833..49a3d15 100644 --- a/README.md +++ b/README.md @@ -17,7 +17,7 @@ by hand on top of a library; it is built in here. | | | | --- | --- | | **Keepalive** | `enquire_link` every 20 s on a quiet link, and a peer that stops answering is dropped. | -| **Reconnect with backoff** | Opt-in `reconnect` reopens the socket and re-binds, 1 s doubling to 30 s. | +| **Reconnect with backoff** | A dropped client link reopens the socket and re-binds by default, 1 s doubling to 30 s. | | **Submit window** | `maxOutstanding` holds requests in flight at 10; further sends queue instead of overrunning the SMSC. | | **Delivery receipts** | Correlated by `receipted_message_id`/`message_state` where the SMSC sends them, falling back to parsing the receipt text — what Kannel and several others send. | | **Multipart** | Long messages split on send; concatenated `deliver_sm` reassembled into one `sms`. | @@ -99,12 +99,12 @@ Every one is optional. | `systemType`, `addressRange`, `addrTon`, `addrNpi` | `''`, `''`, `0`, `0` | The remaining bind fields, for operators that require them. | | `tls` | `false` | `true` for defaults, or a `tls.ConnectionOptions` object for a private CA or a client certificate. | | `enquireLinkInterval` | `20000` | How often to send `enquire_link` on a quiet link. | -| `idleTimeout` | `2 × enquireLinkInterval` | Give up on a link the peer has stopped answering; with `reconnect` set, it re-binds. | +| `idleTimeout` | `2 × enquireLinkInterval` | Give up on a link the peer has stopped answering, and re-bind unless `reconnect` is `false`. | | `responseTimeout` | `30000` | How long to wait for a response before giving up on it; `0` waits forever. | | `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` | off | `{ minDelay, maxDelay }` to re-bind automatically after a drop or an idle timeout, with exponential backoff. | +| `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. | | `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. | @@ -317,7 +317,7 @@ TypeScript users can import `SmppLog` to have the compiler check one. | `dlr` | A delivery report arrives, one per segment. `smsId` is undefined when the peer marked a receipt whose body carries no readable id. `statusMsg` names `statusId` unless the peer sent a `message_state` this library cannot name — then `statusId` is that raw value and `statusMsg` is whatever the body said, or `UNKNOWN`. | | `messageDlr` | Every segment of a multipart message sent with `dlr: true` has been reported on, carrying the worst status of the segments. Merging needs the SMSC to number its segment ids `-`, which is this library's own server's convention — an SMSC that hands out unrelated ids per segment never fires it. A base is merged once: a later message the SMSC gives the same ids is reported on through `dlr` alone, and an earlier one still collecting loses its merged report as well. | | `close` | The connection closed. | -| `reconnected` | The client re-bound after a drop (only with `reconnect` configured). | +| `reconnected` | The client re-bound after a drop. Never fires with `reconnect: false`. | | `sessionError` | Something failed on a live session, including a hook or listener that threw or, if it was `async`, rejected. | | `data` | Raw bytes arrived on the socket. | | `incomingPdu` | A complete PDU arrived, as a buffer. | diff --git a/src/client.ts b/src/client.ts index 0176f19..20c7fd9 100644 --- a/src/client.ts +++ b/src/client.ts @@ -1,6 +1,6 @@ import type { ConnectionOptions } from 'node:tls'; import type { Result, VoidResult } from './result.ts'; -import type { BindType } from './session-options.ts'; +import type { BindType, ReconnectOptions } from './session-options.ts'; import type { SmppLog } from './log.ts'; import type { SmsIdFormat } from './sms-id.ts'; import type { Socket } from 'node:net'; @@ -26,7 +26,7 @@ export type ClientOptions = { maxOutstanding?: number; password?: string; port?: number; - reconnect?: { maxDelay?: number; minDelay?: number }; + reconnect?: { maxDelay?: number; minDelay?: number } | false; responseTimeout?: number; shutdownTimeout?: number; signal?: AbortSignal; @@ -139,6 +139,19 @@ async function bind(session: Session, options: ClientOptions): Promise openSocket(options), + maxDelay: tuning.maxDelay, + minDelay: tuning.minDelay, + onConnected: reconnected => bind(reconnected, options), + }; +} + function createSession(options: ClientOptions, log: SmppLog, sock: Socket): Session { const enquireLinkInterval = options.enquireLinkInterval ?? defaults.enquireLinkInterval; @@ -147,20 +160,11 @@ function createSession(options: ClientOptions, log: SmppLog, sock: Socket): Sess idleTimeout: options.idleTimeout ?? enquireLinkInterval * defaults.idleTimeoutFactor, log, maxOutstanding: options.maxOutstanding, + reconnect: reconnectFor(options), responseTimeout: options.responseTimeout, shutdownTimeout: options.shutdownTimeout, smsIdFormat: options.smsIdFormat, sock, - ...(options.reconnect - ? { - reconnect: { - connect: () => openSocket(options), - maxDelay: options.reconnect.maxDelay, - minDelay: options.reconnect.minDelay, - onConnected: reconnected => bind(reconnected, options), - }, - } - : {}), }); } diff --git a/test/session-extras.test.ts b/test/session-extras.test.ts index b721965..0aa1f9f 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -326,6 +326,43 @@ describe('sendSms()', () => { }); describe('reconnect', () => { + test('re-binds after a drop with nothing asked for, since it is the default', async t => { + const smpp = await startServer(t); + const { session } = await connect(t, smpp); + + assert.ok(session); + + const reconnected = once(resolve => { session.on('reconnected', () => { resolve(true); }); }); + + await peerOf(smpp).close(); + await reconnected; + + assert.ok(session.loggedIn); + }); + + test('schedules nothing after a drop when reconnect is false', async t => { + const smpp = await startServer(t); + const noop = (): void => undefined; + const infos: string[] = []; + const log: SmppLog = { + debug: noop, + error: noop, + info: msg => { infos.push(msg); }, + verbose: noop, + warn: noop, + }; + const { session } = await connect(t, smpp, { log, reconnect: false }); + + assert.ok(session); + + const closed = once(resolve => { session.on('close', () => { resolve(true); }); }); + + await peerOf(smpp).close(); + await closed; + + assert.ok(!infos.includes('reconnect - retrying after a drop')); + }); + test('re-binds after the connection drops, keeping the same session object', async t => { const smpp = await startServer(t); const messages: string[] = []; diff --git a/todo.md b/todo.md index 4a3b0db..2f0d7d9 100644 --- a/todo.md +++ b/todo.md @@ -168,10 +168,6 @@ session message is a change to every call site. Mirror the `onRequest` seam — return a `Dlr` to own the receipt, `undefined` to fall through to the built-in parser. -- [ ] **Turn `reconnect` on by default in `client()`.** Surviving a dropped link is most of why the - session layer exists, and it is opt-in behind an empty object today, so an application that - does not read the options table gets none of it. A default change, so it needs a decision. - ## Declined - **Throughput throttling — a TPS cap, and backing off on `ESME_RTHROTTLED`.** Two reasons, either From 880454b4f4c5fc38d7a3024344f2e871ec14105d Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Mon, 31 Aug 2026 21:11:19 +0200 Subject: [PATCH 02/19] Split a retried drop out of close, and range-check the backoff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `close` fires only where nothing will bring the session back; a drop the reconnect loop will retry is `disconnected`, pairing with `reconnected`. `minDelay: 0` never doubles, so the backoff never started — both delays are range-checked now, and only `false` spells reconnect off. --- AGENTS.md | 7 +++++ README.md | 5 ++-- src/session-options.ts | 37 ++++++++++++++++++++++-- src/session.ts | 7 ++++- test/session-extras.test.ts | 57 ++++++++++++++++++++++++++++++++++++- test/session.test.ts | 6 +++- 6 files changed, 111 insertions(+), 8 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index af3fa53..d837b4f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -288,6 +288,13 @@ exactly 140. and fail on every developer machine, and a committed key leaks in a public repository. Valid while the dev image has no openssl. +- **`close` means the session is over, and a drop the loop will retry is `disconnected`.** Maintainer's + call, 2026-08-31: with reconnect on by default a `close` on every transient drop left an application + unable to tell a retry from the end, and no second one follows because `teardown()` is a no-op once + `closed`. `teardown()` picks the event by whether the reconnect loop is still live, and `end()` stops + 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. + - **A client re-binds after a drop unless it is told not to.** Maintainer's call, 2026-08-31: surviving a dropped link is most of what the session layer is for, and behind an opt-in an application that never read the options table got none of it. `reconnect` takes diff --git a/README.md b/README.md index 49a3d15..52e2148 100644 --- a/README.md +++ b/README.md @@ -316,8 +316,9 @@ TypeScript users can import `SmppLog` to have the compiler check one. | `sms` | An SMS arrives, reassembled if it was multipart. Carries `sendResp()`, `sendDlr()` and the `smsId` it was answered with. | | `dlr` | A delivery report arrives, one per segment. `smsId` is undefined when the peer marked a receipt whose body carries no readable id. `statusMsg` names `statusId` unless the peer sent a `message_state` this library cannot name — then `statusId` is that raw value and `statusMsg` is whatever the body said, or `UNKNOWN`. | | `messageDlr` | Every segment of a multipart message sent with `dlr: true` has been reported on, carrying the worst status of the segments. Merging needs the SMSC to number its segment ids `-`, which is this library's own server's convention — an SMSC that hands out unrelated ids per segment never fires it. A base is merged once: a later message the SMSC gives the same ids is reported on through `dlr` alone, and an earlier one still collecting loses its merged report as well. | -| `close` | The connection closed. | -| `reconnected` | The client re-bound after a drop. Never fires with `reconnect: false`. | +| `close` | The session is over: you closed it, or the link dropped with `reconnect: false`. Nothing brings it back. | +| `disconnected` | The link dropped and the reconnect loop will retry it. Do not open a replacement client here — the session you hold comes back on its own, and `reconnected` says when. | +| `reconnected` | The client re-bound after a drop. | | `sessionError` | Something failed on a live session, including a hook or listener that threw or, if it was `async`, rejected. | | `data` | Raw bytes arrived on the socket. | | `incomingPdu` | A complete PDU arrived, as a buffer. | diff --git a/src/session-options.ts b/src/session-options.ts index 9905f14..9c47b9b 100644 --- a/src/session-options.ts +++ b/src/session-options.ts @@ -12,6 +12,7 @@ import { isSmsIdNotation, smsIdNotations, smsIdPlaces } from './sms-id.ts'; export type SessionEvents = { close: []; data: [Buffer]; + disconnected: []; dlr: [Dlr, PduObject]; incomingPdu: [Buffer]; incomingPduObj: [PduObject]; @@ -116,28 +117,57 @@ export const defaults = { * queued behind a slot that is never freed, so the call never settles at all. */ export function checkSessionOptions(options: CheckableOptions): VoidResult { - const limits: [string, number, number][] = [ + const checked = checkLimits([ ['idleTimeout', options.idleTimeout ?? 0, 0], ['maxOutstanding', options.maxOutstanding ?? defaults.maxOutstanding, 1], ['maxReassembly', options.maxReassembly ?? defaults.maxReassembly, 1], ['reassemblyTimeout', options.reassemblyTimeout ?? defaults.reassemblyTimeout, 0], ['responseTimeout', options.responseTimeout ?? defaults.responseTimeout, 0], ['shutdownTimeout', options.shutdownTimeout ?? defaults.shutdownTimeout, 0], - ]; + ]); + if (checked.err) return checked; + + const backoff = checkReconnect(options.reconnect); + + return backoff.err ? backoff : checkSmsIdFormat(options.smsIdFormat); +} + +function checkLimits(limits: [string, number, number][]): VoidResult { for (const [name, value, min] of limits) { if (!Number.isInteger(value) || value < min) { return { err: new Error(`${name} must be ${String(min)} or more, got ${String(value)}`) }; } } - return checkSmsIdFormat(options.smsIdFormat); + return {}; +} + +function checkReconnect(reconnect: unknown): VoidResult { + if (reconnect === undefined || reconnect === false) return {}; + + if (!isRecord(reconnect)) { + return { err: new Error('reconnect takes { maxDelay, minDelay }, or false to turn it off') }; + } + + // A minDelay of 0 never doubles, so the backoff never starts and every retry lands at once. + return checkLimits([ + ['maxDelay', delayOr(reconnect.maxDelay, defaults.maxDelay), 1], + ['minDelay', delayOr(reconnect.minDelay, defaults.minDelay), 1], + ]); } function isRecord(value: unknown): value is Record { return typeof value === 'object' && value !== null && !Array.isArray(value); } +/** A tuning value that is not a number lands on NaN, which the range check refuses by name. */ +function delayOr(value: unknown, fallback: number): number { + if (value === undefined) return fallback; + + return typeof value === 'number' ? value : NaN; +} + function checkSmsIdFormat(smsIdFormat: unknown): VoidResult { if (smsIdFormat === undefined) return {}; @@ -167,6 +197,7 @@ export type CheckableOptions = { maxOutstanding?: number | undefined; maxReassembly?: number | undefined; reassemblyTimeout?: number | undefined; + reconnect?: unknown; responseTimeout?: number | undefined; shutdownTimeout?: number | undefined; smsIdFormat?: unknown; diff --git a/src/session.ts b/src/session.ts index e18fd53..1dbb84a 100644 --- a/src/session.ts +++ b/src/session.ts @@ -376,7 +376,12 @@ export class Session extends EventEmitter { this.pending.settleAll(new Error('Session closed before a response arrived')); this.incoming.clear(); this.sock.destroy(); - this.emit('close'); + this.emit(this.retrying() ? 'disconnected' : 'close'); + } + + /** A drop the loop will bring the session back from is a disconnect, not the end of it. */ + private retrying(): boolean { + return this.reconnectLoop !== undefined && !this.reconnectLoop.isStopped(); } private nextConcatReference(): number { diff --git a/test/session-extras.test.ts b/test/session-extras.test.ts index 0aa1f9f..611024e 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -334,10 +334,65 @@ describe('reconnect', () => { const reconnected = once(resolve => { session.on('reconnected', () => { resolve(true); }); }); + const dropped = session.sock; + await peerOf(smpp).close(); await reconnected; - assert.ok(session.loggedIn); + assert.notEqual(session.sock, dropped, 'the session should be live on a fresh socket'); + }); + + 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); + + assert.ok(session); + + const events: string[] = []; + + session.on('close', () => { events.push('close'); }); + session.on('disconnected', () => { events.push('disconnected'); }); + + const reconnected = once(resolve => { session.on('reconnected', () => { resolve(true); }); }); + + await peerOf(smpp).close(); + await reconnected; + + assert.deepEqual(events, ['disconnected'], 'a link the loop brings back is not the end'); + + await session.close(); + + assert.deepEqual(events, ['disconnected', 'close']); + }); + + test('reports a drop as close when nothing will retry it', async t => { + const smpp = await startServer(t); + const { session } = await connect(t, smpp, { reconnect: false }); + + assert.ok(session); + + const events: string[] = []; + + session.on('disconnected', () => { events.push('disconnected'); }); + + const closed = once(resolve => { session.on('close', () => { resolve(true); }); }); + + await peerOf(smpp).close(); + await closed; + + assert.deepEqual(events, []); + }); + + test('refuses a backoff that would retry without pausing', () => { + assert.match(checkSessionOptions({ reconnect: { minDelay: 0 } }).err?.message ?? '', /minDelay/); + assert.match(checkSessionOptions({ reconnect: { maxDelay: -1 } }).err?.message ?? '', /maxDelay/); + assert.match( + checkSessionOptions({ reconnect: null }).err?.message ?? '', + /false/, + 'off is spelled false, so nothing else may stand in for it', + ); + assert.equal(checkSessionOptions({ reconnect: false }).err, undefined); + assert.equal(checkSessionOptions({ reconnect: { maxDelay: 60_000, minDelay: 500 } }).err, undefined); }); test('schedules nothing after a drop when reconnect is false', async t => { diff --git a/test/session.test.ts b/test/session.test.ts index 5565ef5..5165d1e 100644 --- a/test/session.test.ts +++ b/test/session.test.ts @@ -1408,7 +1408,11 @@ describe('application hooks that throw or reject', () => { describe('link timers', () => { test('closes a client link the peer has stopped answering', async t => { const peer = await bindOnlyPeer(t); - const { err, session } = await client({ enquireLinkInterval: 50, port: peer.port }); + const { err, session } = await client({ + enquireLinkInterval: 50, + port: peer.port, + reconnect: false, + }); assert.equal(err, undefined); assert.ok(session); From b2d121b4f3da25b1a7cb310118353f30b6ff91e2 Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Mon, 31 Aug 2026 21:53:11 +0200 Subject: [PATCH 03/19] Emit exactly one close, and read the socket through a PduTransport Ending a session while the link was down emitted nothing: the retried drop had already torn it down, so close() found nothing left to do. The terminal event is now guarded by its own flag rather than by `closed`. The socket, the framer and the parse move to PduTransport, which is what takes session.ts back under its line cap; `session.sock` reads through a getter. The backoff also refuses an inverted pair and an unknown key. --- AGENTS.md | 8 +++ README.md | 2 +- src/pdu-transport.ts | 91 +++++++++++++++++++++++++++++ src/session-options.ts | 21 +++++-- src/session.ts | 110 +++++++++++++----------------------- test/session-extras.test.ts | 29 ++++++++-- todo.md | 3 - 7 files changed, 179 insertions(+), 85 deletions(-) create mode 100644 src/pdu-transport.ts diff --git a/AGENTS.md b/AGENTS.md index d837b4f..ce8c175 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -50,6 +50,7 @@ src/ message.ts Encoding detection, splitting, bit counting, SMPP date formatting pdu.ts pduToObj / objToPdu / pduReturn — synchronous, result-returning pdu-framer.ts PduFramer: a byte stream cut into complete PDUs + pdu-transport.ts PduTransport: the socket a session reads complete PDUs off pending-requests.ts PendingRequests: sequence numbers, correlation, timeout, abort reassembly.ts Reassembler: capped, expiring multipart groups reconnect-loop.ts ReconnectLoop: backoff, retry timer, stopped-ness @@ -295,6 +296,13 @@ 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. +- **`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 — + `PduTransport` owns the socket, the framer and the parse, and hands the session raw bytes, framed + PDUs, parsed ones and an unreadable stream. Reading `session.sock` is unchanged; assigning it no + longer compiles, which never rewired the handlers and so never worked. The transport stays + unpublished like the other collaborators. + - **A client re-binds after a drop unless it is told not to.** Maintainer's call, 2026-08-31: surviving a dropped link is most of what the session layer is for, and behind an opt-in an application that never read the options table got none of it. `reconnect` takes diff --git a/README.md b/README.md index 52e2148..8e88f4f 100644 --- a/README.md +++ b/README.md @@ -316,7 +316,7 @@ TypeScript users can import `SmppLog` to have the compiler check one. | `sms` | An SMS arrives, reassembled if it was multipart. Carries `sendResp()`, `sendDlr()` and the `smsId` it was answered with. | | `dlr` | A delivery report arrives, one per segment. `smsId` is undefined when the peer marked a receipt whose body carries no readable id. `statusMsg` names `statusId` unless the peer sent a `message_state` this library cannot name — then `statusId` is that raw value and `statusMsg` is whatever the body said, or `UNKNOWN`. | | `messageDlr` | Every segment of a multipart message sent with `dlr: true` has been reported on, carrying the worst status of the segments. Merging needs the SMSC to number its segment ids `-`, which is this library's own server's convention — an SMSC that hands out unrelated ids per segment never fires it. A base is merged once: a later message the SMSC gives the same ids is reported on through `dlr` alone, and an earlier one still collecting loses its merged report as well. | -| `close` | The session is over: you closed it, or the link dropped with `reconnect: false`. Nothing brings it back. | +| `close` | The session is over, because nothing will bring the link back. Fires once, whether you closed it or the link failed for good. | | `disconnected` | The link dropped and the reconnect loop will retry it. Do not open a replacement client here — the session you hold comes back on its own, and `reconnected` says when. | | `reconnected` | The client re-bound after a drop. | | `sessionError` | Something failed on a live session, including a hook or listener that threw or, if it was `async`, rejected. | diff --git a/src/pdu-transport.ts b/src/pdu-transport.ts new file mode 100644 index 0000000..6ce41be --- /dev/null +++ b/src/pdu-transport.ts @@ -0,0 +1,91 @@ +import type { PduObject } from './pdu.ts'; +import type { SmppLog } from './log.ts'; +import type { Socket } from 'node:net'; +import type { VoidResult } from './result.ts'; +import { PduFramer } from './pdu-framer.ts'; +import { pduToObj } from './pdu.ts'; + +export type PduTransportOptions = { + log: SmppLog; + onClose: () => void; + /** Raw bytes, before framing. */ + onData: (chunk: Buffer) => void; + onError: (err: Error) => void; + /** A complete PDU, before it is parsed. */ + onFramed: (pdu: Buffer) => void; + onPdu: (pduObj: PduObject) => void; + /** Nothing further can be read off this stream, whatever the socket does next. */ + onUnreadable: (err: Error) => void; +}; + +/** A socket read as a stream of complete PDUs. A reconnect attaches a new socket in its place. */ +export class PduTransport { + private readonly options: PduTransportOptions; + private framer = new PduFramer(); + private socket: Socket; + + constructor(options: PduTransportOptions, sock: Socket) { + this.options = options; + this.socket = sock; + } + + get sock(): Socket { + return this.socket; + } + + /** Wires a freshly opened socket in, replacing any previous one. */ + attach(sock: Socket): void { + // The socket being replaced is already dead, and its three handlers still point here. + if (this.socket !== sock) this.socket.removeAllListeners(); + + this.socket = sock; + this.framer = new PduFramer(); + + sock.on('data', chunk => { this.read(chunk); }); + sock.on('close', () => { this.options.onClose(); }); + sock.on('error', err => { + this.options.log.warn('transport - socket error', { message: err.message }); + this.options.onError(err); + this.options.onClose(); + }); + } + + write(pdu: Buffer): VoidResult { + if (this.socket.destroyed) return { err: new Error('Socket is closed') }; + + this.socket.write(pdu); + + return {}; + } + + private read(chunk: Buffer): void { + this.options.onData(chunk); + this.framer.push(chunk); + + const framed = this.framer.next(); + + if (framed.err) { + this.options.log.warn('transport - unusable stream, closing', { message: framed.err.message }); + this.options.onUnreadable(framed.err); + + return; + } + + for (const pdu of framed.pdus) { + this.options.onFramed(pdu); + + const parsed = pduToObj(pdu); + + if (parsed.err) { + this.options.log.warn('transport - could not parse an incoming PDU, closing', { + message: parsed.err.message, + }); + this.options.onUnreadable(parsed.err); + + return; + } + + this.options.onPdu(parsed.pduObj); + } + } +} diff --git a/src/session-options.ts b/src/session-options.ts index 9c47b9b..adc28e2 100644 --- a/src/session-options.ts +++ b/src/session-options.ts @@ -143,6 +143,8 @@ function checkLimits(limits: [string, number, number][]): VoidResult { return {}; } +const backoffDelays: readonly string[] = ['maxDelay', 'minDelay']; + function checkReconnect(reconnect: unknown): VoidResult { if (reconnect === undefined || reconnect === false) return {}; @@ -150,11 +152,20 @@ function checkReconnect(reconnect: unknown): VoidResult { return { err: new Error('reconnect takes { maxDelay, minDelay }, or false to turn it off') }; } - // A minDelay of 0 never doubles, so the backoff never starts and every retry lands at once. - return checkLimits([ - ['maxDelay', delayOr(reconnect.maxDelay, defaults.maxDelay), 1], - ['minDelay', delayOr(reconnect.minDelay, defaults.minDelay), 1], - ]); + for (const key of Object.keys(reconnect)) { + if (!backoffDelays.includes(key)) { + return { err: new Error(`reconnect has no ${key}, name ${backoffDelays.join(' or ')}`) }; + } + } + + const maxDelay = delayOr(reconnect.maxDelay, defaults.maxDelay); + const minDelay = delayOr(reconnect.minDelay, defaults.minDelay); + // A delay of 0 never doubles, so the backoff never starts and every retry lands at once. + const checked = checkLimits([['maxDelay', maxDelay, 1], ['minDelay', minDelay, 1]]); + + if (checked.err || maxDelay >= minDelay) return checked; + + return { err: new Error(`maxDelay must be minDelay or more, got ${String(maxDelay)}`) }; } function isRecord(value: unknown): value is Record { diff --git a/src/session.ts b/src/session.ts index 1dbb84a..63768bc 100644 --- a/src/session.ts +++ b/src/session.ts @@ -11,14 +11,14 @@ import { DlrMerger } from './dlr-merger.ts'; import { EventEmitter } from 'node:events'; import { IncomingRequests } from './incoming-requests.ts'; import { LinkTimers } from './link-timers.ts'; -import { PduFramer } from './pdu-framer.ts'; +import { PduTransport } from './pdu-transport.ts'; import { PendingRequests } from './pending-requests.ts'; import { ReconnectLoop } from './reconnect-loop.ts'; import { SendWindow } from './send-window.ts'; import { errorFrom } from './error-from.ts'; import { optionalParamsMinVersion } from './defs/constants.ts'; import { bindCarries, bindCommands, defaultSystemId, defaults } from './session-options.ts'; -import { isResp, objToPdu, pduReturn, pduToObj } from './pdu.ts'; +import { isResp, objToPdu, pduReturn } from './pdu.ts'; import { silentLog } from './log.ts'; import { submitSms } from './send-sms.ts'; @@ -47,8 +47,6 @@ export class Session extends EventEmitter { declare prependOnceListener: (event: K, listener: SessionListener) => this; declare removeListener: (event: K, listener: SessionListener) => this; - /** Replaced on reconnect, so hold the session rather than this. */ - sock: Socket; readonly log: SmppLog; /** The role the ESME bound with, whichever end of the link this is. Undefined before any bind. */ @@ -64,12 +62,13 @@ export class Session extends EventEmitter { private readonly pending: PendingRequests; private readonly reconnectLoop: ReconnectLoop | undefined; private readonly timers: LinkTimers; + private readonly transport: PduTransport; private readonly window: SendWindow; private closed = false; private concatReference = 0; private draining = false; - private framer = new PduFramer(); + private ended = false; /** A listener that throws is the application's bug; it must not become ours. Hard rule 1. */ override emit( @@ -126,7 +125,6 @@ export class Session extends EventEmitter { }); this.pending = new PendingRequests(this.log); this.reconnectLoop = this.loopFor(options.reconnect); - this.sock = options.sock; this.timers = new LinkTimers({ enquireLinkInterval: options.enquireLinkInterval, idleTimeout: options.idleTimeout, @@ -135,12 +133,18 @@ export class Session extends EventEmitter { // Not close(): a link that went quiet is a drop, and a drop is what reconnect is for. onIdle: () => { this.teardown(); }, }); + this.transport = this.transportFor(options.sock); this.window = new SendWindow(options.maxOutstanding ?? defaults.maxOutstanding); this.attach(options.sock); this.resetTimers(); } + /** Replaced on reconnect, so hold the session rather than this. */ + get sock(): Socket { + return this.transport.sock; + } + /** Whether this session's bind direction carries a command. Consulted by the library's senders. */ bindAllows(cmdName: string): boolean { return bindCarries(this.boundAs, cmdName); @@ -187,7 +191,7 @@ export class Session extends EventEmitter { tlvs?: Record, ): Promise { const built = pduReturn(pdu, status, params, tlvs); - const sent = built.err ? { err: built.err } : this.write(built.buffer); + const sent = built.err ? { err: built.err } : this.transport.write(built.buffer); // A peer that unbinds and drops the link takes our response with it; that is not a failure. if (sent.err && !this.closed) { @@ -249,6 +253,21 @@ export class Session extends EventEmitter { return drained; } + private transportFor(sock: Socket): PduTransport { + return new PduTransport({ + log: this.log, + onClose: () => { this.onClose(); }, + onData: chunk => { this.onData(chunk); }, + onError: err => { this.emit('sessionError', err); }, + onFramed: pdu => { this.emit('incomingPdu', pdu); }, + onPdu: pduObj => { this.dispatch(pduObj); }, + onUnreadable: err => { + this.emit('sessionError', err); + this.end(); + }, + }, sock); + } + private loopFor(reconnect: ReconnectOptions | undefined): ReconnectLoop | undefined { if (!reconnect) return undefined; @@ -289,22 +308,9 @@ export class Session extends EventEmitter { return {}; } - /** Wires a freshly opened socket into this session, replacing any previous one. */ private attach(sock: Socket): void { - // The socket being replaced is already dead, and its three handlers still point here. - if (this.sock !== sock) this.sock.removeAllListeners(); - - this.sock = sock; - this.framer = new PduFramer(); + this.transport.attach(sock); this.closed = false; - - sock.on('data', chunk => { this.onData(chunk); }); - sock.on('close', () => { this.onClose(); }); - sock.on('error', err => { - this.log.warn('session - socket error', { message: err.message }); - this.emit('sessionError', err); - this.onClose(); - }); } private async request( @@ -325,7 +331,7 @@ export class Session extends EventEmitter { signal: options.signal, timeout: this.options.responseTimeout ?? defaults.responseTimeout, }); - const written = this.write(built.buffer); + const written = this.transport.write(built.buffer); if (written.err) { this.pending.settle(seqNr, { err: written.err }); @@ -366,6 +372,15 @@ export class Session extends EventEmitter { this.reconnectLoop?.stop(); this.teardown(); this.dlrMerger.clear(); + this.emitClose(); + } + + /** A session torn down by a drop the loop was retrying reaches here with nothing left to tear down. */ + private emitClose(): void { + if (this.ended) return; + + this.ended = true; + this.emit('close'); } private teardown(): void { @@ -376,10 +391,11 @@ export class Session extends EventEmitter { this.pending.settleAll(new Error('Session closed before a response arrived')); this.incoming.clear(); this.sock.destroy(); - this.emit(this.retrying() ? 'disconnected' : 'close'); + + if (this.retrying()) this.emit('disconnected'); + else this.emitClose(); } - /** A drop the loop will bring the session back from is a disconnect, not the end of it. */ private retrying(): boolean { return this.reconnectLoop !== undefined && !this.reconnectLoop.isStopped(); } @@ -390,55 +406,9 @@ export class Session extends EventEmitter { return this.concatReference; } - private write(pdu: Buffer): VoidResult { - if (this.sock.destroyed) { - return { err: new Error('Socket is closed') }; - } - - this.sock.write(pdu); - - return {}; - } - private onData(chunk: Buffer): void { this.emit('data', chunk); this.resetTimers(); - this.framer.push(chunk); - - const framed = this.framer.next(); - - if (framed.err) { - this.log.warn('session - unusable stream, closing', { message: framed.err.message }); - this.emit('sessionError', framed.err); - this.end(); - - return; - } - - for (const pdu of framed.pdus) { - if (!this.receive(pdu)) return; - } - } - - /** False means the PDU could not be read and the session has been closed. */ - private receive(pdu: Buffer): boolean { - this.emit('incomingPdu', pdu); - - const parsed = pduToObj(pdu); - - if (parsed.err) { - this.log.warn('session - could not parse an incoming PDU, closing', { - message: parsed.err.message, - }); - this.emit('sessionError', parsed.err); - this.end(); - - return false; - } - - this.dispatch(parsed.pduObj); - - return true; } private dispatch(pduObj: PduObject): void { diff --git a/test/session-extras.test.ts b/test/session-extras.test.ts index 611024e..72c10b3 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -365,22 +365,28 @@ describe('reconnect', () => { assert.deepEqual(events, ['disconnected', 'close']); }); - test('reports a drop as close when nothing will retry it', async t => { + test('emits close when the session ends while the link is still down', async t => { const smpp = await startServer(t); - const { session } = await connect(t, smpp, { reconnect: false }); + const { session } = await connect(t, smpp); assert.ok(session); const events: string[] = []; + session.on('close', () => { events.push('close'); }); session.on('disconnected', () => { events.push('disconnected'); }); - const closed = once(resolve => { session.on('close', () => { resolve(true); }); }); + const down = once(resolve => { session.on('disconnected', () => { resolve(true); }); }); await peerOf(smpp).close(); - await closed; + await down; + await session.close(); - assert.deepEqual(events, []); + assert.deepEqual(events, ['disconnected', 'close']); + + await session.close(); + + assert.deepEqual(events, ['disconnected', 'close'], 'closing twice is still one close'); }); test('refuses a backoff that would retry without pausing', () => { @@ -391,11 +397,17 @@ describe('reconnect', () => { /false/, 'off is spelled false, so nothing else may stand in for it', ); + assert.match( + checkSessionOptions({ reconnect: { maxDelay: 1000, minDelay: 30_000 } }).err?.message ?? '', + /maxDelay/, + 'a transposed pair asks to never retry faster than 30 s and gets one every second', + ); + assert.match(checkSessionOptions({ reconnect: { minDelayMs: 20 } }).err?.message ?? '', /minDelayMs/); assert.equal(checkSessionOptions({ reconnect: false }).err, undefined); assert.equal(checkSessionOptions({ reconnect: { maxDelay: 60_000, minDelay: 500 } }).err, undefined); }); - test('schedules nothing after a drop when reconnect is false', async t => { + test('reports a drop as close, and schedules nothing, when reconnect is false', async t => { const smpp = await startServer(t); const noop = (): void => undefined; const infos: string[] = []; @@ -410,11 +422,16 @@ describe('reconnect', () => { assert.ok(session); + let disconnects = 0; + + session.on('disconnected', () => { disconnects++; }); + const closed = once(resolve => { session.on('close', () => { resolve(true); }); }); await peerOf(smpp).close(); await closed; + assert.equal(disconnects, 0); assert.ok(!infos.includes('reconnect - retrying after a drop')); }); diff --git a/todo.md b/todo.md index 2f0d7d9..5049a92 100644 --- a/todo.md +++ b/todo.md @@ -125,9 +125,6 @@ session message is a change to every call site. loses every incomplete group, and a peer has no reason to resend a receipt it already had answered. Surviving one means exposing the merge state for the application to persist and hand back, which is a public-surface decision. -- [ ] **`session.ts` has one seam left in it**, a socket-to-PDU transport, which would move the - deliberately public `sock` field out of `Session` or turn it into a getter — a public-surface - change, so it waits for a decision. - [ ] **Group the session's collaborators under `src/session/`.** Only `session.ts` imports `reassembly`, `dlr-merger`, `send-window`, `link-timers`, `reconnect-loop`, `pending-requests` and `send-sms`, so the directory would make that boundary visible. Do it on the next From b1c790b9a089cdfa3abc536a3b3c73d4a4a0fc5b Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Mon, 31 Aug 2026 22:27:16 +0200 Subject: [PATCH 04/19] Retry a stream we cannot read, and cover the framer reset on attach MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A framing or codec error tears the link down rather than the session, so the loop retries it on a fresh socket with a fresh framer — which is what a desynced stream needs. Removing that reset failed nothing before; the reconnect test now leaves half a PDU on the dying link, and does. `disconnected` counts failed links rather than outages, which the README now says, and the transport wires its socket as it is built. --- AGENTS.md | 22 ++++++++++++++++++---- README.md | 2 +- src/pdu-transport.ts | 15 +++++++++------ src/session-options.ts | 8 ++++++-- src/session.ts | 4 +--- test/session-extras.test.ts | 25 +++++++++++++++++++++++++ todo.md | 4 ++-- 7 files changed, 62 insertions(+), 18 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index ce8c175..a6ecdfd 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -274,10 +274,10 @@ exactly 140. `shutdownTimeout` bounds the drain alone, and `0` waits forever like every other timeout here; `unbind()` then waits `responseTimeout` for its own response, and sends that PDU through `request()` past both the window and the drain gate because it must go out either way. A stream - the framer or the codec cannot read takes `end()` instead, and so does `close({ signal })` on an - aborted signal and a peer's own `unbind` — nothing on a dead link can answer, an abort means stop - now, and a peer that has declared itself finished will not answer what it still owes us, so - draining any of the three would only hold a socket open for the timeout. `SmppServer.close()` + the framer or the codec cannot read takes `teardown()` instead, and `close({ signal })` on an + aborted signal and a peer's own `unbind` take `end()` — nothing on a dead link can answer, an abort + means stop now, and a peer that has declared itself finished will not answer what it still owes us, + so draining any of the three would only hold a socket open for the timeout. `SmppServer.close()` reports each session's unfinished drain through `serverError`, because its own result says nothing but that the listener stopped. `shutdownTimeout` stays a session option rather than a `close()` argument: `server()` builds sessions on the caller's behalf, so the option @@ -296,6 +296,20 @@ 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. +- **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. + +- **`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. + - **`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 — `PduTransport` owns the socket, the framer and the parse, and hands the session raw bytes, framed diff --git a/README.md b/README.md index 8e88f4f..3c9f8ea 100644 --- a/README.md +++ b/README.md @@ -317,7 +317,7 @@ TypeScript users can import `SmppLog` to have the compiler check one. | `dlr` | A delivery report arrives, one per segment. `smsId` is undefined when the peer marked a receipt whose body carries no readable id. `statusMsg` names `statusId` unless the peer sent a `message_state` this library cannot name — then `statusId` is that raw value and `statusMsg` is whatever the body said, or `UNKNOWN`. | | `messageDlr` | Every segment of a multipart message sent with `dlr: true` has been reported on, carrying the worst status of the segments. Merging needs the SMSC to number its segment ids `-`, which is this library's own server's convention — an SMSC that hands out unrelated ids per segment never fires it. A base is merged once: a later message the SMSC gives the same ids is reported on through `dlr` alone, and an earlier one still collecting loses its merged report as well. | | `close` | The session is over, because nothing will bring the link back. Fires once, whether you closed it or the link failed for good. | -| `disconnected` | The link dropped and the reconnect loop will retry it. Do not open a replacement client here — the session you hold comes back on its own, and `reconnected` says when. | +| `disconnected` | The link dropped and the reconnect loop will retry it. Do not open a replacement client here — the session you hold comes back on its own, and `reconnected` says when. Fires again for each attempt that reconnects and then fails, so it is not one-to-one with `reconnected`. | | `reconnected` | The client re-bound after a drop. | | `sessionError` | Something failed on a live session, including a hook or listener that threw or, if it was `async`, rejected. | | `data` | Raw bytes arrived on the socket. | diff --git a/src/pdu-transport.ts b/src/pdu-transport.ts index 6ce41be..d27a823 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, whatever the socket does next. */ + /** Nothing further can be read off this stream; the socket has to go. */ onUnreadable: (err: Error) => void; }; @@ -27,20 +27,23 @@ export class PduTransport { constructor(options: PduTransportOptions, sock: Socket) { this.options = options; this.socket = sock; + this.wire(sock); } get sock(): Socket { return this.socket; } - /** Wires a freshly opened socket in, replacing any previous one. */ + /** Takes over a freshly opened socket. Half a PDU left on the old one must not prefix this one. */ attach(sock: Socket): void { // The socket being replaced is already dead, and its three handlers still point here. - if (this.socket !== sock) this.socket.removeAllListeners(); - + this.socket.removeAllListeners(); this.socket = sock; this.framer = new PduFramer(); + this.wire(sock); + } + private wire(sock: Socket): void { sock.on('data', chunk => { this.read(chunk); }); sock.on('close', () => { this.options.onClose(); }); sock.on('error', err => { @@ -65,7 +68,7 @@ export class PduTransport { const framed = this.framer.next(); if (framed.err) { - this.options.log.warn('transport - unusable stream, closing', { message: framed.err.message }); + this.options.log.warn('transport - unusable stream', { message: framed.err.message }); this.options.onUnreadable(framed.err); return; @@ -77,7 +80,7 @@ export class PduTransport { const parsed = pduToObj(pdu); if (parsed.err) { - this.options.log.warn('transport - could not parse an incoming PDU, closing', { + this.options.log.warn('transport - could not parse an incoming PDU', { message: parsed.err.message, }); this.options.onUnreadable(parsed.err); diff --git a/src/session-options.ts b/src/session-options.ts index adc28e2..eb79360 100644 --- a/src/session-options.ts +++ b/src/session-options.ts @@ -163,9 +163,13 @@ function checkReconnect(reconnect: unknown): VoidResult { // A delay of 0 never doubles, so the backoff never starts and every retry lands at once. const checked = checkLimits([['maxDelay', maxDelay, 1], ['minDelay', minDelay, 1]]); - if (checked.err || maxDelay >= minDelay) return checked; + if (checked.err) return checked; - return { err: new Error(`maxDelay must be minDelay or more, got ${String(maxDelay)}`) }; + if (maxDelay < minDelay) { + return { err: new Error(`maxDelay must be minDelay (${String(minDelay)}) or more, got ${String(maxDelay)}`) }; + } + + return {}; } function isRecord(value: unknown): value is Record { diff --git a/src/session.ts b/src/session.ts index 63768bc..ab229f5 100644 --- a/src/session.ts +++ b/src/session.ts @@ -136,7 +136,6 @@ export class Session extends EventEmitter { this.transport = this.transportFor(options.sock); this.window = new SendWindow(options.maxOutstanding ?? defaults.maxOutstanding); - this.attach(options.sock); this.resetTimers(); } @@ -263,7 +262,7 @@ export class Session extends EventEmitter { onPdu: pduObj => { this.dispatch(pduObj); }, onUnreadable: err => { this.emit('sessionError', err); - this.end(); + this.teardown(); }, }, sock); } @@ -375,7 +374,6 @@ export class Session extends EventEmitter { this.emitClose(); } - /** A session torn down by a drop the loop was retrying reaches here with nothing left to tear down. */ private emitClose(): void { if (this.ended) return; diff --git a/test/session-extras.test.ts b/test/session-extras.test.ts index 72c10b3..d1da3af 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -389,6 +389,26 @@ describe('reconnect', () => { assert.deepEqual(events, ['disconnected', 'close'], 'closing twice is still one close'); }); + 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); + + assert.ok(session); + + const events: string[] = []; + + session.on('close', () => { events.push('close'); }); + session.on('sessionError', () => { events.push('sessionError'); }); + + const reconnected = once(resolve => { session.on('reconnected', () => { resolve(true); }); }); + + // A cmd_length below the 16-octet header is a stream no framing can recover from. + peerOf(smpp).sock.write(Buffer.from([0, 0, 0, 4, 0, 0, 0, 4, 0, 0, 0, 0, 0, 0, 0, 1])); + await reconnected; + + assert.deepEqual(events, ['sessionError']); + }); + test('refuses a backoff that would retry without pausing', () => { assert.match(checkSessionOptions({ reconnect: { minDelay: 0 } }).err?.message ?? '', /minDelay/); assert.match(checkSessionOptions({ reconnect: { maxDelay: -1 } }).err?.message ?? '', /maxDelay/); @@ -453,6 +473,11 @@ describe('reconnect', () => { assert.ok(session); const reconnected = once(resolve => { session.on('reconnected', () => { resolve(true); }); }); + const halfPdu = once(resolve => { session.on('data', () => { resolve(true); }); }); + + // A PDU header promising 32 octets and sending 8: the next link must not continue it. + peerOf(smpp).sock.write(Buffer.from([0, 0, 0, 32, 0, 0, 0, 4])); + await halfPdu; // Drop the connection from the server's side, as a peer restart would. for (const serverSession of smpp.sessions) { diff --git a/todo.md b/todo.md index 5049a92..b83bb50 100644 --- a/todo.md +++ b/todo.md @@ -127,8 +127,8 @@ session message is a change to every call site. back, which is a public-surface decision. - [ ] **Group the session's collaborators under `src/session/`.** Only `session.ts` imports `reassembly`, `dlr-merger`, `send-window`, `link-timers`, `reconnect-loop`, `pending-requests` - and `send-sms`, so the directory would make that boundary visible. Do it on the next - extraction out of `session.ts`, not as a move of its own. + and `send-sms`, so the directory would make that boundary visible. `pdu-transport` joined them + on 2026-08-31 without the move being made, so it is a move of its own now. - [ ] **Does an intermediate delivery notification deserve to be a `dlr`?** `esm_class` message type `INTERMEDIATE_DELIVERY` (0x20) is classified as a message today, so a peer that reports non-final states with it hands the application a raw `id:… stat:ENROUTE` text as an inbound From 8e4d472f72de89f50cb1b13ced522284b8c711e8 Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Mon, 31 Aug 2026 22:45:29 +0200 Subject: [PATCH 05/19] 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; From bfc85ee4dd173fc40558e1afde8d94b263f308c2 Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Mon, 31 Aug 2026 22:59:57 +0200 Subject: [PATCH 06/19] Say in the options table that a proven link resets the backoff maxDelay bounds the wait and sets the bar a link must clear, and only the first was documented, so tuning it up quietly lengthened every recovery. The unreadable PDU the tests write is one named constant now. --- README.md | 2 +- test/session-extras.test.ts | 9 +++++---- test/session.test.ts | 1 - 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 264f756..6f0ba2d 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, 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. | +| `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 and starting over at `minDelay` once a link has lasted `maxDelay`. `{ 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/test/session-extras.test.ts b/test/session-extras.test.ts index fe709eb..3f0dae6 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -69,6 +69,9 @@ function delay(ms: number): Promise { return new Promise(resolve => { setTimeout(resolve, ms); }); } +/** A cmd_length below the 16-octet header: a stream no framing can recover from. */ +const unreadablePdu = Buffer.from([0, 0, 0, 4, 0, 0, 0, 4, 0, 0, 0, 0, 0, 0, 0, 1]); + /** The server's side of the one connection under test. */ function peerOf(smpp: SmppServer): Session { const [peer] = smpp.sessions; @@ -402,8 +405,7 @@ describe('reconnect', () => { const reconnected = once(resolve => { session.on('reconnected', () => { resolve(true); }); }); - // A cmd_length below the 16-octet header is a stream no framing can recover from. - peerOf(smpp).sock.write(Buffer.from([0, 0, 0, 4, 0, 0, 0, 4, 0, 0, 0, 0, 0, 0, 0, 1])); + peerOf(smpp).sock.write(unreadablePdu); await reconnected; assert.deepEqual(events, ['sessionError']); @@ -448,8 +450,7 @@ describe('reconnect', () => { const closed = once(resolve => { session.on('close', () => { resolve(true); }); }); - // 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])); + peerOf(smpp).sock.write(unreadablePdu); await closed; assert.equal(disconnects, 0); diff --git a/test/session.test.ts b/test/session.test.ts index 3579c4b..6f1084b 100644 --- a/test/session.test.ts +++ b/test/session.test.ts @@ -1374,7 +1374,6 @@ describe('application hooks that throw or reject', () => { }); 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; From 8d9656b4e0f5ad4197f66c6ea26864643f364241 Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Tue, 1 Sep 2026 08:02:37 +0200 Subject: [PATCH 07/19] Hold a send with no link for the next one, and count the rest as unanswered --- AGENTS.md | 19 +++- README.md | 15 ++- src/link-gate.ts | 85 +++++++++++++++++ src/pending-requests.ts | 13 ++- src/send-sms.ts | 16 +++- src/session.ts | 93 ++++++++++++------- test/readme.test.ts | 3 +- test/session-extras.test.ts | 179 +++++++++++++++++++++++++++++++++++- test/session.test.ts | 2 +- todo.md | 13 +-- 10 files changed, 386 insertions(+), 52 deletions(-) create mode 100644 src/link-gate.ts diff --git a/AGENTS.md b/AGENTS.md index 4e6633a..03f2087 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -45,6 +45,7 @@ src/ error-from.ts errorFrom(): whatever was thrown or rejected, as an Error expiring-groups.ts ExpiringGroups: the capped, expiring store both of those share incoming-requests.ts Every request the peer sends: messages, receipts, links, unknown commands + link-gate.ts LinkGate: where a request with no link to go out on waits for the next one link-timers.ts LinkTimers: the enquire_link heartbeat and the idle timeout log.ts SmppLog, the logger contract, and silentLog — the default message.ts Encoding detection, splitting, bit counting, SMPP date formatting @@ -178,8 +179,8 @@ exactly 140. - **The published surface is frozen at what `src/index.ts` exports today.** `Session` is exported and publicly constructible, which is why `SessionOptions` and `ReconnectOptions` are public too — that is correct, not a leak, and it has been raised twice. The collaborators `session.ts` delegates to - (`Reassembler`, `PendingRequests`, `SendWindow`, `ReconnectLoop`, `LinkTimers`, `DlrMerger`, - `submitSms`) stay unpublished so they can be reshaped. + (`Reassembler`, `PendingRequests`, `SendWindow`, `ReconnectLoop`, `LinkTimers`, `LinkGate`, + `DlrMerger`, `submitSms`) stay unpublished so they can be reshaped. - **The sub-3.4 optional-parameter rule is a predicate, not a chokepoint.** `acceptsOptionalParams()` is consulted by the library's own senders; `session.send({ tlvs })` is passed through as written, because silently stripping a caller's explicit TLVs off a deliberately public low-level surface @@ -345,3 +346,17 @@ exactly 140. as no number and so reaches `expect()` and `collect()` unchanged. Normalising the base instead would break that pair. The option is on `client()` only — a `server()` session generates its own ids and writes its own receipts, so both places are already one notation. + +- **A send waits for the next link only if it never reached the socket; one that did is counted, not + resent.** Maintainer's call, 2026-09-01: re-queueing everything unanswered would resend a + `submit_sm` the SMSC accepted and answered into a dead socket, which is delivered and billed + twice, while a request that never left this process can be lost for free. `LinkGate` holds a send + that has no link and `comeBackUp()` opens it once the rebind is bound, so a send issued between + links and a segment still queued behind a full window when the drop hit both go out on the new + one. The hold is bounded by `responseTimeout` rather than an option of its own — that is already + the answer to how long one request may take — so the worst case is twice it: the hold, then the + answer. It happens before `window.acquire()` rather than inside it, because the rebind's own + `bind()` goes through `session.send()` and would deadlock behind slots held by waiting sends. + `teardown()` settles what was on the wire with `UnansweredError`, which `collectSent()` counts + into `SendSmsResult.unanswered`: `smsIds` alone cannot tell a message that never left from one + whose every segment the peer took and answered into a socket that was already gone. diff --git a/README.md b/README.md index 6f0ba2d..fdd0510 100644 --- a/README.md +++ b/README.md @@ -100,7 +100,7 @@ Every one is optional. | `tls` | `false` | `true` for defaults, or a `tls.ConnectionOptions` object for a private CA or a client certificate. | | `enquireLinkInterval` | `20000` | How often to send `enquire_link` on a quiet link. | | `idleTimeout` | `2 × enquireLinkInterval` | Give up on a link the peer has stopped answering, and re-bind unless `reconnect` is `false`. | -| `responseTimeout` | `30000` | How long to wait for a response before giving up on it; `0` waits forever. | +| `responseTimeout` | `30000` | How long to wait for a response before giving up on it, and how long a send with no link waits for the next one; `0` waits forever. | | `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. | @@ -135,13 +135,16 @@ Messages too long for one SMS are split automatically and sent as a concatenated one id per segment: ```javascript -const { err, pduObjs, smsIds } = await session.sendSms({ from, message, to }); +const { err, pduObjs, smsIds, unanswered } = await session.sendSms({ from, message, to }); ``` `err` is set when the SMSC refuses a segment, and it names the status it refused with. Because every segment goes on the wire together, `pduObjs` and `smsIds` then hold what the SMSC did accept — enough to reconcile against a later receipt, not enough to resend the rest, so treat a partial failure as a -failed message. A message needing more than 255 segments is refused before anything is sent, since +failed message. `unanswered` counts the segments the link dropped under: the SMSC may have taken +each of them and lost only the response, so a message with `unanswered` above zero cannot be sent +again without risking a duplicate, however empty `smsIds` is. A message needing more than 255 +segments is refused before anything is sent, since the concatenation header numbers segments in a single octet. `maxSegments` lowers that ceiling: most handsets and SMSCs stop well short of 255, and refusing beats a message only half delivered. @@ -341,6 +344,12 @@ const { err, pduObj } = await session.send({ }); ``` +A send issued while the link is down waits for the reconnect instead of failing, and goes out on the +new link once it is bound — up to `responseTimeout`, after which it gives up having sent nothing. A +request already on the wire when the link drops is the other case: the SMSC may have taken it and +lost only the response, so it fails, and `sendSms()` counts it in `unanswered`. Neither happens with +`reconnect: false`, where a drop ends the session and every send after it is refused. + `acceptsOptionalParams()` answers whether the peer declared SMPP 3.4 or later, which is the version at and above which the spec allows optional parameters to be sent to it; `peerInterfaceVersion` is the version it declared, `0x00` if it declared none. The library's own senders consult the first before attaching a TLV — a diff --git a/src/link-gate.ts b/src/link-gate.ts new file mode 100644 index 0000000..d97293e --- /dev/null +++ b/src/link-gate.ts @@ -0,0 +1,85 @@ +import type { VoidResult } from './result.ts'; + +export type LinkGateOptions = { + /** Whether the link can carry nothing right now. */ + isDown: () => boolean; + /** How long a request may wait for a link. 0 waits for as long as one may still arrive. */ + timeout: number; + /** Whether a link that is down will be brought back. */ + willReturn: () => boolean; +}; + +type Waiter = (result: VoidResult) => void; + +function expired(): Error { + return new Error('The link did not come back in time'); +} + +/** Where a request with no link to go out on waits for the next one. */ +export class LinkGate { + private readonly options: LinkGateOptions; + private readonly waiting = new Set(); + + constructor(options: LinkGateOptions) { + this.options = options; + } + + /** When a hold starting now has to give up. 0 never does. */ + deadline(): number { + return this.options.timeout > 0 ? Date.now() + this.options.timeout : 0; + } + + /** Resolves once a link can carry the request, or with the reason none ever will. */ + wait(deadline: number, signal: AbortSignal | undefined): Promise { + if (!this.options.isDown()) return Promise.resolve({}); + + if (!this.options.willReturn()) return Promise.resolve({ err: new Error('Session is closed') }); + + const left = deadline === 0 ? 0 : deadline - Date.now(); + + if (deadline !== 0 && left <= 0) return Promise.resolve({ err: expired() }); + + return this.hold(left, signal); + } + + /** A link is up: everything held goes out on it. */ + open(): void { + this.release({}); + } + + /** No link is coming, and this is why. */ + shut(err: Error): void { + this.release({ err }); + } + + private hold(left: number, signal: AbortSignal | undefined): Promise { + return new Promise(resolve => { + let timer: NodeJS.Timeout | undefined = undefined; + const settle = (result: VoidResult): void => { + if (timer) clearTimeout(timer); + + signal?.removeEventListener('abort', onAbort); + this.waiting.delete(settle); + resolve(result); + }; + + function onAbort(): void { + settle({ err: new Error('Aborted while waiting for a link') }); + } + + if (left > 0) { + timer = setTimeout(() => { settle({ err: expired() }); }, left); + timer.unref(); + } + + signal?.addEventListener('abort', onAbort, { once: true }); + this.waiting.add(settle); + }); + } + + private release(result: VoidResult): void { + for (const settle of [...this.waiting]) { + settle(result); + } + } +} diff --git a/src/pending-requests.ts b/src/pending-requests.ts index 7949106..515a335 100644 --- a/src/pending-requests.ts +++ b/src/pending-requests.ts @@ -12,6 +12,14 @@ type Pending = { settle: (result: Result<{ pduObj: PduObject }>) => void; }; +/** The request went out and the link died before an answer: the peer may have accepted it. */ +export class UnansweredError extends Error { + constructor() { + super('The link dropped after the request went out; the peer may have accepted it'); + this.name = 'UnansweredError'; + } +} + /** Hands out sequence numbers and matches responses to the requests waiting for them. */ export class PendingRequests { private readonly log: SmppLog; @@ -69,9 +77,10 @@ export class PendingRequests { this.pending.get(seqNr)?.settle(result); } - settleAll(err: Error): void { + /** Everything still on the wire when the link died, each one possibly accepted by the peer. */ + settleAll(): void { for (const [seqNr] of this.pending) { - this.settle(seqNr, { err }); + this.settle(seqNr, { err: new UnansweredError() }); } } diff --git a/src/send-sms.ts b/src/send-sms.ts index 2fb7593..96689df 100644 --- a/src/send-sms.ts +++ b/src/send-sms.ts @@ -4,6 +4,7 @@ import type { PduObject, PduObjectInput } from './pdu.ts'; import type { Result } from './result.ts'; import type { SmppLog } from './log.ts'; import type { SmsIdNotation } from './sms-id.ts'; +import { UnansweredError } from './pending-requests.ts'; import { consts } from './defs/constants.ts'; import { detect } from './defs/encodings.ts'; import { normaliseSmsId } from './sms-id.ts'; @@ -28,7 +29,13 @@ export type SendSmsOptions = { }; /** Both arrays hold what the peer accepted, so a partial failure names what is already delivered. */ -export type SendSmsResult = { err?: Error; pduObjs: PduObject[]; smsIds: string[] }; +export type SendSmsResult = { + err?: Error; + pduObjs: PduObject[]; + smsIds: string[]; + /** Segments the link dropped under. The peer may have taken them, so sending again may duplicate. */ + unanswered: number; +}; /** What sending needs from the session: a concat reference and a way onto the wire. */ export type SendSmsDeps = { @@ -107,9 +114,12 @@ function collectSent( const pduObjs: PduObject[] = []; const smsIds: string[] = []; let failure: Error | undefined; + let unanswered = 0; for (const one of sent) { if (one.err) { + if (one.err instanceof UnansweredError) unanswered++; + failure ??= one.err; } else if (one.pduObj.cmdStatus === 'ESME_ROK') { pduObjs.push(one.pduObj); @@ -121,7 +131,7 @@ function collectSent( } } - return failure ? { err: failure, pduObjs, smsIds } : { pduObjs, smsIds }; + return failure ? { err: failure, pduObjs, smsIds, unanswered } : { pduObjs, smsIds, unanswered }; } /** Puts a message on the wire as one submit_sm per segment. */ @@ -131,7 +141,7 @@ export async function submitSms(deps: SendSmsDeps, sms: SendSmsOptions): Promise const segments = splitMessage(sms.message, { encoding, reference: deps.reference }); const refused = checkSegments(allowed, segments.length); - if (refused) return { err: refused, pduObjs: [], smsIds: [] }; + if (refused) return { err: refused, pduObjs: [], smsIds: [], unanswered: 0 }; const multipart = segments.length > 1; diff --git a/src/session.ts b/src/session.ts index ab229f5..a9f4f80 100644 --- a/src/session.ts +++ b/src/session.ts @@ -10,6 +10,7 @@ import type { Socket } from 'node:net'; import { DlrMerger } from './dlr-merger.ts'; import { EventEmitter } from 'node:events'; import { IncomingRequests } from './incoming-requests.ts'; +import { LinkGate } from './link-gate.ts'; import { LinkTimers } from './link-timers.ts'; import { PduTransport } from './pdu-transport.ts'; import { PendingRequests } from './pending-requests.ts'; @@ -38,6 +39,9 @@ export { bindCommands, defaultSystemId }; /** A listener may return a promise: an `async` one that rejects is routed like one that throws. */ type SessionListener = (...args: SessionEvents[K]) => unknown; +/** `unsent` means nothing reached the socket, so the next link can still carry this request. */ +type Attempt = { result: Result<{ pduObj: PduObject }>; unsent: boolean }; + export class Session extends EventEmitter { declare addListener: (event: K, listener: SessionListener) => this; declare off: (event: K, listener: SessionListener) => this; @@ -57,6 +61,7 @@ export class Session extends EventEmitter { userData: unknown = undefined; private readonly dlrMerger: DlrMerger; + private readonly gate: LinkGate; private readonly incoming: IncomingRequests; private readonly options: SessionOptions; private readonly pending: PendingRequests; @@ -112,6 +117,11 @@ export class Session extends EventEmitter { max: defaults.maxDlrMerges, timeout: defaults.dlrMergeTimeout, }); + this.gate = new LinkGate({ + isDown: () => this.linkDown(), + timeout: options.responseTimeout ?? defaults.responseTimeout, + willReturn: () => this.retrying(), + }); this.incoming = new IncomingRequests({ dlrMerger: this.dlrMerger, log: this.log, @@ -160,26 +170,44 @@ export class Session extends EventEmitter { input: PduObjectInput, options: SendOptions = {}, ): Promise> { + const refused = this.refuseSend(input, options); + + if (refused) return { err: refused }; + + const deadline = this.gate.deadline(); + + for (;;) { + const held = await this.gate.wait(deadline, options.signal); + + if (held.err) return { err: held.err }; + + await this.window.acquire(); + + const attempt = await this.attempt(input, options).finally(() => { this.window.release(); }); + + // Nothing reached the socket, so the next link carries it instead of the caller resending. + if (!attempt.unsent || !this.linkDown() || !this.retrying()) return attempt.result; + } + } + + /** Why a request cannot go out at all, as opposed to not yet. */ + private refuseSend(input: PduObjectInput, options: SendOptions): Error | undefined { if (input.cmdName.endsWith('_resp')) { - return { err: new Error(`Use sendReturn() for responses, not send(): ${input.cmdName}`) }; + return new Error(`Use sendReturn() for responses, not send(): ${input.cmdName}`); } - if (this.closed) return { err: new Error('Session is closed') }; + // A drain on a live link. A link that is down is the gate's answer, which says closed instead. + if (this.draining && !this.linkDown()) return new Error('Session is shutting down'); - if (this.draining) return { err: new Error('Session is shutting down') }; + // Before the gate and the window, or an aborted call waits for a link it will not use. + if (options.signal?.aborted === true) return new Error('Aborted before the request was sent'); - // Before the window, or a full window makes an aborted call wait for a slot it will not use. - if (options.signal?.aborted === true) { - return { err: new Error('Aborted before the request was sent') }; - } + return undefined; + } - await this.window.acquire(); - - try { - return await this.request(input, options); - } finally { - this.window.release(); - } + /** Read through a method: a drop can land while a send is awaiting. */ + private linkDown(): boolean { + return this.closed || this.sock.destroyed; } /** Answers a request the peer sent us. Responses are never waited on. */ @@ -207,7 +235,12 @@ export class Session extends EventEmitter { async sendSms(sms: SendSmsOptions, options: SendOptions = {}): Promise { if (!this.bindAllows('submit_sm')) { - return { err: new Error('A receiver-bound session does not carry submit_sm'), pduObjs: [], smsIds: [] }; + return { + err: new Error('A receiver-bound session does not carry submit_sm'), + pduObjs: [], + smsIds: [], + unanswered: 0, + }; } const sent = await submitSms({ @@ -229,9 +262,9 @@ export class Session extends EventEmitter { async unbind(): Promise { const drained = await this.drain(undefined); const wasOpen = !this.closed; - // request(), not send(): the drain gate refuses a send, and the unbind goes out either way. + // attempt(), not send(): the drain gate refuses a send, and the unbind goes out either way. const sent = wasOpen - ? await this.request({ cmdName: 'unbind' }, {}) + ? (await this.attempt({ cmdName: 'unbind' }, {})).result : { err: new Error('Session is closed') }; const closedOnUnbind = wasOpen && this.closed; @@ -303,6 +336,7 @@ export class Session extends EventEmitter { this.resetTimers(); this.log.info('session - reconnected'); this.emit('reconnected'); + this.gate.open(); return {}; } @@ -312,19 +346,16 @@ export class Session extends EventEmitter { this.closed = false; } - private async request( - input: PduObjectInput, - options: SendOptions, - ): Promise> { + private async attempt(input: PduObjectInput, options: SendOptions): Promise { // pending.wait() alone settles the caller while the request still goes out to the peer. if (options.signal?.aborted === true) { - return { err: new Error('Aborted before the request was sent') }; + return { result: { err: new Error('Aborted before the request was sent') }, unsent: false }; } const seqNr = this.pending.nextSeqNr(); const built = objToPdu({ ...input, seqNr }); - if (built.err) return { err: built.err }; + if (built.err) return { result: { err: built.err }, unsent: false }; const response = this.pending.wait(seqNr, { signal: options.signal, @@ -335,10 +366,10 @@ export class Session extends EventEmitter { if (written.err) { this.pending.settle(seqNr, { err: written.err }); - return { err: written.err }; + return { result: { err: written.err }, unsent: true }; } - return response; + return { result: await response, unsent: false }; } /** Stops new sends and waits out the ones already issued. */ @@ -346,13 +377,13 @@ export class Session extends EventEmitter { this.reconnectLoop?.stop(); this.draining = true; - if (this.closed || this.sock.destroyed) return {}; + if (this.linkDown()) return {}; const timeout = this.options.shutdownTimeout ?? defaults.shutdownTimeout; const unfinished = await this.window.idle(timeout, signal); // The window empties on a teardown too, which settles everything the link was carrying. - if (this.isClosed()) return { err: new Error('The session closed before the drain finished') }; + if (this.linkDown()) return { err: new Error('The session closed before the drain finished') }; if (unfinished === 0) return {}; @@ -361,11 +392,6 @@ export class Session extends EventEmitter { return { err: new Error(`Shut down with ${String(unfinished)} request(s) unfinished`) }; } - /** Read through a method: teardown() can land while the drain is awaiting. */ - private isClosed(): boolean { - return this.closed; - } - /** The session is over now, drained or not. Nothing brings it back. */ private end(): void { this.reconnectLoop?.stop(); @@ -378,6 +404,7 @@ export class Session extends EventEmitter { if (this.ended) return; this.ended = true; + this.gate.shut(new Error('Session closed before the link came back')); this.emit('close'); } @@ -386,7 +413,7 @@ export class Session extends EventEmitter { this.closed = true; this.timers.clear(); - this.pending.settleAll(new Error('Session closed before a response arrived')); + this.pending.settleAll(); this.incoming.clear(); this.sock.destroy(); diff --git a/test/readme.test.ts b/test/readme.test.ts index 44794cb..d68ce28 100644 --- a/test/readme.test.ts +++ b/test/readme.test.ts @@ -83,7 +83,7 @@ describe('README: Client', () => { closeAfter(t, session); const reported = once(resolve => { session.on('dlr', resolve); }); - const { err: sendErr, smsIds } = await session.sendSms({ + const { err: sendErr, smsIds, unanswered } = await session.sendSms({ dlr: true, from: '46701113311', message: '«baff»', @@ -92,6 +92,7 @@ describe('README: Client', () => { assert.equal(sendErr, undefined); assert.equal(smsIds.length, 1); + assert.equal(unanswered, 0); assert.equal((await reported).smsId, smsIds[0]); }); diff --git a/test/session-extras.test.ts b/test/session-extras.test.ts index 3f0dae6..9030830 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -557,6 +557,182 @@ describe('reconnect', () => { }); }); +describe('sends across a reconnect', () => { + /** Answers every message after the first, which is left to hold the send window open. */ + function answerAfterTheFirst(smpp: SmppServer, arrived: string[]): Gate { + const first = gate(); + + smpp.on('session', peer => { + peer.on('sms', async sms => { + arrived.push(sms.message); + + if (arrived.length === 1) first.open(); + else await sms.sendResp(); + }); + }); + + return first; + } + + test('holds a send issued while the link is down and puts it on the new link', async t => { + const smpp = await startServer(t); + const arrived: string[] = []; + + smpp.on('session', peer => { peer.on('sms', async sms => { arrived.push(sms.message); await sms.sendResp(); }); }); + + const { session } = await connect(t, smpp, { reconnect: { maxDelay: 100, minDelay: 20 } }); + + assert.ok(session); + + const down = once(resolve => { session.on('disconnected', () => { resolve(true); }); }); + + await peerOf(smpp).close(); + await down; + + const sent = await session.sendSms({ from: '46701113311', message: 'held', to: '46709771337' }); + + assert.equal(sent.err, undefined); + assert.equal(sent.smsIds.length, 1); + assert.equal(sent.unanswered, 0); + assert.deepEqual(arrived, ['held']); + }); + + test('puts a segment still queued behind a full window on the new link', async t => { + const smpp = await startServer(t); + const arrived: string[] = []; + const first = answerAfterTheFirst(smpp, arrived); + const { session } = await connect(t, smpp, { + maxOutstanding: 1, + reconnect: { maxDelay: 100, minDelay: 20 }, + }); + + assert.ok(session); + + const holding = session.sendSms({ from: '46701113311', message: 'first', to: '46709771337' }); + + await first.passed; + + const queued = session.sendSms({ from: '46701113311', message: 'second', to: '46709771337' }); + + peerOf(smpp).sock.destroy(); + + const [dropped, resent] = await Promise.all([holding, queued]); + + assert.equal(dropped.unanswered, 1); + assert.equal(resent.err, undefined, 'a request that never reached the socket is not lost with it'); + assert.equal(resent.smsIds.length, 1); + assert.deepEqual(arrived, ['first', 'second']); + }); + + test('reports a segment the link dropped under as unanswered, not as never sent', async t => { + const smpp = await startServer(t); + const arrived = once(resolve => { smpp.on('session', peer => peer.on('sms', resolve)); }); + const { session } = await connect(t, smpp, { reconnect: false }); + + assert.ok(session); + + const sending = session.sendSms({ from: '46701113311', message: 'in flight', to: '46709771337' }); + + await arrived; + peerOf(smpp).sock.destroy(); + + const sent = await sending; + + assert.match(sent.err?.message ?? '', /may have accepted/); + assert.equal(sent.unanswered, 1, 'the peer may have accepted it, so sending it again would duplicate'); + assert.deepEqual(sent.smsIds, []); + }); + + test('gives up a held send after responseTimeout, with nothing put on the wire', async t => { + const smpp = await startServer(t); + const { session } = await connect(t, smpp, { + reconnect: { maxDelay: 10_000, minDelay: 10_000 }, + responseTimeout: 60, + }); + + assert.ok(session); + + const down = once(resolve => { session.on('disconnected', () => { resolve(true); }); }); + + await peerOf(smpp).close(); + await down; + + const sent = await session.sendSms({ from: '46701113311', message: 'held', to: '46709771337' }); + + assert.match(sent.err?.message ?? '', /did not come back/); + assert.equal(sent.unanswered, 0, 'nothing reached the peer, so the message can be sent again'); + }); + + test('fails a held send when the session closes rather than leaving it waiting', async t => { + const smpp = await startServer(t); + const { session } = await connect(t, smpp, { + reconnect: { maxDelay: 10_000, minDelay: 10_000 }, + }); + + assert.ok(session); + + const down = once(resolve => { session.on('disconnected', () => { resolve(true); }); }); + + await peerOf(smpp).close(); + await down; + + const sending = session.sendSms({ from: '46701113311', message: 'held', to: '46709771337' }); + let settled = false; + + void sending.then(() => { settled = true; }); + await delay(30); + + assert.equal(settled, false, 'the send waits for a link rather than failing on the spot'); + + await session.close(); + + const sent = await sending; + + assert.match(sent.err?.message ?? '', /closed/); + assert.equal(sent.unanswered, 0); + }); + + test('aborts a held send instead of making it wait out the link', async t => { + const smpp = await startServer(t); + const { session } = await connect(t, smpp, { + reconnect: { maxDelay: 10_000, minDelay: 10_000 }, + }); + + assert.ok(session); + + const down = once(resolve => { session.on('disconnected', () => { resolve(true); }); }); + + await peerOf(smpp).close(); + await down; + + const controller = new AbortController(); + const sending = session.sendSms( + { from: '46701113311', message: 'held', to: '46709771337' }, + { signal: controller.signal }, + ); + + controller.abort(); + + const sent = await sending; + + assert.match(sent.err?.message ?? '', /Aborted while waiting for a link/); + assert.equal(sent.unanswered, 0); + }); + + test('refuses a send outright once the session is over', async t => { + const smpp = await startServer(t); + const { session } = await connect(t, smpp, { reconnect: false }); + + assert.ok(session); + await session.close(); + + const sent = await session.sendSms({ from: '46701113311', message: 'too late', to: '46709771337' }); + + assert.match(sent.err?.message ?? '', /closed/); + assert.equal(sent.unanswered, 0); + }); +}); + describe('reassembly bounds', () => { function segment(reference: number, part: number, total: number): PduObject { const udh = Buffer.from([0x05, 0x00, 0x03, reference, total, part]); @@ -789,7 +965,8 @@ describe('graceful shutdown', () => { const result = await sent; assert.ok(result.err instanceof Error); - assert.equal(result.err.message, 'Session closed before a response arrived'); + assert.match(result.err.message, /may have accepted/); + assert.equal(result.unanswered, 1); }); // The window empties on a drop as well as on an answer, so it cannot be what the result reads. diff --git a/test/session.test.ts b/test/session.test.ts index 6f1084b..a8e382b 100644 --- a/test/session.test.ts +++ b/test/session.test.ts @@ -236,7 +236,7 @@ describe('bind', () => { const sent = await session.send({ cmdName: 'enquire_link' }); assert.ok(sent.err instanceof Error); - assert.equal(sent.err.message, 'Session closed before a response arrived'); + assert.match(sent.err.message, /may have accepted/); }); test('reports an unbind the peer left unanswered on a link that stays up', async t => { diff --git a/todo.md b/todo.md index b83bb50..bfe0147 100644 --- a/todo.md +++ b/todo.md @@ -57,6 +57,7 @@ Rules the API follows: | Merged multipart DLRs including across a reconnect, reassembly bounds, per-send abort, the segment cap | `test/session-extras.test.ts` | | `smsIdFormat`: a peer's `submit_sm_resp` and receipt ids read into one notation before they are compared | `test/dlr.test.ts`, `test/session-extras.test.ts` | | A draining `close()` and `unbind()`, bounded by `shutdownTimeout` or an abort | `test/session-extras.test.ts` | +| A send with no link held for the next one, and one the link dropped under counted as `unanswered` | `test/session-extras.test.ts` | | Every runnable README example | `test/readme.test.ts` | | Receipt-versus-message classification by `esm_class` | `test/dlr.test.ts`, `test/session.test.ts` | | A listener that throws, or rejects, reaching `sessionError`/`serverError` rather than the process | `test/session.test.ts`, `test/error-from.test.ts` | @@ -113,9 +114,6 @@ session message is a change to every call site. ## Worth doing, not blocking -- [ ] **In-flight sends across a reconnect.** They currently fail with "Session closed before a - response arrived" and the caller retries. Re-queueing them automatically would be friendlier - but risks duplicate delivery, so it needs a decision before it is built. - [ ] **The drain covers only what this end sent.** `close()` and `unbind()` wait on the send window, which `sendReturn()` never enters, so a server session tears down without waiting for the application to answer the messages it is holding — the duplicate-on-retry outcome again, in @@ -126,9 +124,12 @@ session message is a change to every call site. answered. Surviving one means exposing the merge state for the application to persist and hand back, which is a public-surface decision. - [ ] **Group the session's collaborators under `src/session/`.** Only `session.ts` imports - `reassembly`, `dlr-merger`, `send-window`, `link-timers`, `reconnect-loop`, `pending-requests` - and `send-sms`, so the directory would make that boundary visible. `pdu-transport` joined them - on 2026-08-31 without the move being made, so it is a move of its own now. + `reassembly`, `dlr-merger`, `send-window`, `link-timers`, `link-gate`, `reconnect-loop`, + `pending-requests` and `send-sms`, so the directory would make that boundary visible. + `pdu-transport` joined them on 2026-08-31 and `link-gate` on 2026-09-01, both without the move + being made, so it is a move of its own now. `session.ts` sits within a couple of lines of the + 350-line cap again; the outgoing request path — `send()`, `refuseSend()`, `attempt()` and the + gate and window they drive — is the next thing that would come out of it. - [ ] **Does an intermediate delivery notification deserve to be a `dlr`?** `esm_class` message type `INTERMEDIATE_DELIVERY` (0x20) is classified as a message today, so a peer that reports non-final states with it hands the application a raw `id:… stat:ENROUTE` text as an inbound From 35dd1e7678c7054c4173cc1966945118cf597d83 Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Tue, 1 Sep 2026 10:08:56 +0200 Subject: [PATCH 08/19] Count every request that went out unanswered, and gate a send on a bound link --- AGENTS.md | 38 +++++++++--- README.md | 25 +++++--- src/link-gate.ts | 68 +++++++++++++------- src/pending-requests.ts | 11 ++-- src/session.ts | 43 +++++++------ test/session-extras.test.ts | 120 ++++++++++++++++++++++++++++++++++-- todo.md | 16 ++++- 7 files changed, 247 insertions(+), 74 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 03f2087..a86b4cf 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -351,12 +351,32 @@ exactly 140. resent.** Maintainer's call, 2026-09-01: re-queueing everything unanswered would resend a `submit_sm` the SMSC accepted and answered into a dead socket, which is delivered and billed twice, while a request that never left this process can be lost for free. `LinkGate` holds a send - that has no link and `comeBackUp()` opens it once the rebind is bound, so a send issued between - links and a segment still queued behind a full window when the drop hit both go out on the new - one. The hold is bounded by `responseTimeout` rather than an option of its own — that is already - the answer to how long one request may take — so the worst case is twice it: the hold, then the - answer. It happens before `window.acquire()` rather than inside it, because the rebind's own - `bind()` goes through `session.send()` and would deadlock behind slots held by waiting sends. - `teardown()` settles what was on the wire with `UnansweredError`, which `collectSent()` counts - into `SendSmsResult.unanswered`: `smsIds` alone cannot tell a message that never left from one - whose every segment the peer took and answered into a socket that was already gone. + that has no link, so a send issued between links and a segment still queued behind a full window + when the drop hit both go out on the new one. Once a request has been written, every way it can + fail — the link dropping under it, `responseTimeout` expiring, the caller's own abort — means the + peer may have taken it, so `attempt()` wraps all three in `UnansweredError` and `collectSent()` + counts them into `SendSmsResult.unanswered`. Counting only the dropped-link case, as the first cut + did, would have called the commonest one safe to resend. A count rather than a boolean because + `sendSms()` aggregates segments into one `err` slot, and required rather than optional so every + construction site answers. `UnansweredError` stays unexported: `unanswered` is the one spelling on + the public surface, and a `send()` error that is neither a build failure nor a pre-write abort + means the same thing. + The hold is bounded by `responseTimeout` rather than an option of its own — that is already the + answer to how long one request may wait. It bounds the hold and the answer separately, and the + wait for a `maxOutstanding` slot is bounded by nothing, so `responseTimeout` is not a deadline for + the call; `SendOptions.signal` with `AbortSignal.timeout()` is, and both the gate and + `pending.wait()` honour it. + +- **The gate decides whether a link can carry a request, and a bind is what makes it one.** + Maintainer's call, 2026-09-01: `attach()` clears `closed` the moment a socket is handed over, one + round trip before the bind is answered, so gating on `closed` let a send arriving in that window + go out unbound and come back `ESME_RINVBNDSTS` while a send that arrived a millisecond earlier was + held correctly. `LinkGate` owns the answer instead — `shut(returning)` on every teardown, + `open()` only once `comeBackUp()` has a bound link — and `Session.linkDown()` reads it rather than + `closed`. The bind itself cannot wait for what it creates, so `send()` lets the three bind + commands past the gate and the window, the same door `unbind()` takes through `attempt()`. That + keeps the exemption a predicate on the command, like the `_resp` guard beside it, rather than a + second `send()` on the public surface or a changed `ReconnectOptions.onConnected`. + The gate is told what happened and never reads back into the session: a collaborator that has to + ask does not own its decision, which is how the first cut ended up answering the same question two + different ways at admit and at release. diff --git a/README.md b/README.md index fdd0510..92778c0 100644 --- a/README.md +++ b/README.md @@ -141,11 +141,11 @@ const { err, pduObjs, smsIds, unanswered } = await session.sendSms({ from, messa `err` is set when the SMSC refuses a segment, and it names the status it refused with. Because every segment goes on the wire together, `pduObjs` and `smsIds` then hold what the SMSC did accept — enough to reconcile against a later receipt, not enough to resend the rest, so treat a partial failure as a -failed message. `unanswered` counts the segments the link dropped under: the SMSC may have taken -each of them and lost only the response, so a message with `unanswered` above zero cannot be sent -again without risking a duplicate, however empty `smsIds` is. A message needing more than 255 -segments is refused before anything is sent, since -the concatenation header numbers segments in a single octet. `maxSegments` lowers that ceiling: +failed message. `unanswered` counts the segments that went out and were never answered: the SMSC may +have taken each of them and lost only the response, so a message with `unanswered` above zero cannot +be sent again without risking a duplicate, however empty `smsIds` is. A message needing more than 255 +segments is refused before anything is sent, since the concatenation header numbers segments in a +single octet. `maxSegments` lowers that ceiling: most handsets and SMSCs stop well short of 255, and refusing beats a message only half delivered. ### Receiving @@ -344,11 +344,16 @@ const { err, pduObj } = await session.send({ }); ``` -A send issued while the link is down waits for the reconnect instead of failing, and goes out on the -new link once it is bound — up to `responseTimeout`, after which it gives up having sent nothing. A -request already on the wire when the link drops is the other case: the SMSC may have taken it and -lost only the response, so it fails, and `sendSms()` counts it in `unanswered`. Neither happens with -`reconnect: false`, where a drop ends the session and every send after it is refused. +A send issued while the link is down waits for the reconnect instead of failing, and goes out once +the new link is bound — up to `responseTimeout`, after which it gives up having sent nothing. A +request already on the wire is the other case: the SMSC may have taken it and lost only the response, +so it fails, and `sendSms()` counts it in `unanswered`, whether the link dropped under it, the peer +never answered in time, or you aborted it after it went out. Neither applies with `reconnect: false`, +where a drop ends the session and every send after it is refused. + +`responseTimeout` bounds the wait for a link and the wait for an answer separately, and a send also +queues for a `maxOutstanding` slot, which nothing bounds — so it is not a deadline for the call. +Pass `{ signal: AbortSignal.timeout(ms) }` when you need one. `acceptsOptionalParams()` answers whether the peer declared SMPP 3.4 or later, which is the version at and above which the spec allows optional parameters to be sent to it; `peerInterfaceVersion` is diff --git a/src/link-gate.ts b/src/link-gate.ts index d97293e..6e3bc9f 100644 --- a/src/link-gate.ts +++ b/src/link-gate.ts @@ -1,57 +1,81 @@ import type { VoidResult } from './result.ts'; export type LinkGateOptions = { - /** Whether the link can carry nothing right now. */ - isDown: () => boolean; + now?: (() => number) | undefined; /** How long a request may wait for a link. 0 waits for as long as one may still arrive. */ timeout: number; - /** Whether a link that is down will be brought back. */ - willReturn: () => boolean; }; type Waiter = (result: VoidResult) => void; +function aborted(): Error { + return new Error('Aborted while waiting for a link'); +} + function expired(): Error { return new Error('The link did not come back in time'); } -/** Where a request with no link to go out on waits for the next one. */ +function over(): Error { + return new Error('Session is closed'); +} + +/** + * Where a request with no link to go out on waits for the next one. The owner reports what became of + * the link; nothing here reads back into the owner to find out. + */ export class LinkGate { - private readonly options: LinkGateOptions; + private readonly now: () => number; + private readonly timeout: number; private readonly waiting = new Set(); + private returning = false; + private up = true; constructor(options: LinkGateOptions) { - this.options = options; + this.now = options.now ?? Date.now; + this.timeout = options.timeout; + } + + /** Whether a request can go out right now. A link that is attached but not yet bound cannot. */ + isUp(): boolean { + return this.up; } /** When a hold starting now has to give up. 0 never does. */ deadline(): number { - return this.options.timeout > 0 ? Date.now() + this.options.timeout : 0; + return this.timeout > 0 ? this.now() + this.timeout : 0; + } + + /** A link is up and bound: everything held goes out on it. */ + open(): void { + this.up = true; + this.returning = false; + this.release({}); + } + + /** The link is gone. `returning` says whether another one is on its way. */ + shut(returning: boolean): void { + this.up = false; + this.returning = returning; + + if (!returning) this.release({ err: over() }); } /** Resolves once a link can carry the request, or with the reason none ever will. */ wait(deadline: number, signal: AbortSignal | undefined): Promise { - if (!this.options.isDown()) return Promise.resolve({}); + if (this.up) return Promise.resolve({}); - if (!this.options.willReturn()) return Promise.resolve({ err: new Error('Session is closed') }); + if (!this.returning) return Promise.resolve({ err: over() }); - const left = deadline === 0 ? 0 : deadline - Date.now(); + if (signal?.aborted === true) return Promise.resolve({ err: aborted() }); + + const left = deadline === 0 ? 0 : deadline - this.now(); if (deadline !== 0 && left <= 0) return Promise.resolve({ err: expired() }); return this.hold(left, signal); } - /** A link is up: everything held goes out on it. */ - open(): void { - this.release({}); - } - - /** No link is coming, and this is why. */ - shut(err: Error): void { - this.release({ err }); - } - private hold(left: number, signal: AbortSignal | undefined): Promise { return new Promise(resolve => { let timer: NodeJS.Timeout | undefined = undefined; @@ -64,7 +88,7 @@ export class LinkGate { }; function onAbort(): void { - settle({ err: new Error('Aborted while waiting for a link') }); + settle({ err: aborted() }); } if (left > 0) { diff --git a/src/pending-requests.ts b/src/pending-requests.ts index 515a335..5fdbb1e 100644 --- a/src/pending-requests.ts +++ b/src/pending-requests.ts @@ -12,10 +12,10 @@ type Pending = { settle: (result: Result<{ pduObj: PduObject }>) => void; }; -/** The request went out and the link died before an answer: the peer may have accepted it. */ +/** The request went out and no answer came back: the peer may have accepted it. */ export class UnansweredError extends Error { - constructor() { - super('The link dropped after the request went out; the peer may have accepted it'); + constructor(cause: Error) { + super(`No answer came back, so the peer may have accepted it: ${cause.message}`, { cause }); this.name = 'UnansweredError'; } } @@ -77,10 +77,9 @@ export class PendingRequests { this.pending.get(seqNr)?.settle(result); } - /** Everything still on the wire when the link died, each one possibly accepted by the peer. */ - settleAll(): void { + settleAll(err: Error): void { for (const [seqNr] of this.pending) { - this.settle(seqNr, { err: new UnansweredError() }); + this.settle(seqNr, { err }); } } diff --git a/src/session.ts b/src/session.ts index a9f4f80..6f6d65d 100644 --- a/src/session.ts +++ b/src/session.ts @@ -13,7 +13,7 @@ import { IncomingRequests } from './incoming-requests.ts'; import { LinkGate } from './link-gate.ts'; import { LinkTimers } from './link-timers.ts'; import { PduTransport } from './pdu-transport.ts'; -import { PendingRequests } from './pending-requests.ts'; +import { PendingRequests, UnansweredError } from './pending-requests.ts'; import { ReconnectLoop } from './reconnect-loop.ts'; import { SendWindow } from './send-window.ts'; import { errorFrom } from './error-from.ts'; @@ -39,8 +39,12 @@ export { bindCommands, defaultSystemId }; /** A listener may return a promise: an `async` one that rejects is routed like one that throws. */ type SessionListener = (...args: SessionEvents[K]) => unknown; -/** `unsent` means nothing reached the socket, so the next link can still carry this request. */ -type Attempt = { result: Result<{ pduObj: PduObject }>; unsent: boolean }; +function abortedBeforeSend(): Error { + return new Error('Aborted before the request was sent'); +} + +/** `retryOnNextLink`: the write failed, so nothing reached the socket and another link may carry it. */ +type Attempt = { result: Result<{ pduObj: PduObject }>; retryOnNextLink: boolean }; export class Session extends EventEmitter { declare addListener: (event: K, listener: SessionListener) => this; @@ -117,11 +121,7 @@ export class Session extends EventEmitter { max: defaults.maxDlrMerges, timeout: defaults.dlrMergeTimeout, }); - this.gate = new LinkGate({ - isDown: () => this.linkDown(), - timeout: options.responseTimeout ?? defaults.responseTimeout, - willReturn: () => this.retrying(), - }); + this.gate = new LinkGate({ timeout: options.responseTimeout ?? defaults.responseTimeout }); this.incoming = new IncomingRequests({ dlrMerger: this.dlrMerger, log: this.log, @@ -174,6 +174,9 @@ export class Session extends EventEmitter { if (refused) return { err: refused }; + // A bind is what makes a link usable, so it cannot wait for one. The door unbind() uses too. + if (bindCommands.includes(input.cmdName)) return (await this.attempt(input, options)).result; + const deadline = this.gate.deadline(); for (;;) { @@ -186,7 +189,7 @@ export class Session extends EventEmitter { const attempt = await this.attempt(input, options).finally(() => { this.window.release(); }); // Nothing reached the socket, so the next link carries it instead of the caller resending. - if (!attempt.unsent || !this.linkDown() || !this.retrying()) return attempt.result; + if (!attempt.retryOnNextLink || !this.linkDown() || !this.retrying()) return attempt.result; } } @@ -199,15 +202,15 @@ export class Session extends EventEmitter { // A drain on a live link. A link that is down is the gate's answer, which says closed instead. if (this.draining && !this.linkDown()) return new Error('Session is shutting down'); - // Before the gate and the window, or an aborted call waits for a link it will not use. - if (options.signal?.aborted === true) return new Error('Aborted before the request was sent'); + // Before the gate and the window, or an aborted call waits for what it will never use. + if (options.signal?.aborted === true) return abortedBeforeSend(); return undefined; } /** Read through a method: a drop can land while a send is awaiting. */ private linkDown(): boolean { - return this.closed || this.sock.destroyed; + return !this.gate.isUp() || this.sock.destroyed; } /** Answers a request the peer sent us. Responses are never waited on. */ @@ -349,13 +352,13 @@ export class Session extends EventEmitter { private async attempt(input: PduObjectInput, options: SendOptions): Promise { // pending.wait() alone settles the caller while the request still goes out to the peer. if (options.signal?.aborted === true) { - return { result: { err: new Error('Aborted before the request was sent') }, unsent: false }; + return { result: { err: abortedBeforeSend() }, retryOnNextLink: false }; } const seqNr = this.pending.nextSeqNr(); const built = objToPdu({ ...input, seqNr }); - if (built.err) return { result: { err: built.err }, unsent: false }; + if (built.err) return { result: { err: built.err }, retryOnNextLink: false }; const response = this.pending.wait(seqNr, { signal: options.signal, @@ -366,10 +369,13 @@ export class Session extends EventEmitter { if (written.err) { this.pending.settle(seqNr, { err: written.err }); - return { result: { err: written.err }, unsent: true }; + return { result: { err: written.err }, retryOnNextLink: true }; } - return { result: await response, unsent: false }; + const answered = await response; + + // It went out, so a failure now means the peer may have taken it and the answer was the loss. + return { result: answered.err ? { err: new UnansweredError(answered.err) } : answered, retryOnNextLink: false }; } /** Stops new sends and waits out the ones already issued. */ @@ -404,7 +410,7 @@ export class Session extends EventEmitter { if (this.ended) return; this.ended = true; - this.gate.shut(new Error('Session closed before the link came back')); + this.gate.shut(false); this.emit('close'); } @@ -412,8 +418,9 @@ export class Session extends EventEmitter { if (this.closed) return; this.closed = true; + this.gate.shut(this.retrying()); this.timers.clear(); - this.pending.settleAll(); + this.pending.settleAll(new Error('Session closed before a response arrived')); this.incoming.clear(); this.sock.destroy(); diff --git a/test/session-extras.test.ts b/test/session-extras.test.ts index 9030830..96af9b0 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -12,6 +12,7 @@ import type { SmppLog } from '../src/log.ts'; import type { Sms } from '../src/sms.ts'; import type { SmppServer } from '../src/server.ts'; import type { TestContext } from 'node:test'; +import { LinkGate } from '../src/link-gate.ts'; import { Reassembler, decodeSegments } from '../src/reassembly.ts'; import { Session } from '../src/session.ts'; import { DlrMerger } from '../src/dlr-merger.ts'; @@ -100,10 +101,10 @@ async function sendReceipt(peer: Session, smsId: string, tlvSmsId = smsId): Prom assert.equal(sent.err, undefined); } -type Gate = { open: () => void; passed: Promise }; +type Latch = { open: () => void; passed: Promise }; /** A promise the test opens by hand, guarded by once() against waiting on one it never does. */ -function gate(): Gate { +function latch(): Latch { const opener: { open?: () => void } = {}; const passed = once(resolve => { opener.open = () => { resolve(true); }; }); @@ -559,8 +560,8 @@ describe('reconnect', () => { describe('sends across a reconnect', () => { /** Answers every message after the first, which is left to hold the send window open. */ - function answerAfterTheFirst(smpp: SmppServer, arrived: string[]): Gate { - const first = gate(); + function answerAfterTheFirst(smpp: SmppServer, arrived: string[]): Latch { + const first = latch(); smpp.on('session', peer => { peer.on('sms', async sms => { @@ -624,6 +625,87 @@ describe('sends across a reconnect', () => { assert.deepEqual(arrived, ['first', 'second']); }); + test('holds a send issued while the rebind is still binding', async t => { + const binding = latch(); + const release = latch(); + let binds = 0; + const smpp = await startServer(t, { + authenticate: async () => { + binds++; + + if (binds > 1) { + binding.open(); + await release.passed; + } + + return true; + }, + }); + + smpp.on('session', peer => { peer.on('sms', async sms => { await sms.sendResp(); }); }); + + const { session } = await connect(t, smpp, { reconnect: { maxDelay: 100, minDelay: 20 } }); + + assert.ok(session); + + peerOf(smpp).sock.destroy(); + + // The fresh socket is attached and the bind is in flight, so the link exists but carries nothing. + await binding.passed; + + const sending = session.sendSms({ from: '46701113311', message: 'mid-bind', to: '46709771337' }); + let settled = false; + + void sending.then(() => { settled = true; }); + await delay(30); + + assert.equal(settled, false, 'an unbound link must not take a submit_sm that would be refused'); + + release.open(); + + const sent = await sending; + + assert.equal(sent.err, undefined); + assert.equal(sent.smsIds.length, 1); + }); + + test('counts a segment the peer never answered in time as unanswered', async t => { + const smpp = await startServer(t); + + smpp.on('session', peer => { peer.on('sms', () => undefined); }); + + const { session } = await connect(t, smpp, { responseTimeout: 60 }); + + assert.ok(session); + + const sent = await session.sendSms({ from: '46701113311', message: 'no answer', to: '46709771337' }); + + assert.match(sent.err?.message ?? '', /may have accepted/); + assert.equal(sent.unanswered, 1, 'a slow SMSC may still have taken it'); + }); + + test('counts a segment aborted after it went out as unanswered', async t => { + const smpp = await startServer(t); + const arrived = once(resolve => { smpp.on('session', peer => peer.on('sms', resolve)); }); + const { session } = await connect(t, smpp); + + assert.ok(session); + + const controller = new AbortController(); + const sending = session.sendSms( + { from: '46701113311', message: 'aborted mid-flight', to: '46709771337' }, + { signal: controller.signal }, + ); + + await arrived; + controller.abort(); + + const sent = await sending; + + assert.match(sent.err?.message ?? '', /may have accepted/); + assert.equal(sent.unanswered, 1, 'the abort is ours; the peer still holds the request'); + }); + test('reports a segment the link dropped under as unanswered, not as never sent', async t => { const smpp = await startServer(t); const arrived = once(resolve => { smpp.on('session', peer => peer.on('sms', resolve)); }); @@ -733,6 +815,32 @@ describe('sends across a reconnect', () => { }); }); +describe('LinkGate', () => { + test('refuses a hold whose deadline has already passed', async () => { + let now = 0; + const gate = new LinkGate({ now: () => now, timeout: 100 }); + const deadline = gate.deadline(); + + gate.shut(true); + now = 101; + + const held = await gate.wait(deadline, undefined); + + assert.match(held.err?.message ?? '', /did not come back in time/); + }); + + // addEventListener never fires for a signal that already aborted, so it would wait out the timeout. + test('gives up at once on a signal that was already aborted', async () => { + const gate = new LinkGate({ timeout: 100 }); + + gate.shut(true); + + const held = await gate.wait(gate.deadline(), AbortSignal.abort()); + + assert.match(held.err?.message ?? '', /Aborted while waiting for a link/); + }); +}); + describe('reassembly bounds', () => { function segment(reference: number, part: number, total: number): PduObject { const udh = Buffer.from([0x05, 0x00, 0x03, reference, total, part]); @@ -1090,8 +1198,8 @@ describe('graceful shutdown', () => { assert.ok(first.sock); - const rebinding = gate(); - const release = gate(); + const rebinding = latch(); + const release = latch(); const session = new Session({ reconnect: { connect: open, diff --git a/todo.md b/todo.md index bfe0147..a8370ae 100644 --- a/todo.md +++ b/todo.md @@ -127,9 +127,19 @@ session message is a change to every call site. `reassembly`, `dlr-merger`, `send-window`, `link-timers`, `link-gate`, `reconnect-loop`, `pending-requests` and `send-sms`, so the directory would make that boundary visible. `pdu-transport` joined them on 2026-08-31 and `link-gate` on 2026-09-01, both without the move - being made, so it is a move of its own now. `session.ts` sits within a couple of lines of the - 350-line cap again; the outgoing request path — `send()`, `refuseSend()`, `attempt()` and the - gate and window they drive — is the next thing that would come out of it. + being made, so it is a move of its own now. Do it together with the extraction below rather + than before it — three reactive splits at whatever boundary fitted under the line cap is what + produced the current shape. Raised by review, 2026-09-01. +- [ ] **An `OutgoingRequests` collaborator, owning `LinkGate`, `SendWindow` and `PendingRequests`.** + `session.ts` sits a few lines under its 350-line cap and every split so far has been made to + get back under it. The seam that holds: one object owning the gate, the window, the pending + map and the retry loop, exposing a gated `request()` and the ungated door `unbind()` and the + bind already need, with `Session` calling it when a link comes up or goes down instead of + spreading `linkDown()` and `retrying()` across both sides. It would also close two smaller + things — `LinkGate.deadline()` and `wait(deadline)` are a two-call protocol whose only failure + mode is calling `deadline()` inside the loop, which nothing catches; and `Session.linkDown()` + is read from both sides of the seam. Every one of these is unpublished, so it is a two-way + door and belongs after 1.0.0. Raised by review, 2026-09-01. - [ ] **Does an intermediate delivery notification deserve to be a `dlr`?** `esm_class` message type `INTERMEDIATE_DELIVERY` (0x20) is classified as a message today, so a peer that reports non-final states with it hands the application a raw `id:… stat:ENROUTE` text as an inbound From 58817eebbfec3829f4f601435ef9b96621c25a3a Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Tue, 1 Sep 2026 10:15:07 +0200 Subject: [PATCH 09/19] Loop a held send on the gate's own answer, not on the socket too --- AGENTS.md | 8 +++++++- src/session.ts | 3 ++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index a86b4cf..5d66444 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -379,4 +379,10 @@ exactly 140. second `send()` on the public surface or a changed `ReconnectOptions.onConnected`. The gate is told what happened and never reads back into the session: a collaborator that has to ask does not own its decision, which is how the first cut ended up answering the same question two - different ways at admit and at release. + different ways at admit and at release. For the same reason the retry in `send()` asks + `gate.isUp()` rather than `linkDown()`, which also reads the socket: a condition that loops on + something the gate does not gate on spins against a gate that admits it straight back. + `LinkGate.returning` is a copy of `retrying()` taken at teardown, and stays true only because + nothing stops the reconnect loop without `emitClose()` following it — `drain()` and `end()` are the + only callers of `stop()`. An `OutgoingRequests` owning both would not need the copy; until then, + a third caller of `stop()` has to shut the gate itself. diff --git a/src/session.ts b/src/session.ts index 6f6d65d..8007073 100644 --- a/src/session.ts +++ b/src/session.ts @@ -189,7 +189,8 @@ export class Session extends EventEmitter { const attempt = await this.attempt(input, options).finally(() => { this.window.release(); }); // Nothing reached the socket, so the next link carries it instead of the caller resending. - if (!attempt.retryOnNextLink || !this.linkDown() || !this.retrying()) return attempt.result; + // The gate's own answer, or a loop round one that admits everything spins on a dead socket. + if (!attempt.retryOnNextLink || this.gate.isUp() || !this.retrying()) return attempt.result; } } From 2c0dfd172ff8377b69e75ea00c43172d003d3977 Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Tue, 1 Sep 2026 10:38:16 +0200 Subject: [PATCH 10/19] Keep the process alive while a send waits for a link --- AGENTS.md | 6 +++++- src/link-gate.ts | 36 +++++++++++++++++++++++++++--------- src/send-sms.ts | 2 +- src/session.ts | 10 +++++----- test/session-extras.test.ts | 28 +++++++++++++++++++++++----- test/session.test.ts | 2 +- todo.md | 5 +++-- 7 files changed, 65 insertions(+), 24 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 5d66444..ef3396c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -365,7 +365,11 @@ exactly 140. answer to how long one request may wait. It bounds the hold and the answer separately, and the wait for a `maxOutstanding` slot is bounded by nothing, so `responseTimeout` is not a deadline for the call; `SendOptions.signal` with `AbortSignal.timeout()` is, and both the gate and - `pending.wait()` honour it. + `pending.wait()` honour it. The hold's clock starts when the send is issued rather than when it + first finds the gate shut, so one budget covers every hold a single call makes — a send that spent + it queued behind the window is refused rather than held. The hold timer is the one timer here that + is not `unref()`'d: a held request is awaited with the socket already destroyed, so an unref'd one + lets a process whose only remaining work is that send exit without settling it. - **The gate decides whether a link can carry a request, and a bind is what makes it one.** Maintainer's call, 2026-09-01: `attach()` clears `closed` the moment a socket is handed over, one diff --git a/src/link-gate.ts b/src/link-gate.ts index 6e3bc9f..a86e1d0 100644 --- a/src/link-gate.ts +++ b/src/link-gate.ts @@ -1,6 +1,8 @@ +import type { SmppLog } from './log.ts'; import type { VoidResult } from './result.ts'; export type LinkGateOptions = { + log: SmppLog; now?: (() => number) | undefined; /** How long a request may wait for a link. 0 waits for as long as one may still arrive. */ timeout: number; @@ -20,11 +22,9 @@ function over(): Error { return new Error('Session is closed'); } -/** - * Where a request with no link to go out on waits for the next one. The owner reports what became of - * the link; nothing here reads back into the owner to find out. - */ +/** Where a request with no link to go out on waits for the next one. */ export class LinkGate { + private readonly log: SmppLog; private readonly now: () => number; private readonly timeout: number; private readonly waiting = new Set(); @@ -32,6 +32,7 @@ export class LinkGate { private up = true; constructor(options: LinkGateOptions) { + this.log = options.log; this.now = options.now ?? Date.now; this.timeout = options.timeout; } @@ -41,6 +42,11 @@ export class LinkGate { return this.up; } + /** Why the gate will never admit a request, or undefined while one may still get through. */ + refusal(): Error | undefined { + return this.up || this.returning ? undefined : over(); + } + /** When a hold starting now has to give up. 0 never does. */ deadline(): number { return this.timeout > 0 ? this.now() + this.timeout : 0; @@ -50,6 +56,11 @@ export class LinkGate { open(): void { this.up = true; this.returning = false; + + if (this.waiting.size > 0) { + this.log.verbose('linkGate - sending what was held for a link', { held: this.waiting.size }); + } + this.release({}); } @@ -65,7 +76,9 @@ export class LinkGate { wait(deadline: number, signal: AbortSignal | undefined): Promise { if (this.up) return Promise.resolve({}); - if (!this.returning) return Promise.resolve({ err: over() }); + const refused = this.refusal(); + + if (refused) return Promise.resolve({ err: refused }); if (signal?.aborted === true) return Promise.resolve({ err: aborted() }); @@ -77,6 +90,8 @@ export class LinkGate { } private hold(left: number, signal: AbortSignal | undefined): Promise { + this.log.verbose('linkGate - holding a request until a link is back', { timeout: left }); + return new Promise(resolve => { let timer: NodeJS.Timeout | undefined = undefined; const settle = (result: VoidResult): void => { @@ -86,15 +101,18 @@ export class LinkGate { this.waiting.delete(settle); resolve(result); }; + const giveUp = (): void => { + this.log.warn('linkGate - no link came back in time', { timeout: left }); + settle({ err: expired() }); + }; function onAbort(): void { settle({ err: aborted() }); } - if (left > 0) { - timer = setTimeout(() => { settle({ err: expired() }); }, left); - timer.unref(); - } + // Deliberately not unref()'d: a held request is awaited with a destroyed socket and no + // other handle, so an unref'd timer lets the process exit without ever settling it. + if (left > 0) timer = setTimeout(giveUp, left); signal?.addEventListener('abort', onAbort, { once: true }); this.waiting.add(settle); diff --git a/src/send-sms.ts b/src/send-sms.ts index 96689df..d329846 100644 --- a/src/send-sms.ts +++ b/src/send-sms.ts @@ -33,7 +33,7 @@ export type SendSmsResult = { err?: Error; pduObjs: PduObject[]; smsIds: string[]; - /** Segments the link dropped under. The peer may have taken them, so sending again may duplicate. */ + /** Segments that went out unanswered. The peer may have taken them, so sending again may duplicate. */ unanswered: number; }; diff --git a/src/session.ts b/src/session.ts index 8007073..247d62f 100644 --- a/src/session.ts +++ b/src/session.ts @@ -121,7 +121,7 @@ export class Session extends EventEmitter { max: defaults.maxDlrMerges, timeout: defaults.dlrMergeTimeout, }); - this.gate = new LinkGate({ timeout: options.responseTimeout ?? defaults.responseTimeout }); + this.gate = new LinkGate({ log: this.log, timeout: options.responseTimeout ?? defaults.responseTimeout }); this.incoming = new IncomingRequests({ dlrMerger: this.dlrMerger, log: this.log, @@ -174,7 +174,7 @@ export class Session extends EventEmitter { if (refused) return { err: refused }; - // A bind is what makes a link usable, so it cannot wait for one. The door unbind() uses too. + // A bind is what makes a link usable, so it cannot wait for one. if (bindCommands.includes(input.cmdName)) return (await this.attempt(input, options)).result; const deadline = this.gate.deadline(); @@ -189,7 +189,6 @@ export class Session extends EventEmitter { const attempt = await this.attempt(input, options).finally(() => { this.window.release(); }); // Nothing reached the socket, so the next link carries it instead of the caller resending. - // The gate's own answer, or a loop round one that admits everything spins on a dead socket. if (!attempt.retryOnNextLink || this.gate.isUp() || !this.retrying()) return attempt.result; } } @@ -206,7 +205,8 @@ export class Session extends EventEmitter { // Before the gate and the window, or an aborted call waits for what it will never use. if (options.signal?.aborted === true) return abortedBeforeSend(); - return undefined; + // A bind skips the gate below, so the answer it would have given is given here instead. + return bindCommands.includes(input.cmdName) ? this.gate.refusal() : undefined; } /** Read through a method: a drop can land while a send is awaiting. */ @@ -338,9 +338,9 @@ export class Session extends EventEmitter { } this.resetTimers(); + this.gate.open(); this.log.info('session - reconnected'); this.emit('reconnected'); - this.gate.open(); return {}; } diff --git a/test/session-extras.test.ts b/test/session-extras.test.ts index 96af9b0..17b23c0 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -674,7 +674,7 @@ describe('sends across a reconnect', () => { smpp.on('session', peer => { peer.on('sms', () => undefined); }); - const { session } = await connect(t, smpp, { responseTimeout: 60 }); + const { session } = await connect(t, smpp, { responseTimeout: 200 }); assert.ok(session); @@ -729,7 +729,7 @@ describe('sends across a reconnect', () => { const smpp = await startServer(t); const { session } = await connect(t, smpp, { reconnect: { maxDelay: 10_000, minDelay: 10_000 }, - responseTimeout: 60, + responseTimeout: 200, }); assert.ok(session); @@ -818,7 +818,7 @@ describe('sends across a reconnect', () => { describe('LinkGate', () => { test('refuses a hold whose deadline has already passed', async () => { let now = 0; - const gate = new LinkGate({ now: () => now, timeout: 100 }); + const gate = new LinkGate({ log: silentLog, now: () => now, timeout: 100 }); const deadline = gate.deadline(); gate.shut(true); @@ -829,9 +829,27 @@ describe('LinkGate', () => { assert.match(held.err?.message ?? '', /did not come back in time/); }); + // A held send is awaited with the socket destroyed, so an unref'd timer here lets a process whose + // only remaining work is that send exit without ever settling it, losing the message silently. + test('holds on a timer that keeps the process alive', async () => { + const gate = new LinkGate({ log: silentLog, timeout: 10_000 }); + const timers = (): number => process.getActiveResourcesInfo().filter(name => name === 'Timeout').length; + + gate.shut(true); + + const before = timers(); + const held = gate.wait(gate.deadline(), undefined); + + assert.equal(timers(), before + 1, 'an unref\'d timer is not counted here, which is the point'); + + gate.open(); + + assert.deepEqual(await held, {}); + }); + // addEventListener never fires for a signal that already aborted, so it would wait out the timeout. test('gives up at once on a signal that was already aborted', async () => { - const gate = new LinkGate({ timeout: 100 }); + const gate = new LinkGate({ log: silentLog, timeout: 100 }); gate.shut(true); @@ -1073,7 +1091,7 @@ describe('graceful shutdown', () => { const result = await sent; assert.ok(result.err instanceof Error); - assert.match(result.err.message, /may have accepted/); + assert.match(result.err.message, /may have accepted.*Session closed before a response arrived/); assert.equal(result.unanswered, 1); }); diff --git a/test/session.test.ts b/test/session.test.ts index a8e382b..d7a23d3 100644 --- a/test/session.test.ts +++ b/test/session.test.ts @@ -236,7 +236,7 @@ describe('bind', () => { const sent = await session.send({ cmdName: 'enquire_link' }); assert.ok(sent.err instanceof Error); - assert.match(sent.err.message, /may have accepted/); + assert.match(sent.err.message, /may have accepted.*Session closed before a response arrived/); }); test('reports an unbind the peer left unanswered on a link that stays up', async t => { diff --git a/todo.md b/todo.md index a8370ae..d20940b 100644 --- a/todo.md +++ b/todo.md @@ -137,8 +137,9 @@ session message is a change to every call site. bind already need, with `Session` calling it when a link comes up or goes down instead of spreading `linkDown()` and `retrying()` across both sides. It would also close two smaller things — `LinkGate.deadline()` and `wait(deadline)` are a two-call protocol whose only failure - mode is calling `deadline()` inside the loop, which nothing catches; and `Session.linkDown()` - is read from both sides of the seam. Every one of these is unpublished, so it is a two-way + mode is calling `deadline()` inside the loop, which nothing catches; `Session.linkDown()` is + read from both sides of the seam; and `UnansweredError` sits in `pending-requests.ts`, which + never uses it, for the sole edge that makes `send-sms` import that module at all. Every one of these is unpublished, so it is a two-way door and belongs after 1.0.0. Raised by review, 2026-09-01. - [ ] **Does an intermediate delivery notification deserve to be a `dlr`?** `esm_class` message type `INTERMEDIATE_DELIVERY` (0x20) is classified as a message today, so a peer that reports From 2fe36fa4e80f1612732cd4369c00b96049d8a805 Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Tue, 1 Sep 2026 10:48:20 +0200 Subject: [PATCH 11/19] Say once why the hold timer is not unref'd --- src/link-gate.ts | 3 +-- test/session-extras.test.ts | 2 -- todo.md | 5 +++-- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/link-gate.ts b/src/link-gate.ts index a86e1d0..a6a51b2 100644 --- a/src/link-gate.ts +++ b/src/link-gate.ts @@ -110,8 +110,7 @@ export class LinkGate { settle({ err: aborted() }); } - // Deliberately not unref()'d: a held request is awaited with a destroyed socket and no - // other handle, so an unref'd timer lets the process exit without ever settling it. + // Not unref()'d: a held request is awaited with no other handle, so the process would exit unsettled. if (left > 0) timer = setTimeout(giveUp, left); signal?.addEventListener('abort', onAbort, { once: true }); diff --git a/test/session-extras.test.ts b/test/session-extras.test.ts index 17b23c0..4770767 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -829,8 +829,6 @@ describe('LinkGate', () => { assert.match(held.err?.message ?? '', /did not come back in time/); }); - // A held send is awaited with the socket destroyed, so an unref'd timer here lets a process whose - // only remaining work is that send exit without ever settling it, losing the message silently. test('holds on a timer that keeps the process alive', async () => { const gate = new LinkGate({ log: silentLog, timeout: 10_000 }); const timers = (): number => process.getActiveResourcesInfo().filter(name => name === 'Timeout').length; diff --git a/todo.md b/todo.md index d20940b..52c9db1 100644 --- a/todo.md +++ b/todo.md @@ -139,8 +139,9 @@ session message is a change to every call site. things — `LinkGate.deadline()` and `wait(deadline)` are a two-call protocol whose only failure mode is calling `deadline()` inside the loop, which nothing catches; `Session.linkDown()` is read from both sides of the seam; and `UnansweredError` sits in `pending-requests.ts`, which - never uses it, for the sole edge that makes `send-sms` import that module at all. Every one of these is unpublished, so it is a two-way - door and belongs after 1.0.0. Raised by review, 2026-09-01. + never uses it, for the sole edge that makes `send-sms` import that module at all. Every one of + these is unpublished, so it is a two-way door and belongs after 1.0.0. Raised by review, + 2026-09-01. - [ ] **Does an intermediate delivery notification deserve to be a `dlr`?** `esm_class` message type `INTERMEDIATE_DELIVERY` (0x20) is classified as a message today, so a peer that reports non-final states with it hands the application a raw `id:… stat:ENROUTE` text as an inbound From a99e1227ac4da5e6e4853247c2c473d2b9b420a6 Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Tue, 1 Sep 2026 12:21:58 +0200 Subject: [PATCH 12/19] Give the project ordered goals and let them decide what a decision record is --- AGENTS.md | 426 ++++++++++++++++++++++++++++-------------------------- todo.md | 29 ++-- 2 files changed, 230 insertions(+), 225 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index ef3396c..3151286 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1,7 +1,7 @@ # AGENTS.md -Guidance for LLM agents working in this repository. Human-facing documentation lives in -[README.md](README.md); the remaining work is tracked in [todo.md](todo.md). +Guidance for LLM agents working in this repository. What each file in it is for is under +[Documentation](#documentation). ## What this is @@ -10,7 +10,35 @@ started from an orphan commit — no history from 0.4.0 is carried over. The 0.4 readable on the `master` branch of the same repository and is the reference for protocol behaviour, not for structure or style. -The library's value is its very small API. Do not grow the public surface without being asked. +## Goals + +In priority order, and the order is the point: where two of them pull against each other, the earlier +one wins. They do not override the hard rules below. + +1. **Correct on the wire.** SMPP 3.4 as SMSCs actually run it. Every other goal yields to this one; + the defect table below is what the alternative costs. +2. **Never give the application a wrong answer about what happened.** An outcome we cannot determine + is reported as undetermined rather than guessed; a request the peer may already have taken is + never re-sent on the library's own initiative; work the peer has no reason to send again is not + dropped. +3. **Strict in what we send, generous in what we read.** The library's own senders follow 3.4, and + the codec parses whatever arrives. Where the letter of the spec would discard traffic a real SMSC + sends, keep the traffic. +4. **A peer an operator never has to complain about.** No bind flooding, nothing a bind direction + forbids, no optional parameters to a peer that declared none, nothing held without a bound. +5. **The session layer is in here, and its defaults are what most applications should run.** + Keepalive, reconnect, the send window, reassembly and receipt correlation. An option retunes a + default or opts out of it; an option does not switch on the thing the caller obviously wanted. +6. **A small, stable public surface over reshapeable internals.** Only what `src/index.ts` exports is + published. A new option has to beat "the application can do this itself", and has to keep a + promise this library can verify. The low-level surface is a passthrough: policy binds what the + library composes, never what the caller wrote. +7. **Nothing that needs state wider than one session.** No throughput throttling, no persistence + across a restart, no coordination between processes. This is the scope floor, and it is why an + otherwise reasonable feature is declined without a fresh argument each time. +8. **It builds, tests and runs the same everywhere.** Container-only toolchain, no runtime + dependencies, the Node 18 floor verified in CI rather than asserted, every README example executed + by the suite. ## Hard rules @@ -99,9 +127,10 @@ docker compose run --rm node npm run build ## Defects found in 0.4.0 -Confirmed by reading the 0.4.0 source. The rewrite fixes all of them; each needs a regression test -naming the behaviour, and the wire-affecting ones are cross-checked against a reference -implementation (see todo.md). +Every row names what 0.4.0's own code did, so it is not rebuilt here. +[README.md](README.md#behaviour-that-changed-on-the-wire) names what changed for a consumer, and is +the only place that does. Confirmed by reading the 0.4.0 source; each row has a regression test +naming the behaviour. | Defect | 0.4.0 behaviour | | --- | --- | @@ -118,13 +147,14 @@ implementation (see todo.md). | Unbounded reassembly | Incomplete long-SMS groups are capped by nothing and swept only when other traffic arrives, after 24 hours | | Dead DLR aggregation | `longSmsDlrs` is allocated to merge per-segment receipts and then never used | | Trailing NULL truncation | `types.buffer.size()` subtracts one whenever the value's last octet is `0x00`, so the PDU is allocated one octet short while `sm_length` still reports the full length. Any UCS2 message ending in a character like U+4E00 or U+3000 goes out corrupt | -| Dormant filters | `defs.filters` is declared on commands and TLVs but never invoked anywhere. Dropped in the rewrite; SMPP time formatting is exported as `smppTime` instead | -| Unchecked reads | Wire reads index straight into the buffer, so a short or malformed PDU throws out of the codec. Reads are bounds-checked and return results now | +| Dormant filters | `defs.filters` is declared on commands and TLVs but never invoked anywhere | +| Unchecked reads | Wire reads index straight into the buffer, so a short or malformed PDU throws out of the codec | | Unrangechecked writes | Integer params are handed to `writeUInt8`/`writeUInt16BE` unvalidated, so an out-of-range value throws from inside Node | | `submit_multi` missing `sm_length` | The field is commented out of the command table, so `short_message` never round-trips for that command | | Per-parameter defaults never applied | `calcCmdLength` reads `paramType.default` (the wire type's) rather than the parameter's, so `interface_version: 0x50` on the bind commands did nothing and every bind declared version 0x00 | | `source_telematics_id` width | Defined as a 2-octet integer; SMPP 3.4 5.3.2.8 makes it 1 octet, unlike `dest_telematics_id`, which really is 2 | -| Binary payloads decoded as text | `data_coding` 0x02, 0x04, 0x14 and 0xF4-0xF7 are 8-bit binary and land on the GSM 03.38 table, which rewrites every octet outside it. They resolve to LATIN1 now, so the payload survives as bytes | +| Binary payloads decoded as text | `data_coding` 0x02, 0x04, 0x14 and 0xF4-0xF7 are 8-bit binary and land on the GSM 03.38 table, which rewrites every octet outside it | +| Binary TLVs round-trip corrupt | `pduToObj` turns a `Buffer` TLV value into a hex string (`utils.js:307`), and `objToPdu` writes that string back as its own ASCII, so `message_payload`, `network_error_code`, `callback_num` and the rest are destroyed by any round trip | | `ESME_RINVBCASTCHANIND` typo | Defined as `0x011`, three hex digits; the spec value is `0x0112` | ## Multipart sends and the send window @@ -170,61 +200,186 @@ exactly 140. - `assert.equal` from `node:assert/strict` narrows its first argument, so a following `?.` on the same value is flagged as unnecessary. Assert once with `assert.ok(x)` and use plain access after. +## Documentation + +Each file answers one question, and a fact belongs to the file whose question it answers: + +- **README.md — what you can rely on.** Observable behaviour, for someone using the package. It + carries a reason only where the reason changes how you would call the thing. +- **AGENTS.md — what may not change, and why.** Goals, hard rules, architecture, conventions, and the + decisions the goals do not already settle. It does not restate behaviour README states. +- **todo.md** is a temporary working file that sets its own rules; nothing here governs it. + +A sentence living in two of them is a defect: delete the copy in the file whose question it does not +answer. The toolchain commands are the one deliberate exception — README's copy serves a contributor +who never opens this file, and this file's copy carries the constraint that nothing runs on the host. + +**Write a decision down only when it cannot be put better as a goal.** A goal decides every case that +follows from it; a decision record decides one. So reach for the goal list first — sharpen a goal, +add one, or move one up the order — and write a decision only for what is left over: a choice a +competent change would otherwise re-open, that no goal implies. Give the claim, the constraint that +settled it and the alternative rejected, and nothing the code or README already says. Where a +compiler or a test already forbids the other way, it is not a decision, it is a test name. Delete one +once it no longer constrains anything; this is not a changelog. + ## Decisions +Grouped by what each one constrains. + +### The public surface + +- **`Session` is publicly constructible, which is what makes `SessionOptions` and `ReconnectOptions` + public too.** Raised twice as a leak; it is not one. The collaborators `session.ts` delegates to + (`Reassembler`, `PendingRequests`, `SendWindow`, `ReconnectLoop`, `LinkTimers`, `LinkGate`, + `DlrMerger`, `PduTransport`, `submitSms`) stay unpublished so they can be reshaped. + +- **`acceptsOptionalParams()` and `bindAllows()` are predicates, not chokepoints.** The library's own + senders consult them; `session.send({ tlvs })` is passed through as written, because silently + stripping a caller's explicit TLVs off a deliberately public low-level surface would be worse than + sending them. Only `submit_sm` and `deliver_sm` are policed by bind direction — the only two the + library sends and dispatches by it. + +- **`session.sock` is a getter over `PduTransport`.** Reading it is unchanged; assigning it no longer + compiles, which never rewired the handlers and so never worked. + +- **Both emitters re-declare their listener methods to accept a promise.** Maintainer's call, + 2026-08-27: `EventEmitter` types every listener as void-returning, so the + `session.on('sms', async sms => …)` README documents reads as a misused promise in any strict + consumer. `declare on: …` and its six siblings re-type the inherited methods to return `unknown`, + which emits nothing and needs no cast; overriding them as real methods cannot work, because the + `super.on()` call needs one. The cost is that a subclass can no longer reach those seven through + `super` — re-declaring them the same way is its way out. `unknown` rather than + `void | Promise` because a listener may return anything: `session.on('close', () => + set.delete(session))` returns a boolean. + +### The wire + +- **The declared interface version is an option on both `client()` and `server()`, and is not the + optional-parameter threshold.** That threshold is fixed at 0x34 by the spec, so an implementation + that must declare 5.0 throughout can, without moving it. + +- **A peer that declared no version is pre-3.4, and `undefined` means no bind yet.** `acceptBind()` + records what the ESME declared and the client's `bind()` records the `sc_interface_version` the + SMSC answered with; a peer that declared nothing is recorded as `undeclaredInterfaceVersion` (0x00) + and sent no optional parameters, which is how the spec reads an absent `sc_interface_version`. + +- **`esm_class` decides what a `deliver_sm` is, and the body is read only when it names nothing.** + `MC_DELIVERY_RECEIPT` (0x04) makes it a receipt whatever the body parses to, so a receipt in a + format `dlrFromPdu()` cannot read reaches `dlr` with `smsId` undefined instead of arriving as an + inbound SMS. Any other named type is not a receipt and its body is not scraped; a message type of 0 + or one of the ten reserved keeps the scrape, and a non-empty `receipted_message_id` TLV marks a + receipt on the same footing. A receipt this library recognises never reaches the reassembler, so an + SMSC that splits one across segments gets a `dlr` per segment rather than one merged report. The + `message_state` TLV is authoritative only where it names a state in the table — SMPP reserves + 0x80-0xFF for MC-vendor-specific values, so an unnameable one keeps its raw `statusId` and leaves + `statusMsg` to the body. + +- **`smsIdFormat` names a notation per place, and normalisation never reaches inside a `-` + id.** An SMSC may answer `submit_sm_resp` in hex and write the receipt's `id:` in decimal, so one + transform over both sides cannot make them equal. `submitResp` covers the `receipted_message_id` + TLV too, which SMPP 3.4 5.3.2.26 defines as the id the `submit_sm_resp` carried: naming one + notation for whichever id a receipt yields would break the peer that sends both. Omitting a place + is what leaving it alone means, so there is no `raw` notation, and a caller-supplied formatter is + refused because it would make the promise that the two ids are comparable unverifiable — `onRequest` + and the PDU on the `dlr` event are the escape hatches. A `-` id parses as no number and so + reaches `expect()` and `collect()` unchanged, which is what keeps `DlrMerger` working; normalising + the base instead would break that pair. The option is on `client()` only, since a `server()` session + writes both ids itself. + +### The session's life + - **A close arriving after our own `unbind` is a clean unbind, not an error.** Maintainer's call, 2026-08-26: most SMSCs drop the socket instead of answering, so the documented shutdown would otherwise always report a failure. It does mask a socket that died mid-unbind for an unrelated reason, which is accepted — the peer sees the same TCP close either way. -- **The published surface is frozen at what `src/index.ts` exports today.** `Session` is exported and - publicly constructible, which is why `SessionOptions` and `ReconnectOptions` are public too — that - is correct, not a leak, and it has been raised twice. The collaborators `session.ts` delegates to - (`Reassembler`, `PendingRequests`, `SendWindow`, `ReconnectLoop`, `LinkTimers`, `LinkGate`, - `DlrMerger`, `submitSms`) stay unpublished so they can be reshaped. -- **The sub-3.4 optional-parameter rule is a predicate, not a chokepoint.** `acceptsOptionalParams()` - is consulted by the library's own senders; `session.send({ tlvs })` is passed through as written, - because silently stripping a caller's explicit TLVs off a deliberately public low-level surface - would be worse than sending them. The guarantee is "what this library sends honours the rule", - never "the session cannot send optional parameters to an old peer". -- **Both ends feed `peerInterfaceVersion`, and a peer that declared nothing is pre-3.4.** - `acceptBind()` records what the ESME declared in its bind request; the client's `bind()` records - the `sc_interface_version` the SMSC answered with. A peer that declared no version is recorded as - `undeclaredInterfaceVersion` (0x00) and is sent no optional parameters — the spec reads an absent - `sc_interface_version` as an SMSC that supports none. `undefined` is left to mean one thing only: - no bind has been accepted on this session yet. -- **The library speaks SMPP 3.4 on the wire, and `defs/` keeps the 5.0 tables as a superset.** - Maintainer's call, 2026-08-26: 3.4 is what SMSCs actually run, while the wider tables let the codec - parse and build whatever a peer sends. The declared version is an option on both `client()` and - `server()`, so an implementation that needs 5.0 throughout can have it. The threshold at or above - which a peer may be sent optional parameters is fixed at 0x34 by the spec and is not the same - constant as the declared version. -- **Bind direction is enforced on the library's own senders and on everything incoming, not on - `send()`.** A receiver-bound ESME carries no `submit_sm` and a transmitter-bound one no - `deliver_sm`; `sendSms()` and `sendDlr()` refuse locally, and an arriving PDU is answered - `ESME_RINVBNDSTS`. `bindAllows()` is a predicate on the same footing as `acceptsOptionalParams()`, - so the deliberately public low-level `send()` stays a passthrough. Only those two commands are - policed, because they are the only ones the library sends and dispatches by direction. -- **The logger is a five-method contract this library declares, not a dependency.** `SmppLog` in - `log.ts` is what the code actually calls (`debug`, `error`, `info`, `verbose`, `warn`), so an - application can satisfy it with an object literal and `@larvit/smpp` ships with no runtime - dependencies. `@larvit/log` implements it structurally and stays a devDependency, where - `test/tls.test.ts` passing a real `Log` as the server's logger keeps that compatibility compiled. +- **`close` means the session is over, and a drop the loop will retry is `disconnected`.** + Maintainer's call, 2026-08-31: without the split, an application that opens a replacement client on + `close` ends up holding two binds on one account. `teardown()` picks the event by whether the + reconnect loop is still live, and `end()` stops that loop before tearing down, so every deliberate + shutdown emits `close`. A retry that opens a socket and then loses it clears `closed` through + `attach()`, which is why a second drop emits again. -- **`esm_class` decides what a `deliver_sm` is, and the body is only read when it names nothing.** - Message type `MC_DELIVERY_RECEIPT` (0x04) makes it a receipt whatever the body parses to, so a - receipt in a format `dlrFromPdu()` cannot read reaches `dlr` with `smsId` undefined instead of - arriving as an inbound SMS. Any other named type — delivery or user acknowledgement, conversation - abort, intermediate notification — is not a receipt and its body is not scraped. A message type of - 0, or one of the ten the spec reserves, keeps the scrape: SMSCs that send text-only receipts leave - `esm_class` at 0, and reading that as the spec's "default message type" would lose every one of - them. A non-empty `receipted_message_id` TLV marks a receipt on the same footing there, since - nothing but a receipt carries one. What gets scraped is the decoded `short_message` with any UDH - stripped; a receipt this library recognises never reaches the reassembler, so an SMSC that splits one across segments gets a - `dlr` per segment rather than one merged report. - The `message_state` TLV is authoritative only where it names a state in the table — SMPP reserves - 0x80-0xFF for MC-vendor-specific values, so an unnameable one keeps its raw `statusId` and leaves - `statusMsg` to the body. +- **`reconnect` takes `{ minDelay, maxDelay }` to retune and `false` to turn off**, so absent means + on and there is one spelling for each. Only `client()` reconnects — a `server()` session is a + connection the peer opened, and nothing at this end can reopen it. The retry timer is `unref()`'d, + so a process with nothing else left to do still exits between attempts. + +- **Coming up is not proof a link works, so only one that outlasted `maxDelay` resets the backoff.** + An unreadable stream is found after the bind returns, so resetting on connect gave a link that died + on arrival a fresh `minDelay` every cycle — one TCP connect and bind per second, forever. 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. + +- **A deliberate shutdown drains; an unusable link and an abort do not.** `close()` and `unbind()` + wait on the send window rather than the pending map — the map misses a segment still queued behind + a full window, and finishing a half-sent multipart message is the point. The window counts slots, + never outcomes, and empties on a drop too, where `teardown()` settles everything the link was + carrying, which is why `drain()` reads `closed` before it reads the count. A stream the framer or + the codec cannot read takes `teardown()` instead, and `close({ signal })` on an aborted signal and + a peer's own `unbind` take `end()`: nothing on a dead link can answer, an abort means stop now, and + a peer that has declared itself finished will not answer what it still owes, so draining any of the + three would only hold a socket open for the timeout. `unbind()` sends its own PDU through + `request()` past both the window and the drain gate, because it must go out either way. + `shutdownTimeout` stays a session option rather than a `close()` argument: `server()` builds + sessions on the caller's behalf, so the option is the only composition point. `SmppServer.close()` + reports each session's unfinished drain through `serverError`, because its own result says nothing + but that the listener stopped. + +- **A reconnect keeps the delivery-receipt merges; everything else the link held is dropped.** + `onDeliverSm()` answers each receipt before the group it belongs to is complete, and `teardown()` + runs on every path — an idle timeout and a failed rebind, not only `close()` — so clearing the + merges there loses receipts no peer has a reason to send again. They are cleared where the session + is over instead. Inbound segments stay in `teardown()`, because they go unanswered until the + message is whole: the peer still holds them, and answering it on a later link with the old + segments' sequence numbers would correlate with nothing. + +- **A message id base is merged at most once.** A receipt carries nothing but `-`, so a + straggler for a message whose group is gone cannot be told from a receipt for a later message the + peer handed the same ids — an SMSC whose id counter restarts with its process is the realistic + case. `DlrMerger` remembers the bases it has finished with, capped and expiring exactly like the + groups, and refuses to open one a second time: the later message gets no `messageDlr`, and an + earlier one whose receipts are still arriving is dropped rather than left to collect the later + one's. Every segment still reaches the application as a `dlr`. `expect()` ignores a lone id, so a + single-part message never claims a base. + +- **A send that never reached the socket waits for the next link; one that did is counted, not + resent.** Maintainer's call, 2026-09-01: re-queueing everything unanswered would resend a + `submit_sm` the SMSC accepted and answered into a dead socket, which is delivered and billed twice, + while a request that never left this process can be lost for free. `attempt()` therefore wraps all + three ways a written request can fail in `UnansweredError`; counting only the dropped-link case, as + the first cut did, would have called the commonest one safe to resend. A count rather than a + boolean because `sendSms()` aggregates segments into one `err` slot, and required rather than + optional so every construction site answers. `UnansweredError` stays unexported: `unanswered` is + the one spelling on the public surface. The hold is bounded by `responseTimeout` rather than an + option of its own — that is already the answer to how long one request may wait — and its clock + starts when the send is issued rather than when it first finds the gate shut, so one budget covers + every hold a single call makes. That timer is the one here that is not `unref()`'d: a held request + is awaited with the socket already destroyed, so an unref'd one lets a process whose only remaining + work is that send exit without settling it. + +- **The gate decides whether a link can carry a request, and a bind is what makes it one.** + Maintainer's call, 2026-09-01: `attach()` clears `closed` the moment a socket is handed over, one + round trip before the bind is answered, so gating on `closed` let a send arriving in that window go + out unbound and come back `ESME_RINVBNDSTS`. `LinkGate` owns the answer instead — `shut(returning)` + on every teardown, `open()` only once `comeBackUp()` has a bound link — and `Session.linkDown()` + reads it rather than `closed`. The bind itself cannot wait for what it creates, so `send()` lets the + three bind commands past the gate and the window, the same door `unbind()` takes through + `attempt()`. The gate is told what happened and never reads back into the session: a collaborator + that has to ask does not own its decision, which is how the first cut ended up answering the same + question two different ways at admit and at release. For the same reason the retry in `send()` asks + `gate.isUp()` rather than `linkDown()`, which also reads the socket — a condition that loops on + something the gate does not gate on spins against a gate that admits it straight back. + `LinkGate.returning` is a copy of `retrying()` taken at teardown, and stays true only because + nothing stops the reconnect loop without `emitClose()` following it: `drain()` and `end()` are the + only callers of `stop()`. A third caller has to shut the gate itself. + +### Internals and tests - **A listener that rejects is routed by Node's `captureRejections`, not by hand-dispatching.** Both emitters construct with `captureRejections: true` and implement @@ -235,158 +390,13 @@ exactly 140. handlers normalise through `errorFrom()` rather than inline — a route out of the handler would land on a bare `process.nextTick` with nothing to catch it. -- **Both emitters re-declare their listener methods to accept a promise.** Maintainer's call, - 2026-08-27: `EventEmitter` types every listener as void-returning, so the - `session.on('sms', async sms => …)` the README documents reads as a misused promise in any strict - consumer. `declare on: …` and its six siblings re-type the inherited methods to return `unknown`, - which emits nothing, needs no cast and leaves the runtime method on the prototype. Overriding them - as real methods instead cannot work: the `super.on()` call needs a cast to satisfy the conditional - `Listener` type. The cost is that a subclass can no longer reach those seven through `super` or - override them as methods — re-declaring them the same way is its way out. `unknown` rather than `void | Promise` because a listener may return - anything — `session.on('close', () => set.delete(session))` returns a boolean. - -- **A reconnect keeps the delivery-receipt merges; everything else the link held is dropped.** - `onDeliverSm()` answers each receipt before the group it belongs to is complete, and `teardown()` - runs on every path — an idle timeout and a failed rebind, not only `close()` — so clearing the - merges there loses receipts no peer has a reason to send again. They are cleared where the session - is over instead: `close()`, or a drop with no reconnect loop left to bring the link back. Inbound - segments stay in `teardown()`, because they go unanswered until the message is whole: the peer - still holds them, and answering it on a later link with the old segments' sequence numbers would - correlate with nothing. Surviving a process restart is a separate, public-surface question, and is - in todo.md. - -- **A message id base is merged at most once.** A receipt carries nothing but `-`, so a - straggler for a message whose group is gone cannot be told from a receipt for a later message the - peer handed the same ids — an SMSC whose id counter restarts with its process is the realistic - case. `DlrMerger` remembers the bases it has finished with, capped and expiring exactly like the - groups, and refuses to open one a second time: the later message gets no `messageDlr`, and an - earlier one whose receipts are still arriving is dropped rather than left to collect the later - one's. Every segment still reaches the application as a `dlr`. The rule covers the bases the - merger opened — `expect()` ignores a lone id, so a single-part message never claims one. - -- **A deliberate shutdown drains; an unusable link and an abort do not.** `close()` and `unbind()` - refuse further sends and wait on the send window, not the pending map — the map misses a segment - still queued behind a full window, and finishing a half-sent multipart message is the point. The - window counts slots, never outcomes, so it says when to stop waiting and nothing about what - happened: it empties on a drop too, where `teardown()` settles everything the link was carrying, - which is why `drain()` reads `closed` before it reads the count. The wait covers what this end - sent — a request the peer sent us is answered through `sendReturn()`, which never enters the - window, so a server session waits for none of its inbound work; that half is in todo.md. - `shutdownTimeout` bounds the drain alone, and `0` waits forever like every other timeout here; - `unbind()` then waits `responseTimeout` for its own response, and sends that PDU through - `request()` past both the window and the drain gate because it must go out either way. A stream - the framer or the codec cannot read takes `teardown()` instead, and `close({ signal })` on an - aborted signal and a peer's own `unbind` take `end()` — nothing on a dead link can answer, an abort - means stop now, and a peer that has declared itself finished will not answer what it still owes us, - so draining any of the three would only hold a socket open for the timeout. `SmppServer.close()` - reports each session's unfinished drain through `serverError`, because its own result says - nothing but that the listener stopped. `shutdownTimeout` stays a session option rather than a - `close()` argument: `server()` builds sessions on the caller's behalf, so the option - is the only composition point, and `close({ signal })` already covers a hard deadline. +- **`SmppLog` is a five-method contract this library declares, not a dependency.** `debug`, `error`, + `info`, `verbose` and `warn` are what the code actually calls, so an application can satisfy it + with an object literal. `@larvit/log` implements it structurally and stays a devDependency, where + `test/tls.test.ts` passing a real `Log` as the server's logger keeps that compatibility compiled. - **The TLS tests build their own self-signed certificate in DER** (`test/tls.test.ts`) instead of adding a devDependency or shelling out to openssl. Maintainer's call, 2026-08-26: the dev image - `node:24.18.0-bookworm-slim` ships no openssl binary, so a shelled-out fixture would pass in CI - and fail on every developer machine, and a committed key leaks in a public repository. Valid while - the dev image has no openssl. - -- **`close` means the session is over, and a drop the loop will retry is `disconnected`.** Maintainer's - call, 2026-08-31: with reconnect on by default a `close` on every transient drop left an application - unable to tell a retry from the end, and no second one follows because `teardown()` is a no-op once - `closed`. `teardown()` picks the event by whether the reconnect loop is still live, and `end()` stops - 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 grows to one attempt per - `maxDelay`. - -- **`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 — - `PduTransport` owns the socket, the framer and the parse, and hands the session raw bytes, framed - PDUs, parsed ones and an unreadable stream. Reading `session.sock` is unchanged; assigning it no - longer compiles, which never rewired the handlers and so never worked. The transport stays - unpublished like the other collaborators. - -- **A client re-binds after a drop unless it is told not to.** Maintainer's call, 2026-08-31: - surviving a dropped link is most of what the session layer is for, and behind an opt-in an - application that never read the options table got none of it. `reconnect` takes - `{ minDelay, maxDelay }` to retune the backoff and `false` to turn it off, so absent means on and - there is one spelling for each. Only `client()` reconnects — a `server()` session is a connection - the peer opened, and nothing at this end can reopen it. The retry timer is `unref()`'d, so a - process with nothing else left to do still exits between attempts. - -- **The notation a peer writes message ids in is named per place, and normalisation never reaches - inside a `-` id.** An SMSC may answer `submit_sm_resp` in hex and write the receipt's - `id:` in decimal, so one transform over both sides cannot make them equal — `smsIdFormat` names - `receipt` and `submitResp` separately and reads both into a plain decimal value before `smsIds` - and `dlr.smsId` are compared. `submitResp` covers the `receipted_message_id` TLV too, which SMPP - 3.4 5.3.2.26 defines as the id the `submit_sm_resp` carried: naming one notation for whichever id - a receipt yields would break the peer that sends both, whose TLV correlated before the option was - set. Omitting a place is what leaving it alone means, so there is no - `raw` notation, and a caller-supplied formatter is refused because it would make the promise that - those two are comparable unverifiable — `onRequest` and the PDU on the `dlr` event are the escape - hatches, and the `onReceipt` hook in todo.md is the seam if one is wanted. An id no notation reads - is left exactly as it arrived, which is what keeps `DlrMerger` working: a `-` id parses - as no number and so reaches `expect()` and `collect()` unchanged. Normalising the base instead - would break that pair. The option is on `client()` only — a `server()` session generates its own - ids and writes its own receipts, so both places are already one notation. - -- **A send waits for the next link only if it never reached the socket; one that did is counted, not - resent.** Maintainer's call, 2026-09-01: re-queueing everything unanswered would resend a - `submit_sm` the SMSC accepted and answered into a dead socket, which is delivered and billed - twice, while a request that never left this process can be lost for free. `LinkGate` holds a send - that has no link, so a send issued between links and a segment still queued behind a full window - when the drop hit both go out on the new one. Once a request has been written, every way it can - fail — the link dropping under it, `responseTimeout` expiring, the caller's own abort — means the - peer may have taken it, so `attempt()` wraps all three in `UnansweredError` and `collectSent()` - counts them into `SendSmsResult.unanswered`. Counting only the dropped-link case, as the first cut - did, would have called the commonest one safe to resend. A count rather than a boolean because - `sendSms()` aggregates segments into one `err` slot, and required rather than optional so every - construction site answers. `UnansweredError` stays unexported: `unanswered` is the one spelling on - the public surface, and a `send()` error that is neither a build failure nor a pre-write abort - means the same thing. - The hold is bounded by `responseTimeout` rather than an option of its own — that is already the - answer to how long one request may wait. It bounds the hold and the answer separately, and the - wait for a `maxOutstanding` slot is bounded by nothing, so `responseTimeout` is not a deadline for - the call; `SendOptions.signal` with `AbortSignal.timeout()` is, and both the gate and - `pending.wait()` honour it. The hold's clock starts when the send is issued rather than when it - first finds the gate shut, so one budget covers every hold a single call makes — a send that spent - it queued behind the window is refused rather than held. The hold timer is the one timer here that - is not `unref()`'d: a held request is awaited with the socket already destroyed, so an unref'd one - lets a process whose only remaining work is that send exit without settling it. - -- **The gate decides whether a link can carry a request, and a bind is what makes it one.** - Maintainer's call, 2026-09-01: `attach()` clears `closed` the moment a socket is handed over, one - round trip before the bind is answered, so gating on `closed` let a send arriving in that window - go out unbound and come back `ESME_RINVBNDSTS` while a send that arrived a millisecond earlier was - held correctly. `LinkGate` owns the answer instead — `shut(returning)` on every teardown, - `open()` only once `comeBackUp()` has a bound link — and `Session.linkDown()` reads it rather than - `closed`. The bind itself cannot wait for what it creates, so `send()` lets the three bind - commands past the gate and the window, the same door `unbind()` takes through `attempt()`. That - keeps the exemption a predicate on the command, like the `_resp` guard beside it, rather than a - second `send()` on the public surface or a changed `ReconnectOptions.onConnected`. - The gate is told what happened and never reads back into the session: a collaborator that has to - ask does not own its decision, which is how the first cut ended up answering the same question two - different ways at admit and at release. For the same reason the retry in `send()` asks - `gate.isUp()` rather than `linkDown()`, which also reads the socket: a condition that loops on - something the gate does not gate on spins against a gate that admits it straight back. - `LinkGate.returning` is a copy of `retrying()` taken at teardown, and stays true only because - nothing stops the reconnect loop without `emitClose()` following it — `drain()` and `end()` are the - only callers of `stop()`. An `OutgoingRequests` owning both would not need the copy; until then, - a third caller of `stop()` has to shut the gate itself. + `node:24.18.0-bookworm-slim` ships no openssl binary, so a shelled-out fixture would pass in CI and + fail on every developer machine, and a committed key leaks in a public repository. Valid while the + dev image has no openssl. diff --git a/todo.md b/todo.md index 52c9db1..8ef295f 100644 --- a/todo.md +++ b/todo.md @@ -1,18 +1,17 @@ # todo.md -Remaining work for the `@larvit/smpp` 1.0.0 rewrite. Read [AGENTS.md](AGENTS.md) first — the hard -rules there constrain every item below. +Remaining work for the `@larvit/smpp` 1.0.0 rewrite. Read [AGENTS.md](AGENTS.md) first — the goals +and hard rules there constrain every item below. + +This is a temporary working file: it is deleted when 1.0.0 ships, and until then it sets its own +rules. The documentation conventions in AGENTS.md do not govern it, and nothing here is a source +anything else may cite. ## Status The rewrite is **feature complete and green**: the suite, lint and typecheck are clean on Node 18, 20, 22 and 24. What is left is release work and a few things worth adding before or after 1.0.0. -```bash -docker compose run --rm node npm install -docker compose run --rm node npm test -``` - ## The agreed API Settled with the maintainer before implementation. Do not change any of it without asking. The @@ -180,13 +179,9 @@ session message is a change to every call site. ## Declined -- **Throughput throttling — a TPS cap, and backing off on `ESME_RTHROTTLED`.** Two reasons, either - sufficient. An operator's rate limit is scoped to the account, while the widest thing this library - owns is a session: a bucket here cannot see a second process binding the same account, so it is - wrong in exactly the case it exists for. And a rate limiter's queue drains at a fixed ceiling - rather than at the peer's response rate, so a submit rate sustained above the limit grows it - without bound — and a queue holding messages the caller was told were accepted loses them on - restart, which is worse than refusing them up front. Pacing an account needs durable shared state - this library deliberately has none of. `sendSms()` surfaces `ESME_RTHROTTLED` to the caller - instead, and `maxOutstanding` stays: a window slot frees on the peer's next response, which is - self-limiting in a way a rate ceiling is not. +- **Throughput throttling — a TPS cap, and backing off on `ESME_RTHROTTLED`.** Declined by AGENTS.md + goal 7: an operator's rate limit is scoped to the account, while the widest thing this library owns + is a session, so a bucket here cannot see a second process binding the same account and is wrong in + exactly the case it exists for. `sendSms()` surfaces `ESME_RTHROTTLED` to the caller instead, and + `maxOutstanding` stays — a window slot frees on the peer's next response, which is self-limiting in + a way a rate ceiling is not. From 3bcea108e3ddfe55e605992b41fd9686692724fa Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Tue, 1 Sep 2026 13:58:28 +0200 Subject: [PATCH 13/19] Wait out the messages the application holds before shutting a session down --- AGENTS.md | 17 ++++++++- README.md | 10 +++--- src/held-messages.ts | 29 +++++++++++++++ src/idle-waiters.ts | 47 ++++++++++++++++++++++++ src/incoming-requests.ts | 36 ++++++++++++++++--- src/send-sms.ts | 7 +++- src/send-window.ts | 38 ++++---------------- src/session.ts | 44 ++++++++++++----------- src/sms.ts | 17 ++++++--- src/udh.ts | 11 ++++++ test/session-extras.test.ts | 71 +++++++++++++++++++++++++++++++++++-- todo.md | 15 ++++---- 12 files changed, 263 insertions(+), 79 deletions(-) create mode 100644 src/held-messages.ts create mode 100644 src/idle-waiters.ts diff --git a/AGENTS.md b/AGENTS.md index 3151286..ad670c6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -72,6 +72,8 @@ src/ dlr-merger.ts DlrMerger: per-segment receipts counted into one MessageDlr error-from.ts errorFrom(): whatever was thrown or rejected, as an Error expiring-groups.ts ExpiringGroups: the capped, expiring store both of those share + held-messages.ts HeldMessages: the messages handed to the application and not yet answered + idle-waiters.ts IdleWaiters: waiting for a count to fall to zero, and what is left of a budget incoming-requests.ts Every request the peer sends: messages, receipts, links, unknown commands link-gate.ts LinkGate: where a request with no link to go out on waits for the next one link-timers.ts LinkTimers: the enquire_link heartbeat and the idle timeout @@ -88,7 +90,7 @@ src/ send-window.ts SendWindow: the maxOutstanding semaphore session-options.ts SessionOptions, ReconnectOptions, bind direction and the session defaults sms-id.ts The notation a peer writes message ids in, normalised for comparison - udh.ts User data header: the concatenation fields of a long SMS + udh.ts User data header: the concatenation fields of a long SMS, and their reference uuid.ts uuidv7() — the ids the library generates for messages defs/ commands.ts The 33 commands, their ids and ordered parameter lists @@ -331,6 +333,19 @@ Grouped by what each one constrains. reports each session's unfinished drain through `serverError`, because its own result says nothing but that the listener stopped. +- **The drain waits on the messages the application holds, and `sendResp()` is what says it is done + with one.** Maintainer's call, 2026-09-01: waiting on the send window alone tore a server session + down while the application was still answering a `submit_sm`, so the peer timed out and re-sent — + the duplicate goal 2 forbids, in the direction the window already covers. No completion signal was + added to the `sms` event: `sendResp()` is the answer the peer is waiting for, so it is the one the + drain waits for. Counting every inbound request until `sendReturn()` answered it was rejected — + an `onRequest` that deliberately answers nothing would then cost a full `shutdownTimeout` on every + close — and a message no listener took is released at once, since nothing is going to answer it. + `teardown()` drops what is still held for the same reason it drops inbound segments. The release + is one turn late, so a listener that sends its receipt straight after the response is still + holding when the drain looks; `sendDlr()` is the one send that goes out past the drain's refusal, + being part of answering a message the drain is itself waiting for. + - **A reconnect keeps the delivery-receipt merges; everything else the link held is dropped.** `onDeliverSm()` answers each receipt before the group it belongs to is complete, and `teardown()` runs on every path — an idle timeout and a failed rebind, not only `close()` — so clearing the diff --git a/README.md b/README.md index 92778c0..559b05c 100644 --- a/README.md +++ b/README.md @@ -21,7 +21,7 @@ by hand on top of a library; it is built in here. | **Submit window** | `maxOutstanding` holds requests in flight at 10; further sends queue instead of overrunning the SMSC. | | **Delivery receipts** | Correlated by `receipted_message_id`/`message_state` where the SMSC sends them, falling back to parsing the receipt text — what Kannel and several others send. | | **Multipart** | Long messages split on send; concatenated `deliver_sm` reassembled into one `sms`. | -| **Graceful shutdown** | `close()` and `unbind()` wait out the requests this end already sent, so a submit the SMSC accepted is not reported as a failure. | +| **Graceful shutdown** | `close()` and `unbind()` wait out the requests this end already sent and the messages the application has not answered yet, so neither end has to guess whether a message got through. | | **Never throws** | Everything fallible resolves to `{ err?, … }`, the codec included. | Throughput throttling is deliberately absent: an operator's rate limit is scoped to the account, and @@ -101,7 +101,7 @@ Every one is optional. | `enquireLinkInterval` | `20000` | How often to send `enquire_link` on a quiet link. | | `idleTimeout` | `2 × enquireLinkInterval` | Give up on a link the peer has stopped answering, and re-bind unless `reconnect` is `false`. | | `responseTimeout` | `30000` | How long to wait for a response before giving up on it, and how long a send with no link waits for the next one; `0` waits forever. | -| `shutdownTimeout` | `5000` | How long `close()` and `unbind()` wait for the requests this end already sent; `0` waits forever. | +| `shutdownTimeout` | `5000` | How long `close()` and `unbind()` wait for the requests this end already sent and the messages the application has not answered; `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, an idle timeout, or a stream the library cannot read, backing off from `minDelay` 1 s to `maxDelay` 30 s and starting over at `minDelay` once a link has lasted `maxDelay`. `{ minDelay, maxDelay }` retunes it; `false` turns it off, so a drop ends the session. | @@ -331,8 +331,10 @@ TypeScript users can import `SmppLog` to have the compiler check one. `sendSms()`, `send()`, `sendReturn()`, `unbind()` and `close()`. Both `close()` and `unbind()` refuse further sends, wait out the requests this end already sent for up to `shutdownTimeout`, and -then tear down whatever is left, resolving to an `err` that says what was lost. A request the peer -sent *us* is answered through `sendReturn()` and is not waited for. `close({ signal })` takes an +then tear down whatever is left, resolving to an `err` that says what was lost. They wait on the +`sms` events the application has not answered yet too, so a peer whose `submit_sm` is still being +handled is answered rather than left to re-send it; `sendDlr()` goes out during that wait, and every +other send is refused. `close({ signal })` takes an `AbortSignal` that cuts the wait short; `unbind()` takes none, and waits a further `responseTimeout` for its own response. `send()` reaches any of the 33 SMPP commands the codec knows, not just the four the session handles natively: diff --git a/src/held-messages.ts b/src/held-messages.ts new file mode 100644 index 0000000..3a12be4 --- /dev/null +++ b/src/held-messages.ts @@ -0,0 +1,29 @@ +import type { PduObject } from './pdu.ts'; +import { IdleWaiters } from './idle-waiters.ts'; + +/** The messages handed to the application that it has not answered yet, held by their segments. */ +export class HeldMessages { + private readonly held = new Set(); + private readonly idleWaiters = new IdleWaiters(); + + hold(pduObjs: PduObject[]): void { + this.held.add(pduObjs); + } + + release(pduObjs: PduObject[]): void { + if (!this.held.delete(pduObjs)) return; + + if (this.held.size === 0) this.idleWaiters.settle(); + } + + /** Drops every message: their segments went with the link, so no answer of ours correlates now. */ + clear(): void { + this.held.clear(); + this.idleWaiters.settle(); + } + + /** Resolves 0 once every message has been answered, or with how many have not. */ + idle(timeout: number, signal?: AbortSignal): Promise { + return this.idleWaiters.wait(() => this.held.size, timeout, signal); + } +} diff --git a/src/idle-waiters.ts b/src/idle-waiters.ts new file mode 100644 index 0000000..50c9311 --- /dev/null +++ b/src/idle-waiters.ts @@ -0,0 +1,47 @@ +/** What is left of a budget, in the shape a wait takes it: 0 waits forever. */ +export function leftOf(deadline: number): number { + return deadline === 0 ? 0 : Math.max(1, deadline - Date.now()); +} + +/** Everything waiting for a count to fall to zero, and how such a wait is cut short. */ +export class IdleWaiters { + private readonly waiting: (() => void)[] = []; + + /** Wakes everything waiting, whatever the count reads now. */ + settle(): void { + for (const resolve of this.waiting.splice(0)) { + resolve(); + } + } + + /** + * Resolves 0 once nothing is left, or with what still is when the timeout or the signal cuts the + * wait short. A timeout of 0 waits forever. + */ + wait(remaining: () => number, timeout: number, signal?: AbortSignal): Promise { + if (remaining() === 0) return Promise.resolve(0); + + if (signal?.aborted === true) return Promise.resolve(remaining()); + + return new Promise(resolve => { + let timer: NodeJS.Timeout | undefined = undefined; + const done = (): void => { + const index = this.waiting.indexOf(done); + + if (timer) clearTimeout(timer); + if (index !== -1) this.waiting.splice(index, 1); + + signal?.removeEventListener('abort', done); + resolve(remaining()); + }; + + if (timeout > 0) { + timer = setTimeout(done, timeout); + timer.unref(); + } + + signal?.addEventListener('abort', done, { once: true }); + this.waiting.push(done); + }); + } +} diff --git a/src/incoming-requests.ts b/src/incoming-requests.ts index e81898a..2f28d1d 100644 --- a/src/incoming-requests.ts +++ b/src/incoming-requests.ts @@ -1,9 +1,11 @@ import type { DlrMerger } from './dlr-merger.ts'; import type { OnRequest } from './session-options.ts'; -import type { PduObject } from './pdu.ts'; +import type { PduObject, PduObjectInput } from './pdu.ts'; +import type { Result, VoidResult } from './result.ts'; import type { Session } from './session.ts'; import type { SmppLog } from './log.ts'; import type { SmsIdFormat } from './sms-id.ts'; +import { HeldMessages } from './held-messages.ts'; import { Reassembler, decodeSegments } from './reassembly.ts'; import { bindCommands, defaults } from './session-options.ts'; import { concatInfo } from './udh.ts'; @@ -19,6 +21,8 @@ export type IncomingRequestsOptions = { maxReassembly?: number | undefined; onRequest?: OnRequest | undefined; reassemblyTimeout?: number | undefined; + /** Past the drain gate: a receipt answering a message the shutdown is still waiting for. */ + sendHeld: (input: PduObjectInput) => Promise>; session: Session; smsIdFormat?: SmsIdFormat | undefined; systemId?: string | undefined; @@ -27,9 +31,11 @@ export type IncomingRequestsOptions = { /** Everything the peer asks of a session: messages, receipts, links and the answers to them. */ export class IncomingRequests { private readonly dlrMerger: DlrMerger; + private readonly held = new HeldMessages(); private readonly log: SmppLog; private readonly onRequest: OnRequest | undefined; private readonly reassembler: Reassembler; + private readonly sendHeld: IncomingRequestsOptions['sendHeld']; private readonly session: Session; private readonly smsIdFormat: SmsIdFormat; private readonly systemId: string; @@ -44,6 +50,7 @@ export class IncomingRequests { maxOctets: options.maxOctets, timeout: options.reassemblyTimeout ?? defaults.reassemblyTimeout, }); + this.sendHeld = options.sendHeld; this.session = options.session; this.smsIdFormat = options.smsIdFormat ?? {}; this.systemId = options.systemId ?? defaults.systemId; @@ -82,11 +89,23 @@ export class IncomingRequests { } } - /** Drops the segments of every message that never became whole. */ + /** Drops the segments of every message that never became whole, and of every one still held. */ clear(): void { + this.held.clear(); this.reassembler.clear(); } + /** Waits out the messages the application still holds, and says how many it never answered. */ + async drain(timeout: number, signal?: AbortSignal): Promise { + const unanswered = await this.held.idle(timeout, signal); + + if (unanswered === 0) return {}; + + this.log.warn('session - shutting down with messages unanswered', { timeout, unanswered }); + + return { err: new Error(`Shut down with ${String(unanswered)} message(s) unanswered`) }; + } + private async unhandled(pduObj: PduObject): Promise { if (bindCommands.includes(pduObj.cmdName)) { this.log.info('session - bind on an already bound session', { cmdName: pduObj.cmdName }); @@ -139,12 +158,21 @@ export class IncomingRequests { if (!first) return; - this.session.emit('sms', createSms({ + const sms = createSms({ from: paramText(first.params.source_addr), message: decodeSegments(pduObjs), pduObjs, session: this.session, to: paramText(first.params.destination_addr), - })); + }, { + // A turn later, so a listener sending its receipt straight after the response still holds. + onAnswered: () => { setImmediate(() => { this.held.release(pduObjs); }); }, + send: this.sendHeld, + }); + + this.held.hold(pduObjs); + + // A message nobody took is not work a shutdown can wait for. + if (!this.session.emit('sms', sms)) this.held.release(pduObjs); } } diff --git a/src/send-sms.ts b/src/send-sms.ts index d329846..3277330 100644 --- a/src/send-sms.ts +++ b/src/send-sms.ts @@ -37,6 +37,11 @@ export type SendSmsResult = { unanswered: number; }; +/** A message that never went out, in the shape a caller aggregating segments still reads. */ +export function unsent(err: Error): SendSmsResult { + return { err, pduObjs: [], smsIds: [], unanswered: 0 }; +} + /** What sending needs from the session: a concat reference and a way onto the wire. */ export type SendSmsDeps = { log: SmppLog; @@ -141,7 +146,7 @@ export async function submitSms(deps: SendSmsDeps, sms: SendSmsOptions): Promise const segments = splitMessage(sms.message, { encoding, reference: deps.reference }); const refused = checkSegments(allowed, segments.length); - if (refused) return { err: refused, pduObjs: [], smsIds: [], unanswered: 0 }; + if (refused) return unsent(refused); const multipart = segments.length > 1; diff --git a/src/send-window.ts b/src/send-window.ts index e1492ca..46560c3 100644 --- a/src/send-window.ts +++ b/src/send-window.ts @@ -1,8 +1,10 @@ +import { IdleWaiters } from './idle-waiters.ts'; + /** Caps how many requests are on the wire at once; anything past the limit waits its turn. */ export class SendWindow { + private readonly idleWaiters = new IdleWaiters(); private readonly limit: number; private readonly waiting: (() => void)[] = []; - private readonly waitingForIdle: (() => void)[] = []; private inFlight = 0; constructor(limit: number) { @@ -32,9 +34,7 @@ export class SendWindow { if (this.inFlight > 0) return; - for (const resolve of this.waitingForIdle.splice(0)) { - resolve(); - } + this.idleWaiters.settle(); } /** Everything the caller is still owed: on the wire, plus queued behind a full window. */ @@ -42,34 +42,8 @@ export class SendWindow { return this.inFlight + this.waiting.length; } - /** - * Resolves 0 once nothing is left, or with what still is when the timeout or the signal cuts the - * wait short. A timeout of 0 waits forever. - */ + /** Resolves 0 once nothing is left on the wire, or with what still is. */ idle(timeout: number, signal?: AbortSignal): Promise { - if (this.inFlight === 0) return Promise.resolve(0); - - if (signal?.aborted === true) return Promise.resolve(this.unfinished()); - - return new Promise(resolve => { - let timer: NodeJS.Timeout | undefined = undefined; - const done = (): void => { - const index = this.waitingForIdle.indexOf(done); - - if (timer) clearTimeout(timer); - if (index !== -1) this.waitingForIdle.splice(index, 1); - - signal?.removeEventListener('abort', done); - resolve(this.unfinished()); - }; - - if (timeout > 0) { - timer = setTimeout(done, timeout); - timer.unref(); - } - - signal?.addEventListener('abort', done, { once: true }); - this.waitingForIdle.push(done); - }); + return this.idleWaiters.wait(() => this.unfinished(), timeout, signal); } } diff --git a/src/session.ts b/src/session.ts index 247d62f..b297233 100644 --- a/src/session.ts +++ b/src/session.ts @@ -16,12 +16,14 @@ import { PduTransport } from './pdu-transport.ts'; import { PendingRequests, UnansweredError } from './pending-requests.ts'; import { ReconnectLoop } from './reconnect-loop.ts'; import { SendWindow } from './send-window.ts'; +import { leftOf } from './idle-waiters.ts'; import { errorFrom } from './error-from.ts'; import { optionalParamsMinVersion } from './defs/constants.ts'; import { bindCarries, bindCommands, defaultSystemId, defaults } from './session-options.ts'; import { isResp, objToPdu, pduReturn } from './pdu.ts'; import { silentLog } from './log.ts'; -import { submitSms } from './send-sms.ts'; +import { submitSms, unsent } from './send-sms.ts'; +import { ConcatReference } from './udh.ts'; export type { CloseOptions, @@ -64,6 +66,7 @@ export class Session extends EventEmitter { peerInterfaceVersion: number | undefined = undefined; userData: unknown = undefined; + private readonly concatReference = new ConcatReference(); private readonly dlrMerger: DlrMerger; private readonly gate: LinkGate; private readonly incoming: IncomingRequests; @@ -75,7 +78,6 @@ export class Session extends EventEmitter { private readonly window: SendWindow; private closed = false; - private concatReference = 0; private draining = false; private ended = false; @@ -129,6 +131,7 @@ export class Session extends EventEmitter { maxReassembly: options.maxReassembly, onRequest: options.onRequest, reassemblyTimeout: options.reassemblyTimeout, + sendHeld: input => this.sendThrough(input, {}), session: this, smsIdFormat: options.smsIdFormat, systemId: options.systemId, @@ -166,7 +169,15 @@ export class Session extends EventEmitter { } /** Sends a request and resolves with the peer's response. */ - async send( + send(input: PduObjectInput, options: SendOptions = {}): Promise> { + // A drain on a live link. A link that is down is the gate's answer, which says closed instead. + if (this.draining && !this.linkDown()) return Promise.resolve({ err: new Error('Session is shutting down') }); + + return this.sendThrough(input, options); + } + + /** The same path without that refusal, which a receipt for a held message has to take. */ + private async sendThrough( input: PduObjectInput, options: SendOptions = {}, ): Promise> { @@ -199,9 +210,6 @@ export class Session extends EventEmitter { return new Error(`Use sendReturn() for responses, not send(): ${input.cmdName}`); } - // A drain on a live link. A link that is down is the gate's answer, which says closed instead. - if (this.draining && !this.linkDown()) return new Error('Session is shutting down'); - // Before the gate and the window, or an aborted call waits for what it will never use. if (options.signal?.aborted === true) return abortedBeforeSend(); @@ -239,17 +247,12 @@ export class Session extends EventEmitter { async sendSms(sms: SendSmsOptions, options: SendOptions = {}): Promise { if (!this.bindAllows('submit_sm')) { - return { - err: new Error('A receiver-bound session does not carry submit_sm'), - pduObjs: [], - smsIds: [], - unanswered: 0, - }; + return unsent(new Error('A receiver-bound session does not carry submit_sm')); } const sent = await submitSms({ log: this.log, - reference: this.nextConcatReference(), + reference: this.concatReference.next(), respIdNotation: this.options.smsIdFormat?.submitResp, send: input => this.send(input, options), }, sms); @@ -379,7 +382,7 @@ export class Session extends EventEmitter { return { result: answered.err ? { err: new UnansweredError(answered.err) } : answered, retryOnNextLink: false }; } - /** Stops new sends and waits out the ones already issued. */ + /** Stops new sends and waits out the messages we hold and the requests already issued. */ private async drain(signal: AbortSignal | undefined): Promise { this.reconnectLoop?.stop(); this.draining = true; @@ -387,11 +390,16 @@ export class Session extends EventEmitter { if (this.linkDown()) return {}; const timeout = this.options.shutdownTimeout ?? defaults.shutdownTimeout; - const unfinished = await this.window.idle(timeout, signal); + const deadline = timeout > 0 ? Date.now() + timeout : 0; + // Answering a message can put a receipt on the wire; nothing on the wire produces a message. + const messages = await this.incoming.drain(timeout, signal); + const unfinished = await this.window.idle(leftOf(deadline), signal); // The window empties on a teardown too, which settles everything the link was carrying. if (this.linkDown()) return { err: new Error('The session closed before the drain finished') }; + if (messages.err) return messages; + if (unfinished === 0) return {}; this.log.warn('session - shutting down with requests unfinished', { timeout, unfinished }); @@ -433,12 +441,6 @@ export class Session extends EventEmitter { return this.reconnectLoop !== undefined && !this.reconnectLoop.isStopped(); } - private nextConcatReference(): number { - this.concatReference = this.concatReference >= 255 ? 1 : this.concatReference + 1; - - return this.concatReference; - } - private onData(chunk: Buffer): void { this.emit('data', chunk); this.resetTimers(); diff --git a/src/sms.ts b/src/sms.ts index cefdcf9..be341f2 100644 --- a/src/sms.ts +++ b/src/sms.ts @@ -1,6 +1,6 @@ import type { ErrorName } from './defs/errors.ts'; import type { MessageState } from './defs/constants.ts'; -import type { PduObject, TlvInput } from './pdu.ts'; +import type { PduObject, PduObjectInput, TlvInput } from './pdu.ts'; import type { Result, VoidResult } from './result.ts'; import type { Session } from './session.ts'; import { consts } from './defs/constants.ts'; @@ -43,12 +43,18 @@ export type SmsInput = { to: string; }; +/** What the session's incoming side gives a message so it can be answered and accounted for. */ +export type SmsHandlers = { + onAnswered: () => void; + send: (input: PduObjectInput) => Promise>; +}; + /** Each segment of a multipart message gets its own message_id, as a separate submit_sm must. */ function segmentId(smsId: string, index: number, total: number): string { return total === 1 ? smsId : `${smsId}-${String(index + 1)}`; } -export function createSms(input: SmsInput): Sms { +export function createSms(input: SmsInput, handlers: SmsHandlers): Sms { const first = input.pduObjs[0]; const registered = first?.params.registered_delivery; const dataCoding = first?.params.data_coding; @@ -60,8 +66,8 @@ export function createSms(input: SmsInput): Sms { from: input.from, message: input.message, pduObjs: input.pduObjs, - sendDlr: status => sendDlr(sms, status), - sendResp: options => sendResp(sms, answered, options ?? {}), + sendDlr: status => sendDlr(sms, handlers.send, status), + sendResp: options => sendResp(sms, answered, options ?? {}).finally(handlers.onAnswered), session: input.session, get smsId(): string { return answered.smsId; @@ -124,6 +130,7 @@ function receiptTlvs(smsId: string, status: MessageState): Record> { if (!sms.session.bindAllows('deliver_sm')) { @@ -135,7 +142,7 @@ async function sendDlr( for (let index = 0; index < total; index++) { const smsId = segmentId(sms.smsId, index, total); - const sent = await sms.session.send({ + const sent = await send({ cmdName: 'deliver_sm', params: { destination_addr: sms.from, diff --git a/src/udh.ts b/src/udh.ts index 37fba2d..1b6c399 100644 --- a/src/udh.ts +++ b/src/udh.ts @@ -1,3 +1,14 @@ +/** The 8-bit reference tying a long SMS's segments together, counted per session. */ +export class ConcatReference { + private current = 0; + + next(): number { + this.current = this.current >= 255 ? 1 : this.current + 1; + + return this.current; + } +} + export type ConcatInfo = { part: number; reference: number; diff --git a/test/session-extras.test.ts b/test/session-extras.test.ts index 4770767..ed6eedf 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -1037,8 +1037,12 @@ describe('AbortSignal on a send', () => { }); describe('graceful shutdown', () => { - async function submitInFlight(t: TestContext, options: Parameters[0] = {}) { - const smpp = await startServer(t); + async function submitInFlight( + t: TestContext, + options: Parameters[0] = {}, + serverOptions: Parameters[0] = {}, + ) { + const smpp = await startServer(t, serverOptions); const incoming = once(resolve => { smpp.on('session', bound => bound.on('sms', resolve)); }); @@ -1079,6 +1083,69 @@ describe('graceful shutdown', () => { assert.deepEqual(await unbound, {}); }); + test('close() waits for a message the application has not answered yet', async t => { + const { sent, smpp, sms } = await submitInFlight(t); + const closing = peerOf(smpp).close(); + + await delay(50); + await sms.sendResp({ smsId: 'answered-during-the-inbound-drain' }); + + assert.deepEqual(await closing, {}); + assert.deepEqual((await sent).smsIds, ['answered-during-the-inbound-drain']); + }); + + test('gives up on a message the application never answers', async t => { + const { smpp } = await submitInFlight(t, {}, { shutdownTimeout: 50 }); + const closed = await peerOf(smpp).close(); + + assert.ok(closed.err instanceof Error); + assert.match(closed.err.message, /1 message\(s\) unanswered/); + }); + + // The README's own listener answers and then sends its receipt, one turn later. + test('a receipt sent right after the response still goes out mid-drain', async t => { + const { sent, session, smpp, sms } = await submitInFlight(t); + const receipt = once(resolve => { session.on('dlr', resolve); }); + const closing = peerOf(smpp).close(); + + await sms.sendResp({ smsId: 'held-through-the-drain' }); + + const receiptSent = await sms.sendDlr('DELIVERED'); + + assert.equal(receiptSent.err, undefined); + assert.equal((await receipt).smsId, 'held-through-the-drain'); + assert.deepEqual(await closing, {}); + assert.deepEqual((await sent).smsIds, ['held-through-the-drain']); + }); + + test('a message no listener took does not hold the shutdown up', async t => { + const smpp = await startServer(t, { shutdownTimeout: 30_000 }); + const { session } = await connect(t, smpp); + + assert.ok(session); + + const bound = peerOf(smpp); + const arrived = once(resolve => { + bound.on('incomingPduObj', pduObj => { + if (pduObj.cmdName === 'submit_sm') resolve(pduObj); + }); + }); + const sent = session.sendSms({ + from: '46701113311', + message: 'nobody is listening', + to: '46709771337', + }); + + await arrived; + await delay(50); + + const started = Date.now(); + + assert.deepEqual(await bound.close(), {}); + assert.ok(Date.now() - started < 1000); + assert.ok((await sent).err instanceof Error); + }); + test('gives up on a request that outlasts shutdownTimeout', async t => { const { sent, session } = await submitInFlight(t, { shutdownTimeout: 50 }); const closed = await session.close(); diff --git a/todo.md b/todo.md index 8ef295f..2a62cb4 100644 --- a/todo.md +++ b/todo.md @@ -56,6 +56,7 @@ Rules the API follows: | Merged multipart DLRs including across a reconnect, reassembly bounds, per-send abort, the segment cap | `test/session-extras.test.ts` | | `smsIdFormat`: a peer's `submit_sm_resp` and receipt ids read into one notation before they are compared | `test/dlr.test.ts`, `test/session-extras.test.ts` | | A draining `close()` and `unbind()`, bounded by `shutdownTimeout` or an abort | `test/session-extras.test.ts` | +| A drain that also waits out the messages the application has not answered, with `sendDlr()` the one send that passes it | `test/session-extras.test.ts` | | A send with no link held for the next one, and one the link dropped under counted as `unanswered` | `test/session-extras.test.ts` | | Every runnable README example | `test/readme.test.ts` | | Receipt-versus-message classification by `esm_class` | `test/dlr.test.ts`, `test/session.test.ts` | @@ -113,11 +114,6 @@ session message is a change to every call site. ## Worth doing, not blocking -- [ ] **The drain covers only what this end sent.** `close()` and `unbind()` wait on the send window, - which `sendReturn()` never enters, so a server session tears down without waiting for the - application to answer the messages it is holding — the duplicate-on-retry outcome again, in - the SMSC direction. A full inbound drain needs a completion signal the `sms` event does not - carry, so it is a public-surface decision. Raised by review, 2026-08-30. - [ ] **Merge state does not survive a process restart.** A drop no longer discards it, but a restart loses every incomplete group, and a peer has no reason to resend a receipt it already had answered. Surviving one means exposing the merge state for the application to persist and hand @@ -125,13 +121,14 @@ session message is a change to every call site. - [ ] **Group the session's collaborators under `src/session/`.** Only `session.ts` imports `reassembly`, `dlr-merger`, `send-window`, `link-timers`, `link-gate`, `reconnect-loop`, `pending-requests` and `send-sms`, so the directory would make that boundary visible. - `pdu-transport` joined them on 2026-08-31 and `link-gate` on 2026-09-01, both without the move - being made, so it is a move of its own now. Do it together with the extraction below rather + `pdu-transport` joined them on 2026-08-31, `link-gate` and `idle-waiters` on 2026-09-01, all + without the move being made, so it is a move of its own now. Do it together with the extraction below rather than before it — three reactive splits at whatever boundary fitted under the line cap is what produced the current shape. Raised by review, 2026-09-01. - [ ] **An `OutgoingRequests` collaborator, owning `LinkGate`, `SendWindow` and `PendingRequests`.** - `session.ts` sits a few lines under its 350-line cap and every split so far has been made to - get back under it. The seam that holds: one object owning the gate, the window, the pending + `session.ts` sits three lines under its 350-line cap and every split so far has been made to + get back under it — the inbound drain on 2026-09-01 only fitted once `ConcatReference` and the + unsent-result shape moved out to `udh.ts` and `send-sms.ts`. The seam that holds: one object owning the gate, the window, the pending map and the retry loop, exposing a gated `request()` and the ungated door `unbind()` and the bind already need, with `Session` calling it when a link comes up or goes down instead of spreading `linkDown()` and `retrying()` across both sides. It would also close two smaller From 8becbc0ab6234b58d9cf0d41521e5a6be17fa73d Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Tue, 1 Sep 2026 16:52:15 +0200 Subject: [PATCH 14/19] Give the gate, the window and the pending map one owner --- AGENTS.md | 16 +++- README.md | 12 +-- src/held-messages.ts | 5 ++ src/incoming-requests.ts | 11 +-- src/link-gate.ts | 14 +-- src/outgoing-requests.ts | 168 ++++++++++++++++++++++++++++++++++++ src/pending-requests.ts | 8 -- src/send-sms.ts | 2 +- src/session.ts | 144 ++++++------------------------- src/unanswered-error.ts | 7 ++ test/session-extras.test.ts | 19 +++- todo.md | 38 ++++---- 12 files changed, 276 insertions(+), 168 deletions(-) create mode 100644 src/outgoing-requests.ts create mode 100644 src/unanswered-error.ts diff --git a/AGENTS.md b/AGENTS.md index ad670c6..41703ed 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -66,7 +66,7 @@ src/ index.ts Public surface. Named exports only, no default export. client.ts client() -> { err, session } server.ts server() -> { err, server }, server owns the listener + close() - session.ts Session: dispatch, events, and the collaborators below + session.ts Session: the socket's life, dispatch, events, and the collaborators below sms.ts The live handle emitted as the 'sms' event (sendResp/sendDlr) dlr.ts Delivery receipts: text and TLV parsing, receipt status codes dlr-merger.ts DlrMerger: per-segment receipts counted into one MessageDlr @@ -79,6 +79,7 @@ src/ link-timers.ts LinkTimers: the enquire_link heartbeat and the idle timeout log.ts SmppLog, the logger contract, and silentLog — the default message.ts Encoding detection, splitting, bit counting, SMPP date formatting + outgoing-requests.ts OutgoingRequests: the gate, the window, the pending map and the retry pdu.ts pduToObj / objToPdu / pduReturn — synchronous, result-returning pdu-framer.ts PduFramer: a byte stream cut into complete PDUs pdu-transport.ts PduTransport: the socket a session reads complete PDUs off @@ -91,6 +92,7 @@ src/ session-options.ts SessionOptions, ReconnectOptions, bind direction and the session defaults sms-id.ts The notation a peer writes message ids in, normalised for comparison udh.ts User data header: the concatenation fields of a long SMS, and their reference + unanswered-error.ts UnansweredError: it went out and no answer came back uuid.ts uuidv7() — the ids the library generates for messages defs/ commands.ts The 33 commands, their ids and ordered parameter lists @@ -252,7 +254,10 @@ Grouped by what each one constrains. `super.on()` call needs one. The cost is that a subclass can no longer reach those seven through `super` — re-declaring them the same way is its way out. `unknown` rather than `void | Promise` because a listener may return anything: `session.on('close', () => - set.delete(session))` returns a boolean. + set.delete(session))` returns a boolean. This also settles what the drain can wait on: a listener's + own promise would be the better completion signal, and reaching it needs `listeners()`, which + cannot be re-declared the same way — Node types it invariantly enough that widening `void` to + `unknown` is `TS2416`. Re-probed 2026-09-01; `sendResp()` stays the signal. ### The wire @@ -344,7 +349,12 @@ Grouped by what each one constrains. `teardown()` drops what is still held for the same reason it drops inbound segments. The release is one turn late, so a listener that sends its receipt straight after the response is still holding when the drain looks; `sendDlr()` is the one send that goes out past the drain's refusal, - being part of answering a message the drain is itself waiting for. + and only while the message is still held — past that it is an ordinary send, because the drain it + would slip past is no longer waiting for it. `shutdownTimeout: 0` does not carry over to this + half: waiting forever is safe for the peer, whose every request is bounded by `responseTimeout`, + and unsafe for the application, which nothing bounds — `close()` is what you reach for when the + application is stuck, so it may not block on the application coming unstuck. That half falls back + to `responseTimeout`, the same answer the link gate's hold already takes. - **A reconnect keeps the delivery-receipt merges; everything else the link held is dropped.** `onDeliverSm()` answers each receipt before the group it belongs to is complete, and `teardown()` diff --git a/README.md b/README.md index 559b05c..7f435b7 100644 --- a/README.md +++ b/README.md @@ -101,7 +101,7 @@ Every one is optional. | `enquireLinkInterval` | `20000` | How often to send `enquire_link` on a quiet link. | | `idleTimeout` | `2 × enquireLinkInterval` | Give up on a link the peer has stopped answering, and re-bind unless `reconnect` is `false`. | | `responseTimeout` | `30000` | How long to wait for a response before giving up on it, and how long a send with no link waits for the next one; `0` waits forever. | -| `shutdownTimeout` | `5000` | How long `close()` and `unbind()` wait for the requests this end already sent and the messages the application has not answered; `0` waits forever. | +| `shutdownTimeout` | `5000` | How long `close()` and `unbind()` wait for the requests this end already sent and the messages the application has not answered. `0` waits forever for the requests, which the peer answers or times out; the messages fall back to `responseTimeout`, since nothing but the application ends that wait. | | `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, an idle timeout, or a stream the library cannot read, backing off from `minDelay` 1 s to `maxDelay` 30 s and starting over at `minDelay` once a link has lasted `maxDelay`. `{ minDelay, maxDelay }` retunes it; `false` turns it off, so a drop ends the session. | @@ -331,10 +331,12 @@ TypeScript users can import `SmppLog` to have the compiler check one. `sendSms()`, `send()`, `sendReturn()`, `unbind()` and `close()`. Both `close()` and `unbind()` refuse further sends, wait out the requests this end already sent for up to `shutdownTimeout`, and -then tear down whatever is left, resolving to an `err` that says what was lost. They wait on the -`sms` events the application has not answered yet too, so a peer whose `submit_sm` is still being -handled is answered rather than left to re-send it; `sendDlr()` goes out during that wait, and every -other send is refused. `close({ signal })` takes an +then tear down whatever is left, resolving to an `err` that says what was lost. They also wait for +every `sms` the application has not called `sendResp()` on, so a peer whose `submit_sm` is still +being handled is answered rather than left to re-send it — answering its PDUs through `sendReturn()` +instead leaves that wait running until it gives up. `sendDlr()` is the one send the refusal lets +past, and it catches the wait when issued straight after `sendResp()`; await anything in between and +it races the shutdown like any other send. `close({ signal })` takes an `AbortSignal` that cuts the wait short; `unbind()` takes none, and waits a further `responseTimeout` for its own response. `send()` reaches any of the 33 SMPP commands the codec knows, not just the four the session handles natively: diff --git a/src/held-messages.ts b/src/held-messages.ts index 3a12be4..4e1b9b2 100644 --- a/src/held-messages.ts +++ b/src/held-messages.ts @@ -10,6 +10,11 @@ export class HeldMessages { this.held.add(pduObjs); } + /** Whether a drain is still waiting for this message to be answered. */ + has(pduObjs: PduObject[]): boolean { + return this.held.has(pduObjs); + } + release(pduObjs: PduObject[]): void { if (!this.held.delete(pduObjs)) return; diff --git a/src/incoming-requests.ts b/src/incoming-requests.ts index 2f28d1d..e560e80 100644 --- a/src/incoming-requests.ts +++ b/src/incoming-requests.ts @@ -21,8 +21,8 @@ export type IncomingRequestsOptions = { maxReassembly?: number | undefined; onRequest?: OnRequest | undefined; reassemblyTimeout?: number | undefined; - /** Past the drain gate: a receipt answering a message the shutdown is still waiting for. */ - sendHeld: (input: PduObjectInput) => Promise>; + /** Past a drain's refusal, for a receipt the drain is itself waiting for. */ + sendPastDrain: (input: PduObjectInput) => Promise>; session: Session; smsIdFormat?: SmsIdFormat | undefined; systemId?: string | undefined; @@ -35,7 +35,7 @@ export class IncomingRequests { private readonly log: SmppLog; private readonly onRequest: OnRequest | undefined; private readonly reassembler: Reassembler; - private readonly sendHeld: IncomingRequestsOptions['sendHeld']; + private readonly sendPastDrain: IncomingRequestsOptions['sendPastDrain']; private readonly session: Session; private readonly smsIdFormat: SmsIdFormat; private readonly systemId: string; @@ -50,7 +50,7 @@ export class IncomingRequests { maxOctets: options.maxOctets, timeout: options.reassemblyTimeout ?? defaults.reassemblyTimeout, }); - this.sendHeld = options.sendHeld; + this.sendPastDrain = options.sendPastDrain; this.session = options.session; this.smsIdFormat = options.smsIdFormat ?? {}; this.systemId = options.systemId ?? defaults.systemId; @@ -167,7 +167,8 @@ export class IncomingRequests { }, { // A turn later, so a listener sending its receipt straight after the response still holds. onAnswered: () => { setImmediate(() => { this.held.release(pduObjs); }); }, - send: this.sendHeld, + // Past the refusal only while a drain is still waiting for this message; an ordinary send after. + send: input => (this.held.has(pduObjs) ? this.sendPastDrain(input) : this.session.send(input)), }); this.held.hold(pduObjs); diff --git a/src/link-gate.ts b/src/link-gate.ts index a6a51b2..6711790 100644 --- a/src/link-gate.ts +++ b/src/link-gate.ts @@ -47,9 +47,11 @@ export class LinkGate { return this.up || this.returning ? undefined : over(); } - /** When a hold starting now has to give up. 0 never does. */ - deadline(): number { - return this.timeout > 0 ? this.now() + this.timeout : 0; + /** One budget for a request, however many links it waits through. 0 never gives up. */ + hold(signal: AbortSignal | undefined): () => Promise { + const deadline = this.timeout > 0 ? this.now() + this.timeout : 0; + + return () => this.wait(deadline, signal); } /** A link is up and bound: everything held goes out on it. */ @@ -73,7 +75,7 @@ export class LinkGate { } /** Resolves once a link can carry the request, or with the reason none ever will. */ - wait(deadline: number, signal: AbortSignal | undefined): Promise { + private wait(deadline: number, signal: AbortSignal | undefined): Promise { if (this.up) return Promise.resolve({}); const refused = this.refusal(); @@ -86,10 +88,10 @@ export class LinkGate { if (deadline !== 0 && left <= 0) return Promise.resolve({ err: expired() }); - return this.hold(left, signal); + return this.waitForLink(left, signal); } - private hold(left: number, signal: AbortSignal | undefined): Promise { + private waitForLink(left: number, signal: AbortSignal | undefined): Promise { this.log.verbose('linkGate - holding a request until a link is back', { timeout: left }); return new Promise(resolve => { diff --git a/src/outgoing-requests.ts b/src/outgoing-requests.ts new file mode 100644 index 0000000..4e4c490 --- /dev/null +++ b/src/outgoing-requests.ts @@ -0,0 +1,168 @@ +import type { PduObject, PduObjectInput } from './pdu.ts'; +import type { PduTransport } from './pdu-transport.ts'; +import type { Result, VoidResult } from './result.ts'; +import type { SendOptions } from './session-options.ts'; +import type { SmppLog } from './log.ts'; +import { LinkGate } from './link-gate.ts'; +import { PendingRequests } from './pending-requests.ts'; +import { SendWindow } from './send-window.ts'; +import { UnansweredError } from './unanswered-error.ts'; +import { bindCommands } from './session-options.ts'; +import { objToPdu } from './pdu.ts'; + +export type OutgoingRequestsOptions = { + log: SmppLog; + maxOutstanding: number; + responseTimeout: number; + transport: PduTransport; +}; + +/** `retryOnNextLink`: the write failed, so nothing reached the socket and another link may carry it. */ +type Attempt = { result: Result<{ pduObj: PduObject }>; retryOnNextLink: boolean }; + +function abortedBeforeSend(): Error { + return new Error('Aborted before the request was sent'); +} + +/** Everything this end asks of the peer: which link carries it, how many at once, and the answer. */ +export class OutgoingRequests { + private readonly gate: LinkGate; + private readonly log: SmppLog; + private readonly pending: PendingRequests; + private readonly responseTimeout: number; + private readonly transport: PduTransport; + private readonly window: SendWindow; + + private draining = false; + + constructor(options: OutgoingRequestsOptions) { + this.gate = new LinkGate({ log: options.log, timeout: options.responseTimeout }); + this.log = options.log; + this.pending = new PendingRequests(options.log); + this.responseTimeout = options.responseTimeout; + this.transport = options.transport; + this.window = new SendWindow(options.maxOutstanding); + } + + /** Read through a method: a drop can land while a request is awaiting. */ + linkDown(): boolean { + return !this.gate.isUp() || this.transport.sock.destroyed; + } + + /** A link is up and bound, so everything held for one goes out on it. */ + linkUp(): void { + this.gate.open(); + } + + /** The link is gone; `returning` says whether another one is on its way. */ + linkLost(returning: boolean): void { + this.gate.shut(returning); + this.pending.settleAll(new Error('Session closed before a response arrived')); + } + + /** Hands a response to the request waiting for it. False means nothing was. */ + deliver(pduObj: PduObject): boolean { + return this.pending.deliver(pduObj); + } + + /** Sends a request and resolves with the peer's response. */ + request(input: PduObjectInput, options: SendOptions): Promise> { + // A drain on a live link. A link that is down is the gate's answer, which says closed instead. + if (this.draining && !this.linkDown()) { + return Promise.resolve({ err: new Error('Session is shutting down') }); + } + + return this.pastDrain(input, options); + } + + /** The same path without that refusal, which a receipt for a held message has to take. */ + async pastDrain( + input: PduObjectInput, + options: SendOptions, + ): Promise> { + const refused = this.refuse(input, options); + + if (refused) return { err: refused }; + + // A bind is what makes a link usable, so it cannot wait for one. + if (bindCommands.includes(input.cmdName)) return this.now(input, options); + + const waitForLink = this.gate.hold(options.signal); + + for (;;) { + const held = await waitForLink(); + + if (held.err) return { err: held.err }; + + await this.window.acquire(); + + const attempt = await this.attempt(input, options).finally(() => { this.window.release(); }); + + // Nothing reached the socket, so the next link carries it instead of the caller resending. + if (!attempt.retryOnNextLink || this.gate.isUp() || this.gate.refusal()) return attempt.result; + } + } + + /** Past the gate, the window and a drain, for what has to go out either way. */ + async now(input: PduObjectInput, options: SendOptions = {}): Promise> { + return (await this.attempt(input, options)).result; + } + + /** Refuses every request from here on, on a link that is already down as much as a live one. */ + stopAccepting(): void { + this.draining = true; + } + + /** Waits out the requests already on the wire, and says how many never finished. */ + async drain(timeout: number, signal: AbortSignal | undefined): Promise { + const unfinished = await this.window.idle(timeout, signal); + + if (unfinished === 0) return {}; + + this.log.warn('session - shutting down with requests unfinished', { timeout, unfinished }); + + return { err: new Error(`Shut down with ${String(unfinished)} request(s) unfinished`) }; + } + + /** Why a request cannot go out at all, as opposed to not yet. */ + private refuse(input: PduObjectInput, options: SendOptions): Error | undefined { + if (input.cmdName.endsWith('_resp')) { + return new Error(`Use sendReturn() for responses, not send(): ${input.cmdName}`); + } + + // Before the gate and the window, or an aborted call waits for what it will never use. + if (options.signal?.aborted === true) return abortedBeforeSend(); + + // A bind skips the gate below, so the answer it would have given is given here instead. + return bindCommands.includes(input.cmdName) ? this.gate.refusal() : undefined; + } + + private async attempt(input: PduObjectInput, options: SendOptions): Promise { + // pending.wait() alone settles the caller while the request still goes out to the peer. + if (options.signal?.aborted === true) { + return { result: { err: abortedBeforeSend() }, retryOnNextLink: false }; + } + + const seqNr = this.pending.nextSeqNr(); + const built = objToPdu({ ...input, seqNr }); + + if (built.err) return { result: { err: built.err }, retryOnNextLink: false }; + + const response = this.pending.wait(seqNr, { + signal: options.signal, + timeout: this.responseTimeout, + }); + const written = this.transport.write(built.buffer); + + if (written.err) { + this.pending.settle(seqNr, { err: written.err }); + + return { result: { err: written.err }, retryOnNextLink: true }; + } + + const answered = await response; + + // It went out, so a failure now means the peer may have taken it and the answer was the loss. + return { result: answered.err ? { err: new UnansweredError(answered.err) } : answered, retryOnNextLink: false }; + } +} diff --git a/src/pending-requests.ts b/src/pending-requests.ts index 5fdbb1e..7949106 100644 --- a/src/pending-requests.ts +++ b/src/pending-requests.ts @@ -12,14 +12,6 @@ type Pending = { settle: (result: Result<{ pduObj: PduObject }>) => void; }; -/** The request went out and no answer came back: the peer may have accepted it. */ -export class UnansweredError extends Error { - constructor(cause: Error) { - super(`No answer came back, so the peer may have accepted it: ${cause.message}`, { cause }); - this.name = 'UnansweredError'; - } -} - /** Hands out sequence numbers and matches responses to the requests waiting for them. */ export class PendingRequests { private readonly log: SmppLog; diff --git a/src/send-sms.ts b/src/send-sms.ts index 3277330..8b018b4 100644 --- a/src/send-sms.ts +++ b/src/send-sms.ts @@ -4,7 +4,7 @@ import type { PduObject, PduObjectInput } from './pdu.ts'; import type { Result } from './result.ts'; import type { SmppLog } from './log.ts'; import type { SmsIdNotation } from './sms-id.ts'; -import { UnansweredError } from './pending-requests.ts'; +import { UnansweredError } from './unanswered-error.ts'; import { consts } from './defs/constants.ts'; import { detect } from './defs/encodings.ts'; import { normaliseSmsId } from './sms-id.ts'; diff --git a/src/session.ts b/src/session.ts index b297233..24c2b10 100644 --- a/src/session.ts +++ b/src/session.ts @@ -10,17 +10,15 @@ import type { Socket } from 'node:net'; import { DlrMerger } from './dlr-merger.ts'; import { EventEmitter } from 'node:events'; import { IncomingRequests } from './incoming-requests.ts'; -import { LinkGate } from './link-gate.ts'; import { LinkTimers } from './link-timers.ts'; +import { OutgoingRequests } from './outgoing-requests.ts'; import { PduTransport } from './pdu-transport.ts'; -import { PendingRequests, UnansweredError } from './pending-requests.ts'; import { ReconnectLoop } from './reconnect-loop.ts'; -import { SendWindow } from './send-window.ts'; import { leftOf } from './idle-waiters.ts'; import { errorFrom } from './error-from.ts'; import { optionalParamsMinVersion } from './defs/constants.ts'; import { bindCarries, bindCommands, defaultSystemId, defaults } from './session-options.ts'; -import { isResp, objToPdu, pduReturn } from './pdu.ts'; +import { isResp, pduReturn } from './pdu.ts'; import { silentLog } from './log.ts'; import { submitSms, unsent } from './send-sms.ts'; import { ConcatReference } from './udh.ts'; @@ -41,13 +39,6 @@ export { bindCommands, defaultSystemId }; /** A listener may return a promise: an `async` one that rejects is routed like one that throws. */ type SessionListener = (...args: SessionEvents[K]) => unknown; -function abortedBeforeSend(): Error { - return new Error('Aborted before the request was sent'); -} - -/** `retryOnNextLink`: the write failed, so nothing reached the socket and another link may carry it. */ -type Attempt = { result: Result<{ pduObj: PduObject }>; retryOnNextLink: boolean }; - export class Session extends EventEmitter { declare addListener: (event: K, listener: SessionListener) => this; declare off: (event: K, listener: SessionListener) => this; @@ -68,17 +59,14 @@ export class Session extends EventEmitter { private readonly concatReference = new ConcatReference(); private readonly dlrMerger: DlrMerger; - private readonly gate: LinkGate; private readonly incoming: IncomingRequests; private readonly options: SessionOptions; - private readonly pending: PendingRequests; + private readonly outgoing: OutgoingRequests; private readonly reconnectLoop: ReconnectLoop | undefined; private readonly timers: LinkTimers; private readonly transport: PduTransport; - private readonly window: SendWindow; private closed = false; - private draining = false; private ended = false; /** A listener that throws is the application's bug; it must not become ours. Hard rule 1. */ @@ -123,7 +111,6 @@ export class Session extends EventEmitter { max: defaults.maxDlrMerges, timeout: defaults.dlrMergeTimeout, }); - this.gate = new LinkGate({ log: this.log, timeout: options.responseTimeout ?? defaults.responseTimeout }); this.incoming = new IncomingRequests({ dlrMerger: this.dlrMerger, log: this.log, @@ -131,12 +118,11 @@ export class Session extends EventEmitter { maxReassembly: options.maxReassembly, onRequest: options.onRequest, reassemblyTimeout: options.reassemblyTimeout, - sendHeld: input => this.sendThrough(input, {}), + sendPastDrain: input => this.outgoing.pastDrain(input, {}), session: this, smsIdFormat: options.smsIdFormat, systemId: options.systemId, }); - this.pending = new PendingRequests(this.log); this.reconnectLoop = this.loopFor(options.reconnect); this.timers = new LinkTimers({ enquireLinkInterval: options.enquireLinkInterval, @@ -147,7 +133,12 @@ export class Session extends EventEmitter { onIdle: () => { this.teardown(); }, }); this.transport = this.transportFor(options.sock); - this.window = new SendWindow(options.maxOutstanding ?? defaults.maxOutstanding); + this.outgoing = new OutgoingRequests({ + log: this.log, + maxOutstanding: options.maxOutstanding ?? defaults.maxOutstanding, + responseTimeout: options.responseTimeout ?? defaults.responseTimeout, + transport: this.transport, + }); this.resetTimers(); } @@ -170,56 +161,7 @@ export class Session extends EventEmitter { /** Sends a request and resolves with the peer's response. */ send(input: PduObjectInput, options: SendOptions = {}): Promise> { - // A drain on a live link. A link that is down is the gate's answer, which says closed instead. - if (this.draining && !this.linkDown()) return Promise.resolve({ err: new Error('Session is shutting down') }); - - return this.sendThrough(input, options); - } - - /** The same path without that refusal, which a receipt for a held message has to take. */ - private async sendThrough( - input: PduObjectInput, - options: SendOptions = {}, - ): Promise> { - const refused = this.refuseSend(input, options); - - if (refused) return { err: refused }; - - // A bind is what makes a link usable, so it cannot wait for one. - if (bindCommands.includes(input.cmdName)) return (await this.attempt(input, options)).result; - - const deadline = this.gate.deadline(); - - for (;;) { - const held = await this.gate.wait(deadline, options.signal); - - if (held.err) return { err: held.err }; - - await this.window.acquire(); - - const attempt = await this.attempt(input, options).finally(() => { this.window.release(); }); - - // Nothing reached the socket, so the next link carries it instead of the caller resending. - if (!attempt.retryOnNextLink || this.gate.isUp() || !this.retrying()) return attempt.result; - } - } - - /** Why a request cannot go out at all, as opposed to not yet. */ - private refuseSend(input: PduObjectInput, options: SendOptions): Error | undefined { - if (input.cmdName.endsWith('_resp')) { - return new Error(`Use sendReturn() for responses, not send(): ${input.cmdName}`); - } - - // Before the gate and the window, or an aborted call waits for what it will never use. - if (options.signal?.aborted === true) return abortedBeforeSend(); - - // A bind skips the gate below, so the answer it would have given is given here instead. - return bindCommands.includes(input.cmdName) ? this.gate.refusal() : undefined; - } - - /** Read through a method: a drop can land while a send is awaiting. */ - private linkDown(): boolean { - return !this.gate.isUp() || this.sock.destroyed; + return this.outgoing.request(input, options); } /** Answers a request the peer sent us. Responses are never waited on. */ @@ -269,9 +211,9 @@ export class Session extends EventEmitter { async unbind(): Promise { const drained = await this.drain(undefined); const wasOpen = !this.closed; - // attempt(), not send(): the drain gate refuses a send, and the unbind goes out either way. + // now(), not send(): a drain refuses a send, and the unbind goes out either way. const sent = wasOpen - ? (await this.attempt({ cmdName: 'unbind' }, {})).result + ? await this.outgoing.now({ cmdName: 'unbind' }) : { err: new Error('Session is closed') }; const closedOnUnbind = wasOpen && this.closed; @@ -341,7 +283,7 @@ export class Session extends EventEmitter { } this.resetTimers(); - this.gate.open(); + this.outgoing.linkUp(); this.log.info('session - reconnected'); this.emit('reconnected'); @@ -353,58 +295,27 @@ export class Session extends EventEmitter { this.closed = false; } - private async attempt(input: PduObjectInput, options: SendOptions): Promise { - // pending.wait() alone settles the caller while the request still goes out to the peer. - if (options.signal?.aborted === true) { - return { result: { err: abortedBeforeSend() }, retryOnNextLink: false }; - } - - const seqNr = this.pending.nextSeqNr(); - const built = objToPdu({ ...input, seqNr }); - - if (built.err) return { result: { err: built.err }, retryOnNextLink: false }; - - const response = this.pending.wait(seqNr, { - signal: options.signal, - timeout: this.options.responseTimeout ?? defaults.responseTimeout, - }); - const written = this.transport.write(built.buffer); - - if (written.err) { - this.pending.settle(seqNr, { err: written.err }); - - return { result: { err: written.err }, retryOnNextLink: true }; - } - - const answered = await response; - - // It went out, so a failure now means the peer may have taken it and the answer was the loss. - return { result: answered.err ? { err: new UnansweredError(answered.err) } : answered, retryOnNextLink: false }; - } - /** Stops new sends and waits out the messages we hold and the requests already issued. */ private async drain(signal: AbortSignal | undefined): Promise { this.reconnectLoop?.stop(); - this.draining = true; + this.outgoing.stopAccepting(); - if (this.linkDown()) return {}; + if (this.outgoing.linkDown()) return {}; const timeout = this.options.shutdownTimeout ?? defaults.shutdownTimeout; const deadline = timeout > 0 ? Date.now() + timeout : 0; + // Only the application answers a held message, so that half falls back rather than wait forever. + const answering = timeout > 0 ? timeout : (this.options.responseTimeout ?? defaults.responseTimeout); // Answering a message can put a receipt on the wire; nothing on the wire produces a message. - const messages = await this.incoming.drain(timeout, signal); - const unfinished = await this.window.idle(leftOf(deadline), signal); + const messages = await this.incoming.drain(answering, signal); + const requests = await this.outgoing.drain(leftOf(deadline), signal); // The window empties on a teardown too, which settles everything the link was carrying. - if (this.linkDown()) return { err: new Error('The session closed before the drain finished') }; + if (this.outgoing.linkDown()) { + return { err: new Error('The session closed before the drain finished') }; + } - if (messages.err) return messages; - - if (unfinished === 0) return {}; - - this.log.warn('session - shutting down with requests unfinished', { timeout, unfinished }); - - return { err: new Error(`Shut down with ${String(unfinished)} request(s) unfinished`) }; + return messages.err ? messages : requests; } /** The session is over now, drained or not. Nothing brings it back. */ @@ -419,7 +330,7 @@ export class Session extends EventEmitter { if (this.ended) return; this.ended = true; - this.gate.shut(false); + this.outgoing.linkLost(false); this.emit('close'); } @@ -427,9 +338,8 @@ export class Session extends EventEmitter { if (this.closed) return; this.closed = true; - this.gate.shut(this.retrying()); + this.outgoing.linkLost(this.retrying()); this.timers.clear(); - this.pending.settleAll(new Error('Session closed before a response arrived')); this.incoming.clear(); this.sock.destroy(); @@ -448,7 +358,7 @@ export class Session extends EventEmitter { private dispatch(pduObj: PduObject): void { if (isResp(pduObj)) { - if (!this.pending.deliver(pduObj)) { + if (!this.outgoing.deliver(pduObj)) { this.log.debug('session - response with no matching request', { seqNr: pduObj.seqNr }); } diff --git a/src/unanswered-error.ts b/src/unanswered-error.ts new file mode 100644 index 0000000..b6f45b8 --- /dev/null +++ b/src/unanswered-error.ts @@ -0,0 +1,7 @@ +/** The request went out and no answer came back: the peer may have accepted it. */ +export class UnansweredError extends Error { + constructor(cause: Error) { + super(`No answer came back, so the peer may have accepted it: ${cause.message}`, { cause }); + this.name = 'UnansweredError'; + } +} diff --git a/test/session-extras.test.ts b/test/session-extras.test.ts index ed6eedf..963ce05 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -819,12 +819,12 @@ describe('LinkGate', () => { test('refuses a hold whose deadline has already passed', async () => { let now = 0; const gate = new LinkGate({ log: silentLog, now: () => now, timeout: 100 }); - const deadline = gate.deadline(); + const waitForLink = gate.hold(undefined); gate.shut(true); now = 101; - const held = await gate.wait(deadline, undefined); + const held = await waitForLink(); assert.match(held.err?.message ?? '', /did not come back in time/); }); @@ -836,7 +836,7 @@ describe('LinkGate', () => { gate.shut(true); const before = timers(); - const held = gate.wait(gate.deadline(), undefined); + const held = gate.hold(undefined)(); assert.equal(timers(), before + 1, 'an unref\'d timer is not counted here, which is the point'); @@ -851,7 +851,7 @@ describe('LinkGate', () => { gate.shut(true); - const held = await gate.wait(gate.deadline(), AbortSignal.abort()); + const held = await gate.hold(AbortSignal.abort())(); assert.match(held.err?.message ?? '', /Aborted while waiting for a link/); }); @@ -1102,6 +1102,17 @@ describe('graceful shutdown', () => { assert.match(closed.err.message, /1 message\(s\) unanswered/); }); + // Waiting forever is safe for the peer, which every request times out on. The application is not. + test('falls back to responseTimeout for a held message when the shutdown waits forever', async t => { + const { smpp } = await submitInFlight(t, {}, { responseTimeout: 200, shutdownTimeout: 0 }); + const started = Date.now(); + const closed = await peerOf(smpp).close(); + + assert.ok(closed.err instanceof Error); + assert.match(closed.err.message, /1 message\(s\) unanswered/); + assert.ok(Date.now() - started < 2000); + }); + // The README's own listener answers and then sends its receipt, one turn later. test('a receipt sent right after the response still goes out mid-drain', async t => { const { sent, session, smpp, sms } = await submitInFlight(t); diff --git a/todo.md b/todo.md index 2a62cb4..f4e8443 100644 --- a/todo.md +++ b/todo.md @@ -57,6 +57,7 @@ Rules the API follows: | `smsIdFormat`: a peer's `submit_sm_resp` and receipt ids read into one notation before they are compared | `test/dlr.test.ts`, `test/session-extras.test.ts` | | A draining `close()` and `unbind()`, bounded by `shutdownTimeout` or an abort | `test/session-extras.test.ts` | | A drain that also waits out the messages the application has not answered, with `sendDlr()` the one send that passes it | `test/session-extras.test.ts` | +| `OutgoingRequests`: the gate, the window, the pending map and the retry under one owner, told when a link comes up or goes down | `test/session-extras.test.ts`, `test/session.test.ts` | | A send with no link held for the next one, and one the link dropped under counted as `unanswered` | `test/session-extras.test.ts` | | Every runnable README example | `test/readme.test.ts` | | Receipt-versus-message classification by `esm_class` | `test/dlr.test.ts`, `test/session.test.ts` | @@ -118,26 +119,25 @@ session message is a change to every call site. loses every incomplete group, and a peer has no reason to resend a receipt it already had answered. Surviving one means exposing the merge state for the application to persist and hand back, which is a public-surface decision. -- [ ] **Group the session's collaborators under `src/session/`.** Only `session.ts` imports - `reassembly`, `dlr-merger`, `send-window`, `link-timers`, `link-gate`, `reconnect-loop`, - `pending-requests` and `send-sms`, so the directory would make that boundary visible. - `pdu-transport` joined them on 2026-08-31, `link-gate` and `idle-waiters` on 2026-09-01, all - without the move being made, so it is a move of its own now. Do it together with the extraction below rather - than before it — three reactive splits at whatever boundary fitted under the line cap is what - produced the current shape. Raised by review, 2026-09-01. -- [ ] **An `OutgoingRequests` collaborator, owning `LinkGate`, `SendWindow` and `PendingRequests`.** - `session.ts` sits three lines under its 350-line cap and every split so far has been made to - get back under it — the inbound drain on 2026-09-01 only fitted once `ConcatReference` and the - unsent-result shape moved out to `udh.ts` and `send-sms.ts`. The seam that holds: one object owning the gate, the window, the pending - map and the retry loop, exposing a gated `request()` and the ungated door `unbind()` and the - bind already need, with `Session` calling it when a link comes up or goes down instead of - spreading `linkDown()` and `retrying()` across both sides. It would also close two smaller - things — `LinkGate.deadline()` and `wait(deadline)` are a two-call protocol whose only failure - mode is calling `deadline()` inside the loop, which nothing catches; `Session.linkDown()` is - read from both sides of the seam; and `UnansweredError` sits in `pending-requests.ts`, which - never uses it, for the sole edge that makes `send-sms` import that module at all. Every one of - these is unpublished, so it is a two-way door and belongs after 1.0.0. Raised by review, +- [ ] **Group the session's collaborators under `src/session/`.** `session.ts` imports + `dlr-merger`, `incoming-requests`, `link-timers`, `outgoing-requests`, `pdu-transport`, + `reconnect-loop` and `send-sms`, and nothing else does, so the directory would make that + boundary visible. The `OutgoingRequests` extraction this was to be done with landed on + 2026-09-01, so it is the remaining half. Raised by review, 2026-09-01. + +- [ ] **`leftOf()` and the link gate's own budget are one concept counted twice.** + `idle-waiters.ts` reads what is left of a budget as `Math.max(1, deadline - now)`, because 0 + means "forever" there; `link-gate.ts` runs the same subtraction and calls `<= 0` expired. + Neither is reachable from the other, so nothing can disagree today, but a reader who learns one + and applies it to the other is wrong. A budget type both take would close it. Raised by review, 2026-09-01. + +- [ ] **A message the reconnect dropped is answered into the void, and reported as delivered.** + `teardown()` clears the held messages along with the inbound segments, which is right — but the + application still holds the `Sms`, so `sendResp()` writes the old link's sequence numbers to the + new socket, succeeds, and returns `{}` for a response that correlates with nothing at the peer. + `HeldMessages` now knows exactly which messages went that way, so saying so is a small + addition. Goal 2, low frequency. Raised by review, 2026-09-01. - [ ] **Does an intermediate delivery notification deserve to be a `dlr`?** `esm_class` message type `INTERMEDIATE_DELIVERY` (0x20) is classified as a message today, so a peer that reports non-final states with it hands the application a raw `id:… stat:ENROUTE` text as an inbound From 1e7e113bd75425ad2b1ba1b0cc38bca024250d86 Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Tue, 1 Sep 2026 17:03:33 +0200 Subject: [PATCH 15/19] Put a receipt's segments on the wire together, like a message's --- AGENTS.md | 3 ++- README.md | 2 +- src/outgoing-requests.ts | 13 +++++++------ src/session.ts | 17 ++++++++++++++--- src/sms.ts | 15 +++++++++------ test/session-extras.test.ts | 22 ++++++++++++++++------ todo.md | 7 +++++++ 7 files changed, 56 insertions(+), 23 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 41703ed..3ebb92d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -354,7 +354,8 @@ Grouped by what each one constrains. half: waiting forever is safe for the peer, whose every request is bounded by `responseTimeout`, and unsafe for the application, which nothing bounds — `close()` is what you reach for when the application is stuck, so it may not block on the application coming unstuck. That half falls back - to `responseTimeout`, the same answer the link gate's hold already takes. + to `responseTimeout`, the same answer the link gate's hold already takes — and to that option's + default where it is 0 as well, since neither option is an answer about the application. - **A reconnect keeps the delivery-receipt merges; everything else the link held is dropped.** `onDeliverSm()` answers each receipt before the group it belongs to is complete, and `teardown()` diff --git a/README.md b/README.md index 7f435b7..1df59c3 100644 --- a/README.md +++ b/README.md @@ -101,7 +101,7 @@ Every one is optional. | `enquireLinkInterval` | `20000` | How often to send `enquire_link` on a quiet link. | | `idleTimeout` | `2 × enquireLinkInterval` | Give up on a link the peer has stopped answering, and re-bind unless `reconnect` is `false`. | | `responseTimeout` | `30000` | How long to wait for a response before giving up on it, and how long a send with no link waits for the next one; `0` waits forever. | -| `shutdownTimeout` | `5000` | How long `close()` and `unbind()` wait for the requests this end already sent and the messages the application has not answered. `0` waits forever for the requests, which the peer answers or times out; the messages fall back to `responseTimeout`, since nothing but the application ends that wait. | +| `shutdownTimeout` | `5000` | How long `close()` and `unbind()` wait for the requests this end already sent and the messages the application has not answered. `0` waits forever for the requests, which the peer answers or times out; the messages fall back to `responseTimeout`, or to its default where that is 0 too, since nothing but the application ends that wait. | | `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, an idle timeout, or a stream the library cannot read, backing off from `minDelay` 1 s to `maxDelay` 30 s and starting over at `minDelay` once a link has lasted `maxDelay`. `{ minDelay, maxDelay }` retunes it; `false` turns it off, so a drop ends the session. | diff --git a/src/outgoing-requests.ts b/src/outgoing-requests.ts index 4e4c490..d97ddee 100644 --- a/src/outgoing-requests.ts +++ b/src/outgoing-requests.ts @@ -84,8 +84,12 @@ export class OutgoingRequests { if (refused) return { err: refused }; - // A bind is what makes a link usable, so it cannot wait for one. - if (bindCommands.includes(input.cmdName)) return this.now(input, options); + // A bind is what makes a link usable, so it cannot wait for one: it takes the gate's answer now. + if (bindCommands.includes(input.cmdName)) { + const shut = this.gate.refusal(); + + return shut ? { err: shut } : this.now(input, options); + } const waitForLink = this.gate.hold(options.signal); @@ -131,10 +135,7 @@ export class OutgoingRequests { } // Before the gate and the window, or an aborted call waits for what it will never use. - if (options.signal?.aborted === true) return abortedBeforeSend(); - - // A bind skips the gate below, so the answer it would have given is given here instead. - return bindCommands.includes(input.cmdName) ? this.gate.refusal() : undefined; + return options.signal?.aborted === true ? abortedBeforeSend() : undefined; } private async attempt(input: PduObjectInput, options: SendOptions): Promise { diff --git a/src/session.ts b/src/session.ts index 24c2b10..3f232b4 100644 --- a/src/session.ts +++ b/src/session.ts @@ -304,10 +304,8 @@ export class Session extends EventEmitter { const timeout = this.options.shutdownTimeout ?? defaults.shutdownTimeout; const deadline = timeout > 0 ? Date.now() + timeout : 0; - // Only the application answers a held message, so that half falls back rather than wait forever. - const answering = timeout > 0 ? timeout : (this.options.responseTimeout ?? defaults.responseTimeout); // Answering a message can put a receipt on the wire; nothing on the wire produces a message. - const messages = await this.incoming.drain(answering, signal); + const messages = await this.incoming.drain(this.answering(timeout), signal); const requests = await this.outgoing.drain(leftOf(deadline), signal); // The window empties on a teardown too, which settles everything the link was carrying. @@ -318,6 +316,19 @@ export class Session extends EventEmitter { return messages.err ? messages : requests; } + /** + * How long the drain waits for the application, which is the only thing that can end that wait. + * Neither timeout may hand it "forever": both are answers about a peer, and a peer is not what + * this half is waiting for. + */ + private answering(timeout: number): number { + if (timeout > 0) return timeout; + + const responseTimeout = this.options.responseTimeout ?? defaults.responseTimeout; + + return responseTimeout > 0 ? responseTimeout : defaults.responseTimeout; + } + /** The session is over now, drained or not. Nothing brings it back. */ private end(): void { this.reconnectLoop?.stop(); diff --git a/src/sms.ts b/src/sms.ts index be341f2..5c3b6d1 100644 --- a/src/sms.ts +++ b/src/sms.ts @@ -138,11 +138,11 @@ async function sendDlr( } const total = sms.pduObjs.length; - const pduObjs: PduObject[] = []; - - for (let index = 0; index < total; index++) { + // Together, not one after a response: a drain waiting for this message must see the whole receipt. + const sent = await Promise.all(sms.pduObjs.map((_segment, index) => { const smsId = segmentId(sms.smsId, index, total); - const sent = await send({ + + return send({ cmdName: 'deliver_sm', params: { destination_addr: sms.from, @@ -152,10 +152,13 @@ async function sendDlr( }, ...(sms.session.acceptsOptionalParams() ? { tlvs: receiptTlvs(smsId, status) } : {}), }); + })); + const pduObjs: PduObject[] = []; - if (sent.err) return { err: sent.err }; + for (const one of sent) { + if (one.err) return { err: one.err }; - pduObjs.push(sent.pduObj); + pduObjs.push(one.pduObj); } return { pduObjs }; diff --git a/test/session-extras.test.ts b/test/session-extras.test.ts index 963ce05..9587025 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -1041,6 +1041,7 @@ describe('graceful shutdown', () => { t: TestContext, options: Parameters[0] = {}, serverOptions: Parameters[0] = {}, + message = 'answer me', ) { const smpp = await startServer(t, serverOptions); const incoming = once(resolve => { @@ -1050,7 +1051,7 @@ describe('graceful shutdown', () => { assert.ok(session); - const sent = session.sendSms({ from: '46701113311', message: 'answer me', to: '46709771337' }); + const sent = session.sendSms({ from: '46701113311', message, to: '46709771337' }); return { sent, session, sms: await incoming, smpp }; } @@ -1113,20 +1114,29 @@ describe('graceful shutdown', () => { assert.ok(Date.now() - started < 2000); }); - // The README's own listener answers and then sends its receipt, one turn later. + // The README's own listener answers and then sends its receipt, one turn later. Multipart, because + // a receipt sent one-after-a-response outruns that turn on every segment past the first. test('a receipt sent right after the response still goes out mid-drain', async t => { - const { sent, session, smpp, sms } = await submitInFlight(t); - const receipt = once(resolve => { session.on('dlr', resolve); }); + const { sent, session, smpp, sms } = await submitInFlight(t, {}, {}, 'x'.repeat(400)); + const received: Dlr[] = []; + const receipts = once(resolve => { + session.on('dlr', dlr => { + received.push(dlr); + + if (received.length === 3) resolve(received); + }); + }); const closing = peerOf(smpp).close(); await sms.sendResp({ smsId: 'held-through-the-drain' }); const receiptSent = await sms.sendDlr('DELIVERED'); + const ids = ['held-through-the-drain-1', 'held-through-the-drain-2', 'held-through-the-drain-3']; assert.equal(receiptSent.err, undefined); - assert.equal((await receipt).smsId, 'held-through-the-drain'); + assert.deepEqual((await receipts).map(dlr => dlr.smsId), ids); assert.deepEqual(await closing, {}); - assert.deepEqual((await sent).smsIds, ['held-through-the-drain']); + assert.deepEqual((await sent).smsIds, ids); }); test('a message no listener took does not hold the shutdown up', async t => { diff --git a/todo.md b/todo.md index f4e8443..8c08242 100644 --- a/todo.md +++ b/todo.md @@ -132,6 +132,13 @@ session message is a change to every call site. and applies it to the other is wrong. A budget type both take would close it. Raised by review, 2026-09-01. +- [ ] **An `sms` listener that rejects before answering costs a whole `shutdownTimeout`.** + One that *throws* is fine: `emit()` catches it, returns false, and `emitSms()` releases the + hold. A rejecting `async` one reaches `sessionError` through `captureRejections`, which hands + the handler an `unknown[]` the `Sms` cannot be read out of without a cast, so nothing releases. + Same cost as the `onRequest`-answers-nothing case that was declined, but reached by a bug + rather than a policy. Raised by review, 2026-09-01. + - [ ] **A message the reconnect dropped is answered into the void, and reported as delivered.** `teardown()` clears the held messages along with the inbound segments, which is right — but the application still holds the `Sms`, so `sendResp()` writes the old link's sequence numbers to the From 67c0402def9995d8e1109da30aa6538e81e0ba86 Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Tue, 1 Sep 2026 17:42:22 +0200 Subject: [PATCH 16/19] Bound the messages held for a shutdown, and report a partial receipt --- AGENTS.md | 19 +++++----- README.md | 2 +- src/held-messages.ts | 71 +++++++++++++++++++++++++++++++++---- src/incoming-requests.ts | 9 +++-- src/index.ts | 2 +- src/outgoing-requests.ts | 20 +++++++---- src/session-options.ts | 3 ++ src/session.ts | 12 +++---- src/sms.ts | 31 ++++++++++++---- test/session-extras.test.ts | 66 +++++++++++++++++++++++++++++++++- todo.md | 7 ++-- 11 files changed, 201 insertions(+), 41 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 3ebb92d..edeeab1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -72,7 +72,7 @@ src/ dlr-merger.ts DlrMerger: per-segment receipts counted into one MessageDlr error-from.ts errorFrom(): whatever was thrown or rejected, as an Error expiring-groups.ts ExpiringGroups: the capped, expiring store both of those share - held-messages.ts HeldMessages: the messages handed to the application and not yet answered + held-messages.ts HeldMessages: capped, expiring messages the application has not answered idle-waiters.ts IdleWaiters: waiting for a count to fall to zero, and what is left of a budget incoming-requests.ts Every request the peer sends: messages, receipts, links, unknown commands link-gate.ts LinkGate: where a request with no link to go out on waits for the next one @@ -351,7 +351,8 @@ Grouped by what each one constrains. holding when the drain looks; `sendDlr()` is the one send that goes out past the drain's refusal, and only while the message is still held — past that it is an ordinary send, because the drain it would slip past is no longer waiting for it. `shutdownTimeout: 0` does not carry over to this - half: waiting forever is safe for the peer, whose every request is bounded by `responseTimeout`, + half: waiting forever is safe for the peer, whose every request is bounded by `responseTimeout` + unless the caller set that to 0 as well, and unsafe for the application, which nothing bounds — `close()` is what you reach for when the application is stuck, so it may not block on the application coming unstuck. That half falls back to `responseTimeout`, the same answer the link gate's hold already takes — and to that option's @@ -393,13 +394,13 @@ Grouped by what each one constrains. Maintainer's call, 2026-09-01: `attach()` clears `closed` the moment a socket is handed over, one round trip before the bind is answered, so gating on `closed` let a send arriving in that window go out unbound and come back `ESME_RINVBNDSTS`. `LinkGate` owns the answer instead — `shut(returning)` - on every teardown, `open()` only once `comeBackUp()` has a bound link — and `Session.linkDown()` - reads it rather than `closed`. The bind itself cannot wait for what it creates, so `send()` lets the - three bind commands past the gate and the window, the same door `unbind()` takes through - `attempt()`. The gate is told what happened and never reads back into the session: a collaborator - that has to ask does not own its decision, which is how the first cut ended up answering the same - question two different ways at admit and at release. For the same reason the retry in `send()` asks - `gate.isUp()` rather than `linkDown()`, which also reads the socket — a condition that loops on + on every teardown, `open()` only once `comeBackUp()` has a bound link — and + `OutgoingRequests.linkDown()` reads it rather than `closed`. The bind itself cannot wait for what it + creates, so `pastDrain()` lets the three bind commands past the gate and the window, the same door + `unbind()` takes through `now()`. The gate is told what happened and never reads back into the + session: a collaborator that has to ask does not own its decision, which is how the first cut ended + up answering the same question two different ways at admit and at release. For the same reason the + retry in `pastDrain()` asks `gate.isUp()` rather than `linkDown()`, which also reads the socket — a condition that loops on something the gate does not gate on spins against a gate that admits it straight back. `LinkGate.returning` is a copy of `retrying()` taken at teardown, and stays true only because nothing stops the reconnect loop without `emitClose()` following it: `drain()` and `end()` are the diff --git a/README.md b/README.md index 1df59c3..80fcd3e 100644 --- a/README.md +++ b/README.md @@ -101,7 +101,7 @@ Every one is optional. | `enquireLinkInterval` | `20000` | How often to send `enquire_link` on a quiet link. | | `idleTimeout` | `2 × enquireLinkInterval` | Give up on a link the peer has stopped answering, and re-bind unless `reconnect` is `false`. | | `responseTimeout` | `30000` | How long to wait for a response before giving up on it, and how long a send with no link waits for the next one; `0` waits forever. | -| `shutdownTimeout` | `5000` | How long `close()` and `unbind()` wait for the requests this end already sent and the messages the application has not answered. `0` waits forever for the requests, which the peer answers or times out; the messages fall back to `responseTimeout`, or to its default where that is 0 too, since nothing but the application ends that wait. | +| `shutdownTimeout` | `5000` | How long `close()` and `unbind()` wait for the requests this end already sent and the messages the application has not answered. `0` waits forever for the requests, which end when the peer answers or `responseTimeout` expires — so setting both to `0` never ends. The messages fall back to `responseTimeout`, or to its default where that is `0` too, since nothing but the application ends that wait. | | `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, an idle timeout, or a stream the library cannot read, backing off from `minDelay` 1 s to `maxDelay` 30 s and starting over at `minDelay` once a link has lasted `maxDelay`. `{ minDelay, maxDelay }` retunes it; `false` turns it off, so a drop ends the session. | diff --git a/src/held-messages.ts b/src/held-messages.ts index 4e1b9b2..7abdef7 100644 --- a/src/held-messages.ts +++ b/src/held-messages.ts @@ -1,24 +1,68 @@ import type { PduObject } from './pdu.ts'; +import type { SmppLog } from './log.ts'; +import { ExpiringGroups } from './expiring-groups.ts'; import { IdleWaiters } from './idle-waiters.ts'; +export type HeldMessagesOptions = { + log: SmppLog; + max: number; + timeout: number; +}; + +/** The peer's own sequence number, which is what our answer to this message will carry. */ +function keyOf(pduObjs: PduObject[]): string | undefined { + const first = pduObjs[0]; + + return first ? String(first.seqNr) : undefined; +} + /** The messages handed to the application that it has not answered yet, held by their segments. */ export class HeldMessages { - private readonly held = new Set(); + private readonly held: ExpiringGroups; private readonly idleWaiters = new IdleWaiters(); + private readonly log: SmppLog; + constructor(options: HeldMessagesOptions) { + this.held = new ExpiringGroups({ + max: options.max, + onSweep: () => { this.sweep(); }, + timeout: options.timeout, + }); + this.log = options.log; + } + + /** An application that answers no message at all may not grow this without end. */ hold(pduObjs: PduObject[]): void { - this.held.add(pduObjs); + const key = keyOf(pduObjs); + + if (key === undefined) return; + + if (this.held.full) { + const evicted = this.held.takeOldest(); + + if (evicted) { + this.log.warn('heldMessages - dropping the message held longest', { seqNr: evicted[0] }); + } + } + + this.held.set(key, pduObjs); } /** Whether a drain is still waiting for this message to be answered. */ has(pduObjs: PduObject[]): boolean { - return this.held.has(pduObjs); + const key = keyOf(pduObjs); + + return key !== undefined && this.held.get(key) === pduObjs; } release(pduObjs: PduObject[]): void { - if (!this.held.delete(pduObjs)) return; + const key = keyOf(pduObjs); - if (this.held.size === 0) this.idleWaiters.settle(); + // Identity, not the key: a wrapped sequence number must not release someone else's message. + if (key === undefined || this.held.get(key) !== pduObjs) return; + + this.held.delete(key); + this.settle(); } /** Drops every message: their segments went with the link, so no answer of ours correlates now. */ @@ -28,7 +72,22 @@ export class HeldMessages { } /** Resolves 0 once every message has been answered, or with how many have not. */ - idle(timeout: number, signal?: AbortSignal): Promise { + idle(timeout: number, signal: AbortSignal | undefined): Promise { return this.idleWaiters.wait(() => this.held.size, timeout, signal); } + + private sweep(): void { + const expired = this.held.takeExpired(); + + if (expired.length === 0) return; + + this.log.warn('heldMessages - messages the application never answered', { + messages: expired.length, + }); + this.settle(); + } + + private settle(): void { + if (this.held.size === 0) this.idleWaiters.settle(); + } } diff --git a/src/incoming-requests.ts b/src/incoming-requests.ts index e560e80..202f3af 100644 --- a/src/incoming-requests.ts +++ b/src/incoming-requests.ts @@ -31,7 +31,7 @@ export type IncomingRequestsOptions = { /** Everything the peer asks of a session: messages, receipts, links and the answers to them. */ export class IncomingRequests { private readonly dlrMerger: DlrMerger; - private readonly held = new HeldMessages(); + private readonly held: HeldMessages; private readonly log: SmppLog; private readonly onRequest: OnRequest | undefined; private readonly reassembler: Reassembler; @@ -42,6 +42,11 @@ export class IncomingRequests { constructor(options: IncomingRequestsOptions) { this.dlrMerger = options.dlrMerger; + this.held = new HeldMessages({ + log: options.log, + max: defaults.maxHeldMessages, + timeout: defaults.heldMessageTimeout, + }); this.log = options.log; this.onRequest = options.onRequest; this.reassembler = new Reassembler({ @@ -96,7 +101,7 @@ export class IncomingRequests { } /** Waits out the messages the application still holds, and says how many it never answered. */ - async drain(timeout: number, signal?: AbortSignal): Promise { + async drain(timeout: number, signal: AbortSignal | undefined): Promise { const unanswered = await this.held.idle(timeout, signal); if (unanswered === 0) return {}; diff --git a/src/index.ts b/src/index.ts index 72a4615..3cf22e5 100644 --- a/src/index.ts +++ b/src/index.ts @@ -35,7 +35,7 @@ export { uuidv7 } from './uuid.ts'; export type { BindType, ClientOptions } from './client.ts'; export type { Dlr, Receipt } from './dlr.ts'; -export type { SendRespOptions, Sms, SmsInput } from './sms.ts'; +export type { SendDlrResult, SendRespOptions, Sms, SmsInput } from './sms.ts'; export type { ConcatInfo } from './udh.ts'; export type { Result, VoidResult } from './result.ts'; export type { SmppLog } from './log.ts'; diff --git a/src/outgoing-requests.ts b/src/outgoing-requests.ts index d97ddee..e3b4090 100644 --- a/src/outgoing-requests.ts +++ b/src/outgoing-requests.ts @@ -24,6 +24,13 @@ function abortedBeforeSend(): Error { return new Error('Aborted before the request was sent'); } +/** A response carries the request's sequence number, which only sendReturn() has. */ +function misuse(input: PduObjectInput): Error | undefined { + return input.cmdName.endsWith('_resp') + ? new Error(`Use sendReturn() for responses, not send(): ${input.cmdName}`) + : undefined; +} + /** Everything this end asks of the peer: which link carries it, how many at once, and the answer. */ export class OutgoingRequests { private readonly gate: LinkGate; @@ -67,6 +74,11 @@ export class OutgoingRequests { /** Sends a request and resolves with the peer's response. */ request(input: PduObjectInput, options: SendOptions): Promise> { + // Ahead of the drain, so a misuse is named as one rather than blamed on the shutdown. + const wrong = misuse(input); + + if (wrong) return Promise.resolve({ err: wrong }); + // A drain on a live link. A link that is down is the gate's answer, which says closed instead. if (this.draining && !this.linkDown()) { return Promise.resolve({ err: new Error('Session is shutting down') }); @@ -123,19 +135,15 @@ export class OutgoingRequests { if (unfinished === 0) return {}; - this.log.warn('session - shutting down with requests unfinished', { timeout, unfinished }); + this.log.warn('outgoingRequests - shutting down with requests unfinished', { timeout, unfinished }); return { err: new Error(`Shut down with ${String(unfinished)} request(s) unfinished`) }; } /** Why a request cannot go out at all, as opposed to not yet. */ private refuse(input: PduObjectInput, options: SendOptions): Error | undefined { - if (input.cmdName.endsWith('_resp')) { - return new Error(`Use sendReturn() for responses, not send(): ${input.cmdName}`); - } - // Before the gate and the window, or an aborted call waits for what it will never use. - return options.signal?.aborted === true ? abortedBeforeSend() : undefined; + return misuse(input) ?? (options.signal?.aborted === true ? abortedBeforeSend() : undefined); } private async attempt(input: PduObjectInput, options: SendOptions): Promise { diff --git a/src/session-options.ts b/src/session-options.ts index eb79360..514541b 100644 --- a/src/session-options.ts +++ b/src/session-options.ts @@ -101,8 +101,11 @@ export const undeclaredInterfaceVersion = 0x00; export const defaults = { /** Receipts of a multipart message can be a working day apart, so the cap does the bounding. */ dlrMergeTimeout: 86_400_000, + /** The peer gave up on an unanswered message long before this; the bound is against growth. */ + heldMessageTimeout: 300_000, maxDelay: 30_000, maxDlrMerges: 1000, + maxHeldMessages: 1000, maxOutstanding: 10, maxReassembly: 1000, minDelay: 1000, diff --git a/src/session.ts b/src/session.ts index 3f232b4..34868e9 100644 --- a/src/session.ts +++ b/src/session.ts @@ -313,14 +313,14 @@ export class Session extends EventEmitter { return { err: new Error('The session closed before the drain finished') }; } - return messages.err ? messages : requests; + if (!messages.err) return requests; + + if (!requests.err) return messages; + + return { err: new Error(`${messages.err.message}; ${requests.err.message}`) }; } - /** - * How long the drain waits for the application, which is the only thing that can end that wait. - * Neither timeout may hand it "forever": both are answers about a peer, and a peer is not what - * this half is waiting for. - */ + /** The application half's budget, which may never be "forever": nothing else ends that wait. */ private answering(timeout: number): number { if (timeout > 0) return timeout; diff --git a/src/sms.ts b/src/sms.ts index 5c3b6d1..8942854 100644 --- a/src/sms.ts +++ b/src/sms.ts @@ -3,11 +3,20 @@ import type { MessageState } from './defs/constants.ts'; import type { PduObject, PduObjectInput, TlvInput } from './pdu.ts'; import type { Result, VoidResult } from './result.ts'; import type { Session } from './session.ts'; +import { UnansweredError } from './unanswered-error.ts'; import { consts } from './defs/constants.ts'; import { receiptCodes } from './dlr.ts'; import { smppDate } from './message.ts'; import { uuidv7 } from './uuid.ts'; +/** Both fields hold what the peer took, so a partial failure names what is already receipted. */ +export type SendDlrResult = { + err?: Error; + pduObjs: PduObject[]; + /** Segments that went out unanswered. The peer may have taken them, so sending again may duplicate. */ + unanswered: number; +}; + export type SendRespOptions = { /** The id the peer correlates a later delivery receipt by. Defaults to a generated UUID v7. */ smsId?: string; @@ -25,7 +34,7 @@ export type Sms = { message: string; pduObjs: PduObject[]; /** Sends a delivery report back to the sender. Defaults to DELIVERED. */ - sendDlr: (status?: MessageState) => Promise>; + sendDlr: (status?: MessageState) => Promise; /** Answers every segment. Part of the protocol, not optional. Defaults to ESME_ROK. */ sendResp: (options?: SendRespOptions) => Promise; session: Session; @@ -132,9 +141,13 @@ async function sendDlr( sms: Sms, send: SmsHandlers['send'], status: MessageState = 'DELIVERED', -): Promise> { +): Promise { if (!sms.session.bindAllows('deliver_sm')) { - return { err: new Error('A transmitter-bound session does not carry deliver_sm') }; + return { + err: new Error('A transmitter-bound session does not carry deliver_sm'), + pduObjs: [], + unanswered: 0, + }; } const total = sms.pduObjs.length; @@ -154,12 +167,18 @@ async function sendDlr( }); })); const pduObjs: PduObject[] = []; + let failure: Error | undefined; + let unanswered = 0; for (const one of sent) { - if (one.err) return { err: one.err }; + if (!one.err) { + pduObjs.push(one.pduObj); + } else { + if (one.err instanceof UnansweredError) unanswered++; - pduObjs.push(one.pduObj); + failure ??= one.err; + } } - return { pduObjs }; + return failure ? { err: failure, pduObjs, unanswered } : { pduObjs, unanswered }; } diff --git a/test/session-extras.test.ts b/test/session-extras.test.ts index 9587025..0a6c6a3 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -12,6 +12,7 @@ import type { SmppLog } from '../src/log.ts'; import type { Sms } from '../src/sms.ts'; import type { SmppServer } from '../src/server.ts'; import type { TestContext } from 'node:test'; +import { HeldMessages } from '../src/held-messages.ts'; import { LinkGate } from '../src/link-gate.ts'; import { Reassembler, decodeSegments } from '../src/reassembly.ts'; import { Session } from '../src/session.ts'; @@ -857,6 +858,43 @@ describe('LinkGate', () => { }); }); +// Goal 4: an application that answers nothing must not grow this for the life of the link. +describe('held message bounds', () => { + function message(seqNr: number): PduObject[] { + return [{ + cmdId: 0x00000004, + cmdLength: 0, + cmdName: 'submit_sm', + cmdStatus: 'ESME_ROK', + cmdStatusId: 0, + params: { destination_addr: '46709771337', short_message: 'held', source_addr: '46701113311' }, + seqNr, + tlvs: {}, + }]; + } + + test('drops the message held longest rather than holding every one', async () => { + const held = new HeldMessages({ log: silentLog, max: 2, timeout: 10_000 }); + const oldest = message(1); + + held.hold(oldest); + held.hold(message(2)); + held.hold(message(3)); + + assert.equal(held.has(oldest), false); + assert.equal(await held.idle(1, undefined), 2); + }); + + test('gives up on a message the application never answers', async () => { + const held = new HeldMessages({ log: silentLog, max: 10, timeout: 20 }); + + held.hold(message(1)); + + assert.equal(await held.idle(1, undefined), 1); + assert.equal(await held.idle(1000, undefined), 0); + }); +}); + describe('reassembly bounds', () => { function segment(reference: number, part: number, total: number): PduObject { const udh = Buffer.from([0x05, 0x00, 0x03, reference, total, part]); @@ -1108,10 +1146,36 @@ describe('graceful shutdown', () => { const { smpp } = await submitInFlight(t, {}, { responseTimeout: 200, shutdownTimeout: 0 }); const started = Date.now(); const closed = await peerOf(smpp).close(); + const waited = Date.now() - started; assert.ok(closed.err instanceof Error); assert.match(closed.err.message, /1 message\(s\) unanswered/); - assert.ok(Date.now() - started < 2000); + assert.ok(waited >= 190, `waited ${String(waited)} ms, so the fallback was not what bounded it`); + assert.ok(waited < 2000); + }); + + // leftOf() floors what is left at 1 ms: at 0 the request half would read "wait forever" instead. + test('still ends when the message half has spent the whole shutdown budget', async t => { + const { smpp } = await submitInFlight(t, {}, { shutdownTimeout: 100 }); + const bound = peerOf(smpp); + // The client listens for no 'sms', so this one is never answered and stays in the window. + const unanswered = bound.send({ + cmdName: 'submit_sm', + params: { + destination_addr: '46701113311', + short_message: 'nothing answers this', + source_addr: '46709771337', + }, + }); + const closed = await Promise.race([ + bound.close(), + new Promise<{ err?: Error }>(resolve => { + setTimeout(() => { resolve({ err: new Error('close() never returned') }); }, 2000).unref(); + }), + ]); + + assert.match(closed.err?.message ?? '', /1 message\(s\) unanswered; .*1 request\(s\) unfinished/); + assert.ok((await unanswered).err instanceof Error); }); // The README's own listener answers and then sends its receipt, one turn later. Multipart, because diff --git a/todo.md b/todo.md index 8c08242..1f1cacb 100644 --- a/todo.md +++ b/todo.md @@ -58,6 +58,7 @@ Rules the API follows: | A draining `close()` and `unbind()`, bounded by `shutdownTimeout` or an abort | `test/session-extras.test.ts` | | A drain that also waits out the messages the application has not answered, with `sendDlr()` the one send that passes it | `test/session-extras.test.ts` | | `OutgoingRequests`: the gate, the window, the pending map and the retry under one owner, told when a link comes up or goes down | `test/session-extras.test.ts`, `test/session.test.ts` | +| Held messages capped and expiring, so an application that answers nothing cannot grow them | `test/session-extras.test.ts` | | A send with no link held for the next one, and one the link dropped under counted as `unanswered` | `test/session-extras.test.ts` | | Every runnable README example | `test/readme.test.ts` | | Receipt-versus-message classification by `esm_class` | `test/dlr.test.ts`, `test/session.test.ts` | @@ -135,9 +136,9 @@ session message is a change to every call site. - [ ] **An `sms` listener that rejects before answering costs a whole `shutdownTimeout`.** One that *throws* is fine: `emit()` catches it, returns false, and `emitSms()` releases the hold. A rejecting `async` one reaches `sessionError` through `captureRejections`, which hands - the handler an `unknown[]` the `Sms` cannot be read out of without a cast, so nothing releases. - Same cost as the `onRequest`-answers-nothing case that was declined, but reached by a bug - rather than a policy. Raised by review, 2026-09-01. + the handler an `unknown[]` the `Sms` cannot be read out of without a cast, so nothing releases + until the message expires. Same cost as the `onRequest`-answers-nothing case that was declined, + but reached by a bug rather than a policy. Raised by review, 2026-09-01. - [ ] **A message the reconnect dropped is answered into the void, and reported as delivered.** `teardown()` clears the held messages along with the inbound segments, which is right — but the From 61985fb91439da15710a3bf51aea366d6387f16d Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Tue, 1 Sep 2026 17:59:37 +0200 Subject: [PATCH 17/19] Say what a peer did with each receipt segment, and leave no drop unlogged --- AGENTS.md | 24 +++++++++------ README.md | 3 +- src/held-messages.ts | 22 ++++++++++---- src/idle-waiters.ts | 2 +- src/send-window.ts | 2 +- src/sms.ts | 38 ++++++++++++++---------- test/session-extras.test.ts | 58 +++++++++++++++++++++++++++++++++++-- 7 files changed, 113 insertions(+), 36 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index edeeab1..816a46f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -350,13 +350,18 @@ Grouped by what each one constrains. is one turn late, so a listener that sends its receipt straight after the response is still holding when the drain looks; `sendDlr()` is the one send that goes out past the drain's refusal, and only while the message is still held — past that it is an ordinary send, because the drain it - would slip past is no longer waiting for it. `shutdownTimeout: 0` does not carry over to this - half: waiting forever is safe for the peer, whose every request is bounded by `responseTimeout` - unless the caller set that to 0 as well, - and unsafe for the application, which nothing bounds — `close()` is what you reach for when the - application is stuck, so it may not block on the application coming unstuck. That half falls back - to `responseTimeout`, the same answer the link gate's hold already takes — and to that option's - default where it is 0 as well, since neither option is an answer about the application. + would slip past is no longer waiting for it. `shutdownTimeout: 0` does not carry over to this half: + waiting forever is safe for the peer, whose every request is bounded by `responseTimeout` unless the + caller set that to 0 as well, and unsafe for the application, which nothing bounds — `close()` is + what you reach for when the application is stuck, so it may not block on the application coming + unstuck. That half falls back to `responseTimeout`, the same answer the link gate's hold already + takes — and to that option's default where it is 0 as well, since neither option is an answer about + the application. What is held is capped and expiring like every other inbound store, on constants + rather than options, because a bound the application cannot raise is the point: an application that + answers nothing would otherwise grow it for the life of the link, which goal 4 forbids. A message + that falls out of the bound is one the drain stops waiting for, so `close()` can report fewer + unanswered than there were — accepted, because the alternative is holding what nothing will answer, + and both exits are logged. - **A reconnect keeps the delivery-receipt merges; everything else the link held is dropped.** `onDeliverSm()` answers each receipt before the group it belongs to is complete, and `teardown()` @@ -400,8 +405,9 @@ Grouped by what each one constrains. `unbind()` takes through `now()`. The gate is told what happened and never reads back into the session: a collaborator that has to ask does not own its decision, which is how the first cut ended up answering the same question two different ways at admit and at release. For the same reason the - retry in `pastDrain()` asks `gate.isUp()` rather than `linkDown()`, which also reads the socket — a condition that loops on - something the gate does not gate on spins against a gate that admits it straight back. + retry in `pastDrain()` asks `gate.isUp()` rather than `linkDown()`, which also reads the socket — a + condition that loops on something the gate does not gate on spins against a gate that admits it + straight back. `LinkGate.returning` is a copy of `retrying()` taken at teardown, and stays true only because nothing stops the reconnect loop without `emitClose()` following it: `drain()` and `end()` are the only callers of `stop()`. A third caller has to shut the gate itself. diff --git a/README.md b/README.md index 80fcd3e..b036159 100644 --- a/README.md +++ b/README.md @@ -351,7 +351,8 @@ const { err, pduObj } = await session.send({ A send issued while the link is down waits for the reconnect instead of failing, and goes out once the new link is bound — up to `responseTimeout`, after which it gives up having sent nothing. A request already on the wire is the other case: the SMSC may have taken it and lost only the response, -so it fails, and `sendSms()` counts it in `unanswered`, whether the link dropped under it, the peer +so it fails, and `sendSms()` and `sms.sendDlr()` count it in `unanswered`, whether the link dropped +under it, the peer never answered in time, or you aborted it after it went out. Neither applies with `reconnect: false`, where a drop ends the session and every send after it is refused. diff --git a/src/held-messages.ts b/src/held-messages.ts index 7abdef7..5eeb99e 100644 --- a/src/held-messages.ts +++ b/src/held-messages.ts @@ -21,6 +21,7 @@ export class HeldMessages { private readonly held: ExpiringGroups; private readonly idleWaiters = new IdleWaiters(); private readonly log: SmppLog; + private readonly max: number; constructor(options: HeldMessagesOptions) { this.held = new ExpiringGroups({ @@ -29,6 +30,7 @@ export class HeldMessages { timeout: options.timeout, }); this.log = options.log; + this.max = options.max; } /** An application that answers no message at all may not grow this without end. */ @@ -37,12 +39,10 @@ export class HeldMessages { if (key === undefined) return; - if (this.held.full) { - const evicted = this.held.takeOldest(); - - if (evicted) { - this.log.warn('heldMessages - dropping the message held longest', { seqNr: evicted[0] }); - } + if (this.held.get(key)) { + this.log.warn('heldMessages - replacing a message on a re-used sequence number', { seqNr: key }); + } else if (this.held.full) { + this.dropOldest(); } this.held.set(key, pduObjs); @@ -76,6 +76,16 @@ export class HeldMessages { return this.idleWaiters.wait(() => this.held.size, timeout, signal); } + private dropOldest(): void { + const oldest = this.held.takeOldest(); + + if (!oldest) return; + + const [seqNr] = oldest; + + this.log.warn('heldMessages - buffer full, dropping the oldest message', { max: this.max, seqNr }); + } + private sweep(): void { const expired = this.held.takeExpired(); diff --git a/src/idle-waiters.ts b/src/idle-waiters.ts index 50c9311..dd29a9e 100644 --- a/src/idle-waiters.ts +++ b/src/idle-waiters.ts @@ -18,7 +18,7 @@ export class IdleWaiters { * Resolves 0 once nothing is left, or with what still is when the timeout or the signal cuts the * wait short. A timeout of 0 waits forever. */ - wait(remaining: () => number, timeout: number, signal?: AbortSignal): Promise { + wait(remaining: () => number, timeout: number, signal: AbortSignal | undefined): Promise { if (remaining() === 0) return Promise.resolve(0); if (signal?.aborted === true) return Promise.resolve(remaining()); diff --git a/src/send-window.ts b/src/send-window.ts index 46560c3..524df8d 100644 --- a/src/send-window.ts +++ b/src/send-window.ts @@ -43,7 +43,7 @@ export class SendWindow { } /** Resolves 0 once nothing is left on the wire, or with what still is. */ - idle(timeout: number, signal?: AbortSignal): Promise { + idle(timeout: number, signal: AbortSignal | undefined): Promise { return this.idleWaiters.wait(() => this.unfinished(), timeout, signal); } } diff --git a/src/sms.ts b/src/sms.ts index 8942854..5b741b4 100644 --- a/src/sms.ts +++ b/src/sms.ts @@ -137,6 +137,28 @@ function receiptTlvs(smsId: string, status: MessageState): Record[]): SendDlrResult { + const pduObjs: PduObject[] = []; + let failure: Error | undefined; + let unanswered = 0; + + for (const one of sent) { + if (one.err) { + if (one.err instanceof UnansweredError) unanswered++; + + failure ??= one.err; + } else if (one.pduObj.cmdStatus === 'ESME_ROK') { + pduObjs.push(one.pduObj); + } else { + const refusal = one.pduObj.cmdStatus ?? String(one.pduObj.cmdStatusId); + + failure ??= new Error(`deliver_sm refused by the peer: ${refusal}`); + } + } + + return failure ? { err: failure, pduObjs, unanswered } : { pduObjs, unanswered }; +} + async function sendDlr( sms: Sms, send: SmsHandlers['send'], @@ -166,19 +188,5 @@ async function sendDlr( ...(sms.session.acceptsOptionalParams() ? { tlvs: receiptTlvs(smsId, status) } : {}), }); })); - const pduObjs: PduObject[] = []; - let failure: Error | undefined; - let unanswered = 0; - - for (const one of sent) { - if (!one.err) { - pduObjs.push(one.pduObj); - } else { - if (one.err instanceof UnansweredError) unanswered++; - - failure ??= one.err; - } - } - - return failure ? { err: failure, pduObjs, unanswered } : { pduObjs, unanswered }; + return collectReceipt(sent); } diff --git a/test/session-extras.test.ts b/test/session-extras.test.ts index 0a6c6a3..3bac0f8 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -13,6 +13,8 @@ import type { Sms } from '../src/sms.ts'; import type { SmppServer } from '../src/server.ts'; import type { TestContext } from 'node:test'; import { HeldMessages } from '../src/held-messages.ts'; +import { UnansweredError } from '../src/unanswered-error.ts'; +import { createSms } from '../src/sms.ts'; import { LinkGate } from '../src/link-gate.ts'; import { Reassembler, decodeSegments } from '../src/reassembly.ts'; import { Session } from '../src/session.ts'; @@ -860,8 +862,8 @@ describe('LinkGate', () => { // Goal 4: an application that answers nothing must not grow this for the life of the link. describe('held message bounds', () => { - function message(seqNr: number): PduObject[] { - return [{ + function heldPdu(seqNr: number): PduObject { + return { cmdId: 0x00000004, cmdLength: 0, cmdName: 'submit_sm', @@ -870,7 +872,11 @@ describe('held message bounds', () => { params: { destination_addr: '46709771337', short_message: 'held', source_addr: '46701113311' }, seqNr, tlvs: {}, - }]; + }; + } + + function message(seqNr: number): PduObject[] { + return [heldPdu(seqNr)]; } test('drops the message held longest rather than holding every one', async () => { @@ -893,6 +899,37 @@ describe('held message bounds', () => { assert.equal(await held.idle(1, undefined), 1); assert.equal(await held.idle(1000, undefined), 0); }); + + // A receipt cannot be resent wholesale without duplicating the segments that landed, so sendDlr() + // names what the peer took and what it may have, the way sendSms() does. + test('a partial receipt names the segments the peer took and the ones it may have', async t => { + const session = new Session({ sock: new net.Socket() }); + + closeAfter(t, session); + + let call = 0; + const sms = createSms({ + from: '46701113311', + message: 'three segments', + pduObjs: [heldPdu(1), heldPdu(2), heldPdu(3)], + session, + to: '46709771337', + }, { + onAnswered: () => undefined, + send: () => { + call++; + + return Promise.resolve(call === 2 + ? { err: new UnansweredError(new Error('nothing came back')) } + : { pduObj: heldPdu(call) }); + }, + }); + const report = await sms.sendDlr('DELIVERED'); + + assert.ok(report.err instanceof Error); + assert.equal(report.pduObjs.length, 2); + assert.equal(report.unanswered, 1); + }); }); describe('reassembly bounds', () => { @@ -1112,6 +1149,21 @@ describe('graceful shutdown', () => { assert.deepEqual(await closed, {}); }); + // The drain refuses sends; a response was never a send, and saying so is the more useful answer. + test('names a response put through send() as the misuse it is, even mid-shutdown', async t => { + const { sent, session, sms } = await submitInFlight(t); + const closing = session.close(); + const refused = await session.send({ cmdName: 'submit_sm_resp' }); + + assert.ok(refused.err instanceof Error); + assert.match(refused.err.message, /Use sendReturn\(\)/); + + await sms.sendResp({ smsId: 'answered-after-the-misuse' }); + + assert.deepEqual((await sent).smsIds, ['answered-after-the-misuse']); + assert.deepEqual(await closing, {}); + }); + test('unbind() waits out a submit already on the wire before it unbinds', async t => { const { sent, session, sms } = await submitInFlight(t); const unbound = session.unbind(); From 042a62be9790e7038282608ff1e1461f6e6d281b Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Tue, 1 Sep 2026 18:02:15 +0200 Subject: [PATCH 18/19] Sweep held messages before each hold, so expiry needs no live timer --- src/held-messages.ts | 12 +++++++++++- test/session-extras.test.ts | 20 ++++++++++++++------ 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/src/held-messages.ts b/src/held-messages.ts index 5eeb99e..57ddfde 100644 --- a/src/held-messages.ts +++ b/src/held-messages.ts @@ -6,6 +6,8 @@ import { IdleWaiters } from './idle-waiters.ts'; export type HeldMessagesOptions = { log: SmppLog; max: number; + /** Injected so expiry can be exercised without a wall clock. */ + now?: (() => number) | undefined; timeout: number; }; @@ -26,6 +28,7 @@ export class HeldMessages { constructor(options: HeldMessagesOptions) { this.held = new ExpiringGroups({ max: options.max, + now: options.now, onSweep: () => { this.sweep(); }, timeout: options.timeout, }); @@ -33,12 +36,18 @@ export class HeldMessages { this.max = options.max; } + get size(): number { + return this.held.size; + } + /** An application that answers no message at all may not grow this without end. */ hold(pduObjs: PduObject[]): void { const key = keyOf(pduObjs); if (key === undefined) return; + this.sweep(); + if (this.held.get(key)) { this.log.warn('heldMessages - replacing a message on a re-used sequence number', { seqNr: key }); } else if (this.held.full) { @@ -86,7 +95,8 @@ export class HeldMessages { this.log.warn('heldMessages - buffer full, dropping the oldest message', { max: this.max, seqNr }); } - private sweep(): void { + /** Drops every message past its deadline. Runs before each hold and on its own timer. */ + sweep(): void { const expired = this.held.takeExpired(); if (expired.length === 0) return; diff --git a/test/session-extras.test.ts b/test/session-extras.test.ts index 3bac0f8..f011bbc 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -879,7 +879,7 @@ describe('held message bounds', () => { return [heldPdu(seqNr)]; } - test('drops the message held longest rather than holding every one', async () => { + test('drops the message held longest rather than holding every one', () => { const held = new HeldMessages({ log: silentLog, max: 2, timeout: 10_000 }); const oldest = message(1); @@ -887,17 +887,25 @@ describe('held message bounds', () => { held.hold(message(2)); held.hold(message(3)); + assert.equal(held.size, 2); assert.equal(held.has(oldest), false); - assert.equal(await held.idle(1, undefined), 2); + + held.clear(); }); - test('gives up on a message the application never answers', async () => { - const held = new HeldMessages({ log: silentLog, max: 10, timeout: 20 }); + test('gives up on a message the application never answers', () => { + let now = 0; + const held = new HeldMessages({ log: silentLog, max: 10, now: () => now, timeout: 60 }); held.hold(message(1)); + now = 61; - assert.equal(await held.idle(1, undefined), 1); - assert.equal(await held.idle(1000, undefined), 0); + // The next message sweeps the one that expired, so only the new one is still waited for. + held.hold(message(2)); + + assert.equal(held.size, 1); + + held.clear(); }); // A receipt cannot be resent wholesale without duplicating the segments that landed, so sendDlr() From f0327044b161b636bdc42ffbd188cfdc3da3ec26 Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Tue, 1 Sep 2026 18:13:35 +0200 Subject: [PATCH 19/19] Cover the receipt refusal and the sweep that wakes a drain --- AGENTS.md | 7 ++-- README.md | 3 +- src/held-messages.ts | 7 ++-- src/sms.ts | 2 +- test/session-extras.test.ts | 70 +++++++++++++++++++++++++------------ 5 files changed, 58 insertions(+), 31 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 816a46f..d791e9d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -407,10 +407,9 @@ Grouped by what each one constrains. up answering the same question two different ways at admit and at release. For the same reason the retry in `pastDrain()` asks `gate.isUp()` rather than `linkDown()`, which also reads the socket — a condition that loops on something the gate does not gate on spins against a gate that admits it - straight back. - `LinkGate.returning` is a copy of `retrying()` taken at teardown, and stays true only because - nothing stops the reconnect loop without `emitClose()` following it: `drain()` and `end()` are the - only callers of `stop()`. A third caller has to shut the gate itself. + straight back. `LinkGate.returning` is a copy of `retrying()` taken at teardown, and stays true + only because nothing stops the reconnect loop without `emitClose()` following it: `drain()` and + `end()` are the only callers of `stop()`. A third caller has to shut the gate itself. ### Internals and tests diff --git a/README.md b/README.md index b036159..51acdc6 100644 --- a/README.md +++ b/README.md @@ -352,8 +352,7 @@ A send issued while the link is down waits for the reconnect instead of failing, the new link is bound — up to `responseTimeout`, after which it gives up having sent nothing. A request already on the wire is the other case: the SMSC may have taken it and lost only the response, so it fails, and `sendSms()` and `sms.sendDlr()` count it in `unanswered`, whether the link dropped -under it, the peer -never answered in time, or you aborted it after it went out. Neither applies with `reconnect: false`, +under it, the peer never answered in time, or you aborted it after it went out. Neither applies with `reconnect: false`, where a drop ends the session and every send after it is refused. `responseTimeout` bounds the wait for a link and the wait for an answer separately, and a send also diff --git a/src/held-messages.ts b/src/held-messages.ts index 57ddfde..0a2a665 100644 --- a/src/held-messages.ts +++ b/src/held-messages.ts @@ -49,7 +49,7 @@ export class HeldMessages { this.sweep(); if (this.held.get(key)) { - this.log.warn('heldMessages - replacing a message on a re-used sequence number', { seqNr: key }); + this.log.warn('heldMessages - replacing a message on a re-used sequence number', { seqNr: Number(key) }); } else if (this.held.full) { this.dropOldest(); } @@ -92,7 +92,10 @@ export class HeldMessages { const [seqNr] = oldest; - this.log.warn('heldMessages - buffer full, dropping the oldest message', { max: this.max, seqNr }); + this.log.warn('heldMessages - buffer full, dropping the oldest message', { + max: this.max, + seqNr: Number(seqNr), + }); } /** Drops every message past its deadline. Runs before each hold and on its own timer. */ diff --git a/src/sms.ts b/src/sms.ts index 5b741b4..a93cf6f 100644 --- a/src/sms.ts +++ b/src/sms.ts @@ -9,7 +9,7 @@ import { receiptCodes } from './dlr.ts'; import { smppDate } from './message.ts'; import { uuidv7 } from './uuid.ts'; -/** Both fields hold what the peer took, so a partial failure names what is already receipted. */ +/** `pduObjs` holds what the peer took, so a partial failure names what is already receipted. */ export type SendDlrResult = { err?: Error; pduObjs: PduObject[]; diff --git a/test/session-extras.test.ts b/test/session-extras.test.ts index f011bbc..91c41be 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -73,6 +73,19 @@ function delay(ms: number): Promise { return new Promise(resolve => { setTimeout(resolve, ms); }); } +function submitPdu(seqNr: number, cmdStatus: ErrorName = 'ESME_ROK'): PduObject { + return { + cmdId: 0x00000004, + cmdLength: 0, + cmdName: 'submit_sm', + cmdStatus, + cmdStatusId: 0, + params: { destination_addr: '46709771337', short_message: 'held', source_addr: '46701113311' }, + seqNr, + tlvs: {}, + }; +} + /** A cmd_length below the 16-octet header: a stream no framing can recover from. */ const unreadablePdu = Buffer.from([0, 0, 0, 4, 0, 0, 0, 4, 0, 0, 0, 0, 0, 0, 0, 1]); @@ -862,21 +875,8 @@ describe('LinkGate', () => { // Goal 4: an application that answers nothing must not grow this for the life of the link. describe('held message bounds', () => { - function heldPdu(seqNr: number): PduObject { - return { - cmdId: 0x00000004, - cmdLength: 0, - cmdName: 'submit_sm', - cmdStatus: 'ESME_ROK', - cmdStatusId: 0, - params: { destination_addr: '46709771337', short_message: 'held', source_addr: '46701113311' }, - seqNr, - tlvs: {}, - }; - } - function message(seqNr: number): PduObject[] { - return [heldPdu(seqNr)]; + return [submitPdu(seqNr)]; } test('drops the message held longest rather than holding every one', () => { @@ -885,6 +885,11 @@ describe('held message bounds', () => { held.hold(oldest); held.hold(message(2)); + held.hold(message(2)); + + assert.equal(held.size, 2, 'a re-used sequence number replaces rather than evicting'); + assert.equal(held.has(oldest), true); + held.hold(message(3)); assert.equal(held.size, 2); @@ -908,9 +913,25 @@ describe('held message bounds', () => { held.clear(); }); - // A receipt cannot be resent wholesale without duplicating the segments that landed, so sendDlr() - // names what the peer took and what it may have, the way sendSms() does. - test('a partial receipt names the segments the peer took and the ones it may have', async t => { + // Without this the drain sits out its whole budget before returning what a sweep already settled. + test('wakes a waiting drain when the last message expires', async () => { + let now = 0; + const held = new HeldMessages({ log: silentLog, max: 10, now: () => now, timeout: 60 }); + + held.hold(message(1)); + + const waiting = held.idle(1000, undefined); + + now = 61; + held.sweep(); + + assert.equal(await waiting, 0); + }); +}); + +describe('sendDlr()', () => { + // A receipt cannot be resent wholesale without duplicating the segments that landed. + test('names the segments the peer took, refused, and may have taken', async t => { const session = new Session({ sock: new net.Socket() }); closeAfter(t, session); @@ -919,7 +940,7 @@ describe('held message bounds', () => { const sms = createSms({ from: '46701113311', message: 'three segments', - pduObjs: [heldPdu(1), heldPdu(2), heldPdu(3)], + pduObjs: [submitPdu(1), submitPdu(2), submitPdu(3)], session, to: '46709771337', }, { @@ -927,15 +948,20 @@ describe('held message bounds', () => { send: () => { call++; - return Promise.resolve(call === 2 - ? { err: new UnansweredError(new Error('nothing came back')) } - : { pduObj: heldPdu(call) }); + if (call === 1) return Promise.resolve({ pduObj: submitPdu(1, 'ESME_RX_T_APPN') }); + + if (call === 2) { + return Promise.resolve({ err: new UnansweredError(new Error('nothing came back')) }); + } + + return Promise.resolve({ pduObj: submitPdu(3) }); }, }); const report = await sms.sendDlr('DELIVERED'); assert.ok(report.err instanceof Error); - assert.equal(report.pduObjs.length, 2); + assert.match(report.err.message, /deliver_sm refused by the peer/); + assert.equal(report.pduObjs.length, 1); assert.equal(report.unanswered, 1); }); });