From f891fbe73728fed49a8359b19551319bc6b67276 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Wed, 28 Oct 2026 20:34:31 -0700 Subject: [PATCH] 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. --- agent/auxiliary_client.py | 39 +++-------------- agent/auxiliary_error_shapes.py | 54 ++++++++++++++++++++++++ tests/agent/test_bedrock_no_sdk_retry.py | 41 +++++++++++++++--- 3 files changed, 95 insertions(+), 39 deletions(-) create mode 100644 agent/auxiliary_error_shapes.py diff --git a/agent/auxiliary_client.py b/agent/auxiliary_client.py index 3ffe40dbcf..f6c9e5fe1b 100644 --- a/agent/auxiliary_client.py +++ b/agent/auxiliary_client.py @@ -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. diff --git a/agent/auxiliary_error_shapes.py b/agent/auxiliary_error_shapes.py new file mode 100644 index 0000000000..0618a6c368 --- /dev/null +++ b/agent/auxiliary_error_shapes.py @@ -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")) diff --git a/tests/agent/test_bedrock_no_sdk_retry.py b/tests/agent/test_bedrock_no_sdk_retry.py index 213bee667e..077b909ac2 100644 --- a/tests/agent/test_bedrock_no_sdk_retry.py +++ b/tests/agent/test_bedrock_no_sdk_retry.py @@ -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