fix(llm): bound total in-flight LLM requests, not one analyzer's fan-out - #401
fix(llm): bound total in-flight LLM requests, not one analyzer's fan-out#401Mark2Mac wants to merge 4 commits into
Conversation
`SKILLSPECTOR_MAX_LLM_CONCURRENCY` creates its semaphore inside a single analyzer's
batch fan-out. The analyzers are separate graph nodes and the workflow fans out to
them in parallel, so each one gets its own semaphore and the process puts N x limit
requests on the wire.
It is not merely ineffective, it multiplies. Peak in-flight requests measured with two
analyzers running concurrently:
limit 1 -> 2 in flight
limit 2 -> 4 in flight
The docstring says users on rate-limited endpoints "can set it to 1 to serialize
requests"; they cannot. On a free-tier endpoint this is the difference between one
analyzer completing and all four: the extra requests arrive together and come back 429.
This shares one semaphore per event loop, per resolved limit. Keying by loop keeps
unrelated loops independent (tests, repeated CLI invocations) and the weak key drops
the entry with the loop; keying by limit as well means a caller that resolves a
different value gets its own semaphore instead of replacing one other coroutines are
currently holding.
An explicit `max_concurrency=` argument stays local to its call. Callers that pass a
number are asking for a fan-out width, not for a share of the process-wide budget, and
the existing tests rely on that isolation.
Defaults are unchanged: with the variable unset the ceiling is the same as before, now
applied once instead of once per analyzer.
Closes NVIDIA#387
Signed-off-by: Marco Macrì <62335226+Mark2Mac@users.noreply.github.com>
| ] = weakref.WeakKeyDictionary() | ||
|
|
||
|
|
||
| def _shared_semaphore(limit: int) -> asyncio.Semaphore: |
There was a problem hiding this comment.
Because the graph registers these analyzers as synchronous nodes, each node reaches run_async(), which calls asyncio.run() (and may do so in separate LangGraph worker threads). That gives each analyzer a different event loop, so this WeakKeyDictionary returns different semaphores and the original N × limit burst remains in the real graph. The new tests gather two analyzers on one artificial loop and therefore cannot catch this. Please coordinate at a truly shared layer (or convert the graph nodes to share one async loop) and add a graph-level regression exercising the actual analyzer nodes.
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Requesting changes. The proposed limiter is keyed by event loop, but the production graph runs synchronous analyzer nodes that each enter run_async() / asyncio.run() and therefore do not share that loop or semaphore. The real graph can still issue N × limit requests. Please address the inline blocker and add a regression through the actual graph/analyzer execution path.
The review is right and the previous commit was not. Keying the semaphore by the running event loop keys it by the analyzer: every analyzer node is a synchronous LangGraph node that reaches run_async(), which calls asyncio.run() -- a new loop each time, on a new thread when a loop is already running. So each node got its own semaphore and the N x limit burst survived. The tests could not see it, because they gathered two analyzers on one artificial loop, which is the one shape production never takes. The permits now live outside asyncio, under a plain lock, and each waiter parks on a future belonging to its own loop; releasing hands the permit to the next waiter through that loop's call_soon_threadsafe. Nothing blocks a worker thread while it waits, which rules out the obvious alternative of wrapping a threading.Semaphore in run_in_executor -- that pins one thread per queued request, and the queue is exactly as long as the fan-out this exists to bound. A cancelled waiter is handled explicitly, because that failure mode is silent: it is removed from the queue, and if the permit reached it in the same instant the wait was abandoned, the permit is passed on rather than dropped. A leaked permit does not raise -- the count simply never recovers, and every later request waits for a slot that no longer exists, hours after the cancellation. The new bench runs three analyzers the way the graph does: three threads, three loops, one lock-protected counter, through run_async and not through asyncio .gather. It was seen red against the code this review rejected -- "6 requests were in flight with the limit set to 2" -- which is exactly N x limit with three nodes. It also asserts the bound is a ceiling and not a queue (three nodes at a limit of 3 must reach 3) and that no permit is stranded by a cancellation. 162 tests in this file pass, ruff check and format clean. Three failures in tests/nodes/test_security_*.py are pre-existing on this branch: they fail identically with this change stashed. Signed-off-by: Marco Macrì <Mark2Mac@users.noreply.github.com>
Base update onto d486d0a, signed off so the DCO check has something to read. One conflict, in the test module's import block: main added `append_output_language_instruction` where this branch added `_shared_limiter`. Both are kept. 174 tests in tests/nodes/test_llm_analyzer_base.py pass on the merge result, ruff check and format clean. Signed-off-by: Marco Macrì <Mark2Mac@users.noreply.github.com>
|
You were right, and the bench I had written could not have caught it.
The fix. The permits no longer live in asyncio. A process-wide counter under a plain lock; each waiter parks on a future belonging to its own loop, and A cancelled waiter is handled explicitly. It is removed from the queue, and if the permit reached it in the same instant the wait was abandoned, it is passed on rather than dropped — a leaked permit raises nothing, the count simply never recovers, and every later request waits for a slot that no longer exists. The regression, at the shape you asked for. Six is exactly N × limit with three nodes — the arithmetic you predicted in the inline comment. It also pins the two failure modes on either side of the fix: that the bound is a ceiling and not a queue (three nodes at a limit of 3 must reach 3, or the knob has silently become a serializer), and that a cancellation strands no permit. 174 tests in |
|
Powered by Codex: PR council review result. This is a triage signal, not a maintainer approval.
|
3 participants
Closes #387.
The knob does not bound what it says it bounds
SKILLSPECTOR_MAX_LLM_CONCURRENCYcreates its semaphore inside one analyzer's fan-out:The analyzers are separate graph nodes and
graph.pyfans out to them in parallel, so each onegets its own semaphore. The docstring says users on rate-limited endpoints "can set it to 1 to
serialize requests"; they cannot.
It is not merely ineffective — it multiplies. Peak in-flight requests, counted by instrumenting
ainvokeand running analyzers concurrently on this branch and onmain(29b0dc8, v2.9.6):mainFour is the number that matters:
mcp_tool_poisoning,semantic_security_discovery,semantic_developer_intentandsemantic_quality_policyall extendLLMAnalyzerBase, and thegraph opens them together. So
=1puts four requests on the wire and=2puts eight.The last row is the control: at the default of 10 the peak is identical before and after,
because the ceiling is above the work. Users who never touch the variable see no change.
The behaviour on a rate-limited endpoint is the one reported in #387 (free-tier
build.nvidia.com: 1 of 4 analyzers completing at every setting, the three failures arrivingtogether as
429); that measurement is from the issue, not repeated here.metadata.llm_degradedreports the partial coverage honestly, so the scan does not lie — butthe one knob offered to fix it cannot.
The change
One semaphore per event loop, per resolved limit, shared by every analyzer:
weak key drops the entry with the loop;
instead of replacing one other coroutines are currently holding;
max_concurrency=argument stays local to its call. Callers that pass a numberare asking for a fan-out width, not for a share of the process-wide budget, and the existing
tests rely on that isolation. The documented "an explicit argument still wins" keeps holding.
No change to defaults: with the variable unset the behaviour is the same ceiling as before,
now applied once instead of once per analyzer.
Tests
Three tests, the first two verified red against the unmodified module (
assert 4 == 2on thesecond, which is the defect stated as a number). They use two analyzers rather than four: two is
enough to make the bound observable, and it keeps the test fast:
test_two_analyzers_respect_one_global_slottest_limit_of_two_allows_two_across_analyzerstest_explicit_argument_still_bounds_only_its_own_callThe counting mock yields control with
await asyncio.sleep(0.02)inside the invocation, so asecond in-flight request has the chance to be observed — without it the assertion would pass on
a serialization the code does not actually provide.
Gates reproduced locally
ruff checkandruff format --checkclean;docker build+tests/docker/smoke.shpass,including the GitHub URL scan. Unit suite 2211 passed / 14 skipped / 4 xfailed, with the same
single unrelated failure as any loaded run of
main(
test_mcp_stdio_initialize_registers_scan_skill, hardcoded 15-second handshake budget);it passes on this branch and on an untouched
mainonce the machine is quiet.Merged with #386 on top of
mainthe two apply cleanly and the 204 tests covering the areasboth touch pass together.