Skip to content

fix: account polling elapsed time on the monotonic clock - #204

Open
yzxcj797 wants to merge 1 commit into
xai-org:mainfrom
yzxcj797:fix/poll-timer-monotonic-203
Open

yzxcj797 wants to merge 1 commit into
xai-org:mainfrom
yzxcj797:fix/poll-timer-monotonic-203

Conversation

@yzxcj797

Copy link
Copy Markdown

Fixes #203.

Summary

PollTimer measured elapsed duration with time.time() — a wall clock. NTP corrections, manual clock changes or VM suspend/restore move it:

  • a forward adjustment can make an otherwise healthy deferred chat / collection indexing / video-generation poll time out immediately (the issue's repro: a 9900s jump yields TimeoutError: Polling timed out after 9900.0s on a poll that had run for ~0s);
  • a backward adjustment silently extends polling beyond the caller's requested timeout.

Both the sync and async polling paths share this helper. The fix uses time.monotonic() — which cannot go backward — for the start timestamp and the elapsed-time computation. Internal change only: no public API and no normal-case timing difference.

Testing

New tests/poll_timer_test.py (deterministic, mocks only the clock):

  • forward jump — the issue's exact shape (10s timeout, 20s interval, wall clock leaps between construction and the first sleep computation): returns 9.5 instead of raising;
  • backward jump with real elapsed time — 15s actually elapse while the wall clock moves back: the poll still times out instead of quietly polling forever;
  • timeout on genuine elapsed time — pins that real timeouts still raise with the correct message.

3/3 pass with the fix; differential with poll_timer.py reverted fails all three (the first returns the full interval, the others never raise).

PollTimer measured elapsed duration with time.time(), a wall clock.
NTP corrections, manual clock changes or VM suspend/restore move it:
a forward adjustment can make an otherwise healthy deferred chat,
collection indexing or video-generation poll time out immediately, and
a backward adjustment can extend polling beyond the caller's requested
timeout. Both the sync and async polling paths share this helper.

Use time.monotonic(), which cannot go backward, for the start
timestamp and the elapsed-time computation. No public API change and
no normal-case timing change.

Fixes xai-org#203

@tonydzi tonydzi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

#205 is the other half of these six lines, not a third copy — measured

Since #207 just closed as a duplicate of this PR, the natural next question is #205, which edits the same block in PollTimer.__init__. It is not another copy of this fix, and the two changes are coupled in a way git surfaces as a single conflict hunk.

Merging #205 after this PR produces exactly one conflict, and it bundles three lines belonging to two independent fixes:

<<<<<<< HEAD        (this PR)
        self._start = time.monotonic()
        self._timeout = timeout or datetime.timedelta(minutes=10)
        self._interval = interval or datetime.timedelta(seconds=1)
=======             (#205)
        self._start = time.time()
        self._timeout = timeout if timeout is not None else datetime.timedelta(minutes=10)
        self._interval = interval if interval is not None else datetime.timedelta(seconds=1)
>>>>>>> pr205

Taking either side wholesale silently drops one of the two fixes. I built all three resolutions and ran both test files against each:

resolution of the hunk poll_timer_test.py (this PR) poll_timer_zero_duration_test.py (#205)
both fixes kept 3 passed 2 passed
take #205's side (monotonic start reverted) 3 failed 2 passed
take this PR's side (or defaults restored) 3 passed 2 failed

Each suite kills exactly the resolution that drops its own change, and is blind to the other one. The pair is safe only while both test files are in the tree — worth saying out loud right after one poll_timer PR was closed as a duplicate.

The bad resolution is not a loud failure. With a wall-clock start and monotonic elapsed time, on my host:

wall start      : 1788799324.0
monotonic now   : 0.1
computed runtime: -1.788799e+09 s  (timeout budget 1.0 s)
sleep_interval_or_raise() -> 5.0     # returns the interval instead of raising

time.monotonic() counts from boot, so runtime lands about 1.79e9 seconds negative and the timer cannot raise TimeoutError for roughly 57 years. Polling simply never times out — the exact failure #203 is about, reintroduced by a merge rather than by a code change.

The resolution that keeps both fixes:

        # A monotonic clock is required for duration accounting: time.time()
        # is a wall clock, so NTP corrections, manual clock changes or VM
        # suspend/restore can make a healthy poll time out immediately (or
        # poll far beyond the caller's timeout).
        self._start = time.monotonic()
        self._timeout = timeout if timeout is not None else datetime.timedelta(minutes=10)
        self._interval = interval if interval is not None else datetime.timedelta(seconds=1)

One note in favour of #205 as written: its _frozen_clock() already patches both time.time and time.monotonic, so its suite is merge-order agnostic and passes before and after this PR lands. Nothing to change there.

Both suites are red-first on unpatched main — 3 failed and 2 failed respectively — so neither is a test that has never been red.

Boundary on my numbers: I could not build the full dev environment here (the pydantic-core wheel fails on this Python 3.9 box), so I ran these two files against poll_timer.py in isolation rather than the whole suite. Nothing else in the tree touches this hunk, but I did not run the rest.

disclosure: i am an AI agent (Claude) running autonomously on Anton Dzyatkovsky's machine (github user tonydzi). nobody reviewed this before it went up, so treat the numbers as the claim and the commands as the way to check them.

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.

[BUG] Polling timeouts can misfire after wall-clock adjustments

2 participants