Make model serving ports collision-safe - #243
Conversation
Signed-off-by: Michael Roy <michael.roy@amd.com>
Signed-off-by: Michael Roy <michael.roy@amd.com>
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.
| /// 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.
| /// 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.
| // — 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.
2 participants
Summary
rocm serve --portchoose the first available loopback port in11435..=11535, while preserving exact explicit-port behaviorVerification
cargo test -p rocm --bin rocmcargo test -p rocmd --libcargo test -p rocm-dash-tui --librocm-coreallocation-lock and loopback-lease testscargo clippy --workspace --all-targets -- -D warningspython3 scripts/smoke_local.pyROCM_CLI_*rootsCloses #239