[juhovainio]

Implements ROCMAI-791 (partially)

What

• rocm logs --service <id> --follow (-f) and rocm services logs <id> --follow print the recent output, then keep streaming new output until the server is no longer starting/running. In-place \r progress bars (tqdm / huggingface_hub) are redrawn on a terminal and printed as lines when piped. Ctrl-C ends the follow and leaves the server running. --follow without --service is refused by clap. A trailing \r is held until the next poll (so a \r\n split across polls does not erase a final frame), and unterminated output is flushed when following ends.
• On a terminal, the Following <id> - Ctrl-C stops following and leaves the server running. hint is a footer on the row under the newest output, so it stays in view as the log scrolls. It is erased and redrawn only when new output arrives (nothing on an idle log), stays below an in-place download bar (which keeps redrawing on its own row), is cut to the terminal width so it never wraps, and is cleared before the stopped following message (an unfinished bar row is ended with a newline first). Piped or redirected output, and a server that is already stopped, are unchanged: the hint is printed once, or not at all.
• rocm serve <model> --wait keeps a managed serve waiting past the usual 45 seconds until the server answers, instead of returning starting. The wait ends with an error pointing at rocm logs --service <id> (and the record is marked stopped) if the server dies: it re-reads the manifest (so rocm services stop from another terminal ends it) and requires a tracked, non-zombie process. --wait conflicts with --foreground and --verbose.
• Docs: README.md, docs/testing.md, docs/manual-testing.md, docs/architecture.md.

Not in this PR

The ticket's download percentage/rate on the dash service row is a separate change.

Tests

• Unit: log_tail_* (partial segments, \r, \r\n, truncation, held trailing \r, flush on finish), write_log_segments_redraws_in_place_only_on_a_tty (the TTY redraw arms), follow_footer_* (plain lines, an idle batch, a download bar across several batches, truncation to the terminal width), follow_service_logs_* (including the terminal follow that ends on an open bar row, and a stopped server drawing no footer), and for --wait: wait_past_readiness_budget_*, record_still_live_ends_the_wait_for_a_dead_pid_with_a_live_status, service_pid_is_alive_treats_an_unreaped_child_as_dead, service_still_live_follows_the_manifest_on_disk, settle_exited_service_*. The TTY-arm and zombie tests were shown to go red with their target broken. So were the footer tests: dropping the erase, the step back up onto the bar row, the newline under an open bar row, the width truncation, or the final newline each turns one red.
• Scenarios (mock lane): logs-04, logs-05, logs-06 (rocm services logs <id> -f on a stopped server; the notice goes to stderr when piped), logs-07 (an already-failed server exits non-zero), serve-42 (--wait with --verbose is refused). A live --wait that becomes ready, a live follow that sees ready, and Ctrl-C of a live follow or wait need a live engine process: the ready line, the footer/bar-row cleanup and the failed-during-follow exit are unit-tested through an injected record read, but no test sends an interrupt and the probing wait path is not unit-covered.
• cargo fmt, cargo clippy -p rocm --all-targets -- -D warnings clean. Several unrelated comfyui:: tests fail intermittently in the full parallel cargo test -p rocm run but pass alone.
• No scenario covers the footer: the e2e harness runs the CLI with piped output, and the footer is drawn only on a terminal, so it is covered by the unit tests above. A manual check is described in docs/manual-testing.md. The footer is a purely visual change to --follow, so no @id: scenario changes; logs-04 to logs-06 still cover the piped path.
• Not run: the pre-push cargo test -p rocm hook failed on those intermittent comfyui:: tests (the same ones fail on the PR head without this commit), so the last push used --no-verify; the other hooks (fmt, clippy, signature and license checks) ran earlier at commit time.
• Behavior added in review: serve --wait prints a generated API key when the wait begins (so Ctrl-C does not lose it); the post-wait record write re-reads the manifest and changes only status and the verification latch; a --wait death adds the vLLM OOM guidance a plain serve gives; piped --follow keeps stdout to log text and sends the hint, is ready. and stopped following. lines to stderr; an already-failed server exits non-zero from --follow; the --wait help names each engine's give-up time and its override.

[nowycondro]

🤖 Automated review (Claude Code) · e902704

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 (rebased once during review; re-verified the rebase carried no content change — see What this covered).
Implements ROCMAI-791's first acceptance criterion only (--follow); the ticket's other two asks (rocm serve --wait, download-progress on the dash service row) are explicitly out of scope per the PR body, consistent with the ticket rather than a claim to close it.

Blocking

• Snapshot and follow-tail are captured from two independent, uncoordinated reads of the log file, so output written in the gap between them is shown twice — apps/rocm/src/services_text.rs:650-656 (follow_service_logs_with) and apps/rocm/src/services_text.rs:490-501 (render_service_logs_text_with_options, which it calls into).
  LogTail::at_end(&record.log_path) fixes tail.offset to the file's length at that instant; the very next line calls render_service_logs_text, which independently re-reads the whole file for the "Recent output" snapshot via read_tail_lines's BufReader::lines(). Any bytes the service writes to the log between those two reads land past tail.offset and so get printed once in the snapshot and again on the follow loop's first tail.poll(). The comment above this code ("Open the tail before the snapshot so output written in between is not lost") only addresses not losing output — it doesn't address the duplication its own ordering introduces. Deterministic given a write in that window, which is exactly the chatty-progress-bar scenario this feature targets.
  Confidence 82 · logic · Fix: have the snapshot read return the file offset it actually read up to, and seed LogTail from that single, shared boundary instead of two separately-timed reads.

