fix: audit dropped Results, and document two runtime contracts - #3
Merged
Conversation
… a Defect The drain bug (1e2bc6a) was a class, not an instance: `AsyncResult<T, never>` empties the *error* channel only, so a `Result` awaited for its timing alone can still carry a `Defect`. Every async call site in `packages/start/src` was read — no grep finds all of these shapes — and two more were dropping one. `withApp` awaited `app.exited` in a `finally` and threw the `Result` away, so a shutdown that blew up passed as a green test whenever `use` never read `exited` (invariants #8 is exactly that shape). It now rethrows a `Defect`, and only a `Defect` — a modeled `Err` is an outcome a test may be asserting. A failure thrown by `use` outranks both: it is held while the application is stopped and rethrown unchanged, so a shutdown defect cannot mask the assertion that actually failed. Both READMEs taught `void host.run(...)` in the worked example — a doc sample teaching fire-and-forget propagates further than a bug — and `await unit.result;` in the testing sample. The runtime now observes the unit's failure (`tapFailure`, since a timer has nowhere to return a `Result`) and the test asserts the unit's outcome. The two remaining drops are deliberate and now carry their reason inline: `start.ts`'s `void server.close()` (our own `fromSafePromise` over `server.close(cb)`, so nothing third-party can defect inside it, and it must not be awaited — the socket is `unref`'d and `close` waits out live keep-alive connections, which would delay or strand the exit report) and `drain.ts`'s losing race branch (once the timeout has decided the report, `exited` has settled and a late defect has no consumer left). Neither can float: an `AsyncResult` never rejects. `unthrown/no-unhandled-result` cannot catch this class — it is deliberately syntactic, and an `await` inside a larger expression is not a bare expression statement — so the audit is the substitute for a rule that cannot exist.
The last bare `toBeDefect()` in the suite. It passed on any defect at all — including one the registry minted for a reason the test never intended — while the thrown value is right there in the test. `toBeDefectWith(boom)` pins it, and fails against a mutation that throws something else.
Both surfaced from building the first real runtime against this kernel, and both are silent when broken. Neither was written down anywhere shipped. `UnitMeta.traceId` defaults to `meta.id`, so a runtime that submits a *category* as the id — an HTTP runtime using the route template `"POST /orders"` — gives every request the same trace id, and the ambient record's whole purpose is silently defeated. `traceId` stays optional: `meta.id` genuinely is a correct trace id whenever it is already unique per unit (a queue job id, a broker message id), and a required one would be no more checked — the kernel would have to remember every id ever seen, so `traceId: routeTemplate` would type exactly as well as the bug it replaces. The defect is the unstated contract, not the default, so the contract is now stated: `id` must be unique per unit unless a `traceId` is supplied. `UnitRecord.unitId` is noted as the always-unique field, since telling two units apart never needed `traceId` at all — that is the correlation id, carrying an id from outside the process. A unit is closed the instant its `Result` settles, an idle registry is what the drain waits for, and going idle is the kernel's permission to call `Serving.stop()`. So a runtime that responds to its client *after* `await host.run(...)` returns races the transport being torn down — proven with an 8 MB body (`UND_ERR_SOCKET: other side closed`). The response must be flushed inside the work callback, and the root README carries a compiled sample of the right shape. Documented in the `UnitMeta` / `RunUnit` / `RuntimeHost` TSDoc, both READMEs and CLAUDE.md.
There was a problem hiding this comment.
Pull request overview
This PR tightens the kernel’s “no dropped Result” invariant (ensuring AsyncResult<*, never> outcomes aren’t silently discarded while still able to carry Defects) and documents two non-checkable runtime contracts so runtime authors don’t accidentally violate drain/lifecycle guarantees.
Changes:
- Updates docs/examples to observe unit
Results rather than dropping them (and adds a “two runtime contracts” section in both READMEs). - Adjusts
withAppso shutdownDefects fail tests even when the test body never inspectsexited, while preserving “usethrow wins” semantics. - Strengthens a weak defect assertion (
toBeDefect()→toBeDefectWith()), and adds coverage around the harness behavior.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updates runtime example to observe unit failures; documents the two runtime contracts; avoids dropping unit results in sample test snippet. |
| packages/start/README.md | Mirrors README guidance for package consumers; adds “Writing a runtime” obligations; fixes example result handling. |
| CLAUDE.md | Records the dropped-Result audit rule and the two runtime contracts to keep repo guidance in sync. |
| packages/start/src/with-app.ts | Ensures shutdown Defects are surfaced as throws while preserving use failure precedence. |
| packages/start/src/with-app.spec.ts | Adds tests covering harness behavior for shutdown defects and precedence rules. |
| packages/start/src/units.ts | Adds TSDoc clarifying UnitMeta.id/traceId uniqueness contract. |
| packages/start/src/units.spec.ts | Strengthens defect assertion to verify the expected defect cause. |
| packages/start/src/start.ts | Documents why probe shutdown Result is intentionally dropped on exit. |
| packages/start/src/runtime.ts | Documents runtime contracts on RunUnit/RuntimeHost surfaces. |
| packages/start/src/probes.ts | Documents expectations for probe server shutdown behavior. |
| packages/start/src/drain.ts | Documents the single intentional dropped-Result case (losing race branch) and why it’s safe. |
| packages/start/src/docs-examples.test-d.ts | Keeps compiled docs examples aligned with README changes and matcher availability. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…on node's internals
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to the drain bug found in #2. That was one instance of a class, so this audits the whole kernel for it, and documents two runtime contracts that were recorded nowhere.
1. The dropped-
ResultauditThe trap, restated:
AsyncResult<void, never>looks safe to await-and-discard because the error channel isnever— butnevermeans the error channel is empty, not that theResultcannot be aDefect. Dropping one loses an unmodeled failure entirely.no-unhandled-resultcannot catch this: the rule is deliberately syntactic, and anawaitinside a larger expression is not a bare expression statement. So this audit is the substitute for a rule that cannot exist.Seven sites examined — three needed fixing:
withApp'sfinallyswallowed a shutdownDefect. The worst of the three, because it is in the test harness: an app that failed to shut down cleanly left the test green. It now rethrows a Defect. A modeledErris not rethrown, and a throw fromuseitself outranks both — a test's own assertion failure must always win.void host.run(...)andawait unit.result;— fire-and-forget in a doc sample is worse than a bug, because it propagates into every runtime someone writes from it.Two kept, each with an inline justification rather than silently:
start.ts'svoid server.close()— our own wrapper with no third-party code inside it, and awaiting it would delay or strand the exit report. The socket isunref'd.drain.ts's losing race branch — once the deadline has decided, no consumer remains.2.
toBeDefect()→toBeDefectWith()The last bare one. A bare
toBeDefect()passes on the wrong defect, which is the same weak-assertion class the audit exists to remove.3. Two runtime contracts, documented
Both were surfaced by building the first real runtime, and neither was written down anywhere a runtime author would look.
UnitMeta.traceIddefaults tometa.id. A runtime passing a non-uniqueid("POST /orders") gets the same trace id on every request, silently defeating the ambient record's entire purpose. Kept optional deliberately — a required field would be equally unchecked (traceId: "POST /orders"types fine), andmeta.idis genuinely correct when it is already unique, as a queue job id is. The real fix was stating the contract:idmust be unique per unit if you do not supply atraceId.await host.run(...)returns racesstop()destroying the socket; the response must be flushed inside the unit. Proven with an 8 MB body (UND_ERR_SOCKET: other side closed).Verification
107 tests, up from 104. Coverage still 100% lines and functions. Full six-command gate green, verified locally before pushing.