Say undefined where the SMSC named no id for a segment it took (#101)
* Cover the ids a send gets back where the SMSC named only some of them * Say undefined where the SMSC named no id for a segment it took * Record why the merger takes the wider type, and de-duplicate the interop narrowing * Guard the numbering hole the merger must refuse, and correct the phase record
This commit is contained in:
@@ -944,6 +944,31 @@ Grouped by what each one constrains.
|
||||
only because nothing stops the reconnect loop without `emitClose()` following it: `drain()` and
|
||||
`end()` are the only callers of `stop()`. A third caller has to shut the gate itself.
|
||||
|
||||
- **A segment the SMSC took and named no id for is `undefined` in `smsIds`, not an empty string.**
|
||||
Maintainer's call, 2026-09-12: `paramText()` resolves an absent `message_id` and one a peer wrote
|
||||
empty to the same `''`, which `string[]` then presented as an id — taking `smsIds[0]`, or keying a
|
||||
correlation table by the array, compiled and then misbehaved, and one message's empty entry
|
||||
collides with another's. Telesign names an id for the first segment of a concatenated submit only,
|
||||
so it is a documented operator's shape rather than a hypothesis. Nothing else moves:
|
||||
`parseSegmentId('')` matched nothing, so `DlrMerger` already abandoned such a send and `undefined`
|
||||
reaches that same refusal. `expect()` takes the wider type rather than a filtered `string[]`
|
||||
because the arity is what `idNumbering()` refuses on: filtering `['a-1', undefined, 'a-3']` leaves
|
||||
a numbering that spells out a whole message, and merges one that was never whole. `dlrFromPdu()`
|
||||
reads an id through `nonEmptyText()`, so no receipt could ever have matched an empty entry — and
|
||||
that reading stays separate from this one rather than sharing a helper, since it must leave a
|
||||
Buffer-valued `receipted_message_id` unresolved for `messageType()` to read the PDU as unmarked.
|
||||
What settles it here is the resolved text rather than the parameter, because `writeParams()`
|
||||
substitutes the field's own default: a peer that omits `message_id` and one that writes it empty
|
||||
build the same octets, leaving a raw-parameter test nothing to tell apart. Rejected: keeping `''`
|
||||
and documenting it, which leaves the published type promising what the value does not keep —
|
||||
goal 2, a wrong answer about what the peer named. Rejected: dropping the unnamed entries, which
|
||||
breaks the positional correspondence with `pduObjs` that README promises and loses which segment a
|
||||
PDU belongs to. Rejected: `{ id?: string; pduObj: PduObject }[]`, which makes that positional
|
||||
promise structural where today the compiler cannot check it; deferred to the next major, the first
|
||||
place two documented fields may become one. Accepted: every consumer reading `smsIds` narrows,
|
||||
including the majority whose SMSC names every id; indexing narrows too, except for the consumer
|
||||
who sets `noUncheckedIndexedAccess`, which typed `smsIds[0]` as `string | undefined` already.
|
||||
|
||||
### Internals and tests
|
||||
|
||||
- **A listener that rejects is routed by Node's `captureRejections`, not by hand-dispatching.** Both
|
||||
|
||||
@@ -163,19 +163,19 @@ in `UCS2`. You get one id per segment:
|
||||
const { err, pduObjs, smsIds, unanswered } = await session.sendSms({ from, message, to });
|
||||
```
|
||||
|
||||
`smsIds` is positional with `pduObjs`, and an entry is empty where the SMSC accepted the segment
|
||||
without naming an id for it — some name one for the first segment only. No receipt ever matches an
|
||||
empty entry.
|
||||
`smsIds` is positional with `pduObjs`, and an entry is `undefined` where the SMSC accepted the
|
||||
segment without naming an id for it — some name one for the first segment only. No receipt ever
|
||||
carries an empty id, so an unnamed entry matches nothing.
|
||||
|
||||
`err` is set when the SMSC refuses a segment, and it names the status it refused with. Because every
|
||||
segment goes on the wire together, `pduObjs` and `smsIds` then hold what the SMSC did accept — enough
|
||||
to reconcile against a later receipt, not enough to resend the rest, so treat a partial failure as a
|
||||
failed message. `unanswered` counts the segments that went out and were never answered: the SMSC may
|
||||
have taken each of them and lost only the response, so a message with `unanswered` above zero cannot
|
||||
be sent again without risking a duplicate, however empty `smsIds` is. A message needing more than 255
|
||||
segments is refused before anything is sent, since the concatenation header numbers segments in a
|
||||
single octet. `maxSegments` lowers that ceiling:
|
||||
most handsets and SMSCs stop well short of 255, and refusing beats a message only half delivered.
|
||||
be sent again without risking a duplicate, however few ids `smsIds` carries. A message needing more
|
||||
than 255 segments is refused before anything is sent, since the concatenation header numbers
|
||||
segments in a single octet. `maxSegments` lowers that ceiling: most handsets and SMSCs stop well
|
||||
short of 255, and refusing beats a message only half delivered.
|
||||
|
||||
### Receiving
|
||||
|
||||
@@ -624,6 +624,10 @@ promises and the rough edges taken off.
|
||||
it reports the id the segments were answered with, the id `sendResp()` was given, or the UUID v7
|
||||
generated instead. Delete any `sms.smsId = …` line — assigning to it
|
||||
throws a `TypeError`, since modules are always strict mode — and pass the id to `sendResp()`.
|
||||
- **`smsIds` from `sendSms()` is `(string | undefined)[]`**, one entry per segment and positional
|
||||
with `pduObjs`, `undefined` where the SMSC took the segment without naming an id for it. Reading
|
||||
an id narrows, indexing included: `smsIds[0]` is `string | undefined`, as it already was under
|
||||
`noUncheckedIndexedAccess`.
|
||||
- **`checkuserpass` is now `authenticate`**, takes `{ password, session, systemId, systemType }` and
|
||||
returns `false` or `{ userData }`.
|
||||
- **Renamed options:** `enqLinkTiming` → `enquireLinkInterval`, server `timeout` → `idleTimeout`.
|
||||
|
||||
@@ -177,9 +177,9 @@ the fixtures pin that a reader would not otherwise expect:
|
||||
application wanting one report per message compares each `dlr.smsId` against the `smsIds` array
|
||||
`sendSms()` returned and merges them itself.
|
||||
- **Telesign answers only the first segment of a concatenated submit with a `message_id`.**
|
||||
`sendSms()` returns `['<id>', '', '']` — one entry per segment, positional with `pduObjs`, empty
|
||||
where the SMSC named nothing. `dlr.smsId` is never empty, so an empty entry matches no receipt,
|
||||
and no merge is armed.
|
||||
`sendSms()` returns `['<id>', undefined, undefined]` — one entry per segment, positional with
|
||||
`pduObjs`, `undefined` where the SMSC named nothing; it returned `''` there until #101.
|
||||
`dlr.smsId` is never empty, so an unnamed entry matches no receipt, and no merge is armed.
|
||||
|
||||
## Open questions
|
||||
|
||||
@@ -203,6 +203,5 @@ the fixtures pin that a reader would not otherwise expect:
|
||||
`FAILED` and CM.com's `DELIVERD`. The documented-codes table in `operator-receipts.test.ts` is
|
||||
the place a new one goes, and it fails loudly for anything nothing names.
|
||||
- Whether `smsIds` carrying an empty entry for a segment the SMSC took but named no id for is the
|
||||
right shape for a caller, or whether that case wants saying differently. It is documented in
|
||||
`README.md` and pinned by the Telesign scenario; the question is a product one, not a correctness
|
||||
one, and belongs to the phase 11 product-owner pass.
|
||||
right shape for a caller. Settled in #101: the entry is `undefined`, and the decision with its
|
||||
rejected alternatives is recorded in `AGENTS.md`.
|
||||
|
||||
@@ -476,6 +476,7 @@ describe('C3+C7 - long MT through the fake upstream, receipts and id consistency
|
||||
assert.equal(sent.smsIds.length, testCase.expectedSegments);
|
||||
|
||||
for (const id of sent.smsIds) {
|
||||
assert.ok(id, 'expected Jasmin to name a message id for every segment');
|
||||
assert.match(id, uuidPattern, 'expected a UUID-shaped message id from Jasmin\'s submit_sm_resp');
|
||||
|
||||
// The full round trip - submit_sm to Jasmin's smpps, mtrouter, AMQP, the connector bind,
|
||||
|
||||
@@ -97,6 +97,15 @@ function dlrLooksIntact(received: Received | undefined): received is Received {
|
||||
&& received.pduObj.tlvs.message_state?.tagValue !== undefined;
|
||||
}
|
||||
|
||||
/** SMPPSim names an id for every segment, so an unnamed one is the finding, not the norm. */
|
||||
function namedIds(smsIds: (string | undefined)[]): string[] {
|
||||
const ids = smsIds.filter(id => id !== undefined);
|
||||
|
||||
assert.equal(ids.length, smsIds.length, 'expected SMPPSim to name a message id for every segment');
|
||||
|
||||
return ids;
|
||||
}
|
||||
|
||||
/**
|
||||
* Resends a fresh message until one attempt's segments are every one matched by an intact `dlr`.
|
||||
* Retrying works around the peer+library interaction above without hiding it: a scenario only
|
||||
@@ -119,12 +128,14 @@ async function sendUntilAllDlrsArrive(
|
||||
|
||||
assert.equal(sent.err, undefined);
|
||||
|
||||
const ids = namedIds(sent.smsIds);
|
||||
|
||||
const complete = await waitFor(
|
||||
() => (sent.smsIds.every(id => dlrLooksIntact(dlrs.find(r => r.dlr.smsId === id))) ? true : undefined),
|
||||
() => (ids.every(id => dlrLooksIntact(dlrs.find(r => r.dlr.smsId === id))) ? true : undefined),
|
||||
DLR_RETRY_BUDGET_MS,
|
||||
);
|
||||
|
||||
if (complete) return sent.smsIds;
|
||||
if (complete) return ids;
|
||||
}
|
||||
|
||||
throw new Error(`no attempt got an intact DLR for every segment within ${String(DLR_MAX_ATTEMPTS)} tries`);
|
||||
@@ -149,14 +160,16 @@ async function sendUntilComplete(
|
||||
|
||||
assert.equal(sent.err, undefined);
|
||||
|
||||
const ids = namedIds(sent.smsIds);
|
||||
|
||||
const complete = await waitFor(() => {
|
||||
const allIntact = sent.smsIds.every(id => dlrLooksIntact(dlrs.find(r => r.dlr.smsId === id)));
|
||||
const allIntact = ids.every(id => dlrLooksIntact(dlrs.find(r => r.dlr.smsId === id)));
|
||||
const reassembled = sms.find(s => s.message === message);
|
||||
|
||||
return allIntact && reassembled ? { reassembled } : undefined;
|
||||
}, DLR_RETRY_BUDGET_MS);
|
||||
|
||||
if (complete) return { reassembled: complete.reassembled, smsIds: sent.smsIds };
|
||||
if (complete) return { reassembled: complete.reassembled, smsIds: ids };
|
||||
}
|
||||
|
||||
throw new Error(`no attempt got both an intact DLR per segment and a loopback reassembly within ${String(DLR_MAX_ATTEMPTS)} tries`);
|
||||
|
||||
@@ -38,13 +38,17 @@ async function sendAndAwaitDlrs(session: Session, dlrs: Dlr[], message: string):
|
||||
|
||||
assert.equal(sent.err, undefined);
|
||||
|
||||
const ids = sent.smsIds.filter(id => id !== undefined);
|
||||
|
||||
assert.equal(ids.length, sent.smsIds.length, 'expected smscsim to name a message id for every segment');
|
||||
|
||||
const complete = await waitFor(() => (
|
||||
sent.smsIds.every(id => dlrs.some(dlr => dlr.smsId === id)) ? true : undefined
|
||||
ids.every(id => dlrs.some(dlr => dlr.smsId === id)) ? true : undefined
|
||||
));
|
||||
|
||||
assert.ok(complete, 'every segment of the first send should get a DLR');
|
||||
|
||||
return sent.smsIds;
|
||||
return ids;
|
||||
}
|
||||
|
||||
describe('smscsim - C1 bind, keepalive, unbind', () => {
|
||||
|
||||
+3
-3
@@ -37,12 +37,12 @@ const severity: Record<MessageState, number> = {
|
||||
};
|
||||
|
||||
/** The base and the part numbers one send's ids carry, or nothing when they do not spell out one message. */
|
||||
function idNumbering(smsIds: string[]): { base: string; parts: Set<number> } | undefined {
|
||||
function idNumbering(smsIds: (string | undefined)[]): { base: string; parts: Set<number> } | undefined {
|
||||
const bases = new Set<string>();
|
||||
const parts = new Set<number>();
|
||||
|
||||
for (const smsId of smsIds) {
|
||||
const numbering = parseSegmentId(smsId);
|
||||
const numbering = smsId === undefined ? undefined : parseSegmentId(smsId);
|
||||
|
||||
if (!numbering) return undefined;
|
||||
|
||||
@@ -93,7 +93,7 @@ export class DlrMerger {
|
||||
}
|
||||
|
||||
/** Registers the ids one multipart send got back, so their receipts can be merged. */
|
||||
expect(smsIds: string[]): void {
|
||||
expect(smsIds: (string | undefined)[]): void {
|
||||
if (smsIds.length < 2) return;
|
||||
|
||||
const numbering = idNumbering(smsIds);
|
||||
|
||||
+11
-3
@@ -45,7 +45,8 @@ export type SendSmsInput =
|
||||
export type SendSmsResult = {
|
||||
err?: Error;
|
||||
pduObjs: PduObject[];
|
||||
smsIds: string[];
|
||||
/** Positional with `pduObjs`, and undefined where the SMSC took the segment and named no id. */
|
||||
smsIds: (string | undefined)[];
|
||||
/** Segments that went out unanswered. The peer may have taken them, so sending again may duplicate. */
|
||||
unanswered: number;
|
||||
};
|
||||
@@ -248,12 +249,19 @@ function checkSegments(allowed: number, segments: number): Error | undefined {
|
||||
return undefined;
|
||||
}
|
||||
|
||||
/** An absent message_id and one a peer wrote empty are the same octets, and neither names an id. */
|
||||
function acceptedId(pduObj: PduObject, notation: SmsIdNotation | undefined): string | undefined {
|
||||
const smsId = normaliseSmsId(paramText(pduObj.params.message_id), notation);
|
||||
|
||||
return smsId === '' ? undefined : smsId;
|
||||
}
|
||||
|
||||
function collectSent(
|
||||
sent: Result<{ pduObj: PduObject }>[],
|
||||
notation: SmsIdNotation | undefined,
|
||||
): SendSmsResult {
|
||||
const pduObjs: PduObject[] = [];
|
||||
const smsIds: string[] = [];
|
||||
const smsIds: (string | undefined)[] = [];
|
||||
let failure: Error | undefined;
|
||||
let unanswered = 0;
|
||||
|
||||
@@ -264,7 +272,7 @@ function collectSent(
|
||||
failure ??= one.err;
|
||||
} else if (one.pduObj.cmdStatus === 'ESME_ROK') {
|
||||
pduObjs.push(one.pduObj);
|
||||
smsIds.push(normaliseSmsId(paramText(one.pduObj.params.message_id), notation));
|
||||
smsIds.push(acceptedId(one.pduObj, notation));
|
||||
} else {
|
||||
const refusal = one.pduObj.cmdStatus ?? String(one.pduObj.cmdStatusId);
|
||||
|
||||
|
||||
@@ -489,6 +489,38 @@ describe('an SMSC that reports each segment under an id of its own', () => {
|
||||
|
||||
assert.equal(sent.err, undefined);
|
||||
assert.equal(smsc.octets.length, 3, 'every segment goes out whatever the peer answers');
|
||||
assert.deepEqual(sent.smsIds, [id, '', '']);
|
||||
assert.deepEqual(sent.smsIds, [id, undefined, undefined]);
|
||||
assert.equal(sent.smsIds.length, sent.pduObjs.length, 'an entry per accepted segment, positional with pduObjs');
|
||||
});
|
||||
|
||||
test('reports the segment that was named and merges nothing where the others were not', async t => {
|
||||
const base = '01a09739-0a98-7ac0-8a9a-fcd3a7648e62';
|
||||
const smsc = await dummySmsc(t, { messageIds: [`${base}-1`] });
|
||||
const session = await bindToSmsc(t, smsc.port);
|
||||
const merged: MessageDlr[] = [];
|
||||
const reported = collect<Dlr>(1, push => { session.on('dlr', push); });
|
||||
|
||||
session.on('messageDlr', report => merged.push(report));
|
||||
|
||||
const sent = await session.sendSms({ dlr: true, message: 'x'.repeat(400), ...message });
|
||||
|
||||
assert.deepEqual(sent.smsIds, [`${base}-1`, undefined, undefined]);
|
||||
smsc.deliver(receiptBody(`${base}-1`));
|
||||
|
||||
const [dlr] = await reported;
|
||||
|
||||
assert.equal(dlr?.smsId, `${base}-1`);
|
||||
assert.deepEqual(merged, [], 'a send with an unnamed segment spells out no message to merge');
|
||||
});
|
||||
});
|
||||
|
||||
describe('an SMSC that answers a submit with no message id at all', () => {
|
||||
test('reads an empty message_id as no id, on a single-segment send as much as a split one', async t => {
|
||||
const smsc = await dummySmsc(t, { messageIds: [] });
|
||||
const session = await bindToSmsc(t, smsc.port);
|
||||
const sent = await session.sendSms({ dlr: true, message: 'one segment', ...message });
|
||||
|
||||
assert.equal(sent.err, undefined);
|
||||
assert.deepEqual(sent.smsIds, [undefined]);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -2872,6 +2872,7 @@ describe('message id notation', () => {
|
||||
assert.deepEqual(sent.smsIds, [1, 2].map(part => `${sms.smsId}-${String(part)}`));
|
||||
|
||||
for (const smsId of sent.smsIds) {
|
||||
assert.ok(smsId);
|
||||
await sendReceipt(peerOf(smpp), smsId);
|
||||
}
|
||||
|
||||
|
||||
@@ -2498,11 +2498,18 @@ describe('merged delivery report bounds', () => {
|
||||
test('arms nothing for a send whose ids do not number one message', () => {
|
||||
const dlrMerger = merger();
|
||||
|
||||
dlrMerger.expect(['5cb0ea53b5d61093529174ca44e23871', '', '']);
|
||||
dlrMerger.expect(['5cb0ea53b5d61093529174ca44e23871', undefined, undefined]);
|
||||
assert.equal(dlrMerger.size, 0, 'an id the peer never named numbers nothing');
|
||||
|
||||
dlrMerger.expect(['bf53ad8b-1', '40ccdce2-2']);
|
||||
assert.equal(dlrMerger.size, 0, 'nor do ids numbered off a base each');
|
||||
|
||||
const holed = '01a09739-0a9a-767d-ad1d-1c27667e1ca4';
|
||||
|
||||
// Dropping the unnamed entry rather than passing it through leaves 1 and 3 spelling out a
|
||||
// whole 2-part message, and a send of 3 reports as delivered on two receipts.
|
||||
dlrMerger.expect([`${holed}-1`, undefined, `${holed}-3`]);
|
||||
assert.equal(dlrMerger.size, 0, 'nor does a hole between two parts of one base');
|
||||
});
|
||||
|
||||
// A receipt for whole-3 would otherwise fill the slot whole-2 was registered for, truncating the report.
|
||||
|
||||
@@ -113,13 +113,6 @@ session message is a change to every call site.
|
||||
|
||||
- [ ] Create the `@larvit/smpp` package on npm and add `NPM_TOKEN` to the repository secrets, which
|
||||
`.github/workflows/release.yaml` needs.
|
||||
- [ ] Decide whether `smsIds` should be `(string | undefined)[]`. An SMSC that names an id for the
|
||||
first segment of a concatenated message only — Telesign documents exactly that — leaves the
|
||||
rest empty, so `string[]` promises what the value does not keep: taking the first id, or
|
||||
building a map from the array, compiles and then misbehaves, and an empty string collides with
|
||||
another message's in a correlation table. The cost is that every consumer narrows, including
|
||||
the majority whose SMSC always names an id. Raised by the phase 11 product review, 2026-09-08.
|
||||
**This one is 1.0.0-or-never** — after release it needs a major version.
|
||||
- [ ] Tag `v1.0.0` to publish.
|
||||
- [ ] `npm deprecate larvitsmpp` pointing at `@larvit/smpp`. Maintainer's call to run it; not
|
||||
something CI should do.
|
||||
|
||||
Reference in New Issue
Block a user