From cc295509b412f76785ecf548a7cf0a7d2a4e285f Mon Sep 17 00:00:00 2001 From: Ryan Gu <133598845+ryangu00@users.noreply.github.com> Date: Sun, 6 Sep 2026 15:39:02 -0500 Subject: [PATCH] fix(tirith): half-open the circuit breaker instead of latching it open MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- tools/tirith_security.py | 54 +++++++++++++++++++++++++--------------- 1 file changed, 34 insertions(+), 20 deletions(-) diff --git a/tools/tirith_security.py b/tools/tirith_security.py index e135f3074c..9dad3d80ed 100644 --- a/tools/tirith_security.py +++ b/tools/tirith_security.py @@ -66,19 +66,22 @@ _install_failure_reason: str = "" # reason tag when _resolved_path is _INSTALL_ # ``/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: