[volen-silo]

Problem

After rocm serve and rocm services stop, a detached rocm daemon kept running indefinitely on native Windows, doing nothing.

Scope of this PR: the daemon only. ROCMAI-827 also reports LemonadeServer.exe --silent (run from %LOCALAPPDATA%\lemonade_server) staying alive. That process is not started by rocm-cli: a separate Lemonade install puts a Startup-folder shortcut that launches it at logon, nothing in rocm-cli references that path, rocm-cli's Lemonade lives in its own data folder, and rocm-cli neither starts nor stops it, so this PR leaves it alone and the ticket should stay open for it. The ticket's other accepted outcome, saying plainly what remains and how to stop it, is done for the daemon: rocm services stop (and the Ctrl-C stop in attached serve) prints a note: line, and the rocm uninstall plan warns, when a helper is running or a watcher is enabled.

Root cause

ensure_background_helper_running_quiet (apps/rocm/src/service_launch.rs) spawned rocm daemon on every serve, whatever the config said, and the daemon loop (apps/rocmd/src/daemon.rs) ran until signalled. All automation watchers are off by default, so with nothing enabled the daemon had no work, and nothing ever stopped it. The daemon's own status text already describes the intended design as "on-demand by default, persistent only with background features".

Change

• The start-up code shared by serve and automations enable starts the daemon only when an automation watcher is enabled. automation_daemon_enabled() now means "any watcher is enabled", which is what the spawn gate, the exit, the "background checks: on/off" display and the wording in the README and log lines all say; the stored daemon_enabled flag alone no longer counts. I checked every reader of that flag and of the predicate. The flag is written only by automations enable (sets it) and disable (clears it when no watcher remains), in apps/rocm/src/automations.rs, so for any config the CLI produced the flag and the watchers already agreed. The predicate's readers are the automations status text and render_daemon_text (apps/rocm/src/automations.rs), rocmd status and the loop's exit check (apps/rocmd/src/daemon.rs) and the autostart gate (apps/rocm/src/service_launch.rs). The chat and MCP automations tools and the Codex bridge snapshot read the runtime state and event log, or serialise the raw config, so they never consulted the flag, and rocm dash never references it. Only hand-edited configs with the flag set and every watcher off behave differently: they used to spawn a no-op daemon and now start nothing.
• The daemon re-reads the config every 5 s and exits once no watcher is enabled, marking itself not running. Disabling the last watcher is the documented way to stop it. No process is signalled, so the PID-identity safeguards on service stop are untouched.
• Exit race closed. The exit is decided under the autostart lock: the daemon re-reads the config after taking it, and only if still no watcher is enabled publishes "stopped" and clears its own spawn claim before releasing it. The autostart path no longer reads "already running" without that lock. With enable's existing order (save the config, then ensure the helper), an enable that lands while the daemon is about to exit either makes it stay or finds it stopped and starts a new one, instead of leaving a watcher enabled with no daemon.
• The daemon takes that lock with a non-blocking FileLock::try_acquire polled every 10 ms on the runtime, not a blocking acquire inside the async tick. After a bounded 30 s wait it gives up, stays up, and retries on the next tick, so a stalled lock holder cannot block a runtime worker or the webhook task, and the exit-race guarantee still holds.
• The idle-exit lock wait is its own select! arm, so the webhook-dispatch and shutdown arms stay live while it waits (a tick that finds the config idle starts one wait, and a tick that finds a watcher enabled abandons it); automation_loop_shuts_down_while_waiting_for_a_held_autostart_lock shuts the loop down with the lock still held.
• The loop's tick period and two callbacks are injectable (LoopHooks), so the loop tests run in milliseconds and reproduce the exit window deterministically. The two lock tests wait on an observed event (the loop found the lock held; the enable call queued behind it) instead of fixed sleeps.
• rocm services stop (and the Ctrl-C stop in attached serve) prints a note: line, and the rocm uninstall plan adds a warning, built from two facts: whether a watcher is enabled and whether the helper is running. Enabled and running: "keeps running ... rocm automations disable <watcher>" (the uninstall text adds "uninstall does not stop it"). Enabled but not running: it says so and that the next rocm serve or rocm automations enable starts it. Not enabled but running: it exits on its own (no uninstall suffix). Neither: no note. An unreadable config counts as "no watcher", so serve cannot fail after it has launched the child. The assistant's stop_server tool and the dashboard's stop action run rocm services stop, so they carry it; services restart keeps the service, services remove and prune only delete records, and the MCP stop_server tool in rocmd stops in-process and cannot call the rocm crate, so adding the note there would duplicate the logic and is left as a follow-up.
• Docs: rocm daemon --help (no longer lists GPU metrics and the webhook listener as unconditional work; says rocm dash does not need it), the rocmd run flag help, banner and status text (nothing promises a persistent supervisor any more), docs/automations.md (rocmd run --automations-enabled --local-webhook-port 18080 on a default config now exits within seconds, so it says to enable a watcher first), a new lifecycle section in docs/manual-testing.md, docs/architecture.md, README (Automations, and crash restarts noted as needing server-recover), a misleading comment in the attached-serve path, docs/testing.md (filter names fixed, README sentence rewrapped).

Nothing relied on the always-running daemon. rocm dash uses its own embedded daemon (rocm-dash-daemon) and never reads the automation runtime state; the service records of a server that exited are updated by its own supervisor and refreshed when rocm services or the dashboard reads them; the watcher handlers (GPU metrics, server-recover) run only for enabled watchers. The remaining readers of the daemon's state (automations status, the chat and MCP automations tools) only display it and tolerate "not running". The idle daemon also used to collect a GPU snapshot every 60 s for nothing.

Behaviour changes:

