Keep the delivery-receipt merges across a reconnect instead of dropping them

This commit is contained in:
2026-08-28 14:44:32 +02:00
parent 30e0cfd38a
commit f4e55909a1
4 changed files with 101 additions and 15 deletions
+10
View File
@@ -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<void>` because a listener may return override them as methods — re-declaring them the same way is its way out. `unknown` rather than `void | Promise<void>` because a listener may return
anything — `session.on('close', () => set.delete(session))` returns a boolean. 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 - **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 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 `node:24.18.0-bookworm-slim` ships no openssl binary, so a shelled-out fixture would pass in CI
+10 -2
View File
@@ -228,6 +228,8 @@ export class Session extends EventEmitter<SessionEvents> {
close(): void { close(): void {
this.reconnectLoop?.stop(); this.reconnectLoop?.stop();
this.teardown(); 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 { private loopFor(reconnect: ReconnectOptions | undefined): ReconnectLoop | undefined {
@@ -316,7 +318,6 @@ export class Session extends EventEmitter<SessionEvents> {
this.closed = true; this.closed = true;
this.timers.clear(); this.timers.clear();
this.pending.settleAll(new Error('Session closed before a response arrived')); this.pending.settleAll(new Error('Session closed before a response arrived'));
this.dlrMerger.clear();
this.incoming.clear(); this.incoming.clear();
this.sock.destroy(); this.sock.destroy();
this.emit('close'); this.emit('close');
@@ -406,6 +407,13 @@ export class Session extends EventEmitter<SessionEvents> {
private onClose(): void { private onClose(): void {
this.teardown(); this.teardown();
this.reconnectLoop?.schedule();
if (this.reconnectLoop && !this.reconnectLoop.isStopped()) {
this.reconnectLoop.schedule();
return;
}
this.dlrMerger.clear();
} }
} }
+74
View File
@@ -8,6 +8,7 @@ import type { MessageDlr } from '../src/session.ts';
import type { PduObject, PduObjectInput } from '../src/pdu.ts'; import type { PduObject, PduObjectInput } from '../src/pdu.ts';
import type { Result } from '../src/result.ts'; import type { Result } from '../src/result.ts';
import type { SendSmsResult } from '../src/send-sms.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 { SmppLog } from '../src/log.ts';
import type { Sms } from '../src/sms.ts'; import type { Sms } from '../src/sms.ts';
import type { SmppServer } from '../src/server.ts'; import type { SmppServer } from '../src/server.ts';
@@ -271,6 +272,34 @@ describe('sendSms()', () => {
}); });
describe('reconnect', () => { 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<void> {
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 () => { test('re-binds after the connection drops, keeping the same session object', async () => {
const smpp = await startServer(); const smpp = await startServer();
const messages: string[] = []; const messages: string[] = [];
@@ -316,6 +345,51 @@ describe('reconnect', () => {
await smpp.close(); 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<true>(resolve => { session.on('reconnected', () => { resolve(true); }); });
peerOf(smpp).close();
await reconnected;
const merged = once<MessageDlr>(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 () => { test('does not reconnect after an explicit close', async () => {
const smpp = await startServer(); const smpp = await startServer();
const { session } = await client({ const { session } = await client({
+7 -13
View File
@@ -5,7 +5,7 @@ rules there constrain every item below.
## Status ## 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. 18, 20, 22 and 24. What is left is release work and a few things worth adding before or after 1.0.0.
```bash ```bash
@@ -54,7 +54,7 @@ Rules the API follows:
| Stream framing | `test/pdu-framer.test.ts` | | Stream framing | `test/pdu-framer.test.ts` |
| Delivery receipt parsing, TLV and text | `test/dlr.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` | | 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` | | Every runnable README example | `test/readme.test.ts` |
| Receipt-versus-message classification by `esm_class` | `test/dlr.test.ts`, `test/session.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` | | 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 - [ ] **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 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. 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 - [ ] **Merge state does not survive a process restart.** A drop no longer discards it — `close()`
`dlrMerger.clear()`, and it runs on every path — an idle timeout and a failed rebind, not only and a drop with no reconnect left clear the merges, `teardown()` does not — but a restart still
`close()`. `onDeliverSm()` answers each receipt with `sendReturn()` before the merge completes, loses every incomplete group, and a peer has no reason to resend a receipt it already had
so a peer that has sent two of three segment receipts will never resend them and `messageDlr` answered. Surviving one means exposing the merge state for the application to persist and hand
can never fire for that message. `dlrMergeTimeout` budgets a day for the last receipt to back, which is a public-surface decision.
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.
- [ ] **`close()` and `unbind()` drop in-flight requests instead of draining them.** `teardown()` - [ ] **`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 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 socket in the same tick, so a submit the SMSC has already accepted is reported to the caller