diff --git a/agent/tool_executor.py b/agent/tool_executor.py index b4f3baed7e..fde07af321 100644 --- a/agent/tool_executor.py +++ b/agent/tool_executor.py @@ -154,12 +154,13 @@ def _authorization_gate_lock_timeout() -> float: try: from tools.approval import human_wait_ceiling - # Clamp to _AUTHORIZATION_GATE_LOCK_TIMEOUT_S: the gate serializes - # parallel dispatch — it should never wait longer than a wedged-holder - # timeout even when approvals.timeout is set very large (or infinite). - # On macOS, unbounded values overflow threading.Lock.acquire's timespec - # and raise OverflowError (#83220). - return min(human_wait_ceiling(), _AUTHORIZATION_GATE_LOCK_TIMEOUT_S) + # human_wait_ceiling is platform-safety-capped (agent/deadline.py + # MAX_SAFE_TIMEOUT_S): a huge approvals.timeout can no longer overflow + # Lock.acquire's time_t on macOS (#83220). Deliberately NOT min()'d + # with _AUTHORIZATION_GATE_LOCK_TIMEOUT_S — the gate must never give + # up while a legitimate approval prompt is still answerable (#79719), + # so a configured approvals.timeout above 360s must extend the gate. + return human_wait_ceiling() except Exception: return _AUTHORIZATION_GATE_LOCK_TIMEOUT_S diff --git a/tests/tools/test_approval_timeout_overflow.py b/tests/tools/test_approval_timeout_overflow.py new file mode 100644 index 0000000000..44759f6d07 --- /dev/null +++ b/tests/tools/test_approval_timeout_overflow.py @@ -0,0 +1,96 @@ +"""Regression tests for #83220: oversized approvals.timeout must never +overflow platform wait primitives (macOS time_t OverflowError). + +The clamp lives at the single config-read site (_get_approval_timeout), so +every consumer — CLI prompt thread.join, gateway poll deadline, human-wait +ceiling, and the tool_executor authorization gate — is covered at once. +""" + +from __future__ import annotations + +import threading + +from unittest.mock import patch + +from agent.deadline import MAX_SAFE_TIMEOUT_S + + +def _with_configured_timeout(value): + return patch( + "tools.approval._get_approval_config", + return_value={"timeout": value}, + ) + + +class TestApprovalTimeoutOverflowClamp: + def test_normal_value_passes_through(self): + from tools.approval import _get_approval_timeout + + with _with_configured_timeout(300): + assert _get_approval_timeout() == 300 + + def test_oversized_value_clamped(self): + from tools.approval import _get_approval_timeout + + with _with_configured_timeout(10**18): + assert _get_approval_timeout() == int(MAX_SAFE_TIMEOUT_S) + + def test_invalid_value_falls_back_to_default(self): + from tools.approval import _get_approval_timeout + + with _with_configured_timeout("soon"): + assert _get_approval_timeout() == 300 + + def test_clamped_value_safe_for_lock_acquire(self): + # The exact primitive that crashed in #83220: Lock.acquire on macOS + # converts the relative timeout to an absolute time_t timestamp. + from tools.approval import _get_approval_timeout + + with _with_configured_timeout(10**18): + timeout = _get_approval_timeout() + lock = threading.Lock() + assert lock.acquire(timeout=timeout) # would raise OverflowError unclamped + lock.release() + + def test_clamped_value_safe_for_thread_join(self): + # Sibling crash site: the CLI prompt fallback joins the input thread + # with the configured timeout (tools/approval.py get_input path). + from tools.approval import _get_approval_timeout + + with _with_configured_timeout(10**18): + timeout = _get_approval_timeout() + t = threading.Thread(target=lambda: None) + t.start() + t.join(timeout=timeout) # would raise OverflowError unclamped + assert not t.is_alive() + + def test_human_wait_ceiling_inherits_clamp(self): + from tools.approval import HUMAN_WAIT_MARGIN_S, human_wait_ceiling + + with _with_configured_timeout(10**18): + ceiling = human_wait_ceiling() + assert ceiling == float(int(MAX_SAFE_TIMEOUT_S)) + HUMAN_WAIT_MARGIN_S + lock = threading.Lock() + assert lock.acquire(timeout=ceiling) + lock.release() + + def test_authorization_gate_timeout_safe_and_extends_with_config(self): + # The gate bound must (a) be platform-safe with an oversized config + # and (b) still EXTEND beyond the 360s fallback when approvals.timeout + # is legitimately larger — clamping it down to the fallback would + # break serialization while a real prompt is still answerable (#79719). + from agent.tool_executor import ( + _AUTHORIZATION_GATE_LOCK_TIMEOUT_S, + _authorization_gate_lock_timeout, + ) + + with _with_configured_timeout(10**18): + bound = _authorization_gate_lock_timeout() + lock = threading.Lock() + assert lock.acquire(timeout=bound) + lock.release() + + with _with_configured_timeout(3600): + bound = _authorization_gate_lock_timeout() + assert bound > _AUTHORIZATION_GATE_LOCK_TIMEOUT_S + assert bound == 3600 + 60.0 # approvals.timeout + HUMAN_WAIT_MARGIN_S diff --git a/tools/approval.py b/tools/approval.py index 775aa85972..b8fbd2988a 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -2588,6 +2588,11 @@ def human_wait_ceiling() -> float: authorization gate's serialization-lock acquire, so the two bounds cannot drift. Never call while holding ``_human_wait_lock`` — it reads the config cache. + + Platform safety: ``_get_approval_timeout`` caps at + ``agent.deadline.MAX_SAFE_TIMEOUT_S``, so this value is always safe to + hand to ``Lock.acquire(timeout=...)`` / ``Thread.join(timeout=...)`` + (#83220 macOS time_t overflow). """ return float(_get_approval_timeout()) + HUMAN_WAIT_MARGIN_S @@ -3430,11 +3435,25 @@ def _get_approval_timeout() -> int: approvals arrive as push notifications the user may not see for a couple of minutes; 60s proved too tight in practice (Telegram taps landed after the wait had already failed closed). + + Clamped to ``agent.deadline.MAX_SAFE_TIMEOUT_S`` (1 year — semantically + unbounded): a very large configured value overflows ``time_t`` inside + ``Thread.join(timeout=...)`` / ``Lock.acquire(timeout=...)`` on macOS, + and before this clamp a single oversized ``approvals.timeout`` crashed + every parallel tool batch with OverflowError (#83220). Clamping at the + single config-read site keeps every consumer (prompt join, gateway poll + deadline, human-wait ceiling, authorization gate) platform-safe at once. """ try: - return int(_get_approval_config().get("timeout", 300)) + raw = int(_get_approval_config().get("timeout", 300)) except (ValueError, TypeError): return 300 + try: + from agent.deadline import MAX_SAFE_TIMEOUT_S + + return min(raw, int(MAX_SAFE_TIMEOUT_S)) + except Exception: + return raw def _get_cron_approval_mode() -> str: