feat(core): clear up integrations on dispose#20407
Conversation
size-limit report 📦
|
| typeHandlers.splice(index, 1); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
Array splice during for...of iteration may skip handlers
Low Severity
The new unsubscribe function uses splice to remove handlers from the live handlers[type] array. If an unsubscribe is ever called while triggerHandlers is iterating over the same array with for...of, the in-place mutation can cause the iterator to skip the next handler. JavaScript's ArrayIterator tracks position by index — when splice removes an element at or before the current position, subsequent elements shift down and the next one is silently skipped. While unlikely in current usage (unsubscribe is called from dispose(), not during handler execution), this creates a latent hazard if future code paths ever trigger disposal from within a handler callback.
Additional Locations (1)
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit a589811. Configure here.
There was a problem hiding this comment.
It shouldn't happen that it is called from somewhere else.
| this._outcomes = {}; | ||
| this._hooks = {}; | ||
| this._eventProcessors = []; | ||
| this._disposeCallbacks = []; |
There was a problem hiding this comment.
q: This will only be cleaned up in the server runtime client, should we at least document this here? Might be confusing otherwise
There was a problem hiding this comment.
Good point of adding it there. I'll add it there
16defdb to
295ad22
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
There are 4 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 295ad22. Configure here.
| import: createImport('init', 'browserTracingIntegration', 'replayIntegration'), | ||
| gzip: true, | ||
| limit: '83 KB', | ||
| limit: '84 KB', |
There was a problem hiding this comment.
Bundle size increases in browser packages
Low Severity
Multiple browser package size limits were increased by 0.5–1 KB across several entries (ESM bundles and CDN bundles). This is flagged because the project rules require flagging large bundle size increases in browser packages even when they're unavoidable. The increases stem from adding cleanup infrastructure (registerCleanup, _disposeCallbacks, unsubscribe return values) that primarily benefits server-side runtimes like Cloudflare.
Additional Locations (1)
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 295ad22. Configure here.
| } | ||
| }); | ||
|
|
||
| client.registerCleanup(unsubscribe); |
There was a problem hiding this comment.
Redundant cleanup registration for per-client hooks
Low Severity
The conversationId integration registers the client.on('spanStart') unsubscribe via client.registerCleanup(), but this is unnecessary. Unlike global handlers (used by console instrumentation), client hooks live in this._hooks which ServerRuntimeClient.dispose() already clears unconditionally via this._hooks[hookName]?.clear(). For non-server clients, hooks are GC'd with the client. This adds confusion about when registerCleanup is actually needed vs. when dispose() already handles it.
Reviewed by Cursor Bugbot for commit 295ad22. Configure here.
| triggerHandlers('fetch', { url: 'test3' }); | ||
| expect(fetchHandler).toHaveBeenCalledTimes(2); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing integration or E2E test for feat PR
Low Severity
This feat PR only includes unit tests for addHandler unsubscribe behavior and registerCleanup/dispose(). The project rules require at least one integration or E2E test for feature PRs. An integration test verifying the full lifecycle — client creation, integration setup, handler registration, dispose, and confirmation that global handlers are actually cleaned up — would help catch regressions in the memory-leak fix this PR targets (especially relevant for the Cloudflare per-request client scenario).
Additional Locations (1)
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 295ad22. Configure here.
closes #19573
closes JS-1829
As with Cloudflare we create a new client on every request, that means that every integration that uses an
addHandlerand is used by the Cloudflare SDK is makes the client not disposable - so the garbage collector can't remove it properly.This PR adds a callback for
addHandlerthat basically removes the handler from the global handler array (for now only for integrations, which are used by the Cloudflare SDK). I actually also tried to change the global handler to be aWeakMap, but it still showed some memory leaks with that, so we need to actively remove these callbacks.For now, to not increase the bundle sizes for core too much, it is actually removing the handlers only in the
ServerRuntimeClient, as for browsers it is usually not really an issue.