Skip to content

feat(backend): bound LLM call concurrency and shed burst-rate (429) retries#4294

Merged
WillemJiang merged 10 commits into
mainfrom
fix-4290
Jul 21, 2026
Merged

feat(backend): bound LLM call concurrency and shed burst-rate (429) retries#4294
WillemJiang merged 10 commits into
mainfrom
fix-4290

Conversation

@WillemJiang

Copy link
Copy Markdown
Collaborator

Fixes #4290

Why

At the morning peak (e.g. 08:30), LLM requests ramp from ~0 to full throttle within seconds. The Volcano Engine (Ark / Doubao) provider returns 429 limit_burst_rate — a rate-of-change (slope) limit, not a quota limit — so requests get rejected even when total RPM is within budget. The existing retry made it worse: deterministic 1/2/4/8s backoff with no jitter means every concurrent failure realigns on the same backoff ticks, and retrying into the burst adds demand to the very slope being throttled ("thundering herd"). On top of that, the retry params were hard-coded class attributes with no concurrency cap and no way to tune any of it without a code change.

Triggered by discussion #4227 and tracked in #4290. This PR lands the two-pronged mitigation proposed there: bound aggregate concurrency (caps the slope), and stop retrying into the burst (tighter budget + jittered, longer backoff + honoring Retry-After).

What changed

A new llm_call section in config.yaml now exposes the LLM-call execution knobs (all default to prior values, so existing deployments see no change unless they opt in):

llm_call:
  max_concurrent_calls: 0       # process-wide in-flight LLM call cap (0 = disabled, default)
  retry_max_attempts: 3
  retry_base_delay_ms: 1000
  retry_cap_delay_ms: 8000
  burst_retry_base_delay_ms: 5000  # longer backoff base, used only for limit_burst_rate 429s

From a caller's perspective:

  • Process-global concurrency cap — when max_concurrent_calls > 0, a single asyncio.Semaphore shared across all threads bounds in-flight LLM calls process-wide. The slot wraps one attempt only, so backoff sleeps release it for other callers (we bound in-flight requests, not waiting ones). Disabled by default.
  • Decorrelated-jitter backoff — retry delays now use AWS-style decorrelated jitter (min(cap, random(base, prev*3))) instead of deterministic exponential backoff, so a fleet failing at once no longer retries in lockstep. Retry-After is still honored verbatim.
  • Distinct limit_burst_rate handling — burst-rate 429s are now classified as reason="burst_rate" (detected via error code/message) and get a tighter retry budget (≤1 retry), a longer backoff base (burst_retry_base_delay_ms), and Retry-After preferred over any computed delay — i.e. shed load rather than hammer the throttled slope. Ordinary 429s and other transient errors keep the full budget and normal base.
  • The llm_retry SSE event can now carry reason: "burst_rate" (new enum value on an existing event; no shape change).
  • config_version bumped 26 → 27 in config.example.yaml, deploy/helm/deer-flow/values.yaml, and deploy/helm/deer-flow/README.md (CI enforces the chart not lag the example).

Surface area

  • Frontend UI - page / component / setting / interaction under frontend/
  • Backend API - endpoint / SSE event / request-response shape under backend/app
  • Agents / LangGraph - agent node, graph wiring, langgraph.json, or prompt change
  • Sandbox - docker/ or sandboxed execution
  • Skills - change under skills/
  • Dependencies - new/upgraded entry in backend/pyproject.toml or frontend/package.json (say what it buys us)
  • Default behavior change - changes existing behavior without the user opting in (default model, default setting, data shape)
  • Docs / tests / CI only - no runtime behavior change

Note: the "Default behavior change" box is checked because the retry backoff algorithm changed for all retriable transient errors (deterministic → decorrelated jitter) and limit_burst_rate now gets a distinct policy — both apply without opt-in. The concurrency cap and the new config fields all default to the previous behavior (cap disabled; retry params unchanged).

Screenshots / Recording

N/A — backend-only change, no UI.

Validation

Backend (no frontend touched):

  • ruff check + ruff format on the changed files — clean.
  • pytest tests/test_llm_error_handling_middleware.py tests/test_tool_error_handling_middleware.py tests/test_config_version.py tests/test_app_config_reload.py tests/test_subagent_executor.py → 189 passed, 1 pre-existing skip.
    • test_llm_error_handling_middleware.py alone: 58 passed (concurrency semaphore, decorrelated jitter, burst-rate classification/budget/backoff, retry-param config wiring, end-to-end).
  • bash scripts/check_config_version.sh → OK (example + helm chart both at config_version=27).

I ran the targeted suites covering the blast radius (the middleware, its construction via the tool-error chain, config loading/versioning, and the subagent executor). I did not run the full make test suite — happy to run it before merge if you'd like.

AI assistance

Tool(s) used: Claude Code

How you used it: AI implemented the feature end-to-end from the #4290 spec across three steps (process-global concurrency cap + decorrelated jitter → llm_call config wiring → distinct limit_burst_rate classification + retry-param exposure), including the unit tests. Each step was reviewed and the test suite re-run before continuing.

  • I've read and understand every line of this change and take responsibility for it - it's not unreviewed AI output.

@github-actions github-actions Bot added area:agents Agents, subagents, graph wiring, prompts, langgraph.json area:backend Gateway / runtime / core backend under backend/ area:docs Documentation and Markdown only needs-validation Touches front/back contract surface; needs real-path validation risk:high High risk: backend API, agents, sandbox, auth, deps, CI size/XL PR changes 700+ lines labels Jul 19, 2026
@WillemJiang
WillemJiang requested review from zhfeng July 19, 2026 02:25
@github-actions github-actions Bot added the reviewing A maintainer is reviewing this PR label Jul 19, 2026
WillemJiang and others added 2 commits July 19, 2026 10:39
Address PR #4294 review feedback (github-code-quality bot): the bare
module-level globals _GLOBAL_CONCURRENCY_LOOP / _GLOBAL_CONCURRENCY_LIMIT
were flagged as unused - a false positive, since both are read on the
recreate condition, but the `global`-declaration pattern tripped the
analyzer.

Replace the three globals + `global` declaration with a single
_ConcurrencyState dataclass singleton mutated in place. Behavior is
unchanged (lazy recreate when the running loop or configured limit
changes); the state is now co-located and no longer relies on bare
globals. dataclasses is already an established harness convention.

Co-Authored-By: Claude <[email protected]>
…urst retry

Addresses PR #4294 review (fancyboi999, CHANGES_REQUESTED) - two P1 issues.

P1 #1: the asyncio.Semaphore limiter was loop-bound, so it recreated per
event loop and the cap was NOT process-wide: lead-agent calls (main loop)
and subagent calls (the isolated persistent loop in subagents/executor.py)
each got their own semaphore, and the sync graph path (wrap_model_call)
bypassed the cap entirely. Recreating on loop/limit change also abandoned
permits held by the prior instance.

Replace it with a _ProcessWideLimiter built on threading primitives (not
loop-bound): one limiter shared across every event loop and both sync/async
wrappers. The cap is mutable via set_limit (never recreates, so in-flight
permits are never abandoned); permits release in finally and async waiters
unregister on cancellation, so cancellation never leaks capacity. Wire it
into wrap_model_call (sync) too - previously a direct handler() call.

P1 #2: the first (and only) burst-rate retry was deterministic at 5000ms.
prev_delay_ms was seeded from the 1000ms normal base, so for burst the
window collapsed to randint(5000, max(5000, 1000*3)) = randint(5000, 5000) -
a fleet that failed together realigned on the same 5s tick. Seed the first
retry from the reason-specific base (prev_delay_ms=None on loop init) so
the burst window is [burst_base, cap] = [5000, 8000], non-degenerate.
Retry-After is still honored verbatim.

Tests: rename semaphore tests -> limiter; add an autouse fixture resetting
the process singleton; add regressions the reviewer asked for - cross-loop
(lead + isolated-loop subagent), two concurrent sync calls, limit-change
while a permit is held (same instance, permit preserved), cancellation
no-leak, and burst first-retry non-degeneracy with default config (real
and seeded RNG) plus a concurrent de-synchronization case. Verified the
burst guard goes red on the old logic ({5000}) and green on the new.

