From c0ff00a1ffa9bd89d33c40d1c69ce170625c39b6 Mon Sep 17 00:00:00 2001 From: Patrick Buckley Date: Fri, 3 Jul 2026 19:05:06 -0700 Subject: [PATCH] fix(test): eliminate lost-wakeup race in approval-prompt tests The UI-approval tests drive a blocking approve_tools() by firing resolve_approval() from a fixed 0.05s threading.Timer. approve_tools does _approval_event.clear() -> register _pending_approval -> wait(3600s); on a slow/loaded runner the timer can fire the event's .set() BEFORE that .clear(), so the wakeup is wiped and approve_tools blocks the full _APPROVAL_WAIT_TIMEOUT (one hour) -- surfacing as an intermittent CI hang (observed on the 3.12 runner ~15% into the suite; fast runners win the race, so 3.11/3.13 pass the same commit). Replace the fixed-delay timer with resolve_when_pending() (tests/conftest.py): it waits until the approval is actually registered -- which happens AFTER the clear -- before resolving, so the set can never be lost. The helper mirrors threading.Timer's start()/cancel() so the surrounding scaffolding is unchanged. 10 sites across 3 files; the verdict-delivery timer (bounded to its own 5s budget, not a hang) is left as-is. Validated: the 3 files pass 20/20 under single-CPU stress (taskset -c 0) with no hang or thread leak. --- tests/conftest.py | 54 ++++++++++++++++++++- tests/test_coord_ui_approve_tools.py | 15 +++--- tests/test_session_ui_base.py | 7 +-- tests/test_webui_auto_approve_visibility.py | 12 ++--- 4 files changed, 70 insertions(+), 18 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index cc1519ef..d071ced1 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -52,8 +52,60 @@ def serve_until_exit(server: Any) -> None: loop.close() +class _PendingResolver: + """Race-free drop-in for ``threading.Timer(delay, ui.resolve_approval)``. + + ``approve_tools`` runs ``_approval_event.clear()`` -> register + ``_pending_approval`` -> ``_approval_event.wait(_APPROVAL_WAIT_TIMEOUT)`` + (3600s). A *fixed-delay* timer can fire ``resolve_approval`` + (``_approval_event.set()``) BEFORE that ``.clear()`` on a slow/loaded + runner, so the set is wiped by the clear and ``approve_tools`` blocks the + full hour -- surfacing as a CI hang. This instead waits until the approval + is actually registered (which happens *after* the clear), then resolves, so + the wakeup can never be lost. ``start()`` / ``cancel()`` mirror + ``threading.Timer`` so it drops into existing scaffolding; ``cancel()`` + joins the worker (the resolve has already run or the deadline lapsed). + ``before`` runs just before resolving -- e.g. to snapshot pending-state + fields the test asserts on. + """ + + def __init__( + self, + ui: Any, + *args: Any, + before: Callable[[], None] | None = None, + deadline: float = 10.0, + **kwargs: Any, + ) -> None: + self._ui = ui + self._args = args + self._kwargs = kwargs + self._before = before + self._deadline = deadline + self._thread = threading.Thread(target=self._run, name="resolve-when-pending", daemon=True) + + def _run(self) -> None: + end = time.monotonic() + self._deadline + while self._ui._pending_approval is None and time.monotonic() < end: + time.sleep(0.001) + if self._before is not None: + self._before() + self._ui.resolve_approval(*self._args, **self._kwargs) + + def start(self) -> None: + self._thread.start() + + def cancel(self) -> None: + self._thread.join(timeout=5) + + +def resolve_when_pending(ui: Any, *args: Any, **kwargs: Any) -> _PendingResolver: + """Build a race-free approval resolver (see :class:`_PendingResolver`).""" + return _PendingResolver(ui, *args, **kwargs) + + if TYPE_CHECKING: - from collections.abc import Iterator + from collections.abc import Callable, Iterator from turnstone.core.mcp_client import MCPClientManager, StaticServerState from turnstone.core.mcp_crypto import MCPTokenCipher diff --git a/tests/test_coord_ui_approve_tools.py b/tests/test_coord_ui_approve_tools.py index e62635e4..5b37db29 100644 --- a/tests/test_coord_ui_approve_tools.py +++ b/tests/test_coord_ui_approve_tools.py @@ -16,10 +16,10 @@ to ``SessionUIBase`` automatically enables: from __future__ import annotations -import threading from typing import Any from unittest.mock import MagicMock, patch +from tests.conftest import resolve_when_pending from turnstone.console.coordinator_ui import ConsoleCoordinatorUI @@ -153,7 +153,7 @@ def test_coord_heuristic_verdict_persists_to_storage() -> None: items[0]["_heuristic_verdict"] = hv storage = MagicMock() - timer = threading.Timer(0.05, lambda: ui.resolve_approval(False)) + timer = resolve_when_pending(ui, False) timer.start() try: with _patch_storage(storage): @@ -246,9 +246,8 @@ def test_coord_pending_approval_sets_activity_tag() -> None: def _capture_activity() -> None: captured["activity"] = ui._ws_current_activity captured["state"] = ui._ws_activity_state - ui.resolve_approval(False) - timer = threading.Timer(0.05, _capture_activity) + timer = resolve_when_pending(ui, False, before=_capture_activity) timer.start() try: with _patch_storage(MagicMock()): @@ -292,7 +291,7 @@ def test_coord_judge_pending_flag_dynamic_when_heuristic_present() -> None: captured_events: list[dict[str, Any]] = [] ui._enqueue = captured_events.append # type: ignore[method-assign] - timer = threading.Timer(0.05, lambda: ui.resolve_approval(False)) + timer = resolve_when_pending(ui, False) timer.start() try: with _patch_storage(MagicMock()): @@ -338,7 +337,7 @@ def test_coord_judge_pending_false_when_no_heuristic_verdict() -> None: captured_events: list[dict[str, Any]] = [] ui._enqueue = captured_events.append # type: ignore[method-assign] - timer = threading.Timer(0.05, lambda: ui.resolve_approval(False)) + timer = resolve_when_pending(ui, False) timer.start() try: with _patch_storage(MagicMock()): @@ -410,7 +409,7 @@ def test_coord_budget_override_prompts_even_under_blanket_auto_approve() -> None captured_events: list[dict[str, Any]] = [] ui._enqueue = captured_events.append # type: ignore[method-assign] - timer = threading.Timer(0.05, lambda: ui.resolve_approval(True)) + timer = resolve_when_pending(ui, True) timer.start() try: with _patch_storage(MagicMock()): @@ -453,7 +452,7 @@ def test_coord_budget_override_survives_wildcard_allow_policy() -> None: captured_events: list[dict[str, Any]] = [] ui._enqueue = captured_events.append # type: ignore[method-assign] - timer = threading.Timer(0.05, lambda: ui.resolve_approval(True)) + timer = resolve_when_pending(ui, True) timer.start() try: with _patch_storage(MagicMock()), _patch_policies({"__budget_override__": "allow"}): diff --git a/tests/test_session_ui_base.py b/tests/test_session_ui_base.py index a4162637..6779dc2f 100644 --- a/tests/test_session_ui_base.py +++ b/tests/test_session_ui_base.py @@ -20,6 +20,7 @@ from unittest.mock import MagicMock, patch import pytest +from tests.conftest import resolve_when_pending from turnstone.core.session_ui_base import SessionUIBase @@ -1832,7 +1833,7 @@ def test_approve_tools_skips_smart_stage_when_disabled() -> None: ui.smart_approval_wait_seconds = 1.0 item = _pending_item("c1") ui.seed_verdicts = [_llm_verdict("c1", recommendation="approve", confidence=0.99)] - timer = threading.Timer(0.05, lambda: ui.resolve_approval(True, "ok")) + timer = resolve_when_pending(ui, True, "ok") timer.start() try: with _patch_get_storage(storage), _patch_policies({}): @@ -1941,7 +1942,7 @@ def test_approve_tools_reemits_verdict_after_card_on_held_batch() -> None: item = _pending_item("c1") ui.seed_verdicts = [_llm_verdict("c1", recommendation="review", confidence=0.99)] lq = ui._register_listener() - timer = threading.Timer(0.1, lambda: ui.resolve_approval(False, "no")) + timer = resolve_when_pending(ui, False, "no") timer.start() try: with _patch_get_storage(storage), _patch_policies({}): @@ -1970,7 +1971,7 @@ def test_judge_pending_true_when_llm_verdict_not_yet_cached() -> None: ui = _make_ui() # smart_approvals_enabled defaults False item = _pending_item("c1") # carries _heuristic_verdict, no cached LLM verdict lq = ui._register_listener() - timer = threading.Timer(0.1, lambda: ui.resolve_approval(True, "ok")) + timer = resolve_when_pending(ui, True, "ok") timer.start() try: with _patch_get_storage(MagicMock()), _patch_policies({}): diff --git a/tests/test_webui_auto_approve_visibility.py b/tests/test_webui_auto_approve_visibility.py index 996c867e..b75d4947 100644 --- a/tests/test_webui_auto_approve_visibility.py +++ b/tests/test_webui_auto_approve_visibility.py @@ -21,12 +21,12 @@ exactly the case the visibility fix is meant to surface. from __future__ import annotations import queue -import threading from typing import Any from unittest.mock import MagicMock, patch import pytest +from tests.conftest import resolve_when_pending from turnstone.server import WebUI @@ -107,11 +107,11 @@ def test_policy_partial_allow_then_prompt_records_allowed_items() -> None: ui = WebUI(ws_id="ws-test") items = _make_items(("c1", "read_file"), ("c2", "bash")) - # ``approve_tools`` blocks on ``_approval_event.wait`` for the - # prompt path. Schedule a deny-by-operator on a tiny timer so - # the wait returns promptly; this test asserts on ring-buffer - # state, not the verdict outcome, so a deny is fine. - timer = threading.Timer(0.05, lambda: ui.resolve_approval(False)) + # ``approve_tools`` blocks on ``_approval_event.wait`` for the prompt + # path. Resolve (deny) once the approval is registered so the wait + # returns promptly without the lost-wakeup race a fixed-delay timer has; + # this test asserts on ring-buffer state, not the verdict, so a deny is fine. + timer = resolve_when_pending(ui, False) timer.start() storage = MagicMock()