cacheable-request - fix: keyv error listener leak on every request#1687
Conversation
Keyv v5's event manager wraps once() listeners in an anonymous function, so the existing removeListener(error, errorHandler) cleanup never matched the registered wrapper and one error listener leaked per request, hitting MaxListenersExceededWarning after 100 requests. Register the handler with on() so the cleanup can remove the exact same reference, and also remove it on the underlying request's error/abort/ close events so aborted or destroyed requests that never emit response do not leak either. Fixes #1685 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011tF3VefnzwcH6TQo48uJHQ
There was a problem hiding this comment.
Code Review
This pull request addresses a memory leak where Keyv error listeners were not being properly unregistered upon request completion or abortion. It introduces a unified cleanup handler registered across multiple termination events and adds corresponding unit tests. The reviewer identified a potential issue where registering once listeners on the underlying request object without cleaning them up when the request succeeds could still lead to dangling listeners and a memory leak, and provided a code suggestion to clean up those listeners on any termination path.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1687 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 29 29
Lines 3504 3513 +9
Branches 794 809 +15
=========================================
+ Hits 3504 3513 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…up runs Track the in-flight request and remove its error/abort/close cleanup listeners inside removeErrorHandler, so the closure over the emitter is released deterministically once the request settles instead of waiting for the request object to be garbage collected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011tF3VefnzwcH6TQo48uJHQ
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix — fixes #1685.
cacheable-requestregistered its per-request Keyv error handler withcachek.once("error", errorHandler)and later tried to clean it up withcachek.removeListener("error", errorHandler). Keyv v5's custom event manager wrapsonce()listeners in an anonymous function andremoveListener()matches by exact reference, so the wrapper was never removed — oneerrorlistener leaked per request, triggeringMaxListenersExceededWarning: Possible event memory leak detected. 101 error listeners added.after 100 requests. This is a regression of the original leak fix (jaredwray/cacheable-request#222 / #223) after the Keyv 5 migration.Changes in
packages/cacheable-request/src/index.ts:cachek.on("error", errorHandler)so the sharedremoveListenercleanup removes the exact same reference. Effective once-semantics are preserved: forwarding a Keyv error emitserroron the request emitter, which itself removes the Keyv listener.error,abort, andcloseevents, so aborted or destroyed requests that never emitresponse/erroron the emitter no longer leak the listener.Tests (
packages/cacheable-request/test/cacheable-request-instance.test.ts):errorlistener count returns to baseline after sequential requests across both the network and cached-response paths (fails with 6 leaked listeners before the fix).MaxListenersExceededWarning.pnpm testfor the package passes (67 tests; statements/lines remain at 100%, branch coverage improves 93.97% → 96.38%). The only failure in this environment is the pre-existingreturn with url and porttest, which requires outbound network access tomockhttp.organd fails identically on unmodifiedmainhere.🤖 Generated with Claude Code
https://claude.ai/code/session_011tF3VefnzwcH6TQo48uJHQ
Generated by Claude Code