fix(stella-core): park the retry ladder under sustained rate limiting instead of aborting (#2677) - #2744
Merged
Merged
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
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
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
Reviewer's GuideImplements supervised, budget-aware parking for sustained 429 rate limiting by extending the retry engine with a ParkSupervisor abstraction, wiring an engine-specific park supervisor into the model-call attempt ladder, and adding tests and docs to ensure deterministic policies and auxiliary calls keep their existing fail-fast behavior. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
macanderson
force-pushed
the
fix/2677-budget-aware-retry-parking
branch
from
August 10, 2026 21:07
32f7f3c to
550d022
Compare
macanderson
enabled auto-merge (squash)
August 10, 2026 21:08
… instead of aborting RetryPolicy::standard()'s inline ladder (6 retries, ~16s of total backoff) treated a minutes-long 429 brownout and a transient blip identically, and a Retry-After past the 120s server-hint ceiling failed Terminal outright — of budget left (8/16 cells in one panel). Abort becomes recovery: a rate limit the inline ladder cannot absorb now parks through a new ParkSupervisor port in stella-core::retry — pure plan_rate_limit_park decides the wait (server hint honored verbatim when it fits, doubling 30s→5min backoff when hint-less), the loop sleeps it in 30s chunks through the existing Sleeper port, and the supervisor is consulted per chunk. The engine's supervisor (driver/rate_limit.rs, split out of driver.rs which shrinks by 76 lines) derives the allowance from BudgetGuard's task deadline minus a 30s wake reserve, capped at 6h absolute; emits the TurnParked/TurnWoken span plus per-chunk keep-alive narration; and ends the park on a soft stop. A stated wait that cannot fit the remaining budget still fails fast — waiting less than the server asked guarantees a re-429. retry_with_backoff and every auxiliary call (accounted_call) keep NoParking, so triage/summarizer paths and L-M4 are unchanged. Witness tests (fail on main, pass here): - driver::tests::parked_wait::sustained_rate_limiting_parks_within_budget_and_recovers - driver::tests::parked_wait::a_long_retry_after_hint_is_honored_by_parking_when_budget_allows Closes #2677 Closes #2667
macanderson
force-pushed
the
fix/2677-budget-aware-retry-parking
branch
from
August 10, 2026 21:34
550d022 to
e99cf50
Compare
macanderson
added a commit
that referenced
this pull request
Aug 10, 2026
…t (torn comment, orphaned rate_limit module, doubled retry ladder) (#2759) ## What & why **Main is red at `f83692c82`** — stella-core does not compile. The admin merge of #2752 raced its in-flight rebase over #2744, and the squash text-merged a pre-#2744 branch onto post-#2744 main. Three artifacts in `crates/stella-core/src/driver.rs`: 1. A doc comment torn mid-word at line 1715–1716 (`// lives in `driver/` + a bare `.rs`.` line with no `//`) — the workspace-wide compile error. 2. `mod rate_limit;` was *replaced* by (instead of joined with) `pub(crate) mod overflow_recovery;`, orphaning `driver/rate_limit.rs` — the `check-module-reachability` failure. 3. `run_model_call` contained **both** retry ladders: #2744's extracted `drive_attempt_ladder` call *and* the dead pre-#2744 inline `retry_with_backoff_observed` block — and #2680's `ContextOverflow` interception lived only in the dead copy. ## The fix Single-ladder shape restored, with the two features composed where they belong: `drive_attempt_ladder` (`driver/rate_limit.rs`) now returns `ModelCallFailure`, and on a `ContextOverflow` withholds the terminal events and the breaker feed (an oversized request is the engine's accounting miss, not provider ill-health) so `settle_model_call_failure`'s recovery rungs (#2680) work; every other terminal error keeps the `RetriesExhausted`/`Error` pair + `record_failure` and returns `Fatal`. `RateLimited` parking (#2677) is untouched inside the ladder — the two recoveries are disjoint by error class. Net: −60 lines, driver.rs well under its ceiling. ## Evidence - Both features' witnesses pass on the composition: `context_overflow` (3/3) and `parked_wait` (6/6). - `cargo test -p stella-core --lib`: 1124 passed / 0 failed. - clippy `--all-targets -- -D warnings` clean; `cargo fmt --all` applied; unpiped `RUSTDOCFLAGS="-D warnings" cargo doc --no-deps -p stella-core` exit 0. - `check-module-reachability`: OK (954 files, all reachable). `check-file-size`: OK. ## Companion The stella-model half of the red main (adapter_sources collision + doc link) is **PR #2754** — the two PRs touch different crates and compose; both are needed for green. ## Deleted tests None. (The deleted inline block was production code duplicated by the merge, not tests.) Refs #2752 #2744 #2680 #2677 ## Summary by Sourcery Restore stella-core’s model call retry ladder after a bad merge so main compiles and error handling is correctly composed. New Features: - Propagate model call failures from the rate-limit ladder via ModelCallFailure, including explicit ContextOverflow handling. Bug Fixes: - Fix a broken doc comment in driver.rs that prevented stella-core from compiling. - Reintroduce the rate_limit module alongside overflow_recovery to restore module reachability. - Ensure ContextOverflow and fatal error handling are wired through the unified drive_attempt_ladder path instead of duplicated inline logic. Enhancements: - Centralize terminal event emission and provider outcome recording inside the rate_limit ladder, reducing duplication and shrinking driver.rs.
13 tasks
macanderson
added a commit
that referenced
this pull request
Aug 10, 2026
…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.
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.
What & why
Under a sustained 429 brownout,
RetryPolicy::standard()'s inline ladder (6 retries, ~16s total backoff) exhausted in seconds and aRetry-Afterpast the fixed 120s ceiling failedTerminaloutright — even with hundreds of seconds of task budget left. #2667 measured the loss: 7 attempts burned in 15s, killing a trial with 880s of its 900s budget remaining (8/16 cells inturnloop-cert-panel1). The comparator lineage retries 429s reset-aware for hours with chunked keep-alives.Abort becomes recovery. A rate limit the inline ladder cannot absorb now parks instead of dying, bounded by real wall-clock headroom:
crates/stella-core/src/retry.rs— a newParkSupervisorport (the same seam shape asSleeper: the retry driver stays pure decision logic; clock, budget, events, and the soft-stop latch never leak in). The pure plannerplan_rate_limit_parkdecides the wait: a serverRetry-Afterthat fits the allowance is honored verbatim (waiting less than the server asked guarantees a re-429, so an unaffordable hint still fails fast — the fail-fast the old fixed ceiling provided survives, now measured against real headroom); hint-less parks follow a doubling 30s→5min backoff, the comparator's cap. Long waits sleep in 30s chunks with a supervisor tick before each one. Exemplar for the port-with-no-op-default shape:tokio's injected-time seam / the crate's ownSleeper+ports::Clockdiscipline.crates/stella-core/src/driver/rate_limit.rs(new sibling module, thesettlement.rspattern) — the whole attempt-ladder block moves out of god-filedriver.rs(which shrinks by 76 lines), and the engine's supervisor lands beside it: allowance = task-deadline remaining minus a 30s wake reserve, capped at an absolute 6h;TurnParked/TurnWokenspan (no newAgentEventvariant — the existing Parked waits: dedicated wire events (TurnParked/TurnWoken) and a TUI heartbeat instead of synthetic Text #1857 span fits and keeps this orthogonal to feat(stella-protocol): a signal-consumer ledger — every AgentEvent variant declares what reads it #2720's ledger) plus a per-chunk keep-aliveTextnarration for liveness; soft stop ends the park at the next chunk.RetryPolicy::deterministic()and every auxiliary call (accounted_call.rs— summarizer/triage/authoring) keepNoParking, matching the comparator's background-source posture; the park sits between model attempts, never mid-tool, and the budget's deadline bounds it from outside.Anthropic's
Retry-Afteralready reachesRateLimited::retry_after_msviaparse_retry_after_ms/classify_http_status(crates/stella-model/src/http.rs, 429 arm — wiremock-covered there); this PR consumes that hint at the seam where the abort actually happened.stella-modeldoes not linkstella-core, so the recovery witness lives at the driver seam with a scripted provider.Closes #2677
Closes #2667
The witness
main, pass here)Verified the artisanal way —
git checkout origin/main -- crates/stella-core/src/{retry,driver,accounted_call}.rs && rm crates/stella-core/src/driver/rate_limit.rswith the test file kept (the driver witnesses use only pre-existing API), thencargo test -p stella-core --lib parked_wait:driver::tests::parked_wait::sustained_rate_limiting_parks_within_budget_and_recovers— fails on main (Aborted { reason: "model call failed: provider rate limited: throttled", kind: Failure }), passes here: 9 hint-less 429s → 6 inline retries + 3 parks → completed step,TurnParked/TurnWokenon the stream, noRetriesExhausted.driver::tests::parked_wait::a_long_retry_after_hint_is_honored_by_parking_when_budget_allows— fails on main (Terminal: …asked to wait 300000ms… past this call's 120000ms server-hint ceiling), passes here: the 300s hint parks (span deadline 300–338s incl. jitter) and the step completes.a_hint_past_the_remaining_deadline_still_fails_fast_without_parkingpins the kept fail-fast half (60s deadline vs 300s hint → clean abort, no park).retry::tests::sustained_rate_limiting_beyond_the_ladder_parks_and_recovers,an_abort_tick_ends_the_park_and_surfaces_the_error,the_deterministic_policy_never_parks_even_with_allowance, and propertya_planned_park_never_exceeds_the_allowance_and_never_undercuts_the_hint(proptest, per the crate's pure-engine-logic testing idiom). All pre-existing retry tests pass unchanged —retry_with_backoffrunsNoParking, preserving pre-CORRECTION: retry ladder fails fast under sustained rate limiting — make the Retry-After ceiling budget-aware with parked long waits #2677 behavior exactly for supervisor-less callers.The gate
cargo fmt --checkcargo clippy --workspace --all-targets -- -D warnings(stella-core + all dependents checked)cargo test -p stella-core(1089 passed) — full workspace runs in CIretry.rs,max_server_hint_mssemantics, new module doc)Closes #Nappears both above and as commit trailersNothing left behind
Transport, so a sustained 529 brownout never reaches the park), stella-core: a soft stop during a rate-limit park aborts the turn as a Failure instead of the graceful boundary soft-stop exit #2743 (a soft stop during a park surfaces as aFailureabort instead of the graceful boundary soft-stop exit)Ground-rule check
stella-core(the supervisor is a port; the only clock read isInstant::nowin the driver, matching the existing neighborhood); no new depsTurnParked/TurnWokenreused; nothing added tostella-protocol)Anything reviewers should know?
retry_with_backoff_observedgained a required&mut dyn ParkSupervisorparameter (it ispub(crate)); the publicretry_with_backoffsignature is unchanged.RateLimited(soRetriesExhausted::retryablestays truthful per RetriesExhausted is emitted when no retry was ever attempted #926); only a genuinely oversized-and-unaffordable hint keeps the explanatoryTerminal, now naming hint, inline ceiling, and remaining headroom.AgentEventvariant to stay orthogonal to feat(stella-protocol): a signal-consumer ledger — every AgentEvent variant declares what reads it #2720's consumer ledger; if a typed per-chunk heartbeat is wanted later it should be added post-ledger.max_parked_wait_msfield onRetryPolicy— the allowance is dynamic (deadline-derived), so a policy constant would either lie or duplicate the supervisor's answer.Summary by Sourcery
Add budget-aware parked waiting for sustained provider rate limiting and refactor the model-call retry ladder into a dedicated driver module.
Bug Fixes:
Enhancements:
Tests: