fix: prevent scheduler deadlock when a ready task's delay expires mid-reschedule

[cldmv-bot]

🚀 What's Changed

💥 Breaking Changes

No breaking changes

✨ Features

No new features

🐛 Bug Fixes

• fix(review): only force imminent recheck when nextTime would stay Infinity (fix: prevent scheduler deadlock when a ready task's delay expires mid-reschedule #8) (471bd50)
• fix: prevent scheduler deadlock when a ready task's delay expires mid-reschedule (fix: prevent scheduler deadlock when a ready task's delay expires mid-reschedule #8) (de48ac6)

📦 Dependencies

No dependency updates

🔧 Other Changes

• test(review): poll for the observable state instead of a fixed 120ms sleep (fix: prevent scheduler deadlock when a ready task's delay expires mid-reschedule #8) (a456202)
• test(review): fail fast + always clean up if drain never fires (fix: prevent scheduler deadlock when a ready task's delay expires mid-reschedule #8) (ff6c70f)
• test(review): restore setTimeout spy in finally, capture calls before restore (fix: prevent scheduler deadlock when a ready task's delay expires mid-reschedule #8) (85afcf0)
• test: add regression test for the scheduler stranded-task deadlock (fix: prevent scheduler deadlock when a ready task's delay expires mid-reschedule #8) (964088d)
• test(ci): isolate flaky bypassDelay completion-delay test into its own file (fix: prevent scheduler deadlock when a ready task's delay expires mid-reschedule #8) (ac2ec87)

👥 Contributors

• @Shinrai

[cldmv-bot]

Auto-normalized PR title: rewrote PR title to match the highest-priority commit type (fix).

• Before: test(ci): isolate flaky bypassDelay completion-delay test into its own file
• After: fix: prevent scheduler deadlock when a ready task's delay expires mid-reschedule

If this isn't what you want, edit the title — the normalizer won't re-fire as long as the title stays conventional.

[Copilot]

Pull request overview

Fixes a scheduler edge case that could strand already-ready tasks by incorrectly scheduling the next tick far in the future, and adjusts the test suite to reduce CI flakiness by isolating a timing-sensitive test.

Changes:

• Update traditional polling scheduler timing so ready-heap work triggers an imminent recheck instead of falling through to an effectively “never” timeout.
• Move the bypassDelay completion-delay timing test into its own vitest file to isolate CI flakiness.
• Remove the migrated timing test from the large combined HoldMyTask test file.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
src/hold-my-task.mjs	Adjusts next-tick scheduling to avoid stranding tasks already in the ready heap.
tests/HoldMyTask.test.vitest.mjs	Removes the bypassDelay completion-delay timing test from the main test suite file.
tests/BypassDelayCompletionTiming.test.vitest.mjs	Adds an isolated timing test file to help diagnose/avoid CI-only hangs.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/hold-my-task.mjs:1633

• The new stranded-ready-task safeguard (forcing an imminent recheck when nextTime === Infinity && readyHeap is non-empty) fixes a subtle scheduler deadlock, but there’s no regression test covering the specific failure mode that motivated this change. Without a targeted test, it’s easy for future refactors of _scheduleNextTick() / schedulerTick() to reintroduce the 24.8‑day timeout behavior.

Consider adding a Vitest regression that reproduces the scenario (traditional polling mode) where a task is present in readyHeap while pendingHeap is empty and nextAvailableTime <= now, and assert that the scheduler continues processing (e.g., queue drains within a short timeout) rather than stalling.

		// already set nextTime above and should keep its single efficient timeout
		// rather than degrade into a tick-interval poll loop until it expires.
		if (nextTime === Infinity && this.readyHeap.size() > 0) {
			nextTime = now;
		}

[Shinrai]

Addressing the suppressed comment from the 2026-08-07 review (src/hold-my-task.mjs:1633 — no regression test covered the stranded-ready-task failure mode):

Added a deterministic regression test in 964088d (tests/BypassDelayCompletionTiming.test.vitest.mjs). It uses the constructor's injectable now option to force the exact race (a readyHeap task whose delay has just expired, with pendingHeap empty) without relying on real wall-clock timing luck, then spies on setTimeout to assert _scheduleNextTick() arms an imminent recheck rather than the ~24.8-day fallback. Verified it fails against the pre-fix code and passes with the fix.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (1)

tests/BypassDelayCompletionTiming.test.vitest.mjs:130

• setTimeoutSpy.mockRestore() is called unconditionally right after _scheduleNextTick(). If _scheduleNextTick() ever throws (or future changes add a throw), the spy won't be restored and can leak into later tests. Also, restoring before you inspect mock.calls is brittle if Vitest ever changes the semantics of mockRestore().

Capture the calls before restoring, and restore the spy in a finally so cleanup is guaranteed.

		const setTimeoutSpy = vi.spyOn(global, "setTimeout");
		q._scheduleNextTick();
		setTimeoutSpy.mockRestore();

[Shinrai]

Addressing the suppressed comment from the 2026-08-08 review (tests/BypassDelayCompletionTiming.test.vitest.mjs:130 — setTimeoutSpy.mockRestore() called unconditionally, and mock.calls read after restore):

Fixed in 85afcf0. The _scheduleNextTick() call is now wrapped in try/finally; the recorded calls are captured into a local before restoring, and mockRestore() runs in the finally so the global spy can't leak into later tests even if _scheduleNextTick() throws.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (1)

tests/BypassDelayCompletionTiming.test.vitest.mjs:68

• q.destroy() is only guaranteed to run after the drain await resolves; if drain is never emitted (the exact failure mode described in this file’s header), the test will hang until Vitest’s global timeout and the queue won’t be cleaned up. Consider adding a per-test timeout around the drain wait and wrapping the wait + assertions in a single try/finally so cleanup always runs.

		await new Promise((resolve) => q.once("drain", resolve));

		try {
			expect(results).toEqual(["task1", "task2", "task3"]); // Normal priority order, task2 bypasses task1's delay

[Shinrai]

Addressing the suppressed comment from the latest review (tests/BypassDelayCompletionTiming.test.vitest.mjs:68 — the drain await sits outside the try/finally, so if drain never fires the test hangs to the 30s global timeout and never destroys):

Fixed in ff6c70f. Moved the drain wait inside the try and raced it against a 5s timeout that rejects with a clear message — so if drain never fires (the exact deadlock this file exists to catch), it fails fast well under vitest's global timeout, and q.destroy() in the finally always runs.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (1)

tests/BypassDelayCompletionTiming.test.vitest.mjs:115

• The regression test relies on a fixed 120ms sleep to assume task1 has completed and task2 has moved into readyHeap. On slower/contended CI, this can be nondeterministic and cause intermittent failures. Prefer waiting on an observable condition (e.g., results containing "task1") with a bounded timeout instead of a hard-coded delay.

	// Wait for task1 to actually run and complete (real time - the default poll
	// tick is 25ms, so this needs to clear a few real ticks; the queue's own delay
	// bookkeeping uses the injected now(), not real time).
	await new Promise((resolve) => setTimeout(resolve, 120));

[Shinrai]

Addressing the suppressed comment from the latest review (tests/BypassDelayCompletionTiming.test.vitest.mjs:115 — fixed 120ms sleep is nondeterministic on slow CI):

Fixed in a456202. Replaced the hard-coded sleep with a bounded poll (2s deadline) on the observable condition — results contains task1 and task2 is parked in readyHeap (readyHeap.size === 1, pendingHeap.size === 0) — and kept it inside the try so q.destroy() still runs if it ever times out. Ran it repeatedly locally to confirm it's stable.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.