fix: launchd reload handoff strands gateway when restart races the drain window (#110213)

* fix(daemon): reload handoff outwaits gateway drain before re-bootstrapping The launchd reload handoff waited only 15 x 0.2s (~3s) after bootout for the label to unload, but the booted-out gateway keeps the label registered until its drain-before-exit window (up to 300s) completes. Bootstrap then failed with EIO mid-drain, and the kickstart -k fallback cannot succeed on a booted-out label, leaving the LaunchAgent deregistered and the gateway down until a manual bootstrap. Extend the post-bootout wait to cover the full restart-deferral budget plus margin (315 x 1s, derived from DEFAULT_RESTART_DEFERRAL_TIMEOUT_MS), and replace the single dead kickstart fallback with a bootstrap retry loop that only falls back to kickstart -k while the label is actually registered, so the handoff never exits with the service deregistered. Closes #110137 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(daemon): scale reload bootout wait to the effective drain budget A config-raised gateway.reload.deferralTimeoutMs would outlast the fixed default-derived wait and reopen the stranded-LaunchAgent race. Thread the effective deferral timeout from restartLaunchAgent into the handoff and derive the reload bootout wait from it; unbounded (<=0) configs keep the finite default wait, with the bootstrap retry loop covering the overshoot. Related: #110137 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(daemon): derive reload wait from launchd ExitTimeOut and keep failed bootstrap status Review follow-up: the bootout SIGTERM path is bounded by the plist's ExitTimeOut, not the gateway's restart-deferral config, so derive the reload bootout wait from LAUNCH_AGENT_EXIT_TIMEOUT_SECONDS plus margin and drop the deferralTimeoutMs threading. Capture a failed bootstrap's status in the else branch: after a completed if with a false condition $? is 0, so exhausted retries logged 'restart done' and exit 0 while the LaunchAgent stayed deregistered. Adds an execution-level retry-exhaustion test that runs the generated script against an always-failing launchctl stub. Related: #110137 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(daemon): exercise delayed launchd reload handoff Run the generated reload handoff through delayed-stop and exhausted-bootstrap paths, and clarify that the wait covers launchd's ExitTimeOut stop window. Co-authored-by: MatthewSynthia <299972631+MatthewSynthia@users.noreply.github.com> * test(daemon): narrow generated handoff script Fail clearly when the spawn arguments omit the generated script and pass a narrowed string to the execution helper. Co-authored-by: MatthewSynthia <299972631+MatthewSynthia@users.noreply.github.com> * fix(daemon): retry bootstrap after launchd kickstart race Continue the bootstrap retry loop when a label disappears between the registration check and kickstart, and cover the race by executing the generated handoff script. Co-authored-by: MatthewSynthia <299972631+MatthewSynthia@users.noreply.github.com> * test(daemon): make handoff no-wait sentinel explicit Co-authored-by: MatthewSynthia <299972631+MatthewSynthia@users.noreply.github.com> --------- Co-authored-by: MatthewSynthia <299972631+MatthewSynthia@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Peter Steinberger <steipete@gmail.com>
2026-08-27 04:47:03 -06:00 · 2026-07-17 22:44:02 -07:00
parent d4d23fe954
commit c23dc147b8
2 changed files with 206 additions and 18 deletions
@@ -1,4 +1,9 @@
 // Launchd restart handoff tests cover restart coordination on macOS.
+import { execFile } from "node:child_process";
+import fs from "node:fs";
+import os from "node:os";
+import path from "node:path";
+import { promisify } from "node:util";
 import { afterEach, describe, expect, it, vi } from "vitest";

 const spawnMock = vi.hoisted(() => vi.fn());
@@ -16,6 +21,8 @@ import { scheduleDetachedLaunchdRestartHandoff } from "./launchd-restart-handoff

 type SpawnCall = [string, string[], { env: Record<string, string | undefined> }];

+const execFileAsync = promisify(execFile);
+
 function requireSpawnCall(callIndex = 0): SpawnCall {
  const call = spawnMock.mock.calls[callIndex];
  if (!call) {
@@ -33,6 +40,78 @@ function requireSpawnCall(callIndex = 0): SpawnCall {
  return [command, args as string[], options as SpawnCall[2]];
 }

+async function executeReloadHandoff(launchctlStub: string): Promise<{
+  calls: string[];
+  exitCode: number;
+  log: string;
+}> {
+  const noWaitPid = 0;
+  const stubDir = fs.mkdtempSync(path.join(os.tmpdir(), "launchd-stub-"));
+  try {
+    const home = path.join(stubDir, "home");
+    const callsPath = path.join(stubDir, "launchctl.calls");
+    fs.mkdirSync(path.join(home, ".openclaw", "logs"), { recursive: true });
+    fs.writeFileSync(
+      path.join(stubDir, "launchctl"),
+      `#!/bin/sh\nprintf '%s\\n' "$*" >> "$LAUNCHCTL_CALLS_PATH"\n${launchctlStub}\n`,
+    );
+    fs.chmodSync(path.join(stubDir, "launchctl"), 0o755);
+    fs.writeFileSync(path.join(stubDir, "sleep"), "#!/bin/sh\nexit 0\n");
+    fs.chmodSync(path.join(stubDir, "sleep"), 0o755);
+
+    spawnMock.mockReturnValue({ pid: 4242, unref: unrefMock });
+    scheduleDetachedLaunchdRestartHandoff({
+      env: { HOME: home, OPENCLAW_PROFILE: "default" },
+      mode: "reload",
+      waitForPid: noWaitPid,
+    });
+    const [, args] = requireSpawnCall();
+    const script = args[1];
+    if (!script) {
+      throw new Error("expected generated restart script");
+    }
+
+    let exitCode = 0;
+    try {
+      await execFileAsync(
+        "/bin/sh",
+        [
+          "-c",
+          script,
+          "handoff-test",
+          "gui/501/test.label",
+          "gui/501",
+          "/tmp/test.plist",
+          String(noWaitPid),
+        ],
+        {
+          env: {
+            ...process.env,
+            LAUNCHCTL_CALLS_PATH: callsPath,
+            LAUNCHCTL_STUB_DIR: stubDir,
+            PATH: `${stubDir}:${process.env.PATH}`,
+          },
+        },
+      );
+    } catch (error) {
+      const code = (error as { code?: unknown }).code;
+      if (typeof code !== "number") {
+        throw error;
+      }
+      exitCode = code;
+    }
+
+    const calls = fs.readFileSync(callsPath, "utf8").trim().split("\n");
+    const log = fs.readFileSync(
+      path.join(home, ".openclaw", "logs", "gateway-restart.log"),
+      "utf8",
+    );
+    return { calls, exitCode, log };
+  } finally {
+    fs.rmSync(stubDir, { recursive: true, force: true });
+  }
+}
+
 afterEach(() => {
  spawnMock.mockReset();
  unrefMock.mockReset();
@@ -95,7 +174,7 @@ describe("scheduleDetachedLaunchdRestartHandoff", () => {
    expect(args[1]).not.toContain('basename "$service_target"');
  });

-  it("polls after bootout and falls back to kickstart on bootstrap failure for reload mode", () => {
+  it("outwaits launchd's stop window after bootout and retries bootstrap for reload mode", () => {
    spawnMock.mockReturnValue({ pid: 4242, unref: unrefMock });

    scheduleDetachedLaunchdRestartHandoff({
@@ -111,12 +190,89 @@ describe("scheduleDetachedLaunchdRestartHandoff", () => {
    expect(args[1]).toContain("openclaw restart attempt source=launchd-handoff mode=reload");
    expect(args[1]).toContain('launchctl enable "$service_target"');
    expect(args[1]).toContain('launchctl bootout "$service_target"');
-    // polls until launchd finishes the async unload before re-bootstrapping
-    expect(args[1]).toContain("bootout_wait_count=");
+    // The unload poll must outlast launchd's ExitTimeOut SIGKILL ceiling plus
+    // margin (#110137): 35 × 1s vs the old 15 × 0.2s stop window.
+    expect(args[1]).toContain('bootout_wait_count="35"');
    expect(args[1]).toContain('if ! launchctl print "$service_target" >/dev/null 2>&1; then');
+    expect(args[1]).toContain("sleep 1");
+    // Bootstrap failures retry; kickstart -k only fires while the label is
+    // registered, because it cannot succeed on a booted-out label.
+    expect(args[1]).toContain('bootstrap_retry_count="15"');
    expect(args[1]).toContain('if launchctl bootstrap "$domain" "$plist_path"; then');
-    // fallback: kickstart -k on bootstrap failure so service isn't left deregistered
-    expect(args[1]).toContain('launchctl kickstart -k "$service_target"');
+    expect(args[1]).toContain(
+      'if launchctl print "$service_target" >/dev/null 2>&1; then\n    if launchctl kickstart -k "$service_target"; then',
+    );
+    expect(args[1]).toContain("bootstrap_retry_count=$((bootstrap_retry_count - 1))");
+  });
+
+  it("executes the generated reload handoff through a delayed launchd stop", async () => {
+    const result = await executeReloadHandoff(`
+case "$1" in
+  print)
+    count_file="$LAUNCHCTL_STUB_DIR/print-count"
+    count=0
+    [ -f "$count_file" ] && count=$(sed -n '1p' "$count_file")
+    count=$((count + 1))
+    printf '%s\n' "$count" > "$count_file"
+    [ "$count" -le 20 ] && exit 0
+    exit 113
+    ;;
+  bootstrap) exit 0 ;;
+  *) exit 0 ;;
+esac`);
+
+    expect(result.exitCode).toBe(0);
+    expect(result.calls.filter((call) => call.startsWith("print "))).toHaveLength(21);
+    expect(result.calls.filter((call) => call.startsWith("bootstrap "))).toHaveLength(1);
+    expect(result.log).toContain("restart done");
+    expect(result.log).not.toContain("restart failed");
+  });
+
+  it("retries bootstrap when the label disappears between print and kickstart", async () => {
+    const result = await executeReloadHandoff(`
+case "$1" in
+  print)
+    count_file="$LAUNCHCTL_STUB_DIR/print-count"
+    count=0
+    [ -f "$count_file" ] && count=$(sed -n '1p' "$count_file")
+    count=$((count + 1))
+    printf '%s\n' "$count" > "$count_file"
+    [ "$count" -eq 2 ] && exit 0
+    exit 113
+    ;;
+  bootstrap)
+    count_file="$LAUNCHCTL_STUB_DIR/bootstrap-count"
+    count=0
+    [ -f "$count_file" ] && count=$(sed -n '1p' "$count_file")
+    count=$((count + 1))
+    printf '%s\n' "$count" > "$count_file"
+    [ "$count" -eq 1 ] && exit 5
+    exit 0
+    ;;
+  kickstart) exit 113 ;;
+  *) exit 0 ;;
+esac`);
+
+    expect(result.exitCode).toBe(0);
+    expect(result.calls.filter((call) => call.startsWith("bootstrap "))).toHaveLength(2);
+    expect(result.calls.filter((call) => call.startsWith("kickstart "))).toHaveLength(1);
+    expect(result.log).toContain("restart done");
+    expect(result.log).not.toContain("restart failed");
+  });
+
+  it("reload retry exhaustion exits nonzero instead of reporting a successful restart", async () => {
+    // A completed if with a false condition leaves $? at 0. Keep this
+    // execution-level check so exhausted retries cannot report success while
+    // the LaunchAgent remains deregistered.
+    const result = await executeReloadHandoff(
+      'case "$1" in bootstrap) exit 5 ;; print) exit 113 ;; *) exit 0 ;; esac',
+    );
+
+    expect(result.exitCode).toBe(5);
+    expect(result.calls.filter((call) => call.startsWith("bootstrap "))).toHaveLength(15);
+    const { log } = result;
+    expect(log).toContain("restart failed");
+    expect(log).not.toContain("restart done");
  });

  it("sanitizes restart helper environment overrides before spawning", () => {
@@ -8,6 +8,7 @@ import { sanitizeForLog } from "../../packages/terminal-core/src/ansi.js";
 import { formatErrorMessage } from "../infra/errors.js";
 import { sanitizeHostExecEnv } from "../infra/host-env-security.js";
 import { resolveGatewayLaunchAgentLabel } from "./constants.js";
+import { LAUNCH_AGENT_EXIT_TIMEOUT_SECONDS } from "./launchd-plist.js";
 import { renderPosixRestartLogSetup } from "./restart-logs.js";

 type LaunchdRestartHandoffMode = "kickstart" | "reload" | "start-after-exit";
@@ -23,6 +24,13 @@ type LaunchdRestartTarget = {

 const START_AFTER_EXIT_PRINT_RETRY_COUNT = 15;
 const START_AFTER_EXIT_PRINT_RETRY_DELAY_SECONDS = 0.2;
+// The booted-out label stays registered until launchd finishes stopping the
+// old process. ExitTimeOut bounds that stop with SIGKILL, so the reload wait is
+// that ceiling plus teardown margin. A 3s poll could advance mid-stop and
+// strand the LaunchAgent (#110137).
+const RELOAD_BOOTOUT_WAIT_DELAY_SECONDS = 1;
+const RELOAD_BOOTOUT_WAIT_COUNT = LAUNCH_AGENT_EXIT_TIMEOUT_SECONDS + 15;
+const RELOAD_BOOTSTRAP_RETRY_COUNT = 15;

 type LaunchdRestartLogEnv = {
  HOME?: string;
@@ -138,17 +146,47 @@ exit "$status"
  if (mode === "reload") {
    // Reloading is required after plist content changes; kickstart alone keeps
    // launchd's already-loaded stdout/stderr/stdin paths.
-    // After bootout we poll until launchd finishes the async unload before
-    // re-bootstrapping to avoid EIO (Bootstrap failed: 5) from the race.
-    // If bootstrap still fails, kickstart -k as a fallback to keep the service
-    // alive rather than leaving it deregistered.
-    const bootoutWaitLoop = `bootout_wait_count="${START_AFTER_EXIT_PRINT_RETRY_COUNT}"
+    // After bootout the label stays registered until launchd finishes its
+    // ExitTimeOut-bounded stop, so this poll must outlast that stop window.
+    // Bootstrapping early fails with EIO (Bootstrap failed: 5) and can leave
+    // the LaunchAgent deregistered (#110137).
+    const bootoutWaitLoop = `bootout_wait_count="${RELOAD_BOOTOUT_WAIT_COUNT}"
 while [ "$bootout_wait_count" -gt 0 ]; do
  if ! launchctl print "$service_target" >/dev/null 2>&1; then
    break
  fi
  bootout_wait_count=$((bootout_wait_count - 1))
-  sleep ${START_AFTER_EXIT_PRINT_RETRY_DELAY_SECONDS}
+  sleep ${RELOAD_BOOTOUT_WAIT_DELAY_SECONDS}
+done
+`;
+    // kickstart -k cannot succeed on a booted-out label, so it is only a valid
+    // fallback while the label is registered; otherwise retry bootstrap so the
+    // handoff never exits with the service deregistered (#110137).
+    const bootstrapRetryLoop = `bootstrap_retry_count="${RELOAD_BOOTSTRAP_RETRY_COUNT}"
+while :; do
+  if launchctl bootstrap "$domain" "$plist_path"; then
+    status=0
+    break
+  else
+    # Capture inside the else: after a completed if with a false condition,
+    # $? is 0, which would let exhausted retries report a successful restart.
+    status=$?
+  fi
+  if launchctl print "$service_target" >/dev/null 2>&1; then
+    if launchctl kickstart -k "$service_target"; then
+      status=0
+      break
+    else
+      # The pending bootout can finish between print and kickstart. Keep
+      # retrying bootstrap if that check-then-act race deregisters the label.
+      status=$?
+    fi
+  fi
+  bootstrap_retry_count=$((bootstrap_retry_count - 1))
+  if [ "$bootstrap_retry_count" -le 0 ]; then
+    break
+  fi
+  sleep ${RELOAD_BOOTOUT_WAIT_DELAY_SECONDS}
 done
 `;
    return `service_target="$1"
@@ -159,13 +197,7 @@ status=0
 launchctl enable "$service_target"
 launchctl bootout "$service_target" >/dev/null 2>&1 || true
 ${bootoutWaitLoop}
-if launchctl bootstrap "$domain" "$plist_path"; then
-  status=0
-else
-  status=$?
-  launchctl kickstart -k "$service_target"
-  status=$?
-fi
+${bootstrapRetryLoop}
 if [ "$status" -eq 0 ]; then
  printf '[%s] openclaw restart done source=launchd-handoff mode=${mode}\\n' "$(date -u +%FT%TZ)" >&2
 else