From aa3cf41c7fcff1ac8c2710a90c7dbcf442460dc5 Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Sun, 30 Aug 2026 11:17:02 +0200 Subject: [PATCH] Merge a message id base once so a straggler cannot join a later report --- AGENTS.md | 8 +++++++ README.md | 2 +- src/dlr-merger.ts | 50 +++++++++++++++++++++++++++++++++++++------- test/session.test.ts | 35 +++++++++++++++++++++++++++++++ todo.md | 7 ------- 5 files changed, 86 insertions(+), 16 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 0b553f7..1b780e9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -243,6 +243,14 @@ exactly 140. 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` rather than + the earlier one's receipts folded into its report. 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 `node:24.18.0-bookworm-slim` ships no openssl binary, so a shelled-out fixture would pass in CI diff --git a/README.md b/README.md index 44304ff..ccc3386 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. | +| `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. | | `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 c8d3d3e..93f0a0f 100644 --- a/src/dlr-merger.ts +++ b/src/dlr-merger.ts @@ -20,11 +20,6 @@ type Group = { const numbered = /^(.*)-(\d+)$/; -/** - * 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. - */ /** * MESSAGE_STATE is a flat enum, not a ranking — ACCEPTED is 6 where UNDELIVERABLE is 5 — so reducing * on the wire value reports a part-failed message as delivered. Rank it deliberately instead. @@ -42,10 +37,18 @@ const severity: Record = { UNDELIVERABLE: 9, }; +/** + * 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. + */ export class DlrMerger { private readonly groups: ExpiringGroups; private readonly log: SmppLog; private readonly max: number; + private readonly spent: ExpiringGroups; constructor(options: DlrMergerOptions) { this.groups = new ExpiringGroups({ @@ -56,6 +59,12 @@ export class DlrMerger { }); this.log = options.log; this.max = options.max; + this.spent = new ExpiringGroups({ + max: options.max, + now: options.now, + onSweep: () => { this.sweep(); }, + timeout: options.timeout, + }); } get size(): number { @@ -103,7 +112,7 @@ export class DlrMerger { if (group.parts.size < group.expected) return undefined; - this.groups.delete(base); + this.close(base); const segments = [...group.parts.entries()].sort(([a], [b]) => a - b).map(([, one]) => one); const worst = segments.reduce((carry, one) => (severity[one.statusMsg] > severity[carry.statusMsg] ? one : carry)); @@ -113,24 +122,49 @@ export class DlrMerger { clear(): void { this.groups.clear(); + this.spent.clear(); } /** Drops every group past its deadline. Runs before each collect and on its own timer. */ sweep(): void { for (const [base, group] of this.groups.takeExpired()) { + this.close(base); this.log.info('dlrMerger - incomplete receipts expired', { base, expected: group.expected }); } + + this.spent.takeExpired(); } private open(base: string, expected: number): void { + 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 }); + + return; + } + if (this.groups.full) this.dropOldest(); this.groups.set(base, { expected, parts: new Map() }); } - private dropOldest(): void { - if (!this.groups.takeOldest()) return; + /** 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); + if (this.spent.full && this.spent.get(base) === undefined) this.spent.takeOldest(); + + this.spent.set(base, true); + } + + private dropOldest(): void { + const oldest = this.groups.takeOldest(); + + if (!oldest) return; + + const [base] = oldest; + + this.close(base); this.log.warn('dlrMerger - buffer full, dropping the oldest message', { max: this.max }); } } diff --git a/test/session.test.ts b/test/session.test.ts index c09c157..16c06ed 100644 --- a/test/session.test.ts +++ b/test/session.test.ts @@ -1576,6 +1576,41 @@ describe('merged delivery report bounds', () => { assert.equal(dlrMerger.collect(receipt('late-1')), undefined); assert.equal(dlrMerger.size, 0); + + dlrMerger.expect(['late-1', 'late-2']); + + assert.equal(dlrMerger.collect(receipt('late-1')), undefined); + assert.equal(dlrMerger.collect(receipt('late-2')), undefined); + }); + + test('leaves a base the peer hands out twice unmerged', () => { + const dlrMerger = merger(); + + dlrMerger.expect(['reused-1', 'reused-2']); + + assert.equal(dlrMerger.collect(receipt('reused-1')), undefined); + assert.ok(dlrMerger.collect(receipt('reused-2'))); + + dlrMerger.expect(['reused-1', 'reused-2']); + + assert.equal(dlrMerger.size, 0); + assert.equal(dlrMerger.collect(receipt('reused-1')), undefined); + assert.equal(dlrMerger.collect(receipt('reused-2')), undefined); + }); + + test('keeps another message when a held base is opened again', () => { + const dlrMerger = merger({ max: 2 }); + + dlrMerger.expect(['first-1', 'first-2']); + dlrMerger.expect(['second-1', 'second-2']); + dlrMerger.expect(['second-1', 'second-2']); + + assert.equal(dlrMerger.collect(receipt('first-1')), undefined); + + const merged = dlrMerger.collect(receipt('first-2')); + + assert.ok(merged); + assert.equal(merged.smsId, 'first'); }); }); diff --git a/todo.md b/todo.md index f746048..e734eaa 100644 --- a/todo.md +++ b/todo.md @@ -143,13 +143,6 @@ session message is a change to every call site. closes it on its last line, so an assertion that throws leaves the listener open and `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. -- [ ] **`DlrMerger.open()` evicts a live group when the base is already held.** `groups.set()` - overwrites without growing the store, so the `dropOldest()` before it discarded another - message's receipts for nothing, and logged the eviction. Guard it on `groups.get(base)`. -- [ ] **A peer that reuses message ids folds one message's receipts into another's report.** - `collect()` keys on the base alone, so a straggler for a group that is gone joins the next group - opened under the same base — reporting a fully delivered message as UNDELIVERABLE. An SMSC whose - id counter restarts with its process is the realistic case. Found by review, 2026-08-28. - [ ] **`submit_multi` and the broadcast commands** encode and decode, but nothing exercises them end to end. The interop suite is the natural place.