fix(cli): clear respawn interval and guard shutdown re-entrancy (#181)

The dev-server shutdown path had two teardown bugs: 1. The crash-respawn setInterval was never cleared. During shutdown's async window (await pm.shutdown(); await server.close()), the 500ms interval kept firing; if the child had crashed it could call pm.spawn() and start a BRAND-NEW child while the parent was tearing down — orphaning that child when the parent exits. 2. shutdown was registered directly as the SIGINT/SIGTERM handler with no re-entrancy guard. Pressing Ctrl+C twice (common when shutdown feels slow) invoked shutdown() twice concurrently → double pm.shutdown()/server.close() and two racing process.exit(0). Fix: extract a testable createShutdown(deps) factory that owns a shuttingDown flag (2nd+ invocation is a no-op) and clears the respawn interval FIRST, before the async teardown window. The respawn-tick body is extracted into respawnTick(deps), which re-checks isShuttingDown both before and AFTER the extractBundle await — closing the in-flight window where a tick already mid-extraction could still spawn after clearInterval. Both helpers are module-scoped (not re-exported from the package public index.ts). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-06-14 22:13:54 +08:00
parent 85088f7863
commit 8b10770dc8
2 changed files with 264 additions and 19 deletions
@@ -0,0 +1,159 @@
+import { expect, test, vi } from "vite-plus/test";
+import { createShutdown, respawnTick } from "./dev.ts";
+import type { MemoryFiles } from "./bundle-extractor.ts";
+
+// A controllable deferred promise so two concurrent shutdown invocations can be
+// observed overlapping BEFORE the first one resolves — this is what proves the
+// re-entrancy guard handles true concurrency, not just sequential repeat calls.
+function deferred<T>() {
+  let resolve!: (value: T) => void;
+  const promise = new Promise<T>((r) => {
+    resolve = r;
+  });
+  return { promise, resolve };
+}
+
+function makeDeps() {
+  const gate = deferred<void>();
+  const pm = { shutdown: vi.fn<() => Promise<void>>(() => gate.promise) };
+  const closeServer = vi.fn<() => Promise<void>>(async () => {});
+  const clearRespawn = vi.fn<() => void>();
+  const exit = vi.fn<(code: number) => void>();
+  return {
+    deps: { pm, closeServer, clearRespawn, exit },
+    pm,
+    closeServer,
+    clearRespawn,
+    exit,
+    gate,
+  };
+}
+
+test("shutdown clears the respawn interval BEFORE the async teardown window", async () => {
+  // WHY: the crash-respawn setInterval is otherwise never cleared. It must be
+  // cleared synchronously, BEFORE awaiting pm.shutdown(), or an in-flight respawn
+  // tick during that async window can call pm.spawn() and orphan a brand-new
+  // child. pm.shutdown is gated on a deferred promise, so the handler is
+  // suspended at `await pm.shutdown()` until we resolve it — letting us assert
+  // ordering while teardown is mid-flight.
+  const { deps, pm, closeServer, clearRespawn, exit, gate } = makeDeps();
+  const { handler } = createShutdown(deps);
+
+  const p = handler();
+  // Let the synchronous prologue + the microtask up to `await pm.shutdown()` run.
+  await Promise.resolve();
+
+  // clearRespawn must already have run, and we must be suspended inside
+  // pm.shutdown() — but NOT yet have closed the server or exited. This pins the
+  // order: clearRespawn → pm.shutdown → (gate) → closeServer → exit.
+  expect(clearRespawn).toHaveBeenCalledTimes(1);
+  expect(pm.shutdown).toHaveBeenCalledTimes(1);
+  expect(closeServer).not.toHaveBeenCalled();
+  expect(exit).not.toHaveBeenCalled();
+
+  gate.resolve();
+  await p;
+
+  expect(closeServer).toHaveBeenCalledTimes(1);
+  expect(exit).toHaveBeenCalledWith(0);
+});
+
+test("concurrent shutdown invocations run teardown exactly once (Ctrl+C twice)", async () => {
+  // WHY: shutdown was registered directly as the SIGINT/SIGTERM handler with no
+  // re-entrancy guard. Pressing Ctrl+C twice (common when shutdown feels slow)
+  // invokes shutdown() twice concurrently → double pm.shutdown()/server.close()
+  // and two racing process.exit(0). The deferred gate keeps both invocations
+  // overlapping before the first resolves, so this is concurrent re-entrancy.
+  const { deps, pm, closeServer, clearRespawn, exit, gate } = makeDeps();
+  const { handler } = createShutdown(deps);
+
+  const both = Promise.all([handler(), handler()]);
+  // Both handlers are now past their synchronous prologue and awaiting the gate.
+  gate.resolve();
+  await both;
+
+  expect(pm.shutdown).toHaveBeenCalledTimes(1);
+  expect(closeServer).toHaveBeenCalledTimes(1);
+  expect(exit).toHaveBeenCalledTimes(1);
+  expect(clearRespawn).toHaveBeenCalledTimes(1);
+});
+
+const memoryFiles: MemoryFiles = {
+  files: new Map(),
+  get: () => undefined,
+};
+
+test("respawn tick does not spawn a child when shutdown has begun", async () => {
+  // WHY: clearInterval stops new ticks, but a tick already past its synchronous
+  // guard and awaiting extractBundle could still reach pm.spawn() AFTER
+  // pm.shutdown() finished, orphaning a brand-new child as the parent exits. The
+  // tick re-checks isShuttingDown after the await and must bail.
+  const pm = {
+    running: false,
+    setBundlePath: vi.fn<(p: string) => void>(),
+    spawn: vi.fn<() => void>(),
+  };
+  await respawnTick({
+    crashed: () => true,
+    setCrashed: vi.fn(),
+    pm,
+    memoryFiles,
+    outDir: "/tmp/out",
+    isShuttingDown: () => true,
+    extractBundle: async () => "/tmp/out/entry.js",
+  });
+
+  expect(pm.spawn).not.toHaveBeenCalled();
+  expect(pm.setBundlePath).not.toHaveBeenCalled();
+});
+
+test("respawn tick does not spawn when shutdown begins DURING bundle extraction", async () => {
+  // The realistic race: the tick passes its first isShuttingDown() check, then
+  // shutdown starts while extractBundle is awaiting. The post-await re-check is
+  // what prevents the orphan here.
+  let shuttingDown = false;
+  const pm = {
+    running: false,
+    setBundlePath: vi.fn<(p: string) => void>(),
+    spawn: vi.fn<() => void>(),
+  };
+  await respawnTick({
+    crashed: () => true,
+    setCrashed: vi.fn(),
+    pm,
+    memoryFiles,
+    outDir: "/tmp/out",
+    isShuttingDown: () => shuttingDown,
+    extractBundle: async () => {
+      // Shutdown is triggered mid-extraction.
+      shuttingDown = true;
+      return "/tmp/out/entry.js";
+    },
+  });
+
+  expect(pm.spawn).not.toHaveBeenCalled();
+  expect(pm.setBundlePath).not.toHaveBeenCalled();
+});
+
+test("respawn tick spawns a fresh child on a good build after a crash", async () => {
+  // Positive path: not shutting down, crashed, build available → respawn.
+  const setCrashed = vi.fn<(value: boolean) => void>();
+  const pm = {
+    running: false,
+    setBundlePath: vi.fn<(p: string) => void>(),
+    spawn: vi.fn<() => void>(),
+  };
+  await respawnTick({
+    crashed: () => true,
+    setCrashed,
+    pm,
+    memoryFiles,
+    outDir: "/tmp/out",
+    isShuttingDown: () => false,
+    extractBundle: async () => "/tmp/out/entry.js",
+  });
+
+  expect(pm.setBundlePath).toHaveBeenCalledWith("/tmp/out/entry.js");
+  expect(setCrashed).toHaveBeenCalledWith(false);
+  expect(pm.spawn).toHaveBeenCalledTimes(1);
+});
@@ -49,6 +49,84 @@ export async function handleReloadRequest(deps: ReloadRequestDeps): Promise<void
  }
 }

