Merge pull request #86412 from kshitijk4poor/salvage/83225-overflow-clamp

fix(approval): oversized approvals.timeout crashes parallel tool batches — clamp at config read (salvage #83225/#83298, #85125 2b)

agent/tool_executor.py

@@ -154,6 +154,12 @@ def _authorization_gate_lock_timeout() -> float:
     try:
         from tools.approval import human_wait_ceiling
 
+        # 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

contributors/emails/92573950+RelaxJonh@users.noreply.github.com

@@ -0,0 +1 @@
+RelaxJonh

tests/tools/test_approval_timeout_overflow.py

@@ -0,0 +1,138 @@
+"""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_oversized_float_value_clamped(self):
+        # YAML `1e18` arrives as a float, not an int — different int() path
+        # than the string/int forms; the clamp must cover it too.
+        from tools.approval import _get_approval_timeout
+
+        with _with_configured_timeout(1e18):
+            assert _get_approval_timeout() == int(MAX_SAFE_TIMEOUT_S)
+
+    def test_clamp_engagement_logs_warning(self, caplog):
+        # Capping silently changes behavior for every consumer; operators
+        # must see it happen.
+        import tools.approval as approval_mod
+
+        with _with_configured_timeout(10**18):
+            with caplog.at_level("WARNING", logger=approval_mod.__name__):
+                approval_mod._get_approval_timeout()
+        assert "exceeds the platform-safe maximum" in caplog.text
+
+    def test_deadline_import_failure_fails_closed(self, monkeypatch):
+        # If agent.deadline ever fails to import, the clamp must fail CLOSED
+        # (a finite safe cap) — returning the raw value would re-open the
+        # exact time_t overflow this fix exists to prevent.
+        import builtins
+
+        from tools.approval import _get_approval_timeout
+
+        real_import = builtins.__import__
+
+        def _blocked(name, *args, **kwargs):
+            if name == "agent.deadline" or name.startswith("agent.deadline."):
+                raise ImportError("simulated packaging failure")
+            return real_import(name, *args, **kwargs)
+
+        monkeypatch.setattr(builtins, "__import__", _blocked)
+        with _with_configured_timeout(10**18):
+            value = _get_approval_timeout()
+        assert value == 365 * 24 * 3600
+        # Still platform-safe for the crashing primitive.
+        lock = threading.Lock()
+        assert lock.acquire(timeout=value)
+        lock.release()
+
+    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

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,37 @@ 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
+
+        safe_cap = int(MAX_SAFE_TIMEOUT_S)
+    except Exception:
+        # Fail CLOSED: returning the raw value here would re-open the exact
+        # time_t overflow this clamp exists to prevent. ~1 year, matching
+        # agent.deadline.MAX_SAFE_TIMEOUT_S.
+        safe_cap = 365 * 24 * 3600
+    if raw > safe_cap:
+        logger.warning(
+            "approvals.timeout=%s exceeds the platform-safe maximum; "
+            "clamping to %ss",
+            raw,
+            safe_cap,
+        )
+        return safe_cap
+    return raw
 
 
 def _get_cron_approval_mode() -> str: