feat(sse): invoke finalize on response stream end#2741
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRequestHandler now defers finalize wiring and cleanup timing to response body settlement. SSE handling switches to a controller-backed readable stream with typed events, and event-stream responses are no longer cloned for transfer. ChangesFinalize and response-stream cleanup
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
1 task
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/browser/sse-api/sse.finally.test.ts (1)
15-127: ⚡ Quick winAdd a multi-connection regression test.
The three scenarios cover happy paths for a single connection but won't catch the per-handler emitter cross-fire concern flagged in
src/core/sse.ts(one connection's close running another connection'sfinalize). A test that opens twoEventSourceinstances against the same handler, closes the first, and asserts that the second connection'sfinalizehas NOT yet been invoked would protect against that regression once it's fixed.🤖 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 `@test/browser/sse-api/sse.finally.test.ts` around lines 15 - 127, Add a new regression test that opens two EventSource connections to the same sse handler and verifies closing the first does not trigger the second handler's finalize; specifically, in the test use setupWorker and sse with a finalize(() => window.notifyFinalizedSecond()) for the second connection (and notifyFinalizedFirst for the first), create two EventSource instances in the page.evaluate, close the first (or call client.close() in the handler for the first), and assert that the Promise for the second connection's finalize has not resolved after the first closes (then finally close the second and assert its finalize resolves); reference the existing symbols setupWorker, sse, finalize, client.close and the exposed functions notifyFinalized* to locate where to add this new test.src/core/sse.ts (1)
311-337: ⚡ Quick winMake
error()consistent withclose()(state ordering + try/catch).
close()swallows controller exceptions and uses#closed.resolve(), buterror()does not. Ifthis.#controller.error()throws (e.g., the stream was already cancelled by the reader, putting the controller in a non-pending state on the controller side while#closedis still pending here),#closednever resolves and the'error'event never gets emitted — both client and any awaiters of#closedbecome stuck. Mirroring the structure used inclose()keeps the two methods symmetric and idempotent.♻️ Proposed fix
public error(): void { if (this.#closed.state !== 'pending') { return } - this.#controller.error() - this.#closed.resolve() - this[kClientEmitter]?.emit(new TypedEvent('error')) + this.#closed.resolve() + try { + this.#controller.error() + } catch { + // + } + this[kClientEmitter]?.emit(new TypedEvent('error')) }🤖 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/core/sse.ts` around lines 311 - 337, The error() method must be made symmetric to close(): first check this.#closed.state !== 'pending' and return, then call this.#controller.error() inside a try/catch (swallowing controller exceptions), always call this.#closed.resolve() after the controller call (even if controller.error() threw), and finally emit the 'error' event via this[kClientEmitter]?.emit(new TypedEvent('error')); modify the error() implementation in the class containing methods error() and close() to mirror close()’s ordering and try/catch to ensure idempotence and that `#closed` is always resolved.
🤖 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 `@src/core/sse.ts`:
- Around line 200-211: The current exhaustCleanups registers listeners on the
shared `#emitter` which causes cross-connection firing and misses synchronous
closes; change it to use a per-connection close signal instead of `#emitter`:
create a per-connection promise/emitter (or use the existing `#closed` deferred
tied to this client instance) and, inside exhaustCleanups, if that
per-connection closed is already resolved call super.exhaustCleanups(cleanups)
immediately, otherwise attach a single per-connection listener (or await the
per-connection close promise) to invoke super.exhaustCleanups(cleanups) when
that specific connection closes; remove usage of the shared `#emitter` for
per-connection cleanup wiring to avoid cross-fire and race conditions when
resolvers call client.close() synchronously.
In `@src/mockServiceWorker.js`:
- Around line 137-141: The content-type check in the conditional that guards
sendToClient is too strict and fails to detect SSE headers with parameters
(e.g., "text/event-stream; charset=utf-8"), causing responseClone.body to be
transferred and corrupt SSE streams; update the check used where client,
activeClientIds.has(client.id) and response.headers.get('content-type') are
evaluated so it treats any content type that starts with or has the MIME prefix
"text/event-stream" as SSE (e.g., use a startsWith or parse-and-compare on
response.headers.get('content-type')), and ensure sendToClient is not passed
responseClone.body for those SSE responses to avoid detaching the stream
(references: client, activeClientIds, response.headers.get, responseClone.body,
sendToClient, core/sse.ts).
---
Nitpick comments:
In `@src/core/sse.ts`:
- Around line 311-337: The error() method must be made symmetric to close():
first check this.#closed.state !== 'pending' and return, then call
this.#controller.error() inside a try/catch (swallowing controller exceptions),
always call this.#closed.resolve() after the controller call (even if
controller.error() threw), and finally emit the 'error' event via
this[kClientEmitter]?.emit(new TypedEvent('error')); modify the error()
implementation in the class containing methods error() and close() to mirror
close()’s ordering and try/catch to ensure idempotence and that `#closed` is
always resolved.
In `@test/browser/sse-api/sse.finally.test.ts`:
- Around line 15-127: Add a new regression test that opens two EventSource
connections to the same sse handler and verifies closing the first does not
trigger the second handler's finalize; specifically, in the test use setupWorker
and sse with a finalize(() => window.notifyFinalizedSecond()) for the second
connection (and notifyFinalizedFirst for the first), create two EventSource
instances in the page.evaluate, close the first (or call client.close() in the
handler for the first), and assert that the Promise for the second connection's
finalize has not resolved after the first closes (then finally close the second
and assert its finalize resolves); reference the existing symbols setupWorker,
sse, finalize, client.close and the exposed functions notifyFinalized* to locate
where to add this new test.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c04fab05-4a10-4231-942a-2f89db1e3242
📒 Files selected for processing (4)
src/core/handlers/RequestHandler.tssrc/core/sse.tssrc/mockServiceWorker.jstest/browser/sse-api/sse.finally.test.ts
kettanaito
marked this pull request as draft
May 7, 2026 20:14
Contributor
|
Tested and works as expected: The
|
kettanaito
changed the title
finalize on response stream endfinalize on response stream end
Contributor
|
I'm not sure what's going on, but on msw@2.14.4 (where sse('/ping', ({ client, finalize }) => {
finalize(() => {
console.log('finalize run')
})
}) |
kettanaito
marked this pull request as ready for review
July 7, 2026 17:09
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/sse.ts (1)
171-180: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAvoid registering log listeners on the shared SSE emitter
#attachClientLogger()adds new listeners onthis.#emitterfor every request, so concurrent connections will duplicatemessage/error/closelogs and keep stalepublicUrlclosures alive. Use per-connection listeners or remove them when the stream ends.🤖 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/core/sse.ts` around lines 171 - 180, The shared SSE emitter in `#attachClientLogger`() is accumulating per-request listeners, which causes duplicated message/error/close logs and retains stale publicUrl closures across connections. Update the logging hookup around emitter.on('message'/'error'/'close') to use connection-scoped listeners tied to a specific stream, and ensure they are removed or cleaned up when the SSE connection ends so each request logs independently.
🤖 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 `@src/core/handlers/RequestHandler.ts`:
- Around line 367-386: `getFinalize()` in `RequestHandler` should handle
requests whose signal is already aborted before the first access to `finalize`;
add the same immediate-abort path used by `complete()` so scheduled cleanups run
right away instead of waiting for resolver settlement. Update the lazy
abort-listener setup around `args.request.signal.addEventListener` to first
check `signal.aborted`, then either call `runScheduledCleanups(args.requestId)`
immediately or register the listener as today, and keep `scheduleCleanup`
behavior unchanged. Add a regression test covering an abort that happens before
the resolver first reads `finalize`.
---
Outside diff comments:
In `@src/core/sse.ts`:
- Around line 171-180: The shared SSE emitter in `#attachClientLogger`() is
accumulating per-request listeners, which causes duplicated message/error/close
logs and retains stale publicUrl closures across connections. Update the logging
hookup around emitter.on('message'/'error'/'close') to use connection-scoped
listeners tied to a specific stream, and ensure they are removed or cleaned up
when the SSE connection ends so each request logs independently.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 82a2965e-c6d0-49f0-aa0d-fa9d1e507c7c
📒 Files selected for processing (9)
src/core/handlers/HttpHandler.test.tssrc/core/handlers/RequestHandler.tssrc/core/sse.tssrc/core/utils/HttpResponse/decorators.tssrc/core/utils/internal/observe-response-body-stream.test.tssrc/core/utils/internal/observe-response-body-stream.tssrc/mockServiceWorker.jstest/browser/sse-api/sse.finalize.test.tstest/node/msw-api/finalize.test.ts
✅ Files skipped from review due to trivial changes (1)
- test/node/msw-api/finalize.test.ts
Member
Author
Released: v2.15.0 🎉This has been released in v2.15.0. Get these changes by running the following command: Predictable release automation by Release. |
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
2 participants
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.
finalizeAPI for handler cleanup #2738onCleanupcallback to schedule side effect cleanup when resolvers are finished #2630Todos
RequestHandlerlevel, if possible, to supportfinalize()for user-providedReadableStreamin mocked responses. Otherwise, it will be called immediately when the response is returned and not when the stream is closed.finalize()support is costly (e.g. requires stream piping), apply it conditionally only when the user referenced thefinalizekey from the response resolver argument ala Playwright.