mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-12 23:12:23 -06:00
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.
This commit is contained in:
+53
-1
@@ -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
|
||||
|
||||
@@ -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"}):
|
||||
|
||||
@@ -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({}):
|
||||
|
||||
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user