Test containers: ephemeral host ports by default, so concurrent suites cannot collide - #571
Merged
Merged
Conversation
…s cannot collide The session-scoped testcontainers in `cardpicker/tests/conftest.py` bound FIXED host ports (47000 / 9300). A second `pytest cardpicker/` on this shared box died at container start with `Bind for 0.0.0.0:9300 failed: port is already allocated`, which surfaces as thousands of collection ERRORs that read as catastrophic breakage rather than a port clash - three separate sessions lost real time to it in one day. PR #537 made those ports env-overridable, but nothing ASSIGNED distinct values, so the default still collided. Overridability without allocation does not solve concurrency. Both containers are now started with no host binding at all (their testcontainers constructors already call `with_exposed_ports`), so Docker assigns a free ephemeral port per container and the fixtures read back what it actually assigned via `get_exposed_port()`. Nothing wants a specific port, so nothing can collide - a closed race, not a narrowed window. New session fixtures `postgres_port` / `elasticsearch_port` publish the resolved values; Django's `DATABASES["default"]["PORT"]`, `ELASTICSEARCH_DSL`, `settings.ELASTICSEARCH_PORT` and the pytest-elasticsearch plugin's own `elasticsearch_port` option all follow them, so nothing can be left dialling a stale 9300. `TEST_POSTGRES_PORT` / `TEST_ELASTICSEARCH_PORT` still pin a deterministic host port for CI, debugging, or attaching a client; each is independent, and setting one re-introduces the collision risk for that run by design. Isolation is unchanged - every run still gets its own containers, its own `test_*` database and its own index. `test_harness_isolation.py` is extended rather than relaxed: it now pins that the DEFAULT requests no host binding (the assertion that actually closes the race), that both overrides are still honoured when set, and that every consumer follows the port Docker really assigned. Port-map keys are normalised to `str` because testcontainers is unpinned and 4.14.x / 4.15.x disagree on the key type. Docs: `docs/troubleshooting.md`'s collision entry is rewritten as FIXED with the new allocation described, keeping only the parts that are still true (general resource contention on a shared box, which ephemeral ports do not address). `docs/lessons.md`'s "different ports (47000/9300)" aside is corrected - its point (testcontainers never touch the prod 5432/9200) is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN
WilfordGrimley
added a commit
that referenced
this pull request
Jul 29, 2026
`frontend/playwright.config.ts` hardcoded `http://localhost:3000` in both `use.baseURL` and `webServer.url`, so two concurrent frontend E2E runs on one box collided - the same bug class #571 just fixed for the Python test harness's Docker containers, with a worse failure mode. Off CI (`reuseExistingServer: !process.env.CI`) Playwright does not start a second dev server when something already answers on that URL, it REUSES it. So the second run silently tested the FIRST worktree's checkout, then lost the server outright when that run tore it down. Reproduced on unmodified master: two overlapping runs of the same four spec files, the second started 3s after the first - first passed 1/1, second failed 8 of 10, every failure pointing at application code rather than at the port. `resolvePort()` now asks the kernel for a free port (bind port 0, read the assignment back, release) unless `PLAYWRIGHT_PORT` is set, and exports it into the environment so Playwright's worker processes - which each re-load the config in their own process - inherit the same port rather than drawing their own. `baseURL`, `webServer.url` and the `webServer` command all derive from that one value; `playwright.perf.config.ts` inherits it for free by spreading the base config. `PLAYWRIGHT_PORT=3000` restores the old behaviour, including reuse of an already-running `npm run dev`. `next dev` is now given an explicit `--port`. That matters: Next only walks to the next free port when it chose the port itself; given one explicitly it exits with EADDRINUSE. So the one thing this fix does NOT close - the window between the kernel releasing the probe and Next binding, which spans `npm run dev` plus Next's boot - can only produce a loud abort at webServer startup, never a wrong-but-passing run. Unlike #571 there is no port-0 read-back on `webServer.url` to hold the binding across that gap; that residual, and the fact that two runs in the SAME directory still collide over `frontend/.next`, are written up in docs/troubleshooting.md rather than left to be rediscovered. The `favicon` MSW handler had to move with it. It is in `defaultHandlers`, so every E2E test loads it, and `@msw/playwright` runs it in the Playwright NODE process rather than in the page - it was fetching `http://localhost:3000/favicon.ico` out-of-band, which a per-run port breaks. It now reads the port back out of the environment, keeping the literal 3000 only as the fallback for a caller that never went through the Playwright config (jest), which is exactly the previous behaviour. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN
WilfordGrimley
added a commit
that referenced
this pull request
Jul 29, 2026
`frontend/playwright.config.ts` hardcoded `http://localhost:3000` in both `use.baseURL` and `webServer.url`, so two concurrent frontend E2E runs on one box collided - the same bug class #571 just fixed for the Python test harness's Docker containers, with a worse failure mode. Off CI (`reuseExistingServer: !process.env.CI`) Playwright does not start a second dev server when something already answers on that URL, it REUSES it. So the second run silently tested the FIRST worktree's checkout, then lost the server outright when that run tore it down. Reproduced on unmodified master: two overlapping runs of the same four spec files, the second started 3s after the first - first passed 1/1, second failed 8 of 10, every failure pointing at application code rather than at the port. `resolvePort()` now asks the kernel for a free port (bind port 0, read the assignment back, release) unless `PLAYWRIGHT_PORT` is set, and exports it into the environment so Playwright's worker processes - which each re-load the config in their own process - inherit the same port rather than drawing their own. `baseURL`, `webServer.url` and the `webServer` command all derive from that one value; `playwright.perf.config.ts` inherits it for free by spreading the base config. `PLAYWRIGHT_PORT=3000` restores the old behaviour, including reuse of an already-running `npm run dev`. `next dev` is now given an explicit `--port`. That matters: Next only walks to the next free port when it chose the port itself; given one explicitly it exits with EADDRINUSE. So the one thing this fix does NOT close - the window between the kernel releasing the probe and Next binding, which spans `npm run dev` plus Next's boot - can only produce a loud abort at webServer startup, never a wrong-but-passing run. Unlike #571 there is no port-0 read-back on `webServer.url` to hold the binding across that gap; that residual, and the fact that two runs in the SAME directory still collide over `frontend/.next`, are written up in docs/troubleshooting.md rather than left to be rediscovered. The `favicon` MSW handler had to move with it. It is in `defaultHandlers`, so every E2E test loads it, and `@msw/playwright` runs it in the Playwright NODE process rather than in the page - it was fetching `http://localhost:3000/favicon.ico` out-of-band, which a per-run port breaks. It now reads the port back out of the environment, keeping the literal 3000 only as the fallback for a caller that never went through the Playwright config (jest), which is exactly the previous behaviour. Claude-Session: https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Problem
MPCAutofill/cardpicker/tests/conftest.pybound its session-scoped testcontainers to fixed host ports — Postgres47000, Elasticsearch9300. A secondpytest cardpicker/on a shared box died at container start withBind for 0.0.0.0:9300 failed: port is already allocated, which surfaces as thousands of collectionERRORs that read as catastrophic breakage rather than a port clash. Three separate sessions lost real time to it in a single day (one reported "3036 ERRORS" before diagnosing it).PR #537 made those ports env-overridable (
TEST_POSTGRES_PORT/TEST_ELASTICSEARCH_PORT), but nothing assigned distinct values, so the default still collided. Overridability without allocation does not solve concurrency — it just moves the burden onto every session to hand-pick an offset it hopes nobody else has taken.Fix
Both containers are now started with no host binding at all — their testcontainers constructors already call
with_exposed_ports, so Docker assigns a free ephemeral host port per container and the fixtures read back what it actually assigned viaget_exposed_port(). Nothing wants a specific port, so nothing can collide: a closed race, not a narrowed window (a random-but-fixed constant would only be the latter).postgres_port/elasticsearch_portpublish the resolved values.django_db_modify_db_settings,ELASTICSEARCH_DSL,settings.ELASTICSEARCH_PORTand pytest-elasticsearch's ownelasticsearch_portoption all follow them, so nothing is left dialling a stale9300. The plugin threading moved frompytest_configure(which cannot know an ephemeral port) into the session-scopedelasticsearchfixture;pytest_configurestill handles the explicit-override case up front.TEST_POSTGRES_PORT/TEST_ELASTICSEARCH_PORTstill pin a deterministic port for CI, debugging, or attachingpsql/an ES client. Each is independent; setting one re-introduces collision risk for that run, by design.test_*database and its own index.Tests
test_harness_isolation.pyis extended, not relaxed. It now pins the thing that actually closes the race — that the DEFAULT requests no host binding — plus that both overrides are still honoured when set, and that every consumer follows the port Docker really assigned. Port-map keys are normalised tostrbecausetestcontainersis unpinned inrequirements.txtand 4.14.x (intkeys) / 4.15.x (strkeys) disagree.Verification
Two concurrent full-suite runs on one box, both green — the proof that matters for a concurrency bug:
3046 passed, 8 skipped in 303.37s(exit 0)33260, es332593046 passed, 8 skipped in 301.87s(exit 0)33261, es33258Started within a second of each other, and three unrelated agent
pytestsessions were already running on the same box (holding48511/9931,47037/9337,47010/9310from the old manual-offset workaround) — five concurrent suites, zero bind failures.Also:
test_harness_isolation.pyalone18 passed, 2 skipped(ephemeral default path) and16 passed, 4 skippedunderTEST_POSTGRES_PORT=47654 TEST_ELASTICSEARCH_PORT=9654(override path). All pre-commit hooks pass (ruff, isort, black, mypy, prettier).CI is unaffected:
test-backendsets neither variable and gets ephemeral ports; nothing in the workflow or the composite action depends on knowing the test-container port.Docs
docs/troubleshooting.md— the collision entry is rewritten as FIXED, documenting how ports are now allocated, instead of leaving a workaround for a bug that no longer exists. What is genuinely not fixed (general CPU/memory/connection contention on a shared box, which can still produceOperationalError) is kept and clearly separated from the port issue.docs/lessons.md— the "different ports (47000/9300)" aside is corrected; its actual point (testcontainers never touch the prod5432/9200) is unchanged.Other fixed-port assumptions found
Swept the harness for the same class of problem. Inside
MPCAutofill/,conftest.pywas the only place binding a constant host port, and no test asserts a specific port number. Two findings left alone, reported rather than fixed:frontend/playwright.config.tspinshttp://localhost:3000for bothbaseURLand itswebServer— same class of collision for concurrent frontend E2E runs, but out of scope here and not an unambiguous fix (the URL, the dev-server command and CI would all have to move together).pp_test_pgcontainer on55432is running on this box but originates outside this repo (no reference in the tree), so nothing here can address it.🤖 Generated with Claude Code
https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN