Stop pinning the Synapse container's address on the boxel Docker network - #6017
Conversation
The suite's Synapse container asked Docker for 172.20.0.5, while the three containers started ahead of it — smtp4dev, the mock OIDC upstream, and its Caddy proxy — took whatever Docker handed out. Docker allocates in join order, so Synapse's address was free only while exactly the expected number of containers held the lower ones. One extra tenant on the shared `boxel` network (a local Prometheus from the observability stack sits at .2 and restarts on its own) shifted every allocation up by one, the Caddy proxy landed on .5, and global setup died with "Address already in use" before any test ran. Nothing reaches Synapse by address: the host uses the published port, and containers on the network use its container name through Docker's embedded DNS, which is how Prometheus scrapes boxel-synapse:9001. Dropping the request makes both start paths identical, so the fixed-port and dynamic-host-port branches collapse into one set of flags. The network's subnet was pinned for the same reason and goes with it — requesting one fails outright when another network already holds it. Two adjacent repairs for the same reproduction: a run killed before its teardown left a Synapse container behind under an unpredictable name, still holding the host port, so `stopExisting` now sweeps them by name prefix; and a refused bind now names the host port and whatever publishes it, rather than reporting an address Docker declines to identify. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2ba0c4347
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The name prefix alone does not separate debris from a live tenant: the software-factory harness starts a Synapse under the same prefix and passes stopExisting: false so it can coexist with a dev Synapse. A dev Synapse launch takes the default stopExisting: true, so a prefix-wide sweep would force-remove a running factory test's homeserver over a port it never wanted. What distinguishes them is the port. Debris matters only while it holds the fixed port a launch is about to claim; a harness that published a dynamically chosen port is deliberately sharing the host. The sweep is now the intersection of the two filters, and runs only for a fixed-port launch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The new bind-conflict messaging path can misdiagnose failures when docker ps itself errors (currently indistinguishable from “no container publishes the port”), and should handle/report Docker query errors explicitly.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR removes the hard-coded Synapse container IP (and the fixed boxel Docker network subnet) from the Matrix Playwright harness, relying instead on published host ports and Docker DNS by container name to avoid address-allocation collisions on shared networks.
Changes:
- Refactors Synapse container startup to build a single set of
docker runflags viasynapseDockerParams, removing the fixed--ip=...behavior. - Updates network creation to avoid pinning a subnet, preventing “pool overlaps” failures when Docker already has a conflicting range allocated.
- Adds a targeted Playwright unit test file to lock in the “no fixed IP” invariant and basic flag/message behavior.
File summaries
| File | Description |
|---|---|
| packages/matrix/tests/synapse-container-networking.spec.ts | Adds tests covering Synapse docker flag construction (no --ip, port publish, volume mount, root behavior) and basic conflict-message content. |
| packages/matrix/support/synapse/index.ts | Removes fixed-IP behavior, centralizes docker param construction, adds abandoned-container cleanup, and improves bind-conflict error messaging. |
| packages/matrix/support/docker.ts | Makes dockerCreateNetwork avoid specifying a subnet and documents why subnet pinning is risky. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
dockerCapture collapsed both into an empty string, so a bind conflict raised while the daemon was unreachable would have been reported as a host process listening on the port — sending a reader after something that does not exist. It now returns undefined when the command could not be run, and the message distinguishes the three answers. Formatting moves into a pure function so all three read out deterministically under test; asserting only that the port appears let the branch taken depend on whatever happened to publish it on the machine running the suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Neither the name nor the port separates debris from a live tenant: a matrix suite mid-run holds the fixed port under the same name shape an abandoned container does, so a fixed-port sweep keyed on those would `docker rm -f` a running suite's homeserver whenever a dev stack started. That path is easy to reach — assert-synapse-running.sh looks for a container named boxel-synapse, which a running suite is not, so it starts one. The owning process is the signal that does separate them. Each Synapse container now carries its owner's pid, and only containers whose owner has exited are swept. Every unreadable answer — an owner that does not parse, a pid alive because it was reused — leaves the container alone, since declining to sweep costs a legible port-conflict message while sweeping a live run destroys it. The dev Synapse stays excluded by name: it outlives the process that starts it, and callers meaning to replace it stop it explicitly. Also: bound the docker queries, so an unresponsive daemon cannot stall startup or the failure path; take the container-name prefix from one constant rather than a second copy of the literal in mkdtemp; and drop SynapseConfig.host, which fed nothing but the removed --ip flag. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The harness kept its own sweep: every `sf-test-synapse-*` container on the host, removed by name with no liveness test. That is the shape the matrix suite just moved away from, and it has the same consequence — starting the factory support services destroyed a running Playwright suite's homeserver, and a concurrent harness's, over containers neither of them contended for. There is now one implementation of the rule rather than two definitions that can drift, so the harness inherits the owner-pid check: debris goes, live runs stay, and the dev Synapse remains excluded by name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
backspace
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] This pass went after convergence with the three earlier threads, and whether anything still reaches Synapse by address or reads the removed SynapseConfig.host. It also checked the two claims the new code rests on: that docker ps --filter publish= selects by the host-side port (moby's includeContainerInList compares PublicPort; confirmed against a local Docker 29 daemon), and that on all three start paths the process holding the owner-pid label outlives the container's use (the matrix suite's global setup, the software-factory serve:support child, the same-process harness API).
No blocking issues. Approving. Two non-blocking notes inline.
The Codex thread's reply describes a sweep keyed on the name prefix plus publish=<port>; head replaces that with the owner-pid sweep, which also covers the case that thread raised, so a one-line follow-up there would keep the thread from describing a mechanism the code no longer has.
processIsAliveassumes owner and sweeper share a pid namespace — pin the assumption in a comment or add a host label (see the comment onprocessIsAliveinpackages/matrix/support/synapse/index.ts).- Test title wording (see the comment in
packages/matrix/tests/synapse-container-networking.spec.ts).
Adjacent, out of scope: in environment mode synapseStart names the suite's container boxel-synapse-<slug>, so the sweep's sf-test-synapse- filter never reaches it. Same-slug leftovers are stopped by name in the stopExisting block; leftovers from other slugs accumulate on dynamic ports. Pre-existing, and unaffected by this change.
| .map(([id]) => id); | ||
| } | ||
|
|
||
| function processIsAlive(pid: number): boolean { |
There was a problem hiding this comment.
[Claude Code 🤖] process.kill(pid, 0) answers for the sweeper's own pid namespace, so the "every unreadable answer spares the container" rule has one gap: an owner that started the container from a different namespace against the same daemon — a harness inside a devcontainer on the host's Docker socket, swept by mise run start-synapse on the host, or the reverse. Its pid number is meaningless there and usually unused, which reads as "owner exited" and removes a live run.
If that setup is out of scope, a sentence here saying the sweep assumes owner and sweeper share a pid namespace pins the assumption. If it is in scope, a second label carrying os.hostname() lets a mismatched host count as another unreadable answer:
'--label',
`${SYNAPSE_OWNER_HOST_LABEL}=${os.hostname()}`,with abandonedContainerIds sparing any line whose host is not its own.
Non-blocking; a decision rather than a fix.
There was a problem hiding this comment.
[Claude Code 🤖] Closed rather than documented, in d3cc9b2 — it is the same rule I stated, applied to a case I had not considered, so leaving it as a written assumption would have left the one outcome this whole change exists to prevent.
Containers now carry boxel.synapse-owner-host alongside the pid, the query filters and formats both, and abandonedContainerIds takes the sweeper's own host and spares any line that does not match. A foreign host is simply one more answer the sweep cannot read, joining an unparseable owner and a reused pid.
Verified against real containers, including one labelled for another host:
[live run spared] true
[debris swept] true
[foreign host spared] true
[unlabelled spared] true
The last line is the transitional case: a container from before the labels exists with neither, so the query's label= filters never list it and it is left alone.
| expect(params).toContain('boxel.synapse-owner-pid=4242'); | ||
| }); | ||
|
|
||
| test('the sweep asks only about this harness own containers', () => { |
There was a problem hiding this comment.
[Claude Code 🤖] Nit, non-blocking: "this harness own" is missing its possessive. Rephrasing sidesteps the apostrophe-in-single-quotes formatting question:
| test('the sweep asks only about this harness own containers', () => { | |
| test('the sweep asks only about containers this harness started', () => { |
There was a problem hiding this comment.
[Claude Code 🤖] Taken as suggested in d3cc9b2. The possessive went missing when I stripped an apostrophe out of a single-quoted test name; your rephrasing reads better than what I was trying to write anyway.
A pid is only answerable from the machine that issued it. Two processes in different pid namespaces against the same Docker daemon — a harness inside a devcontainer on the host's socket, or the reverse — number their processes independently, so asking about a foreign pid locally answers for an unrelated process, or for none at all. The latter reads as "owner exited" and takes a live run with it. Containers now carry the owner's host with its pid, and a container labelled with another host is one more answer this sweep cannot read, joining an unparseable owner and a reused pid in being left alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The matrix Playwright suite runs four containers on a shared Docker network called
boxel: smtp4dev, the mock OIDC upstream, a Caddy proxy in front of it, and Synapse. Global setup starts them in that order, and only Synapse asked for a specific address on the network —172.20.0.5. The other three took whatever Docker handed out.Docker allocates addresses on a user-defined network in the order containers join it. On an otherwise empty network that works out exactly: smtp
.2, upstream.3, proxy.4, and.5is still free when Synapse asks for it. One more tenant anywhere on that network shifts every allocation up by one, the Caddy proxy takes.5, and Synapse cannot start:That reads like a host port conflict. It is the container's address, and the suite dies before a single test runs.
The observability stack is what makes this routine locally — its Prometheus joins
boxelto scrape Synapse's metrics, sits at172.20.0.2, and restarts on its own, so a reboot does not clear it. The same mechanism hitsmise run start-synapse, which takes the same fixed-address path as the suite.The shape of the fix
Nothing reaches Synapse by address. The host reaches it through the published port (
localhost:8008); containers on the same network reach it by container name through Docker's embedded DNS, which is exactly how the Prometheus config above names its scrape target —boxel-synapse:9001. The pinned address bought nothing and cost the suite its independence from whatever else was on the network, so the request is gone.With it goes the last difference between the two start paths. The fixed-port path and the dynamic-host-port path (used where several harnesses share a host) now build identical flags — publish the port, join the network, let Docker allocate — so the branch collapses into one
synapseDockerParamsfunction.The network's
--subnet=172.20.0.0/16was pinned to make172.20.0.5meaningful and goes with it. Requesting a subnet is not free: Docker refuses to create a network whose range another network already holds (Pool overlaps with other one on this address space), and aboxelnetwork created any other way —docker network create boxel, whichpackages/observability/docker-compose.ymltells you to run when bringing that stack up first — gets a different range, in which the pinned address is not merely taken but invalid.Two adjacent repairs
Both are part of the same reported reproduction.
An abandoned container no longer poisons the next run. Synapse containers are named after the temp config directory they are given, so a run killed before its teardown leaves one behind under a name no later run can predict — still holding the host port the next run needs.
Nothing about the container itself identifies it as debris. An abandoned Synapse is running and healthy, on the same port and under the same name shape as one whose suite is mid-run — and both the software-factory harness and the Playwright suite start containers under that shape. Neither name nor port can tell them apart.
The owning process can. Each container carries its owner's pid and the host that issued it, and only those whose owner has exited are swept. The host travels with the pid because a pid is only answerable from the machine that issued it — two processes in different namespaces against the same Docker daemon number theirs independently, so a foreign pid read locally names an unrelated process or none. Every unreadable answer leaves the container alone — an owner that does not parse, or a pid that is alive only because it was reused — because declining to sweep costs a legible port-conflict message on the next start, while sweeping a live run destroys it. The dev Synapse is excluded by name: it outlives the process that starts it, and callers meaning to replace it stop it explicitly.
The realm test harness kept a second sweep of its own — every
sf-test-synapse-*container, removed by name with no liveness test — so starting the factory support services destroyed a running Playwright suite's homeserver, and a concurrent harness's, over containers neither contended for. It now calls the same function rather than carrying a second definition that can drift from this one.A refused bind now names what it collided with. Docker's message declines to say which address is in use. A bind failure is now reported as the host port and whatever publishes it:
The three answers stay distinct: containers holding it, nobody holding it (so a host process is), and Docker not answerable at all — a failed query is never reported as a host process.
Verification
Reproduced and re-checked against the real containers with Prometheus holding
172.20.0.2, running the container-startup half of global setup:RESULT: FAILED — ... failed to set up container networking: Address already in use172.20.0.3, passes its health check, and serveshttp://localhost:8008And the three behaviors the change rests on, checked directly:
boxelresolves the Synapse container by name and getsOKfrom/healthon port 8008 — the path Prometheus uses.Host port 8008 is already published by: sf-test-synapse-… (matrixdotorg/synapse:v1.126.0)— the loud failure that path has always had, now naming what it collided with.packages/matrix/tests/synapse-container-networking.spec.tscovers the flag construction directly, so a reintroduced address pin fails a test rather than waiting for a machine that happens to have a neighbour on the network.pnpm lintand the shard-assignment test pass.Scope
The ticket asks whether this is also behind the Synapse container flakes seen in CI shards. It does not appear to be: no workflow puts anything else on the
boxelnetwork, so a fresh runner allocates the addresses in exactly the order the pinned one assumed. This removes the dependency regardless, but it should not be expected to move those shards.