Skip to content

test(pgbouncer): stop the never-listens replacement test flaking under CI load - #40830

Merged
yassin-berriai merged 1 commit into
litellm_internal_stagingfrom
litellm_fix_flaky_pgbouncer_replacement_test
Sep 12, 2026
Merged

yassin-berriai merged 1 commit into
litellm_internal_stagingfrom
litellm_fix_flaky_pgbouncer_replacement_test

Conversation

@devin-ai-integration

Copy link
Copy Markdown
Contributor

TLDR

Problem this solves:

  • test_a_replacement_that_never_listens_is_replaced_again fails on loaded CI runners
  • The first start() only got 0.3s, effectively two 0.1s polls, to see the fake pooler listen
  • Under load the fake pooler's interpreter takes longer than that to start
  • TCP is bound before the unix socket, so the timeout branch says "served by another process"

How it solves it:

  • The pooler gets a 2s readiness budget: still short for the never-listens phase, roomy for the start
  • The port file is restored as soon as the wrong-port replacement is up, before the next spawn is due
  • No production code changes, the fix is scoped to the one test

User Flow

Before: a contributor's PR goes red on the proxy-infra unit shard for a change they did not make

  1. They push a commit and open the test-unit / proxy-infra job on their PR
  2. The job fails on tests/test_litellm/proxy/db/test_pgbouncer.py::TestPgBouncerProcess::test_a_replacement_that_never_listens_is_replaced_again with assert PgBouncerError(reason='127.0.0.1:34569 is served by another process, not the pgbouncer that was started') is None
  3. The two automatic reruns fail the same way, so they have to re-trigger the workflow by hand

After: the same shard passes on the first attempt

  1. They push a commit and open the test-unit / proxy-infra job on their PR
  2. tests/test_litellm/proxy/db/test_pgbouncer.py passes, the test takes about 2s longer than before
  3. Nothing to re-trigger

Relevant issues

Linear ticket

Pre-Submission checklist

