From e6250ef44ef33369b4f34b25da8db83b30644f52 Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Tue, 1 Sep 2026 20:40:17 +0200 Subject: [PATCH] Drop a request whose link went while onRequest ran, and pin the wire in the test --- AGENTS.md | 9 +++------ README.md | 2 +- src/incoming-requests.ts | 6 +++++- src/sms.ts | 2 +- test/session-extras.test.ts | 36 ++++++++++++++++++++++++++++++++---- 5 files changed, 42 insertions(+), 13 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 0993c65..8e083bc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -308,12 +308,9 @@ Grouped by what each one constrains. `attach()`, which is why a second drop emits again. - **An answer belongs to the link the message arrived on; a receipt does not.** Maintainer's call, - 2026-09-01: `sendResp()` carries the peer's own sequence number, which the next link knows nothing - about, so on a message whose link went it writes nothing and returns an `err`. Writing it to the - new socket instead succeeds and reports `{}` for a response that correlates with nothing, which is - goal 2's wrong answer. `sendDlr()` still goes out, because a receipt is a request of its own - correlated by `receipted_message_id`; the accepted cost is that one sent after a refused response - names an id the peer has no record of. + 2026-09-01. Rejected: answering on the new link, which succeeds and reports `{}` for a response + that correlates with nothing — goal 2's wrong answer. Accepted: a receipt sent after a refused + response names an id the peer has no record of. - **`reconnect` takes `{ minDelay, maxDelay }` to retune and `false` to turn off**, so absent means on and there is one spelling for each. Only `client()` reconnects — a `server()` session is a diff --git a/README.md b/README.md index 7e48f98..5c10c48 100644 --- a/README.md +++ b/README.md @@ -316,7 +316,7 @@ 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 the `smsId` it was answered with. | +| `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. | | `close` | The session is over, because nothing will bring the link back. Fires once, whether you closed it or the link failed for good. | diff --git a/src/incoming-requests.ts b/src/incoming-requests.ts index 748dc14..621ed9d 100644 --- a/src/incoming-requests.ts +++ b/src/incoming-requests.ts @@ -39,7 +39,6 @@ export class IncomingRequests { private readonly session: Session; private readonly smsIdFormat: SmsIdFormat; private readonly systemId: string; - /** Bumped when a link goes, so a message from an earlier one knows its answer cannot correlate. */ private linkGeneration = 0; constructor(options: IncomingRequestsOptions) { @@ -64,8 +63,13 @@ export class IncomingRequests { } async handle(pduObj: PduObject): Promise { + const generation = this.linkGeneration; + if (this.onRequest && await this.onRequest(this.session, pduObj)) return; + // The link it arrived on went while the hook ran, so nothing we answer now correlates. + if (this.linkGeneration !== generation) return; + if (!this.session.bindAllows(pduObj.cmdName)) { this.log.info('session - command the peer\'s bind direction does not carry', { bindType: this.session.boundAs ?? '', diff --git a/src/sms.ts b/src/sms.ts index 7a679be..8b74b1d 100644 --- a/src/sms.ts +++ b/src/sms.ts @@ -38,7 +38,7 @@ export type Sms = { /** Answers every segment. Part of the protocol, not optional. Defaults to ESME_ROK. */ sendResp: (options?: SendRespOptions) => Promise; session: Session; - /** The id answered to the peer: what sendResp() was given, or a generated UUID v7. */ + /** The id `sendResp()` was given, or a generated UUID v7. */ readonly smsId: string; submitTime: Date; to: string; diff --git a/test/session-extras.test.ts b/test/session-extras.test.ts index c8b17a7..6a7ca54 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -13,6 +13,7 @@ import type { Sms } from '../src/sms.ts'; import type { SmppServer } from '../src/server.ts'; import type { TestContext } from 'node:test'; import { HeldMessages } from '../src/held-messages.ts'; +import { IncomingRequests } from '../src/incoming-requests.ts'; import { UnansweredError } from '../src/unanswered-error.ts'; import { createSms } from '../src/sms.ts'; import { LinkGate } from '../src/link-gate.ts'; @@ -589,6 +590,7 @@ describe('reconnect', () => { assert.ok(answered); assert.ok(held); + assert.equal(answered.message, 'answered before the drop'); assert.equal((await answered.sendResp()).err, undefined); const reconnected = once(resolve => { session.on('reconnected', () => { resolve(true); }); }); @@ -596,17 +598,43 @@ describe('reconnect', () => { await peerOf(smpp).close({ signal: AbortSignal.abort() }); await reconnected; - const taken: string[] = []; + let taken = 0; - peerOf(smpp).on('incomingPduObj', pduObj => { taken.push(pduObj.cmdName); }); + // A response is dispatched before `incomingPduObj`, so only the raw event sees one arrive. + peerOf(smpp).on('incomingPdu', () => { taken++; }); // The answered one is out of the hold and the held one is not, so neither may reach the answer. assert.match((await answered.sendResp()).err?.message ?? '', /link this message arrived on is gone/); assert.match((await held.sendResp()).err?.message ?? '', /link this message arrived on is gone/); - // A receipt is a request of its own, correlated by its id, so the new link carries it. assert.equal((await held.sendDlr('DELIVERED')).err, undefined); - assert.deepEqual(taken, ['deliver_sm'], 'a response reached the new link'); + assert.equal(taken, 1, 'a refused response reached the new link'); + }); + + test('drops a message whose link went while onRequest was still running', async t => { + const session = new Session({ sock: new net.Socket() }); + + closeAfter(t, session); + session.boundAs = 'transceiver'; + + const incoming = new IncomingRequests({ + dlrMerger: new DlrMerger({ log: silentLog, max: 10, timeout: 10_000 }), + log: silentLog, + onRequest: async () => { await delay(10); return false; }, + sendPastDrain: () => Promise.resolve({ err: new Error('never sent') }), + session, + }); + let messages = 0; + + session.on('sms', () => { messages++; }); + + const handled = incoming.handle(submitPdu(1)); + + incoming.clear(); + + await handled; + + assert.equal(messages, 0); }); test('does not reconnect after an explicit close', async t => {