Skip to content

fix(services): check a restarted engine for a startup death on Windows too - #859

Draft
rominf wants to merge 2 commits into
fix/windows-managed-spawn-livenessfrom
feat/restart-startup-death-check
Draft

rominf wants to merge 2 commits into
fix/windows-managed-spawn-livenessfrom
feat/restart-startup-death-check

Conversation

@rominf

@rominf rominf commented Nov 3, 2026

Copy link
Copy Markdown
Collaborator

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's restart_server tool, 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 recorded running and the restart reported success. On Unix the child was checked, but put on record only after the check, so a failed try_wait left 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_process and settles it with watch_managed_engine_startup, writing through its own RestartStartupRecord:

  • The new child is recorded starting before the check, and only the readiness verdict promotes it. 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. The endpoint key is kept, 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.
  • Every write is guarded on the record still reading as this restart last left it (update_restarting_record over update_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

  • Seven unit tests in services_text.rs drive restart_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, the starting record during the readiness wait, and the runtime pin through the production wrapper. One test reconciles a recorded child once it exits.
  • Shown red by turning RestartStartupRecord::retire into a no-op: three tests fail.
  • No e2e scenario in this PR: the end-to-end restart scenario is test(serve): cover the restart path's startup death end to end (serve-25) (ROCMAI-676) #581, which will be restacked on this branch.
  • Not verified locally: the Windows build (Linux host). The Windows E2E lane exercises the watched spawn.

…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant