Skip to content

feat(rate-limit): stepped cooldown — rebased + aux client coverage (supersedes #3910) - #12250

Closed
thapecroth wants to merge 3 commits into
NousResearch:mainfrom
thapecroth:feat/rate-limiter-rebased
Closed

thapecroth wants to merge 3 commits into
NousResearch:mainfrom
thapecroth:feat/rate-limiter-rebased

Conversation

@thapecroth

@thapecroth thapecroth commented Apr 18, 2026

Copy link
Copy Markdown

Summary

Per-model stepped rate-limit cooldown (ladder: 30s → 60s → 5min, resets after 10 min idle, tracked per model). Replaces the generic jittered backoff on 429s when provider fallback and credential rotation can't save us.

Rate limiter module + tests are cherry-picked verbatim from @teknium1's #3910. This PR rebases that work onto current main and extends the wiring to cover the auxiliary client path.

Why a new PR instead of updating #3910

I considered three options:

  1. Force-push onto feat/rate-limiter to auto-update feat: per-model rate limit handler with stepped cooldown #3910. Rejected — that branch is authored by another contributor, and overwriting someone else's branch without coordination is the wrong default.
  2. Open a stacked/replacement PR, preserving feat: per-model rate limit handler with stepped cooldown #3910 for history/credit. Chose this.
  3. Comment on feat: per-model rate limit handler with stepped cooldown #3910 asking the author to rebase. Rejected for now — the rebase itself is non-trivial (1818 commits, significant drift in run_agent.py / auxiliary_client.py) and easier to show than describe.

If maintainers prefer, #3910 can be closed in favor of this one; attribution to the original author is preserved via commit message. I'll leave the decision there to maintainers rather than commenting on #3910 until this has been reviewed.

What changed vs. #3910

#3910 was cut 2026-03-30. Since then run_agent.py and auxiliary_client.py have evolved substantially (Opus 4.7 migration, vision resolver refactor, TCP keepalives, payment fallback chain, credential pool rotation). The original wiring no longer applies cleanly.

Commit Change
4b2aee1 agent/rate_limiter.py + 23 tests — unchanged from #3910
43398fd New wiring against current main: (a) auxiliary_client.{call_llm,async_call_llm} — check cooldown before create(), record on 429. Covers vision/session-search/compression, which #3910 didn't touch. (b) run_agent.py main loop — stepped cooldown replaces jittered_backoff when rate-limited without a Retry-After header. (c) _is_payment_error extended to classify z.ai code 1311 ("subscription plan does not yet include") as payment, so it routes to the payment fallback chain instead of an infinitely-retripping cooldown. (d) _log_400_diag — logs kwargs keys + message block shape (no content) on HTTP 400s so the next 1210 Invalid API parameter regression is diagnosable.
acc5906 Stacked fix, unrelated to rate-limiting: tools/checkpoint_manager._clear_stale_lock — before any git op in a shadow repo, remove index.lock files older than 60s. A single crashed git add had been wedging checkpointing on one prod shadow for 6 days (56+ identical errors). Checkpoint ops per shadow are strictly serial, so any lock older than 60s is unambiguously orphaned. Happy to split into a separate PR if preferred.

Rationale for extending _is_payment_error

z.ai returns HTTP 429 for two very different conditions:

  • code 1305 / "temporarily overloaded" — transient, cooldown helps
  • code 1311 / "subscription plan does not yet include <model>" — permanent, cooldown is a trap

Without classification, the stepped cooldown would just keep firing against a model the plan doesn't include. Routing 1311 to the payment fallback chain lets a different vision backend pick up instead.

Wiring order (existing logic preserved)

  1. Provider fallback (if configured) — unchanged
  2. Credential pool rotation — unchanged
  3. If rate-limited and Retry-After header present — honor it (cap 120s) — unchanged
  4. If rate-limited and no header — stepped cooldown (new; previously fell through to jittered_backoff which caps at 60s, too tight for provider overloads)

Cooldown behavior (from #3910)

Hit Cooldown
1st 30s
2nd 60s
3rd+ 5 min

Per-model state. Resets after 10 min of no hits.

Test plan

  • pytest tests/agent/test_rate_limiter.py23/23 pass (unchanged from feat: per-model rate limit handler with stepped cooldown #3910)
  • pytest tests/agent/test_auxiliary_client.py tests/agent/test_auxiliary_config_bridge.py tests/agent/test_rate_limit_tracker.py tests/agent/test_nous_rate_guard.py147/148 pass (1 pre-existing async-plugin failure on clean main, unchanged by this PR)
  • pytest tests/run_agent/test_fallback_model.py tests/run_agent/test_long_context_tier_429.py tests/run_agent/test_provider_fallback.py tests/test_retry_utils.py64/64 pass
  • pytest tests/tools/test_checkpoint_manager.py tests/test_batch_runner_checkpoint.py69/69 pass incl. 4 new stale-lock tests
  • Manual e2e: vision aux call hitting 429 records cooldown on first hit; subsequent calls within the window short-circuit with RuntimeError: rate_limit_cooldown: <model> cooling down Ns remaining
  • Classifier verified against real log error strings: code 1311 → _is_payment_error=True, _is_rate_limit_error=False; code 1305 → _is_payment_error=False, _is_rate_limit_error=True
  • Zero regressions confirmed via git stash + rerun on clean main

For reviewers

  • Original rate-limiter logic is byte-identical to feat: per-model rate limit handler with stepped cooldown #3910, so prior review effort on that module carries over.
  • New surface is auxiliary_client.py (+108 lines) and run_agent.py (+24 lines). The aux-client change is small and defensively scoped — cooldown check gates the call, record happens on 429 only, errors other than rate-limit are untouched.
  • The checkpoint fix is orthogonal; easy to split if you'd rather review separately.

Closes nothing automatically, since I can't close #3910 myself. Maintainers welcome to close #3910 at their discretion.

🤖 Generated with Claude Code

thapecroth and others added 3 commits April 18, 2026 01:10
Adds agent/rate_limiter.py — a thread-safe per-model cooldown ladder
(30s → 60s → 5min, resets after 10min idle) for cases where provider
fallback and credential rotation can't save us. Originally from the
feat/rate-limiter branch (1ddb03b); cherry-picked onto current main
unmodified, since the original wiring targeted a March-30 snapshot of
run_agent.py / auxiliary_client.py. Wiring lands in follow-up commits.

Includes the original 23-test suite.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Extends agent.rate_limiter coverage to the paths that actually fire
today:

- auxiliary_client.{call_llm,async_call_llm}: check cooldown before
  create(); record on 429. Prevents vision/session-search from
  flooding a rate-limited model with repeated calls inside the
  cooldown window (observed as 9 x 429 "temporarily overloaded" on
  vision in recent logs).
- run_agent.py main loop: when rate-limited and the provider didn't
  return Retry-After, use the stepped cooldown as wait_time instead
  of jittered_backoff (which caps at 60s — too tight for transient
  provider overloads).

Also widens _is_payment_error to catch z.ai's 429 code 1311
"subscription plan does not yet include <model>" so it routes to the
payment fallback chain (GLM-5V-Turbo wasn't in the user's coding
plan; a cooldown would do nothing for a permanent plan issue). Adds
_is_rate_limit_error as the counterpart classifier.

Adds _log_400_diag — on HTTP 400 from any aux call, logs the kwargs
key list and message block shape (no content — image data URLs would
bloat the log). Makes the next 1210 "Invalid API parameter"
regression diagnosable at a glance.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
_run_git now clears any index.lock older than 60s in the shadow repo
before invoking git. Checkpoint ops per shadow are strictly serial
(one gateway agent per session), so an old lock at entry is
unambiguously orphaned from a crashed or killed git subprocess.

Observed impact without this: a single crashed git add wedged
checkpointing on one shadow for 6 days (56+ identical errors in the
log, all fatal: Unable to create '.../index.lock': File exists).

60s threshold is conservative — real git ops finish in milliseconds,
so any lock that old is a zombie.

Adds 4 tests: removal of stale lock, preservation of fresh lock,
noop when no lock, and an end-to-end that shows git add -A succeeds
after cleanup.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@thapecroth

Copy link
Copy Markdown
Author

Closing after direction check — maintainer triage on #3910 ("not pursuing this approach") and similar triage on the closely-related #12148 makes it clear the repo is moving toward proactive concurrency limiting (#7479) rather than reactive cooldown ladders. Splitting the orthogonal, uncontroversial pieces of this PR into two small focused PRs:

  1. Z.ai code 1311 ("subscription plan does not yet include") classification fix + 400 diagnostic logging in the auxiliary client — a surgical bug fix.
  2. Checkpoint index.lock stale-lock self-heal — unrelated to rate-limiting.

Links to the replacements will follow shortly.

@thapecroth thapecroth closed this Apr 20, 2026
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.

1 participant