Compare commits
2 Commits
fb139ab5ee
...
4c43f6748d
| Author | SHA1 | Date | |
|---|---|---|---|
| 4c43f6748d | |||
| 36d32591c3 |
@@ -0,0 +1,56 @@
|
|||||||
|
# Benchmarks
|
||||||
|
|
||||||
|
What this library sustains, and against what. Run them before setting or changing a scale goal.
|
||||||
|
|
||||||
|
```bash
|
||||||
|
docker compose run --rm node node benchmarks/run.ts # this library, both ends
|
||||||
|
COUNT=100000 docker compose run --rm node node benchmarks/run.ts # longer, steadier
|
||||||
|
```
|
||||||
|
|
||||||
|
`run.ts` spawns `smsc-sink.ts` — a server that answers every `submit_sm` `ESME_ROK` and stores
|
||||||
|
nothing — and drives it with `submit-load.ts` at four window sizes. Point `submit-load.ts` at any
|
||||||
|
SMSC to compare:
|
||||||
|
|
||||||
|
```bash
|
||||||
|
docker compose -f compose.yaml -f interop-tests/compose.jasmin.yaml run --rm node \
|
||||||
|
node benchmarks/submit-load.ts --host=jasmin --port=2775 --username=esme1 --password=esme1pw \
|
||||||
|
--count=20000 --concurrency=50
|
||||||
|
```
|
||||||
|
|
||||||
|
**A short run measures the JIT, not the library.** At 2,000 messages per point the same build
|
||||||
|
reported 16k/s where 100,000 messages reported 40k/s. Give each point several seconds.
|
||||||
|
|
||||||
|
## Results, 2026-09-20
|
||||||
|
|
||||||
|
Single host, 8 cores, Node 24.18.0 in the project container, loopback. Client and SMSC are separate
|
||||||
|
processes competing for the same CPUs, so these are a floor for split hosts.
|
||||||
|
|
||||||
|
`maxOutstanding` is the send window, and it is the setting that matters most:
|
||||||
|
|
||||||
|
| Window | msgs/s | with `dlr: true` |
|
||||||
|
| --- | --- | --- |
|
||||||
|
| 1 | 7,385 | 7,289 |
|
||||||
|
| 10 (default) | 25,358 | 24,282 |
|
||||||
|
| 50 | 37,125 | 37,502 |
|
||||||
|
| 200 | 40,046 | 39,730 |
|
||||||
|
|
||||||
|
Requesting delivery receipts costs nothing measurable at any window. A window of 1 — one request in
|
||||||
|
flight at a time — costs 5x, which is the round trip rather than the codec.
|
||||||
|
|
||||||
|
Against real peers, same driver, window 50, 20,000 messages:
|
||||||
|
|
||||||
|
| SMSC | msgs/s | failed |
|
||||||
|
| --- | --- | --- |
|
||||||
|
| this library's sink | 37,125 | 0 |
|
||||||
|
| Jasmin 0.10 | 2,207 | 0 |
|
||||||
|
| SMPPSim 3.0.0 | — | 19,000 of 20,000 |
|
||||||
|
|
||||||
|
Jasmin routes and persists where the sink does neither, so the gap is not an efficiency ratio
|
||||||
|
between two comparable things — what it establishes is that this library is not the bottleneck
|
||||||
|
against a production SMSC, by more than an order of magnitude. SMPPSim's store fills at roughly a
|
||||||
|
thousand messages and it then refuses the rest, so it cannot be loaded; that is a property of the
|
||||||
|
simulator, not a result.
|
||||||
|
|
||||||
|
`smppload`, the one purpose-built SMPP load generator among the peers, is blocked by a bind defect
|
||||||
|
of its own ([findings/07-load.md](../interop-tests/findings/07-load.md)), so no third-party load
|
||||||
|
tool drives these numbers.
|
||||||
@@ -0,0 +1,63 @@
|
|||||||
|
import { spawn } from 'node:child_process';
|
||||||
|
import { once } from 'node:events';
|
||||||
|
import { createInterface } from 'node:readline';
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Node rather than Python, unlike the repo's other standalone scripts: it spawns the two processes
|
||||||
|
* it measures, and they must run on the same runtime this library is measured under.
|
||||||
|
*/
|
||||||
|
const concurrencies = [1, 10, 50, 200];
|
||||||
|
const count = Number(process.env.COUNT ?? 5000);
|
||||||
|
|
||||||
|
function node(script: string, args: string[] = []) {
|
||||||
|
return spawn(process.execPath, [`${import.meta.dirname}/${script}`, ...args], {
|
||||||
|
stdio: ['ignore', 'pipe', 'inherit'],
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
|
async function firstLine(stream: NodeJS.ReadableStream): Promise<string> {
|
||||||
|
for await (const line of createInterface({ input: stream })) {
|
||||||
|
return line;
|
||||||
|
}
|
||||||
|
|
||||||
|
return '';
|
||||||
|
}
|
||||||
|
|
||||||
|
const sink = node('smsc-sink.ts');
|
||||||
|
const listening: unknown = JSON.parse(await firstLine(sink.stdout));
|
||||||
|
|
||||||
|
if (typeof listening !== 'object' || listening === null || !('port' in listening)) {
|
||||||
|
throw new Error('the sink did not report a port');
|
||||||
|
}
|
||||||
|
|
||||||
|
const port = String(listening.port);
|
||||||
|
const rows: Record<string, unknown>[] = [];
|
||||||
|
|
||||||
|
for (const dlr of [false, true]) {
|
||||||
|
for (const concurrency of concurrencies) {
|
||||||
|
const load = node('submit-load.ts', [
|
||||||
|
`--port=${port}`,
|
||||||
|
`--count=${String(count)}`,
|
||||||
|
`--concurrency=${String(concurrency)}`,
|
||||||
|
...(dlr ? ['--dlr'] : []),
|
||||||
|
]);
|
||||||
|
const reported: unknown = JSON.parse(await firstLine(load.stdout));
|
||||||
|
|
||||||
|
await once(load, 'exit');
|
||||||
|
|
||||||
|
if (typeof reported === 'object' && reported !== null) rows.push({ ...reported });
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
sink.kill('SIGTERM');
|
||||||
|
await once(sink, 'exit');
|
||||||
|
|
||||||
|
process.stdout.write(`\n${'dlr'.padEnd(6)}${'window'.padEnd(9)}${'msgs/s'.padEnd(10)}seconds\n`);
|
||||||
|
|
||||||
|
for (const row of rows) {
|
||||||
|
const dlr = String(row.dlr).padEnd(6);
|
||||||
|
const window = String(row.concurrency).padEnd(9);
|
||||||
|
const rate = String(row.perSecond).padEnd(10);
|
||||||
|
|
||||||
|
process.stdout.write(`${dlr}${window}${rate}${String(row.seconds)}\n`);
|
||||||
|
}
|
||||||
@@ -0,0 +1,33 @@
|
|||||||
|
import { server } from '../src/server.ts';
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Answers every submit_sm ESME_ROK and does nothing else, so a measurement against it reads this
|
||||||
|
* library's own ceiling rather than an SMSC's storage. Prints the bound port on stdout, then waits.
|
||||||
|
*/
|
||||||
|
const port = Number(process.env.PORT ?? 0);
|
||||||
|
const { err, server: smpp } = await server({ port });
|
||||||
|
|
||||||
|
if (err) {
|
||||||
|
process.stderr.write(`sink failed to listen: ${err.message}\n`);
|
||||||
|
process.exit(1);
|
||||||
|
}
|
||||||
|
|
||||||
|
let answered = 0;
|
||||||
|
|
||||||
|
smpp.on('session', session => {
|
||||||
|
session.on('sms', async sms => {
|
||||||
|
answered++;
|
||||||
|
await sms.sendResp();
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
smpp.on('serverError', reason => {
|
||||||
|
process.stderr.write(`sink serverError: ${reason.message}\n`);
|
||||||
|
});
|
||||||
|
|
||||||
|
process.stdout.write(`${JSON.stringify({ port: smpp.port })}\n`);
|
||||||
|
|
||||||
|
process.on('SIGTERM', () => {
|
||||||
|
process.stderr.write(`sink answered ${String(answered)}\n`);
|
||||||
|
void smpp.close({ signal: AbortSignal.abort() }).then(() => process.exit(0));
|
||||||
|
});
|
||||||
@@ -0,0 +1,72 @@
|
|||||||
|
import { client } from '../src/client.ts';
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Pushes `count` single-segment messages and reports what the wire carried per second. Keeps
|
||||||
|
* `concurrency` sends in flight so the send window, not the caller, is what bounds the rate.
|
||||||
|
*/
|
||||||
|
function arg(name: string, fallback: string): string {
|
||||||
|
const found = process.argv.find(one => one.startsWith(`--${name}=`));
|
||||||
|
|
||||||
|
return found === undefined ? fallback : found.slice(name.length + 3);
|
||||||
|
}
|
||||||
|
|
||||||
|
const host = arg('host', '127.0.0.1');
|
||||||
|
const port = Number(arg('port', '2775'));
|
||||||
|
const count = Number(arg('count', '2000'));
|
||||||
|
const concurrency = Number(arg('concurrency', '20'));
|
||||||
|
const message = arg('message', 'benchmark');
|
||||||
|
const dlr = process.argv.includes('--dlr');
|
||||||
|
const username = arg('username', 'user');
|
||||||
|
const password = arg('password', 'pass');
|
||||||
|
|
||||||
|
const { err, session } = await client({
|
||||||
|
host,
|
||||||
|
maxOutstanding: concurrency,
|
||||||
|
password,
|
||||||
|
port,
|
||||||
|
responseTimeout: 60_000,
|
||||||
|
username,
|
||||||
|
});
|
||||||
|
|
||||||
|
if (err) {
|
||||||
|
process.stdout.write(`${JSON.stringify({ error: err.message })}\n`);
|
||||||
|
process.exit(1);
|
||||||
|
}
|
||||||
|
|
||||||
|
// Narrowing from the guard above does not reach into worker(), which runs after it.
|
||||||
|
const bound = session;
|
||||||
|
|
||||||
|
let issued = 0;
|
||||||
|
let failed = 0;
|
||||||
|
let unanswered = 0;
|
||||||
|
|
||||||
|
async function worker(): Promise<void> {
|
||||||
|
while (issued < count) {
|
||||||
|
issued++;
|
||||||
|
|
||||||
|
const sent = await bound.sendSms({ dlr, from: 'BENCH', message, to: '46709771337' });
|
||||||
|
|
||||||
|
if (sent.err) failed++;
|
||||||
|
|
||||||
|
unanswered += sent.unanswered;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
const started = process.hrtime.bigint();
|
||||||
|
|
||||||
|
await Promise.all(Array.from({ length: concurrency }, () => worker()));
|
||||||
|
|
||||||
|
const seconds = Number(process.hrtime.bigint() - started) / 1e9;
|
||||||
|
|
||||||
|
process.stdout.write(`${JSON.stringify({
|
||||||
|
concurrency,
|
||||||
|
count,
|
||||||
|
dlr,
|
||||||
|
failed,
|
||||||
|
perSecond: Math.round(count / seconds),
|
||||||
|
seconds: Number(seconds.toFixed(3)),
|
||||||
|
unanswered,
|
||||||
|
})}\n`);
|
||||||
|
|
||||||
|
await bound.close({ signal: AbortSignal.abort() });
|
||||||
|
process.exit(0);
|
||||||
+5
-5
@@ -125,7 +125,7 @@ rule and an index of the titles below.
|
|||||||
phase: SMPP 3.4 5.3.2.32 makes the TLV the alternative for a body the mandatory field cannot
|
phase: SMPP 3.4 5.3.2.32 makes the TLV the alternative for a body the mandatory field cannot
|
||||||
carry, several SMSCs use it, and Jasmin relays one faithfully — reading `short_message` alone
|
carry, several SMSCs use it, and Jasmin relays one faithfully — reading `short_message` alone
|
||||||
handed the application an empty message
|
handed the application an empty message
|
||||||
([interop-tests/findings/03-jasmin.md](interop-tests/findings/03-jasmin.md)). `messageOctets()` is
|
([interop-tests/findings/03-jasmin.md](../interop-tests/findings/03-jasmin.md)). `messageOctets()` is
|
||||||
the single answer to where a body is, so the message path, the reassembler and `dlrFromPdu()`
|
the single answer to where a body is, so the message path, the reassembler and `dlrFromPdu()`
|
||||||
cannot disagree about it, and `esm_class` still says whether that body starts with a UDH wherever
|
cannot disagree about it, and `esm_class` still says whether that body starts with a UDH wherever
|
||||||
it was carried, which leaves concatenation reading exactly as before. Filling both contradicts the
|
it was carried, which leaves concatenation reading exactly as before. Filling both contradicts the
|
||||||
@@ -142,7 +142,7 @@ rule and an index of the titles below.
|
|||||||
`sar_msg_ref_num`/`sar_total_segments`/`sar_segment_seqnum` the other way to say what a UDH says,
|
`sar_msg_ref_num`/`sar_total_segments`/`sar_segment_seqnum` the other way to say what a UDH says,
|
||||||
Jasmin documents it as its own segmentation and jsmpp writes it, and reading the UDH alone handed
|
Jasmin documents it as its own segmentation and jsmpp writes it, and reading the UDH alone handed
|
||||||
the application one `sms` per fragment
|
the application one `sms` per fragment
|
||||||
([interop-tests/findings/05-java-clients.md](interop-tests/findings/05-java-clients.md)).
|
([interop-tests/findings/05-java-clients.md](../interop-tests/findings/05-java-clients.md)).
|
||||||
`concatOf()` is the single answer to how a PDU says it is a segment, as `messageOctets()` is to
|
`concatOf()` is the single answer to how a PDU says it is a segment, as `messageOctets()` is to
|
||||||
where a body is, and both are exported for the same reason: an application on the low-level
|
where a body is, and both are exported for the same reason: an application on the low-level
|
||||||
surfaces would otherwise rewrite the read this fixed. It carries the spelling beside the
|
surfaces would otherwise rewrite the read this fixed. It carries the spelling beside the
|
||||||
@@ -224,7 +224,7 @@ rule and an index of the titles below.
|
|||||||
ME-, SIM- and TE-specific classes immediate display and missed the 0xF0 group entirely — the only
|
ME-, SIM- and TE-specific classes immediate display and missed the 0xF0 group entirely — the only
|
||||||
one SMPP 3.4 5.2.19 names, since it marks 0x0F to 0xBF reserved and hands 0xF0 to 0xFF to GSM
|
one SMPP 3.4 5.2.19 names, since it marks 0x0F to 0xBF reserved and hands 0xF0 to 0xFF to GSM
|
||||||
03.38, and the one SMPPSim demonstrated
|
03.38, and the one SMPPSim demonstrated
|
||||||
([interop-tests/findings/02-smppsim.md](interop-tests/findings/02-smppsim.md), C17).
|
([interop-tests/findings/02-smppsim.md](../interop-tests/findings/02-smppsim.md), C17).
|
||||||
`messageClassOf()` is the single answer to whether a `data_coding` carries a class and which, as
|
`messageClassOf()` is the single answer to whether a `data_coding` carries a class and which, as
|
||||||
`concatOf()` is to how a PDU says it is a segment: 03.38 section 4 puts the class in bits 1-0,
|
`concatOf()` is to how a PDU says it is a segment: 03.38 section 4 puts the class in bits 1-0,
|
||||||
carried where bit 4 says so in every group below 0x80 and always in the 0xF0 group, and
|
carried where bit 4 says so in every group below 0x80 and always in the 0xF0 group, and
|
||||||
@@ -326,7 +326,7 @@ rule and an index of the titles below.
|
|||||||
Java-client interoperability phase: accepting any parse that merely did not error answered
|
Java-client interoperability phase: accepting any parse that merely did not error answered
|
||||||
`ESME_ROK` to a `deliver_sm` whose three trailing octets were never read, dropping the
|
`ESME_ROK` to a `deliver_sm` whose three trailing octets were never read, dropping the
|
||||||
`receipted_message_id` that makes a receipt a receipt
|
`receipted_message_id` that makes a receipt a receipt
|
||||||
([interop-tests/findings/05-java-clients.md](interop-tests/findings/05-java-clients.md)). Goal 2
|
([interop-tests/findings/05-java-clients.md](../interop-tests/findings/05-java-clients.md)). Goal 2
|
||||||
settles it against goal 3: octets this codec cannot name are a PDU it did not read, so a region
|
settles it against goal 3: octets this codec cannot name are a PDU it did not read, so a region
|
||||||
that does not end on `command_length` — the padded read included — is refused with the `tlvs`
|
that does not end on `command_length` — the padded read included — is refused with the `tlvs`
|
||||||
reason and `ESME_RINVTLVSTREAM` a truncated TLV value already gets. What the rule costs is paid
|
reason and `ESME_RINVTLVSTREAM` a truncated TLV value already gets. What the rule costs is paid
|
||||||
@@ -574,7 +574,7 @@ rule and an index of the titles below.
|
|||||||
Jasmin interoperability phase: Jasmin dispatches one `submit_sm` per connector at a time and will
|
Jasmin interoperability phase: Jasmin dispatches one `submit_sm` per connector at a time and will
|
||||||
not send segment 2 until segment 1 is answered, so holding a group unanswered until it was whole
|
not send segment 2 until segment 1 is answered, so holding a group unanswered until it was whole
|
||||||
deadlocked every multi-segment message against a production gateway
|
deadlocked every multi-segment message against a production gateway
|
||||||
([interop-tests/findings/03-jasmin.md](interop-tests/findings/03-jasmin.md)). Goal 1 has the answer
|
([interop-tests/findings/03-jasmin.md](../interop-tests/findings/03-jasmin.md)). Goal 1 has the answer
|
||||||
a real SMSC gives — one `message_id` per `submit_sm`, immediately — so the group's id base is
|
a real SMSC gives — one `message_id` per `submit_sm`, immediately — so the group's id base is
|
||||||
generated when it opens and each segment is answered `<base>-<n>`, the notation `sms-id.ts` owns
|
generated when it opens and each segment is answered `<base>-<n>`, the notation `sms-id.ts` owns
|
||||||
and `DlrMerger` reads back. The id is therefore fixed by the first segment, which is why an `smsId`
|
and `DlrMerger` reads back. The id is therefore fixed by the first segment, which is why an `smsId`
|
||||||
|
|||||||
@@ -9,8 +9,9 @@ govern it, and nothing here is a source anything else may cite.
|
|||||||
## Status
|
## Status
|
||||||
|
|
||||||
The rewrite is **feature complete and green**: the suite, lint and typecheck are clean on Node 18
|
The rewrite is **feature complete and green**: the suite, lint and typecheck are clean on Node 18
|
||||||
to 26, and 0.5.0 is on npm. What is left is housekeeping around the release, a few things worth
|
to 26, and 0.5.0 is on npm. 0.6.0 is next, and it is a quality cut rather than a feature one — the
|
||||||
adding, and the gaps a comparison with other SMPP libraries found.
|
comprehension gate and the defects under [0.6.0](#060) come first, then the gaps a comparison with
|
||||||
|
other SMPP libraries found.
|
||||||
|
|
||||||
## The agreed API
|
## The agreed API
|
||||||
|
|
||||||
@@ -177,6 +178,191 @@ the rewrite, for a dependency added later. Maintainer's call, 2026-09-14.
|
|||||||
- [x] [#8](https://github.com/larvit/larvitsmpp/issues/8) The socket's remote host and port on log
|
- [x] [#8](https://github.com/larvit/larvitsmpp/issues/8) The socket's remote host and port on log
|
||||||
messages: under Worth doing, not blocking. Maintainer's call, 2026-09-14.
|
messages: under Worth doing, not blocking. Maintainer's call, 2026-09-14.
|
||||||
|
|
||||||
|
## 0.6.0
|
||||||
|
|
||||||
|
A nine-reader comprehension panel read the whole project on 2026-09-20 and scored it 7 overall,
|
||||||
|
mean 6.8. Navigation (7–8) capped nobody. **Locality capped every unit reader at 5–6 and Shape
|
||||||
|
capped both architects at 6**, and those two are what this release lifts. The gate is 7 on all four
|
||||||
|
dimensions, higher where it is cheap. Maintainer's call, 2026-09-20. A systems-architect review the
|
||||||
|
same day returned ALIGN with one blocking-severity finding, which is the first item under Locality
|
||||||
|
and is also what the panel ranked hardest — two methods, one answer.
|
||||||
|
|
||||||
|
### Correctness, ahead of everything below
|
||||||
|
|
||||||
|
- [ ] **Read C-Octet Strings as `latin1`, so an address survives the wire.** `defs/types.ts` reads
|
||||||
|
with `toString('ascii')` at four sites and writes with `write(text, 'ascii')` at three. Node
|
||||||
|
masks bit 7 when decoding and not when encoding, so the codec writes `0xE9` and reads back
|
||||||
|
`0x69`: an inbound `source_addr` of `Kaffeé` reaches the application as `Kaffei`, with no raw
|
||||||
|
escape hatch as `short_message` has in `shortMessageOctets`. Affects `source_addr`,
|
||||||
|
`destination_addr`, `system_id`, `message_id`, `service_type` and the cstring TLVs, and makes
|
||||||
|
`objToPdu(pduToObj(x))` non-idempotent for them. Goals 1 and 3. Verified in the container:
|
||||||
|
`Buffer.from([0xE9]).toString('ascii')` is `'i'`.
|
||||||
|
|
||||||
|
- [ ] **Answer `alert_notification` and `outbind` by not answering them.** Both are response-less in
|
||||||
|
SMPP 3.4, both fall through `route()`'s default into `unhandled()`, which calls
|
||||||
|
`sendReturn(pduObj, 'ESME_RINVCMDID')`; `pduReturn()` then finds no response command, and the
|
||||||
|
failure reaches the application as `sessionError` on every occurrence. `alert_notification`
|
||||||
|
appears nowhere in `src/` but `defs/commands.ts`. One case arm each: log and return.
|
||||||
|
|
||||||
|
- [ ] **Range-check `maxOctets` with its five siblings.** `limitsOf()` in `session-options.ts`
|
||||||
|
covers `idleTimeout`, `maxOutstanding`, `maxReassembly`, `reassemblyTimeout`, `responseTimeout`
|
||||||
|
and `shutdownTimeout`; `maxOctets` is documented, consumed by `Reassembler`, and absent from
|
||||||
|
both that list and `CheckableOptions`. `server({ maxOctets: 0 })` starts, then refuses every
|
||||||
|
multipart message and reports each as lost traffic.
|
||||||
|
|
||||||
|
- [ ] **Read `multiple` in `parseTlvs()` and `writeTlvs()`, or delete it and `tlvMap`.** Five TLVs
|
||||||
|
declare `multiple: true` (`callback_num`, `callback_num_atag`, `callback_num_pres_ind`,
|
||||||
|
`broadcast_area_identifier`, `broadcast_error_status`) and nothing reads it; `parseTlvs()` keys
|
||||||
|
by tag name, so a peer sending two `callback_num` TLVs silently keeps the last. `tlvMap` on
|
||||||
|
`broadcast_sm_resp` is declared, set once and read nowhere. This is the "Dormant filters" row
|
||||||
|
of the 0.4.0 defect table in a new spelling — metadata that reads as a guarantee.
|
||||||
|
|
||||||
|
- [ ] **Arm the merge for the segments the SMSC did take, or say why not.** `collectSent()` sets
|
||||||
|
`failure` if any segment errored, including the `UnansweredError` a mid-send drop produces, and
|
||||||
|
`session.ts` only calls `dlrMerger.expect(sent.smsIds)` when `!sent.err`. So a link drop during
|
||||||
|
a multipart send leaves per-segment `dlr` events firing while `messageDlr` never can, traced
|
||||||
|
only by one `debug` line. `session-extras.test.ts` has the adjacent case — a drop *after* the
|
||||||
|
send — and not this one. If goal 2 forbids reporting on a message we cannot fully account for,
|
||||||
|
that is the answer; it is stated in no file today either way.
|
||||||
|
|
||||||
|
### Locality — 5–6 today, and the gate is 7
|
||||||
|
|
||||||
|
- [ ] **Give `IncomingRequests` a port instead of the `Session` it drives.** It holds its owner and
|
||||||
|
calls eight members of it 18 times, including `this.session.close()` on an inbound `unbind` —
|
||||||
|
a collaborator ending its owner's life. `OutgoingRequests` is the mirror half of the same
|
||||||
|
boundary and takes no session at all. AGENTS.md's "Nothing reaches back up" is false because of
|
||||||
|
this, and `docs/decisions.md` already states the rule under The session's life: "a collaborator
|
||||||
|
that has to ask does not own its decision". It is also the missing test seam — inbound routing,
|
||||||
|
reassembly dispatch, `onRequest` ordering and bind-direction refusal have no unit test because
|
||||||
|
the class cannot be built without a live socket. Carry the eight members as `IncomingDeps`,
|
||||||
|
exactly as `sendPastDrain` is carried now. No public surface changes. **Do this before the
|
||||||
|
store (goal 8), or the back-edge is baked into the store's published interface.**
|
||||||
|
|
||||||
|
- [ ] **Route `sms.ts` through its handlers, all of it.** `createSms()` already injects
|
||||||
|
`handlers.send`, and then reaches `sms.session.sendReturn()`, `sms.session.bindAllows()` and
|
||||||
|
`sms.session.acceptsOptionalParams()` anyway — two channels to one collaborator. `Sms.session`
|
||||||
|
stays public as data the application reads. The cheaper half of the item above, and the one
|
||||||
|
that shows the shape.
|
||||||
|
|
||||||
|
- [ ] **Give the held-message protocol one name and one home.** `emitSms()` is the unit 8 of 9
|
||||||
|
readers named and 4 would least want to modify, and every one proposed the same fix. It runs
|
||||||
|
five mechanisms in one scope: a hold keyed by array identity, a `working` counter seeded from
|
||||||
|
`listenerCount('sms')`, a `WeakMap` keyed by the `Sms` object, a `setImmediate`-deferred
|
||||||
|
release, and a captured `linkGeneration` — with the counter decremented from `session.ts`'s
|
||||||
|
`captureRejectionSymbol` in another file. A `MessageHold` owning `hold/release/listenerGaveUp`
|
||||||
|
collapses three files into one readable object. Every way of getting it wrong is silent: a hung
|
||||||
|
shutdown, or a receipt refused.
|
||||||
|
|
||||||
|
- [ ] **Derive `Reassembler`'s octet total instead of maintaining it at five sites.** `this.octets`
|
||||||
|
and each `group.octets` must agree, adjusted in `collect`, `trim`, `takeOldest`, `sweep` and
|
||||||
|
`clear`, and `collect()` discovers its own eviction by re-reading the map by identity. Push the
|
||||||
|
budget into `ExpiringGroups` as a weighed capacity, and have `trim()` report whether the
|
||||||
|
current group survived. Named by 6 of 9 readers.
|
||||||
|
|
||||||
|
- [ ] **Split the two questions `OutgoingRequests.linkDown()` answers.** `Session.drain()` calls it
|
||||||
|
twice for opposite conclusions — "nothing to drain, success" and "the link died under us,
|
||||||
|
failure" — and `outgoing-requests.ts` reads it a third way. Two named predicates. Named by 7
|
||||||
|
of 9 readers, who each reconstructed the ordering by hand.
|
||||||
|
|
||||||
|
- [ ] **Name `pastDrain()`'s retry condition and what makes the loop end.** The exit is a
|
||||||
|
three-term disjunction over two collaborators, whose comment covers the first term only, and
|
||||||
|
the method is named for what it bypasses. Do not change what it asks: `gate.isUp()` rather than
|
||||||
|
`linkDown()` is deliberate and recorded.
|
||||||
|
|
||||||
|
- [ ] **Replace `resolveBody`'s `settles` boolean with the decision it stands for.** One boolean
|
||||||
|
chooses both whether to overwrite `data_coding` and which params to read it from, across four
|
||||||
|
helpers all named some abstraction of "body". Return a named source — `'short_message' |
|
||||||
|
'payload' | 'caller'` — and branch once. Ranked hardest by three readers and picked by one as
|
||||||
|
the unit they would least want to touch, because a mistake here does not throw, does not fail
|
||||||
|
the types, and reaches the peer as somebody's message rendered wrong.
|
||||||
|
|
||||||
|
### Shape — 6 today, and the gate is 7
|
||||||
|
|
||||||
|
- [ ] **Group `src/` into a second level, and retire whichever record loses.** 34 files on one
|
||||||
|
plane, where `src/defs/` at 7 proves the shape is known one level down. `docs/decisions.md`
|
||||||
|
says "`src/` stays flat until a module has to move for another reason. Valid while that map is
|
||||||
|
what a reader navigates by" — and both architects reported that the map is now AGENTS.md rather
|
||||||
|
than the tree, which is that premise failing. `todo.md` already carries the opposite
|
||||||
|
instruction under Worth doing. Two records, opposite answers; one has to go. Do it in the same
|
||||||
|
change as the `IncomingRequests` port or the imports are rewritten twice.
|
||||||
|
|
||||||
|
- [ ] **Split `test/session-extras.test.ts` by the question each block answers.** 3,010 lines, 19
|
||||||
|
unrelated `describe` blocks whose names are already the file names they should be. With
|
||||||
|
`session.test.ts` it is 54% of all test code and 84% the size of `src/`. "extras" names neither
|
||||||
|
a question nor a module — it names the rest — and AGENTS.md's own convention forbids exactly
|
||||||
|
that. `max-lines` covers `src/**` only, so nothing has stopped it growing.
|
||||||
|
|
||||||
|
- [ ] **Collapse the three objects named `defaults`.** `client.ts`, `server.ts` and
|
||||||
|
`session-options.ts` each export or hold one; `port: 2775` is written twice and the idle
|
||||||
|
timeout is derived two ways to the same 40 000. "What is the default for X" has three answers
|
||||||
|
depending on the entrypoint, and nothing fails when they drift. Named by both architects as the
|
||||||
|
most likely first bug a new contributor ships.
|
||||||
|
|
||||||
|
- [ ] **Rename `EncodingName`'s `ASCII` to `GSM7`, with `ASCII` a deprecated alias for one minor.**
|
||||||
|
It is GSM 03.38, where `$` is 0x02 and `@` is 0x00, and `segmentUnits.ASCII = 153` is a septet
|
||||||
|
budget under a name that says octets. The 2026-09-09 decision removed `consts.ENCODING.ASCII`
|
||||||
|
for exactly this reason and left the option's own vocabulary carrying it. Pre-1.0 the minor is
|
||||||
|
the breaking unit, so this is as cheap as it will ever be, and `todo.md` already requires the
|
||||||
|
`consts.ENCODING` names settled before the custom-encoding registry — this is the other half.
|
||||||
|
|
||||||
|
- [ ] **Split `session-options.ts` into the things it is.** Option types and their validator, the
|
||||||
|
`SessionEvents` map, and the bind-direction rules (`bindCommands`, `bindTypeFromCommand`,
|
||||||
|
`standsInFor`, `bindCarries`) are three questions in one file, and the `defaults` table mixes
|
||||||
|
option defaults with four hard bounds that are not options. Both architects named it as where
|
||||||
|
the codebase rots first: at 34-wide it is where anything session-shaped lands.
|
||||||
|
|
||||||
|
- [ ] **Name the base-versus-segment distinction in the message id types.** `Sms.smsId` is a base,
|
||||||
|
`sendSms().smsIds[]` are segment ids, `Dlr.smsId` is a segment id and `MessageDlr.smsId` is a
|
||||||
|
base again — four fields, one type, `string`. The whole multipart receipt mechanism turns on
|
||||||
|
telling them apart and only `parseSegmentId()` knows.
|
||||||
|
|
||||||
|
### Self-sufficiency — 6–7 today, and the gate is 7
|
||||||
|
|
||||||
|
- [ ] **Move the one-line facts out of the decision log and back to the code.** Five of nine readers
|
||||||
|
independently reported being sent to `docs/decisions.md` for a question they hit while reading,
|
||||||
|
with no link from the code; one counted roughly fifty index redirects. The four worth inlining
|
||||||
|
as one line each: that `segmentUnits`' three numbers are in two units (septets and octets),
|
||||||
|
which body settles `data_coding`, that a receipt's body is read as octets whatever its
|
||||||
|
`data_coding` says, and the `<base>-<n>` id notation. The reasoning stays in the log; the
|
||||||
|
definition belongs at the code.
|
||||||
|
|
||||||
|
- [ ] **Document the two delivery-receipt merge bounds.** `maxDlrMerges` (1000) and
|
||||||
|
`dlrMergeTimeout` (24 h) are hardcoded, are not options, and appear in no README and no test —
|
||||||
|
while README states the equivalent held-message bounds explicitly ("Neither bound is an
|
||||||
|
option"). A sender with more than 1000 concurrent multipart `dlr: true` messages silently
|
||||||
|
evicts the oldest at `warn`. The inherited architect hit this on the 3am walk.
|
||||||
|
|
||||||
|
- [ ] **Add a ten-line SMPP glossary to the README.** Both juniors and the no-domain mid reported
|
||||||
|
the same largest cost: nothing in the repo says what a PDU, `esm_class`, `data_coding`, TON/NPI
|
||||||
|
or `submit_sm`-versus-`deliver_sm` are, and the inline spec citations mark a rule without
|
||||||
|
stating it. One of them put it at a third of their reading time. Four commands, three octets,
|
||||||
|
one sentence each.
|
||||||
|
|
||||||
|
### Doc claims this review falsified
|
||||||
|
|
||||||
|
- [ ] **Make "every README example is executed by the suite" true, or stop claiming it.** Goal 9 and
|
||||||
|
the Done table both promise it; `test/readme.test.ts` transcribes the examples by hand and has
|
||||||
|
drifted — 15 fenced `javascript` blocks in the README against 10 tests, and the test named "the
|
||||||
|
documented sending options" passes none of the five options the README's example passes. Read
|
||||||
|
the fenced blocks at test time and assert each appears verbatim in the executed source, so an
|
||||||
|
edit to either fails the gate.
|
||||||
|
|
||||||
|
- [ ] **Correct AGENTS.md's "Nothing reaches back up".** False while `IncomingRequests` holds a
|
||||||
|
`Session`: either the first Locality item makes it true, or the sentence names the exception
|
||||||
|
until it does.
|
||||||
|
|
||||||
|
- [ ] **Narrow the `src/defs/*` lint exemption to the four table files.** Its stated reason — "the
|
||||||
|
spec tables are data: their length tracks the specification, not any complexity" — is false for
|
||||||
|
`defs/types.ts`, which is 595 lines of wire codec with 25 functions and is the file that parses
|
||||||
|
hostile input from the network. It carries more over-budget methods than any other file in the
|
||||||
|
repo, under a suppression written for something else.
|
||||||
|
|
||||||
|
- [ ] **Run the interop suite before cutting a minor, and date the claim.** README states
|
||||||
|
"Interoperable. Tested as a client against Jasmin and SMPPSim, and as a server against Kannel,
|
||||||
|
jsmpp, Cloudhopper, python-smpplib and php-smpp" in the present tense; `interop-tests/README.md`
|
||||||
|
is honest that the run was 2026-09-08. Nothing runs the peers on a schedule or before a tag, so
|
||||||
|
the claim rots silently. Goals 1 and 9.
|
||||||
|
|
||||||
## Worth doing, not blocking
|
## Worth doing, not blocking
|
||||||
|
|
||||||
- [ ] **Cut the three teardown sentences `test/teardown.ts` already says.** Under AGENTS.md's
|
- [ ] **Cut the three teardown sentences `test/teardown.ts` already says.** Under AGENTS.md's
|
||||||
@@ -228,12 +414,6 @@ the rewrite, for a dependency added later. Maintainer's call, 2026-09-14.
|
|||||||
each Gitea pull request to GitHub for it to review there. Maintainer's ask, 2026-09-14; not
|
each Gitea pull request to GitHub for it to review there. Maintainer's ask, 2026-09-14; not
|
||||||
started until asked.
|
started until asked.
|
||||||
|
|
||||||
- [ ] **Group the session's collaborators under `src/session/`.** `session.ts` imports
|
|
||||||
`dlr-merger`, `incoming-requests`, `link-timers`, `outgoing-requests`, `pdu-transport`,
|
|
||||||
`reconnect-loop` and `send-sms`, and nothing else does, so the directory would make that
|
|
||||||
boundary visible. The `OutgoingRequests` extraction this was to be done with landed on
|
|
||||||
2026-09-01, so it is the remaining half. Raised by review, 2026-09-01.
|
|
||||||
|
|
||||||
- [ ] **`leftOf()` and the link gate's own budget are one concept counted twice.**
|
- [ ] **`leftOf()` and the link gate's own budget are one concept counted twice.**
|
||||||
`idle-waiters.ts` reads what is left of a budget as `Math.max(1, deadline - now)`, because 0
|
`idle-waiters.ts` reads what is left of a budget as `Math.max(1, deadline - now)`, because 0
|
||||||
means "forever" there; `link-gate.ts` runs the same subtraction and calls `<= 0` expired.
|
means "forever" there; `link-gate.ts` runs the same subtraction and calls `<= 0` expired.
|
||||||
|
|||||||
+1
-1
@@ -24,5 +24,5 @@
|
|||||||
"types": ["node"],
|
"types": ["node"],
|
||||||
"verbatimModuleSyntax": true
|
"verbatimModuleSyntax": true
|
||||||
},
|
},
|
||||||
"include": ["eslint.config.ts", "interop-tests", "src", "test"]
|
"include": ["benchmarks", "eslint.config.ts", "interop-tests", "src", "test"]
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user