fix(agent): recover primary before fast transport fallback

[WallaceNodded]

What does this PR do?

Separates fast connection failures from stale-stream failures at the eager-fallback gate.

Ordinary APIConnectionError / timeout failures now complete the existing retry cycle, rebuild the primary client through _try_recover_primary_transport(), and receive one fresh primary cycle before fallback. Provider overload and stale-detector-derived timeouts retain the bounded eager fallback behavior introduced for #22277.

This reuses existing retry, recovery, and fallback machinery. It adds no config key, environment variable, or provider-specific branch.

Related Issue

Fixes #69186

Related: #22277, #53911

Type of Change

• 🐛 Bug fix (non-breaking change that fixes an issue)
• ✨ New feature (non-breaking change that adds functionality)
• 🔒 Security fix
• 📝 Documentation update
• ✅ Tests (adding or improving test coverage)
• ♻️ Refactor (no behavior change)
• 🎯 New skill (bundled or hub)

Changes Made

• agent/conversation_loop.py
  • Keeps rate-limit/billing/upstream-rate-limit eager fallback unchanged.
  • Keeps overloaded eligible for bounded eager fallback.
  • Uses the existing stale-stream streak to keep stale timeouts eligible for bounded eager fallback.
  • Lets non-stale connection errors reach the existing retry/rebuild/retry/fallback path.
• tests/run_agent/test_fast_transport_recovery_before_fallback.py
  • Proves three fast connection failures recover the primary before fallback.
  • Proves a stale-detector-derived timeout still falls back after one retry.

Why this shape?

#22277 correctly fixed a different failure mode: each stale-stream attempt can consume 180–300 seconds, so exhausting every retry cycle can leave a configured fallback idle for 15+ minutes. A normal SDK/socket connection error often returns in seconds and is exactly what the existing primary-client rebuild is designed to recover.

Both currently classify as FailoverReason.timeout, so the eager gate loses the provenance and activates fallback on attempt two. The stale-stream streak already records that provenance; consulting it keeps #22277 intact without adding another retry mechanism.

How to Test

python -m pytest \
  tests/run_agent/test_fast_transport_recovery_before_fallback.py \
  tests/run_agent/test_32646_fallback_429_after_timeout.py \
  tests/run_agent/test_primary_runtime_restore.py \
  tests/run_agent/test_stream_stale_circuit_breaker.py \
  tests/run_agent/test_stream_stale_breaker_reset.py \
  tests/run_agent/test_provider_fallback.py \
  tests/run_agent/test_fallback_reasoning_override.py \
  tests/agent/test_error_classifier.py -q

Result: 281 passed on Python 3.11.15 / macOS Apple Silicon.

ruff check agent/conversation_loop.py tests/run_agent/test_fast_transport_recovery_before_fallback.py also passes.

The canonical full-suite runner was attempted. Before it was stopped, 7,200+ tests passed and three tests in tests/agent/test_anthropic_adapter.py::TestRunOauthSetupToken failed because the mocked subprocess.run().stdout reached json.loads() as a MagicMock. The same 3 failed, 175 passed result reproduces on a clean upstream/main worktree at 9ecacd6bf, so it is unrelated to this change.

Checklist

Code

• I've read the Contributing Guide
• My commit messages follow Conventional Commits
• I searched for existing issues and PRs to make sure this isn't a duplicate
• My PR contains only changes related to this fix
• I've run pytest tests/ -q and all tests pass (see documented upstream baseline failure above)
• I've added tests for my changes
• I've tested on macOS / Apple Silicon / Python 3.11.15

Documentation & Housekeeping

• Documentation update: N/A; no user-facing config or interface changed
• cli-config.yaml.example: N/A; no config keys changed
• CONTRIBUTING.md / AGENTS.md: N/A; no architecture or workflow changed
• Cross-platform impact considered; the change is platform-independent Python logic
• Tool descriptions/schemas: N/A; no tools changed

Screenshots / Logs

Redacted production sequence from an openai-codex primary:

API call failed (attempt 1/3) error_type=APIConnectionError
Retrying API call ... (attempt 1/3)
API call failed (attempt 2/3) error_type=APIConnectionError
Fallback activated: gpt-5.6-terra -> deepseek-v4-flash

There was no stale-stream warning and no primary-recovery event before fallback.

[PRATHAMESH75]

Reviewed against #69186 — this looks correct and complete.

Root cause & fix. Splitting the old _is_transport_failure (timeout ∪ overloaded) so only overloaded and stale-derived timeouts stay in the eager gate is exactly right. An ordinary APIConnectionError classifies as FailoverReason.timeout with _consecutive_stale_streams == 0, so it now falls through to the retry_count >= max_retries → _try_recover_primary_transport() cycle instead of switching providers on the 2nd failure — precisely the behavior the issue asks for.

#22277 not regressed — verified. All three stale-kill sites (chat_completion_helpers.py:934 / 2404 / 3683) call _bump_stale_streak(agent) before raising, so by the time this fallback gate reads _consecutive_stale_streams, a genuine stale kill has already bumped it to ≥1 → _is_stale_timeout stays True → the bounded eager fallback is preserved. Both branches are covered by the new regression test (test_fast_connection_errors_recover_primary_before_fallback + test_stale_stream_timeout_keeps_bounded_eager_fallback).

One minor, non-blocking note: _consecutive_stale_streams resets only on a completed call or a provider swap, so a stale kill followed by an ordinary connection error within the same retry cycle would still be classified eager (counter still >0). That's arguably the desired behavior for an already-degraded session — just worth a sentence in the code comment if you want it explicit. No new config key, matches the issue's stated constraints. Nice fix.

[AIalliAI]

Spent some time verifying this at current main (0157180). The failure is real and this fixes it: test_fast_connection_errors_recover_primary_before_fallback fails on a clean checkout with the premature cascade (primary-model → deepseek-chat on attempt 2/3) and passes with the patch. 316 tests across the fallback, stale-breaker, and error-classifier suites stay green, and the diff applies cleanly despite ~890 lines of drift since the base.

One gap in the provenance invariant worth fixing before this merges: the two Codex watchdog kill sites in interruptible_api_call — the TTFB kill and the stream-idle kill in agent/chat_completion_helpers.py — both break before reaching _bump_stale_streak(agent), so _consecutive_stale_streams stays 0 for exactly those timeouts. I confirmed empirically: after either kill the streak is still 0, and classify_api_error maps both messages to FailoverReason.timeout. Under this PR as written, those two cases lose the bounded eager fallback and only fail over after full max_retries — on openai-codex, the provider from the motivating log. Bumping the streak in both watchdog kill paths (mirroring the generic stale-kill site) closes the gap; as a bonus it also lets _check_stale_giveup actually see persistent Codex stalls, which it currently never does since neither watchdog increments the streak.

[WallaceNodded]

Addressed in 331c6c9. Both Codex watchdog kill paths now bump the stale streak before returning to the retry loop, preserving bounded eager fallback and stale-breaker provenance. Added assertions for both TTFB and stream-idle kills. Verified against the latest upstream/main state: 32 targeted fallback/watchdog/stale-breaker tests passed, and Ruff passed.

[teknium1]

Thanks for the focused recovery-path fix. The premise remains present on current main: agent/conversation_loop.py:4246-4265 treats all timeout-classified transport errors as eager-fallback candidates on retry two, while primary recovery only occurs at agent/conversation_loop.py:5067-5088 after retry exhaustion. agent/error_classifier.py:517,957-958 confirms that APIConnectionError enters that timeout class.

The updated PR also addresses the watchdog provenance gap: current main's Codex TTFB and idle branches break at agent/chat_completion_helpers.py:994 and :1033 without the stale-counter increment used by the generic stale path at :1071-1073; commit 331c6c9a01b8 adds both increments and matching assertions.

The approach preserves the bounded stale-stream behavior introduced by c946e6709f for #22277, adds no configuration or tool surface, and supplies behavioral regression coverage for both recovery and stale fallback branches.

Automated hermes-sweeper review.

[WallaceNodded]

Synced the branch with current upstream/main (v0.19.1) and resolved the test-file conflict without changing the fix scope. The effective PR diff remains limited to the recovery/fallback gate, Codex watchdog stale provenance, and regression coverage. Verification on the updated head: 172 targeted fallback/watchdog/stale-breaker/streaming tests passed, and Ruff passed for all PR-touched Python files.

[GottZ]

This was generated by AI during triage.

Summary

Two PRs address #69186 by separating ordinary timeout-classified APIConnectionError failures from stale-stream failures so primary transport recovery remains reachable. #69190 also records stale provenance in both Codex watchdog paths and adds regression coverage, whereas #69419 changes only the fallback gate and uses a different stale-stream threshold.

Related pull requests

• fix(agent): recover primary before fast transport fallback #69190 best fix — (+226/-23) — n/a: The diff lets ordinary connection failures complete the primary retry/rebuild cycle while retaining the two-failure eager-fallback gate for overload and stale-derived timeouts; it also adds stale-counter bumps and assertions for both Codex watchdog exits plus recovery/fallback regression tests. Consistent with the visible keep_open review, the salvageable scope is the recovery gate, watchdog provenance fix, and associated tests.
• fix: distinguish stale-stream from APIConnectionError in fallback gate (#69186) #69419 [closed] partial — (+13/-5) — n/a: The diff also removes ordinary connection failures from eager fallback, but changes stale-stream fallback to retry_count >= 1 rather than fix(agent): recover primary before fast transport fallback #69190's retained retry_count >= 2 and adds no tests or watchdog provenance changes. It remains relevant despite being closed as a superseded alternative whose distinct threshold policy was explicitly identified by a contributor.

Duplicates

#69190 and #69419 substantially overlap on the primary-recovery change, but they are not exact duplicates: #69419 uses a one-retry stale-stream threshold, while #69190 retains the existing two-failure threshold and additionally covers Codex watchdog provenance and regression tests.

Suggested consolidation

Keep #69190 open with a salvage path focused on its recovery/fallback gate, both Codex watchdog stale-provenance increments, and the accompanying regression coverage, in line with the visible keep_open verdict. Keep #69419 closed as superseded by #69190 rather than labeling it an exact duplicate, because its contributor-noted one-retry stale-stream policy differs from #69190.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I69186(["issue #69186 (open)"])
    P69190["PR #69190 (open)"]
    P69190 -->|best fix| I69186
    class I69186 open
    class P69190 open
    class P69190 best
    class P69190 target
    click I69186 "https://github.com/NousResearch/hermes-agent/issues/69186"
    click P69190 "https://github.com/NousResearch/hermes-agent/pull/69190"

Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 2 pull requests and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 16 kB of PR diffs, 12 kB of issue/PR text, 4 kB of discussion (6 comments), 4 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.