feat(stella-model): streaming→non-streaming fallback on hung or empty streams, with a first-byte deadline (#2686) - #2748
Conversation
There was a problem hiding this comment.
Sorry @macanderson, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Reviewer's GuideImplements a streaming→unary fallback for Zai-based providers when streams hang before the first byte or are empty, adds a first-byte timeout to HTTP streaming, introduces a shared stream recovery latch, and wires a new StreamFallbackPosture parity axis and tests, while splitting Zai’s streaming/unary logic into dedicated modules and updating docs and baselines. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
… streams, with a first-byte deadline A provider stream that hangs before its first byte (a proxy buffering the SSE body) or comes back as a 200 with an empty stream used to burn the whole retry budget re-issuing the identical streaming request into the same broken pipe — the idle bound only fired after 120s per attempt, and nothing ever tried the transport that would have worked. Now the shared chat-completions adapter bounds the FIRST body read by a distinct 90s first-byte deadline (http::FIRST_BYTE_TIMEOUT), classifies the two nothing-arrived shapes as fallback-eligible, and arms a bounded per-session latch (stream_recovery::StreamRecovery) so the caller's ordinary retry of the faulted attempt re-issues the byte-identical payload with stream: false through the unary read bound. The recovery is deliberately split across two attempts rather than hidden inside one provider call: the faulted attempt fails retryably, so the engine's retry ladder stays the single owner of retries and bills the discarded attempt through its existing UsageIncomplete observer. The latch confirms only on unary evidence (Streaming → Probing → Confirmed), reverts on a failed probe, and never arms for a mid-stream death with salvage — partially streamed content keeps its retry-as-a-stream path and partial-usage accounting, so there is nothing to tombstone. Provider parity (invariant 8) gains its third axis, StreamFallbackPosture: the zai-family ids declare UnaryFallback with wiremock witnesses, the other streaming dialects declare the gap, Bedrock is AlwaysUnary. zai.rs's aggregation half moves to zai/stream.rs (shared assembly rules with zai/unary.rs), retiring its god-file baseline entry. Closes #2686
…, not retryable Transport Review catch on #2748: the fallback's unary dispatch mapped every send error to retryable Transport, so once the latch was Confirmed a generation longer than UNARY_READ_TIMEOUT would be re-issued identically until the retry budget died — the exact #547 storm, one adapter over. The unary path now routes send errors through http::classify_unary_dispatch_error, exactly as bedrock.rs does: a read-timeout consumed the whole generation bound and is Terminal; a connect failure generated nothing and stays retryable; the streaming dispatch's classification is untouched (there the same expiry is a header stall the next attempt may clear). Witnessed by a_unary_read_timeout_is_terminal_never_a_retry_storm, which drives dispatch with a millisecond read bound against a stalled mock and pins both directions. Refs #2686
ccb753c to
0ee24e6
Compare
…0s read-timeout to a retryable `Transport` error, reintroducing the #547 retry-storm even after the `.send()` path was fixed. This commit fixes the issue reported at crates/stella-model/src/zai/unary.rs:100 ## Status after commit `0ee24e6` The commit landed a partial fix that matches most of the original suggestion: - `ZaiProvider::dispatch` now takes a `unary: bool` flag (`zai.rs:1232`) and classifies the `.send()` failure with `http::classify_unary_dispatch_error` when `unary == true` (`zai.rs:1242`), keeping the streaming path's send timeout retryable (`false`). - Callers were updated: streaming passes `false` (`zai.rs:1085`), unary passes `true` (`unary.rs:97`). ## Remaining bug The **body read** in `complete_unary_attempt` was left unchanged and still uses the retryable mapping: ```rust let payload = response .text() .await .map_err(|e| ProviderError::transport(e.to_string()))?; ``` The unary client is built with `http::unary_client()`, whose `.read_timeout(UNARY_READ_TIMEOUT)` = **600s** bounds the *entire* response — head *and* body arrive within that single read window. If the connection succeeds but the body streams too slowly (a generation slower than 600s, or an LB that accepts then black-holes mid-body), `response.text()` yields an `is_timeout()` error. **Failure mode / concrete trigger:** a Z.ai unary-fallback completion whose body does not fully arrive within 600s. The timeout surfaces as retryable `Transport`, the driver re-issues the *identical* request, and it times out again — repeating until the retry budget is drained. This is exactly the #547 failure mode the `dispatch` send-path fix now guards against; the body read is the last unguarded seam of the same 600s read bound. ## Fix Classify the `response.text()` error with `http::classify_unary_dispatch_error` (timeout → `Terminal`, connect/ordinary transport → retryable `Transport`), matching the already-fixed `dispatch` send path and `bedrock.rs:768`. Co-authored-by: Vercel <vercel[bot]@users.noreply.github.com> Co-authored-by: macanderson <mac@macanderson.com>
…stream split dangled The doc-warnings gate documents private items (--document-private-items), so complete_inner's intra-doc link to a type #2748 moved into the private zai::stream module fails rustdoc -D warnings on every push to main, while a plain `cargo doc --no-deps` stays green locally — which is how it slipped through. Plain code-font reference with the file named instead. Verify: RUSTDOCFLAGS="-D warnings" cargo doc -p stella-model --no-deps \ --document-private-items --keep-going Refs #2686
…2748 and #2752 collided on Two green PRs composed into a red main: #2748 added zai/tests/stream_fallback.rs to adapter_sources (bumping the array to 15) and #2752 added http.rs (its bump to 15 from the other side); the textual merge kept both entries and one length, so provider_parity.rs no longer compiles under --all-targets and every cargo test on main is red. The length was a shared cell every source-adding PR had to write — the same shape that removed the spelled-out total from GATE_STEPS (#1883) — so the fix returns a slice and deletes the cell rather than writing 16 into it. Refs #2686, #2748, #2752
|
Audit trail: tests renamed/moved by this PR (the record
Also, for anyone reading that merge-check log: the |
×#2752) + the dangling private doc link (#2754) ## What & why **Main (f83692c) is red**, from two independent residues of #2748's admin-merge, and this PR repairs both: 1. **Compile error under `--all-targets`** — a merge composition, not a bug in either parent: #2748 added `zai/tests/stream_fallback.rs` to `provider_parity.rs::adapter_sources` (bumping the array length to 15) and #2752 added `include_str!("http.rs")` from the other side; the textual merge kept **both entries and one length**, so the array literal has 16 items behind a `[&'static str; 15]` return type and `cargo test`/`clippy --all-targets` fail on main (`E0308` at `provider_parity.rs:617`). The length was a shared cell every source-adding PR had to write — the exact shape that removed the spelled-out total from `GATE_STEPS` (#1883) — so the fix returns `&'static [&'static str]` and **deletes the cell** instead of writing 16 into it, with a comment naming the incident. 2. **rustdoc `-D warnings` failure** — #2748 moved `ToolCallAccumulator` into the private `zai::stream` module but left `complete_inner`'s intra-doc link pointing at it. The doc-warnings gate documents private items (`Makefile:247`, `--document-private-items`), so main fails with `unresolved link to 'ToolCallAccumulator::announced'` while a plain `cargo doc --no-deps` stays green — which is how it slipped through #2748's local check. Repointed as plain code font naming the file. ## The witness - [ ] Witness test - [x] No witness needed — both are build/doc repairs whose oracle is the gate itself, red on `main` and green here: ``` cargo test -p stella-model # main: E0308, could not compile → here: 390 passed RUSTDOCFLAGS="-D warnings" cargo doc -p stella-model --no-deps \ --document-private-items --keep-going # main: unresolved link, exit 101 → here: exit 0 ``` ## The gate - [x] `cargo fmt --check` - [x] `cargo clippy -p stella-model --all-targets -- -D warnings` (exit 0) - [x] `cargo test -p stella-model` — 390 passed, including #2748's fallback witnesses and #2752's overflow witnesses together - [x] gate-identical rustdoc (above), exit 0 ## Nothing left behind Nothing new; the remaining-dialects extension of #2686 stays tracked in #2746. Refs #2686, #2748, #2752
…resolve through the router, repair the transcript, continue the turn (#2769) ## What & why The engine held one provider for the whole turn, so a retry ladder exhausting against a wedged provider ended the turn `Aborted { Failure }` even when a healthy fallback was configured and resolvable (`driver.rs`'s abort path) — every completed step's work stranded. This PR is the last ticket of the Phase 2 reliability chain: on retries-exhausted the engine now re-resolves through the router and continues the turn on the replacement. **Trigger classes.** Everything that surfaces as the new `ModelCallFailure::Exhausted` — transport, 5xx, auth, and rate limiting that outlived #2744's parked recovery. `ContextOverflow` keeps its own rung (#2752); #2748's stream fallback still runs below, inside the attempt. The park composes rather than races: parking happens *inside* the ladder, the fallback only after the ladder gives up, and a soft stop typed during a park still wins at the next step boundary. **Router interplay (the #2734 seam, used as designed).** The fallback is a re-resolution via the new `stella_core::ports::FallbackResolver` port, never a hardcoded list. `drive_attempt_ladder` feeds `record_failure` **before** settlement asks for a fallback, so `Router::resolve` already routes around the sick provider; a resolution landing back on the failed provider is a refusal and the turn aborts exactly as before. The bare CLI loops attach a router-backed `SessionFallback` (`agent/engine.rs`) at both `run_turn` engine sites — the `session_router` doc's declared destiny for #2679. **Transcript repair, deterministic.** The failed call appended nothing, so the engine's own path is already well-paired; caller-supplied history with an orphaned `tool_use` is closed through the same `close_open_tool_calls` repair the cancel/soft-stop exits use (stub named for the swap, mirrored onto the event stream). Model-signed thinking blocks — the other thing a naive switch replays into a 400 — are structurally absent: `CompletionMessage` carries no reasoning blocks, so there is nothing to strip; the module doc records that argument. **Latch/bound.** `Engine::provider_override` is a set-once cell and the set IS the latch: at most one swap per engine, ever — two sick providers cannot ping-pong. The override persists for the engine's remaining turns; the breaker's cooldown/half-open cycle is what routes fresh engines back to a recovered primary. **What consumers see.** The ladder now *withholds* the terminal `RetriesExhausted`/`Error` pair (mirroring #2752's overflow arm) and settlement emits it only when no fallback fires — byte-identical to the old shape, pinned by a control test. A latched swap emits the existing `AgentEvent::ProviderFallback` (+ a retryable `Error` notice), so **no new AgentEvent variant** and no new consumer-ledger row (invariant #10 satisfied by reuse); the variant's doc and the generated `docs/wire` descriptions are updated (description-only diff, no shape change). **Accounting.** Every burned attempt still bills through the per-attempt `UsageIncomplete` observer — witnessed. No timings ride `ToolOutput`. **God files.** New logic is in sibling modules (`driver/model_fallback.rs`, the `overflow_recovery.rs`/`settlement.rs` pattern; tests in `driver/tests/model_fallback.rs`). `agent.rs` sat exactly at its ceiling, so the self-contained budget helpers moved to a new `agent/budget.rs` (move, not rewrite; re-exported so every caller path is unchanged) — agent.rs lands 40 lines *under* its inherited size. **Parity (invariant #8's cross-surface sibling).** `stella-parity` gains the `provider.midturn_fallback` row claiming `with_fallback_resolver` (the entry-point sweep enforces this); CLI posture is `ShippedUnwitnessed` with the gap cited (#2733 for the attachment witness, #2765 for pipeline wiring), API `NotApplicable` for the same reason as `provider.breaker_feedback`. `UNWITNESSED_BASELINE` 4 → 5 — the declared-debt direction the ratchet exists to make visible, not an expedient. **Exemplar.** The module shape, withheld-events discipline, latch bound, and test suite deliberately mirror `driver/overflow_recovery.rs` + `driver/tests/context_overflow.rs` (#2752), this repo's canonical recovery-rung shape. Closes #2679 Refs #2733, #2734, #2744, #2748, #2752, #2765 ## The witness - [x] This PR includes a witness test (fails on `main`, passes here) `crates/stella-core/src/driver/tests/model_fallback.rs`: - `exhausted_retries_swap_to_the_resolved_fallback_and_the_turn_completes` — the headline #2679 witness: terminally failing primary, healthy resolved fallback; on main the turn aborts, here it completes with the terminal channels silent and the swap announced. - `a_sick_fallback_is_never_swapped_again` — the bound: one ladder per provider, the resolver asked exactly once, second exhaustion terminal (no ping-pong). - `the_transcript_handed_to_the_fallback_is_well_paired` — pairing asserted on the transcript the replacement provider *actually received*, orphan stubbed with the swap's wording. - `every_attempt_before_the_swap_is_billed_through_usage_incomplete` — 1 attempt + 2 retries = 3 envelopes, then the rescue. - Controls: `without_a_resolver_the_terminal_surfacing_is_unchanged` (the relocated emission is byte-identical), `a_resolution_back_onto_the_failed_provider_is_refused`. **Flip evidence, artisanal (the ablation form, as in #2752 — the true on-main run cannot compile because the port doesn't exist there):** with the one interception main lacks disabled (`if false && self.attempt_provider_fallback(...)` in `settle_model_call_failure`), the suite runs `2 passed; 4 failed` — all four behavior witnesses FAIL (turn aborts, exactly main's behavior) while both abort-path controls stay green; restored, `6 passed; 0 failed`. ## The gate - [x] `cargo fmt --check` (via `make guards-fast`, exit 0 — all toolchain-free guards incl. file-size, god-files, left-behind, typed-errors, module-reachability) - [x] `cargo clippy -p stella-core -p stella-cli --all-targets -- -D warnings` — exit 0 - [x] `cargo test -p stella-core -p stella-cli -p stella-parity` — 1130 + 1693 + 9 (+ integration targets), 0 failed; `cargo test -p stella-pipeline -p stella-serve -p stella-protocol` — 0 failed - [x] `make wire-schema` — regenerated, description-only diff (read above) - [x] `RUSTDOCFLAGS="-D warnings" cargo doc --no-deps -p stella-core -p stella-cli -p stella-parity -p stella-protocol --document-private-items --keep-going` — exit code 0, checked unpiped - [x] Docs updated where behavior changed (module docs, event doc, parity row, `session_router` doc un-staled) - [x] CLA signed - [x] `Closes #2679` appears both above and as a commit trailer ## Nothing left behind - [x] Filed: #2765 — pipeline execute-stage (and serve/runtime) wiring for the `FallbackResolver` port, written as a handoff. The CLI-attachment witness gap was already tracked in #2733 (cited by the parity row rather than duplicated). ## Ground-rule check - [x] No I/O added to `stella-core` (the resolver is a port; the CLI impl owns the adapter build); no new deps - [x] No new outbound network calls - [x] No new cross-boundary types — `ProviderFallback` reused; `docs/wire` diff is description-only ## Anything reviewers should know? - Terminal-event emission moved from `drive_attempt_ladder` into `settle_model_call_failure` (a settlement decision now that exhaustion has a recovery). The control test pins the observable shape; the one ordering change is that the `model.request.failed` bus signal now precedes the terminal pair — cross-channel (hook bus vs event stream), so no consumer can order them anyway. - `ModelCallFailure::Fatal` is renamed/retyped to `Exhausted { message, attempt_reasons, retryable }` — it is `pub(crate)`, no external surface. - Sub-agent child engines deliberately do NOT inherit the resolver: a child's provider is the spec's explicit (possibly pinned cross-family) choice; the commented decision is at `subagent.rs`'s engine assembly. - The prompt cache is necessarily cold on the replacement provider — the documented cost of finishing the turn at all. ## Summary by Sourcery Introduce mid-turn provider fallback in the engine so exhausted retry ladders can re-resolve through the router and continue the turn on a replacement provider, while preserving existing terminal behavior when no fallback is available. New Features: - Add a FallbackResolver port and mid-turn provider fallback mechanism that re-resolves the worker role via the router after retries are exhausted and continues the turn on a replacement provider. - Wire bare CLI loops to attach a session-scoped router-backed SessionFallback so non-pipeline runs can benefit from mid-turn provider failover. Bug Fixes: - Ensure exhausted retry ladders no longer abort turns when a healthy fallback provider is configured and resolvable, preventing work from being stranded mid-turn. Enhancements: - Refine engine failure handling so retry exhaustion is settled in Engine::settle_model_call_failure, enabling either provider fallback or terminal surfacing with unchanged external shape. - Ensure transcript repair and billing behavior around mid-turn provider swaps keep tool-call pairing valid and preserve per-attempt UsageIncomplete accounting. - Add a one-swap-per-engine latch and route all dispatch/attribution through the active provider, avoiding provider ping-pong and mis-attribution after a fallback. - Clarify AgentEvent::ProviderFallback semantics to cover both resolution-time and mid-turn provider substitutions without changing the wire shape. - Keep sub-agent engines opt-out of fallback so their explicitly chosen providers surface failures back to the parent tool call. Documentation: - Update wire schema docs, parity capabilities, and session_router documentation to describe the new mid-turn fallback behavior and its current CLI and API posture. Tests: - Add driver/model_fallback tests that witness successful mid-turn fallback, enforce the one-swap bound, verify transcript repair and accounting, and confirm terminal behavior remains byte-identical when no fallback is attached. Chores: - Extract session budget helper functions from agent.rs into a dedicated agent/budget.rs module to keep the god file under its size ceiling. - Extend stella-parity capabilities with a provider.midturn_fallback entry and bump the unwitnessed baseline to track the new unwitnessed CLI posture.
What & why
A provider stream that hangs before its first byte (a proxy buffering the SSE body) or comes back as a 200 with an empty stream used to burn the whole retry budget re-issuing the identical streaming request into the same broken pipe: the per-chunk idle bound only fired after 120s per attempt, classified as retryable Transport, and every retry streamed again — a provider that would answer fine over a non-streaming request was treated as down. This is #2686's zappy-style "reactive resilience" fallback, adapted to Stella's accounting discipline.
Three pieces, all in
stella-model(the engine is untouched):http::FIRST_BYTE_TIMEOUT(90s, matching the comparator's stream watchdog) bounds only the FIRST body read; any chunk at all moves the stream onto the ordinary 120sSTREAM_IDLE_TIMEOUT. It composes with — rather than duplicates — the engine's 816s idle-based model deadline: a hang now surfaces in 90s instead of 120s/816s.http::next_stream_read(the newStreamReadenum) keeps Idle distinct from Failed;next_with_timeoutis reimplemented on top of it so there is one truth.src/stream_recovery.rs): Streaming → Probing → Confirmed. A fallback-eligible fault (hung before the first byte, or the stream died having delivered no completion signal whatsoever) arms the probe; the probe's unary success confirms the latch for the session (a buffering proxy buffers deterministically); a failed probe reverts to streaming, so one unproven fault never condemns the streaming path. The retried attempt re-issues the byte-identical body withstream: false(zai/unary.rs) throughhttp::unary_client's 600s read bound (stella-model: Bedrock's 120s read timeout bounds the entire generation, and the failure is retryable #547's lesson — a unary call has no first token to reset the clock).UsageIncompleteobserver (driver.rs::run_model_call) — nothing arrived by construction of the eligibility rule, sopartialis honestlyNone, exactlyhttp::partial_usage's "attach nothing rather than a zeroed envelope" discipline. A mid-stream death WITH salvage is deliberately ineligible: it keeps its retry-as-a-stream path and itsattach_partialaccounting, so there is no partial transcript to tombstone, ever.Invariant 8 (parity, declared not assumed): the matrix gains its third axis,
StreamFallbackPosture. The shared chat-completions adapter's identities (zai,openrouter,xai,deepseek,local, settings-defined gateways) declareUnaryFallbackwith named witnesses;anthropic/openai/gemini/vertexdeclareStreamingOnlywith the gap tracked in #2746;bedrockisAlwaysUnary(it calls Converse — there is no stream to fall back from). Enforced from both sides like the other two axes:stella-cli's config test fails a seeded provider with no row, and the parity tests fail a row whose witness rotted. AGENTS.md invariant 8 and the crate README are updated in the same PR (stale "two axes" prose is a bug).Exemplar: the latch follows zappy's watchdog-and-fallback shape; the split-across-attempts structure follows this repo's own
retry.rs/UsageIncompletepattern rather than an internal hidden resend, so accounting and attempt-bounding stay owned by one place.God files:
zai.rswas at its 1565-line ceiling, so the SSE aggregation half moved tozai/stream.rs(thedriver/settlement.rssplit pattern) and the unary path landed inzai/unary.rs;zai.rsis now under 1500 and its baseline entry is retired (minimal baseline edit — deliberately NOT a fullfile-size-updateregen, to avoid the parallel-merge retighten skew).zai/tests.rswas also at its ceiling, so the threereasoning_aware_max_tokenstests moved tozai/tests/zai_effort.rs(tests moved, not deleted — same names, new file). The streaming and unary paths share one set of assembly helpers (tool_call_input,promote_reasoning_as_text,fold_usage,final_finish_reason) so they cannot drift.Closes #2686
The witness
main, passes here)crates/stella-model/src/zai/tests/stream_fallback.rs:a_stream_hung_before_its_first_byte_falls_back_to_a_non_streaming_request— the headline IMPROVEMENT: streaming→non-streaming fallback on hung or empty streams, with a separate first-byte deadline #2686 witness. Wiremock cannot hold a response body open (its delays cover the whole response), so this one uses a minimal hand-rolled std-TCP server that answers a"stream":truerequest with headers and not one body byte, and a"stream":falserequest with a real completion. First call faults retryably at the first-byte deadline naming the switch; second call completes unary.an_empty_stream_falls_back_to_a_non_streaming_request— the other broken shape (200 + EOF before any data), wiremock-based; the unary response carries a tool call, proving the fallback parses the whole dialect.a_healthy_stream_never_arms_the_fallback(every request keepsstream:true),a_stream_that_died_after_content_is_retried_as_a_stream_not_unary(eligibility boundary),a_failed_unary_probe_reverts_the_session_to_streaming(wire sequencestream → unary → streamproves the reversion), plus latch state-machine units instream_recovery.rs.Flip evidence, run the artisanal way (
git checkout origin/main -- crates/stella-model, inject the empty-stream witness,cargo test -p stella-model an_empty_stream_falls_back):— on main the fault carries no fallback and the retry re-streams; with the change the same suite is green (382 passed). The hung-first-byte witness additionally cannot compile on main (
with_first_byte_deadlineand the latch don't exist).The gate
cargo fmt --checkcargo clippy -p stella-model -p stella-cli --all-targets -- -D warnings(exit 0)cargo test -p stella-model(382 passed) andstella-cli's seeded-provider parity tests (3 passed) after rebasing ontoorigin/mainat feat(stella-protocol): a signal-consumer ledger — every AgentEvent variant declares what reads it #2720's ledger mergeRUSTDOCFLAGS="-D warnings" cargo doc --no-deps -p stella-model -p stella-cli— exit 0, checked unpipedcrates/stella-model/README.md(layout + god-file list), module docsCloses #2686appears both here and as a commit trailerNothing left behind
anthropic,openai,gemini/vertexdialects (written as a full handoff; theStreamingOnlyparity rows cite it). Bedrock needs nothing.Summary by Sourcery
Introduce a streaming→unary fallback in the shared OpenAI-compatible adapter when streams hang before the first byte or return empty, backed by a per-session latch and a first-byte timeout, while keeping retry and usage accounting unchanged.
New Features:
Enhancements:
Documentation:
Tests: