fix(agents): make SSH-port allocation atomic so concurrent creates cannot collide (#2215)

[trinity-ability]

Fixes #2215

What changed and why

get_next_available_port() was an unlocked check-then-act. It read existing agent SSH ports from container labels, filtered them through is_port_available() — which binds inside the backend container's own network namespace, so host-published agent ports are invisible to it in production — and returned. The caller then ran containers.run up to ~100 lines and several seconds later.

At first boot with --workers 2 the two first-run seeders hold different SETNX locks (cornelius:provision / system_seed:provision), so worker A's Cornelius create (slow anonymous GitHub clone) runs concurrently with worker B's default-system deploy. Both compute the same max(existing_ports) + 1; the second containers.run fails Bind for 0.0.0.0:2227 failed: port is already allocated. The ent#313 reclaim removes the leaked container, but the agent is never created — and the system-seed flag policy then permanently latches default_system_seeded on the resulting partial, freezing the default fleet incomplete.

Four changes, in the order they defend:

D1 — atomic allocation (services/docker_service.py). A per-port Redis reservation, SET port_alloc:{port} "1" NX EX 600, is the last gate — after the label scan and after is_port_available, so reservations never leak onto candidates the scan walks past. SETNX contention means a concurrent allocator holds that candidate, so the scan continues; only a raised Redis error fails open (warn, return the candidate unreserved, stop attempting reservations). No release and no refresh: once the container exists its trinity.ssh-port label is the durable truth (Invariant #11), so the reservation is transient state bridging check→run, never a port registry — which also means there is no lease anything could release, so the #1919 compare-and-delete bar is N/A by construction. The label scan is now strict (_existing_agent_ports_strict): a Docker listing fault raises instead of degrading to list_all_agents_fast's [], which would compute start_port = 2222 and confidently reserve an existing agent's port. exclude= is merged once, before both scan loops, so it binds the forward scan and the 2222–2500 fallback.

D2 — bounded bind-conflict retry (services/agent_service/crud.py). D1 is fail-open, so a collision can still surface at containers.run — Redis down or restarted, an expired reservation, a foreign host process. _run_agent_container_with_port_retry wraps _create_agent_container for ≤3 attempts. On a bind-classified failure, in strict order: record the failed port → clean up the leaked Created husk (on every bind-classified attempt, the final one included) → if attempts remain, get_next_available_port(exclude=attempted_ports) and retry. Any cleanup doubt — ownership row present, lookup failure, unprovable provenance, removal failure — aborts and re-raises the original bind error, never the cleanup error: proceeding after a failed removal makes attempt N+1 409 against our own husk, which the reclaim then reads as "not ours" and strands. Gated on the port having been auto-allocated, captured before the mutation, so a caller-pinned port never silently moves.

D2b — reclaim classifier precedence (crud.py, 3 additive lines). See "Flagged deviations" below.

D3/D4 — pass-level seed lock + Cornelius alert (services/system_seed_service.py). One first_run_seed:provision lock (SETNX, uuid token, TTL 900s, compare-and-delete release in a finally, fail-open) over the whole first-run pass; the loser skips the entire pass without touching any flag. Ports were never the only cross-seeder hazard — SQLite BUSY under concurrent boot writes is anticipated in this very file, the 60s Docker read timeout is in ent#313's own docstring, and any transient during the burst produces the same permanently-latched partial. AC#3's genuine gap was that a failed Cornelius create was log-only; the orchestrator now captures the result and emits system-seed-cornelius-failed on create_failed/create_blocked and from the belt-except, credential-sanitized and URL-userinfo-redacted at the exit point.

Recreate-gap pre-reservation (services/agent_service/lifecycle.py, 2 lines). recreate_container_with_updated_config reuses the labeled port across its remove→create gap, during which the port is invisible to the allocator and unreserved — and the most-recreated agent tends to hold the fleet's max port, exactly what a concurrent allocation computes as max+1. reserve_port_for_recreate(port) (SET without NX — it is that agent's own port — fail-open) re-asserts it before the old container's removal.

No schema change, no migration on either track, no new env var, no compose change, no new endpoint or MCP tool.

AC#1 guarantee boundary — read this before building on it

With Redis up, two concurrent creations structurally cannot be assigned the same port: the SETNX is the atomic arbiter.

With Redis down, a same-port assignment is convergent-under-retry, not prevented: it is detected at bind time and resolved within the same create call by D2. Every lock in this codebase deliberately fails open and they share one client, so D1, both inner seeder locks and the D3 pass lock all degrade together — D2 alone stands in that mode.

This boundary is in the allocator's docstring verbatim so nobody later reads "atomic" as unconditional and deletes D2 as redundant.

Flagged deviations — please do not re-litigate these blind

1. The D2b guard edits ent#313 reclaim code. The plan's dossier said "reclaim untouched"; this is a deliberate, pre-authorized 3-line deviation (plan §8 audit row 10, demanded independently by both /autoplan voices at HIGH). _is_container_name_conflict's text fallback matches the bare substring "already in use" — and Docker's userland-proxy bind failure, "… listen tcp 0.0.0.0:X: bind: address already in use" (the standard phrasing on Docker Desktop, this repo's primary dev platform), contains it. So a bind failure reaching _reclaim_failed_creation_container with container=None was misclassified as a name conflict, the reclaim declined, the Created husk squatted the name, every retry 409'd — and for Cornelius _agent_is_present counts the husk, so the next boot 409-converges the durable cornelius_seeded flag onto a dead container: the exact unrecoverable latch Cornelius seeder marks itself seeded on a volume-conflict 409 — Cornelius never created, never retried #1790 exists to prevent. A live pre-existing bug, squarely in this issue's blast radius. The change is a condition amend only: if _is_container_name_conflict(exc) and not _is_port_bind_conflict(exc). All three fail-closed gates (409, ownership row, floor-ts provenance) are preserved byte-for-byte, and _rollback_failed_creation is untouched. Tightening _is_container_name_conflict's substring instead was considered and rejected as riskier and non-additive.

2. Port allocation MOVED inside the docker try/rollback fence during review. The plan had the allocation staying where it was (pre-try). It could not: D1's allocator now fails loud on a Docker listing fault, and a raise before that try strands the agent_git_config working-branch reservation _resolve_template already wrote — after which every later create of that name fails agent_git_config already exists, permanently, Cornelius's next-boot retry included. So the allocation sits immediately before containers.run, inside the fence. Second benefit: the 600s reservation TTL no longer has to cover config staging, the MCP-key mint, env build, the ent#15 snapshot prepopulate and the volume builds. Verified that nothing between the old and new call sites reads config.port — it is consumed only inside _create_agent_container (label + ports map, rebuilt per call), and there is no port column anywhere in agent_ownership.

3. Two defensive shapes that look like over-caution and are not. (a) _is_port_bind_conflict wraps str(exc) in its own try — docker.errors.APIError.__str__ dereferences response attributes and can itself raise on a partial or stubbed response object, and this classifier runs inside the never-raises reclaim, where a raise would replace the creation error being reported (the ent#313 isinstance regression, one door over). (b) Cleanup uses a duck-typed docker_client.containers.get rather than the named get_agent_container helper: that helper flattens a lookup failure into "absent", and here absent means "safe to retry" while failure must abort.

Verification

Empirically proven on real Docker (dev stack, port-squat harness):

• Squatted 2233 → the agent landed on 2234, with label == API == published bind agreeing, and /health healthy.
• Squatting 2235/2236/2237 exercised the retry ladder: attempts 2/3, then 3/3, then a clean 500 with no 4th attempt — and the husk was removed on every attempt, the last one included.
• port_alloc:* keys carry TTL ≤ 600 and decay as expected.
• The config-drift recreate held port 2232 across remove→create, proven by its reservation TTL rising 309 → 590, which only reserve_port_for_recreate's no-NX SET … EX 600 can do.

WAVE-4 FULL /verify-local: branch PASS. The harness result.json reported fail at agent-exercise, root-caused across 4 reproductions including a fresh VM to a Docker Desktop 4.36.0 / Engine 27.3.1 bug: docker network connect against a running container with an ephemeral HostPort: "0" silently drops the host binding (gvisor write unixgram: destination address required). The app was healthy in-container throughout; the equivalent checks were driven manually and passed. Not branch-related.

Honest gaps — two claims rest on unit coverage, not end-to-end:

• The agent_git_config stranding scenario (deviation 2) and "exactly one bug: the fleet-wide .gitignore merge never runs at agent creation — in-container auto-sync commits .trinity/ runtime state before any Push can migrate #2069 gitignore-merge task fires against the final container" were not driven end-to-end, because the dev .env GITHUB_PAT is expired — template metadata reads 401, so github: creates are refused before reaching either path.
• D2b's "address already in use" branch is unit-only: this daemon emits "port is already allocated". The "address already in use" phrasing is covered by the classifier disjointness pin, not by a live daemon.

Tests (-p no:randomly, affected-file neighbourhood):

tests/unit/test_2215_port_allocation.py
tests/unit/test_2215_create_port_retry.py
tests/unit/test_ent124_default_system_seed.py
tests/unit/test_ent313_failed_creation_container_reclaim.py
tests/unit/test_1484_create_agent_characterization.py
tests/unit/test_agent_readiness_probe.py
→ 155 passed

tests/lint_sys_modules.py → exit 0, no new violations (both new files use monkeypatch.setitem).

origin/dev merged in (2 conflicts, both resolved as a union)

dev moved 5 commits after this branch was cut, so origin/dev is merged in rather than left conflicting. Neither side's behaviour changed:

• docker_service.py — the typing import line only. bug: Workspace roster lists agents with no container (DB-sourced roster vs Docker-as-truth) #2196 added Dict (its tri-state readers), this branch added Set (the allocator's port set); the merged line carries both. Everything else auto-merged — bug: Workspace roster lists agents with no container (DB-sourced roster vs Docker-as-truth) #2196's agent_container_states / agent_container_state sit above _existing_agent_ports_strict / _try_reserve_port / reserve_port_for_recreate with no overlap (verified by AST: no duplicate top-level definitions in either merged file).
• architecture.md — both sides appended a clause to the same docker_service.py catalog bullet. bug: Workspace roster lists agents with no container (DB-sourced roster vs Docker-as-truth) #2196's tri-state clause is kept first (it landed first), then bug: fresh-install seeding races itself across workers — two seeders allocate the same agent SSH port #2215's, plus one added sentence naming why the two resolve a Docker fault in opposite directions: a roster read must degrade to unchanged, an allocator must never degrade to allocate over the fleet. Worth a reviewer's eye — it is the one place the two changes make a claim about each other.

Post-merge re-run of the same six files: 155 passed. The now-shared docker_service.py neighbours (test_2196_roster_availability.py, test_docker_service_list_fast.py, test_1560_agent_redis_key_parity.py, test_1560_breaker_cleared_on_lifecycle.py, test_ent107_cornelius_seed.py): 77 passed.

Pre-existing reds on dev — not from this PR

The full unit suite under -p no:randomly is 8 failed / 10466 passed, and all 8 reproduce on origin/dev itself:

• 5 × tests/unit/test_ent96_timeline_split.py — hardcoded 2026-08-14 bucket keys, now in the past.
• 3 × tests/unit/test_pat_propagation_properties.py::TestKnownGaps — stale strict-xfail; bug(github-pat): a backslash in the token raises re.error mid-rotation; .env quote-escaping is write-only #2017 fixed the bug the xfails describe, so they now XPASS and fail strictly.

pytest-randomly is installed in this venv, so always compare with -p no:randomly.

backend-unit-test should still go green on this PR, and that is not the check being lax: it is a base-vs-head differential (the suite under three seeds on both the base tip and the head merge commit, failing only on a test ID that is newly failing on head). All 8 fail identically on both sides, so they diff away. Worth knowing while reading dev's own history: on a push those diff jobs are skipped by design (if: github.event_name != 'push'), so a green backend-unit-test on a dev push means the invariant lint ran — not that the suite did.

Follow-ups (listed, deliberately not filed)

1. Dev-tip test rot — the 8 failures above: ent#96's hardcoded dates need to be relative, and bug(github-pat): a backslash in the token raises re.error mid-rotation; .env quote-escaping is write-only #2017's TestKnownGaps strict-xfails need retiring.
2. A failed create strands agent-<name>-workspace. The volume outlives the reclaimed container, so an immediate same-name retry 409s until the bug: Docker volumes are never deleted — volume_remove has zero callers; agent purge leaks workspace/public/shared volumes forever #1581 orphan-volume sweep reclaims it (~15 min, 3 unattached strikes). This partially blunts the "Cornelius retries on next boot" benefit when the failure lands late in creation.
3. The two inner seeder locks remain pre-bug: fleet-restart lock — lease refresh isn't ownership-checked, and acquisition sits outside the try/finally (#1912 review follow-up) #1919 blind-delete. Only the new pass lock got token + compare-and-delete hygiene; cornelius:provision and system_seed:provision still DELETE unconditionally, so a TTL lapse can release a sibling's live lease. Out of scope here, kept as belts.
4. _release_pass_lock's GET→DEL is non-atomic. Correct compare-and-delete needs a Lua CAS (the redis_breaker_util script-cache pattern); the current form narrows the window rather than closing it.
5. The Docker Desktop agent-exercise blocker makes /verify-local unable to complete its final phase on macOS Desktop until 4.36.0/27.3.1 is worked around or upgraded past.

Review pipeline

/autoplan (strategy + engineering voices, both approve-with-changes) + gemini-critic (approve) on the plan; /review on the branch (deviation 2 originated there); WAVE-4 FULL /verify-local; /validate-pr pre-push. No new endpoints, WS channels, MCP tools, auth-boundary changes, or credential paths — /cso --diff skipped per the pipeline table for a non-auth bug fix.

🤖 Generated with Claude Code

[vybe]

Validated per /validate-pr. Approved with the deviations read, not waved through.

The defect and the defence-in-depth

The root cause is stated correctly: get_next_available_port() was an unlocked check-then-act whose only real filter — is_port_available — binds inside the backend container's own netns, so host-published agent ports are invisible to it in production. The two first-run seeders hold different SETNX locks, so they were never serialised against each other, and the resulting partial then permanently latches default_system_seeded. Ports were the symptom; the missing pass-level lock was the disease, and D3 is the right place to fix it.

The guarantee boundary is stated honestly and belongs in the docstring. With Redis up the SETNX is the atomic arbiter; with Redis down a same-port assignment is convergent-under-retry, not prevented. Since every lock here fails open on one shared client, D1/D3 and both inner locks degrade together and D2 alone stands in that mode — which is exactly why D2 must not later be deleted as redundant. Good that this is in the source and not only the PR body.

Reviewed in detail

• D1 ordering is right: the reservation is the last gate, after the label scan and is_port_available, so a host-bound port never holds a 600s reservation. exclude merged once before both loops binds the 2222–2500 fallback too. SETNX contention → keep scanning; only a raised Redis error fails open.
• _existing_agent_ports_strict raising instead of reusing list_all_agents_fast is the correct inversion: a [] degrade computes start_port = 2222 and confidently reserves over the live fleet, which no bounded retry can outrun. Note this makes recreate_missing_container and the system-agent bootstrap fail loud too — intended, and consistent with #2196's stated rule that a roster read degrades to unchanged while an allocator must never degrade to allocate over the fleet.
• _cleanup_bind_failed_container fails closed on every doubt (ownership row, lookup failure, unprovable provenance, removal failure) and re-raises the original bind error, not the cleanup error — correct, since proceeding after a failed removal makes attempt N+1 409 against our own husk which the reclaim then reads as "not ours". The duck-typed containers.get over get_agent_container is justified: that helper flattens failure into absent, and here absent means safe-to-retry.
• Husk cleanup on the final attempt too — the detail that keeps a bind husk from reaching the generic ent#313 reclaim still on the name.
• auto_allocated_port captured before the mutation — captured after, the gate is always-False and the whole retry ships dead. Worth the comment it got.
• D3 release is compare-and-delete with a unique token per #1919, in a finally, fail-open on acquire. _notify_cornelius_failure sanitizes + redacts URL userinfo at the exit point, which is right for a durable UI-rendered surface fed by str(exc) from a PAT-resolving github: create.

Flagged deviations — reviewed, both accepted

1. D2b touching ent#313 reclaim. Accepted. _is_container_name_conflict's text fallback matches the bare substring "already in use", which Docker Desktop's bind failure contains — so a bind failure with container=None was misclassified, the reclaim declined, and for Cornelius _agent_is_present counts the husk, 409-converging cornelius_seeded onto a dead container. That is a live pre-existing bug squarely in this issue's blast radius, and the change is a condition amend only: all three fail-closed gates (409, ownership row, floor-ts provenance) are byte-identical and _rollback_failed_creation is untouched. Tightening the substring instead would have been riskier and non-additive — agreed.
2. Allocation moved inside the docker try/rollback fence. Accepted, and it is a correction rather than a liberty: with D1 now raising, a pre-try raise strands the agent_git_config working-branch reservation _resolve_template already wrote, after which every later create of that name fails permanently — Cornelius's own next-boot retry included. Verified the claim that nothing between the old and new call sites reads config.port.
3. The two defensive shapes (str(exc) in its own try; duck-typed lookup) are not over-caution — the first sits inside a never-raises reclaim where a raise would replace the error being reported.

Verification

All 24 non-skipped checks green, including all six pytest legs and regression diff. The seed-99999 pair that initially showed red were cancelled at an identical timestamp, not failed; I re-ran them and both pass. prod-image-smoke, verify-non-root, gitleaks, both CodeQL analyses green.

Scan: the one ghp_-shaped string is ghp_ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789ab, a synthetic fixture in the test that asserts it is redacted — not a finding. No host paths, no mode/symlink changes, no new env var, no new top-level src/backend/ package, no schema change (so schema-parity/Alembic correctly N/A). Merges clean into current dev after #2234/#2237/#2233.

Honest gaps, noted not blocking

The agent_git_config stranding scenario and the "exactly one #2069 gitignore-merge task" claim rest on unit coverage because the dev .env GITHUB_PAT is expired; D2b's "address already in use" branch is unit-only on this daemon. Both are disclosed rather than papered over, and the empirical port-squat harness results (2233 → 2234, the 2/3 → 3/3 retry ladder with no 4th attempt, husk removed every attempt, recreate holding 2232 with TTL 309 → 590) cover the load-bearing paths on real Docker.