fix(service): make Windows scheduler stop actually stop the proxy #780
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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, 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 { 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; | ||
|
Wibias marked this conversation as resolved.
|
||
| } | ||
| } | ||
|
Comment on lines
+1443
to
+1459
There was a problem hiding this comment. 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: # Inspect the implementation, its call sites, and the project runtime metadata.
printf '%s\n' '--- source context ---'
sed -n '1410,1475p' src/service.ts
printf '%s\n' '--- call sites ---'
rg -n -C 3 'readWindowsPrincipalSource|execFileSync' src/service.ts
printf '%s\n' '--- runtime metadata ---'
rg -n -i 'bun|engines|runtime' package.json bunfig.toml README.md .github 2>/dev/null | head -120Repository: lidge-jun/opencodex Length of output: 14627 🌐 Web query:
💡 Result: In Bun 1.3.14, the Citations:
🏁 Script executed: # Inspect the native-install call path and the declared Bun version.
printf '%s\n' '--- native install references ---'
rg -n -C 5 'installWindowsNative|assertWindowsNativeServiceAccountSupported|service install|--native' src/service.ts src/cli.ts src/cli 2>/dev/null | head -240
printf '%s\n' '--- package runtime declarations ---'
sed -n '20,70p' package.json
printf '%s\n' '--- local runtime availability ---'
command -v bun || true
bun --version 2>/dev/null || trueRepository: lidge-jun/opencodex Length of output: 16543 🌐 Web query:
💡 Result: In Node.js, the execFileSync method supports a timeout option which specifies, in milliseconds, how long a child process is allowed to run [1][2][3]. If the process exceeds this time, it is killed using the signal specified by killSignal (defaulting to SIGTERM) [1][2][3]. Key details regarding the timeout behavior on Windows include: 1. Process Exit Requirement: execFileSync will not return until the child process has completely exited, even after a timeout has occurred [1][3]. If the child process ignores the kill signal and continues running, the parent process will remain waiting until the child finally exits [1][4]. 2. Error Handling: If a process times out, execFileSync will throw an error [1][3]. This error object contains the full result of the underlying spawnSync operation [1][3]. 3. Known Behavioral Issues: In some runtime environments (such as Bun, which implements these child_process APIs), attempting to spawn certain native Windows executables directly may result in an ETIMEDOUT error or a hang, even with a timeout specified [5]. In these cases, wrapping the execution using shell: true or explicitly calling cmd /c is often required to achieve the expected behavior [5]. Citations:
🌐 Web query:
💡 Result: In Bun 1.3.14, issues related to timeout behavior in child_process.execFileSync on Windows typically stem from how Bun's underlying subprocess implementation handles process spawning and timer management on the Windows platform [1][2]. Specific identified issues and behaviors include: 1. Stale Clock/Timer Issues: A known issue involves the synchronous spawn implementation (spawnSync, which execFileSync uses) relying on a cached per-VM libuv loop for the timeout timer [1]. If the loop's cached clock remains idle and goes stale, a timeout can trigger prematurely, resulting in an immediate termination of the child process, an exit code of null, and empty stdout/stderr [1]. 2. Spawning Native Executables: Users have reported that execFileSync and spawnSync may hang indefinitely—eventually throwing an ETIMEDOUT error—when attempting to spawn certain native Windows executables directly without a shell wrapper [2]. In these cases, using { shell: true } often serves as a workaround, as it bypasses the specific spawn path causing the hang [2]. 3. Resource Contention: Under high load, particularly in CI/CD environments on Windows (which lack native fork support and are more expensive for process creation), synchronous child process spawning can occasionally exceed expected time limits due to OS-level scheduler starvation or resource constraints (e.g., Windows Defender scanning) [3]. Bun implements node:child_process by wrapping its native Zig/Rust-based Bun.spawnSync primitives [4]. Developers encountering these issues are often advised to verify if the behavior persists when using { shell: true } (if applicable to their command) or to ensure that the environment is not oversubscribed during process-heavy tasks [2][3]. Fixes for these spawning and timing mechanisms are managed via the oven-sh/bun repository's issue tracker [1][5][6]. Citations:
🏁 Script executed: # Check the exact dependency declaration and whether tests cover the Windows principal probe.
printf '%s\n' '--- package header and dependencies ---'
sed -n '1,75p' package.json
printf '%s\n' '--- principal probe tests and mocks ---'
rg -n -C 4 'PrincipalSource|MicrosoftAccount|assertWindowsNativeServiceAccountSupported|execFileSync|process.platform' tests src | head -260Repository: lidge-jun/opencodex Length of output: 20208 🏁 Script executed: # Inspect the repository's Bun-native Windows subprocess wrapper and its timeout behavior.
printf '%s\n' '--- Windows ACL subprocess implementation ---'
sed -n '70,125p' src/lib/windows-secret-acl.ts
printf '%s\n' '--- timeout-related subprocess patterns ---'
rg -n -C 5 'Bun\.spawnSync|timeoutMs|timeout:|ETIMEDOUT|execFileSync.*hang|hung' src tests | head -280Repository: lidge-jun/opencodex Length of output: 20592 Bound the Windows principal probe and avoid At 🤖 Prompt for AI Agents |
||
|
|
||
| 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]); } | ||
|
|
||
| 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 { | ||
| 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(); | ||
| 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); | ||
| } | ||
| // 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": { | ||
| 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(); | ||
| } | ||
| 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); | ||
| } | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.