Make model serving ports collision-safe

[michaelroy-amd]

Summary

• make omitted rocm serve --port choose the first available loopback port in 11435..=11535, while preserving exact explicit-port behavior
• serialize managed launch and recovery port claims with a bounded cross-process lock and keep the resolved endpoint in the service registry
• simplify the Dash serve wizard to model selection plus inline Advanced settings, with explicit choice/edit affordances and validation before approval
• align Dash loopback validation exactly with the CLI and preserve wizard state when Escape cancels Host/Port editing
• add a real-CLI Gherkin scenario proving automatic serving skips an occupied default port

Review follow-up

• Dash accepts only the exact loopback spellings accepted by the CLI; LocalHost and [::1] are rejected
• the app-level Escape gate now treats the inline editor as a nested layer, so Escape cancels the edit instead of closing the wizard
• @id:serve-auto-port-skips-occupied holds 127.0.0.1:11435, invokes the real rocm serve command, and asserts the reported endpoint advances to 11436
• removed the duplicate unreachable bail!
• preserved the generation-default/recipe mismatch guard while rebasing the launch path onto current main

Verification

• cargo test -p rocm --bin rocm
• cargo test -p rocm-dash-tui --lib
• cargo test -p e2e-cucumber --test e2e --no-run
• cargo test --workspace --all-targets — 2,273 passed, 12 ignored
• cargo clippy --workspace --all-targets -- -D warnings
• python3 scripts/smoke_local.py
• cargo fmt --all -- --check
• code review and Rust review completed with no unresolved critical/high defects

The new @id:serve-auto-port-skips-occupied scenario 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

[juhovainio]

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 serve command — 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.

[juhovainio]

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.

[michaelroy-amd]

Fixed in c526c33. Dash now uses the exact case-sensitive host set accepted by the CLI, with regression coverage for LocalHost and [::1].

[juhovainio]

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.

[michaelroy-amd]

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.

[juhovainio]

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.

[michaelroy-amd]

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.

[juhovainio]

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.

[michaelroy-amd]

Fixed in c526c33. The duplicate bail! is gone; the structurally unreachable fallthrough is now explicit after validate_port_request.

[volen-silo]

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 --port is silently dropped when an equivalent service is already live.

resolve_managed_port_in_transaction answers the idempotency guard before it looks at the request:

if let Some(existing) = existing_live_managed_service_in(&live, engine, canonical_model_id) {
    return Ok(ManagedPortDecision::AlreadyLive(Box::new(existing.clone())));
}

The AlreadyLive consumer then bails only on a recipe mismatch. So rocm serve <model> --port 12000, against a live service for the same engine and model on 11435, reports success on 11435 — a port the user did not ask for, with no note. That contradicts the explicit-port contract this PR otherwise enforces strictly.

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.

serve-auto-port-skips-occupied pins the automatic path, but the explicit path is the other half of the contract and is the one that changes a user-visible failure mode — it now fails up front instead of surfacing as an engine bind error. Worth a sibling scenario holding an address and naming it via --port, asserting both the refusal and that no service record is left behind (the stale record was part of the original complaint).

One tagging note if you do add it: @merge-queue is documented as "a heavy real-GPU serve that is redundant with a cheaper per-engine canary". An explicit-busy-port scenario never launches an engine — it is refused at the address check — so it is cheap enough for the per-PR GPU lane. I tagged mine @merge-queue by copy-paste and it silently did not run; the lane went green without it. Worth checking whether serve-auto-port-skips-occupied wants to be per-PR too, since a scenario that only runs in the merge queue gives no signal on the PR that changes the code it covers.