feat(backend): bound LLM call concurrency and shed burst-rate (429) retries#4294
Conversation
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]>
|
@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):
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 Tests (all reviewer-requested regressions added)
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>
zhfeng
left a comment
There was a problem hiding this comment.
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()+:278run_forever()) — subagents run on a separate persistent loop, so a loop-boundasyncio.Semaphorewould split the cap between lead-agent and subagent loops. Thethreading-based_ProcessWideLimiteris the right design. - Acquire-around-attempt, release-before-backoff — correct;
test_limiter_releases_slot_during_backoff_sleepcovers 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 collapsedrandint(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:
LlmCallConfigparses via Pydantic without loader code (thecircuit_breakerbranch atapp_config.py:367-368is a no-op).config_version26→27 bumped consistently acrossconfig.example.yaml, helmvalues.yaml, chartREADME.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=0makes 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 withGATEWAY_WORKERS>1the aggregate cap islimit × workers(process-wide ≠ cluster-wide — not mentioned anywhere). - Jitter skew: with defaults
burst_base=5000, cap=8000,seed*3=15000 ≫ cap, sorandint(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_retrygainedreason="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_rateexhaustion does not trip the CB (after the fix).
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]>
|
@fancyboi999 @zhfeng 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. |
|
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 The PR's limiter is the wrong primitive in the wrong placeWrong primitive - concurrency ≠ rate. Wrong layer - the middleware leaks most LLM calls.
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 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 Suggested alternative (layered)
Phasing (doesn't waste this PR's work)
Happy to prototype Phase 2 if there's interest. The full reasoning (including the sync-path consideration, the |
…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]>
|
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() 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). |
|
@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. |
…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
…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
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):
From a caller's perspective:
Surface area
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):
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.