fix(host-service): reap orphaned host-service before spawn (single-writer)

Closed
KI
4mo ago
+335-13

Problem

The coordinator only tracks host-services it spawned in this Electron process (its in-memory instances map). After a restart that map is empty, so a host-service left alive by a previous process is invisible — and spawn() never consults the manifest before starting a new one. It just binds a competitor on a fresh port, giving two processes connected to the same org host.db. That concurrent-writer contention is the root trigger behind the migration lock-up (see #4996, which hardens the migration side).

How an orphan arises:

  • Electron died before before-quit ran (hard crash / SIGKILL / OOM) — child never SIGTERMed, parent watchdog hadn't fired yet;
  • a CLI-spawned host-service — no HOST_PARENT_PID, so no watchdog at all;
  • a wedged child from a failed spawn that ignored SIGTERM.

(reset() already reaps via the manifest, but it's only reachable manually via tRPC — never on the normal start path.)

Fix

Reap based on the manifest (the only cross-process record) before spawning, in spawn():

  • Identity-validated kill (PID-reuse safe): only terminate a pid we can positively confirm is our host-service — its endpoint answers health.check with the manifest secret, or (best-effort, POSIX) its command line matches our host-service.js. A live-but-unrecognized pid is left alone and the stale manifest dropped, so a recycled pid can't cause friendly-fire.
  • Escalation: SIGTERM → await exit → SIGKILL → await.

Why safe to kill: host-service is stateless w.r.t. terminals — PTYs live in the detached pty-daemon, which a fresh host-service re-adopts. Reaping an orphan doesn't touch user sessions.

Structure

  • host-service-reap.ts — dependency-injected reap logic (electron-free, so it unit-tests under bun test without spawning real processes).
  • host-service-coordinator.ts — wires real deps + the health/argv identity probe; calls reap at the top of spawn().
  • host-service-utils.ts — adds healthCheckOnce (single-shot identity probe; pollHealthCheck now reuses it).
  • host-service-reap.test.ts — 6 cases: no-manifest, own-live-instance, dead-pid-cleared, unidentified-left-alone (PID reuse), SIGTERM-exit, SIGTERM→SIGKILL escalation.

Desktop typecheck + lint clean; reap tests 6/6.

Relationship to #4996

Independent and complementary. #4996 makes the DB migration survive contention (busy_timeout + fail-closed); this PR removes the source of the contention. Either can merge first.

Open in Stage

Summary by cubic

Prevents two host-service processes from writing to the same org DB by reaping any orphaned host-service before spawn. Uses manifest-driven identity checks to safely terminate previous instances and ensure a single writer, resolving migration contention.

  • Bug Fixes
    • Reap any previous host-service for the org at the start of spawn() using the manifest.
    • Identity-validated kill: health.check with the manifest secret, with a POSIX argv fallback; unrecognized live PIDs are left alone and the stale manifest is removed.
    • Escalation flow: SIGTERM with grace, then SIGKILL if needed; clears the manifest after successful reaping or when the PID is dead.
    • Added healthCheckOnce and updated pollHealthCheck to reuse it.
    • Introduced dependency-injected host-service-reap.ts with unit tests covering no-manifest, own-live, dead-pid, PID-reuse, and SIGTERM→SIGKILL paths.

Summary by CodeRabbit

  • Improvements

    • Enhanced host-service process lifecycle management with proactive cleanup of previous instances to prevent conflicts.

  • Tests

    • Added comprehensive test coverage for host-service process cleanup logic, including edge cases for process termination and manifest handling.

Review Change Stack