close
Skip to content

fix(test_wait_tool_early_end): scope the wait-tool fake clock to the wait loop thread - #2460

Merged
iamwhatever merged 1 commit into
mainfrom
feature/fix-wait-clock-leak
Aug 10, 2026
Merged

fix(test_wait_tool_early_end): scope the wait-tool fake clock to the wait loop thread#2460
iamwhatever merged 1 commit into
mainfrom
feature/fix-wait-clock-leak

Conversation

@tlobinger

@tlobinger tlobinger commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Summary

This PR combines 1 related change:

  • Scope the wait-tool fake clock to the wait loop thread

Changes and rationale

Scope the wait-tool fake clock to the wait loop thread

Problem: test/test_wait_tool_early_end.py fabricates a fake monotonic clock by patching the GLOBAL time module (patch.object(_time, "monotonic", ...) + patch.object(_time, "sleep", ...) in _run_wait, ~lines 62-67). The patch is process-wide, so any background thread alive in the pytest worker that calls time.sleep() advances the fake clock too. The tests then assert EXACT equality (clock.t == float(MIN_WAIT) i.e. == 60.0), so a single leaked 1ms sleep from an unrelated thread fails the test.
Why it matters: Every full-suite run on a busy host can fail on this module, burning local gate attempts and CI reruns for unrelated PRs. It already burned one ship attempt of a pr-maintenance loop (zero signal against that loop's change).
Tests: Regression pinning the leak: while a wait runs under _run_wait, a helper thread calls time.sleep(0.001) repeatedly; clock.t still ends at exactly float(MIN_WAIT). Fails on the current code, passes after. Full existing test_wait_tool_early_end.py suite green: .venv/bin/python -m pytest test/test_wait_tool_early_end.py -o addopts="" -p no:cacheprovider
Review focus: backend

Review map

Change Primary files
Scope the wait-tool fake clock to the wait loop thread test/test_wait_tool_early_end.py

Commits

  • ebd232b fix(test_wait_tool_early_end): scope the wait-tool fake clock to the …

Validation

Local build, static-analysis, and test gates completed before publication. Upstream CI and review checks on the current PR revision remain the authoritative merge signal.

Changed files (1 files, +51/-9)
  • test/test_wait_tool_early_end.py (+51/-9)

This description updates automatically as related changes land; manually written text outside this block is preserved.

@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] ebd232b

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

@tlobinger
tlobinger marked this pull request as ready for review August 10, 2026 02:30
@tlobinger
tlobinger requested a review from a team as a code owner August 10, 2026 02:30
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Aug 10, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

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

Review details

This is a test-only change. The _Clock captures the owner thread's ident and the real time functions at construction (before patching), then routes non-owner threads to the real functions. The wait loop runs synchronously in the owner thread (confirmed by the existing exact-equality asserts that require the fake clock to advance), so the scoping is correct. The new regression test starts its background sleeper before _run_wait, so it exercises the non-owner path as intended.

No correctness, security, or resource concerns on the changed lines.

No findings.

[OPUS-REVIEWED] ebd232b

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

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

@github-actions

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Advisory design-level review of ebd232bade6f177e7b5592bb189a7b579bfab7e9 — updated in place on each push; does not block merge.

Design-Verdict: PASS

Root-cause fix at the right layer: the fake clock now engages only on the wait-loop's own thread, with a regression test that pins the leak.

[DESIGN-REVIEWED] ebd232b

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Aug 10, 2026
@iamwhatever
iamwhatever merged commit b46901d into main Aug 10, 2026
83 of 86 checks passed
@iamwhatever
iamwhatever deleted the feature/fix-wait-clock-leak branch August 10, 2026 04:43
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Aug 10, 2026
encomjp pushed a commit to encomjp/kirocrew-customapi that referenced this pull request Aug 22, 2026
…wait loop thread (kirodotdev#2460)

Co-authored-by: Thomas Lobinger <23685+tlobinger@users.noreply.github.com>
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