Co-Authored-By: Claude <[email protected]>
Comment thread backend/tests/test_llm_error_handling_middleware.py Fixed
Comment thread backend/tests/test_llm_error_handling_middleware.py Fixed
Comment thread backend/tests/test_llm_error_handling_middleware.py Fixed
Comment thread backend/tests/test_llm_error_handling_middleware.py Fixed
Comment thread backend/tests/test_llm_error_handling_middleware.py Fixed
Comment thread backend/tests/test_llm_error_handling_middleware.py Fixed
Comment thread backend/tests/test_llm_error_handling_middleware.py Fixed
@WillemJiang

Copy link
Copy Markdown
Collaborator Author

@fancyboi999 these two P1 issues - both fixed in commit 9887ac6.

P1 #1 — Limiter wasn't actually process-wide

Root cause confirmed in the runtime: subagents/executor.py runs subagents on a separate persistent event loop (asyncio.new_event_loop() + run_forever(), line 328), and asyncio.Semaphore binds to a single loop. So my limiter recreated per-loop → lead-agent (main loop) and subagent (isolated loop) each got their own semaphore, and the sync wrap_model_call bypassed the cap entirely. Recreating on loop/limit change also orphaned permits.

Fix: replaced it with a _ProcessWideLimiter built on threading primitives (not loop-bound):

  • One limiter shared across every event loop and both sync/async wrappers.
  • Cap is mutable via set_limit — never recreates, so in-flight permits are never abandoned (a lower cap takes effect on the next acquire; holders keep their permit until release).
  • Async acquire uses per-loop asyncio.Event waiters woken via call_soon_threadsafe (no executor-thread starvation); cancellation unregisters the waiter, and permits release in finally — no capacity leak.
  • Wired into wrap_model_call (sync) too — previously a direct handler() call.

P1 #2 — Burst retry was deterministic at 5000ms

prev_delay_ms was seeded from the 1000ms normal base, so for burst the wi5000, max(5000, 1000*3)) = randint(5000, 5000). Fix: loops now initprev_delay_ms = None, and _build_retry_delay_ms seeds from the reason-specific base on the first retry → burst window is [5000, 8000], non-degenerate. Retry-After still
honored verbatim.

Tests (all reviewer-requested regressions added)

  • autouse fixture resets the process singleton between tests.
  • Cross-loop: lead (main loop) + isolated-loop subagent, cap=1 → never both in-flight.
  • Two concurrent sync calls → cap respected (sync bypass regression).
  • Limit-change while a permit is held → same instance, permit preserved, in-flight never exceeds cap.
  • Cancellation while waiting → no leak (fresh call admitted afterward).
  • Burst first-retry non-degeneracy with default config (real RNG + seeded RNG + end-to-end async) + concurrent de-synchronization.

Verification:

66 middleware tests pass (177 across middleware + tool-error + subagent suites, 1 pre-existing skip); ruff check/format clean. I confirmed the burst guard is a true red/green — on the old logic it produces {5000} (len 1, fails), on talues (passes).

Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Comment thread backend/tests/test_llm_error_handling_middleware.py Fixed
Comment thread backend/tests/test_llm_error_handling_middleware.py Fixed

@zhfeng zhfeng 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.

I checked out 129622d9 and wrote probe tests against the exact scenarios below. All three blocking findings are empirically reproduced, not just inferred from reading the code.

Summary

The retry-policy half is solid: config wiring, limit_burst_rate classification, and decorrelated jitter are correct and well-tested (99 middleware tests pass). The concurrency-limiter half has two correctness bugs that the prior maintainer review flagged and that are still present on HEAD, plus a circuit-breaker interaction neither prior review caught. The limiter should not ship until the two P1s are fixed.

What's correct (credit)

  • Threading-based limiter is justified. Verified subagents/executor.py:328 (asyncio.new_event_loop() + :278 run_forever()) — subagents run on a separate persistent loop, so a loop-bound asyncio.Semaphore would split the cap between lead-agent and subagent loops. The threading-based _ProcessWideLimiter is the right design.
  • Acquire-around-attempt, release-before-backoff — correct; test_limiter_releases_slot_during_backoff_sleep covers it.
  • Burst first-retry jitter is genuinely non-degenerate. prev_delay_ms=None + reason-specific base → randint(5000, 15000) capped at 8000 → [5000, 8000], not the collapsed randint(5000,5000) of the first iteration. Review #1 P1 #2 is fixed.
  • Sync path now acquires the limiter (was bypassed). Review #1 P1 #1 sync half is fixed.
  • Retry-After honored verbatim (preserved from existing _extract_retry_after_ms).
  • Config plumbing clean: LlmCallConfig parses via Pydantic without loader code (the circuit_breaker branch at app_config.py:367-368 is a no-op). config_version 26→27 bumped consistently across config.example.yaml, helm values.yaml, chart README.md.

