From 82a95593bdc4b7ef0a5119d17996e1bc028310f1 Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Tue, 1 Sep 2026 19:16:21 +0200 Subject: [PATCH 1/4] Refuse to answer a message whose link went, instead of reporting it delivered --- README.md | 4 +++ src/dlr-merger.ts | 4 +-- src/expiring-groups.ts | 7 +++++- src/held-messages.ts | 11 ++++++++- src/incoming-requests.ts | 1 + src/reassembly.ts | 2 +- src/sms.ts | 11 ++++++++- test/session-extras.test.ts | 49 +++++++++++++++++++++++++++++++++++++ todo.md | 7 +----- 9 files changed, 84 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index 51acdc6..cb67bf3 100644 --- a/README.md +++ b/README.md @@ -355,6 +355,10 @@ so it fails, and `sendSms()` and `sms.sendDlr()` count it in `unanswered`, wheth 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. +An answer cannot wait for a link that way, because it carries the sequence number the message arrived +on: `sms.sendResp()` on a message whose link dropped writes nothing and returns an `err`. Its +`sms.sendDlr()` still goes out, since a receipt is a request of its own, correlated by the id it names. + `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. diff --git a/src/dlr-merger.ts b/src/dlr-merger.ts index 71d30a6..678ad10 100644 --- a/src/dlr-merger.ts +++ b/src/dlr-merger.ts @@ -120,8 +120,8 @@ export class DlrMerger { } clear(): void { - this.groups.clear(); - this.spent.clear(); + this.groups.takeAll(); + this.spent.takeAll(); } /** Drops every group past its deadline. Runs before each collect and on its own timer. */ diff --git a/src/expiring-groups.ts b/src/expiring-groups.ts index 584a1a2..3a25d89 100644 --- a/src/expiring-groups.ts +++ b/src/expiring-groups.ts @@ -58,9 +58,14 @@ export class ExpiringGroups { this.idle(); } - clear(): void { + /** Removes every group and hands them over. */ + takeAll(): [string, T][] { + const taken: [string, T][] = [...this.entries].map(([key, entry]) => [key, entry.group]); + this.entries.clear(); this.idle(); + + return taken; } /** Removes every group past its deadline and hands them over. */ diff --git a/src/held-messages.ts b/src/held-messages.ts index 0a2a665..3324784 100644 --- a/src/held-messages.ts +++ b/src/held-messages.ts @@ -20,6 +20,7 @@ function keyOf(pduObjs: PduObject[]): string | undefined { /** The messages handed to the application that it has not answered yet, held by their segments. */ export class HeldMessages { + private readonly droppedWithLink = new WeakSet(); private readonly held: ExpiringGroups; private readonly idleWaiters = new IdleWaiters(); private readonly log: SmppLog; @@ -64,6 +65,11 @@ export class HeldMessages { return key !== undefined && this.held.get(key) === pduObjs; } + /** Whether this message's link went before it was answered, so no answer of ours correlates. */ + lostLink(pduObjs: PduObject[]): boolean { + return this.droppedWithLink.has(pduObjs); + } + release(pduObjs: PduObject[]): void { const key = keyOf(pduObjs); @@ -76,7 +82,10 @@ export class HeldMessages { /** Drops every message: their segments went with the link, so no answer of ours correlates now. */ clear(): void { - this.held.clear(); + for (const [, pduObjs] of this.held.takeAll()) { + this.droppedWithLink.add(pduObjs); + } + this.idleWaiters.settle(); } diff --git a/src/incoming-requests.ts b/src/incoming-requests.ts index 202f3af..0487401 100644 --- a/src/incoming-requests.ts +++ b/src/incoming-requests.ts @@ -170,6 +170,7 @@ export class IncomingRequests { session: this.session, to: paramText(first.params.destination_addr), }, { + lostLink: () => this.held.lostLink(pduObjs), // A turn later, so a listener sending its receipt straight after the response still holds. onAnswered: () => { setImmediate(() => { this.held.release(pduObjs); }); }, // Past the refusal only while a drain is still waiting for this message; an ordinary send after. diff --git a/src/reassembly.ts b/src/reassembly.ts index efc5e2e..59b1634 100644 --- a/src/reassembly.ts +++ b/src/reassembly.ts @@ -163,7 +163,7 @@ export class Reassembler { } clear(): void { - this.groups.clear(); + this.groups.takeAll(); this.octets = 0; } diff --git a/src/sms.ts b/src/sms.ts index a93cf6f..8f8f847 100644 --- a/src/sms.ts +++ b/src/sms.ts @@ -54,6 +54,8 @@ export type SmsInput = { /** What the session's incoming side gives a message so it can be answered and accounted for. */ export type SmsHandlers = { + /** Whether the link this message arrived on is gone, so no answer of ours correlates. */ + lostLink: () => boolean; onAnswered: () => void; send: (input: PduObjectInput) => Promise>; }; @@ -76,7 +78,8 @@ export function createSms(input: SmsInput, handlers: SmsHandlers): Sms { message: input.message, pduObjs: input.pduObjs, sendDlr: status => sendDlr(sms, handlers.send, status), - sendResp: options => sendResp(sms, answered, options ?? {}).finally(handlers.onAnswered), + sendResp: options => sendResp(sms, answered, options ?? {}, handlers.lostLink) + .finally(handlers.onAnswered), session: input.session, get smsId(): string { return answered.smsId; @@ -92,6 +95,7 @@ async function sendResp( sms: Sms, answered: { smsId: string }, options: SendRespOptions, + lostLink: () => boolean, ): Promise { const total = sms.pduObjs.length; @@ -103,6 +107,11 @@ async function sendResp( return { err: new Error('smsId must not be empty') }; } + // A response carries the sequence number it was asked on, which the next link knows nothing about. + if (lostLink()) { + return { err: new Error('The link this message arrived on is gone, so nothing would correlate the response') }; + } + if (options.smsId !== undefined) answered.smsId = options.smsId; const results = await Promise.all(sms.pduObjs.map((pduObj, index) => sms.session.sendReturn( diff --git a/test/session-extras.test.ts b/test/session-extras.test.ts index 91c41be..087deec 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -557,6 +557,40 @@ describe('reconnect', () => { assert.equal(report.segments.length, 3); }); + test('refuses to answer a message whose link went, rather than reporting it delivered', async t => { + const smpp = await startServer(t); + const { session } = await connect(t, smpp, { reconnect: { maxDelay: 100, minDelay: 20 } }); + + assert.ok(session); + + const arrived = once(resolve => { session.on('sms', resolve); }); + + void peerOf(smpp).send({ + cmdName: 'deliver_sm', + params: { + destination_addr: '46701113311', + short_message: 'answered too late', + source_addr: '46709771337', + }, + }); + + const sms = await arrived; + const reconnected = once(resolve => { session.on('reconnected', () => { resolve(true); }); }); + + await peerOf(smpp).close({ signal: AbortSignal.abort() }); + await reconnected; + + const answered = await sms.sendResp(); + + assert.match(answered.err?.message ?? '', /link this message arrived on is gone/); + + // A receipt is a request of its own, correlated by its id, so the new link carries it. + const report = await sms.sendDlr('DELIVERED'); + + assert.equal(report.err, undefined); + assert.equal(report.pduObjs.length, 1); + }); + test('does not reconnect after an explicit close', async t => { const smpp = await startServer(t); const { session } = await connect(t, smpp, { reconnect: { maxDelay: 50, minDelay: 10 } }); @@ -913,6 +947,20 @@ describe('held message bounds', () => { held.clear(); }); + test('remembers which messages went with the link, and which were answered', () => { + const held = new HeldMessages({ log: silentLog, max: 10, timeout: 10_000 }); + const answered = message(1); + const dropped = message(2); + + held.hold(answered); + held.hold(dropped); + held.release(answered); + held.clear(); + + assert.equal(held.lostLink(dropped), true); + assert.equal(held.lostLink(answered), false); + }); + // 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; @@ -944,6 +992,7 @@ describe('sendDlr()', () => { session, to: '46709771337', }, { + lostLink: () => false, onAnswered: () => undefined, send: () => { call++; diff --git a/todo.md b/todo.md index 1f1cacb..b495093 100644 --- a/todo.md +++ b/todo.md @@ -60,6 +60,7 @@ Rules the API follows: | `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` | +| A message whose link dropped refused an answer, with its receipt still allowed out | `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` | @@ -140,12 +141,6 @@ session message is a change to every call site. 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 - 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 c8d265d5fa578f4fd526432ce64e4d93ce2b80a8 Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Tue, 1 Sep 2026 20:25:57 +0200 Subject: [PATCH 2/4] Tie the refusal to the link, not to the hold, so a released message is refused too --- AGENTS.md | 8 +++++ README.md | 2 +- src/dlr-merger.ts | 4 +-- src/expiring-groups.ts | 7 +--- src/held-messages.ts | 11 +------ src/incoming-requests.ts | 7 +++- src/reassembly.ts | 2 +- src/sms.ts | 5 ++- test/session-extras.test.ts | 64 ++++++++++++++++++++----------------- todo.md | 5 +++ 10 files changed, 61 insertions(+), 54 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index d791e9d..0993c65 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -307,6 +307,14 @@ Grouped by what each one constrains. 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. +- **An answer belongs to the link the message arrived on; a receipt does not.** Maintainer's call, + 2026-09-01: `sendResp()` carries the peer's own sequence number, which the next link knows nothing + about, so on a message whose link went it writes nothing and returns an `err`. Writing it to the + new socket instead succeeds and reports `{}` for a response that correlates with nothing, which is + goal 2's wrong answer. `sendDlr()` still goes out, because a receipt is a request of its own + correlated by `receipted_message_id`; the accepted cost is that one sent after a refused response + names an id the peer has no record of. + - **`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, diff --git a/README.md b/README.md index cb67bf3..7e48f98 100644 --- a/README.md +++ b/README.md @@ -398,7 +398,7 @@ The spec tables are exported both individually (`cmds`, `consts`, `encodings`, ` - **`server()` resolves once, when it is listening**, and gives you a handle with `close()`, `port` and a `session` event. It no longer calls your callback once per incoming connection. - **The id a message is answered with goes to `sendResp({ smsId })`**, and `sms.smsId` is read-only: - it reports what the response actually carried. Delete any `sms.smsId = …` line — assigning to it + it reports the id `sendResp()` was given, or the UUID v7 generated instead. Delete any `sms.smsId = …` line — assigning to it throws a `TypeError`, since modules are always strict mode — and pass the id to `sendResp()`. - **`checkuserpass` is now `authenticate`**, takes `{ password, session, systemId, systemType }` and returns `false` or `{ userData }`. diff --git a/src/dlr-merger.ts b/src/dlr-merger.ts index 678ad10..71d30a6 100644 --- a/src/dlr-merger.ts +++ b/src/dlr-merger.ts @@ -120,8 +120,8 @@ export class DlrMerger { } clear(): void { - this.groups.takeAll(); - this.spent.takeAll(); + this.groups.clear(); + this.spent.clear(); } /** Drops every group past its deadline. Runs before each collect and on its own timer. */ diff --git a/src/expiring-groups.ts b/src/expiring-groups.ts index 3a25d89..584a1a2 100644 --- a/src/expiring-groups.ts +++ b/src/expiring-groups.ts @@ -58,14 +58,9 @@ export class ExpiringGroups { this.idle(); } - /** Removes every group and hands them over. */ - takeAll(): [string, T][] { - const taken: [string, T][] = [...this.entries].map(([key, entry]) => [key, entry.group]); - + clear(): void { this.entries.clear(); this.idle(); - - return taken; } /** Removes every group past its deadline and hands them over. */ diff --git a/src/held-messages.ts b/src/held-messages.ts index 3324784..0a2a665 100644 --- a/src/held-messages.ts +++ b/src/held-messages.ts @@ -20,7 +20,6 @@ function keyOf(pduObjs: PduObject[]): string | undefined { /** The messages handed to the application that it has not answered yet, held by their segments. */ export class HeldMessages { - private readonly droppedWithLink = new WeakSet(); private readonly held: ExpiringGroups; private readonly idleWaiters = new IdleWaiters(); private readonly log: SmppLog; @@ -65,11 +64,6 @@ export class HeldMessages { return key !== undefined && this.held.get(key) === pduObjs; } - /** Whether this message's link went before it was answered, so no answer of ours correlates. */ - lostLink(pduObjs: PduObject[]): boolean { - return this.droppedWithLink.has(pduObjs); - } - release(pduObjs: PduObject[]): void { const key = keyOf(pduObjs); @@ -82,10 +76,7 @@ export class HeldMessages { /** Drops every message: their segments went with the link, so no answer of ours correlates now. */ clear(): void { - for (const [, pduObjs] of this.held.takeAll()) { - this.droppedWithLink.add(pduObjs); - } - + this.held.clear(); this.idleWaiters.settle(); } diff --git a/src/incoming-requests.ts b/src/incoming-requests.ts index 0487401..748dc14 100644 --- a/src/incoming-requests.ts +++ b/src/incoming-requests.ts @@ -39,6 +39,8 @@ export class IncomingRequests { private readonly session: Session; private readonly smsIdFormat: SmsIdFormat; private readonly systemId: string; + /** Bumped when a link goes, so a message from an earlier one knows its answer cannot correlate. */ + private linkGeneration = 0; constructor(options: IncomingRequestsOptions) { this.dlrMerger = options.dlrMerger; @@ -96,6 +98,7 @@ export class IncomingRequests { /** Drops the segments of every message that never became whole, and of every one still held. */ clear(): void { + this.linkGeneration++; this.held.clear(); this.reassembler.clear(); } @@ -163,6 +166,8 @@ export class IncomingRequests { if (!first) return; + const generation = this.linkGeneration; + const sms = createSms({ from: paramText(first.params.source_addr), message: decodeSegments(pduObjs), @@ -170,7 +175,7 @@ export class IncomingRequests { session: this.session, to: paramText(first.params.destination_addr), }, { - lostLink: () => this.held.lostLink(pduObjs), + lostLink: () => this.linkGeneration !== generation, // A turn later, so a listener sending its receipt straight after the response still holds. onAnswered: () => { setImmediate(() => { this.held.release(pduObjs); }); }, // Past the refusal only while a drain is still waiting for this message; an ordinary send after. diff --git a/src/reassembly.ts b/src/reassembly.ts index 59b1634..efc5e2e 100644 --- a/src/reassembly.ts +++ b/src/reassembly.ts @@ -163,7 +163,7 @@ export class Reassembler { } clear(): void { - this.groups.takeAll(); + this.groups.clear(); this.octets = 0; } diff --git a/src/sms.ts b/src/sms.ts index 8f8f847..7a679be 100644 --- a/src/sms.ts +++ b/src/sms.ts @@ -54,7 +54,6 @@ export type SmsInput = { /** What the session's incoming side gives a message so it can be answered and accounted for. */ export type SmsHandlers = { - /** Whether the link this message arrived on is gone, so no answer of ours correlates. */ lostLink: () => boolean; onAnswered: () => void; send: (input: PduObjectInput) => Promise>; @@ -107,13 +106,13 @@ async function sendResp( return { err: new Error('smsId must not be empty') }; } + if (options.smsId !== undefined) answered.smsId = options.smsId; + // A response carries the sequence number it was asked on, which the next link knows nothing about. if (lostLink()) { return { err: new Error('The link this message arrived on is gone, so nothing would correlate the response') }; } - if (options.smsId !== undefined) answered.smsId = options.smsId; - const results = await Promise.all(sms.pduObjs.map((pduObj, index) => sms.session.sendReturn( pduObj, options.status ?? 'ESME_ROK', diff --git a/test/session-extras.test.ts b/test/session-extras.test.ts index 087deec..c8b17a7 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -557,38 +557,56 @@ describe('reconnect', () => { assert.equal(report.segments.length, 3); }); - test('refuses to answer a message whose link went, rather than reporting it delivered', async t => { + test('refuses to answer a message whose link went, held or already answered', async t => { const smpp = await startServer(t); const { session } = await connect(t, smpp, { reconnect: { maxDelay: 100, minDelay: 20 } }); assert.ok(session); - const arrived = once(resolve => { session.on('sms', resolve); }); + const arrived: Sms[] = []; + const both = once(resolve => { + session.on('sms', sms => { + arrived.push(sms); - void peerOf(smpp).send({ - cmdName: 'deliver_sm', - params: { - destination_addr: '46701113311', - short_message: 'answered too late', - source_addr: '46709771337', - }, + if (arrived.length === 2) resolve(true); + }); }); - const sms = await arrived; + for (const text of ['answered before the drop', 'never answered']) { + void peerOf(smpp).send({ + cmdName: 'deliver_sm', + params: { + destination_addr: '46701113311', + short_message: text, + source_addr: '46709771337', + }, + }); + } + + await both; + + const [answered, held] = arrived; + + assert.ok(answered); + assert.ok(held); + assert.equal((await answered.sendResp()).err, undefined); + const reconnected = once(resolve => { session.on('reconnected', () => { resolve(true); }); }); await peerOf(smpp).close({ signal: AbortSignal.abort() }); await reconnected; - const answered = await sms.sendResp(); + const taken: string[] = []; - assert.match(answered.err?.message ?? '', /link this message arrived on is gone/); + peerOf(smpp).on('incomingPduObj', pduObj => { taken.push(pduObj.cmdName); }); + + // The answered one is out of the hold and the held one is not, so neither may reach the answer. + assert.match((await answered.sendResp()).err?.message ?? '', /link this message arrived on is gone/); + assert.match((await held.sendResp()).err?.message ?? '', /link this message arrived on is gone/); // A receipt is a request of its own, correlated by its id, so the new link carries it. - const report = await sms.sendDlr('DELIVERED'); - - assert.equal(report.err, undefined); - assert.equal(report.pduObjs.length, 1); + assert.equal((await held.sendDlr('DELIVERED')).err, undefined); + assert.deepEqual(taken, ['deliver_sm'], 'a response reached the new link'); }); test('does not reconnect after an explicit close', async t => { @@ -947,20 +965,6 @@ describe('held message bounds', () => { held.clear(); }); - test('remembers which messages went with the link, and which were answered', () => { - const held = new HeldMessages({ log: silentLog, max: 10, timeout: 10_000 }); - const answered = message(1); - const dropped = message(2); - - held.hold(answered); - held.hold(dropped); - held.release(answered); - held.clear(); - - assert.equal(held.lostLink(dropped), true); - assert.equal(held.lostLink(answered), false); - }); - // 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; diff --git a/todo.md b/todo.md index b495093..e965f55 100644 --- a/todo.md +++ b/todo.md @@ -141,6 +141,11 @@ session message is a change to every call site. 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 refused `sendResp()` releases the hold anyway.** Every early return runs + `.finally(onAnswered)`, so `sendResp({ smsId: '' })` refuses and stops the drain waiting for a + message the peer was never answered, which `close()` then reports as answered. 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 e6250ef44ef33369b4f34b25da8db83b30644f52 Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Tue, 1 Sep 2026 20:40:17 +0200 Subject: [PATCH 3/4] Drop a request whose link went while onRequest ran, and pin the wire in the test --- AGENTS.md | 9 +++------ README.md | 2 +- src/incoming-requests.ts | 6 +++++- src/sms.ts | 2 +- test/session-extras.test.ts | 36 ++++++++++++++++++++++++++++++++---- 5 files changed, 42 insertions(+), 13 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 0993c65..8e083bc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -308,12 +308,9 @@ Grouped by what each one constrains. `attach()`, which is why a second drop emits again. - **An answer belongs to the link the message arrived on; a receipt does not.** Maintainer's call, - 2026-09-01: `sendResp()` carries the peer's own sequence number, which the next link knows nothing - about, so on a message whose link went it writes nothing and returns an `err`. Writing it to the - new socket instead succeeds and reports `{}` for a response that correlates with nothing, which is - goal 2's wrong answer. `sendDlr()` still goes out, because a receipt is a request of its own - correlated by `receipted_message_id`; the accepted cost is that one sent after a refused response - names an id the peer has no record of. + 2026-09-01. Rejected: answering on the new link, which succeeds and reports `{}` for a response + that correlates with nothing — goal 2's wrong answer. Accepted: a receipt sent after a refused + response names an id the peer has no record of. - **`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 diff --git a/README.md b/README.md index 7e48f98..5c10c48 100644 --- a/README.md +++ b/README.md @@ -316,7 +316,7 @@ TypeScript users can import `SmppLog` to have the compiler check one. | Event | Fires when | | --- | --- | -| `sms` | An SMS arrives, reassembled if it was multipart. Carries `sendResp()`, `sendDlr()` and the `smsId` it was answered with. | +| `sms` | An SMS arrives, reassembled if it was multipart. Carries `sendResp()`, `sendDlr()` and its `smsId`. | | `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. | diff --git a/src/incoming-requests.ts b/src/incoming-requests.ts index 748dc14..621ed9d 100644 --- a/src/incoming-requests.ts +++ b/src/incoming-requests.ts @@ -39,7 +39,6 @@ export class IncomingRequests { private readonly session: Session; private readonly smsIdFormat: SmsIdFormat; private readonly systemId: string; - /** Bumped when a link goes, so a message from an earlier one knows its answer cannot correlate. */ private linkGeneration = 0; constructor(options: IncomingRequestsOptions) { @@ -64,8 +63,13 @@ export class IncomingRequests { } async handle(pduObj: PduObject): Promise { + const generation = this.linkGeneration; + if (this.onRequest && await this.onRequest(this.session, pduObj)) return; + // The link it arrived on went while the hook ran, so nothing we answer now correlates. + if (this.linkGeneration !== generation) return; + if (!this.session.bindAllows(pduObj.cmdName)) { this.log.info('session - command the peer\'s bind direction does not carry', { bindType: this.session.boundAs ?? '', diff --git a/src/sms.ts b/src/sms.ts index 7a679be..8b74b1d 100644 --- a/src/sms.ts +++ b/src/sms.ts @@ -38,7 +38,7 @@ export type Sms = { /** Answers every segment. Part of the protocol, not optional. Defaults to ESME_ROK. */ sendResp: (options?: SendRespOptions) => Promise; session: Session; - /** The id answered to the peer: what sendResp() was given, or a generated UUID v7. */ + /** The id `sendResp()` was given, or a generated UUID v7. */ readonly smsId: string; submitTime: Date; to: string; diff --git a/test/session-extras.test.ts b/test/session-extras.test.ts index c8b17a7..6a7ca54 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -13,6 +13,7 @@ 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 { IncomingRequests } from '../src/incoming-requests.ts'; import { UnansweredError } from '../src/unanswered-error.ts'; import { createSms } from '../src/sms.ts'; import { LinkGate } from '../src/link-gate.ts'; @@ -589,6 +590,7 @@ describe('reconnect', () => { assert.ok(answered); assert.ok(held); + assert.equal(answered.message, 'answered before the drop'); assert.equal((await answered.sendResp()).err, undefined); const reconnected = once(resolve => { session.on('reconnected', () => { resolve(true); }); }); @@ -596,17 +598,43 @@ describe('reconnect', () => { await peerOf(smpp).close({ signal: AbortSignal.abort() }); await reconnected; - const taken: string[] = []; + let taken = 0; - peerOf(smpp).on('incomingPduObj', pduObj => { taken.push(pduObj.cmdName); }); + // A response is dispatched before `incomingPduObj`, so only the raw event sees one arrive. + peerOf(smpp).on('incomingPdu', () => { taken++; }); // The answered one is out of the hold and the held one is not, so neither may reach the answer. assert.match((await answered.sendResp()).err?.message ?? '', /link this message arrived on is gone/); assert.match((await held.sendResp()).err?.message ?? '', /link this message arrived on is gone/); - // A receipt is a request of its own, correlated by its id, so the new link carries it. assert.equal((await held.sendDlr('DELIVERED')).err, undefined); - assert.deepEqual(taken, ['deliver_sm'], 'a response reached the new link'); + assert.equal(taken, 1, 'a refused response reached the new link'); + }); + + test('drops a message whose link went while onRequest was still running', async t => { + const session = new Session({ sock: new net.Socket() }); + + closeAfter(t, session); + session.boundAs = 'transceiver'; + + const incoming = new IncomingRequests({ + dlrMerger: new DlrMerger({ log: silentLog, max: 10, timeout: 10_000 }), + log: silentLog, + onRequest: async () => { await delay(10); return false; }, + sendPastDrain: () => Promise.resolve({ err: new Error('never sent') }), + session, + }); + let messages = 0; + + session.on('sms', () => { messages++; }); + + const handled = incoming.handle(submitPdu(1)); + + incoming.clear(); + + await handled; + + assert.equal(messages, 0); }); test('does not reconnect after an explicit close', async t => { From 471923a7b6edf46c2a70a1f0115850d187e5e3f7 Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Tue, 1 Sep 2026 20:53:58 +0200 Subject: [PATCH 4/4] Log the dropped request, and scope the receipt sentence to a reconnect --- README.md | 8 +++++--- src/incoming-requests.ts | 6 +++++- test/session-extras.test.ts | 5 ++++- 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 5c10c48..48108aa 100644 --- a/README.md +++ b/README.md @@ -356,8 +356,9 @@ under it, the peer never answered in time, or you aborted it after it went out. where a drop ends the session and every send after it is refused. An answer cannot wait for a link that way, because it carries the sequence number the message arrived -on: `sms.sendResp()` on a message whose link dropped writes nothing and returns an `err`. Its -`sms.sendDlr()` still goes out, since a receipt is a request of its own, correlated by the id it names. +on: `sms.sendResp()` on a message whose link dropped writes nothing and returns an `err`. Where a +reconnect follows, its `sms.sendDlr()` still goes out on the new link, since a receipt is a request +of its own, correlated by the id it names. `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. @@ -398,7 +399,8 @@ The spec tables are exported both individually (`cmds`, `consts`, `encodings`, ` - **`server()` resolves once, when it is listening**, and gives you a handle with `close()`, `port` and a `session` event. It no longer calls your callback once per incoming connection. - **The id a message is answered with goes to `sendResp({ smsId })`**, and `sms.smsId` is read-only: - it reports the id `sendResp()` was given, or the UUID v7 generated instead. Delete any `sms.smsId = …` line — assigning to it + it reports the id `sendResp()` was given, or the UUID v7 generated instead. Delete any + `sms.smsId = …` line — assigning to it throws a `TypeError`, since modules are always strict mode — and pass the id to `sendResp()`. - **`checkuserpass` is now `authenticate`**, takes `{ password, session, systemId, systemType }` and returns `false` or `{ userData }`. diff --git a/src/incoming-requests.ts b/src/incoming-requests.ts index 621ed9d..547015a 100644 --- a/src/incoming-requests.ts +++ b/src/incoming-requests.ts @@ -68,7 +68,11 @@ export class IncomingRequests { if (this.onRequest && await this.onRequest(this.session, pduObj)) return; // The link it arrived on went while the hook ran, so nothing we answer now correlates. - if (this.linkGeneration !== generation) return; + if (this.linkGeneration !== generation) { + this.log.info('session - dropping a request whose link went', { cmdName: pduObj.cmdName }); + + return; + } if (!this.session.bindAllows(pduObj.cmdName)) { this.log.info('session - command the peer\'s bind direction does not carry', { diff --git a/test/session-extras.test.ts b/test/session-extras.test.ts index 6a7ca54..1608de2 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -603,7 +603,6 @@ describe('reconnect', () => { // A response is dispatched before `incomingPduObj`, so only the raw event sees one arrive. peerOf(smpp).on('incomingPdu', () => { taken++; }); - // The answered one is out of the hold and the held one is not, so neither may reach the answer. assert.match((await answered.sendResp()).err?.message ?? '', /link this message arrived on is gone/); assert.match((await held.sendResp()).err?.message ?? '', /link this message arrived on is gone/); @@ -635,6 +634,10 @@ describe('reconnect', () => { await handled; assert.equal(messages, 0); + + await incoming.handle(submitPdu(2)); + + assert.equal(messages, 1, 'the harness delivers a message whose link stayed'); }); test('does not reconnect after an explicit close', async t => {