mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-11-06 05:17:15 +00:00
fix(bedrock): auxiliary calls retry a Bedrock 503 and see throttles as rate limits
Greptile "Auxiliary calls lose retries": with botocore's retries off, the aux recovery ladder only read HTTP status from exception attributes, so a botocore ClientError (status in response["ResponseMetadata"], code in response["Error"]) skipped the same-provider retry on a 503 and missed rate-limit handling on a ThrottlingException. _exc_http_status now reads botocore response metadata and _is_rate_limit_error matches ThrottlingException, matching how the ladder already owns retries for OpenAI-SDK clients (max_retries=0). The status-shape helpers move to agent/auxiliary_error_shapes.py to keep the auxiliary_client facade under its line cap.
This commit is contained in:
1 parent
e286e63e0e
commit
f891fbe737
3 files changed
+95
-39
No files matched your search
@@ -33,6 +33,9 @@ from agent.error_classifier import (
|
||||
is_reasoning_field_rejection,
|
||||
is_reasoning_required_rejection,
|
||||
)
|
||||
from agent.auxiliary_error_shapes import (
|
||||
_botocore_error, _exc_http_status, _is_invalid_aux_response_error, _is_statusless_structured_provider_error,
|
||||
)
|
||||
from agent.auxiliary_managed_local import _is_managed_local_endpoint
|
||||
from agent.auxiliary_reasoning_floor import remember_reasoning_floor, with_reasoning_floor
|
||||
from agent.auxiliary_structured_output import _route_key, _without_structured_output_format, remember_structured_output_rejection
|
||||
@@ -3292,9 +3295,9 @@ def _is_rate_limit_error(exc: Exception) -> bool:
|
||||
billing keywords counts as a rate limit.
|
||||
"""
|
||||
# (PR #8023 pattern)
|
||||
if type(exc).__name__ == "RateLimitError":
|
||||
if type(exc).__name__ == "RateLimitError" or _botocore_error(exc)[1] == "ThrottlingException":
|
||||
return True
|
||||
if getattr(exc, "status_code", None) != 429:
|
||||
if _exc_http_status(exc) != 429:
|
||||
return False
|
||||
err_lower = str(exc).lower()
|
||||
return _contains_any(err_lower, _RATE_LIMIT_KEYWORDS) or not _contains_any(err_lower, _RATE_LIMIT_BILLING_KEYWORDS)
|
||||
@@ -3330,11 +3333,6 @@ def _is_connection_error(exc: Exception) -> bool:
|
||||
))
|
||||
|
||||
|
||||
def _exc_http_status(exc: Exception) -> Any:
|
||||
"""HTTP status on the exception itself or on its ``response`` (None when neither carries one)."""
|
||||
return getattr(exc, "status_code", None) or getattr(getattr(exc, "response", None), "status_code", None)
|
||||
|
||||
|
||||
def _is_transient_transport_error(exc: Exception) -> bool:
|
||||
"""One-off transport blip worth retrying on the SAME provider: connection/stream-close errors plus pure 5xx/408.
|
||||
|
||||
@@ -3500,33 +3498,6 @@ def _is_model_incompatible_error(exc: Exception) -> bool:
|
||||
))
|
||||
|
||||
|
||||
def _is_invalid_aux_response_error(exc: Exception) -> bool:
|
||||
"""HTTP-200 empty/malformed ChatCompletions — a capability failure routed like model incompatibility."""
|
||||
if not isinstance(exc, RuntimeError):
|
||||
return False
|
||||
msg = str(exc).lower()
|
||||
return "auxiliary " in msg and "llm returned invalid response" in msg and "choices[0].message" in msg
|
||||
|
||||
|
||||
def _is_statusless_structured_provider_error(exc: Exception) -> bool:
|
||||
"""Detect a structured provider failure that has no HTTP status.
|
||||
|
||||
OpenAI-compatible relays may commit SSE with status 200, then send an
|
||||
OpenAI-style ``error`` event. The SDK raises a status-less ``APIError`` with
|
||||
``body=data["error"]`` — the INNER error object or a bare string (an
|
||||
``{"error": ...}`` wrapper is accepted too). Any non-empty structured error in
|
||||
that status-less shape is a route failure; ordinary HTTP errors keep their
|
||||
existing status-based classifiers, and message text alone is insufficient.
|
||||
"""
|
||||
if _exc_http_status(exc) is not None:
|
||||
return False
|
||||
body = getattr(exc, "body", None)
|
||||
err = body.get("error") if isinstance(body, dict) and "error" in body else body
|
||||
if isinstance(err, str):
|
||||
return bool(err.strip())
|
||||
return isinstance(err, dict) and any(err.get(k) for k in ("type", "code", "message"))
|
||||
|
||||
|
||||
# Tasks on a user-visible critical path (compression blocks resuming an oversized session; vision
|
||||
# stalls the serialised turn queue). A same-provider retry after a full-budget timeout costs another
|
||||
# whole ``timeout`` window, so they skip straight to fallback; fast blips still retry.
|
||||
|
||||
@@ -0,0 +1,54 @@
|
||||
"""Shape readers for auxiliary-call errors: where an SDK error carries its HTTP status, and the
|
||||
status-less failures (HTTP-200 malformed answers, SSE ``error`` events) that still end a route.
|
||||
|
||||
botocore's ``ClientError`` keeps its status in ``response["ResponseMetadata"]`` rather than on an
|
||||
attribute; the Bedrock client makes one attempt per call, so the auxiliary recovery ladder has to
|
||||
read it there to retry a 503 or treat a ``ThrottlingException`` as a rate limit.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from typing import Any
|
||||
|
||||
|
||||
def _botocore_error(exc: Exception) -> tuple[Any, Any]:
|
||||
"""``(HTTP status, error code)`` of a botocore ``ClientError``, whose ``response`` is the parsed
|
||||
dict; ``(None, None)`` for any other exception. The Bedrock client makes one attempt per call,
|
||||
so these classifiers own its retries the way they own the OpenAI SDK's (``max_retries=0``)."""
|
||||
response = getattr(exc, "response", None)
|
||||
if not isinstance(response, dict):
|
||||
return None, None
|
||||
return response.get("ResponseMetadata", {}).get("HTTPStatusCode"), response.get("Error", {}).get("Code")
|
||||
|
||||
|
||||
def _exc_http_status(exc: Exception) -> Any:
|
||||
"""HTTP status on the exception itself, its ``response``, or a botocore error's response metadata."""
|
||||
return (getattr(exc, "status_code", None) or getattr(getattr(exc, "response", None), "status_code", None)
|
||||
or _botocore_error(exc)[0])
|
||||
|
||||
|
||||
def _is_invalid_aux_response_error(exc: Exception) -> bool:
|
||||
"""HTTP-200 empty/malformed ChatCompletions — a capability failure routed like model incompatibility."""
|
||||
if not isinstance(exc, RuntimeError):
|
||||
return False
|
||||
msg = str(exc).lower()
|
||||
return "auxiliary " in msg and "llm returned invalid response" in msg and "choices[0].message" in msg
|
||||
|
||||
|
||||
def _is_statusless_structured_provider_error(exc: Exception) -> bool:
|
||||
"""Detect a structured provider failure that has no HTTP status.
|
||||
|
||||
OpenAI-compatible relays may commit SSE with status 200, then send an
|
||||
OpenAI-style ``error`` event. The SDK raises a status-less ``APIError`` with
|
||||
``body=data["error"]`` — the INNER error object or a bare string (an
|
||||
``{"error": ...}`` wrapper is accepted too). Any non-empty structured error in
|
||||
that status-less shape is a route failure; ordinary HTTP errors keep their
|
||||
existing status-based classifiers, and message text alone is insufficient.
|
||||
"""
|
||||
if _exc_http_status(exc) is not None:
|
||||
return False
|
||||
body = getattr(exc, "body", None)
|
||||
err = body.get("error") if isinstance(body, dict) and "error" in body else body
|
||||
if isinstance(err, str):
|
||||
return bool(err.strip())
|
||||
return isinstance(err, dict) and any(err.get(k) for k in ("type", "code", "message"))
|
||||
@@ -22,13 +22,16 @@ from agent.error_classifier import FailoverReason, classify_api_error
|
||||
REGION = "us-east-1"
|
||||
MODEL = "us.amazon.nova-pro-v1:0"
|
||||
MESSAGES = [{"role": "user", "content": [{"text": "hi"}]}]
|
||||
CONVERSE_OK = {"output": {"message": {"role": "assistant", "content": [{"text": "pong"}]}}, "stopReason": "end_turn",
|
||||
"usage": {"inputTokens": 1, "outputTokens": 1, "totalTokens": 2}}
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def fake_bedrock(monkeypatch):
|
||||
"""Local bedrock-runtime endpoint; ``mode`` is ``hang`` (never answers until teardown) or
|
||||
``throttle`` (400 ThrottlingException). Yields the list of request paths it received."""
|
||||
state = {"mode": "hang", "hits": []}
|
||||
``throttle`` (400 ThrottlingException) / ``unavailable`` (503) for the first ``fail_first``
|
||||
requests, then a Converse answer. Yields the list of request paths it received."""
|
||||
state = {"mode": "hang", "hits": [], "fail_first": 10**6}
|
||||
release = threading.Event()
|
||||
|
||||
class Handler(BaseHTTPRequestHandler):
|
||||
@@ -38,10 +41,17 @@ def fake_bedrock(monkeypatch):
|
||||
if state["mode"] == "hang":
|
||||
release.wait(30)
|
||||
return
|
||||
body = json.dumps({"message": "Too many requests, please wait before trying again."}).encode()
|
||||
self.send_response(400)
|
||||
status, error_type, payload = {
|
||||
"throttle": (400, "ThrottlingException", {"message": "Too many requests, please wait before trying again."}),
|
||||
"unavailable": (503, "ServiceUnavailableException", {"message": "Service unavailable, try later."}),
|
||||
}[state["mode"]]
|
||||
if len(state["hits"]) > state["fail_first"]:
|
||||
status, error_type, payload = 200, None, CONVERSE_OK
|
||||
body = json.dumps(payload).encode()
|
||||
self.send_response(status)
|
||||
self.send_header("content-type", "application/json")
|
||||
self.send_header("x-amzn-ErrorType", "ThrottlingException")
|
||||
if error_type:
|
||||
self.send_header("x-amzn-ErrorType", error_type)
|
||||
self.send_header("content-length", str(len(body)))
|
||||
self.end_headers()
|
||||
self.wfile.write(body)
|
||||
@@ -96,3 +106,24 @@ def test_throttle_reaches_hermes_retry_loop_on_first_refusal_with_default_timeou
|
||||
assert client.meta.config.read_timeout == 60, "unset config keeps botocore's own timeouts"
|
||||
classified = classify_api_error(excinfo.value, provider="bedrock", model=MODEL)
|
||||
assert (classified.reason, classified.retryable) == (FailoverReason.rate_limit, True)
|
||||
|
||||
|
||||
@pytest.mark.parametrize("mode", ["unavailable", "throttle"])
|
||||
def test_auxiliary_call_recovers_from_one_transient_bedrock_refusal(fake_bedrock, monkeypatch, mode):
|
||||
"""With botocore no longer retrying, the aux recovery ladder must read the botocore error
|
||||
(status in ResponseMetadata, ThrottlingException code) or a single 503/throttle fails the task."""
|
||||
from agent import auxiliary_client
|
||||
from agent.auxiliary_client import BedrockAuxiliaryClient, call_llm
|
||||
|
||||
_use_config(monkeypatch, {})
|
||||
monkeypatch.setattr(auxiliary_client, "_TRANSIENT_RETRY_BACKOFF_BASE", 0.0)
|
||||
fake_bedrock.update(mode=mode, fail_first=1)
|
||||
client = BedrockAuxiliaryClient(REGION, MODEL)
|
||||
monkeypatch.setattr(auxiliary_client, "_get_auxiliary_task_config",
|
||||
lambda task: {"provider": "bedrock", "model": MODEL, "max_output_tokens": 0})
|
||||
monkeypatch.setattr(auxiliary_client, "_get_cached_client", lambda *a, **k: (client, MODEL))
|
||||
|
||||
response = call_llm(task="session_search", messages=[{"role": "user", "content": "ping"}])
|
||||
|
||||
assert response.choices[0].message.content == "pong"
|
||||
assert len(fake_bedrock["hits"]) == 2
|
||||
Reference in new issue
Block a user