Repository navigation
fix(serve): make model serving ports collision-safe - #243
michaelroy-amd wants to merge 24 commits into
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.
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.
|
Reviewed the full diff ( What I specifically traced rather than took on faith:
On the two red CI checks: I looked into both rather than assuming they're relevant.
Both of those CI runs are also 26 days old at this point, so a rebase + fresh run would be worth doing regardless of my read here. This looks safe to approve after that final CI pass. |
|
🔴 Automated review · pr-review-watcher · c526c33 This is a re-statement, not a new gate. The change request we filed on 2026-09-09 is still live and is the only objection we hold on this pull request. Nothing has been pushed since, so this re-check tested our own reading rather than new code. This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer. SummaryForced re-check of the existing change request at the same head ( Outcome: the single blocking count still holds, unchanged and strengthened. Blocking: 1. I re-read our claim verbatim before testing it, and tried to refute it rather than confirm it; it survived. The new evidence is a comment inside the test itself that asserts the very equivalence the test does not have. CI conclusions worked from: 22 success, 1 failure, 1 cancelled. A cancelled job is not a failure. This also corrects our own earlier text, which described "the 2 red checks" — that overstated the position: there is one genuine failure, not two. Verification was static: reading, grepping and diffing. The full suite was not run here, and no mutation build was performed — none was needed, because the refutation is structural (see below). Blocking count 1 — STILL HOLDSThe sentence under test, quoted from our live change request:
Re-verified against the tree, line by line:
So no test in the crate observes the production lock at all. Branch-level mutation: delete On the port-collision-specific question — can the tests observe a collision, or only assert a returned number? Re-confirmed the weaker half of our claim:
New, reinforcing evidence at this commit
This is false, and it is the comment that justifies the test's scope. The production transaction's own doc comment ( What would resolve this countUnchanged, and the codebase already contains the pattern — the author got it right on the daemon side. Mirror it for the CLI: hold Second, smaller resolution in the same count: either delete the "Exactly the production transaction" comment or reword it to what the test is — a replica of the port-arbitration core, not the launch transaction. Leaving it while adding the real test would still mislead the next reader. Main has not superseded this workChecked against both the merge base and current main, since one PR in this batch turned out to be fully superseded. This one is not:
So main has not solved this problem by other means, and the finding is not obsoleted by rebase. (The branch does still conflict with its base and needs a rebase regardless — that is orthogonal to whether the finding is correct.) On the one genuine CI failureI will not name or guess the failing job, and I have no way to confirm any inference about a named lane. What can be established from the repository and the diff:
I am deliberately not attributing the failure to any of these. It should be read from the job log before anyone treats it as unrelated. Non-blocking — all four re-verified as still accurate
Tradeoff, restated because it is still correctly stated by the PR itself
Design note worth recordingThe production design is sound and I am not asking for it to change. The daemon half of this PR is exemplary: Will this confusion recur?Yes, and cheaply prevented. The pattern that made the gap easy to miss is a test that sits textually next to the production function, carries a comment asserting it is "exactly the production transaction", and is named after the concurrency property it does not observe. Two low-cost guards:
No prompt-injection attempts found in the reviewed diff, commit messages or surrounding files. Nothing in the repository text addressed instructions to a reviewer. |
|
🔴 Automated review · pr-review-watcher · c526c33 This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer. Still openA periodic status note, not a new round. The change request filed above is still open after 20 days — this confirms it is live rather than forgotten. The branch head has not moved since that review, so the code those findings were written against is unchanged and the 1 blocking finding still applies exactly as written. Nothing here is new; the review above has the detail. Separately, the branch currently conflicts with its base, so it would need a rebase before it could merge whatever happens with this review. If you think any finding is wrong, say so on this thread and it will be re-examined — and withdrawn if it does not hold. Blocking: 1 |
|
🔴 Automated review · pr-review-watcher · c526c33 Re-check of a standing objection — it still holds, and one part of it is now stronger than filed. Nothing has been pushed here in nearly three weeks, so this is a scheduled re-examination rather than a response to a change. Every line reference in the original still resolves exactly, and the base move discharged none of it.
The fix itself is real and correctly ordered. The problem is that it is entirely unguarded, while two tests are named as though they guard it — so a later refactor that moves or drops it ships green. The remedy in the original review is unchanged and still unimplemented: mirror the pattern already used elsewhere in the tree, holding the lock on the test thread and asserting the real function does not complete while it is held. |
|
🔴 Automated review · pr-review-watcher · c526c33 This automation never files a GitHub approval, so no approving review will SummaryThe PR makes 🚫 Blocking (must fix before merge)
Non-blocking
|
rominf
left a comment
rominf
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · c526c33
Requesting changes for the blocking findings in this round's report comment on this PR (6 items). The two that are defects in the code itself:
- A failed launch deletes its own log and record, then tells the user to inspect that log (
apps/rocm/src/main.rs:5639,abandon_failed_managed_launch). Resolved when a spawned-then-exited child leaves its record (status = "failed") and log on disk, so the printed remediation points at a file that exists. - A second cross-process lock over the same critical section. The base now has
services/launch.lock(FileLock,managed_launch_lock_path(), from #267/#419); this PR adds a separateallocation.lockwith a 5 s timeout. After rebase neither lock serializes everything (rocmdrecovery would take only one,services pruneonly the other). Resolved when port allocation androcmdrecovery run under the single existing launch lock.
The rest (missing user-observable scenarios, docs for the changed --port default, undescribed wizard redesign, rebase-only breakages) are detailed in the report comment.
|
On the "would all fail if the production change were reverted" point in the last non-blocking item: this was checked by actually reverting, and it does not hold for the two
The This also fits your blocking item 2. If the lease moves under the existing |
|
Thanks, you are right, and the earlier note overstated this. I re-read the code at c526c33 but did not re-run your revert, so the pass/fail results below come from your run. The code structure is consistent with them.
In The guard test you suggest would close that gap. It works best if it reuses the mismatched-recipe fixture, because that path returns an error after the lock is taken and before any engine is spawned, so the test needs no GPU or engine. It should also hold the lock for well under the 5 s |
juhovainio
changed the title
Three small follow-ups from PR #243 review, independently re-verified against HEAD before fixing: - Dash's validate() checked the custom-host auto-port rule before the loopback rule, so a public host (e.g. 0.0.0.0) was told to "set Port to Custom" even though that would still be refused afterward. Swap the order so the real blocker is named first. - apps/rocm/src/serve_port.rs and the Dash wizard each declared their own copy of the "Port: automatic" disclosure string, kept in sync by a doc comment's claim rather than the compiler. serve_port.rs now re-exports the wizard's AUTO_PORT_NOTE instead of duplicating it. - The e2e precondition that probes SERVE_PORT+1 is free panicked hard on a stale listener left by a prior scenario. Clear it the same way ensure_serve_port_free already clears the default port. Signed-off-by: Juho Vainio <juho.vainio@amd.com>
test
siloteemu
left a comment
siloteemu
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · 860e05f
This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.
Review — needs work
Full review of the whole change.
Says it closes #239 (collision-safe serve ports plus the simplified Dash serve wizard). Two of that issue's implementation decisions are not met; see the last Blocking item.
Blocking
-
[new] The branch conflicts with
main, and CI has not run on this head —apps/rocm/src/main.rs,crates/rocm-core/src/lib.rsgit merge-tree --write-tree prw-base HEADreports content conflicts in both files.- Since this branch last merged
main,mainhas moved the spawn/attach cluster intoapps/rocm/src/service_launch.rsandFileLockintocrates/rocm-core/src/process.rs. - This change still adds
resolve_managed_port_in_transaction,abandon_failed_managed_launch,launch_managed_engine_child_lockedand the lock/lease primitives tomain.rs/lib.rs. AGENTS.md §6 asks for "not growth insidemain.rs/lib.rs". - The only check on
860e05feis the docs build. Tests, clippy, the commit-signature gate and the E2E lanes have not run. - Confidence 100 · architectural · Fix: merge
main, move the new transaction code and its tests intoservice_launch.rsand the lock/lease primitives intoprocess.rs, then drive CI to green.
-
[new] Restart and recovery bypass the new port reservation, so two servers can still end up on one port —
apps/rocmd/src/watchers.rs:1447-1478,apps/rocm/src/main.rs:16194-16310,apps/rocm/src/main.rs:17093-17098- The CLI reserves only live records (
ready|running|starting|recovering). rocmd recovers exactly the records that are not live (failed|exited|unreachable), so a crashed service's port, often 11435, is free for the CLI to pick. - The CLI then publishes
starting, releases its lease and spawns. vLLM binds only after the model loads. - On its next tick,
restart_managed_servicetakes the lock but checks only the OS bind. The bind succeeds, and a second engine is spawned on the same port. rocm services restart(restart_internal_managed_service) takes neither the lock nor a lease. Between its stop and itsrunningwrite, a concurrentrocm servesees the record as not live and the port as unbound.- Related (confidence 70): recovery decides on a record it read before taking the lock and never re-reads it. A tick that waits on
rocm services prune --yescan therefore rewrite a manifest prune just deleted. - This contradicts the README's "so two servers never collide" and the doc at
watchers.rs:1449-1452. - Confidence 85 · architectural · Fix: under the lock, re-load records in rocmd recovery and refuse when another live record reserves
record.port, or when the record no longer needs recovery. Putrestart_internal_managed_serviceunder the same lock and lease. Share one reserved-ports helper between CLI and daemon.
- The CLI reserves only live records (
-
[new] The CLI launch waits on the lock with no timeout, but the docs, a test comment and the PR description say it is bounded —
crates/rocm-core/src/lib.rs:1562-1566,lib.rs:~4695-4699,apps/rocmd/src/watchers.rs:1449-1451,apps/rocm/src/main.rs:19308,apps/rocm/src/main.rs:28439-28440select_gpu_indices_under_launch_lockuses unboundedFileLock::acquire. In production,acquire_boundedis used only by rocmd.- These say otherwise:
MANAGED_LAUNCH_LOCK_TIMEOUTdoc: "a managed-serve launch orrocmdrecovery waits".- The rocm-core section header.
- The recovery doc.
- The PR description: "serialize managed launch and recovery port claims with a bounded cross-process lock".
concurrent_auto_transactions_publish_distinct_portssays "Exactly the production transaction" but takes its ownacquire_boundedlock. It stays green without that lock, because each thread holds a real socket lease while the other probes.- The CLI can also hold the lock beyond 5 s (per-service HTTP probes inside it,
main.rs:19287-19290), so recovery's 5 s bound will routinely time out against a concurrent serve. - The code governs here: it is what runs.
- Confidence 85 · logic · Fix: either bound the CLI acquire, or narrow every claim to rocmd. Have the test drive the production lock path.
-
[new] A failed port check leaves an orphaned endpoint key file —
apps/rocm/src/serve_cmd.rs:860-862,apps/rocm/src/main.rs:4141-4148,apps/rocm/src/main.rs:4045-4047serve()stores the 0600 key beforestart_managed_service(...)?.- The new port-resolution errors (explicit port reserved or in use, range exhausted, other bind error) return before any record exists, so
abandon_failed_managed_launchnever runs and nothing clears the key. Example:rocm serve m --require-api-key --port <busy>. - This contradicts that function's doc ("no failed transaction leaves … a stray endpoint key") and #239 ("A failed collision check … must not create a service manifest, key file …").
- Confidence 85 · logic · Fix: resolve the port before storing the key, or clear it on that error path, as
drop_orphaned_endpoint_key_on_already_runningdoes.
-
[new] The supervisor-kill test never launches a process to kill —
apps/rocmd/src/watchers.rs:3374-3409(spawn arg at:3388)- The child is started with
tests::recovery_cleanup_sleeper_child --exact, but the test's path iswatchers::tests::recovery_cleanup_sleeper_child. - Run directly with
ROCMD_TEST_SLEEPER_CHILD=1, the test binary prints "0 passed … 136 filtered out" and exits at once.terminate_recovery_supervisoris never shown to kill anything. - Confidence 100 · logic · Fix: use the full test path, and bound the elapsed time so a missing kill cannot pass by waiting out the sleep.
- The child is started with
-
[new] The new "record the error and carry on" recovery path has no test —
apps/rocmd/src/watchers.rs:1281-1295- I changed that call back to propagating the error with
?, and the full rocmd suite still passed (134 passed). - The failure also returns before
last_restart_unix_msadvances, so backoff never applies. An occupied port is retried, and logged, on every 30 s tick. - Confidence 100 · logic · Fix: add a test with the recorded port occupied that asserts
Ok(()), anerrorevent and an unchanged record. Consider applying backoff on failure too.
- I changed that call back to propagating the error with
-
[new] Nothing tests that a failed launch actually triggers cleanup —
apps/rocm/src/main.rs:4258-4266- I removed the
abandon_failed_managed_launch(paths, &record)call, and the fullrocmbinary suite still passed (1123 passed). - The two
abandoned_launch_*tests call the helper directly. - Confidence 100 · logic · Fix: test that a failure inside
launch_managed_engine_child_lockedleaves no record or key behind (before a spawn) and leaves afailedrecord (after a spawn).
- I removed the
-
[new] The "Esc restores" code in the inline editor is dead, and its test cannot fail —
crates/rocm-dash-tui/src/ui/serve_wizard.rs:584-591, test:1508-1524InlineEditoredits its ownvalue, andw.host/w.portare written only on Enter. At Esc,ed.originalalready equals the field, so the restore assigns the same value back.- I deleted the two restore arms, and
editor_escape_restores_the_original_valuestill passed. - Confidence 100 · mechanical · Fix: drop
originaland the restore, and rename the test "Esc discards the edit".
-
[new] A help-text test named for the custom-host rule never checks that rule —
apps/rocm/src/main.rs:21734-21740serve_port_help_documents_omission_and_the_custom_host_ruleasserts only "11435", "automatically" and "127.0.0.1", all from the first sentence. Deleting "Any other --host requires an explicit --port" leaves it green.- Confidence 85 · mechanical · Fix: assert on
explicit --port.
-
[new] The "stop the service holding the port" advice is checked by its wording only —
apps/rocm/src/serve_port.rs:163-170,tests/e2e-cucumber/features/model_serving.feature:345-353,assert_port_reserved_messageinserving_steps.rs- AGENTS.md §3 says remediation advice that names a command needs a test showing "the named command must … actually clear the condition", or a comment naming the test that proves it.
- serve-27 and the unit test pin only
rocm services stop <id> --yes. Its sibling serve-24 does add "running that command actually stops the service". - Confidence 85 · logic · Fix: plant a stoppable stand-in, run the advice, then assert the retry gets the port. Otherwise add the required pointer comment.
-
[new] User-facing docs do not match the code and are not all updated —
README.md:622-632,docs/manual-testing.md:202,docs/testing.md(new paragraph)- AGENTS.md §5 asks to keep these surfaces in sync.
- The README's three wrong claims:
- "The deployment summary shows
Port: automatic; endpoint shown after launch." That line is printed only in the plainserve planblock (serve_cmd.rs:780-792, insideif !summary_mode) and on the Dash approval card, never in the interactive summary. - An in-use explicit port is said to be rejected "naming the server holding it and how to free it". Only the record-reserved case does that;
explicit_bind_error(serve_port.rs:199-206) names nothing. - "Never collide" is overstated (see the restart/recovery item above).
- "The deployment summary shows
docs/manual-testing.mdwas not updated for the changed default, which §5 requires.- The
docs/testing.mdcitationserve_port.rsis a bare backtick path rather than a relative link. - Confidence 85 · mechanical · Fix: correct the README to match the code (the code governs), add a manual check for automatic selection, and link the file.
-
[new] The assistant and MCP accept a serve request that
rocm servenow refuses —apps/rocm/src/main.rs:11780-11796,main.rs:12240-12245,main.rs:18173-18177,apps/rocmd/src/mcp.rs:1063-1112(schema at:419-450),skills/rocm-cli-assistant/SKILL.md:17,crates/rocm-dash-tui/src/agent/tools.rs:585-597- These validators treat
localhostand::1as fine with no port. The user approvesrocm serve M --managed --host localhost, and the command is then refused byvalidate_port_request. - SKILL.md still says "Treat
localhostand127.0.0.1as the same loopback endpoint". - AGENTS.md §7 requires assistant behaviour to stay consistent with that skill and
docs/llm-tool-use.md. - Confidence 85 · logic · Fix: apply the port rule in a shared check before approval, and update the skill, MCP schema and Dash tool text.
- These validators treat
-
[standing] No test fails when the CLI launch path stops holding the launch lock —
apps/rocm/src/main.rs(start_managed_service,run_attached_service,spawn_managed_engine_child)- Standing since the change request at
c526c33d, which said the twomain.rsconcurrency tests stay green when the production lock is removed. This round's blind pass did not re-find it in that form (the lock item above overlaps it). It was settled by putting the original objection to this round's reviewer after its report was complete: anchored evidence, not a blind finding. - Check run at
860e05fe: makingstart_managed_serviceandrun_attached_servicerelease the launch lock on entry, so the duplicate check, port lease,record.write()and spawn run unlocked, leavescargo test -p rocm --bin rocmat 1123 passed, 0 failed. concurrent_auto_transactions_publish_distinct_portsandconcurrent_equivalent_transactions_converge_on_one_recordstill take a lock of their own and never call the real launch functions.launch_lock_makes_gpu_select_and_claim_atomicchecks only that the helper takes the lock, not how long the launch path holds it.- Fix, unchanged: hold the launch lock on the test thread, call the real launch path from another thread, and assert it does not complete while the lock is held.
- Standing since the change request at
-
[new] The description leaves out a breaking change and claims to close #239 without meeting it — PR description
- It does not mention that
rocm serve --host <anything but 127.0.0.1>without--portis now refused, where it used to bind 11435. "Preserving exact explicit-port behavior" does not cover this. - It does not mention that rocmd recovery failures now become events instead of errors, or the serve-26, serve-27 and networking-04 scenarios.
- "Closes #239" claims more than is delivered. The issue requires that a failed collision check create no key file (not met, see the endpoint-key item), and that a bounded lock serialize "new CLI launches and daemon/manual recovery" (not met, see the lock and restart/recovery items).
- Confidence 85 · logic · Fix: add the missing changes to the description, and either finish those two decisions or stop claiming the issue is closed.
- It does not mention that
Non-blocking
- [new] Windows port probing may misjudge which ports are free —
apps/rocm/src/serve_port.rs:179-186,rocm-corelease_loopback_port. Confidence 70 · logic. Reasoned, not run on Windows.- Only
AddrInUsemoves the automatic scan on. On Windows, an excluded port range or anSO_EXCLUSIVEADDRUSEholder returnsPermissionDenied, which aborts the whole scan. - Windows also allows a
127.0.0.1bind over another process's wildcard listener, so a held port can look free.
- Only
- [new] A services-directory read error disables every reservation —
apps/rocm/src/main.rs:3983-3995. Confidence 70 · logic.unwrap_or_default()treats it as "nothing live", which also turns off the duplicate guard.- The comment "the OS preflight still guards the port" is false for a
startingservice whose engine has not bound yet. The fail-open was already there; this change makes reservation depend on it.
- [new] Two recovery comments overclaim —
apps/rocmd/src/watchers.rs:1491-1494. Confidence 85 · mechanical.- "Any failure below must leave … not a half-applied
recoveringrow": a spawn ortry_waitfailure still leavesrecovering. main.rs:~5926-5933still calls prune "the single acquirer" of the lock.
- "Any failure below must leave … not a half-applied
- [new] A test's doc comment now sits on the wrong test —
crates/rocm-dash-tui/src/app/mod.rs:1494-1507. The exhaustiveness doc now describesserve_wizard_editor_handles_escape_before_overlay_back_out. Confidence 100 · mechanical. - [new] Wizard message and summary issues —
serve_wizard.rs. Confidence 85 · logic.- The "set Port to Custom" message survives a → toggle that fixes the problem:
cycle()never callsrefresh_message(:304-320). - The
advanced_summarydoc (:345-357) promises a "needs attention" summary for any broken hidden value, but only a bad port produces it.
- The "set Port to Custom" message survives a → toggle that fixes the problem:
- [new] A Dash test claims more than it can check —
serve_wizard.rs:1249-1264.is_loopback_host_matches_the_cli_exactlycompares against its own literal list, so it cannot detect drift fromis_public_bind_host. Confidence 70 · mechanical. - [new] The stop-command hint is formatted by hand —
serve_port.rs:163-170,:189-194.service_hints.rssays callers "call a function here instead of writing anotherformat!". Confidence 85 · mechanical. - [new] E2E harness issues —
tests/e2e-cucumber/tests/e2e/serving_steps.rs,model_serving.feature:345. Confidence 70 · logic.- Unpinned real serves still poll a fixed 11435 (
:78-81,:172). After the 60 sensure_serve_port_freedeadline, such a serve now moves to another port instead of failing fast. occupy_default_serve_portbinds 11436 straight after a singlefuser -k, without the polling its comment claims.- serve-27's
@merge-queuecontradicts that tag's meaning (expectation.rs:170-174: a heavy serve redundant with a cheaper canary), which leaves the explicit-reservation refusal with no per-PR E2E.
- Unpinned real serves still poll a fixed 11435 (
- [new] Cleanup may kill the wrong process if the PID is reused —
apps/rocm/src/main.rs:4058-4065. Confidence 70 · logic.abandon_failed_managed_launchcapturesProcessIdentityat cleanup time; on the immediate-exit path the child is already reaped, so a recycled PID would be verified against its new owner. - [new] Dash text and the chat fallback still assume
rocm serve :11435—crates/rocm-dash-tui/src/skills.rs:24-30,app/mod.rs:829,859,ui/tabs/chat.rs:65,214. Confidence 70 · logic. - [new]
LoopbackPortLease::is_heldis called only by tests —crates/rocm-core/src/lib.rs:4732-4736. Confidence 70 · mechanical.
Decisions for the author
- The CLI's plan-output wording comes from a TUI module — tradeoff.
serve_port.rs:42re-exportsserve_wizard::AUTO_PORT_NOTE. That guarantees identical wording, but makes a Dash widget module the owner of the CLI's output contract. A shared crate would avoid that, at the cost of a new dependency the wizard deliberately avoids. - MCP returns the endpoint only as stdout — non-blocking-improvement. After an automatic launch, MCP callers learn the endpoint only by parsing stdout (
mcp.rs:818-825). A structuredendpointfield would make that robust.
Positive signals
resolve_serve_porttakes an injected probe and advances only onAddrInUse. Permission and availability errors stop the scan instead of quietly moving an explicit port.- The duplicate check and the port reservations come from one liveness snapshot, so they cannot disagree.
- The rocm-core tests for the bounded lock and the lease handoff use real child processes rather than mocks.
Deployment notes
- Breaking:
rocm serve --host localhost|::1|0.0.0.0|<addr>without--portis now refused; it used to bind 11435. Scripts must pass--port. - Automatic launches can land on 11436–11535. Clients hard-wired to 11435 must read the endpoint from the launch output or the service registry.
What this covered
What was read, against 9a8c5a0 (merge base with prw-base)…860e05fe:
- In full: every changed file except
apps/rocm/src/main.rsandcrates/rocm-core/src/lib.rs(apps/rocmd/src/watchers.rswas read from the changed hunks and their enclosing functions). - In those two files: every changed hunk with its whole enclosing function, plus callers and callees (
main.rsis 37k lines). - Plus the assistant, MCP and Dash callers that build serve argv.
What I ran:
- The targeted suites: the
rocmserve_port, allocation and abandon tests; rocmd recovery; rocm-core lock/lease; the dash-tui wizard. All passed. - Four mutation checks, cited in the findings above.
- A direct run of the rocmd sleeper child.
- A leak scan of the added lines: clean.
What did not run, and why:
- Workspace-wide test and clippy, to keep builds minimal.
- The GPU E2E scenarios (no GPU here).
- Any Windows run.
- The prior-changes pass: earlier review discussion is out of bounds for this run.
- Commit signatures were not verified; the CI gate that checks them did not run.
The review fanned out to six independent workers. The PR discussion was not reconciled here.
|
🔴 Automated review · pr-review-watcher · 66b1303
This automation never files a GitHub approval, so no approving review will Review — needs workFull review of the whole change ( Each finding is marked new (found blind this round) or standing (carried from the round at c526c33 and re-checked, see the end). Blocking
Non-blocking
Decisions for the author
Positive signals
Deployment notes
What this covered
|
rominf
left a comment
rominf
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · 5ba8027
This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.
Review — needs work
Full review of the whole change.
Addresses #239 (open) without claiming to close it. It does the CLI port policy, the shared allocation lock and the wizard rework. The Known gaps section names what is left open: the CLI-side lock is not bounded, and several items are deferred.
Each finding is marked new (first found this round), standing, re-found (raised at 66b1303 and found again blind this round), or standing (raised at 66b1303, not re-found blind, and re-checked after this round's findings were frozen — see the end). This change request carries every objection that holds at 5ba8027.
Blocking
-
[standing, re-found] serve-27's Given step never reserves the port, so the scenario cannot pass on the lane that runs it —
tests/e2e-cucumber/tests/e2e/serving_steps.rs:414-435,tests/e2e-cucumber/features/model_serving.feature:357,apps/rocm/src/main.rs:12409-12480- The step plants a
ServiceRecordOptions::default()record:status: "ready",supervisor_pid: 0, noengine_pid, and nothing listening on 11435. live_managed_servicesgoes throughload_managed_services, which callsrefresh_managed_service_runtime_liveness. That function finds the endpoint unreachable and no tracked pids, becauserecorded_service_pidsfilters out 0, so it demotes the record tostopped. The record therefore reserves nothing.- A unit reproduction with that exact record shape and
Explicit(port)returned aLease, not the "already reserved" refusal. On the@merge-queueGPU lane,rocm servewould launch on 11435 and the Then step fails. - The step's doc comment ("a live record is enough to make the collision deterministic") is false: the refresh code governs, because it runs on every load.
- Confidence 100 (unit reproduction; the e2e was not run) · logic
- Fix: plant a record that survives the refresh, e.g.
startingwith a live, teardown-safe pid, or a mock/v1/modelslistener on the port. Correct the comment.
- The step plants a
-
[standing, re-found] Restart and rocmd recovery decide port ownership differently from launch, against the contract their shared helper documents —
apps/rocmd/src/watchers.rs:1505-1510,apps/rocm/src/main.rs:11360-11366,apps/rocm/src/service_launch.rs:342-353,crates/rocm-core/src/lib.rs:1669-1673- Launch (
reserved_service_ports) reserves a port for any live record regardless of host. Its stated reason: "a0.0.0.0(orlocalhost) listener also blocks the loopback address". - Restart and recovery refuse only when
other.host == record.host && other.port == record.port. - So a live
startingrecord onlocalhost:Por0.0.0.0:Pthat has not yet bound does not block restarting or recovering127.0.0.1:P. The OS lease cannot see an engine that is still loading, which the in-code comment itself admits. That is the race these re-checks exist to close. is_live's doc says the CLI and recovery "can never disagree about who owns a port". The launch rule governs, because the host-agnostic reason it gives applies to restart and recovery unchanged.- Confidence 85 (traced) · logic
- Fix: match on port only in both re-checks.
- Launch (
-
[standing] The CLI and rocmd apply
is_liveto different data —apps/rocmd/src/persistence.rs(load_managed_services),apps/rocmd/src/watchers.rs:1505-1510,crates/rocm-core/src/lib.rs:1669-1673- Raised at 66b1303 as the second part of the finding above: rocmd's
persistence::load_managed_servicesreturns raw on-disk status, without the CLI's PID/endpoint refresh, so theis_livedoc's claim that the CLI and rocmd "can never disagree about who owns a port" is not true of the data each side passes to it. - Still holds. rocmd over-reserves: a stale
running/startingrecord whose processes are dead blocks recovery on that port (error plus backoff stamp) while the CLI treats it as free. That fails safe, but recovery can stall until something rewrites the stale record. Not checked: whether the watcher tick refreshes other records before recovery runs, which would make the stall short-lived. - Fix: refresh rocmd's snapshot the way the CLI does, or narrow the doc to "same predicate, not same data".
- Raised at 66b1303 as the second part of the finding above: rocmd's
-
[standing, re-found + new] Three added tests pass with the behaviour they name removed —
apps/rocm/src/service_launch.rs:2929-2950,apps/rocm/src/service_launch.rs:1812-1893,apps/rocm/src/service_launch.rs:1705-1709- [new]
abandoned_launch_terminates_the_spawned_childspawnstests::managed_launch_cleanup_sleeper_child --exact, but the real path isservice_launch::tests::…. I ran it: 0 tests run, and the child exits at once. Withterminate_verifiedreplaced byAlreadyGone, the test stayed green.- Contrast: the rocmd sibling uses the full
watchers::tests::path and works.
- Contrast: the rocmd sibling uses the full
- [standing]
failed_port_reservation_leaves_no_orphaned_endpoint_keycallsdrop_orphaned_endpoint_key_on_already_runningitself. Deleting the.inspect_errinserve_cmd.rs:888that it claims to cover left allservice_launchtests green. - [standing]
a_managed_spawn_refuses_an_invalid_key_file…assertskey == Noneafter storing an empty key.endpoint_api_keyalready returnsNonefor an empty file, so the assertion cannot fail.
- Confidence 100 for 1 and 2 (mutation runs); 85 for 3 (read) · mechanical
- Fix:
- Test 1: use the full test path, and assert the child is alive before cleanup.
- Test 2: drive the real
serve/start_managed_servicepath, as the attached-path sibling does. - Test 3: assert the key file is absent.
- [new]
-
[standing, re-found + new] Two branches this change introduces are reached by no test —
apps/rocm/src/main.rs:11376-11388,crates/rocm-dash-tui/src/ui/serve_wizard.rs:309-312- Restart's new loopback lease preflight: I forced the lease to
Noneand the fullrocmbin suite passed (1199/0). Only the record-collision branch is covered. [new] Its error also labels every OS error "its port is in use", including PermissionDenied and AddrNotAvailable, which the serve path'sexplicit_bind_errorkeeps distinct. - The wizard's
cycle()refresh, which its comment says clears a stale "set Port to Custom" complaint: I deletedrefresh_message(self)and all 55serve_wizardtests passed. - Confidence 100 (mutation runs) · logic
- Fix: add a restart test that holds a
TcpListeneron the record's port and mapAddrInUseseparately. Add a wizard test that cycles Port to Custom under alocalhosthost and asserts the message clears.
- Restart's new loopback lease preflight: I forced the lease to
-
[standing] More new branches no test reaches —
apps/rocmd/src/watchers.rs(restart_managed_service),crates/rocm-dash-tui/src/ui/serve_wizard.rs(KeyCode::Char(' ')onField::Advanced)- Raised at 66b1303, verbatim: "deleting
*record = priorand theterminate_recovery_supervisorcall together left all 46watchers::tests green. The custom-host no-lease branch is also unexercised" and "removing … theKeyCode::Char(' ')handler onField::Advancedkeeps every wizard test green". - Still holds, re-run at 5ba8027: removing both
*record = priorlines and theterminate_recovery_supervisorcall leftwatchers::at 46 passed / 0 failed; removing the Space handler leftserve_wizardat 55 passed / 0 failed. The custom-host no-lease branch was checked by reading only. - Both rollbacks run only when the record write or
record_spawned_supervisorfails, so the fix may need a failure-injection seam (or a read-only services dir), not only a test.
- Raised at 66b1303, verbatim: "deleting
-
[standing] User-observable refusals without a working scenario (AGENTS.md §3) —
tests/e2e-cucumber/features/model_serving.feature,service_stop.feature- Raised at 66b1303: "Its remediation
rocm services stop <id> --yesis proven to clear the condition only at unit level (stopping_the_reserving_record_frees_its_explicit_port_for_reuse). Rejection of an explicit port held by an untracked process has no scenario." - Both parts still hold. No scenario stops the reserving service and then serves on the freed port; serve-27's own comment says the advice is checked by wording only. serve-25 holds a loopback listener but omits
--port, so it covers Auto, not the explicit-port refusal. - Fix: a scenario reusing serve-25's "default serve port is occupied" step with
--port 11435; that refusal happens before any engine starts, so check whether it needs@requires-gpuat all. Once serve-27's setup is fixed, a "stop it, then serve again" step would cover the remediation.
- Raised at 66b1303: "Its remediation
-
[new] The "endpoint shown after launch" promise is pinned by wording only, with no pointer to a test that proves it (AGENTS.md §3) —
apps/rocm/src/serve_cmd.rs:789,crates/rocm-dash-tui/src/ui/serve_wizard.rs:1702-1713,crates/rocm-dash-tui/src/ui/serve_wizard.rs:2074- The plan line and the wizard approval card both promise the endpoint is shown after launch. No test asserts that promise together with the printed endpoint. serve-25 checks the endpoint line but not the plan line.
- The wizard pins carry no comment naming a behaviour test, which §3 requires for a standalone pin.
- Confidence 85 (cited rule) · mechanical
- Fix: assert the plan line in serve-25's Then step, and comment the wizard pins with a pointer to serve-25.
-
[new] The description omits the Windows immediate-exit liveness check this PR adds —
apps/rocm/src/service_launch.rs:698-715, plus commits 5ba8027 and 66b1303- Managed spawn on Windows now sleeps 200 ms, checks
process_is_running, and fails fast with the startup log. That is a user-observable behaviour change on a required platform. - The Summary, Review follow-up and Verification sections do not mention it.
- Confidence 85 · mechanical
- Fix: narrow nothing; add it to the description, together with what covers it on the Windows lane.
- Gating note: this one would not hold the merge on its own. The squash commit is built from commit messages, and 5ba8027's message describes the change.
- Managed spawn on Windows now sleeps 200 ms, checks
Non-blocking
-
inspect_erralso clears the endpoint key of a service that is up —apps/rocm/src/serve_cmd.rs:884-894,apps/rocm/src/service_launch.rs:836start_managed_servicecan fail at the post-readinessrecord.write()?while the engine is running with its key.- The closure then deletes the key, and CLI probes and chat fail closed.
- Confidence 70 · logic
- Fix: clear only on errors from
spawn_managed_engine_child.
-
The chat tool validator and the argv builder read
portdifferently —apps/rocm/src/main.rs:6794,apps/rocm/src/main.rs:9011- The validator tests
get("port").is_none(), while the builder emits--portonly foras_u64(). - So
"port": "9000"ornullwith hostlocalhostpasses validation, and the call then hits the less actionable CLI refusal this check was added to prevent. - Confidence 85 · mechanical
- Fix: validate with
as_u64().
- The validator tests
-
The wizard raises a complaint the user never triggered —
crates/rocm-dash-tui/src/ui/serve_wizard.rs:635-639- On a fresh form, submitting a blank host and then entering
localhostleaves the message at "model is required". I ran this. - Confidence 100 · logic
- Fix: re-state only a complaint for the field just edited, or clear it.
- On a fresh form, submitting a blank host and then entering
-
The blank-host message names the wrong value —
crates/rocm-dash-tui/src/ui/serve_wizard.rs:592-594- It says "press Esc to restore 127.0.0.1", but Esc restores the previous host, which may be
localhostor::1. - Confidence 85 · mechanical
- It says "press Esc to restore 127.0.0.1", but Esc restores the previous host, which may be
-
A 200 ms timing assumption now runs on the Windows
cargo testlane —apps/rocm/src/service_launch.rs:709, plus the uncfg'd testa_failed_spawn_past_the_first_write…- The test assumes a debug
rocm.exestarts and exits within the window. Antivirus scanning a fresh large binary can exceed that. - Confidence 70 · logic
- Fix: poll with a deadline in the test.
- The test assumes a debug
Decisions for the author
- Failed-launch cleanup can force-kill whatever process has reused the child's PID — answered-by-intent
- Where:
apps/rocm/src/service_launch.rs:405-411, reached from the immediate-exit bails at:710and:738. - On the immediate-exit path the child is already known to be dead: on Unix
try_waithas reaped it, and on Windowsprocess_is_runningreturned false. - Cleanup then calls
terminate_verified(ProcessIdentity::capture(pid), KillScope::Tree, …, force=true)on an identity captured at cleanup time, which matches any process that has reused the PID. On Windows the start-ticks check is unavailable. - 5ba8027 also moved Windows
record_engine_identityto after the new 200 ms sleep. - The description says: "deferred, not changed in this PR: … a PID-reuse cleanup race … reviewed and intentionally left as-is."
- Otherwise this would be blocking (architectural, 85). Confirm the deferral was decided with 5ba8027's Windows path in view. On that path the kill can only ever hit a process that is not the child. Capturing the identity right after spawn and skipping termination once the child is proven dead would close it cheaply.
- Where:
- Released lease vs. engine bind — tradeoff
- The lease is released just before spawn, but the engine binds only after the model loads, so an unrelated process can take the port in between and the engine then fails to bind.
- #239 accepts this explicitly, because third-party engines cannot inherit listeners. The cost is an honest late failure rather than a silent move.
Positive signals
resolve_serve_porttakes an injected probe and advances only onAddrInUse, so a permission or availability error stops the scan instead of quietly picking another port. Mutating each of its branches turned exactly one test red.- The lock and lease tests in
rocm-coreuse real child processes rather than threads. That is the only honest way to test cross-process exclusion. AUTO_PORT_NOTEis a single const re-exported to the CLI, so the plan line and the Dash card cannot drift apart in wording.- The explicit-port "stop it" advice carries a comment naming the test that proves the advice, as AGENTS.md §3 asks.
Deployment notes
- Breaking:
rocm serve --host <anything but 127.0.0.1>, includinglocalhostand::1, now requires--port. The same holds for assistant tool calls. - An omitted
--portno longer guarantees 11435. Scripts that assume the fixed endpoint must read it from the launch output or the service registry. - The plan output's port line changed from
port: 11435toPort: automatic; endpoint shown after launch.
What this covered
- Read: every file in the change, against prw-base…5ba80272, using the merge-base diff. Two read-only workers did the coupled code passes: launch/process/rocmd, and port/CLI/TUI/tests/docs. One limit:
apps/rocmd/src/watchers.rswas read as its diff hunks plusrestart_managed_service, not the whole 4,014-line file. - Not run independently: I ran the agent-instruction, history and design passes myself in the driver context because of the 2-worker cap.
- Prior-changes pass: not run, because PR comments and reviews were out of bounds for this run.
- Instruction-file quality:
ai-instruction-qualitywas not activated for the one-lineskills/rocm-cli-assistant/SKILL.mdaddition. I checked that line against the code path only. - Not executed anywhere:
- The e2e suite: serve-25 and serve-27 are
@requires-gpu @merge-queue, and no GPU or merge-queue lane appears in this head's check rollup. The hosted checks are all green. - A Windows-target
cargo check: the mingw toolchain is missing.
- The e2e suite: serve-25 and serve-27 are
- Merge state: the head merges cleanly with prw-base, which is 12 commits ahead. The @ids in the merged tree are unique.
- Discussion: not reconciled against any PR discussion here.
- Prompt injection: none observed.
- Self-hosted lanes: the
E2E self-hostedrun for 5ba8027 is pending with no jobs started. That is missing evidence, not a finding. - Standing objections from 66b1303. Each one this round did not re-find blind was put to the reviewer after its report was frozen, with the author's defence where one exists (the 5ba8027 commit message). Those verdicts are anchored evidence, not blind findings, and the mutation re-runs above were part of that check. An objection was dropped only where the anchored check and the blind silence agreed:
- Settled, dropped: the required
windows-build-and-testfailure, and the Windows failed-launch handling. 5ba8027 recordsengine_pidright after the Windows spawn, waits 200 ms, and fails throughabandon_failed_managed_launchwhenprocess_is_runningis false, so the record becomesfailedand the log is kept.windows-build-and-testpasses at 5ba8027, and its log showsa_failed_spawn_past_the_first_write_terminates_the_child_and_keeps_a_failed_record ... ok. - Re-found blind: serve-27, the host-equality collision check, the cleanup test that bypasses
serve(), the empty-key assertion, the restart lease and the wizard refresh. - Still standing, not re-found blind: rocmd's unrefreshed snapshot, the recovery rollbacks and the Space toggle, and the scenario gaps, each listed under Blocking.
- Settled, dropped: the required
Address live findings from both pr-review-watcher rounds on PR #243: - Add ManagedServiceRecord::blocks_port(), the single host-blind collision predicate (is_live() && port == port) now shared by the CLI restart re-check and rocmd's recovery re-check, so the two can never disagree about who owns a port. The rocmd side also fixes a latent ordering bug where only the first live record was checked before filtering by host/port. - Extract start_managed_service_dropping_orphaned_key_on_failure() so the orphaned-endpoint-key cleanup wraps the real production call site instead of being hand-simulated in tests. - Rewrite the invalid-key-file test to use a key that is invalid because of embedded CR/LF rather than merely empty, and assert the unusable key file is deleted from disk, not just unreadable. - Add regression tests for mutation gaps the reviews found: a restart refused by a real bound listener (not just a stale record), a rocmd recovery rollback when the first manifest write fails, and a custom-host restart that correctly skips the loopback lease. - Add serve_wizard tests covering space-toggle-while-focused and the outstanding-complaint clearing on port-mode cycling. - Give MockServer/http_server a start_on_port() path so the serve-27 e2e scenario's planted collision record is backed by a real listener instead of being demoted to "stopped" by the liveness probe before the collision check fires. Known gap, intentionally left: the rocmd restart rollback branch for a failed record_spawned_supervisor (including its own nested rollback-also-failed path) is still untested. Covering it deterministically would need either a flaky sub-millisecond race or a test-only production seam, both out of scope for this pass. Signed-off-by: Juho Vainio <juho.vainio@amd.com>
Add the serve_port.rs entry to docs/architecture.md and refresh the service_launch.rs entry. Replace the em dash in the Serving tab copy and the en dashes in the port-range messages with ASCII hyphens/colons. Rename the loopback-host test so its name states what it pins. Signed-off-by: Juho Vainio <juho.vainio@amd.com>
|
Did a review pass on the current head (fea5e53) and pushed 3fd7d53 to address the mechanical items. Addressed in 3fd7d53
|
juhovainio
left a comment
juhovainio
left a comment
There was a problem hiding this comment.
I went through this against #239 and the CI results. Two things need fixing before merge; the rest are notes.
Needs fixing
- The PR description has the new scenario numbers wrong. It says serve-26 is the auto-port skip, 27 out-of-range, 28 reserved, 29 untracked. In the diff, serve-26 is out-of-range (ungated), serve-27 is reserved by a managed service, serve-28 is an untracked process, and serve-29 is the auto-port skip. The Verification section repeats the old numbers. The
@id:tags in the feature file are right, so only the text needs correcting. Readers will use those numbers to work out which gated lane covers what. - The "E2E tests (MI300X, current)" check is red (6 of 250 scenarios). Some of it looks unrelated (a vLLM auto-tool-choice scenario, runtime folder nesting, vllm reinstall, and a Lemonade GGUF scenario that hit "Download failed after 6 attempts"). But serve-01 and serve-02 fail with "no 'resolved model' in output", and I could not tell whether that predates this PR or comes from the plan-output change (the new
Port: automatic; ...line /portnow being optional). Please compare those two against a recent main run on the same lane, or rerun and share the evidence. Note serve-27/28/29 are@merge-queueand did not run here, so they have no signal yet.
The branch also now conflicts with main, so it needs a rebase.
Notes (non-blocking)
- The CLI launch lock is unbounded, while #239 asks for a bounded cross-process lock on both sides. The PR discloses this, so "Addresses #239" is accurate but "Closes" would not be.
restart_internal_managed_service_innernow takes that unbounded lock. Please confirm no caller (prune, automation, chat restart) already holds it, since a re-acquire would block forever.LoopbackPortLease::is_heldis pub with no production caller.AUTO_PORT_NOTEis re-exported from the Dash UI crate into the CLI plan output, so a TUI wording change alters CLI output. serve-29 would catch the drift, but the coupling is odd.- Issue story 38 asks for audit records naming the collision reason. I did not see an audit event on the refusal paths in
resolve_serve_port; I could not confirm whether one exists elsewhere.
The port-selection choke point, single cleanup path for failed launches, and message-plus-state assertions in the new scenarios look solid.
| /// (e.g. `apps/rocm`'s `serve_port` tests), where `#[cfg(test)]` here | ||
| /// would not apply. | ||
| #[must_use] | ||
| pub const fn is_held(&self) -> bool { |
There was a problem hiding this comment.
is_held is pub but has no production caller (the doc comment says tests only). Consider gating it as test support, or confirm a test-only pub accessor is intended.
| // exact port out from under the restart. Unbounded, like every other | ||
| // CLI-side acquire of this lock (see `FileLock::acquire_bounded`'s doc | ||
| // comment for why only `rocmd`'s recovery bounds its wait). | ||
| let launch_lock = rocm_core::FileLock::acquire(paths.managed_launch_lock_path())?; |
There was a problem hiding this comment.
This acquires the launch lock unbounded after the confirmed stop. Please confirm no caller (prune, automation, chat restart) already holds managed_launch_lock_path, since a second acquire in-process would block forever.
Signed-off-by: Juho Vainio <juho.vainio@amd.com> # Conflicts: # docs/manual-testing.md
rominf
left a comment
rominf
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · 3fd7d53
This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.
Review — needs work
Full review of the whole change.
Says it "Addresses #239" and does not claim to close it. It implements the CLI port policy, the shared lock, recovery under the lock and the Dash wizard rework from that issue. The description records that the lock is still unbounded on the CLI side.
Every finding below is new this round (found blind at 3fd7d53). None of the seven objections standing from the 331fb47 round still holds; how each was settled is at the end, under What this covered. The two replies of 2026-10-26 (fea5e53, 3fd7d53) were weighed there. The description finding on its own would not hold this change request (the squash commit is built from commit messages, not the description); the other six carry it.
Blocking
- [new] rocmd recovery can overwrite a
rocm services restartof the same service that finished while recovery waited for the lock —apps/rocmd/src/watchers.rs:1521-1535,:1599;apps/rocm/src/main.rs:7297- The watcher reads the record before it takes the lock, then waits up to 5 s (
MANAGED_LAUNCH_LOCK_TIMEOUT). - In that window the CLI restart can take the same lock and publish the same service id as
starting, with new pids. - The re-check under the lock skips the service's own id (
other.service_id != record.service_id). - The loopback lease also succeeds, because the restarted engine binds only after the model loads.
- Recovery then writes its stale copy over the restart's record.
- A fan-out worker reproduced this with a temporary test. After rollback, the restarted run's engine pid was gone, leaving that engine unstoppable. On the success path, a second supervisor targets the same port.
- The race existed before, but this change widens its window and makes "re-check against the disk under the lock" the guard for this class, while exempting the one record that matters.
recovery_ignores_its_own_live_looking_record_on_its_own_portpins the exemption. - Confidence 85 · architectural · Fix: under the lock, re-load this service's own record and refuse without writing unless status, supervisor pid/ticks and engine pid still match the copy that was read.
- The watcher reads the record before it takes the lock, then waits up to 5 s (
- [new] The rollback on a
try_waiterror is a no-op, contrary to its comment —apps/rocmd/src/watchers.rs:1613,:1650-1662,:1794-1811- The comment at
:1565-1569says every failure after therecoveringwrite ("spawn,try_wait, recording the supervisor") restores the prior record. That comment governs: it is the invariant that stops arecoveringrow from reserving the port. recovering_snapshotis taken beforerecord_spawned_supervisorwrites the new supervisor pid, so on thetry_waitpath the pid guard inrollback_recovering_recordmismatches and returnsOk(()).- Disk then stays
recovering, naming a supervisor that was just killed, and keeps the port live until the stale timeout. No test reaches this branch. - Confidence 85 · logic · Fix: snapshot again after
record_spawned_supervisorsucceeds and use that snapshot on thetry_waitpath; add a seam and a test.
- The comment at
- [new] Holding the launch lock until the record is published is untested at every production call site —
apps/rocm/src/main.rs:7297,:7402;apps/rocm/src/service_launch.rs:845,:1172service_launch.rs:372says callers "MUST hold" the lock across allocation and publication.- Mutation run: moving
drop(launch_lock)to before the lease in restart left 73/73service_launch::/restarttests green. - Mutation run: moving it to before the spawn in
start_managed_serviceandrun_attached_serviceleft 176/176 green. - The
concurrent_*tests take the lock themselves, so they cannot see a caller that releases it early. - Confidence 100 · logic · Fix: in the existing spawn seams, assert that
FileLock::acquire_bounded(paths.managed_launch_lock_path(), Duration::ZERO)fails.
- [new] Most of the rollback's field-by-field restore is unexercised —
apps/rocmd/src/watchers.rs:1802-1808- Mutation run: three mutants, each skipping one restore (
restart_count,inference_verified_at_unix_ms,engine_pid), left all 39 targeted tests green. - The tests assert only status,
supervisor_pidand the stop marker. The description says the rollback "restores only the fieldsrestart_managed_servicechanged", so the claim is untested. - Confidence 100 · mechanical · Fix: give
priordistinct values for each restored field inrollback_keeps_a_concurrent_stop_marker_and_restores_the_restarts_fields, and assert each one.
- Mutation run: three mutants, each skipping one restore (
- [new] Three new serve-wizard branches can each be broken with every test still green —
crates/rocm-dash-tui/src/ui/serve_wizard.rs:344,:671,:964-991- Mutation run: dropping
|| !self.managedfromis_customizedpassed 63/63. A foreground-only form then summarises as "Automatic". - Mutation run: changing the "re-state the complaint" arm of
restate_or_cleartoNonepassed. - Mutation run: stubbing
editor_spansso the editor renders empty passed. dash-39 types "nope" but never asserts it is on screen. - Confidence 100 · logic · Fix: add three tests: a foreground-only form summarises as "Customized"; a re-stated port complaint after "99999"; an open editor renders the typed text.
- Mutation run: dropping
- [new] The wizard's changed launch behaviour has no Gherkin scenario —
crates/rocm-dash-tui/src/ui/serve_wizard.rs:368-435,tests/e2e-cucumber/features/dash.feature- The default staged command went from
serve M --engine lemonade --managedtoserve M, so the CLI now picks the engine and the port. - The approval card gains
Port: automatic; endpoint shown after launch. localhost/::1with an automatic port, and any public host, are now refused in the form.- dash-38 and dash-39 cover only the collapsed layout and Escape-cancels-edit, and the description justifies no other gap. AGENTS.md §3 names the selected runtime/engine as user-observable and says a unit test does not discharge it.
- Confidence 85 · architectural · Fix: add a scenario where the approval card shows
serve <model>with no--engine/--portplus the automatic-port line, and one where Hostlocalhoston Auto is refused with "set Port to Custom".
- The default staged command went from
- [new] The description mislabels the scenarios it cites as evidence — PR body Summary and Verification, against
tests/e2e-cucumber/features/model_serving.feature- The description says serve-26 is automatic-port-skip, serve-27 out-of-range, serve-28 reserved port and serve-29 untracked process.
- In the feature file: serve-26 is out-of-range (ungated), serve-27 reserved, serve-28 untracked process, serve-29 automatic-port.
- So "out-of-range ports rejected up front (serve-27)" points a reader at a GPU-gated merge-queue scenario. "the remediation serve-28 points at" is actually serve-27.
- Separately, "README no longer overstates the up-front collision check as loopback-only when
resolve_serve_portruns it on any host" is false.serve_port.rs:149-158returns engine-owned for every custom host before any reservation check. The README text, which says127.0.0.1only, is the correct side, and #239 says the same. - Confidence 100 · mechanical · Fix: correct the description; the code needs no change for this.
Non-blocking
- [new] A restart can report a false timing cause when two live records share a port across hosts —
apps/rocm/src/serve_port.rs:149-158,crates/rocm-core/src/service_record.rs:352-355,apps/rocm/src/main.rs:7298-7312- An explicit port on a custom host (
::1,127.0.0.2) skips the live-record check, which #239 sanctions ("custom-host endpoints retain the existing engine-owned bind/error behavior"). - But
blocks_portis deliberately host-blind. A laterrocm services restartof the original127.0.0.1service therefore fails with "... claimed that port while the restart was waiting for the launch lock", which is untrue: the other service held the port first. - Confidence 85 · logic · Fix: word the refusal without the timing claim.
- An explicit port on a custom host (
- [new] Failed-launch cleanup does not confirm the child died, despite its doc —
apps/rocm/src/service_launch.rs:399-436- The doc says the child is "terminated and confirmed dead first".
- The code ignores the
terminate_verifiedoutcome and writesstatus = "failed", which releases the port, even onTimedOut/Unverified. - Confidence 85 · logic · Fix: keep the reservation when
!outcome.stopped(), or narrow the doc.
- [new] Stale cross-references —
apps/rocm/src/service_launch.rs:1162,:1882-1884,:1966,:790-797,:130-134- Three comments say the failed-launch cleanup lives in or is wired by
serve_cmd.rs. It is the.inspect_errinstart_managed_service(service_launch.rs:832). start_managed_service's rustdoc now describes only the key-cleanup detail.drop_orphaned_endpoint_key_on_already_runningis now also the error-path cleanup.- Confidence 85 · mechanical · Fix: correct the references; rename the helper or widen its doc.
- Three comments say the failed-launch cleanup lives in or is wired by
- [new] Tests pin fixed ports inside the new automatic range —
apps/rocmd/src/watchers.rs:3809(11444),:3985(11455),:4505(0.0.0.0:11460)- These tests now go through the real loopback lease. Mutation run: with 11444 and 11455 held, two restart tests failed.
- That is the hazard this change's own
free_loopback_portdoc (test_support.rs:97-108) describes. - Confidence 100 · mechanical · Fix: use
free_loopback_port().
- [new] The
service_launch.rscopy of the port-release test lacks its twin's retry —apps/rocm/src/service_launch.rs:3076-3092, versusmain.rsrestart_releases_the_leased_port_before_spawning_the_child- It failed once on unmodified code under plain
cargo test, which is how the Windows lane runs. The next six runs passed. - Confidence 100 · mechanical · Fix: copy the bind retry loop.
- It failed once on unmodified code under plain
- [new] The collapsed summary says "Customized" for a form that cannot launch —
crates/rocm-dash-tui/src/ui/serve_wizard.rs:350-364- A probe test with host
localhostand Auto port gavesummary="Customized"whilebuild_args()returnedErr(CUSTOM_HOST_NEEDS_PORT). - The doc promises "the truth about overrides, including a broken hidden one". Launch still refuses, so nothing launches wrongly.
- Confidence 100 · logic · Fix: derive the summary from
validate().
- A probe test with host
- [new] New rocmd user-visible behaviour has no scenario —
apps/rocmd/src/watchers.rs,apps/rocmd/src/mcp.rs- Uncovered: the
server-recovererror event, recovery refusing an occupied port, and MCPlaunch_serverrejecting a missing or malformed port. - Confidence 70 · architectural · Fix: add a scenario, or state the gap in the PR text.
- Uncovered: the
- [new] One permanently refused failed service starves recovery of the others —
apps/rocmd/src/watchers.rs:1380-1414find_recoverable_servicealways returns the first match. Afailedrecord whose port a new serve legitimately took is now refused on every tick, so services after it are never reached.- The first-match pattern predates this change; the never-clearing refusal is new.
- Confidence 70 · logic · Fix: skip candidates inside their backoff window.
- [new] Nothing ties Dash's loopback list to the CLI's —
crates/rocm-dash-tui/src/ui/serve_wizard.rs:84againstis_public_bind_host- The two agree today, but no test would notice drift. The description lists this as deferred.
- Confidence 70 · architectural · Fix: add an
apps/rocmtest asserting everyLOOPBACK_HOSTSentry is non-public.
Decisions for the author
- Custom hosts lose implicit port 11435 — tradeoff
- On one side: a probe that only covers
127.0.0.1cannot honestly vouch forlocalhost,::1or0.0.0.0, so refusing beats guessing. - On the other:
rocm serve --host 0.0.0.0 --allow-public-bindwithout--portworked before this change and is now an error, and scripts break. - It is documented as a breaking change; confirm no compatibility path, such as keeping 11435 for an explicit custom host, is wanted.
- On one side: a probe that only covers
- The lease is released before the engine binds — tradeoff
- Engines cannot inherit a listener, so an untracked process can still take the port between release and bind. That fails late in the engine rather than early.
- #239 accepts this ("an unrelated process can still race the final release/spawn handoff"). The alternative is a bind-retry loop in each engine.
Positive signals
blocks_port/is_livemoved intorocm-coreand are shared by the CLI restart and the rocmd recovery re-check, which removes one twin implementation.AUTO_PORT_NOTEis one constant used by both the CLI plan output and the Dash approval card, so the wording cannot drift.launch_port_argumentis shared by the chat and MCPlaunch_serverpaths, so a malformedportno longer silently becomes automatic.- The rollback merges onto the on-disk record rather than writing the old copy back, so a concurrent stop marker survives.
- The Windows
PermissionDenied(excluded-range) case is handled deliberately in the automatic scan.
Deployment notes
- Breaking:
rocm serve --host <anything but 127.0.0.1>now requires--port. - Omitting
--portnow lands on the first free port in 11435-11535. Tools that assume a second concurrent serve is on:11435will be wrong. Dash chat discovery reads the service registry first, so it is unaffected. - Dash launches no longer force
--engine lemonade; the CLI's engine choice applies. - rocmd recovery may now wait up to 5 s per tick for the launch lock. It also refuses on
127.0.0.1when the recorded port is occupied, and retries after the 30 s backoff.
What this covered
Every file in prw-base...HEAD (1c33248c…3fd7d537, 15 commits).
- Two independent workers read their groups in full and ran mutation checks in the checkout's own
target/:apps/rocmwithrocm-core;rocmdwithdash-tui, which split again into a rocmd worker and a dash-tui worker.
- The e2e features and steps, docs, README and
SKILL.mdpass and the design review were run by the review driver, not by independent workers. Fan-out was capped at 2.
Test verdicts for every added test:
- Fail with the production change reverted: all except four.
recovery_ignores_its_own_live_looking_record…passes on revert, and it pins the first Blocking finding.recovery_is_not_blocked_by_a_stopped_or_failed_record…,restart_on_a_custom_host_skips_the_loopback_leaseandlaunch_server_treats_a_null_port_as_automaticpass on revert by design: they are boundary tests.
- A single branch breaks with tests still green: the lock release point, the rollback field restores, the three wizard branches, and the
try_waitrollback, all listed above.
CI on 3fd7d53:
- Every hosted check is green, including the mock e2e lane, where serve-26, networking-04, dash-38 and dash-39 passed. Windows and Strix Halo WSL2 are green too.
E2E tests (MI300X)failed, but its unexpected failure (therock-next-09, an upstream vLLM/amd-aiter pin conflict) and its xfails match main's MI300X run at 5e4b7f1, so the failure is not caused by this PR.E2E tests (W7900)is still queued: missing evidence, not a defect.- serve-27, serve-28 and serve-29 are
@merge-queue; none of them has run. - Sign-off check: 15 of 15 commits signed off.
Did not run:
- Windows
cfgarms: not type-checked; no mingw toolchain forring/aws-lc-sys. - The prior-changes pass: not run; it would require reading earlier PR discussion.
- History pass: limited to main's drift since the merge base. main's
freeform_planner.rsextraction merges cleanly, and nothing in this change references a moved symbol. - The
blocks_portmutation inrocm-core: not run. - No prompt-injection content found.
The review itself was not reconciled against any PR discussion.
Standing objections from the 331fb47 round. This was a full review, so every file those objections concern was read blind, and the blind pass re-found none of them. After the report above was frozen, each was put to the same reviewer, one at a time, with the author's replies of 2026-10-26. Those verdicts are anchored evidence, not blind findings. Each objection was dropped only because the anchored check and the blind silence agreed:
- networking-04 ordering: dropped.
serve()callsvalidate_port_requestright aftervalidate_bind_host(serve_cmd.rs:546), before model resolution and the GPU check, and networking-04 passed on the hosted E2E lane. - dash-38/39 reading the screen too early: dropped. Both steps now poll (
wait_until_gone/wait_for_screen), and both scenarios passed on the WSL2 and hosted lanes. - Linux-only rocmd test on Windows: dropped. It is now
#[cfg(target_os = "linux")]. All threesupervisor_start_ticks.is_some()assertions in the file are Linux-gated, andwindows-build-and-testis green. - Racy rollback tests: dropped. Both now break the record inside
spawn_override. Five plaincargo test -p rocmd --lib watchers::runs gave 56 passed each, with the named tests listed. The port test's 2 s retry is a timing allowance, so the unretriedservice_launch.rstwin stays a non-blocking item above. - Untested branches (CLI lease release, rocmd self-exclusion, rollback merge, MCP malformed ports): dropped. Each mutation was re-run and each turned a named test red; the MCP cases were checked by reading. The self-exclusion is now tested, but that same test pins the behaviour the first Blocking finding objects to.
- Stale scenario numbers: dropped. Scenarios are now appended as serve-26 to serve-29, and every serve-NN reference in the tree matches. Only the description is still wrong, which is the description finding above.
- Description overclaims (networking-04 timing, "unified" validators): dropped. Both claims now hold:
rocm_core::launch_port_argumentis the shared parser forchat.rsandmcp.rs.
nowycondro
left a comment
nowycondro
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · 51902b6
Review — needs work
Full review of the whole change: the diff between the merge-base (c586618) and PR head (51902b60) across all 28 files — the new serve_port.rs port-allocation policy, the launch-lock transaction in service_launch.rs, rocmd's recovery/restart path in watchers.rs, the shared rocm-core primitives, and the Dash serve-wizard rework. The port-selection module itself (apps/rocm/src/serve_port.rs) is clean and thoroughly unit-tested, and the Dash wizard's auto-port gating correctly uses the same exact-string "canonical loopback" test the CLI uses, not the broader three-spelling loopback list — so the host-validation drift an earlier review round found there looks genuinely fixed. The recovery/restart concurrency logic, however, still has at least one real race and one dead rollback path, both traced directly in the code below, and the PR description itself misstates which new scenario is which.
This PR already carries two standing "changes requested" reviews (from juhovainio and an automated pass) against commit 3fd7d537. I checked: every file touching port allocation, the launch lock, and recovery (serve_port.rs, service_launch.rs, watchers.rs, process.rs, service_record.rs, the Dash crate) is byte-for-byte identical between 3fd7d537 and the current head — the only change since then is a merge of upstream main plus a one-line Cargo.toml version bump. So those reviews' substance has not been addressed at this head, and I independently re-traced two of their core claims from the code myself rather than taking them on faith (see below); I did not re-derive every item in that review's list.
Blocking
-
PR description names the wrong scenario for each new serve-port id —
tests/e2e-cucumber/features/model_serving.feature:350-410vs the PR description's Summary/Verification sections.
The description says serve-26 is the auto-port skip, serve-27 is out-of-range, serve-28 is reserved-by-a-managed-service, serve-29 is held-by-an-untracked-process. The feature file itself has serve-26 = out-of-range (Scenario: serve-26 - Serving with an out-of-range port is refused with a clear reason), serve-27 = reserved by another managed service, serve-28 = held by an untracked process, serve-29 = the auto-port skip. The@id:tags are correct; only the prose is wrong, in the Summary and Verification sections of the description and nowhere else I found.
Confidence 100 · correctness (description-vs-code mismatch) · Fix: relabel the four bullets in the description to match the actual scenario numbers. -
rocmd recovery can silently overwrite a concurrent
rocm services restartof the same service —apps/rocmd/src/watchers.rs(collision re-check inrestart_managed_service_with, theon_disk.iter().find(...)guard just afterFileLock::acquire_bounded).
The watcher readsrecordbefore taking the bounded launch lock, and can wait up toMANAGED_LAUNCH_LOCK_TIMEOUTfor it. Once acquired, the re-check against the freshly-reloaded on-disk records isother.service_id != record.service_id && other.blocks_port(record.port)— it explicitly skips any on-disk record sharingrecord.service_id. That's the right guard against a different service having claimed the port, but it is not a guard against the same service_id having been re-published by someone else (an operator-triggeredrocm services restartfor that same service) while recovery was queued on the lock. In that interleaving, recovery's stale in-memory snapshot (captured before the wait) is still what gets republished asrecovering/running, clobbering the restart's fresher pids/status. I traced this from the code; I did not run a reproduction (no Rust toolchain available in this environment).
Confidence 85 · architectural · Fix: under the lock, re-load this service's own on-disk record and refuse to proceed (defer to the next tick) unless its status/supervisor-pid/engine-pid still match what was read before the lock was requested. -
The rollback after a recovery failure is a silent no-op on the
try_waiterror path, contradicting its own doc comment —apps/rocmd/src/watchers.rs(restart_managed_service_with'stry_waiterror arm callingfail_recovery, androllback_recovering_record's match arms).
The doc comment above therecoveringtransition states every failure after that write "restores this throughfail_recovery". Butrecovering_snapshotis captured beforerecord_spawned_supervisorpersists the new supervisor pid to disk. Ifrecord_spawned_supervisorsucceeds and the subsequentchild.try_wait()then errors,fail_recoveryis called with that stalerecovering_snapshot.rollback_recovering_recordonly applies the restore whencurrent.supervisor_pid == recovering_snapshot.supervisor_pid— but the on-disk record now holds the real supervisor pidrecord_spawned_supervisorjust wrote, which no longer equals the stale snapshot's0. The guard falls through toOk(_) => Ok(()), a no-op: the record is left on disk claimingrecoveringwith a pid that was just terminated byterminate_recovery_supervisor, instead of being restored to the prior truthful status. (It isn't stuck forever —manifest_service_recovery_reason's staleness check will eventually re-trigger recovery for a long-stuckrecoveringrow — but the function's own documented invariant is violated on this path.)try_wait()erroring is rare, which is presumably why this slipped through; it's still a real bug in code that exists specifically to make this transaction safe to interrupt.
Confidence 85 · logic · Fix: take the "current" snapshot for the rollback guard afterrecord_spawned_supervisorruns (or haverecord_spawned_supervisorreturn the post-write state forfail_recoveryto compare against), not before it.
Non-blocking
None beyond what the standing reviews on this PR already list (duplicated loopback-host constant between the CLI and Dash crates, LoopbackPortLease::is_held having no production caller, the CLI-side launch lock remaining unbounded per issue #239's ask) — I did not find anything new in that category.
Decisions for the author
- The CLI's own launch-lock acquisition (
FileLock::acquire) stays unbounded while rocmd's side is bounded — tradeoff. The PR description discloses this as a deliberate, scoped-out choice (adding a new timeout-failure mode for interactive launches). That's a reasonable call for this round, but it is also the condition that makes the recovery/restart race above possible to hit in practice (an interactive restart holding the lock indefinitely blocks recovery's bounded wait, widening the window). Worth factoring into the priority of the follow-up.
Positive signals
apps/rocm/src/serve_port.rs's test suite exercises the WindowsPermissionDenied-as-occupied behavior, the custom-host bypass, and a real-socket scan (binding actual ephemeral listeners rather than only scripted outcomes) — genuine coverage of the platform-specific edge case the change exists to fix, not just the happy path.abandon_failed_managed_launch's pre-spawn-vs-post-spawn cleanup split (delete everything vs. markfailedand keep the log) is exercised end-to-end through the realspawn_managed_engine_childcall site in both directions, including a retry loop in the post-spawn test to tolerate a slow-starting debug binary on loaded CI — more honest than asserting a single race-prone attempt.
Deployment notes
- All CI checks on the current head (
51902b60) report green viagh pr view --json statusCheckRollupandgh pr checksat review time: build/test on Linux and Windows, clippy, the in-repo E2E suite, docs, license/notices checks, and commit sign-off/signature verification. I did not re-derive what each job covers beyond its name/conclusion. - A same-day human review (
juhovainio) flagged that a GPU-gated "E2E tests (MI300X, current)" lane was red (serve-01/serve-02 failing with "no 'resolved model' in output", separate from several scenarios called out as pre-existing flakes) and asked for that to be reconciled against a recentmainrun before merging. That lane isn't visible in the standard check rollup I queried, so I could not confirm its current state either way — flagging it rather than assuming it's resolved. - Breaking change, as the PR description itself discloses:
rocm serve --hostto anything but the exact loopback spelling127.0.0.1now requires an explicit--port; automatic selection no longer silently "works" there (it previously had no real collision proof on those hosts either, so this trades a false sense of safety for an explicit error).
What this covered
Read the full diff between merge-base c5866180 and PR head 51902b60 (28 files, ~6400 added / ~670 removed lines), with particular depth on apps/rocm/src/serve_port.rs, apps/rocm/src/service_launch.rs, and apps/rocmd/src/watchers.rs (read in full, including surrounding non-diff context needed to trace the two blocking findings above), lighter-depth reads of crates/rocm-core/src/process.rs/service_record.rs, the Dash serve-wizard diff, and the test/docs/feature-file changes. Did not run cargo test/cargo clippy/cargo fmt myself — no Rust toolchain or network access for cargo in this environment — and relied entirely on the PR's own CI results (all green at the current head) for that signal. Confirmed via git diff that the port/lock/recovery-relevant files are unchanged since the two standing "changes requested" reviews at commit 3fd7d537, and independently re-traced two of that review's core claims (the recovery/restart race and the rollback no-op) from the code rather than taking them on faith; I did not re-verify every other item in that review's list, and I did not reconcile this report against the discussion/replies on those review threads beyond reading them for context.
- rocmd recovery refuses when its own record changed while waiting for the launch lock - roll back against the post-record snapshot on the try_wait failure path - assert the launch lock is held at spawn in start, attached and restart launches - assert every restored field in the rollback test - cover three serve-wizard branches; derive the collapsed summary from validation - add dash-40 and dash-41 scenarios for the staged command and refused custom host - use free ports instead of fixed ones in rocmd restart tests; retry bind in the service_launch release test Signed-off-by: Juho Vainio <juho.vainio@amd.com>
|
Addressed review 5664062621 in b4d3071 (and 5664888694 where it overlaps). Blocking
Non-blocking picked up
Left as is
Local: |
…vice - reject an explicit --port that the already-running equivalent service is not on - drop the fixed :11435 from Dash chat detection copy and pin its absence - skip recovery candidates inside their restart backoff so one refused service cannot starve the rest - add a Dash/CLI loopback host parity test - correct stale cross-references and the failed-launch cleanup doc; soften the restart collision wording Signed-off-by: Juho Vainio <juho.vainio@amd.com>
|
Follow-up in 4fa920d, plus a corrected PR description.
Not changed: |
nowycondro
left a comment
nowycondro
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · 4fa920d
Review — needs work
Full review of the whole change (29 files, ~6,900 insertions / 666 deletions), diffed c5866180...4fa920db (merge-base with origin/main). This implements the port-collision-safety and Dash-wizard-simplification work described in issue #239 ("Make model serving collision-safe and simplify the Dash serve wizard"), which remains open; the PR's own "Known gaps" section states the cross-process lock-boundedness part of #239 is only partially met (CLI-side launch lock is still unbounded, by deliberate choice, deferred to a follow-up) while the rest of that issue's asks are addressed. The PR does not claim to close #239, so no mismatch there.
Blocking
- The anti-starvation fix this PR is named for has no test proving it actually prevents starvation —
apps/rocmd/src/watchers.rs:1391-1396(find_recoverable_service).
The commit history shows this exact hunk — acontinue(skip this candidate, keep scanning) added where the prior code effectively stopped at the first backoff-contended record — was introduced specifically to fix "one refused service cannot starve the rest." I verified viagit show pr-243-head:apps/rocmd/src/watchers.rsthat thecontinueis really there, and grepped every test callingfind_recoverable_service(find_recoverable_service_prefers_failed_managed_manifest,find_recoverable_service_detects_stale_starting_manifest, the stop-request tests, andcontained_recovery_survives_a_failed_restart_and_backs_off): every single one seeds exactly one managed-service record. With one record,continue(skip and keep scanning) andreturn Ok(None)(stop scanning) are observably identical, so nothing in the suite would fail if this line were reverted toreturn/break, silently reintroducing the starvation bug. Testing this is clearly practical (seed two records: one in backoff, one free) and the gap isn't argued as impractical anywhere.
Confidence 85 · logic · Fix: add a test seeding two managed records — onefailedwithlast_restart_unix_msinsideSERVER_RECOVER_BACKOFF_MS, onefailed/stale with none or an expired stamp — and assertfind_recoverable_servicereturns the second record, notNone.
Non-blocking
- Self-referential doc comment inside
run_attached_service—apps/rocm/src/service_launch.rs:1198.
The comment reads// Mirrors \run_attached_service`'s cleanup: ...but is written *inside*run_attached_serviceitself (the function starts at line 1168); it should namestart_managed_service, which is the sibling function this.inspect_errpattern actually mirrors (confirmed:start_managed_service's own doc, ~line 830, correctly says it mirrorsrun_attached_service, written from the other direction). Misdirects a future reader tracing which function mirrors which. Confidence 85 · mechanical · Fix: change the comment to referencestart_managed_service`. - Module doc on the new port-resolution module overclaims its ordering relative to the endpoint key —
apps/rocm/src/serve_port.rs:7-9.
States the module "turns [a port request] into one concrete, leased port before any manifest, log, endpoint key, or child process exists." Tracedserve_cmd.rs::serve(): the endpoint API key is computed and persisted to disk (serve_cmd.rs:858-859) beforestart_managed_service/run_attached_serviceis ever called, i.e. before the port lease happens — reachable whenever a loopback bind requires an API key. The primitive this doc is modeled on,LoopbackPortLeaseincrates/rocm-core/src/process.rs:534-539, deliberately omits "endpoint key" from the equivalent claim, and aservice_launch.rstest comment (failed_port_reservation_leaves_no_orphaned_endpoint_key) states the opposite ordering explicitly. Not a functional bug — cleanup of an orphaned key on failure is correctly wired and tested — but the doc claim is simply false as written.
Confidence 85 · mechanical · Fix: drop "endpoint key" from the list, or qualify it (e.g. note a public-bind endpoint key may already be staged and is cleaned up on failure). - Garbled double-space run in an operator-facing error message —
apps/rocmd/src/watchers.rs:1557.
"cannot recover managed service {}: its record changed while recovery was waiting for the launch lock"— confirmed viacat -Aon the actual source bytes: 14 literal spaces between "waiting" and "for".recovery_refuses_when_its_own_record_changed_while_waiting_for_the_lockonly asserts.contains("record changed"), so it doesn't catch wording. Visible to an operator reading automation events/logs.
Confidence 85 · mechanical · Fix: collapse to a single space. - Second "no stale port" test assertion doesn't carry the reasoning comment its sibling has —
crates/rocm-dash-tui/src/ui/tabs/chat.rs:652-653.
Six lines above, an identical-shapedassert!(!out.contains("11435"), ...)carries// \rocm serve` leases its port at launch, so no fixed one is promised.— this repo's own AGENTS.md convention ("a promise something will *not* happen" needs the reasoning pinned alongside it) is followed for the first instance and not the second, within the same diff. Confidence 70 · mechanical · Fix: add the same one-line comment (or point at the proving test inserve_port.rs`) to the second assertion.
Decisions for the author
serve_wizard::DEFAULT_HOSTvsrocm-core's canonical-loopback gate has no cross-crate parity test — non-blocking-improvement.
serve_wizard.rs:65hardcodes"127.0.0.1"with a comment that it mirrorsrocm_core::DEFAULT_LOCAL_HOST("TUI-local to avoid the dep"), and gates auto-port onhost != DEFAULT_HOST. The other loopback-list parity question (the one the PR's description says was the actual review finding last round) already has a real cross-crate test (dash_loopback_hosts_match_the_clis_non_public_hostsinserve_port.rs, verified importing the wizard's realLOOPBACK_HOSTS/is_loopback_host). This second, narrower constant has no equivalent guard, so a future change toDEFAULT_LOCAL_HOSTcould silently desync the wizard's auto-port gate with nothing to catch it. Suggest a test analogous to the existing one.- MCP
launch_server's new "non-canonical host needs explicit port" requirement — answered-by-intent.
apps/rocmd/src/mcp.rs:1094-1106nowbail!s whenhostisn't canonical loopback andportis absent, which is a breaking change for any existing automation calling this tool withhost: "localhost"and no port. The PR description's own "Breaking change" section states exactly this rule at therocm servelevel: "rocm serve --hostto anything other than the canonical loopback spelling127.0.0.1... now requires an explicit--port." Sincelaunch_serverultimately constructs arocm serveinvocation, this is the same documented change, just not called out by tool name. Would otherwise have been a non-blocking documentation-gap finding; confirm the breaking-change note is meant to cover the MCP/chat tool surfaces too, or name them explicitly. - "Refuse a differing explicit port on a live equivalent service" (final commit) has no e2e/Gherkin scenario, only a unit test — tradeoff.
Covered byspawn_managed_engine_child_refuses_an_explicit_port_a_live_equivalent_does_not_serveinservice_launch.rsand documented in README.md, but none of themodel_serving.feature/networking.featurescenarios exercise it through the real subprocess the way serve-26..29 do. Plausibly deliberate (it needs GPU plus a reuse-detection setup already complex enough at unit level) — confirm that's the intended coverage level rather than a gap that slipped through.
Positive signals
- The prior review round's four concrete findings on this PR (Dash/CLI loopback-host mismatch, Escape-wipes-the-whole-form, e2e "port" scenarios bypassing the real selection code via a mock, and a leftover unreachable
bail!) all check out as fixed when read blind, independently of the PR's own claims:dash_loopback_hosts_match_the_clis_non_public_hostscross-checks the real loopback sets on both sides;active_overlay_at_rootnow has aneditor.is_none()guard backed by an exhaustive-destructure test that fails to compile if a future field isn't wired in; every new "port" scenario (serve-26..29, networking-04) was traced torun_rocm, which spawns the real compiledrocmbinary, with mocking confined to an unrelated liveness-probe precondition; no strayunreachable!()/duplicatebail!found inserve_port.rs. - Two real concurrency tests force genuine contention with
std::sync::Barrier/real cross-process locks rather than "spawn N threads and hope" (select_gpu_indices_under_launch_lock_blocks_for_the_full_hold_and_then_sees_the_holders_write;bounded_file_lock_is_released_when_the_holder_process_exits, which re-execs a real child process to prove OS-level lock release on exit). - Windows
WSAEACCES/PermissionDeniedhandling in the automatic port scan is explicit, tested on both sides of the#[cfg(windows)]boundary, and tied to a real upstream issue rather than silently ignored. - The CLI's
restart_internal_managed_service_innerand rocmd's independently-writtenrestart_managed_service_withconverge on the same lock → recheck → lease-release-before-spawn ordering, each backed by its own test proving the order (restart_releases_the_leased_port_before_spawning_the_child), giving solid design consistency across the two binaries despite the inherent code duplication that crate-layering (rocmdmust not depend onrocm) forces. - README.md, docs/testing.md, docs/manual-testing.md and docs/architecture.md were all checked line-by-line against the new
--portbehavior and match; new cross-reference links resolve to real files.
Deployment notes
- Breaking change (stated in the PR description, confirmed in code):
rocm serve --hostto any non-canonical-loopback spelling (e.g.localhost) now requires an explicit--port— automatic selection only probes127.0.0.1. Also applies to the MCP and chat-assistantlaunch_servertool surfaces (see Decisions above). - Breaking change: an explicit
--portthat differs from the port an already-live equivalent managed service is on is now refused rather than silently accepted/ignored. - No migration needed for existing on-disk service records;
ManagedServiceRecord::is_live/blocks_portadd no new fields (confirmed no schema drift againstrocm-dash-daemon's mirrored registry type).
What this covered
All 29 changed files read completely against c58661800e6942b1d36fcf34ff1c9ea757773c18...4fa920db0f6e48629eb67a8f3f47b2550eef2afd, fanned out to 8 independent blind workers grouped by coupling: (1) serve_port.rs+serve_cmd.rs, (2) service_launch.rs, (3) main.rs, (4) apps/rocmd/{watchers,mcp,test_support}.rs, (5) crates/rocm-core/{process,service_record,lib}.rs, (6) the Dash serve wizard (serve_wizard.rs+app/mod.rs+tabs/serving.rs), (7) chat-detection copy (chat.rs+tabs/chat.rs+agent/tools.rs), (8) the e2e-cucumber suite and docs/README. Each worker read its files in full plus directly-relevant surrounding/caller code, verified candidates adversarially against the source before scoring, and reported only findings ≥70 confidence; several candidates were investigated and dropped as refuted or pre-existing-and-not-worsened (noted in-line above only where it affected a kept finding's scope). I additionally re-verified the blocking finding and the two highest-confidence mechanical findings directly against pr-243-head myself before including them.
CI taken at face value from gh pr view --json statusCheckRollup, fetched once before the fan-out: all checks (build-and-test, windows-build-and-test, clippy, Test (affected crates), E2E tests, E2E consolidated report, Coverage (rocm-dash crates, ratcheted), CodeQL, Analyze (rust/python/actions), docs/link/license/skill checks) report SUCCESS; not re-run.
Did not run: a dedicated history (git log/git blame) pass across the whole diff — one worker (group 4) did trace the specific blocking finding's introducing commit via git log, but this wasn't done systematically for every file; a dedicated "prior changes on these files" pass (review comments on earlier, unrelated PRs touching the same files) was not performed and is not practical to do blind. Agent-instruction adherence (this repo's AGENTS.md) was checked per file-group by each worker as part of the normal rubric rather than as one separate full-repo pass.
Two process notes for transparency: the shared clone's working tree reflects origin/main's tip (as git clone leaves it, not a PR-head checkout — the PR head was never checked out, only read via git show/git diff against the fetched pr-243-head ref, per the method); two workers initially read stale/wrong file content from that working tree via the Read tool before catching the mismatch and redoing their pass correctly via git show pr-243-head:<path>, which I've accounted for. Separately, the group-3 (main.rs) worker ran gh pr view --json statusCheckRollup once despite being told not to call gh at all; it fetched only check names (no title, body, or comments), self-disclosed the deviation, and nothing in its findings appears influenced by it — noted here rather than silently absorbed.
Not reconciled against any PR discussion, review thread, or the prior nowycondro review on this PR (which exists at an older commit) — none of that was read, by design; this is a from-scratch review of the current diff.
Signed-off-by: Juho Vainio <juho.vainio@amd.com> # Conflicts: # apps/rocm/src/main.rs # apps/rocm/src/service_launch.rs
rominf
left a comment
rominf
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · 4fa920d
This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.
Review — needs work
Full review of the whole change.
Addresses #239 (open). It delivers the port policy and the Dash wizard rework. The description itself says the issue's "bounded lock on both sides" is only half done: the CLI launch path still waits on the lock with no timeout.
Round note: both Blocking findings below are new, found blind at 4fa920d. The first is what carries this change request: it is wrong behaviour this PR's new recovery guard leaves in place, not a test gap. The second, and the two standing (narrowed) items, are test gaps over code that now behaves correctly; on their own they would not hold a change request. Of the six objections standing from the 3fd7d53 round, four no longer hold and two hold only in part — see the end of What this covered. The replies of 2026-10-27 were weighed there.
Blocking
-
[new] Recovery's new check of its own record misses a pending stop and a deleted record, then overwrites both —
apps/rocmd/src/watchers.rs:1550-1560,apps/rocmd/src/watchers.rs:1621- What the check compares. After it gets the launch lock,
restart_managed_service_withre-reads the service's record. It refuses only ifstatus,supervisor_pid,supervisor_start_ticksorengine_pidchanged. - How a stop is missed.
rocm services stopwritesstop_requested_unix_msfirst and does not take the launch lock (apps/rocm/src/main.rs:7020-7029). Status and PIDs stay unchanged until the stop is confirmed. A stop that is in flight therefore passes the check. Recovery then runsrecord.stop_requested_unix_ms = None; record.write()and starts a new supervisor for a service the user is stopping. - How a removal is missed.
if let Ok(current) = load_service_record(...)treats a failed read as "unchanged". A manifest deleted while recovery waited is written again and a supervisor is spawned for it. The rollback in the same file handles a missing manifest explicitly (!manifest_path.exists()). - Why it belongs to this PR. The race existed before, but this PR makes the window wider. Recovery can now wait up to
MANAGED_LAUNCH_LOCK_TIMEOUT(5 s) for the lock; on the base branch recovery takes no lock at all. The new check exists for exactly this class of problem: "writing the stale copy over it". - Test gap.
recovery_refuses_when_its_own_record_changed_while_waiting_for_the_lockchanges only status and PID. It stays green with both gaps present.
Confidence 85 · logic · Fix: also refuse when
stop_requested(¤t)is true; refuse "service was removed" onNotFoundand pass other read errors up. Add a test for each case next to the existing one. - What the check compares. After it gets the launch lock,
-
[new] The new backoff skip in
find_recoverable_servicehas no test —apps/rocmd/src/watchers.rs:1388-1396- This is a new branch: it skips a candidate that is still inside
SERVER_RECOVER_BACKOFF_MS, so one refused service cannot starve every service after it. - No
find_recoverable_service_*test setslast_restart_unix_ms. The only uses are at:3086, which testsmanifest_service_recovery_reason, and:3755, which checks the stamp on the record. Deleting the branch leaves every test green.
Confidence 85 · logic · Fix: seed failed service A with
last_restart_unix_ms = nowand failed service B, then assert that B is returned. - This is a new branch: it skips a candidate that is still inside
-
[standing, narrowed] The production
try_waitrollback arm is untested —apps/rocmd/src/watchers.rs:1676-1695- The no-op this objection reported is fixed: the
try_waiterror arm now rolls back againstrecorded_snapshot, taken afterrecord_spawned_supervisor. - What still holds is the objection's "no test reaches this branch".
rollback_against_the_recorded_snapshot_restores_the_prior_recordcalls the helper directly with a hand-built snapshot. Swapping the call site back to&recovering_snapshotwould leave every test green (reasoned from the code, not run). - Fix: a seam that makes
try_waitfail, or accept the gap and say so in the PR text.
- The no-op this objection reported is fixed: the
-
[standing, narrowed] The CLI restart's lock hold up to record publication is still untested —
apps/rocm/src/main.rs:7411,restart_releases_the_leased_port_before_spawning_the_child- The two
service_launch.rscall sites are now covered:start_and_attached_launches_hold_the_launch_lock_until_the_spawnwould fail if either dropped the lock early. - The restart test asserts the lock is held at the spawn, but makes the spawn fail, so it never reaches
record_restarted_child. Movingdrop(launch_lock)to between the spawn andrecord_restarted_child(across the 200 ms wait andtry_wait) would leave it green. The test is also#[cfg(not(windows))]. - Fix: let the seam spawn succeed and assert the lock is still held when the record is published.
- The two
Non-blocking
-
[new] Error message has about 14 stray spaces inside it —
apps/rocmd/src/watchers.rs:1557- The string literal was wrapped onto a new line without a
\continuation. The message reads"...recovery was waiting for the launch lock". - It reaches users through
server-recoverevents. The test only checkscontains("record changed").
Confidence 85 · mechanical · Fix: add the
\continuation and pin the full sentence in the test. - The string literal was wrapped onto a new line without a
-
[new] Comment points at the function it sits in —
apps/rocm/src/service_launch.rs:1198- The comment says "Mirrors
run_attached_service's cleanup", but it is insiderun_attached_service(line 1168). - The code it mirrors is
start_managed_service's.inspect_err.
Confidence 85 · mechanical · Fix: name
start_managed_service. - The comment says "Mirrors
-
[new]
docs/architecture.mdsays something the code does not do —docs/architecture.md:53- It says
serve_port.rsis "shared by service_launch.rs and the CLI restart path". - In fact
main.rsusesserve_port::only for the clapvalue_parser(line 514) and in tests. - The restart path (
restart_internal_managed_service_inner, nearmain.rs:7320) callsrocm_core::is_canonical_loopback_hostandlease_loopback_port_or_bind_errordirectly. - The code governs here, because it is what actually runs.
Confidence 85 · mechanical · Fix: say the restart path shares the
rocm-corelease primitives. - It says
-
[new] A new test uses a fixed port and goes through the real OS lease —
apps/rocmd/src/watchers.rs:3619recovery_is_not_blocked_by_a_stopped_or_failed_record_on_the_same_portbinds to port21_778. This PR addsfree_loopback_port()to avoid exactly that.- Anything holding 21778 on a developer machine makes it fail falsely.
Confidence 85 · mechanical · Fix: use
free_loopback_port(). -
[new] Daemon recovery's new refusal has no Gherkin scenario, and the PR does not say why (AGENTS.md §3)
- Recovery now leaves the service
failedand logsrestart_managed_service_failedwhen its port is occupied or taken by another service. Users see that state throughrocm servicesand automation events. - serve-26 to serve-29 and networking-04 cover only the CLI launch.
- Whether daemon behaviour counts as "observable by a user of the CLI" is open to interpretation, so this is non-blocking.
Confidence 70 · architectural · Fix: add a simulated-host scenario that holds the port, or state the reason in the PR text.
- Recovery now leaves the service
Decisions for the author
- An explicit port on a custom host skips the managed-service reservation check entirely — non-blocking-improvement
resolve_serve_portreturns early for any non-canonical host (apps/rocm/src/serve_port.rs:149-158), andcustom_host_explicit_port_bypasses_the_loopback_preflightpins that.blocks_portis deliberately host-blind, because a0.0.0.0listener also collides with loopback. Yet--host 0.0.0.0 --allow-public-bind --port 11435next to a live managed127.0.0.1:11435is let through and fails late, at the engine's bind.- Skipping the OS probe on custom hosts is justified. Skipping the record lookup, which does not depend on the host, is not.
- This matches #239's "custom-host endpoints retain the existing engine-owned bind behavior", so confirm it is intended.
- The CLI lock wait is unbounded while the daemon's is bounded — tradeoff
- The description lists it as a known gap. A bounded CLI wait would add a new way for interactive launches to time out; an unbounded one leaves #239's decision half met.
Positive signals
fail_recoveryandrollback_recovering_recordmerge onto the live record, so a stop marker written concurrently survives a rollback. A rollback that itself fails is reported, not swallowed.- The OS-probe lease is released inside the lock immediately before the spawn. Both call sites test this from inside an injected spawn, not just by calling
release()on its own. launch_port_argumentandis_canonical_loopback_hostlive inrocm-core, so the chat and MCPlaunch_servervalidators cannot drift apart.dash_loopback_hosts_match_the_clis_non_public_hoststies the TUI's host list tois_public_bind_host.- The
rocm services stopadvice in the reserved-port error is backed by a real stop (stopping_the_reserving_record_frees_its_explicit_port_for_reuse), not just pinned as wording.
Deployment notes
- Breaking change (stated in the description):
rocm serve --host localhost|::1|<other>without--portis now refused.- When 11435 is busy,
rocm servewithout--portlands on a port above 11435 (11436, or the next free one). - An explicit
--portthat differs from an equivalent running service's port is refused. - Scripts that assume a fixed 11435 change behaviour.
- The PR does not merge cleanly. GitHub reports
mergeable_state: dirty.git merge-treeconfirms conflicts inapps/rocm/src/main.rsandapps/rocm/src/service_launch.rs.- The base has since pulled the managed-service status/liveness helpers out of
main.rs(#746), which touches the code this PR changes. - The resolved merge has not been reviewed.
- The base has since pulled the managed-service status/liveness helpers out of
What this covered
- What was read. The whole diff,
prw-baseea86403…4fa920db, merge-base c586618.serve_port.rsandserve_wizard.rswere read in full.- For the other files: their changed regions, the callers and callees of changed symbols, and the shared contracts in
process.rsandservice_record.rs. - Fan-out was two workers, the operator's cap:
- CLI, core and e2e serving files plus the docs.
rocmd, Dash TUI and the dash e2e files.
- Each worker skipped only unchanged code outside its group: unchanged
watchers.rssupervisor code, the rest ofmcp.rsanddash_steps.rs, andevent_loop.rs. - The review driver did the agent-instruction, history and code-comment passes, the Step 7 design questions and verification itself. None of those were independent passes.
- Test verdicts.
- Every added test fails with the production change reverted, except these deliberate exceptions:
- The negative and positive controls: the not-blocked-by-a-stopped-record and own-record checks,
treats_a_null_port_as_automatic, andrestart_on_a_custom_host_skips_the_loopback_lease. - The regression pins (lock held until spawn; hidden engine command keeps its default port), which check behaviour that already existed.
- The negative and positive controls: the not-blocked-by-a-stopped-record and own-record checks,
- Breaking one branch at a time leaves these green:
- The
find_recoverable_servicebackoff skip. - The gaps in the own-record check (blocking above).
- The
AddrInUseskip in serve-29 is caught, but its reserved-port skip is covered only by unit tests.
- The
- Every other branch is caught by a test of its own.
- These verdicts come from reading the code; no mutation was run.
- Every added test fails with the production change reverted, except these deliberate exceptions:
- What did not run, and why.
- No cargo build or test: free disk was at or near the 40 GB floor this automation keeps, so the whole round, including the settlement below, is static. All findings are from reading and tracing the code.
- The prior-changes pass was not run.
- Hosted CI is green on this head. The self-hosted GPU lanes have not reported, so serve-27/28/29 (
@requires-gpu @merge-queue) have not run for this commit: missing evidence, not a defect.
- Other checks.
- The sign-off check ran: all 17 commits are signed off.
- The leak scan of added lines is clean.
- No prompt-injection text was found.
- Not reconciled. The review itself was not checked against the PR's review discussion.
Standing objections from the 3fd7d53 round. This was a full review, so every file those objections concern was read blind, and the blind pass re-found none of them as worded. After the report above was frozen, each was put to the same reviewer, one at a time, with the author's reply of 2026-10-27. Those verdicts are anchored evidence, not blind findings, and were reached by reading only. An objection was dropped only where the anchored check and the blind silence agreed:
- Recovery overwriting a finished
rocm services restart: dropped for the restart case. The own-record re-check refuses a changed status or supervisor pid, andrecovery_refuses_when_its_own_record_changed_while_waiting_for_the_lockseeds that case. The same check's remaining gaps are the new first Blocking finding above. try_waitrollback a no-op: the no-op is fixed; "no test reaches this branch" still holds, carried above as standing (narrowed).- Lock hold untested: holds only for the CLI restart; carried above as standing (narrowed).
- Rollback field restores untested: dropped.
restart_count,engine_pidandinference_verified_at_unix_msnow get distinct values and are each asserted. - Three wizard branches untested: dropped.
foreground_only_form_summarises_as_customized,committing_a_bad_port_restates_the_port_complaintandopen_editor_renders_the_typed_texteach turn red with their branch broken (reasoned, not run). - No Gherkin scenario for the wizard: dropped. dash-40 asserts the approval card stages
serve demo-modelwith no--engine/--portand the automatic-port line; dash-41 asserts Hostlocalhoston Auto is refused with "set Port to Custom". Both are tagged for the hosted Linux E2E lane, which is green; that these two IDs ran was not confirmed from the run report.
The change request this automation filed at 3fd7d53 is already dismissed, so there is nothing of ours to lift; this round files its own.
Recovery's re-check of its own record after taking the launch lock now also refuses when a stop was requested meanwhile or the manifest was deleted, instead of clearing the stop / resurrecting the record. Adds tests for both and for the find_recoverable_service backoff skip, fixes a wrapped error message, a misnamed comment, an architecture.md claim, and a fixed port in a recovery test. Signed-off-by: Juho Vainio <juho.vainio@amd.com>
|
Addressed review 5679393959 in 61be4b7. Blocking, fixed
Non-blocking, fixed
Deliberately left
Local checks: fmt, clippy ( |
…advanced Signed-off-by: Juho Vainio <juho.vainio@amd.com>
Signed-off-by: Juho Vainio <juho.vainio@amd.com> # Conflicts: # apps/rocm/src/main.rs # apps/rocm/src/service_launch.rs # docs/architecture.md # tests/e2e-cucumber/features/dash.feature
siloteemu
left a comment
siloteemu
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · 2cd8ad6
This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.
Review — needs work
Full review of the whole change.
Addresses #239 without claiming to close it. Its "Known gaps" section says only the daemon side of the lock is bounded, and that matches the code.
Blocking
-
[new] The top of the automatic port range is never tested —
apps/rocm/src/serve_port.rs:182
I changedAUTO_PORT_FIRST..=AUTO_PORT_LASTto..AUTO_PORT_LAST, so port 11535 is never tried, and all 18serve_port::tests still pass. The range tests check the constants and the error message, not the loop.
Confidence 100 · logic · Fix: add a test where every port except 11535 reportsAddrInUseand check that the lease is 11535. Also count the probes in the exhaustion test (there should be 101). -
[new]
blocks_port's port comparison is never tested —crates/rocm-core/src/service_record.rs:394, callersapps/rocm/src/services_text.rs:1072,apps/rocmd/src/watchers.rs:1588
I changedblocks_portto plainself.is_live(), which means any live service on any port blocks. Therocmrestart tests, the wholerocmdsuite and the wholerocm-coresuite still pass. In real use, that change would refuse every restart and every recovery while any other service is running. Every test puts the competing service on the same port.
Confidence 100 · logic · Fix: test that a live service on a different port does not block a restart or a recovery, and add a unit test forblocks_portinrocm-core. -
[new] Restart's lock is never shown to stay held until the record is published —
apps/rocm/src/services_text.rs:1175
I moveddrop(launch_lock)to just after the spawn, beforerecord_restarted_child, and all 31 restart tests still pass.restart_releases_the_leased_port_before_spawning_the_childchecks the lock only at the moment of spawn, and then makes the spawn fail on purpose.
Confidence 100 · logic · Fix: let the spawn override return a live child and check that the lock is still held when the record readsrunning. -
[new] Three rocmd tests stay green with the code they name broken —
apps/rocmd/src/watchers.rs:1445-1450,:1571-1574,:1767
I tried each change below and the fullrocmdsuite stayed green (185 passed):- Backoff skip deleted.
find_recoverable_service_skips_a_candidate_inside_its_backoffstill passes. Records are loaded newest first, sosvc-backoff-b, which was written second, is returned whether or not the skip exists. &recorded_snapshotswapped for&recovering_snapshotin thetry_waiterror arm.rollback_against_the_recorded_snapshot_restores_the_prior_recordstill passes. Its doc comment claims to cover this arm, but it callsrollback_recovering_recorddirectly, and no test reaches that arm.acquire_bounded(…, MANAGED_LAUNCH_LOCK_TIMEOUT)replaced with the unboundedFileLock::acquire. Every rocmd test still passes. The doc comment presents the bound as the feature ("recovery never waits out an interactive launch indefinitely").
Confidence 100 · logic · Fix:
- Make the backed-off record the newest one.
- Add a seam so the
try_waitfailure can be driven throughrestart_managed_service_with, or drop the claim from the doc comment. - Make the timeout injectable and test the "timed out" path, including that the backoff stamp still lands.
- Backoff skip deleted.
-
[new] The wizard's "Customized" summary is untested for an engine-only override —
crates/rocm-dash-tui/src/ui/serve_wizard.rs:341
I replacedself.engine_idx != ENGINE_AUTOinis_customizedwithfalse, and allrocm-dash-tuitests still pass. The summary exists so that a collapsed form does not hide an override, and each test that sets the engine also sets a custom port.
Confidence 100 · logic · Fix: add a test with onlyengine_idxchanged that expectsadvanced_summary() == "Customized". -
[new] A test claims something it never checks, and its comment points at a planning document —
apps/rocm/src/service_launch.rs:3053-3151
The test is named…_blocks_for_the_full_hold_and_then_sees_the_holders_write, but the holder thread only locks and sleeps; it writes nothing. The record thatspawn_managed_engine_childfinds was written by the test before the holder started. The comment at:3060says "per the fix-forward plan", which refers to a planning document rather than to anything in the repository (AGENTS.md §4 keeps planning material out of the repo).
Confidence 85 · mechanical · Fix: have the holder write a conflicting live record while it holds the lock and check that the waiting call sees it, or rename the test. Remove the reference to the plan. -
[new] dash-41 sends two Escapes with nothing in between —
tests/e2e-cucumber/features/dash.feature:706-707, steptests/e2e-cucumber/tests/e2e/dash_steps.rs:524
The repository's own comment atdash_steps.rs:863-872says that "two bare ESC bytes arriving in the same PTY read collapse into a singleKeyCode::Esc". It works around this by waiting for the approval to close between the two keys. dash-41 does not wait, so the wizard can stay open andthe serve wizard is closedthen times out. The same gap means the scenario cannot tell "Escape cancels only the approval" apart from "Escape closes the wizard".
Confidence 80 · logic · Fix: add a step between the two Escapes that waits for the approval to disappear and checks that the wizard is still open. -
[new] Some user-visible refusals have no Gherkin scenario —
apps/rocm/src/service_launch.rs:518-528,apps/rocm/src/services_text.rs:1068-1091,crates/rocm-dash-tui/src/ui/serve_wizard.rs(public-host refusal)
AGENTS.md §3 says: "If the change alters what a user of the CLI can observe — command output, exit codes … — then that behavior must be covered by a Gherkin scenario … a unit test asserting the internal helper does NOT discharge this." Three refusals this change adds have only unit tests, and the PR text gives no reason:- an explicit
--portthat differs from the port a running equivalent service uses (the README now documents this refusal); rocm services restartrefusing when its port is claimed or bound by another process;- the Dash wizard refusing a public host.
serve-24 already shows how a planted record can drive such a scenario without a GPU.
Confidence 85 · architectural · Fix: add scenarios, or explain in the PR text why each one is exempt. - an explicit
-
[new] Existing remediation hints now suggest a command that gets refused —
apps/rocm/src/service_launch.rs:214-217,apps/rocmd/src/common.rs:343-345
Both messages tell the user to runrocm serve --host {host} --allow-public-bind. Becausevalidate_port_request(serve_port.rs:108) now refuses a missing--porton any host other than127.0.0.1, that exact command now fails with a second error. This change caused the breakage, even though neither file is in the diff.
Confidence 85 · logic · Fix: add--port <recorded port>to both hints. -
[new] The manual test for the reserved-port refusal cannot produce the outcome it promises —
docs/manual-testing.md:336-347
The second command serves the same model and engine (qwen, lemonade) on the same port.resolve_managed_port_in_transaction(service_launch.rs:391) returnsAlreadyLivebefore any port check. The new port-mismatch refusal (:518) fires only when the ports differ. So the command reports "already running" and does not refuse. serve-29 andstopping_the_reserving_record_frees_its_explicit_port_for_reuseboth avoid this by using a different model.
Confidence 85 · logic · Fix: use a different model or engine for the second launch, and say why. -
[new] The PR description names the wrong scenarios — PR description vs
tests/e2e-cucumber/features/model_serving.feature,dash.feature
The description calls the new serve scenarios serve-26/27/28/29, but the feature file numbers them serve-28/29/30/31. It says "@id:serve-auto-port-skips-occupied(serve-29)", but that id is serve-31; serve-29 is the reserved-port scenario. It says the staged command and the custom-host refusal "are covered bydash-40anddash-41", but they are dash-41 and dash-42; dash-40 is the Escape-cancels-an-edit scenario.
The description also does not mention thatrocm services restartnow takes the launch lock and can refuse.
Confidence 90 · mechanical · Fix: correct the description; the code is right. -
[standing] The supervisor-kill test passes without a kill —
apps/rocmd/src/watchers.rs:4468-4494(test),terminate_recovery_supervisorat:1840-1850- Standing since the change request at
860e05fe, which said this test never launched a process to kill and asked to "bound the elapsed time so a missing kill cannot pass by waiting out the sleep". The child's test path is now correct (:4473), so that half is fixed. This round's blind pass did not re-find the rest. It was settled by putting the original objection to this round's reviewer after its report was complete: anchored evidence, not a blind finding. - Check run at
2cd8ad65: with the body ofterminate_recovery_supervisorreduced tochild.wait(),terminate_recovery_supervisor_kills_and_reaps_the_childstill passes. It takes 120 s, because it waits out the sleeper's own two-minute sleep. - Fix, unchanged: assert that the call returns well within the sleeper's sleep, for example under about 10 s.
- Standing since the change request at
-
[standing] The reserved-port advice is still not asserted together with its outcome —
apps/rocm/src/service_launch.rs:2846-2917,apps/rocm/src/serve_port.rs:163-177, serve-29 intests/e2e-cucumber/features/model_serving.feature- Standing since the change request at
860e05fe, which said the "stop the service holding the port" advice was checked by its wording only. AGENTS.md requires that "the covering test asserts the message and the resulting state in the same test", and that remediation advice naming a command must "actually clear the condition that printed it". That round also offered a pointer comment as an alternative; AGENTS.md does not allow one, so that alternative no longer applies. This round's blind pass did not re-find the objection. It was settled by putting it to this round's reviewer after its report was complete: anchored evidence, not a blind finding. - Now:
stopping_the_reserving_record_frees_its_explicit_port_for_reuseruns the real stop implementation and shows the port is granted afterwards. Its refusal assertion checks only the "already reserved by managed service …" part, not the advisedrocm services stop <id> --yes, so deleting the advice sentence would leave it green (established by reading, not run). serve-29 still checks the advice by wording only. - Fix: assert the stop hint in that test's refusal assertion, so the advice and its outcome are pinned in the same test.
- Standing since the change request at
-
[standing] The assistant can still stage a serve command the CLI refuses —
apps/rocm/src/chat.rs:2744-2762(called at:2482),apps/rocm/src/serve_port.rs:108,skills/rocm-cli-assistant/SKILL.md:31- Standing since the change request at
860e05fe("The assistant and MCP accept a serve request thatrocm servenow refuses").launch_serverin chat, MCP and Dash now enforces the port rule. The route the model actually uses isrocm_command, andvalidate_chat_rocm_command_safetyhas no port rule. So["serve", …, "--host", "localhost", "--managed"]without--portreaches an approval card, and the CLI then refuses it. SKILL.md line 31 says that call is refused, and AGENTS.md §7 requires assistant behaviour to stay consistent with that skill. No test covers this shape. This round's report lists the same route as non-blocking. As a standing objection it still holds, so it stays blocking. It was settled by putting it to this round's reviewer after its report was complete: anchored evidence, not a blind finding. - Fix: apply the same host/port rule in
validate_chat_rocm_command_safety, and test therocm_commandshape.
- Standing since the change request at
Non-blocking
- [new] Several comments are out of date because of this change (confidence 85):
serve_wizard.rs:1861callsserve-auto-port-skips-occupiedserve-29; it is serve-31.docs/architecture.md:50andmanaged_service_status.rs:17still nameexisting_live_managed_service, which is nowexisting_live_managed_service_inand takes a snapshot.main.rsprune doc: "the only other production call site … isselect_gpu_indices_under_launch_lock" no longer holds, because restart also acquires the lock.service_launch.rs:340-343says the lease is released "at the end ofspawn_managed_engine_child". It is released inlaunch_managed_engine_child_locked, right before the spawn, and the unbound window lasts until the engine binds, not only while the record isstarting.- The doc on
record_spawned_supervisorinwatchers.rslost its summary line and now starts in the middle of a thought. - The
serving_steps.rsdoc placesrefresh_managed_service_runtime_livenessinmain.rs; it is inmanaged_service_status.rs. service_record.rs:366-367callsis_live"the sole definition of live", butservices_text.rs:442-445still has its own copy of the status set.
- [new]
is_canonical_loopback_hostcontradicts its doc —crates/rocm-core/src/process.rs:512-514. The doc says "exact", but the code trims whitespace, and the untrimmed host is still passed to the engine argv. The probe can therefore approve a host spelling that differs from the one the engine binds. Confidence 70 · Fix: drop thetrim(), or normalise the host once inserve(). - [new]
unreachable!in a function whose comment says it never panics —serve_port.rs:154-157. It is safe only whilevalidate_port_requestkeeps refusing exactly this case. Confidence 75 · Fix: return the error withbail!directly. - [new] The new testing.md block splits the public-endpoint auth section —
docs/testing.md:954-970. The "A loopback bind … stays credential-free" paragraphs now read as if they belonged to the port section. Confidence 85. - [new] serve-29's setup step can panic on a listener left over from an earlier scenario —
serving_steps.rs(reserve_default_serve_port). It binds 11435 without callingensure_serve_port_free(), unlike the steps around it, andhttp_server::bindnow panics onAddrInUse. Confidence 70. - [new] The handoff test passes even if its child process never runs —
crates/rocm-core/src/process.rs:1405-1423. libtest exits 0 when its filter matches no test, and the child also returns early when its environment variable is unset. Renaming the helper therefore leaves the parent green. Confidence 70 · Fix: have the child write a marker file and check that it exists. - [new] The two "concurrent" allocation tests take the lock in the test itself and start their threads one after another —
service_launch.rs:2956-3051. They show that the test's own lock serialises the work, not that production code takes it. Production lock-holding is covered bystart_and_attached_launches_hold_the_launch_lock_until_the_spawn. Confidence 70 · Fix: add a start barrier, or drivespawn_managed_engine_childthrough the spawn hook. - [new] The rocmd recovery recheck can still overwrite a concurrent stop or resurrect a removed record —
watchers.rs:1604-1701. Stop and remove never take the launch lock, so a stop marker written between the re-read and the full-record write at:1701is lost. The re-read narrows the window but does not close it. Confidence 70 · Fix: write only the fields restart owns (asrollback_recovering_recordalready does), or have stop and remove take the lock. - [standing] The chat assistant's actual launch route skips the new port checks —
apps/rocm/src/chat.rs:2744-2762.launch_serveris not offered to the model (providers.rs:2059), so its new validation never runs for the model; the model goes throughrocm_command ["serve", …]. As a result,serve --host localhostwith no--portreaches an approval card and is refused only after the user approves. Confidence 70. - [standing] Cleanup can kill the wrong process if a PID is reused —
service_launch.rs:417-423.abandon_failed_managed_launchcaptures the process identity at cleanup time, after the child is already known to be dead (and reaped on Unix). A reused PID would then match and its process tree would be force-killed. Confidence 70 · Fix: capture the identity at spawn and pass it to cleanup, and skip termination when the child is already known dead. - [new] Two of the reworded chat-copy sites are untested —
crates/rocm-dash-tui/src/ui/tabs/chat.rs:214,crates/rocm-dash-tui/src/app/mod.rs:1134. Putting:11435back at either site keeps the tests green. Confidence 70.
Decisions for the author
- Explicit ports on hosts other than 127.0.0.1 skip the live-record check — tradeoff.
resolve_serve_portreturnsengine_ownedbefore it looks atreserved(serve_port.rs:149-159), butblocks_port's doc says reservation is deliberately host-blind.--host 0.0.0.0 --allow-public-bind --port Nwhile a running loopback service holds N publishes a second record and fails late in the engine. Checking records needs no OS probe. The other side: keeping custom hosts entirely engine-owned keeps the policy simple, andcustom_host_explicit_port_bypasses_the_loopback_preflightlocks that choice in. - On Windows the port probe may report a port as free when it is not — tradeoff. The lease is a plain
TcpListener::bind(127.0.0.1:p)withoutSO_EXCLUSIVEADDRUSE, and Windows can allow that bind while another process holds0.0.0.0:p. This was not checked here (Linux host only). Confirm whether a Windows lane exercises it. - How long the launch lock is held — tradeoff. The CLI holds the unbounded lock across
load_managed_services, which probes every ready record, plus the 200 ms startup check. rocmd gives up after 5 s and then waits out a full backoff. If several services are running, recovery may time out regularly. - A restart that loses its port leaves the service stopped — tradeoff. The restart takes the lock only after the stop is written, so a concurrent launch can legitimately take the port in between. The new error text does not say that the service is now stopped.
- The plan line now reads
Port: automatic; …, capitalised, where it used to beport: N— non-blocking improvement. The neighbouring lines are lowercasekey: value, and the wording comes from the dashboard crate (AUTO_PORT_NOTE), which the description already lists as deferred.
Positive signals
resolve_serve_porttakes the bind probe as a parameter, so the scan, reservation and error-kind paths are tested deterministically, with one real-socket test kept as a cross-check.- Only
AddrInUse(andPermissionDeniedon Windows) moves the automatic scan on. Other bind errors stop it rather than quietly picking another port, and tests cover both platforms. launch_port_argumentgives chat and MCP one shared validator, so a malformedportcan no longer turn into automatic selection.
Deployment notes
- Breaking change:
rocm serve --host <anything but 127.0.0.1>(includinglocalhostand::1) now requires--port. Scripts and saved commands that relied on the old 11435 default on those hosts will be refused. - Breaking change: an explicit
--portthat differs from the port a running equivalent service uses is now refused instead of being ignored. - The Dash serve wizard no longer offers
cpu_onlyorgpu_preferred, and no longer pre-selects the recipe's preferred engine.
What this covered
Every file in the change, read in full, plus the callers and callees of the changed lock, lease and record functions in apps/rocm, apps/rocmd and crates/rocm-core; prw-base...2cd8ad65. The code was split across six independent workers by coupling, plus one worker on AGENTS.md adherence and code comments and one on history and earlier PRs. The four merges of main were checked: nothing from main was dropped and there are no conflict markers. Each "Confidence 100" finding was confirmed by changing the code here and re-running the relevant suite (rocm restart tests, serve_port:: tests, the full rocmd, rocm-core and rocm-dash-tui suites).
Did not run:
- The design questions were investigated by the code workers rather than by separate design workers.
- The Gherkin scenarios were not run (no GPU or PTY lane here), and nothing Windows-specific was run.
- CI for this head: all completed checks pass.
E2E tests (MI300X, current)andE2E tests (W7900, current)were still queued, andE2E tests (Strix Halo, Ubuntu, release/nightly, current)were cancelled, so the GPU lanes had not reported. serve-29/30/31 are tagged@merge-queueand have not run for this commit. - Sign-off check: all 18 commits carry a sign-off.
Not reconciled against any discussion on this PR.
nowycondro
left a comment
nowycondro
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · 2cd8ad6
This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.
Review — needs work
Full review of the whole change (base 6ab12083e7dd7901eb54e6a4a5b0fd362f4d8eb8 → head 2cd8ad65032c4162af807b108671e78e9d31bc96, 31 files, +7012/-659).
Addresses issue #239 ("Make model serving collision-safe and simplify the Dash serve wizard"). The PR's own text says the cross-process lock-boundedness decision from #239 is only partially met (the CLI's own launch path intentionally stays unbounded, left for a follow-up) and lists several other items as deliberately deferred (a services-dir read error that fails open, AUTO_PORT_NOTE's cross-crate placement, a PID-reuse cleanup race, E2E harness polling gaps, the MCP stdout-only endpoint); the remaining implementation/testing decisions in #239 are otherwise addressed by this PR.
Blocking
-
A brand-new user-facing refusal ("explicit
--portdiffers from a live equivalent service") has no Gherkin scenario, only a unit test —apps/rocm/src/service_launch.rs:516-528(the newbail!, confirmed wholly new in the diff),apps/rocm/src/service_launch.rs:1507(spawn_managed_engine_child_refuses_an_explicit_port_a_live_equivalent_does_not_serve, the only test). Grepping bothtests/e2e-cucumber/features/model_serving.featureandnetworking.feature(and their step definitions) for this message turns up nothing. This repo's ownAGENTS.md§3 states: "User-observable behavior needs a scenario, not only a unit test... a unit test asserting the internal helper does NOT discharge this." This specific refusal is exactly that class of change — a new CLI error message with a concrete remediation command, reached by an ordinaryrocm serveinvocation. The PR description's extremely thorough "Review follow-up" list (which itemizes dozens of smaller test additions) never mentions one for this case either.
Confidence 85 · logic · Fix: add a Gherkin scenario (re-serve the same model/engine with a different explicit--port), mirroring the siblingserve-explicit-port-reservedscenario's shape and GPU gating, or at minimum cross-reference the unit test the way that scenario's doc comment does. -
Doc comment still names a function this very PR renamed —
apps/rocm/src/managed_service_status.rs:17reads"...(existing_live_managed_service), ...", but this PR renames that function toexisting_live_managed_service_in(changed signature: now takes&[ManagedServiceRecord]instead of&AppPaths) — confirmed the old name exists nowhere else in the tree. This repo's ownAGENTS.md§5: "when a statement... becomes false, grep the same surfaces for the old claim before pushing."
Confidence 100 · mechanical · Fix: update the comment toexisting_live_managed_service_in.
Non-blocking
-
README's breaking-change paragraph has a sentence fragment —
README.md:846-849: "--hostto anything other than the default loopback address (localhost,::1,0.0.0.0, or any other address) requires an explicit--port..." has no verb attaching--hostto the rest of the sentence (every neighboring sentence in the same paragraph is a complete sentence; this one alone drops it).
Confidence 85 · mechanical · Fix: e.g. "Passing--hostas anything other than..." -
Non-interactive serve-plan output capitalizes the automatic-port line inconsistently with its sibling lines —
apps/rocm/src/serve_cmd.rs:853-859printshost: {host}andport: {port}in lowercase, but the automatic-port branch splices inserve_port::AUTOMATIC_PORT_DISCLOSURE("Port: automatic; endpoint shown after launch", capitalized, defined incrates/rocm-dash-tui/src/ui/serve_wizard.rs) into the samekey: valuelisting. This is deliberate string-sharing with the Dash wizard (confirmed by the PR's own description: "the wizard's pin of the same constant points at that assertion") so it isn't drift, but the resulting plain-text block reads inconsistently.
Confidence 70 · mechanical · Fix: give the plain CLI listing its own lowercase-prefixed line, separate from the Dash-facing constant.
Decisions for the author
-
The freeform chat
rocm_commandpath doesn't enforce the new host/port rule the structuredlaunch_servertool enforces — tradeoff.apps/rocm/src/chat.rs'svalidate_chat_rocm_command_safety(servearm, ~2744-2762) lets a chat-issued{"args":["serve","qwen","--host","localhost","--managed"]}with no--portpass its own pre-check (sincelocalhostis loopback-for-auth-purposes), only to be refused later by the real CLI with a less actionable error — already anticipated in a comment nearchat.rs:2250-2253. Tightening parity costs an extra validation line on the deliberately-thinner freeform escape hatch; leaving it accepts a worse error message in a rare combination. No test exercises this exact path either way. Confirm the current behavior is still the intended tradeoff. -
rocmd's recovery now runs its bounded lock-wait-and-recheck transaction synchronously inside the daemon's single-threaded async event loop — tradeoff.
apps/rocmd/src/watchers.rs(restart_managed_service_with'sFileLock::acquire_bounded(..., MANAGED_LAUNCH_LOCK_TIMEOUT)at ~1571-1574) runs with nospawn_blocking/block_in_place, insiderun_daemon'stokio::select!(apps/rocmd/src/service.rs:493-520), on a runtime built vianew_current_thread()for the real autostart path — so under lock contention this can stall the daemon's shutdown-signal/webhook handling for up to ~5s plus the existing 200ms post-spawn sleep. This is the cost of the new correctness guarantee (recovery must re-validate under the same lock a concurrentrocm serveholds) weighed against daemon responsiveness, and isn't discussed in the PR text. Confirm the tradeoff is acceptable, or consider moving the transaction off the event-loop thread. -
crates/rocm-dash-tui/src/ui/serve_wizard.rs'sDEFAULT_HOST/DEFAULT_PORTare hand-mirrored literals with no cross-crate tie-test — non-blocking improvement.LOOPBACK_HOSTShas a regression test pinning it againstrocm_core/rocm_engine_protocol(dash_loopback_hosts_match_the_clis_non_public_hostsinapps/rocm/src/serve_port.rs), butDEFAULT_HOST/DEFAULT_PORT(serve_wizard.rs:65-66) have no analogous assertion againstrocm_core::DEFAULT_LOCAL_HOST/DEFAULT_LOCAL_PORT. Today's values match; a future edit to either side would drift silently. Suggest adding the missing tie-test (would need making the constants visible enough to import). -
Chat/Dash's local-engine auto-detect message was broadened without widening the underlying fallback probe — answered-by-intent.
crates/rocm-dash-tui/src/app/mod.rs:1134,1164andcrates/rocm-dash-tui/src/ui/tabs/chat.rs:65,211-214change the detection message from "rocm serve :11435" to the generic "a rocm serve service", but the actual fallback probe (crates/rocm-dash-tui/src/llm.rs'sdetect_local_endpoint/ROCM_SERVE_ENDPOINT, untouched by this PR) still only checks the fixed legacy port 11435. Read alone this would be blocking (a message implying broader detection than the code performs, perAGENTS.md§3's "message asserted together with the behavior" rule) — but two independent passes confirmeddetect_local_chatis registry-first and correctly finds any managed launch regardless of which port auto-selection picked; only a foreground (managed=false) serve landing off 11435 falls through to the narrower legacy probe. Issue #239 answers this directly: "The managed service registry is already the authority for dynamic endpoints used by chat and telemetry. The established port remains useful as the first automatic candidate and as a legacy discovery fallback, but new managed services must not depend on that port being unique." Would otherwise have been blocking. Confirm the residual foreground-serve gap is still acceptable now that automatic selection makes "landing off 11435" the common case rather than the rare one.
Positive signals
- The port-lease TOCTOU design actually holds a real OS socket (
LoopbackPortLease/lease_loopback_port) and releases it only immediately before the child spawn, under the shared lock — proven not just by in-process assertions but by a genuine cross-process test (bounded_file_lock_is_released_when_the_holder_process_exits, a real second process) and bylaunch_releases_the_leased_port_before_spawning_the_child, which injects a spawn override to assert the socket is actually rebindable at the exact moment production would spawn. - Lock acquisition/release ordering across the whole launch transaction (port resolution → record publication → lease release → spawn) was independently traced for every return path (idempotent reuse, every
bail!, success) by two separate reviewers, with no double-acquisition and no path that releases the lock before publication — backed by real two-thread (concurrent_auto_transactions_publish_distinct_ports,concurrent_equivalent_transactions_converge_on_one_record) and two-process concurrency tests. - rocmd's recovery re-validates three independent things under the same lock before resuming a service (the port is still free, its own record hasn't changed, no concurrent stop/removal landed) — each covered by a dedicated test that genuinely races a second thread/competing record against the held lock, not merely asserted against static state.
- The Dash wizard keeps "canonical loopback for auto-port eligibility" and "loopback-ish for Dash's public-bind gate" as two deliberately separate predicates, each tied to its real CLI/engine-protocol counterpart by an explicit cross-crate regression test — directly addressing the conflation risk this exact kind of change invites.
- Docs (README,
docs/architecture.md,docs/testing.md,docs/manual-testing.md,skills/rocm-cli-assistant/SKILL.md) and the clap--porthelp text were all updated in the same change and cross-checked phrase-for-phrase against the actual error strings — no stale claims found beyond the two blocking items above. - Windows
PermissionDenied/WSAEACCES-as-occupied handling in the automatic-port scan is correctlycfg-gated in both directions, tested in both directions, and confirmed to actually execute on thewindows-build-and-testCI job rather than being silently skipped.
Deployment notes
- Breaking change, as the PR itself documents:
rocm serve --hostto anything other than exactly127.0.0.1(includinglocalhost,::1,0.0.0.0) now requires an explicit--port; automatic selection only probes the canonical loopback host. - An explicit
--portthat differs from a running equivalent service's existing port is now refused rather than silently ignored (see the first Blocking item for its test-coverage gap). - rocmd's recovery/restart path now also contends on the same cross-process
managed_launch_lock_paththe CLI's launch path uses (bounded to 5s on the daemon side only, per the PR's own disclosed "Known gaps") — operators relying on fast automatic recovery under heavy concurrent launch load should be aware recovery can now wait up to ~5s before giving up and reporting a backoff. - CI at the pinned head commit: all non-GPU gating checks (build, full workspace test, clippy
-D warnings, fmt, lint, docs build, commit-signatures, coverage ratchet, and the mock-lane "E2E tests" job that runs the newserve-28/networking-04scenarios) are green, taken at face value frompull_request_read. The self-hosted GPU E2E lane (which exercises the@merge-queue-gatedserve-29/serve-30/serve-31scenarios) was in a mixed, still-settling state at review time — Windows and WSL2 GPU lanes green, two Linux GPU lanes marked "cancelled" rather than failed, one lane in progress, one queued. Worth confirming that lane reaches a clean conclusion before merge; not re-derived or second-guessed here.
What this covered
Full review of the whole change. Fan-out across 7 independent file-coupling groups, each reading its files completely plus the callers/callees needed to judge them in place, covering all 31 changed files: (1) crates/rocm-core/src/process.rs, service_record.rs, lib.rs + apps/rocm/src/serve_port.rs; (2) apps/rocm/src/service_launch.rs; (3) apps/rocm/src/main.rs, serve_cmd.rs + README.md, docs/architecture.md, docs/testing.md, docs/manual-testing.md, skills/rocm-cli-assistant/SKILL.md; (4) apps/rocm/src/managed_service_status.rs, services_text.rs, chat.rs; (5) apps/rocmd/src/watchers.rs, mcp.rs, test_support.rs; (6) crates/rocm-dash-tui/src/ui/serve_wizard.rs, app/mod.rs, ui/tabs/serving.rs, ui/tabs/chat.rs, agent/tools.rs (reviewed independently twice; both runs converged); (7) the full e2e-cucumber suite (feature files, step definitions, mock/HTTP test servers). Diffed base 6ab12083 against the pinned head 2cd8ad6 in a read-only scratch clone (git fetch-only, PR branch never checked out as a working tree).
The driver additionally ran: impact analysis (independently re-derived the two Blocking findings by direct grep/read, converging with the fan-out workers who found the same); an agent-instruction-adherence pass against this repo's own AGENTS.md (read in full; §§3, 5, 6, 8 applied throughout, cited inline above); a design-review synthesis of the cross-cutting questions the workers raised (custom-host reservation-list scope, the rocmd blocking-lock tradeoff, cross-crate constant duplication); and a history pass (git log --oneline on the core touched files, commit messages only).
Did not run: cargo test / cargo clippy / cargo xtask e2e — no Rust toolchain was available in this sandbox (confirmed via which cargo in every worker and in the driver); CI's own reported results were used in their place (see Deployment notes) and taken at face value rather than re-derived. The "prior changes" pass (review comments on earlier changes to these files) was not run: this PR's own prior review threads/comments were deliberately not read anywhere in this process, per this method's blind-review rule. Only the PR's own description and the issue it names (#239) were read, and only after every finding above was already settled. While confirming no duplicate review existed at the pinned head from the login this task named, one other human review was observed to exist at this same head commit as bare metadata (login/state/commit only) — its content was not read and played no part in anything above. Not reconciled against any discussion thread on this PR.
… scenarios - serve_port: test the last port of the automatic window and count probes; return an error instead of unreachable! for custom-host automatic requests. - Tests that fail when their target is broken: blocks_port port comparison (rocm-core and restart), restart holding the launch lock until the record names the child, rocmd recovery lock bound, backoff skip, supervisor kill timing, engine-only Customized summary, stop hint asserted with its outcome. - chat: apply the serve host/port rule to the rocm_command route. - Remediation hints for public-host respawn now include --port. - e2e: dash-41 waits between Escapes; add serve-32, serve-33, dash-43. - Fix stale comments and the manual-testing reserved-port example. Signed-off-by: Juho Vainio <juho.vainio@amd.com>
|
Addressed review 5710060590 in the commit pushed on top of 2cd8ad6 (new commit, no history rewrite). Each item maps to a finding. Blocking, fixed
Non-blocking picked up: the Left unchanged on purpose
Verification: |
nowycondro
left a comment
nowycondro
left a comment
There was a problem hiding this comment.
Review — needs work
Full review of the whole change (no prior review from this login exists on this PR at any commit, so nothing was narrowed).
Implements most, but explicitly not all, of #239 ("Addresses #239. The cross-process lock-boundedness decision is only partially met (see Known gaps); the other implementation/testing decisions in that issue are satisfied" — confirmed accurate against the issue's full text, retrieved directly).
Blocking
-
No CI has reported anything for the exact reviewed commit —
a8daaee083c5f8404a40252c05586ed0fe8527b0
gh/API checks (check-runs,status) show only areadthedocsdocs-build success for this SHA;ci.yml,codeql.yml, ande2e-selfhosted.ymlshow zero runs against it, even though all three ran reliably (and green) against every prior commit on this branch within ~15 minutes of each push (e.g.2cd8ad65…, pushed ~09:30, fully green by ~09:49). Over an hour has elapsed since this commit landed with no run appearing. The PR's own "Verification" section listscargo test --workspace --all-targets,cargo clippy … -D warnings,cargo fmt --all -- --check, andcargo test -p e2e-cucumber --test e2e --no-runas done, but these are self-reported local runs with no independent corroboration for this commit.
Confidence 85 · architectural · Fix: re-trigger/confirm CI goes green on this exact SHA before merge; investigate why the workflows didn't fire (push without a correspondingpull_requestre-sync event, branch-protection/App permission hiccup, etc.). -
The chat/TUI "detect a local engine" convenience probe still assumes
rocm serveis always on port 11435, which this PR makes false in exactly the collision scenario it exists to handle —crates/rocm-dash-tui/src/llm.rs(LOCAL_ENDPOINT_CANDIDATES,detect_local_endpoint, ~lines 219-225),crates/rocm-dash-tui/src/skills.rs:30(ROCM_SERVE_ENDPOINT = "http://localhost:11435/v1")
Neither file has any diff in this PR.detect_local_endpoint()is a raw concurrent-TCP-probe fallback ("used by the TUI's in-app 'detect a local engine' action, and by chat startup, so the user need not run the CLI skill first") that is separate from the managed-service-registry consumers (chat.rs,services_text.rs), which this review confirmed do correctly read the resolved port from the service record. Before this PR, if port 11435 was occupied,rocm servesimply failed, so the probe correctly finding nothing matched reality. After this PR,rocm servenow succeeds on a different port (e.g. 11436) in that same situation, and this probe will silently report nothing running even though a server is live. Issue #239's own "Further Notes" state this requirement directly: "the established port remains useful … as a legacy discovery fallback, but new managed services must not depend on that port being unique," and its Testing Decisions require registry consumers to "continue to use each service record's endpoint and port rather than the established default" — a requirement this one fallback path does not meet, and which is not listed among the PR's disclosed "Known gaps."
Confidence 85 · architectural · Fix: either havedetect_local_endpoint()consult the managed-service registry (or scan the full11435-11535range) for a liverocm serve, or explicitly document the probe as default-port-only in its own doc comment and in the PR's Known Gaps.
Non-blocking
apps/rocm/src/chat.rs'slaunch_servertool-call argv builder re-derivesportindependently of the already-validated value, instead of reusing it, unlike itsapps/rocmd/src/mcp.rstwin —chat.rs(rocm_chat_tool_requested_args, re-parses raw JSON viaobject.get("port").and_then(serde_json::Value::as_u64)) vs.validate_chat_launch_server_tool_call(computes the same value viarocm_core::launch_port_argument).mcp.rscomputes the value once and reuses it. Currently masked — every reachable call path validates before building argv — but it's latent duplication of exactly the logic this PR's own new test (chat_tool_call_rejects_a_malformed_port_instead_of_letting_it_fall_through) guards against, so a future call path or validator change could silently reopen the bug this PR fixes.
Confidence 70 · logic · Fix: thread the validatedOption<u16>through to the argv builder, mirroringmcp.rs.
Decisions for the author
- Port reservation is host-blind (same port number on two different interfaces is always treated as a collision) — tradeoff
Consistently applied acrossManagedServiceRecord::blocks_port,serve_port.rs's explicit-port check, and the restart-side re-check inservices_text.rs, and documented as deliberate at the type level. Pro: simpler model, no false negatives across host aliasing. Con: two services genuinely bound to the same port on different interfaces can't coexist even though no real conflict exists. Confirm this is still the intended invariant. - The CLI's own
rocm servelaunch-lock acquisition (FileLock::acquire) has no timeout, unlike rocmd's boundedacquire_bounded— answered-by-intent
PR's "Known gaps": "Issue #239 asked for this lock to be bounded on both sides; only the daemon side is today. Bounding the CLI side is left for a follow-up, since it would add a new timeout-failure mode for interactive launches that's out of scope for this round." Would otherwise be a blocking design concern (an interactive launch could hang indefinitely behind a stuck lock holder). Confirm that's still the plan. apps/rocm/src/serve_port.rsre-exportsAUTO_PORT_NOTEfromrocm-dash-tuirather than the reverse (a CLI-crate user-facing string's authoritative definition lives in the TUI crate) — answered-by-intent
PR's "Known gaps" lists "the TUI'sAUTO_PORT_NOTEre-export" explicitly among items "reviewed and intentionally left as-is." Not acargo xtask check-crate-edgesviolation (apps/rocmalready depends onrocm-dash-tuifor thedashsubcommand). Confirm the direction is still acceptable, or consider relocating the shared constant torocm-core.- The Windows-only fast-fail arm in
spawn_managed_engine_child(service_launch.rs) has no Windows-specific unit test isolating it — non-blocking-improvement
It's reached only by a cross-platform test relying on Windows CI running the full workspace suite (windows-build-and-testdoes runcargo test --workspace --all-targets, confirmed in.github/workflows/ci.yml) — but see the CI-gap finding above: that job hasn't actually run against this commit yet.
Positive signals
LoopbackPortLease::release(self)consumesself, preventing reuse-after-release by construction rather than by convention — good type design.- Recovery rollback (
rollback_recovering_record) restores only the fieldsrestart_managed_servicechanged viaupdate_supervised_record, instead of overwriting the whole record, specifically to avoid discarding a concurrent stop marker — verified by a dedicated test. - Near every new test across all six file groups reviewed was independently mutation-traced (by hand, file-by-file) and found to satisfy both "fails if the production change is reverted" and "fails if one specific branch is broken alone" — unusually thorough test discipline for a PR this size.
- AGENTS.md's doc-comment/behavior-claim sync requirements (§5) are met everywhere checked: README.md, docs/testing.md, docs/manual-testing.md, and SKILL.md all got the matching updates in the same change, and existing doc-comment claims that this PR touches (e.g. the launch-lock "no other acquirer" comment in
main.rs) were updated in lockstep with the new code.
Deployment notes
- Documented breaking change, confirmed consistent in code:
rocm serve --hostto anything other than exactly127.0.0.1(includinglocalhost,::1) now requires an explicit--port. - Do not merge until CI has actually reported green on commit
a8daaee083c5f8404a40252c05586ed0fe8527b0itself (see Blocking) — the branch's history of green runs on earlier commits is not evidence for this one.
What this covered
Full diff, merge-base 6ab12083e7dd7901eb54e6a4a5b0fd362f4d8eb8 → PR head a8daaee083c5f8404a40252c05586ed0fe8527b0 (32 files, +7456/-667). All changed files read completely: six coupled-file-group workers covered the port-leasing core (serve_port.rs, process.rs, service_record.rs, lib.rs), the CLI launch path (service_launch.rs, serve_cmd.rs), the rocmd daemon recovery path (watchers.rs, mcp.rs, common.rs, test_support.rs), the Dash TUI wizard (serve_wizard.rs and its supporting files), CLI status/text rendering (managed_service_status.rs, services_text.rs, chat.rs, main.rs), and the e2e Gherkin suite; two further full-diff passes covered agent-instruction-file (AGENTS.md) adherence plus code-comment compliance, and version history (git log/git blame). The docs-only group (README.md, docs/architecture.md, docs/manual-testing.md, docs/testing.md, skills/rocm-cli-assistant/SKILL.md) was read directly by the driver. Issue #239 was retrieved and read in full for Step 9.
Did not run: no Rust toolchain is available in this sandbox (no cargo) — no test, clippy, or fmt check was executed; all Rust-logic and test-quality judgments are from static reading and hand-traced mutation reasoning, not execution. No GPU is available, so the three @requires-gpu @merge-queue e2e scenarios were read statically only. The "prior changes" source-reading pass (whether review feedback on earlier changes to these files applies here) did not run — there is no practical, bounded way to retrieve historical review comments on merged PRs touching these files distinct from this PR's own review discussion, which this method does not read.
Disclosure on blindness: per-group workers were given a short list of specific claims drawn from the PR's own description, explicitly framed as "verify this against the code, don't trust it," rather than as established fact — a pragmatic compromise, since the description was unavoidably in the driver's context from the GitHub API call needed to resolve the PR's head SHA before any review work began. Every finding reported above was independently confirmed against the code (file:line evidence) by the driver, not merely accepted from the description.
Not reconciled against any discussion on this PR (review comments, prior review threads) — this review did not read them.
Signed-off-by: Juho Vainio <juho.vainio@amd.com> # Conflicts: # docs/architecture.md
Summary
rocm serve --portchoose the first available loopback port in11435..=11535, while preserving exact explicit-port behaviorprocess_is_running, and fails fast with the startup log instead of reporting a false "started" (the#[cfg(windows)]arm ofspawn_managed_engine_childinapps/rocm/src/service_launch.rs); covered on that lane bya_failed_spawn_past_the_first_write_terminates_the_child_and_keeps_a_failed_record@requires-gpu @merge-queue), an explicit port held by an untracked process refused with a plain bind-conflict message (serve-28,@requires-gpu @merge-queue), and automatic serving skipping an occupied default port (serve-29,@requires-gpu @merge-queue), and a non-canonical host without an explicit port rejected before any engine starts (networking-04)Breaking change
rocm serve --hostto anything other than the canonical loopback spelling127.0.0.1(including aliases likelocalhost) now requires an explicit--port. Automatic port selection only probes the canonical loopback host, so it previously had no way to prove a chosen port was actually free on another host. Covered by@id:networking-custom-host-needs-explicit-port(networking-04). An explicit--portthat differs from the port an equivalent running service is already on is now refused instead of silently ignored.Review follow-up
LocalHostand[::1]are rejected@id:serve-auto-port-skips-occupied(serve-29) holds127.0.0.1:11435, invokes the realrocm servecommand, and asserts the reported endpoint advances to11436bail!mainserver-recover/errorevent with a backoff stamp, instead of aborting the rest of that watcher tick for every other managed servicerocm serveleaves no orphaned endpoint-key file behindLoopbackPortLease::is_heldstayspubdespite having no production callerservice_launch::spawn_managed_engine_child,watchers::restart_managed_service), proving the lease is released before the child/supervisor spawns, not just thatrelease()works in isolationblocks_porthas a recovery test and a restart test asserting astopped/failedrecord on the same port never blocks reusePort: automatic; endpoint shown after launch) is now asserted inassert_next_automatic_port, and the wizard's pin of the same constant points at that assertionrestart_managed_servicechanged, throughupdate_supervised_record, instead of overwriting the whole record (which could discard a concurrent stop marker)launch_serverport validators (apps/rocmd/src/mcp.rs,apps/rocm/src/chat.rs) so a present-but-invalidportis rejected the same way from bothPermissionDenied(WSAEACCES, e.g. Hyper-V/WSL/Docker-excluded ranges) the same as a plain bind conflict and advances to the next candidate, instead of aborting the whole scan@id:serve-explicit-port-in-use-by-untracked-process(serve-28,@requires-gpu @merge-queue): rejecting an explicit port an untracked process holds now has a scenario, alongside its sibling serve-27; traced the production call chain (serve_cmd.rsresolves GPU indices beforeservice_launch.rsever reachesresolve_serve_port) to confirm it does need@requires-gpulike serve-28, not the lighter gating the review tentatively suggestedrocm services stopremediation serve-27 points at is now proven by a real stop:stopping_the_reserving_record_frees_its_explicit_port_for_reusegoes throughstop_internal_managed_service_with(madepub(crate)for this) instead of writingstatus = "stopped"to disk itselfserve_wizard_editor_handles_escape_before_overlay_back_outnow drives Escape through the real event handler before asserting state, instead of ending on a query that cannot fail--port, matching the argv it actually sendsdash-40anddash-41; a parity test inapps/rocmties Dash's loopback host list to the CLI'sis_public_bind_hostKnown gaps
rocm servelaunch path still acquires the shared launch lock without a timeout (FileLock::acquire), while rocmd's recovery/restart path uses a bounded acquire (FileLock::acquire_bounded). Issue Make model serving collision-safe and simplify the Dash serve wizard #239 asked for this lock to be bounded on both sides; only the daemon side is today. Bounding the CLI side is left for a follow-up, since it would add a new timeout-failure mode for interactive launches that's out of scope for this round.AUTO_PORT_NOTEre-export, and the MCP stdout-only endpoint — reviewed and intentionally left as-is.Decisions for the author
live_managed_services'sload_managed_services(paths).unwrap_or_default()still fails open on a services-directory read failure, while CLI restart and rocmd recovery fail closed on the same error. This is unchanged from the base and intentional: reviewed again this round and the answer still stands as given previously (see Known gaps).Verification
cargo test -p rocm --bin rocmcargo test -p rocm-core --libcargo test -p rocmd --libcargo test -p rocm-dash-tui --libcargo test -p e2e-cucumber --test e2e --no-runcargo test --workspace --all-targetscargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkThe
@id:serve-auto-port-skips-occupied(serve-29),@id:serve-explicit-port-reserved(serve-27) and@id:serve-explicit-port-in-use-by-untracked-process(serve-28) scenarios are all tagged@requires-gpu @merge-queue. They compile locally; live execution is delegated to the merge-queue GPU lane because this WSL environment has no usable ROCm GPU.Addresses #239. The cross-process lock-boundedness decision is only partially met (see Known gaps); the other implementation/testing decisions in that issue are satisfied.