Repository navigation
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Merging this PR will not alter performance
Comparing |
979a0b6 to
6aa2db3
Compare
chrikrah
left a comment
There was a problem hiding this comment.
@timrsmith I would merge this at 6aa2db3. Your tests bound the sync end to end; in process, upsert_credentials stays linear up to 40,000 credentials, at 13 ms for 20,000 and 28 ms for 40,000. Your revert result reproduces here too.
Five of the six red checks also fail on main at 632b69b. The sixth, the schema.d.ts check, compares the OpenAPI spec, and this diff adds no route or model.
$ python probe_upsert.py # upsert_credentials at 6aa2db3, n refreshed in place
5000 0.0028
10000 0.0056
20000 0.0132
40000 0.0279
$ pytest tests/unit/litellm_core_utils/test_credential_accessor.py \
tests/test_litellm/proxy/proxy_server/test_proxy_config.py -k credential -q
10 passed, 276 deselected in 9.42s
# credential_accessor.py and proxy_server.py reverted to b41715b, tests kept
AssertionError: delete_credentials over 20000 credentials took 10.2s
AssertionError: upsert of 20000 credentials took 15.6s
2 failed, 8 passed, 276 deselected in 33.11s
# Python 3.13; not run: the rest of tests/test_litellm/proxy
@yassin-berriai, you merged #39876, the last rework of this credential sync. Could this go into the next release, given the liveness restarts in #43922?
6aa2db3 to
a0c8094
Compare
|
Codecov's two missed lines are hit in the proxy-server shard's coverage.xml artifact; details in the Caveats section. |
…of credentials ProxyConfig.get_credentials runs on every config reload (default 30s) on the serving event loop. delete_credentials rebuilt the list of all credential names once per existing credential, and CredentialAccessor.upsert_credentials rescanned the list once per incoming credential, so both were O(n^2) in the size of the credentials table. With ~6k rows that is ~10s of blocking per reload: every request queued and the liveness probe timed out. Index the current list once and use set membership instead.
…ilter deletes in one pass Index the existing list by first occurrence so a name that appears twice (a config duplicate) still has the entry find_credential reads replaced, as before. Replace the index/sort/pop delete with a single in-place filter. Move the accessor tests under tests/unit so a CI shard owns them, hoist imports, drop narrative comments, assert whole values.
Follow the repo's functional conventions on the touched lines: a MappingProxyType built in one shot for the first-occurrence index, a frozenset for the names to keep, Sequence parameters and explicit None returns.
a0c8094 to
13f25d1
Compare
TLDR
Problem this solves:
/health/livelinesstimes out and Kubernetes kills the pod mid-requestHow it solves it:
delete_credentialskeeps names from one frozenset instead of rebuilding a name list per credentialupsert_credentialsindexes the current list once, first occurrence per name, instead of rescanning per credentialReplaces #43923, which closed when its head branch was renamed
User Flow
Before: a developer calling a proxy whose database holds thousands of credentials sees stalls of tens of seconds, and the pod restarts under them
x-litellm-overhead-duration-mssays 20After: the same proxy answers every call promptly and health probes never miss
Relevant issues
Fixes #43922
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
uv run pytest tests/unit/litellm_core_utils/test_credential_accessor.py tests/unit/proxy/proxy_server/test_proxy_config.py -k credential -vThe two scale tests go red on the previous code: with the fix reverted,
test_upsert_credentials_scales_linearly_with_credential_countfails with "upsert of 20000 credentials took 24.2s" and the delete test fails the same way. The three behavioural tests pass on both versions, which is the point of a performance-only rewriteScreenshots / Proof of Fix
Local
litellm-databaseproxy on localhost:4000 against Postgres withstore_model_in_db: true. Credentials were created with a curl loop, then/health/livelinesswas curled once a second for 60 seconds. Nothing here calls an LLM provider: the bug is in the reload job, and the health endpoint is what the kubelet seesBefore (657bb77)
20,000 credentials, 60 liveliness probes at 1/s
Only probes that waited over 1s are listed. One reload every 30s, and every one of them froze the worker for 16 to 34s, so the 60 probes took almost five minutes to complete
After (13f25d1, identical diff measured at d1bdd4d before rebasing onto main)
20,000 credentials, 60 liveliness probes at 1/s
Same database, same 20,000 credentials, same probe loop across two reloads. The slowest probe took 69ms
Caveats
Six checks fail on this PR and the same six fail on main's own head b41715b, so they are not caused by this change. code-quality and documentation report undocumented
CLICKHOUSE_*variables and the schema.d.ts check reports a newtrace_refquery parameter, all from the tracing feature in #43915. proxy-endpoints fails two batches tests oninput_file_id, proxy-infra fails the component allowlist test on three new routes, and misc fails four interactions OpenAPI compliance tests. None of those paths read or write credentialsCodecov's patch status briefly reported the two
delete_credentialsbody lines as missed while uploads were still landing; the proxy-server shard'scoverage.xmlartifact had them at 1 hit each, and codecov now reports all modified lines covered. The earlier schema-migration failure wastest_db_push_timeout_hint_names_the_per_command_budget, which failed on main itself and on a dozen branches after #44034 until main fixed it; this head is rebased past the fixStill on the serving loop after this fix, as follow-ups:
get_credentialsre-reads the config file just to learn the config credential names and decrypts every credential value even when no row changed, about 0.25s at 6k rows. Request-path lookups inCredentialAccessor.find_credentialand the Rust preflight scan stay linear per request; a name-keyed index there needs a single writer forlitellm.credential_list, which several call sites reassign directly todayType
🐛 Bug Fix