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

[jussielo-amd]

Summary

Seventh PR of Phase 5 (ROCMAI-83, part of the modularization epic ROCMAI-27). Pulls 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 into their own service.rs module.

• 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 in this PR.
• No behavior change. Reaches back into still-crate-root items (load_managed_services, record_event, start_local_webhook_source, receive_local_webhook_event, evaluate_watchers/evaluate_watchers_for_events/reconcile_watcher_snapshots, WATCHER_TICK_INTERVAL, parse_gpu_indices_arg/optional_arg/ensure_public_service_has_endpoint_key/apply_endpoint_key_env/engine_healthcheck_ready, load_service_record) via crate::, since the modules that will eventually own those (persistence.rs/webhook.rs/watchers.rs/common.rs) are separate, independent PRs not present on this branch (see ROCMAI-83: extract persistence.rs from apps/rocmd/src/lib.rs #477, ROCMAI-83: extract common.rs from apps/rocmd/src/lib.rs #479, ROCMAI-83: extract webhook.rs from apps/rocmd/src/lib.rs #480, ROCMAI-83: extract cli.rs from apps/rocmd/src/lib.rs #481, ROCMAI-83: extract sandbox.rs from apps/rocmd/src/lib.rs #483, ROCMAI-83: extract mcp.rs from apps/rocmd/src/lib.rs #484).
• 13 tests that exercise this module's own logic (the supervise key-guard regression tests, run_daemon's automation-loop gate, stop_managed_service, descendant-pid discovery, startup-phase log parsing, engine_serve_http_args) moved into service.rs's own #[cfg(test)] mod in this same PR. recovery_supervise_args and sandbox/mcp-domain tests that construct ManagedServiceRecord or call stop_managed_service merely as setup stayed in lib.rs for watchers.rs/other PRs.
• docs/architecture.md updated in this PR.

Independent of #477, #479, #480, #481, #483, #484 — branched separately from main, not stacked, per the one-off PR approach for this phase.

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)

[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.