fix(bedrock): enforce the model deadline and connect timeout on the SDK client - #845
Conversation
…DK client The Bedrock bridge was the only dispatch path where a model's timeout did not actually bound the call: the SDK client was built with no timeout_config at all, and ctx.deadline was consulted only inside map_sdk_error to relabel an already-failed call as Timeout. A Bedrock upstream that accepted the connection and then stalled held the request open indefinitely — while every other bridge cancels via its with_deadline/tokio::time::timeout wrapper. - build_client now sets TimeoutConfig: connect_timeout from the shared upstream.connect_timeout_ms, operation_timeout from the per-request ctx.deadline. An elapsed deadline surfaces as SdkError::TimeoutError, which map_sdk_error already turns into BridgeError::Timeout, so retry / fallback / cooldown classification is unchanged. - SDK-internal retries are disabled: retries belong to the gateway's routing::effective_retries budget (per-attempt telemetry, per-model config). Left at the default the SDK would silently add a standard-mode retry layer (3 attempts) underneath it — no other bridge's HTTP client retries on its own. - converse_stream bodies stay bounded per-chunk by the proxy-side read wrapper, same as other bridges; operation_timeout covers send + response headers only. The regression test drives the real bridge.chat() through wiremock with an 8 s response delay and a 300 ms deadline: before this fix the call sat through the full delay and returned Ok; now it cancels at the deadline with BridgeError::Timeout.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
…ut_ms=0 Audit follow-ups: - New wiremock hit-count test: a 500 from the upstream must reach the gateway's routing budget after exactly one wire attempt. Verified to fail without RetryConfig::disabled() — the SDK's standard mode hits the upstream 3 times. - connect_timeout None (operator set upstream.connect_timeout_ms: 0, the documented off switch) now calls disable_connect_timeout(); leaving the builder slot unset let the SDK's default plugins restore their own 3.1 s connect timeout. - Comment wording: the pre-fix client was not timeout-free — the SDK defaults supplied a 3.1 s connect timeout; what was unbounded is everything after connect (operation/read), which is the actual bug.
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Problem
The Bedrock bridge was the only dispatch path where a model's
timeoutdid not actually bound the call. The SDK client is built with notimeout_configat all, andctx.deadlinewas consulted only insidemap_sdk_error— i.e. after a call had already failed for some other reason, purely to relabel the error asTimeout. A Bedrock upstream that accepted the connection and then went silent held the request open indefinitely, while every other bridge cancels at the deadline via itswith_deadline/tokio::time::timeoutwrapper. (The connect phase itself was bounded only by the SDK default plugins' own 3.1 s —upstream.connect_timeout_ms, which reaches every reqwest-based bridge through the shared client builder, never reached the AWS SDK client, including its documented0= disabled convention.)Change
build_clientnow installs aTimeoutConfigon the SDK client:connect_timeout← the sharedupstream.connect_timeout_ms(same knob every other bridge honors);operation_timeout← the per-requestctx.deadline(the model/group/deployment-resolved timeout). An elapsed deadline surfaces asSdkError::TimeoutError, whichmap_sdk_erroralready maps toBridgeError::Timeout, so retry / fallback / cooldown classification is untouched. The per-request client construction the bridge already does (stateless by design) is what makes a per-requestoperation_timeoutpossible.converse_streambodies stay bounded per-chunk by the proxy-side read-timeout wrapper, exactly like every other bridge's streams;operation_timeoutcovers send + response headers only.SDK-internal retries are disabled (
RetryConfig::disabled()): retries belong to the gateway'srouting::effective_retriesbudget, which emits per-attempt telemetry and honors per-model config. Left at its default, the SDK adds a hidden standard-mode retry layer (up to 3 attempts) underneath the gateway's own budget, grinding the upstream invisibly — no other bridge's HTTP client retries on its own.Not in scope: the Bedrock guardrail client builds its own SDK client but wraps every call in
tokio::time::timeout(guardrail.timeout_ms)(default 5 s), so it is already bounded end-to-end.Behaviour changes
Model.retries/routing.retries/upstream.retries.Tests
chat_deadline_cancels_a_silent_upstreamdrives the realbridge.chat()through wiremock with an 8 s response delay and a 300 ms deadline, assertingBridgeError::Timeoutand an elapsed time far below the delay (cancellation, not relabelling). Verified to fail without the fix: the call sits through the full 8 s and returnsOk.sdk_does_not_retry_on_its_ownpins the single-attempt semantics with a wiremock hit counter: a 500 reaches the gateway after exactly one wire attempt. Verified to fail withoutRetryConfig::disabled()— the SDK hits the upstream 3 times.