Skip to content

fix: close 18 defects found by a full review of the repo - #11

Merged
btravers merged 4 commits into
mainfrom
fix/review-findings
Aug 12, 2026
Merged

fix: close 18 defects found by a full review of the repo#11
btravers merged 4 commits into
mainfrom
fix/review-findings

Conversation

@btravers

Copy link
Copy Markdown
Contributor

A full review of packages/start, examples/ and the docs turned up 18
defects. All are fixed here, test-first: every code change was driven by a test
watched failing for the defect itself, and the RED output is quoted in the
review notes below.

Kernel (7)

Five change observable behaviour, so this is a minor changeset.

DefectEffect
drain.ts sampled awaitIdle() alongside Serving.drain instead of behind ita unit opening while drain resolved was abandoned with the whole budget unspent
stop() / uncaught never aborted in-flight unitswork ran on unsignalled against the state the skip exists to avoid
runMain ignored teardownErrorsa shutdown whose finalisers all failed exited 0
the pre-drain delay ignored time already elapseda signal mid-build paid the delay twice, risking SIGKILL
a bad probe port escaped as a Defectexit 70 where a modeled startup failure exits 1
stderrSink dropped every Error cause{"type":"uncaught","cause":{}}, and nothing at all when circular
the probe server kept no 'error' listener after bindinga transient accept fault tore down the process

Two are worth reading the diff for. The drain one is invisible to
testRuntime, whose drain resolves synchronously — it only bites a runtime
whose drain is a real wait, which is why the matching order-api defect below
compounds it. The probe listener one had an existing test asserting
listenerCount === 0, itself a deliberate fix for the opposite bug (a stale
onBindError resolving a settled deferred); neither zero listeners nor a stale
one is right, so the bind listener is now swapped rather than removed, and
that test was rewritten to pin both halves.

Examples (5)

  • orpc-runtime: drain now marks open responses Connection: close.
    closeIdleConnections() reaches only connections idle at that instant, so a
    busy keep-alive connection survived and node kept serving requests down it —
    measured on Node 22.19: handler invoked twice, socket never destroyed. The
    load-bearing half is marking responses already in flight; their request
    callback ran before the drain existed.
  • orpc-runtime: permanent 'error' listener, same shape as the kernel.
  • orpc-runtime: an empty x-request-id no longer becomes the trace id.
    traceId falls back to meta.id only when nullish, so "" won.
  • queue.ts: a reused message id no longer strands the first publisher
    forever. The example's headline scenario is the duplicate order; only distinct
    ids in the specs hid this.
  • temporal-runtime: forceAfter defaults to 15 seconds, below the
    kernel's drainTimeoutMs, as its own TSDoc already advised.

Docs (6)

#10 moved two sections into packages/start/CLAUDE.md but left the root README
pointing where they used to be, and dropped two facts the repo cannot tell a
session itself — the .js import rule (the options live in an external
tsconfig) and the deliberate >=20 vs >=22.19engines divergence. Also
fixed: six→eight example packages, which CLAUDE.md the sync rule means, and
three→four needs-gate.test-d.ts files (the fourth pins di's gate, not
start's). Every behaviour change above is documented in the same commit as its
code.

Gate

format, lint, typecheck, knip, build green. 101 kernel tests (+8)
and 85 example tests (+3). Coverage holds at 100% lines/functions/statements;
branch coverage improved 96.55% → 97.7%.

One caveat: invariants.spec.ts"binds 9000 when no probe port is given"
could not be run locally — a proxy holds 127.0.0.1:9000 on this machine. It is
unmodified and should pass in CI. Coverage was verified by skipping it
temporarily and restoring it.

Two deliberate omissions

  • orpc-runtime's error-listener fix ships without its own test. Reaching
    the raw http.Server needs vi.mock("node:http") atop the spec (vi.spyOn
    fails with Cannot redefine property), which breaks the examples' test
    convention feat: the application kernel #1 in the workspace whose spec shape is the teaching material. The
    identical mechanism is guarded in packages/start/src/probes.spec.ts.
  • The port-9000 test is untouched. Every alternative weakens it: mocking the
    bind proves the default is passed but not that it works, and a conditional
    skip is exactly the assertion-that-declines-to-run the test conventions
    forbid. Its comment shows this was already weighed once.

🤖 Generated with Claude Code

Benoit Traversand others added 3 commits August 12, 2026 14:36
Five change observable behaviour; each landed test-first, and each test was
watched failing for the defect itself before the fix went in.
- The drain now sequences `awaitIdle()` behind the runtime's `drain` instead of
sampling it alongside. `awaitIdle` answers about the registry at the instant
it is called, so a unit opening while `drain` was still resolving went
unwaited, then was aborted and reported `abandoned` with the entire
`drainTimeoutMs` unspent. `testRuntime`'s synchronous `drain` is what hid it.
- `stop()` and the uncaught path abort in-flight units. Skipping the drain is a
decision not to wait for work, not to leave it running unsignalled — which
contradicted the very reason `"uncaught"` skips it, and let a unit holding a
ref'd socket outlive the exit report.
- `runMain` exits 2 on a non-empty `teardownErrors`. A shutdown whose finalisers
all failed reported success to its orchestrator.
- The pre-drain delay is charged from when the shutdown was requested. A signal
arriving mid-build is buffered, so the delay was paid twice over — enough,
with a slow build, to overrun `terminationGracePeriodSeconds`.
- An out-of-range or non-integer probe port is a modeled `RuntimeStartFailed`.
`listen` validates synchronously and throws, so it escaped as a `Defect`.
- `stderrSink` renders an `Error` cause rather than `{}`, and survives one it
cannot serialise instead of throwing into `safeSink` and losing the event.
- The probe server keeps an `'error'` listener for life. `onBindError` is
replaced rather than removed: neither a stale bind listener nor zero
listeners is correct.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- order-api: `Serving.drain` now marks every response still open
`Connection: close`. `closeIdleConnections()` reaches only the connections
idle at that instant, so a busy keep-alive connection survived it and node
went on serving requests down it for the whole drain window — new kernel
units the drain exists to stop admitting, and ones the deadline then
abandoned. Marking responses already in flight is the load-bearing half: the
request callback for those ran before the drain existed.
- order-api: the HTTP server keeps an `'error'` listener for life, so an accept
failure such as EMFILE cannot reach the kernel's `uncaughtException` handler
and take the whole application down with it.
- order-api: an empty or whitespace-only `x-request-id` is treated as absent.
`traceId` falls back to `meta.id` only when nullish, so `""` won and every
request from such a caller shared one blank correlation id — the defect the
runtime's own doc comment says it exists to prevent.
- order-worker: the queue keeps every waiter for a message id, not the latest.
A second publish with an id still pending overwrote the first, whose
`AsyncResult` then never settled at all.
- order-temporal: `forceAfter` defaults to 15 seconds, below the kernel's
`drainTimeoutMs` default, as its own TSDoc already advised. The old 30s was
above it and equal to Kubernetes' default grace period.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The trim in #10 moved two sections into `packages/start/CLAUDE.md` but left the
root README pointing at where they used to be, and dropped two facts that the
repo cannot tell a session itself.
- README: the Documentation section now points at `packages/start/CLAUDE.md`
for the invariants and internal design, and says eight example packages
rather than six — the two contract packages were added after it was written.
- CLAUDE.md: restores "relative imports carry `.js`". The compiler options
behind it live in `@btravstack/tsconfig/base.json`, under `node_modules`, so
it is the one convention here nothing in the tree can show you; getting it
wrong is a TS2835 a session can only pattern-match its way out of.
- CLAUDE.md: restores the note that the published package's `engines` floor of
`>=20` diverges from the root's `>=22.19` deliberately. Nothing else recorded
that, and aligning them for tidiness is a breaking change to consumers.
- CLAUDE.md: the doc-sync rule now names which of the two `CLAUDE.md` files to
update, which is what went wrong last time.
- CLAUDE.md: four `needs-gate.test-d.ts` files, not three — and the fourth pins
di's UNSATISFIED DEPENDENCIES gate rather than `start`'s runtime-needs gate.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CopilotAI lite review requested due to automatic review settings August 12, 2026 12:37

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses a set of shutdown/drain correctness defects across the @btravstack/start kernel and its example runtimes, with accompanying test coverage and documentation updates to keep runtime invariants and operator-facing semantics aligned.

Changes:

  • Kernel: fixes drain sequencing, shutdown timing (pre-drain delay accounting), unit abort behavior on drain-skipping exits, probe server error handling, and stderr event rendering; updates exit-code semantics to treat teardown failures as unclean exits.
  • Examples: fixes HTTP drain behavior for keep-alive connections, trace ID handling for empty headers, queue settlement behavior for reused message IDs, and aligns Temporal force-shutdown defaults with kernel drain timing.
  • Docs/changeset: updates READMEs / CLAUDE guidance and adds a minor changeset for the kernel behavior changes.

Reviewed changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.

Show a summary per file
FileDescription
README.mdUpdates root documentation for exit codes and corrects doc pointers and example counts.
packages/start/src/start.tsTracks shutdown-request time, adjusts pre-drain delay, and aborts in-flight units on drain-skipping exits.
packages/start/src/start.spec.tsAdds coverage for pre-drain delay accounting and abort-on-stop behavior.
packages/start/src/run-main.tsTreats teardown errors as “unclean” and maps them to exit code 2.
packages/start/src/run-main.spec.tsAdds test ensuring teardown errors produce exit code 2.
packages/start/src/probes.tsPrevents synchronous listen() throws from escaping as defects; keeps a permanent post-bind error listener.
packages/start/src/probes.spec.tsAdds tests for modeled bad-port failures and post-bind error absorption.
packages/start/src/events.tsImproves stderr event rendering for Error causes and adds a safe fallback when JSON serialization fails.
packages/start/src/events.spec.tsAdds tests ensuring Error causes are rendered and circular causes don’t drop the event.
packages/start/src/drain.tsSequences awaitIdle() after runtime drain completion to avoid abandoning units that open while drain is still resolving.
packages/start/src/drain.spec.tsAdds coverage for units opening while runtime drain is still pending.
packages/start/README.mdUpdates package README exit-code semantics to match runMain.
packages/start/CLAUDE.mdUpdates kernel-internals guidance to reflect the fixed invariants and sequencing rationale.
examples/order-worker/src/queue.tsChanges settlement tracking to handle multiple concurrent waiters per message id.
examples/order-worker/src/queue-runtime.spec.tsAdds test ensuring both publishes settle when IDs are reused concurrently.
examples/order-temporal/src/temporal-runtime.tsAdjusts forceAfter default to 15s and documents the rationale vs drain timeout/k8s grace period.
examples/order-api/src/test-fixtures.tsAdds raw keep-alive socket fixture helpers to test drain behavior and empty header cases.
examples/order-api/src/orpc-runtime.tsMarks in-flight responses Connection: close during drain and adds a permanent server error listener; treats blank x-request-id as absent.
examples/order-api/src/orpc-runtime.spec.tsAdds tests for empty x-request-id and for draining a busy keep-alive connection.
examples/order-api/README.mdDocuments the Connection: close requirement during drain for busy keep-alive connections.
CLAUDE.mdUpdates root spec (stderr cause rendering, exit-code semantics, example counts, and doc-drift sync rules).
.changeset/drain-and-shutdown-fixes.mdAdds a minor changeset describing the kernel shutdown/drain fixes.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment threadexamples/order-api/src/orpc-runtime.ts
Comment threadexamples/order-worker/src/queue.ts
…ight
Both from PR review.
- order-api: `closeAfterResponse` could only set `Connection: close` while the
headers were unsent, so a response already streaming when the drain began
kept its connection reusable — the exact gap the marking exists to close. It
now ends that socket on `finish` instead, making the guarantee "no reuse"
rather than "no reuse where we caught the header in time". This router cannot
reach the branch today (`writeHead` and `end` are adjacent in `endWith`), but
a streamed response would, and the README's claim should not rest on that.
- order-worker: append to the waiter array in place instead of rebuilding it.
Reusing a message id is the case the map now exists to support, so copying
per publish made that path quadratic for nothing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@btravers
btravers merged commit 946707c into mainAug 12, 2026
13 checks passed
@btravers
btravers deleted the fix/review-findings branch August 12, 2026 12:50
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@btravers