Test + I/O timeouts and the end-of-run engine wedge fix#3959
Conversation
Every test body now races a deadline inside the stdlib runner (testing/registry.baml run_test): a hung test — stalled network read, deadlocked await — becomes a FAIL with "test timed out after Nms" instead of hanging the whole run. baml.future.race cancels the loser on settle (BEP-034), so a timed-out body is actively cancelled and a finished test's timer doesn't linger. Default 300000ms; override with BAML_TEST_TIMEOUT_MS. Verified: a 60s-sleep test fails at a 2s override while the suite exits promptly; full offline corpus unaffected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
null timeout no longer means "no deadline": _timeout_nanos resolves it to 300s for send/fetch (deliberately no unbounded spelling), and fetch_sse gets the same 300s total deadline in the host builder (connect + whole stream). A stalled socket now surfaces as baml.errors.Timeout instead of wedging the VM in a read that never yields. Verified: offline corpus green pre-kill; live sweep 29/29 on the rebuilt binary earlier this morning. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… wedge) A spawned child whose event loop returned Err(EngineError) — e.g. an awaiter parked on a future that settles to InternalError — was only LOGGED in spawn_thread_inner, leaving the child's heap Future Pending forever. Its awaiter then parked forever, cascading up to the B-650 end-of-run wait_for_outstanding_child_futures: all work done, workers parked, zero sockets, but run_filtered -> block_on never returns (the stuck-shell bug; live-only because the leak needs the awaiter parked before the error fires). Fix: settle_internal_error_if_pending on the FutureManager (tolerant of racing cancels) + settle_spawn_engine_error called from BOTH the Err and invariant-violating Ok(RootValue) arms of spawn_thread_inner, so the error propagates deterministically up the await chain instead of leaking. Regression test: spawned_awaiter_of_internal_error_future_does_not_wedge (deterministic, offline; wedges ~11s without the fix, passes ~2s with). Verified additionally by 5+ clean live sweeps on the patched build. The separate type-soundness hole used during diagnosis is filed as B-797. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
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):
|
📝 WalkthroughWalkthroughHTTP requests and SSE calls now default to 5 minutes, spawned engine errors settle pending child futures instead of leaving them stuck, and test execution now races the body against a configurable timeout. ChangesHTTP Default Timeout
Engine Spawned Future Settlement
Test Runner Timeout
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@baml_language/crates/sys_native/src/io_impls.rs`:
- Around line 2213-2218: Remove the hardcoded 300-second RequestBuilder::timeout
in fetch_sse within io_impls.rs, since it applies to the full request and can
terminate active SSE streams mid-flight. Replace it with an idle/read timeout
approach that only trips when the stream stalls, or thread a configurable
SSE-specific timeout through fetch_sse so callers can control long-lived
streams.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cc1d0e67-3f94-4456-8600-34cca28116df
📒 Files selected for processing (6)
baml_language/crates/baml_builtins2/baml_std/baml/ns_http/http.bamlbaml_language/crates/baml_builtins2/baml_std/testing/registry.bamlbaml_language/crates/bex_engine/src/future.rsbaml_language/crates/bex_engine/src/lib.rsbaml_language/crates/bex_engine/tests/host_value_callable.rsbaml_language/crates/sys_native/src/io_impls.rs
| // Default 5-minute total deadline (connect + the whole stream): a | ||
| // stalled SSE socket must surface as a Timeout instead of wedging | ||
| // the VM in a read that never yields. Mirrors `_send`/`_fetch`'s | ||
| // stdlib-level default. Legitimately longer-lived streams need a | ||
| // dedicated timeout parameter on `fetch_sse` (not yet plumbed). | ||
| builder = builder.timeout(std::time::Duration::from_secs(300)); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
reqwest RequestBuilder timeout applies to streamed response body bytes_stream total deadline
💡 Result:
In reqwest, the timeout set on a RequestBuilder (or the global ClientBuilder) acts as a total deadline for the entire request, from the start of the connection until the response body has been completely consumed [1][2][3]. Key points regarding the timeout and streamed response bodies: 1. Total Deadline: When you configure a timeout (e.g., via RequestBuilder::timeout), it applies to the entire duration of the request and response lifecycle [1][3]. If the response body is being streamed using bytes_stream, the total time taken to stream all bytes must be within this deadline [4][2][5]. If the stream is not finished by this time, the request will error with a timeout [4][5]. 2. Alternative for Stalled Streams: Because the standard timeout is a total deadline, it is often unsuitable for monitoring long-lived or large streams where the total duration is unknown or naturally exceeds a reasonable timeout. For this use case, reqwest provides a separate read_timeout option [4][3]. 3. Read Timeout: Configured via ClientBuilder::read_timeout, this timeout applies to each individual read operation on the stream [4][3]. It resets after each successful read, making it effective for detecting stalled connections without requiring knowledge of the total download size or duration [4][3]. If both a total timeout and a read timeout are configured, reqwest applies both: the operation must respect the individual read limit and must complete in its entirety before the total deadline is reached [5].
Citations:
- 1: https://docs.rs/reqwest/latest/reqwest/struct.RequestBuilder.html
- 2: https://docs.rs/reqwest/latest/reqwest/struct.ClientBuilder.html
- 3: https://github.com/seanmonstar/reqwest/blob/04a216fc/src/async_impl/client.rs
- 4: Add read_timeout option seanmonstar/reqwest#2237
- 5: https://docs.rs/reqwest/latest/src/reqwest/async_impl/body.rs.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant implementation around fetch_sse.
FILE="baml_language/crates/sys_native/src/io_impls.rs"
echo "== File size =="
wc -l "$FILE"
echo
echo "== Relevant lines around 2213-2218 =="
sed -n '2185,2245p' "$FILE" | cat -n
echo
echo "== Search for fetch_sse and timeout-related logic in this file =="
rg -n "fetch_sse|timeout\\(|read_timeout|bytes_stream|sse" "$FILE"Repository: BoundaryML/baml
Length of output: 4746
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="baml_language/crates/sys_native/src/io_impls.rs"
echo "== apply_http_timeout helper =="
sed -n '1850,1888p' "$FILE" | cat -n
echo
echo "== fetch_sse stream consumption =="
sed -n '2268,2315p' "$FILE" | cat -n
echo
echo "== nearby SSE-related functions =="
sed -n '2315,2350p' "$FILE" | cat -nRepository: BoundaryML/baml
Length of output: 5547
Avoid a hard 5-minute deadline on SSE streams RequestBuilder::timeout covers the whole request, so response.bytes_stream() can be cut off mid-stream after 300s even if data keeps arriving. If the goal is to fail stalled sockets, use an idle/read timeout or make the SSE timeout configurable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baml_language/crates/sys_native/src/io_impls.rs` around lines 2213 - 2218,
Remove the hardcoded 300-second RequestBuilder::timeout in fetch_sse within
io_impls.rs, since it applies to the full request and can terminate active SSE
streams mid-flight. Replace it with an idle/read timeout approach that only
trips when the stream stalls, or thread a configurable SSE-specific timeout
through fetch_sse so callers can control long-lived streams.
Binary size checks passed✅ 7 passed
Generated by |
…ak fix Pins the second real-world casualty of the leaked-pending-future bug: the scenario-15 "concurrent guards" shape — children spawned inside a .map closure, joined with baml.future.all under a spawned parent with a linked CancelToken, one child hitting an uncatchable engine error while siblings are parked. Documented at the time as a VM deadlock (parks at 0% CPU); A/B proven here to be the same root cause: with the settle fix reverted this test wedges (Elapsed at the 10s guard), with it it resolves in ~4s. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Added a second regression test: the spawn-in-map + future.all + CancelToken guard topology (the scenario-15 'VM deadlock') is A/B-proven to be the same leaked-pending-future root cause — wedges with the fix reverted, resolves in ~4s with it. |
1 participant
Three related reliability fixes, extracted from the LLM-provider scenario work where they were found and battle-tested:
1. Per-test timeout in
baml-cli test(default 5 min)Every test body races a deadline inside the stdlib runner (
testing/registry.baml::run_test). A hung test — stalled network read, deadlocked await — becomes a FAIL withtest timed out after Nmsinstead of hanging the whole run.baml.future.racecancels the loser on settle (BEP-034), so a timed-out body is actively cancelled and a finished test's timer doesn't linger. Override withBAML_TEST_TIMEOUT_MS.2. Default 5-minute deadline on
http.send/http.fetch/http.fetch_ssenulltimeout no longer means "no deadline": it resolves to 300s (pass an explicitDurationfor longer — there is deliberately no unbounded spelling), andfetch_ssegets the same 300s total deadline in the host builder. A stalled socket now surfaces asbaml.errors.Timeoutinstead of wedging the VM in a read that never yields — the one hang class BAML-level timeouts can't preempt.3. Engine: settle spawned-child futures on engine error
A spawned child whose event loop returned
Err(EngineError)(e.g. an awaiter parked on a future that settles toInternalError) was only logged inspawn_thread_inner, leaving the child's heapFuturepending forever — its awaiter parked forever, cascading up to the end-of-runwait_for_outstanding_child_futures, sorun_filtered → block_onnever returned. Symptom in the wild:baml-cli testlive sweeps completing all tests, all tokio workers parked, zero open sockets, process wedged for hours. Fix:settle_internal_error_if_pending+settle_spawn_engine_erroron both error arms, so the error propagates deterministically up the await chain.Deterministic regression test included (
spawned_awaiter_of_internal_error_future_does_not_wedge: wedges ~11s without the fix, passes ~2s with). Verified additionally by repeated clean live-key test sweeps on the fixed build; the hung-test timeout behavior is probe-verified on this branch (1 passed, 1 failed — "test timed out after 2000ms"at a 2s override).🤖 Generated with Claude Code
Note
High Risk
Changes default HTTP timeout semantics (breaking for callers that relied on unbounded waits) and touches async spawn/future settlement on error paths in the engine, which can affect shutdown and error propagation across concurrent runs.
Overview
Adds default 5-minute deadlines where work could hang indefinitely:
http.fetch/http.sendnow map anulltimeout to 300s (no unbounded spelling),fetch_ssegets the same total deadline in the native client, andrun_testraces each body against a timer (override viaBAML_TEST_TIMEOUT_MS) usingbaml.future.raceso losers are cancelled.Fixes an engine shutdown wedge: when a spawned child’s event loop exits with
EngineErroror an invariantRootValuewithout settling its heap future,settle_internal_error_if_pending/settle_spawn_engine_errorsettle still-pending child futures so awaiters wake and errors propagate instead of parking forever at end-of-run quiescence.Includes regression tests for nested-spawn / map+
future.alltopologies that previously hungcall_function.Reviewed by Cursor Bugbot for commit 2db4133. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit