Skip to content

fix(core): clear the queue task timeout timer on every attempt - #1395

Open
LHMQ878 wants to merge 2 commits into
VoltAgent:mainfrom
LHMQ878:fix/queue-timeout-timer-leak
Open

fix(core): clear the queue task timeout timer on every attempt#1395
LHMQ878 wants to merge 2 commits into
VoltAgent:mainfrom
LHMQ878:fix/queue-timeout-timer-leak

Conversation

@LHMQ878

@LHMQ878 LHMQ878 commented Aug 3, 2026

Copy link
Copy Markdown

PR Checklist

Bugs / Features

  • Related issue(s) linked
  • Tests for the changes have been added
  • Docs have been added / updated — no public API change
  • Changesets have been added

What is the current behavior?

BackgroundQueue.executeTask clears an attempt's timeout timer only after Promise.race resolves, so a rejected attempt jumps to the catch block 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.

MemoryManager builds its queue with defaultTimeout: 30000 and defaultRetries: 5, so a memory operation that keeps failing leaves six 30-second timers pending.

What is the new behavior?

The Promise.race is wrapped in a try/finally that 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 at task.timeout.

Measured with the MemoryManager options and a task that throws, using the reproduction from #1394:

before after
defaultRetries: 0, task throws exits after 30017ms exits after 1ms
defaultRetries: 5, task throws exits after 30807ms exits after 813ms
defaultRetries: 0, task succeeds exits after 14ms exits after 14ms

The 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.ts under Timeout timers, asserting vi.getTimerCount() === 0 after 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 main at 9aedd49expected 1 to be +0 and expected 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/core suite 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 all Cannot find package '@voltagent/internal/utils' in my checkout (the workspace package is not built) plus one spawn C:Program ENOENT in workspace/sandbox/local.spec.ts from 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 has core.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).

  • Bug Fixes
    • Clear the attempt’s timeout whether it succeeds, fails, or times out, and do it before any retry backoff.
    • Keep timeout behavior unchanged: tasks that never settle still reject at the configured timeout and start the next attempt when retries are enabled.
    • With MemoryManager defaults (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.

Review in cubic

Summary by CodeRabbit

  • Bug Fixes

    • Improved background task timeout cleanup after failed attempts and retries.
    • Prevented completed or failed tasks from leaving pending timers that could keep short-lived processes running.
    • Ensured timeout handling remains reliable for successful, failed, retried, and never-completing tasks.
  • Tests

    • Added coverage for timeout cleanup across task execution outcomes.

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-bot

changeset-bot Bot commented Aug 3, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 17a3fa1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@voltagent/core Patch

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

@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

BackgroundQueue now clears timeout timers regardless of task outcome. New fake-timer tests cover failures, retries, success, and never-settling tasks. A patch changeset documents the fix.

Changes

BackgroundQueue timeout cleanup

Layer / File(s) Summary
Clear attempt timers on every outcome
packages/core/src/utils/queue/queue.ts
executeTask clears each attempt timeout in a finally block.
Validate timer cleanup behavior
packages/core/src/utils/queue/queue.spec.ts, .changeset/lucky-timers-rest.md
Tests cover failed attempts, retries, successful completion, and never-settling tasks. The changeset records the patch release.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The implementation and tests satisfy issue #1394 by clearing timeout timers on success, failure, and timeout without changing timeout behavior.
Out of Scope Changes check ✅ Passed The changeset and queue tests directly support the timer cleanup fix, and no unrelated code changes are identified.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check ✅ Passed The title clearly and concisely describes the primary change: clearing queue task timeout timers on every attempt.
Description check ✅ Passed The description follows the template and explains the bug, fix, tests, issue, changeset, and validation results.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 9aedd49 and 7c736ea.

📒 Files selected for processing (3)
  • .changeset/lucky-timers-rest.md
  • packages/core/src/utils/queue/queue.spec.ts
  • packages/core/src/utils/queue/queue.ts

Comment thread packages/core/src/utils/queue/queue.spec.ts
@LHMQ878

LHMQ878 commented Aug 3, 2026

Copy link
Copy Markdown
Author

The unstable state on this PR isn't a failure of the change — the workflow-run list shows PR Checks sitting at action_required twice for fix/queue-timeout-timer-leak, i.e. the fork-CI approval gate. The only checks that could run did pass (cubic · AI code reviewer success, CodeRabbit "Review completed").

So that there's a basis to approve the gated run, I ran the pull-request.yml steps locally at 17a3fa11. Environment: Windows 11, Node 22, pnpm 8.10.5, NX_DAEMON=false (the bundled nx 20.8.2 daemon rejects REQUEST_FILE_DATA on this box — unrelated to the change).

Step Command Result
Lint with Biome biome check on both changed files ✅ clean
Syncpack pnpm sp lint 815 already valid, 248 semver valid, 0 mismatches
Build pnpm build:all ✅ 29/29 projects
Publint pnpm publint:all ✅ 14/14 projects (suggestions only, all pre-existing, none in files I touched)
Test Package lerna run test --scope @voltagent/core 1396 passed, 5 failed — all 5 pre-existing, see below

queue.spec.ts on its own: 13/13 passing.

The 5 failures are pre-existing and Windows-only

All five are in src/workspace/sandbox/, nowhere near the queue, and every one fails with spawn C:Program ENOENT — an unquoted C:\Program Files\... path being split at the space. I checked out the merge base (9aedd49) and ran the same two spec files there:

base 9aedd49 : Test Files 2 failed | Tests 5 failed | 7 passed
head 17a3fa11: Test Files 2 failed | Tests 5 failed | 7 passed

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 useful

Biome initially reported 2 format errors on my two files, and they were an artifact of my checkout, not the PR. This clone has core.autocrlf=true, so git ls-files --eol reports i/lf w/crlf — the worktree copies have CRLF while the blobs in the index are LF. I extracted the committed blobs with git show HEAD:<path> into a temp dir and re-ran Biome against those under the repo config: clean. Worth knowing if another Windows contributor reports phantom format errors.

The test job needs build:all first. Running lerna run test --scope @voltagent/core on a fresh checkout gives 69 failed test files with Cannot find module '@voltagent/internal/types', because that dependency's dist doesn't exist yet. Once built, it's 108/110 files passing. CI gets this for free from job ordering; locally it looks alarming for a moment.

The regression test is mutation-checked, as noted earlier in this thread — removing the Promise.race against timeoutPromise turns it into expected 1 to be 2, whereas the pre-fix assertion still passed.

Happy to rebase if main has moved. Nothing here needs action from me — it just needs the CI run approved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BackgroundQueue leaves a pending timeout timer for every failed attempt, blocking process exit

1 participant