Repository navigation
fix(proxy): look up hashed key names with two spend log rows per key #43656
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
dbeb405
bea6f49
f2d7062
59d6c46
54da4e5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,7 @@ | |
| from dataclasses import dataclass | ||
| from datetime import datetime, timedelta | ||
| from types import MappingProxyType | ||
| from typing import Final, TypeVar | ||
| from typing import Final, Literal, TypeVar | ||
|
|
||
| from pydantic import BaseModel, TypeAdapter | ||
| from typing_extensions import ReadOnly, TypedDict | ||
|
|
@@ -39,26 +39,37 @@ | |
| ORDER BY token, deleted_at DESC | ||
| """ | ||
|
|
||
| _SPEND_LOG_ALIAS_SQL: Final = """ | ||
| SELECT api_key AS digest, | ||
| MIN(key_alias) AS first_alias, | ||
| MAX(key_alias) AS last_alias, | ||
| MIN(team_id) AS first_team, | ||
| MAX(team_id) AS last_team, | ||
| MIN(user_id) AS first_owner, | ||
| MAX(user_id) AS last_owner | ||
| FROM ( | ||
| SELECT api_key, | ||
| NULLIF(metadata->>'user_api_key_alias', '') AS key_alias, | ||
| COALESCE(NULLIF(team_id, ''), NULLIF(metadata->>'user_api_key_team_id', '')) AS team_id, | ||
| COALESCE(NULLIF("user", ''), NULLIF(metadata->>'user_api_key_user_id', '')) AS user_id | ||
| FROM "LiteLLM_SpendLogs" | ||
| WHERE api_key = ANY($1::text[]) | ||
| AND "startTime" >= $2::timestamp | ||
| AND "startTime" < $3::timestamp | ||
| ) named | ||
| WHERE COALESCE(key_alias, user_id, team_id) IS NOT NULL | ||
| GROUP BY api_key | ||
|
|
||
| def _named_spend_log_edge_row_sql(direction: Literal["ASC", "DESC"]) -> str: | ||
| return f""" | ||
| SELECT key_alias, team_id, user_id | ||
| FROM ( | ||
| SELECT "startTime", | ||
| NULLIF(metadata->>'user_api_key_alias', '') AS key_alias, | ||
| COALESCE(NULLIF(team_id, ''), NULLIF(metadata->>'user_api_key_team_id', '')) AS team_id, | ||
| COALESCE(NULLIF("user", ''), NULLIF(metadata->>'user_api_key_user_id', '')) AS user_id | ||
| FROM "LiteLLM_SpendLogs" | ||
| WHERE api_key = keys.digest | ||
| AND "startTime" >= $2::timestamp | ||
| AND "startTime" < $3::timestamp | ||
| ) named | ||
| WHERE COALESCE(key_alias, user_id, team_id) IS NOT NULL | ||
| ORDER BY "startTime" {direction} | ||
| LIMIT 1 | ||
| """ | ||
|
|
||
|
|
||
| _SPEND_LOG_ALIAS_SQL: Final = f""" | ||
| SELECT keys.digest, | ||
| first_row.key_alias AS first_alias, | ||
| last_row.key_alias AS last_alias, | ||
| first_row.team_id AS first_team, | ||
| last_row.team_id AS last_team, | ||
| first_row.user_id AS first_owner, | ||
| last_row.user_id AS last_owner | ||
| FROM unnest($1::text[]) AS keys(digest) | ||
| CROSS JOIN LATERAL ({_named_spend_log_edge_row_sql("ASC")}) first_row | ||
| CROSS JOIN LATERAL ({_named_spend_log_edge_row_sql("DESC")}) last_row | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Covering interior rows needs a probe per field, and a field missing on every row reads them all again, the timeout this PR removes
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That’s fair. Recovering interior values independently would require additional per-field probes, and a field absent from every row would still force a full scan—reintroducing the timeout this change is intended to eliminate. Given the documented trade-off of using only the oldest/newest named rows and the flat-cost requirement, I’m withdrawing the finding; no change is needed for this PR. |
||
| """ | ||
|
|
||
| _SPEND_LOG_STATEMENT_TIMEOUT_SQL: Final = f"SET LOCAL statement_timeout = {SPEND_LOG_KEY_METADATA_QUERY_TIMEOUT_MS}" | ||
|
|
@@ -84,7 +95,9 @@ class _TokenDigestRow(BaseModel): | |
|
|
||
|
|
||
| def _unanimous(first: str | None, last: str | None) -> str | None: | ||
| return first if first == last else None | ||
| if first is None: | ||
| return last | ||
| return first if last is None or first == last else None | ||
|
|
||
|
|
||
| class _SpendLogDigestRow(BaseModel): | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a key’s owner, team, or alias changes and then changes back within the window, the oldest and newest named spend logs agree while a row between them does not. The two
LIMIT 1lookups miss that conflict, so recovery can report a misleading owner or label in daily-activity data. The previous all-row comparison withheld fields with conflicting values. Logs tied at an endpoint timestamp can also make the selected value arbitrary.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deliberate, see the Low caveat: the all-row check is what timed out, and a mid-window flip back still names one of that key's real owners
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That’s fair. The behavior is deliberate and explicitly documented in the Low caveat: the endpoint now treats agreement between the oldest and newest named rows as sufficient, and a flip-back still resolves to a real owner/alias rather than inventing metadata. Given the timeout/performance goal and the accepted semantics, I’m withdrawing this concern.