From ae057c4211c39673eb40183318deb0fa31bdd649 Mon Sep 17 00:00:00 2001 From: JUN Date: Thu, 10 Sep 2026 08:54:17 +0900 Subject: [PATCH] fix(service): recover a stale launchd job with bootout before load After `ocx update` the service never came back. Update stops the service, replaces the binary, then runs `ocx service repair`, which on darwin is `installLaunchd`. That function best-effort `unload`ed the plist, ran `load -w`, and threw on any stderr matching Load failed or Bootstrap failed. `unload` is the legacy verb and it does not evict a job bootstrapped into the GUI domain. That is exactly the state modern launchd reports by writing "Load failed: 5: Input/output error" to stderr AND exiting 0, so a live-but-stale job was precisely the case that could not repair itself. The thrown text carried the `launchctl bootout` recipe as a hint that nothing ever executed. Evict with `bootout` instead, and if `load -w` still reports the job as bootstrapped, bootout once more and retry the load a single time before keeping the existing throw. `startLaunchd` already handles the same stderr correctly by asking whether the live job matches the current plist; repair does not go through it. This kills the live gui job. That is the repair the issue asks for, and it is also why the previous code only printed the command. Two things bound it: it runs only inside `installLaunchd`, which is already the "put the job back" path and has just rewritten the plist, so whatever is loaded is stale by construction; and it fires only after `load -w` has already failed, so a healthy job that loads cleanly is evicted once and reloaded, never retried. `ocx service start` is untouched and still refuses to evict anything. `launchctlLoadFailed` is deliberately unchanged. That regex is the 2026-08-02 silent-success guard; the fix is to recover from the condition, not to stop detecting it. The retry is scoped to that signal rather than to a non-zero exit, so a malformed plist surfaces its real stderr immediately instead of being retried pointlessly. `installLaunchd` gains the same all-optional `launchctl` injection seam `startLaunchd` has. This is not just for convenience: the live-service-manager guard added in #4152 refuses every mutating verb from an armed test process and `bootout` is not on its read-only list, so without the seam the regression tests would fail closed on the guard instead of exercising the sequence. No `matches` dep, because unlike `startLaunchd` this function never consults `launchdJobMatchesPlist`. The throw's hint no longer tells the operator to run `launchctl bootout` by hand, since the code now runs it twice. It reports what was attempted and points at `launchctl print` instead. `stopLaunchd`, `statusLaunchd` and `uninstallLaunchd` keep legacy `unload`: once install boots out before loading, changing them is not required and each has a test pinning its exact string. Closes #4141. --- src/service.ts | 54 +++++++++++-- tests/service/service.test.ts | 139 +++++++++++++++++++++++++++++++++- 2 files changed, 185 insertions(+), 8 deletions(-) diff --git a/src/service.ts b/src/service.ts index d2ab05299d..15a8876ba3 100644 --- a/src/service.ts +++ b/src/service.ts @@ -2338,7 +2338,23 @@ export function readWindowsSchedulerXmlState( } // ── macOS (launchd) ── -function installLaunchd(): void { +/** + * Deps follow {@link startLaunchd}: `launchctl` replaces the LAYER, returning a + * {@link runLaunchctl} result, not a spawnSync result. It is optional so this stays + * assignable to `ServiceOps.install` and `RepairServiceDeps.repairLaunchd` + * (`() => void`), and so `platformOps` wires the same function the tests exercise. + * + * The seam is what makes the eviction below testable at all. The live-service-manager + * guard refuses every mutating verb from an armed test process and `bootout` is not on + * its read-only list, so a test reaching the real runner would fail closed on the guard + * instead of exercising the sequence. + * + * No `matches` dep: unlike `startLaunchd`, this function never consults + * {@link launchdJobMatchesPlist}. It has just rewritten the plist, so a live job is stale + * by construction and there is nothing to compare against. + */ +export function installLaunchd(deps: { launchctl?: typeof runLaunchctl } = {}): void { + const run = deps.launchctl ?? runLaunchctl; const dir = join(homedir(), "Library", "LaunchAgents"); if (!existsSync(dir)) mkdirSync(dir, { recursive: true }); recordOwnedConfigPath(getConfigDir(), serviceStatePath()); @@ -2352,17 +2368,41 @@ function installLaunchd(): void { // so the staleness diagnostic judges exactly what launchd runs. const launcher = stableLauncherEntry(); writeServiceDefinitionFile(p, buildPlist(resolvedProxyEnv(), { launcher }), "utf8"); - // Best-effort: an absent job is fine here, and a failed unload is caught by the - // load verification below with a better message than a raw unload error. - runLaunchctl(["unload", p]); - const loaded = runLaunchctl(["load", "-w", p]); + // `unload` is the legacy verb and it does not evict a job bootstrapped into the GUI + // domain — which is precisely the state that could not repair itself. Modern launchd + // answers `load -w` for an already-bootstrapped job with "Load failed: 5: + // Input/output error" AND exits 0, so `ocx update` replaced the binary, ran repair, + // and left launchd running the PREVIOUS job while the fresh plist sat unused (#4141). + // + // This EVICTS the running job. That is the repair being asked for, and it is why it + // lives here and nowhere else: `installLaunchd` has already rewritten the plist, so + // whatever is loaded is stale by construction. `ocx service start` must never do + // this, and `startLaunchd` accordingly still refuses to. + // + // Absence is fine: booting out a job that is not there is a no-op, and a real failure + // is reported by the load verification below with a better message than a raw + // eviction error would carry. + const bootoutTarget = `${launchdGuiDomain()}/${LABEL}`; + run(["bootout", bootoutTarget]); + let loaded = run(["load", "-w", p]); + if (launchctlLoadFailed(loaded.stderr)) { + // Still bootstrapped after an eviction: the job re-registered between the two calls, + // or the first `bootout` raced a job that had not finished exiting. Evict and load + // once more — ONCE. A bounded retry recovers the race; a loop would turn a genuinely + // wedged domain into a hang instead of the diagnosable throw below. + run(["bootout", bootoutTarget]); + loaded = run(["load", "-w", p]); + } if (!loaded.ok || launchctlLoadFailed(loaded.stderr)) { // Do NOT write install state for a load that did not take: state describing an // unused plist is what made this failure invisible. throw new Error( `launchctl could not load ${p}: ${loaded.stderr || "load reported failure"}\n` - + "A previous job may still be bootstrapped. Try:\n" - + ` launchctl bootout ${launchdGuiDomain()}/${LABEL}\n` + // The hint used to tell the operator to run `bootout` by hand. It now runs twice + // above, so naming it as an untried remedy would send someone to repeat what just + // failed. Report what was attempted instead. + + `A previous job is still bootstrapped after two attempts to boot it out of ${launchdGuiDomain()}.\n` + + `Inspect it with:\n launchctl print ${bootoutTarget}\n` // macOS `service repair` delegates straight to installLaunchd, so this fires for // an already-installed service too; repair reloads it without re-registering. + `then re-run '${wasInstalled ? "ocx service repair" : "ocx service install"}'.`, diff --git a/tests/service/service.test.ts b/tests/service/service.test.ts index 2eadf7f4fc..e6362201c8 100644 --- a/tests/service/service.test.ts +++ b/tests/service/service.test.ts @@ -9,7 +9,7 @@ import { saveConfig } from "../../src/config"; import { windowsEnvIndirectBatchValue } from "../../src/lib/win-paths"; import { assertServiceAuthEnvironment, assertServiceEnvironmentMatchesInstall, bakedServicePathsDiagnostic, confirmServiceServing, launchdListenPort, systemdListenPort, buildPlist, buildUnit, buildWindowsLauncherVbs, buildWindowsSchtasksCreateArgs, buildWindowsSchtasksCreateArgsForXml, buildWindowsServiceScript, buildWindowsTaskXml as buildWindowsTaskXmlProduction, buildWindowsTaskXmlDocument, deriveWindowsServiceDiagnostic, deriveWindowsServiceDiagnosticForCurrentUser, expectedLaunchdCommand, installFreshWindowsSchedulerSafely, installServiceSafely, launchctlLoadFailed, launchdJobMatchesPlist, normalizeServiceSubcommand, parseServiceArgs, parseServiceInstallState, planServiceCommand, prepareServiceInstall, probeServiceInstallation, readWindowsSchedulerXmlState, registerFreshWindowsSchedulerTask, removeNativeWindowsServiceForScheduler, repairService, reportServiceServing, resolveServiceListenPort, runLaunchctl, selectServiceSubcommand, SERVICE_INSTALL_HEALTH_MS, SERVICE_INSTALL_HEALTH_WINDOWS_MS, serviceInstallHealthMs, serviceLogPath, serviceStartableFromTray, serviceStatusReport, serviceRetryCommand, serviceStatusSummary, stableLauncherEntry, systemdNeedsDaemonReload, systemdServiceInstallCleanupOps, uninstallSystemd, windowsListenPort, winswListenPort, startLaunchd, windowsTaskRegistrationHealthy as windowsTaskRegistrationHealthyProduction } from "../../src/service"; import type { ServiceDiagnostic } from "../../src/service"; -import { definitionCarriesCredential, resolvedProxyEnv, writeServiceDefinitionFile } from "../../src/service"; +import { definitionCarriesCredential, installLaunchd, resolvedProxyEnv, writeServiceDefinitionFile } from "../../src/service"; import { buildWinswXml } from "../../src/lib/winsw"; import { CONFIG_OWNER_FILE, CONFIG_UNINSTALL_MANIFEST, recordOwnedConfigPath, removeOwnedConfigState } from "../../src/lib/config-ownership"; import { serviceApiTokenFilePath } from "../../src/lib/service-secrets"; @@ -3326,6 +3326,143 @@ describe("launchctl load verification", () => { })).toThrow(/service repair/); }); }); + + /** + * #4141: after `ocx update` the job never came back. Repair on darwin is + * `installLaunchd`, which best-effort `unload`ed the plist and then threw on any + * `Load failed`. But `unload` does not evict a job bootstrapped into the GUI domain, + * and that is exactly the state modern launchd reports with "Load failed: 5: + * Input/output error" while exiting 0 — so a live-but-stale job was precisely the case + * that could not repair itself, and the thrown text carried the `bootout` recipe as a + * hint that nothing ever ran. + * + * These drive the injected seam. They never reach launchd: the live-service-manager + * guard refuses `bootout` from an armed test process, so a test on the real runner + * would fail closed on the guard rather than exercise anything. + */ + describe("installLaunchd", () => { + // A runLaunchctl RESULT, not a spawnSync result. + function recordingLaunchctl(loadResults: Array<{ ok: boolean; stderr: string }>) { + const argv: string[][] = []; + let loads = 0; + const launchctl = ((args: string[]) => { + argv.push([...args]); + if (args[0] !== "load") return { ok: true, stdout: "", stderr: "", status: 0 }; + // Exhausting the queue is a fixture bug, not a passing case. Defaulting a missing + // entry to success once hid a retry that never got the failure it was meant to + // assert, so make the fixture state its own call count or fail loudly. + const next = loadResults[loads++]; + if (!next) throw new Error(`unexpected load #${loads}: the fixture queued ${loadResults.length}`); + return { ok: next.ok, stdout: "", stderr: next.stderr, status: next.ok ? 0 : 1 }; + }) as typeof runLaunchctl; + return { argv, launchctl }; + } + + const BOOTSTRAPPED = "Load failed: 5: Input/output error"; + + /** + * installLaunchd writes the plist under `homedir()/Library/LaunchAgents`. The preload + * already sandboxes HOME; pinning a fresh one per case keeps these from writing into + * whatever another test left there. + */ + function withLaunchAgentHome(run: () => void): void { + const previousHome = process.env.HOME; + const previousUserProfile = process.env.USERPROFILE; + const dir = mkdtempSync(join(tmpdir(), "ocx-launchd-install-")); + process.env.HOME = dir; + process.env.USERPROFILE = dir; + try { + run(); + } finally { + if (previousHome === undefined) delete process.env.HOME; + else process.env.HOME = previousHome; + if (previousUserProfile === undefined) delete process.env.USERPROFILE; + else process.env.USERPROFILE = previousUserProfile; + } + } + + const verbs = (argv: string[][]): string[] => argv.map(args => args[0] ?? ""); + + test("evicts a stale job, and recovers on the retried load", () => { + const { argv, launchctl } = recordingLaunchctl([ + { ok: true, stderr: BOOTSTRAPPED }, + { ok: true, stderr: "" }, + ]); + + withLaunchAgentHome(() => { + expect(() => installLaunchd({ launchctl })).not.toThrow(); + }); + + // The whole repair, in order. Red before the fix: it threw on the first + // `Load failed` without ever running `bootout`. + expect(verbs(argv)).toEqual(["bootout", "load", "bootout", "load"]); + // The legacy verb is gone: it is what failed to evict the job in the first place. + expect(verbs(argv)).not.toContain("unload"); + expect(argv[0]?.[1]).toMatch(/^gui\/\d+\/com\.opencodex\.proxy$/); + expect(argv[2]?.[1]).toBe(argv[0]?.[1]); + expect(argv[1]?.slice(0, 2)).toEqual(["load", "-w"]); + expect(argv[3]?.slice(0, 2)).toEqual(["load", "-w"]); + }); + + test("a clean load is never retried, so a healthy job is evicted once and reloaded", () => { + const { argv, launchctl } = recordingLaunchctl([{ ok: true, stderr: "" }]); + + withLaunchAgentHome(() => { + expect(() => installLaunchd({ launchctl })).not.toThrow(); + }); + + expect(verbs(argv)).toEqual(["bootout", "load"]); + }); + + test("still throws when the job survives both evictions", () => { + const { argv, launchctl } = recordingLaunchctl([ + { ok: true, stderr: BOOTSTRAPPED }, + { ok: true, stderr: BOOTSTRAPPED }, + ]); + + withLaunchAgentHome(() => { + expect(() => installLaunchd({ launchctl })).toThrow(/could not load/); + }); + + // Bounded: two evictions and two loads, then the diagnosable throw. A loop here + // would turn a wedged domain into a hang. + expect(verbs(argv)).toEqual(["bootout", "load", "bootout", "load"]); + }); + + /** + * `launchctlLoadFailed` matches "Bootstrap failed" as well as "Load failed", and both + * mean the same thing here: something is still bootstrapped in the domain. So this + * retries, and the regex itself is left alone — it is the 2026-08-02 silent-success + * guard, and the fix is to recover from the condition rather than stop detecting it. + */ + test("a Bootstrap failed load takes the same eviction and retry", () => { + const bootstrapFailed = { ok: false, stderr: "Bootstrap failed: 37: Operation already in progress" }; + const { argv, launchctl } = recordingLaunchctl([bootstrapFailed, bootstrapFailed]); + + withLaunchAgentHome(() => { + expect(() => installLaunchd({ launchctl })).toThrow(/could not load/); + }); + + expect(verbs(argv)).toEqual(["bootout", "load", "bootout", "load"]); + }); + + /** + * The retry is scoped to that signal on purpose. A load that fails for another + * reason — a malformed plist, say — is not fixed by evicting a job, so retrying + * would only delay the real stderr reaching the operator. + */ + test("a plain non-zero load with unrelated stderr throws without a second eviction", () => { + const { argv, launchctl } = recordingLaunchctl([ + { ok: false, stderr: "Could not read plist: invalid XML" }, + ]); + + withLaunchAgentHome(() => { + expect(() => installLaunchd({ launchctl })).toThrow(/invalid XML/); + }); + + expect(verbs(argv)).toEqual(["bootout", "load"]); + }); + }); }); /**