Skip to content

feat: add bounded model rate-limit recovery - #141

Open
zhuowp wants to merge 5 commits into
pinvou3-cleanfrom
feat/provider-rate-limit-recovery
Open

zhuowp wants to merge 5 commits into
pinvou3-cleanfrom
feat/provider-rate-limit-recovery

Conversation

@zhuowp

@zhuowp zhuowp commented Nov 5, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Temporary model throttling can exhaust the ordinary short network retry loop before capacity recovers. This fork adaptation adds an independent 429 policy, shared effective-credential/account admission, suspendable HTTP leases, stable logical-operation budgets, persisted server minima, and checkpoint-preserving root/owned-child interruption. Successful responses end the shared refusal episode and restore capacity gradually. Recovery receipts remain metadata carried by the existing lifecycle.

The follow-up in c04edec47 adapts the applicable upstream transport review fixes:

  • Bound native MiniMax/Zai JSON inspection at 32 MiB and apply first-byte/idle deadlines while retaining caller totals and existing SSE handling.
  • Preserve the maximum header/body/prose/nested server hint, the local backoff floor, and jitter above server minima. Long minima that cannot fit the operation budget return a terminal quota window, including with transport retries disabled.
  • Preserve the actual last transport failure when recovery runs out, restore ordinary recovery receipts, and count local token admission and ordinary network backoff toward active deadlines.
  • Keep raw classification separate from error disclosure. Both senders share the display boundary, including decoded JSON string values; tests exercise ordinary and isolated HTTP/native-envelope paths.
  • Remove the transport-level checkpoint claim. Checkpoint preservation remains owned by the existing Engine and child lifecycle.

The fork retains full persisted reset times and its shared operation recovery semantics. It does not import the newer upstream HTTP-attempt ledger or account-guidance APIs into this older foundation. Details are in the recovery notes.

Upstream and integration status

  • Applicable transport fixes were adapted from feat: add bounded HTTP rate-limit recovery codewhale-hq/Codewhale#7220 at 159c25f3299b0ebf71e685e98470b3f41cc2b490.
  • The follow-up preserves the existing candidate 8f3e80e9b95029303593d66ed8f36049e916fa06 as its parent; the original feature is based on f1eb10efcfe2468fde42d1724d7f5f19d54b904c.
  • Merge commit 19132162830b38aba7d900ef806427171d9203ef resolves the target conflict against public maintenance 11cba403d while retaining the transport follow-up. This remains a draft pending applicable current-head CI and review.
  • Desktop review PR feat: recover model calls from rate limits pinvou-agent#830 now pins this integration and updates its inventory/fingerprints. Before Ready, it must still consume the accepted public maintenance head and pass its public-submodule gate.

Maintenance conflict integration (2026-11-05)

19132162830b38aba7d900ef806427171d9203ef merges c04edec47 and public maintenance 11cba403d
without rewriting either history. The only manual source conflict combines the
child tool recovery observer with the existing write-reservation completion leg:
execute once under the observer, settle the reservation, then return the result.
Cancellation while executing or waiting for settlement retains Drop reconciliation.

Fresh Windows integration evidence:

  • Linked TUI rate_limit filter: 74 passed, 0 failed.
  • Five exact reservation, cancellation and lock-wait cleanup tests: each 1 passed, 0 failed.
  • cargo fmt --all -- --check: passed.
  • Desktop review integration: all-target dev-tools metadata, relevant Node contracts,
    architecture and fork/ancestry guards passed in feat: recover model calls from rate limits pinvou-agent#830.

The Unix-only background-shell integration cases, full workspace lint/behavior
suites and non-Windows execution were not rerun here; current-head CI remains
required. The prior follow-up's evidence below keeps its original commit boundary.
Protected maintenance and immutable tags are unchanged.

Validation at transport follow-up c04edec47

Windows, rustc 1.99.1, locked offline dependencies. The real TUI library test target built successfully on the reviewed source. Test child processes used the existing fork CI/dev-script 16 MiB stack setting.

Selected test functions Reported passed Failed
rate_limit 74 0
Ordinary LLM/retry tests, excluding prior selections 20 0
Vision, excluding prior selections 18 0
Governor, excluding prior selections 12 0
Stable model operations, excluding prior selections 22 0
Three exact Compaction error-reporting tests 3 0

Each selected group had a matching registration list. The sets contain 149 different test functions; these are not counts of independent workflows. The existing Windows symlink test returned early on permission error 1314, so its containment assertion was not executed.

The rate-limit group includes real loopback HTTP requests, quota persistence/admission, root/owned-child interruption and recovery receipts. One function covers the 12 ordinary/isolated, JSON representation and response combinations. Additional assertions cover native body limits/deadlines, the local backoff floor, active-time accounting, and actual-error preservation after budget expiry. Source-matched display-helper probes also verified the correction against prior behavior and boundary inputs; they are helper evidence alongside the actual HTTP fixtures.

  • cargo fmt --all -- --check: passed.
  • Workspace/all-features Clippy: passed with -D warnings and the five existing fork CI allowances (uninlined_format_args, too_many_arguments, unnecessary_map_or, collapsible_if, assertions_on_constants).
  • Default-feature TUI behavior tests were run as above. Whole-workspace/all-feature behavior, non-Windows runtime, live-provider traffic, GUI/full-host restart, and packaged artifacts remain outside this local verification.

Checklist

  • Updated affected behavior tests and recovery documentation.
  • Verified the original transport follow-up's formatting, workspace lint and selected real test target.
  • Resolve target conflicts against 11cba403d and publish the review integration.
  • Obtain applicable current-head CI and external approval.
  • Repin desktop review PR fix(session): scope latest resume to workspace codewhale-hq/Codewhale#830 and compile its integration.
  • Before desktop Ready, consume the accepted public maintenance head and pass public-submodule gates.

No-Issue: Issues are disabled in this repository. This PR adapts the transport work in codewhale-hq#7220 for the Pinvou lifecycle; the companion integration is Pinvou/pinvou-agent#830.

Follow-up for fork CI and desktop review

Fixed commit c38db6a49497d6248d7f4a3654ea0fc56557ab0a, tree 77eb9590a2bdb249adb5519bc790bbbe49b08cde. The scoped repair passed independent v3 review.

  • Preserve the isolated Messages envelope and request counts, correct the deferred permit test, and make child spawn check the resolved provider credential's live or saved quota before workspace creation.
  • Start the recovery deadline on a confirmed refusal. Publish and save explicit minima before diagnostic reads, and bound quota persistence and provider-slot admission. Preserve the typed quota outcome if receipt delivery stalls. Do not replay a POST after accepted stream headers, including an incomplete native JSON response.
  • Update the locked rio-vt family to 0.5.29 for RUSTSEC-2026-0430. Windows format and workspace Clippy passed. The fixed tree passed 25 targeted tests, including the four prior fork CI failures. The Windows workspace run stopped at the existing codewhale-app-server /tmp/codewhale-primary fixture, which is unchanged from the maintenance target. The fresh Linux CI and dependency audit remain separate.
  • feat: recover model calls from rate limits pinvou-agent#830 still points to 191321628. Re-pin and validate the combined host tree after the accepted CodeWhale commit enters public maintenance.

Signed-off-by: z27014 <zhuo.wenpei@h3c.com>
Adapt the applicable upstream transport fixes while preserving the fork's
persisted resets, shared admission and stable operation recovery budget.
Bound native JSON inspection, preserve all server minima and local backoff
floors, retain actual failures and ordinary recovery receipts, and count
local admission and network backoff toward active deadlines. Keep raw
classification separate from HTTP error disclosure and document the
foundation compatibility boundary.

Validation on Windows with locked offline dependencies:
- Real TUI lib test target built successfully
- rate_limit filter: 74 passed, 0 failed
- Related retry, vision, governor, operation and compaction filters:
  75 additional non-overlapping functions reported passed, 0 failed
- Existing Windows symlink assertion not executed (permission 1314)
- Workspace formatting passed
- Workspace/all-features Clippy passed with the five existing CI allowances
- Source-matched disclosure helper negative control and boundaries passed

Refs codewhale-hq#7220

Signed-off-by: z27014 <zhuo.wenpei@h3c.com>
Signed-off-by: z27014 <zhuo.wenpei@h3c.com>
Signed-off-by: z27014 <zhuo.wenpei@h3c.com>
@zhuowp
zhuowp requested a review from asto18089 November 9, 2026 10:12
Merges pinvou3-clean@6f7a3a748 (#136/#142/#148/#150/#152) into the
rate-limit recovery branch, preserving both histories per the
established integration pattern.

Conflict resolutions (composed, not one-sided):
- client.rs: keep the deferred `prepare_outbound_request` fallback from
  the maintenance route-budget work and wrap the prepared-stream
  operation in `rate_limit::with_operation` so recovery still owns the
  logical operation regardless of preparation timing.
- turn_loop.rs: adopt the maintenance-typed `TurnFailure` +
  `split_stream_open_error` plumbing while keeping the rate-limit
  interruption contract: a RateLimited/QuotaWindow failure parks the
  turn as `Interrupted` (message surfaced, no terminal error source
  consumed) and otherwise `finish()` records the typed source as
  before. Both stream-open sites classify first, then flag
  `rate_limit_interrupted`.

Signed-off-by: asto18089 <asto18089@126.com>

@asto18089 asto18089 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fresh 13-shard review (2026-11-12), zero prior-review reuse. I first integrated the branch onto the current base: head is now 6735bf890 (a merge of pinvou3-clean@6f7a3a748 — #136/#142/#148/#150/#152 — into c38db6a49, composed rather than one-sided; PR is MERGEABLE again). The composition itself is in review scope: client.rs keeps base's deferred prepare_outbound_request fallback and wraps the prepared-stream operation in rate_limit::with_operation; turn_loop.rs adopts #142's typed TurnFailure/split_stream_open_error while keeping the PR's interruption contract (rate-limited → Interrupted + message, no terminal error source consumed; everything else → finish()). If you disagree with any resolution, say so — it is one commit to redo.

Gates (this machine, Linux/arm64, rustc 1.99.1)

  • cargo fmt --all -- --check: pass.
  • Clippy, lib+bins, with the fork-CI allowance set: only too_many_arguments ×6 and collapsible_if ×2 — exactly the families fork-ci -As, so the lint lane is green-equivalent. (With --tests added, five warn-level findings appear in the new test files — field_reassign_with_default ×3, is_multiple_of, items_after_test_module — but fork-ci's lint step does not lint test targets; listing for hygiene only.)
  • cargo test -p codewhale-tui --lib rate_limit: test result: ok. 81 passed; 0 failed (4 runs, incl. single-threaded). governor: 17 passed; 0 failed. operation: 61 passed; 0 failed under the fork's 16 MiB stack; the default-stack abort is the pre-existing runtime_api test, not this PR.

1. Claim completeness — delivered and root-caused

Every audited claim maps to a mechanism with a non-vacuous test: shared per-credential admission (SharedQuota keyed by sha256(endpoint + credential-ish headers), root/children/auxiliary clones share one gate); genuinely suspendable HTTP leases (slot released in all three backoff paths — at base the slot was held across backoff, so this is the real fix, not a reshuffle); stable logical-operation budgets (identity reused across transparent reopens, 24h/65k-bounded retention); persisted server minima (atomic tmp+rename, 4 KiB-bounded read, survives restart); rate-limited turns park Interrupted with children checkpointed (api_rate_limited), explicit cancel still cancels; AIMD episode end on 2xx; receipts ride the existing event channel only (KV-prefix untouched, no volatile splice); 32 MiB envelope bound enforced twice; delay = local.max(minimum) + local/2 (jitter above server minima); unfitting minima → terminal QuotaWindow in both retry arms; actual last transport failure preserved at exhaustion; raw classification separated from disclosure (classified on raw, errors built only from sanitized copy, incl. decoded JSON string values); no POST replay once stream headers are accepted (outcome_known() blocks the H1 fallback). The upstream adaptation (codewhale-hq#7220 @ 159c25f32) is a faithful minimal port; both claimed non-imports (HTTP-attempt ledger, account-guidance APIs) are verifiably absent. Deletion audit: all ~971 deleted lines are moved/superseded/obsolete-with-successor; DynamicGate moved to core/capacity.rs byte-identical (diffed), re-exported, no second gate invented.

2. Smuggling — clean

Commit-by-commit accounting of all 33 files: every hunk serves the rate-limit claim or declared infrastructure. No new env vars, endpoints, telemetry, platform gates, or feature flags. http = "1.5" is used directly for the envelope rebuild. Two accuracy notes only: the rio* 0.5.29 bump in c38db6a49 nets to zero against this base (#152 already locked it — the claim is stale at the integration head, harmless), and native classification also covers qwen/dashscope/glm families that the claim's "MiniMax/Zai" headline understates. events.rs enum reformatting and turn_loop reindentation are not rustfmt-required and ride the diff (churn, zero semantics).

3. Bugs

MAJOR — the shared operation_permit cell cross-wires leases between concurrent operations; admission can over-admit in exactly the PR's target scenario. operation_permit: Arc<Mutex<Weak<ProviderRequestPermit>>> (client.rs:305) is cloned into every clone of the client (client.rs:884 — same lineage as request_concurrency, so parent turns, detached children, and vision auxiliary calls share one cell and one gate). Every acquire_provider_request_permit() overwrites the cell with its own Arc::downgrade (client.rs:2776-2779), and the admission loop (client.rs:3575-3582) and retry callback (client.rs:3538 → suspend at ~3818-3825) upgrade() whatever is current. Interleaving with default concurrency ≥ 1: parent dispatches (holds slot), child clone dispatches (cell now = child's permit, child queued on the gate), parent 429s → parent's loop upgrades the cell → gets the child's permit → suspend() drops the child's in-flight lease → the freed gate slot admits another operation while the child's wire request is still running; with cap 1, the parent's resume() then re-acquires the slot for the child's permit while the parent still holds its own — two in-flight over a cap-1 gate. The single-operation case works only because the cell happens to hold the operation's own permit; the suspend-during-backoff feature this PR exists to provide is defeated (and the gate under-counted) under concurrency. active_provider_requests() drifts with it. Localized fix: pass the operation's own Arc<ProviderRequestPermit> into send_retry_loop's admission/retry paths (or key the cell by operation id) instead of round-tripping through a lineage-shared cell.

MINOR (top) — interactive TUI drops every recovery receipt. tui/ui/event_loop.rs:1708 is EngineEvent::ModelRecovery { .. } => {}. During an in-loop recovery wait (inside send_retry_loop's admission block, default budget 600 s, delays up to 60 s, persisted minima can park across restarts) the engine emits only receipts; protocol hosts get Status messages, but the interactive user sees only the spinner — no countdown, no "waiting for provider reset" — and on success never learns why the turn stalled. (Base TUI had no footer display either, so this is a completeness gap, not a regression.) Given the feature's whole point is surviving long throttle episodes, a status line (even one Event::status per "waiting" receipt) would close it cheaply.

MINOR — a nonconformant integer Retry-After (epoch-ms) bricks the provider scope persistently. server_delay treats any integer as seconds (rate_limit.rs:345-346) while body_delay's reset fields use the > 10^10 ms-vs-seconds heuristic. Retry-After: 1765280000000 → ~56k-year pause → persisted → every process start re-admits only a terminal QuotaWindow until the JSON file is hand-deleted. Same heuristic (or a sane max) at parse time fixes it. Related, deliberate but worth an explicit check: the old 1-hour RETRY_AFTER_MAX parse ceiling is gone (docs claim it; the budget clamp is the only remaining bound).

MINOR — MiniMax 200-envelope auth failures classify as InvalidRequest. base_resp.status_code = 1004 ("invalid api key") has no entry in the native code tables, so the user gets "Invalid request (200)" with no key guidance (rate_limit.rs:597-604, 669-690, fallback 440-444). Non-retryable, so no storm — just a classification gap in the exact envelope class this PR owns.

MINOR — independent_billing_evidence's code channel is looser than its phrase channel. Bare quotaexceeded/quotaexhausted codes match with no billing/account scoping (rate_limit.rs:520-522 vs mod.rs:651-666), so a provider signaling a per-minute throttle with "code": "quota_exceeded" on a 429 becomes permanent QuotaExhausted from transient evidence. The phrase channel deliberately requires durable scope; the code channel should too.

MINOR — persist failure replaces the real provider error, terminally. A failed minimum-persist write maps to QuotaWindow and remember_retry_minimum(...)? surfaces it (client.rs:3416-3432, 3796): disk-full/read-only home converts a recoverable 429 into a stop that discards the provider's message. Chain the original error, or degrade to in-memory like the terminal path does (let _ =, 3889-3894). Similarly, one corrupt advisory JSON in rate-limit-recovery/ blocks all model traffic with "repair local recovery storage" (client.rs:529-550, 3527) — fail-closed is defensible, but falling back to relearn would match how a missing file is treated.

MINOR — shared episode-attempt counter borrows a per-operation knob. admit_recovery counts every wire admission on the credential during a refusal episode against retry.rate_limit.max_attempts (client.rs:590-607), so with max_attempts = 1 (validation allows it) the first 429 pins the episode and every subsequent admission — including the refused operation's own retry after the server minimum expires — returns terminal QuotaWindow until a success or budget expiry. Probably intended ("shared admission"), but the interaction with an operation-scoped knob deserves a comment or its own bound.

Remaining minors/nits (acknowledged, non-blocking): zero-length quota window fabricated at episode-expiry race (client.rs:590-608); attempt: 0 receipts on first admission (client.rs:3532 + rate_limit.rs:198); terminal receipt lost if the runtime is shutting down when the future drops (rate_limit.rs:164-185); nested without_active_time would undercount paused time (latent — one is_none() guard); open-budget timeout can replay a POST whose delivery was unobserved on routes without an idempotency header (conventional tradeoff, deserves a stated bound); budget-exhaustion receipt mislabels a preserved transport failure as "interrupted" (mod.rs:1189-1201); disabled-arm "recovered" receipt asymmetry (mod.rs:1121 vs 1210); QuotaWindow Display renders Some(7200s)/None Debug into transcript text (mod.rs:346-352, protocol_parity.rs:403); settled_open_deadline's "did not finish processing the response headers" message is false in the Headers case (stream_entry.rs:173); parity test vector omits ModelRecovery — the exhaustive match pins arm-existence, not the Status shape (protocol_parity.rs:1249-1361); RecoveryReceipt's serde derive has no serializer anywhere — speculative scaffolding; parity projection drops agent_id/operation_id/turn_id so hosts cannot correlate; stale docs that will mislead the next editor: park_and_flush contract (engine.rs:9500, now contradicted by recovery parking), with_operation_header_for_id (client.rs:4259), child retry helper's dead RateLimited arm + governor.rs:23-28 module doc (mod.rs:12300-12310), retry_status.rs:145-147, governor.rs:3-13; the child wall-clock timeout silently changed from total to per-attempt active time (mod.rs:12419-12427) — intended, but unnamed in docs/tests; orphaned global retry_status window is now test-only (migrate-the-last-consumer receipt); in-test wall-clock race in the persist-poll peer (rate_limit_tests.rs:344-352, 700 ms bound); ~520 whitespace lines and the events.rs enum reformat inflate the diff; #![recursion_limit = "256"] is a symptom-level license (helpers already Box::pin).

Verdict

REQUEST_CHANGES on the single MAJOR: the permit-cell cross-wiring breaks the admission invariant under concurrent 429 recovery — the scenario this PR is built for — and the fix is localized. Everything else is quality: the claim is real, root-caused, completely delivered with honest tests (three engine mutations I ran — revert recovery parking, drop the !cancelled guard, unwrap the observer — all redden their pins), zero smuggling, faithful upstream adaptation, and the merge composition with #142's typed errors is coherent (no consumer misreads TurnComplete{Interrupted, error: Some, error_source: None}). Fix the MAJOR (and ideally the TUI status line + Retry-After ms guard) and this is ready.

@zhuowp
zhuowp marked this pull request as ready for review November 13, 2026 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants