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

[nowycondro]

Automated review (medium/high-recall pass). Two findings posted inline: a tie-break-order inconsistency between the new recorded_service_pids and the pre-existing terminate_recorded_service_pids (this file and apps/rocmd/src/service.rs) that can make the liveness check and the stop path disagree about the same record, and a reuse note about the resulting triplicated merge logic.

[nowycondro]

Tie-break order diverges from the stop path, which can make liveness and stop disagree about the same record.

When record.engine_pid and record.supervisor_pid alias the same pid (explicitly anticipated by this function's own doc comment, and by the new test liveness_refresh_demotes_recycled_supervisor_when_engine_ticks_unknown), this merge iterates engine first, so if engine_start_ticks is Some(x) it always wins over supervisor_start_ticks here (the .or() only fills a None, it never overrides an existing Some).

terminate_recorded_service_pids (line ~6942, pre-existing/untouched by this PR) performs the same kind of dedup but iterates supervisor first, so it prefers supervisor_start_ticks in the identical collision case. apps/rocmd/src/service.rs's own terminate_recorded_service_pids also prefers supervisor-first.

Failure scenario: engine_pid/engine_start_ticks can be refreshed independently of supervisor_pid/supervisor_start_ticks (via refresh_from_engine_state, which only touches the engine_* fields and runs before this liveness check on every load_managed_services). If a stale engine_state_path file (not yet overwritten by a freshly-restarted engine) reports a server_pid/ticks pair that now numerically collides with the current supervisor_pid — exactly the kind of PID-recycling coincidence this PR is about — recorded_service_pids will keep the stale/wrong engine ticks and report the pid as Recycled (dead), while terminate_recorded_service_pids would correctly keep the fresh/matching supervisor ticks and report it Matches (alive). That means rocm services list/the liveness refresher could wrongly demote an actually-live, correctly-identified service to stopped, while a stop/restart issued against the very same record would have signalled the real process — the two code paths silently disagreeing about one record.

Suggest extracting one shared merge helper (used by all three call sites) with one canonical tie-break rule, rather than three independent re-implementations with two different orders.

[nowycondro]

Reuse: this is now the third independent implementation of the same "dedupe engine/supervisor pid+ticks, preferring a known token" logic, alongside terminate_recorded_service_pids in this file (line ~6942) and terminate_recorded_service_pids in apps/rocmd/src/service.rs (line ~226). The three copies differ in iteration order (this one and apps/rocmd's prefer different roles — see the other comment) and in whether the acting process's own pid is excluded. A single shared helper in rocm-core returning the deduped Vec<ProcessIdentity> (with the tie-break rule decided once) would remove the duplication and the risk of the three call sites drifting out of sync, as they already have.

[nowycondro]

🤖 Automated review (Claude Code) · 7580227

Reviewed the diff in apps/rocm/src/main.rs (+114/-10) that adds recorded_service_pids and routes managed-service liveness refresh through rocm_core::identity_state to verify start-ticks before trusting a recorded PID. Read the full diff via GitHub's PR API (get_diff/get_files), the surrounding callers (refresh_managed_service_runtime_liveness, managed_service_record_is_in_use, load_managed_services), and the analogous pre-existing terminate_recorded_service_pids in this file and in apps/rocmd/src/service.rs. No Rust toolchain available in this sandbox, so I did not build or run the test suite — this is a static read of the diff plus cross-referencing other code paths, not a verified repro.

Findings (posted as inline comments on this PR already; summarized here):

1. Non-blocking (medium confidence) — tie-break inconsistency across three duplicate implementations. apps/rocm/src/main.rs:8235 (new recorded_service_pids) merges engine-then-supervisor, preferring the engine's start_ticks on a pid collision. The pre-existing terminate_recorded_service_pids in the same file (~line 6942) and the copy in apps/rocmd/src/service.rs merge supervisor-then-engine, preferring the supervisor's. Since engine_pid/engine_start_ticks can be refreshed independently of supervisor_pid/supervisor_start_ticks (via refresh_from_engine_state), a stale engine-state snapshot that now numerically collides with the current supervisor pid could make the liveness check (this PR) and the stop path (existing code) resolve the same record to opposite verdicts — one says "recycled/dead", the other says "matches/alive". This requires a fairly specific PID-recycling coincidence to trigger, so I'm flagging it non-blocking/worth-a-look rather than blocking, consistent with the existing reviewer's assessment that today nothing actually produces a conflicting pair of known tokens.
2. Non-blocking (cleanup) — the same pid+start_ticks dedup/merge logic is now implemented three times (this new function, plus the two terminate_recorded_service_pids copies) with no shared helper, which is how the tie-break order in finding 1 was able to drift in the first place.

I did not find any new blocking correctness bugs beyond what's noted above. I note the existing jussielo-amd review on this same head commit already raised two blocking items (red CI checks, missing Gherkin coverage for the user-visible behavior change) — I did not re-verify CI status or test coverage myself and defer to that review on those points; my pass focused on the identity/merge logic itself.

Not verified: build/compile, cargo test/cargo clippy, CI status, e2e coverage gaps.

🤖 Generated with Claude Code

[nowycondro]

🤖 Automated review (Claude Code) · 7580227

I reviewed the diff in apps/rocm/src/main.rs (the only changed file), comparing the new/modified recorded_service_pids path against the existing terminate_recorded_service_pids and crates/rocm-core/src/proc_lifecycle.rs identity-matching logic it's meant to mirror.

Two issues worth flagging, both medium confidence:

1. apps/rocm/src/main.rs:8235 — recorded_service_pids merges a duplicate pid by preferring the engine's start-ticks over the supervisor's (existing.start_ticks.or(ticks) with engine processed first), the opposite precedence from terminate_recorded_service_pids, which prefers the supervisor's ticks. If a record's engine_pid equals its supervisor_pid and the engine's recorded start-ticks are stale (e.g. from a crashed engine whose state file was never rewritten) while the supervisor's ticks are fresh and correct, the two code paths will disagree about the same pid's identity — one may call it Recycled/dead, the other Matches/alive. That's exactly the false-negative liveness misclassification this PR appears to be trying to eliminate, just reintroduced via inconsistent precedence between the two call sites.

2. apps/rocm/src/main.rs:4151 — managed_service_record_is_in_use still does a bare process_is_running(pid) check without the new identity/start-ticks verification, even though recorded_service_pids now returns ProcessIdentity. If a record's status is already stopped but its recorded pid has been recycled by an unrelated live process, this function reports "in use," producing a self-contradictory error from remove_managed_service_record ("is stopped ... while it is running") and blocking a rocm services remove that should be safe. The PR description calls this acceptable because it "only blocks a prune," but the error message is confusing and unactionable for the user.

Non-blocking: I was not able to build or run the test suite in this sandbox (no Rust toolchain / network restrictions), so these findings are based on static reading of the diff and surrounding logic, not verified by execution or by the existing test suite if one covers this path.

🤖 Generated with Claude Code

[nowycondro]

🔴 Automated review · pr-review-watcher · 7580227

Review — needs work

Full review of the whole change (one file, apps/rocm/src/main.rs, +114/-10,
origin/main...75802273, single commit). No ticket named in the PR body. The
change pairs each recorded managed-service PID with its own start-ticks via
rocm_core::identity_state in the liveness-refresh path (settle_pending_stop_key_cleanup,
refresh_managed_service_runtime_liveness), bringing it in line with the
pre-existing stop path (terminate_recorded_service_pids); it deliberately
leaves managed_service_record_is_in_use (the prune guard) on bare-PID
liveness.

Blocking

• Two CI checks are red on this exact head commit. E2E tests (Strix Halo, Windows, current) (run 39168498720) and E2E tests (run 39168498465),
  re-verified live immediately before writing this review — both still
  FAILURE as of now. The failing scenario names (Windows PATH/apostrophe
  handling in diagnose; dashboard TPS-expiry timing and a TUI quit-confirm
  prompt) are unrelated to this diff, which only touches managed-service
  PID/liveness classification, so these likely are flakes rather than a
  regression this PR caused — but a red check is not a ready-to-merge state.
  Confidence 85 · Fix: rerun the two jobs; escalate if either reproduces on
  rerun.

• The user-visible bug this PR fixes has no Gherkin coverage, and the PR
  body gives no exemption. apps/rocm/src/main.rs (whole change). The bug
  is externally observable: rocm serve prints model already being served
  and exits 0 without launching anything, and rocm services list shows a
  stuck status for a dead service — exactly the class of command-output/exit-
  code behavior AGENTS.md §3 requires a scenario for in
  tests/e2e-cucumber/features/, or an explicit statement of why not. I
  grepped the whole tests/e2e-cucumber tree for start_ticks, recycl, and
  already being served: the only start_ticks fixtures are in
  service_stop_steps.rs, driven with matching ticks to test the stop path —
  nothing exercises a deliberately mismatched ("recycled") record through
  serve/services list. The repo already has what's needed to add one
  cheaply (plant a starting/ready record pointed at a live decoy process
  with different start-ticks, then run rocm serve/rocm services list,
  following the pattern the new Rust unit tests already use).
  Confidence 85 · Fix: add that scenario, or state in the PR body why this
  change is scenario-exempt.

Non-blocking

• managed_service_record_is_in_use is left on bare-PID liveness, and the
  PR body's "harmless" framing doesn't hold up against open issue #614.
  apps/rocm/src/main.rs:4155 (managed_service_record_is_in_use). Issue
  #614 (open) reproduces this exact function stranding recycled-PID records as
  permanently unprunable — stop and prune disagree about liveness — and
  documents an incident where following the prune skip-hint ("stop it first")
  actually killed a live, ready server because the hint didn't distinguish a
  truly-dead record from a live one. This code is functionally unchanged here
  (just retyped to read id.pid off the new ProcessIdentity), so it isn't
  new or worsened by this PR — but calling the over-reporting "harmless" in
  the PR body conflicts with a reproduced, documented live-server kill. Now
  that recorded_service_pids returns ProcessIdentity with ticks attached,
  wiring this guard to the new recorded_service_process_is_alive would reuse
  code this PR already adds.
  Confidence 85 · Fix: either fix this guard in this PR, or soften the PR
  body's "harmless" claim and link #614 as the known follow-up.

Decisions for the author

• The "dedupe pid+ticks, prefer the known token" merge is now implemented
  three times, with different tie-break order — non-blocking-improvement.
  apps/rocm/src/main.rs:8235 (new recorded_service_pids, engine-then-
  supervisor: on a pid collision the already-seen engine entry's ticks win
  unless engine's own ticks are None) vs. apps/rocm/src/main.rs:6942
  (pre-existing terminate_recorded_service_pids, supervisor-then-engine) vs.
  apps/rocmd/src/service.rs's own terminate_recorded_service_pids
  (supervisor-then-engine). Traced through by hand: today nothing populates
  conflicting known tokens for the same pid across engine/supervisor, so the
  three disagreeing implementations produce the same answer in practice — but
  if a stale engine-state snapshot ever numerically collides with a current
  supervisor pid, the liveness check (this PR) and the stop path (existing
  code) could resolve the same record to opposite verdicts. Worth extracting
  into one rocm_core helper so the three can't drift further.

Positive signals

• Routing the liveness-refresh path through rocm_core::identity_state
  incidentally also fixes zombie-process misclassification there: the old
  process_is_running is a bare kill(pid, 0), which reads a zombie engine
  process as alive, while identity_state additionally checks
  process_has_exited. This brings the liveness path in line with the stop
  path, which already had that fix.
• The four new unit tests spawn real sleep children and read real
  /proc/<pid>/stat rather than mocking kernel state, and are gated correctly
  behind #[cfg(target_os = "linux")]. I could not run them myself (no Rust
  toolchain in this environment), but their structure and assertions match the
  PR's own stated before/after table and the surrounding non-test code.

Deployment notes

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

What this covered

Read the whole diff (origin/main...75802273, single file, +114/-10,
single commit) completely, together with its callers/callees in the same
file (managed_service_record_is_in_use, settle_pending_stop_key_cleanup,
refresh_managed_service_runtime_liveness, terminate_recorded_service_pids),
crates/rocm-core/src/proc_lifecycle.rs (ProcessIdentity/IdentityState/
identity_state/terminate_verified) in full, and the parallel
terminate_recorded_service_pids in apps/rocmd/src/service.rs. Verified
independently rather than assumed: fetched the PR head directly by SHA and
confirmed it matches 75802273; pulled live CI status twice (including
immediately before posting) and confirmed the same two checks are still red;
grepped the full tests/e2e-cucumber tree for existing coverage of this
behavior and found none; read AGENTS.md's own Gherkin-scenario rule (§3) in
this repo; fetched and read open issue #614 in full and cross-checked its
reproduction against the current managed_service_record_is_in_use code;
hand-traced the tie-break logic of all three pid/ticks merge implementations
against each other. Did not run: cargo test/cargo clippy/cargo fmt (no
Rust toolchain available in this sandbox) — relying on CI's own report for
build/lint/test results instead. This is a single, small, single-file change;
it didn't warrant fanning out into parallel reviewers. Not reconciled against
any prior review or discussion on this PR — reached independently from the
code, CI, issue tracker, and repo guidance only.