Repository navigation
Follow-ups from nan-devops#290 (credential scrub): unhooked routes, previous_models, enterprise pod-age, and test/CI pins #46
Description
nan-devops#290 merged as a548384 and is deployed. Its three blocking reviewers each answered MERGE-BLOCKING: none on the deployed artifact and filed these as follow-ups. Grouped by whether they can leak.
Can leak, latent today
-
Unhooked routes get no scrub, and the counter cannot see it. Every sink is reachable only because the scrub runs in
async_pre_call_hook.google_endpoints,batches,openai_files_endpoints,rag_endpointsand the assistants/threads handlers calladd_litellm_data_to_request— which is what creates the leak — but neverpre_call_hook. There it is sink 1 that leaks, the one that was in ~100% of rows. Measured: zero requests to any of them in 7 days, so nothing flows today. Butnan-credential-scrub-stoppedkeeps moving on the hooked traffic, so if one of those routes is ever enabled the alert stays green through a leak. -
metadata.previous_models— LiteLLM:metadata.previous_modelsleaks one member's credentials into another member's SpendLogs row #45, still open, and not mentioned inMONITORING.md.Router.previous_modelsis an INSTANCE attribute, so a row can carry another member's headers. Needs the post-deploy probe with a forced fallback that the hook docstring calls for, and a line in MONITORING.md so a responder reading "clean" from step 3 knows a second column can still carry a token. -
_redact_credential_urldoes not visitmetadata.requester_metadata.endpoint. The header scrub coversrequester_metadata.headers; the URL scrub coversproxy_server_request.url,metadata.endpointandlitellm_metadata.endpointonly. Inert in practice (only the GooglegenerateContentroutes acceptkey=in the query, and those are the unhooked routes from item 1 with zero traffic), but it is an asymmetry against the header scrub. -
raw_headersas a list of tuples is not scrubbed (isinstance(raw, dict)). Not silent: the counter would fall to zero andnan-credential-scrub-stoppedwould fire. Worth aMapping/sequence branch if upstream ever changes the shape. -
data["api_key"]is a top-level STRING, frozen by the shallow copy, so no in-place scrub reaches it. It only appears ifforward_llm_provider_auth_headersis set, which is unset in both tenants. If anyone turns that on, this scrub does not cover that sink.
Operational
-
Enterprise still has the pod-age bug.
litellm-enterprise/values.yamlkeepsmaximum_spend_logs_retention_interval: "6h"with no cron. Same latent failure the community comment diagnoses in litellm-community: the SpendLogs pruner is not running — retention window grows monotonically #44 — it looks healthy only because its pods happen to outlive the interval. Give itmaximum_spend_logs_cleanup_crontoo. -
The traffic gate is the only consumer of
litellm_proxy_total_requests_metric_totalin the whole alerting file. If a LiteLLM upgrade renames it,nan-credential-scrub-stoppedgoes silent and no other rule notices. Either anabsent()canary on that metric or reuse a metric more rules depend on. -
Stale operator-facing figures. The runbook and the
values.yamlcomment say "~136 GB"; the table measured 154 GB with ~970k rows during the review (the width is TOAST, not the row count). Also worth recording: the interval job DID fire once, ~24h after the 2026-09-08 22:56 restart, so the backlog beyond the 2d window is only ~30k rows — which confirms the pod-age diagnosis rather than contradicting it. -
MONITORING.mdplaces the counter bullet in the ENTERPRISE counters section with a bold "THIS IS ALERTED", while the rule is community-only (job="litellm-community"). The section preamble already says enterprise is not paged, so no wrong action follows, but "community only" would close the ambiguity. Related: the enterprise reading of the runbook table givesrows>0, path_present=0, which the table calls "THE PATH MOVED"; on enterprise it just meansstore_prompts_in_spend_logsis unset. -
Step 3 of the runbook has no
SET statement_timeout. ThestartTimepredicate is indexed and returned well under a second on the primary, so it is not a danger today, but prefixing it costs nothing — and a reviewer's exploratory query took that primary down during this review.
Test and CI pins
- Assorted surviving mutants in test files and workflow wiring, none of which reach production:
if:/continue-on-errorat other levels, unpinned fields, and the general problem that each pin is itself new unpinned surface. Seventeen rounds of review closed ~125 of these; the remainder are bounded by the structural checks now in place (label parity by corpus, sink parity by result, and the CI meta-gate that requires every file a suite reads to trigger a workflow that runs it).
Not doing
Key rotation. The owner decided against it on #43, recorded there with the reasoning and the compensating controls. That decision stands; do not reopen it by default.
Activity