From ac2ec877bb6942a33d8dfff1c872ccb0cd579d71 Mon Sep 17 00:00:00 2001 From: Shinrai Date: Tue, 4 Aug 2026 05:48:46 -0700 Subject: [PATCH 1/7] test(ci): isolate flaky bypassDelay completion-delay test into its own file The "bypassed task still applies its own completion delay" test hangs to the 30s vitest timeout on the lts/* CI matrix job every time it runs there, while the explicit 20/22/24 jobs (same Node binary) pass. Doesn't reproduce locally, including under CPU throttling. Moving it out of HoldMyTask.test.vitest.mjs (72 tests, ~34 of which never call destroy() on their queue instance) into its own file isolates it from any state/timer accumulation earlier tests in that file might leave behind, to see if that's a factor. --- ...ypassDelayCompletionTiming.test.vitest.mjs | 68 +++++++++++++++++++ tests/HoldMyTask.test.vitest.mjs | 52 -------------- 2 files changed, 68 insertions(+), 52 deletions(-) create mode 100644 tests/BypassDelayCompletionTiming.test.vitest.mjs diff --git a/tests/BypassDelayCompletionTiming.test.vitest.mjs b/tests/BypassDelayCompletionTiming.test.vitest.mjs new file mode 100644 index 0000000..0d1aef2 --- /dev/null +++ b/tests/BypassDelayCompletionTiming.test.vitest.mjs @@ -0,0 +1,68 @@ +import { test, expect, describe } from "vitest"; +import { HoldMyTask } from "../src/hold-my-task.mjs"; + +// Isolated from HoldMyTask.test.vitest.mjs: this test hangs to 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, 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); + + 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 }); From de48ac6154d0d85dcd22b8fdb13b306a58e7b603 Mon Sep 17 00:00:00 2001 From: Shinrai Date: Tue, 4 Aug 2026 07:35:45 -0700 Subject: [PATCH 2/7] fix: prevent scheduler deadlock when a ready task's delay expires mid-reschedule _scheduleNextTick() only derived its next wake time from pendingHeap and nextAvailableTime. If it ran 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 - a permanent hang, not a slow test. This is what caused the bypassDelay + concurrency:1 + postDelay tests to intermittently time out at exactly 30000ms on CI's lts/* job. Reproduced locally via a stress loop (1 stall per ~6000 iterations); the fix (accounting for a non-empty readyHeap when computing nextTime) ran 20000 iterations with zero stalls. --- src/hold-my-task.mjs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/hold-my-task.mjs b/src/hold-my-task.mjs index 9cdb198..f570d4b 100644 --- a/src/hold-my-task.mjs +++ b/src/hold-my-task.mjs @@ -1622,6 +1622,13 @@ export class HoldMyTask extends EventEmitter { nextTime = Math.min(nextTime, this.nextAvailableTime); } + // A task already sitting in the ready heap needs an imminent recheck even if + // pendingHeap is empty and nextAvailableTime has already expired - otherwise + // nextTime falls through to Infinity and the 24.8-day fallback below strands it. + if (this.readyHeap.size() > 0) { + nextTime = Math.min(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); From 471bd5003f3f21238639a4e76f3c64dd9c51f214 Mon Sep 17 00:00:00 2001 From: Shinrai Date: Tue, 4 Aug 2026 10:03:11 -0700 Subject: [PATCH 3/7] fix(review): only force imminent recheck when nextTime would stay Infinity Addresses PR #8 review feedback: the prior fix forced nextTime = now whenever readyHeap was non-empty, which degrades an active post-completion delay into a 25ms setInterval poll loop instead of a single setTimeout. Narrow the guard to only fire when nextTime would otherwise fall through to Infinity - the actual stranded-ready-task case. Verified: 20000-iteration stress run stays at 0 stalls, and a normal delayed task now arms a single timeoutId (not intervalId) again. Also addresses three test-file review comments: a stale "priority 2" code comment, wrap the isolated test's assertions in try/finally with `once` instead of `on` so q.destroy() always runs even on assertion failure, and a grammar fix in the file header comment. --- src/hold-my-task.mjs | 13 ++++++---- ...ypassDelayCompletionTiming.test.vitest.mjs | 26 ++++++++++--------- 2 files changed, 22 insertions(+), 17 deletions(-) diff --git a/src/hold-my-task.mjs b/src/hold-my-task.mjs index f570d4b..a428686 100644 --- a/src/hold-my-task.mjs +++ b/src/hold-my-task.mjs @@ -1622,11 +1622,14 @@ export class HoldMyTask extends EventEmitter { nextTime = Math.min(nextTime, this.nextAvailableTime); } - // A task already sitting in the ready heap needs an imminent recheck even if - // pendingHeap is empty and nextAvailableTime has already expired - otherwise - // nextTime falls through to Infinity and the 24.8-day fallback below strands it. - if (this.readyHeap.size() > 0) { - nextTime = Math.min(nextTime, now); + // 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 diff --git a/tests/BypassDelayCompletionTiming.test.vitest.mjs b/tests/BypassDelayCompletionTiming.test.vitest.mjs index 0d1aef2..58474bb 100644 --- a/tests/BypassDelayCompletionTiming.test.vitest.mjs +++ b/tests/BypassDelayCompletionTiming.test.vitest.mjs @@ -1,7 +1,7 @@ import { test, expect, describe } from "vitest"; import { HoldMyTask } from "../src/hold-my-task.mjs"; -// Isolated from HoldMyTask.test.vitest.mjs: this test hangs to the 30s vitest +// 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 @@ -31,7 +31,7 @@ describe.each([ { priority: 1 } ); - // Task 2: Same priority but bypasses the delay from task1, priority 2 means higher priority but has delay + // Task 2: Same priority but bypasses the delay from task1 q.enqueue( () => { timestamps.push(Date.now()); @@ -51,18 +51,20 @@ describe.each([ { priority: 1 } ); - await new Promise((resolve) => q.on("drain", resolve)); + await new Promise((resolve) => q.once("drain", resolve)); - expect(results).toEqual(["task1", "task2", "task3"]); // Normal priority order, task2 bypasses task1's delay + try { + 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); + // 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); - - q.destroy(); + // 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(); + } }); }); From 964088d3b927b7306f0d429f6a5a110f5d87372f Mon Sep 17 00:00:00 2001 From: Shinrai Date: Fri, 7 Aug 2026 18:06:37 -0700 Subject: [PATCH 4/7] test: add regression test for the scheduler stranded-task deadlock Addresses PR #8 review feedback (suppressed comment on src/hold-my-task.mjs:1633): no regression test covered the specific failure mode the fix addresses, so a future refactor of _scheduleNextTick()/schedulerTick() could reintroduce the 24.8-day timeout fallback silently. Uses the constructor's injectable `now` option to deterministically force the exact race (a readyHeap task whose delay has just expired, with pendingHeap empty) instead of relying on real wall-clock timing luck. Verified the test fails against the pre-fix code (armed a ~2147483647ms setTimeout) and passes with the fix (armed delay < 1000ms). --- ...ypassDelayCompletionTiming.test.vitest.mjs | 66 ++++++++++++++++++- 1 file changed, 65 insertions(+), 1 deletion(-) diff --git a/tests/BypassDelayCompletionTiming.test.vitest.mjs b/tests/BypassDelayCompletionTiming.test.vitest.mjs index 58474bb..986d5fa 100644 --- a/tests/BypassDelayCompletionTiming.test.vitest.mjs +++ b/tests/BypassDelayCompletionTiming.test.vitest.mjs @@ -1,4 +1,4 @@ -import { test, expect, describe } from "vitest"; +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 @@ -68,3 +68,67 @@ describe.each([ } }); }); + +// 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 } + ); + + // 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)); + + try { + // task2 should now be 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"); + q._scheduleNextTick(); + setTimeoutSpy.mockRestore(); + + if (setTimeoutSpy.mock.calls.length > 0) { + const armedDelay = setTimeoutSpy.mock.calls.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(); + } +}); From 85afcf0c662dd0652fd2dc976f7db3ed0a2e40c7 Mon Sep 17 00:00:00 2001 From: Shinrai Date: Sat, 8 Aug 2026 10:04:12 -0700 Subject: [PATCH 5/7] test(review): restore setTimeout spy in finally, capture calls before restore Addresses the suppressed Copilot comment on PR #8 (BypassDelayCompletionTiming.test.vitest.mjs:130): the regression test called setTimeoutSpy.mockRestore() unconditionally right after _scheduleNextTick(), so a throw there would leak the global spy into later tests, and it read mock.calls after restoring. Now wrap the _scheduleNextTick() call in try/finally, capture the recorded calls into a local before restoring, and restore the spy in the finally so cleanup is guaranteed. --- tests/BypassDelayCompletionTiming.test.vitest.mjs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/tests/BypassDelayCompletionTiming.test.vitest.mjs b/tests/BypassDelayCompletionTiming.test.vitest.mjs index 986d5fa..9805735 100644 --- a/tests/BypassDelayCompletionTiming.test.vitest.mjs +++ b/tests/BypassDelayCompletionTiming.test.vitest.mjs @@ -116,11 +116,18 @@ test("a readyHeap task is not stranded when its delay has just expired (regressi fakeNow = q.nextAvailableTime + 1; const setTimeoutSpy = vi.spyOn(global, "setTimeout"); - q._scheduleNextTick(); - setTimeoutSpy.mockRestore(); + 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 (setTimeoutSpy.mock.calls.length > 0) { - const armedDelay = setTimeoutSpy.mock.calls.at(-1)[1]; + 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); From ff6c70f3f5503bf3fe59f75004c6050ba9fd30d5 Mon Sep 17 00:00:00 2001 From: Shinrai Date: Sat, 8 Aug 2026 17:08:59 -0700 Subject: [PATCH 6/7] test(review): fail fast + always clean up if drain never fires Addresses the suppressed Copilot comment on PR #8 (BypassDelayCompletionTiming.test.vitest.mjs:68): the `drain` await sat OUTSIDE the try/finally, so if drain is never emitted - the exact deadlock this file exists to catch - the test would hang to vitest's 30s global timeout and never run q.destroy(), leaking timers. Moved the wait inside the try and raced it against a 5s timeout that rejects with a clear message, so the failure surfaces fast and cleanup always runs. --- tests/BypassDelayCompletionTiming.test.vitest.mjs | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/tests/BypassDelayCompletionTiming.test.vitest.mjs b/tests/BypassDelayCompletionTiming.test.vitest.mjs index 9805735..b2dc9be 100644 --- a/tests/BypassDelayCompletionTiming.test.vitest.mjs +++ b/tests/BypassDelayCompletionTiming.test.vitest.mjs @@ -51,9 +51,19 @@ describe.each([ { priority: 1 } ); - await new Promise((resolve) => q.once("drain", resolve)); - 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) From a456202592ae191bbf4f6354c2e693be0c2e92aa Mon Sep 17 00:00:00 2001 From: Shinrai Date: Sat, 8 Aug 2026 17:56:06 -0700 Subject: [PATCH 7/7] test(review): poll for the observable state instead of a fixed 120ms sleep Addresses the suppressed Copilot comment on PR #8 (BypassDelayCompletionTiming.test.vitest.mjs:115): the regression test used a hard-coded 120ms sleep to assume task1 had completed and task2 had reached readyHeap, which is nondeterministic on slow/contended CI. Replaced it with a bounded poll (2s deadline) on the observable condition - task1 in results and task2 parked in readyHeap - and kept it inside the try so q.destroy() still runs if it ever times out. --- ...ypassDelayCompletionTiming.test.vitest.mjs | 21 +++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/tests/BypassDelayCompletionTiming.test.vitest.mjs b/tests/BypassDelayCompletionTiming.test.vitest.mjs index b2dc9be..9936455 100644 --- a/tests/BypassDelayCompletionTiming.test.vitest.mjs +++ b/tests/BypassDelayCompletionTiming.test.vitest.mjs @@ -109,13 +109,22 @@ test("a readyHeap task is not stranded when its delay has just expired (regressi { priority: 1 } ); - // 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)); - try { - // task2 should now be sitting in readyHeap, blocked by task1's post-completion delay. + // 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);