mirror of
https://github.com/BerriAI/litellm.git
synced 2026-10-10 03:28:53 +00:00
fix(proxy/auth): stop tainting the check-cache DD APM span with the
expected cache-miss exception (#25966) 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
This commit is contained in:
parent
b8f7d61400
commit
32d7f0013d
2 changed files with 121 additions and 5 deletions
|
|
@ -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"):
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
)
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue