[rominf]

• 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. — N/A, test-only.
• If this PR adds a new subcommand or subsystem, its domain implementation lives in its own file per docs/architecture.md. — N/A.
• Every new or changed user-facing message was read against the code path that runs after it, and its test asserts the resulting state — not only the wording — per AGENTS.md §3. — N/A, no message changes.

Why

A tree stop signals every PID that collect_process_tree returns for the service's root PID: terminate_verified walks once to snapshot the tree it will wait on and again to pick the signal targets. Since #499 routed rocmd's managed-service stop through terminate_verified, that one walk decides the kill set for both rocm services stop and rocmd. A wrong walk either leaves a GPU worker running or signals a process that is not the service's, and until now the walk had no test of its own: the existing tree tests only use small real process trees, where cycles and other odd tables never occur.

What changes

• Refactor, no behaviour change. The traversal moves out of collect_process_tree into walk_process_tree, which takes an already-built parent-to-children table. collect_process_tree still reads /proc and hands the table over. The walk's doc now states its ordering: root first, every other PID after its parent, no PID twice.
• Property tests (crates/rocm-core/src/process_tree_properties.rs, Unix-only, mounted as a child module of process.rs like examine_driver_proptests.rs):
  • Generator. Tables of one to six PIDs over a six-PID alphabet (including 0 and 1, so init's ppid == 0 shape occurs), with unconstrained parents, so shared parents, cycles and self-parents come up in a large share of cases. A consistent process tree has no cycle, but the table is built from one stat read per PID at different moments, so a cycle from a PID reused mid-scan is rare but cannot be ruled out, and the walk must terminate on it. A self-parent cannot come from /proc and is included as the one-node case of the same cycle.
  • walk_matches_reachability_oracle: the walk's PID set equals an independent breadth-first reachability oracle; the root leads; no PID is listed twice.
  • parents_are_listed_before_their_children: every member after the root comes after its parent, as the walk documents.
  • Each walk runs on a thread with a 5 s bound. A walk that spins on a cycle fails with its input instead of hanging the job (the process exits rather than letting proptest shrink by re-running a spinning walk; under nextest that ends one test).
  • generator_reaches_the_interesting_shapes guards against a vacuous suite. Over 1024 deterministic draws the generator reaches a shared parent in 567, a self-parent in 443, a cycle of two or more PIDs in 238, and a root lying on such a cycle (the shape where the walk's seen check matters) in 116; the floors are about half of each.
  • 4096 cases by default, the same depth therock_version_proptests.rs uses for cheap pure functions; PROPTEST_CASES overrides it, as in examine_driver_proptests.rs.

No new dependencies: proptest is already a rocm-core dev-dependency, so Cargo.lock, MANIFEST.md and the third-party notices are untouched.

Can the properties fail?

Each mutant was applied to the walk (or the generator), the suite run, and the original restored:

Mutation	Caught by
Walk direct children only (grandchildren left behind)	reachability oracle
Push a PID before the seen check (a PID signalled twice)	oracle, parent-order
Seed the stack with root + 1 (a stranger adopted)	oracle, parent-order
Drop the seen check	5 s bound: exits 101 printing the offending table
Sort everything after the root in descending order (child before parent)	parent-order only
Generator forces ppid >= pid (no cycles)	generator guard: cycle of two or more PIDs: 0/1024, floor 120

The walk itself passes every property on main; no defect was found in it.

Scope

• Not covered here, and unchanged: the /proc side of the walk (read_parent_pid and the table assembly in collect_process_tree); the test's children_of mirrors that table builder rather than exercising it. A parse property for read_parent_pid (comm names containing ) or spaces) would be a natural follow-up.
• rocmd's former ps-based descendant walk is not tested here because fix(rocmd): verify process identity before stopping a managed service #499 removed it; fix(rocmd): verify process identity before stopping a managed service #499 also ships the tests that a recorded PID whose start time no longer matches is refused, and that its subtree survives (stop_managed_service_refuses_a_pid_whose_recorded_identity_no_longer_matches, stop_managed_service_leaves_the_subtree_of_a_refuted_pid_running).

AGENTS.md §3

Test-only apart from a private refactor: no CLI output, exit code, file layout or runtime selection changes, so no Gherkin scenario is needed.

Verification

On Linux (WSL2):

• cargo test -p e2e-cucumber -p rocm -p rocm-core -p rocm-dash-daemon -p rocm-engine-aim -p rocm-engine-lemonade -p rocm-engine-protocol -p rocm-engine-vllm -p rocmd -p xtask (the set cargo xtask affected selects): exit 0, 3501 passed.
• cargo clippy --locked --workspace --all-targets -- -D warnings: exit 0.
• cargo fmt --all -- --check: exit 0.
• prek run --from-ref origin/main --to-ref HEAD: all hooks pass except the hawkeye license-header hook, which could not run here because the locally installed hawkeye is older than the version licenserc.toml targets; the new file carries the same SPDX header as its siblings, and the License header check (hawkeye) CI job uses the pinned version.

Not verified locally: Windows. The new code is cfg(not(windows)) throughout, so a Windows build does not compile it.

[nowycondro]

🤖 Automated review (Claude Code) · d140541

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.
No ticket named (the PR references #499 only as background context for why the walk matters, not as a dependency or a "fixes" relationship).

Blocking

None

Non-blocking

• walk_bounded's recv_timeout conflates "timed out" with "sender disconnected" — crates/rocm-core/src/process_tree_properties.rs:62-70
mpsc::Receiver::recv_timeout returns Err both on a genuine 5s timeout and immediately on RecvTimeoutError::Disconnected (e.g. if the spawned thread panicked instead of hanging). Both cases take the same branch and print "walk_process_tree did not terminate within 5s", which would misattribute a future panic as a hang. Currently dormant: walk_process_tree's body (HashMap insert/push/extend only) has no path that can panic today, and Rust's default panic hook would still print the real panic to stderr first, so the true cause wouldn't be fully hidden — just awkwardly double-reported if the walk ever grows a fallible operation.
  Confidence 72 · mechanical · Fix: match on the RecvTimeoutError variant and report "panicked" vs. "timed out" distinctly, or let the panic propagate via .join() instead of a channel.

Decisions for the author

• read_parent_pid and the /proc table-assembly side of collect_process_tree remain untested by this PR — non-blocking-improvement
  The property tests cover the traversal (walk_process_tree) thoroughly but not the parsing that builds its input table from /proc/<pid>/stat (handling of comm names containing spaces or )). The PR's own scope section already names this as a deliberate follow-up rather than an oversight, and the traversal is the half carrying the cycle/correctness risk this PR set out to close, so not fixing it now is a reasonable choice — raised here so the author can confirm a follow-up is still intended.

Positive signals

• The independent oracle (reachable, a BFS over VecDeque) is a genuinely different algorithm from the DFS-with-stack implementation under test, so a bug shared between "how I check" and "how I compute" is unlikely to slip through both — verified by hand-tracing a 2-cycle, a self-parent, and a shared-parent diamond against both.
• generator_reaches_the_interesting_shapes guards the suite against going vacuous (a generator that silently stopped producing cycles/self-parents would otherwise pass every property while testing nothing interesting) — a good instance of a pattern already established in this crate (examine_driver_proptests.rs's inputs_reach_every_region) and applied correctly here with its own calibrated floors.
• The bounded-thread-plus-process::exit(101) harness turns a would-be CI hang (on a walk that spins forever on a cycle) into an immediate, diagnosable failure that prints the offending table — thoughtful test-infrastructure design for a traversal that must terminate.
• The refactor pulling walk_process_tree out of collect_process_tree is byte-identical in the moved logic (same seen-before-push order, same stack semantics) — a genuinely behavior-preserving extraction, confirmed by diffing the moved body directly rather than trusting the PR's own "no behaviour change" claim.

Deployment notes

None — test-only change (plus a behavior-preserving internal refactor), Unix/Linux-gated (#[cfg(not(windows))]), no CLI-observable, config, or dependency changes. proptest was already a rocm-core dev-dependency before this PR.

What this covered

Read both changed files completely at the PR head (crates/rocm-core/src/process.rs's full ~2045 lines and the new 223-line process_tree_properties.rs), plus the surrounding callers (signal_process_scope, process_tree_pids, read_parent_pid, the existing mod tests block) and the two precedent files this PR explicitly imitates (examine_driver_proptests.rs, archive_props.rs), in full. Checked history of process.rs and of both precedent proptest files for prior mistakes or caveats (none found relevant), and checked review feedback on PR #499 (the PR that made this walk load-bearing for rocmd) for carryover relevance — that feedback concerned service-record/identity-verification logic, not process-tree traversal, so none of it applies here. Checked the repo's root AGENTS.md rubric in full against the change (license headers, platform-gating conventions, coverage-floor interaction, the Gherkin-scenario requirement, the subprocess/FFI checklist) and found full compliance. CI checked live via the GitHub API: all required checks on d140541 are green (clippy, fmt/prek, the affected-crates test job, Windows build-and-test, Commit signatures + sign-off, CodeQL, etc.); two Strix Halo Ubuntu E2E hardware lanes (nightly/release) show "cancelled" rather than failed, matching a previously-documented benign hardware-lane-scheduling pattern on this repo rather than a regression from this change.
Reviewed via two independent fanned-out passes (a code-coupling pass over both files together, and an agent-instruction-adherence pass against AGENTS.md) plus a design-review pass reasoned through directly. No local cargo/rustc toolchain was available in this environment, so no test was executed directly — findings and verifications are from reading the code and hand-tracing concrete example tables (a 2-cycle, a self-parent, a shared-parent diamond) against both the implementation and its oracle, not from a run; CI's own green Test (affected crates) run is relied on for the executed result. The history/prior-changes pass (Step 4b) was run by the orchestrating pass directly rather than as a separate independent fanned-out worker, because a one-time subagent concurrency limit (shared with unrelated sessions on this host) prevented a third worker from launching; it is therefore not fully independent of the rest of this review, though it used the same blind, evidence-only method. Not reconciled against any discussion on this PR — it currently has zero existing reviews.

---

🤖 Generated with Claude Code

[jussielo-amd]

Looks good, thanks Romain. No blockers.

• The walk_process_tree extraction is a behaviour-preserving move (body matches the old tail of collect_process_tree; all callers still go through it).
• The BFS oracle is independent of the DFS under test, and the generator guard keeps the suite from going vacuous. Windows gating is complete.

Non-blocking

• Without the seen check, a cyclic root makes the spawned thread grow order for the full 5 s before exit(101); that could be a lot of memory and an OOM-kill could pre-empt the diagnostic. Only reachable when the suite is already red, but capping output inside the thread would make failures cleaner.
• process::exit(101) ends the whole binary under plain cargo test (one test under nextest). The doc comment says so; just confirming it's a deliberate trade.

Heads-up: main has moved process.rs, but merge-tree is clean.

---

🤖 by agenthub on AMD AgentHub