The reset-race fix's new except Exception blocks pushed BLE001 4 over
ruff-strict-budget.json's limit. Each catches a Redis or reconcile
failure of unknown type by design, matching the noqa pattern this file
already uses on the equivalent non-cancel release path, so suppress
rather than narrow.
Addresses CodeQL and greptile findings on the reset-race fix.
CodeQL flagged reset_budget_job.py's module-level imports of RedisCache,
the two RESET_BUDGET_SPEND_COUNTER_RESET_* constants, and
evict_and_broadcast as cyclic-import risks: each target module can reach
back to this one before finishing its own initialization. RedisCache is
only used as a type hint, so it moves under TYPE_CHECKING with the
annotations quoted; the constants and evict_and_broadcast are only used
inside single functions, so they move to local imports there, per this
repo's own stated exception for avoiding circular imports.
Also fixes a CodeQL "empty except with no explanatory comment" on the
cancel-path's CancelledError handler, and trims the docstrings this fix
added or touched down to this repo's comment conventions (concise,
non-obvious-why only).
Addresses two concurrency findings from automated security review on
the reset/invalidation fallbacks added for #30460.
reset_budget_job.py: a reset that failed and retried replayed an
unconditional SET to new_spend on every attempt. A request's Redis
INCR landing in the delay between a failed attempt and its retry (a
legitimate reservation against the just-reset budget) would be
silently erased by the next attempt's write, since retrying and the
final delete both treat the counter as static rather than possibly
having moved. Replaced the retry's SET with
async_reset_preserving_delta, a single Lua GET/compute/SET that resets
to new_spend plus whatever was added on top of a snapshot taken once
before the first attempt and held fixed across retries, so a
concurrent increment survives no matter which attempt eventually
succeeds. If the snapshot read itself fails, there's no safe baseline
to preserve against, so that case now skips straight to the existing
delete fallback rather than attempting a reset that could guess wrong.
budget_reservation.py: release_budget_reservation_on_cancel's fallback
on a reconcile failure invalidates the shared key/user/team counters
outright. Unlike reconcile, which only ever adjusts this reservation's
own recorded contribution, that invalidation deletes state a
concurrent reservation or recorded spend also shares on the same
counter. A transient failure (e.g. a Redis timeout) during
cancellation would take the destructive path immediately. Now retries
the reconcile itself, which is safe and idempotent, a bounded number
of times first, and only falls back to invalidating the aggregate
counters once every retry hits the same failure.
Both fixes are covered by regression tests that simulate the race
directly (a concurrent increment landing during the reset retry delay,
and a reconcile that fails once then recovers on the cancel path) and
assert the concurrent write survives / the aggregate counter is not
touched.
Fixes three of the phantom Redis spend-counter inflation mechanisms
reported in #30460 for a multi-pod deployment with intermittent Redis
timeouts.
Path 2: reset_budget_for_litellm_keys/_users/_teams (the per-row
budget_duration reset path) invalidated the Redis spend counter but
never dropped the corresponding user_api_key_cache entry, unlike the
budget-table cascade path. A stale cached object could get read back by
_get_source_cache_base_spend and re-seed the counter with the pre-reset
spend. Also route that invalidation through the existing cross-pod
broadcast (evict_and_broadcast/LIT-3803) instead of a local-only
delete, since only one pod runs the reset job per tick and every other
pod's cache needs to drop its copy too.
Path 3: a failed Redis SET-to-zero during a budget reset only logged a
warning and left the inflated pre-reset counter in place until its TTL.
Now retries the SET a bounded number of times with a short backoff, and
falls back to deleting the key (which reads as cold and reseeds from
the DB) if every attempt fails, logging at ERROR instead of WARNING.
Path 1 (narrow): release_budget_reservation_on_cancel silently
swallowed a reconcile failure with no invalidation fallback, unlike its
sibling release_or_invalidate_budget_reservation. Added the same
invalidate-and-mark-finalized fallback so a Redis timeout during the
cancel-path reconcile cannot leave a pre-charge stuck in the counter.
The broader pre-charge/reconcile path already retries via a
delete-and-reseed pattern (increment_spend_counters_pipeline /
_reconcile_budget_reservation_for_counter_update), so no further change
was made there.
Relates to #30460
feat(auth): breached password detection and forced change
BREAKING CHANGE: users can no longer change their password by issuing a request with a password parameter to /user/update; this has been replaced with /user/password/change dedicated to secure password change.
Annotate screen_login_password_for_breach's update/where dicts with
prisma input TypedDicts and replace authenticate_user's conditional
dict splat with plain keyword arguments, clearing the LIT002 lines
this branch added in login_utils.py. No behavior change: an unflagged
login now passes allowed_routes=None and metadata={} explicitly, which
are the parameter defaults
A breach found during a login previously only flagged the account for the
NEXT login, handing out one free unrestricted 24h session. The HIBP screen
is now awaited before the session key is minted (worst case one 5s window
per user per 24h, fail-open unchanged), so a fresh hit restricts the
current session and the dashboard routes straight to change-password.
Also repairs two casualties of merge f5e47974db that the layout tests
caught: the lost usePathname import and a call to migratedHref, which
staging renamed to uiHref.
The Terraform endpoint audit wanted POST /user/password/change covered
or allowlisted; it is a caller-scoped one-shot action, so allowlist it
next to /user/bulk_update. leftnav.test.tsx mocked next/navigation
without useRouter, which SidebarAccountMenu now calls, so every render
in that file threw. The two unannotated audit-log patches in
test_password_endpoints.py get their test-quality-ok reasons.
Also removes the LIT002 violations the PR added: prisma input TypedDicts
annotate the where/data dicts, a shared HTTPExceptionErrorDetail
TypedDict covers the HTTPException detail dicts, and the route decorator
takes a tags tuple.
Admin password sets on /user/update and per-user /user/bulk_update stay
supported and policy-enforced. The request model hides the password from
repr so management alerts never format the plaintext, and the all_users
bulk path rejects passwords instead of writing one plaintext value to
every row.
The staging merge brought BLE001 into the strict ruff set and lowered the
LIT002 ceiling, so the HIBP fail-open except and the params/headers dicts
in password_policy.py now need their noqa and mutable-ok reasons. The
headers dict moves to an annotated Final so the suppression fits the line
limit.
/user/bulk_update awaited a separate HIBP lookup for each user in the
batch, so a degraded-slow HIBP (5s timeout per lookup) could stretch a
500-user batch to ~2500s and time out the request after some updates
had already persisted.
validate_passwords_bulk dedupes the batch's passwords, strength-checks
first, then fires every needed HIBP lookup concurrently, bounding the
worst case at one 5s timeout window. bulk_update_processed_users now
screens the whole batch before the serial update loop, so a rejected
password fails only its own entry and validation failures precede any
persistence.
CredentialLiteLLMParams omitted tenant_id, client_id, client_secret,
azure_scope, azure_username and azure_password, so the strict dump used
by credential reuse and Azure client init dropped them and the reused
credential ended with no auth at all
Co-authored-by: yassin <yassin@berri.ai>
Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
The e2e harness exists to prove product features end to end against a live
proxy. The prior Hard Rule carved out an exception for "tests that cover the
harness itself" and pointed at coverage_registry/test_collector.py, which in
practice invited unit tests of harness helpers to be staged alongside e2e
work. That is the wrong tool: harness logic that is worth locking down does
not need a mock-driven unit test living under tests/e2e.
Drop the carve-out. The Hard Rule now reads that no unit tests of any kind
belong under tests/e2e, and the passing mention of unmarked harness coverage
in the transport section is removed so the doc no longer contradicts itself.
coverage_registry/test_collector.py still exists on disk and is left in place
for now; whether to relocate or remove it is a separate decision.
Keeps the base's rule that a non-admin id lookup matching no spend-log row answers 403, so the detail route never consults cold storage without an owner row
* fix(proxy): bound tool and guardrail index create_many by the spend-log statement budgets
One flush drains up to MAX_LOGS_PER_INTERVAL source transactions or logs, but a
transaction fans out to one LiteLLM_SpendLogToolIndex row per tool and a log
to one LiteLLM_SpendLogGuardrailIndex row per guardrail, so the index
create_many payload was unbounded. Both index writes now go through
spend_log_write_batches(SPEND_LOG_WRITE_BATCH_MAX_BYTES, SPEND_LOG_WRITE_BATCH_MAX_ROWS).
The tool index write moves out of the rollup batch_() so the split reduces
the query-engine payload; replayed index rows are no-ops under
skip_duplicates, and the daily rollup upserts stay in one transaction
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
* test(proxy): pin the row budget in the index fan-out tests
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
---------
Co-authored-by: yucheng <yucheng@berri.ai>
Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