🔴 P1 — Cancellation handoff race strands the next waiter (prior review, unfixed)

Reproduced. Cap=1, A holds the permit, B and C queued. A releases → _wake_waiters_locked dequeues B and schedules event_b.set via call_soon_threadsafe. B is cancelled in the handoff window (after dequeue, before reacquire). B's except asyncio.CancelledError runs self._async_waiters.remove((loop, event))ValueError (already popped) → pass → re-raises. The released permit sits idle (_in_flight=0) while C stays queued with its event never set. Probe result: C timed out.

The except ValueError: pass comment ("there is nothing to release") is correct about permits but wrong about wakeups — the wakeup destined for B is consumed by B's cancellation, and C is never woken.

The existing test test_limiter_cancellation_does_not_leak_capacity cancels B while purely queued, before A releases — it does not exercise the post-dequeue / pre-reacquire window, so it misses this.

Reachability: any queued caller cancelled by client disconnect / SSE cancel / timeout in that window. Not a permanent deadlock (the next call that acquires + releases drains the stranded waiter), but a real capacity-idle stall and latency spike — worst at the low-traffic tail of the peak.

Fix direction: reserve the permit for a specific waiter at dequeue time, or track dequeued-vs-granted state and pass the wakeup to the next waiter when cancellation wins. Adding more sleeps/probes around the current queue won't close the window.

🔴 P1 — Hot-reload generation: stale instance overwrites a lowered cap (prior review, unfixed)

Reproduced. Old middleware (captured cap=3) calling _get_process_limiter(3) after a newer instance lowered the live cap to 1 bumps it back to 3 (lim_old_call.limit == 3 after the new instance set it to 1). Every middleware instance calls _get_process_limiter(self.max_concurrent_llm_calls) on every attempt and runs set_limit(limit) whenever the captured value differs from the live cap, so a stale in-flight run can overwrite a freshly-hot-reloaded lower cap.

The limit-change test calls the singleton directly in one direction only; it doesn't exercise old/new instance contention.

Fix direction: give the limiter one generation-aware config owner; stale runs may acquire/release but cannot rewrite the active cap.

🟠 P2 — Circuit breaker counts burst_rate failures toward its trip (neither prior review caught)

Reproduced. limit_burst_rate classifies as (True, "burst_rate"), _max_attempts_for(., "burst_rate") == 2, and on exhaustion wrap_model_call/awrap_model_call call _record_failure(). 5 consecutive burst-rate failures flip _circuit_state to "open" → 60s fast-fail of ALL calls.

This turns a soft provider slope-throttle into a self-inflicted hard outage — the precise failure mode #4290 is trying to prevent. Pre-existing (any 429 already counted as transient→CB), but the PR's tighter 2-attempt budget exhausts each call one attempt sooner so the CB trips faster in wall-clock, and the PR is the natural place to fix it since it already distinctively classifies burst_rate.

Fix: burst_rate (and arguably generic 429) should not increment the CB — they're transient-by-design, not "provider down." In the retriable-exhaust branch, gate _record_failure() on reason not in {"burst_rate"} (or route 429-family reasons to _release_half_open_probe() like non-retriables do).

🟠 P2 — Reload-boundary inconsistency

llm_call is absent from STARTUP_ONLY_FIELDS (config/reload_boundary.py) and LlmCallConfig fields don't carry the startup-only: prefix, so the registry contract treats llm_call as hot-reloadable. But max_concurrent_calls is effectively startup-only (limiter is a process singleton; value captured at __init__; and P1 #2 shows hot-reload is unsafe). test_reload_boundary enforces drift bidirectionally. Pick one: register llm_call as startup-only (honest, simplest), or implement generation-aware ownership (P1 #2 fix). The current state is neither.

