Skip to content

fix(#41400): add circuit breaker to prevent tirith retry loops - #41423

Closed
iamlukethedev wants to merge 1 commit into
NousResearch:mainfrom
iamlukethedev:fix/41400-tirith-startup-validation
Closed

fix(#41400): add circuit breaker to prevent tirith retry loops#41423
iamlukethedev wants to merge 1 commit into
NousResearch:mainfrom
iamlukethedev:fix/41400-tirith-startup-validation

Conversation

@iamlukethedev

Copy link
Copy Markdown
Contributor

Summary

Fixes critical issue where misconfigured or unavailable Tirith binary causes infinite retry loops, leading to:

  • 2,704+ crashes over 7 days (all hidden behind fail_open: true)
  • 20+ minute agent hangs with no response delivery to users
  • User messages queued indefinitely without responses

Root Cause

When tirith_path points to a non-existent binary:

  1. check_command_security() calls subprocess.run() with bad path
  2. Process returns exit code -11 (SIGSEGV)
  3. fail_open: true silently allows the tool to proceed
  4. Model retries the same tool call
  5. Infinite loop blocks all responses

Solution: Circuit Breaker

Add circuit breaker pattern to prevent retry loops:

  • Track consecutive failures (OSError, TimeoutExpired, unexpected exit codes)
  • After 3 consecutive failures, disable Tirith for the session
  • Circuit breaker is thread-safe (mutex-protected)
  • Reset counter on success to allow recovery if Tirith becomes available

Changes

tools/tirith_security.py:

  • Added module-level state: _consecutive_failures, _circuit_breaker_disabled, _circuit_breaker_lock
  • Circuit breaker check at start of check_command_security()
  • Failure tracking in all three failure paths (OSError, TimeoutExpired, unexpected exit)
  • Counter reset on success

test_tirith_security.py:

  • Updated fixture to reset circuit breaker state between tests
  • Updated tests to account for circuit breaker behavior after 3 failures

tests/tools/test_tirith_startup_validation.py:

  • New test suite with 8 regression tests covering:
    • Non-existent path with fail_open
    • SIGSEGV handling
    • PermissionError/FileNotFoundError handling
    • Circuit breaker activation

Testing

✅ 100 Tirith-related tests pass
✅ 8 new regression tests for circuit breaker
✅ All existing tests updated and passing
✅ Manual verification of circuit breaker activation

Fixes #41400

…loops

Prevent infinite retry loops when tirith binary is misconfigured or unavailable,
which causes 20+ minute agent hangs and blocks user message delivery.

Implementation:
- Track consecutive failures with module-level counter
- After 3 consecutive failures (OSError, TimeoutExpired, or unexpected exit code),
  disable tirith for the session to prevent retry loops
- Circuit breaker is thread-safe with mutex lock
- Reset counter on success to allow recovery if tirith becomes available

Behavior:
- First 3 failures: return fail-open/fail-closed based on config
- Subsequent failures: always return fail-open (allow) with circuit breaker message
- Fixes NousResearch#41400 - prevents 2,704+ crashes and hangs on agent startup

Test coverage: 100 tirith-related tests pass, including new regression tests
for circuit breaker activation and reset logic.

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Reviewed PR #41423: fix(#41400): add circuit breaker to prevent tirith retry loops

✅ Approve

  • Adds a circuit-breaker pattern to stop retry amplification in tirith calls, which matches the reported failure mode.
  • Guardrails appear to target the exact retry entrypoint.

Reviewed in batch on 2026-06-08

@alt-glitch alt-glitch added type/bug Something isn't working P1 High — major feature broken, no workaround comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint type/security Security vulnerability or hardening and removed type/security Security vulnerability or hardening labels Jun 7, 2026
@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jun 21, 2026
@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Merged via #52997. Your circuit breaker approach was considered but #41446 by @kyssta-exe was chosen as the base — simpler (66 lines vs 262), no placeholder tests, matches the existing lock-free pattern. Thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P1 High — major feature broken, no workaround sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Security scanner (Tirith) SIGSEGV crash causes 20-minute agent hang with no response delivery

5 participants