fix: prevent frame abort listener leak#2737
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)
📝 WalkthroughWalkthroughRemoves per-frame listeners after emitting network response events to allow GC; adds a Vitest memory test suite + config and CI steps to run it; and bumps the ChangesMemory Leak Fix & Regression Testing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/experimental/sources/interceptor-source.ts (1)
89-108:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnsure frame listener cleanup runs even if response emission throws.
removeAllListeners()currently depends onemit(...)completing successfully. If a response listener throws, cleanup is skipped and the per-frame emitter can remain retained.💡 Suggested fix
queueMicrotask(() => { - httpFrame.events.emit( - new ResponseEvent( - isMockedResponse ? 'response:mocked' : 'response:bypass', - { - requestId, - request, - response, - }, - ), - ) - - /** - * `@note` Remove any listeners from this frame. - * Past this point, it won't emit anything. The removal is crucial - * to prevent "rettime" from keeping the abort cleanup listeners internally. - * `@see` https://github.com/mswjs/msw/issues/2735 - */ - httpFrame.events.removeAllListeners() + try { + httpFrame.events.emit( + new ResponseEvent( + isMockedResponse ? 'response:mocked' : 'response:bypass', + { + requestId, + request, + response, + }, + ), + ) + } finally { + /** + * `@note` Remove any listeners from this frame. + * Past this point, it won't emit anything. The removal is crucial + * to prevent "rettime" from keeping the abort cleanup listeners internally. + * `@see` https://github.com/mswjs/msw/issues/2735 + */ + httpFrame.events.removeAllListeners() + } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/experimental/sources/interceptor-source.ts` around lines 89 - 108, The call to httpFrame.events.removeAllListeners() is skipped if httpFrame.events.emit(...) throws; wrap the emit in a try/finally inside the existing queueMicrotask callback so removeAllListeners() always runs (use try { httpFrame.events.emit(new ResponseEvent(...)) } finally { httpFrame.events.removeAllListeners() }) referencing queueMicrotask, httpFrame.events.emit, ResponseEvent and httpFrame.events.removeAllListeners to locate the change.
🧹 Nitpick comments (1)
test/memory/2735-memory-leak.test.ts (1)
50-54: ⚡ Quick winFail fast when
global.gcis unavailable to avoid silent false signals.
global.gc?.()can no-op if the test is run without--expose-gc, weakening the regression check.💡 Suggested fix
async function forceGc(): Promise<void> { + if (typeof global.gc !== 'function') { + throw new Error('Memory tests require Node to run with --expose-gc') + } for (let i = 0; i < 6; i++) { - global.gc?.() + global.gc() await delay(30) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/memory/2735-memory-leak.test.ts` around lines 50 - 54, The forceGc function currently calls global.gc?.() which silently no-ops when Node is run without --expose-gc; update forceGc to fail fast by checking for global.gc at the start (e.g., if (typeof global.gc !== 'function') throw new Error(...)) and document/mention that tests must be run with --expose-gc; keep the existing loop and delay logic (function name forceGc, usage of global.gc and delay) but bail immediately with a clear error if global.gc is unavailable so the test doesn't produce silent false signals.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/core/experimental/sources/interceptor-source.ts`:
- Around line 89-108: The call to httpFrame.events.removeAllListeners() is
skipped if httpFrame.events.emit(...) throws; wrap the emit in a try/finally
inside the existing queueMicrotask callback so removeAllListeners() always runs
(use try { httpFrame.events.emit(new ResponseEvent(...)) } finally {
httpFrame.events.removeAllListeners() }) referencing queueMicrotask,
httpFrame.events.emit, ResponseEvent and httpFrame.events.removeAllListeners to
locate the change.
---
Nitpick comments:
In `@test/memory/2735-memory-leak.test.ts`:
- Around line 50-54: The forceGc function currently calls global.gc?.() which
silently no-ops when Node is run without --expose-gc; update forceGc to fail
fast by checking for global.gc at the start (e.g., if (typeof global.gc !==
'function') throw new Error(...)) and document/mention that tests must be run
with --expose-gc; keep the existing loop and delay logic (function name forceGc,
usage of global.gc and delay) but bail immediately with a clear error if
global.gc is unavailable so the test doesn't produce silent false signals.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 10a10f70-8747-476b-92e6-b01af4f63227
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
.github/workflows/ci.ymlpackage.jsonsrc/core/experimental/sources/interceptor-source.tstest/memory/2735-memory-leak.test.tstest/memory/vitest.config.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/browser/sources/service-worker-source.ts (2)
382-394: 💤 Low value
removeAllListeners()placement is correct; consider documenting the intentional timing difference vs.interceptor-source.ts.
interceptor-source.tswraps bothemit()andremoveAllListeners()insidequeueMicrotask()and has a test that asserts cleanup occurs "after the microtask flush." Here the operations run synchronously in the same turn of#handleResponse. Both approaches achieve the GC goal (rettime'sonAbortclosures are detached), but the observable emission timing differs.If the synchronous path is intentional for the SW source (e.g., because the call is already in a message-event callback rather than in the middle of the interceptor's async chain), a brief inline comment cross-referencing the
queueMicrotaskrationale ininterceptor-source.tswould help future maintainers.💡 Suggested comment to document the intentional divergence
frame.events.emit( new ResponseEvent( isMockedResponse ? 'response:mocked' : 'response:bypass', { requestId: frame.data.id, request: fetchRequest, response: fetchResponse, isMockedResponse, }, ), ) + /** + * `@note` Remove any listeners from this frame. Unlike the interceptor source, + * no queueMicrotask deferral is needed here because `#handleResponse` is + * already invoked in a message-event callback and all frame.events listeners + * are registered synchronously before this point. + * Removal is critical to prevent "rettime" from retaining abort cleanup + * listeners on the shared AbortSignal. + * `@see` https://github.com/mswjs/msw/issues/2735 + */ frame.events.removeAllListeners()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/browser/sources/service-worker-source.ts` around lines 382 - 394, Add a brief inline comment above the frame.events.removeAllListeners() call in `#handleResponse` explaining that the synchronous emit() + removeAllListeners() is intentional for the service-worker-source (it runs inside the message-event callback), and explicitly cross-reference interceptor-source.ts where emit/removeAllListeners() are deferred with queueMicrotask() and a test asserting "after the microtask flush"; mention that both approaches detach rettime onAbort closures but differ only in observable emission timing (response:mocked/response:bypass via ResponseEvent).
341-344: ⚡ Quick winOpaque-response path leaves the frame emitter's listeners uncleared.
When a response is opaque, the frame is removed from
this.#framesbutframe.events.removeAllListeners()is never called. The rettimeonAbortclosure registered onlistenersController.signal(viaframe.events.on('*', ..., { signal })indefine-network.ts) still holds a reference to that frame's emitter, so the frame is retained untildisable()— the same leak vector the main fix targets.This is acknowledged as out of scope in the PR, but the fix is a straightforward two-line change:
♻️ Proposed fix for the opaque-response path
- if (response.type?.includes('opaque')) { - this.#frames.delete(request.id) - return - } + if (response.type?.includes('opaque')) { + const frame = this.#frames.get(request.id) + this.#frames.delete(request.id) + frame?.events.removeAllListeners() + return + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/browser/sources/service-worker-source.ts` around lines 341 - 344, In the opaque-response branch in service-worker-source (the block checking response.type?.includes('opaque')), grab the stored frame for request.id and call frame.events.removeAllListeners() before deleting it from this.#frames; i.e., retrieve the frame from this.#frames (using the same request.id key), invoke frame.events.removeAllListeners() to clear the rettime listeners, then call this.#frames.delete(request.id) and return. This ensures the onAbort closure registered with listenersController.signal no longer retains the frame.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/browser/sources/service-worker-source.ts`:
- Around line 382-394: Add a brief inline comment above the
frame.events.removeAllListeners() call in `#handleResponse` explaining that the
synchronous emit() + removeAllListeners() is intentional for the
service-worker-source (it runs inside the message-event callback), and
explicitly cross-reference interceptor-source.ts where emit/removeAllListeners()
are deferred with queueMicrotask() and a test asserting "after the microtask
flush"; mention that both approaches detach rettime onAbort closures but differ
only in observable emission timing (response:mocked/response:bypass via
ResponseEvent).
- Around line 341-344: In the opaque-response branch in service-worker-source
(the block checking response.type?.includes('opaque')), grab the stored frame
for request.id and call frame.events.removeAllListeners() before deleting it
from this.#frames; i.e., retrieve the frame from this.#frames (using the same
request.id key), invoke frame.events.removeAllListeners() to clear the rettime
listeners, then call this.#frames.delete(request.id) and return. This ensures
the onAbort closure registered with listenersController.signal no longer retains
the frame.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c4a174ba-80c6-4f4e-b8d9-6e022637869a
📒 Files selected for processing (1)
src/browser/sources/service-worker-source.ts
commit: |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/browser/sources/service-worker-source.ts (1)
341-344:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSecondary listener leak for opaque responses is not addressed here.
When the opaque response guard fires, the frame has already had listeners attached (via the
'frame'event indefine-network.tstriggered bythis.queue(frame)in#handleRequest). Returning early without callingframe.events.removeAllListeners()leaves those listeners — and rettime'sonAbortclosures on the sharedlistenersController.signal— alive for the network's lifetime. The PR description notes this as out of scope, but it's the same root cause as the leak being fixed on the happy path.🛡️ Proposed one-liner to cover this path
if (response.type?.includes('opaque')) { - this.#frames.delete(request.id) - return + const opaqueFrame = this.#frames.get(request.id) + this.#frames.delete(request.id) + opaqueFrame?.events.removeAllListeners() + return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/browser/sources/service-worker-source.ts` around lines 341 - 344, When the opaque-response guard in `#handleRequest` returns early after calling this.#frames.delete(request.id), it doesn't remove per-frame listeners added earlier, leaking the frame's event handlers and onAbort closures; update the opaque branch to also clean up the frame's listeners by calling frame.events.removeAllListeners() (and abort or remove any listenersController.signal hooks) before returning so the listeners attached via this.queue(frame) / define-network.ts 'frame' event are released.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/browser/sources/service-worker-source.ts`:
- Around line 341-344: When the opaque-response guard in `#handleRequest` returns
early after calling this.#frames.delete(request.id), it doesn't remove per-frame
listeners added earlier, leaking the frame's event handlers and onAbort
closures; update the opaque branch to also clean up the frame's listeners by
calling frame.events.removeAllListeners() (and abort or remove any
listenersController.signal hooks) before returning so the listeners attached via
this.queue(frame) / define-network.ts 'frame' event are released.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 202c05cf-dc9d-40bc-8e5f-51ef942c9c81
📒 Files selected for processing (2)
src/browser/sources/service-worker-source.tssrc/core/experimental/sources/interceptor-source.ts
Released: v2.14.3 🎉This has been released in v2.14.3. Get these changes by running the following command: Predictable release automation by Release. |
1 participant
Emitterretained for the network's lifetime via sharedAbortSignal(interceptor-source) #2735@mswjs/interceptorsv0.41.7: HTTPParser instances accumulate forever in long-running Node servers interceptors#779 (likely outdatedrettimethere)