fix: guard retryer key propagation when parent has no key - #199
fix: guard retryer key propagation when parent has no key#199simonmeyerrr wants to merge 6 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe four asynchronous utilities now pass ChangesRetryer key propagation
Estimated code review effort: 2 (Simple) | ~15 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/pacer/tests/async-debouncer.test.ts (1)
1364-1446: Tests look good — minor placement nit.This
retryer key propagationblock is nested inside thedescribe('asyncDebounce helper function', ...)suite, but the tests exercise theAsyncDebouncerclass directly (not theasyncDebouncehelper). Consider moving it into the outerdescribe('AsyncDebouncer', ...)block alongside the other class-level suites for better organization. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pacer/tests/async-debouncer.test.ts` around lines 1364 - 1446, The "retryer key propagation" describe block tests AsyncDebouncer directly but is currently nested under the asyncDebounce helper suite; move that entire describe('retryer key propagation', ...) block out of the asyncDebounce helper describe and into the outer describe('AsyncDebouncer', ...) suite so the tests sit alongside other class-level tests for AsyncDebouncer and clearly reference AsyncDebouncer (and not the asyncDebounce helper).packages/pacer/src/async-queuer.ts (1)
619-622: LGTM — fix is correct.Minor optional consistency observation:
packages/pacer/src/async-batcher.ts(around lines 389–396) constructsAsyncRetryerby passingthis.options.asyncRetryerOptionsdirectly, without the${this.key}-retryer-${...}namespacing used here. It does not have the memory-leak bug (no truthy key is ever propagated), but a keyedAsyncBatcherwon't produce namespaced retryer keys for devtools either. Consider applying the same conditional pattern there for consistency — can be deferred.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pacer/src/async-queuer.ts` around lines 619 - 622, In AsyncBatcher (the class/method that constructs new AsyncRetryer in packages/pacer/src/async-batcher.ts), apply the same conditional key namespacing used in async-queuer.ts: when creating the AsyncRetryer with this.options.asyncRetryerOptions, set the key option to this.key ? `${this.key}-retryer-${currentExecuteCount}` : undefined (or equivalent) instead of passing the options directly so keyed AsyncBatcher produces consistent namespaced retryer keys for devtools; update the AsyncRetryer constructor call within AsyncBatcher to merge options and conditionally add the namespaced key.packages/pacer/tests/async-rate-limiter.test.ts (2)
825-841: Consider also asserting no listener registration / leak.Filtering
emitcalls by event name'AsyncRetryer'is sufficient to prove the immediate leak path is gone, but the underlying issue#198was that retryers were also registered as devtools instances and listeners. If you want to harden this against future regressions, consider additionally asserting thatpacerEventClient.on/registerPacerDevtoolsInstanceis not invoked for the child retryer when the parent has no key. Optional — current assertion already catches the memory-growth scenario.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pacer/tests/async-rate-limiter.test.ts` around lines 825 - 841, Extend the test for AsyncRateLimiter.maybeExecute to also assert that no devtools listener/instance registration occurs when the rate limiter has no key: spy on pacerEventClient.on (and/or the helper registerPacerDevtoolsInstance) before calling rateLimiter.maybeExecute and assert those spies were not called, in addition to the existing emit filter; this ensures no listener registration/leak for child retryers leaking via registerPacerDevtoolsInstance or pacerEventClient.on.
780-860: Suite is nested under the wrong parentdescribe.The new
describe('retryer key propagation', ...)block is placed insidedescribe('asyncRateLimit', ...)(the factory-function suite, which starts at line 694), but every test exercises theAsyncRateLimiterclass directly. It should live underdescribe('AsyncRateLimiter', ...)(which closes at line 692) to keep suite labels accurate in test output and to inherit the same fake-timer setup as the other class tests.♻️ Proposed relocation
Move the block so it sits alongside the other
AsyncRateLimitersub-suites (e.g., just before the closing})of theAsyncRateLimiterdescribe at line 692), rather than inside theasyncRateLimitdescribe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pacer/tests/async-rate-limiter.test.ts` around lines 780 - 860, The new describe block describe('retryer key propagation', ...) is nested inside the asyncRateLimit suite but tests use the AsyncRateLimiter class; move the entire describe('retryer key propagation', ...) block out of the asyncRateLimit describe and place it as a sibling inside the AsyncRateLimiter describe so it shares the same fake-timer setup and context; locate the block by the exact describe title and relocate it to sit alongside the other AsyncRateLimiter sub-suites (e.g., just before the closing brace of the AsyncRateLimiter describe) without changing the test bodies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/pacer/src/async-queuer.ts`:
- Around line 619-622: In AsyncBatcher (the class/method that constructs new
AsyncRetryer in packages/pacer/src/async-batcher.ts), apply the same conditional
key namespacing used in async-queuer.ts: when creating the AsyncRetryer with
this.options.asyncRetryerOptions, set the key option to this.key ?
`${this.key}-retryer-${currentExecuteCount}` : undefined (or equivalent) instead
of passing the options directly so keyed AsyncBatcher produces consistent
namespaced retryer keys for devtools; update the AsyncRetryer constructor call
within AsyncBatcher to merge options and conditionally add the namespaced key.
In `@packages/pacer/tests/async-debouncer.test.ts`:
- Around line 1364-1446: The "retryer key propagation" describe block tests
AsyncDebouncer directly but is currently nested under the asyncDebounce helper
suite; move that entire describe('retryer key propagation', ...) block out of
the asyncDebounce helper describe and into the outer describe('AsyncDebouncer',
...) suite so the tests sit alongside other class-level tests for AsyncDebouncer
and clearly reference AsyncDebouncer (and not the asyncDebounce helper).
In `@packages/pacer/tests/async-rate-limiter.test.ts`:
- Around line 825-841: Extend the test for AsyncRateLimiter.maybeExecute to also
assert that no devtools listener/instance registration occurs when the rate
limiter has no key: spy on pacerEventClient.on (and/or the helper
registerPacerDevtoolsInstance) before calling rateLimiter.maybeExecute and
assert those spies were not called, in addition to the existing emit filter;
this ensures no listener registration/leak for child retryers leaking via
registerPacerDevtoolsInstance or pacerEventClient.on.
- Around line 780-860: The new describe block describe('retryer key
propagation', ...) is nested inside the asyncRateLimit suite but tests use the
AsyncRateLimiter class; move the entire describe('retryer key propagation', ...)
block out of the asyncRateLimit describe and place it as a sibling inside the
AsyncRateLimiter describe so it shares the same fake-timer setup and context;
locate the block by the exact describe title and relocate it to sit alongside
the other AsyncRateLimiter sub-suites (e.g., just before the closing brace of
the AsyncRateLimiter describe) without changing the test bodies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 94ba8c48-6f48-4361-9d9e-aa3072644a0b
📒 Files selected for processing (9)
.changeset/fluffy-spies-guess.mdpackages/pacer/src/async-debouncer.tspackages/pacer/src/async-queuer.tspackages/pacer/src/async-rate-limiter.tspackages/pacer/src/async-throttler.tspackages/pacer/tests/async-debouncer.test.tspackages/pacer/tests/async-queuer.test.tspackages/pacer/tests/async-rate-limiter.test.tspackages/pacer/tests/async-throttler.test.ts
There was a problem hiding this comment.
Pull request overview
This PR fixes a Node.js memory leak caused by unintentionally propagating a truthy "undefined-retryer-N" devtools key into per-execution AsyncRetryer instances when parent async utilities are created without a key.
Changes:
- Guard
AsyncRetryerkey propagation inAsyncQueuer,AsyncThrottler,AsyncRateLimiter, andAsyncDebouncerso children don’t get a truthy key when the parent has no key. - Add tests (4 per class) validating child retryer key behavior and whether
pacerEventClient.emit()is called forAsyncRetryerevents. - Add a changeset for a patch release.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/pacer/src/async-queuer.ts | Guards child retryer key propagation during execute() to avoid "undefined-..." keys. |
| packages/pacer/src/async-throttler.ts | Guards child retryer key propagation during #execute() to avoid "undefined-..." keys. |
| packages/pacer/src/async-rate-limiter.ts | Guards child retryer key propagation during #execute() to avoid "undefined-..." keys. |
| packages/pacer/src/async-debouncer.ts | Guards child retryer key propagation during #execute() to avoid "undefined-..." keys. |
| packages/pacer/tests/async-queuer.test.ts | Adds retryer key propagation + devtools emit tests for AsyncQueuer. |
| packages/pacer/tests/async-throttler.test.ts | Adds retryer key propagation + devtools emit tests for AsyncThrottler. |
| packages/pacer/tests/async-rate-limiter.test.ts | Adds retryer key propagation + devtools emit tests for AsyncRateLimiter. |
| packages/pacer/tests/async-debouncer.test.ts | Adds retryer key propagation + devtools emit tests for AsyncDebouncer. |
| .changeset/fluffy-spies-guess.md | Patch changeset describing the leak fix and affected classes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Moved the retryer key propagation suites under the |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/pacer/tests/async-throttler.test.ts:8
vi.useFakeTimers()is enabled inbeforeEach, but this suite never switches back to real timers. That can leak fake-timer state into other test files and cause unrelated tests to behave incorrectly depending on execution order. Add a top-levelafterEach(() => vi.useRealTimers())for cleanup (similar to other pacer test files).
describe('AsyncThrottler', () => {
beforeEach(() => {
vi.useFakeTimers()
})
packages/pacer/src/async-debouncer.ts:372
AsyncDebouncer#getAbortSignal()looks up the active retryer usingthis.store.state.maybeExecuteCount, but#execute()stores the retryer undercurrentMaybeExecuteCount = this.store.state.maybeExecuteCount + 1. This off-by-one meansgetAbortSignal()will returnnullduring an in-flight execution, and the generated retryerkeysuffix is also shifted. Consider aligning the retryer map key/count withmaybeExecuteCount(and update the related tests accordingly).
this.#setState({ isExecuting: true })
const currentAsyncRetryer = new AsyncRetryer(this.fn, {
...this.options.asyncRetryerOptions,
key: this.key ? `${this.key}-retryer-${currentMaybeExecuteCount}` : undefined,
})
|
gave you co-auther credit in a larger PR that fixed this and even more issues at all once |
🎯 Changes
Fixes #198
When
AsyncQueuer,AsyncThrottler,AsyncRateLimiter, orAsyncDebouncerare created without akey(the common case), they propagate a truthy key like"undefined-retryer-1"to every childAsyncRetryervia template literal coercion ofundefined. This causespacerEventClient.emit()to be called for every execution, queuing events into a module-level singleton that is never flushed in Node.js — resulting in unbounded heap growth (+583 MB over 100k items with AsyncQueuer).Same one-line fix in all four classes:
Benchmark (1,000 rounds × 100 items, Node.js 22,
--expose-gc):You can checkout to the first commit to test the script before the fix and to the second commit to test the script with the fix:
npx tsx --expose-gc reproduce-leak.ts(I removed the script in the last commit as it was just to demonstrate the issue)16 new tests (4 per class):
AsyncRetryergetskey: undefinedwhen parent has no key (inspected mid-execution via controlled promise)AsyncRetryergets namespaced key when parent has a keypacerEventClient.emit()is never called withAsyncRetryerevents when parent has no keypacerEventClient.emit()is called withAsyncRetryerevents when parent has a key✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit