Make model serving ports collision-safe - #243
Conversation
juhovainio
left a comment
juhovainio
left a comment
There was a problem hiding this comment.
I reviewed this PR and found four issues worth fixing before merge, left as inline comments below.
The backend work — port locking, the allocation lock, recovery rollback — is solid and well tested; nothing to flag there. The problems are in the new Dash wizard UI plus one test-coverage gap:
- Dash's notion of a "loopback" host doesn't match the CLI's, so the wizard can validate and approve a launch that the CLI then refuses.
- Pressing Esc while editing the Host or Port field wipes the entire form instead of just canceling that one edit.
- Nothing exercises the actual auto-port-selection behavior through the real
rocm servecommand — the existing e2e "port" scenarios all bypass it with a mock server. - One leftover unreachable
bail!in the new port-resolution module.
Not blocking, but worth a look: the loopback-host list and a couple of user-facing strings are independently duplicated between the CLI and TUI crates, which is exactly how the first issue above happened — sharing that definition would prevent the next drift. The PR also bundles an unrelated test-flakiness fix in with the port-safety work; consider splitting it out.
Ran a full cargo check/cargo clippy --all-targets -- -D warnings across the workspace and they're clean, and the concurrency tests for the new allocation lock genuinely exercise the race with real threads rather than mocks — good approach there.
|
|
||
| /// Whether `host` is one of the loopback spellings Dash may bind. | ||
| #[must_use] | ||
| pub fn is_loopback_host(host: &str) -> bool { |
There was a problem hiding this comment.
is_loopback_host here does not match the CLI's actual gate, is_public_bind_host in crates/rocm-engine-protocol/src/lib.rs. This version compares case-insensitively and accepts "[::1]"; the CLI's check is case-sensitive and does not special-case "[::1]" (its own test asserts "LOCALHOST" and "[::1]" are treated as public, deliberately). So a host like LocalHost or [::1] passes validation here and Dash approves the launch, but the CLI then rejects it at spawn time since it never received --allow-public-bind. Worth making this byte-for-byte identical to the CLI's check (or sharing one definition) rather than a hand-rolled duplicate.
There was a problem hiding this comment.
Fixed in c526c33. Dash now uses the exact case-sensitive host set accepted by the CLI, with regression coverage for LocalHost and [::1].
| /// Whether the inline `Advanced settings` rows are showing. | ||
| pub advanced_expanded: bool, | ||
| /// Inline text editor; `Some` while editing Host or the custom port. | ||
| pub editor: Option<InlineEditor>, |
There was a problem hiding this comment.
This new editor field isn't wired into active_overlay_at_root() in crates/rocm-dash-tui/src/app/mod.rs (not touched by this PR, so I couldn't comment on it directly — the check is around lines 965-971 there). That function only looks at browser/picker/approval/active_job to decide whether Esc should close the whole wizard vs. defer to a sub-screen. With editor unlisted, opening Advanced, starting to edit Host or Port, and hitting Esc gets misread as "nothing open" and close_overlays() wipes the entire in-progress form (including the typed model name) instead of just canceling the field edit that editor_key already handles correctly. Suggest adding && w.editor.is_none() to the serve_wizard arm there, matching the existing pattern for install_manager.active_job.
There was a problem hiding this comment.
Fixed in c526c33. active_overlay_at_root() now includes w.editor.is_none(), and the regression test proves Escape reaches the editor without discarding the in-progress wizard.
| /// is exhausted, or when the OS reports anything other than "address in use" — | ||
| /// a permission or address-availability error must never be papered over by | ||
| /// quietly choosing a different port. | ||
| pub(crate) fn resolve_serve_port( |
There was a problem hiding this comment.
I couldn't find any e2e/Gherkin scenario that exercises this function's actual behavior through the real rocm serve CLI. The port-related scenarios in tests/e2e-cucumber/features/model_serving.feature ("served on the default port" / "on a non-default port") are both backed by a MockServer started directly in the step definitions, not by invoking rocm serve — so nothing proves auto-selection actually skips a reserved port, exhausts the range correctly, or refuses automatic selection on a custom host. Given this is new user-observable CLI behavior, it seems worth a scenario that starts one service on the default port and asserts a second rocm serve lands on the next one.
There was a problem hiding this comment.
Fixed in c526c33. Added @id:serve-auto-port-skips-occupied: it holds 11435 with a real listener, invokes the real rocm serve binary without --port, and asserts endpoint 11436. It runs on the merge-queue GPU lane.
| // — and it is refused by returning an error, never by panicking. | ||
| let Some(port) = request.explicit() else { | ||
| validate_port_request(host, request)?; | ||
| bail!( |
There was a problem hiding this comment.
This bail! looks unreachable. It only runs when request.explicit() is None, i.e. request == PortRequest::Auto, and we're already inside if !is_canonical_loopback_host(host). But validate_port_request on the line above bails whenever request.is_auto() && !is_canonical_loopback_host(host) — exactly the condition guaranteed true here — so the ? on that call always short-circuits before this bail! can execute. Harmless (near-identical message either way), but dead code; maybe replace with unreachable!() or restructure to drop the duplicate message.
There was a problem hiding this comment.
Fixed in c526c33. The duplicate bail! is gone; the structurally unreachable fallthrough is now explicit after validate_port_request.
Signed-off-by: Michael Roy <michael.roy@amd.com>
Signed-off-by: Michael Roy <michael.roy@amd.com>
513e98d to
c526c33
Compare
|
I had a parallel fix for this defect in #301 — I missed this PR when planning and duplicated it. #301 is now closed as superseded; this change is the better one, particularly holding a real lease across the launch and serialising allocation behind a lock rather than probing and releasing. Two gaps I hit in my own version that look like they apply here too. Both are small; neither is a blocker. 1. An explicit
if let Some(existing) = existing_live_managed_service_in(&live, engine, canonical_model_id) {
return Ok(ManagedPortDecision::AlreadyLive(Box::new(existing.clone())));
}The It reads like the same class as the recipe-mismatch guard immediately below it, so the fix probably belongs in the same place: if let Some(port) = request.explicit()
&& port != existing.port
{
bail!(
"managed service `{}` is already serving {} on port {}, so port {port} would not be used; \
stop it with `rocm services stop {}`, or drop --port to reuse it",
existing.service_id, canonical_model_id, existing.port, existing.service_id
);
}A reviewer caught this in my branch; I would not have found it by reading the happy path. 2. The explicit-busy-port refusal has no scenario.
One tagging note if you do add it: |
rominf
left a comment
rominf
left a comment
There was a problem hiding this comment.
I read through this at c526c33 — after the round of fixes you pushed for juhovainio's earlier comments, which all look correctly addressed. The port-allocation core is sound: the bind probe holds a real TcpListener lease that is only released immediately before the spawn, and the whole sequence sits inside lock_service_allocation, so the classic bind-close-hand-off window really is closed on the rocm serve path. I also tried to break the probe with SO_REUSEADDR (a wildcard 0.0.0.0:P listener making the 127.0.0.1:P probe look free) and couldn't — on Linux the probe still gets EADDRINUSE, so that concern doesn't apply. I didn't verify the Windows behaviour there.
What I did find is one launch path left outside the new lock, plus a handful of smaller things. Details inline.
The one I'd most want resolved before merge isn't in the diff at all, though: #267 introduces a second cross-process lock over the same critical sections. It adds AppPaths::managed_launch_lock_path() -> services_dir()/launch.lock, acquired in serve() and threaded into start_managed_service / run_attached_service as a new parameter. Neither lock file exists on main today, so this isn't a pre-existing overlap — both PRs are inventing one. If both land, one resource ends up guarded by two different lock files with divergent policies: yours is bounded at 5s with an actionable error, #267's FileLock::acquire calls file.lock() and blocks indefinitely. Two locks that don't mutually exclude are worse than either alone — this PR's "the lock guarantees no concurrent allocation" invariant stops being true the moment #267's path also mutates records. They also both touch apps/rocm/src/main.rs, crates/rocm-core/src/lib.rs, model_serving.feature, and serving_steps.rs, and both change overlapping function signatures. #267 is currently MERGEABLE and this one isn't, so it will likely land first by default rather than by decision. Worth agreeing on one lock file and one timeout policy (I'd argue for the bounded one with the actionable message) and rebasing the loser on top.
PR title. Make model serving ports collision-safe doesn't follow Conventional Commits, which CONTRIBUTING.md:83 requires for PR titles specifically — and since the repo squash-merges, the title becomes the commit subject verbatim (recent main: fix(release): ... (#330), test(e2e): ... (#322)). Something like fix(serve): make model serving ports collision-safe would match. Minor, but easy to miss until merge time.
Merge state. CONFLICTING/DIRTY — the base is 38 commits behind main and git merge-tree conflicts in tests/e2e-cucumber/tests/e2e/serving_steps.rs. Since #267 touches that same file, the rebase order matters here too.
CI. The last run was 2026-08-18 at this head. E2E tests (Strix Halo, Windows) is red: 40 scenarios, 4 failed, 3 expected xfail, 1 unexpected — serve-hf-checkpoint-inference regressed. That's a real serve scenario on a PR that changes how the serve port is chosen, so I don't think it can be waved through as unrelated without a look at the log. Coverage (rocm-dash crates, ratcheted) was cancelled. Both will need to be green after the rebase anyway.
| /// `rocm serve` launch: a CLI automatic selection and a recovery re-bind can | ||
| /// never interleave, and the CLI sees this record's `recovering` reservation. | ||
| fn restart_managed_service(paths: &AppPaths, record: &mut ManagedServiceRecord) -> Result<()> { | ||
| let _allocation = rocm_core::lock_service_allocation(paths)?; |
There was a problem hiding this comment.
rocm services restart respawns outside the new allocation lock.
This is the daemon-side restart and it correctly takes lock_service_allocation + lease_loopback_port (neither of which existed on main — this PR adds them here). The CLI-side twin at apps/rocm/src/main.rs:14045 (restart_internal_managed_service) did not get the same treatment, which is why this reads as an oversight rather than a scoping decision.
That function is reachable from rocm services restart <id> --yes via run_approved_service_action -> SandboxToolArg::RestartServer (main.rs:13876). It:
- calls
stop_internal_managed_service, which flips the record to stopped — andlive_managed_services(main.rs:5415-5421) filters onmanaged_service_is_live, soreserved_service_portsimmediately stops reserving that port; - rebuilds the serve args on the bare
record.port(main.rs:14085) with nolock_service_allocation, nolease_loopback_portprobe, and no re-check that the port is still free; - spawns.
A concurrent rocm serve auto-scan can claim the freed port inside that stop->spawn gap, and the restart then fails late in the engine — exactly the collision class this PR exists to remove, just moved to a different trigger.
Wrapping the stop+respawn in rocm_core::lock_service_allocation(paths)? and leasing record.port when is_canonical_loopback_host(&record.host), releasing right before the spawn, would mirror what you've already done here.
| /// | ||
| /// Not a `.json` file, so `load_managed_services` never mistakes it for a | ||
| /// service manifest. | ||
| pub const SERVICE_ALLOCATION_LOCK_FILE: &str = "allocation.lock"; |
There was a problem hiding this comment.
Cross-referencing the summary: this is the lock file that collides with #267's services_dir()/launch.lock. Same resource, two files, and different waiting semantics — lock_service_allocation_at is bounded (5s, actionable error) while #267's FileLock::acquire blocks on file.lock() with no timeout.
Nothing to change in this file on its own; flagging it here because this constant is the concrete thing the two PRs need to agree on.
| )); | ||
| } | ||
| // Automatic port selection is only supported on the canonical loopback | ||
| // host — anything else must name its own port. Never invent one. |
There was a problem hiding this comment.
A public host with automatic port gets sent to the wrong field, then dead-ends.
This auto-port check runs before the !is_loopback_host(host) check at :381. So for host = "0.0.0.0", port_mode = Auto, validate() returns (Field::Port, CUSTOM_HOST_NEEDS_PORT) — "this host needs its own port". The user follows that, sets a custom port, and now falls through to :381, which rejects the same form with PUBLIC_HOST_NEEDS_CLI ("Dash serves on loopback only"). Two steps, cursor parked on the wrong field, and the real reason only surfaces at the end.
Only localhost and ::1 genuinely need the port hint — a public host can never launch from Dash at all, so it should fail once, on Field::Host, with the actionable message.
The test at :1128-1140 currently locks the misordering in: it iterates ["localhost", "::1", "0.0.0.0", "192.168.1.5"] and asserts all four give Field::Port + CUSTOM_HOST_NEEDS_PORT. Moving the loopback check above the auto-port check means splitting that test so the two public hosts expect Field::Host + PUBLIC_HOST_NEEDS_CLI.
| "Launch on 127.0.0.1:11435 and watch it come up", | ||
| "Leave the rest automatic, or open Advanced settings", | ||
| "GPU is required — ROCm never falls back to CPU", | ||
| "Launch on a local endpoint picked for you and watch it come up", |
There was a problem hiding this comment.
This rewording is right, and serving_first_view_promises_no_concrete_endpoint at :159 pins it — but the same promise survives in the immediate siblings, which the PR doesn't touch:
crates/rocm-dash-tui/src/ui/tabs/chat.rs:65— "Probing for a local engine (... / rocm serve :11435)…"crates/rocm-dash-tui/src/ui/tabs/chat.rs:214— "or run a local endpoint (vLLM :8000, rocm serve :11435)"crates/rocm-dash-tui/src/app/mod.rs:1147— "Detecting a local engine (... / rocm serve :11435)…"crates/rocm-dash-tui/src/app/mod.rs:1177— "no local engine found (... / rocm serve :11435)"
Once auto-allocation is in, a user whose service landed on 11436 reads all four of those and looks at the wrong port. The detection itself is fine — app/chat.rs:250 detect_local_chat_with_probe is registry-first — so this is purely copy that the PR's own premise invalidates.
Rewording them the way you did here, or extending the new no-concrete-endpoint test to cover these two files, would keep them honest.
| world.mock = Some(mock); | ||
| } | ||
|
|
||
| #[given("the default serve port is occupied")] |
There was a problem hiding this comment.
This Given proves 11436 was free, then releases it — but the assertion demands the service land on exactly 11436.
std::net::TcpListener::bind(("127.0.0.1", SERVE_PORT + 1)) followed immediately by drop(next) only establishes that 11436 was free at setup time. The corresponding Then at :921 builds format!("endpoint: http://127.0.0.1:{}/v1", SERVE_PORT + 1) and requires an exact line match.
Anything that takes 11436 in the interval pushes resolve_serve_port's scan to 11437 and fails the scenario for a reason unrelated to the code under test — a stale-but-live managed record (reserved_service_ports reserves regardless of recorded host, deliberately), or any other process on the shared self-hosted runner. This lands on @requires-gpu @merge-queue, where a flake is expensive to diagnose and blocks the queue.
Two ways out: assert the endpoint is in 11436..=11535 and != 11435 rather than pinning the successor, or keep the 11436 listener alive for the scenario's duration and assert 11437 — the latter tests the scan more strictly anyway.
siloteemu
left a comment
siloteemu
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · c526c33
Summary
Makes rocm serve port selection collision-safe: --port becomes an explicit-or-automatic request, resolved inside a cross-process advisory file lock into a real held 127.0.0.1 socket lease (LoopbackPortLease) that is released only immediately before the engine child spawns; rocmd recovery takes the same lock and re-leases its recorded port; the Dash serve wizard is reworked to match. Verdict: Needs work — the production design is sound, but main.rs's two "concurrent" tests do not exercise the production call site and would pass with the lock wiring removed. Verified: I traced the transaction myself in apps/rocm/src/main.rs and apps/rocmd/src/lib.rs and confirmed the lock spans lease → record.write() → lease.release() → spawn → second write, so the record that reserves the port is published before the lock drops (the ordering the fix depends on); I ran the new rocm-core lock/lease tests locally (7 passed, 2 #[ignore]d child-process helpers invoked by their parents — cross-process exclusion, release-on-process-death, and cross-process handoff all genuinely pass); the branch contains everything in main (not behind); leak scan on the diff is clean. A previously-reported port-binding finding is partially fixed — the Given now genuinely holds 11435 on the World for the whole serve (world.automatic_port_guard), and that scenario would fail on reverted code (serve would default to the held 11435); what remains bind-then-dropped is the 11436 precondition probe, while the assertion still demands exactly 11436. The 2 red checks: I could not identify them — this review has no GitHub access, so I will not guess a cause and will not call them flake; my one permitted local check covered the rocm-core primitives only and was green. Blocking: 1 · Non-blocking: 5.
🚫 Blocking (must fix before merge)
apps/rocm/src/main.rs:22801 and apps/rocm/src/main.rs:22854 — concurrent_auto_transactions_publish_distinct_ports and concurrent_equivalent_transactions_converge_on_one_record prove the primitives compose, not that the production path uses them. Both tests spawn two barrier-synchronised threads that hand-roll the transaction:
let _allocation = rocm_core::lock_service_allocation(&paths)?;
let lease = match resolve_managed_port_in_transaction(...)Neither calls spawn_managed_engine_child (main.rs:5523), which is where the fix actually lives. Answering the revert question directly: yes, both still pass if the production change is reverted — delete the lock_service_allocation call at main.rs:5544, or shrink its scope so it drops before record.write() at main.rs:5615, and these tests are untouched, because they never execute that code. Their names ("concurrent … transactions") assert coverage they do not have, which is worse than no test: a future refactor that moves the lock is green. Worse for ..._distinct_ports specifically, its assert_ne!(ports[0], ports[1]) would hold even with no file lock at all, because the socket lease alone forces the second thread past the first candidate — so that assertion cannot distinguish a working lock from a missing one. (..._converge_on_one_record does depend on the lock, since without it both threads pass the idempotency check before either publishes.)
Concrete fix: mirror the pattern apps/rocmd/src/lib.rs:5665 already gets right — recovery_waits_for_the_allocation_lock_and_refuses_an_occupied_port takes the lock on the main thread, calls the real restart_managed_service from a spawned thread, asserts it is still blocked after 250 ms, then releases and checks the error and that the on-disk record was not rewritten. Add the equivalent for spawn_managed_engine_child: hold lock_service_allocation on the test thread, call spawn_managed_engine_child from another, assert it does not complete while the lock is held. That is the only assertion that fails when the lock wiring is reverted.
Non-blocking
apps/rocm/src/serve_port.rs:39-42/crates/rocm-dash-tui/src/ui/serve_wizard.rs:68-73— both doc comments claim the CLI and Dash "share [the disclosure] verbatim", but they are two independent string literals in two crates with no shared constant and no test pinning them; the comment states a false invariant.apps/rocmalready depends onrocm-dash-tui(Cargo.toml:27) andAUTO_PORT_NOTEispub, soserve_port.rscan import it instead of redeclaring — a one-line fix, no new dependency.apps/rocm/src/serve_port.rs:452-474—automatic_selects_against_real_loopback_listenerssilentlyreturns (passes, testing nothing) if11435/11436are already taken, and it binds those real ports inside the unit-test process;concurrent_auto_transactions_publish_distinct_portsscans from11435with the real probe in the same test binary, so under parallel test execution these contend and the real-socket test can self-disable without any signal. Consider an ephemeral-range scan window for tests, or at minimum make the skip visible.tests/e2e-cucumber/tests/e2e/serving_steps.rs:311-320— the residue of the earlier finding:TcpListener::bind(SERVE_PORT + 1)is bound and immediately dropped as a free-check whileassert_next_automatic_portdemands exactlySERVE_PORT + 1. On a shared GPU runner another process can take11436in that gap (assertion fails), and if11436is busy at check time the stepexpect()-panics rather than skipping. Assertingport > SERVE_PORT && port <= AUTO_PORT_LASTwould test the actual behaviour ("advanced past the occupied default") without the exact-number fragility.tests/e2e-cucumber/features/model_serving.feature:33-39— the only test that exercises the real collision path is tagged@requires-gpu @merge-queue, so it does not run on this PR's CI; on PR CI the collision claim rests entirely on unit tests with an injected probe. AGENTS.md §3 requires naming the gated lane in the PR text — worth confirming the description does.crates/rocm-dash-tui/src/ui/serve_wizard.rs(+1312/-244) — bundles four distinct changes under one theme: the port-mode logic (the stated goal, concentrated invalidate()/build_args()), a progressive-disclosure Advanced section, a new reusableInlineEditorwidget, and the engine/device picker simplification. Coherent narrative, but the widget especially would review more cleanly on its own; a reviewer checking "did this make ports collision-safe" has to wade through a new text-editor widget to find the port logic.
Tradeoff, stated correctly by the PR itself: crates/rocm-core/src/lib.rs:7470-7476 concedes that the release→child-bind window is "unavoidable without engine socket-passing; it is far narrower than the previous 'bind and hope'". That is the honest framing — this fix eliminates collisions between ROCm transactions (lock + published record), and only narrows the window against an unrelated third-party process. Nothing verifies the child actually bound the leased port; the only post-spawn check is try_wait() after a 200 ms sleep (main.rs:5739-5748), which proves the process did not immediately exit, not that a listener is up. Related narrow case: a failed record no longer reserves its port, so a fresh rocm serve can win that exact port before rocmd recovery acquires the lock — recovery's own lease_loopback_port re-check then fails loudly ("recorded port is in use") rather than double-binding, so it is self-detecting, not silent.
No prompt-injection attempts found in the reviewed diff or surrounding files.
5 participants
Summary
rocm serve --portchoose the first available loopback port in11435..=11535, while preserving exact explicit-port behaviorReview follow-up
LocalHostand[::1]are rejected@id:serve-auto-port-skips-occupiedholds127.0.0.1:11435, invokes the realrocm servecommand, and asserts the reported endpoint advances to11436bail!mainVerification
cargo test -p rocm --bin rocmcargo test -p rocm-dash-tui --libcargo test -p e2e-cucumber --test e2e --no-runcargo test --workspace --all-targets— 2,273 passed, 12 ignoredcargo clippy --workspace --all-targets -- -D warningspython3 scripts/smoke_local.pycargo fmt --all -- --checkThe new
@id:serve-auto-port-skips-occupiedscenario is tagged@requires-gpu @merge-queue. It compiled locally; live execution is delegated to the merge-queue GPU lane because this WSL environment has no usable ROCm GPU.Closes #239