Repository navigation
fix(serve): make model serving ports collision-safe - #243
michaelroy-amd wants to merge 20 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>
advanced_summary()'s doc comment promised a "needs attention" summary for any broken hidden value, but only a bad port actually produced one. Mirror validate()'s host-invalidity rule (empty, or not a recognized loopback spelling) so a customized-but-collapsed panel with a bad host says so too, instead of silently reading "Customized". Signed-off-by: Juho Vainio <juho.vainio@amd.com>
The xtask ascii_output_contract test flags en dashes that can reach a redirectable sink (ROCMAI-543: mojibake on Windows code page 437). Signed-off-by: Juho Vainio <juho.vainio@amd.com>
Upstream's write_atomic (write-then-rename) migration of ManagedServiceRecord::write() no longer respects a chmod on the manifest file itself -- fs::rename only needs write permission on the containing directory. Point both tests' write-failure simulation at services_dir() instead, pre-creating the launch lock and service log files first so only the manifest write is blocked. Signed-off-by: Juho Vainio <juho.vainio@amd.com>
Responds to rominf's CHANGES_REQUESTED review on the serve-port collision-safety work: - unify the launch_server port validators in apps/rocmd/src/mcp.rs and apps/rocm/src/chat.rs so a malformed port is rejected the same way from both, instead of silently falling through to automatic selection on hosts that don't support it - treat Windows PermissionDenied as a bind conflict during automatic port scanning instead of aborting the whole scan - fix a blind whole-record write in restart rollback to only restore the fields restart_managed_service changed - fix a vacuous assertion in the Dash wizard Escape-cancels-edit test - add Gherkin coverage for the reworked Dash serve wizard and for an explicit port held by an untracked process (serve-29), gated @requires-gpu after tracing the real call chain - back the serve-28 "stop the reserving service" remediation with a real stop path instead of a direct disk write - strengthen several existing tests: prove the leased port is released before spawn from inside the injected spawn (both call sites), prove stopped/failed records never block port reuse, assert the automatic- port disclosure line order, assert the restart rollback actually kills the supervisor process - README/SKILL.md wording fixes - fix two clippy lints (type_complexity via a SpawnOverride type alias, a redundant clone) introduced by the above Signed-off-by: Juho Vainio <juho.vainio@amd.com>
a800feb to
331fb47
Compare
rominf
left a comment
rominf
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · 331fb47
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.
The PR says it addresses #239, not that it closes it. It implements the port-policy, Dash wizard and recovery-lock parts. It openly leaves out the "bounded on both sides" lock decision, and says so under Known gaps.
Every finding below is new this round (found blind at 331fb47). None of the objections standing from the 250d150 round still holds; how each was settled is at the end, under What this covered. The reply of 2026-10-22 (fixed in 86a9ffc) was weighed there.
Blocking
-
[new] The custom-host
--portcheck never runs where the change says it does; networking-04 fails in CI —apps/rocm/src/serve_cmd.rs(serve(), around line 545),apps/rocm/src/serve_port.rs:103-118and:148-156,tests/e2e-cucumber/features/networking.feature:43-54- What the code does:
serve_port::validate_port_requestis only called insideresolve_serve_port.resolve_serve_portruns at launch time. That is after model resolution, after the "no usable AMD GPU" check (serve_cmd.rs:676), afterensure_self_managed_engine_ready(which can download an engine) and after the printed plan.
- What the change claims:
- The doc comment on
validate_port_requestsays it runs "before any engine resolution or side effect happens". resolve_serve_portsays automatic is "refused here as well as inserve()".- The scenario comment says the check runs "before any engine or GPU work, so it needs no GPU and runs on the mock lane every PR".
- The PR's Breaking-change section says it is "covered by networking-04".
- The doc comment on
- CI shows it fails: the hosted
E2E testsjob and theE2E tests (Strix Halo, WSL2, current)job both fail withFAIL: 'networking-custom-host-needs-explicit-port' … a regression. The step receivedError: no usable AMD GPU detected…instead of the--portmessage. The base commit's CI on main is green. - A worker reproduced this locally with the checkout's own
rocmand a sandboxed HOME. - Confidence 100 · logic · Fix: call
serve_port::validate_port_request(&host, port_request)?inserve()right aftervalidate_bind_host.
- What the code does:
-
[new] dash-39 snapshots the screen without waiting, and fails on two lanes —
tests/e2e-cucumber/tests/e2e/dash_steps.rs(serve_wizard_still_open_host_unchanged, around line 579),tests/e2e-cucumber/features/dash.feature:654-667- The step reads
screen_text()immediately after Escape is sent. - The MI300X and WSL2 lanes both fail it with
FAIL: 'dash-serve-wizard-escape-cancels-edit-without-closing' … a regression. The captured frame is still mid-redraw, showingHost [nop ]. - A worker reproduced it 3 out of 3 times locally. Adding
wait_until_gone("editing host")made it pass. serve_wizard_shows_only_basic_fields(dash-38) uses the same immediate-read pattern; it is latent rather than observed failing.- Confidence 100 · logic · Fix: wait for the screen to settle (
wait_until_gone/wait_for_screen) before asserting in both steps.
- The step reads
-
[new] A new rocmd test depends on Linux-only process data and fails the Windows lane —
apps/rocmd/src/watchers.rs:3545-3616(recovery_is_not_blocked_by_a_stopped_or_failed_record_on_the_same_port)- The test asserts
supervisor_start_ticks.is_some().rocm_core::process_start_ticksalways returnsNoneon non-Linux (proc_lifecycle.rs:312-320). - Its sibling
restart_managed_service_persists_the_supervisor_identity_tokenis gated#[cfg(target_os = "linux")]for exactly this reason. windows-build-and-testfails atwatchers.rs:3611: "recovery must reach the spawn, proving neither sibling blocked it".- Confidence 100 · logic · Fix: add
#[cfg(target_os = "linux")], or assert something that holds on both platforms.
- The test asserts
-
[new] New rocmd rollback tests fail intermittently —
apps/rocmd/src/watchers.rs:~4160-4210(restart_rolls_back_and_kills_the_supervisor_when_the_second_write_cannot_be_read),:~4250-4300(restart_reports_when_the_rollback_write_also_fails)- Both tests need a background thread to win a spin-loop race inside a fork+exec window. They assert "precondition: the race won".
- I ran
cargo test -p rocmd watchers::six times: 52 tests ran each time and 2 of the 6 runs failed, each on one of these tests. - A worker also saw
restart_releases_the_leased_port_before_spawning_the_supervisorfail in 2 of 6 runs. - CI Linux runs each test in its own process, which hides this. Local
cargo testand the Windows lane do not. - Confidence 100 · logic · Fix: make the failure deterministic with an injected write or spawn seam (one already exists,
spawn_override) instead of racing a thread against the code under test.
-
[new] Several branches the change adds are not tested: they can be broken and every test stays green —
apps/rocm/src/main.rs:7323-7327,apps/rocmd/src/watchers.rs:1527and:1770,apps/rocmd/src/mcp.rs:1097-1112- CLI restart lease release. I replaced
if let Some(lease) = port_lease { lease.release(); }withlet _keep_lease_mutation = port_lease;. That would make every loopbackrocm services restartfail at the engine bind.cargo test -p rocm --bin rocm restartstill passed: 30 passed, 0 failed.- The release-before-spawn seam tests cover only
service_launchand rocmd, not this third copy. restart_is_not_blocked_by_a_stopped_or_failed_record_on_the_same_portaccepts anyErrthat lacks one phrase, so it passes even when the restart fails for another reason.
- The release-before-spawn seam tests cover only
- rocmd self-exclusion. I dropped
other.service_id != record.service_id &&from the collisionfind. All 52watchers::tests passed.- The guard matters because rocmd reads records as-is, so a crashed service can still read
running/recoveringon its own port.
- The guard matters because rocmd reads records as-is, so a crashed service can still read
- rocmd rollback merge. I changed
Ok(_) => Ok(())toOk(_) => restore.write()inrollback_recovering_record, which is the blind restore its doc says must not happen. All 52 tests passed. - mcp.rs
launch_serverport type and range check. No test sends"8000",0,65536ornull. Removing the check leaves thelaunch_server_*tests green: a worker ran that mutation, and I confirmed by reading the tests. Thechat.rstwin does have a malformed-port test. - Worker mutation runs also found that
MANAGED_LAUNCH_LOCK_TIMEOUTcan be raised to 3600 s with everything green. - Confidence 100 (mcp.rs: 85) · logic · Fix: a spawn-seam test for the CLI restart path; a test recovering a stale live-status record on its own port; a test that persists a stop marker or a different supervisor between the two writes; malformed-port cases in
mcp.rs.
- CLI restart lease release. I replaced
-
[new] Renumbering serve-05…26 left stale scenario numbers, including the AGENTS §3 test pointer and a CI-docs claim that is now wrong —
crates/rocm-dash-tui/src/ui/serve_wizard.rs:1847,docs/ci-hardware-testing.md:278,docs/testing.md:991,tests/e2e-cucumber/features/local_server_records.feature:4,20,tests/e2e-cucumber/features/runtime_lifecycle.feature:161,tests/e2e-cucumber/tests/e2e/service_stop_steps.rs:198,apps/rocm/src/service_launch.rs:1640, plus four lines inexpectation.rs(serve-19/20 → 20/21)- Inserting the new serve-05 shifted every later index. Go by the feature file, since the indexes come from there.
serve_wizard.rs:1847is the §3 comment that names the test proving the pinned "Port: automatic" line. It saysserve-auto-port-skips-occupied(serve-26), but that scenario is now serve-05.docs/ci-hardware-testing.mdstill callsserve-05the per-PR vLLM canary. serve-05 is now the new@merge-queuescenario, so the sentence is wrong in substance, not just in number.- The PR description also says serve-26.
- Confidence 100 · mechanical · Fix: add the scenario at the end (serve-27+) instead of renumbering, or update every reference.
-
[new] The description claims work the code does not contain
- It says networking-04 is "rejected before any engine starts" and that the breaking change is covered by it. See the first finding.
- It says the change "unified the two
launch_serverport validators (mcp.rs,chat.rs)". The code has two hand-copied blocks (apps/rocm/src/chat.rs:2238-2264,apps/rocmd/src/mcp.rs:1097-1123), not a shared one. This is exactly the twin-copy pattern that already split their test coverage (see above). - Confidence 100 · logic · Fix: correct the description, or extract one shared validator (for example next to
is_canonical_loopback_hostin rocm-core).
Non-blocking
-
[new] Escape on a Host-editor error can move focus to Model and show "model is required" —
crates/rocm-dash-tui/src/ui/serve_wizard.rs:584-588,:659-670- The editor's Esc arm calls
recompute_message, which runs the fullvalidate()and refocuses whatever it flags. - With an empty model, the sequence Host edit → empty Enter → Esc swaps "host cannot be empty" for "model is required" and jumps focus to Model.
- The adjacent
restate_or_clearexists precisely to avoid surfacing an untouched field's complaint. A worker reproduced this with a scratch test; I confirmed it by reading the code. - Confidence 85 · logic · Fix: use
restate_or_clear(w, field)or plain clear-on-cancel.
- The editor's Esc arm calls
-
[new] A recovery spawn failure leaves
recoveringon disk, which now blocks the port for rocmd —apps/rocmd/src/watchers.rs:1623-1627,:1640-1643- The
.context("failed to spawn recovery supervisor")?andtry_waiterror paths return without the rollback that therecord_spawned_supervisorfailure gets. is_live()now countsrecovering. rocmd reads records without refreshing them, so that port stays reserved until something rewrites the record.- The early return was already there; the port blocking is new in this change.
- Confidence 70 · logic · Fix: route those errors through
rollback_recovering_recordas well.
- The
-
[new] Doc comments misplaced, duplicated or stale —
apps/rocm/src/service_launch.rs- The doc for
launch_managed_engine_child_lockedsits ontype SpawnOverride(around lines 625-633), so the function is undocumented. - A five-line comment block is pasted twice (around lines 770-779).
- The module header still says "Mechanically relocated … with no behavior change" and names
existing_live_managed_service, which is nowexisting_live_managed_service_in. - A worker reported similar misplaced docs in
watchers.rsaround lines 1497 and 1674, and a comment there that cites "Finding #2". - Confidence 100 · mechanical · Fix: move or delete as appropriate.
- The doc for
-
[new] Unit tests bind fixed ports inside the automatic range 11435-11535 —
apps/rocm/src/service_launch.rs(11_514+attempt, 11599),apps/rocmd/src/watchers.rs(11459-11462)- A real
rocm serverunning on the developer's machine makes these fail. automatic_selects_against_real_loopback_listenerspasses vacuously (returns early) if 11435/11436 are busy.- Confidence 70 · logic · Fix: use ephemeral ports through the injected probe.
- A real
-
[new]
FileLock::acquire_boundedduplicatesacquire's open/create sequence —crates/rocm-core/src/process.rs:448-460- Confidence 85 · mechanical · Fix: share a private opener.
Decisions for the author
-
Unbounded CLI lock hold vs rocmd's 5 s bound — tradeoff
- Under the launch lock, the CLI runs
load_managed_services, which does an HTTP liveness probe of up to 750 ms per ready/running record, plus GPU selection. - rocmd recovery gives up after
MANAGED_LAUNCH_LOCK_TIMEOUT(5 s). With several live services, recovery will regularly time out and back off. - Known gaps acknowledges the asymmetry, but not this ratio. Either shorten what the CLI holds the lock across, or accept periodic recovery deferrals.
- Under the launch lock, the CLI runs
-
The port policy is hand-copied into three places — non-blocking-improvement
- The canonical-host and explicit-port rules now live in
serve_port.rs,chat.rs/mcp.rs, and the Dash wizard's constants. is_loopback_host_matches_the_cli_exactlypins literals only.- A parity test in
apps/rocm, which already importsserve_wizard, or one shared rocm-core helper would keep them from drifting.
- The canonical-host and explicit-port rules now live in
Positive signals
- The launch transaction is ordered correctly: the record is published before
lease.release(), release happens immediately before the spawn, and the lock is held through the final write.launch_releases_the_leased_port_before_spawning_the_childfails when the release is removed; a worker ran that mutation. - The cross-process lock and port-handoff tests in
process.rsreally spawn separate processes instead of simulating them in one. bind_errorkeepsAddrInUsedistinct from other bind errors, and the automatic scan never silently skips past a permission error on Linux.- serve-28 asserts the "Stop it with
rocm services stop …" advice together with the state.stopping_the_reserving_record_frees_its_explicit_port_for_reuseexercises the real stop path, which is what AGENTS §3 asks for. - The scenario
@ids and indexes passfeature_naming(4 tests).
Deployment notes
- Breaking CLI change:
rocm serve --host <anything but 127.0.0.1>without--portis now refused. Today it is refused late, after engine preparation; see the first blocking finding. - The default endpoint is no longer guaranteed to be
:11435. Clients hardcoding it should read the endpoint fromrocm services. The PR's Known gaps also lists hardcoded:11435text in Dash and chat.
What this covered
- What was read: all 26 changed files in full, plus the callers of changed symbols, the rocm-core
proc_lifecycleandservice_recordcontracts, and sibling restart/recovery paths. Range:1c33248c…331fb471. The merge base equalsprw-base, so base drift was not a factor. - Fan-out: two coupled workers.
- Group A: rocm app plus rocm-core.
- Group B: rocmd, dash-tui and e2e. It split itself into two read-only sub-workers and skimmed about 3,000 unchanged lines of
watchers.rs. - I ran the agent-instruction, history and code-comment passes and the design review myself, so the design review is not independent.
- What I ran myself:
- Re-ran the CLI restart lease mutation, the rocmd self-exclusion and rollback mutations, and the rocmd flakiness runs; each run confirmed a non-zero count of the named tests.
- Read the CI check runs and the failing job logs.
- CI attribution:
- The hosted E2E, Windows and WSL2 failures are attributable to this change: the base commit's main CI is green, and the failing scenarios and test are new here.
- MI300X's
chat-06failure also fails on main's nightly run, so it is pre-existing. Itsdash-39failure is new. - The three Strix Halo Ubuntu/Windows lanes were cancelled, so that evidence is missing.
- What did not run:
- The prior-changes pass, because it would need PR discussion, which is out of bounds here.
- Windows compile/clippy of the
cfg(windows)arms, because no mingw toolchain is available; the Windows CI lane did compile and run the tests. - The GPU-gated scenarios serve-05/28/29.
- Sign-off: the check reported 13 commits checked, none missing a trailer.
- Not reconciled against any PR discussion.
- Standing objections from the 250d150 round. The head was rebased, so this was a full review and every file those objections concern was read blind. The blind pass did not re-find B2, B4, B5, B6, B7 or B8. After the report above was frozen, each was put to the same reviewer together with the reply of 2026-10-22. Those verdicts are anchored evidence, not blind findings. An objection was dropped only where the anchored check and the blind silence agreed:
- B5 (automatic-port plan line never asserted): dropped.
assert_next_automatic_portnow asserts the disclosure line before theendpoint:line. The wizard pin's stale "serve-26" pointer is part of the renumbering finding above. - B6 (supervisor kill in the restart rollback untested): dropped. Removing the
terminate_recovery_supervisorcall turned the rollback test red in 3 of 3 runs. The test is racy, but that produces false reds, not false greens; it is covered by the intermittent-tests finding above. - B7 (untracked-port refusal and the
services stopremediation): dropped. serve-29 now exists, and the stop unit test goes throughstop_internal_managed_service_with. Both GPU scenarios run only on the merge-queue lane. - B8 (Windows liveness check missing from the description): dropped. The description now documents it and names the covering test, which passed on the Windows lane.
- B2 (no Gherkin for the wizard rework): the original objection is met, because dash-38 and dash-39 now exist. That coverage does not work yet; that is the new dash-39 finding above.
- B4 (
blocks_port'sis_livehalf untested): dropped. Mutating it toself.port == portturned the new rocm and rocmd tests red. - B3 (lease release before spawn untested): settled by the blind pass itself. The
service_launchseam test fails when the release is removed; a worker ran that mutation. The rocmd seam test exists but is intermittent; the CLI-restart copy is untested, which is a new finding above. - B1 (red Windows lane): superseded by this round's CI attribution.
- B5 (automatic-port plan line never asserted): dropped.
- The reply of 2026-10-22 was weighed through the settlement above, and that settlement is what answers it. One of its claims did not hold up: it says the
launch_servervalidators were "unified", but they are still two copies (see the description finding). Its localcargo testresult does not cover the failures above, which are in the e2e lanes and on Windows.
- serve(): reject automatic port on a custom host right after validate_bind_host, before any GPU or engine work (networking-04). - dash e2e: wait for the screen to settle before asserting in dash-38/39. - rocmd tests: gate the Linux-only recovery test; drive the rollback tests through the spawn seam instead of racing a thread; retry the lease-release bind briefly for sibling-fork races. - Add a spawn seam to the CLI restart path and tests for lease release, recovery self-exclusion, the rollback merge and the spawn-failure rollback. - rocmd recovery: roll the recovering record back when the spawn or try_wait fails, so it stops reserving the port. - Share one launch_port_argument validator (rocm-core) between chat and MCP launch_server, with malformed-port tests. - Restore the original serve-NN numbering and append the new scenarios as serve-26..29, fixing the stale references. - Dash wizard: Esc on a host-editor error no longer surfaces or focuses another field's complaint. - Use ephemeral ports in tests that bind for real; share the FileLock opener; fix misplaced and duplicated doc comments. Signed-off-by: Juho Vainio <juho.vainio@amd.com>
|
Addressed review 5651221433 in fea5e53. Each item below maps to a finding. Blocking
Non-blocking, done
Deliberately left unchanged
|
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 ( |
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.