[jussielo-amd]

Closes ROCMAI-874, ROCMAI-875, ROCMAI-876 and ROCMAI-879.

Summary

rocm and rocmd are two binaries that act on the same managed-service records, and four pieces of logic they need in common had each been written twice, or three times, and had drifted apart. This change replaces each pair with one implementation in a crate both binaries may depend on (rocm-core, or rocm-engine-protocol for the engine client; rocmd must not depend on rocm, so the shared code cannot live in either app), deletes the private copies, and makes the callers thin. In each case the copy that reports more or fails more safely is the one that survives, and every resulting difference a user or an MCP client can see is listed below. Along the way the work found and fixed three real defects that the duplication had hidden: the standalone rocmd could not reach any engine, a printed remediation command was not runnable, and a log view reported a log that had content as empty.

The four consolidations are independent of one another and are delivered as four sections below, each with its own commits, in the order they appear in the history: the managed-service stop routine, the endpoint-key respawn guard, the engine stdio client and the log tails. Within each section the first commit only pins what the copies did before (tests that pass on the unmodified code), so the commit that follows reads as a diff of behaviour. Signed merge commits bring the four lines of work together (and later merges bring in main); the conflicts were in docs/architecture.md, docs/testing.md, the service_stop feature file and apps/rocmd/src/engine_ipc.rs, and the one scenario-number collision is described under User-visible behaviour.

Managed-service stop routine (ROCMAI-874)

rocm services stop and rocmd's stop_server stop the same service records, but each carried its own copy of MANAGED_STOP_GRACE (10 s, kept in step only by the doc comment "Matches rocm services stop") and its own terminate_recorded_service_pids. The copies had diverged. rocmd reported the stopping process's own PID as self and, on Windows, ran taskkill /PID <pid> /T /F on a verified root before terminate_verified, because the engine runs one level below the recorded PID and terminate_verified cannot walk a Windows process tree (its process_tree_pids returns only the root there). The rocm copy dropped its own PID from the report and had no tree-kill step.

Whether rocm services stop on Windows actually leaves the engine child running is not confirmed: I did not reproduce it and have no Windows host here. What can be verified from the code is only that the rocm copy had no step that could take a child below the recorded PID on Windows, and that rocmd added one for exactly that reason. This change makes both binaries run the same routine, so the question stops depending on which binary stopped the service.

rocmd's behaviour wins, because it is a superset: it reports every recorded PID including the stopping process's own, and it carries the Windows descendant kill. The one routine lives in rocm-core's proc_lifecycle.rs beside terminate_verified, with the per-PID termination still injectable as rocmd already had it.

Symbol	Was	Now
MANAGED_STOP_GRACE	private const in apps/rocm/src/service_control.rs and in apps/rocmd/src/service.rs	rocm_core::MANAGED_STOP_GRACE
terminate_recorded_service_pids	two private fns, returning RecordedStopTermination (rocm) and Vec<RecordedPidStop> (rocmd)	rocm_core::terminate_recorded_service_pids(record, terminate) -> Vec<RecordedPidStop>
terminate_recorded_pid (Windows taskkill /T /F before the verified termination)	private, cfg(windows) block in rocmd only	rocm_core::terminate_recorded_pid, built on terminate_recorded_pid_with whose tree-kill, identity read and termination are arguments
RecordedPidStop, RecordedPidOutcome	rocmd private struct (string outcome) and rocm private struct	rocm_core::RecordedPidStop / RecordedPidOutcome with signaled(), forced(), stopped() and report_entry()
rocm's RecordedStopTermination, RecordedPidOutcome	private structs	deleted; stop_recorded_processes in service_control.rs is a thin wrapper that adds only the ROCM_E2E_STOP_OUTCOME test seam

Commits, in order: the first pins what each copy did (grace value, visiting order, start-time pairing, aliasing, zero PIDs, classification) and the two divergences, with no production change; the Windows tree kill could not be pinned there because it was a private cfg(windows) block with no injection point, only its portable half is. The second adds the routine and constant to rocm-core, deletes both private copies, rewrites the two callers, replaces the stop stand-ins in services_text.rs's tests, and updates the docs. Two commits from review: a confirmed rocm stop now clears the recorded processes through the new shared ManagedServiceRecord::clear_recorded_processes (which rocmd calls too), and the last commit adds the end-to-end scenarios for the Linux and Windows stops. Two merges of main sit inside this line of work because the stop path was moved out of services_text.rs into service_control.rs by an earlier merged change; the consolidation was re-applied at service_control.rs.

Differences a user can see, with the surface each appears on:

1. Windows only: rocm now takes a stopped service's descendants down with taskkill /PID <pid> /T /F. This applies to every path that goes through stop_internal_managed_service_confirmed_with: rocm services stop, rocm services restart (and the restart that moves a service onto a new runtime), rocm sandbox-run stop_server|restart_server, and the stop in the attached rocm serve Ctrl-C handler. The step runs only for a root whose identity gate matches, before the bounded wait. A root the tree kill already took would read back as already_gone, so it is reported forced instead (as rocmd already did); that means such a PID now appears in signaled_pids and in the stopped processes: N count. On Linux nothing is added: terminate_recorded_pid has no tree-kill step there, exactly as before.
2. pid_outcomes in rocm's stop report now includes the stopping process's own PID as {"pid": <pid>, "role": ..., "outcome": "self"}, as rocmd's already did. It was previously omitted. It is not signalled, counts as stopped, stays out of signaled_pids, and does not change the unconfirmed-stop text, which only acts on timed_out and unverified outcomes. In practice it appears only when the record's PID is the calling process (an attached rocm serve, or a test); a separate rocm services stop process is never the recorded one.
3. rocmd: no change. Same visiting order (supervisor, then engine, each with its own start-time, an aliased PID once under supervisor_and_engine), same self handling, same classification into signaled_pids, force_signaled_pids and skipped_pids, same 10 s grace. The grace is 10 s in both before and after.
4. A confirmed rocm services stop now clears the recorded processes from the record, as rocmd's stop already did: after it, the persisted record has supervisor_pid 0, engine_pid null and both start-time tokens null (status stopped, no stop request). Before, rocm left the stopped PIDs and tokens in the manifest. It is one call, ManagedServiceRecord::clear_recorded_processes, made from the confirmed branch of both stops (the four assignments in rocmd's stop are replaced by it). An unconfirmed stop keeps the PIDs, because the record is the only thing that names a survivor. This is what makes the next point safe: a later stop of the same record finds nothing to signal.

Windows identity (review fix). The tree kill is behind the same identity check as the rest of the stop. On Windows process_start_ticks is now the process creation time (GetProcessTimes, a FILETIME); it returned None there, so no recorded identity could be refuted and a recycled PID passed the gate in front of taskkill /T /F. A record written by this version refutes a PID that an unrelated process has taken: the stop reads it back as recycled, runs no taskkill on it and reports identity_mismatch. A record written before this has no token and stays best-effort, and a confirmed stop clears the recorded PIDs (difference 4), so that exposure is limited to records that predate the token and have not been stopped since. A recorded token that can no longer be read (a process owned by a more privileged account) reads as unverified and is not signalled. That outcome used to be Linux-only, so the check it advises is now tasklist /FI "PID eq <pid>" on Windows (it was ps -p <pid> -o pid,args, which Windows does not have). I could not run any Windows code here: the Windows-only tests and the Windows half of service-stop-09 are for the Windows jobs to exercise, and the Windows clippy check compiles the new FFI.

rocm-core now exports MANAGED_STOP_GRACE, RecordedPidOutcome, RecordedPidStop, terminate_recorded_pid and terminate_recorded_service_pids; terminate_recorded_pid_with stays private. taskkill is run exactly as rocmd ran it (blocking std::process::Command::status, no timeout, output discarded). I kept that so this stays a consolidation; a bounded wait would be a separate change.

Endpoint-key respawn guard (ROCMAI-876)

The guard that refuses to bring a managed service back up when it needs an endpoint API key and has none existed twice: in rocm (service_launch.rs) and in rocmd (engine_ipc.rs, which said it "mirrors the guard of the same name in rocm"). Both applied the same two rules (a service launched with --require-api-key that has no key; a service bound to a public host with no key) but worded them differently and named slightly different remediation commands, so a user saw one text when rocm services restart refused and another when the daemon's recovery refused the same service. Both are replaced by one guard in rocm-core that takes the action wording as a parameter, and both copies are deleted. is_public_bind_host, which the guard has to apply itself, moves with it from rocm-engine-protocol to rocm-core (no engine used it, and rocm-engine-protocol depends on rocm-core, not the other way round).

While proving that the remediation command the refusal names actually clears the condition, the new test found that the printed command was not runnable: rocm serve --require-api-key leaves out the required model, so clap rejects it as printed. The shared text now names rocm serve <model> --require-api-key (and rocm serve <model> --host {host} --port <port> --allow-public-bind; main has since started requiring an explicit --port with any host but the default loopback, and the text names it).

Symbol	From	To
ensure_public_service_has_endpoint_key	apps/rocm/src/service_launch.rs and apps/rocmd/src/engine_ipc.rs (two copies, deleted)	crates/rocm-core/src/endpoint_key.rs, taking a ServiceStart
ServiceStart (Launch, Restart, Respawn)	new	crates/rocm-core/src/endpoint_key.rs
is_public_bind_host	crates/rocm-engine-protocol/src/lib.rs	crates/rocm-core/src/endpoint_key.rs (its test moved with it, same leaf name)

Callers: service_launch.rs (spawn, Launch), service_control.rs (rocm services restart, Restart), supervise.rs (two sites) and watchers/server_recover.rs (Respawn). mcp_argv.rs and is_loopback_host in service_launch.rs now reach is_public_bind_host through rocm_core. Commits, in order: the first pins both copies' shipped text and the check that the command each names clears the refusal (for rocm, through the real clap tree, validate_bind_host, resolve_endpoint_auth and the stored key file; for rocmd, the key file rocm serve writes), and records the missing-model gap as it stood; the second moves the guard, flips that assertion, rewrites the call-site tests to go through the shared guard, adds the restart scenario and updates the docs. A merge of main follows.

Which wording wins: rocm's. It says why the key cannot be recovered ("the key is dropped when a service stops") and how to choose one (--api-key, ROCM_SERVE_API_KEY), and it states the rule that actually fires (--require-api-key) where rocmd's said "launched with an endpoint API key", which is also true of every public bind. The action wording is the only thing the caller supplies. Every difference, per binary:

• rocm services restart, both rules: the text is byte-identical except the named command now carries the model, rocm serve <model> --require-api-key and rocm serve <model> --host {host} --port <port> --allow-public-bind (was rocm serve --require-api-key and rocm serve --host {host} --port <port> --allow-public-bind).
• rocm serve spawn (reached only when a key file is present but unusable, since serve() stores the key first): "so restarting it would reopen it without authentication" becomes "so launching it would open it without authentication", because nothing is being restarted there; plus the same <model> change.
• rocmd respawn (the supervisor's refusal on stderr, and the server-recover watcher's restart_managed_service_refused event, which wraps it in "cannot recover managed service ... : ..."): rule 1 was "refusing to recover a service that was launched with an endpoint API key but no longer has one: it would come back up without authentication, and something outside this machine may still be publishing its port. Relaunch it with rocm serve --require-api-key to issue a new key." and is now rocm's rule 1 with "respawning it would reopen it without authentication", the dropped-key sentence, the <model> command and the "add --api-key <key>, or set ROCM_SERVE_API_KEY" hint. Rule 2 was "refusing to respawn a service bound to the public host {host} without an endpoint API key: it would come back up without authentication. Relaunch it with rocm serve --host {host} --port <port> --allow-public-bind to issue a new key." and is now the shared rule 2 with the same changes. The "refusing to ..." prefix is gone.
• No change to which services are refused, to the rule order (a service that asked for a key is told about the key before the public bind is mentioned), or to the refusal happening before a restart stops anything.

rocm_engine_protocol::is_public_bind_host no longer exists; nothing in the workspace outside the two binaries used it. The <model> placeholder is the one advised_commands.rs already substitutes, so its scan covers the shared text too (it scans crates/).

Engine stdio client (ROCMAI-875)

rocm and rocmd each carried their own client for the JSON-over-stdio request to an engine process, and the two had drifted apart. They now run the child through one implementation, stdio_client.rs in rocm-engine-protocol, and each caller keeps only what is genuinely its own: which program to start, and a small set of options for what it asks of the client. The wire format does not change: the request and response envelopes are untouched, so the engines need no change.

Writing the characterisation tests first also settled the open question about the standalone rocmd binary, and it turned out to be a live bug rather than a corner case. rocmd reached an engine by re-executing its own executable as __engine-stdio <engine>, a verb that only rocm serves. Embedded as rocm daemon that works. The standalone rocmd shipped beside rocm (it is in the release packages and is what the e2e suite drives) answers every such request with error: unrecognized subcommand '__engine-stdio' and exit status 2. So from the standalone daemon the engine Stop of rocmd sandbox-tool stop_server always came back as an error, and every healthcheck failed; for an engine whose stop decides confirmation (aim) the stop could never be confirmed. One commit fixes that on its own.

Was	Now
rocm's engine_stdio_child_request body: spawn, write the request, collect stderr, read stdout, wait, parse (apps/rocm/src/engine_ipc.rs)	run_engine_stdio in crates/rocm-engine-protocol/src/stdio_client.rs; rocm keeps a thin engine_stdio_child_request that picks the options
rocmd's engine_request body (apps/rocmd/src/engine_ipc.rs)	the same run_engine_stdio; rocmd keeps engine_request / engine_request_via and picks its options
decode_engine_response (rocm) and the inline decode (rocmd)	decode_engine_response in stdio_client.rs
the two request-envelope constructions	engine_request_envelope in stdio_client.rs
with_sigpipe_ignored and its re-exec test in apps/rocm/src/main.rs	rocm_core::with_sigpipe_ignored in the new crates/rocm-core/src/sigpipe.rs, test with the same name
the three ROCM_CLI_*_DIR pairs listed in rocm's app_path_env_vars and in rocmd's run_rocm_capture_for_paths	AppPaths::env_vars in rocm-core; both call it. The ROCM_CLI_ENGINE_ENVS_ROOT name is rocm_core::ENGINE_ENVS_ROOT_ENV
byte-identical optional_arg in apps/rocmd/src/common.rs and apps/rocm/src/engine_runtime_select.rs	rocm_engine_protocol::optional_arg, beside gpu_indices_to_csv
the stand-in engine child each caller would otherwise grow	FakeEngine in crates/rocm-engine-protocol/src/fake_engine.rs behind a test-support feature, planted with rocm_core::fake_executable

The shared client lives in rocm-engine-protocol rather than rocm-core because it needs the envelope types and rocm-engine-protocol already depends on rocm-core; cargo xtask check-crate-edges is unchanged and green. The crate gains a dependency on anyhow (already in the workspace) and the test-support feature.

Commits, in order: the first only pins behaviour: it adds FakeEngine, splits each caller's out-of-process half into a function that takes the executable (engine_stdio_child_request, engine_request_via) so a test can aim it at the fake, and adds tests that pass on the unmodified code, including the SIGPIPE case in a re-executed child and the divergences below. The second is the shared client; read its test changes as the diff of what the first commit pinned. The third makes the standalone rocmd run the rocm beside it and is independent of the second. Two review fixes follow: the forwarding writes of the child's stderr are now guarded against SIGPIPE too (and with_sigpipe_ignored is safe to overlap, restoring the disposition in force before the first region when the last one ends, so two regions in different threads cannot restore "ignored" over a default action), and FakeEngine quotes the response and scratch directory it puts in shell literals, so a response containing an apostrophe or a TMPDIR containing one no longer breaks the fake.

Where the two copies differed, the client takes the behaviour that reports more or fails more safely. Every user-visible difference:

• A failing exit status is an error only when the child printed no answer. An answer on stdout is decoded even if the child then exited non-zero. rocm already did this; rocmd threw the answer away for the status. In practice rocm __engine-stdio always exits 0, so this only matters for a real crash.
• A parse failure carries the child's stderr (failed to parse engine response envelope; stderr: ...), and the decode errors use rocm's wording. rocmd used failed to parse engine response, engine response did not include data and failed to deserialize engine response data; those strings appear only in the daemon's own errors, and nothing in the repository matched on them.
• The write of the request is guarded against SIGPIPE, and so are the writes that forward the child's stderr. rocm had the guard on the request write only; rocmd had none. Rust starts a process with SIGPIPE ignored, so the standalone rocmd was never exposed, but rocm daemon runs the daemon inside rocm, which resets SIGPIPE to its default action in main. There, a write to an engine child that had already exited killed the whole daemon before it could say why, and forwarding progress to a closed stderr ended the CLI. Both are now ordinary errors.
• rocmd now sets ROCM_CLI_CONFIG_DIR, ROCM_CLI_DATA_DIR and ROCM_CLI_CACHE_DIR on the engine child from its own AppPaths, as rocm did. Before, it relied on inheriting them. No change in production, where the two agree; it makes the child act on the roots the daemon was given.
• The spawn error names the program in both (failed to spawn engine stdio process <path>); rocm's used to omit the path.
• When writing the request fails, the child is killed and reaped. Before, it was left unreaped, which a long-lived daemon would turn into one zombie per failure.
• The same when reading the child's answer fails (review fix): the read now sits in a small read_response step that kills and waits on the child before it returns the error. The stderr collector is not joined on that path, because a grandchild holding the pipe could block the join; it ends with the pipe. a_failed_read_of_the_answer_kills_and_reaps_the_child (Linux) reads through a failing reader on a real child and checks that its /proc entry is gone.
• Forwarded stderr no longer panics if this process's stderr is closed (rocm used eprint!; the client ignores a failed forward and still collects the text).
• Standalone rocmd: when the running executable is exactly rocmd, engine requests run the rocm beside it (rocm_core::sibling_binary_path), and the error says so if there is none. Embedded in rocm, a test binary or a renamed copy still use their own executable.

What stays an option per caller, deliberately: rocm forwards the child's stderr to the terminal (that is how an install's progress shows) and sets ROCM_ENGINE_PROGRESS_STDERR=1 for an install; rocmd only keeps stderr for the error and asks for no progress. rocmd threads the service's endpoint key file onto the child when it holds a usable key (the healthcheck's HTTP probe needs it); rocm passes none, because none of its requests (detect, install, resolve-model, stop) is about a running service's endpoint, so rocm does not gain the key. The engine environments root is rocm-only. Neither client has, or gains, a timeout; an engine install legitimately runs for minutes, so adding one is a separate decision. The in-process answer for the built-in engines stays in rocm; rocmd cannot link the engines.

Two things found and not changed. In the embedded daemon, restart_managed_service re-launches current_exe() supervise ..., but rocm has no supervise verb (rocm supervise answers unrecognized subcommand), so recovery restarts from the embedded daemon cannot start a supervisor; this is the mirror of the bug fixed here, and outside this change. And rocm's resolve_engine_binary_path looks plugins up through AppPaths::discover() rather than the paths it is given.

Log tails (ROCMAI-879)

Three places read the tail of a log their own way, and the seek-bounded routine in rocm-core that the engine adapters use served none of them: the ComfyUI log view read the whole file, decoded sixteen-bit text and stripped ANSI; the services text module streamed every line and failed on any byte that is not valid UTF-8; and the rocmd MCP service_logs tool read the whole file into one string and failed the same way. The visible symptoms were that one stray byte turned rocm services logs / rocm logs into a single <failed to read ...> placeholder line (and the MCP tool call into an error), and that a multi-gigabyte log was read in full just to show its last few lines.

All three now go through rocm_core::tail_lines_decoded, which takes a LogDecode option: PLAIN (UTF-8, what the engines and the service logs use) or CONSOLE (UTF-16 little-endian detected by a byte order mark or by the zero-byte pattern the old code used, plus ANSI stripping, what the ComfyUI view used). The read is bounded by a byte budget, the window starts on a character boundary (for UTF-16 too), a partial first line is dropped, and invalid text is decoded lossily. The log views share one LOG_VIEW_MAX_TAIL_READ budget of 4 MiB. The routine lives in rocm-core because rocmd must not depend on rocm. The private readers (decode_log_bytes, looks_like_utf16_le, the ComfyUI strip_ansi_sequences, and the stream/whole-file bodies in the other two) are deleted; each of the three callers keeps a thin adapter that picks a LogDecode, passes the shared budget and adds its own error label.

Was	Now
comfyui.rs read_tail_lines body, decode_log_bytes, looks_like_utf16_le, strip_ansi_sequences	rocm_core::tail_lines_decoded with LogDecode::CONSOLE; strip_ansi_sequences is exported from rocm-core (the doc note in tool_policy.rs points there now)
services_text.rs read_tail_lines body (streams with BufRead::lines)	rocm_core::tail_lines (LogDecode::PLAIN); the function stays as a labelled adapter because chat.rs, service_launch.rs and serve_summary.rs call it
rocmd mcp.rs read_tail_lines body (read_to_string)	rocm_core::tail_lines (LogDecode::PLAIN), joined with newlines as before
rocm_core::tail_lines	unchanged signature, now a shorthand for tail_lines_decoded(.., LogDecode::PLAIN); engine callers are untouched

Commits, in order: the first only adds tests that pass on the unmodified code and pin each reader's behaviour (large file, UTF-16 with ANSI, invalid text, missing file, limit 0, fewer lines than the limit, no trailing newline, CRLF); the two readers that read whole files return a line far outside any window there, which is the divergence. The second converges the three callers on the routine, flips the pinned divergences to the new behaviour (the diff of those tests is the clearest list of what changed), adds the routine's own tests and updates docs/architecture.md. The third adds the scenario for a log with an invalid byte. Two review fixes follow. The UTF-8 branch seeked from the size taken by the earlier stat and then read to the current end of the file, so a log whose writer kept appending was consumed past the byte budget; it now caps the read at that snapshot as the sixteen-bit branch already did. A console log ending in a colour reset after its last newline left an unterminated fragment made only of an escape sequence that was counted as a line, so with a limit of one it evicted the real last line; a final unterminated fragment that strips to nothing is no longer counted when ANSI stripping is on, while a terminated blank line still is. The last fix covers a log whose newest line is itself longer than the 4 MiB bound: the window then holds only a fragment, the read returned no lines, and the views said "The saved log is empty." (rocm comfyui logs) or "Recent output: no output yet" (rocm services logs) for a file that is not empty. rocm-core gains tail_log_decoded, which returns the lines in a LogTail that also records how many bytes at the start of the file the budget left unread, and LogTail::newest_line_exceeds_the_read_bound tells an empty tail of a log with content from an empty log; the byte bound stays, and the two views name it, derived from LOG_VIEW_MAX_TAIL_READ so the number cannot drift.

Where the three copies disagreed, the shared routine's choices win, because it is the one that is bounded and the one that never fails a log view. Every difference a user or an MCP client can see:

• A log larger than 4 MiB is read from its last 4 MiB instead of whole (all three). Output differs only when the requested lines do not fit in the last 4 MiB.
• Invalid eight-bit text no longer fails the view. rocm services logs <id>, rocm logs, the chat tool summaries and the managed-start failure detail showed one <failed to read ...> placeholder line, and the MCP service_logs tool returned an error; they now show the readable lines with U+FFFD in place of the bad bytes. ComfyUI logs were already lossy and are unchanged.
• Service logs and the MCP tool still read UTF-8 and keep ANSI sequences; only the ComfyUI view detects UTF-16 and strips ANSI (it is the only log written through a Windows shell redirect). A UTF-16 service log used to be an error and is now shown as lossy text.
• ComfyUI ANSI stripping is per line: an unterminated escape sequence ends at the end of its line. Before, it could consume the following lines up to the next letter.
• Error wording for a missing log: services text says failed to read <label> <path> with the cause failed to open <path> (it was failed to open <label> <path>); ComfyUI and MCP keep their outer failed to read <path> and now carry failed to open <path> as the cause instead of the bare I/O message.
• A limit of 0 returns no lines without opening the file in all three. The MCP reader used to return the whole file for 0 (its window never evicted), and ComfyUI and MCP failed on a missing file before looking at the limit. All callers clamp the limit to at least 1, so only a direct call could see this.
• A log whose only content is a line longer than the 4 MiB bound is no longer reported as empty or as having no output; the view says the newest line does not fit and names the bound. An empty file still says it is empty. The service_logs tool (review fix), in the daemon's MCP server and in the CLI's own tool bridge (rocm mcp-call service_logs), now says so too, through one shared sentence (rocm_core::service_log_tool_summary): "has no log output yet" for an empty log, "has log output, but none is shown: the newest line is longer than the 4 MiB a log view reads" for an over-long newest line, and "Read the last N line(s)" otherwise, where N is the number of lines returned (it was the number asked for, so an empty log read "Read the last 80 line(s)"). Their structured reply gains newest_line_exceeds_read_bound, a missing log is still an error, and the notice clause moved from the CLI into rocm-core so the daemon words it the same way. The launch-failure reader prints no claim about emptiness and is unchanged.

Two judgement calls worth a look: the UTF-16 window is rounded up to an even offset and a byte-order mark is not mistaken for the end of a previous line (both pinned by the every-budget test), and strip_ansi_sequences is now public in rocm-core because a doc note in tool_policy.rs already named it as the tool for assistant prose.

Verification

On the branch after the review fixes and the last merge of main (tip 806c907cd), I ran, in a private target directory:

• cargo fmt --all --check: clean.
• cargo clippy --locked -p rocm-engine-protocol -p rocm-core -p rocmd -p rocm --all-targets -- -D warnings (one invocation per crate) and cargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings: clean.
• The same clippy for x86_64-pc-windows-msvc over those four crates and e2e-cucumber (--lib --bins --tests), with a stub C toolchain (the build scripts of ring and aws-lc-sys need a compile and archive step that produces files, and check and clippy never link them): clean. It compiles the new GetProcessTimes code, the cfg(windows) tests and the Windows scenario's step definitions; it does not execute them.
• Name-filtered tests, not the whole suites: rocm-engine-protocol stdio_client (18 passed), rocm-core proc_lifecycle:: and log_tail:: (63), rocmd mcp:: and service:: (35), rocm services_text::, service_control::, attached_ctrl_c_ and chat:: (185), and a wider set over the respawn guard, launch, restart, supervisor and recovery tests (rocm-core 84, rocmd 138, rocm 293): all passed.
• --list of the unit tests of the four crates before the review fixes and after the last merge: no name disappeared except five that main moved or renamed (four managed_service_status::tests tests now in service_launch::tests, and save_runtime_manifest_replaces_devel_rather_than_merging, now write_runtime_manifest_replaces_devel_rather_than_merging).
• cargo xtask manifest --check, cargo xtask check-crate-edges (rocmd still does not depend on rocm), cargo xtask check-architecture-doc and cargo xtask lychee: clean; feature_naming (4 passed).
• With a debug build with e2e-test-hooks (ROCM_CLI_BINARY, ROCM_CLI_ROCMD_BINARY) and the scenario ids named: 19 scenarios, 78 steps, all passed. They are the rocm logs --follow scenarios of main (logs-04 to logs-07), logs-08 to logs-14, and service-stop-01 to -06, -08 and -09 (service-stop-07 is Windows-only).
• Leak scan of the review-fix commits with the documented pattern: every hit is the word internal used in an identifier (run_internal_mcp_call, stop_internal_managed_service); no ticket tracker, wiki, .corp or .internal host. No conflict markers. Every commit I added carries a signature and a Signed-off-by trailer.

Not run on the combined branch: the full workspace gate (cargo test --workspace --all-targets, workspace clippy), cargo xtask coverage, scripts/smoke_local.py, the full cargo xtask e2e, the GPU lanes, and the tests of the three engine crates (their sources are untouched by the combined diff apart from being callers of the shared tail routine, which has its own tests); CI carries those. The Windows scenario was not run anywhere, see below. The per-change mutation checks and test-count comparisons below are from each change's own branch and were not repeated on the combination.

New tests were each shown to fail when the code they cover is broken (targeted edit, red, restored):

