From 3c10513c72a695e4f6cbbe287dcd882fd2a780d4 Mon Sep 17 00:00:00 2001 From: Alix-007 Date: Tue, 21 Jul 2026 11:36:04 +0800 Subject: [PATCH] fix(doctor): bound legacy launchd cleanup (#109116) * fix(doctor): bound legacy launchd cleanup Bound both legacy launchctl cleanup attempts to five seconds while keeping the existing bootout-then-unload order. Only move the legacy plist after a successful command or an explicit already-unloaded result; leave it in place when timeout or another failure prevents confirmation so a later doctor run can retry. Report filesystem cleanup failures as skipped instead of claiming removal. * fix(doctor): verify launchd cleanup state before moving plist Poll a bounded launchctl print postcondition after legacy bootout and unload. Treat loaded, unknown, and timed-out probes as unconfirmed so doctor leaves the plist available for retry. * test(doctor): clarify launchd cleanup postcondition * fix(doctor): reject timed-out launchd probes Preserve timeout evidence from the command runner, including sanitized timeout messages, so partial not-loaded output cannot authorize plist removal. * fix(doctor): classify no-output launchd timeouts * test(doctor): trim launchd cleanup cases Co-authored-by: Alix-007 --------- Co-authored-by: Peter Steinberger --- src/commands/doctor-gateway-services.test.ts | 249 +++++++++++++++++++ src/commands/doctor-gateway-services.ts | 84 ++++++- src/daemon/launchd.ts | 2 +- 3 files changed, 325 insertions(+), 10 deletions(-) diff --git a/src/commands/doctor-gateway-services.test.ts b/src/commands/doctor-gateway-services.test.ts index 56adbebc21c1..052c092f8c4b 100644 --- a/src/commands/doctor-gateway-services.test.ts +++ b/src/commands/doctor-gateway-services.test.ts @@ -48,6 +48,7 @@ const mocks = vi.hoisted(() => ({ uninstallLegacySystemdUnits: vi.fn().mockResolvedValue([]), readWindowsProcessArgsSync: vi.fn(), readWindowsStartupFallbackRuntimeForUpdate: vi.fn(), + runExec: vi.fn(), note: vi.fn(), })); @@ -111,6 +112,10 @@ vi.mock("../infra/windows-port-pids.js", () => ({ readWindowsProcessArgsSync: mocks.readWindowsProcessArgsSync, })); +vi.mock("../process/exec.js", () => ({ + runExec: mocks.runExec, +})); + vi.mock("../../packages/terminal-core/src/note.js", () => ({ note: mocks.note, })); @@ -174,6 +179,70 @@ function mockProcessPlatform(platform: NodeJS.Platform) { }); } +const LEGACY_MAC_LABEL = "com.openclaw.gateway"; +const LEGACY_MAC_PLIST = "/Users/test/Library/LaunchAgents/com.openclaw.gateway.plist"; + +function setupLegacyMacService() { + mockProcessPlatform("darwin"); + mocks.findExtraGatewayServices.mockResolvedValue([ + { + platform: "darwin", + label: LEGACY_MAC_LABEL, + detail: `plist: ${LEGACY_MAC_PLIST}`, + scope: "user", + legacy: true, + }, + ]); +} + +function launchctlFailure( + params: { + message?: string; + stderr?: string; + stdout?: string; + timedOut?: boolean; + } = {}, +) { + return Object.assign(new Error(params.message ?? "launchctl failed"), { + stderr: params.stderr ?? "", + stdout: params.stdout ?? "", + ...(params.timedOut ? { timedOut: true } : {}), + }); +} + +function expectBoundedLaunchctlCleanup() { + const domain = typeof process.getuid === "function" ? `gui/${process.getuid()}` : "gui/501"; + expect(mocks.runExec).toHaveBeenNthCalledWith( + 1, + "launchctl", + ["bootout", domain, LEGACY_MAC_PLIST], + { logOutput: false, timeoutMs: 5_000 }, + ); + expect(mocks.runExec).toHaveBeenNthCalledWith(2, "launchctl", ["unload", LEGACY_MAC_PLIST], { + logOutput: false, + timeoutMs: 5_000, + }); + expect(mocks.runExec).toHaveBeenNthCalledWith( + 3, + "launchctl", + ["print", `${domain}/${LEGACY_MAC_LABEL}`], + { + logOutput: false, + timeoutMs: expect.any(Number), + }, + ); + const probeTimeout = mocks.runExec.mock.calls[2]?.[2]?.timeoutMs; + expect(probeTimeout).toBeGreaterThan(0); + expect(probeTimeout).toBeLessThanOrEqual(5_000); +} + +function mockConfirmedUnloaded(stderr = "Could not find service") { + mocks.runExec + .mockResolvedValueOnce({ stdout: "", stderr: "" }) + .mockResolvedValueOnce({ stdout: "", stderr: "" }) + .mockRejectedValueOnce(launchctlFailure({ stderr })); +} + async function runRepair(cfg: OpenClawConfig, options: { allowExecSecretRefs?: boolean } = {}) { await maybeRepairGatewayServiceConfig(cfg, "local", makeDoctorIo(), makeDoctorPrompts(), options); } @@ -1593,9 +1662,11 @@ describe("maybeScanExtraGatewayServices", () => { mocks.renderGatewayServiceCleanupHints.mockReturnValue([]); mocks.isSystemdUnitActive.mockResolvedValue(false); mocks.uninstallLegacySystemdUnits.mockResolvedValue([]); + mocks.runExec.mockResolvedValue({ stdout: "", stderr: "" }); }); afterEach(() => { + vi.restoreAllMocks(); mockProcessPlatform(originalPlatform); }); @@ -1773,6 +1844,184 @@ describe("maybeScanExtraGatewayServices", () => { ); }); + it.each(["Could not find service", "No such process"])( + "moves a legacy macOS plist only after print reports '%s'", + async (stderr) => { + setupLegacyMacService(); + mockConfirmedUnloaded(stderr); + const runtime = makeDoctorIo(); + const rename = vi.spyOn(fs, "rename").mockResolvedValue(undefined); + vi.spyOn(fs, "mkdir").mockResolvedValue(undefined); + vi.spyOn(fs, "access").mockResolvedValue(undefined); + + await maybeScanExtraGatewayServices({ deep: false }, runtime, makeDoctorPrompts()); + + expectBoundedLaunchctlCleanup(); + expect(rename).toHaveBeenCalledTimes(1); + expectNoteContaining(LEGACY_MAC_LABEL, "Legacy gateway removed"); + expectNoNoteContaining(LEGACY_MAC_LABEL, "Legacy gateway cleanup skipped"); + expect(runtime.log).toHaveBeenCalledWith( + "Legacy gateway services removed. Installing OpenClaw gateway next.", + ); + }, + ); + + it.each([ + ["timeouts", launchctlFailure({ timedOut: true })], + ["unknown failures", launchctlFailure({ stderr: "Permission denied" })], + ])("keeps the plist when both launchctl calls end in %s", async (_, failure) => { + setupLegacyMacService(); + mocks.runExec.mockRejectedValue(failure); + const mkdir = vi.spyOn(fs, "mkdir").mockResolvedValue(undefined); + const access = vi.spyOn(fs, "access").mockResolvedValue(undefined); + const rename = vi.spyOn(fs, "rename").mockResolvedValue(undefined); + const runtime = makeDoctorIo(); + + await maybeScanExtraGatewayServices({ deep: false }, runtime, makeDoctorPrompts()); + + expectBoundedLaunchctlCleanup(); + expect(mkdir).not.toHaveBeenCalled(); + expect(access).not.toHaveBeenCalled(); + expect(rename).not.toHaveBeenCalled(); + expectNoteContaining( + `${LEGACY_MAC_LABEL} (launchctl could not confirm unload)`, + "Legacy gateway cleanup skipped", + ); + expectNoNoteContaining(LEGACY_MAC_LABEL, "Legacy gateway removed"); + expect(runtime.log).not.toHaveBeenCalledWith( + "Legacy gateway services removed. Installing OpenClaw gateway next.", + ); + }); + + it("keeps the plist when a successful cleanup command is followed by a loaded probe", async () => { + setupLegacyMacService(); + mocks.runExec + .mockRejectedValueOnce(launchctlFailure({ timedOut: true })) + .mockResolvedValueOnce({ stdout: "", stderr: "" }) + .mockResolvedValueOnce({ stdout: "state = waiting\npid = 0\n", stderr: "" }) + .mockRejectedValueOnce(launchctlFailure({ stderr: "Permission denied" })); + const mkdir = vi.spyOn(fs, "mkdir").mockResolvedValue(undefined); + const access = vi.spyOn(fs, "access").mockResolvedValue(undefined); + const rename = vi.spyOn(fs, "rename").mockResolvedValue(undefined); + const runtime = makeDoctorIo(); + + await maybeScanExtraGatewayServices({ deep: false }, runtime, makeDoctorPrompts()); + + expect(mocks.runExec).toHaveBeenCalledTimes(4); + expect(mkdir).not.toHaveBeenCalled(); + expect(access).not.toHaveBeenCalled(); + expect(rename).not.toHaveBeenCalled(); + expectNoteContaining( + `${LEGACY_MAC_LABEL} (launchctl could not confirm unload)`, + "Legacy gateway cleanup skipped", + ); + expectNoNoteContaining(LEGACY_MAC_LABEL, "Legacy gateway removed"); + }); + + it("keeps the plist when the postcondition probe times out", async () => { + setupLegacyMacService(); + mocks.runExec + .mockResolvedValueOnce({ stdout: "", stderr: "" }) + .mockResolvedValueOnce({ stdout: "", stderr: "" }) + .mockRejectedValueOnce( + launchctlFailure({ + message: "Command timed out after 5000 milliseconds", + stderr: "Could not find service", + }), + ); + const mkdir = vi.spyOn(fs, "mkdir").mockResolvedValue(undefined); + const access = vi.spyOn(fs, "access").mockResolvedValue(undefined); + const rename = vi.spyOn(fs, "rename").mockResolvedValue(undefined); + const runtime = makeDoctorIo(); + + await maybeScanExtraGatewayServices({ deep: false }, runtime, makeDoctorPrompts()); + + expectBoundedLaunchctlCleanup(); + expect(mkdir).not.toHaveBeenCalled(); + expect(access).not.toHaveBeenCalled(); + expect(rename).not.toHaveBeenCalled(); + expectNoteContaining( + `${LEGACY_MAC_LABEL} (launchctl could not confirm unload)`, + "Legacy gateway cleanup skipped", + ); + expectNoNoteContaining(LEGACY_MAC_LABEL, "Legacy gateway removed"); + }); + + it("polls a still-registered stopped label until launchd reports it gone", async () => { + setupLegacyMacService(); + mocks.runExec + .mockResolvedValueOnce({ stdout: "", stderr: "" }) + .mockResolvedValueOnce({ stdout: "", stderr: "" }) + .mockResolvedValueOnce({ stdout: "state = waiting\npid = 0\n", stderr: "" }) + .mockRejectedValueOnce(launchctlFailure({ stderr: "Could not find service" })); + vi.spyOn(fs, "mkdir").mockResolvedValue(undefined); + vi.spyOn(fs, "access").mockResolvedValue(undefined); + const rename = vi.spyOn(fs, "rename").mockResolvedValue(undefined); + + await maybeScanExtraGatewayServices({ deep: false }, makeDoctorIo(), makeDoctorPrompts()); + + expect(mocks.runExec).toHaveBeenCalledTimes(4); + expect(rename).toHaveBeenCalledTimes(1); + expectNoteContaining(LEGACY_MAC_LABEL, "Legacy gateway removed"); + }); + + it("reports removal when launchctl confirms unload and the plist is already absent", async () => { + setupLegacyMacService(); + mockConfirmedUnloaded(); + vi.spyOn(fs, "mkdir").mockResolvedValue(undefined); + const missing = Object.assign(new Error("missing"), { code: "ENOENT" }); + vi.spyOn(fs, "access").mockRejectedValue(missing); + const rename = vi.spyOn(fs, "rename").mockResolvedValue(undefined); + + await maybeScanExtraGatewayServices({ deep: false }, makeDoctorIo(), makeDoctorPrompts()); + + expectBoundedLaunchctlCleanup(); + expect(rename).not.toHaveBeenCalled(); + expectNoteContaining(LEGACY_MAC_LABEL, "Legacy gateway removed"); + expectNoNoteContaining(LEGACY_MAC_LABEL, "Legacy gateway cleanup skipped"); + }); + + it("does not report removal when the plist cannot be inspected", async () => { + setupLegacyMacService(); + mockConfirmedUnloaded(); + vi.spyOn(fs, "mkdir").mockResolvedValue(undefined); + vi.spyOn(fs, "access").mockRejectedValue( + Object.assign(new Error("permission denied"), { code: "EACCES" }), + ); + const rename = vi.spyOn(fs, "rename").mockResolvedValue(undefined); + + await maybeScanExtraGatewayServices({ deep: false }, makeDoctorIo(), makeDoctorPrompts()); + + expectBoundedLaunchctlCleanup(); + expect(rename).not.toHaveBeenCalled(); + expectNoteContaining( + `${LEGACY_MAC_LABEL} (could not inspect plist)`, + "Legacy gateway cleanup skipped", + ); + expectNoNoteContaining(LEGACY_MAC_LABEL, "Legacy gateway removed"); + }); + + it("does not report removal when the confirmed-unloaded plist cannot be moved", async () => { + setupLegacyMacService(); + mockConfirmedUnloaded(); + vi.spyOn(fs, "mkdir").mockResolvedValue(undefined); + vi.spyOn(fs, "access").mockResolvedValue(undefined); + vi.spyOn(fs, "rename").mockRejectedValue(new Error("permission denied")); + const runtime = makeDoctorIo(); + + await maybeScanExtraGatewayServices({ deep: false }, runtime, makeDoctorPrompts()); + + expectBoundedLaunchctlCleanup(); + expectNoteContaining( + `${LEGACY_MAC_LABEL} (could not move plist)`, + "Legacy gateway cleanup skipped", + ); + expectNoNoteContaining(LEGACY_MAC_LABEL, "Legacy gateway removed"); + expect(runtime.log).not.toHaveBeenCalledWith( + "Legacy gateway services removed. Installing OpenClaw gateway next.", + ); + }); + it("reports legacy services but skips cleanup when service repair policy is external", async () => { await withEnvAsync({ OPENCLAW_SERVICE_REPAIR_POLICY: "external" }, async () => { mocks.findExtraGatewayServices.mockResolvedValue([ diff --git a/src/commands/doctor-gateway-services.ts b/src/commands/doctor-gateway-services.ts index e45c7fa99b33..7c22d6cf62fc 100644 --- a/src/commands/doctor-gateway-services.ts +++ b/src/commands/doctor-gateway-services.ts @@ -16,6 +16,7 @@ import { renderGatewayServiceCleanupHints, type ExtraGatewayService, } from "../daemon/inspect.js"; +import { isLaunchctlNotLoaded } from "../daemon/launchd.js"; import { OPENCLAW_WRAPPER_ENV_KEY } from "../daemon/program-args.js"; import { renderSystemNodeWarning, resolveSystemNodeInfo } from "../daemon/runtime-paths.js"; import { readWindowsStartupFallbackRuntimeForUpdate } from "../daemon/schtasks.js"; @@ -108,8 +109,60 @@ const EXECSTART_REPAIR_CODES = new Set([ SERVICE_AUDIT_CODES.gatewayCommandMissing, SERVICE_AUDIT_CODES.gatewayEntrypointMismatch, ]); -const runLaunchctlQuietly = (args: string[]) => - runExec("launchctl", args, { logOutput: false }).catch(() => undefined); +const DOCTOR_LAUNCHCTL_TIMEOUT_MS = 5_000; +const DOCTOR_LAUNCHCTL_CONFIRM_POLL_MS = 100; +type LaunchctlCleanupAttempt = + | { status: "succeeded"; stdout: string; stderr: string } + | { status: "failed"; stdout: string; stderr: string; timedOut: boolean }; + +const runLaunchctlQuietly = async ( + args: string[], + timeoutMs = DOCTOR_LAUNCHCTL_TIMEOUT_MS, +): Promise => { + try { + const output = await runExec("launchctl", args, { + logOutput: false, + timeoutMs, + }); + return { status: "succeeded", ...output }; + } catch (error) { + const record = error && typeof error === "object" ? (error as Record) : {}; + const message = typeof record.message === "string" ? record.message : ""; + return { + status: "failed", + stdout: typeof record.stdout === "string" ? record.stdout : "", + stderr: typeof record.stderr === "string" ? record.stderr : "", + timedOut: + record.timedOut === true || + record.noOutputTimedOut === true || + /\bcommand timed out\b/i.test(message), + }; + } +}; + +async function confirmLegacyLaunchdServiceUnloaded(serviceTarget: string): Promise { + const deadline = Date.now() + DOCTOR_LAUNCHCTL_TIMEOUT_MS; + while (Date.now() < deadline) { + const remainingMs = Math.max(1, deadline - Date.now()); + const probe = await runLaunchctlQuietly( + ["print", serviceTarget], + Math.min(DOCTOR_LAUNCHCTL_TIMEOUT_MS, remainingMs), + ); + if (probe.status === "failed") { + // A successful print (including a stopped job) means launchd still owns + // the label. Unknown errors and probe timeouts stay fail-closed. + return !probe.timedOut && isLaunchctlNotLoaded(probe); + } + const delayMs = Math.min(DOCTOR_LAUNCHCTL_CONFIRM_POLL_MS, deadline - Date.now()); + if (delayMs <= 0) { + break; + } + await new Promise((resolve) => { + setTimeout(resolve, delayMs); + }); + } + return false; +} const GATEWAY_SERVICES_EXTRA_CHECK_ID = "core/doctor/gateway-services/extra"; function detectGatewayRuntime(programArguments: string[] | undefined): GatewayDaemonRuntime { @@ -343,11 +396,17 @@ export function extraGatewayServiceToRepairEffects( async function cleanupLegacyLaunchdService(params: { label: string; plistPath: string; -}): Promise { +}): Promise<{ status: "removed"; destination?: string } | { status: "failed"; reason: string }> { const domain = typeof process.getuid === "function" ? `gui/${process.getuid()}` : "gui/501"; await runLaunchctlQuietly(["bootout", domain, params.plistPath]); await runLaunchctlQuietly(["unload", params.plistPath]); + // bootout/unload can return before launchd finishes stopping the job. A plist + // must stay in place unless a bounded print probe observes the label gone. + if (!(await confirmLegacyLaunchdServiceUnloaded(`${domain}/${params.label}`))) { + return { status: "failed", reason: "launchctl could not confirm unload" }; + } + const trashDir = path.join(os.homedir(), ".Trash"); try { await fs.mkdir(trashDir, { recursive: true }); @@ -357,16 +416,19 @@ async function cleanupLegacyLaunchdService(params: { try { await fs.access(params.plistPath); - } catch { - return null; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") { + return { status: "removed" }; + } + return { status: "failed", reason: "could not inspect plist" }; } const dest = path.join(trashDir, `${params.label}-${Date.now()}.plist`); try { await fs.rename(params.plistPath, dest); - return dest; + return { status: "removed", destination: dest }; } catch { - return null; + return { status: "failed", reason: "could not move plist" }; } } @@ -416,11 +478,15 @@ async function cleanupLegacyDarwinServices( failed.push(`${svc.label} (missing plist path)`); continue; } - const dest = await cleanupLegacyLaunchdService({ + const result = await cleanupLegacyLaunchdService({ label: svc.label, plistPath, }); - removed.push(dest ? `${svc.label} -> ${dest}` : svc.label); + if (result.status === "removed") { + removed.push(result.destination ? `${svc.label} -> ${result.destination}` : svc.label); + } else { + failed.push(`${svc.label} (${result.reason})`); + } } return { removed, failed }; diff --git a/src/daemon/launchd.ts b/src/daemon/launchd.ts index 0209226c14b7..03fad7ae124e 100644 --- a/src/daemon/launchd.ts +++ b/src/daemon/launchd.ts @@ -854,7 +854,7 @@ export async function uninstallLaunchAgent({ } } -function isLaunchctlNotLoaded(res: { stdout: string; stderr: string; code: number }): boolean { +export function isLaunchctlNotLoaded(res: { stdout: string; stderr: string }): boolean { const detail = normalizeLowercaseStringOrEmpty(res.stderr || res.stdout); return ( detail.includes("no such process") ||