• A saved config with automations.daemon_enabled=true and no watcher enabled used to mean a persistent daemon. It now means nothing: the helper starts only while a watcher is enabled, and a running one exits on its first tick without one. The same goes for a foreground rocm daemon or rocmd run --automations-enabled used for debugging, which exits within a tick unless a watcher is enabled (docs/automations.md says to enable one first). The flag is still written by automations enable and disable, so CLI-produced configs were already consistent.
• Nothing is lost when no watcher is enabled. Every duty of the helper beyond its idle tick was already watcher-gated (therock_update_due, server_recover_due, gpu_metrics_due, gpu_thermal_protect_due, and evaluate_watchers_for_events skips disabled watchers). The helper never reconciled self-exited records on its own, because persistence::load_managed_services in rocmd is read-only: a self-exited server's record is settled by its supervisor (supervise_service calls record_supervised_exit, apps/rocmd/src/supervise.rs) and on every read (refresh_managed_service_runtime_liveness, apps/rocm/src/managed_service_status.rs). The stale comment in run_attached_service saying otherwise is replaced.
• Running rocm daemon by hand with no watcher enabled now prints its banner and exits instead of idling. Daemons already started by an older binary keep running until ended by hand.

Tests

• Unit (rocm, rocmd): the spawn gate in both directions; the loop persisting over several ticks while a watcher is enabled and exiting on the tick after it is disabled; a watcher enabled inside the exit window keeps the loop alive; the exit waits for the autostart lock and clears its claim; and an enable that finds the exiting daemon already stopped starts a new one.
• Each new test goes red when its target is broken (restored after each):

Break	Test that fails
remove the config re-check under the lock	automation_loop_stays_when_a_watcher_is_enabled_inside_the_exit_window
remove the lock acquire	automation_loop_exit_waits_for_the_autostart_lock_and_clears_its_claim
remove the claim release	the same test (claim assertion)
make the exit condition always true	automation_loop_keeps_running_while_automations_are_enabled and the exit-window test
put back a lock-free "already running" read	background_helper_is_started_when_the_exiting_daemon_publishes_stopped
make the predicate also count the stored flag	background_helper_not_spawned_when_no_automation_is_enabled
remove the spawn gate	background_helper_not_spawned_when_no_automation_is_enabled

• Scenarios:
  • automations-11 (@id:automations-helper-stops-when-no-watcher-is-enabled, mock lane, runs in the required E2E tests job; the harness teardown now stops a helper it left behind if an earlier step fails, verified by making the scenario fail while the helper was alive): enabling a watcher starts a live daemon and prints the helper: line; disabling it makes that process exit and rocm daemon --status reports it not running. This is the only scenario for the watcher-enabled-starts-the-helper direction.
  • serve-40 (@id:serve-stop-leaves-no-background-helper, @requires-gpu @requires-engine:lemonade): serving with a fresh config starts no daemon, prints no helper line, and leaves no autostart claim file (the spawn writes it synchronously, so the check cannot pass before a fresh daemon has published state). It runs only on the GPU lanes and asserts only the no-watcher case.
• cargo nextest run -p rocm -p rocmd (1444 passed), clippy for rocm, rocmd and e2e-cucumber (including --test e2e), cargo fmt --check, the automations scenarios against debug builds: pass.

Notes

• serve-40 is the next free number on main (after serve-36..serve-39), so a later merge may renumber it. automations-10 is taken on main by the torn-audit-tail scenario, hence automations-11 here.
• The two E2E tests (Strix Halo, Ubuntu, …) lanes ran out at 45 minutes with no logs on this PR, and the same two lanes fail the same way on several unrelated open PRs.

Not covered by a scenario

The services stop note has unit tests for the message and the state but no scenario, because it needs a real service record.

[juhovainio]

Nothing here is broken in the code I could see, but I'm requesting changes because the PR only covers half of ROCMAI-827 and a few claims need tightening.

Scope vs the ticket. The ticket reports that rocm services stop leaves both the daemon and LemonadeServer.exe running, and accepts either stopping helpers once nothing needs them or saying plainly that they remain and how to stop them. This diff handles the daemon only. LemonadeServer.exe is untouched, services stop prints nothing about leftover helpers, and the "background processes are not stopped automatically" text in rocm uninstall is unchanged. Please either cover the remaining process / messaging, or narrow the PR description to "daemon only" and keep the ticket open for the rest.

Behavior change to confirm. With no watcher enabled (the default), the daemon no longer runs, so self-exited server records are no longer reconciled and nothing feeds the dashboard from it. Please confirm the dashboard and that reconciliation have another source, and update the rocm daemon help text, which still lists metrics and the webhook listener as unconditional (inline comment).

Race. The daemon exits on its next 5s tick after the last watcher is disabled. If a watcher is re-enabled inside that window, the already-running check still sees the old process and spawns nothing, then the daemon exits, leaving a watcher enabled with no daemon. Same shape if the daemon starts before the enabling config is saved. Worth confirming the ordering in the enable path and/or adding a re-check.

Tests. automation_loop_keeps_running_while_automations_are_enabled only proves the first tick (the interval fires immediately), so it would not catch an exit check that misbehaves later. I couldn't see a record that the new tests were shown to fail with their target broken. docs/manual-testing.md wasn't touched; if it describes the daemon lifecycle it should be updated. Also worth saying in the PR text that serve-28 only runs on the GPU lanes (still pending at the time of review) and only asserts the fresh-config case; the watcher-enabled-starts-the-helper direction is covered only by the unit test.

The automations-10 and serve-28 scenarios and docs/testing.md entries are in place, and the leak scan is clean. The PR also currently conflicts with main and needs a rebase.

[juhovainio]

The daemon now exits on its next tick after the last watcher is disabled. If a watcher is re-enabled inside that window, background_helper_already_running still reads the old state as running and spawns nothing, then this branch exits, leaving a watcher enabled with no daemon. Is there a grace/re-check, or does enable guarantee ordering?

[volen-silo]

Fixed in abc0f3c. The daemon now decides to exit under the autostart lock: it re-reads the config after taking it, and only if still no watcher is enabled publishes "stopped" and clears its own spawn claim before releasing it. The autostart path no longer has a lock-free "already running" read, so an enable that lands in the window either makes the daemon stay or finds it stopped and starts a new one. enable already saves the config before ensuring the helper, so the "daemon started before the config was saved" case does not arise. The tick period and the two callbacks are injectable, so automation_loop_stays_when_a_watcher_is_enabled_inside_the_exit_window reproduces the window deterministically; the description lists which break turns each new test red.