Change	Mutation	Tests that went red
stop	MANAGED_STOP_GRACE 10 s to 5 s	managed_stop_grace_is_ten_seconds
stop	drop the tree_kill(identity.pid) call	the_tree_is_killed_before_the_root_is_terminated
stop	map AlreadyGone to Forced after a tree kill removed	a_root_the_tree_kill_already_took_is_reported_forced_not_already_gone
stop	give non-Windows a tree-kill step	only_windows_has_a_separate_tree_kill_step
stop	skip the stopping process's own PID instead of reporting self	two tests in rocm-core, a_stop_reports_its_own_pid_as_self and a_stop_of_a_record_naming_only_its_own_pid_confirms in rocm, a_stop_reports_its_own_pid_as_self_and_skips_it in rocmd
stop	visit the engine before the supervisor	the order tests in all three crates
stop	re-introduce a private MANAGED_STOP_GRACE in either caller	this_module_carries_no_private_copy_of_the_shared_stop_routine, in rocm and in rocmd
stop	drop the clearing call from rocm's confirmed stop	a_confirmed_stop_clears_the_recorded_processes_so_a_second_stop_signals_nothing, and scenario service-stop-06
stop	drop the clearing call from rocmd's confirmed stop	stop_managed_service_clears_recorded_pids_once_the_processes_are_gone
stop	leave engine_start_ticks out of the clearing	the rocm-core test and the rocm test above
respawn guard	drop the public-bind rule from the shared guard	the rocm-core guard tests, three rocm caller tests, the two service_control restart tests, three rocmd tests, and scenario service-stop-08 at its refusal step
respawn guard	drop the --require-api-key rule	the analogous set, including a_managed_spawn_refuses_an_invalid_key_file_on_a_service_that_requires_one and the_key_file_rocm_serve_stores_clears_the_recovery_refusal
respawn guard	a private copy of rule 1 in the rocmd supervisor; the wrong ServiceStart at the rocm spawn, the rocm restart or the server-recover site	exactly the test for that call site (each compares the caller's refusal to the guard's text for its own start)
engine client	remove the SIGPIPE guard from the shared client	three tests (its own, rocm's and rocmd's, each a re-executed process killed by SIGPIPE)
engine client	drop the endpoint key from rocmd's options, flip rocm's forward_stderr, drop the environments-root variable	the tests for that option
engine client	replace the call in rocmd with a private copy of its old client	five rocmd tests
engine client	make rocm_executable_from ignore the standalone case	its unit test, and the engine_stop step added to service-stop-02 (it fails against the shared client without the standalone fix with the unrecognized subcommand '__engine-stdio' error)
log tails	make the shared routine read the whole file	6 core tests and 4 caller tests (large-file bound for the routine, services text, both ComfyUI cases and MCP)
log tails	remove the UTF-16 alignment or the byte-order-mark exception	the every-budget UTF-16 test
log tails	disable ANSI stripping	the ComfyUI and core console-decode tests
log tails	swap the ComfyUI decode to PLAIN	four ComfyUI tests
log tails	put the old private copies back into the services text and MCP adapters	5 services text and 5 MCP tests, and the invalid-byte scenario
engine client (review)	remove the kill, or the wait, from the failed-read path	a_failed_read_of_the_answer_kills_and_reaps_the_child (without the kill it also takes the 60 s of the child's own sleep to fail)
log tails (review)	test the empty case before the over-long case in service_log_tool_summary	the rocm-core summary test, the rocmd service_logs test and scenario logs-13
stop (review)	swap the two halves of the FILETIME	a_filetime_is_its_high_word_above_its_low_word
stop (review)	run the tree kill for every PID that is not gone, not only a matching one	no_tree_kill_is_run_for_a_pid_that_is_gone_recycled_or_unverifiable
stop (review)	let a recycled PID through the verified termination	scenario service-stop-09, at its bystander step
stop (review)	swap the Windows and Linux forms of the check command	the_process_check_command_is_the_one_the_host_has
respawn guard (merge with main)	drop --port <port> from the public-bind text	the_command_a_respawn_refusal_names_clears_the_refusal, which now also applies rocm serve's port rule
stop (Windows lane)	refuse a record with no token; let a differing token through; let an unreadable token through; refuse a matching token	what_a_stop_does_for_each_recorded_and_live_token and a_recorded_token_refutes_a_live_one_only_when_both_are_known_and_differ, one row each
SIGPIPE guard (review)	per-call save and restore instead of the shared counter	overlapping_guards_restore_the_original_disposition_once, five of five runs, now that the interleaving is fixed
scenario teardown (review)	end a gone or reused process, or leave the matching one	only_a_process_that_is_still_the_recorded_one_is_ended

The stop routine is tested in rocm-core (proc_lifecycle::tests): visiting order and per-PID start-time pairing, an aliased PID, zero PIDs, the stopping process's own PID, and the signalled/forced/stopped classification of every TerminationOutcome. The Windows step runs through terminate_recorded_pid_with: the tree kill comes before the termination, runs only for an identity that matches (not for a gone, recycled or indeterminate PID), maps AlreadyGone to Forced, and passes every other verdict through; only_windows_has_a_separate_tree_kill_step ties that injected path to what production uses. Each caller's own tests drive its stop through the shared routine and assert the report. The source checks are the "a test fails if a copy re-diverges" guard for the constant and the routine; they read their own source file and assemble their needles so they do not match themselves. The log-tail bound itself is shown by a counting reader: a read of an 8 MiB source under a 64 KiB budget touches at most the budget plus one probe byte (plus the 512-byte sniff for UTF-16).

User-visible behaviour

Scenarios that cover this change, all with no GPU and no network:

• @id:service-stop-yes-stops-a-running-service (service-stop-01), @id:service-stop-confirms-a-stop-and-cleans-up (service-stop-02, Linux, drives the standalone rocmd sandbox-tool stop_server and now also asserts that the engine Stop request was answered rather than turned into the engine_stop error), and service-stop-03 to -05 (Linux, the ROCM_E2E_STOP_OUTCOME seam) existed already; they pass and are unaffected apart from what is said here. They run on the Linux mock lane (blocking, GitHub-hosted).
• New: @id:service-stop-cli-confirms-a-stop-and-clears-the-record (service-stop-06, Linux, mock lane) runs rocm services stop --yes against a recorded process the scenario starts and checks the persisted state with the report: the process exited, the record reads stopped and names no process, the key file is gone. With the clearing call removed from the CLI's stop it fails on the record step.
• New: @id:service-stop-takes-down-the-whole-process-tree-on-windows (service-stop-07, @requires-os:windows) records a PowerShell parent that has started a ping child, with the PID and no start-time token as Windows records it, runs rocm services stop --yes, and asserts that both processes are gone together with the report (Local server stopped, stopped processes: 1) and the record (stopped, no process, key file gone). It fails if the taskkill /T step is removed, because the stop would then end only the recorded process and the child would survive. The Windows scenario was written on Linux and has never been run on Windows. It compiles (the Windows-target clippy over the e2e harness covers its step definitions), the suite's host filter skips it on Linux, and the lane that will run it is the full-suite self-hosted Windows lane e2e-gpu-strix-windows (non-blocking). The tree-kill step is also covered by unit tests that drive it through injected functions on every host, which the required windows-debug lane runs, and by the Windows clippy gate compiling the cfg(windows) arm. service-stop-01 has no @requires-os tag and runs on that Windows lane too, but it plants PID 0 and never reaches the tree kill. I added no scenario for the self row in difference 2 of the stop section: it is observable only when the recorded PID is the calling process, which no CLI invocation a user types produces, so it is pinned at the caller level instead.
• New: @id:service-stop-restart-refuses-a-public-server-without-its-key (service-stop-08, Linux, mock lane) plants a publicly bound server with no key file and a live recorded process, runs rocm services restart --yes, and asserts the exit status, the refusal text, that the named rocm serve <model> ... command is accepted by the real parser (model filled in, --help last so the parse ends in help only if every earlier word was accepted, with a bogus flag as the negative control), and that the recorded process is still running with no stop recorded and no key created. With the public-bind rule dropped it fails at its refusal step. Nothing in the feature files printed these messages before; serve-24 and serve-30 exercise the separate reuse refusal, which is unchanged, and serve-24 is @requires-gpu @requires-engine:lemonade so it does not run on the mock lane. The daemon's respawn has no scenario because it needs the daemon's recovery tick over a failed service; each of its call sites is covered by a unit test that drives the real call site.
• New (review fix): @id:logs-service-logs-tool-longer-than-the-read-bound-is-not-called-empty (logs-13) and @id:logs-service-logs-tool-empty-says-no-output-yet (logs-14), mock lane: rocm mcp-call service_logs against a server log holding one line longer than the bound and against an empty log. Each checks the reply (text, empty tail, newest_line_exceeds_read_bound) against the file on disk that it describes. With the empty-log check placed before the over-long one, logs-13 fails with "has no log output yet".
• New (review fix): @id:service-stop-leaves-a-process-that-reused-the-recorded-pid-alone (service-stop-09, every OS, mock lane): records a running bystander process under a start-time token no live process has (the real one plus one on Linux, 1 on Windows), runs rocm services stop --yes, and asserts the stop is reported, the bystander is still running, the record names no process and the key file is gone. With a recycled PID treated as matching, it fails at the bystander step. The Windows half has not run.
• New: @id:logs-invalid-byte-keeps-matching-lines (logs-08, mock lane): an action log with one invalid byte still yields all nine matching lines; it fails with the old services text reader and passes now. logs-09 to logs-12 (one pair per view, mock lane) assert each message together with the log on disk that it describes: for a line longer than the bound, the size and the absence of a newline; for an empty file, zero bytes. logs-01 (logs-search-reports-match-count) and logs-02 (logs-search-absent-term-no-matches) read the CLI action logs through the changed path and pass unchanged; logs-03 does not reach the reader. There is no scenario for rocm comfyui logs beyond logs-11 and logs-12 (its output is otherwise unchanged apart from the 4 MiB window and the per-line ANSI edge, covered by unit tests through the adapter) The service_logs tool is covered by logs-13 and logs-14 below for the CLI's copy and by a rocmd unit test for the daemon's copy (the harness drives only the rocm binary).
• The engine client change has no scenario of its own beyond the engine_stop assertion in service-stop-02, which is its user-visible effect. Install, serve and engines scenarios go through the same rocm code and are unchanged apart from the spawn error now naming the program; the ones that start an engine child run on the GPU or lifecycle lanes.

Both service-stop-06 and the endpoint-key restart scenario were added with the number 06 on their own lines of work; on this branch they are numbered 06 (stop) and 08 (restart), the Windows stop keeps 07, the review fix adds 09, and feature_naming passes. main's rocm logs --follow scenarios hold logs-04 to logs-07, so this branch's log scenarios are logs-08 to logs-14. The documented command to run the Linux stop scenarios on a Linux host is -n "service-stop-0[1-689]"; a name filter bypasses the suite's host filter, so -n "service-stop" would also run the Windows-only scenario and fail for want of PowerShell.

Notes for reviewers

Read each section's commits in order as described above; the history is the four lines of work, the merge commits that join them, and then the review fixes, so the diff against main is large but each consolidation can be reviewed alone. The review fixes (after the first review round) are four commits plus the merges of main they needed: 5d9a5704e (the engine child is killed and reaped when its answer cannot be read), 439899851 and its lint follow-up 8ff2ff9bf (the service_logs tool tells an empty log from an over-long newest line), 5f9baadd8 (a Windows start-time token, so a recycled PID is refused before the tree kill, and the check command that outcome names), then the merges 852acf359, 106af9224, 2f907097c and 9d166667c, and after a Windows lane run ef877b78b (the stop's identity verdict decided in one place, with a table over no token, a matching token, a differing token and an unreadable one), 2d1960939 (the overlapping SIGPIPE guard test now enters its regions in a fixed order) and 806c907cd (scenario teardown ends the child a stop scenario recorded). 106af9224 adapts this branch to main's port-lease work: main now requires an explicit --port with any host but the default loopback, so the public-bind remediation names --port <port>, and the test that runs the named command through rocm serve's own policy applies that port rule too (it fails with the port left out). docs/architecture.md and docs/testing.md are the files most likely to conflict with other open work, and crates/rocm-core/src/lib.rs is touched by all four. The merged result has no conflict with main as of the last fetch (git merge-tree exits 0). After the last merge I ran the full cargo test -p rocm --bin rocm (1714 passed, 9 ignored), cargo test -p rocmd (220 passed, 2 ignored) and cargo test -p rocm-engine-protocol (44 passed); the Windows clippy and the e2e scenarios listed under Verification were last run on the previous merge tip 2f907097c.

One note on files touched by more than one section: apps/rocmd/src/engine_ipc.rs lost its own copy of the respawn guard in the guard section and gained the shared-client call in the engine-client section, and the module doc entry in docs/architecture.md describes both. The rocmd module's key-file helper usable_endpoint_key_file stays there; only the guard moved.

[Copilot]

🟡 Changes recommended

Confirmed CLI stops retain stale process identities, allowing later Windows tree kills to terminate unrelated processes after PID reuse, while the tree-kill behavior lacks end-to-end Windows coverage.

2 open issues

• Clear process identities after a confirmed Windows stop · New
• Add Windows integration coverage for process-tree stopping · New

What changed in this PR

Consolidates managed-service process termination in rocm-core so rocm and rocmd share one routine and the same 10-second stop grace period.

Changes:

• Replaces duplicated termination logic with shared PID outcomes and reporting helpers.
• Aligns CLI stops with daemon behavior, including Windows descendant termination and explicit self PID reporting.
• Adds regression tests and documents the shared architecture and Windows coverage limitations.

File	Description
docs/​testing.md	Documents shared-stop tests and Windows coverage gaps.
docs/​architecture.md	Updates module ownership and shared termination responsibilities.
crates/​rocm-core/​src/​proc_lifecycle.rs	Adds shared termination routines, outcome types, grace constant, and tests.
crates/​rocm-core/​src/​lib.rs	Exports the shared managed-stop API.
apps/​rocmd/​src/​service.rs	Replaces private termination logic with shared calls and adds caller tests.
apps/​rocm/​src/​service_control.rs	Routes stop/restart cleanup through the shared routine and updates tests.

_{Review effort: Balanced}

[jussielo-amd]

