fix(prometheus): bound per-request budget metric emission with a timeout - #31632
Conversation
Greptile SummaryThis PR bounds the
Confidence Score: 5/5Safe to merge; the change is narrowly scoped to wrapping one gather call in a timeout and adds no new code paths that affect the main request flow. The implementation is logically correct: the timeout is bounded and validated, only TimeoutError is swallowed (CancelledError propagates), the gather branches already used return_exceptions=True so individual failures were already isolated, and the env-var parser guards against all degenerate inputs. The tests cover every specified behavior deterministically without real network calls. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/integrations/prometheus.py | Wraps asyncio.gather for budget metrics in asyncio.wait_for with a configurable timeout; env-var parser correctly validates finiteness and positivity before use. |
| tests/test_litellm/integrations/test_prometheus_budget_metrics_timeout.py | New test file covering timeout branch, happy path, invalid/valid env parsing, and CancelledError propagation; all network calls are mocked, satisfying the no-real-network-calls rule. |
Reviews (4): Last reviewed commit: "fix(prometheus): reject non-finite and n..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
@greptileai please re-review the latest commits. The earlier P1 (env parser accepting 0, negatives, nan, inf) is fixed in 250bf7f: the value is now validated as finite and positive, otherwise it falls back to the default. 74ac661 only reformats to line-length 88. |
26ac40c to
cca71a0
Compare
Wrap the per-request budget-metric gather in asyncio.wait_for so a slow Redis or DB lookup cannot consume the whole LoggingWorker watchdog and get the success-logging event cancelled. On timeout the emission is skipped in isolation; budget gauges are still refreshed by the periodic cron. The timeout is configurable via PROMETHEUS_BUDGET_METRICS_PER_REQUEST_TIMEOUT and defaults to 5.0 seconds, falling back to the default on an invalid value instead of raising
…meout env float() accepts 0, negatives, nan and inf, which bypass the fallback: a value <= 0 makes asyncio.wait_for time out immediately and skip every per-request emission, and inf reintroduces the unbounded wait the timeout was meant to bound. Validate the parsed value is finite and greater than zero before using it, otherwise fall back to the default
74ac661 to
ce482b8
Compare
f90b174
into
BerriAI:litellm_oss_staging
…out (#31632) * fix(prometheus): bound per-request budget metric emission with a timeout Wrap the per-request budget-metric gather in asyncio.wait_for so a slow Redis or DB lookup cannot consume the whole LoggingWorker watchdog and get the success-logging event cancelled. On timeout the emission is skipped in isolation; budget gauges are still refreshed by the periodic cron. The timeout is configurable via PROMETHEUS_BUDGET_METRICS_PER_REQUEST_TIMEOUT and defaults to 5.0 seconds, falling back to the default on an invalid value instead of raising * fix(prometheus): reject non-finite and non-positive budget-metrics timeout env float() accepts 0, negatives, nan and inf, which bypass the fallback: a value <= 0 makes asyncio.wait_for time out immediately and skip every per-request emission, and inf reintroduces the unbounded wait the timeout was meant to bound. Validate the parsed value is finite and greater than zero before using it, otherwise fall back to the default
* fix(prometheus): bound per-request budget metric emission with a timeout (#31632) * fix(prometheus): bound per-request budget metric emission with a timeout Wrap the per-request budget-metric gather in asyncio.wait_for so a slow Redis or DB lookup cannot consume the whole LoggingWorker watchdog and get the success-logging event cancelled. On timeout the emission is skipped in isolation; budget gauges are still refreshed by the periodic cron. The timeout is configurable via PROMETHEUS_BUDGET_METRICS_PER_REQUEST_TIMEOUT and defaults to 5.0 seconds, falling back to the default on an invalid value instead of raising * fix(prometheus): reject non-finite and non-positive budget-metrics timeout env float() accepts 0, negatives, nan and inf, which bypass the fallback: a value <= 0 makes asyncio.wait_for time out immediately and skip every per-request emission, and inf reintroduces the unbounded wait the timeout was meant to bound. Validate the parsed value is finite and greater than zero before using it, otherwise fall back to the default * fix: report the blocked LLM response's real token usage (#31217) When a guardrail blocks a post-call response, the synthetic violation response reported hard-coded zero usage, discarding the token usage the upstream call had already consumed. Fix the root cause rather than re-counting tokens: - Add an optional `original_response` field to ModifyResponseException. - The unified guardrail's post-call success hook attaches the blocked LLM response to the exception. - The /v1/messages and OpenAI-format (/v1/chat/completions, /v1/completions) block handlers report `original_response.usage` directly. Pre-call blocks never invoked the LLM, so usage is zero. Mock-based tests cover the helper (returns original usage / zero), the success hook attaching original_response, and the endpoint reporting it end-to-end. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(guardrails): buffer + cleanly terminate streamed responses on block (#31389) Streaming moderation improvements for the unified guardrail post-call streaming iterator hook: - streaming_buffer_until_moderated: withhold all chunks until end-of-stream moderation passes, then release the original response (clean) or only the block message (blocked) -- the original content is never delivered on a block. Snapshot chunks with a shallow list() copy (end-of-stream builds a separate assembled response; chunks aren't mutated in place). - Clean Anthropic SSE on block: synthesize a well-formed termination sequence instead of a bare data: {"error": ...} blob that truncates the stream. Provider-specific synthesis lives in AnthropicMessagesHandler via build_block_sse_chunks (format-agnostic routing stays in the hook). - Mid-stream blocks continue the in-progress message (close open content block, append block message, terminate) rather than emitting a second message_start, which clients reject. Standalone envelope only when no chunks were sent (buffered path). - ModifyResponseException imported under TYPE_CHECKING + locally at runtime to avoid a module-level cyclic import. Adds regression tests for buffering (content withheld on block) and mid-stream continuation (single message_start). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix: report real usage on streaming blocks, disable buffered mode for content-rewriting guardrails - _standalone_block_chunks and _block_continuation_chunks now read real token usage from ModifyResponseException.original_response instead of hardcoding zero, matching the non-streaming _blocked_response_usage path. Shared helper moved to guardrail_translation/utils.py. - streaming_buffer_until_moderated is now forced off when the guardrail has mask_response_content=True, since buffered replay releases the withheld original chunks verbatim -- unsafe for a guardrail that rewrites content (e.g. PII masking). - Fix inverted streaming-flag precedence comment. * style: ruff format after greploop fixes * fix: handle Anthropic streaming guardrail blocks * fix(responses): check terminal event type for streaming guardrail end-of-stream detection _check_streaming_has_ended assumed responses_so_far held ModelResponse objects with .choices, but for the Responses API the accumulated chunks are raw SSE event dicts, causing an AttributeError on every call * fix: preserve Anthropic blocked stream usage --------- Co-authored-by: FERNANDO IZAR <fizar@me.com> Co-authored-by: Joseph Barker <156112794+seph-barker@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com>
…out (BerriAI#31632) * fix(prometheus): bound per-request budget metric emission with a timeout Wrap the per-request budget-metric gather in asyncio.wait_for so a slow Redis or DB lookup cannot consume the whole LoggingWorker watchdog and get the success-logging event cancelled. On timeout the emission is skipped in isolation; budget gauges are still refreshed by the periodic cron. The timeout is configurable via PROMETHEUS_BUDGET_METRICS_PER_REQUEST_TIMEOUT and defaults to 5.0 seconds, falling back to the default on an invalid value instead of raising * fix(prometheus): reject non-finite and non-positive budget-metrics timeout env float() accepts 0, negatives, nan and inf, which bypass the fallback: a value <= 0 makes asyncio.wait_for time out immediately and skip every per-request emission, and inf reintroduces the unbounded wait the timeout was meant to bound. Validate the parsed value is finite and greater than zero before using it, otherwise fall back to the default
Relevant issues
N/A
Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewDelays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
Deterministic, self-contained reproduction — no Redis/DB, no live load. It calls the patched method directly and captures
verbose_loggerinto an in-memory buffer, exercising both the timeout branch and the happy path. Run from the repo root.Why
timeout=0.0is a reliable trigger: on CPython,asyncio.wait_for(coro, timeout)short-circuits whentimeout <= 0and raisesTimeoutErrorimmediately, before the coroutine runs. SoPROMETHEUS_BUDGET_METRICS_PER_REQUEST_TIMEOUT=0.0deterministically forces the timeout branch on every call — simulating an "infinitely slow Redis" with zero real load. Production default is5.0s.Output:
raised = None✅True✅False✅raised = None✅The two cases together prove the exact patch behavior: when budget emission exceeds the timeout it is dropped in isolation and silently, without tearing down the whole success-logging event (the original failure mode); when it doesn't exceed, nothing changes. The
5.0case is the control proving the wrapper adds no regression on the happy path.Type
🐛 Bug Fix
Changes
Problem
PrometheusLogger._increment_remaining_budget_metricsawaits anasyncio.gatherover the key/team/user/org budget branches with no timeout. Each branch reads the Redis cache and the Prisma DB. Under load these reads stall, the gather is awaited unbounded, and the success-logging coroutine exceeds theLoggingWorkerwatchdog (LOGGING_WORKER_MAX_TIME_PER_COROUTINE, default 20s). The watchdog then cancels the whole event, so every metric emitted after this await (latency, cache, total requests) is dropped too, not only the budget gaugesRequests still return 200, but the Prometheus gauges go incomplete and the logs fill with
LoggingWorker error/CancelledErrorFix
Bound only the budget-metric gather with its own
asyncio.wait_for. The branches are already non-blocking (return_exceptions=True); on timeout we log and return, lettingasync_log_success_eventkeep emitting the remaining metrics instead of being cancelled wholesale. The budget gauges are independently refreshed by the periodic cron, so a skipped per-request emission only loses sub-cron real-time detail, not correctnessThe bound is configurable via a new env var
PROMETHEUS_BUDGET_METRICS_PER_REQUEST_TIMEOUT(default 5.0s, well below the 20s watchdog). Onlyasyncio.TimeoutErroris caught so an injectedCancelledError(cooperative shutdown / outer watchdog) still propagates. The env value is validated to be a finite positive number; anything else (a typo,0, a negative,nan, orinf) falls back to the default instead of being used, so a bad value cannot silently disable emission (<= 0skips every call) or reintroduce the unbounded wait (inf)Tests
tests/test_litellm/integrations/test_prometheus_budget_metrics_timeout.pycovers the behaviors: a branch slower than the timeout is skipped without propagating and the skip is logged; with a generous timeout every branch is awaited and nothing is skipped; the env parser returns a finite positive value as-is and falls back to the default for unusable inputs (not-a-number,0,-1,nan,inf,-inf) and when unset; an outer cancellation while awaiting still propagates instead of being swallowed