feat(core): clear up integrations on dispose#20407
Conversation
size-limit report 📦
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a589811. Configure here.
| 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.
| cleanups.forEach(cleanup => { | ||
| expect(cleanup).toHaveBeenCalledTimes(1); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
No integration or E2E test for feature
Low Severity
This feat PR only includes unit tests. There is no integration or E2E test verifying the end-to-end behavior — e.g., that setting up consoleIntegration on a ServerRuntimeClient, then calling dispose(), actually removes the handler from the global handlers array. An integration test exercising this full flow (especially in a Cloudflare-like scenario with multiple client lifecycles) would help catch regressions in the memory-leak fix.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit a589811. Configure here.
1 participant
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.