fix: schedule-runner setTimeout overflow for far-future runAt schedules#80
fix: schedule-runner setTimeout overflow for far-future runAt schedules#80Nivesh353 wants to merge 2 commits into
Conversation
…e-runner setTimeout's delay is a 32-bit signed int under the hood — any runAt more than ~24.8 days out silently clamps to 1ms and fires almost immediately instead of waiting. scheduleAt() now re-schedules in safe chunks until the real target time is reached.
shreyas-lyzr
left a comment
There was a problem hiding this comment.
The fix is correct and the approach is sound — chunking into MAX_TIMEOUT_MS segments handles the 32-bit overflow cleanly, and the activeTimers bookkeeping stays consistent throughout (the delete-before-onDue path is fine since executeScheduledJob's clearTimeout call on an already-absent key is a benign no-op).
One blocking gap before merge: there are no tests for scheduleAt. The function is exported and testable in isolation with fake timers (Jest's useFakeTimers / Vitest's vi.useFakeTimers), and the existing test suite has no coverage of the overflow path. Changed logic with no covering test is a blocking finding — if someone refactors the chunking loop later, there is nothing to catch a regression.
Suggested test cases:
- delay < MAX_TIMEOUT_MS: fires in a single setTimeout, callback called exactly once
- delay slightly above MAX_TIMEOUT_MS: first chunk fires at MAX_TIMEOUT_MS, second chunk fires at the remainder, callback called once
- delay <= 0 at call time: callback fires synchronously, no setTimeout scheduled
- cancel mid-chunk: clearTimeout on the activeTimers entry prevents the callback
Everything else looks good — the comment is accurate, the constant is correct (2^31 - 1), and the call site in startScheduler correctly passes new Date(schedule.runAt).getTime() rather than reusing the pre-computed delay (which would drift slightly). No security issues, no new dependencies.
| // and emits a TimeoutOverflowWarning). Chunk long waits into safe segments. | ||
| const MAX_TIMEOUT_MS = 2_147_483_647; | ||
|
|
||
| export function scheduleAt(id: string, targetTime: number, onDue: () => void): void { |
There was a problem hiding this comment.
scheduleAt is exported and testable in isolation — this function needs unit tests before merge. Jest/Vitest fake timers make it straightforward: advance by MAX_TIMEOUT_MS in the long-delay case and confirm the callback fires exactly once at the right time, not before.
There was a problem hiding this comment.
Added test/schedule-timer-overflow.test.ts covering all 4 cases: delay
under the limit (fires once), delay just over it (fires via two chunks,
once, at the right time), an already-past target (fires synchronously),
and cancellation mid-chunk (clearTimeout correctly blocks the callback).
Used the existing node:test + mock.timers setup (matches sdk.test.ts /
telemetry.test.ts) rather than Jest/Vitest, so no new dependency needed.
npm run build and npm test both pass — 31/31, no regressions.
Exports activeTimers alongside scheduleAt so tests can verify cancellation. Covers: delay under the limit, delay requiring a second chunk, an already-past target firing synchronously, and cancellation mid-chunk.
shreyas-lyzr
left a comment
There was a problem hiding this comment.
Tests added covering all four cases I flagged — the short-delay single-chunk path, the two-chunk overflow path, the already-past synchronous path, and the mid-chunk cancel. Each assertion is tight and would catch a regression in the chunking logic or the activeTimers bookkeeping. The mock timer setup (node:test t.mock.timers with both setTimeout and Date mocked together) is the right approach for this kind of recursive setTimeout code.
Everything from the previous review is resolved. The fix is correct, the test coverage is solid, no security issues. Good to merge.
Problem
One-shot schedules (
mode: "once"with arunAttimestamp) computed thefull delay and passed it directly to
setTimeout(). Node'ssetTimeoutuses a 32-bit signed int internally — any delay beyond ~24.8 days
(2,147,483,647 ms) isn't waited out. Node silently clamps it to 1ms and
fires almost instantly instead, emitting a
TimeoutOverflowWarningthatgitagent never surfaces anywhere. A job scheduled 60 days out would run
within milliseconds of starting the scheduler, with no indication anything
was wrong.
Confirmed directly by running the overflow case on Node — a >24.8-day
delay fires in ~6ms instead of waiting.
Fix
Added
scheduleAt()insrc/schedule-runner.ts: instead of one unsafesetTimeout(callback, delay)call, it waits in safe chunks (max ~24.8days at a time), rechecking the real remaining time after each chunk and
re-scheduling until the actual target is reached.
activeTimersis keptupdated with whichever chunk is currently pending, so existing
cancel/disable logic (
clearTimeout) continues to work unchanged.Delays under ~25 days behave identically to before — this only changes
the previously-broken long-delay path.
Refs #30 (this addresses only the schedule-timer finding; the Windows
plugin-loading and resume-integrity findings in that issue are tracked
separately).