diff --git a/src/lib/winsw.ts b/src/lib/winsw.ts index 5c00e47af..42a5f9553 100644 --- a/src/lib/winsw.ts +++ b/src/lib/winsw.ts @@ -165,6 +165,12 @@ function runWinsw(args: string[]): string { /** `install /p` prompts for the service-account password on the console — stdin must be inherited. */ function runWinswInteractive(args: string[]): void { + if (!process.stdin.isTTY) { + throw new Error( + "WinSW install requires an interactive console to prompt for the service account password. " + + "Run `ocx service install --native` from an elevated Command Prompt or PowerShell window, not a hidden or piped session.", + ); + } execFileSync(winswExePath(), args, { stdio: "inherit" }); } diff --git a/src/server/proxy-liveness.ts b/src/server/proxy-liveness.ts index 0ed7e6bbf..913afb057 100644 --- a/src/server/proxy-liveness.ts +++ b/src/server/proxy-liveness.ts @@ -109,6 +109,13 @@ export async function findLiveProxy(io: LivenessIo = {}): Promise { + if (reported === null) return null; + if (!Number.isSafeInteger(reported) || reported <= 0) return null; + const verified = verifyPidFn(reported); + return verified === reported ? verified : null; + }; + const pid = readPidFn(); let probedPort: number | null = null; if (pid) { @@ -136,7 +143,7 @@ export async function findLiveProxy(io: LivenessIo = {}): Promise * scheduler backend first; on failure the machine is left with NO service (explicitly * reported) — never a silent fallback to the scheduler. */ +/** Refuse WinSW when the interactive user is a Microsoft account (SCM cannot authenticate it). */ +export function assertWindowsNativeServiceAccountSupported(): void { + if (process.platform !== "win32") return; + const source = readWindowsPrincipalSource(); + if (source?.toLowerCase() === "microsoftaccount") { + throw new Error( + "The native (WinSW) service backend cannot run under a Microsoft-account Windows login. " + + "Keep the Task Scheduler backend (`ocx service install`) or sign in with a local/domain account before `ocx service install --native`.", + ); + } +} + +function readWindowsPrincipalSource(): string | null { + if (process.platform !== "win32") return null; + const ps = join(process.env.SystemRoot ?? "C:\\Windows", "System32", "WindowsPowerShell", "v1.0", "powershell.exe"); + if (!existsSync(ps)) return null; + try { + const out = execFileSync(ps, [ + "-NoLogo", + "-NoProfile", + "-NonInteractive", + "-Command", + "(Get-LocalUser -Name $env:USERNAME -ErrorAction SilentlyContinue).PrincipalSource", + ], { encoding: "utf8", stdio: ["ignore", "pipe", "pipe"], windowsHide: true }).trim(); + return out || null; + } catch { + return null; + } +} + async function installWindowsNative(): Promise { + assertWindowsNativeServiceAccountSupported(); recordOwnedConfigPath(getConfigDir(), serviceStatePath()); if (!existsSync(getConfigDir())) mkdirSync(getConfigDir(), { recursive: true }); writeServiceApiTokenFile(); @@ -1463,11 +1494,49 @@ async function installWindowsNative(): Promise { writeServiceInstallState("native"); } function startWindows(): void { schtasks(["/run", "/tn", TASK]); } -function stopWindows(): void { try { schtasks(["/end", "/tn", TASK]); } catch { /* not running */ } } + +export function isWindowsSchedulerEndBenign(error: unknown): boolean { + const detail = schtasksErrorDetail(error).toLowerCase(); + return detail.includes("no running instance") + || detail.includes("not currently running") + || detail.includes("0x41330"); +} + +/** + * End the scheduler task. "Already stopped" is success; other `/end` failures are + * swallowed so callers can still run tracked-proxy + live-proxy cleanup. + * + * Do not key a restart-window wait on `/end` failure: the #764 case is an `/end` + * that *succeeds* while the wrapper survives and respawns. That verification lives + * on the stop-verification path (poll across the restart window), not here. + */ +export function stopWindows(): void { + try { + schtasks(["/end", "/tn", TASK]); + } catch (error) { + if (isWindowsSchedulerEndBenign(error)) return; + } +} function statusWindows(): string { try { return schtasks(["/query", "/tn", TASK]); } catch { return ""; } } function statusWindowsXml(): string { try { return schtasks(["/query", "/tn", TASK, "/xml"]); } catch { return ""; } } function uninstallWindows(): void { - try { schtasks(["/delete", "/tn", TASK, "/f"]); } catch { /* absent */ } + const probe = probeWindowsSchedulerTask(TASK); + if (probe.status === "present") { + try { + schtasks(["/delete", "/tn", TASK, "/f"]); + } catch (error) { + throw new Error(`Failed to delete Task Scheduler task ${TASK}: ${error instanceof Error ? error.message : String(error)}`); + } + const afterDelete = probeWindowsSchedulerTask(TASK); + if (afterDelete.status === "present") { + throw new Error(`Task Scheduler task ${TASK} is still present after delete — refusing to remove service assets. Retry from an elevated shell.`); + } + if (afterDelete.status === "unknown") { + throw new Error(`Task Scheduler task ${TASK} presence could not be verified after delete — refusing to remove service assets.`); + } + } else if (probe.status === "unknown") { + throw new Error(`Task Scheduler task ${TASK} presence could not be verified — refusing to remove service assets.`); + } if (existsSync(windowsServiceScriptPath())) unlinkSync(windowsServiceScriptPath()); if (existsSync(windowsLauncherVbsPath())) unlinkSync(windowsLauncherVbsPath()); if (existsSync(windowsTaskXmlPath())) unlinkSync(windowsTaskXmlPath()); @@ -1626,6 +1695,12 @@ function platformOps(backend: ServiceBackend = "scheduler"): ServiceOps | null { type TrackedProxyCleanupResult = "none" | "stale" | "stopped"; +function verifiedKillTarget(pid: number | null | undefined): number | null { + if (typeof pid !== "number" || !Number.isSafeInteger(pid) || pid <= 0) return null; + const verified = verifyPidIdentity(pid); + return verified === pid ? verified : null; +} + /** * Whether a proxy is still answering after the service manager claimed to stop it. * @@ -1666,17 +1741,31 @@ export async function proxyStillLiveAfterStop(deps: { } async function stopTrackedProxyIfRunning(): Promise { + let stopped = false; const pid = readPid(); - if (!pid) return "none"; - if (!isProcessAlive(pid)) { + const trackedKillPid = verifiedKillTarget(pid); + if (trackedKillPid !== null && isProcessAlive(trackedKillPid)) { + await stopProxy(trackedKillPid); + removePid(trackedKillPid); + removeRuntimePort(trackedKillPid); + stopped = true; + } else if (pid) { removePid(pid); removeRuntimePort(pid); - return "stale"; } - await stopProxy(pid); - removePid(pid); - removeRuntimePort(pid); - return "stopped"; + // Orphan recovery: the pid file can be missing/stale while the service wrapper keeps + // a live proxy running — mirror `ocx stop`'s identity-checked findLiveProxy fallback. + const live = await findLiveProxy({ timeoutMs: 1500 }); + const liveKillPid = verifiedKillTarget(live?.pid); + if (liveKillPid !== null) { + await stopProxy(liveKillPid); + removePid(liveKillPid); + removeRuntimePort(liveKillPid); + stopped = true; + } + if (stopped) return "stopped"; + if (pid) return "stale"; + return "none"; } async function stopTrackedProxyForServiceCommand(): Promise { @@ -1984,11 +2073,13 @@ export async function serviceCommand(...args: (string | undefined)[]): Promise { readPidFn: () => null, readRuntimeFn: () => null, configFn: () => ({ port: 10100 }), + verifyPidFn: candidate => candidate, fetchFn: (async () => healthz(OURS)) as typeof fetch, }); @@ -98,6 +99,7 @@ describe("findLiveProxy", () => { readPidFn: () => null, readRuntimeFn: () => ({ pid: 4242, port: 58195, hostname: "::1" }), configFn: () => ({ port: 10100 }), + verifyPidFn: candidate => candidate, fetchFn: (async (url: string | URL | Request) => { urls.push(String(url)); return healthz(OURS); @@ -138,14 +140,14 @@ describe("findLiveProxy", () => { test("a runtime record whose healthz reports a different pid is rejected", async () => { const live = await findLiveProxy({ readPidFn: () => 1111, + verifyPidFn: () => null, readRuntimeFn: () => ({ port: 58195 }), configFn: () => ({ port: 58195 }), fetchFn: (async () => healthz({ ...OURS, pid: 9999 })) as typeof fetch, }); - // The runtime probe fails the pid check; the config fallback probes the same port - // without a pid expectation and adopts the reported live pid instead. - expect(live).toEqual({ pid: 9999, port: 58195, source: "config" }); + // healthz-reported pids must pass identity verification before they become kill targets. + expect(live).toEqual({ pid: null, port: 58195, source: "config" }); }); test("a pidless legacy healthz never promotes an unverified cheap pid to a kill target", async () => { diff --git a/tests/service.test.ts b/tests/service.test.ts index 9bdb6d0fc..382e9c23f 100644 --- a/tests/service.test.ts +++ b/tests/service.test.ts @@ -644,31 +644,67 @@ describe("service lifecycle cleanup ordering", () => { expect(service).toContain('code !== "EBUSY" && code !== "EPERM" && code !== "EACCES"'); }); - test("Windows service uninstall removes generated task XML", async () => { + test("Windows service uninstall verifies task deletion before removing assets", async () => { const service = await readText("src/service.ts"); const uninstallWindows = service.slice(service.indexOf("function uninstallWindows()"), service.indexOf("function serviceDiagnosticsSummary()")); + expect(uninstallWindows).toContain("probeWindowsSchedulerTask(TASK)"); expect(uninstallWindows).toContain("windowsServiceScriptPath()"); expect(uninstallWindows).toContain("windowsTaskXmlPath()"); expect(uninstallWindows).toContain("unlinkSync(windowsTaskXmlPath())"); + expect(uninstallWindows).toContain("refusing to remove service assets"); }); - test("service cleanup stops gracefully first via the shared stopper and clears the pid file", async () => { + test("service cleanup falls back to findLiveProxy and clears the pid file", async () => { const service = await readText("src/service.ts"); - expect(service).toContain('import { expandUserPath, getConfigDir, readPid, removePid, removeRuntimePort } from "./config";'); + expect(service).toContain('verifyPidIdentity'); expect(service).toContain("removeRuntimePort(pid);"); expect(service).toContain('import { isProcessAlive, stopProxy } from "./lib/process-control";'); + expect(service).toContain('import { findLiveProxy } from "./server/proxy-liveness";'); expect(service).toContain('type TrackedProxyCleanupResult = "none" | "stale" | "stopped";'); expect(service).toContain("async function stopTrackedProxyIfRunning(): Promise"); - expect(service).toContain('if (!pid) return "none";'); - expect(service).toContain("if (!isProcessAlive(pid))"); - expect(service).toContain('return "stale";'); - expect(service).toContain("await stopProxy(pid);"); + expect(service).toContain("await findLiveProxy({ timeoutMs: 1500 })"); + expect(service).toContain("await stopProxy(trackedKillPid);"); + expect(service).toContain("await stopProxy(liveKillPid);"); expect(service).toContain("removePid(pid);"); expect(service).toContain('return "stopped";'); }); + + test("Windows scheduler stop does not wait on schtasks /end failure", async () => { + const service = await readText("src/service.ts"); + const stopCase = service.slice(service.indexOf('case "stop":'), service.indexOf('case "status":')); + // #764 is an /end that succeeds while the wrapper respawns; waiting only when + // /end errors cannot catch that path. Restart-window polling is proxyStillLiveAfterStop. + expect(stopCase).not.toContain("WINDOWS_SCHEDULER_WRAPPER_RESTART_MS"); + expect(stopCase).not.toContain("schedulerEndOk"); + expect(stopCase).not.toContain("await Bun.sleep("); + expect(stopCase).toContain("await proxyStillLiveAfterStop()"); + }); + + test("tracked proxy cleanup verifies health-reported pids before stopProxy", async () => { + const service = await readText("src/service.ts"); + expect(service).toContain("function verifiedKillTarget(pid: number | null | undefined): number | null"); + expect(service).toContain("const liveKillPid = verifiedKillTarget(live?.pid);"); + expect(service).toContain("const trackedKillPid = verifiedKillTarget(pid);"); + }); + test("service stop refuses success while the proxy is still live", async () => { + const service = await readText("src/service.ts"); + const stopCase = service.slice(service.indexOf('case "stop":'), service.indexOf('case "status":')); + expect(stopCase).toContain("await proxyStillLiveAfterStop()"); + expect(stopCase).toContain("a proxy is still listening on port"); + expect(stopCase).toContain("Native Codex was NOT restored"); + expect(stopCase).toContain("process.exitCode = 1"); + }); + + test("native install refuses Microsoft-account logins before removing the scheduler backend", async () => { + const service = await readText("src/service.ts"); + const installNative = service.slice(service.indexOf("async function installWindowsNative()"), service.indexOf("function startWindows()")); + expect(installNative.indexOf("assertWindowsNativeServiceAccountSupported()")).toBeLessThan(installNative.indexOf("uninstallWindows()")); + expect(service).toContain("Microsoft-account Windows login"); + }); + test("service command cleanup logs kill failures without skipping restore/delete", async () => { const service = await readText("src/service.ts"); diff --git a/tests/winsw.test.ts b/tests/winsw.test.ts index 2f728a8b8..8c922d095 100644 --- a/tests/winsw.test.ts +++ b/tests/winsw.test.ts @@ -165,6 +165,13 @@ describe("winsw install flow", () => { expect(calls).toEqual([["interactive", "install", "/p"], ["verify"], ["run", "start"]]); }); + test("install /p refuses non-interactive stdin instead of hanging", () => { + const winsw = readFileSync(new URL("../src/lib/winsw.ts", import.meta.url), "utf8"); + const fn = winsw.slice(winsw.indexOf("function runWinswInteractive"), winsw.indexOf("function scQc()")); + expect(fn).toContain("process.stdin.isTTY"); + expect(fn).toContain("interactive console"); + }); + test("repair over an existing service rewrites assets and restarts without re-prompting", async () => { const calls: string[][] = []; await installWinswService(entry, {