refactor(FN-1251): harden child runtime timer lifecycle

- Add generation tracking plus dedicated SIGKILL and restart timer fields in ChildProcessRuntime - Guard delayed kill/restart callbacks so stale generations and stopping runtimes are ignored - Clear pending lifecycle timers during stop and before rescheduling to prevent stacked callbacks - Expand child-process runtime tests to cover timer cancellation, generation safety, and spawn generation increments
2026-04-08 09:56:28 -07:00
parent 2a76761ce8
commit 185ee2e2f5
2 changed files with 294 additions and 12 deletions
--- a/packages/engine/src/runtimes/child-process-runtime.test.ts
+++ b/packages/engine/src/runtimes/child-process-runtime.test.ts
@@ -1,21 +1,78 @@
 import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
 import type { CentralCore } from "@fusion/core";
 import { ChildProcessRuntime } from "./child-process-runtime.js";
+import { IpcHost } from "../ipc/ipc-host.js";
 import type { ProjectRuntimeConfig } from "../project-runtime.js";

+type MockChildProcess = {
+  on: ReturnType<typeof vi.fn>;
+  kill: ReturnType<typeof vi.fn>;
+  killed: boolean;
+  connected: boolean;
+  send: ReturnType<typeof vi.fn>;
+  disconnect: ReturnType<typeof vi.fn>;
+};
+
+const createMockChildProcess = (): MockChildProcess => {
+  const child: MockChildProcess = {
+    on: vi.fn(),
+    kill: vi.fn((signal?: string | number) => {
+      if (signal === "SIGTERM" || signal === "SIGKILL") {
+        child.killed = true;
+      }
+      return true;
+    }),
+    killed: false,
+    connected: true,
+    send: vi.fn((_message: unknown, callback?: (error: Error | null) => void) => {
+      callback?.(null);
+      return true;
+    }),
+    disconnect: vi.fn(() => {
+      child.connected = false;
+    }),
+  };
+
+  return child;
+};
+
+const { forkMock, forkedChildren } = vi.hoisted(() => {
+  const forkedChildren: MockChildProcess[] = [];
+  const forkMock = vi.fn(() => {
+    const child: MockChildProcess = {
+      on: vi.fn(),
+      kill: vi.fn((signal?: string | number) => {
+        if (signal === "SIGTERM" || signal === "SIGKILL") {
+          child.killed = true;
+        }
+        return true;
+      }),
+      killed: false,
+      connected: true,
+      send: vi.fn((_message: unknown, callback?: (error: Error | null) => void) => {
+        callback?.(null);
+        return true;
+      }),
+      disconnect: vi.fn(() => {
+        child.connected = false;
+      }),
+    };
+
+    forkedChildren.push(child);
+    return child;
+  });
+
+  return { forkMock, forkedChildren };
+});
+
 // Mock child_process
 vi.mock("node:child_process", () => ({
-  fork: vi.fn().mockReturnValue({
-    on: vi.fn(),
-    kill: vi.fn(),
-    killed: false,
-    connected: false,
-    send: vi.fn(),
-  }),
+  fork: forkMock,
 }));

 describe("ChildProcessRuntime", () => {
  let runtime: ChildProcessRuntime;
+  let runtimeAny: any;
  let mockCentralCore: CentralCore;
  const testConfig: ProjectRuntimeConfig = {
    projectId: "proj_test123",
@@ -36,6 +93,9 @@ describe("ChildProcessRuntime", () => {
    } as unknown as CentralCore;

    runtime = new ChildProcessRuntime(testConfig, mockCentralCore);
+    runtimeAny = runtime as any;
+
+    vi.spyOn(IpcHost.prototype, "sendCommand").mockResolvedValue(undefined);
  });

  afterEach(async () => {
@@ -44,7 +104,11 @@ describe("ChildProcessRuntime", () => {
    } catch {
      // Ignore errors during cleanup
    }
+
+    vi.useRealTimers();
+    vi.restoreAllMocks();
    vi.clearAllMocks();
+    forkedChildren.length = 0;
  });

  describe("lifecycle", () => {
@@ -86,7 +150,7 @@ describe("ChildProcessRuntime", () => {
    it("should support health-changed event", () => {
      const handler = vi.fn();
      runtime.on("health-changed", handler);
-      
+
      // The constructor may emit health-changed, so we just verify
      // the event listener can be registered
      expect(handler).not.toHaveBeenCalled();
@@ -95,8 +159,161 @@ describe("ChildProcessRuntime", () => {
    it("should support error event", () => {
      const handler = vi.fn();
      runtime.on("error", handler);
-      
+
      expect(handler).not.toHaveBeenCalled();
    });
  });
+
+  describe("timer lifecycle and generation safety", () => {
+    it("cancels SIGKILL path after stop and never force-kills", async () => {
+      vi.useFakeTimers();
+
+      await runtime.start();
+      const child = forkedChildren.at(-1);
+      expect(child).toBeDefined();
+
+      const errorHandler = vi.fn();
+      runtime.on("error", errorHandler);
+
+      await runtime.stop();
+      vi.advanceTimersByTime(6000);
+
+      expect(child?.kill).toHaveBeenCalledWith("SIGTERM");
+      expect(child?.kill).not.toHaveBeenCalledWith("SIGKILL");
+      expect(errorHandler).not.toHaveBeenCalled();
+    });
+
+    it("cancels prior SIGKILL timer when killChild is called again", () => {
+      vi.useFakeTimers();
+
+      const clearTimeoutSpy = vi.spyOn(globalThis, "clearTimeout");
+      const firstChild = createMockChildProcess();
+      runtimeAny.child = firstChild;
+
+      runtimeAny.killChild();
+      const firstTimer = runtimeAny.sigkillTimer;
+      expect(firstTimer).not.toBeNull();
+
+      const secondChild = createMockChildProcess();
+      runtimeAny.child = secondChild;
+      runtimeAny.killChild();
+
+      expect(clearTimeoutSpy).toHaveBeenCalledWith(firstTimer);
+
+      vi.advanceTimersByTime(6000);
+
+      expect(firstChild.kill).toHaveBeenCalledWith("SIGTERM");
+      expect(secondChild.kill).toHaveBeenCalledWith("SIGTERM");
+      expect(secondChild.kill).not.toHaveBeenCalledWith("SIGKILL");
+    });
+
+    it("prevents stale SIGKILL timers from killing a newer generation child", () => {
+      vi.useFakeTimers();
+
+      const oldChild = createMockChildProcess();
+      runtimeAny.child = oldChild;
+      runtimeAny.generation = 10;
+
+      runtimeAny.killChild();
+
+      const replacementChild = createMockChildProcess();
+      runtimeAny.generation = 11;
+      runtimeAny.child = replacementChild;
+
+      vi.advanceTimersByTime(6000);
+
+      expect(oldChild.kill).toHaveBeenCalledWith("SIGTERM");
+      expect(replacementChild.kill).not.toHaveBeenCalledWith("SIGKILL");
+    });
+
+    it("cancels restart timer on stop", async () => {
+      vi.useFakeTimers();
+
+      runtimeAny.status = "active";
+      const spawnSpy = vi.spyOn(runtimeAny, "spawnChild").mockResolvedValue(undefined);
+
+      runtimeAny.handleUnhealthy();
+      await runtime.stop();
+
+      vi.advanceTimersByTime(20000);
+
+      expect(spawnSpy).not.toHaveBeenCalled();
+      expect(forkMock).not.toHaveBeenCalled();
+    });
+
+    it("prevents stale restart callbacks when generation changes", () => {
+      vi.useFakeTimers();
+
+      runtimeAny.status = "active";
+      const killSpy = vi.spyOn(runtimeAny, "killChild").mockImplementation(() => {});
+      const spawnSpy = vi.spyOn(runtimeAny, "spawnChild").mockResolvedValue(undefined);
+
+      runtimeAny.handleUnhealthy();
+      runtimeAny.generation += 1;
+
+      vi.advanceTimersByTime(1000);
+
+      expect(killSpy).not.toHaveBeenCalled();
+      expect(spawnSpy).not.toHaveBeenCalled();
+      expect(runtimeAny.restartTimer).toBeNull();
+    });
+
+    it("prevents restart callbacks while stopping/stopped", () => {
+      vi.useFakeTimers();
+
+      runtimeAny.status = "active";
+      const killSpy = vi.spyOn(runtimeAny, "killChild").mockImplementation(() => {});
+      const spawnSpy = vi.spyOn(runtimeAny, "spawnChild").mockResolvedValue(undefined);
+
+      runtimeAny.handleUnhealthy();
+      runtimeAny.status = "stopping";
+
+      vi.advanceTimersByTime(1000);
+
+      expect(killSpy).not.toHaveBeenCalled();
+      expect(spawnSpy).not.toHaveBeenCalled();
+      expect(runtimeAny.restartTimer).toBeNull();
+    });
+
+    it("clearAllTimers cancels both SIGKILL and restart timers together via stop", async () => {
+      vi.useFakeTimers();
+
+      runtimeAny.status = "active";
+      const child = createMockChildProcess();
+      runtimeAny.child = child;
+
+      const spawnSpy = vi.spyOn(runtimeAny, "spawnChild").mockResolvedValue(undefined);
+
+      runtimeAny.killChild();
+      runtimeAny.handleUnhealthy();
+
+      expect(runtimeAny.sigkillTimer).not.toBeNull();
+      expect(runtimeAny.restartTimer).not.toBeNull();
+
+      await runtime.stop();
+      vi.advanceTimersByTime(20000);
+
+      expect(child.kill).toHaveBeenCalledTimes(1);
+      expect(child.kill).toHaveBeenCalledWith("SIGTERM");
+      expect(child.kill).not.toHaveBeenCalledWith("SIGKILL");
+      expect(spawnSpy).not.toHaveBeenCalled();
+      expect(runtimeAny.sigkillTimer).toBeNull();
+      expect(runtimeAny.restartTimer).toBeNull();
+    });
+
+    it("increments generation on each spawnChild call", async () => {
+      vi.useFakeTimers();
+
+      expect(runtimeAny.generation).toBe(0);
+
+      await runtimeAny.spawnChild();
+      expect(runtimeAny.generation).toBe(1);
+
+      runtimeAny.killChild();
+      await runtimeAny.spawnChild();
+      expect(runtimeAny.generation).toBe(2);
+
+      expect(forkMock).toHaveBeenCalledTimes(2);
+    });
+  });
 });
--- a/packages/engine/src/runtimes/child-process-runtime.ts
+++ b/packages/engine/src/runtimes/child-process-runtime.ts
@@ -126,6 +126,12 @@ class HealthMonitor {
 * - Graceful shutdown with configurable timeout
 * - Event forwarding from child process to host listeners
 *
+ * Timer/generation safety pattern:
+ * - Every delayed callback (SIGKILL fallback, restart backoff) is tracked in a field.
+ * - Timers are cleared during shutdown and before replacement to avoid stacked callbacks.
+ * - Each spawned child increments a monotonic generation counter captured by callbacks.
+ *   If a callback's captured generation no longer matches, it bails as stale.
+ *
 * @example
 * ```typescript
 * const config: ProjectRuntimeConfig = {
@@ -153,6 +159,15 @@ export class ChildProcessRuntime
  private child: ChildProcess | null = null;
  private ipcHost: IpcHost | null = null;
  private healthMonitor: HealthMonitor;
+  /**
+   * Monotonic child-process generation.
+   *
+   * Incremented before every spawn so delayed callbacks can invalidate themselves
+   * if they were scheduled against an older process generation.
+   */
+  private generation = 0;
+  private sigkillTimer: ReturnType<typeof setTimeout> | null = null;
+  private restartTimer: ReturnType<typeof setTimeout> | null = null;
  private lastMetrics: RuntimeMetrics = {
    inFlightTasks: 0,
    activeAgents: 0,
@@ -220,6 +235,8 @@ export class ChildProcessRuntime
    // Determine worker entry point
    const workerPath = this.getWorkerPath();

+    this.generation += 1;
+
    runtimeLog.log(`Forking child process: ${workerPath}`);

    // Fork child process
@@ -321,6 +338,9 @@ export class ChildProcessRuntime
    this.setStatus("stopping");
    runtimeLog.log(`Stopping ChildProcessRuntime for project ${this.config.projectId}`);

+    // Cancel all pending timers (SIGKILL timeout, restart backoff)
+    this.clearAllTimers();
+
    // Stop health monitoring
    this.healthMonitor.stop();

@@ -345,12 +365,23 @@ export class ChildProcessRuntime
   * Kill the child process forcefully.
   */
  private killChild(): void {
+    if (this.sigkillTimer !== null) {
+      clearTimeout(this.sigkillTimer);
+      this.sigkillTimer = null;
+    }
+
    if (this.child && !this.child.killed) {
      runtimeLog.log("Killing child process");
      this.child.kill("SIGTERM");

-      // Force kill after 5 seconds if still running
-      setTimeout(() => {
+      const gen = this.generation;
+      this.sigkillTimer = setTimeout(() => {
+        this.sigkillTimer = null;
+
+        if (this.generation !== gen) {
+          return;
+        }
+
        if (this.child && !this.child.killed) {
          runtimeLog.warn("Force killing child process");
          this.child.kill("SIGKILL");
@@ -362,6 +393,24 @@ export class ChildProcessRuntime
    this.ipcHost = null;
  }

+  /**
+   * Clears all delayed lifecycle timers.
+   *
+   * This is called during shutdown and before replacing pending callbacks so
+   * stale SIGKILL/restart timers cannot fire against newer runtime state.
+   */
+  private clearAllTimers(): void {
+    if (this.sigkillTimer !== null) {
+      clearTimeout(this.sigkillTimer);
+      this.sigkillTimer = null;
+    }
+
+    if (this.restartTimer !== null) {
+      clearTimeout(this.restartTimer);
+      this.restartTimer = null;
+    }
+  }
+
  /**
   * Get the current runtime status.
   */
@@ -436,6 +485,11 @@ export class ChildProcessRuntime
  private handleUnhealthy(): void {
    const maxRestarts = 3;

+    if (this.restartTimer !== null) {
+      clearTimeout(this.restartTimer);
+      this.restartTimer = null;
+    }
+
    if (this.healthMonitor.getRestartAttempts() >= maxRestarts) {
      runtimeLog.error(`Max restart attempts (${maxRestarts}) reached, transitioning to errored`);
      this.setStatus("errored");
@@ -448,7 +502,18 @@ export class ChildProcessRuntime

    runtimeLog.log(`Attempting restart ${this.healthMonitor.getRestartAttempts()}/${maxRestarts} after ${delay}ms`);

-    setTimeout(async () => {
+    const gen = this.generation;
+    this.restartTimer = setTimeout(async () => {
+      this.restartTimer = null;
+
+      if (this.generation !== gen) {
+        return;
+      }
+
+      if (this.status === "stopping" || this.status === "stopped") {
+        return;
+      }
+
      try {
        this.killChild();
        await this.spawnChild();