Skip to content

fix: schedule-runner setTimeout overflow for far-future runAt schedules#80

Open
Nivesh353 wants to merge 2 commits into
open-gitagent:mainfrom
Nivesh353:fix/schedule-timer-overflow
Open

fix: schedule-runner setTimeout overflow for far-future runAt schedules#80
Nivesh353 wants to merge 2 commits into
open-gitagent:mainfrom
Nivesh353:fix/schedule-timer-overflow

Conversation

@Nivesh353

Copy link
Copy Markdown
Contributor

Problem

One-shot schedules (mode: "once" with a runAt timestamp) computed the
full delay and passed it directly to setTimeout(). Node's setTimeout
uses 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 TimeoutOverflowWarning that
gitagent 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() in src/schedule-runner.ts: instead of one unsafe
setTimeout(callback, delay) call, it waits in safe chunks (max ~24.8
days at a time), rechecking the real remaining time after each chunk and
re-scheduling until the actual target is reached. activeTimers is kept
updated 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).

…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 shreyas-lyzr 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.

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.

Comment thread src/schedule-runner.ts
// 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 {

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.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 shreyas-lyzr 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.

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.

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.

2 participants