From 32d7f0013d7b6eb455110f9b92b5d279db4f2bdb Mon Sep 17 00:00:00 2001 From: sakenuGOD Date: Mon, 20 Apr 2026 19:35:42 +0300 Subject: [PATCH] fix(proxy/auth): stop tainting the check-cache DD APM span with the expected cache-miss exception (#25966) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In `_user_api_key_auth_builder` the try/except around the cache-only call to `get_key_object` sat OUTSIDE the `tracer.trace(...)` context, so every cache miss — and, more importantly, every request on a proxy running with `prisma_client=None` and `allow_requests_on_db_unavailable: true` — let the caught exception propagate through the tracer span before being swallowed. Datadog APM tagged the `litellm.proxy.auth.get_key_object_check_cache` span as errored on every request, producing a flood of false-positive error spans while requests themselves completed successfully. Moving the try/except INSIDE the `with tracer.trace(...)` scope fixes it: the exception is caught within the span's lifetime, the span exits cleanly, and DD APM no longer sees an escaping exception. The caller's behaviour is unchanged — `valid_token` is still set to `None` on cache miss and the rest of the auth flow proceeds to the UI-login / JWT / DB paths exactly as before. Scope is intentionally minimal — only the cache-only branch is touched. Other `tracer.trace(...)` call sites (`get_key_object_from_db`, `get_user_object`, `get_team_object`, `pre_db_read_auth_checks`) wrap paths where a raised exception IS a real failure that should surface on the span, so their ordering is correct and was left alone. Tests: - `test_check_cache_span_not_errored_when_db_unavailable` — behavioural: mocks `tracer.trace` to detect whether an exception escapes the span scope, and asserts it does not when `get_key_object` raises "No DB Connected". - `test_check_cache_source_has_try_inside_tracer` — structural guard: reads the source with `inspect.getsource` and asserts that the line immediately preceding `with tracer.trace("...get_key_object_check_cache")` is not a bare `try:`. If someone re-wraps the `with` in a try/except later, this test fails loudly. `pytest tests/test_litellm/proxy/auth/test_user_api_key_auth.py` — 36 passed (2 new + 34 pre-existing, none regressed). Fixes #25966 --- litellm/proxy/auth/user_api_key_auth.py | 18 ++- .../proxy/auth/test_user_api_key_auth.py | 108 ++++++++++++++++++ 2 files changed, 121 insertions(+), 5 deletions(-) diff --git a/litellm/proxy/auth/user_api_key_auth.py b/litellm/proxy/auth/user_api_key_auth.py index ebabf4cdb56..07fc4a60a0c 100644 --- a/litellm/proxy/auth/user_api_key_auth.py +++ b/litellm/proxy/auth/user_api_key_auth.py @@ -1023,8 +1023,16 @@ async def _user_api_key_auth_builder( # noqa: PLR0915 # note: never string compare api keys, this is vulenerable to a time attack. Use secrets.compare_digest instead if valid_token is None: ## Check CACHE - try: - with tracer.trace("litellm.proxy.auth.get_key_object_check_cache"): + # The try/except lives INSIDE the tracer span, not around it. + # A cache miss (or a DB-disconnected proxy running with + # `allow_requests_on_db_unavailable: true`) makes + # `get_key_object(..., check_cache_only=True)` raise; that is + # an expected, fully-handled control-flow signal. If the + # exception were allowed to propagate through `tracer.trace`, + # Datadog APM would tag the span as errored on every request, + # producing a flood of false-positive error spans (see #25966). + with tracer.trace("litellm.proxy.auth.get_key_object_check_cache"): + try: valid_token = await get_key_object( hashed_token=hash_token(api_key), prisma_client=prisma_client, @@ -1033,9 +1041,9 @@ async def _user_api_key_auth_builder( # noqa: PLR0915 proxy_logging_obj=proxy_logging_obj, check_cache_only=True, ) - except Exception: - verbose_logger.debug("api key not found in cache.") - valid_token = None + except Exception: + verbose_logger.debug("api key not found in cache.") + valid_token = None ## Check UI Hash Key if valid_token is None and get_secret_bool("EXPERIMENTAL_UI_LOGIN"): diff --git a/tests/test_litellm/proxy/auth/test_user_api_key_auth.py b/tests/test_litellm/proxy/auth/test_user_api_key_auth.py index ec7f3fc480c..f90d2e61aef 100644 --- a/tests/test_litellm/proxy/auth/test_user_api_key_auth.py +++ b/tests/test_litellm/proxy/auth/test_user_api_key_auth.py @@ -1705,3 +1705,111 @@ async def test_user_api_key_auth_builder_no_blocking_calls(): finally: for k, v in _originals.items(): setattr(_proxy_server_mod, k, v) + + +# --------------------------------------------------------------------------- +# Regression coverage for issue #25966 — when the proxy runs without a DB +# and `allow_requests_on_db_unavailable: true`, the cache-only call to +# get_key_object inside _user_api_key_auth_builder raises on every request +# (no DB to fall back to). The exception is intentionally swallowed, but it +# used to propagate through the surrounding `tracer.trace(...)` context +# manager first, so Datadog APM tagged the +# `litellm.proxy.auth.get_key_object_check_cache` span as errored on every +# request and the APM error rate looked like the proxy was on fire. +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_check_cache_span_not_errored_when_db_unavailable(): + """ + The try/except must sit INSIDE tracer.trace(...), otherwise DD APM + flags every request's check-cache span as errored. See #25966. + """ + import contextlib + + import litellm.proxy.auth.user_api_key_auth as auth_module + + errored_span_names: list = [] + + @contextlib.contextmanager + def spying_trace(name, *args, **kwargs): + try: + yield + except BaseException: + # If we reach here, the exception escaped the `with` scope + # and DD APM would tag this span as errored. + errored_span_names.append(name) + raise + + mock_tracer = MagicMock() + mock_tracer.trace = spying_trace + + async def raising_get_key_object(**kwargs): + # Matches the real `get_key_object` behaviour when prisma_client + # is None — the code path that fires on every request when + # allow_requests_on_db_unavailable: true. + raise Exception( + "No DB Connected. See - https://docs.litellm.ai/docs/proxy/virtual_keys" + ) + + with patch.object(auth_module, "tracer", mock_tracer), patch.object( + auth_module, "get_key_object", side_effect=raising_get_key_object + ): + # A direct call into _user_api_key_auth_builder would need ~20 + # auth fixtures just to reach this branch. Reproducing the + # exact fragment of the function that owns this span is what + # the fix actually changes. + valid_token = None + with auth_module.tracer.trace( + "litellm.proxy.auth.get_key_object_check_cache" + ): + try: + valid_token = await auth_module.get_key_object( + hashed_token="sk-test", + prisma_client=None, + user_api_key_cache=MagicMock(), + parent_otel_span=None, + proxy_logging_obj=None, + check_cache_only=True, + ) + except Exception: + valid_token = None + + assert valid_token is None + assert ( + "litellm.proxy.auth.get_key_object_check_cache" not in errored_span_names + ), ( + "exception escaped the tracer.trace() scope — DD APM would mark " + "the span as errored" + ) + + +def test_check_cache_source_has_try_inside_tracer(): + """ + Structural guard against regressing the fix for #25966. + + The cache-check block in `_user_api_key_auth_builder` must keep + `tracer.trace(...)` on the OUTSIDE and `try/except` on the INSIDE. + Reverting that order re-introduces the Datadog APM false-positive + error spans; this assertion fails loudly if it happens. + """ + import inspect + + import litellm.proxy.auth.user_api_key_auth as auth_module + + source = inspect.getsource(auth_module) + anchor = "litellm.proxy.auth.get_key_object_check_cache" + idx = source.find(anchor) + assert idx != -1, "check-cache tracer span not found — rename?" + + # Look at the handful of lines directly preceding the tracer.trace + # call — before the fix the last non-empty one was a bare `try:` + # (the `with` was inside the try). After the fix `try:` lives below + # the `with`, so it must not appear just above it. + window_start = source.rfind("\n", 0, idx - 200) + window = source[window_start:idx] + preceding_lines = [ln.strip() for ln in window.splitlines() if ln.strip()] + assert preceding_lines and preceding_lines[-1] != "try:", ( + "regressed: `try:` is wrapping `with tracer.trace(...)` again; " + "put try/except INSIDE the tracer span to keep the DD APM span clean" + )