diff --git a/AGENTS.md b/AGENTS.md index 9265dcd..3fdc99a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -18,16 +18,19 @@ one wins. They do not override the hard rules below. 1. **Correct on the wire.** SMPP 3.4 as SMSCs actually run it. Every other goal yields to this one; the defect table below is what the alternative costs. 2. **Never give the application a wrong answer about what happened.** An outcome we cannot determine - is reported as undetermined rather than guessed; a request the peer may already have taken is - never re-sent on the library's own initiative; work the peer has no reason to send again is not - dropped. + is reported as undetermined rather than guessed; a report the peer marked as not final settles + nothing, so nothing the library concludes may rest on one; a request the peer may already have + taken is never re-sent on the library's own initiative; work the peer has no reason to send again + is not dropped. 3. **Strict in what we send, generous in what we read.** The library's own senders follow 3.4, and the codec parses whatever arrives. Where the letter of the spec would discard traffic a real SMSC sends, keep the traffic. 4. **A peer an operator never has to complain about.** No bind flooding, nothing a bind direction forbids, no optional parameters to a peer that declared none, nothing held without a bound. 5. **The session layer is in here, and its defaults are what most applications should run.** - Keepalive, reconnect, the send window, reassembly and receipt correlation. An option retunes a + Keepalive, reconnect, the send window, reassembly and receipt correlation. What the network says + about a message the application sent reaches it as a report rather than as an inbound message, and + says whether it is final, so nothing has to read the PDU to tell those apart. An option retunes a default or opts out of it; an option does not switch on the thing the caller obviously wanted. 6. **A small, stable public surface over reshapeable internals.** Only what `src/index.ts` exports is published. A new option has to beat "the application can do this itself", and has to keep a @@ -273,12 +276,15 @@ Grouped by what each one constrains. and sent no optional parameters, which is how the spec reads an absent `sc_interface_version`. - **`esm_class` decides what a `deliver_sm` is, and the body is read only when it names nothing.** - `MC_DELIVERY_RECEIPT` (0x04) makes it a receipt whatever the body parses to, so a receipt in a - format `dlrFromPdu()` cannot read reaches `dlr` with `smsId` undefined instead of arriving as an - inbound SMS. Any other named type is not a receipt and its body is not scraped; a message type of 0 - or one of the ten reserved keeps the scrape, and a non-empty `receipted_message_id` TLV marks a - receipt on the same footing. A receipt this library recognises never reaches the reassembler, so an - SMSC that splits one across segments gets a `dlr` per segment rather than one merged report. The + The two types the MC writes about a message we submitted — `MC_DELIVERY_RECEIPT` (0x04) and + `INTERMEDIATE_DELIVERY` (0x20) — are reports whatever the body parses to, so one in a format + `dlrFromPdu()` cannot read reaches `dlr` with `smsId` undefined instead of arriving as an inbound + SMS. The three the far-end SME writes (0x08, 0x10, 0x18) are messages and their bodies are not + scraped: Kannel reads 0x08 as report-bearing and this does not, because a delivery acknowledgement + is the handset's word about a message, not the network's. A message type of 0 or one of the ten + reserved keeps the scrape, and a non-empty `receipted_message_id` TLV marks a report on the same + footing. A report this library recognises never reaches the reassembler, so an SMSC that splits one + across segments gets a `dlr` per segment rather than one merged report. The `message_state` TLV is authoritative only where it names a state in the table — SMPP reserves 0x80-0xFF for MC-vendor-specific values, so an unnameable one keeps its raw `statusId` and leaves `statusMsg` to the body. diff --git a/README.md b/README.md index 0c13ad8..be8e4f6 100644 --- a/README.md +++ b/README.md @@ -163,7 +163,9 @@ session.on('sms', async sms => { Delivery receipts travel on the same SMPP command but reach you as `dlr`, so nothing you write has to tell the two apart. `esm_class` is what tells them apart; where it names no message type a `receipted_message_id` TLV does, and failing both the message body is read for the standard -`id:` and `stat:` receipt fields. +`id:` and `stat:` receipt fields. An intermediate delivery notification is the SMSC reporting too, +and arrives with `dlr.intermediate` true: nothing about the message is settled, and a later receipt +says how it ended. Matching a receipt to a send means comparing `dlr.smsId` against the `smsIds` that `sendSms()` returned. Some SMSCs write the two in different notations — a hex `message_id` on the @@ -317,8 +319,8 @@ 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 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. | +| `dlr` | A delivery report arrives, one per segment. `intermediate` is true where the SMSC marked it a report it will follow with a final receipt. `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. An intermediate report never counts towards it. 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. | | `disconnected` | The link dropped and the reconnect loop will retry it. Do not open a replacement client here — the session you hold comes back on its own, and `reconnected` says when. Fires again for each attempt that reconnects and then fails, so it is not one-to-one with `reconnected`. | | `reconnected` | The client re-bound after a drop. | diff --git a/src/dlr-merger.ts b/src/dlr-merger.ts index 71d30a6..00b1924 100644 --- a/src/dlr-merger.ts +++ b/src/dlr-merger.ts @@ -41,7 +41,8 @@ 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 reused id cannot be told apart from a straggler. + * A base is merged at most once: a reused id cannot be told apart from a straggler, and a report the + * peer marked intermediate is never counted — it would fill a slot before the real receipt arrives. */ export class DlrMerger { private readonly groups: ExpiringGroups; @@ -95,7 +96,7 @@ export class DlrMerger { collect(dlr: Dlr): MessageDlr | undefined { this.sweep(); - if (dlr.smsId === undefined) return undefined; + if (dlr.intermediate || dlr.smsId === undefined) return undefined; const match = numbered.exec(dlr.smsId); const base = match?.[1]; diff --git a/src/dlr.ts b/src/dlr.ts index 6b90bef..b891c87 100644 --- a/src/dlr.ts +++ b/src/dlr.ts @@ -50,6 +50,7 @@ export type Receipt = { export type Dlr = { doneDate: Date | undefined; errorCode: string | undefined; + intermediate: boolean; receipt: Receipt | undefined; smsId: string | undefined; statusId: number; @@ -127,13 +128,12 @@ export function parseReceipt(message: string): Receipt { }; } -type MessageType = 'other' | 'receipt' | 'unmarked'; +type MessageType = 'intermediate' | 'other' | 'receipt' | 'unmarked'; -/** The message types the spec names that are not receipts. It reserves the remaining ten. */ -const notReceiptTypes: number[] = [ +/** The types the far-end SME writes, rather than the MC reporting. The spec reserves the remaining ten. */ +const smeMessageTypes: number[] = [ consts.ESM_CLASS.CONVERSATION_ABORT, consts.ESM_CLASS.DELIVERY_ACKNOWLEDGEMENT, - consts.ESM_CLASS.INTERMEDIATE_DELIVERY, consts.ESM_CLASS.USER_ACKNOWLEDGEMENT, ]; @@ -145,7 +145,8 @@ function messageType(pduObj: PduObject): MessageType { const type = messageTypeOf(paramNumber(pduObj.params.esm_class, 0)); if (type === consts.ESM_CLASS.MC_DELIVERY_RECEIPT) return 'receipt'; - if (notReceiptTypes.includes(type)) return 'other'; + if (type === consts.ESM_CLASS.INTERMEDIATE_DELIVERY) return 'intermediate'; + if (smeMessageTypes.includes(type)) return 'other'; return nonEmptyText(pduObj.tlvs.receipted_message_id?.tagValue) === undefined ? 'unmarked' : 'receipt'; } @@ -214,6 +215,7 @@ export function dlrFromPdu(pduObj: PduObject, format: SmsIdFormat = {}): Dlr | u return { doneDate: receiptDate(receipt?.doneDate), errorCode: receipt?.err, + intermediate: type === 'intermediate', receipt, smsId, statusId, diff --git a/test/dlr.test.ts b/test/dlr.test.ts index 8aef21e..9c0d03d 100644 --- a/test/dlr.test.ts +++ b/test/dlr.test.ts @@ -145,6 +145,7 @@ describe('dlrFromPdu()', () => { assert.equal(dlr.smsId, undefined); assert.equal(dlr.statusMsg, 'UNKNOWN'); assert.equal(dlr.statusId, 7); + assert.equal(dlr.intermediate, false); }); // pduToObj leaves a UDH-carrying short_message a buffer, so the body needs decoding before it @@ -196,17 +197,31 @@ describe('dlrFromPdu()', () => { assert.equal(dlr.statusMsg, 'DELIVERED'); }); - test('leaves a message the peer marked as another type to arrive as an SMS', () => { + test('leaves a message the far-end SME marked as another type to arrive as an SMS', () => { for (const esmClass of [ consts.ESM_CLASS.CONVERSATION_ABORT, consts.ESM_CLASS.DELIVERY_ACKNOWLEDGEMENT, - consts.ESM_CLASS.INTERMEDIATE_DELIVERY, consts.ESM_CLASS.USER_ACKNOWLEDGEMENT, ]) { assert.equal(dlrFromPdu(deliverSm(receiptText, undefined, esmClass)), undefined); } }); + test('reads an intermediate delivery notification as a report, marked as one', () => { + const enroute = 'id:0195f0c7 sub:001 dlvrd:000 submit date:2508251430 done date:2508251431 stat:ENROUTE err:000 text:'; + const dlr = dlrFromPdu(deliverSm(enroute, undefined, consts.ESM_CLASS.INTERMEDIATE_DELIVERY)); + + assert.ok(dlr); + assert.equal(dlr.smsId, '0195f0c7'); + assert.equal(dlr.statusMsg, 'ENROUTE'); + assert.equal(dlr.intermediate, true); + + const unreadable = dlrFromPdu(deliverSm('no fields here', undefined, consts.ESM_CLASS.INTERMEDIATE_DELIVERY)); + + assert.ok(unreadable, 'the marker makes it a report whatever the body parses to'); + assert.equal(unreadable.intermediate, true); + }); + test('exposes the raw receipt alongside the resolved fields', () => { const dlr = dlrFromPdu(deliverSm(receiptText)); diff --git a/test/session-extras.test.ts b/test/session-extras.test.ts index e3e259f..35f146e 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -203,10 +203,11 @@ describe('merged delivery reports', () => { }); describe('merging segment statuses', () => { - function receipt(smsId: string, statusMsg: MessageState): Dlr { + function receipt(smsId: string, statusMsg: MessageState, intermediate = false): Dlr { return { doneDate: undefined, errorCode: undefined, + intermediate, receipt: undefined, smsId, statusId: consts.MESSAGE_STATE[statusMsg], @@ -214,6 +215,24 @@ describe('merging segment statuses', () => { }; } + // Filling every slot with a non-final report merges early and spends the base, so the receipts + // that say what actually happened would report nothing. + test('never counts an intermediate report toward a merge', () => { + const merger = new DlrMerger({ log: silentLog, max: 10, now: () => 0, timeout: 60_000 }); + + merger.expect(['msg-1', 'msg-2']); + + assert.equal(merger.collect(receipt('msg-1', 'ENROUTE', true)), undefined); + assert.equal(merger.collect(receipt('msg-2', 'ENROUTE', true)), undefined); + assert.equal(merger.collect(receipt('msg-1', 'DELIVERED')), undefined); + + const merged = merger.collect(receipt('msg-2', 'DELIVERED')); + + assert.ok(merged); + assert.equal(merged.statusMsg, 'DELIVERED'); + assert.equal(merged.segments.length, 2); + }); + // MESSAGE_STATE is a flat enum: ACCEPTED is 6 where UNDELIVERABLE is 5, so reducing on the // wire value called a part-failed message delivered. test('reports the worse of two states the wire numbers the other way round', () => { diff --git a/test/session.test.ts b/test/session.test.ts index d7a23d3..878fb00 100644 --- a/test/session.test.ts +++ b/test/session.test.ts @@ -685,7 +685,7 @@ describe('receiving', () => { assert.equal(answered.pduObj.params.message_id, 'inbound-id'); }); - test('hands a client a receipt it cannot read as a dlr rather than as an sms', async t => { + test('hands a client a report as a dlr rather than as an sms', async t => { const { peer, session } = await inbound(t); const reported = once(resolve => { session.on('dlr', resolve); }); let messages = 0; @@ -711,6 +711,28 @@ describe('receiving', () => { assert.ok(answered.pduObj); assert.equal(answered.pduObj.cmdName, 'deliver_sm_resp'); + + const notified = once(resolve => { session.on('dlr', resolve); }); + const notification = peer.send({ + cmdName: 'deliver_sm', + params: { + destination_addr: '46709771337', + esm_class: consts.ESM_CLASS.INTERMEDIATE_DELIVERY, + short_message: 'id:0195f0c7 stat:ENROUTE err:000 text:', + source_addr: '46701113311', + }, + }); + const report = await raceWithin(2000, notified); + + assert.ok(report, 'an intermediate notification is the MC reporting on our send, not an inbound SMS'); + assert.equal(report.intermediate, true); + assert.equal(report.statusMsg, 'ENROUTE'); + assert.equal(messages, 0); + + const answeredNotification = await notification; + + assert.ok(answeredNotification.pduObj); + assert.equal(answeredNotification.pduObj.cmdName, 'deliver_sm_resp'); }); test('reassembles a multipart inbound SMS before the sms event', async t => { @@ -1494,6 +1516,7 @@ describe('merged delivery report bounds', () => { return { doneDate: undefined, errorCode: undefined, + intermediate: false, receipt: undefined, smsId, statusId: 2, diff --git a/todo.md b/todo.md index 9860410..f36977f 100644 --- a/todo.md +++ b/todo.md @@ -64,6 +64,7 @@ Rules the API follows: | The hold released exactly when the peer was answered: a refused `sendResp()` keeps it, a listener that rejected drops it | `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` | +| An intermediate delivery notification read as a report marked `intermediate`, and never counted into a merge | `test/dlr.test.ts`, `test/session.test.ts`, `test/session-extras.test.ts` | | A listener that throws, or rejects, reaching `sessionError`/`serverError` rather than the process | `test/session.test.ts`, `test/error-from.test.ts` | | Cross-checked against node-smpp both ways and over a live session | `test/interop.test.ts` | | CI on Node 18/20/22/24, Renovate, tag-triggered publish | `.github/workflows/` | @@ -131,13 +132,6 @@ session message is a change to every call site. and applies it to the other is wrong. A budget type both take would close it. 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 - SMS. Kannel treats 0x04, 0x08 and 0x20 alike as report-bearing. Against it: a non-final report - would take a segment's slot in `DlrMerger` and complete the group early. Raised by review, - 2026-08-27; needs a decision. - - [ ] **`once()` is copied into four test files, and two copies never give up.** `session-extras.test.ts` and `readme.test.ts` reject after 5000 ms; `session.test.ts` and `tls.test.ts` wait forever, so an event that never fires still hangs the run the way an