fix(async-queuer): keep pendingTick true during wait period#189
fix(async-queuer): keep pendingTick true during wait period#189caluhm wants to merge 2 commits intoTanStack:mainfrom
Conversation
When addItem() is called on a running queue during the wait period, it checks isRunning && !pendingTick to decide whether to trigger #tick(). Previously, pendingTick was set to false synchronously at the end of #tick(), even when async work (execute + wait timer) was still pending. This caused addItem() to trigger immediate processing that bypassed the configured wait. This fix tracks whether async work was scheduled in the while loop and only clears pendingTick when no async work is pending. Closes TanStack#188
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughAsyncQueuer's tick logic now tracks whether async work was scheduled during a tick and only clears the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/pacer/src/async-queuer.ts (1)
448-465:⚠️ Potential issue | 🔴 CriticalAdd error recovery to async work scheduling in
#tickWhen
await this.execute()throws (e.g.,throwOnError: true), the async branch exits before scheduling the next#tick()or wait timeout. SincescheduledAsyncWorkis alreadytrue,pendingTickis never cleared at line 464, leaving it stucktrueand preventingaddItem()from retriggering processing (line 542 check:isRunning && !pendingTickstays false). This can permanently freeze the queue when errors occur duringstart().🛠️ Proposed fix
scheduledAsyncWork = true ;(async () => { - await this.execute() - - const wait = this.#getWait() - if (wait > 0) { - const timeoutId = setTimeout(() => this.#tick(), wait) - this.#timeoutIds.add(timeoutId) - return - } - - this.#tick() + try { + await this.execute() + } finally { + const wait = this.#getWait() + if (wait > 0) { + const timeoutId = setTimeout(() => this.#tick(), wait) + this.#timeoutIds.add(timeoutId) + return + } + + this.#tick() + } })() }🤖 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 448 - 465, The async IIFE in `#tick` can exit early when await this.execute() throws, leaving the local scheduledAsyncWork flag true and pendingTick stuck; wrap the call to this.execute() in try/catch (or use try/finally) so that errors do not skip the scheduling logic: on both success and error compute wait via this.#getWait(), schedule the next tick (setTimeout + this.#timeoutIds.add or call this.#tick()), and ensure you set scheduledAsyncWork = false before the surrounding if (!scheduledAsyncWork) { this.#setState({ pendingTick: false }) } runs; reference the async IIFE, scheduledAsyncWork, this.execute(), this.#getWait(), this.#timeoutIds, this.#tick(), and this.#setState() when making the fix.
🤖 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 `@packages/pacer/src/async-queuer.ts`:
- Around line 448-465: The async IIFE in `#tick` can exit early when await
this.execute() throws, leaving the local scheduledAsyncWork flag true and
pendingTick stuck; wrap the call to this.execute() in try/catch (or use
try/finally) so that errors do not skip the scheduling logic: on both success
and error compute wait via this.#getWait(), schedule the next tick (setTimeout +
this.#timeoutIds.add or call this.#tick()), and ensure you set
scheduledAsyncWork = false before the surrounding if (!scheduledAsyncWork) {
this.#setState({ pendingTick: false }) } runs; reference the async IIFE,
scheduledAsyncWork, this.execute(), this.#getWait(), this.#timeoutIds,
this.#tick(), and this.#setState() when making the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c61369d7-3b97-4b0b-8b87-b8be3aa6476d
📒 Files selected for processing (3)
.changeset/fix-async-queuer-pending-tick.mdpackages/pacer/src/async-queuer.tspackages/pacer/tests/async-queuer.test.ts
|
View your CI Pipeline Execution ↗ for commit d1a8fe1
☁️ Nx Cloud last updated this comment at |
1 participant
🎯 Changes
Fixes #188
When
addItem()is called on a runningAsyncQueuerduring the wait period between item processing, it bypasses the configuredwaitdelay and triggers immediate processing.Root cause: In
#tick(),pendingTickis set tofalsesynchronously at the end of the method, even when async work (execute + wait timer) has been scheduled. SinceaddItem()checksisRunning && !pendingTickto decide whether to trigger#tick(), it incorrectly starts immediate processing during the wait period.Fix: Track whether async work was scheduled in the
whileloop with ascheduledAsyncWorkflag. Only clearpendingTickwhen no async work is pending. When async work is scheduled,pendingTickstaystrueuntil the next#tick()call (afterexecute()completes and the wait timer fires), preventingaddItem()from triggering a premature tick.Test added: Verifies that items added via
addItem()during the wait period are not processed until the wait completes.✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit