ROCMAI-83: extract service.rs and watchers.rs from apps/rocmd/src/lib.rs

[jussielo-amd]

Summary

Seventh and eighth (final) PRs of Phase 5 (ROCMAI-83, part of the modularization epic ROCMAI-27), consolidated into one PR. Pulls service.rs and watchers.rs out of apps/rocmd/src/lib.rs. Originally opened separately as #487/#489; consolidated here to cut down the PR count for this phase. The two are coupled — watchers.rs's server-recover recovery logic calls directly into service.rs's managed-service lifecycle.

service.rs — the managed-service PID lifecycle (stop/terminate/descendant-pid discovery), run_daemon's foreground loop, supervise_service's spawn-and-recover path, and serve-log startup-phase polling. Non-contiguous in the source (the startup-phase/shutdown-signal helpers sit far from the rest of the cluster, separated by the entire test block) — both halves moved.

watchers.rs — event collection/dispatch for all built-in watchers (TheRock update, GPU metrics/thermal-pressure, cache-warm, driver-upgrade, server-recover), managed-service recovery classification, and automation-proposal queuing. Also non-contiguous: queue_proposal/queue_proposal_with_arguments/proposal_tool_for_action/proposal_arguments_for_action sit between persistence.rs-territory code, and detached_rocmd_command sits at the tail of the file before the test module. Watcher-only consts (SERVER_RECOVER_BACKOFF_MS, SERVER_TRANSIENT_STALE_MS, ENDPOINT_HEALTH_TIMEOUT, THEROCK_UPDATE_INTERVAL_MS, GPU_METRICS_INTERVAL_MS, GPU thermal/VRAM pressure thresholds) moved with it.

• No behavior change. Reaches back into still-crate-root items that are actually in common.rs/persistence.rs (already merged/landed via ROCMAI-83: extract persistence.rs from apps/rocmd/src/lib.rs #477 and ROCMAI-83: extract common.rs, webhook.rs, and cli.rs from apps/rocmd/src/lib.rs #479) via the appropriate qualified paths.
• 47 tests that exercise these modules' own logic (13 service + 34 watchers: the supervise key-guard regression tests, run_daemon's automation-loop gate, descendant-pid discovery, startup-phase log parsing, event-collector/gpu/cache-warm/driver-upgrade/server-recover dispatch, therock-update handling, watcher-policy mode mapping, recovery-reason classification) moved into each module's own #[cfg(test)] mod in this same PR.
• docs/architecture.md updated in this PR — this completes Phase 5: lib.rs is now top-level glue (module declarations and the two externally-consumed entry points re-exported from cli.rs).

Targets main directly (not stacked on a sibling PR branch) so this PR can't be silently auto-closed if an intermediate branch gets deleted. Depends on #479 and #483 merging first — this code calls into sandbox::/mcp:: paths that only exist once #483 lands, so CI will be red until then; that's expected, not a regression. Merge order: #479 → #483 → this PR.

Supersedes #489, which is closed in favor of this PR.

Test plan

• cargo build (rocmd, and rocm+rocmd together to confirm the external API surface is untouched)
• cargo clippy --workspace --all-targets -- -D warnings
• cargo test -p rocmd — 130 tests, same count as before this change (129 passed + 1 pre-existing ignored)
• cargo xtask manifest --check
• cargo fmt / prek hooks clean
• tests/e2e-cucumber (CI) — expected red until ROCMAI-83: extract common.rs, webhook.rs, and cli.rs from apps/rocmd/src/lib.rs #479 and ROCMAI-83: common.rs follow-up, webhook.rs, cli.rs and sandbox.rs extraction from apps/rocmd/src/lib.rs #483 merge

[r0x0r]

Review (draft): extraction is clean; held by the phase-wide merge problem

The code motion verified as faithful, including the endpoint-key guard that the regression tests in this PR exist to protect. The reasons to hold are not about this diff in isolation — they are about how the eight PRs in this phase combine.

service.rs has 13 crate:: references that break when siblings merge

This module reaches back into the crate root for items that other open PRs move elsewhere:

crate:: reference	relocated by
record_event, load_managed_services	#477 → persistence.rs
optional_arg, parse_gpu_indices_arg, apply_endpoint_key_env, engine_healthcheck_ready, ensure_public_service_has_endpoint_key	#479 → common.rs
start_local_webhook_source, receive_local_webhook_event	#480 → webhook.rs
evaluate_watchers, evaluate_watchers_for_events, reconcile_watcher_snapshots, load_service_record	#489 → watchers.rs

Each becomes a dangling path once the owning PR lands. The part that makes this worth blocking on rather than noting: these references live in service.rs, a file none of those PRs touches. There is no overlapping hunk, so git merges them cleanly and the breakage shows up as a compile error on main with no conflict to warn anyone.

