diff --git a/src/hold-my-task.mjs b/src/hold-my-task.mjs index 9cdb198..a428686 100644 --- a/src/hold-my-task.mjs +++ b/src/hold-my-task.mjs @@ -1622,6 +1622,16 @@ export class HoldMyTask extends EventEmitter { nextTime = Math.min(nextTime, this.nextAvailableTime); } + // A task already sitting in the ready heap needs an imminent recheck if nothing + // else would schedule one - otherwise nextTime falls through to Infinity and the + // 24.8-day fallback below strands it. Only force this when nextTime would + // otherwise stay Infinity (the stranded-task case); an active future delay + // 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; + } + // If next event is imminent or past, run immediately if (nextTime <= now + this.options.tick) { this.intervalId = setInterval(() => this.schedulerTick(), this.options.tick); diff --git a/tests/BypassDelayCompletionTiming.test.vitest.mjs b/tests/BypassDelayCompletionTiming.test.vitest.mjs new file mode 100644 index 0000000..9936455 --- /dev/null +++ b/tests/BypassDelayCompletionTiming.test.vitest.mjs @@ -0,0 +1,160 @@ +import { test, expect, describe, vi } from "vitest"; +import { HoldMyTask } from "../src/hold-my-task.mjs"; + +// Isolated from HoldMyTask.test.vitest.mjs: this test hangs until the 30s Vitest +// timeout on the `lts/*` CI matrix job specifically (never on the explicit 20/22/24 +// jobs running the identical Node binary), and never reproduces locally even under +// CPU throttling. Splitting it into its own file removes it from the tail end of a +// 72-test file where ~30 prior HoldMyTask instances are never destroy()'d, to check +// whether accumulated per-instance timer/state leakage from earlier tests in the same +// file is a factor. +describe.each([ + { smartScheduling: true, mode: "Smart Scheduling" }, + { smartScheduling: false, mode: "Traditional Polling" } +])("HoldMyTask with $mode", ({ smartScheduling }) => { + test("bypassed task still applies its own completion delay", async () => { + const q = new HoldMyTask({ + concurrency: 1, + delays: { 1: 200, 2: 400 }, + smartScheduling + }); + const results = []; + const timestamps = []; + + // Task 1: Priority 1 (200ms delay) - will complete first + q.enqueue( + () => { + timestamps.push(Date.now()); + return "task1"; + }, + (err, result) => results.push(result), + { priority: 1 } + ); + + // Task 2: Same priority but bypasses the delay from task1 + q.enqueue( + () => { + timestamps.push(Date.now()); + return "task2"; + }, + (err, result) => results.push(result), + { priority: 1, bypassDelay: true } + ); + + // Task 3: Should wait for whatever delay task2 creates (task2 has no specific delay config, so uses priority 1 = 200ms) + q.enqueue( + () => { + timestamps.push(Date.now()); + return "task3"; + }, + (err, result) => results.push(result), + { priority: 1 } + ); + + try { + // Race the drain wait against a short timeout: if drain never fires (the exact + // deadlock this file exists to catch), fail fast instead of hanging to vitest's + // 30s global timeout - and keep the wait inside the try so the finally still + // runs q.destroy() and doesn't leak timers into later tests. + await new Promise((resolve, reject) => { + const timer = setTimeout(() => reject(new Error("drain not emitted within 5000ms")), 5000); + q.once("drain", () => { + clearTimeout(timer); + resolve(); + }); + }); + + expect(results).toEqual(["task1", "task2", "task3"]); // Normal priority order, task2 bypasses task1's delay + + // Task2 bypasses task1's delay (should start immediately after task1) + const task1ToTask2Gap = timestamps[1] - timestamps[0]; + expect(task1ToTask2Gap).toBeLessThan(100); + + // Task3 waits for task2's completion delay (200ms since task2 is priority 1) + const task2ToTask3Gap = timestamps[2] - timestamps[1]; + expect(task2ToTask3Gap).toBeGreaterThan(150); + } finally { + q.destroy(); + } + }); +}); + +// Regression test for the scheduler deadlock fixed alongside the test above: +// _scheduleNextTick() only derived its next wake time from pendingHeap and +// nextAvailableTime. Called at the exact moment nextAvailableTime had just +// expired (a narrow race against schedulerTick's own timing), with pendingHeap +// empty, nextTime fell through to Infinity - arming a ~24.8-day setTimeout and +// stranding any task already sitting in readyHeap forever. Traditional Polling +// only: Smart Scheduling uses a different scheduleSmartTimeout()/runScheduler() +// path unaffected by this bug. Uses the constructor's injectable `now` option to +// force the exact race deterministically instead of racing real wall-clock time. +test("a readyHeap task is not stranded when its delay has just expired (regression)", async () => { + let fakeNow = Date.now(); + const q = new HoldMyTask({ + concurrency: 1, + delays: { 1: 50 }, + smartScheduling: false, + now: () => fakeNow + }); + const results = []; + + q.enqueue( + () => "task1", + (err, r) => results.push(r), + { priority: 1 } + ); + q.enqueue( + () => "task2", + (err, r) => results.push(r), + { priority: 1 } + ); + + try { + // Wait (bounded) until task1 has actually run + completed and task2 has moved + // into readyHeap, blocked by task1's post-completion delay - polling the + // observable state instead of a fixed sleep, which is nondeterministic on slow/ + // contended CI. The queue's delay bookkeeping uses the injected now(), not real + // time, so task2 stays parked in readyHeap. Kept inside the try so the finally + // still runs q.destroy() if this ever times out. + const deadline = Date.now() + 2000; + while (!(results.includes("task1") && q.readyHeap.size() === 1 && q.pendingHeap.size() === 0)) { + if (Date.now() > deadline) { + throw new Error("task1 did not complete / task2 did not reach readyHeap within 2000ms"); + } + await new Promise((resolve) => setTimeout(resolve, 5)); + } + + // task2 is now sitting in readyHeap, blocked by task1's post-completion delay. + expect(q.readyHeap.size()).toBe(1); + expect(q.pendingHeap.size()).toBe(0); + expect(q.nextAvailableTime).toBeGreaterThan(0); + + // Simulate wall-clock time crossing nextAvailableTime right before the + // scheduler gets a chance to recheck it - the exact race window that caused + // the deadlock. + fakeNow = q.nextAvailableTime + 1; + + const setTimeoutSpy = vi.spyOn(global, "setTimeout"); + let armedCalls; + try { + q._scheduleNextTick(); + } finally { + // Capture the recorded calls BEFORE restoring, and restore in finally so the + // global spy never leaks into later tests even if _scheduleNextTick throws. + armedCalls = setTimeoutSpy.mock.calls.slice(); + setTimeoutSpy.mockRestore(); + } + + if (armedCalls.length > 0) { + const armedDelay = armedCalls.at(-1)[1]; + // Before the fix this was ~2147483647 (the 24.8-day fallback), stranding + // task2 forever. After the fix it must be an imminent recheck. + expect(armedDelay).toBeLessThan(1000); + } else { + // Took the interval branch instead - also an imminent recheck, also fine. + expect(q.intervalId).toBeTruthy(); + } + } finally { + q.destroy(); + } +}); diff --git a/tests/HoldMyTask.test.vitest.mjs b/tests/HoldMyTask.test.vitest.mjs index 82ea457..1ed7331 100644 --- a/tests/HoldMyTask.test.vitest.mjs +++ b/tests/HoldMyTask.test.vitest.mjs @@ -803,58 +803,6 @@ describe.each([ // Test passed if no timeout occurred (task2 started immediately) }); - test("bypassed task still applies its own completion delay", async () => { - const q = new HoldMyTask({ - concurrency: 1, - delays: { 1: 200, 2: 400 }, - smartScheduling - }); - const results = []; - const timestamps = []; - - // Task 1: Priority 1 (200ms delay) - will complete first - q.enqueue( - () => { - timestamps.push(Date.now()); - return "task1"; - }, - (err, result) => results.push(result), - { priority: 1 } - ); - - // Task 2: Same priority but bypasses the delay from task1, priority 2 means higher priority but has delay - q.enqueue( - () => { - timestamps.push(Date.now()); - return "task2"; - }, - (err, result) => results.push(result), - { priority: 1, bypassDelay: true } - ); - - // Task 3: Should wait for whatever delay task2 creates (task2 has no specific delay config, so uses priority 1 = 200ms) - q.enqueue( - () => { - timestamps.push(Date.now()); - return "task3"; - }, - (err, result) => results.push(result), - { priority: 1 } - ); - - await new Promise((resolve) => q.on("drain", resolve)); - - expect(results).toEqual(["task1", "task2", "task3"]); // Normal priority order, task2 bypasses task1's delay - - // Task2 bypasses task1's delay (should start immediately after task1) - const task1ToTask2Gap = timestamps[1] - timestamps[0]; - expect(task1ToTask2Gap).toBeLessThan(100); - - // Task3 waits for task2's completion delay (200ms since task2 is priority 1) - const task2ToTask3Gap = timestamps[2] - timestamps[1]; - expect(task2ToTask3Gap).toBeGreaterThan(150); - }); - test("handles maxQueue: -1 as unlimited queue", async () => { const q = new HoldMyTask({ smartScheduling, maxQueue: -1 });