Merge a message id base once so a straggler cannot join a later report

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 `<base>-<n>`, 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

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 `<base>-<n>`, 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 `<base>-<n>`, 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. |

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 `<base>-<n>` 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<MessageState, number> = {
 	UNDELIVERABLE: 9,
 };
 
+/**
+ * Merges the per-segment receipts of a multipart message into one report, but only when the peer
+ * numbered its ids `<base>-<n>` 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<Group>;
 	private readonly log: SmppLog;
 	private readonly max: number;
+	private readonly spent: ExpiringGroups<true>;
 
 	constructor(options: DlrMergerOptions) {
 		this.groups = new ExpiringGroups<Group>({
@@ -56,6 +59,12 @@ export class DlrMerger {
 		});
 		this.log = options.log;
 		this.max = options.max;
+		this.spent = new ExpiringGroups<true>({
+			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 {
+	/** Ends the base: whatever it still held goes, and it is remembered so nothing merges under it again. */
-		if (!this.groups.takeOldest()) return;
+	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 });
 	}
 }

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');
 	});
 });
 

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.