[juhovainio]

automation_daemon_enabled() gates both the spawn and the exit. The tests set daemon_enabled = true as well as a watcher, so the predicate is not just "a watcher is enabled". Please confirm it matches the "no automation watcher is enabled" wording used in the README, doc comments and the exit log line.

[volen-silo]

Agreed, they had drifted. automation_daemon_enabled() is now "any watcher is enabled", with the stored daemon_enabled flag no longer consulted, so the spawn gate, the exit, the "background checks" display and the README, doc-comment and log wording all say the same thing. automations enable and disable still keep the flag in step, and a test covers a config with only the flag set.

[juhovainio]

The list here still describes GPU metrics every 60s and the webhook listener as always-on work, but the loop now exits within seconds when no watcher is enabled (the default). Worth qualifying those too, and confirming the dashboard does not rely on the daemon being up.

[volen-silo]

Updated: the rocm daemon help now says it runs only while a watcher is enabled, qualifies the server-recover and GPU-metrics bullets, says rocm dash does not need it, and no longer mentions a webhook listener (rocm daemon never starts one). On your second question, nothing relies on the daemon being up by default: rocm dash has its own embedded daemon and never reads the automation runtime state, and a server that exited is recorded by its own supervisor. The evidence is in the description.

[nowycondro]

Review — no blocking findings

Full review of the whole change.
Implements the daemon-orphan fix from ROCMAI-827 (the detached rocm daemon no longer outlives services stop when no automation watcher is enabled); the PR's own "Out of scope" section notes it does not touch the separate LemonadeServer.exe process named in the same ticket report (argued, independently of this review, to not be started by rocm-cli at all) nor add messaging for the still-persists-while-a-watcher-is-enabled case.

Blocking

None

Non-blocking

• docs/testing.md's new nextest filter is miscounted as "the first pair" — docs/testing.md:1510,1514
cargo nextest run -p rocm -E 'test(background_helper_)' is a substring filter, and apps/rocm/src/service_launch.rs contains four test functions matching background_helper_, not two: the two new ones (background_helper_not_spawned_when_no_automation_is_enabled, background_helper_spawned_when_an_automation_is_enabled) plus two pre-existing, unrelated ones (background_helper_already_running_true_for_live_pid, background_helper_already_running_false_for_dead_or_missing, testing double-spawn liveness detection). The following line calls this "the first pair," which is wrong for this command (the second command, filtering automation_loop_, does correctly match exactly two tests). Confidence 90 · mechanical · Fix: narrow the filter to the two new test names, or reword to drop the "pair" claim and note the other two matches are pre-existing.
• README.md line breaks the file's ~80-column hard-wrap convention — README.md:833
  The edited crash-restart sentence (...after a crash (with the \server-recover` automation enabled) inherits the daemon's environment...`) is 121 characters; every other line in the surrounding paragraph (and the file generally) is wrapped to 69-80 characters, including the PR's own newly-added paragraph a few lines below it (73-79 chars/line). The inserted parenthetical pushed this one sentence past the file's convention without being rewrapped. Confidence 95 · mechanical · Fix: rewrap the sentence to ~80 columns.

Decisions for the author

None

Positive signals

• spawn_background_helper's process-spawn code is extracted behind an injectable impl FnOnce(&AppPaths) -> Result<Result<u32>> closure (ensure_background_helper_running_with), letting the two new gating unit tests exercise the decision logic without ever launching a real detached process — a clean way to make a side-effecting autostart path unit-testable.
• Both new unit-test pairs (service_launch.rs's spawn-gate tests, service.rs's loop-exit tests) were traced and are not vacuous: each would fail if its corresponding gate/exit check were removed.

Deployment notes

• Behaviour change, called out in the PR itself: a rocm daemon process started by an older binary is not affected by this change and keeps running until stopped by hand; only daemons started after this ships pick up the new self-exit behavior.
• Running rocm daemon by hand with no automation watcher enabled now prints its banner and exits immediately instead of idling in the foreground.

What this covered

Every file in the diff, completely, plus the surrounding code needed to judge them: crates/rocm-core/src/config.rs (AutomationsConfig, automation_daemon_enabled, watcher_config_mut), crates/rocm-core/src/automation.rs, apps/rocmd/src/watchers.rs, all call sites of ensure_background_helper_running*, all readers of AutomationRuntimeState, and the full history of the two logic-bearing files back to the original introduction of the "always spawn" behavior (504 commits reachable, non-shallow). Three independent passes were run: (1) the core gating/loop logic in apps/rocm/src/service_launch.rs, apps/rocmd/src/service.rs, apps/rocm/src/automations.rs, apps/rocm/src/main.rs; (2) the e2e feature files, step definitions, and docs (tests/e2e-cucumber/**, docs/testing.md, README.md, plus a check of docs/manual-testing.md for now-false claims — none found); (3) version history and code-comment-vs-code consistency. Verified directly: no failing CI check for this commit (all required checks, including the required E2E tests job covering automations-10, report SUCCESS; the GPU-only lanes covering serve-28 were queued, consistent with the PR's own note); the two reported findings above (exact test-name grep, exact line-length measurement); the Duration::from_mins usage against the pinned rust-version = "1.96" toolchain (compiles fine, feature stabilized in 1.91).

Investigated and refuted during the passes, below the reporting threshold: a possible TOCTOU window between the config-enabled check and the autostart lock (no production writer of watcher-enabled state exists outside the automations enable handler, which re-invokes the autostart check itself right after saving); the split automations.daemon_enabled boolean being settable independently of any individual watcher's enabled flag (true in principle, but no code path does this outside tests); lack of a dedicated scenario for "serve with a watcher already enabled still starts/keeps the helper" (the shared gating function is unit-tested for that branch and used identically by both callers, so a regression there is unlikely to go undetected, but it is a real coverage gap worth the author's attention even though it didn't meet this review's confidence bar).

Did not run: the Rust test suite itself (no cargo/rustc toolchain available in the review sandbox) — all conclusions on logic correctness are from tracing the code, not execution. Not reconciled against any other review or discussion on this PR.

[volen-silo]

Pushed abc0f3c (squashed, rebased onto main: the rocmd refactor moved the daemon loop into apps/rocmd/src/daemon.rs, so the change is applied there). Points in the reviews:

• Scope: the description now says this PR covers the daemon only. LemonadeServer.exe is started by a separate Lemonade install (its own Startup shortcut), not by rocm-cli; the plain-language note about what remains and how to stop it is not done, so the ticket should stay open for that.
• Behaviour change: nothing relied on the always-running daemon (evidence in the description). The help text is updated.
• Race: closed under the autostart lock, with a deterministic test.
• Tests: the keeps-running test now covers several ticks and a later exit, and the description has a table of which break turns each new test red. docs/manual-testing.md has no daemon lifecycle text. The description says serve-28 runs only on the GPU lanes and covers only the no-watcher case, and that the watcher-enabled direction is covered by automations-11 and the unit tests.
• Non-blocking: the docs/testing.md filter now names the new tests, and the README sentence is rewrapped.
• Rebase: done; the scenario is now automations-11 because main took automations-10.

All points are addressed in abc0f3c and the updated description (scope narrowed to the daemon, ticket stays open for the rest); see the replies and the summary comment. A re-review is welcome.

[juhovainio]

No blocking correctness bugs found in the diff, but I'm requesting changes for a handful of things to settle before merge. The exit-race handling (re-reading the config under the autostart lock, publishing "stopped" before releasing it) looks sound.

• automation_daemon_enabled() no longer consults the stored daemon_enabled flag. Please confirm no other reader (status, chat/MCP automations tools, existing configs with only the flag set) depended on it, and say so in the PR body.
• FileLock::acquire blocks inside the async tick in rocmd.
• rocmd run / --help text: please confirm nothing still promises a persistent supervisor, since the daemon now exits whenever no watcher is enabled.
• AGENTS.md asks for docs/manual-testing.md to be updated alongside behavior changes; I didn't see it touched.
• Two unit tests rely on fixed sleeps; minor flake risk.
• With a watcher enabled the daemon outlives services stop, and rocm uninstall still doesn't say so. ROCMAI-827 asks to either stop helpers or say plainly what remains, so that half is still open (the PR body already says the ticket stays open).
• The PR is currently conflicting and needs a rebase (the serve-28 renumber clash predicted in the PR body).
• E2E tests (Strix Halo, Windows, current) failed at "Run E2E tests on Strix Halo Windows"; logs weren't available yet, so please check whether it's the new daemon scenario or a lane flake. The W7900, MI300X and Strix Halo Ubuntu lanes were still pending.

Inline comments below.

[juhovainio]

Dropping daemon_enabled from this predicate changes it for every caller. Please confirm no other reader (status, chat/MCP automations tools, existing configs where only the flag is set) depended on the flag alone, and note it in the PR body.

[volen-silo]

Checked every reader, and the paragraph is now in the PR description. The stored automations.daemon_enabled flag is written only by automations enable and disable in apps/rocm/src/automations.rs, so for any config the CLI produced it already agreed with the watchers. The predicate is read by the automations status text, rocmd status, the loop exit check and the autostart gate; the chat/MCP automations tools and the Codex bridge snapshot never consulted the flag, and rocm dash never references it. Only a hand-edited config with the flag set and every watcher off behaves differently: it used to spawn a no-op daemon and now starts nothing.

[juhovainio]

FileLock::acquire blocks inside an async tick; if the holder stalls, this blocks a runtime worker (and the webhook task). Consider spawn_blocking or a bounded wait.

[volen-silo]

Agreed. The daemon now takes the autostart lock with a non-blocking FileLock::try_acquire (new, in rocm-core) polled every 10 ms on the runtime. After a bounded 30 s it gives up, stays up and retries on the next tick, so a stalled holder cannot block a runtime worker or the webhook task, and the exit-race guarantee is unchanged. Two tests cover it: a task on the loop's single-threaded runtime keeps ticking while the loop waits on a held lock, and a stuck lock does not make the loop exit but a released one does.

[juhovainio]

This line is now overlong after the edit; please rewrap the paragraph.

[volen-silo]

Rewrapped.

[juhovainio]

Fixed 300 ms sleep to let the other thread reach the lock. Fine for the pass direction as documented, but polling until the thread is actually blocked would cut flake risk.

[volen-silo]

Replaced with an observed event: ensure_background_helper_running_with now polls the lock and takes an on_lock_busy callback, and the test waits until the enable call has actually queued behind the lock. If a caller skipped the lock the test sees the thread finish and fails fast instead of waiting out its deadline.

[juhovainio]

Same here: the fixed 500 ms sleep is a timing assumption; consider polling instead.

[volen-silo]

Same change here: the test waits until the loop reports (via a new on_lock_busy hook) that it found the lock held, instead of sleeping 500 ms.

[volen-silo]

Pushed 7d3a5b3 (squashed, rebased onto main). Points in the review:

• daemon_enabled no longer in the predicate: every reader checked, paragraph in the description; no other reader depended on the flag.
• Blocking lock in the async tick: non-blocking try_acquire polled with a bounded wait, with tests.
• Fixed sleeps: both replaced by waiting on an observed event.
• Help text: found and fixed leftover "persistent supervisor" wording (the rocmd run flag help, banner and status line) and a real regression in docs/automations.md: rocmd run --automations-enabled --local-webhook-port 18080 on a default config now exits within seconds, so the doc says to enable a watcher first.
• docs/manual-testing.md: new lifecycle section.
• The services stop / rocm uninstall message: done. rocm services stop (and the Ctrl-C stop in attached serve) now prints a note: line, and the uninstall plan warns, derived from the same predicate plus the helper's liveness, with tests on the message and the state. The LemonadeServer.exe half stays out, as it is not started by rocm-cli, so the ticket should stay open for it.
• README rewrapped.
• Rebase: done. The serve scenario is serve-33, the next free number on main; the vLLM refusal PR (ROCMAI-823: refuse vLLM serve and install up front on native Windows #837) also adds serve-33..serve-35, so whichever merges second renumbers.
• CI: the failing Strix Halo, Windows scenarios on the previous head were serve-01, serve-02, runtime-02 and runtime-09, none of them related (they also fail on main runs), and automations-11 and the serve scenario passed on that lane. The two Ubuntu lanes hit the 45-minute cap; the fix is in ci(e2e): move the AIM serve scenarios off the per-PR Strix Ubuntu lane #852.

All points are addressed in 7d3a5b3 and the updated description (the LemonadeServer.exe half of the ticket stays open, as the description says); see the replies and the summary comment. A re-review is welcome.

[nowycondro]

🤖 Automated review (Claude Code) · 7d3a5b3

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 — no blocking findings

Full review of the whole change.
Implements ROCMAI-827 for the rocm daemon helper only. The PR's own text deliberately and explicitly leaves LemonadeServer.exe (the ticket's other named process) out of scope, with a stated reason (not started by rocm-cli) and says the ticket should stay open for that remainder. Jira currently shows ROCMAI-827 as Done, which is in tension with the PR's own "ticket should stay open" statement — worth the author or a maintainer confirming the ticket isn't meant to close on this merge.

Blocking

None

Non-blocking

• A newly introduced idle-exit check can stall the daemon's own webhook-dispatch and shutdown handling for up to 30s under lock contention — apps/rocmd/src/daemon.rs:151 (the _ = ticker.tick() => arm), apps/rocmd/src/daemon.rs:179 (acquire_autostart_lock(paths, &hooks).await?), apps/rocmd/src/daemon.rs:266-285 (acquire_autostart_lock, bounded by AUTOSTART_LOCK_WAIT = Duration::from_secs(30) at line 19).
  tokio::select! only polls its other arms (webhook-event receive, shutdown signal) once the chosen arm's body runs to completion; the idle-exit path now awaits acquire_autostart_lock, which can poll for up to 30s before giving up, entirely inside the ticker arm. Confirmed via diff against origin/main that pre-PR this arm did only a quick config read/write with no comparable await, so this is new, not pre-existing. Concretely: if the autostart lock happens to be held by a concurrent serve/automations enable for the full 30s (the code's own comment calls this "stuck"), an already-received webhook event sits undispatched and a Ctrl-C/SIGTERM is not acted on for up to 30s. The in-code comment ("a stalled lock holder cannot block a runtime worker or the webhook task") is accurate for the separate webhook-listener task, but doesn't fully hold for this loop's own webhook-dispatch and shutdown arms, which do stall. Low real-world likelihood (the lock is normally held only for a fast config-read + maybe one process spawn), and the 30s bound keeps it from hanging forever, which is presumably why it wasn't caught by the existing tests (they use injected fast ticks and don't specifically assert responsiveness during a stuck-lock window).
  Confidence 85 · architectural · Fix: give the lock wait its own select! branch (e.g. spawn it and await a channel/oneshot) so the webhook-dispatch and shutdown arms stay pollable while it waits, or add a test that asserts shutdown/webhook responsiveness during a held lock.
• background_helper_remains_note's "a watcher is enabled" branch reports the helper as running without checking actual liveness, contradicting its own doc comment — apps/rocm/src/service_launch.rs:846-868.
  The doc comment says the note is "derived from the same two facts the helper itself acts on: whether a watcher is enabled … and whether it is running," but the enabled-non-empty branch returns "...keeps running while an automation is enabled..." unconditionally — only the no-watcher fallback branch calls background_helper_already_running. If the watcher is enabled but the daemon process has died (spawn failures from ensure_background_helper_running_with are only logged, not propagated, per that function's own doc comment; nothing auto-restarts a crashed detached helper), rocm services stop/rocm uninstall --dry-run still print "keeps running" and name the disable command, even though no such process exists right now. The existing test (stop_output_names_the_helper_that_outlives_it_only_when_it_does) only sets config state, never a runtime state, so it doesn't distinguish "watcher enabled + daemon alive" from "watcher enabled + daemon dead" — both produce the identical message today, which is arguably the intended policy-level wording, but it doesn't match the doc comment.
  Confidence 72 · mechanical · Fix: either have the enabled branch also check background_helper_already_running and vary the wording, or correct the doc comment so it no longer claims this branch is running-state-aware.

Decisions for the author

• File-lock polling (detect-and-exit) vs. a long-lived single supervisor — tradeoff. The chosen design (each tick re-reads config, decides exit under a file lock shared with the CLI-side autostart path) fits the existing detached-process architecture with a minimal diff and closes the race it sets out to close. A long-lived supervisor/OS-service model would remove the polling and the lock-contention window above entirely, at the cost of a much larger architectural change outside this PR's stated scope. Worth confirming the smaller fix is the deliberate choice rather than a deferred one.
• LemonadeServer.exe left out of scope — tradeoff, already argued in the PR text (not started by rocm-cli) and consistent with the ticket's "or say plainly what remains" acceptance criterion. Flagging only because Jira shows the ticket Done while the PR says it should stay open; confirm that's intentional bookkeeping and not an accidental early close.

Positive signals

• The race the PR sets out to close is solid: traced concretely that automations enable always saves config before calling the autostart gate, the gate's "already running" check is made only after taking the shared lock with no lock-free fast path, and the daemon re-reads config and publishes "stopped" before releasing that same lock — both orderings (enable wins the lock first vs. daemon wins it first) leave the system consistent, and this is exercised by background_helper_is_started_when_the_exiting_daemon_publishes_stopped plus the automation_loop_* tests in apps/rocmd/src/daemon.rs, none of which rely on fixed sleeps (they wait on injected hook callbacks instead).
• The automations-11 e2e scenario genuinely drives a live rocm daemon process and polls its real pid/AutomationRuntimeState/rocm daemon --status output rather than asserting a log line in isolation — traced the step definitions directly to confirm this.
• Every behavioral claim checked against the code held: the "5 second" tick interval, the exact helper: started background automation daemon and uninstall does not stop it string literals, and the docs/testing.md nextest filter commands all match the actual code (the prior round's filter-miscount and README line-wrap findings are both fixed in this version).

Deployment notes

• Behaviour change, stated in the PR and verified: a rocm daemon started by a pre-upgrade binary is unaffected and keeps running until stopped by hand; only daemons started by this binary pick up the new self-exit behavior.
• rocmd run --automations-enabled --local-webhook-port <port> on a default (no watcher enabled) config now exits within seconds instead of staying up — anything scripted around that invocation needs to enable a watcher first.

What this covered

Every file in the 18-file diff (6b175eee…7d3a5b3, +1119/-80) read completely, or — for the handful of very large files (apps/rocm/src/main.rs ~9.6k lines, apps/rocm/src/services_text.rs, crates/rocm-core/src/process.rs, crates/rocm-core/src/config.rs) — every touched region plus its direct callers/callees, logged per-file by each pass. Three passes ran: (1) core gating/concurrency (service_launch.rs, daemon.rs, process.rs, config.rs, automations.rs) and (2) CLI wiring/messaging (main.rs, cli.rs, services_text.rs, uninstall_plan.rs) each ran as an independent fresh-context worker; (3) docs/e2e coverage (README, docs/*.md, the two .feature files and their step definitions) could not be spawned as an independent worker — three attempts all hit the platform's 20-concurrent-subagent cap, saturated by unrelated sessions, not by any failure of the pass itself — so the driver ran that pass directly instead; it is disclosed here as not independent. The driver additionally ran impact-analysis sweeps (grep for stale "persistent daemon" claims across docs/skills — none found; completeness check of every render_service_action_result call site), a partial history pass (git log -S on daemon_enabled to rule out a legacy-upgrade hazard; diffed daemon.rs against origin/main to confirm the select!-arm finding is newly introduced, not pre-existing), and the design-question pass.

CI read live for the exact reviewed commit: all 25 check runs green (build-and-test, clippy, coverage floors, Windows build+test, Sphinx docs, lychee markdown links, commit-signatures, E2E tests + consolidated report); no GPU-hardware-lane check (Strix Halo/W7900/MI300X) appears in that set, consistent with the PR's own note that the GPU-gated serve-33 scenario runs outside it — its actual pass/fail status could not be independently confirmed here. No Rust toolchain was available in this sandbox; no test was executed directly, so conclusions on test/logic correctness are from tracing the code plus the green CI, not from a run.

Did not run: "prior changes" pass (review comments on earlier PRs to these files) — no practical way to retrieve historical review threads for these specific files within this review's scope.

Unavoidable exposure: this review's mandatory claim-check (pull_request_read method=get/get_reviews) necessarily returned the PR's own description and the full bodies of the prior human reviews on this PR before any code-level finding was reached. Nothing from that text was used to derive a finding — both findings above are not mentioned in the PR description or in any prior review on this PR. The description and ticket (ROCMAI-827, retrieved directly by key) were read for the intent line above only after findings were otherwise settled from the code. Not reconciled against the discussion on this PR.

---

🤖 Generated with Claude Code

[juhovainio]

Requesting changes. I found nothing that is broken in CI (all checks are green), but a few things should be settled before merge. Also note GitHub currently reports this branch as having merge conflicts, so it needs a rebase.

Ticket scope. ROCMAI-827 names both rocm daemon and LemonadeServer.exe as left running after services stop. This PR only deals with rocm daemon; nothing stops or mentions the Lemonade server process in the stop or uninstall output. Either cover it (stop it, or say plainly that it remains and how to stop it), or say in the PR body that it is split out.

Behavior changes to confirm and call out in the PR body

• automation_daemon_enabled() no longer reads automations.daemon_enabled. A saved config with daemon_enabled=true and no watcher enabled used to mean a persistent daemon; now it exits on its first tick. A foreground rocm daemon / rocmd run used for debugging exits immediately unless a watcher is enabled.
• serve no longer starts the daemon by default. The old comment in run_attached_service said the daemon reconciles a self-exited server's record. Please state which of those duties only applied under a watcher; anything that did not is now silently lost.
• Only run_approved_service_action and the attached Ctrl-C path print the helper note. Please confirm no other stop surface should carry it.

Messages vs. state. The "keeps running while an automation is enabled" note is printed whenever a watcher is enabled, without checking the helper is actually running, and the uninstall idle-case text contradicts itself (see inline). Per the repo's rules, derive the text from the same state the behavior depends on.

Tests

• serve-33's state check can pass vacuously before a fresh daemon publishes state (inline).
• automations-11 spawns a real detached daemon; please confirm World teardown kills it if an earlier step fails before the watcher is disabled.
• Please say that the new tests were checked by breaking their target (e.g. restore the lock-free fast path, or replace the try_acquire loop with a blocking acquire) and went red.

Minor: the lock wait in ensure_background_helper_running_with has no deadline, unlike the daemon side (30 s). It matches the old blocking acquire, so not a regression.

The structure is good: one predicate shared by spawn, exit and display, exit decided under the autostart lock, test seams that avoid sleeps, and docs/--help updated consistently.

[juhovainio]

The note says the helper "keeps running" whenever a watcher is enabled, without checking that it is running. Derive the wording from background_helper_already_running as well, or soften it, so the message cannot disagree with the state.

[volen-silo]

Fixed in 14c90e1: the note is built from both facts (watcher enabled, helper running). Enabled and running says it keeps running and names the disable command; enabled but not running says so and that the next rocm serve or rocm automations enable starts it; running with nothing enabled says it exits on its own; neither prints nothing. One test pins the exact text for all four combinations, with the running fact planted as a state file, and another checks the services stop output.

[juhovainio]

This config load now runs after run_attached_service has already spawned the child, and propagates with ?, whereas background_helper_remains_note uses unwrap_or_default. An unreadable config would fail serve after launch; treat it as "no watcher" here or load it earlier.

[volen-silo]

Agreed, fixed: an unreadable config now counts as "no watcher" in ensure_background_helper_running_with, so serve cannot fail after it has launched the child. background_helper_autostart_treats_an_unreadable_config_as_no_watcher covers it, and the test goes red when the load propagates with ? again.

[juhovainio]

For the idle case (no watcher enabled, helper about to exit) the note already says it exits on its own, so the "; uninstall does not stop it" suffix contradicts it. Only append the suffix for the enabled case.

[volen-silo]

Right, it contradicted the idle text. "; uninstall does not stop it" is now appended only when a watcher is enabled and the helper is running; an idle helper gets only the "exits on its own" text and a helper that is not running gets no uninstall warning. A test asserts the exact strings for each case.

[juhovainio]

A freshly spawned daemon may not have published its runtime state yet, so this check can pass vacuously. Also assert that the autostart claim file (automation_autostart_claim_path) does not exist, since the spawn writes it synchronously.

[volen-silo]

Done: the step also asserts the autostart claim file does not exist (the spawn writes it synchronously, and a unit test shows it exists after a spawn). This scenario only runs on the GPU lanes, so I did not run the step red locally; the unit tests cover the claim file.

[volen-silo]

Pushed 14c90e1 (squashed, rebased onto main; the serve scenario is now serve-36, after the vLLM refusal scenarios serve-33..serve-35). On the review:

• Ticket scope: unchanged. LemonadeServer.exe is started by a separate Lemonade install, not by rocm-cli; the description says so and that the ticket stays open for it. The plain statement of what remains is now done for the daemon.
• Behaviour changes: the three confirmations you asked for are in the description (the daemon_enabled flag and a foreground daemon exiting; what the always-on daemon did and did not do, with the supervisor and read-path evidence and the stale run_attached_service comment replaced; and every stop surface, which carry the note and which do not).
• Messages vs state: the note is now derived from both whether a watcher is enabled and whether the helper is running, for services stop and the uninstall plan, with the suffix only where it is true.
• Tests: the scenario step is tightened (claim file), and automations-11 had no teardown for a helper left behind by a failing step: added, and verified by making the scenario fail while the helper was alive (with the teardown disabled the daemon leaked).
• Second review: the lock wait is now its own select! arm so the webhook and shutdown arms stay live (a test shuts the loop down with the lock held), and the "keeps running" wording now follows the helper state. On the design question, the file-lock approach is the deliberate smaller fix; a long-lived supervisor would be a separate change.

I did not look into the Jira status of ROCMAI-827 from this PR; it should stay open for the Lemonade half.

All points are addressed in 14c90e1 and the updated description (the Lemonade half of the ticket stays open, as stated); see the replies and the summary comment. A re-review is welcome.

[juhovainio]

Looks good; no blocking findings. Exit is decided under the autostart lock with a config re-read, the note is derived from the same state that drives behavior, and docs are in sync.

Non-blocking:

• Claim format <pid> <ms> is parsed in rocmd and written in rocm with no shared helper, and the test hand-writes it.
• An unreadable config is reported as "no automation is enabled" in the stop/uninstall note.
• New error paths in the daemon exit arm leave running=true published.
• CLI-side autostart lock loop has no deadline.
• The services stop note and uninstall warning only have unit tests; the uninstall warning looks coverable by a mock-lane scenario.

Also: ROCMAI-827 is already marked Done, but the PR says the LemonadeServer.exe half stays open, so that half has no open tracking. serve-36 (GPU lane) was not seen running in CI.

[juhovainio]

Claim format "<pid> <ms>" is re-parsed here separately from the writer in apps/rocm/src/service_launch.rs; a format change would make this release a silent no-op. Consider one shared helper/constant (e.g. in rocm-core) and have the test write the claim through the real writer.

[juhovainio]

The test hand-writes the claim instead of using the producer, so nothing ties the writer to this consumer.

[juhovainio]

attempt? (and the config load just below) returns with running = true still published; the new stop note and uninstall warning read that flag. Consider recording stopped on this error path.

[juhovainio]

unwrap_or_default() makes an unreadable config read as "no automation is enabled", and the printed note then asserts that as fact. Consider omitting or qualifying the note on a load error.

[juhovainio]

This CLI-side lock loop has no deadline, unlike the daemon side (30 s); a stuck lock holder would hang serve / automations enable. Intended, or worth a bound?

[nowycondro]

🤖 Automated review (Claude Code) · 14c90e1

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 — no blocking findings

Full review of the whole change.
Implements the rocm daemon (background automation helper) half of ROCMAI-827: the helper now starts only while an automation watcher is enabled and self-exits once none is, closing the "detached daemon outlives rocm services stop" orphan. The PR's own text explicitly leaves LemonadeServer.exe (the ticket's other named process) out of scope, with a stated reason (not started by rocm-cli), and says the ticket should stay open for that remainder. Jira shows ROCMAI-827 as Done (retrieved directly by key), which is in tension with the PR's own "should stay open" statement — worth a maintainer confirming that bookkeeping is intentional rather than an early close.

Blocking

None

Non-blocking

• docs/testing.md:1586 wraps well past the surrounding paragraph's own convention — docs/testing.md:1586
  The new paragraph (lines 1571–1590) wraps prose at roughly 65–81 characters per line, and line 1586 ("bad config from failing a serve after its launch. The user-visible side is automations-11 in") runs to 96 visible characters while every neighboring line in the same paragraph stays ≤81. Confidence 80 · mechanical · Fix: rewrap the line to match the paragraph's own width.
• Stray double blank line at a scenario boundary — tests/e2e-cucumber/features/model_serving.feature:564-565
  Two consecutive blank lines separate serve-35 from the new serve-36 comment block; every other scenario boundary in the file uses exactly one. Harmless to Gherkin parsing, inconsistent with the file's own formatting convention. Confidence 85 · mechanical · Fix: drop the extra blank line.

Decisions for the author

• LemonadeServer.exe left out of scope — tradeoff, already argued in the PR text (not started by rocm-cli, lives in its own Startup-folder shortcut) and consistent with the ticket's "or say plainly what remains" acceptance criterion. Flagging only because Jira shows ROCMAI-827 Done while the PR says the ticket should stay open for that remainder — confirm that's intentional bookkeeping, not an accidental early close.

Positive signals

• The exit/autostart race the PR sets out to close is solid: traced that the daemon's self-exit and the CLI's autostart "already running" check share one lock (automation_autostart_lock_path), the daemon re-reads config and publishes running=false (and clears its own spawn claim) strictly before releasing that lock, and the CLI side has no lock-free fast path — both orderings (an enable winning the lock first vs. the exiting daemon winning it first) leave the system consistent, each exercised by a dedicated test (automation_loop_stays_when_a_watcher_is_enabled_inside_the_exit_window, background_helper_is_started_when_the_exiting_daemon_publishes_stopped).
• The lock wait is modeled as its own tokio::select! arm (a pinned, boxed future) rather than an inline .await inside the ticker arm, so a stuck lock holder cannot delay the loop's own webhook-dispatch or shutdown handling — directly exercised by automation_loop_shuts_down_while_waiting_for_a_held_autostart_lock and automation_loop_gives_up_a_stuck_lock_wait_and_exits_once_it_is_free, both traced to fail under the natural "inline await" or "blocking acquire" mistakes.
• background_helper_already_running's liveness check (state.running && process_is_running(pid)) is what every behavior-affecting caller (autostart gating, the services stop/uninstall note) consults, not the raw persisted flag; the note-generation tests (helper_note_follows_watcher_enabled_and_helper_running) exercise all four combinations of watcher-enabled × actually-running by writing a real runtime-state file, not just config.
• New tests are deliberately non-vacuous: for every new gate/exit check, a corresponding test was traced against the reverted or mutated branch and confirmed to go red (spawn-gate removal, lock-acquire removal, claim-release removal, exit-condition-always-true, lock-free fast path restored).
• Docs (README.md, docs/architecture.md, docs/automations.md, docs/manual-testing.md, rocm daemon --help/rocmd run --help/status text) were updated in the same change with no stale "persistent supervisor" claims left in the files checked.

Deployment notes

• Behaviour change, stated in the PR and confirmed in the code: a rocm daemon process started by a pre-upgrade binary is unaffected and keeps running until stopped by hand; only daemons started by this binary pick up the new self-exit behavior.
• rocmd run --automations-enabled --local-webhook-port <port> on a default (no watcher enabled) config now exits within seconds instead of staying up; anything scripted around that invocation needs to enable a watcher first.

What this covered

All 19 changed files read completely at the PR head (d7faa84e…14c90e1f, fetched read-only via refs/remotes/pr/838, never checked out), plus surrounding/calling code. Three independent blind fan-out workers covered: (1) the core gating/concurrency logic (service_launch.rs, daemon.rs, process.rs, config.rs, automations.rs) — no findings ≥70; (2) CLI wiring/messaging (main.rs, services_text.rs, uninstall_plan.rs, cli.rs) — no findings ≥70; (3) docs and e2e coverage (README, docs/*.md, both .feature files and their step definitions) — the two non-blocking findings above. None of the three workers were shown the PR description, ticket, or any review discussion. I separately re-read daemon.rs and the automation_daemon_enabled()/background_helper_already_running predicate chain directly, cross-checked every claimed non-diff reader (chat.rs, mcp.rs, host_snapshot.rs, codex_bridge.rs) for the flag/predicate, and ran a git log history pass on service_launch.rs/daemon.rs (confirms this module has a recurring history of race/recovery-focused review-fix commits, consistent with the scrutiny this PR's concurrency design received).

CI verified live for this exact commit: all 25 required/listed checks (build-and-test, clippy, Windows build+test, coverage floors, E2E tests + consolidated report, commit-signatures, CodeQL, docs build, etc.) report SUCCESS; Skill checks (skillscope) is SKIPPED, which is advisory-only per this repo's own AGENTS.md and not yet a required context. No GPU-hardware lane (covering serve-36) appears in that set, consistent with the PR's own note that it runs only on gated lanes.

Unavoidable exposure, disclosed per this review's own protocol: this review's mandatory pre-work claim-check (pull_request_read get/get_reviews, required before any review effort to detect a duplicate at the live head commit) necessarily returned the PR description and the full bodies of every prior human and automated review on this PR before any code was read. To keep findings blind, every item above was either produced by a fan-out worker that never saw that text, or re-derived and verified independently against the code before being included. Several topics visible only in that already-exposed text (a prior-round finding about a select! arm stalling shutdown/webhook handling, a note-branch not checking real liveness, an unbounded CLI-side lock-wait, and a daemon error path that could leave a stale running=true record) were investigated directly against the current code: the first two are now fixed in this commit (confirmed structurally — the lock wait is its own select arm, and the note-generation now threads a real liveness-checked running bool through every branch) and are credited above as positive signals rather than restated as open findings; the latter two I could not cleanly attribute to blind derivation on my part (I had already seen them named in the exposed review text before examining that code), so rather than claim them as independent findings I am omitting them — a maintainer weighing them should rely on the discussion already on this PR, not on this review. The ticket (ROCMAI-827) was retrieved directly by key via the tracker tool for the intent line only, after findings were otherwise settled from the code. Not reconciled against the discussion on this PR.

---

🤖 Generated with Claude Code

[volen-silo]

Pushed 547677b: rebased onto main again (the GPU-lane serve scenario is now serve-40, after main's serve-36..serve-39). No other change; the full gate is rerun on the rebased tree and the automations scenarios, including automations-11, pass.

[volen-silo]

Pushed 1eb574a: ported onto main after the services command cluster moved out of main.rs into services_cmd.rs (the note: line is wired in run_approved_service_action there; the Ctrl-C attached-serve path, services_text.rs, uninstall_plan.rs and the rocmd changes were unaffected). No scenario renumbering was needed (serve-40 is still the next free number). The full gate is rerun on the rebased tree and the automations scenarios, including automations-11, pass.