From b9167073d44cc087b5ab6e3420af8d2ec9ff8eda Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Sat, 26 Sep 2026 14:41:10 +0200 Subject: [PATCH] Warn once at the held-message bound, tell receivers sendResp() is what releases a message, and pin that a refused segment joins no group --- CHANGELOG.md | 9 +++++---- README.md | 18 +++++++++++------- docs/decisions.md | 4 +++- interop-tests/compose.dumbclient.yaml | 1 + interop-tests/findings/07-load.md | 11 +++++------ src/incoming-requests.ts | 14 ++++++++++++++ test/session-extras.test.ts | 9 ++++++--- todo.md | 5 +++++ 8 files changed, 50 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 27ad67a..fc2bcb3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,10 +40,11 @@ count as next to nothing, so a peer could hold far more than the cap. **Raise a `maxOctets` you tuned low**: it now holds several times fewer segments, and an incomplete message evicted over the cap is lost, since its segments were already answered. -- A message arriving while the application holds 1000 unanswered, or 64 MiB of them by the - `maxOctets` charge, is refused with `ESME_RTHROTTLED` (`ESME_RX_T_APPN` on a `deliver_sm`), so the - peer keeps it and retries. The oldest used to be dropped to make room, which freed nothing while - the application still held it and let `close()` stop waiting for a message the peer was owed. +- A message arriving while the application holds 1000 unanswered, or 64 MiB of them counted the way + `maxOctets` counts segments, is refused with `ESME_RTHROTTLED` (`ESME_RX_T_APPN` on a + `deliver_sm`), so the peer keeps it and retries. **Call `sendResp()` on every `sms`, multipart + included**: 1000 left unanswered now stop inbound traffic for up to five minutes, where the oldest + used to be dropped with a warning. - A `submit_sm` segment the reassembly buffer has no room for is refused with `ESME_RTHROTTLED`, where it was `ESME_RMSGQFUL`. - `server()` refuses a `maxOctets` below 1 or not a whole number, `Infinity` included, like its diff --git a/README.md b/README.md index c9f69b0..988e2e6 100644 --- a/README.md +++ b/README.md @@ -101,9 +101,10 @@ session.on('sms', async sms => { }); ``` -Call `sendResp()` for every message; it is part of the protocol. Delivery receipts reach you as -`dlr` events, not here. A multipart message arrives reassembled and already answered segment by -segment, so `sendResp()` there only says you are done with it: [Receiving in depth](#receiving-in-depth). +Call `sendResp()` for every message, multipart included: until you do, it counts toward the bound +past which the peer's messages are refused. Delivery receipts reach you as `dlr` events, not here. A +multipart message arrives reassembled and already answered segment by segment, so `sendResp()` there +puts nothing on the wire and releases it: [Receiving in depth](#receiving-in-depth). ## Run an SMPP server @@ -383,10 +384,7 @@ holds for `session.send()`. message has failed. Answering through `sendReturn()` instead leaves the wait running. 3. Tear down what is left, resolving to an `err` that says what was lost. -A message arriving while 1000 unanswered messages, or 64 MiB of them by the `maxOctets` charge, are -held is refused with `ESME_RTHROTTLED` (`ESME_RX_T_APPN` on a `deliver_sm`), so the peer retries it. -One held five minutes is dropped with a warning on the log and waited for no longer. None of the -bounds is an option. +A message left unanswered for five minutes is no longer waited for. `close({ signal })` cuts the wait short. `unbind()` takes no signal, and waits a further `responseTimeout` for its own response. @@ -434,6 +432,12 @@ const { err, pduObj } = await session.send({ each is two messages. - **Answered on arrival.** Each segment was answered as it landed, before you see the message: [Server in depth](#server-in-depth). +- **Unanswered messages.** While 1000 messages you have not called `sendResp()` on, or 64 MiB of + them counted the way `maxOctets` counts segments, are held, every new message is refused with + `ESME_RTHROTTLED` (`ESME_RX_T_APPN` on a `deliver_sm`) so the peer retries it, and no `sms` fires. + Reaching the bound logs one `warn`, and falling back below it one `info`. A message left five + minutes is dropped from the count with a `warn`; a later `sendResp()` still answers it. Neither + bound is an option. - **Where the body is.** A body in the `message_payload` TLV, SMPP's way of carrying up to 64 KB and the only place a `data_sm` has, reads exactly like one in `short_message`, concatenated messages and receipts included. A PDU filling both is read from `short_message`. diff --git a/docs/decisions.md b/docs/decisions.md index 4190bd6..ea9cad1 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -685,7 +685,9 @@ rule and an index of the titles below. retry status, so the peer keeps it (goal 2). Rejected: dropping the oldest to make room, which frees nothing while the application still holds its `Sms`, and stops the drain waiting for a message the peer is owed. Rejected: pausing the socket, which also stalls every answer and - `enquire_link` on the link. A message held past its timeout is still dropped, so `close()` can + `enquire_link` on the link. Reaching the bound is a log line and nothing more (goal 8): an event + or a public count would be surface for what the application already knows, since it is the one + not answering. A message held past its timeout is still dropped, so `close()` can report fewer unanswered than there were — accepted, because the alternative is holding what nothing will answer. diff --git a/interop-tests/compose.dumbclient.yaml b/interop-tests/compose.dumbclient.yaml index b29e726..e6c4278 100644 --- a/interop-tests/compose.dumbclient.yaml +++ b/interop-tests/compose.dumbclient.yaml @@ -19,6 +19,7 @@ services: image: nicolaka/netshoot:v0.16 <<: *log-limits command: ["sleep", "infinity"] + init: true dumbclient-w2000: build: ./interop-tests/peers/dumbclient diff --git a/interop-tests/findings/07-load.md b/interop-tests/findings/07-load.md index 73cddd4..3b6a73e 100644 --- a/interop-tests/findings/07-load.md +++ b/interop-tests/findings/07-load.md @@ -100,8 +100,8 @@ event to a listener added after it fired. Fixed by attaching every session's `cl Three runs of `./interop-tests/run.py dumbclient`. Run 1 (the original 300,000-count soak) surfaced the S6 harness bug above; run 2 fixed it; run 3, with the namespace owner above and the held-message -throttle in `src/`, is the one Scenarios reports. The capture figures below are run 2's. `smppload.test.ts` passed on every run it was given (three, across the investigation -above); its one scenario needs no repeat - a second run reproduces the identical corrupted PDU, +throttle in `src/`, is the one Scenarios reports. The capture figures below are run 2's. +`smppload.test.ts` passed on every run it was given (three, across the investigation above); its one scenario needs no repeat - a second run reproduces the identical corrupted PDU, adding nothing. ``` @@ -152,10 +152,9 @@ that never gets as far as a readable PDU. - **smppload's `bind_transceiver` is corrupted on the wire** - see Setup. Not chased past `oserl`'s `pack/2` (which is correct on inspection) given the time-box. - **`smpp-dumb-client` treats `ESME_RTHROTTLED` as final** - a throttled message counts as sent - and is never resubmitted. Its `enquire_link` - interval (10s once bound as an ESME) is also hardcoded (`smpp.go`, `enquireSender(10)`), not - exposed through `config.yml` at all - the no-ping binary built for S6 patches the call site out - rather than configuring it. + and is never resubmitted. Its `enquire_link` interval (10s once bound as an ESME) is hardcoded + (`smpp.go`, `enquireSender(10)`), not exposed through `config.yml` at all - the no-ping binary + built for S6 patches the call site out rather than configuring it. - **`smpp.remote` takes a literal IP, never a hostname** (`net.ParseIP`, no DNS resolution) - see Setup. diff --git a/src/incoming-requests.ts b/src/incoming-requests.ts index 54ffb3d..817227e 100644 --- a/src/incoming-requests.ts +++ b/src/incoming-requests.ts @@ -71,6 +71,7 @@ export class IncomingRequests { private readonly smsIdFormat: SmsIdFormat; private readonly systemId: string; private linkGeneration = 0; + private refusing = false; constructor(options: IncomingRequestsOptions) { this.dlrMerger = options.dlrMerger; @@ -148,6 +149,7 @@ export class IncomingRequests { /** Drops the segments of every message that never became whole, and of every one still held. */ clear(): void { this.linkGeneration++; + this.refusing = false; this.held.clear(); this.reassembler.clear(); } @@ -217,6 +219,13 @@ export class IncomingRequests { */ private async onMessage(pduObj: PduObject): Promise { if (this.held.full()) { + if (!this.refusing) { + this.refusing = true; + this.log.warn('session - unanswered messages at their bound, refusing new ones until the application answers', { + messages: this.held.size, + }); + } + this.log.verbose('session - unanswered messages at their bound, asking the peer to retry', { cmdName: pduObj.cmdName, seqNr: pduObj.seqNr, @@ -226,6 +235,11 @@ export class IncomingRequests { return; } + if (this.refusing) { + this.refusing = false; + this.log.info('session - unanswered messages below their bound, accepting again', { messages: this.held.size }); + } + const concat = concatOf(pduObj); if (!concat) { diff --git a/test/session-extras.test.ts b/test/session-extras.test.ts index e541a93..98a540d 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -1530,9 +1530,10 @@ describe('held message bounds', () => { closeAfter(t, session); session.boundAs = 'transceiver'; + const warnings: string[] = []; const incoming = new IncomingRequests({ dlrMerger: new DlrMerger({ log: silentLog, max: 10, timeout: 10_000 }), - log: silentLog, + log: { ...silentLog, warn: message => { warnings.push(message); } }, sendPastDrain: () => Promise.resolve({ err: new Error('never sent') }), session, }); @@ -1557,12 +1558,14 @@ describe('held message bounds', () => { assert.equal(received.length, defaults.maxHeldMessages); assert.deepEqual(answers, ['ESME_RTHROTTLED', 'ESME_RTHROTTLED']); + assert.equal(warnings.length, 1, 'reaching the bound warns once, not per refusal'); - // The refused first segment joined no group, so the second one completes nothing. + // The refused first segment joined no group, so the second one is taken and completes nothing. await received[0]?.sendResp(); - await delay(0); + await new Promise(resolve => { setImmediate(resolve); }); await incoming.handle(segment(7, 2, 2)); + assert.equal(answers.at(-1), 'ESME_ROK'); assert.equal(received.length, defaults.maxHeldMessages); incoming.clear(); }); diff --git a/todo.md b/todo.md index 843a501..ee74247 100644 --- a/todo.md +++ b/todo.md @@ -405,6 +405,11 @@ and is also what the panel ranked hardest — two methods, one answer. ## Worth doing, not blocking +- [ ] **Make the dumbclient soak's memory sample evidence of no library leak again.** Its rss ends at + its maximum (298 MiB, heapUsed 81 MiB after 173,820 messages), which the harness's own per-id + `Set` and `answerOrder` explain but cannot separate from a leak in `src/`: sample the heap + after the bookkeeping is cleared. From the stability review of #29. + - [ ] **Decide whether `alert_notification` reaches the application as more than `incomingPduObj`.** It is the SMSC saying a handset it could not reach is reachable again (`esme_addr`, `ms_availability_status`); a client has no `onRequest`, so the raw PDU event is the only way in.