From f4e55909a111d3d49d5b6308e4c7ed12bc40355d Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Fri, 28 Aug 2026 14:44:32 +0200 Subject: [PATCH] Keep the delivery-receipt merges across a reconnect instead of dropping them --- AGENTS.md | 10 +++++ src/session.ts | 12 +++++- test/session-extras.test.ts | 74 +++++++++++++++++++++++++++++++++++++ todo.md | 20 ++++------ 4 files changed, 101 insertions(+), 15 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 64b75ba..0b553f7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -233,6 +233,16 @@ exactly 140. override them as methods — re-declaring them the same way is its way out. `unknown` rather than `void | Promise` because a listener may return anything — `session.on('close', () => set.delete(session))` returns a boolean. +- **A reconnect keeps the delivery-receipt merges; everything else the link held is dropped.** + `onDeliverSm()` answers each receipt before the group it belongs to is complete, and `teardown()` + runs on every path — an idle timeout and a failed rebind, not only `close()` — so clearing the + merges there loses receipts no peer has a reason to send again. They are cleared where the session + is over instead: `close()`, or a drop with no reconnect loop left to bring the link back. Inbound + segments stay in `teardown()`, because they go unanswered until the message is whole: the peer + still holds them, and answering it on a later link with the old segments' sequence numbers would + correlate with nothing. Surviving a process restart is a separate, public-surface question, and is + in todo.md. + - **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/src/session.ts b/src/session.ts index dbd533c..a1b49f0 100644 --- a/src/session.ts +++ b/src/session.ts @@ -228,6 +228,8 @@ export class Session extends EventEmitter { close(): void { this.reconnectLoop?.stop(); this.teardown(); + // Held across a drop instead: the peer never resends a receipt this session already answered. + this.dlrMerger.clear(); } private loopFor(reconnect: ReconnectOptions | undefined): ReconnectLoop | undefined { @@ -316,7 +318,6 @@ export class Session extends EventEmitter { this.closed = true; this.timers.clear(); this.pending.settleAll(new Error('Session closed before a response arrived')); - this.dlrMerger.clear(); this.incoming.clear(); this.sock.destroy(); this.emit('close'); @@ -406,6 +407,13 @@ export class Session extends EventEmitter { private onClose(): void { this.teardown(); - this.reconnectLoop?.schedule(); + + if (this.reconnectLoop && !this.reconnectLoop.isStopped()) { + this.reconnectLoop.schedule(); + + return; + } + + this.dlrMerger.clear(); } } diff --git a/test/session-extras.test.ts b/test/session-extras.test.ts index 1f37467..e27cbde 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -8,6 +8,7 @@ import type { MessageDlr } from '../src/session.ts'; import type { PduObject, PduObjectInput } from '../src/pdu.ts'; import type { Result } from '../src/result.ts'; import type { SendSmsResult } from '../src/send-sms.ts'; +import type { Session } from '../src/session.ts'; import type { SmppLog } from '../src/log.ts'; import type { Sms } from '../src/sms.ts'; import type { SmppServer } from '../src/server.ts'; @@ -271,6 +272,34 @@ describe('sendSms()', () => { }); describe('reconnect', () => { + /** The server's side of the one connection under test, replaced by every reconnect. */ + function peerOf(smpp: SmppServer): Session { + const [peer] = smpp.sessions; + + assert.equal(smpp.sessions.size, 1); + assert.ok(peer); + + return peer; + } + + async function sendReceipt(peer: Session, smsId: string): Promise { + const sent = await peer.send({ + cmdName: 'deliver_sm', + params: { + destination_addr: '46701113311', + esm_class: consts.ESM_CLASS.MC_DELIVERY_RECEIPT, + short_message: `id:${smsId} stat:DELIVRD err:000 text:`, + source_addr: '46709771337', + }, + tlvs: { + message_state: { tagValue: consts.MESSAGE_STATE.DELIVERED }, + receipted_message_id: { tagValue: smsId }, + }, + }); + + assert.equal(sent.err, undefined); + } + test('re-binds after the connection drops, keeping the same session object', async () => { const smpp = await startServer(); const messages: string[] = []; @@ -316,6 +345,51 @@ describe('reconnect', () => { await smpp.close(); }); + test('merges the receipts of a multipart message across a drop', async () => { + const smpp = await startServer(); + + smpp.on('session', bound => { + bound.on('sms', sms => { void sms.sendResp({ smsId: 'across-the-drop' }); }); + }); + + const { session } = await client({ + port: smpp.port, + reconnect: { maxDelay: 100, minDelay: 20 }, + }); + + assert.ok(session); + + const sent = await session.sendSms({ + dlr: true, + from: '46701113311', + message: 'x'.repeat(400), + to: '46709771337', + }); + + assert.deepEqual(sent.smsIds, ['across-the-drop-1', 'across-the-drop-2', 'across-the-drop-3']); + + // Answered on the link that then drops, so an SMSC has no reason to ever send it again. + await sendReceipt(peerOf(smpp), 'across-the-drop-1'); + + const reconnected = once(resolve => { session.on('reconnected', () => { resolve(true); }); }); + + peerOf(smpp).close(); + await reconnected; + + const merged = once(resolve => { session.on('messageDlr', resolve); }); + + await sendReceipt(peerOf(smpp), 'across-the-drop-2'); + await sendReceipt(peerOf(smpp), 'across-the-drop-3'); + + const report = await merged; + + assert.equal(report.smsId, 'across-the-drop'); + assert.equal(report.segments.length, 3); + + session.close(); + await smpp.close(); + }); + test('does not reconnect after an explicit close', async () => { const smpp = await startServer(); const { session } = await client({ diff --git a/todo.md b/todo.md index 66bebd9..6e40c2d 100644 --- a/todo.md +++ b/todo.md @@ -5,7 +5,7 @@ rules there constrain every item below. ## Status -The rewrite is **feature complete and green**: 235 tests, lint and typecheck clean, verified on Node +The rewrite is **feature complete and green**: 236 tests, lint and typecheck clean, verified on Node 18, 20, 22 and 24. What is left is release work and a few things worth adding before or after 1.0.0. ```bash @@ -54,7 +54,7 @@ Rules the API follows: | Stream framing | `test/pdu-framer.test.ts` | | Delivery receipt parsing, TLV and text | `test/dlr.test.ts` | | Session, client, server: bind, auth, send, reassembly, DLRs, timeouts, abort, send window | `test/session.test.ts` | -| Merged multipart DLRs, reconnect, reassembly bounds, per-send abort, the segment cap | `test/session-extras.test.ts` | +| Merged multipart DLRs including across a reconnect, reassembly bounds, per-send abort, the segment cap | `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` | | A listener that throws, or rejects, reaching `sessionError`/`serverError` rather than the process | `test/session.test.ts`, `test/error-from.test.ts` | @@ -114,17 +114,11 @@ session message is a change to every call site. - [ ] **In-flight sends across a reconnect.** They currently fail with "Session closed before a response arrived" and the caller retries. Re-queueing them automatically would be friendlier but risks duplicate delivery, so it needs a decision before it is built. -- [ ] **A drop discards delivery receipts already acknowledged to the peer.** `teardown()` calls - `dlrMerger.clear()`, and it runs on every path — an idle timeout and a failed rebind, not only - `close()`. `onDeliverSm()` answers each receipt with `sendReturn()` before the merge completes, - so a peer that has sent two of three segment receipts will never resend them and `messageDlr` - can never fire for that message. `dlrMergeTimeout` budgets a day for the last receipt to - arrive, while the state does not survive a thirty-second link drop. Reassembly is not affected - the same way: inbound segments go unanswered until the message is whole, so the peer keeps - them. Clearing is right when `close()` ends the session for good and wrong on the reconnect - path; separating the two is most of the fix. Surviving a process restart as well means - exposing the merge state for the application to persist and hand back, which is a - public-surface decision. +- [ ] **Merge state does not survive a process restart.** A drop no longer discards it — `close()` + and a drop with no reconnect left clear the merges, `teardown()` does not — but a restart still + loses every incomplete group, and a peer has no reason to resend a receipt it already had + answered. Surviving one means exposing the merge state for the application to persist and hand + back, which is a public-surface decision. - [ ] **`close()` and `unbind()` drop in-flight requests instead of draining them.** `teardown()` settles every pending request with "Session closed before a response arrived" and destroys the socket in the same tick, so a submit the SMSC has already accepted is reported to the caller