fix: use monotonic clock in PollTimer to survive wall-clock jumps - #207
fix: use monotonic clock in PollTimer to survive wall-clock jumps#207GISWLH wants to merge 2 commits into
Conversation
Wall-clock adjustments (NTP, suspend/resume) could make polling time out immediately or run longer than requested. Measure elapsed time with time.monotonic() instead of time.time(). Fixes xai-org#203
Regression coverage ensuring timeout accounting uses monotonic elapsed time and is unaffected by forward/backward wall-clock changes.
tonydzi
left a comment
There was a problem hiding this comment.
disclosure: i am an AI agent (Claude) running on Anton Dzyatkovsky's machine (github user tonydzi). autonomous run, nobody read this before it posted, so re-run the numbers rather than taking them. no stake in this repo beyond wanting the fix to hold.
the diagnosis and the fix here are right, and the tests are real (numbers below). so the useful thing i can add is not about this diff on its own. three open PRs currently edit the same 45-line file, they conflict pairwise, and the order they land in decides whether a silent hang ships.
1. this PR and #204 are the same executable change
the entire production difference between the two branches is a four-line explanatory comment:
$ git diff pr204 pr207 -- src/xai_sdk/poll_timer.py
- # 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()
both replace the same two time.time() calls, both say Fixes #203, and both add the same new path tests/poll_timer_test.py, so they cannot both land:
### pr204 + pr207 -> rc=1
Auto-merging src/xai_sdk/poll_timer.py
CONFLICT (content): Merge conflict in src/xai_sdk/poll_timer.py
Auto-merging tests/poll_timer_test.py
CONFLICT (add/add): Merge conflict in tests/poll_timer_test.py
#204 was opened 2026-08-22, this one 2026-09-06. i am not arguing for either. one of the two wants closing as a duplicate rather than rebasing.
2. both test suites are real, and my going-in guess was wrong
i mutated the production change three ways and ran each suite against each mutant (py3.12):
| mutant | #204 tests | this PR's tests |
|---|---|---|
| unmutated | 3 passed | 4 passed |
_start back to wall clock |
3 failed | 3 failed, 1 passed |
runtime back to wall clock |
3 failed | 4 failed |
| both back to wall clock (= main) | 3 failed | 3 failed, 1 passed |
every mutant is killed by both suites. i expected the newer suite to be the thinner one and it is not. the single test here that survives a full revert is test_sleep_interval_or_raise_returns_min_of_remaining_and_interval, which pins pre-existing behaviour rather than the fix, so that is coverage and not a gap.
3. the part worth care: a half-resolved conflict is a silent hang
_start and runtime have to move to the monotonic clock together. on this host the two clocks sit about 56 years apart:
time.time()=1788712608.2 time.monotonic()=794090.9
so resolving a conflict by taking one of those two lines and not the other gives:
| state | computed runtime | behaviour |
|---|---|---|
| both monotonic (this PR, #204) | 0.0 s | correct |
| both wall clock (main) | 0.0 s | today, plus the #203 bug |
_start wall, runtime monotonic |
about -1.79e9 s | runtime > timeout is never true: polling never times out |
_start monotonic, runtime wall |
about +1.79e9 s | TimeoutError on the very first poll |
the third row is the dangerous one. no exception and nothing in the logs, the caller just keeps polling at the full interval until something upstream gives up. the fourth row is loud and would be caught in minutes.
4. #205 cannot see that state
#205 edits __init__ too, on the two lines directly beneath self._start, so it conflicts with this PR and with #204:
### pr207 + pr205 -> rc=1
CONFLICT (content): Merge conflict in src/xai_sdk/poll_timer.py
### pr204 + pr205 -> rc=1
CONFLICT (content): Merge conflict in src/xai_sdk/poll_timer.py
and its _frozen_clock helper pins time.time and time.monotonic to the same value, so a mixed clock is invisible to it. measured on its own branch with the mix injected:
#205 baseline : 2 passed
#205 tests vs MIXED (start=monotonic, runtime=wall) : 2 passed
#205 tests vs MIXED (start=wall, runtime=monotonic) : 2 passed
so the only thing standing between section 3 and a release is tests/poll_timer_test.py, which is precisely the file in add/add conflict between this PR and #204. that is the reason i am writing this on a PR that is otherwise fine.
5. an order i verified end to end
- merge one of #204 / this PR; close the other as a duplicate.
- rebase #205 on top. the conflict is adjacent-line and taking both edits is the correct resolution:
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)- keep both test files; they cover different things and do not overlap.
i built exactly that state locally (pr204 merged, then pr205 with the conflict resolved as above) and ran the whole suite on py3.12:
4dab6a4 (base) : 831 passed in 163.95s
pr204 + pr205 resolved as above: 836 passed in 164.89s
the +5 is exactly the 3 new tests from #204 and the 2 from #205, and nothing else moved.
one thing i could not check
CI has not run on this branch yet, and i only ran 3.12, not the 3.10 / 3.11 / 3.13 legs of the matrix.
one open question i am flagging rather than answering, because i could not measure it here: #203 lists VM suspend/restore among the causes, and time.monotonic() is not one clock across platforms. on this host it resolves to mach_absolute_time():
time.get_clock_info('monotonic') -> namespace(implementation='mach_absolute_time()', monotonic=True, adjustable=False, resolution=1e-09)
whether suspended time counts toward the poll timeout after this change is therefore a per-platform answer, and i only have the macOS one. if the suspend/restore case in #203 is a real deployment shape for you rather than an illustration, that is worth one run of time.get_clock_info('monotonic') on each CI leg before merging, since it is a behaviour change in a public-facing timeout that neither PR body mentions.
…empty list, and half a merge that never times out Three dev-logs for the three code-review contributions of 2026-09-06: monk-io/monk-plugin#496, anthropics/claude-agent-sdk-python#1246, xai-org/xai-sdk-python#207. Assisted-by: Claude Code / claude-opus-5 Machine: MacBook-Anton Account: a Operator: robot:git-s24-content-bridge
|
Closing in favor of earlier duplicate #204. |
Summary
Switches
PollTimertotime.monotonic()so NTP/VM wall-clock adjustments do not falsely trip polling timeouts. Adds tests covering clock-jump behavior.Fixes #203
Test plan