fix(llm): release concurrency permits during retry backoff - #3145
Conversation
Review summaryThe attempt-scoped permit design addresses the reported starvation mechanism correctly. OpenAI-compatible network requests remain concurrency-limited, while retry backoff occurs outside the permit context. Retry classification, attempt counts, and delay calculations appear unchanged. Observability regressionThe worker-stage contract currently states that a call remains marked Consequently, a call waiting on either semaphore can appear to be an active provider attempt—the ambiguity previously addressed by #3002. Suggested behavior:
Minimal regression coverageA compact parameterized test covering both
Separate tests for every retry exception and provider path do not appear necessary because those paths share the same attempt context. CIThe Core LLM test job was skipped in the current workflow run. The new concurrency regression should execute in required CI before merge. Overall, the implementation direction is sound. Preserving the queued-stage contract and covering both public call paths would close the remaining correctness gap without expanding the scope of the PR. |
nicoloboschi
left a comment
There was a problem hiding this comment.
good catch, can you apply this to all llm providers?
…130-release-permits-during-backoff # Conflicts: # hindsight-api-slim/hindsight_api/engine/llm_wrapper.py # hindsight-api-slim/hindsight_api/engine/providers/openai_compatible_llm.py # hindsight-api-slim/tests/test_llm_per_op_concurrency.py
|
Updated the PR to scope concurrency permits to each upstream attempt across OpenAI-compatible, Anthropic, Gemini/Vertex, LiteLLM/Router, Claude Code, Codex, and the LlamaCpp delegate. Retry backoff, auth refresh, parsing, and validation now run outside the permits. Added deterministic coverage for backoff release, stage ordering, and LlamaCpp context forwarding. Local validation: 180 passed, 4 skipped; Ruff, formatting, ty, and repository pre-commit hooks passed. |
…its-during-backoff
… attempt labels, test coverage - llm_wrapper: attempt-gated providers no longer stamp the bare base stage before holding any permit — a call queued on the semaphore stays '.queued' until the provider's post-acquire 'attempt=N' stamp (vectorize-io#3002), and _attempt_permits suffixes '.backoff' when an attempt fails so backoff sleeps are distinguishable from in-flight requests. - codex tools path: attempt-numbered stage labels (1/2, 2/2) and 401/403 before the reactive refresh logs as warning, not error. - typing: deprecated typing.AsyncContextManager -> contextlib.AbstractAsyncContextManager; uniform 'is not None' guards; document attempt_context in LLMInterface. - tests: end-to-end regression through the real OpenAI-compatible retry loop (permit released during backoff, reacquired on attempt 2), cancellation while queued on the global permit releases the per-op permit, stage queued->attempt->backoff lifecycle; fix provider stubs missing supports_attempt_scoped_concurrency.
Problem
Providers run their retry loops inside the wrapper semaphore scope. A call therefore holds its per-operation and global concurrency permits while sleeping between attempts, which can starve unrelated operations when the global limit is small. Fixes #3130.
Fix
Permits now represent active upstream attempts instead of whole logical calls. Providers that own a retry loop declare
supports_attempt_scoped_concurrency(); the wrapper then skips holding the semaphores and passes anattempt_contextfactory that the provider enters around each individual upstream request — including structured output, tool calls, streaming SSE consumption, and native Ollama structured requests. This applies to all real providers (OpenAI-compatible, OpenAI Responses, Anthropic, Gemini, LiteLLM, Codex, Claude Code, llama.cpp via delegation), per review. Permit acquisition remains centralized and keeps the per-operation-then-global order.Worker-stage observability is preserved and extended (#3002): a call queued on a saturated semaphore keeps its
.queuedstage until the first post-acquireattempt=Nstamp, and a failed attempt gains a.backoffsuffix while the provider sleeps without permits — so "waiting for a permit", "request in flight", and "backing off" are all distinguishable in worker logs.Tests
.queued→attempt=N(only once permits are held) →.backoffduring retry sleep.uv run pytest tests/test_llm_per_op_concurrency.py tests/test_llamacpp_attempt_context.py tests/test_openai_compatible_response_hardening.py tests/test_openai_responses_provider.py tests/test_llm_strict_schema.py tests/test_llm_trace.py ...— all green;ruff+tyclean.Risk
Retry classification, backoff calculation, attempt counts, tracing context, and usage aggregation are unchanged. The main behavioral change is that queued retries now compete fairly with other calls instead of retaining their previous slot — under saturation a retrying call's end-to-end latency can grow, which is the intended fairness trade-off.