feat(agent): make overload (503/529) backoff configurable - #129
feat(agent): make overload (503/529) backoff configurable#129hashbender wants to merge 1 commit into
Conversation
|
Review Complete Files Reviewed: 4 By Severity:
Adds configurable overload_max_retries but has two medium-severity issues: missing upper-bound clamping against api_max_retries, and an off-by-one fallback bug making values 0 and 1 identical. Files Reviewed (4 files) |
There was a problem hiding this comment.
Risk: 🟠 High (55/100) — 2 medium findings · 210 LOC across 4 files
Summary
This PR introduces a new overload_max_retries configuration key in agent/agent_init.py, consumed by the retry/fallback logic in agent/conversation_loop.py, with corresponding example config and tests.
Issues Found
1. Missing upper-bound clamping (agent/agent_init.py) — overload_max_retries is parsed and clamped to a minimum of 0 and maximum of 5, but is not clamped against api_max_retries. When overload_max_retries exceeds api_max_retries, the overload retry budget can silently skip fallback progression because the outer retry loop exhausts first. The prior hardcoded behavior used api_max_retries as the implicit ceiling; the configurable version should enforce min(overload_max_retries, api_max_retries) at parse time.
2. Off-by-one in fallback check (agent/conversation_loop.py line ~2943) — The guard if overload_attempt >= overload_max_retries treats the counter as 0-indexed but the config value as a count, making values 0 and 1 produce identical behavior (no retries in either case). The comparison should be > to make N mean exactly N retries, or the counter should be 1-indexed.
Risk Assessment: Both findings affect the reliability of LLM API failover under overload conditions (503/529 responses). Misconfiguration could cause silent fallback skipping or unintended retry behavior. No security or data-integrity concerns.
Recommendations
- Clamp
overload_max_retriestoapi_max_retriesin the init parsing atagent/agent_init.py. - Fix the off-by-one comparison in the fallback guard at
agent/conversation_loop.py. - Add a test case for
overload_max_retries > api_max_retriesclamping.
| try: | ||
| _overload_retries = int(_agent_section.get("overload_max_retries", 2)) | ||
| _overload_retries = max(_overload_retries, 0) # 0 = immediate fallback | ||
| except (TypeError, ValueError): | ||
| _overload_retries = 2 | ||
| agent._overload_max_retries = _overload_retries |
There was a problem hiding this comment.
🟡 overload_max_retries not clamped to api_max_retries, causing silent fallback skip when exceeding the outer loop ceiling (bug)
The config key overload_max_retries (default 2) controls when an overload (503/529) triggers provider fallback. The documentation correctly warns 'Must be <= api_max_retries to take effect (the outer loop is the hard ceiling).' However, the code in agent/agent_init.py:1362-1367 only floors the value to 0 (no upper bound relative to api_max_retries). If a user sets overload_max_retries: 10 while api_max_retries is 3 (or the default), the overload fallback path at conversation_loop.py:2942-2943 never triggers — the outer loop exits at retry_count=3 before retry_count can reach 10. The overload then falls through to the generic 'max_retries exhausted' fallback instead of the targeted overload fallback, losing differentiated behavior. Default values (2 vs 3) happen to work, but any user customization risks this silent mismatch.
💡 Suggestion: Clamp _overload_max_retries to at most _api_max_retries (or _api_max_retries - 1 if the intent is to fire before the loop exhausts). Add _overload_retries = min(_overload_retries, _api_retries) after line 1364 to enforce the documented constraint.
📋 Prompt for AI Agents
In agent/agent_init.py, after line 1364 (_overload_retries = max(_overload_retries, 0) # 0 = immediate fallback), add a line: _overload_retries = min(_overload_retries, _api_retries) # per doc: must be <= api_max_retries. This ensures the overload-specific eager fallback threshold never exceeds the outer retry loop ceiling. Also add a test to tests/agent/test_overload_backoff_config.py verifying that overload_max_retries=10 with api_max_retries=3 gets clamped to 3.
| is_rate_limited | ||
| or (_is_transport_failure and retry_count >= 2) | ||
| or (classified.reason == FailoverReason.overloaded | ||
| and retry_count >= agent._overload_max_retries) |
There was a problem hiding this comment.
🟡 overload_max_retries values 0 and 1 produce identical behavior due to off-by-one in fallback threshold (bug)
The overload fallback check uses retry_count >= agent._overload_max_retries at conversation_loop.py:2943, but retry_count was already incremented from 0 to 1 at line 2738. This means overload_max_retries=0 and overload_max_retries=1 both trigger fallback after the first API failure (retry_count=1). The YAML comment says 'Controls how many retries to attempt before falling back' — default 2 gives only 1 retry (fallback after 2nd failure, retry_count=2). A user setting overload_max_retries=1 expecting 1 retry before fallback will instead get 0 retries (identical to overload_max_retries=0). The comparison should be > (strict greater than) to make N mean 'N retries before fallback'.
💡 Suggestion: Change >= to > at line 2943 so that overload_max_retries=N means: after N failures (N retries), fallback. With >: N=0 → retry_count=1 > 0 → fallback (0 retries ✓); N=1 → retry_count=1 > 1 false, retry again, retry_count=2 > 1 → fallback (1 retry ✓); N=2 → fallback after 2 retries (matching default). This makes values 0 and 1 distinguishable.
| and retry_count >= agent._overload_max_retries) | |
| and retry_count > agent._overload_max_retries) |
📋 Prompt for AI Agents
In agent/conversation_loop.py line 2943, change retry_count >= agent._overload_max_retries to retry_count > agent._overload_max_retries. Also update tests/agent/test_overload_backoff_config.py: the test at line 233 asset self._should_fallback_overloaded(0, 0) should be changed to assert self._should_fallback_overloaded(1, 0) since retry_count is always 1 after the first failure.
Summary
overload_max_retries,overload_base_delay,overload_max_delay) underagent:so users can tune how aggressively the retry loop backs off on provider 503/529 before falling back or surfacing the error.overloaded/timeoutfallback threshold so each can evolve independently.Closes NousResearch#55540
Changes
agent/agent_init.pyagent/conversation_loop.pycli-config.yaml.exampletests/agent/test_overload_backoff_config.pyTest plan
pytest tests/agent/test_overload_backoff_config.py— 15 passed)overload_max_retries: 0and verify immediate fallback on 503overload_base_delay: 30.0and verify longer waits on overload retryMirror-of: NousResearch#56037
NousResearch#56037