From f2977638f23e6ee6c0f84ded90eb1f6773994ab5 Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Sat, 26 Sep 2026 14:44:00 +0200 Subject: [PATCH] Stop refusing only once the held messages are down to half, so a full window warns once, and name the octets in the warning --- README.md | 7 ++++--- interop-tests/findings/07-load.md | 4 ++-- src/held-messages.ts | 4 ++++ src/incoming-requests.ts | 28 ++++++++++++++++++++-------- test/session-extras.test.ts | 10 ++++++++++ 5 files changed, 40 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index 988e2e6..4f5f349 100644 --- a/README.md +++ b/README.md @@ -435,7 +435,8 @@ const { err, pduObj } = await session.send({ - **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 + Reaching the bound logs one `warn`, and the first message accepted once both are down to half 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 @@ -498,8 +499,8 @@ the unanswered messages are at their bound, which asks the SMSC to keep it and t `sms.answeredOnArrival` says whether the message you hold was answered that way; a segment count cannot, since a peer may number a message one part of one. -- The id was fixed with the first segment, so `sendResp()` there only says you are done, and - returns `err` for an `smsId` or a refusing `status`. +- The id was fixed with the first segment, so `sendResp()` there puts nothing on the wire and + releases the message, and returns `err` for an `smsId` or a refusing `status`. - `sms.smsId` is the base. `sendDlr()` names `-1`, `-2` and so on: the ids the `submit_sm` responses carried. - A `deliver_sm` is answered with no id at all, since SMPP marks that field unused, so an inbound diff --git a/interop-tests/findings/07-load.md b/interop-tests/findings/07-load.md index 3b6a73e..51ba1fd 100644 --- a/interop-tests/findings/07-load.md +++ b/interop-tests/findings/07-load.md @@ -101,8 +101,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, -adding nothing. +`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. ``` dumbclient run 2: frames 111300, bind_transceiver 4/4, enquire_link 12 (enquire_link_resp 9 - the diff --git a/src/held-messages.ts b/src/held-messages.ts index adab745..9585382 100644 --- a/src/held-messages.ts +++ b/src/held-messages.ts @@ -44,6 +44,10 @@ export class HeldMessages { this.maxOctets = options.maxOctets; } + get octetsHeld(): number { + return this.octets; + } + get size(): number { return this.held.size; } diff --git a/src/incoming-requests.ts b/src/incoming-requests.ts index 817227e..64867c2 100644 --- a/src/incoming-requests.ts +++ b/src/incoming-requests.ts @@ -213,16 +213,13 @@ export class IncomingRequests { await this.session.sendReturn(pduObj); } - /** - * A concatenated message is answered segment by segment as it arrives: a peer that dispatches - * one request at a time never sends the second segment until the first has been answered. - */ - private async onMessage(pduObj: PduObject): Promise { + private async refusedAtBound(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, + octets: this.held.octetsHeld, }); } @@ -232,14 +229,29 @@ export class IncomingRequests { }); await this.session.sendReturn(pduObj, throttledStatus(this.carriedAs(pduObj))); - return; + return true; } - if (this.refusing) { + // Half, so a peer keeping its window full does not flip this on every answer. + if ( + this.refusing + && this.held.size <= defaults.maxHeldMessages / 2 + && this.held.octetsHeld <= defaults.maxHeldOctets / 2 + ) { this.refusing = false; - this.log.info('session - unanswered messages below their bound, accepting again', { messages: this.held.size }); + this.log.info('session - unanswered messages down to half their bound, accepting again', { messages: this.held.size }); } + return false; + } + + /** + * A concatenated message is answered segment by segment as it arrives: a peer that dispatches + * one request at a time never sends the second segment until the first has been answered. + */ + private async onMessage(pduObj: PduObject): Promise { + if (await this.refusedAtBound(pduObj)) return; + const concat = concatOf(pduObj); if (!concat) { diff --git a/test/session-extras.test.ts b/test/session-extras.test.ts index 98a540d..fa52f1a 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -1567,6 +1567,16 @@ describe('held message bounds', () => { assert.equal(answers.at(-1), 'ESME_ROK'); assert.equal(received.length, defaults.maxHeldMessages); + + // A peer keeping its window full crosses the bound on every answer, and that is still one warning. + await received[1]?.sendResp(); + await new Promise(resolve => { setImmediate(resolve); }); + await incoming.handle(submitPdu(defaults.maxHeldMessages + 2)); + await incoming.handle(submitPdu(defaults.maxHeldMessages + 3)); + await incoming.handle(submitPdu(defaults.maxHeldMessages + 4)); + + assert.equal(answers.at(-1), 'ESME_RTHROTTLED'); + assert.equal(warnings.length, 1); incoming.clear(); });