fix(irq): inject the watch kernel's clock so floor tests are deterministic - #7609
bolichen97 wants to merge 1 commit into
Conversation
…istic The wall-clock budget in test_irq.py raced on loaded CI runners: the two 'has NOT waited' assertions in test_an_entry_joining_after_a_partial_fire_serves_its_own_floor could see a >10ms scheduling stall close a joining entry's own floor and flip Skip to Report. Add a keyword-only clock: Callable[[], float] = time.time to run() and route both time.time() reads through it. In the test, a controllable clock seeded at real time.time() only advances when _settle() (or a test) moves it, so the floor/cap/age math is exact. _COALESCE stays 0.01 and the guarded behavior is unchanged.
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
Design Review (Fable 5) — ✅ PASSDesign-level review of Design-Verdict: PASS Root-cause fix in the right shape: an injectable clock (keyword-only, defaulted) removes the wall-clock race instead of widening the sleep, per repo testing conventions. [DESIGN-REVIEWED] 84413a4 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
First Principles Review (Fable 5) — ✅ PASSPremise-level review of All verification complete. The claims in the description hold: exactly two wall-clock reads in First-Principles-Verdict: PASS A reported flake (#7598) fixed at cause level — wall-clock coupling removed, not widened — with every rider mandatory once the clock is injected. What this change shipsIntent: make the coalescing-floor test stop racing CI scheduling by letting tests control the kernel's clock. FIX.
Verification counts: [FIRST-PRINCIPLES-REVIEWED] 84413a4 |
|
Superseded: issue #7598 is already fixed on main by my own merged PR #7619 (test(irq): replace real sleeps with a stepped fake clock), which added _FakeClock + monkeypatch.setattr("kiro_crew.irq.time", _clock) now live in test/test_irq.py. This PR took a different design (inject a clock param into irq.py's run()) for the same problem. Closing as superseded by #7619; the DI-seam idea in irq.py can be revisited separately if still wanted. |
Fixes the intermittent failure of
test/test_irq.py::test_an_entry_joining_after_a_partial_fire_serves_its_own_floorreported in #7598.Problem
The target test budgeted its coalescing floor in real wall clock (
_COALESCE = 0.01,_settle()sleeping_COALESCE * 3). Two of its assertions require that a floor has NOT expired, but they were separated only by the call/return between two_verdict()invocations. On a loaded xdist CI runner, any scheduling stall past 10 ms let the joining entry's own floor close, so it reported instead of skipping and the assertion flipped to a bareassert False. Sibling tests that assert a window HAS closed were safe because a stall only helps them; the exposure was specific to the 'must stay short' assertions.Fix
Stop measuring the guard in wall clock by making the kernel's time source injectable (the reporter's recommended approach; raising
_COALESCEwas explicitly rejected).from typing import Callable; added a keyword-onlyclock: Callable[[], float] = time.timeparameter torun()(positional(ctx, probe)signature unchanged); routed the module's only two wall-clock reads (now = time.time()in the blind-alert re-arm path and in the main coalescing/dedupe path) throughnow = clock(); added a short docstring note. No other clock read remains inrun()._Clock(callable, seeded at realtime.time(), withadvance()); a module-level_CLOCKreset per test inside the existing autouse_isolated_homefixture;_settle()now advances_CLOCKby_COALESCE * 3instead oftime.sleep;_verdictinjectsclock=_CLOCK. Two sibling tests were made consistent with the frozen clock (_CLOCK.advance(0.05)and seeding from_CLOCK())._COALESCEstays0.01. The guarded test is intact (not deleted/skipped/xfailed).The sole production caller
pr_watch.py:603 run(ctx, PrWatchProbe())is unchanged and uses thetime.timedefault (backward-compatible).Testing
test/test_irq.py: 60 passed, 0 skips/xfails/failures (Python 3.12,PYTHONPATH=src:<site-packages>,--noconftestbecause the repo root conftest needshypothesis, not installable under the sandbox's repository-access-only network mode).Notes
clock()reads).