From 5c797f5383b660ecfa77654749ced6692de4a1b4 Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Sun, 30 Aug 2026 11:31:22 +0200 Subject: [PATCH] Forget the base closed longest ago rather than the one handed out again --- AGENTS.md | 6 +++--- README.md | 2 +- src/dlr-merger.ts | 17 ++++++++--------- test/session.test.ts | 25 +++++++++++++++++++++++++ todo.md | 5 +++++ 5 files changed, 42 insertions(+), 13 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 1b780e9..bfba8a0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -247,9 +247,9 @@ exactly 140. 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` rather than - the earlier one's receipts folded into its report. Every segment still reaches the application as - a `dlr`. + groups, and refuses to open one a second time. Neither message merges: the later one 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 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 diff --git a/README.md b/README.md index ccc3386..0329d0c 100644 --- a/README.md +++ b/README.md @@ -297,7 +297,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. | +| `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). | | `sessionError` | Something failed on a live session, including a hook or listener that threw or, if it was `async`, rejected. | diff --git a/src/dlr-merger.ts b/src/dlr-merger.ts index 93f0a0f..71d30a6 100644 --- a/src/dlr-merger.ts +++ b/src/dlr-merger.ts @@ -41,8 +41,7 @@ const severity: Record = { * Merges the per-segment receipts of a multipart message into one report, but only when the peer * numbered its ids `-` off one base — the convention this library's own server follows. An * SMSC that hands out unrelated ids per segment cannot be merged, so nothing is reported for it. - * A base is merged at most once: a receipt under an id the peer has handed out before cannot be - * told from a straggler for the message that held it first. + * A base is merged at most once: a reused id cannot be told apart from a straggler. */ export class DlrMerger { private readonly groups: ExpiringGroups; @@ -62,7 +61,7 @@ export class DlrMerger { this.spent = new ExpiringGroups({ max: options.max, now: options.now, - onSweep: () => { this.sweep(); }, + onSweep: () => { this.spent.takeExpired(); }, timeout: options.timeout, }); } @@ -131,14 +130,14 @@ export class DlrMerger { this.close(base); this.log.info('dlrMerger - incomplete receipts expired', { base, expected: group.expected }); } - - this.spent.takeExpired(); } private open(base: string, expected: number): void { + this.spent.takeExpired(); + if (this.groups.get(base) !== undefined || this.spent.get(base) === true) { this.close(base); - this.log.warn('dlrMerger - message id handed out again, leaving its receipts unmerged', { base }); + this.log.info('dlrMerger - message id handed out again, leaving its receipts unmerged', { base }); return; } @@ -148,11 +147,11 @@ export class DlrMerger { this.groups.set(base, { expected, parts: new Map() }); } - /** Ends the base: whatever it still held goes, and it is remembered so nothing merges under it again. */ private close(base: string): void { this.groups.delete(base); + this.spent.delete(base); - if (this.spent.full && this.spent.get(base) === undefined) this.spent.takeOldest(); + if (this.spent.full) this.spent.takeOldest(); this.spent.set(base, true); } @@ -165,6 +164,6 @@ export class DlrMerger { const [base] = oldest; this.close(base); - this.log.warn('dlrMerger - buffer full, dropping the oldest message', { max: this.max }); + this.log.warn('dlrMerger - buffer full, dropping the oldest message', { base, max: this.max }); } } diff --git a/test/session.test.ts b/test/session.test.ts index 16c06ed..a3dc4fe 100644 --- a/test/session.test.ts +++ b/test/session.test.ts @@ -1563,6 +1563,11 @@ describe('merged delivery report bounds', () => { assert.equal(dlrMerger.collect(receipt('first-1')), undefined); assert.equal(dlrMerger.collect(receipt('first-2')), undefined); + dlrMerger.expect(['first-1', 'first-2']); + + assert.equal(dlrMerger.collect(receipt('first-1')), undefined); + assert.equal(dlrMerger.collect(receipt('first-2')), undefined); + dlrMerger.clear(); assert.equal(dlrMerger.size, 0); }); @@ -1598,6 +1603,26 @@ describe('merged delivery report bounds', () => { assert.equal(dlrMerger.collect(receipt('reused-2')), undefined); }); + test('forgets the base closed longest ago, not the one handed out again', () => { + const dlrMerger = merger({ max: 2 }); + + for (const base of ['reused', 'other']) { + dlrMerger.expect([`${base}-1`, `${base}-2`]); + assert.equal(dlrMerger.collect(receipt(`${base}-1`)), undefined); + assert.ok(dlrMerger.collect(receipt(`${base}-2`))); + } + + dlrMerger.expect(['reused-1', 'reused-2']); + dlrMerger.expect(['third-1', 'third-2']); + + assert.equal(dlrMerger.collect(receipt('third-1')), undefined); + assert.ok(dlrMerger.collect(receipt('third-2'))); + + dlrMerger.expect(['reused-1', 'reused-2']); + + assert.equal(dlrMerger.size, 0); + }); + test('keeps another message when a held base is opened again', () => { const dlrMerger = merger({ max: 2 }); diff --git a/todo.md b/todo.md index e734eaa..aa03081 100644 --- a/todo.md +++ b/todo.md @@ -144,6 +144,11 @@ session message is a change to every call site. `node --test` never exits: all four CI legs burn the ten-minute cap instead of reporting the five-second failure. `t.after(() => smpp.close())` fixes it, at every call site. +- [ ] **A peer whose message ids share one base logs a refused merge on every send.** `smsc01-000123` + and `smsc01-000124` carry the same base, so `DlrMerger` merges the first message and refuses + every one after it, one log line per send. Left at `info` — nothing the operator can fix is + wrong — but a rate guard or silence may suit it better. Raised by review, 2026-08-30. + - [ ] **`submit_multi` and the broadcast commands** encode and decode, but nothing exercises them end to end. The interop suite is the natural place. - [ ] **Move to TypeScript 7** once `typescript-eslint` supports it; `renovate.json` pins TypeScript