Tool-path rate limits: retry at the bridge, then fail the run fast - #492
Merged
Merged
Conversation
A PageIndex cloud 429 (or 5xx) on a tool call used to reach the model as an INTERNAL_ERROR envelope saying "try again": the model re-called once with no wait, then wrote the failure into its answer, and chat() returned normally with no status anywhere. The same 429 before the loop (the doc_id targeting lookup) already propagated raw. - McpBridge mounts a urllib3 Retry: 429/502/503 and connection failures, three attempts, 0/2/4 s apart or as Retry-After says; read timeouts are never replayed (240 s each, and the server may have acted); a Retry-After past a minute is a quota, not a blip, so the backoff runs instead of sleeping it out. Exhausted, the last response falls through to the existing >= 400 branch, so the status_code survives. - _bridge_invoker re-raises 429/5xx alongside 401/403. The frameworks turn a raised tool exception back into model-visible text, so each chat() door gets its own escape: the in-process MCPServer's failure_error_function lets a PageIndex-caused failure propagate and _translate_run_error unwraps it from the framework's wrapper (which also un-flattens the 401 case); the Messages lane runs each turn's tools through the runner's public generate_tool_call_response() and raises before the next model call. - _model_backend_error keeps the provider's status_code. Claude Agent SDK tools cannot fail fast: the SDK MCP server converts handler exceptions into JSON-RPC errors for Claude Code by design. Claude-Session: https://claude.ai/code/session_014S88dcSz7jykegAWyWZk8E
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| def log_message(self, *args): | ||
| pass | ||
|
|
||
| def do_POST(self): |
| def test_bridge_read_timeout_is_not_retried(mcp_stub, monkeypatch): | ||
| """A read timeout is a full wait the server may have acted on: | ||
| surfaced once, never replayed.""" | ||
| import pageindex.mcp_bridge as mcp_bridge |
| escapes the run (a model-side slip staying model-visible is covered end | ||
| to end in test_local_chat).""" | ||
| pytest.importorskip("agents") | ||
| import pageindex.mcp_bridge as mcp_bridge |
The bridge retry is now a plain urllib3 Retry: 429 and every 5xx retried three times at the fixed 0/2/4 s backoff, Retry-After ignored. That drops the _Retry subclass, whose get_retry_after raised InvalidHeader on a non-integer header (turning a 429 into "could not reach the server"), honoured a 60 s Retry-After three times over, and let a 413 carrying Retry-After replay. 500 and 504 join the forcelist so the invoker's "what survived the bridge's retries" holds for every status it re-raises. Retry is imported from requests.adapters, the declared dependency. The invoker re-raises transport failures too: once the bridge's own connection retries fail, the model cannot reach the server either, and the envelope only sent it round the retry loop. The handshake error blames the API key only on 401/403: a rate-limited handshake is now a run-terminating error and was telling users to rotate a working key. Docstrings on agent_tools()/build_agent_tools and the Anthropic adapter state the real raise set: 401/403, post-retry 429/5xx, unreachable server. Claude-Session: https://claude.ai/code/session_013xk3xt9KgHNTjsYmFLKxbu
|
|
||
| def test_handshake_failure_blames_the_key_only_on_auth_statuses(monkeypatch): | ||
| """A rate-limited or failing handshake is not a key problem.""" | ||
| import types |
Raise ToolError from the tools chat(protocol="messages") runs instead of the raw PageIndexAPIError: the Anthropic runner log.exception()s any other exception, so every fail-fast printed a 20-line traceback from anthropic's internals before the SDK raised its own error. The lane still records the failure and raises it right after the runner's tool batch, so the ToolError content never reaches the model. Drop the two post-loop _messages_fail_fast calls: the runner executes tools only through the public generate_tool_call_response (0.108.0 through 1.4.0), which checked_tool_response wraps, so they could never fire. Annotate _pageindex_cause for the py.typed package. Claude-Session: https://claude.ai/code/session_01CfYSeq8kM7HjfF79TGbsiT
RATE_LIMITED / USAGE_LIMIT_REACHED (pageindex-chat #472) arrive as a normal tool error inside HTTP 200, already retried server-side; the invoker re-raises them as 429 / 402, the way a post-retry status escapes, so every lane fails fast without a per-lane change. The bridge retries the whole 5xx range, the same range the invoker re-raises; a non-JSON 200 body (a JSONDecodeError is a RequestException too) stays a model-visible envelope, since the server was reached. The MCP stub tests keep the machine's proxy out of 127.0.0.1. Claude-Session: https://claude.ai/code/session_01F7vqmZdnWeKC9SUBytDrdf
Member
Author
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 622646133d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Retry-After is ignored: a long one is a quota, not a blip. | ||
| _RETRY = Retry(total=3, read=0, backoff_factor=1, | ||
| status_forcelist=(429, *range(500, 600)), | ||
| allowed_methods=None, raise_on_status=False, |
There was a problem hiding this comment.
With include_management=True, the same session executes dynamically discovered write tools such as upload/delete, and allowed_methods=None makes urllib3 replay every POST after any listed 5xx. A server can commit a management operation and then return 500, so this transparently invokes the tool again and may create duplicate uploads or repeat other non-idempotent effects. Apply status retries only to read-only/idempotent tools (using their MCP annotations), rather than mounting this policy for every request.
Useful? React with 👍 / 👎.
_raise_account_limit fired inside _invoke's try, so its escape depended on 429 and 402 also appearing in the except's re-raise tuple: two lists in one function that had to agree. The check now runs after the try, where the except cannot swallow it, and 402 leaves the tuple (the MCP route never answers HTTP 402; it was there only to let the raise through). The mcp_stub fixture also sets the lowercase no_proxy: requests reads that spelling first, so a machine with no_proxy set still routed the stub requests through its proxy despite NO_PROXY.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A PageIndex cloud 429 (or 5xx) on a tool call used to reach the model as an
INTERNAL_ERRORenvelope saying "try again": the model re-called once with no wait, then wrote the failure into its answer, andchat()returned normally with no status anywhere. The same 429 before the loop (the doc_id targeting lookup) already propagated raw.Changes
McpBridgemounts a urllib3Retry: 429, every 5xx and connection failures, three attempts, 0/2/4 s apart.Retry-Afteris ignored: a long one is a quota, not a blip, and parsing it was the one way a 429 could turn into "could not reach the server" (urllib3 raises on a non-integer value). Read timeouts are never replayed (240 s each, and the server may have acted). Exhausted, the last response falls through to the existing>= 400branch, sostatus_codesurvives._bridge_invokerre-raises 429/5xx and an unreachable server (a transport failure that outlived the connection retries) alongside 401/403, and the cloud's two account-level tool errors, which arrive inside a normal HTTP 200 tool result with anerrorCode(pageindex-chat fix: extra_body refuses the skeleton keys; sampling fields ride ModelSettings on LiteLLM-routed models #472):RATE_LIMITED(upstream 429, already retried server-side;retry_after_secondswhen known) as 429 andUSAGE_LIMIT_REACHED(plan quota,open_url) as 402. A non-JSON 200 body stays a model-visible envelope: the server was reached. The frameworks turn a raised tool exception back into model-visible text, so eachchat()door gets its own escape:MCPServerpasses afailure_error_functionthat lets a PageIndex-caused failure propagate, and_translate_run_errorunwraps it from the framework's wrapper (this also un-flattens the mid-session 401 case).generate_tool_call_response(), and a recorded failure raises before the next model call._model_backend_errorkeeps the provider'sstatus_code.The Claude Agent SDK lane and
as_openai_tools(hosted=True)are out of reach: for a cloud client they hand the framework a URL and the framework's own MCP client makes the request, so neither the bridge retry nor_bridge_invokeris in the path.Behaviour change on the public tool surfaces
as_openai_tools()users running their ownRunner.runnow get an exception for a post-retry 429/5xx or an unreachable server (the 401/403 re-raise always intended this; the framework absorbed it).as_anthropic_tools()users' runners absorb it into anis_errorresult and log a traceback. The plain functions fromagent_tools()raisePageIndexAPIErrorfor the same set, where their docstring used to promise only 401/403; the docstrings now state the real raise set.Verification
Retry-Afterheader present and ignored), exhausted-retry status for 429, 504 and 529, read-timeout non-replay, an unreachable server re-raised through the invoker, the handshake wording per status; plus the invoker re-raise matrix (including theRATE_LIMITED/USAGE_LIMIT_REACHEDenvelopes and a non-JSON 200 body), openai-agents escape, run-error unwrap, providerstatus_code, end-to-end fail-fast on the chat and Messages doors, and a regression guard for model-side slips.generate_tool_call_response.PageIndexAPIError(status_code=429)onchat()default, streamed, andprotocol="messages", after one tool call.https://claude.ai/code/session_014S88dcSz7jykegAWyWZk8E
https://claude.ai/code/session_013xk3xt9KgHNTjsYmFLKxbu
https://claude.ai/code/session_01F7vqmZdnWeKC9SUBytDrdf