feat: request throttling — bounded concurrency and per-provider pacing - #76
Open
JNK234 wants to merge 3 commits into
Open
feat: request throttling — bounded concurrency and per-provider pacing#76JNK234 wants to merge 3 commits into
JNK234 wants to merge 3 commits into
Conversation
A permit is taken before pacing runs, but withPermit could only register its release once acquire() completed. A pacing delay that failed - or a scheduler that rejected it by throwing - therefore stranded the permit for good. That is worse than a failed request: capacity never returns, so with max_concurrent_requests=1 the gate is dead and every later request waits forever for a permit nobody holds. Release is now attached in acquire(), covering both the failed-Future and synchronous-throw paths. Also mark the integration stub's blocking wait with scala.concurrent.blocking. Holding a send open parks a worker of the global fork-join pool, and the unthrottled case deliberately holds five open at once. Without the hint the pool spawns no compensation thread, so on a 4-core runner the fifth send never starts and the test times out while the production code is correct.
ClaudeRequestSpec builds a ClaudeProvider in its constructor, which reads the provider registry, but nothing in the suite installed it. ProviderDefaultsSpec cleared that same global registry from its own constructor. Which suite won depended on the order ScalaTest built them in, so CI aborted ClaudeRequestSpec with "Unknown provider: anthropic". ClaudeRequestSpec now registers what it needs, and ProviderDefaultsSpec no longer resets shared state out from under suites already built against it. registerAll() inserts by name, so re-registering is all either suite needs. Both now pass in isolation, which is what makes them order-independent.
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
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.
Summary
Adds proactive request throttling: a configurable cap on in-flight requests per provider, plus optional pacing between request starts. Both are off by default, so existing models are unaffected until a modeler opts in.
retry_max_retries(#37) recovers from a rate limit after it happens. This is the proactive counterpart — not tripping one in the first place.ask turtles [ llm:chat-async ... ]currently sends one request per turtle with nothing between the agents and the socket, so a 200-turtle model opens 200 connections on the first tick.Addresses #48.
Scope and remaining work
Issue #48 lists five design requirements. This PR delivers two of them and deliberately does not close the issue:
max_concurrent_requests) and optional per-provider pacing (min_request_interval_ms).Design and behavior
RequestThrottleis a non-blocking FIFO gate wrapped around the request path inBaseHttpProvider.executeWithRetry.Future; its continuation runs when a permit is handed over. ASemaphore.acquire()would park a thread per queued agent and can starve the global pool.LLMExtensionrebuilding its provider (whichset-provider,load-config, andset-thinkingall do). Different providers never share a cap — a Gemini free tier will not throttle a paid OpenAI key. Endpoint spellings differing only by trailing slash, host case, or default port resolve to one gate, so a trailing slash cannot silently double the configured cap.All chat traffic routes through this gate: every provider extends
BaseHttpProviderand none overrideschat. Ollama's/api/tagshealth-check and model-list calls are deliberately outside it — they are not per-agent fan-out and hit a local server with no quota.Configuration
max_concurrent_requests— whole number of simultaneous requests. Unset or0disables (the default, unbounded as before).min_request_interval_ms— milliseconds between request starts. Unset or0disables. A concurrency cap bounds simultaneity, not rate, so a small cap recycling quickly can still exceed an RPM quota; pacing covers that.Documented in
docs/CONFIGURATION.mdanddocs/SETUP.md, including how to pick values from a provider's published limits.Compatibility
One documented limitation: queue time is deliberately not added to the
timeout_secondsawait bound. Queue depth is set by how many agents call in a tick, not by the cap, so no fixed formula could size it honestly. A model whose fan-out greatly exceeds its cap can therefore still exceedtimeout_secondswhile queued — modelers setting a small cap for a large population should raisetimeout_secondsto match. This is called out in both the code andSETUP.md. Unthrottled models are unaffected.Test plan
Added 18 tests across two suites, all verified locally.
RequestThrottleSpec(16 tests) — unit level, using a manual clock so pacing is asserted on the delays the throttle requests rather than on wall-clock timing (which would make the suite a scheduler-noise detector):0disable; negative and unparseable warn and disable; warn-once across 50 resolutionsThrottleIntegrationSpec(4 tests) — throughBaseHttpProvider's real request path with a latch-gated stub backend so admitted requests genuinely overlap:Verified results
sbt testsbt testwith pool pinned to 4 threads (matches a 4-vCPU CI runner)sbt "testOnly ...RequestThrottleSpec ...ThrottleIntegrationSpec"sbt assembly(from a removed jar, not cached)llm.jargit diff --checksbt test, ubuntu-latest, Java 17)Test counts go from 115 to 117 because the two permit-release regression tests below were added; the pre-existing 115 continue to pass.
Two defects found and fixed during review
Both were caught by an independent review pass and fixed test-first (failing test demonstrated first, then the minimal fix):
Stranded permit on pacing failure (production defect,
RequestThrottle.scala). The permit was taken before pacing ran, butwithPermitcould only register its release onceacquire()completed — so a pacing delay that failed, or a scheduler that rejected it by throwing, lost the permit permanently. With a cap of 1 the gate deadlocked for the rest of the run. The failing test showed the second request timing out because the first leaked. Release is now attached insideacquire(), covering both the failed-Futureand synchronous-throw paths.Test-only pool starvation (test defect). The integration stub blocked in
CountDownLatch.await()inside aFuturewithoutscala.concurrent.blocking. The unthrottled case needs five sends open simultaneously; on a 4-vCPU runner the fork-join pool spawns no compensation thread, so the fifth never starts. Reproduced by pinning the pool to 4 threads — this would have failed CI — and fixed withblocking. Production code was correct; only the test was at fault.Order-dependent provider registry tests (pre-existing test defect, surfaced by this branch). CI aborted
ClaudeRequestSpecwithUnknown provider: anthropic. The cause is unrelated to throttling:ClaudeRequestSpecbuilds aClaudeProviderin its constructor, which reads the global provider registry, but nothing in that suite installed it — whileProviderDefaultsSpeccleared the same global registry from its constructor. Which suite won depended on the order ScalaTest built them in, and adding two suites to this branch changed that order. Fixed by havingClaudeRequestSpecregister what it needs and by dropping thereset()that emptied shared state out from under already-constructed suites.registerAll()inserts by name, so re-registering suffices. Both suites now pass in isolation, which is what makes them order-independent. CI is green.