fix(tests): stop DATABASE_URL env pollution from read-replica tests breaking DB e2e tests - #32653
Conversation
…reaking DB e2e tests
Greptile SummaryThis PR fixes an intermittent CI failure where
Confidence Score: 5/5Test-only changes with no production code modified; the fix is mechanically correct and matches the failure mode described in the PR. All changes are confined to test infrastructure for the proxy/db directory. The hook-based guard correctly places the snapshot before any fixture runs and the check after all fixture teardowns. No assertions are weakened and no production behavior changes. No files require special attention.
|
| Filename | Overview |
|---|---|
| tests/test_litellm/proxy/db/conftest.py | New conftest that snapshots DB env vars before setup and enforces no-leak at teardown via hook wrappers; also provides the unset_database_url fixture that gives monkeypatch a restore record even when the key starts unset. |
| tests/test_litellm/proxy/db/test_routing_prisma_wrapper.py | Two leaking tests now declare unset_database_url and drop the monkeypatch.delenv(raising=False) that was the root cause of xdist worker poisoning. |
| tests/test_litellm/proxy/db/test_rds_iam_token_expiry.py | Both setup_env fixtures converted from manual os.environ pop/set to monkeypatch.setenv; cleanup ordering is now correct and hand-rolled finally blocks are removed. |
| tests/test_litellm/proxy/db/test_db_url_settings.py | The autouse _scrub_db_env fixture converted from hand-rolled try/finally to monkeypatch.setenv+delenv, fixing an ordering bug where the hand-rolled restore was clobbered by monkeypatch's own undo. |
Reviews (1): Last reviewed commit: "fix(tests): stop DATABASE_URL env pollut..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
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 4058667. Configure here.
Relevant issues
Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@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
This fixes a CI-only test flake, so the proof is the deterministic reproduction of the worker poisoning itself; there is no proxy behavior change to demonstrate
CI occurrences of the flake, all on PRs that touch nothing related: https://github.com/BerriAI/litellm/actions/runs/28994072776/job/86039902861 (first attempt, poisoned worker gw1), https://github.com/BerriAI/litellm/actions/runs/28994072776/job/86048515541 (full job rerun, poisoned worker gw0), https://github.com/BerriAI/litellm/actions/runs/29036129022/job/86181354539 (different branch, same failure). Each shows
1 failed, 4242 passed, ..., 2 rerunwithwriter.aurora.localis a fake hostname that only exists intests/test_litellm/proxy/db/test_routing_prisma_wrapper.pyBefore, captured at 60729f7 (the branch point).
PYTEST_XDIST_WORKER=gw0mirrors a CI xdist worker (it disables the single-process litellm module reload intests/test_litellm/conftest.py), and poppingDATABASE_URLafter importing litellm mirrors CI where the proxy-infra job defines noDATABASE_URL(locally a.envcan set one at import time)That is the exact CI failure, reproduced deterministically in one process: the routing wrapper tests pass but leave the fake URL in
os.environ, and the DB e2e test, which skips whenDATABASE_URLis unset, arms itself on the poisoned value and diesRegression guard demonstration, captured at 60729f7 plus only the new
tests/test_litellm/proxy/db/conftest.py(no other change): the leaking tests now fail at teardown, at the culprit, instead of poisoning a distant testAfter, captured at 4058667 (this PR)
And the CI-shaped run at 4058667 (2 xdist workers, loadscope, whole db dir plus the e2e file,
DATABASE_URLunset)Type
🐛 Bug Fix
✅ Test
Changes
PrismaWrapper.get_rds_iam_token(litellm/proxy/db/prisma_client.py:329) writes the freshly minted writer URL intoos.environ["DATABASE_URL"]as a side effect. Two tests intests/test_litellm/proxy/db/test_routing_prisma_wrapper.pyexercise that path with the fake hostwriter.aurora.localand guarded the env var withmonkeypatch.delenv("DATABASE_URL", raising=False). When the key is absent, which is the case in the proxy-infra CI job,delenv(raising=False)records nothing, so monkeypatch has nothing to restore and the fake URL written by product code survives the test. The pytest-xdist worker process is then permanently poisoned:TestDeprecatedKeyLookupDbE2E::test_deprecated_key_grace_period_cache_hit_pathskips itself whenDATABASE_URLis unset, but on a poisoned worker it sees the fake URL, connects Prisma towriter.aurora.local:5432and fails with P1001. pytest-rerunfailures reruns the failed test inside the same worker process whereos.environis still poisoned, so the two configured reruns can never rescue it; and--dist=loadscopeassigns the routing module and the single-test e2e class to workers based on runtime timing, so the two only sometimes land on the same worker, which is why unrelated PRs fail intermittently (gw1 in one linked run, gw0 in its rerun)The fix has two layers. First, a new
tests/test_litellm/proxy/db/conftest.pymakes the bug class impossible for the whole directory: apytest_runtest_setup/pytest_runtest_teardownhookwrapper pair snapshots theDATABASE_*env keys before any fixture runs and, after all fixture finalizers (including monkeypatch undo) have run, restores the snapshot and fails the test loudly if anything leaked. It has to be a hook pair rather than an autouse fixture becauseisolate_host_aws_configin the parent conftest requestsmonkeypatch, so the shared monkeypatch instance always finalizes after any module-level fixture and a fixture-based guard would compare against pre-undo state. Second, the two leaking tests now use aunset_database_urlfixture that registers a setenv+delenv pair, giving monkeypatch a restore record even when the key started out unsetRunning the guard over the directory also exposed two more latent leaks of the opposite direction (deleting a
DATABASE_URLthat was set at session start, which matters for anyone running with a.envand for future CI jobs that setDATABASE_URL): thesetup_envfixtures intest_rds_iam_token_expiry.pywrote and poppedos.environdirectly, and_scrub_db_envintest_db_url_settings.pyused a hand-rolled snapshot/restore that runs before monkeypatch undo and gets clobbered by it. Both now route through monkeypatch so restoration ordering is correctThe product-code
os.environmutation itself is left alone: the writer wrapper deliberately relies on Prisma re-readingDATABASE_URLfrom env on reconnect, so removing the side effect is a larger refactor than a test flake fix should carryNote
Low Risk
Changes are confined to test infrastructure and env isolation; no proxy or runtime behavior is modified.
Overview
Adds
tests/test_litellm/proxy/db/conftest.pywith setup/teardown hooks that snapshot allDATABASE_*env keys, restore them after fixtures (includingmonkeypatchundo), and fail the test if anything still leaked. Also adds anunset_database_urlfixture that registerssetenv+delenvsomonkeypatchcan restoreDATABASE_URLwhen it started unset—fixing the case wherePrismaWrapper.get_rds_iam_tokenwrites a fake URL intoos.environand poisons later tests on the same xdist worker.Updates
_scrub_db_envintest_db_url_settings.pyandsetup_envintest_rds_iam_token_expiry.pyto usemonkeypatchinstead of manual snapshot/pop. The two writer IAM tests intest_routing_prisma_wrapper.pynow depend onunset_database_urlinstead ofdelenv(..., raising=False)alone.No production code changes.
Reviewed by Cursor Bugbot for commit 4058667. Bugbot is set up for automated code reviews on this repo. Configure here.