Repository navigation
fix(proxy): retry lock-timed-out daily spend batches in place so the shutdown flush keeps them - #45273
Conversation
…shutdown flush keeps them Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
…xt in assert messages Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
bugbot run |
…_daily_spend_lock_timeout
|
bugbot run |
…_daily_spend_lock_timeout
|
bugbot run |
…_daily_spend_lock_timeout
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit f56a891. Configure here.
TLDR
Problem this solves:
integration-accountingred on every scheduledmainrun since fix(proxy): bound daily spend rollup row-lock waits with lock_timeout and requeue 55P03 #44450How it solves it:
lock_timeout(55P03) batch in placeUser Flow
Before: an admin restarting a 2-worker proxy under load sees a user's daily request count fall short of the team's
After: the same restart keeps every request, so both views agree
CircleCI
Red on
main, jobintegration-accounting, failing testtests/integration/spend/test_shutdown_flush.py::test_daily_spend_burst_across_two_workers_survives_shutdown_during_commit_exactly_onceCulprit: 2477635
fix(proxy): bound daily spend rollup row-lock waits with lock_timeout and requeue 55P03(#44450). Bisect over the scheduled green/red range: its parent passes every time, it and every later commit fail most runsMechanism: #44450 runs every spend rollup transaction under
lock_timeout=5s. InDBSpendUpdateWriter._update_daily_spenda 55P03 counts as requeue-safe, but the in-place retry loop only retries connection errors and deadlocks, so the batch leaves immediately and_flush_daily_spend_queueputs it back on the in-memory queue "for retry on next tick". The shutdown flush (flush_spend_counters_on_shutdown) is the last tick, so a daily batch waiting more than 5s on a row a sibling worker is still committing is lost. #44450 already documents 55P03 as retry-safe and retries it in_handle_spend_update_retry; the daily writer was missedFix:
_update_daily_spendtreatsPrismaDBExceptionHandler.is_lock_timeout_erroras retryable alongside deadlocks, with the existing backoff andn_retry_times. Once retries run out the batch is still requeued as before, so normal ticks keep #44450's bounded waits. New unit testtest_update_daily_spend_retries_lock_timeout_errorsPR-branch CircleCI verification: https://app.circleci.com/pipelines/github/BerriAI/litellm/91047 at f56a891 (main 9f150f7 merged in, PR diff unchanged). On https://app.circleci.com/pipelines/github/BerriAI/litellm/91042 at 69d5750
test_shutdown_flushpassed inintegration-accounting(job 2251802) and every other failure matched main 91036 except intermittent replica and migration jobs;schema-migrationpassed on rerun and the Lens pagination flake is fixed on main by #45346, now merged in. Earlier https://app.circleci.com/pipelines/github/BerriAI/litellm/91023 at 436e83b (earlier 91018 at 4733842, where test_shutdown_flush passed while it failed on main pipeline 91016, and 91015 at 74d4a2c). Baseline on currentmain(03ba79d) for jobs this PR does not touch: https://app.circleci.com/pipelines/github/BerriAI/litellm/91016Deferred (distinct root causes, equally red on
main, left for the next run):integration-providers:test_cache_control_tool_call_marks_wire.pyresponse-cache tests send 5+ message conversations that fix(caching): skip the cache past max_messages and keep tool_result text in semantic prompts #43878'smax_messages=4cache cap now skips by design; also usage-based routing Redis testsintegration-accounting:test_vertex_image_regional_uplift.pyburst tests, red since they landed in fix(vertex_ai): apply regional endpoint uplift on image generation cost path #44679integration-database,integration-database-replica:test_partition_transactions::test_real_partition_ddl_survives_witnessed_lock_and_is_idempotentintegration-extensions:test_passthrough_upstream_error_chaos::test_passthrough_worker_sigkill_leaves_sibling_serving_and_loggingintegration-management:test_cli_sso_login_ui_disabled::test_burst_of_logins_recovers_from_an_idp_token_outageproxy-behavior: Lenstest_scan_lifecycle_persists_results_and_revokes_worker, ande2e_ui_testing,e2e_ui_testing_server_root_path, red identically on main pipeline 91016integration-providerson 91023 also fails thetest_decisions_basic_*andtest_decisions_wiretests and a realtime transcription guardrail test that arrived with the main merge (feat(decisions): serve System One format at /v1/systemone and OpenAI format at /v1/decisions #45184, feat(decisions): add OpenAI as a Decisions provider behind a shared decisions format #45214), not in this diffrust-wheel(test_schema_setup_uses_configured_retention) fails on main d6e453f too; neither it norrust-testis a required checkPre-Submission checklist
uv run pytest tests/unit/<your_test_file>.py -v. Leave the suites (make test-unit-*,make test-unit) to CI: it finishes in ~15 minutes where a laptop takes an hour or more@greptileaito re-request a review after pushing changes)Screenshots / Proof of Fix
Live A/B on a real proxy (
litellm --num_workers 2, real Postgres, real OpenAI gpt-4o-mini), merge base 0e09e99 vs 74d4a2c (later commits up to 436e83b only touch the unit test or merge main, product code is identical). A user key on a team sends 1 request, a 15s slow COMMIT is injected on that user's daily row (same fault as the CircleCI test), 30 concurrent POST /v1/chat/completions follow, the proxy gets SIGTERM mid-commit and is booted again, then GET /user/daily/activity and GET /team/daily/activity are readThe loss is a race (a sibling commit must overlap the 5s lock_timeout), which is why CircleCI flakes rather than failing every run. Without a shutdown and with the same slow commit, both commits record exactly 40 requests for user and team (no loss, no double count), and /v1/chat/completions, /v1/messages and /v1/responses bill identically on both
CircleCI on this branch: https://app.circleci.com/pipelines/github/BerriAI/litellm/91047 (latest), https://app.circleci.com/pipelines/github/BerriAI/litellm/91042, https://app.circleci.com/pipelines/github/BerriAI/litellm/91023
Type
🐛 Bug Fix
Final Attestation
The tests check the right things, including the edge cases, and regressions in the respective real-world customer use-cases are not possible after this PR
436e83b passes /live-pr-risk (f56a891 only merges main, PR product diff unchanged)
Note
Medium Risk
Touches proxy spend accounting commit paths under contention; behavior change is narrowly scoped to retrying safe lock-timeout rollbacks, with a regression test.
Overview
Daily spend commits now retry Postgres
lock_timeout(55P03) inside_update_daily_spend, matching the retry rules already used by the other spend writers viahandle_spend_update_failure.When a bulk daily upsert hits a lock timeout, the batch is retried with the existing backoff instead of failing out and relying on a later queue tick. That matters on shutdown flush, where there is no next tick and under-counted user daily activity could diverge from team totals after a multi-worker restart under load.
A unit test asserts that a simulated 55P03 on the first attempt triggers a second
execute_rawand drains the in-memory transaction dict without invoking the failure handler.Reviewed by Cursor Bugbot for commit f56a891. Bugbot is set up for automated code reviews on this repo. Configure here.