🟡 P3 — Minor

  • Default max_concurrent_calls=0 makes the primary mitigation opt-in; deployments that don't read release notes still get the thundering herd. At minimum the config comment should warn that with GATEWAY_WORKERS>1 the aggregate cap is limit × workers (process-wide ≠ cluster-wide — not mentioned anywhere).
  • Jitter skew: with defaults burst_base=5000, cap=8000, seed*3=15000 ≫ cap, so randint(5000,15000) clamped to 8000 puts ~70% of draws at exactly 8000 — better than all-at-5000 but a fleet still clusters at the cap. high = min(cap, max(base, seed*3)) would spread the window. (It is the literal AWS decorrelated-jitter formula, so defensible.)
  • No limiter observability: no metric/event for queue wait time, depth, or admission count. You can't size the cap without seeing it. llm_retry gained reason="burst_rate" (good), but there's no "blocked on limiter" signal.

Test gaps to close alongside the fixes

  • Cancel a waiter after dequeue, before reacquire (the handoff window); assert the next waiter completes without another release.
  • Two middleware instances with different captured caps and overlapping calls; assert the active cap is never overwritten by a stale instance.
  • burst_rate exhaustion does not trip the CB (after the fix).

WillemJiang and others added 2 commits July 19, 2026 16:11
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
…t-rate CB gate

Address the open P1/P2 review findings on #4294:

- P1 #1 (cancellation handoff): reserve the permit for a specific waiter at
  dequeue time (grant-at-dequeue, _AsyncWaiter.granted) so a waiter cancelled
  in the post-dequeue / pre-reacquire window hands its reservation to the next
  waiter (_handoff_granted_permit_locked) instead of stranding it. No
  cancellation window remains.
- P1 #2 (hot-reload generation): move cap updates out of the per-attempt path;
  give the limiter one generation-aware owner (set_limit_if_newer with a
  monotonic instance seq proxy for config freshness) so a stale in-flight run
  cannot rewrite a freshly-lowered cap. max_concurrent_calls is now genuinely
  hot-reloadable, resolving the reload-boundary inconsistency by option (b) -
  no STARTUP_ONLY_FIELDS change (retry params truly hot-reload).
- P2 (circuit breaker): gate _record_failure on reason != "burst_rate" so
  burst-rate (limit_burst_rate) exhaustion - a transient slope-throttle, not
  "provider down" - does not trip the CB and fast-fail ALL calls.
- P3: clamp the jitter window to the cap before drawing (uniform spread
  instead of piling at the cap); document the per-process / GATEWAY_WORKERS
  cap semantics in config + the field description.

