fix(core): clear the queue task timeout timer on every attempt - #1395
fix(core): clear the queue task timeout timer on every attempt#1395LHMQ878 wants to merge 2 commits into
Conversation
A `BackgroundQueue` attempt clears its timeout timer however the attempt ends. An uncleared timer keeps the Node event loop alive for its full duration, which blocks process exit.
🦋 Changeset detectedLatest commit: 17a3fa1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthrough
ChangesBackgroundQueue timeout cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/utils/queue/queue.spec.ts`:
- Around line 311-334: Update the “should still time out a task that never
settles” test to use defaultRetries: 1 and track operation invocations with a
counter. After advancing timers by 1,500 ms, assert that the operation was
called twice, while preserving the never-settling behavior and existing timer
cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: efb64d1e-351c-4a76-9142-eb08e6535c4a
📒 Files selected for processing (3)
.changeset/lucky-timers-rest.mdpackages/core/src/utils/queue/queue.spec.tspackages/core/src/utils/queue/queue.ts
|
The So that there's a basis to approve the gated run, I ran the
The 5 failures are pre-existing and Windows-onlyAll five are in Identical list, identical error, so they are not caused by this PR. They also won't appear on Linux CI. Two notes on method, in case they're usefulBiome initially reported 2 The test job needs The regression test is mutation-checked, as noted earlier in this thread — removing the Happy to rebase if |
PR Checklist
Bugs / Features
What is the current behavior?
BackgroundQueue.executeTaskclears an attempt's timeout timer only afterPromise.raceresolves, so a rejected attempt jumps to thecatchblock and leaves the timer pending. The timer keeps the Node event loop alive for its full duration, blocking process exit, and with retries one timer is left per attempt.MemoryManagerbuilds its queue withdefaultTimeout: 30000anddefaultRetries: 5, so a memory operation that keeps failing leaves six 30-second timers pending.What is the new behavior?
The
Promise.raceis wrapped in atry/finallythat clears the timer, so the timer is released whether the attempt resolves, rejects, or times out — and before the retry backoff rather than after it. The timeout itself is unchanged: a task that never settles is still rejected attask.timeout.Measured with the
MemoryManageroptions and a task that throws, using the reproduction from #1394:defaultRetries: 0, task throwsdefaultRetries: 5, task throwsdefaultRetries: 0, task succeedsThe 813ms is the sum of the retry backoffs (50 + 100 + 150 + 200 + 250ms), which the fix does not change.
fixes #1394
Notes for reviewers
Four tests were added to
queue.spec.tsunderTimeout timers, assertingvi.getTimerCount() === 0after a task fails, after every attempt of a retried task fails, and after a task succeeds, plus one that a task which never settles is still timed out.Two of the four fail on
mainat9aedd49—expected 1 to be +0andexpected 3 to be +0. The other two pass before and after and are there as guards.npx vitest run src/utils/queue/queue.spec.ts→ 13 passed.Full
@voltagent/coresuite before and after: the set of failing tests is byte-identical (79 lines), 12 failed both ways, and passed goes 353 → 357, matching the 4 added tests. The pre-existing failures are allCannot find package '@voltagent/internal/utils'in my checkout (the workspace package is not built) plus onespawn C:Program ENOENTinworkspace/sandbox/local.spec.tsfrom a space in a Windows path; none of them touch the queue.One note on
biome check: it reports formatting diffs for these files, but it reports the same for files I did not touch (e.g.packages/core/src/utils/id.ts) because my checkout hascore.autocrlf=true. The committed diff contains zero CR bytes.Summary by cubic
Fixes a timer leak in BackgroundQueue by clearing each attempt’s timeout in a finally block around Promise.race. Prevents pending timers from keeping the Node event loop alive and blocking process exit, especially with retries (fixes #1394).
MemoryManagerdefaults (30s timeout, 5 retries), failing operations no longer leave multiple 30s timers that delay exit.Written for commit 17a3fa1. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests