feat: add onDispose hook#1488
Conversation
Resolves #1487 Adds `event.onDispose(cb)`: fires exactly once after global `onResponse` when the event is fully over — response body finished, client disconnected, or body errored — on every runtime, not just Node. - Node: observes `res` "close" (accurate for streaming and non-streaming bodies alike; body is never wrapped) - Other runtimes: streaming bodies are piped through an identity TransformStream whose `pipeTo` promise settles on all three termination paths; bodyless/non-streaming responses dispose at response production - Opt-in and lazily armed: routes that never register pay one symbol check - Callback receives `undefined` on normal completion or the cancel/abort reason; sync throws and async rejections are absorbed (reported unless `silent`), pending async work goes to `waitUntil` - `createEventStream` autoclose now uses `onDispose`, fixing the web-runtime leak where a stream that is created but never sent never closes Co-Authored-By: Claude Fable 5 <[email protected]>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesAdds the public Event disposal lifecycle
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/utils/internal/dispose.ts (2)
1-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefix this new internal module with
_.Rename
dispose.tsto_dispose.tsand update its direct imports.As per coding guidelines, “Use
_prefix for internal files.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/internal/dispose.ts` around lines 1 - 5, Rename the internal module from dispose.ts to _dispose.ts and update every direct import or reference to the kEventDispose symbol to use the new _dispose module path, without changing the module’s behavior.Source: Coding guidelines
22-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse options objects for these multi-argument functions.
Convert each second and subsequent parameter into an options object, including the private helpers.
As per coding guidelines, “Use options object as second parameter for multi-argument functions.”
Also applies to: 44-44, 84-99
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/internal/dispose.ts` at line 22, Update registerDispose and the other affected disposal functions and private helpers around the referenced symbols to accept a single options object after the primary parameter, moving each existing second and subsequent argument into named properties. Update all internal call sites and destructuring so behavior remains unchanged, including the helpers near lines 84–99.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/1.guide/900.api/2.h3event.md`:
- Around line 78-86: Update the onDispose callback documentation in the event
disposal example so a reason of undefined is described only as normal disposal,
not as confirmation that the client received the complete response. Keep the
abort/disconnect or body-error description unchanged and align the comment with
the runtime-specific caveat below.
In `@src/utils/internal/dispose.ts`:
- Around line 72-76: Update the rejection handler in the TransformStream body
pipeTo flow to convert an undefined rejection reason into an AbortError before
passing it to fireDispose, while preserving existing non-undefined reasons. Add
a dispose test assertion covering reader.cancel() without an argument and
verifying the resulting AbortError.
---
Nitpick comments:
In `@src/utils/internal/dispose.ts`:
- Around line 1-5: Rename the internal module from dispose.ts to _dispose.ts and
update every direct import or reference to the kEventDispose symbol to use the
new _dispose module path, without changing the module’s behavior.
- Line 22: Update registerDispose and the other affected disposal functions and
private helpers around the referenced symbols to accept a single options object
after the primary parameter, moving each existing second and subsequent argument
into named properties. Update all internal call sites and destructuring so
behavior remains unchanged, including the helpers near lines 84–99.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3c792328-9c2d-4d5f-b7ac-55a0b4ac9460
📒 Files selected for processing (8)
docs/1.guide/900.api/2.h3event.mdsrc/event.tssrc/response.tssrc/utils/internal/dispose.tssrc/utils/internal/event-stream.tstest/bench/bundle.test.tstest/dispose.test.tstest/sse.test.ts
Moves the end-of-event machinery behind the composable util: the first registration installs the arming function on the event, so core (`toResponse`) only keeps a symbol check and an `arm` call. Apps that never import `onDispose` no longer bundle any of it. Core bundle overhead drops from ~1030 B min / ~400 B gzip to ~105 B min / ~42 B gzip vs main (limits ratcheted accordingly). Also matches h3's util-first surface (`getQuery(event)`, `readBody(event)`, ...) instead of adding an `H3Event` method. Co-Authored-By: Claude Fable 5 <[email protected]>
The public wrapper moves from `utils/event.ts` to `utils/response.ts` so automd renders it on the response utils page; export relocated to the Response block in `index.ts` accordingly. No behavior change. Co-Authored-By: Claude Fable 5 <[email protected]>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/utils/internal/dispose.ts (1)
99-102: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve abort reasons from
pipeTo().As noted in a previous review,
reader.cancel()with no argument rejectspipeTo()withundefined, whichfireDispose()incorrectly treats as normal completion. Normalize the rejectedundefinedto anAbortErrorso that a client abort correctly surfaces as a disposal reason rather than a successful completion.🐛 Proposed fix
body.pipeTo(writable).then( () => fireDispose(event, state, undefined), - (reason) => fireDispose(event, state, reason), + (reason) => fireDispose(event, state, reason ?? new DOMException("Stream aborted.", "AbortError")), );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/internal/dispose.ts` around lines 99 - 102, Update the rejection handler in the body.pipeTo flow to normalize an undefined rejection reason to an AbortError before passing it to fireDispose, while preserving any existing non-undefined reason unchanged. Ensure client aborts are reported as disposal failures rather than successful completion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/utils/internal/dispose.ts`:
- Around line 99-102: Update the rejection handler in the body.pipeTo flow to
normalize an undefined rejection reason to an AbortError before passing it to
fireDispose, while preserving any existing non-undefined reason unchanged.
Ensure client aborts are reported as disposal failures rather than successful
completion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 52e61f24-5fe2-4d6f-a257-37c67ee47988
📒 Files selected for processing (10)
docs/1.guide/900.api/2.h3event.mddocs/2.utils/9.more.mdsrc/index.tssrc/response.tssrc/utils/event.tssrc/utils/internal/dispose.tssrc/utils/internal/event-stream.tstest/bench/bundle.test.tstest/dispose.test.tstest/unit/package.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/utils/internal/event-stream.ts
- test/bench/bundle.test.ts
- src/response.ts
- test/dispose.test.ts
Clearer internal vocabulary: `state.observe(response)` starts observing the prepared response for end-of-event; `observing` guards against a second attach. Matches the "body observation" wording used in the docs and comments. No behavior change. Co-Authored-By: Claude Fable 5 <[email protected]>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…ompletion `reader.cancel()` with no argument rejects the observing `pipeTo` with `undefined` — which is the documented reason for *normal completion*. Normalize an undefined rejection to an `AbortError` `DOMException` (strict undefined check, so a deliberate `cancel(null)` reason is preserved). Extracts the shared `abortError()` helper used by the Node path. Co-Authored-By: Claude Fable 5 <[email protected]>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/utils/internal/dispose.ts (1)
100-103: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNormalize undefined rejection reason to AbortError.
As per past review comments,
reader.cancel()with no argument rejectspipeTo()withundefined, whichfireDisposetreats as a normal completion instead of an abort. Normalizeundefinedto anAbortErrorto correctly preserve the cancellation semantics.🐛 Proposed fix
- body.pipeTo(writable).then( - () => fireDispose(event, state, undefined), - (reason) => fireDispose(event, state, reason), - ); + body.pipeTo(writable).then( + () => fireDispose(event, state, undefined), + (reason) => + fireDispose( + event, + state, + reason ?? new DOMException("Connection closed prematurely.", "AbortError"), + ), + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/internal/dispose.ts` around lines 100 - 103, Update the rejection handler in the body.pipeTo(...).then flow to convert an undefined reason into an AbortError before passing it to fireDispose. Preserve existing rejection reasons unchanged, while ensuring cancellation without a reason is treated as an abort rather than normal completion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/utils/internal/dispose.ts`:
- Around line 100-103: Update the rejection handler in the body.pipeTo(...).then
flow to convert an undefined reason into an AbortError before passing it to
fireDispose. Preserve existing rejection reasons unchanged, while ensuring
cancellation without a reason is treated as an abort rather than normal
completion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8d349cd6-08b3-4bd3-b6b9-369ae974362e
📒 Files selected for processing (2)
src/response.tssrc/utils/internal/dispose.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/response.ts
… node Node emits `res` "close" as soon as the socket dies. When the client aborts while the handler is still running, the event fires before `toResponse` attaches the listener, so disposal never fired. Check `res.closed`/`res.destroyed` at observe time and dispose immediately. Co-Authored-By: Claude Fable 5 <[email protected]>
On web adapters srvx's `FastResponse` is the native `Response`, so piping every non-null body through the identity `TransformStream` replaced buffered sources (string, bytes, JSON) with a JS stream — losing the runtime's native-source send path and implicit content-length. `toResponse` now passes the resolved handler value to the observer, which wraps only live producers (`ReadableStream`, `Blob`, an opaque `Response`, or an `HTTPResponse` carrying a stream). Buffered bodies dispose at response production — matching the documented semantics for non-streaming bodies on non-Node runtimes. Co-Authored-By: Claude Fable 5 <[email protected]>
The state lookup stays inside each exit (not hoisted) so a first registration made inside the `onResponse` hook is still observed. Co-Authored-By: Claude Fable 5 <[email protected]>
onDispose portable end-of-event hookonDispose hook
Resolves #1487 (prototype of the refined proposal). Related: #1478 / #1484.
What
You start a timer, subscribe to something, or open an upstream connection inside a request handler. Where do you clean it up? Today the answer depends on where your app runs: on Node.js you can listen to
res.on("close"), on Bun / Deno / Cloudflare Workers there is no reliable place at all — so timers keep ticking after clients are long gone.onDispose(event, cb)is one answer that works on every runtime:The callback runs exactly once, when the request is truly over — the response finished streaming, the client disconnected, or something errored mid-stream. On disconnect/error, it receives the reason.
Like other h3 utils (
getQuery(event),readBody(event)), it's just an importable function — apps that never use it don't ship a single byte of it.How
event.runtime.node.respresent): observesres"close"— accurate for streaming and non-streaming bodies, and the body is never wrapped, so srvx'sFastResponse_toNodeResponsefast paths stay intact. Premature close disposes with anAbortErrorDOMException(orres.errored).TransformStream; thepipeTopromise settles on all three termination paths (normal end → resolve, consumer cancel → reject with the cancel reason, which also propagates upstream, source error → reject). Bodyless responses dispose at response production — once theResponseis handed to the runtime, delivery is not observable on web (per the fidelity table in Addevent.onDispose(): a portable end-of-event hook for resource cleanup #1487).onDisposeregistration installs anobservefunction on the event;toResponseonly keeps a symbol check +observecall at its single final exit (afteronResponseresolves, so the hook ordering holds on every path). Because registration carries the machinery, apps that never importonDisposedon't bundle any of it — this is why it is a util rather than anH3Eventmethod (a method would anchor ~1 kB min in every core bundle).console.errorunlesssilent) — the exact rake found in fix(event-stream): correct stream teardown on close and client disconnect #1484. Pending async callbacks are handed toevent.waitUntil()for serverless runtimes. Registering after disposal fires immediately with the recorded reason.Consumer:
createEventStreamThe Node-only
res.once("close", () => this.close())autoclose is replaced withonDispose(event, () => this.close()). This fixes two pre-existing web-runtime leaks (verified failing onmain, matrixwebtarget):send()-ed never ran itsonClosedcallbacks (not covered by fix(event-stream): correct stream teardown on close and client disconnect #1484);Note:
onClosed's own disconnect-path bug (writer.closedrejection swallowed) remains #1484's scope — this PR intentionally doesn't touch it.Costs
toResponse); the ~1 kB machinery is bundled only whereonDispose(orcreateEventStream) is imported. Limits ratcheted with the same tight headroom as before (mainsat 32 B under).Tests
test/dispose.test.ts(matrix, 8 cases): non-streaming fire, mid-stream non-fire + fire on full consumption, disconnect reason propagation (web: exact cancel reason; node: abort error),onResponseordering, callback order + sync/async error absorption, late registration, unmodified fast path, status/header preservation on wrapped responses. Plus the two SSE regression tests above. Full suite green (1543 passed), lint/typecheck/build clean.Caveats / open questions
EventStreamautoclose now depends on the event flowing throughtoResponse(true for any H3 app). A consumer invoking handlers directly withouttoResponseloses the old Node-only fallback.event.disposed: Promise<unknown>remains a possible alternative/additional shape (see Addevent.onDispose(): a portable end-of-event hook for resource cleanup #1487) — trivial to derive from this registry if preferred (though as anH3Eventmember it would re-anchor the machinery in core bundles).onDispose(event, cb)sits next to theonRequest/onResponse/onErrormiddleware factories in the public namespace despite having a different shape (per-event util). Open to a different name if that reads as a footgun.🤖 Generated with Claude Code
Summary by CodeRabbit
onDisposeto register one-time per-event cleanup callbacks that run when the event/response lifecycle fully ends, including cancellation and errors.onDisposedocs covering argument semantics, callback timing relative toonResponse, late registration, and failure handling.