+export interface RespawnTickDeps {
+  // True after a crash; false once a respawn succeeds.
+  crashed: () => boolean;
+  setCrashed: (value: boolean) => void;
+  pm: { running: boolean; setBundlePath(p: string): void; spawn(): void };
+  memoryFiles: MemoryFiles;
+  outDir: string;
+  // True once shutdown() has begun; the tick must not spawn during teardown.
+  isShuttingDown: () => boolean;
+  extractBundle?: (memoryFiles: MemoryFiles, outDir: string) => Promise<string>;
+}
+
+// One iteration of the crash-respawn interval: when the child has crashed and a
+// fresh good build is available, re-extract it and respawn the child.
+//
+// WHY the two isShuttingDown() checks: the body is async (awaits extractBundle
+// before pm.spawn()). The interval is cleared at the top of shutdown(), but a
+// tick already mid-`await` when that happens would still reach pm.spawn() and
+// orphan a brand-new child as the parent exits. The first check skips ticks that
+// start during teardown; the second re-checks AFTER the await, since shutdown
+// may have begun while we were extracting.
+export async function respawnTick(deps: RespawnTickDeps): Promise<void> {
+  if (deps.isShuttingDown()) return;
+  if (!deps.crashed() || deps.pm.running) return;
+  const extract = deps.extractBundle ?? extractBundle;
+  try {
+    const newPath = await extract(deps.memoryFiles, deps.outDir);
+    if (deps.isShuttingDown()) return;
+    deps.pm.setBundlePath(newPath);
+    deps.setCrashed(false);
+    deps.pm.spawn();
+  } catch {
+    // Build still broken, keep waiting.
+  }
+}
+
+export interface ShutdownDeps {
+  pm: { shutdown(): Promise<void> };
+  closeServer: () => Promise<void>;
+  // Clears the crash-respawn setInterval. Called FIRST so no new respawn tick
+  // can be scheduled once teardown starts.
+  clearRespawn: () => void;
+  exit: (code: number) => void;
+}
+
+// Build the SIGINT/SIGTERM handler. Factored out (not inlined in dev()) so the
+// re-entrancy guard and teardown ordering are unit-testable without a real
+// process.exit / Vite server.
+//
+// WHY a `shuttingDown` flag:
+//  - Re-entrancy: this is registered directly on SIGINT/SIGTERM. Pressing Ctrl+C
+//    twice (common when shutdown feels slow) invokes the handler twice
+//    concurrently. Without the guard that means double pm.shutdown()/closeServer
+//    and two racing exit() calls. The flag makes the 2nd+ invocation a no-op.
+//  - Orphan child: the crash-respawn interval is async (it awaits extractBundle
+//    before pm.spawn()). clearRespawn() stops new ticks, but a tick already
+//    in flight when shutdown starts could still reach pm.spawn() AFTER
+//    pm.shutdown() finished — orphaning a brand-new child as the parent exits.
+//    That in-flight tick checks the same `shuttingDown` flag (exposed via
+//    isShuttingDown) and bails before spawning. See the respawn interval body.
+export function createShutdown(deps: ShutdownDeps): {
+  handler: () => Promise<void>;
+  isShuttingDown: () => boolean;
+} {
+  let shuttingDown = false;
+  const handler = async () => {
+    if (shuttingDown) return;
+    shuttingDown = true;
+    // Stop the respawn interval BEFORE the async teardown window so no fresh
+    // child is spawned while we're tearing the old one down.
+    deps.clearRespawn();
+    await deps.pm.shutdown();
+    await deps.closeServer();
+    deps.exit(0);
+  };
+  return { handler, isShuttingDown: () => shuttingDown };
+}
+
 export async function dev(entry?: string) {
  const logger = createLogger();
  logger.info("Starting vue-tui dev server...");
@@ -112,26 +190,34 @@ export async function dev(entry?: string) {
    });
  });

+  // Declared before the interval so clearRespawn (below) can capture it; the
+  // closure reads it at call time, after the assignment.
+  let respawnInterval: ReturnType<typeof setInterval>;
+
+  // Graceful shutdown. createShutdown owns the re-entrancy guard and clears the
+  // respawn interval first; isShuttingDown is read by the respawn tick so an
+  // in-flight tick can't spawn a child mid-teardown. See createShutdown.
+  const shutdown = createShutdown({
+    pm,
+    closeServer: () => server.close(),
+    clearRespawn: () => clearInterval(respawnInterval),
+    exit: (code) => process.exit(code),
+  });
+
  // Re-spawn after crash on next successful bundle update
-  setInterval(async () => {
-    if (!crashed || pm.running) return;
-    try {
-      const newPath = await extractBundle(clientEnv.memoryFiles, outDir);
-      pm.setBundlePath(newPath);
-      crashed = false;
-      pm.spawn();
-    } catch {
-      // Build still broken, keep waiting
-    }
+  respawnInterval = setInterval(() => {
+    void respawnTick({
+      crashed: () => crashed,
+      setCrashed: (value) => {
+        crashed = value;
+      },
+      pm,
+      memoryFiles: clientEnv.memoryFiles,
+      outDir,
+      isShuttingDown: shutdown.isShuttingDown,
+    });
  }, 500);

-  // Graceful shutdown
-  const shutdown = async () => {
-    await pm.shutdown();
-    await server.close();
-    process.exit(0);
-  };
-
-  process.on("SIGINT", shutdown);
-  process.on("SIGTERM", shutdown);
+  process.on("SIGINT", shutdown.handler);
+  process.on("SIGTERM", shutdown.handler);
 }