diff --git a/scripts/test-layout/layout.json b/scripts/test-layout/layout.json index d9502640531..96d87979ec5 100644 --- a/scripts/test-layout/layout.json +++ b/scripts/test-layout/layout.json @@ -1680,6 +1680,7 @@ "service-auth-qualified-localhost.test.ts": "service", "service-child-ownership.test.ts": "service", "service-handled-signal-exit.test.ts": "service", "service-health-sentinel-linux.test.ts": "service", "service-health-sentinel-launch.test.ts": "service", "service-health-sentinel-policy.test.ts": "service", "service-health-sentinel-process.test.ts": "service", "service-health-sentinel-runner.test.ts": "service", + "service-install-wrapper-cleanup.test.ts": "service", "service-claim.test.ts": "service", "service-ownership-compatibility.test.ts": "service", "service-ownership-handover.test.ts": "service", diff --git a/src/service/orchestration.ts b/src/service/orchestration.ts index 6ca03ba1372..6a10b2a8325 100644 --- a/src/service/orchestration.ts +++ b/src/service/orchestration.ts @@ -238,6 +238,11 @@ export interface ServiceInstallPreparationDeps extends RuntimePreflightDeps { supervisionLatch?: SupervisionLatch; diagnose?: () => ServiceDiagnostic; managerOps?: (backend: ServiceBackend) => ServiceInstallCleanupOps | null; + /** + * Task Scheduler only: ends surviving launcher/wrapper processes of THIS + * installation. Scoped to canonical script/launcher tokens, never a bare PID. + */ + killServiceWrappers?: () => void; stopTrackedProxy?: () => Promise; platform?: NodeJS.Platform; } @@ -279,6 +284,18 @@ export async function prepareServiceInstall( if (!manager) throw new Error(`Background service manager is unavailable for ${backend}.`); if (manager.status() !== null) manager.stop(); } + if (platform === "win32") { + // `schtasks /end` ends the task instance but leaves the cmd `:loop` wrapper + // alive to respawn the proxy mid-install — the same survivor + // `ocx stop` kills via killWindowsServiceWrapperProcesses. Run it + // after every manager has been asked to stop and before the tracked-proxy + // kill so a child respawned in that gap is still caught below. It also runs + // when no manager stop happened: a registration deleted mid-uninstall can + // leave the detached wrapper looping. A manager stop that refused propagates + // above before this runs, so the respawner of a still-running service is + // never killed. + (deps.killServiceWrappers ?? killWindowsServiceWrapperProcesses)(); + } await (deps.stopTrackedProxy ?? stopTrackedProxyIfRunning)(); } diff --git a/structure/ops/service-and-sidecars.md b/structure/ops/service-and-sidecars.md index 5127a5452a8..e82d02ff5f4 100644 --- a/structure/ops/service-and-sidecars.md +++ b/structure/ops/service-and-sidecars.md @@ -95,7 +95,7 @@ Windows, claimed, supervised and uncertain targets remain ineligible for this up Before service install/repair or a Windows Codex shim mutation, the selected Bun executable must create and remove an exclusive nonce-named directory inside the config root: admission runs that exact lexical path with `-e`, no shell, a hidden window and a five-second timeout, and only a matching nonce acknowledgment admits. Failure refuses with `OCX_RUNTIME_PREFLIGHT_FAILED` (`spawn`, `timeout`, `create`, `remove` or `protocol`) and never discovers another runtime; the Node launcher may pick a validated PATH Bun before CLI startup when bundled Bun is unusable, and durable admission only probes that resulting selection. The frozen selection feeds scheduler rendering, the WinSW entry and the install-state writer. `installServiceSafely` refuses live Desktop supervision before selection or admission, then admits once before cleanup and uses internal commit functions; repair refuses Desktop before diagnosis, checks ownership and auth before admission, and admits before native repair or scheduler stop; direct public installers keep their own admission gate. -The fresh scheduler path captures root absence and freezes the runtime at entry, stages and registers its definition, removes staging and claims config ownership, then probes with `rootWasAbsent: false` immediately before `prepare()`; a refused probe rolls back the new registration before service-manager cleanup or asset/state publication. Healthy or disabled shims do not probe; refused automatic restore defers with guidance and startup continues. Non-Windows admission performs no spawn or filesystem work, selection stays pre-dotenv and `cliEntry` stays I/O-free. +The fresh scheduler path captures root absence and freezes the runtime at entry, stages and registers its definition, removes staging and claims config ownership, then probes with `rootWasAbsent: false` immediately before `prepare()`; a refused probe rolls back the new registration before service-manager cleanup or asset/state publication. Healthy or disabled shims do not probe; refused automatic restore defers with guidance and startup continues. Non-Windows admission performs no spawn or filesystem work, selection stays pre-dotenv and `cliEntry` stays I/O-free. That cleanup (`prepareServiceInstall`) ends every recorded, conflicting and requested manager, then — Windows only — issues the same best-effort launcher/wrapper termination `ocx stop` performs, scoped to this install's canonical script and launcher tokens (an `schtasks /end` survivor can respawn the proxy mid-install; this layer guarantees neither that the wrapper actually exited nor that the new instance comes up), then stops the tracked standalone listener; a refused or unverifiable manager stop aborts before that cleanup runs. An absent root reaching admission is created with an exclusive non-recursive mode-0700 mkdir under an existing parent and left empty for later ownership claiming; the preflight never deletes the config root (refusal, concurrent creation, contents and replacements are all preserved). Existing roots must be real directories, not files or symlinks/junctions, and probe cleanup is non-recursive. A standalone selection is admitted in-process only when its path equals `process.execPath` exactly (standalone is executable packaging, not Desktop ownership); that path has neither child isolation nor a timeout, so synchronous filesystem operations may block, and other standalone selections refuse. Recovery is an operator-selected trusted `OPENCODEX_BUN_PATH` before launching ocx, or an `npm install -g @bitkyc08/opencodex` reinstall followed by retry; no runtime discovery or probe memo is added. diff --git a/tests/fixtures/test-layout-expected.json b/tests/fixtures/test-layout-expected.json index 3a82672e452..9a0ebbc388c 100644 --- a/tests/fixtures/test-layout-expected.json +++ b/tests/fixtures/test-layout-expected.json @@ -1154,6 +1154,7 @@ "server-xai-oauth-401-replay.test.ts": "server", "server-xai-responses-streaming.test.ts": "server", "service-auth-qualified-localhost.test.ts": "service", "service-child-ownership.test.ts": "service", "service-handled-signal-exit.test.ts": "service", "service-claim.test.ts": "service", + "service-install-wrapper-cleanup.test.ts": "service", "service-ownership-compatibility.test.ts": "service", "service-ownership-handover.test.ts": "service", "service-ownership-state.test.ts": "service", "ownership-mutation-lease-diagnostics.test.ts": "service", "service-probe-docker.test.ts": "service", "service-secrets.test.ts": "service", "service-sqlite-home.test.ts": "service", diff --git a/tests/service/service-desktop-runtime-preflight.test.ts b/tests/service/service-desktop-runtime-preflight.test.ts index b3065bf13cd..e60e6f75c2a 100644 --- a/tests/service/service-desktop-runtime-preflight.test.ts +++ b/tests/service/service-desktop-runtime-preflight.test.ts @@ -59,6 +59,7 @@ function fixture(supervised: boolean, passes: boolean) { ...admission, inspectSupervision, diagnose: () => { order.push("diagnose"); return diagnostic; }, managerOps: () => ({ status: () => "present", stop: () => { order.push("stop"); } }), + killServiceWrappers: () => { order.push("wrappers"); }, stopTrackedProxy: async () => { order.push("proxy"); }, }); if (action === "fresh install") return installFreshWindowsSchedulerSafely({ diff --git a/tests/service/service-install-wrapper-cleanup.test.ts b/tests/service/service-install-wrapper-cleanup.test.ts new file mode 100644 index 00000000000..0b4dab028fa --- /dev/null +++ b/tests/service/service-install-wrapper-cleanup.test.ts @@ -0,0 +1,94 @@ +import { describe, expect, test } from "bun:test"; +import { prepareServiceInstall } from "../../src/service"; + +describe("service install preparation wrapper cleanup", () => { + test("install preparation kills surviving scheduler wrappers before the tracked proxy stop", async () => { + // The defect: `schtasks /end` ends the task instance but the detached cmd + // `:loop` wrapper survives and respawns its child seconds later — landing + // after the one-shot tracked-proxy kill and holding the install port. Model + // the respawner: while the wrapper lives, a stopped proxy comes back. + const calls: string[] = []; + let wrapperAlive = true; + let respawned = false; + await prepareServiceInstall("scheduler", { + platform: "win32", + diagnose: () => ({ supported: true, installed: true, enabled: true, running: true, viable: true, startable: true, stale: false, conflict: false, backend: "scheduler", summary: "test" }), + managerOps: () => ({ + status: () => "present", + // `schtasks /end` ends the task instance; it cannot reach the detached wrapper. + stop: () => { calls.push("stop:scheduler"); }, + }), + killServiceWrappers: () => { calls.push("kill:wrappers"); wrapperAlive = false; }, + stopTrackedProxy: async () => { + calls.push("stop:standalone"); + if (wrapperAlive) respawned = true; + }, + }); + expect(calls).toEqual(["stop:scheduler", "kill:wrappers", "stop:standalone"]); + expect(respawned).toBe(false); + }); + + test("install preparation kills stray scheduler wrappers even when no manager stop runs", async () => { + // A registration deleted mid-uninstall leaves no task for `schtasks /end`, but + // the detached wrapper may still be looping: it must not respawn the old proxy + // onto the port the fresh install is about to claim. + let killed = false; + const calls: string[] = []; + await prepareServiceInstall("scheduler", { + platform: "win32", + diagnose: () => ({ supported: true, installed: false, enabled: false, running: false, viable: false, startable: false, stale: false, conflict: false, backend: null, summary: "test" }), + managerOps: () => ({ + status: () => null, + stop: () => { calls.push("stop:scheduler"); throw new Error("no task to stop"); }, + }), + killServiceWrappers: () => { calls.push("kill:wrappers"); killed = true; }, + stopTrackedProxy: async () => { calls.push("stop:standalone"); }, + }); + expect(killed).toBe(true); + expect(calls).toEqual(["kill:wrappers", "stop:standalone"]); + }); + + test("a manager that refuses to stop aborts install preparation before the wrapper kill", async () => { + // Propagating the refusal is the fail-closed contract — and killing the wrapper + // of a service whose task is still running would break that surviving service. + const calls: string[] = []; + await expect(prepareServiceInstall("scheduler", { + platform: "win32", + diagnose: () => ({ supported: true, installed: true, enabled: true, running: true, viable: true, startable: true, stale: false, conflict: false, backend: "scheduler", summary: "test" }), + managerOps: () => ({ + status: () => "present", + stop: () => { calls.push("stop:scheduler"); throw new Error("end refused"); }, + }), + killServiceWrappers: () => { calls.push("kill:wrappers"); }, + stopTrackedProxy: async () => { calls.push("stop:standalone"); }, + })).rejects.toThrow("end refused"); + expect(calls).toEqual(["stop:scheduler"]); + }); + + test("non-Windows install preparation never invokes the scheduler wrapper kill", async () => { + let killed = 0; + await prepareServiceInstall("scheduler", { + platform: "linux", + managerOps: () => ({ status: () => "present", stop: () => {} }), + killServiceWrappers: () => { killed++; }, + stopTrackedProxy: async () => {}, + }); + expect(killed).toBe(0); + }); + + test("live Desktop supervision refuses install preparation before the wrapper kill", async () => { + // The pre-mutation supervision refusal verified here stays ahead of every + // cleanup step: a desktop-owned runtime keeps its respawner until the + // operator quits Desktop and retries. + const calls: string[] = []; + await expect(prepareServiceInstall("scheduler", { + platform: "win32", + inspectSupervision: () => ({ kind: "desktop", runtimePid: 4242, supervisorPid: 4200, app: "opencodex-desktop", proxy: "ocx" }), + diagnose: () => { calls.push("diagnose"); return { supported: true, installed: true, enabled: true, running: true, viable: true, startable: true, stale: false, conflict: false, backend: "scheduler", summary: "test" }; }, + managerOps: () => { calls.push("manager"); return { status: () => "present", stop: () => {} }; }, + killServiceWrappers: () => { calls.push("kill:wrappers"); }, + stopTrackedProxy: async () => { calls.push("stop:standalone"); }, + })).rejects.toThrow(/OpenCodex Desktop supervises the running proxy/); + expect(calls).toEqual([]); + }); +}); diff --git a/tests/service/service-ownership-handover.test.ts b/tests/service/service-ownership-handover.test.ts index 80c66ae76bb..94715cb5081 100644 --- a/tests/service/service-ownership-handover.test.ts +++ b/tests/service/service-ownership-handover.test.ts @@ -214,6 +214,7 @@ describe("live Desktop service command guards", () => { inspectSupervision: () => evidence, diagnose: () => { touched.push("diagnose"); return INSTALLED; }, managerOps: () => { touched.push("manager"); return { status: () => "running", stop: () => { touched.push("stop"); } }; }, + killServiceWrappers: () => { touched.push("wrappers"); }, stopTrackedProxy: async () => { touched.push("tracked"); }, }; await expect(prepareServiceInstall("scheduler", deps)).rejects.toThrow(desktopServiceRefusal(evidence, true)!); diff --git a/tests/service/service-runtime-preflight.test.ts b/tests/service/service-runtime-preflight.test.ts index 7ea60dc5172..6452f0f5064 100644 --- a/tests/service/service-runtime-preflight.test.ts +++ b/tests/service/service-runtime-preflight.test.ts @@ -45,6 +45,7 @@ describe("service selected-runtime admission", () => { await expect(installServiceSafely("scheduler", () => { effects.push("install"); }, { ...deps, diagnose: () => { effects.push("diagnose"); return diagnostic; }, managerOps: () => ({ status: () => "present", stop: () => { effects.push("stop"); } }), + killServiceWrappers: () => {}, stopTrackedProxy: async () => { effects.push("proxy"); }, })).rejects.toMatchObject({ code: "OCX_RUNTIME_PREFLIGHT_FAILED", reason }); const home = join(temporary(), "absent-home"); @@ -90,10 +91,11 @@ describe("service selected-runtime admission", () => { assertRuntimeWritable: admitted => { order.push("preflight"); expect(admitted).toEqual(runtime); selected.path = "unprobed.exe"; }, diagnose: () => diagnostic, managerOps: () => ({ status: () => "present", stop: () => { order.push("stop"); } }), + killServiceWrappers: () => { order.push("wrappers"); }, stopTrackedProxy: async () => { order.push("proxy"); }, }); expect(selections).toBe(1); - expect(order).toEqual(["preflight", "stop", "proxy", "install"]); + expect(order).toEqual(["preflight", "stop", "wrappers", "proxy", "install"]); }); for (const backend of ["scheduler", "native"] as const) { @@ -138,6 +140,7 @@ describe("service selected-runtime admission", () => { const result = installServiceSafely(backend, ops.install, { ...admission, diagnose: () => ({ ...diagnostic, backend }), managerOps: () => ({ status: () => "present", stop: () => { order.push("stop"); } }), + killServiceWrappers: () => { order.push("wrappers"); }, stopTrackedProxy: async () => { order.push("proxy"); }, }); if (refuses) { @@ -146,8 +149,8 @@ describe("service selected-runtime admission", () => { } else { await result; expect(order).toEqual(backend === "scheduler" - ? ["preflight", "stop", "proxy", "ownership", "remove-native", "installer-stop", "assets", "/create", "/run", "state"] - : ["preflight", "stop", "proxy", "account", "native-config", "/query", "winsw", "state"]); + ? ["preflight", "stop", "wrappers", "proxy", "ownership", "remove-native", "installer-stop", "assets", "/create", "/run", "state"] + : ["preflight", "stop", "wrappers", "proxy", "account", "native-config", "/query", "winsw", "state"]); expect(JSON.parse(readFileSync(join(home, "state.json"), "utf8"))).toMatchObject({ backend, bunPath: runtime.path }); } expect(probes).toBe(1); expect(selections).toBe(1); diff --git a/tests/service/service.test.ts b/tests/service/service.test.ts index b61ea1edf20..ceb04a99fc1 100644 --- a/tests/service/service.test.ts +++ b/tests/service/service.test.ts @@ -2283,12 +2283,13 @@ describe("service lifecycle cleanup ordering", () => { platform: "win32", diagnose: () => ({ supported: true, installed: true, enabled: true, running: true, viable: true, startable: true, stale: false, conflict: false, backend: "scheduler", summary: "test" }), managerOps, + killServiceWrappers: () => { calls.push("kill:wrappers"); }, stopTrackedProxy: async () => { calls.push("stop:standalone"); }, }); expect(calls).toEqual([ "status:scheduler", "stop:scheduler", "status:native", "stop:native", - "stop:standalone", "install:native", + "kill:wrappers", "stop:standalone", "install:native", ]); }); @@ -2396,6 +2397,7 @@ describe("service lifecycle cleanup ordering", () => { if (failure === "stop") throw new Error("stop failed"); }, }), + killServiceWrappers: () => {}, stopTrackedProxy: async () => { if (failure === "standalone") throw new Error("standalone failed"); }, @@ -2411,9 +2413,10 @@ describe("service lifecycle cleanup ordering", () => { platform: "win32", diagnose: () => ({ supported: true, installed: true, enabled: true, running: true, viable: false, startable: false, stale: false, conflict: true, backend: "scheduler", summary: "test" }), managerOps: backend => ({ status: () => "present", stop: () => { stopped.push(backend); } }), + killServiceWrappers: () => { stopped.push("wrappers"); }, stopTrackedProxy: async () => {}, }); - expect(stopped).toEqual(["scheduler", "native"]); + expect(stopped).toEqual(["scheduler", "native", "wrappers"]); }); test("direct service stop kills the tracked proxy before restoring native Codex", async () => {