Tests: add the reviewer-requested regressions (cancel-after-dequeue handoff;
stale-instance-doesn't-overwrite-lowered-cap across sync + isolated-loop async;
burst_rate-exhaustion-doesn't-trip-CB sync + async). Each is red on the prior
buggy logic and green on the fix. _build_middleware now routes llm_call knobs
through AppConfig so __init__ applies the cap. 71 middleware tests pass; 212
across the blast radius (1 pre-existing skip).

Co-Authored-By: Claude <[email protected]>
@WillemJiang

Copy link
Copy Markdown
Collaborator Author

@fancyboi999 @zhfeng
I addressed the open P1/P2 review findings on PR #4294 (worked against the actual PR head e0161aa, after fast-forwarding my stale local checkout):

P1 #1 — Cancellation handoff race (llm_error_handling_middleware.py): Replaced wake-then-recheck with grant-at-dequeue. A permit is reserved for a specific waiter at dequeue time (_AsyncWaiter.granted); if that waiter is cancelled in the post-dequeue/pre-reacquire window, its reservation is handed to the next waiter (_handoff_granted_permit_locked) instead of stranding it. No cancellation window remains.

P1 #2 — Stale instance overwrites a lowered cap: Moved cap updates out of the per-attempt path entirely. The limiter now has one generation-aware owner (set_limit_if_newer with a monotonic instance seq); per-attempt callers only acquire/release, so a stale in-flight run can't bump a freshly-lowered cap back up. This makes max_concurrent_calls genuinely hot-reloadable — which resolves the P2 reload-boundary inconsistency by option (b), so no STARTUP_ONLY_FIELDS change was needed (the retry params truly hot-reload; marking the whole section startup-only would've been dishonest).

P2 — Circuit breaker counts burst_rate failures: Gated _record_failure() on reason != "burst_rate" in both wrappers; burst-rate exhaustion now releases the half-open probe instead of counting toward a trip, so a provider slope-throttle can't cause a self-inflicted 60s hard outage.

P3 (cheap wins): Clamped the jitter window to the cap before drawing (high = min(cap, max(base, seed*3))) for a uniform spread; documented the per-process / GATEWAY_WORKERS>1 cap semantics in config.example.yaml and the field description. (Richer limiter metrics deferred as a feature.)

Tests: Added the reviewer-requested regressions — cancel-after-dequeue handoff, stale-instance-doesn't-overwrite-lowered-cap (sync + isolated-loop async), and burst_rate-exhaustion-doesn't-trip-CB (sync + async). I verified each is red on the prior buggy logic and green on the fix (P1 #1 → timeout; P1 #2 → assert 3 == 1; P2 → CB trips). Also refactored _build_middleware to route llm_call knobs through AppConfig so init applies the cap, and made the thread-based tests leak-proof on failure.

Verification: ruff check + ruff format --check clean; 71 middleware tests pass; 212 across the blast radius (test_llm_error_handling_middleware, test_tool_error_handling_middleware, test_config_version, test_app_config_reload, test_reload_boundary, test_subagent_executor), 1 pre-existing skip; scripts/check_config_version.sh OK.

Comment thread backend/tests/test_llm_error_handling_middleware.py Fixed
Comment thread backend/tests/test_llm_error_handling_middleware.py Fixed
Comment thread backend/tests/test_llm_error_handling_middleware.py Fixed
Comment thread backend/tests/test_llm_error_handling_middleware.py Fixed
@zhfeng

zhfeng commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator

Following up on my earlier review with an alternative direction for the limiter specifically (the retry/jitter/burst-classification half of this PR is good and should ship). I wrote this up as a design note in docs/plans/2026-07-19-llm-burst-rate-rate-limiter.md; condensed version below.

The PR's limiter is the wrong primitive in the wrong place

Wrong primitive - concurrency ≠ rate. limit_burst_rate is a rate-of-change (slope) limit: the provider throttles when RPM ramps up too steeply, even when absolute RPM is within budget. The _ProcessWideLimiter is a concurrency (in-flight) cap. It clips peak in-flight but does not smooth the RPM ramp-up slope that burst-rate penalizes (RPM ≈ concurrency / avg_latency - a fleet going 0 -> cap in one second still has a steep slope). The direct fix for a rate-of-change limit is a rate limiter (leaky/token bucket), not a concurrency semaphore.

Wrong layer - the middleware leaks most LLM calls. LLMErrorHandlingMiddleware only wraps the agent reasoning loop. I verified these call paths bypass it and call model.ainvoke()/.invoke() directly:

  • title_middleware.py:220 (title generation)
  • summarization_middleware.py:124,141 (sync + async)
  • memory/.../updater.py:898 (sync memory update)
  • runtime/goal.py:323 (goal/plan generation)
  • utils/oneshot_llm.py:65
  • skills/security_scanner.py:107

At the 08:30 peak these all hit the provider concurrently with the agent loop, and the PR's cap covers none of them. The single universal chokepoint is create_chat_model (models/factory.py:174) - every one of those paths constructs its model through it. Putting the limiter at the model-client layer covers all paths uniformly; the agent middleware does not.

Per-process and hand-rolled. The two P1 bugs I confirmed (cancellation handoff race; stale-instance overwrites a hot-reloaded cap) both stem from re-implementing rate limiting under cancellation/reload - a solved problem in existing libraries. And with GATEWAY_WORKERS=N, aggregate cap is N × configured - a process-local semaphore cannot solve the reported multi-worker peak regardless of how the bugs are fixed.

Suggested alternative (layered)

  1. Primary - leaky/token bucket at create_chat_model via aiolimiter (or a thin wrapper around _agenerate/_generate). Correct primitive (rate, not concurrency), library not hand-rolled (no cancellation/generation bugs), and covers all call paths via the single chokepoint. Config: llm_call.max_rpm + burst instead of max_concurrent_calls.
  2. Production - Redis-back the bucket. DeerFlow already has the redis extra and an async-Redis connection pattern in runtime/stream_bridge/redis.py to reuse. A Lua token-bucket keyed by provider + base_url gives a cluster-wide RPM cap across workers/pods - the only configuration that actually solves the reported multi-worker peak. Per-provider keying (not per-model, not global) also answers AnnaSuSu's scope question on [feat] Added the configuration to avoid the burst-rate control of LLM provider #4290.
  3. Ingress - Nginx limit_req. No rate limiting exists at nginx today. Add limit_req_zone + limit_req burst=… nodelay. Shared across nginx workers, zero Python. Smooths user-request arrival (not internal fan-out, so not sufficient alone).
  4. Keep this PR's decorrelated jitter + burst_rate classification + Retry-After honoring - they compose cleanly with a rate limiter. (Still fix the CB-counts-burst-rate gap from my review: a retriable exhaust calls _record_failure(); 5 consecutive burst failures trip the CB -> 60s fast-fail of ALL calls, the precise outage [feat] Added the configuration to avoid the burst-rate control of LLM provider #4290 is trying to prevent.)
  5. Future - adaptive concurrency (AIMD/gradient2) if a static RPM proves hard to tune. Defer.

Phasing (doesn't waste this PR's work)

  • Phase 1 (merge-ready): ship this PR's retry/jitter/burst-classification + the CB-exclusion fix. Drop or feature-flag _ProcessWideLimiter (the buggy, wrong-primitive part). Improves retry behavior without the limiter's bugs and without new deps.
  • Phase 2 (the real fix): aiolimiter bucket at create_chat_model, Redis-backed when Redis is configured. Replaces _ProcessWideLimiter entirely.
  • Phase 3: Nginx limit_req.

Happy to prototype Phase 2 if there's interest. The full reasoning (including the sync-path consideration, the reload_boundary.py startup-only registration for llm_call, and the test matrix) is in the design note.

…etry budget

Addresses review feedback on #4294 (fancyboi999 CHANGES_REQUESTED on acfc761):

P1 - the generation guard measured construction order, not config freshness, so
a stale AppConfig(cap=3) constructed after a fresher AppConfig(cap=1) could
restore the higher cap; and on a downscale 3->1 release() handed excess
permits to queued waiters, keeping in_flight pegged at the old cap. Replace the
pseudo-generation path with a startup-only cap: the first middleware __init__
resolves and freezes the cap; later instances (newer or older config) are
no-ops. No runtime cap mutation means no downscale race and no
freshness/construction-order race. Per-call gate is now `limiter is None` only,
so a reloaded instance with max_concurrent_calls=0 cannot silently drop the
frozen cap. Removes _owner_seq / set_limit_if_newer / _grant_to_queued_locked
/ _next_instance_seq; file 946 -> 926 lines.

P2 - burst-rate calls are capped at 2 attempts but the retry log line, the
llm_retry stream event max_attempts, and the user-facing message still used
self.retry_max_attempts (3), so the frontend showed 1/3 then stopped after
attempt 2. Thread the effective max_attempts (_max_attempts_for) through the
logger, _emit_retry_event, and _build_retry_message.

Also: document max_concurrent_calls as startup-only in the config field
description and config.example.yaml (prose only - the startup-only: prefix is
top-level AppConfig-field granularity and would mislabel the otherwise
hot-reloadable llm_call section / break the reload_boundary drift test).
Rewrite the cap-mutation tests for startup-only semantics; add P2 retry-budget
event tests (sync+async, teeth-verified red on the bug); fix bot nits (empty
except blocks -> gather(return_exceptions=True); bare await statements ->
assigned+asserted).

Co-Authored-By: Claude <[email protected]>
Comment thread backend/packages/harness/deerflow/agents/middlewares/llm_error_handling_middleware.py Dismissed
@WillemJiang

Copy link
Copy Markdown
Collaborator Author

Finding: P1 (fancyboi999) — generation guard measured construction order, not config freshness (stale AppConfig(cap=3) could restore a higher cap); on downscale 3→1 release()
handed excess permits to waiters, pegging in_flight at the old cap
Fix: Startup-only cap (your chosen direction): the first middleware init resolves and freezes the cap; later instances — newer or older config — are no-ops. No runtime cap
mutation ⇒ no downscale race, no freshness race. Per-call gate is now limiter is None only, so a reloaded instance with max_concurrent_calls=0 can't silently drop the
frozen cap. Removed _owner_seq / set_limit_if_newer / _grant_to_queued_locked / _next_instance_seq (file 946→926).
────────────────────────────────────────
Finding: P2 (fancyboi999) — burst budget is 2 but log/event/message used self.retry_max_attempts (3) → frontend showed 1/3 then stopped after attempt 2
Fix: Threaded the effective max_attempts (_max_attempts_for) through logger.warning, _emit_retry_event, and _build_retry_message.
────────────────────────────────────────
Finding: Bot nits — empty except TimeoutError: pass; bare await task_a/await task_b "statement has no effect"
Fix: Empty-except blocks replaced with gather(..., return_exceptions=True); bare awaits → assigned+asserted result (and gather(return_exceptions=True) for the cancellation
assertion).
────────────────────────────────────────
Finding: Config docs
Fix: max_concurrent_calls documented as startup-only in the field description + config.example.yaml (prose only — the startup-only: prefix is top-level-AppConfig-field
granularity and would mislabel the otherwise-hot-reloadable llm_call section).

Tests (all teeth-verified red-on-bug / green-on-fix): 3 cap-mutation tests rewritten for startup-only (frozen-at-first-construction, reverse-construction-order, cross-loop); 2 new P2 retry-budget event tests (sync+async, capture the llm_retry stream event via monkeypatched get_stream_writer). Fixture resets _CAP_RESOLVED.

Verification: ruff check + ruff format --check clean; 73 middleware tests pass (+10 reload_boundary drift); full backend suite 7887 passed, 27 skipped — the 12 failures are pre-existing network/server-dependent tests (test_browserless_client, test_crawl4ai_tools, test_fastcrw_tools, test_client_live, test_checkpointer), identical on the clean head (confirmed via stash); check_config_version.sh OK (still v27).

@WillemJiang

Copy link
Copy Markdown
Collaborator Author

@zhfeng Thanks for the review. I agree that we need to do more to address the burst rate issue at different levels. The current PR focuses on handling 429 with a burst rate. We can create a follow-up issue to set the request limitation for the scheduled task and hold the limit request per LLM provider.

@github-actions github-actions Bot removed the area:docs Documentation and Markdown only label Jul 20, 2026
@WillemJiang
WillemJiang merged commit ac5fd46 into main Jul 21, 2026
21 of 23 checks passed
@WillemJiang
WillemJiang deleted the fix-4290 branch July 21, 2026 00:07
jtaynl added a commit to jtaynl/deer-flow that referenced this pull request Jul 21, 2026
…EAN, migration-free (head 0005), consolidation OFF, 3a/3b/3c green; PG hardening + sandbox peer-kill fix + bytedance#4294 qwen-Bailian burst-429 nuance; RBAC/middlewares/memory all inert-by-default; client.py chat() intact
yogyoho added a commit to yogyoho/eai-flow-main that referenced this pull request Jul 23, 2026
…UI hardening

- bytedance#4117 XSS hardening (isSafeHref, iframe sandbox, safeLocalStorage)
- bytedance#4354 streaming chunk batcher for large file tools
- bytedance#4294 LLM concurrency cap + burst-rate retry shedding
- bytedance#4355 E2B release serialization
- bytedance#4267 transient image context cleanup
- bytedance#4374 AI disclaimer (bytedance#4373 reasoning-effort default)
- bytedance#4337 /goal length validation + counter
- bytedance#4278/bytedance#4302/bytedance#4312 URL encoding fixes
- bytedance#4375 list_uploaded_files schema fix
- bytedance#4288 uploads markdown companion name claim
- bytedance#4258 remove frontend debug logs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:agents Agents, subagents, graph wiring, prompts, langgraph.json area:backend Gateway / runtime / core backend under backend/ needs-validation Touches front/back contract surface; needs real-path validation reviewing A maintainer is reviewing this PR risk:high High risk: backend API, agents, sandbox, auth, deps, CI size/XL PR changes 700+ lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[feat] Added the configuration to avoid the burst-rate control of LLM provider

3 participants