fix(db): apply the configured connection params to the read replica URL - #37691
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
|
Greptile SummaryThe PR now applies configured pool and timeout parameters to read-replica URLs while preventing writer-specific schema options from reaching the replica.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains from the previously reported writer-options or configured-extra-options replica leaks.
|
| Filename | Overview |
|---|---|
| litellm/proxy/db/db_url_settings.py | Adds allowlisted connection-parameter inheritance while excluding writer options and schema; the previously reported writer-options leak is fixed. |
| litellm/proxy/proxy_cli.py | Applies only replica-safe configured parameters and derives timeout options from the replica URL itself; the previously reported extra-options leak is fixed. |
| tests/test_litellm/proxy/db/test_db_url_settings.py | Adds focused coverage for inheritance, replica overrides, URL preservation, and exclusion of schema-affecting writer parameters. |
| tests/test_litellm/proxy/test_proxy_cli.py | Adds CLI-level regression coverage confirming configured writer schema options remain absent from the replica URL. |
Reviews (3): Last reviewed commit: "fix(db): apply the configured connection..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
0771b61 to
7f888c9
Compare
|
@greptileai re-review 7f888c9: options is out of the inherited allowlist, plus tests pinning allowlist semantics and the reader's own search_path |
The read replica never received the operator's DB pool settings, so its Prisma pool fell back to `num_physical_cpus * 2 + 1` and the configured cap was not enforced. Both startup paths now pass the same params to the reader: the CLI, and the componentized entrypoints that go through `DatabaseURLSettings.apply_to_env`. Only pool and timeout params are inherited, through a single allowlist both paths share. Anything that decides which tables a query resolves against stays on the writer, including entries smuggled in through `database_extra_connection_params`, so a writer `search_path` cannot repoint reader queries. Params the operator pinned on the replica URL still win.
7f888c9 to
d3f801f
Compare
|
@greptileai re-review d3f801f: the CLI reader path now filters database_extra_connection_params through the same allowlist, with a regression test covering it |
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
Staging merge 0bf8fe0 was authored by someone else. Verified independently: import block is the union, allowlist and all four regression tests intact. |
TLDR
Problem this solves:
How it solves it:
User Flow
Before: an operator caps DB connections to protect their database, but the read replica opens as many connections as it likes
general_settings.database_connection_pool_limit: 3anddatabase_connection_pool_timeout: 20, pointDATABASE_URL_READ_REPLICAat their reader, and start the proxyGET http://localhost:4592/key/listwith the master key, all returning 200num_physical_cpus * 2 + 1default, far more than the 3 they configuredAfter: the same cap applies to the reader, so the total stays where the operator put it
DATABASE_URL_READ_REPLICAat their reader, and start the proxyGET http://localhost:4592/key/list, all returning 200?connection_limit=50on the replica URL themselves, that value is still what gets usedsearch_paththey set on the writer, whether onDATABASE_URLor throughdatabase_extra_connection_params, still does not reach the reader, so replica queries keep resolving against the reader's own schemaRelevant issues
Linear ticket
Resolves LIT-5692
Review notes
Inheritance is an allowlist of pool and timeout params (
connection_limit,pool_timeout,connect_timeout,socket_timeout,pgbouncer), deliberately not a denylist. A denylist over Postgres' open-ended parameter space cannot be completed, and the one you forget is the one that hurts:optionscarriessearch_path, so copying it wholesale would silently repoint every replica query at the writer's schema. An allowlist fails closed, and a parameter nobody has vetted for the reader stays on the writer by default. There is a test pinning exactly that, so flipping this back to a denylist breaks the build rather than quietly widening what the reader inherits.Both startup paths share the one allowlist. That matters for the CLI in particular, because
database_extra_connection_paramsis an untyped passthrough whose keys override everything else, so without the filter an operator could route a writersearch_pathto the reader through config rather than through the URL. The CLI does still setoptionson the reader, but it builds that value from the reader's ownoptionsplus the configured statement and lock timeouts, never from the writer's.History note: this branch carries a staging merge,
0bf8fe09ba, that was authored by someone other than the PR author, resolving conflicts against the Azure Entra ID token-auth work. The resolution was reviewed independently before being adopted: thedb_url_settings.pyimport block is the union of both sides rather than a pick, which matters because taking either side alone yields a module that imports cleanly and fails at runtime, and the reader-parameter allowlist, its two helpers, the CLI call site, and all four regression tests are present and unchanged.Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
uv run pytest tests/test_litellm/<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)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
Shared setup: a real local Postgres, a replica URL pointing at the same database, and
pg_stat_activity.application_nameused purely to tell the two pools apart. The configured cap is 3 throughout. A namespaced port is deliberate:--port 4000silently rebinds to a random port when it is busy, so on a shared machine the health check can be answered by an unrelated proxyCase 1 pins the reader's own marker, so the reader pool is directly countable
Case 2 pins a
search_pathand a marker on the writer URL and nothing on the reader, so writeroptionsreaching the reader would relabel the reader's connections tooCase 3 does the same through config instead of the URL, which is the passthrough that overrides everything else
Before (a974464)
Case 1: reader pool size against a configured cap of 3
curl -o /dev/null -w '%{http_code}' http://localhost:4592/health/livelinessreturns 200count_connsCase 2: writer URL search_path must not follow the reader
count_conns(unset), so the writer'soptionsnever reached them, but they climb to 9 rather than the configured 3Case 3: configured extra connection params must not carry a search_path to the reader
count_conns(unset), and the reader holds 13 connections rather than the configured 3After (0bf8fe0)
Case 1: reader pool size against a configured cap of 3
count_connslitellm_readermarker, so the reader's ownoptionssurvived the params being appendedCase 2: writer URL search_path must not follow the reader
count_conns(unset), so it picked up the cap without picking up the writer'ssearch_pathCase 3: configured extra connection params must not carry a search_path to the reader
count_conns(unset)Type
🐛 Bug Fix
Caveats (if any)
schemaand postgresoptionsare never inheritedFinal Attestation
Link to Devin session: https://app.devin.ai/sessions/2d1f0c3cd9234051a365e54ab1d647d0
Requested by: @yassin-berriai