Repository navigation
Conversation
…s too `rocm services restart` spawned its engine with its own copy of the platform code. On Windows that meant a bare pid, a blind 200 ms sleep and no liveness check, so a restarted engine that died at startup was recorded as running. On Unix the restart checked the child, but put it on record only after the check. The restart now starts its engine through the launch's spawn seam (spawn_managed_engine_process) and settles it with the same startup check (watch_managed_engine_startup), writing through its own RestartStartupRecord: - The new child is recorded as "starting" before the check, and is promoted only by the readiness verdict. A "running" record whose port is not open yet would be restarted by rocmd's server-recover watcher. - A child seen dead at startup fails the restart, naming the engine's log, and retires the record as "failed", keeping the endpoint key as for a crash. A child the Windows spawn saw die is never put on record. - A Unix startup query that fails leaves the child on record as "starting" rather than retiring it. - Every write is guarded on the record still reading as this restart last left it (update_restarting_record over update_managed_record_on_disk). A stop asked for meanwhile keeps its marker. The spawn and the readiness wait are injectable, so the unit tests drive the production restart, including the runtime pin it forwards, without starting an engine. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
The reconciliation test in services_text.rs that uses pre_spawn_record is #[cfg(not(windows))]. The Windows build therefore saw the import as unused, and Clippy (Windows targets) failed with unused-imports. Gate the import the same way as the one test that uses it. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
This was referenced Nov 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Depends on #437 (stacked on it). Draft until #437 merges; then rebased onto main. Split out of #437 at review request.
Symptom
rocm services restart(and the assistant'srestart_servertool, which calls the same function) started its engine with its own copy of the platform spawn code. On Windows that meant a bare pid, a blind 200 ms sleep and no liveness check, so a restarted engine that died at startup was recordedrunningand the restart reported success. On Unix the child was checked, but put on record only after the check, so a failedtry_waitleft a record naming the previous run's pids.Fix
The restart now goes through the launch's spawn seam from #437. It starts the engine with
spawn_managed_engine_processand settles it withwatch_managed_engine_startup, writing through its ownRestartStartupRecord:startingbefore the check, and only the readiness verdict promotes it. Arunningrecord whose port is not open yet would be restarted by rocmd's server-recover watcher.failed. The endpoint key is kept, as for a crash. A child the Windows spawn saw die is never put on record.starting.update_restarting_recordoverupdate_managed_record_on_disk), so a stop asked for meanwhile keeps its marker. The read and the write are not atomic: this narrows the race, it does not close it.User-visible change: on Windows, a restart whose engine exits within 200 ms now fails instead of reporting the service restarted. An engine that dies later, while loading its runtime or model, is still reported as starting.
Tests
services_text.rsdriverestart_internal_managed_service(_with)with injected spawn and readiness stand-ins: death at startup, a child seen dead by the spawn, spawn failure, a check that cannot answer, a stop asked for during the check, thestartingrecord during the readiness wait, and the runtime pin through the production wrapper. One test reconciles a recorded child once it exits.RestartStartupRecord::retireinto a no-op: three tests fail.