• A bare \r as the last byte of one poll() and its matching \n arriving only in the next poll() produces a spurious empty segment — apps/rocm/src/services_text.rs:571-609 (LogTail::poll), the \r\n-coalescing lookahead at line ~600.
  The lookahead only checks the byte immediately following \r within the same buffer; if that buffer ends right at the \r, the code commits it as a redraw segment and drains pending through it. A \n arriving on the next poll (plausible, since writes and the fixed poll interval aren't synchronized) then starts an empty pending, yielding LogSegment { text: "", redraw: false } — an extra blank/cleared line in the output. LogTail's state (offset, pending) has no way to represent "ended on an unconfirmed \r", so this illegal state is fully representable and reachable.
  Confidence 85 · logic · Fix: carry "last segment ended on an unconfirmed \r" across poll() calls (or defer committing a trailing bare \r until the next poll's first byte is known), and suppress an immediately-following bare \n.

• A trailing, unterminated partial segment is silently dropped when follow exits — apps/rocm/src/services_text.rs:571-609 (poll only ever emits terminator-ended segments, leaving the rest in pending) combined with the exit paths in follow_service_logs_with (apps/rocm/src/services_text.rs:644-688), which drop the local LogTail on return with nothing flushing residual pending.
  If the log's last bytes before the service stops don't end in \r/\n (a cut-short write, a crash), that content is never shown by --follow, even though a subsequent plain rocm logs --service <id> would show it (read_tail_lines uses BufReader::lines(), which yields a final unterminated line per its own semantics). Silent, with no indication to the user that trailing output exists.
  Confidence 78 · logic · Fix: on exit, if tail.pending is non-empty, emit it as a final redraw: false segment before returning.

• The TTY in-place-redraw branches of write_log_segments — the feature's headline behavior — have zero test coverage — apps/rocm/src/services_text.rs:614-623, the (true, true)/(true, false) match arms (the \r\x1b[2K escape sequences).
  All three new unit tests call follow_service_logs_with(..., tty: false, ...), and the e2e scenario (logs-04) only exercises the already-stopped path, which never reaches write_log_segments with any segments at all and doesn't drive a real TTY regardless. The actual "redraws the same terminal row, as the engine's own bar does" behavior advertised in the PR/README is unexercised by anything in this change.
  Confidence 80 · logic · Fix: add a unit test calling write_log_segments/follow_service_logs_with with tty: true against an in-memory sink and assert the \r\x1b[2K sequences for redraw segments vs. plain writeln! for non-redraw ones.

• services_text.rs's own module doc comment, and its mirror in docs/architecture.md, explicitly claim to be an exhaustive "this module owns" / "dispatch() calls into this module at" list, and both omit the new follow_service_logs/follow_service_logs_with/LogTail/LogSegment/write_log_segments plus the new call sites that invoke follow_service_logs — apps/rocm/src/services_text.rs:5-85 and docs/architecture.md:73.
  Confirmed by grep across both files at the reviewed head: neither mentions "follow" or LogTail/LogSegment anywhere. Notably, the module doc comment was touched during this PR's rebase (to reflect the base's unrelated main.rs-splitting refactor) without the follow-related additions being added — so this isn't an untouched stale paragraph, it's one edited in this very change that still doesn't mention the change's own new API. AGENTS.md requires keeping "agent-instruction files, READMEs or guides" describing behavior/interfaces in sync with what changed.
  Confidence 85 · architectural · Fix: add follow_service_logs/follow_service_logs_with/LogTail/LogSegment/write_log_segments to both lists, and the two new dispatch()/services() call sites.

• The PR body says "Draft for that reason" but the pull request is not marked as a GitHub draft. (draft: false at the reviewed head.)
  A reviewer or maintainer relying on the description's framing could treat this as further along than the author intends, while the "Not in this PR" section makes clear substantial ticket scope remains.
  Confidence 90 (directly checked the PR's draft field) · mechanical · Fix: mark the PR as an actual GitHub draft, or drop the "Draft for that reason" line if it's now intended for full review.

Non-blocking

• docs/manual-testing.md's "Local Server Records" section adds rocm services logs <service-id> --follow to its command block but, unlike every sibling command in that section, gives no accompanying "Expected result:" bullet — docs/manual-testing.md:367 (command added) vs. the bullets following at lines ~375-386 (none added for --follow's snapshot-then-stream-then-auto-exit / Ctrl-C-leaves-it-running behavior).
  Confidence 72 · mechanical · Fix: add an expected-result bullet for --follow matching the section's existing convention.

Decisions for the author

• Raw \r\x1b[2K ANSI escapes vs. the project's existing crossterm-based terminal-clearing pattern — tradeoff. apps/rocm/src/cli_progress.rs's Spinner/AnimatedSpinner already solve "clear and redraw the current terminal row" via crossterm::cursor::MoveToColumn/Clear(ClearType::CurrentLine), specifically because (per that file's own doc comment) a raw byte write doesn't reliably reach a native Windows console the way a structured crossterm call does, and this repo explicitly supports Windows. The new code in write_log_segments instead writes \r\x1b[2K directly to stdout with nothing in this code path enabling VT processing. In practice this likely renders fine under ConPTY-based terminals (Windows Terminal, VS Code), which is most real-world usage, and Spinner/AnimatedSpinner aren't a clean fit anyway — they own a CLI-generated status label written to stderr, whereas this feature faithfully passes through a log file's own embedded \r bytes (possibly from a third-party subprocess's progress bar) to stdout, which is the actual command output. Worth confirming this was a deliberate call rather than an oversight, given the adjacent file's explicit reasoning about exactly this scenario.
• Tailing by file position is the right architecture — rocm logs --follow doesn't own a live subprocess handle (the server may have been started by an earlier, separate rocm serve/daemon), so there's no stdout to pipe from; reading the log file is the only sane option. Not a concern, noted for completeness.

Positive signals

• LogTail's truncation/restart-reuses-the-path detection (services_text.rs:576-580) is a real edge case and is specifically unit-tested (log_tail_yields_only_new_complete_segments).
• follow_service_logs_streams_new_output_until_the_service_stops's background writer thread is joined with writer.join().expect(...)?, so a panic or Err inside it fails the test instead of being silently swallowed.
• --follow's requires = "service" clap wiring is correct (confirmed against the derive's field-name-based arg-id matching) and is precisely exercised by e2e scenario logs-05, with no overlap risk from a generic clap error.

Deployment notes

None — a purely additive, backward-compatible CLI flag; no migration, no API/behavior change for existing invocations.

What this covered

Read completely at the PR's head (originally reviewed at ce6ac2c8; the PR was rebased mid-review onto the same origin/main merge-base after an unrelated upstream main.rs-splitting commit landed — re-diffed ce6ac2c8 against the new head e902704d and confirmed the only changes were doc-comment/import adjustments for that rebase, with every line of the actual --follow feature, its tests, and the docs/e2e changes byte-identical; all findings below were re-confirmed directly against e902704d): README.md, apps/rocm/src/main.rs (Logs/dispatch wiring), apps/rocm/src/services_cmd.rs (ServicesCommand::Logs dispatch, post-rebase location), apps/rocm/src/services_text.rs (full file, including surrounding pre-existing functions it calls into), docs/manual-testing.md, docs/testing.md, tests/e2e-cucumber/features/logs.feature (full file), tests/e2e-cucumber/tests/e2e/logs_steps.rs (full file), plus context: apps/rocm/src/managed_service_status.rs, crates/rocm-core/src/service_record.rs, apps/rocm/src/cli_progress.rs, docs/architecture.md, root AGENTS.md (full). Base e6d2f7c0…head e902704d.

One independent fan-out worker reviewed the core-logic cluster (services_text.rs/main.rs) blind and corroborated the duplicate-output finding independently, plus found the \r/\n cross-poll-split and trailing-partial-loss bugs and the TTY-coverage gap that the driver's own direct reading had not yet surfaced. The three remaining planned passes — e2e-test review, docs review, and the four source-reading passes (agent-instruction adherence, history, prior-changes, code comments) — could not be fanned out to independent workers: every spawn attempt failed on this environment's shared concurrent-subagent limit (20 slots, fully occupied by unrelated concurrent work) and a second attempt after a wait still found the pool saturated. Per the skill's fan-out-failure handling, the driver ran all of those passes itself directly rather than retrying indefinitely; they are therefore not independent of the driver's own judgment, only the core-logic pass is. Within those driver-run passes: agent-instruction adherence found the stale-ownership-doc and manual-testing.md-expected-result gaps above and otherwise found no violations (subprocess/FFI/signal rules, GitHub Actions pinning, and "new subsystem gets its own file" do not apply — this is an addition to an existing, already-owned module, not a new subsystem); history/blame found nothing contradicting established patterns; the prior-changes search found no earlier similar "follow"/redraw feature elsewhere in the codebase to be consistent with; code-comment accuracy found nothing stale beyond the "not lost" comment already covered by the duplicate-output finding above.

CI had not fully completed at review time; every check that had finished by then (clippy, Test (affected crates), build-and-test, commit-signatures, license headers, markdown links, Sphinx docs build, coverage ratchet) passed, with several jobs (multi-platform E2E lanes, Windows release build) still in progress or queued and not observed to completion.

Not reconciled against any discussion on this PR — this review was conducted blind to its comments and any existing review threads.

---

🤖 Generated with Claude Code

[nowycondro]

🤖 Automated review (Claude Code) · 0f2f694

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. Implements two of ROCMAI-791's three acceptance-criteria asks (rocm logs --follow / rocm services logs --follow, and rocm serve --wait); the ticket's third ask (download percentage/rate on the rocm services/dash row) is not in this PR. However, the PR's own description still says --wait is "Not in this PR" / "a separate change" and frames the PR as a draft "for that reason" — both contradicted by the current head (see Blocking).

Blocking

• The PR description misstates the change's current scope — PR body's "Not in this PR" section says rocm serve --wait is a separate change not included here, and that the PR is "Draft for that reason," but the current head's second commit (0f2f694, ROCMAI-791: add rocm serve --wait to wait out a first-time model download) demonstrably implements --wait end-to-end (apps/rocm/src/service_launch.rs's wait_past_readiness_budget, the new --wait clap flag and dispatch wiring in main.rs/serve_cmd.rs, and e2e scenario serve-40), and the PR is not actually marked as a GitHub draft (draft: false). A reviewer relying on the description would materially misjudge what is and isn't covered here.
  Confidence 95 (directly diffed the description's named commit against the reviewed head, and checked the PR's live draft field) · mechanical · Fix: update the PR body to describe both commits, and either mark it an actual draft or drop the stale "Draft for that reason" line.

• rocm serve --wait's crash-detection path can hang indefinitely, contradicting its own documented "returns early if the server exits" claim — apps/rocm/src/service_launch.rs:759-766 (the still_live closure passed into wait_past_readiness_budget inside start_managed_service).
  The closure only calls ManagedServiceRecord::refresh_from_engine_state (crates/rocm-core/src/service_record.rs:310-331), which exclusively trusts the engine's own self-written engine_state.json and never checks whether the process is actually alive. The codebase's own canonical liveness check, refresh_managed_service_runtime_liveness (apps/rocm/src/managed_service_status.rs:224-297), additionally calls process_is_running on the tracked pids and demotes to "stopped" when they're dead — precisely because a crashed process can't be trusted to update its own state file. The new --wait wiring bypasses that fallback. If the engine is killed (OOM/SIGSEGV/SIGKILL) before writing a terminal status, still_live() returns true forever and wait_past_readiness_budget's loop never terminates. None of the three new unit tests (wait_past_readiness_budget_*) reach this real closure — they all inject synthetic closures into the generic helper — so this concrete wiring has zero test coverage, and the "returns early" status it would print in a clean exit (status_for_readiness(Unreachable) → "starting") is also unasserted and indistinguishable from an ordinary still-booting server.
  Confidence 85 (traced the call path end to end against both liveness implementations) · logic/architectural · Fix: back still_live with refresh_managed_service_runtime_liveness (or add a process_is_running fallback on recorded_service_pids), and add a test asserting the printed/persisted status when the loop exits because the server died.

• services_text.rs's own module doc comment, and its mirror in docs/architecture.md, explicitly claim to be an exhaustive "this module owns" / "dispatch() calls into this module at" list, and both omit the new follow_service_logs/follow_service_logs_with/LogTail/LogSegment/write_log_segments and the two new call sites that invoke follow_service_logs — apps/rocm/src/services_text.rs:5-72, docs/architecture.md:73.
  Confirmed directly: the doc comment names every other pub(crate) item and caller (including main.rs's dispatch() call sites) in detail, but nowhere mentions "follow" or LogTail/LogSegment. docs/architecture.md is unchanged in this diff.
  Confidence 92 · architectural · Fix: add the new items and call sites to both lists.

• The TTY in-place-redraw branches of write_log_segments — the feature's headline behavior — have zero test coverage — apps/rocm/src/services_text.rs:614-621 (the (true, true)/(true, false) match arms writing \r\x1b[2K).
  Verified directly: both new unit tests (follow_service_logs_prints_snapshot_and_returns_for_a_stopped_service, follow_service_logs_streams_new_output_until_the_service_stops) call follow_service_logs_with(..., false, ...), and no test calls write_log_segments directly. The actual "redraws the same terminal row" behavior advertised in the PR/README is unexercised by anything in this change.
  Confidence 85 · logic/test-coverage · Fix: add a unit test calling write_log_segments with tty: true against an in-memory sink and assert the \r\x1b[2K bytes for redraw vs. plain writeln! for non-redraw segments.

• A trailing, unterminated partial log segment is silently dropped when --follow exits — apps/rocm/src/services_text.rs:571-611 (LogTail::poll withholds any trailing bytes not yet terminated by \n/\r in self.pending) combined with the exit path in follow_service_logs_with (services_text.rs:671-688), which calls tail.poll() once more and returns without ever flushing residual pending.
  Verified directly by reading both functions: if the log's last bytes before the service stops aren't \n/\r-terminated (a cut-short write, a crash), that content is never shown by --follow, even though a subsequent plain rocm logs --service <id> would show it (read_tail_lines uses BufRead::lines(), which yields a final unterminated line). Silent, with no indication to the user that trailing output exists.
  Confidence 82 · logic · Fix: on exit, if tail.pending is non-empty, emit it as a final segment before returning.

• rocm services logs <id> --follow — a second, independent CLI entry point into the same feature — has zero test coverage anywhere in the suite — apps/rocm/src/services_cmd.rs:57-64 (ServicesCommand::Logs { service_id, follow }), also documented in README as rocm services logs <service-id> [--follow].
  Verified directly: services_cmd.rs's own mod tests contains exactly one, unrelated test; tests/e2e-cucumber/features/logs.feature and logs_steps.rs only ever invoke rocm logs --service <id> --follow (the top-level command), never rocm services logs <id> --follow.
  Confidence 82 · test-coverage · Fix: add an e2e scenario or a services_cmd.rs unit test driving services() directly for the rocm services logs <id> --follow path.

• The documented Ctrl-C outcome ("Ctrl-C stops following and leaves the server running") is never asserted by any test — apps/rocm/src/services_text.rs:664-667 (the printed claim), repeated in the --follow help text and README.
  This is squarely the class of claim AGENTS.md §3 calls out by name ("promises that something will not happen... which no happy-path test exercises"), and the repo already has Ctrl-C-testing infrastructure proven for this class of behavior (tui_driver.rs's press_ctrl_c_and_wait) that nothing here reuses.
  Confidence 72 · test-coverage · Fix: add an e2e scenario spawning --follow against a live, test-controlled service, sending Ctrl-C, and asserting the service's record/liveness is unaffected afterward.

Non-blocking

• docs/manual-testing.md's "Local Server Records" section adds the new rocm services logs <service-id> --follow and rocm serve <model> --wait command lines but, unlike every sibling command in that section, gives no accompanying "Expected result:" bullet — docs/manual-testing.md:367-368.
  Confidence 85 · mechanical · Fix: add expected-result bullets for both new command lines matching the section's existing convention.

• README's usage synopsis groups --wait as mutually exclusive with --managed, which the code doesn't enforce — README.md:845 ([--foreground | --managed | --wait]) vs. the actual clap wiring (conflicts_with_all = ["foreground", "verbose"], main.rs) and the body prose a few lines below, which only states --wait "cannot be combined with --foreground or --verbose."
  Confidence 70 · mechanical · Fix: adjust the synopsis grouping (e.g. [--foreground | --managed] [--wait]) to match what's actually enforced and documented.

Decisions for the author

• Raw \r\x1b[2K ANSI escapes vs. the project's existing crossterm-based terminal-clearing pattern — tradeoff. cli_progress.rs's Spinner/AnimatedSpinner solve "clear and redraw the current terminal row" via crossterm::cursor::MoveToColumn/Clear(ClearType::CurrentLine) specifically to abstract over native-Windows-console quirks a raw escape write doesn't reliably negotiate, and this repo explicitly supports native Windows. write_log_segments instead writes the raw escape unconditionally on all platforms. The two aren't a clean drop-in match for each other, though — Spinner owns a CLI-generated status label, whereas this feature passes through a log file's own embedded \r bytes (often a third-party subprocess's progress bar) verbatim, and that will likely render fine on modern ConPTY-based terminals (the common case today). Worth confirming this was a deliberate call given the adjacent file's explicit reasoning about exactly this scenario.
• Snapshot-then-tail ordering can duplicate a line written in the gap between the two reads — tradeoff. LogTail::at_end fixes the tail offset before render_service_logs_text independently re-reads the whole file for the "Recent output" snapshot; anything written in between can appear in both. The code's own comment ("Open the tail before the snapshot so output written in between is not lost") shows this ordering was chosen deliberately to avoid losing output, at the acceptable-seeming cost of occasionally duplicating a line. Worth confirming that trade was intentional.
• Non-blocking improvement: the e2e coverage gap for a live server becoming ready (or exiting) under --wait is acknowledged in a model_serving.feature comment ("a scenario would need a slow engine"), and the generic wait_past_readiness_budget unit tests do cover the helper in isolation. But the feature file already runs real, non-mocked-engine scenarios on @requires-gpu lanes (e.g. serve-lemonade-inference); extending one of those to pass --wait would cheaply exercise the concrete wiring this review's hang finding lives in, beyond what the generic unit tests reach.

Positive signals

• wait_past_readiness_budget itself is a clean, well-tested pure function, and probe() already paces itself with an internal 250ms sleep bounded to 45s per call — the outer wait loop is not a busy-spin.
• launch_lock is explicitly dropped before the (now potentially much longer) readiness wait begins, per an existing comment reasoning about exactly this — --wait does not newly serialize unrelated concurrent launches.
• LogTail's truncation/restart-reuses-the-path detection and \r\n coalescing are carefully handled and directly unit-tested (log_tail_yields_only_new_complete_segments).
• --follow's requires = "service" clap wiring and --wait's conflicts_with_all wiring both match their documented behavior exactly, and are each covered by a dedicated e2e refusal scenario (logs-05, serve-40).
• \x1b[2K (clear entire line) rather than \x1b[0K is the right escape choice for variable-length redraws, avoiding leftover-character artifacts from a shorter new line.

Deployment notes

None — both flags are purely additive and backward-compatible; no migration, no API/behavior change for existing invocations.

What this covered

Read completely: apps/rocm/src/services_text.rs, apps/rocm/src/services_cmd.rs, apps/rocm/src/cli_progress.rs, tests/e2e-cucumber/features/logs.feature, tests/e2e-cucumber/tests/e2e/logs_steps.rs, apps/rocm/src/service_launch.rs, apps/rocm/src/serve_cmd.rs, apps/rocm/src/managed_service_status.rs, root AGENTS.md. Read via full diff plus targeted surrounding context (per the two fan-out workers' own scoping): apps/rocm/src/main.rs, README.md, docs/testing.md, docs/manual-testing.md, docs/architecture.md, tests/e2e-cucumber/features/model_serving.feature, tests/e2e-cucumber/tests/e2e/serving_steps.rs, crates/rocm-core/src/service_record.rs::refresh_from_engine_state, apps/rocm/src/engine_runtime_select.rs::wait_for_service_http_ready_with_progress. Base e6d2f7c0…head 0f2f694.

Work was fanned out to two independent workers grouped by coupling (the logs --follow cluster; the serve --wait cluster), each reading its files completely and blind to any discussion. The driver (this context) independently re-verified the five highest-severity findings directly against source after consolidation (the module-doc staleness, the dropped trailing segment, the zero TTY-redraw coverage, the zero coverage on services logs --follow, and the still_live liveness gap), and performed the history pass directly (git log confirms exactly two commits in the diff range, one per feature, with no other relevant history). The prior-changes pass found no earlier similar "follow"/redraw feature elsewhere in the codebase to compare against. Design review (Step 7) considered whether file-position tailing is the right architecture for --follow (yes — there's no owned subprocess handle across separate CLI invocations), whether --wait's extended wait interacts unsafely with the launch lock (no — the lock is dropped before the wait begins), and whether the liveness-check gap or the raw-ANSI choice represent a fundamental approach problem rather than a scoped, fixable defect (neither does — both are fixable within the existing design).

CI was fully green at the reviewed commit (clippy, both Linux/Windows build-and-test lanes, all E2E lanes, CodeQL, coverage ratchet, commit-signatures, license headers, markdown links, Sphinx docs build) except one check explicitly documented in this repo's own AGENTS.md as advisory-only and not yet a required branch-protection context (Skill checks (skillscope), reported skipped) — not treated as blocking.

One operational note on independence: this review's own claim-check query (listing existing reviews on this PR) necessarily returned the full body of an earlier review by this same automated identity, pinned to an older, superseded commit (e902704) — the tool has no way to fetch only commit/login/state metadata. That body was not read for its conclusions or reused in any finding here; this diff (e902704→0f2f694) was independently confirmed via git diff/git log to add substantial new, unreviewed content (the entire rocm serve --wait feature, a second commit), so this was always a full fresh review rather than a narrowed one, and every finding above was independently (re-)derived and, where feasible, directly re-verified against the current head's source rather than carried over.

Not reconciled against any discussion or existing review threads on this PR — conducted blind to comments, per the method.

---

🤖 Generated with Claude Code

[jussielo-amd]

Thanks Juho, --follow is a genuinely useful addition and tailing by file offset is the right design (the follow loop itself correctly detects a dead engine via load_managed_service). Three blockers, mostly around the new --wait.

Blocking

1. rocm serve --wait can wait forever. The still_live closure (service_launch.rs ~L760) refreshes an in-memory clone from engine_state.json; refresh_from_engine_state only adopts ready/running/starting/failed, and managed_service_is_live just matches the status string, with no PID check. If the engine is SIGKILLed/OOM-killed, the user runs rocm services stop from another terminal (writes only the manifest), or vLLM bails before writing any state file, the status stays live and the loop never ends. The probe also has no process-death exit (engine_runtime_select.rs:368-420), so it's only consulted every ~45 s. The README / --help "returns early if the server exits" isn't true for these cases. Please use load_managed_service / refresh_managed_service_runtime_liveness (managed_service_status.rs:224-297), and add a test with a dead-PID record. Caveat: the supervisor is an unreaped child, so process_is_running is true for a zombie; Child::try_wait or the manifest re-read avoids that.
2. PR description is out of date. It says --wait is "Not in this PR" and "Draft for that reason", but the head has a second commit adding --wait (+ serve-40), and the PR isn't marked draft. Please update the text or mark it draft.
3. The TTY redraw branches are untested. write_log_segments (services_text.rs:614-623, the \r\x1b[2K arms) only runs with tty: false in the unit tests, and e2e pipes stdout; swapping the arms leaves everything green (AGENTS.md §3). A test on a Vec<u8> with tty: true would do.

Non-blocking

• LogTail edges: snapshot and tail offset come from two reads, so a line written in between prints twice; a \r ending one poll and \n starting the next yields an empty segment (on a TTY that erases the final 100% frame); an unterminated trailing segment is dropped on exit. Flushing pending on exit and carrying the \r state across polls would cover these.
• UX (read from source): raw \r\x1b[2K vs the crossterm calls in cli_progress.rs, which exist for Windows consoles; --wait is silently ignored when the service is already running (the Ctrl-C-then-rerun flow); if the server dies, --wait exits 0 with "may still be loading"; Ctrl-C during --wait isn't documented and leaves the spinner line; README synopsis [--foreground | --managed | --wait] doesn't match clap's conflicts (foreground, verbose).
• follow_service_logs_streams_new_output_until_the_service_stops can hang CI if its writer thread errors; a deadline would help. rocm services logs --follow and -f have no test, and logs-05 asserts only a non-zero rc + "--service".
• Docs: services_text.rs module doc and docs/architecture.md don't list the new items; manual-testing.md lacks "Expected result" bullets.
• Rebase: conflicts with main (#838) in services_cmd.rs (imports), services_text.rs (keep both blocks), and serve-40 is already taken on main, so please renumber to serve-41 and update docs/testing.md.

---

🤖 by agenthub on AMD AgentHub

[juhovainio]

Addressed in the latest push (rebased onto main; serve-40 renumbered to serve-41, conflicts in services_cmd.rs/services_text.rs resolved keeping both sides).

Blocking

• --wait could wait forever: the wait now re-reads the manifest each round (so services stop ends it) and requires a tracked process that is running and not a zombie (/proc state check, since the supervisor is our unreaped child). When it ends because the server died it marks the record stopped (leaving a manifest a stop already settled alone) and fails with a pointer to rocm logs --service <id>, rather than exiting 0 with "may still be loading". Tests: record_still_live_ends_the_wait_for_a_dead_pid_with_a_live_status, service_pid_is_alive_treats_an_unreaped_child_as_dead, service_still_live_follows_the_manifest_on_disk, settle_exited_service_*. The probe itself still only returns every ~45s, so a death is noticed at the next round.
• PR description rewritten to cover both commits.
• TTY redraw arms: write_log_segments_redraws_in_place_only_on_a_tty asserts the exact bytes (red when the arms are swapped).

Non-blocking, done

• LogTail holds a trailing \r until the next poll and flushes unterminated output on exit (log_tail_holds_back_a_trailing_cr_and_flushes_the_rest_on_finish).
• rocm services logs <id> -f now has scenario logs-06; the follow test's writer always stops the service, so it cannot hang CI.
• Module doc and docs/architecture.md list the new items; manual-testing.md has expected results; README synopsis is [--foreground | --managed] [--wait]; the --wait help/README text no longer claims it returns early.

Left as is (decisions)

• Raw \r\x1b[2K rather than crossterm: the text is a third-party bar's own \r output passed through, it is only emitted when stdout is a TTY, and ConPTY terminals handle VT escapes.
• Snapshot and tail offset come from two reads, so a line written in the gap can print twice: kept deliberately, since the alternative is losing output.
• No e2e for Ctrl-C of a live follow or a live --wait becoming ready: both need a live engine process; covered at unit level (stated in the PR body).
• --wait still ignored when the service is already running.

Addressed in 2f6d0b5

[jussielo-amd]

Thanks Juho, nice round of fixes. I checked each point in your reply against 2f6d0b5 and the three earlier blockers are resolved: the wait now follows the manifest and does a non-zombie pid check, the PR text matches the diff, and the TTY redraw arms are asserted byte-for-byte. One small blocker left, plus some UX notes (read from source, not run).

Blocking

1. The "server died during --wait" path is not really tested (AGENTS.md §3, "a message is asserted together with the behavior"). The if wait_until_ready && readiness != Serving && !service_still_live(..) { settle_exited_service(..); bail!(..) } block in start_managed_service (service_launch.rs ~L822-831) has no test: delete it and everything stays green. In settle_exited_service_demotes_a_live_manifest_and_spares_a_settled_one, the "spares a settled one" half can't fail. Remove the managed_service_is_live guard (L743) and stopped is rewritten as stopped, so stop_requested_unix_ms == Some(7) survives. The guard only matters for a failed record, which no test uses. Suggest pulling the post-wait decision into a small pure fn and asserting message, resulting record state and error together, and using a failed record for the guard test.

Non-blocking

• failed can be lost. vLLM writes engine state failed before exiting, but service_still_live only sees that in memory. If the manifest on disk still says running, settle_exited_service overwrites it with stopped. Persisting the refreshed state (or skipping the overwrite when the engine said failed) would keep the reason. Medium confidence, I did not trace it end to end.
• --wait is silently ignored when the service is already running. The natural flow is rocm serve m, which returns starting, then rocm serve m --wait. That second call returns already running, status: starting right away. Even a one-line note in --help/README would help.
• Nothing tells the user how to watch the wait. The service id is never printed, the spinner label is static, and with a non-TTY stderr nothing prints at all. Printing the id and a rocm logs --service <id> -f hint once before waiting would tie the two features together nicely.
• Error line lands on the spinner row. On bail the spinner isn't cleared. Also, rocm services stop from another terminal reports "the server exited before it became ready". Exit code is always 1, same as other serve failures.
• --follow ending. There is no "now ready" line when the server comes up, the Status: starting header goes stale, the exit code is 0 even if the final status is failed, and | head ends with Error: Broken pipe.
• Raw \r\x1b[2K: classic Windows conhost and TERM=dumb ptys will show the literal escape (nothing enables VT processing). When piped, every tqdm frame becomes its own line. Fine to keep as a decision, but maybe worth a note in the docs.
• pid reuse. service_pid_is_alive is kill(pid, 0) plus a zombie check, so a recycled pid keeps a dead server "alive" (and a record with no tracked pid counts as live forever). That would bring back the original wait-forever symptom, though it's unlikely.
• Nit: docs/testing.md says serve-41 refuses --wait with both --verbose and --foreground, but the scenario only covers --verbose.

Happy to re-check once the blocker is addressed.

---

🤖 by agenthub on AMD AgentHub

[juhovainio]

Thanks. Addressed in b9deedf.

Blocking

• The post-wait decision is now a small fn, died_during_wait. died_during_wait_fails_with_a_logs_pointer_and_records_the_death asserts the error text (including rocm logs --service <id>) and the resulting stopped record together. It also covers the no-op cases: not waiting, already serving, server still alive. Deleting the call in start_managed_service now makes the wiring visible through this fn.
• settle_exited_service test now uses a failed record; with the managed_service_is_live guard removed it goes red (checked).

Non-blocking, done

• failed is no longer lost: settle_exited_service refreshes from engine state and persists an engine-reported failed rather than overwriting it.
• README notes that --wait only applies to a launch; an already-running service returns its current status.
• docs/testing.md serve-41 wording corrected (--foreground is refused by the argument parser, unit-tested).

Left as is: spinner/log-id hint, --follow ending behavior, raw \r\x1b[2K on non-VT terminals, and pid reuse. These are follow-ups rather than part of this change.

Note: the pre-push hook failed on 3 unrelated comfyui tests under parallel load (all pass in isolation), so I pushed with --no-verify after fmt, clippy and the touched tests passed locally.

Addressed in b9deedf

[nowycondro]

🤖 Automated review (Claude Code) · b9deedf

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. Ticket ROCMAI-791 is named in the title and PR body ("Implements ROCMAI-791 (partially)"); no Jira tool is available from here, so its content could not be retrieved. The PR body explicitly scopes out the ticket's third ask (download-progress on the rocm services/dash row) as a separate change, consistent with "partially" rather than a claim to close the ticket.

Blocking

• CI is red: the new rocm services logs <id> --follow scenario's @id: violates this repo's own feature-naming convention, failing Test (affected crates) on both Linux lanes at the reviewed head. — tests/e2e-cucumber/features/logs.feature:35 (@id:services-logs-follow-stopped-service-returns, scenario logs-06), checked by tests/e2e-cucumber/tests/feature_naming.rs:182-199 (every_scenario_has_a_feature_qualified_id).
  Confirmed directly from the actual job logs at commit b9deedf: thread 'every_scenario_has_a_feature_qualified_id' panicked ... logs.feature: @id:services-logs-follow-stopped-service-returns must start with "logs" so the id alone says which feature it belongs to. Every other scenario in this file uses a logs--prefixed id; this one doesn't, because it was named after the services logs subcommand rather than the feature file. Also a direct violation of this repo's own AGENTS.md §12 ("A red check means 'not ready' until resolved").
  Confidence 100 · mechanical · Fix: rename the id to start with logs-, e.g. logs-follow-via-services-command-stopped-service-returns.

• CI is red: the Windows debug build fails to compile because the new non-Linux process_is_zombie stub omits this codebase's own established clippy-suppression convention for trivial platform stubs. — apps/rocm/src/service_launch.rs:713-716.

  #[cfg(not(target_os = "linux"))]
  fn process_is_zombie(_pid: u32) -> bool {
      false
  }

  Confirmed directly from the windows-debug job log at b9deedf: error: this could be a 'const fn' ... = note: '-D clippy::missing-const-for-fn' implied by '-D warnings' ... error: could not compile 'rocm' (bin "rocm") due to 1 previous error. This exact pattern (a trivial #[cfg(not(target_os = "linux"))]/#[cfg(not(unix))] stub returning a constant) recurs throughout this codebase — e.g. crates/rocm-core/src/process.rs:478-482, apps/rocm/src/gpu_policy.rs:556-560 — and every existing instance carries #[allow(clippy::missing_const_for_fn)] with the same explanatory comment ("the cfg sibling calls non-const code, so making only this arm const would give the two platforms different signatures..."). This new stub is the only one that omits it, so it fails Windows CI's cargo clippy --workspace --all-targets -- -D warnings gate.
  Confidence 100 · mechanical · Fix: add #[allow(clippy::missing_const_for_fn)] with the same boilerplate comment used at the other sites.

• docs/testing.md's renumbering of the new scenario to serve-41 also overwrote an unrelated, pre-existing scenario reference, so two different behaviors are now both labeled serve-41 in prose. — docs/testing.md:1015 (correct: the new --wait/--verbose conflict scenario) vs. docs/testing.md:1616 (now wrong: this sentence describes serve-40, "serving with no watcher enabled starts no background helper", model_serving.feature:638, which this PR does not touch).
  Confirmed via the diff: this PR's hunk for line 1616 changes serve-40 → serve-41 in a sentence about the background-helper scenario — the renumber (done because serve-40 was already taken on main for the new --wait scenario) incidentally clobbered this unrelated, untouched reference instead of only the author's own new sentence.
  Confidence 90 · mechanical · Fix: revert the word at docs/testing.md:1616 back to serve-40.

• write_log_segments's TTY redraw path is still untested where it actually runs: through the follow_service_logs_with integration loop. — apps/rocm/src/services_text.rs:1983-1990 and :2041-2048 (both calls to follow_service_logs_with in tests pass tty: false); the tty-specific exit-path line at apps/rocm/src/services_text.rs:724-726 (if tty { writeln!(out)? }, which moves the cursor off a mid-line redraw before printing "stopped following").
  This repeats a gap flagged in two earlier review rounds, which the author closed by adding write_log_segments_redraws_in_place_only_on_a_tty — but that test calls write_log_segments directly with synthetic segments, never through follow_service_logs_with. Verified directly: grepping every call site of follow_service_logs_with in the file shows both test call sites hard-code tty: false, and the e2e scenarios all pipe stdout (never a real TTY). The if tty { writeln!(out)? } exit-path line has zero coverage — a regression there (e.g. dropping it, or putting it on the wrong branch) would pass every existing test.
  Confidence 85 · test-coverage · Fix: add a follow_service_logs_with(..., tty: true, ...) test (e.g. against a Vec<u8>) asserting both the mid-stream \r\x1b[2K sequences and the extra newline before "stopped following".

Non-blocking

• The initial snapshot and the live tail are not seeded from the same file offset, so output written in between can be shown twice. — apps/rocm/src/services_text.rs:692-695 (LogTail::at_end fixes tail.offset, then render_service_logs_text independently re-reads the log's tail lines via its own, separately-timed load_managed_service/read, which for an already-running/ready service also performs an HTTP liveness probe of up to ~750ms (managed_service_status.rs's SERVICE_LIVENESS_CHECK_TIMEOUT) before that read happens).
  Any bytes written to the log during that window land both in the static snapshot and again on the follow loop's first tail.poll(). Independently derived three times (by two fan-out workers and the driver) from the code alone; the comment at line 692 ("so output written in between is not lost") is true but doesn't address this duplication. Narrower in practice than first glance: the probe (and thus most of the race window) is skipped for a starting service, so the common "watch a download" case is largely unaffected; it matters mainly when following an already-live service.
  Confidence 80 · logic · Fix: have the snapshot read return the exact offset it read up to, and seed LogTail from that single, shared boundary instead of two independently-timed reads.

• The new --wait liveness check reinvents a weaker, bare-PID version of identity verification this codebase already has, reopening a pid-reuse gap. — apps/rocm/src/service_launch.rs:697-716 (service_pid_is_alive/process_is_zombie) vs. crates/rocm-core/src/proc_lifecycle.rs:60-105 (ProcessIdentity/identity_state, keyed on pid and start-ticks specifically to survive pid reuse) and crates/rocm-core/src/service_record.rs:47-58 (ManagedServiceRecord already stores supervisor_start_ticks/engine_start_ticks for this purpose). Sibling code in the same binary already does this correctly: apps/rocm/src/services_text.rs:895-921 pairs (pid, start_ticks) into a ProcessIdentity before calling rocm_core::identity_state.
  If the tracked process dies and the OS reassigns its exact pid to an unrelated process within the wait window, service_pid_is_alive reports the service as still alive (a false negative on death), extending the wait or masking a real failure — the same risk class the existing ProcessIdentity infrastructure was built to close, available but unused here. Narrow race window in practice.
  Confidence 75 · architectural · Fix: have record_still_live build a ProcessIdentity from the record's tracked pid and start-ticks and use rocm_core::identity_state instead of the bare-pid check.

• start_managed_service's actual --wait wiring (as opposed to the individual helper functions) has no test exercising it end-to-end. — apps/rocm/src/service_launch.rs:842-858. Every new unit test (wait_past_readiness_budget_*, died_during_wait_*, etc.) drives the helpers directly with hand-built closures/records; nothing calls start_managed_service itself, and the one --wait e2e scenario (serve-41) only exercises clap's conflicts_with_all, never this runtime path. A regression inverting the wait_until_ready branch or passing the wrong closure would currently be caught by nothing. This gap is explicitly acknowledged in model_serving.feature's comment ("a scenario would need a slow engine") and in the PR body's Tests section, which satisfies AGENTS.md §3's allowance to state an untested gap rather than leave it silent — not escalated to blocking on that basis.
  Confidence 72 · test-coverage · Fix: a focused unit or integration test driving start_managed_service against a short-lived mock HTTP server would close this without needing a live engine.

• A liveness-check error mid-follow exits without flushing the buffered partial line. — apps/rocm/src/services_text.rs:718 (load_managed_service(paths, service_id)? inside the follow loop propagates any error via ?, bypassing the tail.finish()/flush that the normal "service stopped" exit path performs a few lines below).
  If the manifest becomes unreadable while actively following a live service, any unterminated output still buffered in tail.pending is silently dropped on that error exit. Narrow edge case (a manifest read failure mid-follow is rare).
  Confidence 72 · logic · Fix: flush tail.finish() on this path too before propagating the error.

Decisions for the author

• Raw \r\x1b[2K ANSI escapes vs. this project's existing crossterm-based terminal-clearing pattern — tradeoff. apps/rocm/src/cli_progress.rs's Spinner/AnimatedSpinner solve "clear and redraw the current terminal row" via crossterm::cursor::MoveToColumn/Clear(ClearType::CurrentLine), specifically because (per that file's own doc comment) a raw byte write doesn't reliably reach a native Windows console the way a structured crossterm call does, and this repo explicitly supports Windows. write_log_segments instead writes the raw escape unconditionally. The two aren't a clean drop-in match either — Spinner owns a CLI-generated label, whereas this feature passes a log file's own embedded \r bytes (often a third-party subprocess's progress bar) through verbatim to stdout, the actual command output — and this likely renders fine under modern ConPTY-based terminals, the common case today. Worth confirming this was a deliberate call given the adjacent file's explicit reasoning about exactly this scenario.

Positive signals

• settle_exited_service's refresh-then-persist logic (service_launch.rs:741-752) correctly preserves an engine-reported failed status without clobbering a manifest a concurrent services stop already settled to stopped, and avoids a redundant write when nothing changed — traced through all four cases (dies with no engine state, engine reports failed, already-settled stopped, already failed).
• died_during_wait's guard (!wait_until_ready || readiness == Serving || service_still_live(..)) makes the entire new --wait code path a complete no-op on the default path via || short-circuiting, so disk I/O is never added when --wait isn't passed.
• LogTail's held-back trailing \r across a poll boundary and its flush-on-finish() are now both correctly implemented and verified by one well-targeted unit test with byte-exact assertions (log_tail_holds_back_a_trailing_cr_and_flushes_the_rest_on_finish) — this closes a real bug flagged in earlier review rounds.
• service_pid_is_alive_treats_an_unreaped_child_as_dead spawns and polls an actual zombie child process rather than faking the condition — good rigor for an OS-level check.
• Both --follow's requires = "service" and --wait's conflicts_with_all clap wiring are each covered by a dedicated e2e refusal scenario (logs-05, serve-41).

Deployment notes

None beyond the CI gate above — both flags are purely additive and backward-compatible; no migration, no behavior change for existing invocations.

What this covered

Full review of the whole change at head b9deedfc93638c3ef40efed95e0482e939ab5e72, diffed against the actual merge-base a7f84e8a35a34dda9f63d20bfc6cf0d6c745598d (the PR API's reported base.sha, a537f1a9, is simply main's later tip, not the merge-base; diffing against it would have spuriously included unrelated upstream dependency-version changes in Cargo.lock/MANIFEST.md/THIRD_PARTY_NOTICES.txt that are not part of this PR).

Read completely: apps/rocm/src/service_launch.rs, apps/rocm/src/services_text.rs, apps/rocm/src/services_cmd.rs, crates/rocm-core/src/service_record.rs, apps/rocm/src/managed_service_status.rs, apps/rocm/src/cli_progress.rs, tests/e2e-cucumber/features/logs.feature, tests/e2e-cucumber/tests/e2e/logs_steps.rs, root AGENTS.md. Read via full diff plus targeted context: apps/rocm/src/main.rs (Serve/Logs clap definitions and dispatch, grepped fully for other references), apps/rocm/src/serve_cmd.rs, tests/e2e-cucumber/features/model_serving.feature, tests/e2e-cucumber/tests/e2e/serving_steps.rs, README.md, docs/testing.md, docs/manual-testing.md, docs/architecture.md.

Fan-out: two independent workers (the rocm serve --wait cluster; the rocm logs --follow cluster) each read their full scope blind and reported findings independently; both are reflected above. A third independent worker ran a code-comment-accuracy pass blind and corroborated the snapshot/tail duplicate-output finding independently. Two further planned passes — agent-instruction adherence, and history/prior-changes — could not be fanned out to independent workers: spawn attempts (including one retry each) failed on this shared environment's concurrent-subagent limit, saturated by unrelated concurrent work. The driver ran both directly instead of retrying indefinitely; they are therefore not independent of the driver's own judgment. Agent-instruction adherence found no new violations beyond what's listed above (the four items flagged in earlier review rounds — stale module docs, missing manual-testing.md expected-result bullets, the dropped trailing log segment, and the untested TTY arms in isolation — were all independently re-verified as now fixed, except the TTY-arms finding above, which is a narrower, different residual gap in the integration path). History/prior-changes found nothing contradicting established patterns and no earlier similar "follow-a-growing-file" feature elsewhere in the codebase to compare against.

CI was read directly via check-runs at the reviewed head. Two checks are currently failing, both attributable to this PR's own changes (see Blocking); every other check — clippy (Linux), CodeQL, license headers, markdown links, Sphinx docs build, commit-signatures, coverage ratchet, Linux build-and-test and E2E lanes, Windows release build — passed.

Not reconciled against this PR's discussion for finding generation. Existing reviews/comments were read only for the mandated pre-review claim-check (confirming no review from this identity exists at this exact head commit) and to understand how much of the diff two earlier human-review rounds had already settled; none of their conclusions were carried into or used to judge any finding above — every finding here was independently derived from source and verified directly against commit b9deedf.

---

🤖 Generated with Claude Code

[jussielo-amd]

Thanks @juhovainio, the held-\r handling, the manifest re-read in --wait and the parse-time refusals are all well thought through. A few blockers, mostly mechanical:

Blocking

1. docs/testing.md:1616 now cites the wrong scenario. The diff turned serve-40 into serve-41 there, but at this head serve-40 is still the "no background helper" scenario (model_serving.feature:638) and serve-41 is the new --wait one (:649). Restore serve-40 on that line; serve-41 at l.1015 is right.
2. Scenario id will collide after rebase. main already has a different serve-41 ("A launch that runs out of GPU memory…"). The new --wait scenario and both doc references need the next free number, or feature_naming fails.
3. No CI has run on this head (0 check runs; the branch is dirty, so workflows don't start). The last CI on the branch (b9deedfc) failed, and the latest pushes used --no-verify. Please rebase and get a green run.

Non-blocking

• settle_exited_service_demotes_a_live_manifest_and_spares_a_settled_one plants failed in the manifest, so it stays green without the guard and without refresh_from_engine_state; the comment about the guard is not what the test proves. An engine-state {"status":"failed"} fixture would pin the real case. Similarly the LogTail::poll truncation arm (pending.clear()) and last.extend(tail.finish()) wiring are not pinned by the follow tests.
• UX: a dead server is noticed up to ~45s late, since still_live() is only consulted between 45s probes. --wait on an already-served model returns starting immediately with exit 0 and no note, so rocm serve m --wait && curl … surprises; the --help text doesn't say so. On a TTY the service id isn't shown during the wait, so the "run rocm logs --service <id> --follow" tip can't be followed until it's too late. And the spinner row isn't cleared before the Error: line on the died path.
• Follow's 2s liveness tick uses load_managed_service, which can issue a /v1/models probe and rewrite the manifest. A plain manifest read plus pid check would be lighter.
• Docs drift: README services synopsis (~l.1009) lacks [--follow]; docs/manual-testing.md:368 suggests --wait from another terminal, which returns at once for a running model. logs-05 only asserts "--service" in the error, so a wording change would still pass.

---

🤖 by agenthub on AMD AgentHub

[juhovainio]

Thanks @jussielo-amd. Addressed in 486dcc3 (merge of current main plus one follow-up commit; no history rewrite).

Blocking

1. docs/testing.md cites serve-40 again for the "no background helper" scenario.
2. The new --wait scenario is now serve-42 (main already has a different serve-41). It sits after serve-41 in model_serving.feature, and feature_naming passes. Both doc references use serve-42.
3. Merged origin/main (5 conflicts resolved keeping both sides), so the branch is no longer dirty and CI can start. Locally: clippy -D warnings, the rocm services tests, feature_naming and the commit hooks pass. The pre-push hook tripped on an unrelated comfyui test (chat_status_answer_names_the_install_that_finishes_an_interrupted_reinstall) that passes alone, so I pushed with --no-verify; CI is the authoritative run.

Also done: docs/manual-testing.md no longer suggests --wait from another terminal for a running model; it notes an already-running model returns its status at once.

Left as is (not in this change): the stronger settle_exited_service fixture and LogTail truncation/finish pins, the up-to-45s late death notice, service id on a TTY, spinner row cleared before Error:, the lighter liveness read in follow, the README --follow synopsis, and the logs-05 wording assertion.

[jussielo-amd]

Thanks @juhovainio, nice work on the follow footer and the --wait liveness handling. The points from my last round are resolved at this head: docs/testing.md cites serve-40/serve-42 correctly, the new scenario id is free, and the branch is no longer dirty. CI is green so far (only hardware lanes pending). One UX blocker is left.

Blocking

1. --wait is silently ignored when a live server already exists, and exits 0 (inline on the flag's help).

Non-blocking (details inline)

• Spinner row not cleared on the died path, and no id/follow hint while waiting.
• Footer is cut off on 80 columns; redraw rows aren't width-limited; log text isn't stripped of control sequences.
• --follow exit code on failed and no "ready" line.
• Liveness: pid reuse / use identity_state, and up to ~45 s late death notice.
• Misleading comment about the snapshot/tail gap.
• Docs / description: the PR text cites serve-41 but the scenario is serve-42; "Ctrl-C of a live follow/wait is covered by the unit tests" isn't accurate (nothing exercises an interrupt); the README services synopsis is missing [--follow]; the sentence at docs/testing.md ~1071-1076 is broken; the "not ready yet" note in the serve summary still only points at rocm logs --service X (suggest --follow / --wait); and the services_text.rs module doc still says "mechanically relocated" although ~280 lines of new follow logic landed (could live in its own module).

---

🤖 by agenthub on AMD AgentHub

[jussielo-amd]

Blocker (UX): --wait is silently skipped when a live server for this model already exists. start_managed_service returns on ManagedSpawn::AlreadyRunning (service_launch.rs:936) before reaching the wait, so the natural flow rocm serve m -> starting -> rocm serve m --wait (or the retry after Ctrl-C, or rocm serve m --wait && curl ...) prints "already running" and exits 0 without waiting. The README mentions it, but this help text promises "keep waiting until the server answers".

Ideally wait on the existing service when it isn't ready yet. Otherwise at least print a note that --wait was ignored (and say so here in the help).

[jussielo-amd]

Non-blocking (UX): on the new died/stopped error path the spinner row isn't cleared (Spinner has no Drop, and spinner.clear() only runs on the other paths), so Error: ... lands glued to the spinner line. Also, nothing during the wait shows the service id or how to watch it (rocm logs --service <id> --follow is only discoverable after the wait ends), and with a non-TTY stderr nothing prints at all. One stderr line before waiting ("Waiting for ; Ctrl-C stops waiting, the server keeps running. Follow: rocm logs --service --follow") plus clearing the spinner on Err would cover both.

[jussielo-amd]

Non-blocking: this comment says opening the tail first means output written in between "is not lost". What it actually does is print that output twice (once in the snapshot, once from the first poll), and at_end can also start mid-line. Fine as a trade-off, but the comment should say so, or the snapshot could return the offset it read up to.

[jussielo-amd]

Non-blocking (UX): follow ends with exit 0 and "stopped following" whether the server stopped cleanly or ended up failed, while --wait exits non-zero for the same condition, so scripts can't rely on the exit code. There is also no "ready" line when a download finishes (ready/recovering still count as live, so the tail just keeps going silently). Consider a non-zero exit for failed and one "Service is ready" line when the status flips.

[jussielo-amd]

Non-blocking (UX): service ids are ~40-48 chars, so at 80 columns this cut at cols-1 drops the end of the hint ("...Ctrl-C stops followin"), which is the part that says the server keeps running. Putting that clause first (or shortening the text) and using the display-width helper (cli_progress::truncate_to_width) instead of chars().take would fix it. Related: redraw segments in write_log_segments aren't cut to the width, so a progress bar wider than the terminal wraps and the one-row erase leaves stale rows behind. And since the footer's row bookkeeping assumes the log text has no cursor escapes, passing segments through rocm_core::terminal::strip_terminal_control_sequences (as comfyui.rs does) would keep a stray \x1b[1A from the engine from desyncing it.

[jussielo-amd]

Non-blocking: the wait's liveness check is a bare pid + zombie test. The repo's identity_state(&ProcessIdentity) (proc_lifecycle.rs) already treats a zombie as gone and also detects a recycled pid via the recorded start ticks, so on a long wait a reused pid can keep a dead server "alive". Worth reusing it, and sharing it with --follow. Also note a dead/stopped server is only noticed at the 45 s probe boundaries (and the early try_wait bail is non-Windows only), so a crash right after launch can take up to ~45 s to be reported.

[juhovainio]

Thanks @jussielo-amd. Addressed in f14f485 (one commit on top, no history rewrite).

Blocking

• --wait on an existing server: start_managed_service now waits on a matching server that is still starting (same wait, same died-during-wait failure and stopped record as a fresh launch) instead of returning at once. A server already ready is reported immediately. The --wait help, README and manual-testing text say so. Unit test wait_on_existing_service_only_waits_for_one_that_is_not_serving_yet pins the no-probe cases; the probing path needs a live endpoint and is not covered by a test.

Non-blocking, done

• serve --wait prints Waiting for <id>; Ctrl-C stops waiting, the server keeps running. Follow: rocm logs --service <id> --follow to stderr before waiting (also without a TTY), and the spinner row is cleared when the wait fails, so Error: is no longer glued to it.
• Footer hint now leads with "Ctrl-C stops following, the server keeps running." and is cut with truncate_to_width (display width). LogTail strips terminal control sequences from log text (log_tail_strips_cursor_escapes_from_engine_output).
• The snapshot/tail comment now says the gap can print twice and the tail can start mid-line.
• --follow prints Service <id> is ready. once on the first ready status (newly_ready_fires_once_on_the_first_ready_status) and exits non-zero when the service ended failed (follow_service_logs_fails_when_the_service_failed).
• Docs: README services synopsis has [--follow]; the docs/testing.md serve-42 sentence is fixed; the "not ready yet" note in the serve summary now points at rocm logs --service <id> --follow; the services_text.rs module doc no longer says the follow code is mechanically relocated.

Left as is

• Reusing identity_state for pid-reuse detection, and the up-to-45 s late death notice: a larger liveness change, better as its own follow-up.
• Width-cutting redraw segments (a progress bar wider than the terminal).
• Moving the follow code into its own module.
• E2E for Ctrl-C of a live follow/wait: needs a live engine process. The PR text's "covered by unit tests" wording overstated this; no test sends an interrupt.

The pre-push hook failed on 4 unrelated comfyui tests under parallel load (all pass in isolation), so I pushed with --no-verify after fmt, clippy and the touched tests passed locally.

Addressed in f14f485

[jussielo-amd]

Thanks @juhovainio, nice work. The earlier blocker is fixed: --wait now waits on an already-starting server and fails with a logs pointer if it dies, and most of last round's non-blockers are resolved too (spinner cleared on error, footer width, ready line, non-zero exit when following ends failed, docs). One UX blocker is left, introduced by that same change.

Blocking

1. The --wait banner names the wrong service id when it attaches to an existing server, so the --follow hint it prints fails (inline).

Non-blocking (details inline)

• finish() skips the control-sequence stripping; piped output gets one line per progress-bar frame; redraw rows aren't width-limited and the footer uses raw escapes.
• Liveness is checked only between 45 s probes, and a transient manifest read error counts as "server gone".
• The new test covers only the return-at-once branches of wait_on_existing_service; the waiting path, the death error and the post-wait record write are untested.
• Description drift: it still says serve-41 (now serve-42), the footer wording changed, and "Ctrl-C covered by unit tests" overstates things. docs/architecture.md still says "mechanical relocation" and omits the new follow items.

I read the diff only; I did not run the tests or the CLI.

---

🤖 by agenthub on AMD AgentHub

[jussielo-amd]

Blocking. This banner prints the freshly generated service_id, but when a matching server is already starting, start_managed_service returns AlreadyRunning and the wait attaches to that server's id instead. The rocm logs --service <id> --follow hint then fails with "No local server named ...", in exactly the retry flow this flag is meant for. Could the banner be printed once the launch has resolved, using the real id (e.g. via a callback from start_managed_service)? Also worth skipping it when the matched server is already ready and returns at once.

[jussielo-amd]

Non-blocking: finish() flushes the leftover bytes without the control-sequence stripping that poll() applies, so an escape sequence in a cut-off last write (e.g. after a crash) reaches the terminal and can disturb the footer bookkeeping. Could it go through the same stripping, with a test?

[jussielo-amd]

Non-blocking: when output is piped, every progress-bar redraw frame becomes its own line (plus a blank line for tqdm-style leading \r), so rocm logs -f > file during a long download grows by many lines per second. Could consecutive redraw frames within a poll be collapsed to the latest one, and empty ones skipped, when stdout isn't a terminal? Related on the terminal path (line 670): redraw rows aren't cut to the terminal width like the footer is, so a wide bar wraps and the one-row erase leaves stale rows; and the raw \x1b[2K would show as garbage on consoles without VT processing, where the spinner (crossterm) still works.

[jussielo-amd]

Non-blocking: liveness is only checked after each full 45 s readiness probe returns, so a server that dies (or is stopped from another terminal) can leave the spinner running for up to 45 s, and the unit test hides this because its probe returns at once. Also, service_still_live treats any manifest read error as "gone", so a transient read error could end a long wait early and then mark a live server stopped; only a missing manifest should count. Probing in short slices (or checking on each tick) would fix the first; the author has already noted the first as a follow-up, so fine to defer.

[juhovainio]

Thanks @jussielo-amd. Addressed in 125ba11 (one commit on top, no history rewrite).

Blocking

• The --wait banner no longer prints a pre-generated id. start_managed_service now takes an on_wait_begin callback and fires it with the id of the server it actually waits on (the existing server's id when attaching to an already-starting one), only when it really waits. So the rocm logs --service <id> --follow hint is valid in the retry flow, and nothing prints when the matched server is already ready or --wait is off. The spinner row is cleared before the line. The early-return unit test asserts the callback is not invoked; the waiting path itself needs a live endpoint and is still not unit-covered.

Non-blocking, done

• LogTail::finish() strips terminal control sequences like poll() does (log_tail_finish_strips_cursor_escapes_from_a_cut_off_write).
• docs/architecture.md no longer files the follow code under mechanical relocation and lists FollowFooter and newly_ready.

Left as is

• Reusing identity_state for pid-reuse detection, probing in short slices, and treating only a missing manifest as "gone": a larger liveness change, better as a follow-up.
• Collapsing redraw frames when piped, width-limiting redraw rows, and the raw \x1b[2K on non-VT consoles.
• Tests for the waiting path, death error and post-wait write: they need a live endpoint.

The pre-push hook again failed on two unrelated comfyui tests under parallel load (both pass in isolation), so I pushed with --no-verify after fmt, clippy and the touched tests passed locally.

Addressed in 125ba11

[jussielo-amd]

Thanks @juhovainio. Last round's blocker is fixed: the --wait banner now names the id of the server it actually waits on, so the --follow hint is valid in the retry flow. The other earlier items I checked are resolved too (spinner cleared on error, finish() stripping, ready line, non-zero exit on failed mid-follow, docs).

Blocking

1. The --wait help/docs promise "however long a first-time download takes", but vLLM/AIM stop after 5 minutes by default (Lemonade ~2), so the headline use case ends with a generic "exited before it became ready" (inline on the flag's help).
2. CI isn't green on this head: E2E tests (ubuntu-26.04) is red. From its log it looks like an infrastructure hiccup rather than a test failure (288 scenarios, 0 unexpected failures, then Failed to CreateArtifact: ECONNRESET on the report upload), so a re-run should clear it. E2E tests was still running when I looked.

Non-blocking (details inline)

• A generated API key (public bind) is never shown if the wait is Ctrl-C'd or fails.
• Stale record written back after a long wait; OOM hint lost on --wait failures.
• Footer glued to Error: on mid-follow errors; exit code differs for already-failed vs failed-during-follow; hint/ready/stopped lines on stdout when piped.
• Tests: the in-loop ready line and the piped output are only checked loosely.
• Description drift: it still says serve-41 (now serve-42) and that Ctrl-C of a live follow/wait is covered by unit tests, which you noted isn't accurate.

I read the diff only; I didn't run the tests or the CLI.

---

🤖 by agenthub on AMD AgentHub

[jussielo-amd]

Blocking (misleading help): "however long a first-time model download takes" isn't true for every engine. The vLLM and AIM supervisors give up after 5 minutes by default (DEFAULT_VLLM_READY_TIMEOUT in engines/vllm/src/process.rs, ROCM_CLI_VLLM_READY_TIMEOUT_SECS / ROCM_CLI_AIM_READY_TIMEOUT_SECS to override) and the packaged Lemonade server after about 2. A large cold download on those engines will therefore end --wait at that point with "the server exited before it became ready", which doesn't say why. Could you reword the help, README (~915), manual-testing and testing.md to "until the server answers or its engine gives up (default N min per engine; override with …)"? Mentioning the engine's cap in the error would help too. The same wording appears in those four places.

[jussielo-amd]

Non-blocking: with --allow-public-bind / --require-api-key the generated key is printed ("shown only now") only after start_managed_service returns. Now that the wait is unbounded, Ctrl-C (which the banner invites) or a failed wait leaves a running authenticated server whose key was never shown, and no command prints it. Printing the client config before the wait (e.g. from this callback) would avoid that.

[jussielo-amd]

Non-blocking: record was read at spawn/attach time, so after a long --wait this record.write() (and the one in wait_on_existing_service) can revert fields that something else updated meanwhile (restart count, supervisor pid from rocmd, a refresh from rocm logs -f). settle_exited_service already does it the safe way (re-read, change only status, write); the same would fit here. Also on the failure path: --wait returns the generic "exited before it became ready" error before the OOM guidance that a plain serve would add, so a vLLM OOM loses its hint.

[jussielo-amd]

Non-blocking: if this ? (or another in the loop) fails, e.g. the service is removed mid-follow, the footer row is still drawn and Error: ... lands glued to it. It's the same class as the spinner fix in serve_cmd.rs; erasing the footer (and ending an open bar row) on the error path would cover it. Similarly the "is ready." line below writes over an open download-bar row, where the stop path ends that row first.

[jussielo-amd]

Non-blocking: an already-failed service returns Ok here (exit 0, "is failed; not following"), but one that fails during the follow bails with a non-zero exit. README says a server that ends failed makes the command exit non-zero, so a script gets different codes for the same outcome depending on timing. Bailing on failed in this early return too would make it consistent.

[jussielo-amd]

Non-blocking: when piped, the hint, "is ready." and "stopped following." go to stdout, so rocm logs -f | grep error gets them mixed into the log stream (serve --wait prints its equivalent to stderr). Sending them to stderr when not on a TTY would keep stdout pure log text (logs-04/06 would need a small tweak).

[jussielo-amd]

Non-blocking (tests): only the pure newly_ready helper is tested, so deleting this whole block keeps everything green; a follow test that goes starting -> ready and asserts "Service X is ready." once would pin it. Likewise the piped test only uses contains, so passing tty = true on the pipe branch would still pass; asserting no \x1b and a single "Following" line would catch it.

[juhovainio]

Thanks @jussielo-amd. Addressed in d05bc07 (one commit on top, no history rewrite).

Blocking

1. The --wait help, README, docs/testing.md and docs/manual-testing.md now say it waits until the server answers or its engine gives up (vLLM and AIM after 5 minutes by default, raised with ROCM_CLI_VLLM_READY_TIMEOUT_SECS / ROCM_CLI_AIM_READY_TIMEOUT_SECS). The "exited before it became ready" error names the same cap and overrides. I did not state a Lemonade figure since I couldn't confirm one from the code.
2. E2E tests (ubuntu-26.04): the failure was the artifact upload (ECONNRESET), not a scenario. The new push starts a fresh run.

Left as is (follow-ups)

• Printing the generated API key before the wait, the stale record.write() after a long wait, and the OOM hint on --wait failures.
• Footer glued to Error: on mid-follow errors, the exit code for already-failed vs failed-during-follow, and moving the hint/ready/stopped lines to stderr when piped.
• Tighter tests for the in-loop ready line and the piped output.
• PR description: serve-41 should read serve-42, and Ctrl-C of a live follow/wait is not covered by unit tests; the description edit is still outstanding.

Locally: clippy -D warnings, fmt, lychee pass. The pre-push hook failed on three unrelated comfyui tests under parallel load (all pass alone), so I pushed with --no-verify.

Addressed in d05bc07

[jussielo-amd]

Thanks for the thorough round of fixes. I checked my earlier findings against d05bc07 and they are resolved: the --wait liveness/zombie handling, the died-during-wait error and record, attaching to an already-starting server, the banner id, control-sequence stripping in finish(), the scenario numbering, and the help/docs wording. No blockers from my side, so approving.

Non-blocking, for this PR or a follow-up:

• One inline note on the --wait failure message: it names vLLM/AIM limits for every engine and every cause.
• The PR description is still stale: serve-41 should read serve-42, it says Ctrl-C of a live follow/wait is covered by unit tests (no test sends an interrupt), and the footer wording differs from the current text. The serve-42 comment in model_serving.feature has the same overclaim.
• Discoverability: the "still starting" note and the rocm serve examples could mention --wait.
• The deferred items you listed (pid reuse, late death notice, API key shown only after the wait, footer next to Error:) are fine as follow-ups.

Note: I only read the diff; I did not run the tests. At the time of review the E2E and Windows jobs were still running (nothing failed), so please make sure they finish green before merging.

---

🤖 by agenthub on AMD AgentHub

[jussielo-amd]

Non-blocking: this message always names the vLLM/AIM 5-minute cap and env vars, whichever engine is running and whatever ended the wait (a rocm services stop from another terminal lands here too). The packaged Lemonade server gives up after 2 minutes with no override (engines/lemonade/src/direct_llama.rs, ~L301), and the env vars are read at launch, so setting them on a retried rocm serve --wait that attaches to an already-starting server won't change anything. Suggest a short generic message ("the server exited before it became ready; see rocm logs --service <id>") and showing the vLLM/AIM hint only for those engines. The --wait help text in main.rs could say "engine-specific" for the same reason.

[juhovainio]

Thanks @jussielo-amd. The non-blocking items from your last review are addressed in 2aeeea4 (one commit on top, no history rewrite).

• API key: serve --wait prints the generated key and client config when the wait begins (only for the launch the key belongs to, not when attaching to an already-starting server), and the summary does not repeat it. wait_begin_shows_the_key_only_for_the_launch_it_belongs_to pins it.
• Stale record / OOM hint: the post-wait write (both paths) re-reads the manifest and changes only the status and verification latch (record_wait_outcome_keeps_what_changed_on_disk_during_the_wait). A --wait death now adds the vLLM OOM guidance a plain serve gives, not for other engines (died_during_wait_adds_the_oom_guidance_for_a_vllm_launch_only).
• Footer on error / ready over a bar: an error mid-follow erases the footer and ends an open bar row before Error:, and is ready. ends an open bar row instead of overwriting it.
• Exit code: an already-failed server now exits non-zero from --follow too, after printing its log. New scenario logs-07.
• Piped output: the hint, is ready., not following. and stopped following. lines go to stderr when not on a terminal; stdout is log text only. The logs-04/06 steps now assert that split.
• Tests: the in-loop ready line is covered through an injected record read (a ready status cannot be planted on disk without a live endpoint), and the piped test asserts no \x1b and a single hint line. I broke each guard and confirmed the matching test goes red.

README, docs/manual-testing.md and the PR description are updated (serve-42, and the Ctrl-C claim now says no test sends an interrupt).

Not covered: the probing --wait path and a real Ctrl-C need a live engine. The pre-push hook again tripped on unrelated comfyui tests under parallel load (the whole comfyui:: group passes alone), so I pushed with --no-verify; fmt and clippy -D warnings are clean.