Plan the comprehension rewrite and file its background
Test / lint (pull_request) Successful in 23s
Test / test (18) (pull_request) Successful in 30s
Test / test (20) (pull_request) Successful in 31s
Test / test (22) (pull_request) Successful in 31s
Test / test (24) (pull_request) Successful in 30s
Test / test (26) (pull_request) Successful in 30s
Mirror / push (push) Successful in 5s
Test / lint (pull_request) Successful in 23s
Test / test (18) (pull_request) Successful in 30s
Test / test (20) (pull_request) Successful in 31s
Test / test (22) (pull_request) Successful in 31s
Test / test (24) (pull_request) Successful in 30s
Test / test (26) (pull_request) Successful in 30s
Mirror / push (push) Successful in 5s
This commit was merged in pull request #50.
This commit is contained in:
@@ -0,0 +1,605 @@
|
||||
# Round 1: drafts A and B
|
||||
|
||||
## Draft A, junior seat
|
||||
|
||||
1. **Hardest places, ranked**
|
||||
|
||||
1. `src/outgoing-requests.ts:75-113` (`OutgoingRequests.request` / `requestDuringDrain` / `carrier`) together with `src/session.ts:336-371` (`linkLost`, `end`, `nextLinkExpected`). Whether a send is refused, waits or retries depends on `life`, `link.canCarry()` and `reconnectLoop`. Those live in `Session` and reach here only through the three `LinkView` closures, so reading one file means holding the other's state in my head. Line 82, `closing() && current().canCarry()`, beat me until I traced `carrier()` → `nextExpected()` → `life === 'open'`. Its comment ("refused as closed further on") points at the answer without giving it. `linkLost` reads `nextLinkExpected()` before `close()`, and only the comment explains why. That comment helped; the rest stayed half-opaque.
|
||||
2. `src/held-messages.ts:40-170` (`HeldMessage`, `HeldMessages.offer`), with `src/sms.ts:77-162` and `src/session.ts:104`. A message has six exits across three files: a `working` listener counter, `answered()` deferring by `setImmediate`, a `WeakMap` from `Sms` to hold, and `emit()` returning false meaning release. `captureRejectionSymbol` calls `this.link.held.rejected(...)`, which is always the current link, so the message is only found if the link has not been replaced since the emit. The numbered exit list in the doc comment is the only reason I followed this.
|
||||
3. `src/pdu.ts:84-136` (`resolveShortMessage`, `resolveBody`). The `CodingSource` idea is hard: `data_coding` is rewritten from whichever body "owns" it, an empty buffer counts as `message_payload`, and a string gets encoded while a Buffer does not. That is four branches at once. The read side has a hidden coupling too: at `pdu.ts:249` `readParams` passes the already-read `sm_length` to every wire type's `read`. Only the comment at `defs/commands.ts:19-23` hints at it. Partly resolved.
|
||||
4. `src/defs/encodings.ts:147-190` (`messageClassOf`, `messageClassEncoding`, `encodingByDataCoding`). Bit masks over GSM 03.38 coding groups, and I have no domain background for them. `ASCII` means GSM 03.38 7-bit, a name that lies; only the README encoding table fixed that for me. The `//` comment at 159-160 sits above the JSDoc of the function it describes, so it reads as floating. Stayed opaque at bit level.
|
||||
5. `src/dlr.ts:157-239` (`messageType`, `receiptStatus`, `dlrFromPdu`). There are four message types, TLV-over-body precedence for both id and state, and an `unmarked` case that needs both an id and a status to count as a receipt. Comments cite spec sections I can't check. It reads correctly, but only after two passes.
|
||||
6. `src/reassembly.ts:187-207` (`Reassembler.trim`) with `src/expiring-groups.ts:18,70-88`. `weigh()` may evict the very group being added, and `answered = parts.size - 1` subtracts the refused segment. The contract "enforces neither max nor timeout itself; only weigh() evicts" splits enforcement between owner and store. Resolved by the comments, but costly.
|
||||
7. `src/dlr-merger.ts:150-172` (`open`, `close`). `close()` does not close a group: it moves the base into a second `ExpiringGroups` called `spent`. The misleading name cost me a reread. The class doc resolved it.
|
||||
8. `src/drain.ts:20-56` (`drain`, `leftOf`, `messagesBudget`). The doc says "one budget", but messages get their own budget with a fallback while requests get what is left. 0 means "forever", so `leftOf` clamps to 1. Small but inverted.
|
||||
|
||||
2. **Least want to modify:** `HeldMessages` / `HeldMessage` (`src/held-messages.ts`). Its lifetime is decided by timing (`setImmediate` so that `sendDlr` still goes out past a drain), by listener counts, by identity checks against reused sequence numbers, and by callers in `session.ts` and `sms.ts`. A change to any exit risks a drain that hangs or one that ends early, and nothing local would show it.
|
||||
|
||||
3. **Expected hard, found easy:** `PduFramer`, `PendingRequests`, `SendWindow`, `ReconnectLoop`, and the TLV table's type-level keying (`defs/tlvs.ts`). `defs/types.ts` is 684 lines but repetitive and uniform. `client.ts` and `server.ts` are shallow and read top-down.
|
||||
|
||||
4. **Prose debt:**
|
||||
- Needed:
|
||||
- The README encoding table, to learn that `ASCII` means GSM 7-bit.
|
||||
- The AGENTS "GSM 7-bit is sent unpacked" section, to see why `segmentUnits` holds 153 against 134.
|
||||
- The README "Server in depth" and "Shutdown" sections, to understand `answeredOnArrival` and what the drain waits for.
|
||||
|
||||
Finding them meant scanning a 773-line README; AGENTS has no anchors from code to sections.
|
||||
- The decisions index in AGENTS gives titles only. The reasoning is in `docs/decisions.md`, which I was not allowed to read, so rules like "a drain ignores `shutdownTimeout: 0`" had to be recovered from code comments.
|
||||
- Told me nothing the code did not:
|
||||
- The 0.4.0 defect table, which says nothing about the current code.
|
||||
- Most of the AGENTS architecture list, which restates filenames.
|
||||
- Most decision-index lines, which repeat what an adjacent code comment already says.
|
||||
- One-liners such as "Sends a request and resolves with the peer's response" on `send()`.
|
||||
|
||||
I opened no tests.
|
||||
|
||||
5. **Scores.** The problem is intrinsically hard (a protocol I don't know, plus async link lifecycles); that gets no bonus below.
|
||||
- **Navigation 7 (Predictable):** file names match behaviours one-to-one, but "what happens to a send during shutdown" lives across `session.ts`, `outgoing-requests.ts`, `link.ts` and `drain.ts`, with no single entry point.
|
||||
- **Locality 5 (Honest middle):** `Session` injects its state as closures (`LinkView`, `Link.on`) and passes itself back into `HeldMessages` and `IncomingRequests`, and correctness hangs on ordering that is only named in comments (`linkLost` before `close`, `setImmediate` in `answered`).
|
||||
- **Shape 6 (between 5 and 7):** fan-out stays bounded per level, but several names lie: `ASCII` for GSM, `DlrMerger.close` for "mark spent", `string` for a length-prefixed Octet String next to `cstring`, and `answered()` meaning "release a turn later".
|
||||
- **Self-sufficiency 6 (between 5 and 7):** dense, spec-citing comments carry most units, but the domain vocabulary (GSM alphabet naming, segment budgets, what `answeredOnArrival` means) needs the README open beside the code.
|
||||
- **Overall 6:** capped at locality plus one by the cross-file lifecycle state.
|
||||
|
||||
SCORES nav=7 loc=5 shape=6 self=6 overall=6
|
||||
|
||||
## Draft A, mid seat
|
||||
|
||||
1. **Hardest places, hardest first**
|
||||
|
||||
1. `src/held-messages.ts:252-435`, `HeldMessage` / `HeldMessages` (and `session.ts:95-107`, the `captureRejectionSymbol` override). There are six exits, each in a different method. I had to trace this chain across files: a listener rejects, Node's `captureRejections` calls the session, the session calls `this.link.held.rejected(rest[0])`, a WeakMap is searched by object identity, `listenerGaveUp()` counts down from a `listenerCount('sms')` taken when the message was offered, `answered()` waits a turn in `setImmediate`, `release()` compares the array by identity, `settle()` runs, and finally `IdleWaiters` wakes the drain in `drain.ts`. The comment listing the six exits made it readable. What stayed unclear: why the rejection goes to the *current* link's store, which may not be the link the message arrived on after a reconnect. I also had to work out that `send()` choosing `sendPastDrain` exists only because `OutgoingRequests.request` refuses sends while closing.
|
||||
2. `src/outgoing-requests.ts:510-613`, `request` / `requestDuringDrain` / `bindOnCurrentLink` / `requestOnCurrentLink` / `carrier`. That is four ways onto the wire, and each skips a different check. Line 517 (`closing() && current().canCarry()`) refuses only when a link is up. The comment "refused as closed further on" meant tracing `carrier()` to see that `nextExpected()` is false while closing. `responseTimeout` is reused as the deadline for waiting on a link, `deadline === 0` means forever, and the retry loop depends on `retryOnNextLink` from `Link.send`. The `LinkView` closures read `Session` private state from a distance. The comments resolved most of it after two reads.
|
||||
3. `src/session.ts:208-366`, `unbind` / `drain` / `comeBackUp` / `linkLost` / `end`. The ordering does the work. `unbind` drains, then goes around the closing refusal via `requestOnCurrentLink`. `closedOnUnbind` decides which error wins. `comeBackUp` sets `this.link` before the bind succeeds, and `linkLost` must read `nextLinkExpected()` before `close()` because a listener may re-enter. The inline comments ("Read before close()…", "close() can land while…") resolved it, but I had to hold five states at once.
|
||||
4. `src/reassembly.ts:187-207`, `Reassembler.trim`, with `src/expiring-groups.ts:245-315`, `ExpiringGroups.weigh`. `ExpiringGroups` applies its three limits differently: the owner checks `full`, the owner calls `takeExpired`, and only `weigh` evicts. Map insertion order stands in for age, and `set()` re-inserts, so a replaced entry becomes the newest. `trim` can evict its own group, and it then counts `size - 1` as lost because the refused segment "stays with the peer". The comments state each rule, but checking the arithmetic took three rereads.
|
||||
5. `src/pdu.ts:84-136`, `resolveShortMessage` / `resolveBody`, plus `pdu.ts:249`. `CodingSource` decides whether `short_message` or `message_payload` is allowed to set `data_coding`, and an empty buffer flips the answer. `readParams` passes `sm_length` as the length argument to *every* field read, and only `buffer.read` uses it. That is a hidden coupling that neither line mentions. With no SMPP background this stayed half-opaque.
|
||||
6. `src/defs/encodings.ts:1,105-190`, `EncodingName` and `messageClassEncoding` / `encodingByDataCoding`. The name `'ASCII'` means GSM 03.38, and nothing says so until `dataCodingByEncoding`'s comment. It also misleads in `message.ts:343` and `message.ts:402` (`resolved === 'ASCII'` means septet packing). The bit masks (`0x80`, `0xF0`, bits 3-2) were an algorithm I had no context for. Two comments sit stacked in reverse order at lines 159-161. The charter's "GSM 7-bit is sent unpacked" section explained the 153.
|
||||
7. `src/dlr.ts:357-439`, `messageType` / `receiptStatus` / `dlrFromPdu`. There are four message types, and `'unmarked'` becomes a receipt only if both an id and a state can be scraped. Otherwise `IncomingRequests.onDelivery` (`incoming-requests.ts:152`) quietly reroutes it to `onMessage`. The rule is local and commented, but it only makes sense with the spec's `esm_class` bits in mind.
|
||||
8. `src/client.ts:258-330`, `keepTrying` / `initialAttempts`. There is a second `ReconnectLoop` outside the session, a fresh `Session` per attempt, and a `lastErr` captured in closures. The code itself is clear; the cost was noticing that there are two loops.
|
||||
|
||||
2. **Unit I would least want to modify:** `HeldMessages` / `HeldMessage` (`src/held-messages.ts`). Its correctness depends on timing (`setImmediate`), a listener count taken at `emit` time, identity lookups (the WeakMap, and array identity in the store), and callers in `session.ts`, `incoming-requests.ts`, `sms.ts` and `drain.ts` that each use a different exit. A change there fails as a hung or cut-short shutdown, which is hard to see in a test.
|
||||
|
||||
3. **Expected hard, found easy:** the wire codec. `defs/types.ts` is long but uniform. `PduFramer`, `parseTlvs` / `writeTlvs`, `readOptionalParams` (its NULL-pad rule is commented), `ReconnectLoop`, `bind-direction.ts`, `sms-id.ts` and the `Result<T>` convention were all quick. `udh.ts`'s `concatInfo` explains its walk over the header elements well enough for a newcomer to the domain.
|
||||
|
||||
4. **Prose debt**
|
||||
- **Needed:**
|
||||
- The AGENTS.md architecture map was cheap and correct; it is how I found every file. The file list matches `src/`.
|
||||
- The "GSM 7-bit is sent unpacked" section was necessary for `segmentUnits`.
|
||||
- The README's Receive-SMS text was necessary to see why `sendResp()` on a multipart message writes nothing.
|
||||
- Missing everywhere: a one-line glossary of ESME/SMSC, `esm_class`, `data_coding`, UDH and `sar_*`. The only one is the `LinkEnd` comment for ESME/SMSC. Comments cite spec sections ("SMPP 3.4 5.2.12") that I cannot open, so for someone new to the domain they are pointers, not definitions.
|
||||
- The decisions index names rules ("the drain's wait on the application ignores `shutdownTimeout: 0`") that I then found stated in the code (`drain.ts` `messagesBudget`). The index cost scrolling and gave nothing the code did not.
|
||||
- I opened no tests.
|
||||
- **Told me nothing new:**
|
||||
- The AGENTS "Defects found in 0.4.0" table: history, not needed to read this code.
|
||||
- Most of the Conventions paragraph on test fixtures, for reading `src/`.
|
||||
- Doc comments that restate the code: `Link.canCarry` ("Whether a request can go out on it right now"), `HeldMessages.isGone`, `Session.bindAllows` ("Consulted by the library's senders"), `IdleWaiters.settle`, `PduRefusedError`'s class comment.
|
||||
|
||||
5. **Scores**
|
||||
- **Navigation 7:** at the "predictable" anchor. The one-line-per-file map and concept-named files (`link.ts`, `drain.ts`, `reassembly.ts`) got me from symptom to file first try. It stops short of 9 because shutdown behaviour lives in five files (`session.ts`, `drain.ts`, `held-messages.ts`, `outgoing-requests.ts`, `idle-waiters.ts`).
|
||||
- **Locality 5:** at the "honest middle" anchor. Most modules stand alone, but the held-message/drain path runs on hidden timing (`setImmediate`), a listener count taken early, rejection routing to whatever link is current, and `LinkView` closures reading `Session` private state. The order-dependent sequences in `Session.linkLost` and `unbind` add to it.
|
||||
- **Shape 6:** between the middle and predictable anchors. Fan-out is bounded (`Session` → `Link` → `PendingRequests` / `HeldMessages` / `Reassembler`), but some names lie or clash:
|
||||
- `'ASCII'` means GSM 03.38.
|
||||
- In `sms.ts`, `answered` is both a mutable `{ smsId }` holder (line 81) and a handler function (line 69).
|
||||
- `HeldMessage.held.held` chains through two different things both called `held`.
|
||||
- `lostLink()` is a predicate named like an event.
|
||||
- There are four request entry points on `OutgoingRequests`.
|
||||
- **Self-sufficiency 6:** between the middle and predictable anchors. Comments are dense and carry the why at the call site (the six-exit list, "Read before close()"). But domain terms are never glossed and the spec section numbers point outside the repo, so the `data_coding` and `esm_class` bit logic in `encodings.ts` and `dlr.ts` cannot stand alone for a reader new to SMPP.
|
||||
- **Overall 6:** capped at locality + 1. The hard parts are few and mostly marked, but the one I would fear most (held messages and the drain) spreads across files and relies on timing.
|
||||
- **Intrinsic difficulty** (no bonus): moderate-high. The protocol has two segmentation spellings, receipts that share a command with messages, a direction-dependent `data_sm`, and graceful drain combined with reconnect.
|
||||
|
||||
SCORES nav=7 loc=5 shape=6 self=6 overall=6
|
||||
|
||||
## Draft A, senior seat
|
||||
|
||||
1. **Hardest places, ranked**
|
||||
|
||||
1. **`src/held-messages.ts:40` `HeldMessage`, and `HeldMessages.offer` at `:148`.** Following this one flow meant holding five files at once:
|
||||
- `sms.ts:127` `sendResp` and `:210` `sendDlr`.
|
||||
- The `setImmediate` in `answered()` at `:58`.
|
||||
- `HeldMessage.send` at `:77`, which picks between `sendPastDrain` and `session.send` by asking `isHeld()`.
|
||||
- `OutgoingRequests.requestDuringDrain`.
|
||||
- `Session`'s `captureRejectionSymbol` at `session.ts:104`, which gets back to the hold through a `WeakMap` keyed on the `Sms`.
|
||||
|
||||
A `sendDlr()` gets past a drain only while the release has not yet happened, and that is one event-loop turn. `working` is `listenerCount('sms')` taken at offer time, and the guarded `emit` returning false feeds exit 3. The six-exits comment and the README's Shutdown section settled it, but only after I had read both.
|
||||
|
||||
2. **`src/outgoing-requests.ts:75` `request`, with `:90` `requestDuringDrain`, `:115` `bindOnCurrentLink`, `:133` `requestOnCurrentLink` and `:160` `carrier`.** There are four ways onto a link, and each skips a different mix of four things: the drain refusal, the send window, the wait for a link and the retry.
|
||||
- Line 94, `closing() && current().canCarry()`, only makes sense with the comment "refused as closed further on".
|
||||
- `misuse()` is checked twice.
|
||||
- Whether the bind and the unbind count toward the drain's `window.idle()` has to be worked out from the fact that they skip `attemptOn`.
|
||||
|
||||
I followed it in the end, but did not come away sure of the edge cases.
|
||||
|
||||
3. **`src/session.ts:234` `answer`, with `incoming-requests.ts:85` and `sms.ts:127`.** `sendReturn` always writes to `this.link`, the current link. The rule that "an answer belongs to the link the message arrived on" is held by callers checking `link.isClosed()` or `lostLink()` before they call, in two separate places. `sendReturn` never enforces it. I had to hunt for this, and only the decision titles in AGENTS.md told me the rule exists.
|
||||
|
||||
4. **`src/pdu.ts:84` `resolveShortMessage` / `:113` `resolveBody`.** `CodingSource` decides whether `short_message` or `message_payload` sets `data_coding`. I had to hold these cases at once:
|
||||
- Buffer or string or absent.
|
||||
- Empty or non-empty.
|
||||
- `data_coding` given or not.
|
||||
- An empty `short_message` that still makes `message_payload` the source.
|
||||
|
||||
The type comment at `:74` helps. It still took two reads.
|
||||
|
||||
5. **`src/reassembly.ts:111` `collect` / `:188` `trim`, on top of `expiring-groups.ts:18`.** `ExpiringGroups` enforces its limits unevenly:
|
||||
- `set()` never evicts and `weigh()` does.
|
||||
- `full` is only advisory.
|
||||
- `onSweep` must itself call `takeExpired()`.
|
||||
|
||||
`trim` counts `parts.size - 1` because the segment was added before weighing and may be the one evicted. The comments state each quirk, but I needed all of them at the same time.
|
||||
|
||||
6. **`src/session.ts:208` `unbind` and `:336` `linkLost`.** In `unbind`, the three booleans `wasOpen`, `closedOnUnbind` and `drained` decide which error wins. In `linkLost`, "read `nextLinkExpected` before `close()`" depends on `link.close()` calling `reassembler.clear()`, which emits `sessionError` synchronously to a listener that might call `close()`. That is state changed out of sight. The comment names the risk but not the path it takes.
|
||||
|
||||
7. **`src/dlr-merger.ts:150` `open` / `:165` `close`.** Here `close` means "mark as spent", not "tear down", and `spent` is a second `ExpiringGroups<true>` with its own cap and eviction. The class comment explains the purpose, but the method name misleads.
|
||||
|
||||
8. **`src/client.ts:286` `keepTrying` / `:263` `initialAttempts`.** There are two different `ReconnectLoop` owners: the session's loop, and a separate one that runs only for the first connect. There is also a `lastErr` closure and a comment about `unref: false`. It was readable once I saw that `fromStart` builds a fresh `Session` for every attempt.
|
||||
|
||||
2. **The unit I would least want to modify:** `OutgoingRequests` together with its callers `HeldMessage.send` and `Session.unbind`. Whether a send is refused, queued or bypassed depends on which of the four entry points was chosen, and those choices are made in three other files. A change to one bypass has no local test of whether the drain still counts it.
|
||||
|
||||
3. **Expected hard, found easy:**
|
||||
- The codec: `defs/types.ts` and the TLV read/write. It is mechanical and every read is range-checked.
|
||||
- UDH walking in `udh.ts`.
|
||||
- GSM encoding and `splitMessage`.
|
||||
- Parsing receipt dates.
|
||||
- `ReconnectLoop`'s backoff reset.
|
||||
|
||||
These are local and commented at the right spots, with SMPP section references.
|
||||
|
||||
4. **Prose debt.**
|
||||
- **Needed:** the AGENTS.md architecture map (accurate, and my main way to navigate). The README "Session / Shutdown" and "Sends and the link" bullets, for the drain and held-message rules. The AGENTS decision-index titles, for why answers are tied to a link and why a close after our own `unbind` is clean. Cost was moderate: all of it in two files I had already read, but the "one turn later" rule for `sendDlr` is stated in full only in README step 1.
|
||||
|
||||
A comment that is wrong: `SessionOptions.shutdownTimeout` says it bounds only "the requests already on the wire", but `drain.ts:25` also uses it for held messages.
|
||||
|
||||
Defaults are scattered with no pointer between them:
|
||||
- A separate `defaults` object in each of `client.ts`, `server.ts` and `session-options.ts`.
|
||||
- `backoffDefaults` in `reconnect-loop.ts`.
|
||||
- `defaultMaxOctets` in `reassembly.ts`, which duplicates `maxHeldOctets`.
|
||||
|
||||
Finding where the default for a given option lives took a search.
|
||||
- **Told me nothing about the current code:**
|
||||
- The AGENTS 0.4.0 defects table: history, and it never helped me read `src/`.
|
||||
- Most of AGENTS "Conventions", which is about test fixtures and teardown.
|
||||
- One-liners that restate the code, such as `/** Sends a request and resolves with the peer's response. */` and `/** Answers a request the peer sent us. */`, plus the `ConcatInfo`/`udhLength` comments.
|
||||
|
||||
I did not open any test.
|
||||
|
||||
5. **Scores.** The problem is intrinsically hard: an SMPP session layer with reconnect, drain, windowing, reassembly and receipt merging. It gets no bonus here.
|
||||
- **Navigation 7** (anchor 7): the AGENTS file map answers "where does this live" accurately for every file. It is held below 8 because a symptom like "`sendDlr` refused during shutdown" lands across four files, and "which default" across three objects all called `defaults`.
|
||||
- **Locality 6** (between 5 and 7): the codec, defs, dlr and message modules are fully local. The session layer is not: `Session` passes itself into `IncomingRequests`, `HeldMessages` and `createSms`, the link-answer rule is enforced by callers, and the drain bypass depends on a `setImmediate` in another file.
|
||||
- **Shape 6** (between 5 and 7): fan-out is bounded and most names are true. Some are not:
|
||||
- `ASCII` means GSM 03.38.
|
||||
- `HeldMessages.full()` sweeps, logs and flips state.
|
||||
- `DlrMerger.close` means "mark as spent".
|
||||
- `Session.link` is documented as "the latest socket".
|
||||
|
||||
Four near-identical send entry points on `OutgoingRequests` also cost this score.
|
||||
- **Self-sufficiency 7** (anchor 7): the why-comments sit where they are needed (the six exits, the read-before-close note, the SMPP section numbers). Only the drain and held-message semantics needed the README open beside the code.
|
||||
- **Overall 6:** capped at 7 by locality, and held at 6 because the part that is hardest to change safely, the session layer, is also where the cross-file coupling sits.
|
||||
|
||||
SCORES nav=7 loc=6 shape=6 self=7 overall=6
|
||||
|
||||
## Draft A, architect seat
|
||||
|
||||
**Comprehension panel report: Architect, inherited.** Target: `/tmp/claude-1000/-home-lilleman-code-smpp-js/fe9b6791-4543-5342-9fc7-efa1e22d8fc7/scratchpad/draft-a`
|
||||
|
||||
I read README.md, AGENTS.md in draft-a, and every non-test file under src/. I read `defs/` down to its exports and skimmed the rest of it for shape. I opened no test.
|
||||
|
||||
## 1. Map from README and the file tree only (verbatim)
|
||||
|
||||
```
|
||||
Top-level areas I expect (7):
|
||||
A. Entry/wiring — index.ts (public surface), client.ts (connect+bind+reconnect policy), server.ts (listener, auth, sessions set).
|
||||
B. Session life — session.ts (the EventEmitter, lifecycle, close/unbind), session-options.ts (options + defaults + validation),
|
||||
bind-direction.ts (bind types, which end, what a bind allows), reconnect-loop.ts (backoff), link.ts (?? one socket? timers?),
|
||||
drain.ts (graceful-shutdown wait).
|
||||
C. Requests out — outgoing-requests.ts (send path), pending-requests.ts (seqNr correlation + timeout),
|
||||
send-window.ts (maxOutstanding), unanswered-error.ts (the "may have been taken" error), idle-waiters.ts (?? something waits for zero).
|
||||
D. Requests in / messages — incoming-requests.ts (dispatch of what the peer sends), sms.ts (the 'sms' handle, sendResp/sendDlr),
|
||||
held-messages.ts (the 1000-unanswered bound from README "Unanswered messages"), reassembly.ts + expiring-groups.ts
|
||||
(multipart store; expiring-groups probably shared), concat.ts + udh.ts (segment detection UDH vs sar_*),
|
||||
message-body.ts (short_message vs message_payload), message.ts (encode/split), send-sms.ts (sendSms composition),
|
||||
retained-pdu.ts (?? memory accounting per maxOctets).
|
||||
E. Receipts — dlr.ts (parse), dlr-merger.ts (messageDlr), sms-id.ts (smsIdFormat notations, <base>-<n>).
|
||||
F. Codec — pdu.ts, pdu-framer.ts, pdu-refusal.ts (PduRefusedError), defs/* (spec tables, wire types, encodings).
|
||||
G. Plumbing — result.ts, log.ts, error-from.ts (?? error from unknown), uuid.ts.
|
||||
|
||||
Unclear by name: link.ts, idle-waiters.ts, retained-pdu.ts, error-from.ts; overlap suspected between drain.ts / idle-waiters.ts / held-messages.ts.
|
||||
Expected but not visible: no keepalive/timer file (README's enquire_link + idleTimeout) — guess it's in link.ts or session.ts;
|
||||
no socket/transport file; no store (goal 9) — README says it has not shipped, so absence is honest; interop-tests/ and benchmarks/ are outside src/test.
|
||||
```
|
||||
|
||||
**Where the map was wrong, and what each correction cost:**
|
||||
- **link.ts (medium).** I expected a socket plus its timers. `Link` also owns the `PendingRequests`, the `Reassembler` and the `HeldMessages`. So held messages and reassembly belong to one socket, not to the session. That changes how you reason about the drain after a reconnect, and I had to rebuild part of the model.
|
||||
- **held-messages.ts (medium).** I expected a counter. It holds six exit paths, a WeakMap for listener rejections and a back-reference to `Session`. It also sends through a bypass path (`sendPastDrain`).
|
||||
- **sms.ts (low to medium).** I expected only the inbound handle. It also builds outbound delivery receipts (`receiptText`, `receiptTlvs`, `collectReceipt`), while `dlr.ts` parses them. Writing and reading receipts are split across two files.
|
||||
- **Smaller surprises (low).** `reassembly.ts` exports `decodeSegments`, which `sms.ts` uses to build `sms.message`. `message.ts` also holds `smppTime` and `smppDate`.
|
||||
- **drain, idle-waiters, retained-pdu, error-from (cheap).** Each turned out as guessed. `drain.ts` is budget arithmetic only.
|
||||
- **Keepalive (right).** The timers are in `link.ts` (`resetTimers`).
|
||||
|
||||
## 2. Fan-out level by level
|
||||
- **L0, repo:** src/, test/, README, AGENTS. Trivial.
|
||||
- **L1, src/: 35 files plus defs/, all flat. This is the worst level.** My map needed 7 areas, but the layout shows none of them, and the AGENTS.md architecture list is not grouped by area either. I had to hold about 36 names to sort them.
|
||||
- **L2, defs/:** 7 files, all spec tables. Bounded.
|
||||
- **L3, units:**
|
||||
- `session.ts`: about 20 members, but grouped, with a stated invariant ("every event about the life is emitted from one of these four").
|
||||
- `link.ts`: about 12 members.
|
||||
- `outgoing-requests.ts`: 4 public ways in (`request`, `requestDuringDrain`, `requestOnCurrentLink`, `bindOnCurrentLink`), the one level where the fan-out is too wide for the concept.
|
||||
- `defs/types.ts`: 684 lines, but a table of wire types, so it is wide without being hard.
|
||||
|
||||
## 3. Names
|
||||
**Misleading:**
|
||||
- **`idle`** means two things. `HeldMessages.idle()`, `SendWindow.idle()`, `OutgoingRequests.idle()` and `IdleWaiters` mean "wait until the count reaches zero". `idleTimeout` and "closing an idle peer" in link.ts mean the peer has gone silent.
|
||||
- **`ExpiringGroups`** enforces neither its `max` nor its timeout (its own comment says owners must). `DlrMerger.spent` is an `ExpiringGroups<true>`, a set dressed as groups.
|
||||
- **`EncodingName 'ASCII'`** means GSM 03.38. It is public, legacy and documented, but it is still a false name.
|
||||
- **`lostLink()`** on `SmsHandlers` is a predicate named like an event.
|
||||
- **`message.ts`** also holds `smppTime` and `smppDate`.
|
||||
|
||||
**One concept with two or more names:**
|
||||
- **Answering a request has four spellings:** `sendReturn`, `pduReturn`, `Session.answer()` and `sendResp`.
|
||||
- **Letting a send past the drain has two:** `sendPastDrain` and `requestDuringDrain`.
|
||||
- **The socket has three:** link, `sock` and socket.
|
||||
- **A delivery report has three:** receipt, dlr and report.
|
||||
- **A segment has two:** part and segment.
|
||||
|
||||
**One name over two concepts:**
|
||||
- **"held"** covers messages the application has not answered (`Link.held`), requests waiting for a link ("holding a request until a link is back", "sending what was held for a link"), and `HeldMessages.held`, the inner ExpiringGroups.
|
||||
- **`drain`** is both `Session.drain()` (private) and `drain()` in drain.ts.
|
||||
- **`defaults`** is three different objects: session-options.ts, client.ts and server.ts, with `systemId` in two of them.
|
||||
- **`Waiter`/`waiting`** is defined separately in outgoing-requests.ts and send-window.ts with different meanings.
|
||||
- **"refuse/refusal"** covers codec refusal (`PduRefusedError`), segment refusal (`Refusal 'full'|'unplaceable'`) and option refusal in send-sms.
|
||||
|
||||
## 4. What I would restructure, ranked
|
||||
1. **Group src/ into about 5 directories:** codec/, link+session/, inbound messages/, outbound send/, receipts/, plus defs/. This is the only thing pushing L1 past its bound. AGENTS.md records "src/ stays flat" as a decision, and I would contest it: at 36 files, the map lives in AGENTS.md, not in the layout.
|
||||
2. **Settle on one verb for answering a request.**
|
||||
3. **Move receipt composition out of sms.ts** next to the parsing in dlr.ts. Merge `collectReceipt` (`sms.ts:188`) with `collectSent` (`send-sms.ts:274`); they are near-duplicates.
|
||||
4. **Make `ExpiringGroups` enforce its own `max` and weight, or rename it to say the caps are advisory.** Today three owners each implement eviction differently: `Reassembler.open` plus `trim`, `DlrMerger.dropOldest` plus `spent`, and `HeldMessages.full()` with its own weight comparison.
|
||||
5. **Rename the "wait until zero" methods** (`idle()` → `drained()`), and give "held" one meaning.
|
||||
6. **Move `smppTime` and `smppDate` into their own module, and keep one `defaults`.**
|
||||
|
||||
**What the structure gets right:**
|
||||
- `session.ts` is a readable orchestrator with a single exit for a link (`linkLost`) and a single end (`end`).
|
||||
- `Link.close()` runs once and takes down everything tied to that socket.
|
||||
- The Result discipline is uniform.
|
||||
- The codec is pure and synchronous.
|
||||
- Small modules with honest names: `pdu-framer`, `send-window`, `pending-requests`, `reconnect-loop`, `unanswered-error`.
|
||||
- Log messages are static and prefixed with the unit (`'heldMessages - …'`, `'drain - …'`), so a log line leads straight to its unit. This is the strongest navigation aid in the code.
|
||||
- Comments give the WHY, often with a spec section.
|
||||
|
||||
## 5. The 3am question
|
||||
Symptom: during a graceful shutdown the session hangs until the shutdown timeout, even though the application called `sms.sendResp()` on every message.
|
||||
|
||||
**Cold time to the right unit: about 5 minutes.**
|
||||
1. `Session.close` leads to `Session.drain` (`session.ts:251`).
|
||||
2. That leads to `drain()` (`drain.ts:32`). Its err text and warn logs already say which half stalled: "messages unanswered" or "requests unfinished".
|
||||
3. **Messages half:** `HeldMessages.idle` and `HeldMessages.release` (`held-messages.ts:185-222`), reached from `HeldMessage.answered()` (`held-messages.ts:57`).
|
||||
- The gate is `sms.ts:159`, `if (!failure) handlers.answered()`. If any `sendReturn` for the message fails, the message is never released. Two ways that happens: an `smsId` the latin1 codec refuses, or a failed write. It then stays held until the drain budget runs out (and the 5-minute sweep drops it after that). The application saw `err` from `sendResp()` and ignored it.
|
||||
- Release also works by object identity (`held.get(key) !== pduObjs`), so check that too.
|
||||
4. **Requests half:** `sendDlr` goes past the drain through `requestDuringDrain` (`outgoing-requests.ts:90`). It holds a send-window slot until the peer answers the `deliver_sm`, so a peer that is itself shutting down leaves it until the deadline. That is the other likely cause, and nothing in the symptom rules it out.
|
||||
|
||||
**Where it rots first:** the triangle of `HeldMessage`, `HeldMessages`, `Sms` and `Session`.
|
||||
- `Link` is handed a `session` through its options.
|
||||
- `HeldMessages` emits `'sms'` on `Session`.
|
||||
- A listener rejection comes back through `Session[captureRejectionSymbol]` into `this.link.held.rejected(rest[0])` (`session.ts:104`). That is the current link, not necessarily the one the message arrived on.
|
||||
- Release depends on ordering: `setImmediate` in `answered()` exists so that a `sendDlr()` called straight after `sendResp()` still gets past the drain.
|
||||
- The next fix here will add a seventh exit.
|
||||
|
||||
**Where the next two features would land:**
|
||||
- **Goal 9, the store.** It lands on `ExpiringGroups`, the store its three owners share. That class is synchronous and in-memory, and each owner enforces part of its contract, so moving to an async store interface touches `DlrMerger`, `Reassembler` and `HeldMessages` at once. Expensive.
|
||||
- **Goal 7, a per-PDU rate limit or a custom alphabet.**
|
||||
- A rate limit lands cleanly at `OutgoingRequests.attemptOn` (`outgoing-requests.ts:124`).
|
||||
- A custom alphabet runs into the closed `EncodingName` union. It spreads into `message.ts:14` (`segmentUnits`), `send-sms.ts:92` (`dataCodingFor`, with hard-coded 0x10/0x18), `defs/encodings.ts` and `bitCount`: 4 to 5 places.
|
||||
|
||||
## 6. Hardest places, ranked
|
||||
1. `src/held-messages.ts:40-223`, `HeldMessage` and `HeldMessages`: six exits, release by identity, WeakMap for rejections, `setImmediate` release, back-reference to the session, the drain bypass.
|
||||
2. `src/session.ts:95-107`, `[captureRejectionSymbol]`: a rejected `'sms'` listener reaches the held messages through an `unknown` argument on the current link. Action at a distance.
|
||||
3. `src/outgoing-requests.ts:75-137`: `request`, `requestDuringDrain`, `bindOnCurrentLink` and `requestOnCurrentLink` are four ways in with different bypass rules. The condition `closing() && current().canCarry()` (line 82) reads inverted until you notice the fall-through.
|
||||
4. `src/expiring-groups.ts:19-147`, `ExpiringGroups`: the contract is half-enforced, and each owner fills in the rest differently.
|
||||
5. `src/dlr-merger.ts:68-184`, `DlrMerger`: `groups` plus `spent`, and `close()` always marks an id spent.
|
||||
6. `src/drain.ts:20-57` with `src/session.ts:251`: budget arithmetic where 0 means forever and the messages half falls back to `responseTimeout`. `session-options.ts:63` documents `shutdownTimeout` as covering only the requests on the wire, a partial truth next to the code that owns the behaviour.
|
||||
7. `src/client.ts:228-330`, `bindOn`, `initialAttempts` and `keepTrying`: a second `ReconnectLoop` outside `Session`, with a fresh session per attempt.
|
||||
8. `src/pdu.ts:84-136`, `resolveShortMessage` and `resolveBody`: which body field gets to set `data_coding`.
|
||||
|
||||
**The unit I would least want to modify:** `HeldMessages` and `HeldMessage` in `src/held-messages.ts`.
|
||||
|
||||
**Intrinsic difficulty (no bonus):** high. An SMPP session layer with reconnect, a drain split into two budgets, sequence-number correlation that belongs to one link, reassembly under caps, and asynchronous lifecycle races. Most of the hard spots are hard because of this.
|
||||
|
||||
## 7. Scores
|
||||
- **Navigation: 7.** At the "Predictable" anchor: honest file names plus unit-prefixed static log strings took the 3am symptom from `session.ts` to `drain.ts` to `held-messages.ts`/`sms.ts:159` with no detour. It stays below 9 because the flat 36-file src/ makes you consult AGENTS.md's list to find areas.
|
||||
- **Locality: 6.** Between the anchors: `Link` and `Session` hold clean boundaries, but the held-message flow reaches back into `Session` through `Link` options, relies on `setImmediate` ordering, and gets rejections through `captureRejections` on the current link. And three owners each re-enforce `ExpiringGroups`' caps.
|
||||
- **Shape: 6.** Between the anchors: the flat L1 of 36 files breaks the bound and has no grouping. "held", "idle", `defaults` and "drain" each name two things, and a response has four verbs. The files are small and single-purpose.
|
||||
- **Self-sufficiency: 7.** At the "Predictable" anchor: invariants are stated at the code (the six-exits list, the `linkLost` comment, spec citations), and I needed no second document to follow a unit. It stays below 9 because of the partial `shutdownTimeout` doc at `session-options.ts:63` and the drain semantics, which only README fully states.
|
||||
- **Overall: 6.** Held to the Locality and Shape 6s. It reads close to "Predictable": a cold senior would be productive within a week and would know to fear `held-messages.ts`.
|
||||
|
||||
SCORES nav=7 loc=6 shape=6 self=7 overall=6
|
||||
|
||||
## Draft B, junior seat
|
||||
|
||||
1. **Hardest places, ranked hardest first**
|
||||
|
||||
1. **`src/outgoing-requests.ts:70-168`, `OutgoingRequests.request` → `requestPastDrain` → `carry` → `attempt`.** A request has four ways in. One is a recursive retry (`carry` calls itself at :130) that fires only when `retryOnNextLink && link.awaitsNextLink()`. Each hop asks `LinkLife` a different question: `isStopping`, `canCarry`, `refusal`, `budget`, `awaitsNextLink`. I had to keep LinkLife's phase table open to follow it. The guard at :77, `isStopping() && canCarry()`, has the comment "With no link, the request is refused as closed further on". That describes the branch that is *not* taken here, and I read it three times. `requestPastDrain` is named for the caller that needs it (a receipt during a drain), not for what it does. The `UnansweredError` wrap at :167 was clear. The rest stayed half-opaque.
|
||||
2. **`src/link-life.ts:74-171`, `LinkLife.transition` / `lose` / `end`.** There are three pieces of state: `linkPhase`, `stopping` and `drops`. `stopping` is set in two places (:89, :167). `drops` increments both in `lose` and in `end`, and `bound` while stopping falls through to `lose()`. The effects returned are carried out elsewhere, in `session.ts:269` `run()`, so behaviour is split across two files in an order I had to trust. The type comments at :13-33 made it tractable. What really cost me was the initial phase at :60: `linkPhase = 'up'` on a socket that has not bound yet. That contradicts the `up` doc ("a bound socket carries requests") and the charter's "a bind is what makes it one". I never resolved why the first link starts `up`.
|
||||
3. **`src/held-messages.ts:40-181`, `HeldMessages`.** The numbered "six ways a hold ends" comment helps, but the ways live in three files. Way 2 enters from `session.ts:97`, where `captureRejectionSymbol` passes `rest[0]` as `unknown`. It is then looked up in a `WeakMap`, and `working` is seeded from `session.listenerCount('sms')` at :161. Way 1 arrives through a callback built in `sms.ts`. Way 3 depends on `emit` returning false, both for no listener and for a throw (the override in `session.ts:78`). I pieced this together; it did not stay opaque, but it was the most action at a distance in the codebase.
|
||||
4. **`src/pdu.ts:84-136`, `resolveShortMessage` / `resolveBody`.** `CodingSource` decides which of two fields may set `data_coding`. An empty buffer counts as `message_payload`, and `sm_length` is filled only sometimes. With no SMPP background I could not tell why an empty `short_message` hands authority to the TLV until I read `message-body.ts` and the README's "Where the body is". After that it made sense, but it took two files and a doc.
|
||||
5. **`src/client.ts:228-350`, `bindOn` / `initialAttempts` / `keepTrying` / `client`.** The `fromStart` path builds a second `ReconnectLoop` outside any session. Each attempt gets a fresh `Session`, and a `lastErr` closure is shared between two lambdas. The comment at :249, "close() must reach the loop's stop() before its first await", states an ordering invariant that lives in `session.ts` `drain()`/`apply('stopping')`. It is correct, but invisible from here.
|
||||
6. **`src/defs/encodings.ts:151-198`, `messageClassOf` / `messageClassEncoding` / `encodingByDataCoding`.** This is bit-masking over `data_coding` groups I had no context for. The `//` comment sits above the `/** */` block at :159-161, so it reads as belonging to the wrong function. The "ASCII" name meaning GSM 03.38 is a lie I only caught because the README's encoding table says so. It stayed partly opaque: I trust it, but I could not verify it.
|
||||
7. **`src/reassembly.ts:188-207`, `Reassembler.trim`.** `ExpiringGroups.weigh()` returns evicted groups, possibly including the current one. `answered = parts.size - 1` for the current group depends on the refused segment already being in `parts`. The comments at :200 and :125 resolved it after a reread.
|
||||
8. **`src/drain.ts:70-112`, `drain` / `answeringBudget`.** Two budgets with different zero-semantics, a fallback to `defaults.responseTimeout` imported from `session-options`, and a `setImmediate` turn. The comments explain each step. It was harder than it looked, but it resolved.
|
||||
|
||||
2. **The unit I would least want to modify:** `OutgoingRequests.carry` together with `requestPastDrain` (`src/outgoing-requests.ts:85-133`). Its correctness depends on LinkLife's phase at the exact moment of each await: whether a failed write has already produced `lost` → `dropLink`, so that `awaitsNextLink()` is true. It also depends on the send-window slot being released in `finally` before the recursion. Nothing in the unit states those timing assumptions. A change would be guessed and then tested.
|
||||
|
||||
3. **Expected to be hard, found easy.**
|
||||
- The wire codec: `defs/types.ts` is long but completely regular, and every read/write is range-checked.
|
||||
- `PduFramer`, `concatInfo`, `sms-id.ts`, `dlr.ts` receipt parsing (comments name the operators and spec sections).
|
||||
- The `Result` pattern.
|
||||
- `server.ts` `handleRequest`.
|
||||
- `DlrMerger`, whose severity table comment says exactly why it exists.
|
||||
- The session constructor, which reads as a wiring diagram.
|
||||
|
||||
4. **Prose debt.**
|
||||
- **Needed:**
|
||||
- The SMPP vocabulary: ESME vs SMSC, `deliver_sm` vs `submit_sm` direction, `esm_class`, `data_coding`, TON/NPI, UDH vs `sar_*`. Only the README supplies it, scattered across "Receiving in depth", "Bind direction" and "Delivery receipts". There is no glossary, and finding each term cost several searches.
|
||||
- The charter's "GSM 7-bit is sent unpacked" section, to understand `segmentUnits` in `message.ts:14`. Cheap, because the architecture map pointed there.
|
||||
- The AGENTS decision index. Its lines, for example "One owner decides whether a link can carry a request", told me a rule exists but not its reasoning. I was not allowed to open `docs/decisions.md`, and the initial-`up` puzzle is exactly where I needed it.
|
||||
- **Told me nothing:**
|
||||
- The 0.4.0 defect table. It is history, and useless for reading the current code.
|
||||
- The long test-fixture paragraph in Conventions.
|
||||
- Duplicated doc comments: `deliver` "False means nothing was" appears in both `outgoing-requests.ts` and `pending-requests.ts`, and the `idle()` "Resolves 0 once…" wording is repeated four times.
|
||||
- `/** Injected so expiry can be exercised without a wall clock. */` repeated on four option types.
|
||||
- `get sock` "Replaced on reconnect" in `session.ts`, which restates `PduTransport`.
|
||||
|
||||
5. **Scores.** The problem itself is hard (async link lifecycle, reconnect, protocol quirks); that gets no bonus below.
|
||||
- **Navigation: 7.** At the "predictable" anchor: the AGENTS architecture map names every file by its question and the filenames match. It stops short of 9 because a behaviour like "a send during shutdown" spans `outgoing-requests.ts`, `link-life.ts`, `drain.ts` and `session.ts`, with no single landing file.
|
||||
- **Locality: 6.** Between the middle and predictable anchors. The collaborators are cleanly split, but LinkLife's `stopping`/phase flags are read by OutgoingRequests at await boundaries, effects run in a different file from where they are decided, and HeldMessages is entered from the EventEmitter's `captureRejectionSymbol`. All of that is action at a distance.
|
||||
- **Shape: 6.** Fan-out is bounded (Session has about 9 collaborators, each small), but some names lie: `ASCII` means GSM 03.38, `drain.ts` houses `IdleWaiters`, `requestPastDrain` is named for a caller, and the link starts in phase `up` before any bind.
|
||||
- **Self-sufficiency: 5.** At the middle anchor. Comments are dense with spec citations and explain the WHY locally. Still, a reader without the domain needs README open for SMPP terms, and some rules exist only as index lines pointing at a decisions file.
|
||||
- **Overall: 6.** Capped at 6 by self-sufficiency. The layout is good and the hard parts are really hard, but three of them (sections 1.1-1.3) need two or three files open at once.
|
||||
|
||||
SCORES nav=7 loc=6 shape=6 self=5 overall=6
|
||||
|
||||
## Draft B, mid seat
|
||||
|
||||
1. **Hardest places, hardest first**
|
||||
|
||||
1. **`src/link-life.ts:74` `LinkLife.transition()`, with `lose()` :148, `end()` :159, and `src/session.ts:263` `apply()` / :269 `run()`.** The table depends on two hidden variables besides the phase: the `stopping` flag and the `drops` counter. `lose()` sets the phase to `down` before it calls `end()`, and that is the only thing that stops `end()` counting the same drop twice. `'bound'` while stopping goes through `lose()` and ends up in `end()`. The effect order matters: `dropLink` clears held, incoming and outgoing before `emitClose`. The phase names mislead too. The doc at :14 says "`up`: a bound socket carries requests", but the phase starts as `up` (:60) on a socket nothing has bound yet, both on a server session and on a client before its bind. I only resolved that after reading `OutgoingRequests.requestPastDrain` (outgoing-requests.ts:85), where the bind path skips the link check. The table's JSDoc helps. The transitions themselves I had to trace by hand.
|
||||
2. **Shutdown, spread over `src/session.ts:237` `unbind()` / :300 `drain()`, `src/drain.ts:96` `drain()` / :70 `answeringBudget()`, `src/outgoing-requests.ts:70` `request()` / :85 `requestPastDrain()`, and `src/held-messages.ts` `sendReceipt`.** To know whether a send is refused I had to hold five things at once: `isStopping() && canCarry()`, the "refused as closed further on" path through `LinkLife.refusal()`, the receipt's route past the drain, the `setImmediate` turn between the two waits, and the fallback for `shutdownTimeout: 0`. `IdleWaiters` lives in `drain.ts` but serves `SendWindow` and `HeldMessages`, so I went to the wrong file for it once. The comments explain each step, but no single place explains the whole sequence.
|
||||
3. **`src/held-messages.ts:94` `offer()` / :117 `listenerRejected()` / :154 `keep()`.** Release path 3 depends on `Session.emit` being overridden (session.ts:78) to return `false` when a listener throws. Path 2 depends on `captureRejections` routing through session.ts:106 back into a `WeakMap` lookup by object identity. The `working` count is taken from `session.listenerCount('sms')` at keep time. That is action at a distance in both directions. The numbered "six ways" comment is what made it followable.
|
||||
4. **`src/client.ts:228` `bindOn()`, :263 `initialAttempts()`, :286 `keepTrying()`.** These are three layers of session creation for `fromStart`, with abort listeners added and removed at different points. The comment at :249 says `close()` has to reach `stop()` before its first await. That rule depends on `Session.drain()` calling `apply('stopping')` synchronously (session.ts:301). The comment resolved it, but it is an ordering dependency across files.
|
||||
5. **`src/pdu.ts:84` `resolveShortMessage()` / :113 `resolveBody()`.** The `CodingSource` idea (which of the two bodies gets to set `data_coding`) has about six branches: empty buffer, non-empty buffer, a string that encodes to empty, the command having no `short_message`, and a string `message_payload`. I had no domain background for why the payload may override `data_coding` only when `short_message` is empty. The type comment at :74 got me about half of it.
|
||||
6. **`src/reassembly.ts:188` `trim()` with `src/expiring-groups.ts:70` `weigh()`.** `weigh()` returns evicted groups, possibly including the current one, and `trim` then uses `parts.size - 1` for that one. `ExpiringGroups` enforces its limits unevenly: `max` never, `maxWeight` only in `weigh`. Its three users each handle that differently: `HeldMessages` checks the weight itself, and `DlrMerger` runs two instances (`groups` + `spent`, dlr-merger.ts:150/:165). The class comment says all this, but I needed a second pass.
|
||||
7. **`src/defs/encodings.ts:162` `messageClassEncoding()` / :182 `encodingByDataCoding()` / :151 `messageClassOf()`.** This is bit arithmetic over GSM 03.38 coding groups, which I have never seen. The comments cite the spec but I can't check them. It stayed partly opaque, but it is small and self-contained.
|
||||
8. **`src/outgoing-requests.ts:114` `carry()` / :141 `attempt()`.** A recursive retry that shares one link budget, where a retry happens only when `retryOnNextLink && awaitsNextLink()`. Inside `carry()`, `const held = await waitForLink()` reuses the word "held", which elsewhere means `HeldMessages`. Readable once you know the link model.
|
||||
|
||||
2. **Least want to modify:** `LinkLife.transition()` together with `Session.run()`. Every lifecycle path goes through it (idle timeout, socket close, unreadable stream, unbind, close, rebind). The order of effects and the `stopping`/`drops` side state are invariants that nothing in the types enforces. A wrong effect order would show up as a leaked pending request or a double `close` event somewhere far away.
|
||||
|
||||
3. **Expected hard, found easy:** the wire codec. `defs/types.ts`, TLV read/write and `PduFramer` are mechanical, range-checked and uniform. The same goes for `PendingRequests`, `SendWindow`, `ReconnectLoop`, the linear check chain in `send-sms.ts`, and `udh.ts`. Receipt parsing in `dlr.ts` was clearer than I expected for an unfamiliar domain.
|
||||
|
||||
4. **Prose debt**
|
||||
- **Needed, and cheap to find:**
|
||||
- The charter's architecture list. It is accurate and maps one file to one question.
|
||||
- The "GSM 7-bit is sent unpacked" section, needed for the 153/134 figures in `message.ts:129`.
|
||||
- README "Bind direction" and "Receiving in depth", needed for the `data_sm` direction and for multipart being answered on arrival.
|
||||
- README "Shutdown", needed to see why the drain waits on messages at all.
|
||||
- **Needed, and costly:** nothing tells a newcomer what `esm_class`, `data_coding`, TON/NPI or `sar_*` are beyond scattered spec citations. I had to piece them together from the README and comments.
|
||||
- **Stale prose:** `SessionOptions.shutdownTimeout` (session-options.ts:63) says "How long a drain waits for the requests already on the wire. 0 waits forever". That omits the wait on messages and the rule that 0 does not wait forever for them, which is exactly what `drain.ts` implements.
|
||||
- **Told me nothing beyond the code:**
|
||||
- The charter's defect table (0.4.0 history, no help reading today's code).
|
||||
- The long test-fixture convention paragraph, for this read.
|
||||
- The decision index titles, which I can't expand, and several of which repeat nearby code comments.
|
||||
- `Injected so expiry can be exercised without a wall clock`, repeated in four option types.
|
||||
- The duplicated `deliver()` doc on `OutgoingRequests` and `PendingRequests`.
|
||||
- `/** Starts both timers over… */`-style restatements in `link-timers.ts`.
|
||||
|
||||
5. **Scores**
|
||||
- **Navigation: 8.** Above 7, below 9. The charter's per-file map and concept-named files took me straight to the right place almost every time. The detours were `IdleWaiters` living in `drain.ts`, and receipt-sending split between `sms.ts` and `OutgoingRequests.requestPastDrain`.
|
||||
- **Locality: 6.** Between 5 and 7. Collaborators are injected and the lifecycle effects are an explicit list. But `HeldMessages` and `IncomingRequests` call back into `Session` (`emit` override semantics, `listenerCount`, `close`), and correctness depends on synchronous ordering (`apply('stopping')` before the first await, the `setImmediate` in `drain`).
|
||||
- **Shape: 7.** The Session hub's fan-out is wide but flat (about 9 collaborators, each small). A few names lie: the `up` phase before any bind, the `ASCII` encoding meaning GSM 03.38, the word "held" reused in `carry()`, and two different `onConnected` signatures (a `Session` in `ReconnectOptions`, a `Socket` in `ReconnectLoopOptions`).
|
||||
- **Self-sufficiency: 7.** Comments give the why and the spec section at nearly every surprising line, so most units stand alone. The domain vocabulary (the `esm_class` and `data_coding` bit layouts) and the whole shutdown story needed the README beside the code.
|
||||
- **Overall: 6.** Held down by Locality. The hard part (link life plus the drain) is localized and marked, but it takes rereads. A mid-level reader takes about two days to feel safe in `session`, `link-life` and `held-messages`.
|
||||
- **Intrinsic difficulty:** high. An asynchronous protocol session with reconnect, a send window, reassembly and a graceful drain, on a domain the reader doesn't know. That earns no bonus here.
|
||||
|
||||
SCORES nav=8 loc=6 shape=7 self=7 overall=6
|
||||
|
||||
## Draft B, senior seat
|
||||
|
||||
1. **Hardest places, ranked hardest first**
|
||||
|
||||
1. **The shutdown path**: `src/session.ts:237` (`unbind`), `:254` (`close`) and `:300` (`drain`); `src/drain.ts:96` (`drain`), `:70` (`answeringBudget`) and `:65` (`leftOf`); `src/outgoing-requests.ts:70` (`request`).
|
||||
- To answer "what does a send do during shutdown?" I had to hold four files at once. `LinkLife.stopping` is set by `apply('stopping')`. `request()` refuses only when `isStopping() && canCarry()`. Otherwise it relies on `awaitsNextLink()` going false because `retrying()` checks `!stopping`, which I had to hunt for in `link-life.ts:132`.
|
||||
- The two drain budgets differ in a way only the code shows. Messages fall back to `responseTimeout`; requests get `leftOf(deadline)`, clamped to at least 1 ms. There is also an ordering dependency on a `setImmediate` turn between the two waits (`drain.ts:108`).
|
||||
- The comments are correct but terse. I resolved it after two reads, plus the README "Shutdown" section.
|
||||
2. **The held-message lifecycle**: `src/held-messages.ts:94` (`offer`), `:117` (`listenerRejected`) and `:154` (`keep`); `src/session.ts:106`; `src/sms.ts:91-93` and `:151-161`.
|
||||
- A hold ends in six ways, and they are spread across three files. `Session`'s `captureRejectionSymbol` reaches into `held` by identity, through a `WeakMap`. `working` is a snapshot of `listenerCount('sms')` taken when the message is kept.
|
||||
- The numbered "1–6" comments are what resolved it. Without them this was action at a distance.
|
||||
3. **The link state machine**: `src/link-life.ts:74` (`transition`), `:148` (`lose`) and `:159` (`end`).
|
||||
- `lose()` sets `down` and then calls `end()`, which rereads `attached()` (now false). `drops` is incremented in two places.
|
||||
- The initial `linkPhase = 'up'` (`:60`) contradicts the type doc at `:13-17`, which says `up` means "a bound socket carries requests". A fresh client or server socket is `up` before any bind.
|
||||
- Session's `bound()` (records the bind) and LinkLife's `'bound'` event (a rebind was answered) share a name and mean different things. This stayed partly opaque until I traced `comeBackUp` (`session.ts:359`).
|
||||
4. **Reassembly under the octet cap**: `src/reassembly.ts:111` (`collect`) and `:188` (`trim`), with `src/expiring-groups.ts:70` (`weigh`).
|
||||
- The segment is inserted before `trim`. `weigh` can evict the current group itself, and `answered = parts.size - 1` then excludes the refused segment from the loss.
|
||||
- `ExpiringGroups` enforces max, timeout and weight differently: `full` is only a flag, `weigh` evicts, `set` never does. Its own doc comments make that explicit, which resolved it.
|
||||
5. **Encoding a body under `data_coding`**: `src/pdu.ts:84` (`resolveShortMessage`) and `:113` (`resolveBody`).
|
||||
- `CodingSource` decides whether `short_message` or `message_payload` may overwrite `data_coding`. An empty encoded buffer flips the source to `message_payload`, and a Buffer `short_message` of length 0 does the same.
|
||||
- I needed the decision titles in the charter to see why. The branches are stateful rather than hard, but I reread them three times.
|
||||
6. **The client's first connect**: `src/client.ts:333` (`client`), `:286` (`keepTrying`), `:263` (`initialAttempts`) and `:228` (`bindOn`).
|
||||
- There are two `ReconnectLoop`s: one outside any session for `fromStart`, and one inside each session. Each attempt builds and discards a whole `Session`.
|
||||
- `bindOn` relies on `close()` reaching `stop()` before its first await (the comment at `:249`). That ordering invariant lives in `session.ts:300` (`drain` calling `apply('stopping')` synchronously).
|
||||
- The comment named the ordering rule; checking it held took a look at `session.ts`.
|
||||
7. **`DlrMerger.open`/`close`**: `src/dlr-merger.ts:150` and `:165`.
|
||||
- `close()` means "delete, then mark spent". It runs on completion, on expiry, on eviction and on a reused base, and the delete-then-set on `spent` is only there to refresh its deadline.
|
||||
- A second `ExpiringGroups<true>` used as a tombstone set is clever but not named as one. The class doc resolved it.
|
||||
8. **Server hook composition**: `src/server.ts:208` (`handleRequest`) with `src/incoming-requests.ts:89` (`handle`) and `:147` (`unhandled`).
|
||||
- What happens to a rebind or a pre-bind `unbind` is decided half in each file, through `false` return values. `boundAs === undefined` makes `bindAllows` return true.
|
||||
- I resolved it by reading both. A smaller cost of the same kind: `pdu.ts:249` passes `sm_length` as the `length` argument to every wire type's `read`, and only `buffer` uses it. That implicit coupling depends on wire order.
|
||||
|
||||
2. **The unit I would least want to modify: `OutgoingRequests`** (`src/outgoing-requests.ts:70-168`)
|
||||
- It has three entry points: `request`, `requestPastDrain` and `requestOnCurrentLink`. Each skips a different subset of checks (drain refusal, waiting for a link, the window).
|
||||
- Every sender in the library picks one of them by name: `enquire_link`, `sendSms`, receipts from `sendDlr`, bind and unbind.
|
||||
- The retry recursion in `carry` depends on `LinkLife.awaitsNextLink()`, `pending.settle` ordering, and window release in `finally`.
|
||||
- Whether a request may be resent (goal 2) is decided here, and it hinges on the `retryOnNextLink` flag. A wrong edit silently duplicates billed traffic.
|
||||
|
||||
3. **Expected hard, found easy**
|
||||
- The codec: `defs/types.ts` wire types, `PduFramer`, and `parseTlvs`/`writeTlvs` with the typed `Tlvs`.
|
||||
- GSM 03.38 escaping, `encodingByDataCoding`, receipt text parsing (`dlr.ts`), `sms-id.ts` normalisation, `ReconnectLoop` backoff, and `udh.ts` IE walking.
|
||||
- Each is self-contained, total, and commented at the exact surprising line.
|
||||
|
||||
4. **Prose debt**
|
||||
- **Needed**:
|
||||
- The README "Shutdown" and "Sends and the link" sections, to confirm the drain semantics I was reverse-engineering. They cost a scroll through a 773-line README.
|
||||
- The charter's decision index. The titles hinted at intent ("One owner decides whether a link can carry a request, and a bind is what makes it one"), but I was forbidden from `decisions.md`, so several stayed claims I could only check against code. The one above contradicts LinkLife starting `up`.
|
||||
- The GSM-unpacked section in the charter, to trust `segmentUnits` (`message.ts:343`).
|
||||
- **Defaults live in five places**: `client.ts:45`, `server.ts:403`, `session-options.ts:74`, `reconnect-loop.ts:5` (`backoffDefaults`) and `reassembly.ts:45` (`defaultMaxOctets`, duplicated by `maxHeldOctets`). "Where is the default of X" is a grep.
|
||||
- **Told me nothing new**:
|
||||
- The charter's architecture table mostly restates file names.
|
||||
- Its long paragraph on test conventions is irrelevant to `src/`.
|
||||
- `session.ts:201` ("Sends a request and resolves with the peer's response") restates the code.
|
||||
- `reconnect-loop.ts:47` ("Read through a method: stop() can land while an attempt is awaiting") hides its real reason: it defeats TS narrowing.
|
||||
- `client.ts:101` and `reconnect-loop.ts:84` and `:140` re-implement `errorFrom` inline.
|
||||
|
||||
5. **Scores**
|
||||
|
||||
The problem's intrinsic difficulty is high: an interop-heavy protocol, reconnect, and correct-accounting shutdown. It gets no bonus here.
|
||||
|
||||
- **Navigation: 7.** Against "Predictable": file names and the charter's table put a symptom in the right file first try. What keeps it from 8 is that defaults and the shutdown rules are spread across five files each.
|
||||
- **Locality: 5.** Against "Honest middle": LinkLife's phase and generation are read from Session, OutgoingRequests, IncomingRequests and HeldMessages. Understanding shutdown or a held message means holding 4–5 files and a synchronous-ordering invariant.
|
||||
- **Shape: 6.** Between 5 and 7: most names tell the truth. The named lies are the `up` phase on an unbound socket, `'ASCII'` for GSM 03.38, "bound" meaning two things, `DlrMerger.close` meaning "tombstone", and three near-synonym request methods. Session's constructor fans out to nine collaborators.
|
||||
- **Self-sufficiency: 7.** Against "Predictable": the one-line why-comments at the surprising lines resolved nearly every question. I needed the README only for the drain semantics, and the decision titles were claims I could not verify inside the code.
|
||||
- **Overall: 6.** Capped by locality at 5 + 1. The hard parts are marked but not localized: they are the problem's own difficulty, spread across collaborators that share link state.
|
||||
|
||||
SCORES nav=7 loc=5 shape=6 self=7 overall=6
|
||||
|
||||
## Draft B, architect seat
|
||||
|
||||
**Comprehension panel: Architect, inherited. @larvit/smpp, draft-b, whole project**
|
||||
|
||||
Process note: I read `AGENTS.md` in the same call as `README.md` during step 1, so the map below was not formed from the README and file tree alone. I opened no test file.
|
||||
|
||||
## 1. Map from the README and the file tree (verbatim)
|
||||
|
||||
Top-level areas I believe exist:
|
||||
1. **Public surface**: `index.ts`.
|
||||
2. **Endpoints**: `client.ts` (connect, bind, reconnect-from-start) and `server.ts` (listener, auth, bind answering).
|
||||
3. **Session core**: `session.ts`, `session-options.ts`, `bind-direction.ts`.
|
||||
4. **Link lifecycle**: `link-life`, `link-timers`, `reconnect-loop`, `pdu-transport`, `drain` (shutdown?).
|
||||
5. **Outbound requests**: `outgoing-requests`, `pending-requests`, `send-window`, `send-sms`, `unanswered-error`.
|
||||
6. **Inbound messages**: `incoming-requests`, `sms.ts` (the handle), `held-messages`, `reassembly`, `concat`, `udh`, `message-body`.
|
||||
7. **Receipts**: `dlr`, `dlr-merger`, `sms-id`.
|
||||
8. **Codec**: `pdu`, `pdu-framer`, `pdu-refusal`, `retained-pdu`, and `defs/*` as pure spec tables.
|
||||
9. **Text**: `message.ts` (encode, split, `smppTime`) and `defs/encodings`.
|
||||
10. **Plumbing**: `result`, `log`, `error-from`, `uuid`, `expiring-groups`.
|
||||
|
||||
Names that do not give their purpose:
|
||||
- `drain.ts`
|
||||
- `retained-pdu.ts`
|
||||
- `expiring-groups.ts`
|
||||
- `error-from.ts`
|
||||
- `link-life` vs `link-timers`
|
||||
- `outgoing-requests` vs `pending-requests` vs `send-window`: three names for "requests we sent"
|
||||
- `held-messages` vs `reassembly`: both "held" inbound state
|
||||
|
||||
Expected from the README but not there: the goal-9 store interface. The README says it has not shipped, so that costs nothing. `interop-tests/` and `benchmarks/` sit outside `src`/`test`.
|
||||
|
||||
## 2. Where the map was wrong, and what each correction cost
|
||||
|
||||
| Correction | Cost |
|
||||
| --- | --- |
|
||||
| `defs/` is not just tables. `defs/types.ts` (684 lines) and `defs/tlvs.ts` are half the wire codec: read, write and size for every field, plus the TLV stream. `pdu.ts` is only the envelope and the body rules. | Moderate. "Where is a field written" lands in `defs`, not `pdu`. |
|
||||
| `drain.ts` also holds `IdleWaiters`, which `SendWindow` and `HeldMessages` import. The send window depends on the shutdown module. | Low, but it breaks the "which way do imports point" picture. |
|
||||
| `held-messages` is not a reassembly buffer. It is the inbound messages the *application* has not answered yet. | Moderate: a name-level misread. |
|
||||
| `link-life` is liveness plus a queue: the phase state machine, and where a request waits for the next link. | Low. |
|
||||
| Bind handling is not in the session. `server.ts` does it through the `onRequest` hook (`handleRequest`), and `client.ts` has its own `bind()`. | Moderate: two homes for one protocol step. |
|
||||
| `udh.ts` reads UDHs. Writing one is hand-rolled in `message.ts:114` (`0x05,0x00,0x03,…`), and the outbound reference counter `ConcatReference` sits in `udh.ts`. | Low. |
|
||||
| `decodeSegments` lives in `reassembly.ts` but is used by `sms.ts`. | Low. |
|
||||
| `session.ts` is thinner than I expected (424 lines): a coordinator, not a god class. | Pleasant surprise. |
|
||||
|
||||
## 3. Fan-out, level by level
|
||||
|
||||
- **L0, the README:** about 10 areas. Fine.
|
||||
- **L1, `src/`:** 37 flat files plus `defs/` (7). **This is the worst level.** Nothing in the layout groups the 10 areas, so only filenames and the `AGENTS.md` list stand in for directories.
|
||||
- **L2, `session.ts`:** 10 collaborators (DlrMerger, HeldMessages, IncomingRequests, LinkLife, LinkTimers, OutgoingRequests, PduTransport, ReconnectLoop, ConcatReference, drain), plus 6 LinkEffects and 5 LinkEvents. At the edge but legible.
|
||||
- **L3:**
|
||||
- `IncomingRequests`: 8 (Reassembler, DlrMerger, HeldMessages, Session, bind-direction, dlr, concat, sms-id).
|
||||
- `OutgoingRequests`: 4.
|
||||
- `HeldMessages`: 5.
|
||||
- `server.ts`: about 6.
|
||||
- `client.ts`: about 8 local functions and a second `ReconnectLoop`.
|
||||
- **`defs/`:** 7. Fine.
|
||||
|
||||
## 4. Names
|
||||
|
||||
**One name over two concepts:**
|
||||
- **"unanswered"** means inbound messages the application has not answered (`drain.ts:52` `messagesUnanswered`, README "Unanswered messages"). It also means our requests the peer did not answer (`UnansweredError`, `SendSmsResult.unanswered`, `SendDlrResult.unanswered`). The drain waits on both kinds in one function.
|
||||
- **"held"** means `HeldMessages` (inbound, unanswered by the app). It also means a request waiting for a link: `link-life.ts:191` "holding a request until a link is back", and `outgoing-requests.ts:119` `const held = await waitForLink()`.
|
||||
- **`answered`** in `createSms` (`sms.ts:81`) is a mutable `{ smsId }` box, while `handlers.answered` is the release callback. They are two things on adjacent lines.
|
||||
|
||||
**Names that mislead:**
|
||||
- `EncodingName 'ASCII'` is GSM 03.38.
|
||||
- `drain.ts` houses the general `IdleWaiters`.
|
||||
- `defs/` houses the codec.
|
||||
- `ExpiringGroups` expires nothing itself. Its own doc at `expiring-groups.ts:19` says owners sweep and only `weigh()` evicts.
|
||||
- `session-options.ts:63` documents `shutdownTimeout` as "how long a drain waits for the requests already on the wire". It also bounds the messages half. That comment is false on exactly the 3am path.
|
||||
|
||||
**One concept, many homes:**
|
||||
- **Defaults live in five places:** `client.ts:45`, `server.ts:403` (idleTimeout 40 000 here vs 2 × enquireLink in the client), `session-options.ts:74`, `reconnect-loop.ts:5` `backoffDefaults`, and `reassembly.ts` `defaultMaxOctets`. The last duplicates `defaults.maxHeldOctets` (same 64 MiB).
|
||||
- **Two near-identical collectors:** `send-sms.ts:274` `collectSent` and `sms.ts:188` `collectReceipt`.
|
||||
- **Five concat names:** `Concat`, `ConcatInfo`, `concatOf`, `concatInfo`, `ConcatReference`, spread over `concat.ts` and `udh.ts`.
|
||||
|
||||
## 5. What I would restructure, ranked
|
||||
|
||||
1. **Group `src/` into about five directories:** `link/`, `outbound/`, `inbound/`, `codec/`, `receipts/`. The seams already exist in the imports; only the layout hides them.
|
||||
2. **Give defaults one home:** a single `defaults` module, with the client/server differences expressed as named overrides.
|
||||
3. **Split "unanswered" into two words**, for example "unreleased" for app-side messages and "unanswered" for peer-side requests. Move `IdleWaiters` out of `drain.ts`.
|
||||
4. **Put UDH read and write in one file,** together with `ConcatReference`, and move `decodeSegments` next to `messageOctets`.
|
||||
5. **Rename `defs/types.ts`** to what it is, the wire field codec, or move it beside `pdu.ts`.
|
||||
|
||||
**What the structure gets right:**
|
||||
- `LinkLife.transition()` is one explicit table returning effects, and `Session.run` (`session.ts:269`) is the only interpreter of them.
|
||||
- Collaborators take narrow option objects.
|
||||
- The result-everywhere rule is applied uniformly.
|
||||
- `drain()` names its two halves, and `report()` logs which half was left over.
|
||||
- Comments cite the SMPP section or the operator that forced each odd rule.
|
||||
|
||||
## 6. The 3am question
|
||||
|
||||
A graceful shutdown hangs until `shutdownTimeout` although `sendResp()` was called on everything.
|
||||
|
||||
**Route, cold:** `Session.close` (`session.ts:254`) → `Session.drain` (`session.ts:300`) → `drain()` (`drain.ts:96`). That took about 3 minutes, because the filename and the function name agree. Deciding which half took about 10 more minutes:
|
||||
- With every `sendResp` done, `HeldMessages.idle` should settle. Release happens at `held-messages.ts:170` via `sms.ts:159`.
|
||||
- So the right unit is the second half: `OutgoingRequests.idle` (`outgoing-requests.ts:109`) → `SendWindow.idle`/`unfinished` (`send-window.ts:65`).
|
||||
- That means a request of ours is still in flight. Typically it is the `sendDlr()` receipts that `requestPastDrain` let through, or a heartbeat `enquire_link` the peer is not answering.
|
||||
- Each such request is bounded by `responseTimeout` (30 s), which is longer than `shutdownTimeout` (5 s).
|
||||
|
||||
**Second suspect:** `sendResp` returned `err` because the write failed, and the application ignored it. The message is then never released (`sms.ts:159` releases only on success).
|
||||
|
||||
The log lines `drain - shutting down with requests unfinished` and `drain - shutting down with messages unanswered` separate the two. The false comment at `session-options.ts:63` costs a detour.
|
||||
|
||||
**Where it rots first:**
|
||||
- **`HeldMessages`:** six numbered exits. A listener count taken via `listenerCount('sms')` at `keep()` (`held-messages.ts:154`) and decremented through `captureRejections` routed from `session.ts:97`. A `WeakMap` keyed on the handle's identity, and a back-reference to the half-constructed `Session` (`session.ts:137`).
|
||||
- **The link-generation checks:** repeated in `sms.ts` (`lostLink`), `incoming-requests.ts:90/97` and `held-messages.ts:95`. Each is a local copy of one invariant.
|
||||
|
||||
**Where the next two features land:**
|
||||
- **Goal-9 store:** behind `ExpiringGroups`, whose three owners (`DlrMerger`, `Reassembler`, `HeldMessages`) each use a different subset of its semantics: sweep callbacks, `weigh()` eviction, `full`. A store has to replicate all three contracts.
|
||||
- **Per-PDU rate-limit hook (goal 7):** `OutgoingRequests.carry` (`outgoing-requests.ts:114`), between `window.acquire` and `attempt`. That place is clean and local.
|
||||
- A custom alphabet would instead ripple through the closed `EncodingName` union: `message.ts:14` `segmentUnits`, `dataCodingByEncoding`, and the `send-sms` checks.
|
||||
|
||||
## 7. Hardest places, ranked
|
||||
|
||||
1. **`src/held-messages.ts:40`, `HeldMessages`:** `offer` :94, `listenerRejected` :117, `keep` :154, `release` :170. It has six exits, identity-keyed release, a listener-count countdown, and is coupled to the `Session` emitter.
|
||||
2. **`src/outgoing-requests.ts:70/85/101`, `request` / `requestPastDrain` / `requestOnCurrentLink`:** three entry points that differ in which gates they skip (drain refusal, link wait, window). The bind bypass is buried inside `requestPastDrain`, and the `isStopping() && canCarry()` gate needs a second read.
|
||||
3. **`src/link-life.ts:74`, `LinkLife.transition`,** with `lose` :148 and `end` :159. A `stopping` flag orthogonal to the phase, `'bound'` while stopping turning into a loss, and effect order that matters in `Session.run` (`session.ts:269`, where `dropLink` clears four stores).
|
||||
4. **`src/drain.ts:70/96`, `answeringBudget` and `drain`:** `0` means forever except for the messages half, plus a `setImmediate` turn whose job is to catch a receipt issued right after the last answer.
|
||||
5. **`src/reassembly.ts:188`, `Reassembler.trim`,** with `collect` :111. The eviction arithmetic (`parts.size - 1` when the current group is itself evicted) is correct but has to be derived.
|
||||
6. **`src/expiring-groups.ts:19/70`, `ExpiringGroups.weigh`:** it evicts, `set` does not, and owners must sweep. Its callers' correctness depends on remembering which.
|
||||
7. **`src/pdu.ts:84/113`, `resolveShortMessage` / `resolveBody`:** which field the `data_coding` describes, and when detection may overwrite it.
|
||||
8. **`src/client.ts:263/286`, `initialAttempts` / `keepTrying`:** a second `ReconnectLoop`, with a fresh `Session` per attempt, for `fromStart`.
|
||||
|
||||
**Single unit I would least want to modify:** `HeldMessages` (`src/held-messages.ts`).
|
||||
|
||||
## 8. Intrinsic difficulty
|
||||
|
||||
This is an SMPP session layer with reconnect, graceful drain, bounded reassembly, receipt merging and a hand-rolled wire codec. The problem itself is moderately high in difficulty, and gets no bonus in the scores.
|
||||
|
||||
## 9. Scores
|
||||
|
||||
- **Navigation 7:** matches the "predictable" anchor. Descriptive filenames and function names took me from the symptom to `drain()` in minutes, and `report()` logs which half stalled. The flat 37-file `src/` and the false `shutdownTimeout` comment keep it from 8.
|
||||
- **Locality 6:** between "honest middle" and "predictable". `HeldMessages` and `IncomingRequests` call back into `Session` (emit, `sendReturn`, `close`), the link-generation invariant is copied into three places, `Session.run` effect order is load-bearing, and defaults live in five files.
|
||||
- **Shape 6:** between the anchors. L2 and L3 fan-out is bounded and most names tell the truth. `src/` fans out to 37 files, and "unanswered", "held", `'ASCII'`, `drain.ts` and `defs/` each name something other than what they hold.
|
||||
- **Self-sufficiency 7:** the "predictable" anchor. Invariants are stated at the code (ExpiringGroups' contract, the six exits, SMPP section citations), and I did not need a second document open. It misses 8 on one false option comment and a few budget rules (`answeringBudget`) that need rereading.
|
||||
- **Overall 6:** capped at the lowest dimension plus one (7). It sits at 6 because the hardest code (the held-message flow and outbound gating) is exactly where locality is weakest.
|
||||
|
||||
SCORES nav=7 loc=6 shape=6 self=7 overall=6
|
||||
@@ -0,0 +1,658 @@
|
||||
# Round 2: drafts C and D
|
||||
|
||||
## Draft C, junior seat
|
||||
|
||||
1. **Hardest places, ranked hardest first**
|
||||
|
||||
1. **`src/session.ts:336-404` (`drain`, `dropSocket`, `linkLost`, `end`), `comeBackUp` at `:303`, and `src/session/link-life.ts:17-82` (`LinkLife`'s phase plus its predicates).** Link state is a four-value `Phase` plus a separate `stopped` flag. Seven predicates read it: `isAttached`, `isUp`, `isOver`, `isStopped`, `retrying`, `awaitsNextLink`, `refusal`. On top of that, `OutgoingRequests.canCarry()` adds `!sock.destroyed`. To follow `comeBackUp`'s `!this.link.retrying() || !this.link.isUp()` (`:319`) or `drain`'s two `canCarry()` checks, I had to hold every phase transition at once. `linkLost` is order-dependent ("Read first: a `disconnected` listener may close() the session"), so a synchronous listener re-enters the session in the middle of the method. The comments resolved each line on its own. The whole state machine never became clear to me; I would need to draw it.
|
||||
|
||||
2. **`src/session/outgoing-requests.ts:83-150` (`request`, `carry`, `attempt`).** The three lanes, the `for(;;)` retry loop, `window.release()` in a `finally`, and `pending.wait()` registered before `write()` all interact. The loop-exit comment at `:118` ("Until the link is dropped it admits the retry straight back onto the dead socket, and the loop spins") took three rereads. The `Lane` doc comment at `:20-26` rescued the lanes. The retry exit stayed half-opaque.
|
||||
|
||||
3. **`src/session/incoming-requests.ts:103-271` (`handle`, `route`, `onDelivery`, `onMessage`, plus `refusedSegmentStatus` at `:28`).** This is where the domain costs the most. `data_sm` routes by `carriedAs`. `deliver_sm` might be a receipt or a message, and `onDelivery` falls through to `onMessage`. The status codes come as a family: `ESME_RX_P_APPN`, `ESME_RX_T_APPN`, `ESME_RTHROTTLED`, `ESME_RINVTLVVAL`, `ESME_RINVESMCLASS`. The `arrivedOn` socket-identity check at `:111` is state compared across an `await`. The flow reads cleanly, but I only knew why each status was right after reading README "Receiving in depth" and "Server in depth".
|
||||
|
||||
4. **`src/messages/reassembly.ts:110-206` (`Reassembler.collect`, `trim`) with `src/messages/expiring-groups.ts:70-88` (`weigh`).** `weigh()` can evict the group currently being added to. `trim` then counts `parts.size - 1` for that group, because the segment just added is refused and so is not lost. The contract is split across two classes: `ExpiringGroups` "enforces neither max nor timeout itself", so every owner must remember to call `full` or `takeExpired`. It resolved after reading the `ExpiringGroups` doc comments. Action at a distance, but it is marked.
|
||||
|
||||
5. **`src/messages/dlr.ts:157-238` (`messageType`, `receiptStatus`, `dlrFromPdu`).** A four-value `MessageType` is derived from `esm_class` bits and then from a TLV. The TLV state and the body's state take precedence over each other in different ways. `statusId` falls back to `UNKNOWN`, and an `unmarked` PDU without both an id and a state is not a receipt. The comments cite spec sections (5.3.2.26, Appendix B) that I cannot open. The README `Dlr` field table is what finally made the output shape make sense.
|
||||
|
||||
6. **`src/client.ts:219-322` (`bindOn`, `initialAttempts`, `keepTrying`).** There are two routes into `ReconnectLoop`: the session's own, and a second one here that builds a fresh `Session` per attempt. There is closure state (`lastErr`, `settled`) and abort-listener bookkeeping. The comment at `:241` ("close() must reach the loop's stop() before its first await") depends on how `Session.close` is ordered inside, in another file. It stayed partly opaque.
|
||||
|
||||
7. **`src/wire/pdu.ts:84-136` (`resolveShortMessage`, `resolveBody`).** `CodingSource` decides whether `short_message` or `message_payload` owns `data_coding`. The cases are Buffer or string, empty or not, crossed with a string TLV being present. That is four or more branches returning objects that are almost the same. The `CodingSource` doc comment helped, but only after I had read `messageOctets()` in `message-body.ts`.
|
||||
|
||||
8. **`src/defs/encodings.ts:151-190` (`messageClassOf`, `messageClassEncoding`, `encodingByDataCoding`).** Bit masking over GSM 03.38 coding groups, which I had no context for. `messageClassEncoding` has a `//` comment and a `/** */` comment stacked on top of it that say different things. It stayed opaque, and I would trust the tests over my reading.
|
||||
|
||||
2. **The unit I'd least want to modify:** `Session.linkLost`, `dropSocket` and `end` (`src/session.ts:366-404`) together with `LinkLife`. They are re-entered from the transport (close, error, unreadable), from `LinkTimers.onIdle`, from `comeBackUp`, from `unbind` and `close`, and synchronously from application listeners (`disconnected`, `close`). The correctness depends on call order and on idempotence guards (`drop()` returning false, `end()` returning false). None of that is visible at any single call site.
|
||||
|
||||
3. **Expected hard, found easy:**
|
||||
- The codec in `defs/types.ts`: 684 lines, but it is one pattern repeated (read, size, write, all returning Results).
|
||||
- `PduFramer`.
|
||||
- The no-throw `Result` convention, which is applied the same way everywhere.
|
||||
- `send-sms.ts`: `checkOptions` is a flat chain of guards.
|
||||
- `bind-direction.ts`.
|
||||
- The folder layout. The AGENTS.md architecture map matched the files one to one, and a one-line purpose per file made cold navigation fast.
|
||||
|
||||
4. **Prose debt**
|
||||
- **What I needed and what it cost:**
|
||||
- The basic domain vocabulary: ESME vs SMSC/MC, bind types, what `deliver_sm` doubles as, `esm_class`, `data_coding`, UDH vs `sar_*`, `registered_delivery`. There is no glossary anywhere. I rebuilt it from README "Receiving in depth", "Delivery receipts" and "Bind direction", which cost a pass over a 764-line README with a lot of jumping.
|
||||
- The AGENTS section "GSM 7-bit is sent unpacked", which was essential for `segmentUnits` in `message.ts`.
|
||||
- Many code comments cite SMPP section numbers with no summary, so they are pointers I could not follow.
|
||||
- The AGENTS decision index names choices, for example "Every request leaves through one `request()`, in one of three lanes", whose reasoning is in `docs/decisions.md`, which was out of bounds. The titles alone helped a little.
|
||||
- I did not open any test.
|
||||
- **Prose that added nothing the code didn't already say:**
|
||||
- The seven `declare` listener lines in each emitter class.
|
||||
- `OutgoingRequests.deliver` ("False means nothing was") and `PendingRequests.deliver`, both restating their code.
|
||||
- README's "Everything exported" table, which repeats `index.ts`.
|
||||
- The AGENTS Conventions paragraph about test fixtures, for reading `src/`.
|
||||
- The 0.4.0 defect table, which is history and says nothing about the current code's structure.
|
||||
|
||||
5. **Scores**
|
||||
- **Navigation: 7.** It sits at "predictable": the `session/`, `messages/`, `wire/`, `defs/` split plus the per-file map in AGENTS.md got me from a symptom to a file on the first try. It stays below 8 because concatenation logic is split three ways (`concat.ts`, `udh.ts`, `reassembly.ts`), refusal statuses are split between `pdu-refusal.ts` and `incoming-requests.ts`, and `udh.ts` holds `ConcatReference`.
|
||||
- **Locality: 6.** It is between "honest middle" and "predictable". The hard parts are marked, but `IncomingRequests` holds the `Session` and calls back into it (`emit`, `sendReturn`, `close`, `sock`, `linkEnd`). `LinkLife` is shared by `Session` and `OutgoingRequests`. `ExpiringGroups` depends on its owners to sweep. `linkLost` is re-entrant through listeners.
|
||||
- **Shape: 6.** Files are small and fan-out is bounded, but some names mislead:
|
||||
- The GSM7 codec is called `ascii`.
|
||||
- `ExpiringGroups<true>` is used as a "spent" set.
|
||||
- `linkLost()` means something different in `Session`, `OutgoingRequests` and `IncomingRequests`.
|
||||
- `refusal()` returns an `Error` on `LinkLife` and an `ErrorName` on `IncomingRequests`.
|
||||
- `IdleWaiters.settle()` wakes waiters whatever the count reads.
|
||||
- **Self-sufficiency: 5.** Honest middle. The comments are dense and often state invariants. But for a reader with no SMPP background, the domain terms and bare spec citations mean the README has to stay open beside `incoming-requests.ts`, `dlr.ts` and `encodings.ts`.
|
||||
- **Overall: 6.** Capped at self-sufficiency plus one. The layout and conventions are clearly cared for; what costs a junior is the lifecycle state machine and the unexplained domain.
|
||||
- **Intrinsic difficulty:** high. It is an asynchronous protocol session with reconnect, drain, windowing and reassembly under hostile input. That gets no bonus in the scores above.
|
||||
|
||||
SCORES nav=7 loc=6 shape=6 self=5 overall=6
|
||||
|
||||
## Draft C, mid seat
|
||||
|
||||
1. **Hardest places, hardest first**
|
||||
|
||||
1. **`src/session.ts:303-404`: `Session.comeBackUp` / `drain` / `dropSocket` / `linkLost` / `end`.** Whether the link is alive is held in four places:
|
||||
- `LinkLife.phase`, which is `binding`, `up`, `down` or `ended`
|
||||
- `LinkLife.stopped`
|
||||
- `ReconnectLoop.halted`
|
||||
- `transport.sock.destroyed`
|
||||
|
||||
Order matters in several spots, and only comments say so. `linkLost` reads `retrying()` before the drop because a `disconnected` listener may call `close()`. `comeBackUp` checks `!retrying() || !isUp()` after `bind()`. That only makes sense once you find that `bind()` reaches `session.bound()`, which calls `link.open()` out of sight. `canCarry()` (`outgoing-requests.ts:58`) asks both `link.isUp()` and `sock.destroyed`, and nothing explains why both are needed. The comments helped, but I had to trace the state by hand and it is still not fully clear to me.
|
||||
2. **`src/session/outgoing-requests.ts:83-150`: `OutgoingRequests.request` / `carry` / `attempt`.** The loop in `carry` has three waits: the link budget, a window slot, and the response. The window slot is released in a `finally`, and a retry is allowed only when `attempt.retry && link.awaitsNextLink()`. You need LinkLife's phase logic in your head (`link-life.ts:71-82`) to see why the loop ends. The comment "the loop spins" warns about it but does not explain it. The three lanes are well documented in the `Lane` type's comment (line 25). This resolved, slowly.
|
||||
3. **`src/wire/pdu.ts:84-136`: `resolveShortMessage` / `resolveBody`.** The code decides whether `short_message` or `message_payload` owns `data_coding`. An empty Buffer means `message_payload`, and a string body gets encoded and can overwrite `data_coding`, but only for one of the two sources. I had no domain context and read it three times. The `CodingSource` comment helped, and the README "Building" bullets confirmed what it is meant to do.
|
||||
4. **`src/session/incoming-requests.ts:103-271`: `handle` / `route` / `onMessage` / `refusedSegmentStatus`.** A `data_sm` becomes `submit_sm` or `deliver_sm` depending on `linkEnd` (via `standsInFor`). A `deliver_sm` that is not a receipt falls through to `onMessage`. The status codes (`ESME_RX_T_APPN`, `ESME_RTHROTTLED`, `ESME_RINVTLVVAL`, `ESME_RINVESMCLASS`) are domain rules I cannot check. The class also calls back into `Session` through `sock`, `emit`, `sendReturn` and `close`. The socket-identity check after the `onRequest` await was clear once the comment was read. The status choices stayed opaque; I trusted README "Receiving in depth".
|
||||
5. **`src/messages/reassembly.ts:110-206` with `expiring-groups.ts:70-88`: `Reassembler.collect` / `trim` and `ExpiringGroups.weigh`.** The rules are split between the two classes:
|
||||
- ExpiringGroups enforces neither max nor timeout, and its owners must.
|
||||
- `set()` resets weight to zero.
|
||||
- `weigh()` may evict the key you are weighing.
|
||||
- `trim` works out `answered = parts.size - 1` for the current group.
|
||||
|
||||
The comments state each rule, but I needed all of them at once. This resolved.
|
||||
6. **`src/messages/dlr-merger.ts:105-185`: `DlrMerger.collect` / `open` / `spend`.** There are two stores (`groups` and `spent`), and `spend()` is the exit for four different situations: completion, expiry, reuse and eviction. The class docblock and the `severity` comment explain it. This resolved.
|
||||
7. **`src/defs/encodings.ts:162-190`: `messageClassEncoding` / `encodingByDataCoding`.** This is bit-level decoding of `data_coding`. The comments say which bits are which but not the table behind them. The code is short, but it stayed opaque without the GSM 03.38 spec.
|
||||
8. **`src/client.ts:220-322`: `bindOn` / `initialAttempts` / `keepTrying`.** A second `ReconnectLoop` is built outside the session for `fromStart`. `bindOn` depends on `close()` reaching `stop()` before its first await, which the comment at line 241 states. There are also two abort listeners with different lifetimes. This resolved with effort.
|
||||
|
||||
I did not open any tests.
|
||||
|
||||
2. **Unit I would least want to modify:** the session lifecycle cluster, `Session.linkLost` / `dropSocket` / `end` / `comeBackUp` together with `LinkLife`. Every change there interacts with the reconnect loop's own stopped flag, with the drain's `canCarry()` checks before and after it waits, and with whatever an event listener does during `emit`. Tests would be the only way to know a change is safe.
|
||||
|
||||
3. **Expected hard, found easy:**
|
||||
- `PduFramer`
|
||||
- the parse path in `pduToObj`/`parsePdu`
|
||||
- `PendingRequests`
|
||||
- `SendWindow`
|
||||
- `ReconnectLoop` backoff
|
||||
- the listener guards that stop an application listener's throw or rejection from escaping (the no-throw rule)
|
||||
- the `defs/` tables
|
||||
- `defs/types.ts`, which is 684 lines but repetitive and predictable
|
||||
|
||||
The rule that nothing throws makes every call site easy to read.
|
||||
|
||||
4. **Prose debt**
|
||||
- **Needed:**
|
||||
- README "Shutdown" and "Sends and the link", to understand the lanes and the drain.
|
||||
- README "Receiving in depth", for which way a `data_sm` goes and the throttle statuses.
|
||||
- The AGENTS architecture map, which is accurate and was the best way in.
|
||||
- The AGENTS decision index lines, such as "Every message is answered on arrival" and "One owner decides whether a link can carry a request". They gave intent cheaply because each is one line.
|
||||
- **Cost to find:** low, because the map and the index point to the right places. The code's references to spec sections (e.g. "SMPP 3.4 5.2.19") assume a document I do not have.
|
||||
- **One false claim:** AGENTS says "`wire` uses `defs`, `messages` uses `wire`", but `wire/pdu.ts:10` imports `decodeMessage` and `encodeBody` from `messages/message.ts`. So wire and messages depend on each other.
|
||||
- **Told me nothing:**
|
||||
- the AGENTS 0.4.0 defect table (history, not needed to read the current code)
|
||||
- most of the test-convention prose in AGENTS
|
||||
- the README feature bullets
|
||||
- one-line docstrings that restate the method name, such as `isStopped` and `get sock`
|
||||
- **Duplicated code:** `quoted()` is duplicated in `bind-direction.ts` and `session-options.ts`. `collectSent` and `collectReceipt` are near-copies.
|
||||
|
||||
5. **Scores**
|
||||
- **Navigation: 7 (Predictable).** The AGENTS file map matches the layout, and the `session/` / `messages/` / `wire/` grouping took me straight to the right file. It falls short of 9 because answering a request is split across `server.handleRequest`, `IncomingRequests.unhandled` (`ESME_RALYBND`) and `Session.refuse`.
|
||||
- **Locality: 6 (between 5 and 7).** The collaborators are small and injected. But link liveness is spread over `LinkLife`, `ReconnectLoop.halted` and `sock.destroyed`, and three comments carry order rules: "Read first", "must reach stop() before its first await", and "the loop spins". `IncomingRequests` also reaches back into `Session`.
|
||||
- **Shape: 7 (Predictable).** Fan-out at each level is bounded, and the `Session` constructor wires seven named collaborators. Some names mislead:
|
||||
- the GSM codec is called `ascii` (`encodings.ts:45`)
|
||||
- `idle` means three different things: `IdleWaiters`, `ExpiringGroups.idle()` and the `LinkTimers.idle` timer
|
||||
- the near-synonyms `stop`, `end`, `close`, `release`, `linkLost` and `dropSocket` blur which one is final
|
||||
- **Self-sufficiency: 7 (Predictable).** Almost every non-obvious branch carries a one-line reason, often with a spec section. The lanes, the drain and the refusal statuses still needed the README open beside the code.
|
||||
- **Overall: 6.** It is capped by Locality. The lifecycle corners are marked, but they are not contained.
|
||||
|
||||
The problem is hard in itself: a protocol full of peer quirks, plus async lifecycle with reconnect and drain. That gets no bonus here.
|
||||
|
||||
SCORES nav=7 loc=6 shape=7 self=7 overall=6
|
||||
|
||||
## Draft C, senior seat
|
||||
|
||||
1. **Hardest places, ranked**
|
||||
|
||||
1. **`src/session.ts:303-404`, the `Session` lifecycle: `comeBackUp`, `drain`, `stop`, `dropSocket`, `linkLost`, `end`.** Whether the session is still alive is spread across three places: `LinkLife`'s phase and its separate `stopped` flag, `ReconnectLoop.halted`, and `link.end()`. To follow any one of these methods I had to hold all three. `comeBackUp:319` tests `!retrying() || !isUp()`. That only makes sense once you know `isUp()` got set as a side effect: the `onConnected` callback in `client.ts:bind` calls `session.bound()`, which calls `link.open()`. The ordering is load-bearing in several places. `linkLost:379` has to read `retrying()` before the drop. `end()` calls `dropSocket()`, which is also a guard. `client.ts:241` says "close() must reach the loop's stop() before its first await". The comments at the call sites marked each ordering rule. None of them explained why the whole thing is split this way. It stayed expensive to read.
|
||||
2. **`src/session/outgoing-requests.ts:83-121`, `request` / `carry`, together with `src/session/link-life.ts:34-186`.** `LinkLife` exposes six overlapping predicates: `isAttached`, `isUp`, `isStopped`, `retrying`, `awaitsNextLink` and `refusal`. The lanes mix them. `message` checks `refusal() ?? isStopped()`. `receipt` skips both checks up front and only meets `refusal()` inside `budget()`. `link` skips everything. The loop exits on `!attempt.retry || !awaitsNextLink()`. The comment about it spinning helped, but I had to walk the phase transitions by hand to convince myself. The `Lane` doc comment resolved what each lane is for. It did not resolve what each lane actually checks.
|
||||
3. **`src/messages/expiring-groups.ts:18`, `ExpiringGroups`, with `src/messages/reassembly.ts:110-136, 187-206`, `Reassembler.collect` / `trim`.** The store says outright that it enforces neither its `max` nor its timeout itself. The owner has to check `full`, call `takeExpired()`, and let `weigh()` evict, and `weigh()` can evict the very group being written. In `trim`, `answered = size - 1` for the current key, and the refused segment has already been `set` into the group. That is an invariant spread across two files. The doc comments state the contract honestly, so it was readable, just slow.
|
||||
4. **`src/wire/pdu.ts:84-136`, `resolveShortMessage` / `resolveBody`.** Deciding which field is allowed to set `data_coding` goes through `CodingSource`. An empty Buffer `short_message` counts as source `message_payload`. A string body rewrites `data_coding`, but only when it encodes to something non-empty. Three branches return three different param shapes. The `CodingSource` doc comment is the key, and I only understood it after going back to `message-body.ts`.
|
||||
5. **`src/messages/dlr-merger.ts:68-184`, `DlrMerger` (`open`, `spend`, `dropOldest`).** It keeps a second `ExpiringGroups<true>` as a tombstone set. `spend()` is called on completion, on expiry, on eviction and on id reuse, and each time it deletes and re-inserts the tombstone. The class doc comment explains why ("merged at most once"). I still had to trace which paths end up in `spend` to be sure a straggler receipt is ignored.
|
||||
6. **`src/client.ts:219-322`, `bindOn` / `initialAttempts` / `keepTrying`.** For `fromStart` there are two `ReconnectLoop`s: one in the client and one inside each session. `lastErr` lives in a closure, and `settle` is idempotent. `bindOn` removes its abort listener on failure but deliberately keeps it after success, so a later abort closes a bound session. Only README ("That signal also closes the session once bound") told me that was intended rather than a leak.
|
||||
7. **`src/defs/encodings.ts:483-514`, `messageClassEncoding` / `encodingByDataCoding`.** Bit masks over coding groups I had no background in. There is an orphan `//` comment sitting above a `/** */` doc comment. The GSM 03.38 codec is named `ascii`, and "fall back to ASCII" (line 504) actually means GSM7. That misled me until I checked the `encodings` map.
|
||||
8. **`src/session/incoming-requests.ts:103-153`, `handle` / `route`.** `arrivedOn` is captured before the application's `onRequest` await and compared afterwards. The `unbind` case closes the session with `AbortSignal.abort()` from inside a handler that the dispatch itself is running. Both have comments, and both still needed a second read to be sure nothing re-enters.
|
||||
|
||||
2. **Least want to modify:** the `Session` lifecycle cluster (`session.ts:303-404` plus `LinkLife`). A change to when the link counts as up, stopped or ended touches `LinkLife`, `ReconnectLoop`, `OutgoingRequests.canCarry`, `client.ts` `bindOn`/`bind`, and `IncomingRequests`' `sock` check. Nothing in the types enforces the ordering, so it only lives in the comments.
|
||||
|
||||
3. **Expected hard, found easy:** `PduFramer`; the wire types in `defs/types.ts` (long but uniform); TLV read and write, including repeatable tags and keying by id; the reconnect backoff; `bind-direction.ts`; the `Result` convention; `udh.ts` `concatInfo`; `send-sms.ts` validation, which is a flat checklist; `PendingRequests`; `SendWindow`.
|
||||
|
||||
4. **Prose debt.**
|
||||
- **Documentation I needed:**
|
||||
- README "Reconnect" section, to know the abort listener kept alive in `bindOn` is intended.
|
||||
- AGENTS "GSM 7-bit is sent unpacked", to trust `segmentUnits` GSM7 153 vs UCS2 134. The one-line comment at `message.ts:13` is close to enough on its own.
|
||||
- README "Shutdown", to learn that `drain()` returning `{}` when `!canCarry()` also skips waiting on handlers. The code says "nothing is on the wire", which does not mention handlers.
|
||||
- The charter's decision index points at `docs/decisions.md`, which I was not allowed to open. For several decisions I had only the title and had to take the rest on trust.
|
||||
- **Where the charter's map is wrong:**
|
||||
- It says `messages` uses `wire`. In fact `wire/pdu.ts` imports `messages/message.ts` (`decodeMessage`, `encodeBody`).
|
||||
- `decodeSegments` lives in `reassembly.ts` but is used by `sms.ts`.
|
||||
- `ConcatReference`, a per-session counter, lives in `udh.ts`.
|
||||
|
||||
Each of these cost a wrong turn.
|
||||
- **Prose that told me nothing new:**
|
||||
- `send()`'s "Sends a request and resolves with the peer's response".
|
||||
- `LinkTimers`' "Keeps a quiet connection honest".
|
||||
- `PduFramer`'s "Cuts a byte stream into whole PDUs".
|
||||
- The charter's test conventions, which are irrelevant to reading `src/`.
|
||||
- Most of the decision-index lines, which restate README behaviour.
|
||||
- The 0.4.0 defect table. It is useful history but did not help me read the current code.
|
||||
- **Duplicated code:** `collectSent` and `collectReceipt` are near-copies, `quoted()` exists twice, and the `emit` / `captureRejectionSymbol` guards are copied between `Session` and `SmppServer`.
|
||||
|
||||
5. **Scores.** The problem is intrinsically hard (SMPP session state, reassembly under memory caps, receipt correlation), and that gets no bonus.
|
||||
- **Navigation 7:** near the "predictable" anchor. The `session/`, `messages/`, `wire/`, `defs/` split and the charter's file map took me from symptom to file first try. It is held below 8 by the misplaced units above and the false import-direction claim.
|
||||
- **Locality 6:** between the 5 and 7 anchors. Link lifecycle state is split across `LinkLife`, `ReconnectLoop` and `Session`, with ordering rules marked only by comments. `IncomingRequests` reaches back into `session.sock`, `boundAs` and `linkEnd`.
|
||||
- **Shape 7:** at "predictable". Fan-out per level is small and most names are honest. It is held there by `ascii` for the GSM codec, "fall back to ASCII", `udh.ts` also holding the reference counter, and six overlapping liveness predicates on `LinkLife`.
|
||||
- **Self-sufficiency 7:** at "predictable". Comments cite SMPP sections and state invariants beside the code. Two behaviours (the kept abort listener, the drain skipping handlers) needed README open beside the code.
|
||||
- **Overall 6:** a cold senior would be productive within a week and would know to fear the lifecycle cluster. The locality cost there is what holds it below 7.
|
||||
|
||||
SCORES nav=7 loc=6 shape=7 self=7 overall=6
|
||||
|
||||
## Draft C, architect seat
|
||||
|
||||
**Comprehension panel report: Architect, inherited (draft-c, @larvit/smpp)**
|
||||
|
||||
I opened no test file. I read every non-test source file under `src/`. I read `defs/types.ts` and `defs/tlvs.ts` by outline plus key sections, and `commands.ts`, `constants.ts` and `errors.ts` only as far as their outline.
|
||||
|
||||
## 1. Map from README and file tree only, verbatim
|
||||
|
||||
- **Root, the entry points:** `client.ts` has `client()`; `server.ts` has `server()` and `SmppServer`; `session.ts` has `Session`, the orchestrator; `index.ts` is the public surface.
|
||||
- **Root, cross-cutting:** `result.ts` (Result), `log.ts` (SmppLog), `error-from.ts` (unknown → Error). `defaults.ts` I expect to hold session defaults, and I am unsure how it relates to `session/session-options.ts`.
|
||||
- **`defs/`:** the SMPP spec tables: commands, TLVs, errors, constants, encodings, wire types.
|
||||
- **`wire/`:** the codec. `pdu.ts` is pduToObj/objToPdu, `pdu-framer.ts` turns a byte stream into PDUs, `pdu-refusal.ts` is PduRefusedError.
|
||||
- **`session/`:** what a Session is made of: transport, keepalive timers, reconnect, send window, pending-request correlation, incoming and outgoing requests, bind direction, options. `running-handlers.ts` I guess counts `onSms` promises (README: "1000 handlers", "close() waits for your handlers"). `idle-waiters.ts` and `link-life.ts` are unclear from their names.
|
||||
- **`messages/`:** message-level logic: encode/split, UDH, concatenation, DLR parse and merge, reassembly, the inbound `Sms` handle, sendSms composition, ids, uuid.
|
||||
- **Names that do not give their purpose:** `retained-pdu.ts`, `expiring-groups.ts`, `unanswered-error.ts` (why in messages/?), `uuid.ts` (why in messages/?), `pdu-transport.ts` (session, not wire?), `link-life.ts`.
|
||||
- **README promises I expect to find:** a drain that waits for handlers, then for requests. `smppTime` somewhere in messages. No store (goal 9 says it has not shipped).
|
||||
|
||||
## 2. Where the map was wrong, and what each correction cost
|
||||
|
||||
| Map claim | Reality | Cost |
|
||||
|---|---|---|
|
||||
| `wire/` is the codec | The per-field codec is `defs/types.ts` (684 lines of read/size/write) and `defs/tlvs.ts` (`parseTlvs`/`writeTlvs`). `wire/pdu.ts:10` imports `encodeBody` and `decodeMessage` from `messages/message.ts`, so wire depends on messages. AGENTS.md says the reverse ("`messages` uses `wire`"). | High. I had to reopen `defs/`, and the documented dependency direction is false. |
|
||||
| `defaults.ts` holds session defaults | It holds every option's default plus `bounds` (limits that are not options). | Low. |
|
||||
| `session-options.ts` holds option types | It also holds `SessionEvents`, `OnRequest`, `SmsHandler`, and the validation for client and server options (`CheckableOptions` includes `authenticate`, `connectTimeout`, `fromStart`). | Medium. |
|
||||
| One reconnect concept | `client.ts:278` `keepTrying` runs a second, separate `ReconnectLoop` for `fromStart`. `ReconnectOptions` (session: `connect`/`onConnected`) and the client's `reconnect` (`ReconnectTuning`) are two shapes under one name. | Medium. |
|
||||
| `running-handlers.ts` counts `onSms` promises | Correct. | None. |
|
||||
| `messages/` is message logic | It is a 14-file grab-bag with 5 themes: codec helpers, receipts, bounded stores, sending, ids/errors. | Medium. |
|
||||
|
||||
## 3. Fan-out, level by level
|
||||
|
||||
- **L0, `src/`:** 8 files and 4 directories. It mixes 4 entry points with 4 utilities. Acceptable.
|
||||
- **L1:**
|
||||
- `session/`: 12 files. `Session` composes 7 collaborators plus `ConcatReference`.
|
||||
- `messages/`: 14 files across about 5 themes.
|
||||
- `wire/`: 3 files.
|
||||
- `defs/`: 7 files.
|
||||
- **L2, inside `session.ts`:** about 25 members. The lifecycle cluster alone (`drain`/`stop`/`dropSocket`/`linkLost`/`end`) touches 6 collaborators.
|
||||
- **Worst level:**
|
||||
- By count and cohesion, `messages/` (14 files).
|
||||
- By reading cost, `session/`. `link-life.ts` has 7 near-synonymous predicates, `OutgoingRequests.canCarry` is an 8th, and `ReconnectLoop.isStopped` a 9th.
|
||||
|
||||
## 4. Names
|
||||
|
||||
**Names that mislead**
|
||||
- `encodings.ts:45` `ascii` is the GSM 03.38 codec. The comment at `encodings.ts:180` says "alphabets with no codec fall back to ASCII", but the code returns `'GSM7'`.
|
||||
- `ExpiringGroups` enforces neither the cap nor the expiry; its own doc comment says so at `expiring-groups.ts:18`.
|
||||
- `defs/` is described as "spec tables" but holds most of the codec.
|
||||
- `udh.ts` holds the outbound `ConcatReference` counter next to UDH parsing.
|
||||
- `messages/unanswered-error.ts` is used by `session/outgoing-requests.ts`.
|
||||
|
||||
**Concepts with two names**
|
||||
- `sendSms` and `submitSms` name the same action.
|
||||
- GSM7 and `ascii` name the same codec.
|
||||
- "stopped" is held twice: `LinkLife.stopped` and `ReconnectLoop.halted`, both set by `Session.stop()`.
|
||||
- `smsId`, `message_id` and `base` refer to the same id.
|
||||
|
||||
**One name over several concepts**
|
||||
- `idle`: the `LinkTimers` idle timeout, `IdleWaiters` (a count falling to zero), and `ExpiringGroups.idle()` (stop the sweeper).
|
||||
- `release`: `SendWindow.release` frees a slot, `RunningHandlers.release` wakes the drain, `LinkLife.release` settles link waiters.
|
||||
- `settle`: used everywhere.
|
||||
- `reconnect`: the session's `ReconnectOptions` and the client's `ReconnectTuning`. `checkReconnect` validates only the client shape.
|
||||
|
||||
## 5. What I would restructure, ranked
|
||||
|
||||
1. **Move the codec into `wire/`:** `defs/types.ts` read/write, `defs/tlvs.ts` parse/write, and `encodeBody`/`decodeMessage`. This makes the documented dependency direction true.
|
||||
2. **Split `messages/`** into inbound, outbound and receipts. Move `unanswered-error` to `session/`, and move `uuid` out.
|
||||
3. **Collapse the link predicates** in `LinkLife` into one query per lane (for example `admits(lane)`), absorbing `canCarry` and `ReconnectLoop.halted`.
|
||||
4. **Split `session-options.ts`:** event and hook types in one place, client/server option checking in another.
|
||||
5. **Renames:** `ascii` → `gsm7`, `ExpiringGroups` → something that says it only holds keyed deadlines, and distinct names for the `idle`, `release` and `settle` overloads.
|
||||
|
||||
**What the structure gets right**
|
||||
- `Session` is split into collaborators, each with a one-line owner doc.
|
||||
- `Result` is used uniformly.
|
||||
- Comments record why at the call site (for example `link-life.ts:173`, `reassembly.ts:162`, `dlr-merger.ts:23`).
|
||||
- The AGENTS.md file map is accurate at file level.
|
||||
- Every store is bounded and says so.
|
||||
|
||||
## 6. The 3am question
|
||||
|
||||
**Time and route, cold:** about 5–10 minutes. README "Shutdown" → `session.ts:267` `close()` → `session.ts:336` `drain()` → `incoming.idle` → `session/running-handlers.ts:54` `RunningHandlers.run` and `:89` `idle`.
|
||||
|
||||
**The premise does not match this code.** `sms.sendResp()` does not exist here; a grep for it finds nothing. Every message is answered on arrival (`incoming-requests.ts:233`), and the drain waits for the promise the `onSms` handler returned to settle, not for any answer.
|
||||
|
||||
**Likely cause:** a handler whose promise has not settled. The typical case is a handler awaiting `sms.sendDlr()` while the peer never answers the `deliver_sm`: `responseTimeout` (30 s) is longer than `shutdownTimeout` (5 s). The second place to look is the phase after it: `outgoing.idle` → `SendWindow.unfinished()` (`send-window.ts:82`), which counts queued waiters as well as requests on the wire.
|
||||
|
||||
**Adjacent hazard (plausible, not confirmed):** `session.ts:340` returns before waiting for handlers when the link cannot carry requests. A `close()` during a reconnect gap therefore skips the handler wait, which README step 2 says always happens. A slow `onRequest` hook is never counted by the drain either.
|
||||
|
||||
**Where it rots first:** the lifecycle cluster in `session.ts:336-404`. Its correctness depends on call order: `linkLost` reads `retrying()` before `dropSocket`, and `end` calls `stop` and `dropSocket` before the phase check. Every new link state adds a predicate to `LinkLife`.
|
||||
|
||||
**Where the next two features land:**
|
||||
- Goal 9's store cuts across `DlrMerger`, `Reassembler` and `ExpiringGroups` in `messages/`, and `RunningHandlers` in `session/`. It has no single seam today.
|
||||
- A per-PDU rate limit (goal 7) becomes a fourth wait in the `OutgoingRequests.carry` loop (`outgoing-requests.ts:100`).
|
||||
|
||||
## 7. Hardest places, ranked
|
||||
|
||||
1. `src/session.ts:336-404`, `drain`/`stop`/`dropSocket`/`linkLost`/`end`: order dependence, and the early return that skips handlers.
|
||||
2. `src/session/link-life.ts:50-82`, the `LinkLife` predicates: phase × stopped × reconnects expressed as 7 booleans.
|
||||
3. `src/session/outgoing-requests.ts:83-121`, `request`/`carry`: 3 lanes and a retry loop whose own comment warns that it spins.
|
||||
4. `src/session/incoming-requests.ts:103-128`, `IncomingRequests.handle`: holds a Session back-reference, awaits `onRequest`, then re-checks the socket. The session is reachable by two routes: the object and the `sendReceipt` closure.
|
||||
5. `src/messages/reassembly.ts:110-206`, `Reassembler.collect`/`trim`: eviction by weight, with the `parts - 1` accounting for a segment that was refused.
|
||||
6. `src/messages/dlr-merger.ts:150-173`, `DlrMerger.open`/`spend`: a second `ExpiringGroups` used as a set of spent ids.
|
||||
7. `src/wire/pdu.ts:84-136`, `resolveShortMessage`/`resolveBody`: which source's `data_coding` wins.
|
||||
8. `src/client.ts:250-322`, `keepTrying`/`initialAttempts`: a second reconnect mechanism.
|
||||
|
||||
**The unit I would least want to modify:** the `Session` lifecycle cluster, `session.ts:366-404` (`dropSocket`/`linkLost`/`end`).
|
||||
|
||||
## 8. Scores
|
||||
|
||||
The problem is intrinsically hard: SMPP session lifecycle with reconnect, drain, send window and bounded reassembly. That earns no bonus.
|
||||
|
||||
- **Navigation 7.** Sits at "Predictable": the AGENTS.md map and file names took me from symptom to `RunningHandlers` in minutes. It is held below 8 because the codec is split between `defs/` and `wire/`, and `messages/` is a grab-bag.
|
||||
- **Locality 6.** Between "Honest middle" and "Predictable". The collaborators are real. It is held there by `IncomingRequests` holding a `Session` back-reference, by stopped/up state spread across `LinkLife`, `ReconnectLoop` and `transport.sock.destroyed`, and by the order-dependent `end`/`linkLost`.
|
||||
- **Shape 6.** Between the anchors. It is held there by the 14-file `messages/`, a `LinkLife` API of 9 predicates, the wire→messages import that contradicts AGENTS.md, and names that lie (`ascii`, `ExpiringGroups`, `idle`/`release` overloads).
|
||||
- **Self-sufficiency 7.** Sits at "Predictable": the invariants and whys are stated beside the code. It is held below 8 because the drain's early return at `session.ts:340` contradicts the README's shutdown contract, and the file map's layering claim is false.
|
||||
- **Overall 6.** Capped at 7 by the lowest dimension plus one. It sits at 6 because the hardest code (the lifecycle and the link predicates) is exactly where the order dependence lives.
|
||||
|
||||
SCORES nav=7 loc=6 shape=6 self=7 overall=6
|
||||
|
||||
## Draft D, junior seat
|
||||
|
||||
1. **Hardest places, hardest first**
|
||||
|
||||
1. **`src/handled-messages.ts:92` `HandledMessages.offer`, `:125` `run`, and `src/sms.ts:83` `createSms`.**
|
||||
- `offer` creates an object whose own closure sets `handled.answered`. `sendResp` then takes one of two paths depending on `answeredAs` (`sms.ts:97`).
|
||||
- After a handler fails, `run` calls `sms.sendResp({ status: retryStatus })` (`:133`). For a multipart message, `answeredOnArrival()` returns an `err` there and nobody reads it.
|
||||
- I had to trace three files to see that this is intended: the segments were already answered, so there is nothing left to refuse. No comment at `:133` says so.
|
||||
- Still opaque: what "settle" means here, compared with `IdleWaiters.settle`.
|
||||
|
||||
2. **`src/session.ts:239-323`, the shutdown verbs: `unbind`, `close`, `drain`, `finish`, `linkLost`, `dropLink`.**
|
||||
- There are six near-synonyms, plus `LinkLife.stop`/`drop`/`end` underneath them. `stop()` gets called twice (in `drain` and again in `finish`).
|
||||
- `unbind`'s return line, `sent.err && !closedOnUnbind ? … : drained`, needed a truth table.
|
||||
- Re-entrancy: an inbound `unbind` (`incoming-requests.ts:149`) calls `session.close()`, which calls `incoming.drain()` on the same object that is still mid-`route`.
|
||||
- Doc comments helped with each piece. The overall state machine was never written down in one place.
|
||||
|
||||
3. **`src/outgoing-requests.ts:120` `refusal()` and `:74` `request()`, with `src/link-life.ts:31` `LinkLife`.**
|
||||
- `LinkLife` exposes seven predicates (`isAttached`, `isUp`, `isOver`, `isStopped`, `retrying`, `awaitsNextLink`, `refusal`).
|
||||
- Callers mix them with `canCarry()` (which also checks `sock.destroyed`). `refusal()` at `:129` reads `isStopped() && canCarry()`, and its comment ("the link's own refusal names the session closed instead") only made sense once I had held all four phases in my head.
|
||||
- Also surprising: the phase starts at `'up'` before any bind. I had to hunt through `comeBackUp` to see that `'binding'` exists only on reconnect.
|
||||
|
||||
4. **`src/reassembly.ts:186` `Reassembler.trim`, with `src/expiring-groups.ts:70` `ExpiringGroups.weigh`.**
|
||||
- `ExpiringGroups` is a store whose rules its owners enforce: `set()` never evicts, `weigh()` does, `full` is only advisory, and `onSweep` must call `takeExpired()`.
|
||||
- `trim` reweighs the whole group, may evict the group it is working on, and then computes `answered = parts.size - 1` for that one.
|
||||
- The `:199` comment explains the `-1`. It took several rereads to see why `open()` evicts by count and `trim` by weight.
|
||||
|
||||
5. **`src/pdu.ts:84` `resolveShortMessage` and `:113` `resolveBody`.**
|
||||
- `CodingSource` decides whether `short_message` or `message_payload` gets to set `data_coding`. An empty-string `short_message` flips it to `'message_payload'`.
|
||||
- I had to hold four cases at once: Buffer vs string, empty vs not, plus the TLV text. The type comment at `:74` is accurate but dense.
|
||||
- I had no domain context for why a body could live in two places. The README's "Where the body is" resolved that.
|
||||
|
||||
6. **`src/client.ts:253` `initialAttempts`, `:276` `keepTrying`, `:218` `bindOn`.**
|
||||
- This is a second use of `ReconnectLoop`, separate from the one `Session` owns. It builds a fresh `Session` per attempt, and a `lastErr` closure is shared across attempts.
|
||||
- `bindOn` comes with an ordering warning: "close() must reach the loop's stop() before its first await". Checking that required reading `Session.close` → `drain` → `reconnectLoop.stop()`.
|
||||
- It resolved once I saw that `fromStart` is the only path into this code.
|
||||
|
||||
7. **`src/dlr-merger.ts:150` `open`, `:166` `spend`.**
|
||||
- There are two `ExpiringGroups`, and the second one (`spent`) is a tombstone set. `spend` deletes from both, evicts the oldest tombstone, then re-adds.
|
||||
- The class doc (`:62-67`) explains the "merged once" rule. Without it this would have stayed opaque.
|
||||
|
||||
8. **`src/defs/encodings.ts:162` `messageClassEncoding`, `:182` `encodingByDataCoding`, `:45` `ascii`.**
|
||||
- Bitmask rules come from a spec I have not read. The codec named `ascii` is actually the GSM 03.38 table (`GSM: ascii`), which misled me at first.
|
||||
- I took the comments on trust; I could not check them.
|
||||
|
||||
I opened no tests.
|
||||
|
||||
2. **The unit I would least want to modify:** `HandledMessages` together with `createSms` (`handled-messages.ts:92-140`, `sms.ts:83-157`). The "answered" state lives in three places: `answer.done` in the closure, `handled.answered`, and `answeredOnArrival`. They are updated by callbacks across two files. Whether the peer gets exactly one response depends on all three agreeing, and a mistake silently double-answers or never answers a request.
|
||||
|
||||
3. **Expected hard, found easy:**
|
||||
- The wire codec. `defs/types.ts` is long but repetitive, and every read and write checks its range the same way.
|
||||
- `PduFramer` and `PduTransport`.
|
||||
- `send-sms.ts`: `checkOptions` is a flat, ordered pipeline.
|
||||
- `sms-id.ts`, `concat.ts`, `udh.ts`: small files whose names tell the truth.
|
||||
- The "nothing throws" rule makes every call site look the same, so I stopped needing to think about control flow.
|
||||
|
||||
4. **Prose debt.**
|
||||
- **Needed:**
|
||||
- I needed domain background: what a DLR is, `esm_class`, `data_coding`, UDH vs `sar_*`, and why `data_sm` changes meaning with direction. None of it is in `src/`.
|
||||
- I found it in README.md sections "Receiving in depth", "Server in depth" and "Delivery receipts". That cost reading about 780 lines to extract about 60 useful ones.
|
||||
- Comments cite SMPP section numbers (e.g. "5.3.2.26", "4.6.2") that a junior cannot resolve without the spec.
|
||||
- The AGENTS.md architecture list was the most valuable single piece: one line per file, and accurate.
|
||||
- **Told me nothing:**
|
||||
- Comments that restate the code: `Session.send` "Sends a request and resolves with the peer's response.", `client()` "Connects to an SMSC and binds.", `LinkTimers.clear` context, and `bindCarries`'s doc, which mostly repeats its three lines.
|
||||
- The long AGENTS.md "Conventions" paragraph on test fixtures (irrelevant to reading `src/`).
|
||||
- The defects table: it is history, not an explanation of the current code, though it did hint at domain pitfalls.
|
||||
|
||||
5. **Scores** (the problem's own difficulty is high: a stateful protocol with reconnect, drain and reassembly, and it gets no bonus here):
|
||||
- **Navigation 7.** Predictable: the AGENTS.md file map plus descriptive file names got me to the right file first try for almost every question. What holds it below 8: one symptom such as "why was this refused with ESME_RTHROTTLED" is spread across `incoming-requests.ts` (`retryStatus`, `refusedSegmentStatus`), `handled-messages.ts` (`refuses`) and `reassembly.ts` (`Refusal`).
|
||||
- **Locality 5.** Honest middle: liveness state in `LinkLife` is read through seven predicates from three classes. `generation()` is captured in closures (`incoming-requests.ts:106`, `:253`), `answered` is mutated through a callback, and ordering constraints are documented only in comments (`client.ts:239`, `session.ts:349`).
|
||||
- **Shape 6.** Between 5 and 7: classes are small and fan-out is bounded, but some names mislead. The GSM codec is called `ascii`, and six-plus near-synonymous teardown verbs (`stop`/`end`/`drop`/`finish`/`linkLost`/`dropLink`/`clear`) mark distinctions I had to work out myself.
|
||||
- **Self-sufficiency 5.** Honest middle: the code comments give terse, accurate reasons, but the domain model a newcomer needs to read them lives only in README.md and the SMPP spec, so I kept the README open the whole time.
|
||||
- **Overall 5.** Capped at 6 by locality; I land at 5 because both locality and self-sufficiency cost me rereads on the stateful session core. The codec and message layers alone would sit near 7.
|
||||
|
||||
SCORES nav=7 loc=5 shape=6 self=5 overall=5
|
||||
|
||||
## Draft D, mid seat
|
||||
|
||||
1. **Hardest places, ranked hardest first**
|
||||
|
||||
1. **`src/session.ts:265-362`: `Session.drain` / `finish` / `linkLost` / `dropLink` / `comeBackUp`, read together with `src/link-life.ts:31-135` (`LinkLife`).**
|
||||
- One question, "can a request go out right now?", depends on four pieces of state: `LinkLife.phase` (binding/down/ended/up), `LinkLife.stopped`, `ReconnectLoop.halted`, and `OutgoingRequests.canCarry()`. The last one is `link.isUp() && !sock.destroyed`.
|
||||
- `drain()` calls `link.stop()`, then branches on `canCarry()`. `finish()` calls `stop()` again, then `dropLink()`, then `end()`.
|
||||
- `comeBackUp` uses `!this.link.retrying()` to mean "close() landed during the rebind". Here `retrying()` is being used as a stand-in for "not stopped", which the name hides.
|
||||
- I had to trace every caller by hand to be sure `close` fires exactly once and `disconnected` is never followed by `close` on the same drop. The per-method doc comments helped. Nothing ties the whole state machine together in one place; this stayed the most expensive read.
|
||||
2. **`src/outgoing-requests.ts:262-351`: `OutgoingRequests.request` / `refusal` / `attempt`.**
|
||||
- A `for(;;)` loop holds one link-wait budget (`link.hold()` returns a closure) plus a window slot, and retries only when `retryOnNextLink && awaitsNextLink()`.
|
||||
- `refusal` line 317 (`pastDrain !== true && isStopped() && canCarry()`) is a three-way condition. Its comment explains why the *other* branch exists, not this one.
|
||||
- `attempt` registers `pending.wait` before `transport.write`, and checks abort twice (in `refusal` and again in `attempt`). The comment at line 325 resolved the second check.
|
||||
- The `pastDrain` flag reaches here from `IncomingRequests` through `Session.incomingFor`. It is action at a distance.
|
||||
3. **`src/pdu.ts:84-136`: `resolveShortMessage` / `resolveBody` (plus `readParams` at 249).**
|
||||
- The `CodingSource` return value decides whether an encoded `message_payload` may overwrite `data_coding`. Without the domain, I had to derive why an empty `short_message` hands `data_coding` to the TLV. The `CodingSource` doc comment half-resolves it.
|
||||
- `readParams` passes `paramNumber(params.sm_length, 0)` as the length to *every* field's `read`. It only works because `sm_length` precedes `short_message` in wire order. That is an order dependence stated only as the general "parameter order is wire order" warning, not at the call.
|
||||
4. **`src/handled-messages.ts:359-423` and `src/expiring-groups.ts:442-554`: `HandledMessages.offer` / `run` / `refuses`, and `ExpiringGroups`.**
|
||||
- `ExpiringGroups`'s contract is inverted: it "enforces neither max nor timeout itself", only `weigh()` evicts, `onSweep` must call `takeExpired()`, and `takeOldest` goes through `delete` (which stops the timer) while `takeExpired` goes through `remove`. Each owner (Reassembler, DlrMerger, HandledMessages) re-implements the policy.
|
||||
- In `HandledMessages`, the `answered` flag is set by a closure threaded into `createSms`. `run` uses an identity check (`running.get(key) !== handled`) to detect that `clear`/`sweep` got there first. `refuses()` has hysteresis state (`atBound`) and calls `sweep()` as a side effect.
|
||||
- The class doc comment resolved the intent. The mechanics took rereads.
|
||||
5. **`src/incoming-requests.ts:105-260` together with `src/server.ts:542-567`: `IncomingRequests.handle` / `route` / `onMessage` / `offer`, and `handleRequest`.**
|
||||
- Searching for where a bind is accepted, I found `IncomingRequests.unhandled` answering binds with `ESME_RALYBND`. The real bind handling is in server.ts, injected as `onRequest`, so the "application hook" slot is also the server's own bind handler.
|
||||
- The charter's Decisions index says this ("composes the application's onRequest after its own bind handling"), but the reader gets there only after a wrong turn.
|
||||
- Link generation is checked twice by different mechanisms: inline in `handle`, and as a `lostLink` closure in `offer`.
|
||||
- `carriedAs`/`standsInFor` rewrites `data_sm` depending on `linkEnd`, a mutable public field set after construction (`session.linkEnd = 'smsc'` in server.ts:586).
|
||||
6. **`src/reassembly.ts:425-444`: `Reassembler.trim`.**
|
||||
- `weigh()` returns evicted groups, possibly including the current one. `answered = parts.size - 1` excludes the refused segment from the loss count.
|
||||
- `collect` only weighs incomplete groups; the completing segment is never weighed. I had to confirm that is intended.
|
||||
- The comments at 361 and 437 resolved it, after two reads.
|
||||
7. **`src/dlr.ts:342-424`: `messageType` / `receiptStatus` / `dlrFromPdu`.**
|
||||
- Four message types, with `'unmarked'` meaning "maybe a receipt if the body parses to both an id and a state". Status comes from TLV, then body, then UNKNOWN, and `statusId` and `statusMsg` can disagree by design.
|
||||
- This is domain-heavy but well commented with spec sections. README's "Delivery receipts" section closed the gap.
|
||||
8. **`src/defs/encodings.ts:302-341`: `messageClassOf` / `messageClassEncoding` / `encodingByDataCoding`.**
|
||||
- Bit-twiddling over GSM 03.38 coding groups that I had no context for. The comments give the bit positions, so it resolves with care.
|
||||
- The GSM codec object is named `ascii` (line 196), which is a lying name for GSM 03.38.
|
||||
|
||||
2. **The unit I would least want to modify: the Session link lifecycle (`Session.drain` / `finish` / `linkLost` / `comeBackUp`) together with `LinkLife`.**
|
||||
- Correctness depends on call order across three classes. `stop()` must reach the loop before the first await; client.ts:239 says so from *another file*.
|
||||
- `close` and `disconnected` must stay exclusive, and `end()` must release waiters exactly once.
|
||||
- Any change risks a hung `close()` or a double `close` event, and nothing local tells you which invariant you just broke.
|
||||
|
||||
3. **Expected hard, found easy.**
|
||||
- The wire codec in `defs/types.ts`: repetitive, every read is range-checked, and `Result` is uniform.
|
||||
- `PduFramer`: short, with the quadratic concern stated.
|
||||
- The `send-sms.ts` check pipeline: linear and flat, each refusal named.
|
||||
- `DlrMerger` severity ranking: one comment explains why wire values can't be compared.
|
||||
- `bind-direction.ts`: `standsInFor`/`bindCarries` are tiny and explained.
|
||||
- The UDH walk in `udh.ts`.
|
||||
- The no-throw discipline made every call site read the same way.
|
||||
|
||||
4. **Prose debt.**
|
||||
- **Needed, and where I found it:**
|
||||
- What ESME and SMSC are. Only inferable from `LinkEnd`'s comment and README.
|
||||
- What "answered on arrival" means. README "Server in depth", roughly a 400-line scroll.
|
||||
- Why `data_sm` flips direction. The comment in bind-direction.ts is sufficient.
|
||||
- How the server's bind handling composes with `onRequest`. Only in the AGENTS.md Decisions index, as one line whose reasoning is in docs/decisions.md, which I was barred from.
|
||||
- The 134/153 segment budget. Covered both inline (message.ts:364) and in AGENTS, so it was cheap.
|
||||
- Many AGENTS decision lines are pointers into a file I couldn't open. For the lifecycle ("A deliberate shutdown drains; an unusable link and an abort do not"), the one-liner was the only statement of the rule the code implements.
|
||||
- The AGENTS architecture map was accurate and was the cheapest, most useful prose.
|
||||
- **Told me nothing the code didn't already say:**
|
||||
- `Session.send`'s "Sends a request and resolves with the peer's response."
|
||||
- README's "Everything exported" table, which duplicates `index.ts`.
|
||||
- The `defaults.ts` preamble.
|
||||
- AGENTS "Conventions", about 30 lines on test fixtures and teardown, which are irrelevant to reading `src/`.
|
||||
- The repeated "Injected so expiry can be exercised without a wall clock" on four options types.
|
||||
- **Minor drift:** README types `onSms` as `(sms) => Promise<void> | void`; the code declares `(sms) => unknown`.
|
||||
|
||||
5. **Scores.** Intrinsic difficulty is high: a stateful protocol session with reconnect, drain, windowing and reassembly. It gets no bonus.
|
||||
- **Navigation 8.** Above 7 "the layout answers where does this live": `src/` is flat, file names match contents, and the AGENTS map is accurate. It stops short of 9 because server bind handling lives behind the `onRequest` slot, which cost one wrong turn.
|
||||
- **Locality 6.** Between 5 and 7: most units stand alone. Link liveness is split across `LinkLife.phase`, `stopped`, `ReconnectLoop.halted`, `canCarry()`'s socket check and generation counters. Changing shutdown means holding session.ts, link-life.ts, outgoing-requests.ts and client.ts at once, and `linkEnd` is mutated after construction.
|
||||
- **Shape 7.** At "predictable": classes are small and fan-out is bounded per level. A few names lie: `ascii` for the GSM codec, `HandledMessages` for messages still being handled, `retrying()` used as "not closed", and `settle` meaning different things in five classes.
|
||||
- **Self-sufficiency 7.** At 7: comments carry the why with spec section references at the hard points (receipts, UDH, data_coding bits). What is missing is the lifecycle invariant and the bind composition, which exist only as index lines pointing at a decisions file.
|
||||
- **Overall 6.** Capped at loc+1 = 7. I place it at 6 because the hardest part, the session lifecycle, is hard both because the problem is hard and because its state is spread across files. It is neither localized nor marked as one place.
|
||||
|
||||
SCORES nav=8 loc=6 shape=7 self=7 overall=6
|
||||
|
||||
## Draft D, senior seat
|
||||
|
||||
1. **Hardest places, hardest first**
|
||||
|
||||
1. **The link lifecycle across four owners.** `src/session.ts:265-362` (`drain`, `finish`, `linkLost`, `dropLink`, `comeBackUp`), `src/link-life.ts:31` (`LinkLife`), `src/reconnect-loop.ts` (`stop`/`halted`) and `src/outgoing-requests.ts:120` (`refusal`, which uses `canCarry()` = `link.isUp() && !sock.destroyed`).
|
||||
- There are three "stopped" notions: `LinkLife.stopped`, `ReconnectLoop.halted`, and `phase === 'ended'`, which also sets `stopped`. `drain()` and `finish()` each call `link.stop()` and `reconnectLoop?.stop()`, so I had to hold the order of the calls to see which one decides the `close` event versus `disconnected`.
|
||||
- `link-life.ts:38` starts `phase` at `'up'`, although `'binding'` exists. The first link therefore reports `isUp()` before any bind, and the charter's "a bind is what makes it one" holds only for reconnected links. I worked this out myself; nothing in the code says so. The code mostly explains itself, but that one point stayed opaque.
|
||||
2. **Who answers a failed handler.** `src/handled-messages.ts:92-140` (`offer`, `run`), together with `src/sms.ts:83-157` (`createSms`, `sendResp`, `answeredOnArrival`) and `src/incoming-requests.ts:252` (`offer`).
|
||||
- An `answered` flag is set through a callback that `offer` builds around a `handled` const, which that same closure refers to. `SmsRoute = Omit<SmsHandlers,'answered'>` and a mutable `Answer` object add further state, and the `lostLink` generation closure is built in yet another class.
|
||||
- When a multipart handler fails, `run()` calls `sendResp({status: retryStatus})`. That returns an `err` from `answeredOnArrival`, and the `err` is discarded. That is how "its answer stands" comes out right, and nothing says so. README's "A handler that fails" bullet resolved it.
|
||||
3. **Reassembly eviction.** `src/reassembly.ts:110` (`collect`) and `:187` (`trim`), with `src/expiring-groups.ts:70` (`weigh`).
|
||||
- `weigh()` can evict the group that is being weighed, so `trim` counts it as `parts.size - 1` answered. The `full` / `unplaceable` refusals map to three statuses in `refusedSegmentStatus`.
|
||||
- `ExpiringGroups` enforces its limits unevenly: only `weigh` evicts, while `max` and `timeout` fall to the owners, and I had to find that in its class docstring. The inline comments resolved it, but it took a reread.
|
||||
4. **Building and reading the message body.** `src/pdu.ts:84-136` (`resolveShortMessage`, `resolveBody`) and `:249` (`readParams`).
|
||||
- On the write side, `CodingSource` decides which of `short_message` and `message_payload` may overwrite `data_coding`. An empty buffer counts as `'message_payload'`. I had to hold four branches at once.
|
||||
- On the read side, `readParams` passes `sm_length` as the `length` argument to every wire type's `read`. The same parameter means the TLV length in `defs/types.ts`. Only the `commands.ts` comment on wire order hints at this coupling.
|
||||
5. **data_coding bit logic.** `src/defs/encodings.ts:159-190` (`messageClassEncoding`, `encodingByDataCoding`).
|
||||
- It is dense bit-twiddling with no context, and a `//` comment sits above a separate `/** */` block, so I could not tell which of the two it belonged to.
|
||||
- Some names are wrong. `'LATIN1'` is returned for 8-bit binary. The GSM codec is named `ascii` (`:45`).
|
||||
- The docblocks resolved most of it. The class-group comment stayed only half clear.
|
||||
6. **`DlrMerger`'s two stores.** `src/dlr-merger.ts:68`, with `spend` at `:166` and `open` at `:150`. Two `ExpiringGroups` (`groups` and `spent`) are both mutated by `spend()`. `open()` checks both, and `dropOldest` spends. The class docstring explained the "merged at most once" rule. I still had to trace the steps by hand.
|
||||
7. **Retrying the first bind.** `src/client.ts:218-320` (`bindOn`, `initialAttempts`, `keepTrying`). This is a second `ReconnectLoop` outside `Session`, with a fresh session per attempt and a `lastErr` closure. Correctness depends on the order in which abort listeners are added and removed, and on the comment "close() must reach the loop's stop() before its first await". The comments resolved it.
|
||||
|
||||
2. **The unit I would least want to modify:** `LinkLife` (`src/link-life.ts`). `Session`, `OutgoingRequests` (`isUp`, `awaitsNextLink`, `refusal`, `hold`) and `IncomingRequests` (`generation`) all read its phase. Its `stopped` flag duplicates the reconnect loop's, and its initial `'up'` is an unstated exception to its own `binding` rule. A change there reaches the drain, the queued sends and response correlation, and no single file shows all of that.
|
||||
|
||||
3. **Expected to be hard, found easy:**
|
||||
- The codec: `defs/types.ts` is long but uniform, and every read is range-checked the same way.
|
||||
- `PduFramer`, `ReconnectLoop` and `PendingRequests`.
|
||||
- The typing of `TlvInputs` and `Tlvs`.
|
||||
- `splitMessage` and the budget per segment.
|
||||
- Navigation overall: the charter's one-line-per-file map matched the tree exactly.
|
||||
|
||||
4. **Prose debt**
|
||||
- **Needed, and what it cost to find:**
|
||||
- README "Receiving in depth" and "Shutdown", to learn the half-bound hysteresis, the five-minute handler cutoff, and what happens when a handler fails after answering. Finding them was cheap, but they sit in a user document, not beside `HandledMessages`.
|
||||
- The rationale for the link-life decisions. AGENTS.md only indexes it ("One owner decides whether a link can carry a request…") and I was not allowed to open `docs/decisions.md`, so the initial-`'up'` question stayed open.
|
||||
- I opened no tests.
|
||||
- **Told me nothing the code did not already say:**
|
||||
- `session.ts:203` "Sends a request and resolves with the peer's response".
|
||||
- The getter docstrings on `boundAs` and `peerInterfaceVersion`.
|
||||
- `UnansweredError`'s docstring, which restates its message.
|
||||
- `retryStatus`'s docstring.
|
||||
- The idle-timeout rationale, written twice (`defaults.ts:13` and `client.ts:193`).
|
||||
- `checkSessionOptions`'s docstring describes one case, `maxOutstanding: 0`, not the function, which misleads slightly.
|
||||
- AGENTS.md's 0.4.0 defect table and its long test-fixture paragraph cost reading time and did not help with `src/`.
|
||||
- **Small duplication noticed:** `collectSent` in `send-sms.ts` and `collectReceipt` in `sms.ts`, and `quoted()` in both `session-options.ts` and `bind-direction.ts`.
|
||||
|
||||
5. **Scores**
|
||||
|
||||
| Dimension | Score | Anchor and cause |
|
||||
| --- | --- | --- |
|
||||
| Navigation | 8 | Between 7 and 9. The Architecture map and truthful file names took me from symptom to file first try every time. The lifecycle behaviour spread over `Session`, `LinkLife` and `ReconnectLoop` is what keeps it from 9. |
|
||||
| Locality | 6 | Between 5 and 7. Most collaborators are standalone, with injected `now` and dependencies. But whether a link can carry a request, is stopped, or has ended is split across `LinkLife`, `ReconnectLoop`, `OutgoingRequests.canCarry` and order-dependent calls in `Session`, and the handled-message `answered` state runs through closures in three files. |
|
||||
| Shape | 7 | Predictable. Fan-out is bounded per level and nearly every name tells the truth. The exceptions are `ascii` for GSM, `LATIN1` standing for binary, and `isUp()` being true before the first bind. |
|
||||
| Self-sufficiency | 7 | Predictable. Inline comments carry most of the "why" (SMPP section references, peer quirks). The handler bound and failure semantics needed README, and the reasoning behind the link-life decisions sits in a document I could not open. |
|
||||
| Overall | 7 | Predictable, within the cap of lowest dimension plus one. The hard corners are few and I know which to fear, but the lifecycle corner is spread across four files instead of sitting in one marked place. |
|
||||
|
||||
The problem is intrinsically hard: SMPP session semantics, reconnecting with no resends, a draining shutdown, and reassembly under memory bounds. The scores give no bonus for that.
|
||||
|
||||
SCORES nav=8 loc=6 shape=7 self=7 overall=7
|
||||
|
||||
## Draft D, architect seat
|
||||
|
||||
**Comprehension panel report: Architect, inherited (draft-d)**
|
||||
|
||||
**Order note:** I read AGENTS.md right after README and the tree, before I had written the map down. Its architecture listing matched the map below and changed nothing in it. I opened no test files.
|
||||
|
||||
### 1. Map (README + tree only, verbatim)
|
||||
|
||||
Top-level areas I expected in `src/`:
|
||||
- **A. Entry points:** `index.ts` for the public surface, `client.ts` for connect and bind with reconnect, `server.ts` for the listener, auth and close.
|
||||
- **B. Session core:** `session.ts` as the hub. `session-options.ts` and `defaults.ts` for options. `bind-direction.ts` for which commands a bind type carries.
|
||||
- **C. Link lifecycle:** `link-life.ts` (up, down or ended?), `link-timers.ts` (enquire_link and idle), `reconnect-loop.ts` (backoff), `pdu-transport.ts` (socket to PDUs).
|
||||
- **D. Outbound:** `send-sms.ts` (split and submit), `outgoing-requests.ts` (the request path), `pending-requests.ts` (seqNr correlation), `send-window.ts` (maxOutstanding), `unanswered-error.ts`.
|
||||
- **E. Inbound:** `incoming-requests.ts` (dispatch), `sms.ts` (the onSms handle), `handled-messages.ts` (probably the "held while the handler runs" bound), `reassembly.ts`, `concat.ts`, `udh.ts`, `message-body.ts`.
|
||||
- **F. Receipts:** `dlr.ts` (parse), `dlr-merger.ts` (messageDlr), `sms-id.ts` (hex/decimal and `<base>-<n>`).
|
||||
- **G. Codec:** `pdu.ts`, `pdu-framer.ts`, `pdu-refusal.ts` (PduRefusedError), `retained-pdu.ts` (?), `defs/*` (spec tables).
|
||||
- **H. Text:** `message.ts` (encode, split, smppTime) and `defs/encodings.ts`.
|
||||
- **I. Utilities:** `result.ts`, `error-from.ts`, `log.ts`, `uuid.ts`, `idle-waiters.ts` (?), `expiring-groups.ts` (?).
|
||||
|
||||
Names that did not give their purpose:
|
||||
- `idle-waiters`: idle peer or idle count?
|
||||
- `retained-pdu`
|
||||
- `expiring-groups`: groups of what?
|
||||
- `handled-messages`: reads as "already handled".
|
||||
- `error-from`
|
||||
- `defaults` vs `session-options`
|
||||
|
||||
README features I could not place, or that were missing:
|
||||
- Goal 9's store is absent, as the README says.
|
||||
- smppTime and smppDate: presumably `message.ts`.
|
||||
- At the repo root, `MIGRATION-NOTES.md` and `DESIGN.md` beside `MIGRATION.md` and `docs/decisions.md`: I cannot tell their purpose apart from the others.
|
||||
|
||||
**Where the map was wrong, and what each correction cost:**
|
||||
- **`handled-messages.ts` (medium cost, and it is the crux of the 3am question).** I guessed "held until `sendResp()`". It actually holds a message until the **handler's promise settles**. `answered` is recorded only to decide the retry refusal.
|
||||
- **`link-life.ts` (medium).** It is not only a state flag. It is also the queue where requests wait for the next link, with two orthogonal state variables, `phase` and `stopped`.
|
||||
- **`expiring-groups.ts` (medium).** It is a shared TTL store whose contract is "enforces neither max nor timeout itself" (`expiring-groups.ts:18`). Each of its three owners re-implements the policy differently. `HandledMessages` uses it for running handlers, which are not groups. `DlrMerger` uses a second instance as a "spent" set.
|
||||
- **Cheap corrections:**
|
||||
- `session-options.ts` also holds `SessionEvents` and all option validation.
|
||||
- `bind-direction.ts` also holds bind-record validation (`checkedBind`) and `undeclaredInterfaceVersion`.
|
||||
- `reassembly.ts` holds `decodeSegments`, which `sms.ts` uses.
|
||||
- `idle-waiters.ts` is "wait until a count reaches 0".
|
||||
- `retained-pdu.ts` copies PDUs off the wire and weighs them.
|
||||
|
||||
### 2. Fan-out by level
|
||||
- **L0, repo root:** 22 entries, 8 of them prose documents. Two documents I could not tell apart by name.
|
||||
- **L1, `src/`: 36 files plus `defs/`, all flat. This is the worst level.** Only names and the AGENTS listing group them into the 9 areas above. Nine is bounded; 37 is not, and the directory gives no help.
|
||||
- **L2, `defs/`:** 7 files, bounded and clear.
|
||||
- **L3, units:**
|
||||
- `Session` wires 9 collaborators and has about 15 methods.
|
||||
- `IncomingRequests` owns 3 things and reaches back into `Session`.
|
||||
- `OutgoingRequests` owns 3.
|
||||
- `LinkLife` has 12 methods over 2 state variables. By method count it is the densest unit.
|
||||
|
||||
### 3. Names
|
||||
**One name over several concepts:**
|
||||
- **idle** means four things:
|
||||
- `IdleWaiters`: a count falls to 0.
|
||||
- The `LinkTimers` idle timeout: a silent peer.
|
||||
- `ExpiringGroups.idle()`: stop the sweeper.
|
||||
- `SendWindow.idle()`: the drain.
|
||||
- **refusal** means four things:
|
||||
- `pdu-refusal`: an unreadable PDU.
|
||||
- `LinkLife.refusal()`: the session is over.
|
||||
- `OutgoingRequests.refusal()`: a request cannot go out.
|
||||
- The reassembly `Refusal`: `'full' | 'unplaceable'`.
|
||||
- **settle** covers waiters, pending requests, `IdleWaiters.settle` and `HandledMessages.settle()`, which means "wake the drain if empty".
|
||||
- **Shutdown verbs:** `stop`, `end`, `finish`, `drop`, `dropLink`, `linkLost`, `halted`, `isOver`, `isStopped`. `link.stop()` refuses new work while `reconnectLoop.stop()` halts timers: same verb, different meanings.
|
||||
|
||||
**One concept with several names:**
|
||||
- "Answered": `Answer.done` (`sms.ts:81`), `Handled.answered` (`handled-messages.ts`), `answeredAs` and `answeredOnArrival`.
|
||||
- Writing a response: `answer()`, `sendReturn()`, `sendResp()`.
|
||||
- The reassembly octet cap: option `maxOctets` vs `defaults.maxReassemblyOctets`. The option name does not say "reassembly", yet a sibling cap exists (`maxHandledOctets`).
|
||||
- The server's idle timeout: the literal `defaults.idleTimeout` 40 000 vs the client's derived `2 × enquireLinkInterval`.
|
||||
- Two date formatters, `smppDate` and `smppTime.encode`, in `message.ts`, with duplicated pad chains.
|
||||
|
||||
**Misleading:**
|
||||
- `HandledMessages` means "being handled". The README calls them "messages being handled".
|
||||
- `ExpiringGroups` holds running handlers, which are not groups.
|
||||
- `bind-direction.ts` holds more than direction.
|
||||
|
||||
**Copies:**
|
||||
- `quoted()` appears twice (`session-options.ts:78`, `bind-direction.ts:54`).
|
||||
- `collectSent` (`send-sms.ts:274`) and `collectReceipt` (`sms.ts:183`) are near-twins.
|
||||
- An inline `thrown instanceof Error ? … : new Error(String(thrown))` appears three times (`client.ts:89`, `reconnect-loop.ts:80`, `reconnect-loop.ts:136`) instead of `errorFrom()`.
|
||||
|
||||
### 4. What I would restructure, ranked
|
||||
1. **Group `src/` into about 6 folders:** link, outbound, inbound, receipts, codec, text. The AGENTS listing already draws those lines, so this only moves the map from a document into the layout.
|
||||
2. **Give the shutdown/link vocabulary one owner.**
|
||||
- Collapse `LinkLife.phase` and `stopped` into one state enum.
|
||||
- Rename so that "stop" means one thing everywhere.
|
||||
- Move `OutgoingRequests.refusal`'s `pastDrain && isStopped && canCarry` condition (`outgoing-requests.ts:129`) behind one `LinkLife` predicate.
|
||||
3. **Make `ExpiringGroups` enforce its own policy,** or split it into a TTL store and a set. Today three owners re-implement "full", weight and sweep, and its sweeper interval equals its timeout. So expiry is lazy by up to 2× (`expiring-groups.ts:60`): the README's "five minutes" handler cap is really 5–10 minutes when no traffic arrives. That is a plausible claim drift; I derived it from the code and have not verified it.
|
||||
4. **Merge the "answered" state into one place,** so `sms.ts` and `HandledMessages` stop tracking the same fact.
|
||||
5. **Use `errorFrom()` everywhere.**
|
||||
- `reconnect-loop.ts:80` runs `String(thrown)` inside the `.catch` that is meant to contain an application throw. `errorFrom`'s own comment says `String()` can throw.
|
||||
- If it does, `void this.run()` rejects unhandled and `attempting` stays `true`, which wedges the loop. The trigger is a null-prototype object thrown from an application-supplied `ReconnectOptions.connect` or `onConnected`, reachable because `Session` is publicly constructible.
|
||||
- I call this plausible, not verified.
|
||||
|
||||
**What the structure gets right:**
|
||||
- Files are small (all under 420 lines).
|
||||
- Every file name maps to one noun that also appears in `Session`'s fields.
|
||||
- `Session` reads as a table of contents.
|
||||
- Imports point one way.
|
||||
- Hard-rule-1 result types are uniform.
|
||||
- Comments carry the WHY at the line: spec section numbers, peer quirks, past defects.
|
||||
- `defs/` is clean.
|
||||
- `PduFramer`, `PendingRequests`, `SendWindow` and `ReconnectLoop` each fit in the head alone.
|
||||
|
||||
### 5. The 3am question
|
||||
**Symptom:** during a graceful shutdown the session hangs until `shutdownTimeout`, although the application called `sendResp()` on every message.
|
||||
|
||||
**Path, cold, about 2–3 minutes:** `Session.close` → `drain` (`session.ts:265`) → `this.incoming.drain(...)` (`session.ts:274`) → `IncomingRequests.drain` (`incoming-requests.ts:163`) → `HandledMessages.idle` (`handled-messages.ts:111`).
|
||||
|
||||
**The unit is `HandledMessages.run` (`handled-messages.ts:125`).** The entry is deleted at `:138` only after `await this.handle()` returns. `sendResp()` only flips `handled.answered`, and the drain never reads it.
|
||||
|
||||
**So the peer's handler has not returned.** The likely cause is that it is awaiting `sms.sendDlr()`. That call waits for every `deliver_sm_resp`, up to `responseTimeout`, which defaults to 30 s, longer than the 5 s shutdown. It can also wait without bound behind a full send window, because `sendDlr` passes no signal.
|
||||
|
||||
This is designed behaviour: the `OnSms` type doc, README lines 108 and 389, and the AGENTS decision all say it. The fix is on the caller's side (return the handler, or fire-and-forget the receipt). The code states this at the type (`session-options.ts:39`), so no document is needed.
|
||||
|
||||
**Where it rots first:**
|
||||
- The link/shutdown triangle: `Session.drain`/`finish`/`linkLost`/`dropLink`/`comeBackUp` plus `LinkLife` plus `OutgoingRequests.refusal`. Three units read `LinkLife` state. Correctness depends on call order: `link.stop()` before `canCarry()`, `drop()` before `end()`. `IncomingRequests` also snapshots `link.generation()`.
|
||||
- Next, `ExpiringGroups` and its three divergent owners.
|
||||
|
||||
**Where the next two features land:**
|
||||
- **Goal 9's store** would have to thread an interface through `Session` → `IncomingRequests` → `Reassembler`/`HandledMessages`/`DlrMerger`, which is every `ExpiringGroups` owner. That is the costliest seam in the code base.
|
||||
- **A per-PDU rate-limit hook (goal 7)** lands cleanly in `OutgoingRequests.request` beside `SendWindow.acquire`.
|
||||
|
||||
### 6. Hardest places, ranked
|
||||
1. `session.ts:265-362`: `drain`, `finish`, `linkLost`, `dropLink`, `comeBackUp`. Order-dependent shutdown across 4 collaborators.
|
||||
2. `link-life.ts:31` `LinkLife` as a whole: `phase` × `stopped`, and 7 predicates with overlapping meanings.
|
||||
3. `outgoing-requests.ts:74-134`, `request` and `refusal`: the link-wait/window retry loop and the `pastDrain` exemption.
|
||||
4. `handled-messages.ts:67-138`, `refuses`/`offer`/`run`: hysteresis, a hidden sweeper timer, and the answered flag written from `sms.ts`.
|
||||
5. `expiring-groups.ts` `weigh` together with `reassembly.ts:187` `trim`: eviction can take the current key.
|
||||
6. `pdu.ts:84-136`, `resolveShortMessage` and `resolveBody`: the `CodingSource` rules.
|
||||
7. `incoming-requests.ts:218-260`, `onMessage` and `offer`: the bound check, reassembly, answer on arrival, and generation capture.
|
||||
8. `dlr.ts:216` `dlrFromPdu`: the `messageType`/`receiptStatus` precedence.
|
||||
|
||||
**The unit I would least want to modify is `LinkLife`.** Everything that decides whether a request may go out reads it, and its meaning is spread over 7 predicates.
|
||||
|
||||
**Intrinsic difficulty:** high. It is an SMPP session layer with reconnect, a send window, reassembly, receipt merging and drain semantics, and it earns no bonus for that.
|
||||
|
||||
### 7. Scores
|
||||
- **Navigation 7:** "Predictable". The file names plus `Session`'s field list got me from symptom to unit in 3 hops. The flat 37-file `src/` and the misleading name `HandledMessages` keep it below 9.
|
||||
- **Locality 6:** between "honest middle" and "predictable". `LinkLife` state is read by `Session`, `OutgoingRequests` and `IncomingRequests`, and shutdown correctness depends on call order. `ExpiringGroups` pushes its own policy onto three owners.
|
||||
- **Shape 6:** between "honest middle" and "predictable". Units are small and mostly named truthfully. But L1 has 37 ungrouped entries, and "idle", "refusal", "settle" and "stop" are each overloaded.
|
||||
- **Self-sufficiency 7:** "Predictable". Comments at each unit state its invariants and the spec section behind them, and the 3am answer is readable at the `OnSms` type with no document open. A reader still needs the AGENTS listing to see the area grouping that the layout does not show.
|
||||
- **Overall 6:** capped by locality and shape at 6. A cold senior is productive within a week on everything except the link/shutdown triangle.
|
||||
|
||||
SCORES nav=7 loc=6 shape=6 self=7 overall=6
|
||||
@@ -0,0 +1,710 @@
|
||||
# Round 3: drafts E and F
|
||||
|
||||
## Draft E, junior seat
|
||||
|
||||
1. **Hardest places, hardest first**
|
||||
|
||||
1. **`src/session-life.ts:175` `SessionLife.attempt()`, with `enter()` at `:115` and `linkLost()` at `:109`.** The async reconnect continuation re-enters the state machine after two awaits. It snapshots `this.links` and later compares it, and it reads state through `is()` only to stop TypeScript narrowing. The step to `bound` is not in this file: it goes `rebind` → `client.ts` `bind()` → `Session.bound()` → `life.bound()`, three files away. There is also hidden re-entrancy. `effects.linkDown()` calls `sock.destroy()`, which fires the transport's `onClose` → `life.linkLost()`. That call is harmless only because `attached()` is already false. The ASCII diagram at `:8-24` and the comment "a transition from any other state is ignored" resolved most of it. Why `links` could change during `rebind` stayed opaque.
|
||||
2. **`src/outgoing-requests.ts:97` `request()`, `sendOnce()` at `:116` and `attempt()` at `:162`.** There is a `for(;;)` retry around three nested waits: link, window slot, response. Each has its own deadline or timeout semantics, and `written` plus `state() !== 'down'` decide whether to loop. The `misuse()` check runs here and again in `Session.send()` (`session.ts:180`), and the reason for the duplicate is a riddle comment ("named as one ahead of the drain"). The JSDoc on `request()` and the `UnansweredError` naming resolved the intent. I had to read `README` "Sends and the link" to trust it.
|
||||
3. **`src/handled-messages.ts:61` `refuses()` and `:120` `run()`.** `refuses()` looks like a predicate but sweeps, logs, and flips `atBound` hysteresis. `run()` answers the peer after the handler, and the answer depends on whether `sms.answered` was flipped by a closure inside `sms.ts`. It then removes the entry only if `running.get(key) === sms`, because a sweep may already have dropped it while the handler keeps running. `ExpiringGroups` gets `max` here but, by its own doc, does not enforce it, so I had to go and read `expiring-groups.ts` to know who does.
|
||||
4. **`src/reassembly.ts:186` `trim()`, with `ExpiringGroups.weigh()` at `src/expiring-groups.ts:70`.** `weigh()` evicts the oldest groups and may return the current key itself. Then `answered = parts.size - 1` subtracts the just-arrived segment, because that one gets a `full` refusal and the peer keeps it. Holding "set never evicts, weigh does, owners check full" across two files cost two rereads. The inline comments resolved it.
|
||||
5. **`src/sms.ts:82` `createSms()` and `:126` `sendResp()`.** A mutable `answer` record is captured in a closure and exposed through getters. `link` is the socket at arrival, compared by `destroyed` rather than against `session.sock`. `answeredAs` means "multipart, already answered on arrival". I only understood why `sendResp()` on a multipart message is a no-op after reading README "Server in depth" (answered on arrival). The code alone did not tell me.
|
||||
6. **`src/pdu.ts:84` `resolveShortMessage()` and `:113` `resolveBody()`.** The `CodingSource` idea is hard for someone without SMPP: which of `short_message` and `message_payload` gets to set `data_coding`, and when an empty buffer counts as "payload". Also, `readOptionalParams()` at `:267` retries parsing with one skipped NULL. The comments are accurate but assume the domain. README "Building" and the SMPP terms table were needed.
|
||||
7. **`src/defs/encodings.ts:152-191` `messageClassOf()`, `messageClassEncoding()`, `encodingByDataCoding()`.** Bit masks (`0x80`, `0x10`, `0xF0`, `>> 2 & 0x03`) against GSM 03.38 coding groups I have never seen. The comments cite spec sections I cannot open. This stayed opaque. I trust it only because the tests presumably pin it; I did not open them.
|
||||
8. **`src/client.ts:240` `retryUntilBound()` and `:278` `keepTrying()`.** This is a second backoff loop, separate from `SessionLife`'s. Its `settle` callback is called on every failure and does not settle anything; it only records `lastErr`. The name misled me until I read the callback body at `:290`. AGENTS' architecture line ("the first-connect retry of reconnect.fromStart") told me why it exists.
|
||||
|
||||
2. **The unit I would least want to modify:** `SessionLife.enter()` / `attempt()` (`src/session-life.ts:115-203`). Every lifecycle effect fans out from it through `LifeEffects` closures defined in `session.ts:260`. Those closures call back into the transport, whose socket events call `life.linkLost()` again. Correctness rests on "ignored from any other state" and on the ordering of effects before emit. I could not predict what a new transition would re-trigger without running it.
|
||||
|
||||
3. **Expected hard, found easy:**
|
||||
- `PduFramer`: small, one job, and the quadratic-avoidance comment explains the only trick.
|
||||
- The wire-type table in `defs/commands.ts`, where the wire-order warning sits right at the table.
|
||||
- The result pattern and "nothing throws": consistent everywhere, so no surprises.
|
||||
- `defaults.ts`: one place, grouped.
|
||||
- `sms-id.ts`, `concat.ts`, `message-body.ts`, `log.ts`, `pdu-refusal.ts`: each read cold in one pass.
|
||||
- `send-sms.ts`: long but linear, a chain of `check*` functions.
|
||||
- The AGENTS architecture map, which got me to the right file first time for every question I had.
|
||||
|
||||
4. **Prose debt**
|
||||
- **Documentation I needed:**
|
||||
- README "SMPP terms" table: essential and cheap to find, linked from the table of contents.
|
||||
- README "Server in depth" and "Receiving in depth", for answered-on-arrival and the handled-message bound. The rules behind `sms.ts` and `handled-messages.ts` live there, not in the code.
|
||||
- AGENTS architecture list: essential for navigation.
|
||||
- The AGENTS decisions index lines are cryptic without `docs/decisions.md`, which I was not allowed to open (for example "A report is final unless its `esm_class` or its state says otherwise").
|
||||
- Spec knowledge (`esm_class` bits, `data_coding` groups) is cited by section number and never explained. That cost the most and was never repaid.
|
||||
- **Prose that told me nothing the code did not already say:**
|
||||
- The duplicated deliver JSDoc in `outgoing-requests.ts:84` and `pending-requests.ts:57`.
|
||||
- `/** A socket is on the link. */` on `attached()`.
|
||||
- The AGENTS "Defects found in 0.4.0" table, which is history and not a guide to the current code.
|
||||
- The AGENTS test conventions, which are irrelevant to reading `src/`.
|
||||
- A different cost: many comments are compressed to riddles and needed several reads each. Examples are "Announced when the wait is over rather than when it starts: a cancelled one never happened." (`session-life.ts:160`) and "A misuse is named as one ahead of the drain, rather than blamed on the shutdown."
|
||||
|
||||
5. **Scores**
|
||||
- **Navigation 7:** at the "Predictable" anchor. The AGENTS file map and honest file names (`pdu-framer`, `link-timers`, `sms-id`) took me symptom → file first try. It stops short of 9 because reconnect lives in two places (`session-life.ts` and the separate `client.ts` retry loop).
|
||||
- **Locality 5:** at the "Honest middle" anchor. Collaborators are wired by closures back into `Session` (`LifeEffects`, `PduTransport` callbacks, `IncomingRequests` calling `session.emit`, `.sock` and `.close`). The `Sms` answer record is mutated from two modules. Safe changes in `SessionLife` and `HandledMessages` need the whole session held in your head.
|
||||
- **Shape 6:** between 5 and 7. Fan-out per level is bounded and the files are small. Names overload or lie, though:
|
||||
- `settle` means four different things (`IdleWaiters`, `PendingRequests`, `SendWindow`, the `client.ts` callback that settles nothing).
|
||||
- `handlers` in `IncomingRequests` means `{answer, send}`, not `onSms`.
|
||||
- `refuses()` and `closing()` read as predicates but mutate.
|
||||
- `SessionLife.carries()` is dead: unused in `src/`, duplicated by the switch in `OutgoingRequests.waitForLink()`.
|
||||
- **Self-sufficiency 5:** at the "Honest middle" anchor. The generic parts (framer, codec types, results, timers) stand alone. The session semantics (answered-on-arrival, the handled-message bound) and all the `data_coding`/`esm_class` bit logic needed README sections or the spec open beside the code.
|
||||
- **Overall 5:** as someone new to the domain, I would take an area in a day or two, with rereads and some wrong turns. The problem's intrinsic difficulty is genuinely high (a protocol state machine, reassembly, and receipt correlation over a flaky link), and it gets no bonus here.
|
||||
|
||||
SCORES nav=7 loc=5 shape=6 self=5 overall=5
|
||||
|
||||
## Draft E, mid seat
|
||||
|
||||
1. **Hardest places, hardest first**
|
||||
|
||||
1. **The answer to an inbound message**, `incoming-requests.ts:218` (`IncomingRequests.onMessage`), `handled-messages.ts:120` (`HandledMessages.run`) and `sms.ts:126` (`sendResp`), with `sms.ts:82` (`createSms`) holding the `Answer` record.
|
||||
- Whether the peer has been answered, and under which id, is decided in three places:
|
||||
- `onMessage` answers multipart segments one by one on arrival and passes `answeredAs`.
|
||||
- `createSms` turns `answeredAs` into a pre-set `status: 'ESME_ROK'`.
|
||||
- `run` answers after the handler only when `!sms.answered`, choosing the retry status if the handler failed.
|
||||
- "Which link" is carried as a `Socket` identity that is compared through `.destroyed` in two places (`IncomingRequests.handle` and `sendResp`).
|
||||
- To reason about "handler throws after a multipart message" I had to keep all three files in my head. The `Sms` type docs and the README section "Server in depth" resolved it. It is followable but not local.
|
||||
2. **`ExpiringGroups` and the classes built on it**, `expiring-groups.ts:19`, with `reassembly.ts:187` (`Reassembler.trim`) and `dlr-merger.ts:105/150/166` (`collect`, `open`, `spend`).
|
||||
- The store refuses to enforce its own `max` and `timeout` ("owners check full and call takeExpired(); only weigh() evicts"). So each of the three owners re-implements the capping itself (`open` → `dropOldest`, `sweep` before `collect`).
|
||||
- `weigh()` can evict the very key being weighed. `trim` then does `answered = parts.size - 1` for that case, and I needed two reads to see why.
|
||||
- `DlrMerger` runs a second `ExpiringGroups<true>` (`spent`) as a tombstone set, with `spend()` writing to both stores.
|
||||
- The doc comments stop you misusing the store, but understanding any one owner means holding the store's partial contract.
|
||||
3. **`resolveShortMessage` / `resolveBody`**, `pdu.ts:84` and `pdu.ts:113`.
|
||||
- `CodingSource` decides whether `short_message` or `message_payload` may set `data_coding`. That depends on Buffer vs string vs empty, and on whether the command's table has a `short_message` at all.
|
||||
- I had to hold four or five branches at once, and `data_coding` gets rewritten in two different spots.
|
||||
- The type comment on `CodingSource` helped. The rule behind it ("a string body is written in the alphabet its own data_coding names") is only a title in the AGENTS index.
|
||||
4. **The retry loop in `OutgoingRequests.request` / `sendOnce`**, `outgoing-requests.ts:97` and `:116`.
|
||||
- `sendOnce` returns `undefined` to mean "loop again". Whether to loop depends on `attempt.written` and on a live read of `state() !== 'down'` after two awaits.
|
||||
- With `LinkWaiters`, `SendWindow` and `PendingRequests` underneath, a send passes through four waits with three different abort and timeout rules.
|
||||
- The doc comments on `request` and `Attempt.written` resolved it, as did the README bullets under "Sends and the link".
|
||||
5. **`SessionLife.enter` / `attempt`**, `session-life.ts:115` and `:175`.
|
||||
- The ASCII state diagram is very good. What cost me was `attempt()`: it re-checks `is('down')` after `connect`, then `this.links !== link || !is('connected')` after `rebind`.
|
||||
- `rebind` calls `Session.bound()`, which calls `life.bound()` → `enter('bound')`. That is a re-entrant path back into the machine from inside an await.
|
||||
- The comment "the one continuation that re-enters the machine" marks it, and the diagram resolved it.
|
||||
6. **`Session.unbind` / `drain`**, `session.ts:218` and `:317`.
|
||||
- `life.closing()` is a query-named method that performs the transition.
|
||||
- `closedOnUnbind = wasOpen && !this.life.attached()` infers that the peer dropped the link in answer to our unbind.
|
||||
- The return value orders two errors with different priorities.
|
||||
- The doc comment helped. The mutating `closing()` still surprised me.
|
||||
7. **The two reconnect loops**, `client.ts:240` (`retryUntilBound`) and `client.ts:278` (`keepTrying`).
|
||||
- AGENTS says "the reconnect loop is the `down` state", but `fromStart` is a second, hand-rolled backoff loop in `client.ts`. The charter's file list does mention it.
|
||||
- The callback named `settle` is called on every failed attempt and does not settle: it only records `lastErr`. That name misled me until I read the body of `keepTrying`.
|
||||
8. **The overloaded third argument of `WireType.read`**, `pdu.ts:249` (`readParams` passes `sm_length` to every param reader) and `defs/types.ts:280` (`tlvInt`).
|
||||
- For a mandatory parameter it is `sm_length`; for a TLV it is the TLV header length.
|
||||
- Only `buffer` and the tlv variants use it, and nothing names the dual meaning. I found it by grepping callers. It stayed half-opaque until then.
|
||||
|
||||
2. **The unit I would least want to modify:** `Reassembler.collect` + `trim` together with `ExpiringGroups.weigh`.
|
||||
- Weight accounting is spread across `set` (zeroes the weight), `weigh` (evicts, possibly the caller's own key) and `trim` (recomputes the group total from scratch).
|
||||
- Every refusal path decides a peer-visible status, and a lost group is reported to the application as traffic gone. An off-by-one there is silent data loss.
|
||||
|
||||
3. **Expected hard, found easy:**
|
||||
- The codec tables (`defs/commands.ts`, `defs/tlvs.ts`) and the TLV typing, including `tlvSpecs` keying each definition to its own name.
|
||||
- `PduFramer` and `PduTransport`.
|
||||
- The DLR parsing in `dlr.ts`: every regex and every fallback has a one-line reason.
|
||||
- `SessionLife` itself, thanks to the diagram.
|
||||
- GSM packing (153 vs 134). The `segmentUnits` comment, plus the AGENTS section "GSM 7-bit is sent unpacked", made it obvious even to someone who knows nothing about SMPP.
|
||||
|
||||
4. **Prose debt**
|
||||
- **What I needed and what it cost:**
|
||||
- The README "SMPP terms" table (ESME/SMSC, `esm_class`, UDH, `sar_*`). Cheap to find, essential without domain knowledge.
|
||||
- The README sections "Sends and the link", "Receiving in depth" and "Server in depth", to confirm the intent behind items 1 and 4. Each took a scroll-and-search.
|
||||
- The AGENTS architecture map, for navigation.
|
||||
- Several `// SMPP 3.4 x.y.z` comments point at a spec I have never read, and I had to take them on trust. Examples: `respIdParams`, `refusalAnswer`, `messageClassOf`.
|
||||
- The AGENTS decision index gives titles only. Twice (the drain ordering, and `data_coding` ownership in the codec) the title told me a rule existed without telling me the rule, and the reasoning lives in `docs/decisions.md`, which I was told not to open.
|
||||
- I opened no tests.
|
||||
- **Prose that told me nothing the code did not:**
|
||||
- For reading `src/`: the AGENTS test-conventions bullets (fixtures, `resume()`, `t.after` ordering) and the "Defects found in 0.4.0" table, which is history about another codebase.
|
||||
- The README Goals and Audience.
|
||||
- A few restating doc comments: `PendingRequests.deliver` ("False means nobody was"), `LinkTimers.clear`'s neighbours, `defs/index.ts`'s grouping.
|
||||
- The duplicate `emit` / `captureRejectionSymbol` guard comments in `session.ts` and `server.ts`.
|
||||
|
||||
5. **Scores.** Intrinsic difficulty is moderate to high (wire protocol, reconnect, backpressure, multipart), and it gets no bonus below.
|
||||
- **Navigation 8.** Between 7 and 9: the AGENTS file map plus one concept per file (`dlr-merger.ts`, `link-waiters.ts`, `pdu-refusal.ts`) got me from a symptom to the right file cold every time. The one detour was the second reconnect loop living in `client.ts`.
|
||||
- **Locality 6.** Between 5 and 7: `SessionLife` does centralise the state, but the answered-or-not state spans `IncomingRequests`, `HandledMessages` and `Sms`. `ExpiringGroups` also pushes enforcement of its own invariants onto three owners, and link identity is a shared `Socket` reference compared across modules.
|
||||
- **Shape 7.** Predictable: fan-out is bounded (`Session` wires about six collaborators through narrow option objects) and most names tell the truth. It is held there by a few that lie or hide effects: `closing()` mutates, the `settle` callback in `keepTrying` does not settle, and `IncomingRequests.clear()` also empties the handled messages.
|
||||
- **Self-sufficiency 7.** Predictable: nearly every non-obvious line carries a one-line why, often with a spec reference, and the hard corners are marked. It stays below 9 because several rules (codec `data_coding` ownership, drain ordering) exist in the code as outcomes whose reasons are only indexed titles, and the domain vocabulary needs the README glossary open.
|
||||
- **Overall 7.** Capped at 7 by Locality 6. A cold mid-level reader knows within a day which corners to fear (items 1, 2 and 3 above). They are few and marked, but item 1 is harder than the problem requires.
|
||||
|
||||
SCORES nav=8 loc=6 shape=7 self=7 overall=7
|
||||
|
||||
## Draft E, senior seat
|
||||
|
||||
**Comprehension panel report: senior maintainability seat, `@larvit/smpp` (draft-e), whole project**
|
||||
|
||||
I read `README.md`, `AGENTS.md` and every file under `src/`, including `defs/`. I opened no tests.
|
||||
|
||||
## 1. Hardest places, hardest first
|
||||
|
||||
1. **`src/reassembly.ts:187` `Reassembler.trim()`, and `src/expiring-groups.ts:70` `ExpiringGroups.weigh()`**
|
||||
- `weigh()` can evict the group that is being weighed. `trim()` then has to work out whether that group survived. It also counts only `parts.size - 1` segments as lost when that group is the victim, because the new segment "stays with the peer".
|
||||
- To get this right I had to hold four things at once: `weigh`'s eviction order, `takeOldest → delete → idle`, the "answered" arithmetic, and the caller in `collect()`, which answers `full`.
|
||||
- The inline comments made it clear in the end, after two passes.
|
||||
|
||||
2. **`src/expiring-groups.ts:19` `ExpiringGroups`, as a contract**
|
||||
- The class docstring says it "enforces neither max nor timeout itself", so every owner has to call `full` and `takeExpired()` in the right order.
|
||||
- `HandledMessages` stretches this across two classes. `IncomingRequests.onMessage` (`incoming-requests.ts:218`) calls `handled.refuses()` before reassembly, and `offer()` comes later, when the message is whole.
|
||||
- Nothing states that the cap holds only because `refuses()` ran first. I had to reconstruct that myself, and it stayed implicit.
|
||||
|
||||
3. **`src/session-life.ts:115` `SessionLife.enter()` / `:175` `attempt()`, with `src/session.ts:260` `lifeFor()`**
|
||||
- The ASCII transition table in the header is the best piece of prose in the repo.
|
||||
- Two things cost me:
|
||||
- The header says "a transition from any other state is ignored", but `enter()` has no guard. The guards live in the public wrappers (`bound()`, `closing()`, `linkLost()`, `end()`), so I had to check each one.
|
||||
- The effects are closures defined in `Session`. To follow `down → connected → bound` I had to jump between `session-life.ts`, `session.ts` `lifeFor`/`linkDown`, `OutgoingRequests.linkUp`/`linkLost` and `PduTransport.attach`.
|
||||
- `attempt()`'s re-check after the await (`this.links !== link || !this.is('connected')`) is commented and fine.
|
||||
|
||||
4. **`src/client.ts:240` `retryUntilBound()` / `:278` `keepTrying()`**
|
||||
- This is a second backoff loop, separate from `SessionLife`'s. The charter and README say `fromStart` goes "through that same loop", but it doesn't: it's a separate loop that uses the same `Backoff` class.
|
||||
- The callback type is named `Settle`, but on an error it doesn't settle anything; it only records `lastErr`. You learn that from a comment inside the lambda in `keepTrying`.
|
||||
- Add the `waiting()` closure and an abort listener, and this is four nested pieces of control flow for one feature. It resolved in the end, but the name misled me along the way.
|
||||
|
||||
5. **`src/pdu.ts:84` `resolveShortMessage()` / `:113` `resolveBody()`**
|
||||
- The `CodingSource` rule decides which of `short_message` and `message_payload` may rewrite `data_coding`. It depends on whether the command's table has a `short_message` at all, on Buffer vs string, and on zero length.
|
||||
- The comment on the `CodingSource` type states the rule, but I had to trace three return shapes to confirm it.
|
||||
- Next to it, `readParams()` (`:240`) passes `sm_length` as the length argument to every param read. That works only because `sm_length` comes earlier in wire order. `commands.ts` documents wire order, but not that this depends on it.
|
||||
|
||||
6. **`src/sms.ts:126` `sendResp()`, with `src/session.ts:304` `answer()`**
|
||||
- `sendResp` checks the socket the message arrived on, `link.destroyed`. `answer()` then writes to `transport.sock`, the current socket.
|
||||
- This is correct only because a socket is replaced only after it has been destroyed (`PduTransport.attach`).
|
||||
- `IncomingRequests.handle()` (`:104`) relies on the same equivalence. It is not stated anywhere I read.
|
||||
|
||||
7. **`src/handled-messages.ts:120` `HandledMessages.run()`**
|
||||
- Covers expiry, a handler failure, answering after the handler settles, and the identity check `running.get(key) !== sms`, which catches a `clear()` or sweep that ran in the meantime.
|
||||
- The class docstring covers it. It is compact but dense.
|
||||
|
||||
8. **`src/defs/encodings.ts:163` `messageClassEncoding()` / `encodingByDataCoding()`**
|
||||
- Bit-twiddling over GSM 03.38 coding groups that I had no background for.
|
||||
- The comments are adequate. The problem is inherently hard; it is not badly written. Stacking a `//` comment on top of a `/** */` comment made it unclear which one belonged to which function.
|
||||
|
||||
## 2. The unit I would least want to modify
|
||||
|
||||
`Reassembler.trim()` together with `ExpiringGroups.weigh()`.
|
||||
|
||||
- The eviction, the "is the current group gone" question, the lost-segment count and the ESME-facing status (`full` → throttled) are all spread over two files, and each file assumes the other's behaviour.
|
||||
- A wrong change here silently loses traffic the peer will never resend, which is the README's worst outcome.
|
||||
- Nothing in the code would stop me. Only a test would catch the mistake.
|
||||
|
||||
## 3. Expected hard, found easy
|
||||
|
||||
- **Framing (`pdu-framer.ts`):** short, and the quadratic-join rationale is right there.
|
||||
- **The send path:** `OutgoingRequests.request/sendOnce/attempt`. The `written` flag makes "resend or not" a single boolean.
|
||||
- **`PendingRequests` and `SendWindow`**
|
||||
- **The TLV table's self-keyed generic**
|
||||
- **Receipt parsing (`dlr.ts`)**
|
||||
- **`splitMessage`:** the budget comment together with the charter's "GSM 7-bit is sent unpacked" section settled the 153/134 question at once.
|
||||
|
||||
## 4. Prose debt
|
||||
|
||||
**Needed:**
|
||||
- The `session-life.ts` state table: essential, and cheap to find.
|
||||
- `AGENTS.md`'s file map: accurate, and the fastest route from a symptom to a file.
|
||||
- The charter's "GSM 7-bit is sent unpacked" section.
|
||||
- README "Server in depth", to understand why segments are answered on arrival.
|
||||
|
||||
**Missing:**
|
||||
- The invariants in items 2 and 6. They are written down nowhere I was allowed to read.
|
||||
- The decisions index points at `docs/decisions.md`, which I couldn't open. Twice (the `fromStart` "same loop" wording, and the abort-dance duplication) the one-line index entry made a claim the code did not obviously bear out, and there was nothing local to check it against.
|
||||
|
||||
**Prose that told me nothing new:**
|
||||
- The `/** Injected so expiry can be exercised without a wall clock. */` comment, repeated on four option types.
|
||||
- `// Called unbound, so the application's hook never sees this class as its this` (`incoming-requests.ts`).
|
||||
- Most of the charter's test-convention paragraph (fixtures, `recordingDeps`), which is irrelevant for reading `src/`.
|
||||
- Much of the defect table, as far as reading `src/` goes.
|
||||
|
||||
## 5. Scores
|
||||
|
||||
- **Navigation 7:** Predictable. The `AGENTS.md` file map is accurate line by line, and file names match their content (`link-waiters`, `pdu-refusal`, `sms-id`), so I landed first try on every symptom I tried. It doesn't reach 9 because `defaults` are applied in three different layers (`Session`, `IncomingRequests`, `Reassembler`), so finding "where does this default apply" takes a search.
|
||||
- **Locality 6:** Between honest middle and predictable. It is held down by three unwritten cross-class invariants: owners enforce `ExpiringGroups`' limits, `refuses()` must run before `offer()`, and a destroyed arrival socket stands in for the current socket. `SessionLife`'s effects are also closures back into `Session`.
|
||||
- **Shape 7:** Predictable. Every file is small, fan-out per level is bounded, and names mostly tell the truth. It is kept from 8 by a few names that mislead:
|
||||
- `closing()` is a transition, not a predicate.
|
||||
- `Settle` doesn't settle on an error.
|
||||
- `bound()` exists on two layers with different contracts.
|
||||
- `onRequest` names both the server's wrapper and the application's hook.
|
||||
- **Self-sufficiency 7:** Predictable. The why-comments sit on the lines that need them, and spec section numbers are cited. It is held back by a charter index that points to a decisions file whose claims I could not check locally.
|
||||
- **Overall 6:** The capped maximum is 7 (lowest dimension plus one). I gave 6 because the costs are concentrated in exactly the bounded-store and link-identity code where a mistake loses traffic.
|
||||
|
||||
The problem itself is moderately hard: flow control across two peers, the SMPP body and encoding rules, and reconnect races. That earns no bonus.
|
||||
|
||||
SCORES nav=7 loc=6 shape=7 self=7 overall=6
|
||||
|
||||
## Draft E, architect seat
|
||||
|
||||
# Comprehension panel, seat "Architect, inherited": @larvit/smpp (draft-e)
|
||||
|
||||
I read README.md, AGENTS.md and every non-test file in src/. I opened no test, ran nothing, edited nothing and ignored the pre-loaded AGENTS.md.
|
||||
|
||||
## 1. Map from README.md and `ls -R src test` only (verbatim)
|
||||
|
||||
> Flat `src/` of 36 files plus `defs/` (7). I expect seven areas that the layout does not show:
|
||||
> A. **Spec tables**, `defs/`: commands, constants, encodings, errors, TLVs, wire types, plus an index grouping them.
|
||||
> B. **Codec**: `pdu.ts` (pduToObj/objToPdu), `pdu-framer.ts` (stream to PDUs), `pdu-refusal.ts` (PduRefusedError), `retained-pdu.ts` (?), `result.ts`, `error-from.ts` (?).
|
||||
> C. **Message content**: `message.ts` (encode/split/bitCount, and probably smppTime), `message-body.ts`, `concat.ts`, `udh.ts`, `reassembly.ts`, `expiring-groups.ts` (a TTL map, probably under reassembly).
|
||||
> D. **Receipts**: `dlr.ts` (parse *and* build receipts), `dlr-merger.ts` (messageDlr), `sms-id.ts` (hex/decimal ids).
|
||||
> E. **Session core**: `session.ts`, `session-life.ts` (?), `session-options.ts` (types), `defaults.ts`, `link-timers.ts` (enquire_link/idle), `backoff.ts` (reconnect loop?), `pdu-transport.ts`, `link-waiters.ts` (?), `idle-waiters.ts` (?).
|
||||
> F. **Request flow**: `outgoing-requests.ts`, `pending-requests.ts` (seq/correlation), `send-window.ts`, `unanswered-error.ts`, `send-sms.ts`, `incoming-requests.ts`, `sms.ts` (the onSms handle), `handled-messages.ts` (messages whose onSms runs, for the bound and the drain).
|
||||
> G. **Entry points**: `client.ts`, `server.ts`, `index.ts`, `log.ts`, `uuid.ts`.
|
||||
> Names that do not give their purpose: session-life, link-waiters vs idle-waiters, retained-pdu, error-from, defaults vs session-options, and backoff (a loop or only a delay?).
|
||||
> Expected from the README but missing: the goal-9 store (the README says it has not shipped). At the repo root, MIGRATION-NOTES.md and DESIGN.md sit beside the six documents the charter names, with no stated purpose.
|
||||
|
||||
### Corrections, cheapest first
|
||||
|
||||
| Map guess | Reality | Cost |
|
||||
|---|---|---|
|
||||
| backoff.ts is the reconnect loop | Delay arithmetic only. The loop is the `down` state of `SessionLife`. | Low. The AGENTS table says so. |
|
||||
| link-waiters / idle-waiters | Requests waiting for a bound link / a count falling to zero | Low |
|
||||
| dlr.ts parses and builds receipts | Building lives in `sms.ts` (`receiptText`, `sendDlr`, `collectReceipt`) | Medium. I went to dlr.ts first, then grepped for `stat:`. |
|
||||
| session-options.ts is option types | Also holds `SessionEvents`, bind-direction logic (`bindCarries`, `standsInFor`, `bindCommands`) and all option validation (`checkSessionOptions`) | Medium. The name hides three concerns. |
|
||||
| Whole-message decoding lives in message.ts | `decodeSegments` is in `reassembly.ts:63` and is called from `sms.ts` | Low-medium |
|
||||
| One reconnect loop | Two. `SessionLife.attempt/schedule`, plus a second in `client.ts:240` `retryUntilBound`/`keepTrying` for `fromStart`. Both log `'reconnect - retrying'`. | Medium. The same log line from two places is a 3am trap. |
|
||||
| retained-pdu, error-from | Heap-detach and weight of a held PDU; turning a thrown value into an Error | Low. The AGENTS table covers both. |
|
||||
|
||||
## 2. Fan-out by level
|
||||
|
||||
- **Repo root:** about 8 prose documents (README, AGENTS, CHANGELOG, MIGRATION, MIGRATION-NOTES, DESIGN, todo, CLAUDE) plus 5 directories. MIGRATION-NOTES and DESIGN are not in the charter's "each file answers one question" list.
|
||||
- **`src/`, the worst level:** 37 entries. They fall into 7 real areas, but only the AGENTS.md table shows that. Without the table this level is past any bound you can hold at once.
|
||||
- **`src/defs/`:** 7. Good.
|
||||
- **Within the session:**
|
||||
- `Session` holds 8 collaborators.
|
||||
- `OutgoingRequests` holds 3 (PendingRequests, LinkWaiters, SendWindow).
|
||||
- `IncomingRequests` holds 2 (HandledMessages, Reassembler) plus 6 injected fields.
|
||||
- `HandledMessages` holds 2 (ExpiringGroups, IdleWaiters).
|
||||
- Depth is at most 4 and fan-out at most 8 per level. Bounded.
|
||||
- **Files:** most are under 250 lines. `defs/types.ts` (684 lines) is long but uniform: one wire type after another.
|
||||
|
||||
## 3. Names
|
||||
|
||||
**Misleading:**
|
||||
- `SessionLife.closing()` (`session-life.ts:97`) reads like a predicate but performs the transition and returns whether it did. `carries()` and `attached()` next to it really are predicates.
|
||||
- `session-options.ts` holds events, bind direction and validation as well as options.
|
||||
- The comment on `SessionOptions.shutdownTimeout` (`session-options.ts:117`) says "How long a drain waits for the requests already on the wire". It also bounds the wait on running handlers (`session.ts:324`). That comment is false.
|
||||
- In `client.ts:234,288`, `Settle` is called once per failed attempt as well as on success, so it does not settle anything.
|
||||
- `maxOctets` (a public option) bounds reassembly only. The handled-message octet cap is a separate constant, `maxHandledOctets`.
|
||||
|
||||
**One name over several concepts:**
|
||||
- **`idle`** has four meanings:
|
||||
- A count reaching zero: `IdleWaiters`, `SendWindow.idle`, `HandledMessages.idle`.
|
||||
- Stopping the sweep timer: `ExpiringGroups.idle()`.
|
||||
- The idle-timeout timer: `LinkTimers.idle`.
|
||||
- The `idleTimeout` option.
|
||||
- **`settle`** has four meanings: wake all waiters (`IdleWaiters.settle`), wake only if the count is zero (`HandledMessages.settle`), resolve one request (`PendingRequests.settle`), and the local `settle` closures in LinkWaiters, SendWindow and client.
|
||||
- **`link`** means the socket (`SmsInput.link`, `const link = this.session.sock`), the connection's lifetime (`LinkState`, `LinkTimers`, `LinkWaiters`) and which end of the connection this is (`LinkEnd`).
|
||||
|
||||
**Two names for one concept:**
|
||||
- The lost link is spelled `linkLost` (the SessionLife transition and `OutgoingRequests.linkLost`) and `linkDown` (the LifeEffects hook and `Session.linkDown`).
|
||||
- The end of the session is spelled `end()`, the state `ended`, and the effect `over`.
|
||||
- A message whose handler is running is spelled "handled" (the class and its logs), `running` (its field) and "being handled" (README).
|
||||
|
||||
## 4. What I would restructure, ranked
|
||||
|
||||
1. **Group `src/` into about 6 directories:** `codec/` (pdu, framer, refusal, retained-pdu, defs), `message/` (message, message-body, concat, udh, reassembly), `receipts/` (dlr, dlr-merger, sms-id, and receipt building moved out of sms.ts), `session/` (session, session-life, link-timers, pdu-transport, backoff), `requests/` (outgoing/pending/send-window/link-waiters/idle-waiters, incoming/handled-messages/sms), and top level (client, server, index). Today the AGENTS table does the job the directory tree should do.
|
||||
2. **One retry loop.** Have `fromStart` reuse the `SessionLife` loop, or give client.ts's loop a distinct name and log line.
|
||||
3. **Split `session-options.ts`** into `bind-direction.ts` and `option-checks.ts`, and move `SessionEvents` into `session.ts`.
|
||||
4. **Retire the overloaded verbs** (`idle`, `settle`, `linkLost`/`linkDown`) and rename `closing()` to something like `beginDrain()`.
|
||||
5. **Deduplicate the collectors.** `collectReceipt` (`sms.ts:182`) is `collectSent` (`send-sms.ts:274`) without ids.
|
||||
|
||||
**What the structure gets right:**
|
||||
- `SessionLife` is one state value with the transition diagram in its own header (`session-life.ts:7-24`). Effects reach the rest of the session only through `LifeEffects`.
|
||||
- `OutgoingRequests` reads state through a function and never copies it.
|
||||
- Every collaborator takes a narrow options object.
|
||||
- Result types are used throughout, so control flow has no second error channel.
|
||||
- The comments are dense and mostly say why, not what.
|
||||
- The AGENTS table matches the code file for file.
|
||||
|
||||
## 5. The 3am question
|
||||
|
||||
**Symptom:** during a graceful shutdown the session hangs until the shutdown timeout, even though the application already called `sms.sendResp()` on every message.
|
||||
|
||||
**Cold path, about 3 to 5 minutes:**
|
||||
1. `Session.close` (`session.ts:235`) calls `drain` (`session.ts:317`).
|
||||
2. That calls `this.incoming.drain(timeout)` (`incoming-requests.ts:163`).
|
||||
3. That calls `HandledMessages.idle` (`handled-messages.ts:106`).
|
||||
4. The unit is **`HandledMessages.run`** (`handled-messages.ts:120`). A message leaves `running`, which is what releases the drain, only after `onSms`'s promise settles. `sendResp()` records the answer and releases nothing.
|
||||
|
||||
**Likely cause:** the handler is still awaiting something, typically `sms.sendDlr()`. That goes through `handlers.send`, which is `outgoing.request`: it bypasses the closing-state refusal and waits up to `responseTimeout` (30 s), which is longer than `shutdownTimeout` (5 s).
|
||||
|
||||
**What gets you there:** the README (lines 107-109) and the `OnSms` doc (`session-options.ts:87-92`) both say the handler's promise is the hold. **What slows you down:** the `Sms.sendResp` doc (`sms.ts:47-51`) says nothing about the hold, and the `shutdownTimeout` comment wrongly claims it only bounds requests.
|
||||
|
||||
**Where it rots first:** the wiring in `Session`'s constructor and `transportFor`/`lifeFor`/`incomingFor` (`session.ts:105-294`).
|
||||
- Eight collaborators are cross-wired through closures that capture `this.life` and `this.transport` before those are assigned. That is safe today only because of construction order.
|
||||
- `IncomingRequests` reaches back into `Session` for `sock`, `close()`, `emit`, `bindAllows`, `boundAs` and `linkEnd`, so the dependency runs in both directions.
|
||||
- Every lifecycle feature touches three files: session-life (transition), session.ts (effect) and the collaborator.
|
||||
|
||||
**Where the next two features land:**
|
||||
1. **The goal-9 store** lands under `ExpiringGroups`, which has three owners: `DlrMerger`, `Reassembler` and `HandledMessages`. Each wraps it differently (weigh-evicts, a "spent" set, sweep callbacks), so a persistent seam would have to be cut three times or `ExpiringGroups` would have to become the store interface.
|
||||
2. **A per-PDU rate-limit hook** lands in `OutgoingRequests.sendOnce` (`outgoing-requests.ts:116`), between the window acquire and `attempt`, and must respect the rule that a written request is never retried. The code states that rule, so the change is contained. An alphabet hook would be far worse: `EncodingName` is a closed union that ripples through defs/encodings, message.ts and send-sms.ts.
|
||||
|
||||
## 6. Hardest places, ranked
|
||||
|
||||
1. `session.ts:105-294`, `Session` constructor plus `transportFor`/`lifeFor`/`incomingFor`: closure wiring, and initialisation order matters.
|
||||
2. `session-life.ts:175`, `SessionLife.attempt`: re-enters after two awaits and guards with the `links` counter plus `is('connected')`. It is correct, but you have to hold the whole diagram to read it.
|
||||
3. `session.ts:218`, `Session.unbind`: the `wasOpen`/`closedOnUnbind` arithmetic and the order of error precedence.
|
||||
4. `outgoing-requests.ts:97-183`, `request`/`sendOnce`/`attempt`: a `for(;;)` retry keyed on `written` and a fresh `state()` read.
|
||||
5. `handled-messages.ts:62,120`, `refuses()` (a query that mutates the hysteresis flag and sweeps) and `run()` (answer after settle, then conditional delete).
|
||||
6. `pdu.ts:84-136`, `resolveShortMessage`/`resolveBody`: which field's encoding sets `data_coding`.
|
||||
7. `reassembly.ts:187` `trim` with `expiring-groups.ts:70` `weigh`: eviction can take the current key, with off-by-one accounting of lost parts.
|
||||
8. `client.ts:240-309`, `retryUntilBound`/`keepTrying`: the second loop and its misnamed settle callback.
|
||||
|
||||
**The unit I would least want to modify** is `SessionLife.enter` (`session-life.ts:115`) together with its effect bindings in `Session.lifeFor` (`session.ts:260`). One transition's meaning is split across two files and five callbacks.
|
||||
|
||||
**Intrinsic difficulty is high**, and none of the scores below credit it: an async protocol with reconnect, drain, correlation, reassembly and a byte-exact codec against peers that do not follow the spec.
|
||||
|
||||
## 7. Scores
|
||||
|
||||
- **Navigation 7:** at the "Predictable" anchor. The AGENTS file table plus accurate file names got me from the 3am symptom to `HandledMessages.run` in minutes. It sits no higher because the 37-file flat `src/` depends on that table, and two parallel retry loops share one log line.
|
||||
- **Locality 6:** between "Honest middle" and "Predictable". `LifeEffects` and the `state()` accessor are real seams. Holding it down: `IncomingRequests` reaches back into `Session`, the constructor wiring depends on order, and one lifecycle change spans session-life, session and a collaborator.
|
||||
- **Shape 6:** between 5 and 7. Nesting below the top level is bounded (8 at most per level). Holding it down: the 37-entry top level, a misnamed `session-options.ts`, a mutating `closing()`, and `idle`/`settle`/`link` each covering several concepts.
|
||||
- **Self-sufficiency 7:** at "Predictable". The state diagram, invariant comments and "why" comments sit at the code. Holding it back from higher: the false `shutdownTimeout` comment (`session-options.ts:117`), and `Sms.sendResp` not saying it does not release the drain.
|
||||
- **Overall 6:** capped at 7 by the lowest dimension plus one. The hard parts are marked but spread across the session wiring, and the top level needs a document to map it.
|
||||
|
||||
SCORES nav=7 loc=6 shape=6 self=7 overall=6
|
||||
|
||||
## Draft F, junior seat
|
||||
|
||||
**Junior A: comprehension report for draft-f**
|
||||
|
||||
I read README.md, AGENTS.md and every file under `src/`, all in draft-f. I opened no test.
|
||||
|
||||
## 1. Hardest places, hardest first
|
||||
|
||||
1. **`src/expiring-groups.ts:38-54, 111`: the `ExpiringGroups` getters `full`, `size` and `weight`, and `get()`, which all call `expire()`.**
|
||||
- Reading a property has side effects. It can fire `onDrop`.
|
||||
- In `Reassembler`, `onDrop` becomes `onLost`, then `port.report`, then a `sessionError` emit. In `RunningHandlers` it logs a warn and settles the drain's waiters.
|
||||
- So `this.running.size` in `IncomingRequests.refusedAtBound` (`incoming-requests.ts:224`) can emit events on the application's emitter. I only found this by following `onDrop` through three classes.
|
||||
- No comment at the call sites says so. It stayed a trap even after I understood it.
|
||||
|
||||
2. **`src/reassembly.ts:113` (`Reassembler.collect`) with `weighed()` at `:180`, `dropped()` at `:194` and the `weighing` field at `:91`.**
|
||||
- `weighing` is a side channel. It is set around a `weigh()` call so that the drop callback, which fires synchronously inside it, can subtract the newest segment from the reported loss.
|
||||
- `weighed()` then reads `get(key)` again to find out whether its own group was evicted.
|
||||
- To follow it I had to hold several things at once:
|
||||
- part and total validation;
|
||||
- a group that is new or already there;
|
||||
- eviction by count inside `set`;
|
||||
- eviction by weight inside `weigh`;
|
||||
- the "newest segment stays with the peer" rule.
|
||||
- The field's comment explains why but not how. It was resolved only after a second read.
|
||||
|
||||
3. **`src/incoming-requests.ts:260` (`onMessage`) and `:292` (`handOver`), together with `sms.ts:126` (`sendResp`) and `:196` (`sendDlr`).**
|
||||
- A message's answer is spread over four places:
|
||||
- answered on arrival for segments;
|
||||
- the handler's return;
|
||||
- an early `sendResp()`;
|
||||
- the refusal on a throw, which is itself split by `answeredOnArrival`.
|
||||
- The shared state is the mutable `answer` object captured in the closures of `createSms` (`sms.ts:80`).
|
||||
- `throttledStatus` versus `refusedSegmentStatus` needs the `carriedAs` / `standsInFor` indirection for `data_sm`.
|
||||
- The README's "Receiving in depth" section resolved it. The code alone did not.
|
||||
|
||||
4. **`src/pdu.ts:84` (`resolveShortMessage`) and `:113` (`resolveBody`).**
|
||||
- `CodingSource` decides which of `short_message` and `message_payload` may rewrite `data_coding`.
|
||||
- There are five return paths, depending on Buffer or string, empty or not, and a string `message_payload`.
|
||||
- `data_coding` is patched onto the params from two places.
|
||||
- The type comment at `:74` helps, but I had to trace each branch by hand. It remained partly opaque, for example why an empty encoded string falls to `message_payload`.
|
||||
|
||||
5. **`src/defs/encodings.ts:152-191`: `messageClassOf`, `messageClassEncoding`, `encodingByDataCoding`.**
|
||||
- Bit masks (`& 0x80`, `>> 2 & 0x03`, `0xF0`) that I had no background for.
|
||||
- Two comments are stacked oddly at `:160-162`: a `//` block and then a `/** */`, both describing the function below.
|
||||
- It stayed opaque without the GSM 03.38 spec. The intrinsic difficulty is high.
|
||||
|
||||
6. **`src/defs/tlvs.ts:110` (`WriteValue`) and `:284` (`keyedTlvs`, with the `isTlvs` guard).**
|
||||
- Conditional types nested three deep, plus a runtime re-validation of what the code just built, because casts are banned.
|
||||
- Hard rule 4 in AGENTS.md explains why the guard exists. The type gymnastics remained costly.
|
||||
|
||||
7. **`src/reconnect-loop.ts:82` (`schedule`), `:127` (`attempt`) and `:64` (`adopt`).**
|
||||
- `upAt` resets the backoff only after a link lasted `maxDelay`.
|
||||
- `links > 0` decides `unref`.
|
||||
- A `stopped` check comes after an `await` in `attempt`.
|
||||
- There are three flags (`timer`, `attempting`, `stopped`) guarding re-entry.
|
||||
- The comments explain each rule, so it was resolved, but it is order-sensitive.
|
||||
|
||||
8. **`src/send-window.ts:42` (`release`).** Handing a slot to a waiter without decrementing `inFlight` is correct but not commented. I had to reason out that the slot transfers.
|
||||
|
||||
## 2. The unit I would least want to modify
|
||||
|
||||
`Reassembler.collect` / `weighed` / `dropped`. A change to eviction order inside `ExpiringGroups.weigh`, or to when `get()` expires, silently changes the loss counts reported to the application, which are sessionError events. Nothing at the call site tells me that the coupling exists.
|
||||
|
||||
## 3. Expected hard, found easy
|
||||
|
||||
- **`PduFramer`:** short, one clear purpose.
|
||||
- **`PendingRequests` and `OutgoingRequests`:** the split between `LinkLostError` ("never written, retry") and `UnansweredError` ("may have been taken") is named well. `SmppClient.send`'s retry loop (`client.ts:143`) read at once.
|
||||
- **The layering of Session, IncomingRequests and SessionPort:** the port type (`incoming-requests.ts:50`) says exactly what the collaborator may touch.
|
||||
- **`server.ts` `handleRequest`:** bind-before-anything is compact.
|
||||
- **`defs/types.ts`:** long but repetitive and uniform.
|
||||
|
||||
## 4. Prose debt
|
||||
|
||||
**What I needed, and what it cost to find:**
|
||||
|
||||
- **README "Glossary":** needed for ESME/SMSC, `esm_class`, `data_coding`, UDH and `sar_*`. It sits about 600 lines down the README and nothing in `src/` points at it.
|
||||
- **README "Receiving in depth" and "Server in depth":** needed to understand the answer-on-arrival model before `onMessage` made sense.
|
||||
- **AGENTS "GSM 7-bit is sent unpacked":** needed to believe the 153/134 budget in `message.ts` (the `segmentUnits` constant near the top).
|
||||
- **The `ASCII` encoding name:** it means GSM 03.38. Only the comment in `encodings.ts` near line 45 and a README table say so. The name lies.
|
||||
- **Bit layout of `esm_class` and `data_coding`:** not documented anywhere I was allowed to read. I would need the spec.
|
||||
|
||||
**Prose that told me nothing the code did not:**
|
||||
|
||||
- AGENTS' architecture map repeats most file-level doc comments almost word for word. It was still useful as an index.
|
||||
- Several Conventions paragraphs about tests (`dummy-smsc`, `recordingDeps`) were irrelevant to reading `src/`.
|
||||
- AGENTS' decision index names `ReconnectOptions`, which does not exist in `src/`. The type is `ReconnectTuning`. That line is stale.
|
||||
- Comments that add nothing:
|
||||
- `'Whether the socket has closed.'` on `closed` (`session.ts:148`);
|
||||
- `'Sends a request and resolves with the peer's response.'` (`session.ts:172`);
|
||||
- `'Connects to an SMSC and binds.'` (`client.ts:414`).
|
||||
|
||||
## 5. Scores
|
||||
|
||||
| Dimension | Score | Anchor and cause |
|
||||
|---|---|---|
|
||||
| Navigation | 7 | Predictable. AGENTS' one-line-per-file map and honest file names (`pdu-framer`, `dlr-merger`, `send-window`) took me from a symptom to a file first time. `ASCII` meaning GSM and the three meanings of "refuse" and "answer" (`Session.refuse`, `IncomingRequests.refuse`, `sendReturn` / `answer` / `sendResp`) keep it off 8. |
|
||||
| Locality | 5 | Honest middle. The getters in `ExpiringGroups` fire callbacks that emit on the session. The `weighing` side-channel field depends on synchronous re-entry. The mutable `answer` closure is shared by `sendResp` and `sendDlr`. Changing one piece means holding its callback chain. |
|
||||
| Shape | 6 | Between honest middle and predictable. Fan-out is bounded (Session builds four collaborators; IncomingRequests builds two). Some names mislead: `ASCII`; `stopping`, which the peer's unbind also sets; `over` versus `closed`; `SessionListener`'s emit guard copied three times. |
|
||||
| Self-sufficiency | 6 | Between honest middle and predictable. Inline spec citations ("SMPP 3.4 5.3.2.26", "4.6.2") and why-comments mostly carry it. The answer-on-arrival model and the data_coding bit groups still need the README or the spec open beside the code. |
|
||||
| Overall | 6 | Capped by Locality (5 + 1). |
|
||||
|
||||
**Intrinsic difficulty:** the problem is hard, and gets no bonus in these scores. It involves concurrency, the protocol's split between UDH and `sar_*`, receipts that look like messages, and the GSM alphabets.
|
||||
|
||||
SCORES nav=7 loc=5 shape=6 self=6 overall=6
|
||||
|
||||
## Draft F, mid seat
|
||||
|
||||
1. **Hardest places, ranked hardest first**
|
||||
|
||||
- **`src/reassembly.ts:330` `Reassembler.weighed()` / `dropped()` (:344), together with `src/expiring-groups.ts:38-54` (the `full`, `size` and `weight` getters).** `weighed()` sets `this.weighing` so that `dropped()`, reached re-entrantly through `ExpiringGroups.weigh()` → `dropOldest()` → `onDrop`, knows to subtract the newest segment from the loss count. That is a side channel through a field. On top of it, every getter on `ExpiringGroups` runs `expire()`, which fires `onDrop`. So reading `size` in a log line (`reassembly.ts:358`, `this.groups.weight` inside `lost()`) can drop other groups and report them mid-report. The comments at :240 and :274 got me to what it intends. Whether the re-entrant drops are harmless stayed unresolved.
|
||||
- **`src/session.ts:211-332` `Session.unbind()` / `drain()` / `finish()` / `end()`.** Three flags carry a lifecycle: `over`, `stopping` and the `ended` promise. `closed` is a getter over `over`, and `port().end` (:262) sets `stopping` from outside the drain. `unbind()`'s `droppedOnUnbind` depends on `UnansweredError` and `this.over` agreeing after an await. The field comments (:64, :66) and the class doc resolved which flag means what, but only after I built a table by hand.
|
||||
- **`src/client.ts:515` `SmppClient.send()`.** It loops on `LinkLostError`. Whether it terminates depends on `ReconnectLoop.bound()` (which hands back the current session until the socket's `close` event), `Session.send()` (which checks `over`, set only on `close`) and `PduTransport.write()` (which fails on `sock.destroyed`). A socket that is destroyed but has not yet emitted `close` looks to me like it gives a loop that resolves only through microtasks and may never yield. I could not rule that out without a test. This is the plainest action-at-a-distance in the codebase, and it stayed opaque.
|
||||
- **`src/incoming-requests.ts:260-351` `onMessage()` → `handOver()` → `refuse()`, with `src/sms.ts:796-860` `createSms()` / `sendResp()`.** I had to hold several things at once:
|
||||
- whether the message was answered on arrival;
|
||||
- whether the handler threw;
|
||||
- whether it returned `{ smsId }` or `{ status }`;
|
||||
- the `Answer` object mutated inside the closure;
|
||||
- `carriedAs` choosing the retry status.
|
||||
|
||||
`alreadyAnswered` has four error texts for these combinations. The README section "Receiving in depth" resolved it; the code alone did not.
|
||||
- **`src/defs/encodings.ts:152-191` `messageClassOf()`, `messageClassEncoding()`, `encodingByDataCoding()`.** This is bit arithmetic on a GSM 03.38 layout I have never seen. There is a `//` comment and then a `/** */` comment stacked on one function (:160-162), and the `//` one reads as though it belongs to the function above. The comments give the bit positions. Why 0x03 means one thing below 0x80 and another in a class group stayed half opaque.
|
||||
- **`src/dlr.ts:157-238` `messageType()` / `receiptStatus()` / `dlrFromPdu()`.** `'unmarked'` versus `'receipt'`, whether the TLV or the body wins, and `statusMsg` possibly being `undefined` before it falls back to `'UNKNOWN'`. The README's Delivery receipts section resolved it. Without the README I would have guessed wrong about `'other'`.
|
||||
- **`src/pdu.ts:323-375` `resolveShortMessage()` / `resolveBody()`.** The `CodingSource` naming feels inverted: an empty `short_message` yields `source: 'message_payload'`, meaning "`message_payload` may set `data_coding`". The comment at :313 explains it, but I had to read it twice.
|
||||
- **`src/reconnect-loop.ts:83` `schedule()`.** It resets the backoff only if `upAt` shows the link outlasted `maxDelay`, clears `upAt`, doubles the delay after capturing it, and calls `unref()` only once a link has existed. Each step has a comment, which resolved it. It is dense rather than opaque.
|
||||
|
||||
2. **The unit I would least want to modify: `ExpiringGroups` (`src/expiring-groups.ts`).** Three owners (`Reassembler`, `DlrMerger` with its `spent` store, and `RunningHandlers`) depend on when drops happen and in what order. Reads mutate state and fire callbacks. `set()` can evict. `weigh()` can evict the entry being weighed. Any change moves loss accounting, drain wake-ups and receipt-merge refusal all at once. Note also that `RunningHandlers` logs "giving up on a handler that never returned" for an *eviction*, not only an expiry.
|
||||
|
||||
3. **Expected hard, found easy**
|
||||
- The wire codec: `defs/types.ts`, `pdu.ts` parse/build, TLV read/write. It is long but regular: every reader range-checks, and every error names the parameter.
|
||||
- `PduFramer`.
|
||||
- `PendingRequests` and `SendWindow`.
|
||||
- The server's bind handling (`handleRequest`).
|
||||
- The option validation in `session-options.ts`.
|
||||
|
||||
Result-typed code with no throws made control flow easy to follow.
|
||||
|
||||
4. **Prose debt**
|
||||
- **Needed:**
|
||||
- the README Glossary for ESME, SMSC, `esm_class`, `data_coding`, UDH and `sar_*` (cheap to find, essential for me);
|
||||
- the README "Receiving in depth" and "Server in depth" sections, for the answer rules in `sms.ts` and `incoming-requests.ts` (about 10 minutes to locate);
|
||||
- the AGENTS "GSM 7-bit is sent unpacked" section, for `segmentUnits` 153 versus 134.
|
||||
|
||||
The AGENTS decision index names choices, such as "A message id base is merged at most once", whose reasoning lives in `docs/decisions.md`, which I was not allowed to open. For a few (the `spent` store and `LinkLostError` retry), the title alone left me unsure whether the behaviour I saw was the intent. The charter also says `IncomingRequests` and `Sms` get "named functions, never the session itself". That is false as written: `SessionPort.session` and `Sms.session` both hand over the full `Session` (`incoming-requests.ts:65`, `:293`).
|
||||
- **Told me nothing:**
|
||||
- most AGENTS architecture one-liners, which restate the file names;
|
||||
- the seven `declare` listener lines, repeated in all three emitters;
|
||||
- `/** Whether the socket has closed. */` on `closed`;
|
||||
- `defaults.ts`'s "README's option tables restate the public ones";
|
||||
- many decision-index bullets that simply restate what the code shows, such as "`reconnect` takes `{ minDelay, maxDelay }`…".
|
||||
|
||||
Intrinsic difficulty: moderate to high. Two alphabets' bit layouts, two concatenation spellings, and receipts sharing a command with messages are the problem's own difficulty, not the code's. They get no bonus.
|
||||
|
||||
5. **Scores**
|
||||
- **Navigation: 7.** At the "predictable" anchor: `src/` is flat, file names follow concepts (`dlr-merger`, `pdu-framer`, `send-window`), and the AGENTS map matched the tree exactly. It stops short of 8 because of placements like `leftOf` in `idle-waiters.ts` and `refusedSegmentStatus` exported from `incoming-requests.ts`.
|
||||
- **Locality: 5.** At the "honest middle": getters with side effects in `ExpiringGroups`, the `weighing` side channel in `Reassembler`, and `SmppClient.send()` being correct only across the timing of three modules are all state changed out of sight.
|
||||
- **Shape: 6.** Between 5 and 7. Files are small and fan-out is bounded, but the vocabulary is overloaded:
|
||||
- answering: `answer`, `sendReturn`, `sendResp`;
|
||||
- refusing: `refuse` in two classes with different meanings, plus `refusedAtBound`;
|
||||
- ending: `over`, `closed`, `stopping`, `end`, `finish`, `ended`;
|
||||
- and `SessionPort` claims a narrow seam while carrying the whole `Session`.
|
||||
- **Self-sufficiency: 6.** Between 5 and 7. Comments cite SMPP sections and state the why in place (for example `respIdParams` and `refusalStatus`). But the answer rules and receipt classification needed README sections open beside them, and the decision index points at reasoning that is not in the code.
|
||||
- **Overall: 6.** Capped by locality (5 + 1). The code reads cleanly line by line; what costs is the few places where state moves out of sight.
|
||||
|
||||
SCORES nav=7 loc=5 shape=6 self=6 overall=6
|
||||
|
||||
## Draft F, senior seat
|
||||
|
||||
1. **Hardest places, ranked**
|
||||
|
||||
1. **`src/incoming-requests.ts:260-351`, `IncomingRequests.onMessage` / `handOver` / `refuse`, plus `refusedAtBound` at :224.** Before I could predict the answer to one inbound PDU, I had to hold eight branches at once:
|
||||
- the running-handler bound;
|
||||
- concatenated or whole;
|
||||
- kept or refused, and full or unplaceable, with the refusal status differing between sar and UDH;
|
||||
- whole or partial;
|
||||
- link already closed;
|
||||
- no `onSms`;
|
||||
- the handler threw or returned;
|
||||
- answered on arrival or not.
|
||||
|
||||
"Who answers the peer, and when" is spread over `onMessage`, `handOver`, `refuse`, `createSms(answeredAs)` and `sms.sendResp`/`alreadyAnswered` in another file (`sms.ts:106-144`). `refusedAtBound` returns a boolean but also writes the answer and flips the `refusing` flag. The comment at :256 and README "Receiving in depth" / "Server in depth" resolved it, but only after two passes.
|
||||
2. **`src/reassembly.ts:91,113-137,180-198`, `Reassembler.collect` / `weighed` / `dropped`.** The `weighing` field is a side channel. It is set around `groups.weigh()` so that the synchronous `onDrop` callback, which re-enters `dropped()`, can subtract the newest segment from the reported loss. `collect` also inserts the segment into `group.parts` before it knows whether the group survives the weighing. The comments at :90 and :124 made it resolvable, but only by tracing the re-entrancy by hand.
|
||||
3. **`src/expiring-groups.ts:250-266,295-306,323-334`, the `ExpiringGroups` getters `full` / `size` / `weight` and `weigh`.** Reading a property runs `expire()`, which fires `onDrop` callbacks. Through `RunningHandlers` (`running-handlers.ts:259`) that means a plain read of `this.running.size` inside a log call in `refusedAtBound` (`incoming-requests.ts:229`) can log a "giving up on a handler" warning and wake a drain. The class comment says "the expired go on every access". Nothing at the call sites marks it. This stayed partly opaque: I am not certain every caller tolerates it.
|
||||
4. **`src/session.ts:211-221,291-332`, `Session.unbind` / `drain` / `finish` / `end`.**
|
||||
- There are three lifecycle flags: `over`, `stopping` and `bind`.
|
||||
- `stopping` is also set from outside, through `SessionPort.end` (:262).
|
||||
- `drain` reads the same state as `this.over` at :294 and as `this.closed` at :303.
|
||||
- `unbind` deliberately bypasses `send()`'s stopping check by calling `outgoing.request` directly, uncommented.
|
||||
- `droppedOnUnbind` needed its docblock plus the charter's decision index ("a close arriving after our own unbind is clean") before I trusted it.
|
||||
5. **`src/client.ts:143-156,202-217,415-438` with `src/reconnect-loop.ts:64-153`, `SmppClient.send` retry loop / `takeFirst` / `keepTrying` / `ReconnectLoop`.** The `LinkLostError` contract spans four files. `send-window.close` sets it for queued requests and `OutgoingRequests.attempt` sets it on a write failure (`outgoing-requests.ts:269,287`). `SmppClient.send` consumes it, with `ReconnectLoop.bound`/`release` in between. The first session is opened outside the loop and then `adopt`ed. `links === 1` means "first", and `links > 0` decides `unref()`. Resolved by the `LinkLostError` class doc and the README's "Sends and the link".
|
||||
6. **`src/pdu.ts:74-136`, `resolveShortMessage` / `resolveBody`.** The rule for which of `short_message` and `message_payload` may overwrite `data_coding` uses `CodingSource`. An empty Buffer counts as `message_payload`, and only a non-empty encoded `short_message` rewrites the coding. I read it three times. The `CodingSource` doc resolved it.
|
||||
7. **`src/defs/encodings.ts:476-515`, `messageClassOf` / `messageClassEncoding` / `encodingByDataCoding`.** This is bit arithmetic over GSM 03.38 coding groups that I had no background in. A `//` comment and a `/** */` for different concerns are stacked on one function (:484-486). The comments carry the rule, so it was domain cost rather than code cost.
|
||||
8. **`src/defs/tlvs.ts:103-126,284-306`, the `WriteValue` / `Repeated` conditional types and `keyedTlvs` → `isTlvs`.** The type layer takes a slow read. The runtime re-validation that ends in "a defect in this library" exists only to avoid a cast. I understood why only through hard rule 4 in the charter.
|
||||
|
||||
2. **The unit I would least want to modify:** `IncomingRequests.onMessage`/`handOver` together with `sms.ts` `sendResp`/`alreadyAnswered`. The invariant "every PDU gets exactly one answer, and no answer names an id or refusal after the segments were answered" is not stated in one place. It is enforced jointly by the `answeredAs` argument, the mutable `answer` object captured in `createSms` closures, the `closed()` check placed after `createSms`, and `refuse`'s `answeredOnArrival` branch. A change to any one of these can double-answer or silently drop, and only a test would tell me.
|
||||
|
||||
3. **Expected to be hard, found easy:**
|
||||
- The codec: `pdu.ts` read/write, `defs/types.ts` wire types, `pdu-framer.ts`. It is long but uniform and bounds-checked the same way everywhere.
|
||||
- `PendingRequests`, `SendWindow` and `IdleWaiters`: small, one job each.
|
||||
- `dlr.ts` receipt parsing, with the operator quirks commented inline.
|
||||
- `send-sms.ts`: a linear checklist, then fan-out.
|
||||
- The server bind composition in `server.ts:508-533`.
|
||||
|
||||
4. **Prose debt**
|
||||
- **Needed:**
|
||||
- README "Receiving in depth" and "Server in depth", for answered-on-arrival semantics and the retry statuses. Found by the table of contents, so the cost was low.
|
||||
- The charter's architecture map. It was the most valuable document: every file is listed with a truthful one-liner.
|
||||
- The decision index titles. They told me that behaviours like the unbind-close and five-minute handler were deliberate. With `docs/decisions.md` off-limits, a title was sometimes all I had.
|
||||
- I opened no tests.
|
||||
- **Told me nothing new:**
|
||||
- The AGENTS.md "Defects found in 0.4.0" table (history, irrelevant to reading `src/`).
|
||||
- The long test-fixture convention paragraphs.
|
||||
- README "Methods", which lists names already typed.
|
||||
- Comments that restate code:
|
||||
- `session.ts:148` "Whether the socket has closed."
|
||||
- `session.ts:172` "Sends a request and resolves with the peer's response."
|
||||
- `server.ts:631` "Starts listening… Resolves once the socket is bound."
|
||||
- `tlvs.ts:14` "Ordered by tag id", which repeats the charter.
|
||||
- The seven `declare` listener lines, copied across three emitters, are boilerplate rather than prose, but they are reading cost all the same.
|
||||
|
||||
5. **Scores**
|
||||
- **Navigation 8:** between "predictable" and "near duress-proof". AGENTS.md's file map matches `src/` one-to-one, and names like `pdu-refusal.ts`, `send-window.ts` and `dlr-merger.ts` lead from a symptom to the file first try. The detour is the answer path, split across `incoming-requests.ts` and `sms.ts`.
|
||||
- **Locality 6:** between "honest middle" and "predictable". The seams are named and narrow (`SessionPort`, `SmsDeps`, `SendSmsDeps`, `LinkLostError`). Three things still break locality:
|
||||
- `ExpiringGroups` getters fire callbacks on read.
|
||||
- `Reassembler.weighing` is a re-entrancy side channel.
|
||||
- `Session`'s flag trio is mutated through `port.end`.
|
||||
- **Shape 7:** "predictable". No file is past about 440 lines and fan-out per level is small. A few names mislead:
|
||||
- `ExpiringGroups` is used for running handlers and spent ids, which are not groups.
|
||||
- `closed` and `over` are two names for one state.
|
||||
- `refusedAtBound` answers the peer as a side effect.
|
||||
- `ReconnectLoop.attempt`'s comment calls the library's own connect and bind "the application's".
|
||||
- **Self-sufficiency 7:** "predictable". Comments carry the SMPP section and the why at the non-obvious points (`sms-id.ts:623`, `message.ts:13`, `dlr.ts:247-253`, `pdu-refusal.ts:446`). The answered-on-arrival contract is the one thing that needed the README open beside the code.
|
||||
- **Overall 7:** capped at 7 by locality. A cold senior is productive within a week and knows which corners to fear. Intrinsic difficulty is moderately high (protocol quirks plus a concurrent drain and reconnect), and that earns no bonus.
|
||||
|
||||
SCORES nav=8 loc=6 shape=7 self=7 overall=7
|
||||
|
||||
## Draft F, architect seat
|
||||
|
||||
**Architect, inherited: comprehension report on @larvit/smpp (draft-f)**
|
||||
|
||||
## 1. Map from README and tree only (verbatim, written before opening any source)
|
||||
|
||||
Top-level areas I expect, although `src/` is flat and shows none of them:
|
||||
1. **Public handles**: `index.ts`, `client.ts` (SmppClient, reconnecting), `server.ts` (listener, one Session per connection), `session.ts` (one socket's life), `sms.ts` (the inbound `sms` handle with sendResp/sendDlr).
|
||||
2. **Session machinery**: `link-timers` (enquire_link and idle), `reconnect-loop` (backoff), `send-window` (maxOutstanding), `pending-requests` (seqNr correlation and timeout), `outgoing-requests` (window plus pending), `incoming-requests` (dispatch of peer requests), `running-handlers` (onSms handlers in flight, "Handlers still running" in the README), `idle-waiters` (maybe the idle timeout?), `pdu-transport` and `pdu-framer` (socket to PDUs).
|
||||
3. **Codec**: `pdu.ts`, `pdu-refusal` (PduRefusedError), `retained-pdu` (a PDU kept for retry?), and `defs/` for the spec tables and wire types.
|
||||
4. **Message content**: `message.ts` (encode, split, bitCount, smppTime), `message-body` (short_message vs message_payload), `concat` and `udh` (probably the reading and writing of concatenation), `reassembly`, `expiring-groups` (the reassembly store?), `send-sms` (submit composition).
|
||||
5. **Receipts**: `dlr.ts`, `dlr-merger` (messageDlr), `sms-id` (notations, `<base>-<n>`).
|
||||
6. **Plumbing**: `result`, `log`, `error-from`, `uuid`, `defaults`, `session-options`, `unanswered-error` (went out, no answer), `link-lost-error` (never reached the socket).
|
||||
|
||||
Names that do not give their purpose: `idle-waiters`, `retained-pdu`, `expiring-groups`, `error-from`, and `defaults` vs `session-options`.
|
||||
|
||||
What the README led me to expect: a store interface (goal 9). The README itself says it has not shipped, so its absence is fine.
|
||||
|
||||
## Where the map was wrong, and what each correction cost
|
||||
|
||||
- **`idle-waiters`** is a primitive that waits for a count to fall to zero. It is not the idle timeout, and it also exports `leftOf()`, the deadline arithmetic `client.ts` uses. Cost: low. The misleading part is `leftOf` living there.
|
||||
- **`retained-pdu`** copies a PDU off the wire so holding it does not pin the chunk, and weighs what holding it costs. It has nothing to do with retry. Cost: low. The copy invariant is split with `defs/tlvs.ts:250`, which already copies TLVs, so `retained-pdu.ts:5`'s claim that "wire reads hand back views" is only half true.
|
||||
- **`expiring-groups`** is a generic capped, weighed, expiring map. Besides reassembly it also backs `DlrMerger` (twice) and, surprisingly, `RunningHandlers` as a counter (`ExpiringGroups<true>` keyed by serial). Cost: medium. "Groups" misleads for the handler counter.
|
||||
- **`concat` / `udh`**: splitting is in `message.ts`. `udh.ts` holds the outgoing `ConcatReference` counter plus the parse `concatInfo`. `concat.ts` chooses between UDH and `sar_*`. Cost: medium. It took three files to place the concatenation concepts.
|
||||
- **`session-options`** is not only options. It also holds bind-direction policy (`bindCarries`, `standsInFor`), `SessionEvents`, the hook types, and validation for client- and server-only options (`authenticate`, `connectTimeout`, `reconnect`, `fromStart`). Cost: medium. I would never have looked there for "which way does a data_sm travel".
|
||||
- **`pdu.ts` depends on `message.ts`** (`encodeBody`, `decodeMessage`), which depends on `udh.ts`. So the codec sits above the message layer, not just above `defs/`. Cost: low, but the layering in the AGENTS text is incomplete.
|
||||
- The rest of the map held.
|
||||
|
||||
## Fan-out, level by level
|
||||
|
||||
- **L0, the repo:** `src`, `test`, `docs`, `benchmarks`, `interop-tests`, plus about 8 top-level `.md` files. Fine.
|
||||
- **L1, `src/`:** 36 files plus `defs/`, so 37 entries. **This is the worst level.** About six real areas exist, but the layout shows none of them. The only map is the Architecture block in AGENTS.md.
|
||||
- **L2, `defs/`:** 7 files, clean.
|
||||
- **L3, the big units:**
|
||||
- `Session` composes 4 collaborators plus a hand-built `SessionPort` of 12 members.
|
||||
- `IncomingRequests` holds `Reassembler`, `RunningHandlers`, `createSms`, the port and the hooks, and imports 23 symbols.
|
||||
- `SmppClient` holds `ReconnectLoop`, `DlrMerger` and `ConcatReference`, plus about 200 lines of free connect and bind functions.
|
||||
|
||||
## Names
|
||||
|
||||
**Names that mislead**
|
||||
- `IncomingRequests.drain` (`incoming-requests.ts:147-153`) calls the count of running handlers `unanswered` and reports "Shut down with N message(s) unanswered". A handler that already called `sendResp()` is still counted. That is the 3am bug's own error text pointing the operator at the wrong thing.
|
||||
- `RunningHandlers`' `onDrop` logs "giving up on a handler that never returned" for any drop, including an `evicted` one. Eviction can only be avoided because `refusedAtBound` is checked first, somewhere else (`running-handlers.ts:28`).
|
||||
- `session-options.ts`, as above.
|
||||
- `decodeSegments` lives in `reassembly.ts` but is used by `sms.ts`.
|
||||
- `ExpiringGroups` used as a counter.
|
||||
|
||||
**One name over several concepts**
|
||||
- **refuse:** `Session.refuse` (codec-refused PDU), `IncomingRequests.refuse` (application did not take the message), `refusedAtBound`, `Refusal` (a reassembly slot), `refusedSegmentStatus`, `refusalAnswer`, `PduRefusedError`.
|
||||
- **end:** `Session.end`, `OutgoingRequests.end` and `IncomingRequests.end` all mean "the socket is gone". `SessionPort.end` means "the peer unbound, destroy the socket". `SmppClient.end` means "the client is over".
|
||||
- **answer:** `SessionPort.answer` writes a response. `sms.ts`'s `Answer` is mutable answered-state. `answerOf` converts the handler's return.
|
||||
- **closed:** a public getter on `Session`, a private field on `SmppClient`, and a port function.
|
||||
|
||||
**One concept with two names**
|
||||
- Session lifecycle: `over` / `closed`, and `stopping` / "shutting down".
|
||||
- The UDH indicator is checked through `hasUdh` in 5 places, and the body is sometimes `params.short_message` (a string, or a Buffer when a UDH is present) and sometimes `shortMessageOctets`.
|
||||
- The submit bind check is spelled twice: `client.ts:159` reads `options.bindType`, `session.ts:195` calls `bindAllows`.
|
||||
|
||||
## Restructure, ranked
|
||||
|
||||
1. **Split `IncomingRequests`.** Routing (`route`, `unhandled`, `onDelivery`) is one piece. Message intake (bound refusal, reassembly, answer-on-arrival, `handOver`, `refuse`) is a second, called something like `MessageIntake`. It is the densest unit and the one that grows.
|
||||
2. **Carve bind-direction policy out of `session-options.ts`** into `bind-direction.ts`, and move client/server option validation next to its owners or into an `option-checks.ts`.
|
||||
3. **Make the "drain waits on" counter say what it counts.** Either release it on `sendResp()` plus pending receipts, or rename the error to "handlers still running". Also stop using `ExpiringGroups` as the handler counter.
|
||||
4. **Put concatenation in one module:** `ConcatReference`, `concatInfo`, `concatOf`, `udhLength`, and the UDH build in `splitMessage`.
|
||||
5. **Group `src/` into 4–5 folders:** handles, link, codec, message, receipts. The flat 37 files is the main Shape cost.
|
||||
6. **Extract the emitter guard.** The `emit` override, `captureRejectionSymbol` and 7 `declare` lines are copied three times (Session, SmppClient, SmppServer).
|
||||
|
||||
## What the structure gets right
|
||||
|
||||
- Narrow seams: `SessionPort`, `SmsDeps`, `SendSmsDeps`, and the `ReconnectLoopOptions` callbacks. Collaborators do not reach into the Session.
|
||||
- Every unit is small and single-noun (`SendWindow`, `PendingRequests`, `LinkTimers`, `PduFramer`).
|
||||
- `Result` is used everywhere, so control flow reads top-down.
|
||||
- Comments carry spec sections and the peer quirks behind them (Jasmin, CM.com, Kaleyra).
|
||||
- `defaults.ts` is the single source of numbers.
|
||||
- `LinkLostError` vs `UnansweredError` encodes goal 2 in the type.
|
||||
|
||||
## The 3am question
|
||||
|
||||
**Time to the right unit, cold: about 5–10 minutes, three hops.**
|
||||
- Hop 1: grep "drain" lands in `session.ts:291`, `Session.drain`.
|
||||
- Hop 2: that calls `this.incoming.drain(timeout, signal)` at `incoming-requests.ts:146`.
|
||||
- Hop 3: that calls `RunningHandlers.idle` (`running-handlers.ts:63`), and I had to find where `start()` and `done()` are called: `IncomingRequests.handOver`, `incoming-requests.ts:311-314`.
|
||||
|
||||
**The answer:** `done()` fires when the `onSms` handler *returns*, not when `sms.sendResp()` is called. `sendResp` (`sms.ts:126`) never touches `RunningHandlers`. So a handler that answers early and keeps working holds the drain until `shutdownTimeout`. That includes a handler awaiting `sendDlr()` to a slow peer: `sendDlr` goes through `port.request`, which bypasses the stopping check, and the outgoing drain then waits on it too.
|
||||
|
||||
- **Is it a bug?** README line 399 documents "Wait … for every onSms handler still running", so it is by design. The `sendResp` docstring ("for a handler that keeps working after the answer") invites exactly this expectation.
|
||||
- **Right file and unit:** `incoming-requests.ts`, `handOver`, together with `running-handlers.ts`.
|
||||
- **What slows the hunt:** the reported error, "message(s) unanswered", is false for this peer and costs an extra detour into `sms.ts`.
|
||||
|
||||
**Where it rots first:** `IncomingRequests.onMessage` / `handOver`. Every new inbound rule lands there (per-PDU rate limiting, the store for half-reassembled messages, new `data_sm` semantics), and each one adds another `port.closed()` check and another answer path.
|
||||
|
||||
**Where the next two features would land**
|
||||
- **Goal 9's store** would land across `Reassembler`, `DlrMerger` and `ExpiringGroups`. `ExpiringGroups` is the obvious seam, but it is shared with the handler counter, which must not be persisted. Separate them first.
|
||||
- **A per-PDU rate limit (goal 7)** would land in `OutgoingRequests.request` beside `SendWindow`. That is a clean place. The inbound side would land in `IncomingRequests` again.
|
||||
|
||||
## Hardest places, ranked
|
||||
|
||||
1. `src/incoming-requests.ts:260-326`, `IncomingRequests.onMessage` / `handOver`. Bound refusal, reassembly, answer-on-arrival, the handler run, and refusal-or-loss are interleaved with `closed()` checks and `sms.sendResp` side effects.
|
||||
2. `src/reassembly.ts:179-198`, `Reassembler.weighed` / `dropped`. The transient `weighing` field is read inside an `onDrop` callback to discount the newest segment. That is action at a distance through a callback.
|
||||
3. `src/session.ts:291-310` with `incoming-requests.ts:146` and `running-handlers.ts:50-75`, `Session.drain`. It orders handlers, then requests, on a shared deadline (`timeout` for the first, `leftOf(deadline)` for the second), then checks `closed`. The meaning of "unanswered" is wrong.
|
||||
4. `src/expiring-groups.ts:111-122`, `ExpiringGroups.expire`. It fires `onDrop` mid-iteration. `RunningHandlers.onDrop` → `settle()` → `size` → `expire()` re-enters it.
|
||||
5. `src/reconnect-loop.ts:64-153` with `client.ts:143-156`, `ReconnectLoop.adopt` / `attempt` / `down` / `stop` and the client `send` retry loop on `LinkLostError`. The state lives in `session`, `timer`, `attempting`, `stopped`, `upAt` and `links`.
|
||||
6. `src/pdu.ts:84-136`, `resolveShortMessage` / `resolveBody`. The rules for which of `short_message` or `message_payload` owns `data_coding`.
|
||||
7. `src/sms.ts:126-231`, `sendResp` / `sendDlr` over the shared mutable `Answer` object.
|
||||
|
||||
**The unit I would least want to modify:** `IncomingRequests`, `src/incoming-requests.ts:87-358`.
|
||||
|
||||
## Scores
|
||||
|
||||
Intrinsic difficulty, which earns no bonus: moderately high. It is an async request/response protocol with reassembly, reconnect, a drain and two ends of the link.
|
||||
|
||||
- **Navigation 7.** Sits at "Predictable". File names map to concepts well enough that the 3am path took three hops. It is held below 8 by the flat 37-file `src/` and by the drain's "unanswered" error text pointing at `sendResp`.
|
||||
- **Locality 6.** Between 5 and 7. The narrow ports (`SessionPort`, `SmsDeps`) keep collaborators apart. Hidden coupling holds it down: `RunningHandlers` evicting live handlers unless `refusedAtBound` runs first, the reassembler's `weighing` side channel, and the drain's ordering and shared deadline.
|
||||
- **Shape 6.** Between 5 and 7. Units are small and mostly honest. Held down by the flat `src/` with no visible areas, `session-options.ts` as a grab bag, concatenation spread over three files, and the overloaded refuse/end/answer/closed vocabulary.
|
||||
- **Self-sufficiency 7.** Sits at "Predictable". Most units state their invariant and cite the SMPP section at the site (`message.ts:13`, `pdu.ts:166`, `sms-id.ts:58`). Held below 8 because the area map and the import direction exist only in the AGENTS.md architecture block, not in the layout.
|
||||
- **Overall 6.** Capped at 7 by the lowest dimension plus one. It sits at 6 because the unit that grows, `IncomingRequests`, is the hardest one to change safely.
|
||||
|
||||
SCORES nav=7 loc=6 shape=6 self=7 overall=6
|
||||
Reference in New Issue
Block a user