Repository navigation
fix: fail over and retry on a member's own timeout in Fallback and Retry - #5085
Merged
Merged
Conversation
ai.clients.Fallback and ai.clients.Retry treated baml.errors.Timeout as terminal: a member that hit its own request_timeout_ms or time_to_first_token_timeout_ms ended the whole call, streaming or not, and Retry never retried it. Legacy clients failed over and retried on these timeouts, and the timeout docs promise that behavior. A member's Timeout now passes the shared safety gate (_may_try_again) like a safe transport failure: Fallback moves to the next member and Retry tries again (retry_if keeps judging typed failures). When no member or attempt is left, the original Timeout propagates unchanged, so callers that match on the type keep working. A caller's deadline still cannot be retried: baml.future.with_timeout cancels the members, and every wrapper re-throws baml.panics.Cancelled. Streams still fail over only before the first text delta. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughRetry and Fallback now handle member timeouts alongside typed failures in invocation and streaming paths. Streaming recovery can replace a stream only before text is emitted. Tests cover scripted timeout scenarios and real HTTP request deadlines. ChangesComposite client timeout recovery
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant CompositeClient
participant ResponsesClient
participant MockServer
CompositeClient->>ResponsesClient: Send request with member timeout
ResponsesClient->>MockServer: POST /responses
MockServer-->>ResponsesClient: Delayed response
ResponsesClient-->>CompositeClient: Timeout
CompositeClient->>ResponsesClient: Retry or invoke backup member
ResponsesClient->>MockServer: Send next request
MockServer-->>ResponsesClient: Successful response
ResponsesClient-->>CompositeClient: Return response
Merge Risk: ⚪ Minimal · up to Retry and Fallback now recover from member timeouts while preserving caller cancellation and terminal timeouts after visible streaming output. No merge-blocking issue was established; merge after normal checks pass. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Recovery remains bounded by configured attempts and members, and streaming stops recovery once text is visible. The main residual risk is repeating provider work when a timed-out request may already have been accepted. No new privileged attack path was established. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 1 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit counts each timeout beat, Comment |
aaronvg
enabled auto-merge
October 2, 2026 01:19
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Binary size checks failed❌ 2 violations · ✅ 1 passed
Details & how to fixViolations:
Add/update baselines:
[artifacts.bridge_wasm]
file_bytes = 25687842
gzip_bytes = 7281310
[artifacts.packed-program]
file_bytes = 37369760
gzip_bytes = 15189916Generated by |
hellovai
approved these changes
Oct 2, 2026
This was referenced Oct 2, 2026
meefs
pushed a commit
to meefs/baml
that referenced
this pull request
Oct 2, 2026
…5054) Gives BAML v1 the four timeouts a v0 user relies on (`request_timeout_ms`, `connect_timeout_ms`, `time_to_first_token_timeout_ms`, `idle_timeout_ms`), on every LLM client. This replaces the earlier contents of this PR with a new design. ## `baml.http` ```baml let req = baml.http.Request.from_url("https://api.example.com/items") .set_method("POST") .set_headers({ "content-type": "application/json" }) .set_body("{}") .set_timeout(baml.time.Duration.from_seconds(10)) .set_connect_timeout(baml.time.Duration.from_seconds(2)); let resp = baml.http.send(req); ``` - `Request` gains `timeout` (end to end, connecting included) and `connect_timeout`, plus a builder: `Request.from_url` and `set_method` / `set_headers` / `set_body` / `set_timeout` / `set_connect_timeout`. The setters mutate and return the request. `from_url` leaves the method empty, and sending a request with no method throws `InvalidArgument`. - The `timeout` argument stays on `fetch`, `send` and `send_sse`, and overrides `Request.timeout`. - Defaults: 5 minutes total and 10 seconds to connect when left `null`. Canary was unbounded. A zero or negative `Duration` means no limit. - `fetch_sse` is renamed `send_sse`, and its `first_event_timeout` argument is removed. - `http.Server` handlers (`handler`, `websocket`, `reject_websocket`) take a new `baml.http.ServerRequest` instead of `Request`, so `Request` is outgoing-only. - `baml.errors.Timeout` is now `{ message, duration: baml.time.Duration?, timeout_type: string }`. `duration_ms` is gone. `timeout_type` is the name of the field or argument that set the limit: `"timeout"` or `"connect_timeout"` from `baml.http`, `"first_token_timeout"` or `"stream_token_timeout"` from an LLM client, `"limit"` from `baml.future.with_timeout`, and `"timeout"` everywhere else (process, net and WebSocket timeouts). ## LLM clients ```baml let bounded = openai.ChatClient.new( model = "gpt-4o-mini", timeout = ai.LlmTimeoutOptions { timeout: baml.time.Duration.from_seconds(120), connect_timeout: baml.time.Duration.from_seconds(10), first_token_timeout: baml.time.Duration.from_seconds(30), stream_token_timeout: baml.time.Duration.from_seconds(10), }, ); // A bare Duration is the end-to-end total. let simple = openai.ChatClient.new(model = "gpt-4o-mini", timeout = baml.time.Duration.from_seconds(120)); ``` - All 13 HTTP clients take `timeout: baml.time.Duration | ai.LlmTimeoutOptions | null = null`: `openai.{Chat,Generic,Responses,Azure,OpenRouter,Ollama,Image}Client`, `anthropic.Client`, `google.{Gemini,Vertex}Client`, `aws.BedrockClient`, `typesafeai.Client`, `vercel.AiGatewayImageClient`. Seven of these had no timeout before. - `request_timeout_ms` and `time_to_first_token_timeout_ms` are removed from the clients, and `request_timeout_ms` from `ai.wire.send_as` / `send_as_with_wire`. There is no deprecation period. A client author now sets the limits on the request itself. - `timeout` and `connect_timeout` are enforced by the transport. `first_token_timeout` and `stream_token_timeout` are enforced by `ai.stream.TurnStream`. - `first_token_timeout` starts when the attempt is dispatched (before connecting) and ends on the first non-empty content delta. `stream_token_timeout` is the gap between non-empty content deltas after that. Content is a non-empty text, reasoning or tool-input delta, or a complete tool-call or file part. Keep-alives, `message_start`, usage-only frames and empty deltas do not count. - Stream decoders yield a new `ai.stream.ContentSeen` event for content that has no text delta. Code that matches exhaustively on `ai.stream.Event` needs an arm for it. - The two token timers only run while the client is waiting on the provider, so a slow consumer is not charged. - Clients that do not stream accept the two token timeouts and ignore them. - A timeout from any of the four deadlines is retried by `Retry` and fails over in `Fallback` (already on canary from BoundaryML#5085; covered here for the token timeouts). ## Behavior changes to know about - Every `baml.http` request and every LLM call now has a 5 minute total and a 10 second connect limit unless told otherwise, including a streaming response body. Pass a zero `Duration` to opt out. - The first-token timer now starts at dispatch, not when response headers arrive, and ends on content, not on the first SSE event. A v0 value carried over unchanged also has to cover connecting, sending the request and waiting for headers. - Browser/Wasm transports still accept the limits and do not enforce them. ## Testing - New corpus namespace `ns_llm_timeouts` (17 tests): the first-token timer ignores `message_start` and empty deltas and ends on tool input; the stream-token timer fires between deltas, does not run before the first content, and does not charge a slow consumer; zero means no limit; non-streaming calls ignore the token timeouts; Ollama, OpenRouter, Azure and Gemini honor `timeout`; `Fallback` advances on a first-token timeout. - New transport tests in `crates/baml_tests/tests/http.rs`: `Request.timeout`, the argument override, `send_sse`, the connect timeout (a peer that never answers the TLS handshake), zero and negative limits, and a request with no method. - Existing provider timeout tests are ported to the new argument and now assert `timeout_type`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added flexible timeout settings for HTTP requests and AI clients, covering total request, connection, first-content, and between-content deadlines. Unset total and connection limits default to five minutes and ten seconds. * Streaming deadlines account for non-text content, such as reasoning and tool input, and report typed timeout errors. * Separated incoming server requests from outgoing HTTP requests; outgoing requests support configurable total and connection timeouts. * **Breaking Changes** * Replaced `fetch_sse` with `send_sse` and updated timeout configuration and timeout error fields. Server handlers now use the distinct incoming-request type. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5.5 <[email protected]>
This branch was successfully deployed
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
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.
Issue Reference
Found while migrating a production app from
baml-py==0.221to the imperative language:ai.clients.Fallbackandai.clients.Retrystop on a member timeout instead of failing over or retrying. The app has 16 fallback chains whose member timeouts were tuned so a hung model hands off fast; on 0.20 a hung primary fails the call.Changes
A member's own
baml.errors.Timeoutis now a failed attempt, like a safe transport failure:Fallback(invoke)Timeoutfrom member 1 ends the callFallback(stream, before first delta)Timeoutends the streamRetry(invoke and stream, before first delta)baml.errors.Timeoutbaml.future.with_timeoutaround the call)Cancelledinside,TimeoutoutsideHow:
clients.bamlgets one safety gate,_may_try_again(cause: Retryable), wheretype Retryable = ai.errors.Failure | baml.errors.Timeout.Retry._attempt,Retry.invoke_stream,Fallback._try,Fallback.invoke_stream, andStreamRelay.recoverall use it, so the rule exists once.retry_ifstill judges typed failures only (Retry._wants_retry); a member timeout passes the gate and is retried.Backoff.delay_msand the failed-legLLMCallrecords accept a timeout (a timed-out leg reports provider"unknown").Timeoutis safe here: a caller's deadline never reaches these catches.with_timeoutcancels the work it wraps, so members seebaml.panics.Cancelled, which every wrapper already re-throws. ATimeoutcaught inside a wrapper is always the member's own (request_timeout_ms,time_to_first_token_timeout_ms, or awith_timeoutthe member applies itself).This reverses the "preserve typed timeout without replay" choice from #4459. The type is still preserved; only the replay changes.
Legacy parity
The legacy timeout docs promise this behavior: "Timeouts let you fail fast and either retry or fallback to alternative clients" (
fern/01-guide/04-baml-basics/timeouts.mdx), and "A timeout triggers the retry mechanism (if configured)" (fern/03-reference/baml/clients/timeouts.mdx). Measured against a local server that holds its response 3 s, with a 1 s member timeout:baml-py 0.221fallback [timed-out, fast], invokefastanswers, 1.0 sTimeout, 1.0 sfastanswersfallback [timed-out, fast], stream (first token)fastanswers, 1.0 sTimeout, 1.0 sfastanswersTimeoutTesting
Unit tests added/updated
Manual testing performed
BAML corpus (offline scripted clients):
invoke_error_normalization: the old "without replay" test now asserts the new contract (Retry makes 3 attempts; Fallback calls both members and surfaces the last member'sTimeout; RoundRobin unchanged), plus "Fallback moves past a timed-out member to the next one" and "Retry retries a timed-out attempt until one answers" (both check the failed-leg records).streaming_composite_clients: retry after a timeout while opening; fallback after a timeout before the first delta; the last member'sTimeoutsurfaces; a timeout after a visible delta stays terminal.End to end (
tests/streaming_composite_clients.rs, wiremock, a realopenai.ResponsesClientwithrequest_timeout_ms = 200and a held response):fallback_moves_past_member_request_timeout,fallback_stream_moves_past_member_request_timeout,retry_retries_member_request_timeout. All three fail without theclients.bamlchange.cargo insta test --test-runner nextest --dnd -p baml_tests -p baml_cli -p baml_pack_host --all-features --unreferenced=reject: 1,595 tests pass, no snapshot changes. Run locally withBAML_AGENT_SKILL_CHECK=off; without it, 12 CLI-driven tests refuse to run under an agent.Full corpus: 5,028 passed, 2 tolerated failures.
clippy -p baml_tests --tests -D warningsandcargo fmt --checkare clean.clients.bamlisbaml fmt-clean.Additional Notes
Not in this PR (found in the same review, worth separate issues):
aws.BedrockClientandgoogle.GeminiClientexpose no timeout fields at all.🤖 Generated with Claude Code
Note
Medium Risk
Changes default reliability behavior for every Retry/Fallback chain (invoke and stream), reversing a recent no-replay-on-timeout choice; caller vs member timeout distinction must stay correct to avoid unsafe replays.
Overview
ai.clients.Retryandai.clients.Fallbacknow treat a member’s ownbaml.errors.Timeout(e.g.request_timeout_ms) like a safely retryable transport failure instead of ending the call immediately.A shared
Retryabletype and_may_try_againgate drive invoke, streaming (viaStreamRelay), backoff, and failed-legLLMCallrecording.Retry._wants_retrystill runs customretry_ifonly on typed failures; member timeouts always retry when attempts remain. Caller deadlines stayCancelled/ outerTimeout; timeouts after the first streamed text delta stay terminal.Tests flip the old “no replay on timeout” contract and add BAML scenarios plus wiremock e2e with real
request_timeout_msonopenai.ResponsesClient.Reviewed by Cursor Bugbot for commit 7a7749f. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit