diff --git a/src/dlr-merger.ts b/src/dlr-merger.ts index 00b1924..4dfd9b7 100644 --- a/src/dlr-merger.ts +++ b/src/dlr-merger.ts @@ -14,7 +14,7 @@ export type DlrMergerOptions = { }; type Group = { - expected: number; + expected: Set; parts: Map; }; @@ -37,6 +37,30 @@ const severity: Record = { UNDELIVERABLE: 9, }; +/** The base and the part numbers one send's ids carry, or nothing when they do not spell out one message. */ +function idNumbering(smsIds: string[]): { base: string; parts: Set } | undefined { + const bases = new Set(); + const parts = new Set(); + + for (const smsId of smsIds) { + const match = numbered.exec(smsId); + const base = match?.[1]; + const part = match?.[2]; + + if (base === undefined || part === undefined) return undefined; + + bases.add(base); + parts.add(Number(part)); + } + + const [base] = bases; + + // A repeated id leaves a part no receipt can fill, so the group would complete on a short set. + if (!base || bases.size !== 1 || parts.size !== smsIds.length) return undefined; + + return { base, parts }; +} + /** * 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 @@ -75,21 +99,9 @@ export class DlrMerger { expect(smsIds: string[]): void { if (smsIds.length < 2) return; - const bases = new Set(); + const numbering = idNumbering(smsIds); - for (const smsId of smsIds) { - const base = numbered.exec(smsId)?.[1]; - - if (base === undefined || base === '') return; - - bases.add(base); - } - - if (bases.size !== 1) return; - - for (const base of bases) { - this.open(base, smsIds.length); - } + if (numbering) this.open(numbering.base, numbering.parts); } /** The whole message's report, on the receipt that completes it. */ @@ -108,9 +120,13 @@ export class DlrMerger { if (!group) return undefined; - group.parts.set(Number(part), dlr); + const number = Number(part); - if (group.parts.size < group.expected) return undefined; + if (!group.expected.has(number)) return undefined; + + group.parts.set(number, dlr); + + if (group.parts.size < group.expected.size) return undefined; this.close(base); @@ -129,11 +145,11 @@ export class DlrMerger { 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.log.info('dlrMerger - incomplete receipts expired', { base, expected: group.expected.size }); } } - private open(base: string, expected: number): void { + private open(base: string, expected: Set): void { this.spent.takeExpired(); if (this.groups.get(base) !== undefined || this.spent.get(base) === true) { diff --git a/test/session.test.ts b/test/session.test.ts index 878fb00..2b7f7d8 100644 --- a/test/session.test.ts +++ b/test/session.test.ts @@ -1548,6 +1548,22 @@ describe('merged delivery report bounds', () => { assert.equal(dlrMerger.size, 0); }); + // A receipt for whole-3 would otherwise fill the slot whole-2 was registered for, truncating the report. + test('ignores a receipt for a part the send never registered', () => { + const dlrMerger = merger(); + + dlrMerger.expect(['whole-1', 'whole-2']); + + assert.equal(dlrMerger.collect(receipt('whole-1')), undefined); + assert.equal(dlrMerger.collect(receipt('whole-3')), undefined); + assert.equal(dlrMerger.size, 1); + + const merged = dlrMerger.collect(receipt('whole-2')); + + assert.ok(merged); + assert.deepEqual(merged.segments.map(one => one.smsId), ['whole-1', 'whole-2']); + }); + // Every multipart send registered a group, and only a complete set of receipts ever removed it. test('drops the oldest group once the cap is reached', () => { const dlrMerger = merger({ max: 2 });