Added in f8a44e2, with one caveat: it has not been run on Windows, because I have no Windows host here. service-stop-07 (@requires-os:windows, no GPU) records a PowerShell parent that has started a ping child, with the PID alone and no start-time token as Windows records it, runs rocm services stop --yes, and asserts that both processes are gone together with the stop report (Local server stopped, stopped processes: 1) and the persisted record (stopped, no process, no stop request, key file gone). Its Given checks both processes are running first, so the Then cannot pass for the wrong reason. Without the taskkill /T step the stop ends only the recorded process and the scenario fails on the surviving child. It compiles on the Windows target (clippy for the e2e harness's test target), the scenario ids are sequential on this branch and pass the feature naming checks, and the suite's host filter skips it on Linux (verified by a run that named all the stop scenarios by id: six ran, it was skipped). I also added service-stop-06, the Linux counterpart that runs rocm services stop --yes against a real recorded process and checks the persisted state, since the CLI's own stop of a real process had no scenario either. Note the number collision: the endpoint-key respawn guard PR adds its own service-stop-06, so whichever lands second renumbers.

[Copilot]

🔵 Needs a closer look

The untested Windows subtree-termination path and documented recycled-PID risk from unavailable creation-time identity checks require live Windows validation and maintainer judgment.

1 open issue

• Add Windows integration coverage for process-tree stopping

1 resolved since last review

• Clear process identities after a confirmed Windows stop

_{Review effort: Balanced}

[juhovainio]

Thanks for the thorough consolidation work; the shared stop routine, single IPC client, respawn guard and log-tail routine all read well, CI is green, and the added scenario coverage (service-stop-06..08, logs-04..08) is solid.

Requesting changes mainly on scope, plus three smaller items inline:

1. Scope: four independent changes in one PR. The stop routine, the engine IPC client, the endpoint-key respawn guard and the log-tail routine are described in the PR body as independent, and each tracking ticket asks for its key to be delivered in its own PR. Bundling them (with three merge commits) makes ~8.5k lines hard to review and to revert selectively. Please split into four PRs, or get agreement from the ticket owner to bundle.
2. Child not reaped on stdout read failure (inline, stdio_client.rs).
3. MCP service_logs still returns an ambiguous empty tail (inline, mcp.rs): same class as the CLI log views fixed here.
4. Windows taskkill /T /F blast radius (inline, proc_lifecycle.rs): needs an explicit decision on recycled-PID protection.

Also worth confirming each of these user-visible changes is intended: rocmd now decodes the answer on a non-zero engine exit, the rocmd respawn wording is now rocm's, and a confirmed rocm services stop clears the recorded pids.

[juhovainio]

If reading stdout fails, ? returns before child.wait() and leaves the stderr thread detached, so the child is neither reaped nor killed (the write-failure branch above does kill+wait). Consider killing and waiting here too, since this client now runs in the long-lived daemon.

[jussielo-amd]

You are right: the ? on the stdout read returned before child.wait(), so a failed read left the child running and never reaped, in a client that now lives in the daemon. Fixed in 5d9a570: the read moved into a small read_response step that kills and waits on the child before returning the error, like the write-failure branch above it. I did not join the stderr collector on that path, because a grandchild holding the pipe could block the join; the thread ends by itself when the pipe closes. The new test reads through a failing reader on a real child and checks that its /proc entry is gone, and I confirmed it fails with the kill removed and with the wait removed.

[juhovainio]

tail_lines returns no lines both for an empty log and for a log whose newest line exceeds LOG_VIEW_MAX_TAIL_READ; the caller then reports "Read the last N line(s)" with an empty tail. The CLI views got tail_log_decoded + newest_line_exceeds_the_read_bound for exactly this; the MCP tool is the same class. Use the same signal here or note the gap in the PR.

[jussielo-amd]

Agreed, same class as the CLI log views. Fixed in 4398998 (plus a lint-only follow-up, 8ff2ff9): service_logs now reads with tail_log_decoded and answers through one shared sentence, rocm_core::service_log_tool_summary, which says "has no log output yet" for an empty log, names the 4 MiB bound when the newest line exceeds it, and otherwise reports how many lines were read. The structured reply also carries newest_line_exceeds_read_bound. The CLI's own copy of the tool (rocm mcp-call service_logs) had the same problem and uses the same sentence, so scenarios logs-13 and logs-14 cover it against the file on disk, and a rocmd unit test covers the daemon's copy because the harness only drives the rocm binary. A missing log is still an error rather than an empty tail.

[juhovainio]

This puts taskkill /T /F behind every rocm stop path on Windows while process_start_ticks is still None there, so a recycled PID passes the Matches gate (documented above). Clearing the record on a confirmed stop removes the repeat-stop route but not the "service died by itself, PID recycled, record still names it" route. Worth an explicit decision: land a Windows creation-time identity first, or accept and track.

[jussielo-amd]

You are right that a recycled PID passed the gate. I closed it instead of tracking it, in 5f9baad: on Windows process_start_ticks is now the process creation time from GetProcessTimes, so a record carrying a token refutes a recycled PID the same way as on Linux, and the stop reports identity_mismatch without running taskkill on it. A record written before this has no token and stays best-effort, and a token that can no longer be read is reported as unverified rather than signalled; that outcome is now reachable on Windows, so the check command it prints is tasklist /FI "PID eq <pid>" there instead of ps. Scenario service-stop-09 stops a server whose recorded PID belongs to a running bystander and checks the bystander is still running (it passed on Linux and fails when a recycled PID is let through), and there are unit tests for the FILETIME join and for the refusal. I could not run Windows code here, so the Windows-only tests and the Windows half of service-stop-09 are for the Windows jobs; the Windows clippy check of the new FFI is clean.

[jussielo-amd]

On scope: I bundled the four changes deliberately. They share one routine per concern in rocm-core and touch the same files (docs/architecture.md, docs/testing.md, the service_stop feature file and apps/rocmd/src/engine_ipc.rs), so separate PRs would conflict with each other on every merge, and each section keeps its own commits in the order the body lists them. I agree it is a lot to review at once and I am happy to split along those commit ranges into four PRs if you prefer; the merge commits only join the lines and would disappear. Tell me which you want and I will do it.

On the three inline items: the unreaped child on a stdout read failure is fixed in 5d9a570, the ambiguous MCP service_logs tail in 4398998, and the Windows recycled-PID question is closed in 5f9baad with a creation-time token rather than tracked. Each has its own test and a note on its thread.

On the user-visible changes you asked about: each is intended and listed in the body. rocmd decodes an answer printed before a non-zero exit because rocm already did and the answer says more than the status; in practice rocm __engine-stdio always exits 0, so it only matters for a real crash. The rocmd respawn refusal uses rocm's wording because it says why the key cannot be recovered and names the rule that fired, and the command it names is checked against the real parser. A confirmed rocm services stop clears the recorded pids as rocmd's stop already did, so a repeated stop cannot reach a stale pid; with the new Windows token, a stale pid recorded with one is also refused rather than killed.

I also merged main three times while doing this (the conflicts were in docs/architecture.md, logs.feature, the service launch and control modules, and the engine runtime selection doc). Two consequences are worth a glance: main's rocm logs --follow scenarios keep logs-04 to logs-07, so this branch's log scenarios are now logs-08 to logs-14, and since main now requires an explicit --port with any non-default host, the public-bind remediation the shared guard prints names --port <port> and its test now applies that port rule.

[Copilot]

🟡 Changes recommended

The SIGPIPE regression test can miss the ordering bug, and the service-stop scenario lacks independent descendant cleanup after failure.

3 open issues

• Make the regression test enforce the failing thread interleaving · New
• Terminate the ping descendant independently during teardown · New
• Add Windows integration coverage for process-tree stopping

_{Review effort: Balanced}

[jussielo-amd]

You are right: a stop that killed PowerShell but left ping would have left it running for its five minutes, because teardown only killed the recorded parent. Fixed in 806c907: the Given step keeps the child's identity (PID plus creation time where the host reads one) on the world, and teardown, which runs when a step fails too, ends it when it is still that process. Whether to end it is a pure function, stray_process::leftover_action, with a table test on every host (ended only while it is still the recorded process, never once gone, reused or unconfirmed), and the Windows clippy check compiles the teardown; the scenario itself has not run on Windows here.

[Copilot]

🔵 Needs a closer look

The regression test lacks failure-safe cleanup for the descendant process, so a failed assertion can leave ping running on the Windows runner for the remainder of its five-minute lifetime.

3 open issues

• Terminate the ping descendant independently during teardown
• Make the regression test enforce the failing thread interleaving
• Add Windows integration coverage for process-tree stopping

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

Human assessment is needed to weigh an unpublished, nit-level finding about outdated Windows PID-verification descriptions.

2 open issues

• Terminate the ping descendant independently during teardown
• Add Windows integration coverage for process-tree stopping

1 resolved since last review

• Make the regression test enforce the failing thread interleaving

_{Review effort: Balanced}