I checked this across all eight PRs rather than just this one. 52 such references break, spread over six of them — #489 watchers.rs (18), this PR (13), #481 cli.rs (11), #484 mcp.rs (5), #480 webhook.rs (1), #479 common.rs (1). The payload_string case between #480 and #489 is the cleanest illustration and I've written it up there.

The crate::-reach-back approach is fine for one PR against main; it only fails because eight are in flight against the same base. Two ways out:

1. Stack them — each branches off its predecessor, so every crate:: path is correct by construction. AGENTS.md §11 already describes this ("keep stacked PRs in draft until dependencies merge upstream, then rebase and move out of draft").
2. Keep them independent, rebase serially — after each lands, rebase the next and repoint its paths before merging.

The architecture-doc hunk

The doc text lists seven modules as extracted; this branch has lib.rs, main.rs, service.rs.

Worth being precise, because it would be reasonable to assume this is inherited drift rather than new: it isn't. main's current text is `lib.rs` is **not yet modularized** — see EAI-7768 (docs/architecture.md:34). Every one of the eight PRs rewrites that same single line with its own cumulative list, so each is independently introducing the inaccuracy, and the result is only true at merge order 477 → 479 → 480 → 481 → 483 → 484 → 487 → 489. Scoping each hunk to its own module makes it correct at any order, and turns eight whole-line rewrites into eight trivial appends.

Verified clean

• The endpoint-key guard survived intact. ensure_public_service_has_endpoint_key is still called twice in supervise_service (service.rs:574, :619), in the same order, with the same arguments and the same previously_required registry-propagation path via load_managed_services. This is the one I most wanted to be sure of: the PR moves the key-guard regression tests in the same commit as the code they guard, so a mistake in the guard could have travelled with its own tests and still gone green.
• PID lifecycle unchanged — terminate_process sends -TERM, force_terminate_remaining_processes sends -KILL after thread::sleep(Duration::from_millis(750)), and descendant-PID discovery is identical. Signal choice, ordering and escalation delay all preserved; nothing here can orphan or over-kill a process differently than before.
• run_daemon's loop unchanged — all three tokio::select! arms (ticker, webhook event, shutdown) identical, MissedTickBehavior::Delay preserved.
• Startup-phase log polling unchanged — offset arithmetic *pos += read as u64 and the rotation reset *pos = 0 when len < *pos both preserved.
• GPU-required behavior intact (AGENTS.md §6) — device_policy still threaded through to engine_serve_http_args; no CPU fallback introduced.
• The non-contiguous move is correct — both halves landed despite being separated by the entire test block, with nothing swept in from between them.
• Call sites — stop_managed_service, run_daemon, supervise_service and print_status all repointed to service:: (lib.rs:281, :294, :309, :615, :2270); no definition left behind in lib.rs.
• Imports — HashSet and tokio::time::{self, MissedTickBehavior} correctly moved; BufRead correctly kept in lib.rs, where stdin.lock().read_line() at line 1556 still needs it, and the #[cfg(test)] use tokio::time; is correctly narrowed to the one test that uses time::timeout. Nothing fails under -D warnings.
• Tests — 130 attributes conserved (117 + 13), matching the claimed 13 exactly.
• No dead code, stubs, todo! or unimplemented!; every let _ = suppression matches its original.
• No module-name collision — no sibling PR creates a service.rs or defines run_daemon/supervise_service/stop_managed_service/print_status.
• Blast radius — nothing outside apps/rocmd/src/ references any moved symbol.

---

🤖 by agent-hub on AMD AgentHub

[jussielo-amd]

Thanks for the thorough review. Both blocking points are resolved by stacking, per AGENTS.md §11:

• The 13 crate:: references: rebased this branch onto rocmai-83-mcp (base now set to rocmai-83-mcp, PR back to draft). All references owned by already-landed siblings (persistence.rs, common.rs, webhook.rs) are now repointed to those modules (persistence::load_managed_services/record_event, common::parse_gpu_indices_arg/optional_arg/apply_endpoint_key_env/engine_healthcheck_ready/ensure_public_service_has_endpoint_key, webhook::start_local_webhook_source/receive_local_webhook_event). The remaining crate:: reaches (evaluate_watchers, evaluate_watchers_for_events, reconcile_watcher_snapshots, WATCHER_TICK_INTERVAL, load_service_record) are correctly left as-is — they're still owned by lib.rs until #489/watchers.rs lands, so the path is valid today; that PR will repoint them when it extracts.
• architecture.md: scoped to an incremental append onto the predecessor's cumulative list (adds just service.rs's entry, narrows "still pending" to watchers.rs) rather than a whole-line rewrite, so it's correct regardless of final merge timing.

While rebasing I also found and fixed the other direction of the same hazard: cli.rs, mcp.rs, and sandbox.rs (landed by predecessor PRs #481/#484/#483) had their own stale bare crate:: calls into stop_managed_service/run_daemon/supervise_service/print_status — those now point at service::. And I deduped service.rs's test-only temp_app_paths/unique_test_root helpers in favor of the shared crate::test_support module (same duplicate-ownership pattern as update_check_message/CommandCapture in the earlier PRs of this stack).

All 130 test attributes (117 + 13) still present and green, cargo clippy --workspace --all-targets -- -D warnings clean, cargo fmt --check clean.

[jussielo-amd]

Scope note: this PR now covers service.rs + watchers.rs (consolidated from #489, which is closed), and now targets main directly instead of rocmai-83-mcp. Diff and description updated accordingly. Depends on #479 and #483 merging first — CI will be red until then.

[r0x0r]

Follow-up review (draft): the code motion holds, but the pushed head is not what the description says

Since my review at fbb96b9, the branch was stacked and rebased (f9bd7cf), #489 was closed, and the description and a comment now say this PR is service.rs + watchers.rs against main. The branch on GitHub does not match that. I'm commenting only, since it is a draft with a conflict.

Status of my earlier findings

• (b) crate:: breakage across sibling PRs: resolved in mechanism, unverifiable in practice. The serial order #479 -> #483 -> this PR turns a silent break into an explicit dependency. I overlaid service.rs and watchers.rs (from #489's 1c066ea) on main and on #483's head and resolved every crate:: path. On #483's head there are zero unresolved paths, so the order works. On main there are 14. See Blocking 2 for why #483 is not yet a sound base.
• (c) architecture.md: not resolved. The hunk is still a whole-line cumulative rewrite. Head's text still says "Still pending: the watcher cluster", which contradicts "completes Phase 5". It also drops the "earns a place in common.rs" admission-rule sentence that #479 put on main. Resolve the conflict by keeping that sentence.

Blocking before approval

1. Head f9bd7cf has no watchers.rs and is still the old stack. Against main, the diff is 8 files, +9325/-9161. It carries the common/webhook/cli/sandbox/mcp commits on the 57b78c6 base, and service.rs is only the last of those. watchers.rs exists only on closed #489's head. git merge-tree shows add/add on common.rs and conflicts in lib.rs and architecture.md. The commit message is also stale ("Stacked on rocmai-83-mcp"). The "47 tests / 13 + 34" and "scope note" comments describe an unpushed state. Please push the rebased branch.
2. The dependency claims do not match main or #483.
   • #479 landed common.rs only. webhook.rs and cli.rs are not on main.
   • #479's review fixup moved parse_gpu_indices_arg, optional_arg and engine_healthcheck_ready back into lib.rs. So crate::common::{parse_gpu_indices_arg, optional_arg, engine_healthcheck_ready} does not resolve on main.
   • #483's head (36513c1) re-adds those helpers, plus print_bridge_snapshot, to common.rs. That quietly reverses the landed fixup and the rule in docs/architecture.md. By that rule parse_gpu_indices_arg and engine_healthcheck_ready have exactly one caller (service.rs:525, :839) and belong in service.rs. optional_arg is shared by service.rs and watchers.rs, so common.rs is right for it.
   • The body says the code needs mcp:: paths from #483, but neither module references mcp::, and #483's head has no mcp.rs (its body says sandbox + mcp). The paths actually needed are webhook::, cli:: and sandbox::.
   • #483's inline MCP code still calls bare stop_managed_service (lib.rs:775). This PR has to repoint it once rebased.
     Please make the descriptions and branches agree, and decide where those three helpers live.

Non-blocking

• lib.rs keeps WATCHER_TICK_INTERVAL (used only by run_daemon) and ARTIFACT_PREFETCH_TIMEOUT (sandbox). It also keeps one sandbox-subject test, sandbox_tool_stop_server_updates_manifest_and_skips_current_pid. The crate-level #![allow(clippy::items_after_test_module)] is now vacuous. Moving these next to their users would make "lib.rs is glue" literal.
• The "130 tests" claim is stale. main now has 131 because of install_sdk_rejects_system_prefix_reached_by_escaping_home. It will conflict in lib.rs on rebase and likely belongs in mcp.rs.
• Only a docs-build status is reported on the head. cargo xtask coverage (AGENTS.md coverage floors, rocmd 75.36) is not in the test plan. Motion should be neutral, but run it.

Verified clean (service.rs from f9bd7cf, watchers.rs from #489's 1c066ea; no cargo here, so nothing was built or run)

• A token-level diff of the removed lib.rs code against service.rs and watchers.rs shows only module-path prefixes, pub(crate) and rustfmt reflow. That covers the endpoint-key guard, the TERM/KILL 750ms escalation, the run_daemon select! arms, log offset arithmetic, watcher dispatch, recovery classification, proposal queuing and detached_rocmd_command (both cfg variants). Every watcher threshold constant is unchanged and moved with it.
• No function was lost from lib.rs. pub(crate) coverage and path resolution check out in both heads.
• Tests: the 130 test names are identical to pre-stack (13 service + 34 watchers). There is still 1 #[ignore], and no todo!/stubs.
• Nothing outside apps/rocmd/src and docs/ is touched.

---

🤖 by agent-hub on AMD AgentHub