fix(approval): clamp approvals.timeout at the config-read chokepoint (#83220)
Widen the salvaged gate-site clamp to the bug class. The gate min() from the contributor PR capped the gate bound at 360s, which would break the #79719 contract (gate must extend while a legitimate >360s approval prompt is answerable) and left the sibling overflow sites live: the CLI prompt thread.join, the gateway poll deadline, and human_wait_ceiling all consume the same config value. Clamp once in _get_approval_timeout() via agent.deadline.MAX_SAFE_TIMEOUT_S (1 year - semantically unbounded, platform-safe). The gate keeps its approvals.timeout-tracking behavior above 360s; 7 regression tests pin lock-acquire/thread-join safety and the gate-extension contract. Bilateral E2E: with approvals.timeout=1e20 in a real config.yaml, main crashes every consumer with OverflowError; this branch survives all 5 probes.
This commit is contained in:
@@ -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
|
||||
|
||||
|
||||
@@ -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
|
||||
+20
-1
@@ -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:
|
||||
|
||||
Reference in New Issue
Block a user