feat(core): carry the unit's AbortSignal on the ambient record - #44
Merged
Conversation
Closes #23. `RunUnit` hands its work callback the per-unit signal, and @btravstack/http forwards it to the handler — but a middleware-shaped runtime opens its unit around a call it does not own the arguments of: temporal's activityUnits and amqp's messageUnits both hand the kernel a callback that IS the library's next(), so an activity or a handler had no parameter to receive it through, and the kernel's drainTimeoutMs was unobservable from inside the work. UnitRecord gains `signal` — the very controller the callback is handed, not a copy — so currentUnit()?.signal answers in every transport with no per- transport plumbing and no context channel the contract cannot type. It is data about the unit, like `deadline`, with nothing to substitute in a test. Both middlewares now say so where they live, including what the transport's own cancellation is and is not: Temporal's Context.current().cancellationSignal is a different clock (workflow-side cancellation, and shutdownGraceTime), and AMQP has none — a redelivery is recovery, not cancellation. Each starter's suite gains a spec proving the signal reaches the work through a real transport, and both worker examples honour it: the AMQP handler answers a RetryableError so the broker hands the delivery to the next worker, and the Temporal shipping adapter fails as a defect so the platform retries the attempt elsewhere.
There was a problem hiding this comment.
Pull request overview
This PR extends the kernel’s per-unit ambient record (UnitRecord) to include the unit’s AbortSignal, so middleware-shaped runtimes (Temporal activity middleware and AMQP worker middleware) can observe the drain deadline via currentUnit()?.signal without injecting transport-specific context.
Changes:
- Add
signal: AbortSignaltoUnitRecordand ensure it is the same signal passed to unit work. - Add Temporal and AMQP runtime tests/fixtures that assert abort visibility through the ambient record during a timed-out drain.
- Update examples and documentation/CLAUDE specs to describe and demonstrate the new deadline/cancellation story.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/units.ts | Add signal to UnitRecord and populate it from the unit’s AbortController. |
| packages/core/src/units.spec.ts | Add a regression test asserting record signal identity with the work parameter and abort behavior. |
| packages/core/README.md | Document signal as part of the ambient record and why it exists. |
| packages/core/CLAUDE.md | Update kernel invariants/spec narrative to include the ambient signal contract. |
| packages/temporal/src/activity-units.ts | Document why Temporal activities read currentUnit()?.signal (middleware-shaped next() callback). |
| packages/temporal/src/test-fixtures.ts | Add a deadline fixture activity that waits on the ambient-record signal. |
| packages/temporal/src/temporal-runtime.spec.ts | Add a spec proving drain-timeout abort is observable from inside an activity via the record. |
| packages/temporal/CLAUDE.md | Update Temporal package spec to describe the ambient signal route and new spec count. |
| packages/amqp/src/message-units.ts | Document why handlers read currentUnit()?.signal (middleware-shaped next() callback; no native cancellation). |
| packages/amqp/src/test-fixtures.ts | Add a deadline fixture handler that waits on the ambient-record signal. |
| packages/amqp/src/amqp-runtime.spec.ts | Add a spec proving drain-timeout abort is observable from inside a handler via the record. |
| packages/amqp/CLAUDE.md | Update AMQP package spec to describe the ambient signal route and new spec count. |
| examples/order-temporal-worker/src/fulfillment.ts | Demonstrate honoring drain deadline in a Temporal adapter via currentUnit()?.signal. |
| examples/order-amqp-worker/src/handlers.ts | Demonstrate honoring drain deadline in an AMQP handler by returning RetryableError when aborted. |
| docs/reference/core/runtime.md | Update runtime reference to include UnitRecord.signal and clarify it matches the work argument. |
| docs/reference/glossary.md | Update “ambient record” and “unit” glossary entries to include signal. |
| docs/reference/temporal.md | Document the ambient record as the only route to the kernel signal inside activities. |
| docs/reference/amqp.md | Document the ambient record as the only route to the kernel signal inside handlers. |
| docs/how-to/write-a-runtime.md | Add guidance for middleware-shaped runtimes to read currentUnit()?.signal. |
| docs/how-to/tune-the-drain-for-kubernetes.md | Explain that aborted work only stops if it observes the abort, and where to read the signal. |
| docs/how-to/run-a-temporal-worker.md | Add a section showing how to honor the drain deadline from an activity via the ambient signal. |
| docs/how-to/read-the-ambient-unit.md | Expand the how-to to include honoring drain deadline from activities/handlers via signal. |
| docs/how-to/consume-amqp-messages.md | Add a section showing honoring drain deadline in AMQP handlers via the ambient signal. |
| docs/explanation/draining-in-three-beats.md | Clarify abort is observable via both the work parameter and ambient record signal. |
| docs/explanation/ambient-vs-context.md | Update explanation to “five fields” and justify signal as ambient data for middleware runtimes. |
| docs/examples/order-temporal-worker.md | Update example write-up to include the new signal-based deadline honoring behavior. |
| docs/examples/order-amqp-worker.md | Update example write-up to include the new signal-based deadline honoring behavior. |
| CLAUDE.md | Update root thesis/spec sections to include signal in UnitRecord and explain rationale. |
| .changeset/unit-signal.md | Changeset describing the new UnitRecord.signal surface and its motivation. |
| .changeset/initial-kernel.md | Update initial-kernel changeset text to reflect the new ambient record shape. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
Closes #23.
The gap
RunUnithands its work callback the kernel's per-unitAbortSignal, and@btravstack/httpforwards it to the handler as a third parameter. But a middleware-shaped runtime opens its unit around a call it does not own the arguments of —packages/temporal/src/activity-units.tsandpackages/amqp/src/message-units.tsboth hand the kernel a work callback that is the library'snext()— so an activity or a handler had no parameter to receive it through, and the kernel'sdrainTimeoutMswas unobservable from inside the work. The issue is right that a deadline the work cannot observe defeats the point of handing one out.The fix: the ambient record, not a context
UnitRecordgainssignal: AbortSignal— the very controller the callback is handed, not a copy.currentUnit()?.signalnow answers in every transport with no per-transport plumbing.Injecting it into each library's middleware context (
next({ context: { signal } })) was the alternative and was rejected: it reopens the context channel all three starters just removed, it cannot be typed by the transport's contract, and it would be three implementations of one idea. A signal is data about this unit — likedeadline, which was already there in a less usable form — with nothing to substitute in a test, which is the line thesis 2 draws.@btravstack/httpis unchanged.And the written reason the issue also asked for
Both middlewares now state it where they live, including what the transport's own cancellation is and is not:
Context.current().cancellationSignalis a different clock — workflow-side cancellation, and worker shutdown aftershutdownGraceTime. It is not the kernel'sdrainTimeoutMs, so the two are honoured together rather than one standing in for the other.Proof, and the examples (acceptance 2)
packages/temporalandpackages/amqpeach gain adeadlinefixture and a spec driving a real transport: the activity / handler waits oncurrentUnit()?.signal, the drain runs out of time, and the spec asserts{ abandoned: 1, sawAbort: true }in one assertion.packages/coregains a registry spec pinning that the record's signal is the work's own, aborted byabortAll.examples/order-amqp-worker's handler answers aRetryableErrorwhen the signal is aborted, leaving the delivery un-acked for the next worker.examples/order-temporal-worker'sShippingService.arrangefails as a defect, which the platform retries on another worker — the contract'sShippingUnavailableis a permanent no and would be the wrong error for "we ran out of time".Test plan
pnpm --filter @btravstack/docs buildclean