Merge pull request lidge-jun#780 from lidge-jun/fix/764-windows-service-stop

fix(service): make Windows scheduler stop actually stop the proxy

Author: Wibias

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" });
 }
 

src/server/proxy-liveness.ts

@@ -109,6 +109,13 @@ export async function findLiveProxy(io: LivenessIo = {}): Promise<LiveProxy | nu
     return verified === candidate ? verified : null;
   };
 
+  const verifiedReportedPid = (reported: number | null): number | null => {
+    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<LiveProxy | nu
     // (its process dead, the port reused by a pidless legacy proxy) — synthesizing it
     // would hand destructive callers (stopProxy → kill fallback) a reusable pid.
     if (identity) {
-      return { pid: identity.pid ?? null, port: record.port, hostname: record.hostname, source: "runtime" };
+      return { pid: verifiedReportedPid(identity.pid), port: record.port, hostname: record.hostname, source: "runtime" };
     }
   }
 
@@ -145,7 +152,7 @@ export async function findLiveProxy(io: LivenessIo = {}): Promise<LiveProxy | nu
   const identity = await proxyIdentityAt(port, { hostname: config.hostname }, io);
   if (identity) {
     return {
-      pid: identity.pid ?? killablePid(pid),
+      pid: verifiedReportedPid(identity.pid) ?? killablePid(pid),
       port,
       hostname: config.hostname,
       source: "config",

src/service.ts

@@ -6,18 +6,18 @@
  * restore it via the command.
  */
 import { execFileSync, execSync } from "node:child_process";
+import { findLiveProxy } from "./server/proxy-liveness";
 import { chmodSync, existsSync, mkdirSync, readFileSync, unlinkSync, writeFileSync } from "node:fs";
 import { homedir } from "node:os";
 import { dirname, join, resolve } from "node:path";
-import { expandUserPath, getConfigDir, readPid, removePid, removeRuntimePort } from "./config";
+import { expandUserPath, getConfigDir, readPid, removePid, removeRuntimePort, verifyPidIdentity } from "./config";
 import { loadConfig } from "./config";
 import { restoreNativeCodex } from "./codex/inject";
 import { stripGrokConfig } from "./grok/inject";
 import { isWslRuntime } from "./codex/home";
 import { durableBunPath, durableBunRuntime } from "./lib/bun-runtime";
 import { isProcessAlive, stopProxy } from "./lib/process-control";
 import { serviceApiTokenFilePath } from "./lib/service-secrets";
-import { findLiveProxy } from "./server/proxy-liveness";
 import { randomUUID } from "node:crypto";
 import {
   ELEVATION_REQUEST_TIMEOUT_MS,
@@ -1428,7 +1428,38 @@ export async function repairService(deps: RepairServiceDeps = {}): Promise<void>
  * 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<void> {
+  assertWindowsNativeServiceAccountSupported();
   recordOwnedConfigPath(getConfigDir(), serviceStatePath());
   if (!existsSync(getConfigDir())) mkdirSync(getConfigDir(), { recursive: true });
   writeServiceApiTokenFile();
@@ -1463,11 +1494,49 @@ async function installWindowsNative(): Promise<void> {
   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<TrackedProxyCleanupResult> {
+  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<TrackedProxyCleanupResult> {
@@ -1984,11 +2073,13 @@ export async function serviceCommand(...args: (string | undefined)[]): Promise<v
       ops.start();
       console.log("✅ service started.");
       break;
-    case "stop":
+    case "stop": {
       assertServiceEnvironmentMatchesInstall();
       // Only stop what is actually installed. The unguarded call ran a real `launchctl unload`
       // (and its Windows/Linux twins) even with nothing installed.
-      if (ops.status() !== null || isServiceInstalled()) ops.stop();
+      if (ops.status() !== null || isServiceInstalled()) {
+        ops.stop();
+      }
       await stopTrackedProxyForServiceCommand();
       {
         // Verify rather than trust the stop command: a surviving wrapper respawns its child
@@ -2015,6 +2106,7 @@ export async function serviceCommand(...args: (string | undefined)[]): Promise<v
         else if (!grok.ok) console.error(`⚠️  ${grok.message}`);
       }
       break;
+    }
     case "status": {
       if (process.platform === "win32" && backend === "scheduler") {
         console.log(await inspectWindowsSchedulerServiceStatus());
@@ -2060,3 +2152,4 @@ export async function serviceCommand(...args: (string | undefined)[]): Promise<v
       process.exit(1);
   }
 }
+

tests/proxy-liveness.test.ts

@@ -75,6 +75,7 @@ describe("findLiveProxy", () => {
       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 () => {

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<TrackedProxyCleanupResult>");
-    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");
 

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, {