Give the project ordered goals and let them decide what a decision record is
This commit is contained in:
@@ -1,7 +1,7 @@
|
|||||||
# AGENTS.md
|
# AGENTS.md
|
||||||
|
|
||||||
Guidance for LLM agents working in this repository. Human-facing documentation lives in
|
Guidance for LLM agents working in this repository. What each file in it is for is under
|
||||||
[README.md](README.md); the remaining work is tracked in [todo.md](todo.md).
|
[Documentation](#documentation).
|
||||||
|
|
||||||
## What this is
|
## What this is
|
||||||
|
|
||||||
@@ -10,7 +10,35 @@ started from an orphan commit — no history from 0.4.0 is carried over. The 0.4
|
|||||||
readable on the `master` branch of the same repository and is the reference for protocol behaviour,
|
readable on the `master` branch of the same repository and is the reference for protocol behaviour,
|
||||||
not for structure or style.
|
not for structure or style.
|
||||||
|
|
||||||
The library's value is its very small API. Do not grow the public surface without being asked.
|
## Goals
|
||||||
|
|
||||||
|
In priority order, and the order is the point: where two of them pull against each other, the earlier
|
||||||
|
one wins. They do not override the hard rules below.
|
||||||
|
|
||||||
|
1. **Correct on the wire.** SMPP 3.4 as SMSCs actually run it. Every other goal yields to this one;
|
||||||
|
the defect table below is what the alternative costs.
|
||||||
|
2. **Never give the application a wrong answer about what happened.** An outcome we cannot determine
|
||||||
|
is reported as undetermined rather than guessed; a request the peer may already have taken is
|
||||||
|
never re-sent on the library's own initiative; work the peer has no reason to send again is not
|
||||||
|
dropped.
|
||||||
|
3. **Strict in what we send, generous in what we read.** The library's own senders follow 3.4, and
|
||||||
|
the codec parses whatever arrives. Where the letter of the spec would discard traffic a real SMSC
|
||||||
|
sends, keep the traffic.
|
||||||
|
4. **A peer an operator never has to complain about.** No bind flooding, nothing a bind direction
|
||||||
|
forbids, no optional parameters to a peer that declared none, nothing held without a bound.
|
||||||
|
5. **The session layer is in here, and its defaults are what most applications should run.**
|
||||||
|
Keepalive, reconnect, the send window, reassembly and receipt correlation. An option retunes a
|
||||||
|
default or opts out of it; an option does not switch on the thing the caller obviously wanted.
|
||||||
|
6. **A small, stable public surface over reshapeable internals.** Only what `src/index.ts` exports is
|
||||||
|
published. A new option has to beat "the application can do this itself", and has to keep a
|
||||||
|
promise this library can verify. The low-level surface is a passthrough: policy binds what the
|
||||||
|
library composes, never what the caller wrote.
|
||||||
|
7. **Nothing that needs state wider than one session.** No throughput throttling, no persistence
|
||||||
|
across a restart, no coordination between processes. This is the scope floor, and it is why an
|
||||||
|
otherwise reasonable feature is declined without a fresh argument each time.
|
||||||
|
8. **It builds, tests and runs the same everywhere.** Container-only toolchain, no runtime
|
||||||
|
dependencies, the Node 18 floor verified in CI rather than asserted, every README example executed
|
||||||
|
by the suite.
|
||||||
|
|
||||||
## Hard rules
|
## Hard rules
|
||||||
|
|
||||||
@@ -99,9 +127,10 @@ docker compose run --rm node npm run build
|
|||||||
|
|
||||||
## Defects found in 0.4.0
|
## Defects found in 0.4.0
|
||||||
|
|
||||||
Confirmed by reading the 0.4.0 source. The rewrite fixes all of them; each needs a regression test
|
Every row names what 0.4.0's own code did, so it is not rebuilt here.
|
||||||
naming the behaviour, and the wire-affecting ones are cross-checked against a reference
|
[README.md](README.md#behaviour-that-changed-on-the-wire) names what changed for a consumer, and is
|
||||||
implementation (see todo.md).
|
the only place that does. Confirmed by reading the 0.4.0 source; each row has a regression test
|
||||||
|
naming the behaviour.
|
||||||
|
|
||||||
| Defect | 0.4.0 behaviour |
|
| Defect | 0.4.0 behaviour |
|
||||||
| --- | --- |
|
| --- | --- |
|
||||||
@@ -118,13 +147,14 @@ implementation (see todo.md).
|
|||||||
| Unbounded reassembly | Incomplete long-SMS groups are capped by nothing and swept only when other traffic arrives, after 24 hours |
|
| Unbounded reassembly | Incomplete long-SMS groups are capped by nothing and swept only when other traffic arrives, after 24 hours |
|
||||||
| Dead DLR aggregation | `longSmsDlrs` is allocated to merge per-segment receipts and then never used |
|
| Dead DLR aggregation | `longSmsDlrs` is allocated to merge per-segment receipts and then never used |
|
||||||
| Trailing NULL truncation | `types.buffer.size()` subtracts one whenever the value's last octet is `0x00`, so the PDU is allocated one octet short while `sm_length` still reports the full length. Any UCS2 message ending in a character like U+4E00 or U+3000 goes out corrupt |
|
| Trailing NULL truncation | `types.buffer.size()` subtracts one whenever the value's last octet is `0x00`, so the PDU is allocated one octet short while `sm_length` still reports the full length. Any UCS2 message ending in a character like U+4E00 or U+3000 goes out corrupt |
|
||||||
| Dormant filters | `defs.filters` is declared on commands and TLVs but never invoked anywhere. Dropped in the rewrite; SMPP time formatting is exported as `smppTime` instead |
|
| Dormant filters | `defs.filters` is declared on commands and TLVs but never invoked anywhere |
|
||||||
| Unchecked reads | Wire reads index straight into the buffer, so a short or malformed PDU throws out of the codec. Reads are bounds-checked and return results now |
|
| Unchecked reads | Wire reads index straight into the buffer, so a short or malformed PDU throws out of the codec |
|
||||||
| Unrangechecked writes | Integer params are handed to `writeUInt8`/`writeUInt16BE` unvalidated, so an out-of-range value throws from inside Node |
|
| Unrangechecked writes | Integer params are handed to `writeUInt8`/`writeUInt16BE` unvalidated, so an out-of-range value throws from inside Node |
|
||||||
| `submit_multi` missing `sm_length` | The field is commented out of the command table, so `short_message` never round-trips for that command |
|
| `submit_multi` missing `sm_length` | The field is commented out of the command table, so `short_message` never round-trips for that command |
|
||||||
| Per-parameter defaults never applied | `calcCmdLength` reads `paramType.default` (the wire type's) rather than the parameter's, so `interface_version: 0x50` on the bind commands did nothing and every bind declared version 0x00 |
|
| Per-parameter defaults never applied | `calcCmdLength` reads `paramType.default` (the wire type's) rather than the parameter's, so `interface_version: 0x50` on the bind commands did nothing and every bind declared version 0x00 |
|
||||||
| `source_telematics_id` width | Defined as a 2-octet integer; SMPP 3.4 5.3.2.8 makes it 1 octet, unlike `dest_telematics_id`, which really is 2 |
|
| `source_telematics_id` width | Defined as a 2-octet integer; SMPP 3.4 5.3.2.8 makes it 1 octet, unlike `dest_telematics_id`, which really is 2 |
|
||||||
| Binary payloads decoded as text | `data_coding` 0x02, 0x04, 0x14 and 0xF4-0xF7 are 8-bit binary and land on the GSM 03.38 table, which rewrites every octet outside it. They resolve to LATIN1 now, so the payload survives as bytes |
|
| Binary payloads decoded as text | `data_coding` 0x02, 0x04, 0x14 and 0xF4-0xF7 are 8-bit binary and land on the GSM 03.38 table, which rewrites every octet outside it |
|
||||||
|
| Binary TLVs round-trip corrupt | `pduToObj` turns a `Buffer` TLV value into a hex string (`utils.js:307`), and `objToPdu` writes that string back as its own ASCII, so `message_payload`, `network_error_code`, `callback_num` and the rest are destroyed by any round trip |
|
||||||
| `ESME_RINVBCASTCHANIND` typo | Defined as `0x011`, three hex digits; the spec value is `0x0112` |
|
| `ESME_RINVBCASTCHANIND` typo | Defined as `0x011`, three hex digits; the spec value is `0x0112` |
|
||||||
|
|
||||||
## Multipart sends and the send window
|
## Multipart sends and the send window
|
||||||
@@ -170,61 +200,186 @@ exactly 140.
|
|||||||
- `assert.equal` from `node:assert/strict` narrows its first argument, so a following `?.` on the
|
- `assert.equal` from `node:assert/strict` narrows its first argument, so a following `?.` on the
|
||||||
same value is flagged as unnecessary. Assert once with `assert.ok(x)` and use plain access after.
|
same value is flagged as unnecessary. Assert once with `assert.ok(x)` and use plain access after.
|
||||||
|
|
||||||
|
## Documentation
|
||||||
|
|
||||||
|
Each file answers one question, and a fact belongs to the file whose question it answers:
|
||||||
|
|
||||||
|
- **README.md — what you can rely on.** Observable behaviour, for someone using the package. It
|
||||||
|
carries a reason only where the reason changes how you would call the thing.
|
||||||
|
- **AGENTS.md — what may not change, and why.** Goals, hard rules, architecture, conventions, and the
|
||||||
|
decisions the goals do not already settle. It does not restate behaviour README states.
|
||||||
|
- **todo.md** is a temporary working file that sets its own rules; nothing here governs it.
|
||||||
|
|
||||||
|
A sentence living in two of them is a defect: delete the copy in the file whose question it does not
|
||||||
|
answer. The toolchain commands are the one deliberate exception — README's copy serves a contributor
|
||||||
|
who never opens this file, and this file's copy carries the constraint that nothing runs on the host.
|
||||||
|
|
||||||
|
**Write a decision down only when it cannot be put better as a goal.** A goal decides every case that
|
||||||
|
follows from it; a decision record decides one. So reach for the goal list first — sharpen a goal,
|
||||||
|
add one, or move one up the order — and write a decision only for what is left over: a choice a
|
||||||
|
competent change would otherwise re-open, that no goal implies. Give the claim, the constraint that
|
||||||
|
settled it and the alternative rejected, and nothing the code or README already says. Where a
|
||||||
|
compiler or a test already forbids the other way, it is not a decision, it is a test name. Delete one
|
||||||
|
once it no longer constrains anything; this is not a changelog.
|
||||||
|
|
||||||
## Decisions
|
## Decisions
|
||||||
|
|
||||||
|
Grouped by what each one constrains.
|
||||||
|
|
||||||
|
### The public surface
|
||||||
|
|
||||||
|
- **`Session` is publicly constructible, which is what makes `SessionOptions` and `ReconnectOptions`
|
||||||
|
public too.** Raised twice as a leak; it is not one. The collaborators `session.ts` delegates to
|
||||||
|
(`Reassembler`, `PendingRequests`, `SendWindow`, `ReconnectLoop`, `LinkTimers`, `LinkGate`,
|
||||||
|
`DlrMerger`, `PduTransport`, `submitSms`) stay unpublished so they can be reshaped.
|
||||||
|
|
||||||
|
- **`acceptsOptionalParams()` and `bindAllows()` are predicates, not chokepoints.** The library's own
|
||||||
|
senders consult them; `session.send({ tlvs })` is passed through as written, because silently
|
||||||
|
stripping a caller's explicit TLVs off a deliberately public low-level surface would be worse than
|
||||||
|
sending them. Only `submit_sm` and `deliver_sm` are policed by bind direction — the only two the
|
||||||
|
library sends and dispatches by it.
|
||||||
|
|
||||||
|
- **`session.sock` is a getter over `PduTransport`.** Reading it is unchanged; assigning it no longer
|
||||||
|
compiles, which never rewired the handlers and so never worked.
|
||||||
|
|
||||||
|
- **Both emitters re-declare their listener methods to accept a promise.** Maintainer's call,
|
||||||
|
2026-08-27: `EventEmitter` types every listener as void-returning, so the
|
||||||
|
`session.on('sms', async sms => …)` README documents reads as a misused promise in any strict
|
||||||
|
consumer. `declare on: …` and its six siblings re-type the inherited methods to return `unknown`,
|
||||||
|
which emits nothing and needs no cast; overriding them as real methods cannot work, because the
|
||||||
|
`super.on()` call needs one. The cost is that a subclass can no longer reach those seven through
|
||||||
|
`super` — re-declaring them the same way is its way out. `unknown` rather than
|
||||||
|
`void | Promise<void>` because a listener may return anything: `session.on('close', () =>
|
||||||
|
set.delete(session))` returns a boolean.
|
||||||
|
|
||||||
|
### The wire
|
||||||
|
|
||||||
|
- **The declared interface version is an option on both `client()` and `server()`, and is not the
|
||||||
|
optional-parameter threshold.** That threshold is fixed at 0x34 by the spec, so an implementation
|
||||||
|
that must declare 5.0 throughout can, without moving it.
|
||||||
|
|
||||||
|
- **A peer that declared no version is pre-3.4, and `undefined` means no bind yet.** `acceptBind()`
|
||||||
|
records what the ESME declared and the client's `bind()` records the `sc_interface_version` the
|
||||||
|
SMSC answered with; a peer that declared nothing is recorded as `undeclaredInterfaceVersion` (0x00)
|
||||||
|
and sent no optional parameters, which is how the spec reads an absent `sc_interface_version`.
|
||||||
|
|
||||||
|
- **`esm_class` decides what a `deliver_sm` is, and the body is read only when it names nothing.**
|
||||||
|
`MC_DELIVERY_RECEIPT` (0x04) makes it a receipt whatever the body parses to, so a receipt in a
|
||||||
|
format `dlrFromPdu()` cannot read reaches `dlr` with `smsId` undefined instead of arriving as an
|
||||||
|
inbound SMS. Any other named type is not a receipt and its body is not scraped; a message type of 0
|
||||||
|
or one of the ten reserved keeps the scrape, and a non-empty `receipted_message_id` TLV marks a
|
||||||
|
receipt on the same footing. A receipt this library recognises never reaches the reassembler, so an
|
||||||
|
SMSC that splits one across segments gets a `dlr` per segment rather than one merged report. The
|
||||||
|
`message_state` TLV is authoritative only where it names a state in the table — SMPP reserves
|
||||||
|
0x80-0xFF for MC-vendor-specific values, so an unnameable one keeps its raw `statusId` and leaves
|
||||||
|
`statusMsg` to the body.
|
||||||
|
|
||||||
|
- **`smsIdFormat` names a notation per place, and normalisation never reaches inside a `<base>-<n>`
|
||||||
|
id.** An SMSC may answer `submit_sm_resp` in hex and write the receipt's `id:` in decimal, so one
|
||||||
|
transform over both sides cannot make them equal. `submitResp` covers the `receipted_message_id`
|
||||||
|
TLV too, which SMPP 3.4 5.3.2.26 defines as the id the `submit_sm_resp` carried: naming one
|
||||||
|
notation for whichever id a receipt yields would break the peer that sends both. Omitting a place
|
||||||
|
is what leaving it alone means, so there is no `raw` notation, and a caller-supplied formatter is
|
||||||
|
refused because it would make the promise that the two ids are comparable unverifiable — `onRequest`
|
||||||
|
and the PDU on the `dlr` event are the escape hatches. A `<base>-<n>` id parses as no number and so
|
||||||
|
reaches `expect()` and `collect()` unchanged, which is what keeps `DlrMerger` working; normalising
|
||||||
|
the base instead would break that pair. The option is on `client()` only, since a `server()` session
|
||||||
|
writes both ids itself.
|
||||||
|
|
||||||
|
### The session's life
|
||||||
|
|
||||||
- **A close arriving after our own `unbind` is a clean unbind, not an error.** Maintainer's call,
|
- **A close arriving after our own `unbind` is a clean unbind, not an error.** Maintainer's call,
|
||||||
2026-08-26: most SMSCs drop the socket instead of answering, so the documented shutdown would
|
2026-08-26: most SMSCs drop the socket instead of answering, so the documented shutdown would
|
||||||
otherwise always report a failure. It does mask a socket that died mid-unbind for an unrelated
|
otherwise always report a failure. It does mask a socket that died mid-unbind for an unrelated
|
||||||
reason, which is accepted — the peer sees the same TCP close either way.
|
reason, which is accepted — the peer sees the same TCP close either way.
|
||||||
- **The published surface is frozen at what `src/index.ts` exports today.** `Session` is exported and
|
|
||||||
publicly constructible, which is why `SessionOptions` and `ReconnectOptions` are public too — that
|
|
||||||
is correct, not a leak, and it has been raised twice. The collaborators `session.ts` delegates to
|
|
||||||
(`Reassembler`, `PendingRequests`, `SendWindow`, `ReconnectLoop`, `LinkTimers`, `LinkGate`,
|
|
||||||
`DlrMerger`, `submitSms`) stay unpublished so they can be reshaped.
|
|
||||||
- **The sub-3.4 optional-parameter rule is a predicate, not a chokepoint.** `acceptsOptionalParams()`
|
|
||||||
is consulted by the library's own senders; `session.send({ tlvs })` is passed through as written,
|
|
||||||
because silently stripping a caller's explicit TLVs off a deliberately public low-level surface
|
|
||||||
would be worse than sending them. The guarantee is "what this library sends honours the rule",
|
|
||||||
never "the session cannot send optional parameters to an old peer".
|
|
||||||
- **Both ends feed `peerInterfaceVersion`, and a peer that declared nothing is pre-3.4.**
|
|
||||||
`acceptBind()` records what the ESME declared in its bind request; the client's `bind()` records
|
|
||||||
the `sc_interface_version` the SMSC answered with. A peer that declared no version is recorded as
|
|
||||||
`undeclaredInterfaceVersion` (0x00) and is sent no optional parameters — the spec reads an absent
|
|
||||||
`sc_interface_version` as an SMSC that supports none. `undefined` is left to mean one thing only:
|
|
||||||
no bind has been accepted on this session yet.
|
|
||||||
- **The library speaks SMPP 3.4 on the wire, and `defs/` keeps the 5.0 tables as a superset.**
|
|
||||||
Maintainer's call, 2026-08-26: 3.4 is what SMSCs actually run, while the wider tables let the codec
|
|
||||||
parse and build whatever a peer sends. The declared version is an option on both `client()` and
|
|
||||||
`server()`, so an implementation that needs 5.0 throughout can have it. The threshold at or above
|
|
||||||
which a peer may be sent optional parameters is fixed at 0x34 by the spec and is not the same
|
|
||||||
constant as the declared version.
|
|
||||||
- **Bind direction is enforced on the library's own senders and on everything incoming, not on
|
|
||||||
`send()`.** A receiver-bound ESME carries no `submit_sm` and a transmitter-bound one no
|
|
||||||
`deliver_sm`; `sendSms()` and `sendDlr()` refuse locally, and an arriving PDU is answered
|
|
||||||
`ESME_RINVBNDSTS`. `bindAllows()` is a predicate on the same footing as `acceptsOptionalParams()`,
|
|
||||||
so the deliberately public low-level `send()` stays a passthrough. Only those two commands are
|
|
||||||
policed, because they are the only ones the library sends and dispatches by direction.
|
|
||||||
|
|
||||||
- **The logger is a five-method contract this library declares, not a dependency.** `SmppLog` in
|
- **`close` means the session is over, and a drop the loop will retry is `disconnected`.**
|
||||||
`log.ts` is what the code actually calls (`debug`, `error`, `info`, `verbose`, `warn`), so an
|
Maintainer's call, 2026-08-31: without the split, an application that opens a replacement client on
|
||||||
application can satisfy it with an object literal and `@larvit/smpp` ships with no runtime
|
`close` ends up holding two binds on one account. `teardown()` picks the event by whether the
|
||||||
dependencies. `@larvit/log` implements it structurally and stays a devDependency, where
|
reconnect loop is still live, and `end()` stops that loop before tearing down, so every deliberate
|
||||||
`test/tls.test.ts` passing a real `Log` as the server's logger keeps that compatibility compiled.
|
shutdown emits `close`. A retry that opens a socket and then loses it clears `closed` through
|
||||||
|
`attach()`, which is why a second drop emits again.
|
||||||
|
|
||||||
- **`esm_class` decides what a `deliver_sm` is, and the body is only read when it names nothing.**
|
- **`reconnect` takes `{ minDelay, maxDelay }` to retune and `false` to turn off**, so absent means
|
||||||
Message type `MC_DELIVERY_RECEIPT` (0x04) makes it a receipt whatever the body parses to, so a
|
on and there is one spelling for each. Only `client()` reconnects — a `server()` session is a
|
||||||
receipt in a format `dlrFromPdu()` cannot read reaches `dlr` with `smsId` undefined instead of
|
connection the peer opened, and nothing at this end can reopen it. The retry timer is `unref()`'d,
|
||||||
arriving as an inbound SMS. Any other named type — delivery or user acknowledgement, conversation
|
so a process with nothing else left to do still exits between attempts.
|
||||||
abort, intermediate notification — is not a receipt and its body is not scraped. A message type of
|
|
||||||
0, or one of the ten the spec reserves, keeps the scrape: SMSCs that send text-only receipts leave
|
- **Coming up is not proof a link works, so only one that outlasted `maxDelay` resets the backoff.**
|
||||||
`esm_class` at 0, and reading that as the spec's "default message type" would lose every one of
|
An unreadable stream is found after the bind returns, so resetting on connect gave a link that died
|
||||||
them. A non-empty `receipted_message_id` TLV marks a receipt on the same footing there, since
|
on arrival a fresh `minDelay` every cycle — one TCP connect and bind per second, forever. A drop
|
||||||
nothing but a receipt carries one. What gets scraped is the decoded `short_message` with any UDH
|
after a healthy link still retries at `minDelay`.
|
||||||
stripped; a receipt this library recognises never reaches the reassembler, so an SMSC that splits one across segments gets a
|
|
||||||
`dlr` per segment rather than one merged report.
|
- **A stream this library cannot read is a dead link, not a dead session.** Maintainer's call,
|
||||||
The `message_state` TLV is authoritative only where it names a state in the table — SMPP reserves
|
2026-08-31: a framing or codec error tears the link down through `teardown()`, so the reconnect
|
||||||
0x80-0xFF for MC-vendor-specific values, so an unnameable one keeps its raw `statusId` and leaves
|
loop retries it on a fresh socket with a fresh framer — which is what a desynced stream needs, and
|
||||||
`statusMsg` to the body.
|
the common cause. `sessionError` still carries every failure, so a peer that only ever sends
|
||||||
|
garbage is visible in the log rather than silent.
|
||||||
|
|
||||||
|
- **A deliberate shutdown drains; an unusable link and an abort do not.** `close()` and `unbind()`
|
||||||
|
wait on the send window rather than the pending map — the map misses a segment still queued behind
|
||||||
|
a full window, and finishing a half-sent multipart message is the point. The window counts slots,
|
||||||
|
never outcomes, and empties on a drop too, where `teardown()` settles everything the link was
|
||||||
|
carrying, which is why `drain()` reads `closed` before it reads the count. A stream the framer or
|
||||||
|
the codec cannot read takes `teardown()` instead, and `close({ signal })` on an aborted signal and
|
||||||
|
a peer's own `unbind` take `end()`: nothing on a dead link can answer, an abort means stop now, and
|
||||||
|
a peer that has declared itself finished will not answer what it still owes, so draining any of the
|
||||||
|
three would only hold a socket open for the timeout. `unbind()` sends its own PDU through
|
||||||
|
`request()` past both the window and the drain gate, because it must go out either way.
|
||||||
|
`shutdownTimeout` stays a session option rather than a `close()` argument: `server()` builds
|
||||||
|
sessions on the caller's behalf, so the option is the only composition point. `SmppServer.close()`
|
||||||
|
reports each session's unfinished drain through `serverError`, because its own result says nothing
|
||||||
|
but that the listener stopped.
|
||||||
|
|
||||||
|
- **A reconnect keeps the delivery-receipt merges; everything else the link held is dropped.**
|
||||||
|
`onDeliverSm()` answers each receipt before the group it belongs to is complete, and `teardown()`
|
||||||
|
runs on every path — an idle timeout and a failed rebind, not only `close()` — so clearing the
|
||||||
|
merges there loses receipts no peer has a reason to send again. They are cleared where the session
|
||||||
|
is over instead. Inbound segments stay in `teardown()`, because they go unanswered until the
|
||||||
|
message is whole: the peer still holds them, and answering it on a later link with the old
|
||||||
|
segments' sequence numbers would correlate with nothing.
|
||||||
|
|
||||||
|
- **A message id base is merged at most once.** A receipt carries nothing but `<base>-<n>`, so a
|
||||||
|
straggler for a message whose group is gone cannot be told from a receipt for a later message the
|
||||||
|
peer handed the same ids — an SMSC whose id counter restarts with its process is the realistic
|
||||||
|
case. `DlrMerger` remembers the bases it has finished with, capped and expiring exactly like the
|
||||||
|
groups, and refuses to open one a second time: the later message gets no `messageDlr`, and an
|
||||||
|
earlier one whose receipts are still arriving is dropped rather than left to collect the later
|
||||||
|
one's. Every segment still reaches the application as a `dlr`. `expect()` ignores a lone id, so a
|
||||||
|
single-part message never claims a base.
|
||||||
|
|
||||||
|
- **A send that never reached the socket waits for the next link; one that did is counted, not
|
||||||
|
resent.** Maintainer's call, 2026-09-01: re-queueing everything unanswered would resend a
|
||||||
|
`submit_sm` the SMSC accepted and answered into a dead socket, which is delivered and billed twice,
|
||||||
|
while a request that never left this process can be lost for free. `attempt()` therefore wraps all
|
||||||
|
three ways a written request can fail in `UnansweredError`; counting only the dropped-link case, as
|
||||||
|
the first cut did, would have called the commonest one safe to resend. A count rather than a
|
||||||
|
boolean because `sendSms()` aggregates segments into one `err` slot, and required rather than
|
||||||
|
optional so every construction site answers. `UnansweredError` stays unexported: `unanswered` is
|
||||||
|
the one spelling on the public surface. The hold is bounded by `responseTimeout` rather than an
|
||||||
|
option of its own — that is already the answer to how long one request may wait — and its clock
|
||||||
|
starts when the send is issued rather than when it first finds the gate shut, so one budget covers
|
||||||
|
every hold a single call makes. That timer is the one here that is not `unref()`'d: a held request
|
||||||
|
is awaited with the socket already destroyed, so an unref'd one lets a process whose only remaining
|
||||||
|
work is that send exit without settling it.
|
||||||
|
|
||||||
|
- **The gate decides whether a link can carry a request, and a bind is what makes it one.**
|
||||||
|
Maintainer's call, 2026-09-01: `attach()` clears `closed` the moment a socket is handed over, one
|
||||||
|
round trip before the bind is answered, so gating on `closed` let a send arriving in that window go
|
||||||
|
out unbound and come back `ESME_RINVBNDSTS`. `LinkGate` owns the answer instead — `shut(returning)`
|
||||||
|
on every teardown, `open()` only once `comeBackUp()` has a bound link — and `Session.linkDown()`
|
||||||
|
reads it rather than `closed`. The bind itself cannot wait for what it creates, so `send()` lets the
|
||||||
|
three bind commands past the gate and the window, the same door `unbind()` takes through
|
||||||
|
`attempt()`. The gate is told what happened and never reads back into the session: a collaborator
|
||||||
|
that has to ask does not own its decision, which is how the first cut ended up answering the same
|
||||||
|
question two different ways at admit and at release. For the same reason the retry in `send()` asks
|
||||||
|
`gate.isUp()` rather than `linkDown()`, which also reads the socket — a condition that loops on
|
||||||
|
something the gate does not gate on spins against a gate that admits it straight back.
|
||||||
|
`LinkGate.returning` is a copy of `retrying()` taken at teardown, and stays true 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.
|
||||||
|
|
||||||
|
### Internals and tests
|
||||||
|
|
||||||
- **A listener that rejects is routed by Node's `captureRejections`, not by hand-dispatching.** Both
|
- **A listener that rejects is routed by Node's `captureRejections`, not by hand-dispatching.** Both
|
||||||
emitters construct with `captureRejections: true` and implement
|
emitters construct with `captureRejections: true` and implement
|
||||||
@@ -235,158 +390,13 @@ exactly 140.
|
|||||||
handlers normalise through `errorFrom()` rather than inline — a route out of the handler would land
|
handlers normalise through `errorFrom()` rather than inline — a route out of the handler would land
|
||||||
on a bare `process.nextTick` with nothing to catch it.
|
on a bare `process.nextTick` with nothing to catch it.
|
||||||
|
|
||||||
- **Both emitters re-declare their listener methods to accept a promise.** Maintainer's call,
|
- **`SmppLog` is a five-method contract this library declares, not a dependency.** `debug`, `error`,
|
||||||
2026-08-27: `EventEmitter` types every listener as void-returning, so the
|
`info`, `verbose` and `warn` are what the code actually calls, so an application can satisfy it
|
||||||
`session.on('sms', async sms => …)` the README documents reads as a misused promise in any strict
|
with an object literal. `@larvit/log` implements it structurally and stays a devDependency, where
|
||||||
consumer. `declare on: …` and its six siblings re-type the inherited methods to return `unknown`,
|
`test/tls.test.ts` passing a real `Log` as the server's logger keeps that compatibility compiled.
|
||||||
which emits nothing, needs no cast and leaves the runtime method on the prototype. Overriding them
|
|
||||||
as real methods instead cannot work: the `super.on()` call needs a cast to satisfy the conditional
|
|
||||||
`Listener` type. The cost is that a subclass can no longer reach those seven through `super` or
|
|
||||||
override them as methods — re-declaring them the same way is its way out. `unknown` rather than `void | Promise<void>` because a listener may return
|
|
||||||
anything — `session.on('close', () => set.delete(session))` returns a boolean.
|
|
||||||
|
|
||||||
- **A reconnect keeps the delivery-receipt merges; everything else the link held is dropped.**
|
|
||||||
`onDeliverSm()` answers each receipt before the group it belongs to is complete, and `teardown()`
|
|
||||||
runs on every path — an idle timeout and a failed rebind, not only `close()` — so clearing the
|
|
||||||
merges there loses receipts no peer has a reason to send again. They are cleared where the session
|
|
||||||
is over instead: `close()`, or a drop with no reconnect loop left to bring the link back. Inbound
|
|
||||||
segments stay in `teardown()`, because they go unanswered until the message is whole: the peer
|
|
||||||
still holds them, and answering it on a later link with the old segments' sequence numbers would
|
|
||||||
correlate with nothing. Surviving a process restart is a separate, public-surface question, and is
|
|
||||||
in todo.md.
|
|
||||||
|
|
||||||
- **A message id base is merged at most once.** A receipt carries nothing but `<base>-<n>`, so a
|
|
||||||
straggler for a message whose group is gone cannot be told from a receipt for a later message the
|
|
||||||
peer handed the same ids — an SMSC whose id counter restarts with its process is the realistic
|
|
||||||
case. `DlrMerger` remembers the bases it has finished with, capped and expiring exactly like the
|
|
||||||
groups, and refuses to open one a second time: the later message gets no `messageDlr`, and an
|
|
||||||
earlier one whose receipts are still arriving is dropped rather than left to collect the later
|
|
||||||
one's. Every segment still reaches the application as a `dlr`. The rule covers the bases the
|
|
||||||
merger opened — `expect()` ignores a lone id, so a single-part message never claims one.
|
|
||||||
|
|
||||||
- **A deliberate shutdown drains; an unusable link and an abort do not.** `close()` and `unbind()`
|
|
||||||
refuse further sends and wait on the send window, not the pending map — the map misses a segment
|
|
||||||
still queued behind a full window, and finishing a half-sent multipart message is the point. The
|
|
||||||
window counts slots, never outcomes, so it says when to stop waiting and nothing about what
|
|
||||||
happened: it empties on a drop too, where `teardown()` settles everything the link was carrying,
|
|
||||||
which is why `drain()` reads `closed` before it reads the count. The wait covers what this end
|
|
||||||
sent — a request the peer sent us is answered through `sendReturn()`, which never enters the
|
|
||||||
window, so a server session waits for none of its inbound work; that half is in todo.md.
|
|
||||||
`shutdownTimeout` bounds the drain alone, and `0` waits forever like every other timeout here;
|
|
||||||
`unbind()` then waits `responseTimeout` for its own response, and sends that PDU through
|
|
||||||
`request()` past both the window and the drain gate because it must go out either way. A stream
|
|
||||||
the framer or the codec cannot read takes `teardown()` instead, and `close({ signal })` on an
|
|
||||||
aborted signal and a peer's own `unbind` take `end()` — nothing on a dead link can answer, an abort
|
|
||||||
means stop now, and a peer that has declared itself finished will not answer what it still owes us,
|
|
||||||
so draining any of the three would only hold a socket open for the timeout. `SmppServer.close()`
|
|
||||||
reports each session's unfinished drain through `serverError`, because its own result says
|
|
||||||
nothing but that the listener stopped. `shutdownTimeout` stays a session option rather than a
|
|
||||||
`close()` argument: `server()` builds sessions on the caller's behalf, so the option
|
|
||||||
is the only composition point, and `close({ signal })` already covers a hard deadline.
|
|
||||||
|
|
||||||
- **The TLS tests build their own self-signed certificate in DER** (`test/tls.test.ts`) instead of
|
- **The TLS tests build their own self-signed certificate in DER** (`test/tls.test.ts`) instead of
|
||||||
adding a devDependency or shelling out to openssl. Maintainer's call, 2026-08-26: the dev image
|
adding a devDependency or shelling out to openssl. Maintainer's call, 2026-08-26: the dev image
|
||||||
`node:24.18.0-bookworm-slim` ships no openssl binary, so a shelled-out fixture would pass in CI
|
`node:24.18.0-bookworm-slim` ships no openssl binary, so a shelled-out fixture would pass in CI and
|
||||||
and fail on every developer machine, and a committed key leaks in a public repository. Valid while
|
fail on every developer machine, and a committed key leaks in a public repository. Valid while the
|
||||||
the dev image has no openssl.
|
dev image has no openssl.
|
||||||
|
|
||||||
- **`close` means the session is over, and a drop the loop will retry is `disconnected`.** Maintainer's
|
|
||||||
call, 2026-08-31: with reconnect on by default a `close` on every transient drop left an application
|
|
||||||
unable to tell a retry from the end, and no second one follows because `teardown()` is a no-op once
|
|
||||||
`closed`. `teardown()` picks the event by whether the reconnect loop is still live, and `end()` stops
|
|
||||||
that loop before tearing down, so every deliberate shutdown emits `close`. Without the split an
|
|
||||||
application that opens a replacement client on `close` ends up holding two binds on one account.
|
|
||||||
|
|
||||||
- **Only a link that outlasted `maxDelay` resets the backoff.** Coming up is not proof it works: an
|
|
||||||
unreadable stream is found after the bind returns, so resetting there gave a link that died on
|
|
||||||
arrival a fresh `minDelay` every cycle — one TCP connect and bind per second, forever, which is how
|
|
||||||
an account gets blocked for bind flooding. `ReconnectLoop` records when it brought the owner up and
|
|
||||||
resets only if the link then lasted longer than the longest wait it would ever schedule. A drop
|
|
||||||
after a healthy link still retries at `minDelay`.
|
|
||||||
|
|
||||||
- **A stream this library cannot read is a dead link, not a dead session.** Maintainer's call,
|
|
||||||
2026-08-31: a framing or codec error tears the link down through `teardown()`, so the reconnect
|
|
||||||
loop retries it on a fresh socket with a fresh framer — which is what a desynced stream needs, and
|
|
||||||
the common cause. `sessionError` still carries every failure, so a peer that only ever sends
|
|
||||||
garbage is visible in the log rather than silent, and the backoff grows to one attempt per
|
|
||||||
`maxDelay`.
|
|
||||||
|
|
||||||
- **`disconnected` counts failed links, not outages.** A retry that opens a socket and then loses it
|
|
||||||
clears `closed` through `attach()`, so the next `teardown()` emits again: each emission is a link
|
|
||||||
that went down, which makes the event deliberately not one-to-one with `reconnected`. Suppressing
|
|
||||||
the later ones would leave a failed rebind with nothing but a log line.
|
|
||||||
|
|
||||||
- **`session.sock` is a getter over `PduTransport`, and stays public.** Maintainer's call, 2026-08-31:
|
|
||||||
`session.ts` had reached its line cap, so the socket-to-PDU seam todo.md named was opened —
|
|
||||||
`PduTransport` owns the socket, the framer and the parse, and hands the session raw bytes, framed
|
|
||||||
PDUs, parsed ones and an unreadable stream. Reading `session.sock` is unchanged; assigning it no
|
|
||||||
longer compiles, which never rewired the handlers and so never worked. The transport stays
|
|
||||||
unpublished like the other collaborators.
|
|
||||||
|
|
||||||
- **A client re-binds after a drop unless it is told not to.** Maintainer's call, 2026-08-31:
|
|
||||||
surviving a dropped link is most of what the session layer is for, and behind an opt-in an
|
|
||||||
application that never read the options table got none of it. `reconnect` takes
|
|
||||||
`{ minDelay, maxDelay }` to retune the backoff and `false` to turn it off, so absent means on and
|
|
||||||
there is one spelling for each. Only `client()` reconnects — a `server()` session is a connection
|
|
||||||
the peer opened, and nothing at this end can reopen it. The retry timer is `unref()`'d, so a
|
|
||||||
process with nothing else left to do still exits between attempts.
|
|
||||||
|
|
||||||
- **The notation a peer writes message ids in is named per place, and normalisation never reaches
|
|
||||||
inside a `<base>-<n>` id.** An SMSC may answer `submit_sm_resp` in hex and write the receipt's
|
|
||||||
`id:` in decimal, so one transform over both sides cannot make them equal — `smsIdFormat` names
|
|
||||||
`receipt` and `submitResp` separately and reads both into a plain decimal value before `smsIds`
|
|
||||||
and `dlr.smsId` are compared. `submitResp` covers the `receipted_message_id` TLV too, which SMPP
|
|
||||||
3.4 5.3.2.26 defines as the id the `submit_sm_resp` carried: naming one notation for whichever id
|
|
||||||
a receipt yields would break the peer that sends both, whose TLV correlated before the option was
|
|
||||||
set. Omitting a place is what leaving it alone means, so there is no
|
|
||||||
`raw` notation, and a caller-supplied formatter is refused because it would make the promise that
|
|
||||||
those two are comparable unverifiable — `onRequest` and the PDU on the `dlr` event are the escape
|
|
||||||
hatches, and the `onReceipt` hook in todo.md is the seam if one is wanted. An id no notation reads
|
|
||||||
is left exactly as it arrived, which is what keeps `DlrMerger` working: a `<base>-<n>` id parses
|
|
||||||
as no number and so reaches `expect()` and `collect()` unchanged. Normalising the base instead
|
|
||||||
would break that pair. The option is on `client()` only — a `server()` session generates its own
|
|
||||||
ids and writes its own receipts, so both places are already one notation.
|
|
||||||
|
|
||||||
- **A send waits for the next link only if it never reached the socket; one that did is counted, not
|
|
||||||
resent.** Maintainer's call, 2026-09-01: re-queueing everything unanswered would resend a
|
|
||||||
`submit_sm` the SMSC accepted and answered into a dead socket, which is delivered and billed
|
|
||||||
twice, while a request that never left this process can be lost for free. `LinkGate` holds a send
|
|
||||||
that has no link, so a send issued between links and a segment still queued behind a full window
|
|
||||||
when the drop hit both go out on the new one. Once a request has been written, every way it can
|
|
||||||
fail — the link dropping under it, `responseTimeout` expiring, the caller's own abort — means the
|
|
||||||
peer may have taken it, so `attempt()` wraps all three in `UnansweredError` and `collectSent()`
|
|
||||||
counts them into `SendSmsResult.unanswered`. Counting only the dropped-link case, as the first cut
|
|
||||||
did, would have called the commonest one safe to resend. A count rather than a boolean because
|
|
||||||
`sendSms()` aggregates segments into one `err` slot, and required rather than optional so every
|
|
||||||
construction site answers. `UnansweredError` stays unexported: `unanswered` is the one spelling on
|
|
||||||
the public surface, and a `send()` error that is neither a build failure nor a pre-write abort
|
|
||||||
means the same thing.
|
|
||||||
The hold is bounded by `responseTimeout` rather than an option of its own — that is already the
|
|
||||||
answer to how long one request may wait. It bounds the hold and the answer separately, and the
|
|
||||||
wait for a `maxOutstanding` slot is bounded by nothing, so `responseTimeout` is not a deadline for
|
|
||||||
the call; `SendOptions.signal` with `AbortSignal.timeout()` is, and both the gate and
|
|
||||||
`pending.wait()` honour it. The hold's clock starts when the send is issued rather than when it
|
|
||||||
first finds the gate shut, so one budget covers every hold a single call makes — a send that spent
|
|
||||||
it queued behind the window is refused rather than held. The hold timer is the one timer here that
|
|
||||||
is not `unref()`'d: a held request is awaited with the socket already destroyed, so an unref'd one
|
|
||||||
lets a process whose only remaining work is that send exit without settling it.
|
|
||||||
|
|
||||||
- **The gate decides whether a link can carry a request, and a bind is what makes it one.**
|
|
||||||
Maintainer's call, 2026-09-01: `attach()` clears `closed` the moment a socket is handed over, one
|
|
||||||
round trip before the bind is answered, so gating on `closed` let a send arriving in that window
|
|
||||||
go out unbound and come back `ESME_RINVBNDSTS` while a send that arrived a millisecond earlier was
|
|
||||||
held correctly. `LinkGate` owns the answer instead — `shut(returning)` on every teardown,
|
|
||||||
`open()` only once `comeBackUp()` has a bound link — and `Session.linkDown()` reads it rather than
|
|
||||||
`closed`. The bind itself cannot wait for what it creates, so `send()` lets the three bind
|
|
||||||
commands past the gate and the window, the same door `unbind()` takes through `attempt()`. That
|
|
||||||
keeps the exemption a predicate on the command, like the `_resp` guard beside it, rather than a
|
|
||||||
second `send()` on the public surface or a changed `ReconnectOptions.onConnected`.
|
|
||||||
The gate is told what happened and never reads back into the session: a collaborator that has to
|
|
||||||
ask does not own its decision, which is how the first cut ended up answering the same question two
|
|
||||||
different ways at admit and at release. For the same reason the retry in `send()` asks
|
|
||||||
`gate.isUp()` rather than `linkDown()`, which also reads the socket: a condition that loops on
|
|
||||||
something the gate does not gate on spins against a gate that admits it straight back.
|
|
||||||
`LinkGate.returning` is a copy of `retrying()` taken at teardown, and stays true only because
|
|
||||||
nothing stops the reconnect loop without `emitClose()` following it — `drain()` and `end()` are the
|
|
||||||
only callers of `stop()`. An `OutgoingRequests` owning both would not need the copy; until then,
|
|
||||||
a third caller of `stop()` has to shut the gate itself.
|
|
||||||
|
|||||||
@@ -1,18 +1,17 @@
|
|||||||
# todo.md
|
# todo.md
|
||||||
|
|
||||||
Remaining work for the `@larvit/smpp` 1.0.0 rewrite. Read [AGENTS.md](AGENTS.md) first — the hard
|
Remaining work for the `@larvit/smpp` 1.0.0 rewrite. Read [AGENTS.md](AGENTS.md) first — the goals
|
||||||
rules there constrain every item below.
|
and hard rules there constrain every item below.
|
||||||
|
|
||||||
|
This is a temporary working file: it is deleted when 1.0.0 ships, and until then it sets its own
|
||||||
|
rules. The documentation conventions in AGENTS.md do not 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,
|
||||||
20, 22 and 24. What is left is release work and a few things worth adding before or after 1.0.0.
|
20, 22 and 24. What is left is release work and a few things worth adding before or after 1.0.0.
|
||||||
|
|
||||||
```bash
|
|
||||||
docker compose run --rm node npm install
|
|
||||||
docker compose run --rm node npm test
|
|
||||||
```
|
|
||||||
|
|
||||||
## The agreed API
|
## The agreed API
|
||||||
|
|
||||||
Settled with the maintainer before implementation. Do not change any of it without asking. The
|
Settled with the maintainer before implementation. Do not change any of it without asking. The
|
||||||
@@ -180,13 +179,9 @@ session message is a change to every call site.
|
|||||||
|
|
||||||
## Declined
|
## Declined
|
||||||
|
|
||||||
- **Throughput throttling — a TPS cap, and backing off on `ESME_RTHROTTLED`.** Two reasons, either
|
- **Throughput throttling — a TPS cap, and backing off on `ESME_RTHROTTLED`.** Declined by AGENTS.md
|
||||||
sufficient. An operator's rate limit is scoped to the account, while the widest thing this library
|
goal 7: an operator's rate limit is scoped to the account, while the widest thing this library owns
|
||||||
owns is a session: a bucket here cannot see a second process binding the same account, so it is
|
is a session, so a bucket here cannot see a second process binding the same account and is wrong in
|
||||||
wrong in exactly the case it exists for. And a rate limiter's queue drains at a fixed ceiling
|
exactly the case it exists for. `sendSms()` surfaces `ESME_RTHROTTLED` to the caller instead, and
|
||||||
rather than at the peer's response rate, so a submit rate sustained above the limit grows it
|
`maxOutstanding` stays — a window slot frees on the peer's next response, which is self-limiting in
|
||||||
without bound — and a queue holding messages the caller was told were accepted loses them on
|
a way a rate ceiling is not.
|
||||||
restart, which is worse than refusing them up front. Pacing an account needs durable shared state
|
|
||||||
this library deliberately has none of. `sendSms()` surfaces `ESME_RTHROTTLED` to the caller
|
|
||||||
instead, and `maxOutstanding` stays: a window slot frees on the peer's next response, which is
|
|
||||||
self-limiting in a way a rate ceiling is not.
|
|
||||||
|
|||||||
Reference in New Issue
Block a user