Please complete all items before asking a LiteLLM maintainer to review your PR

  • I have added meaningful tests
  • The handful of test files covering my change pass locally, e.g. 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
  • My PR passes all required CI/CD checks (e.g., lint, schema.d.ts sync check, etc.)
  • My PR's scope is as isolated as possible; it only solves 1 specific problem
  • I have received a Greptile Confidence Score of at least 4/5 before requesting a maintainer review (Greptile reviews automatically once the PR is opened; only comment @greptileai to 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 PR only touches a unit test, so the proof is that test under the CPU contention that makes it fail in CI. Both runs below start 96 busy python3 -c 'while True: pass' loops on an 8 core box (about 12x oversubscription, comparable to a 4 worker xdist shard on a small runner) and then run the base and PR versions of the test side by side in the same pytest invocation. The base version is copied to a sibling file so both can run in one process

for _ in $(seq 96); do python3 -c 'while True: pass' & done
git show 7057b2f6c4:tests/test_litellm/proxy/db/test_pgbouncer.py > tests/test_litellm/proxy/db/test_pgbouncer_old_tmp.py
for i in 1 2 3; do
  LITELLM_LOCAL_MODEL_COST_MAP=True uv run --no-sync pytest \
    tests/test_litellm/proxy/db/test_pgbouncer_old_tmp.py tests/test_litellm/proxy/db/test_pgbouncer.py \
    -q -p no:cacheprovider -k test_a_replacement_that_never_listens_is_replaced_again 2>&1 \
    | grep -E "passed|failed|PgBouncerError"
done

Before (7057b2f)

  1. Run the loop above; test_pgbouncer_old_tmp.py is the merge base version of the test
  2. Output, three of three iterations fail with the CI error:
E       AssertionError: assert PgBouncerError(reason='127.0.0.1:58941 is served by another process, not the pgbouncer that was started') is None
E        +  where PgBouncerError(reason='127.0.0.1:58941 is served by another process, not the pgbouncer that was started') = start()
1 failed, 1 passed, 126 deselected, 1 warning in 84.24s (0:01:24)
E       AssertionError: assert PgBouncerError(reason='127.0.0.1:42449 is served by another process, not the pgbouncer that was started') is None
E        +  where PgBouncerError(reason='127.0.0.1:42449 is served by another process, not the pgbouncer that was started') = start()
1 failed, 1 passed, 126 deselected, 1 warning in 78.94s (0:01:18)
E       AssertionError: assert PgBouncerError(reason='127.0.0.1:34223 is served by another process, not the pgbouncer that was started') is None
E        +  where PgBouncerError(reason='127.0.0.1:34223 is served by another process, not the pgbouncer that was started') = start()
1 failed, 1 passed, 126 deselected, 1 warning in 75.28s (0:01:15)
  1. Isolating the first start() of the base test in a 40 iteration loop under the same load: 39 of 40 fail, 35 with "served by another process" and 4 with "did not start listening within 0s". start() took 0.33s to 0.37s, past the 0.3s budget

After (cf186cb)

  1. Same loop; test_pgbouncer.py is this PR's version of the test
  2. Output, the 1 passed in each of the three lines above is the PR version, run in the same process under the same load as the failing base version
  3. Isolating the first start() of the PR test in the same 40 iteration loop under the same load: 0 of 40 fail, start() took 0.35s median and 0.47s max against the 2s budget
  4. Without load, the whole mapped file: uv run --no-sync pytest tests/test_litellm/proxy/db/test_pgbouncer.py -q -n 4 -> 64 passed in 15.86s

Type

✅ Test

Caveats (if any)

Low

  • The never-listens phase now waits 2s instead of 0.3s, so the test is about 2s slower
  • 2s is a margin, not a proof: a runner 5x more starved than the 12x oversubscribed repro above could still trip it
  • The misleading "served by another process" wording for a pooler that bound TCP but not yet the unix socket at the deadline is unchanged; real PgBouncer binds both back to back, so it only shows up with a slow fake

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

Link to Devin session: https://app.devin.ai/sessions/df2cd953c2aa44ea86966af72381f8e1
Open in Devin Desktop: https://app.devin.ai/desktop/session/df2cd953c2aa44ea86966af72381f8e1?variant=devin
Requested by: @yassin-berriai

…r CI load

The initial start() ran with ready_timeout_seconds=0.3, so the fake pooler
had to be listening on both TCP and the unix socket within two 0.1s polls.
On a loaded runner the interpreter takes longer than that to start, TCP is
bound before the unix socket, and the timeout branch reports the port as
served by another process. Give the start 2s and restore the port file as
soon as the wrong-port replacement is up, so the next spawn reads the right
port however the supervisor thread is scheduled

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@greptile-apps

greptile-apps Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR stabilizes the PgBouncer replacement test under CI load

  • Increases the fake pooler's readiness budget from 0.3 to 2 seconds
  • Restores the intended port before the next replacement is spawned
  • Preserves assertions covering timeout logging, successful replacement, and wrong-port cleanup

Confidence Score: 5/5

The PR appears safe to merge because the timing change removes the CI race while preserving the test's regression coverage

The fake pooler reads its configured port only at startup, and the existing assertions still require the wrong-port process to time out, terminate, and be replaced on the intended port

Important Files Changed

Filename Overview
tests/test_litellm/proxy/db/test_pgbouncer.py Adjusts test-only readiness timing and port-file sequencing without weakening the replacement lifecycle checks

Reviews (1): Last reviewed commit: "test(pgbouncer): stop the never-listens ..." | Re-trigger Greptile

@codecov

codecov Bot commented Sep 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@yassin-berriai
yassin-berriai merged commit 98f6c14 into litellm_internal_staging Sep 12, 2026
79 checks passed
@yassin-berriai
yassin-berriai deleted the litellm_fix_flaky_pgbouncer_replacement_test branch September 12, 2026 02:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants