[mikeroySoft]

Summary

A managed-service record stays "live" after its process is gone if the kernel has given the recorded PID to an unrelated process. This happens routinely after a reboot, because PIDs restart in the same low range. rocm serve then reuses the stale record and exits 0, so a systemd unit using Restart=on-failure never recovers.

 refresh_managed_service_runtime_liveness / settle_pending_stop_key_cleanup
   recorded_service_pids(record)
-    -> [engine_pid, supervisor_pid]                      # bare u32
-  any(process_is_running(pid))
+    -> [ProcessIdentity(pid, own start_ticks)]           # deduped, known ticks win
+  any(identity_state(id) in {Matches, Indeterminate})    # Recycled/Gone = dead

• The stop path (terminate_recorded_service_pids) already pairs each PID with its own start-ticks through rocm_core::identity_state. This applies the same check to liveness.
• Indeterminate (start-ticks unreadable) still counts as alive, which keeps the existing conservative behaviour.
• Legacy records with no start-ticks fall back to plain PID liveness, so nothing changes for them.
• managed_service_record_is_in_use (prune guard) keeps bare PID liveness on purpose. It only blocks a prune, and over-reporting "in use" there is harmless.

Evidence

Observed on a host after a reboot. The record had status: starting, supervisor_pid: 1634, supervisor_start_ticks: 16200. In the new boot, PID 1634 was a wireplumber thread with start-ticks 79452:

$ rocm serve ornith --foreground --port 11435 --gpu 0      # systemd ExecStart
model already being served
  service_id: lemonade-ornith-ai-ornith-1-5-35b-1792352102829
  status: starting
# exit 0; nothing listening on :11435; no lemonade/llama process

To reproduce outside the host, I put a starting record in an isolated ROCM_CLI_DATA_DIR and pointed its PIDs at a live unrelated process with the wrong start-ticks:

• Before: rocm services list → Status: 1 starting
  After: rocm services list → Status: 1 not running

New Linux-gated unit tests in apps/rocm/src/main.rs:

test	without fix	with fix
liveness_refresh_demotes_record_whose_pid_was_recycled	FAIL	pass
liveness_refresh_demotes_recycled_supervisor_when_engine_ticks_unknown	FAIL	pass
liveness_refresh_keeps_record_whose_pid_identity_matches_not_recycled	pass	pass
liveness_refresh_keeps_legacy_record_without_start_ticks_not_recycled	pass	pass

cargo fmt --all --check, cargo clippy -p rocm --all-targets -- -D warnings, and cargo test -p rocm --all-targets (1250 passed) all pass on this branch.

Merge Danger

Door: two-way

It only changes how existing records are classified. Nothing is written beyond the status demotion that a dead-PID record already got.

Blast Radius: narrow

Only records whose recorded start-ticks provably mismatch are affected. Those are now reported stopped instead of starting/ready, so rocm serve relaunches instead of reusing them, and daemon recovery sees them as stopped.

---

• If this PR fixes a bug, searched tests/e2e-cucumber/expectations.toml for the fixed ticket ID and removed/narrowed any now-stale xfail rows. (No upstream ticket ID; no matching xfail row.)
• If this PR adds a new subcommand or subsystem, its domain implementation lives in its own file per docs/architecture.md. (N/A: no new subcommand.)
• Every new or changed user-facing message was read against the code path that runs after it, and its test asserts the resulting state, per AGENTS.md §3. (No user-facing message changed; tests assert the resulting record status.)

[jussielo-amd]

🔴 Automated review · pr-review-watcher · 7580227

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 (one file, apps/rocm/src/main.rs, +114/-10,
d4e7c751...75802273). No ticket named (PR body: "No upstream ticket ID; no
matching xfail row"). Implements exactly what the PR body describes: pairs each
recorded managed-service PID with its own start-ticks via
rocm_core::identity_state in the liveness-refresh path, matching what the stop
path already does; deliberately leaves managed_service_record_is_in_use (the
prune guard) on bare-PID liveness.

Blocking

• Two currently-red CI checks on this commit — E2E tests (Strix Halo, Windows, current) (run 39168498720, job 117895933738) and E2E tests (run
  39168498465, job 117898980237).
  Windows: FAIL: 'diagnose-fix-path-prepends-with-apostrophe-intact' was expected to pass on this host but FAILED — a regression (145 scenarios, 141
  passed). The plain CI workflow's E2E tests job: FAIL: 'dash-gen-tps-expiry-boundary' and FAIL: 'dash-esc-menu-quit-row-confirms-when-serving' (224 scenarios, 220 passed).
  Both failing areas are unrelated to this diff (PATH/apostrophe handling in
  diagnose; dashboard TPS-expiry timing and TUI quit-confirm) — the change
  touches only managed-service PID/liveness classification — and main's most
  recent runs, including this PR's own base commit, are green, so these read as
  host/timing flakes rather than regressions this PR caused. Still, a red check
  is "not ready" until resolved per this repo's own CI-ownership policy. Please
  rerun the two jobs, and escalate if either reproduces.
  Confidence 85 · Fix: rerun; escalate with evidence if it reproduces.

• The fixed behavior is user-observable and has no Gherkin coverage, with no
  stated justification. Whole change — apps/rocm/src/main.rs.
  The bug this PR fixes is externally visible: rocm serve wrongly prints
  model already being served and exits 0 without launching anything, and
  rocm services list shows a stuck starting/ready status for a dead
  service. That's exactly the kind of command-output/exit-code/which-
  engine-it-selects behavior this repo's AGENTS.md §3 requires a Gherkin
  scenario for (tests/e2e-cucumber/features/), or an explicit statement of
  why not. I grepped the whole tests/e2e-cucumber/ tree for recycl,
  start_ticks, and the "model already being served" text: the only
  start_ticks fixtures are in service_stop_steps.rs, used with matching
  ticks to test the stop path — nothing exercises a deliberately-mismatched
  ("recycled") record through serve/services list. The repo already has the
  primitives to write one cheaply (a planted record pointed at a real decoy
  process with wrong ticks, as the new Rust unit tests do, driven through the
  actual CLI the way serve-24 and service-cleanup-07 already drive planted
  records).
  Confidence 85 · Fix: add a scenario (plant a starting/ready record whose
  PID now belongs to a live decoy process with different start-ticks; run
  rocm serve/rocm services list; assert the corrected status/relaunch), or
  state in the PR body why this is scenario-exempt.

Non-blocking

• managed_service_record_is_in_use is left on bare-PID liveness, and that's
  a documented, reproduced bug, not the "harmless" over-reporting the PR body
  describes. apps/rocm/src/main.rs:4155 (see inline comment).
  Open issue #614 reproduces this exact function stranding recycled-PID
  records as permanently unprunable (stop and prune disagree about
  liveness) and sharing a misleading "stop it first" hint with genuinely
  ready records — one incident in #614 actually killed a live server by
  following that hint. The code here is unchanged in behavior (just retyped to
  read id.pid off the new ProcessIdentity), so this isn't new or worsened
  by this PR and isn't a reason to block — but the "harmless" framing in the PR
  body doesn't hold up against #614. Worth noting: now that
  recorded_service_pids already returns ProcessIdentity with ticks
  attached, wiring this guard to recorded_service_process_is_alive instead
  would be close to a one-line change reusing code this PR already added.
  Confidence 85 · Fix: either fix this guard in this PR, or soften the
  "harmless" claim and link #614 as the known follow-up.

Decisions for the author

• The "dedupe pid+ticks, prefer the known token" pattern is now implemented
  three times, with different tie-break iteration order — non-blocking-improvement.
  apps/rocm/src/main.rs:8244 (see inline comment) — recorded_service_pids
  (this PR, engine-then-supervisor) vs. the pre-existing
  terminate_recorded_service_pids in the same file (supervisor-then-engine)
  vs. apps/rocmd/src/service.rs's own terminate_recorded_service_pids
  (supervisor-then-engine). Functionally equivalent today — the tie-break only
  matters if both tokens are known and conflict for the same pid, which
  nothing currently produces — but worth extracting into one rocm_core
  helper so the three can't drift further apart.

Positive signals

• Routing through rocm_core::identity_state incidentally also fixes zombie-
  process misclassification in the liveness-refresh path: the old
  process_is_running is a bare kill(pid, 0), which reads a zombie engine
  process as alive; identity_state catches it. Brings this path in line with
  the stop path, which already had that fix.
• The four new tests spawn real sleep children and read real
  /proc/<pid>/stat rather than mocking kernel state. I reverse-applied just
  this PR's four production-code hunks (keeping the new tests) and reran them:
  liveness_refresh_demotes_record_whose_pid_was_recycled and
  liveness_refresh_demotes_recycled_supervisor_when_engine_ticks_unknown
  genuinely FAIL against the pre-fix code and pass after, exactly matching the
  PR's own before/after table.

Deployment notes

None beyond what the PR body already discloses (records currently stuck
"starting" after a PID-recycling event will reclassify to "stopped" on the
next refresh — the intended fix).

What this covered

Single-pass review of the whole diff (one file, +114/-10,
d4e7c751...75802273), read completely along with its callers/callees
(managed_service_record_is_in_use, settle_pending_stop_key_cleanup,
refresh_managed_service_runtime_liveness, load_managed_services),
crates/rocm-core/src/proc_lifecycle.rs (ProcessIdentity/identity_state),
and apps/rocmd/src/service.rs's parallel stop-path implementation. Single
file at this size didn't warrant fanning out into parallel reviewers.
Verified rather than assumed: ran the new tests on the PR head (pass),
reverse-applied just the production hunks and reran them (2 of 4 fail exactly
as the PR's own table claims), ran this repo's AGENTS.md leak-scan over the
diff (clean), read the full CI status and the two failing jobs' logs, grepped
tests/e2e-cucumber/ for existing coverage, and cross-referenced prior
history (git log -L, PR #499/#667, which this PR's code descends from) and
open-issue search (#614). Did not run cargo clippy/cargo fmt locally — CI
already reports both green for this exact head commit. Not reconciled against
any discussion on this PR — it had zero comments before this review.

[jussielo-amd]

Non-blocking: this bare-PID check is unchanged in behavior by this PR (by design, per the PR body — the description calls over-reporting "in use" here "harmless"), but it's a documented, reproduced bug: open issue #614 shows it strands recycled-PID records as permanently unprunable (stop and prune disagree about liveness) and shares a misleading "stop it first" hint with genuinely ready records — one incident in #614 actually killed a live server by following that hint.

Not a reason to block this PR — pre-existing, not worsened here — but worth softening the "harmless" framing and linking #614. Also: now that recorded_service_pids already returns ProcessIdentity with ticks attached, wiring this to recorded_service_process_is_alive instead of process_is_running(id.pid) would be close to a one-line change reusing code this PR already added.

(See the full review for context.)

[jussielo-amd]

Non-blocking / decision for the author: this "dedupe pid+ticks, prefer the known token" pattern is now implemented three times with a different tie-break iteration order each time:

• here (this PR): engine-then-supervisor
• terminate_recorded_service_pids elsewhere in this file (pre-existing): supervisor-then-engine
• apps/rocmd/src/service.rs's own terminate_recorded_service_pids (pre-existing): supervisor-then-engine

Functionally equivalent today — the tie-break only matters if both tokens are known and conflict for the same pid, which nothing currently produces — but worth extracting into one rocm_core helper so the three implementations can't drift further apart. Not blocking.

(See the full review for context.)