diff --git a/tests/tools/test_approval_gateway_wait_late_choice.py b/tests/tools/test_approval_gateway_wait_late_choice.py index 22e9c2abda..b6e901ae7d 100644 --- a/tests/tools/test_approval_gateway_wait_late_choice.py +++ b/tests/tools/test_approval_gateway_wait_late_choice.py @@ -47,3 +47,46 @@ def test_plain_timeout_still_reports_unresolved(monkeypatch): assert decision == {"resolved": False, "choice": None, "reason": None} # Nothing is left for a late /approve to hit: the client learns nothing was pending. assert mod.resolve_gateway_approval(SESSION_KEY, "once") == 0 + + +def test_resolve_commits_the_choice_before_releasing_the_approval_lock(monkeypatch): + """``_drop_entry`` reads ``entry.result`` and leaves the queue in one ``_lock`` section, which only + closes the race if ``resolve_gateway_approval`` commits ``result``/``event`` INSIDE the section that + pops the entry. A commit after the lock is released is a window where the waiter pops-and-loses an + acked choice as a timeout (#112548).""" + _clear() + entry = wait_mod._ApprovalEntry(APPROVAL) + mod._gateway_queues[SESSION_KEY] = [entry] + seen: dict = {} + real_lock = mod._lock + + class _Instrumented: + def __enter__(self): + return real_lock.__enter__() + + def __exit__(self, *exc): + seen["result_at_release"], seen["event_at_release"] = entry.result, entry.event.is_set() + return real_lock.__exit__(*exc) + + monkeypatch.setattr(mod, "_lock", _Instrumented()) + assert mod.resolve_gateway_approval(SESSION_KEY, "once", reason="fine") == 1 + assert seen == {"result_at_release": "once", "event_at_release": True} + assert entry.reason == "fine" + + +def test_withdrawn_entry_settles_with_a_wire_reason(monkeypatch): + """A wait woken with no choice (session teardown) withdraws its open request with a RequestCancelReason, + not the poll-state token ``"set"``.""" + _clear() + monkeypatch.setattr(wait_mod._ctx, "_fire_approval_hook", lambda name, **kw: None) + reasons: list[str] = [] + + def torn_down(event, session_key, *, interrupt_log): + mod.register_gateway_settle(session_key, mod._gateway_queues[session_key][0].data["request_id"], reasons.append) + mod.clear_session(session_key) + return "set" + + monkeypatch.setattr(wait_mod, "_poll_event", torn_down) + decision = wait_mod._await_gateway_decision(SESSION_KEY, lambda data: None, APPROVAL) + assert decision["choice"] is None and decision["cancelled"] + assert reasons == ["session_closed"] diff --git a/tests/tui_gateway/test_protocol.py b/tests/tui_gateway/test_protocol.py index 966a8bae45..d64edd29dc 100644 --- a/tests/tui_gateway/test_protocol.py +++ b/tests/tui_gateway/test_protocol.py @@ -30,6 +30,12 @@ def server(): # e.g. hermes_cli.active_sessions would bind the mocked get_hermes_home # (a fixed shared path) forever, leaking active-session registry entries # across every later test in the process. Scope the patch to the import. + # + # Import server_requests (pure stdlib) BEFORE the window: the patch drops every module first imported + # inside it, so otherwise the module server.py binds its sinks on (write/emit/answerable) would vanish + # from sys.modules and a test's own ``from tui_gateway import server_requests`` would get a fresh, + # unbound copy whose default sinks drop frames and treat every client as answerable. + import tui_gateway.server_requests # noqa: F401 with patch.dict("sys.modules", { "hermes_constants": MagicMock(get_hermes_home=MagicMock(return_value="/tmp/hermes_test")), "hermes_cli.env_loader": MagicMock(), @@ -433,6 +439,23 @@ def test_server_request_waits_for_a_ws_client_that_advertised(server): assert server_requests.answers_requests(peer) is False +def test_server_request_error_response_fails_fast(server): + """A client that advertised but has no handler for the method answers -32601: that error frame settles + send() to None at once instead of the agent waiting for the deadline.""" + from tui_gateway import server_requests + + box = {} + thread = threading.Thread(target=lambda: box.setdefault("result", server_requests.send("sudo", "s1", {}, timeout=5)), + daemon=True) + thread.start() + req = _wait_open(server_requests) + t0 = time.monotonic() + assert server_requests.resolve_response({"id": req.id, "error": {"code": -32601}}) + thread.join(timeout=1) + assert not thread.is_alive() and time.monotonic() - t0 < 1 + assert box["result"] is None + + @pytest.mark.parametrize("method", ["secret", "sudo", "terminal.read", "tour"]) def test_server_request_timeout_emits_one_request_cancel(capture, method): from tui_gateway import server_requests @@ -1545,3 +1568,30 @@ def test_unregister_live_transport_stops_delivery(capture): assert a.frames == [] # No live transports left → fell back to stdio. assert json.loads(buf.getvalue())["params"]["type"] == "skin.changed" + + +def test_approval_for_a_ws_client_that_never_advertised_settles_the_queue_entry(server, monkeypatch): + """The approval wait is owned by ``tools.approval``'s queue, not by ``server_requests``. When the request + cannot be sent (the only client predates server→client requests) the queue entry must be withdrawn too, + otherwise ``_await_gateway_decision`` idles for the whole approvals.timeout with no prompt anywhere + (#112548). The decision is a withdrawal (``cancelled`` cause), never a user deny.""" + from tools import approval as approval_mod + from tools import approval_gateway_wait as wait_mod + + peer = _silent_ws() + _ws_session(server, "ws-old-approval", peer) + monkeypatch.setattr(wait_mod._ctx, "_get_approval_timeout", lambda: 3) + monkeypatch.setattr(wait_mod._ctx, "_fire_approval_hook", lambda name, **kw: None) + approval_mod.register_gateway_notify("ws-old-approval", lambda data: server._emit_approval_request("ws-old-approval", data)) + try: + t0 = time.monotonic() + decision = wait_mod._await_gateway_decision( + "ws-old-approval", approval_mod._gateway_notify_cbs["ws-old-approval"], + {"command": "rm -rf build", "description": "", "pattern_key": "dangerous", "pattern_keys": ["dangerous"]}) + waited = time.monotonic() - t0 + finally: + approval_mod.unregister_gateway_notify("ws-old-approval") + assert waited < 1, decision + assert decision["choice"] is None and decision["cancelled"] + assert peer.frames == [] + assert "ws-old-approval" not in approval_mod._gateway_queues diff --git a/tools/approval.py b/tools/approval.py index 8f482df852..984470a7bd 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -131,9 +131,8 @@ def unregister_gateway_notify(session_key: str) -> None: they don't hang forever (agent run finished or interrupted).""" with _lock: _gateway_notify_cbs.pop(session_key, None) - entries = _gateway_queues.pop(session_key, []) - for entry in entries: - entry.event.set() + for entry in _gateway_queues.pop(session_key, []): + entry.event.set() def resolve_gateway_approval(session_key: str, choice: str, @@ -162,15 +161,34 @@ def resolve_gateway_approval(session_key: str, choice: str, targets = [queue.pop(0)] if not queue: _gateway_queues.pop(session_key, None) - - for entry in targets: - entry.result = choice - if reason: - entry.reason = reason - entry.event.set() + # Popping the entry and committing its outcome are ONE critical section: the waiter's + # ``_drop_entry`` reads ``entry.result`` under this same lock after its deadline check, so a + # choice acked to the client here can never be popped-and-lost as a timeout (#112548). + for entry in targets: + entry.result = choice + if reason: + entry.reason = reason + entry.event.set() return len(targets) +def withdraw_gateway_approval(session_key: str, request_id: str, cause: str) -> bool: + """Withdraw one pending approval nobody can answer (the only attached client cannot render it). + The waiter wakes at once with ``cancelled=cause`` — a withdrawal, never a user deny — instead of + idling for the whole approvals.timeout (#112548). False when it is no longer pending.""" + with _lock: + queue = _gateway_queues.get(session_key, []) + entry = next((e for e in queue if e.data.get("request_id") == request_id), None) + if entry is None: + return False + queue.remove(entry) + if not queue: + _gateway_queues.pop(session_key, None) + entry.cancelled = cause + entry.event.set() + return True + + def list_gateway_approvals(session_key: str) -> list[dict]: """Return replay-safe snapshots of unresolved approvals for one session.""" with _lock: @@ -272,12 +290,11 @@ def clear_session(session_key: str) -> None: _session_approved.pop(session_key, None) _session_yolo.discard(session_key) _pending.pop(session_key, None) - entries = _gateway_queues.pop(session_key, []) - for entry in entries: - # Cancel blocked waits now so the old run unwinds instead of idling until timeout; - # the prompt was withdrawn, nobody denied it. - entry.cancelled = "the session ended before the prompt was answered" - entry.event.set() + for entry in _gateway_queues.pop(session_key, []): + # Cancel blocked waits now so the old run unwinds instead of idling until timeout; + # the prompt was withdrawn, nobody denied it. + entry.cancelled = "the session ended before the prompt was answered" + entry.event.set() _release_permission_mode_dependents(session_key) # Session-persistent code kernels (local and remote) share this owner key and die at the same boundary so a # finished conversation cannot leak a live interpreter. diff --git a/tools/approval_gateway_wait.py b/tools/approval_gateway_wait.py index d7d55801d3..c564997e81 100644 --- a/tools/approval_gateway_wait.py +++ b/tools/approval_gateway_wait.py @@ -181,8 +181,15 @@ def _await_gateway_decision(session_key: str, notify_cb, approval_data: dict, *, _approval._gateway_queues.pop(session_key, None) settle, entry.settle = entry.settle, None if settle is not None: + # ``request.cancel`` carries a RequestCancelReason: a choice committed from another surface is + # ``resolved``; a withdrawn entry (woken with no choice — session torn down, turn ended, client + # cannot answer) is ``session_closed``; never the raw poll-state token "set". + if state == "set": + reason = "resolved" if choice is not None else "session_closed" + else: + reason = state try: - settle("answered" if choice is not None and state != "interrupted" else state) + settle(reason) except Exception: logger.debug("approval settle hook failed", exc_info=True) return choice diff --git a/tui_gateway/server.py b/tui_gateway/server.py index 59313a4774..561ce477bb 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -747,7 +747,15 @@ def _emit_approval_request(sid: str, data: dict | None) -> None: session_key = str((_sessions.get(sid) or {}).get("session_key") or "") def on_result(result: dict | None) -> None: - if result is None: # withdrawn: the queue entry resolves on its own path + if result is None: + # No client can answer this prompt: the request was never sent (the only attached client predates + # server→client requests) or the client answered -32601 (no handler). Without withdrawing the + # queue entry the agent would idle for the whole approvals.timeout with no prompt anywhere + # (#112548). A withdrawal, not a deny: nobody refused the command. + if request_id: + _approval.withdraw_gateway_approval(session_key, request_id, + "the attached client cannot answer approval requests " + "(update the Hermes app)") return choice = str(result.get("choice") or "deny") _approval.resolve_gateway_approval(session_key, choice, resolve_all=bool(result.get("all")),