fix(tirith): half-open the circuit breaker instead of latching it open
After _CRASH_LIMIT consecutive spawn/execution failures the breaker opened and never closed again: check_command_security returns early while _circuit_open is set, so the reset below it was unreachable for the rest of the process. A transient failure (a slow disk during install, a binary being replaced) meant every later command silently skipped scanning until restart — a security control that fails open permanently, not temporarily. The breaker now half-opens after _CIRCUIT_RETRY_S: exactly one caller claims the probe slot and runs a real scan. Claiming re-arms _circuit_open_at under _breaker_lock first, so concurrent callers see a fresh TTL and stay on the fail-open path — single-flight, one probe per TTL window. The lock guards only the claim (a TTL comparison and a timestamp write); it is never held across the subprocess, so it cannot reintroduce the #41400 hang. Crash counting stays lock-free, matching the mcp_tool.py error counters. Any completed scan closes the breaker. exit 0/1/2 are all verdicts — allow/block/warn — and all three prove the binary is healthy, so recovery no longer depends on the next command happening to be clean. That also fixes the streak never resetting on block/warn. time.monotonic() throughout, matching the MCP breaker: a wall-clock jump backwards must not strand the breaker open past its TTL. Rebased onto current main; the file was reformatted upstream in the tools/ compaction wave, so the change is re-expressed in the current structure (_verdict / _EXIT_ACTIONS). The latch itself is unchanged upstream. (cherry picked from commit 2ff4ee6c745060e9aaf4f4ee2d04ac68ba55b2e8)
This commit is contained in:
@@ -66,19 +66,22 @@ _install_failure_reason: str = "" # reason tag when _resolved_path is _INSTALL_
|
||||
# ``<home>/bin/tirith`` are per profile, so the launch profile's slot above must not answer for them.
|
||||
_resolved_path_by_home: dict[str, str] = {}
|
||||
|
||||
# Circuit breaker: after _CRASH_LIMIT consecutive spawn/execution failures tirith is disabled
|
||||
# for the rest of the process so a broken binary can't turn every tool call into a fail-open
|
||||
# retry loop. Reset on success. Lock-free on purpose: a racing double-increment only opens the
|
||||
# breaker one call early; no corruption or security bypass is possible.
|
||||
# Circuit breaker: after _CRASH_LIMIT consecutive spawn/execution failures tirith is disabled so a broken
|
||||
# binary can't turn every tool call into a fail-open retry loop (#41400). The breaker HALF-OPENS after
|
||||
# _CIRCUIT_RETRY_S: one caller re-probes tirith for real, and any completed scan (exit 0/1/2 — allow/block/warn
|
||||
# all prove the binary is healthy) closes it, while a failed probe re-arms the timer. Without the TTL this was
|
||||
# a one-way latch: once open, the reset branch below was unreachable for the rest of the process.
|
||||
# Thread safety: crash counting stays lock-free — a racing double-increment only opens the breaker one call
|
||||
# early, which is harmless, and matches the mcp_tool.py error counters rather than the locked _warn_once
|
||||
# pattern. _breaker_lock guards ONLY the half-open claim (TTL check + timestamp re-arm, nanoseconds); it is
|
||||
# never held across the subprocess probe, so it cannot reintroduce the #41400 hang. Claiming re-arms
|
||||
# _circuit_open_at first, so concurrent callers see a fresh TTL and stay fail-open: one probe per TTL window.
|
||||
_CRASH_LIMIT = 3
|
||||
# Reset on successful execution (see _record_tirith_crash / check_command_security). Thread safety:
|
||||
# _crash_count and _circuit_open are module-level globals mutated without a lock. check_command_security can
|
||||
# be called from concurrent agent threads (gateway multi-session). The race is benign — at worst two threads
|
||||
# both increment past _CRASH_LIMIT and both set _circuit_open = True, opening the breaker one call early.
|
||||
# This intentionally matches the lock-free style of error counters in mcp_tool.py rather than the locked
|
||||
# _warn_once pattern, because the worst case is harmless. See #41400.
|
||||
_CIRCUIT_RETRY_S = 300 # half-open probe interval (seconds)
|
||||
_crash_count: int = 0
|
||||
_circuit_open: bool = False
|
||||
_circuit_open_at: float = 0.0
|
||||
_breaker_lock = threading.Lock()
|
||||
|
||||
_install_lock = threading.Lock()
|
||||
_install_thread: threading.Thread | None = None
|
||||
@@ -92,12 +95,12 @@ _MARKER_TTL = 86400 # disk failure marker validity (24h) -- avoids retry across
|
||||
|
||||
|
||||
def _record_tirith_crash() -> None:
|
||||
global _crash_count, _circuit_open
|
||||
global _crash_count, _circuit_open, _circuit_open_at
|
||||
_crash_count += 1
|
||||
if _crash_count >= _CRASH_LIMIT:
|
||||
_circuit_open = True
|
||||
_circuit_open, _circuit_open_at = True, time.monotonic()
|
||||
logger.warning("tirith circuit breaker opened after %d consecutive failures; "
|
||||
"disabling for the rest of the process", _crash_count)
|
||||
"disabling for %ds", _crash_count, _CIRCUIT_RETRY_S)
|
||||
|
||||
|
||||
def _warn_once(key: str, message: str, *args) -> None:
|
||||
@@ -509,15 +512,21 @@ def _crash(fail_open: bool, open_summary: str, closed_summary: str) -> dict:
|
||||
def check_command_security(command: str) -> dict:
|
||||
"""Run the tirith scan on a command -> ``{"action": allow|warn|block, "findings", "summary"}``.
|
||||
Exit code determines the action; JSON enriches. Spawn failures/timeouts respect fail_open."""
|
||||
global _crash_count
|
||||
global _crash_count, _circuit_open, _circuit_open_at
|
||||
cfg = _load_security_config()
|
||||
if not cfg["tirith_enabled"]:
|
||||
return _verdict("allow")
|
||||
# Circuit breaker: if tirith has crashed _CRASH_LIMIT times in a row, stop trying for the rest of the
|
||||
# process. Without this, a corrupted or missing binary causes every tool call to hit the same spawn
|
||||
# failure → fail-open → agent retry loop, hanging the user for 20+ minutes (issue #41400).
|
||||
# Circuit breaker: if tirith has crashed _CRASH_LIMIT times in a row, stop trying and fail open (issue
|
||||
# #41400). After _CIRCUIT_RETRY_S the breaker half-opens: exactly one caller claims the probe slot —
|
||||
# claiming re-arms _circuit_open_at under _breaker_lock, so concurrent callers see a fresh TTL and stay
|
||||
# fail-open — and falls through to a real scan below.
|
||||
if _circuit_open:
|
||||
return _verdict("allow", "tirith disabled (circuit breaker)")
|
||||
with _breaker_lock:
|
||||
if _circuit_open and time.monotonic() - _circuit_open_at < _CIRCUIT_RETRY_S:
|
||||
return _verdict("allow", "tirith disabled (circuit breaker)")
|
||||
if _circuit_open: # TTL expired: claim the single-flight probe slot for this window
|
||||
_circuit_open_at = time.monotonic()
|
||||
logger.info("tirith circuit breaker half-open: probing after %ds", _CIRCUIT_RETRY_S)
|
||||
# No binary for this platform, ever: skip the resolver so we never spawn.
|
||||
if not is_platform_supported():
|
||||
return _verdict("allow")
|
||||
@@ -546,8 +555,13 @@ def check_command_security(command: str) -> dict:
|
||||
logger.warning("tirith returned unexpected exit code %d", exit_code)
|
||||
return _crash(fail_open, f"tirith exit code {exit_code} (fail-open)",
|
||||
f"tirith exit code {exit_code} (fail-closed)")
|
||||
if action == "allow":
|
||||
_crash_count = 0 # successful execution resets the circuit breaker
|
||||
# Any completed scan (allow/block/warn) proves the binary is healthy: clear the streak and close the
|
||||
# breaker. This is the half-open probe's recovery path, and it also fixes the streak never resetting on
|
||||
# block/warn verdicts.
|
||||
_crash_count = 0
|
||||
if _circuit_open:
|
||||
_circuit_open, _circuit_open_at = False, 0.0
|
||||
logger.info("tirith circuit breaker closed after successful probe")
|
||||
# JSON enriches findings/summary; a parse failure never changes the verdict.
|
||||
findings, summary = [], ""
|
||||
try:
|
||||
|
||||
Reference in New Issue
Block a user