ci(e2e): use ephemeral host port for e2e Postgres to stop port-collision flake - #590
Open
yansun1996 wants to merge 2 commits into
Open
ci(e2e): use ephemeral host port for e2e Postgres to stop port-collision flake#590yansun1996 wants to merge 2 commits into
yansun1996 wants to merge 2 commits into
Conversation
The accounting E2E fixture published every per-test Postgres container to a fixed host port (55432). `docker rm -f` returns before docker-proxy releases the host port, so the next test's `docker run` intermittently failed with "address already in use", erroring the accounting_cluster fixture at setup. A leaked container could hold the port for an entire run, cascading the failure across all accounting tests. Publish with `-p 5432` so Docker allocates a free ephemeral port atomically at bind time, then resolve it via `docker port` and thread it into database_url. SPUR_TEST_PG_PORT still pins the port when set. Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a recurring native-host E2E flake where accounting tests intermittently fail to start Postgres due to collisions on a fixed host port, by switching to Docker-assigned ephemeral host ports and resolving the assigned port at runtime.
Changes:
- Stop pinning Postgres to a fixed host port by default; allow Docker to allocate an ephemeral port (
-p 5432). - Add logic to discover the ephemeral host port via
docker portand constructdatabase_urldynamically. - Preserve
SPUR_TEST_PG_PORTas an escape hatch to explicitly pin the host port when needed.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #590 +/- ##
==========================================
- Coverage 76.32% 76.28% -0.04%
==========================================
Files 166 166
Lines 65511 65511
==========================================
- Hits 49998 49973 -25
- Misses 15513 15538 +25 🚀 New features to boost your workflow:
|
docker run -p published the ephemeral-port Postgres container on all interfaces with known test credentials, even though spurctld only ever connects to it from the same node. Bind the publish spec to 127.0.0.1 explicitly; the existing docker port parsing is unaffected.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this fixes
The native-host E2E suite intermittently fails at
accounting_clusterfixture setup with:This is a top recurring flake in the
E2E / native-hostjob (e.g. run 31138116947 errored one accounting test and went green on re-run; run 31132066804's sibling run had a leaked container hold the port for ~3h, cascading the error across every accounting test).Root cause
Each accounting test stands up its own Postgres container on node 0, but the fixture published them all to a fixed host port (
-p 55432:5432). The ~30 accounting tests run serially, each start/stop cycling Postgres.docker rm -freturns before docker-proxy actually releases the host port, so the next test'sdocker runintermittently collides. A leaked container can hold the port for a whole run, turning a single flake into a full-suite cascade.Approach
Stop pinning the host port. Publish with
-p 5432so Docker allocates a free ephemeral port atomically at bind time (race-free — no window between "check free" and "bind"), then discover the assigned port viadocker portand thread it intodatabase_url. A leaked/slow-release container now holds a random port, never the one the next test needs.SPUR_TEST_PG_PORTstill pins the port when explicitly set, preserving the deterministic-port escape hatch.Testing
Ran on a real 2-node lab: 11 accounting tests, each cycling its own Postgres container serially (the exact pattern that produced the collision on the old code). All passed; containers came up on ephemeral host ports (32780, 32795, ...) and the harness resolved each correctly. A/B check confirmed the previously-observed
pool timed outfailure was an unrelated stale-process artifact on the shared lab node, identical with both fixed and ephemeral ports.