Failover to fallback_providers on plain server_error (500/502)

[rrschott-ai]

Summary

• The eager-fallback gate in the main retry loop (conversation_loop.py) only treats FailoverReason.timeout and .overloaded (503/529) as transport failures eligible to fail over to fallback_providers after 2 retries.
• Plain server_error (500/502) wasn't in that set.
• Correction (per review): the fallback chain was not sitting completely unused for 500/502 — run_conversation's max-retry-exhaustion handler already calls agent._try_activate_fallback() unconditionally once retry_count >= max_retries, for any error reason. The actual improvement here is timing: server_error now joins timeout/overloaded in the eager gate, so failover happens after 2 retries instead of only once the full retry budget (api_max_retries, default 3) is exhausted — less time spent retrying a primary that's already failing with a configured fallback available.
• Also gives server_error its own status message ("Provider server error...") instead of reusing the transport "Provider unreachable" text, since a 500 means the provider responded, just with an error.

Test plan

• File parses clean (ast.parse)
• Extracted the eager-fallback reason set into _is_eager_fallback_transport_reason(), a plain testable function, matching the codebase's existing pattern for logic embedded in run_conversation (see test_gemini_fast_fallback.py's treatment of _pool_may_recover_from_rate_limit)
• tests/agent/test_server_error_fallback.py: unit coverage of the extracted helper (server_error/overloaded/timeout eligible; rate_limit/billing/unknown not), a reproduction of _should_fallback confirming eager fallback activates at retry_count >= 2 and not before, and an inspect.getsource() regression confirming run_conversation actually calls the extracted helper rather than a drift-prone inline literal
• Full suite: 212 passed (this file + test_gemini_fast_fallback.py, test_error_classifier.py, test_credential_pool_routing.py)

[teknium1]

Thanks for isolating the missing eager-fallback classification.

Problems

• Plain 500/502 already reach the configured chain at the max-retry handler (agent/conversation_loop.py:4003-4011), so the summary should describe this as earlier failover rather than a chain that is never used.
• The diff changes only agent/conversation_loop.py; please add a full-loop regression covering primary 500/502 failures followed by a successful configured fallback. Classifier-only coverage already establishes the 500/502 → server_error mapping in tests/agent/test_error_classifier.py:358-367, but it does not exercise the gate at agent/conversation_loop.py:3203-3275.

Suggested changes

• Assert that the fallback activates after the second failed primary request, matching the existing retry_count >= 2 transport-failure threshold.
• Update the PR description to distinguish eager fallback from the existing max-retry fallback.

Automated hermes-sweeper review.

[rrschott-ai]

Addressed — thanks for the correction, you're right and I've updated the PR description accordingly (pushed as commit 1eefdd1):

Confirmed at the max-retry-exhaustion handler: agent._try_activate_fallback() was already called unconditionally there for any error reason once retry_count >= max_retries, including server_error, before this PR. My original description overclaimed "sitting completely unused" — the real, accurate improvement is that server_error now fails over earlier (after 2 retries via the eager gate, same as timeout/overloaded) instead of only reaching the fallback chain once the full retry budget is spent.

Also extracted the reason-set check into _is_eager_fallback_transport_reason() and added tests/agent/test_server_error_fallback.py covering the helper directly, the retry_count >= 2 threshold, and an inspect.getsource() wiring check confirming run_conversation actually calls the extracted (now unit-tested) helper rather than a literal that could drift out of sync with it.

[GottZ]

This was generated by AI during triage.

Summary

Two PRs address the 500/502 failover timing gap: both add FailoverReason.server_error to the retry_count >= 2 eager-fallback path, causing earlier fallback rather than enabling a chain that was previously unreachable, because the max-retry handler already attempts fallback for every error reason. #58355 is the focused implementation, while #71336 mixes the same core change with unrelated Kanban ownership and process-wait heartbeat changes.

Related pull requests

• #58355 related — (+103/-9) — canonical candidate, changes requested: The diff correctly adds server_error to the eager-fallback predicate and gives it an accurate status message, so 500/502 responses switch providers after two failed primary attempts rather than at max-retry exhaustion. The keep_open review on #58355 remains unresolved: helper-level assertions and inspect.getsource wiring do not provide the requested full-loop regression demonstrating two primary failures followed by success on a configured fallback.
• #71336 duplicate — (+277/-6) — duplicate core fix, split required: Its conversation-loop change is materially the same server_error addition as #58355, but its source-parsing tests do not exercise actual fallback activation, and the diff also includes unrelated Kanban handoff/ownership and process-registry heartbeat changes. The contributor review explicitly requires separating those unrelated changes before selecting a canonical failover patch.

Duplicates

#58355 and #71336 implement essentially the same eager-fallback classification for HTTP 500/502; #71336 additionally contains unrelated changes that should be moved to separate PRs.

Suggested consolidation

Merge #58355 after adding the contributor-requested full-loop regression that proves fallback activation after the second failed primary 500/502 request and successful continuation on the configured fallback. Then close #71336 as a duplicate of #58355 for the failover portion; preserve its unrelated Kanban and process-registry work only through separately scoped PRs.

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
    subgraph Dup58355 ["PRs duplicating each other"]
        P58355["PR #58355 (open)"]
        P71336["PR #71336 (open)"]
    end
    class P58355 open
    class P71336 open
    class P58355 target
    click P58355 "https://github.com/NousResearch/hermes-agent/pull/58355"
    click P71336 "https://github.com/NousResearch/hermes-agent/pull/71336"

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 or no verify verdict yet (state tag in the node label).

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

[rrschott-ai]

Addressed the outstanding item from the 2026-07-29 cross-PR triage (and the underlying request in the original review): a full-loop regression test, not just helper-level coverage.

Rebased onto latest main and pushed as a new commit (ff1f226a5e):

Added test_run_conversation_activates_fallback_after_two_server_errors to tests/agent/test_server_error_fallback.py, modeled on the existing full-loop pattern in tests/run_agent/test_32646_fallback_429_after_timeout.py. It drives the real run_conversation() loop end-to-end: two plain HTTP 500s on the primary provider, then a successful response from the configured fallback_providers entry — asserting the eager-fallback gate (agent/conversation_loop.py:4285-4289) actually fires on the second failure (retry_count >= 2) and switches providers, rather than only checking the extracted helper or an inspect.getsource() wiring string.

Verified: uv run --extra dev python -m pytest tests/agent/test_server_error_fallback.py -q → 9 passed. Also ran the full tests/agent/ directory in natural collection order to check for cross-test interference (a real risk here, since the new test constructs a live AIAgent and calls run_conversation()) — confirmed no regression against test_auxiliary_main_first.py or elsewhere; the ~220 failures seen in that broader run are pre-existing drift in unrelated modules (e.g. test_verification_stop.py, test_codex_app_server_session.py), not touched by this change.

Re: the triage's note that #71336 is a duplicate — that's not my PR, so no action needed here; leaving that consolidation to whoever owns it.

[GottZ]

This was generated by AI during triage.

Delta since our previous triage comment

Since our previous comment, @rrschott-ai reports that #58355 was rebased and extended with the contributor-requested full-loop regression: two primary HTTP 500 failures followed by success through the configured fallback after the second failure. This directly responds to the visible keep_open review, but the current evidence includes only the author’s report and test result, not an available diff independently confirming the new test implementation.

Changed pull requests

• #58355 related — (+214/-9) — updated evidence, verification pending: Despite the keep_open review on #58355 requesting a full-loop regression, @rrschott-ai now reports adding exactly that scenario and obtaining 9 passing tests; because the updated diff is unavailable here, the review concern cannot yet be considered independently verified as resolved.

Duplicates

The prior duplicate designation is withdrawn: #58355 and #71336 target the same 500/502 eager-fallback gap, but the available evidence does not establish a maintainer-selected canonical implementation or support closing either solely as a duplicate.

Suggested consolidation

Recommendation revised: inspect #58355’s current diff to verify the reported full-loop test, require #71336’s unrelated Kanban/process-registry changes to be split, and only then select between the overlapping failover patches.

Complex graph unchanged since our previous triage comment.

Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Diffs were read for 1 of 2 PRs (rest unavailable); Assessment working set: 18 kB of PR diffs, 4 kB of issue/PR text, 4 kB of discussion (4 comments), 1 verify verdict. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

[rrschott-ai]

Re: the follow-up triage note that the diff wasn't available to independently verify — here's a direct pointer:

• Commit: ff1f226
• Diff: https://github.com/NousResearch/hermes-agent/pull/58355/files
• New test: test_run_conversation_activates_fallback_after_two_server_errors in tests/agent/test_server_error_fallback.py

That test constructs a real AIAgent, patches _interruptible_api_call to raise a plain HTTP 500 twice then return a successful response, and asserts run_conversation() actually switches to the configured fallback provider (zai/glm-4.7) after the second failure — not just that the extracted helper returns the right boolean. Ran standalone (9 passed) and alongside the rest of tests/agent/ in natural collection order with no interference.

[GottZ]

This was generated by AI during triage.

Delta since our previous triage comment

Since the previous comment, @rrschott-ai provided the updated #58355 diff, which now independently confirms the requested full-loop regression: two primary HTTP 500 failures are followed by successful activation of the configured fallback after the second failure. @teknium1’s new review of #71336 confirms that its source-parsing tests remain non-behavioral and prohibited by AGENTS.md, while its unrelated Kanban/process-registry changes still require separation.

Changed pull requests

• #58355 related — (+214/-9) — canonical candidate, review concern addressed in the diff: The new behavioral test executes run_conversation(), produces two primary 500 failures, and verifies successful continuation on the configured fallback after the existing retry_count >= 2 gate, directly addressing the visible keep_open review on #58355; the reported 9-test pass remains author-reported.
• #71336 duplicate — (+277/-6) — duplicate core fix, split required: Despite the new keep_open review on #71336, its current diff still tests the failover change by parsing production source rather than exercising fallback behavior and still bundles unrelated Kanban and process-registry changes; @teknium1 explicitly requests a behavioral test and separation before selection.

Suggested consolidation

Recommendation unchanged: merge the focused #58355 after normal maintainer verification, then close #71336 as a duplicate for the failover portion while preserving its unrelated work only in separate PRs.

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
    subgraph Dup58355 ["PRs duplicating each other"]
        P58355["PR #58355 (open)"]
        P71336["PR #71336 (open)"]
    end
    class P58355 open
    class P71336 open
    class P58355 target
    click P58355 "https://github.com/NousResearch/hermes-agent/pull/58355"
    click P71336 "https://github.com/NousResearch/hermes-agent/pull/71336"

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 0 issues in this complex. Each diff was read against this issue; Assessment working set: 30 kB of PR diffs, 4 kB of issue/PR text, 5 kB of discussion (6 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.