Skip to content

fix(irq): inject the watch kernel's clock so floor tests are deterministic - #7609

Closed
bolichen97 wants to merge 1 commit into
mainfrom
fix/irq-deterministic-floor-clock-7598
Closed

bolichen97 wants to merge 1 commit into
mainfrom
fix/irq-deterministic-floor-clock-7598

Conversation

@bolichen97

Copy link
Copy Markdown
Collaborator

Fixes the intermittent failure of test/test_irq.py::test_an_entry_joining_after_a_partial_fire_serves_its_own_floor reported 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 bare assert 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 _COALESCE was explicitly rejected).

  • src/kiro_crew/irq.py: added from typing import Callable; added a keyword-only clock: Callable[[], float] = time.time parameter to run() (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) through now = clock(); added a short docstring note. No other clock read remains in run().
  • test/test_irq.py: added a _Clock (callable, seeded at real time.time(), with advance()); a module-level _CLOCK reset per test inside the existing autouse _isolated_home fixture; _settle() now advances _CLOCK by _COALESCE * 3 instead of time.sleep; _verdict injects clock=_CLOCK. Two sibling tests were made consistent with the frozen clock (_CLOCK.advance(0.05) and seeding from _CLOCK()). _COALESCE stays 0.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 the time.time default (backward-compatible).

Testing

  • Full module test/test_irq.py: 60 passed, 0 skips/xfails/failures (Python 3.12, PYTHONPATH=src:<site-packages>, --noconftest because the repo root conftest needs hypothesis, not installable under the sandbox's repository-access-only network mode).
  • Target test deterministic: 30/30 in a stress loop.
  • Test-meaningfulness check: reverting the guarded per-entry-floor behavior (fix(irq): give a coalescing entry its own floor instead of the window's age #7431) makes the target test fail deterministically at the 'No _settle() on purpose' joined assertion, then pass again after restore, confirming the guard still bites and now fails cleanly instead of racing.

Notes

  • No build/Docker verification was possible under the sandbox's repository-access-only network mode (no PyPI/registry access). This is a pure-Python change and the affected module's tests are self-contained and green.
  • No overlap with open PR fix(irq): preserve sticky window age across epochs #7584 (its edits are in the epoch-change tests around line 877; this change touches shared fixtures plus the target/sibling tests and the two clock() reads).

…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.
@bolichen97
bolichen97 requested a review from a team as a code owner September 1, 2026 11:34
@bolichen97
bolichen97 requested a review from smeyffret September 1, 2026 11:34
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 1, 2026
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 84413a498cb9adaef07e3e93f65a8498707970c3 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 84413a4

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 84413a498cb9adaef07e3e93f65a8498707970c3: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of 84413a498cb9adaef07e3e93f65a8498707970c3 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 84413a498cb9adaef07e3e93f65a8498707970c3 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 84413a4

Verdict parsed from the review's SHA-scoped output markers for commit 84413a498cb9adaef07e3e93f65a8498707970c3.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 84413a498cb9adaef07e3e93f65a8498707970c3: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 84413a498cb9adaef07e3e93f65a8498707970c3 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All verification complete. The claims in the description hold: exactly two wall-clock reads in irq.py, both routed through clock(); one production caller (pr_watch.py:603) unchanged; every test in test_irq.py reaches run() through _verdict, so no fake/real clock mismatch is possible; zero time.sleep calls remain in test_irq.py; and the three sleeps in test_babysit_pr_watch.py all sit on the "stall helps" side (asserting a window HAS closed), so they are not siblings of the racy root cause.

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 ships

Intent: make the coalescing-floor test stop racing CI scheduling by letting tests control the kernel's clock. FIX.

  1. run() accepts an injectable clock, defaulting to real time — justified
  2. Test suite gets a per-test frozen _Clock, reset by the existing fixture — justified
  3. _settle() steps the fake clock instead of sleeping real time — justified (this is the fix)
  4. Every test_irq.py test now runs on the fake clock via _verdict — justified (required, or _settle moves nothing)
  5. Two sibling tests converted to the fake clock — justified (real sleeps no longer advance the injected clock)
  6. Suite drops ~30 ms real sleep per settling test — declared, rides along harmlessly

Verification counts: time.time|clock in src/kiro_crew/irq.py — 2 reads, both converted, 0 remaining; run( in test_irq.py — 1 call site, inside _verdict; time.sleep in test_irq.py — 0 remaining; time.sleep in test_babysit_pr_watch.py — 3, all asserting a window HAS closed, where a stall only helps, so no unfixed siblings of this root cause. The rejected alternative (raising _COALESCE) was the symptom patch; this sits at the cause. The clock kwarg's only callers are tests, but it is the narrowest mechanism for a derived requirement — the in-repo alternative (globally monkeypatching time.time, as test_autonudge.py:44 does) mutates the shared stdlib module process-wide, which is broader, not smaller.

[FIRST-PRINCIPLES-REVIEWED] 84413a4

@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 1, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author

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.

@bolichen97 bolichen97 closed this Sep 1, 2026
@github-actions github-actions Bot removed the readiness: action required A blocking check or review needs attention label Sep 1, 2026
@bolichen97
bolichen97 deleted the fix/irq-deterministic-floor-clock-7598 branch September 6, 2026 03:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge conflict Branch has merge conflicts with its base — author must resolve before merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants