Stop refusing only once the held messages are down to half, so a full window warns once, and name the octets in the warning
Mirror / push (push) Successful in 5s
Test / test (22) (pull_request) Successful in 31s
Test / lint (pull_request) Successful in 22s
Test / test (18) (pull_request) Successful in 30s
Test / test (20) (pull_request) Successful in 30s
Test / test (24) (pull_request) Successful in 31s
Test / test (26) (pull_request) Successful in 31s

This commit is contained in:
2026-09-26 14:44:00 +02:00
parent b9167073d4
commit f2977638f2
5 changed files with 40 additions and 13 deletions
+4 -3
View File
@@ -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 - **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 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. `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 minutes is dropped from the count with a `warn`; a later `sendResp()` still answers it. Neither
bound is an option. 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 - **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 `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. 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 - The id was fixed with the first segment, so `sendResp()` there puts nothing on the wire and
returns `err` for an `smsId` or a refusing `status`. releases the message, and returns `err` for an `smsId` or a refusing `status`.
- `sms.smsId` is the base. `sendDlr()` names `<smsId>-1`, `<smsId>-2` and so on: the ids the - `sms.smsId` is the base. `sendDlr()` names `<smsId>-1`, `<smsId>-2` and so on: the ids the
`submit_sm` responses carried. `submit_sm` responses carried.
- A `deliver_sm` is answered with no id at all, since SMPP marks that field unused, so an inbound - A `deliver_sm` is answered with no id at all, since SMPP marks that field unused, so an inbound
+2 -2
View File
@@ -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 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 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. 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, `smppload.test.ts` passed on every run it was given (three, across the investigation above); its one
adding nothing. 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 dumbclient run 2: frames 111300, bind_transceiver 4/4, enquire_link 12 (enquire_link_resp 9 - the
+4
View File
@@ -44,6 +44,10 @@ export class HeldMessages {
this.maxOctets = options.maxOctets; this.maxOctets = options.maxOctets;
} }
get octetsHeld(): number {
return this.octets;
}
get size(): number { get size(): number {
return this.held.size; return this.held.size;
} }
+20 -8
View File
@@ -213,16 +213,13 @@ export class IncomingRequests {
await this.session.sendReturn(pduObj); await this.session.sendReturn(pduObj);
} }
/** private async refusedAtBound(pduObj: PduObject): Promise<boolean> {
* 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<void> {
if (this.held.full()) { if (this.held.full()) {
if (!this.refusing) { if (!this.refusing) {
this.refusing = true; this.refusing = true;
this.log.warn('session - unanswered messages at their bound, refusing new ones until the application answers', { this.log.warn('session - unanswered messages at their bound, refusing new ones until the application answers', {
messages: this.held.size, messages: this.held.size,
octets: this.held.octetsHeld,
}); });
} }
@@ -232,14 +229,29 @@ export class IncomingRequests {
}); });
await this.session.sendReturn(pduObj, throttledStatus(this.carriedAs(pduObj))); 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.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<void> {
if (await this.refusedAtBound(pduObj)) return;
const concat = concatOf(pduObj); const concat = concatOf(pduObj);
if (!concat) { if (!concat) {
+10
View File
@@ -1567,6 +1567,16 @@ describe('held message bounds', () => {
assert.equal(answers.at(-1), 'ESME_ROK'); assert.equal(answers.at(-1), 'ESME_ROK');
assert.equal(received.length, defaults.maxHeldMessages); 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(); incoming.clear();
}); });