From 36facc6ba455f927f4e16b784a1d58519eab51fc Mon Sep 17 00:00:00 2001 From: Nayan Date: Tue, 11 Aug 2026 23:11:52 +0530 Subject: [PATCH] fix(cli): repair editor launch, process cleanup and port selection on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Open-in-editor was broken outright, and took the daemon with it. All four editors ship as `.cmd` shims on Windows and libuv's PATH search only tries `.com` and `.exe` — so `where code` found the shim, onPath said yes, and the spawn failed with ENOENT. spawn reports that asynchronously, so the try/catch never saw it, and an `error` event with no listener throws: one click on a file it could not open exited the server. The URL-scheme fallback was no better, since it embedded a Windows path with backslashes, which are not legal in a URI path. Both are fixed, and every detached spawn here now has an error listener. `--exec` orphaned the dev server. The child is spawned with a shell, so on Windows it is cmd.exe and the real tree is cmd.exe -> pnpm -> node -> Vite; child.kill() maps to TerminateProcess on cmd.exe alone and left that tree holding the port, outliving airship and blocking the next launch. taskkill /T takes the descendants with it. Since that is already a forced kill, the SIGTERM-then-grace-then-SIGKILL escalation is skipped there. firstFreePort connect-probed, which answers "is anything accepting" rather than "can I bind" — and the two diverge on Windows, where Hyper-V, WSL2 and Docker Desktop reserve whole port ranges that accept nothing yet refuse to be bound. It now binds and lets go. The residual case is handled properly too: listen() had no error listener, so EADDRINUSE/EACCES became an uncaughtException that escaped the caller's try/catch, leaving the dev server we started unstopped. Also: opencode resolves through PATHEXT, because `npm i -g opencode-ai` — which our own error message recommends — writes opencode.cmd, never opencode.exe; its credential probe reads %LOCALAPPDATA%/%APPDATA% rather than XDG paths that are never set on Windows; SIGBREAK is registered, since Windows never delivers SIGTERM; and openInBrowser uses `cmd /c start ""` rather than a shell, where Node does no argument escaping and a URL carrying `&` would be split in two. --- apps/cli/src/commands/serve.ts | 31 ++++- apps/cli/src/lib/detect.ts | 21 ++- apps/cli/src/lib/exec.ts | 51 ++++++-- .../core/src/providers/opencode-server.ts | 41 +++++- packages/server/src/index.ts | 13 +- packages/server/src/open-editor.ts | 122 ++++++++++++++---- 6 files changed, 235 insertions(+), 44 deletions(-) diff --git a/apps/cli/src/commands/serve.ts b/apps/cli/src/commands/serve.ts index b858474..c418742 100644 --- a/apps/cli/src/commands/serve.ts +++ b/apps/cli/src/commands/serve.ts @@ -204,6 +204,28 @@ async function assertFree(port: number): Promise { } } +/** + * Turn a failure to bind the overlay's port into something actionable. + * + * EACCES is the Windows-specific one: Hyper-V, WSL2 and Docker Desktop reserve + * whole ranges of dynamic ports, and a port inside one accepts no connection + * yet refuses to be bound — so it looks free right up until it isn't. + */ +function asBindError(err: unknown, port: number): unknown { + const code = (err as NodeJS.ErrnoException | null)?.code; + if (code === "EADDRINUSE") { + return new CliError(`Port ${port} is already in use`, { + hint: "Pass --port with a free one.", + }); + } + if (code === "EACCES") { + return new CliError(`Not allowed to bind port ${port}`, { + hint: "On Windows this port may sit in a reserved range (see `netsh interface ipv4 show excludedportrange tcp`). Pass --port with one outside it.", + }); + } + return err; +} + export const serve = defineCommand({ args: argsFor(SERVE_FLAGS), meta: { @@ -274,7 +296,7 @@ export const serve = defineCommand({ // We started the dev server; if the proxy cannot come up it is ours to // clean up, or the user is left with a stray process holding the port. await dev?.stop(); - throw err; + throw asBindError(err, port); } if (opts.json) { @@ -332,5 +354,12 @@ export const serve = defineCommand({ }; process.on("SIGINT", onSignal); process.on("SIGTERM", onSignal); + // Windows never delivers SIGTERM — registering it is legal, it just never + // fires — so without SIGBREAK the only clean exit there is Ctrl-C. Ctrl-Break + // and a console close would otherwise skip shutdown entirely and strand the + // `opencode serve` child and any dev server we started. + if (process.platform === "win32") { + process.on("SIGBREAK", onSignal); + } }, }); diff --git a/apps/cli/src/lib/detect.ts b/apps/cli/src/lib/detect.ts index 666847d..b10f59f 100644 --- a/apps/cli/src/lib/detect.ts +++ b/apps/cli/src/lib/detect.ts @@ -172,6 +172,25 @@ export async function detectTarget( return first ? { ...first, listening: false } : undefined; } +/** + * Whether we can actually take this port, by taking it and letting go. + * + * `isListening` answers a different question — "is something accepting here" — + * and the two diverge on Windows, where Hyper-V, WSL2 and Docker Desktop + * reserve whole ranges of dynamic ports (`netsh interface ipv4 show + * excludedportrange tcp`). Nothing accepts on a reserved port, so a connect + * probe calls it free, and the bind then fails with EACCES. + */ +function canBind(port: number, host: string): Promise { + return new Promise((resolvePromise) => { + const probe = net.createServer(); + probe.once("error", () => resolvePromise(false)); + probe.listen({ host, port }, () => { + probe.close(() => resolvePromise(true)); + }); + }); +} + /** A port nothing is bound to, so the proxy does not collide on startup. */ export async function firstFreePort( start: number, @@ -182,7 +201,7 @@ export async function firstFreePort( // Also ordered: "the first free port at or after `start`" is the answer, so // the probes cannot be collapsed into one parallel batch. // biome-ignore lint/performance/noAwaitInLoops: ordered first-match search - if (!(await isListening(port, host))) { + if (await canBind(port, host)) { return port; } } diff --git a/apps/cli/src/lib/exec.ts b/apps/cli/src/lib/exec.ts index b315872..7eb206f 100644 --- a/apps/cli/src/lib/exec.ts +++ b/apps/cli/src/lib/exec.ts @@ -10,11 +10,13 @@ * first, and our own shutdown would be racing a corpse. */ -import { type ChildProcess, spawn } from "node:child_process"; +import { type ChildProcess, spawn, spawnSync } from "node:child_process"; import { isListening } from "./detect"; import { CliError } from "./errors"; import { style } from "./terminal"; +const WIN32 = process.platform === "win32"; + const READY_TIMEOUT_MS = 90_000; const POLL_INTERVAL_MS = 250; const STOP_GRACE_MS = 3000; @@ -33,9 +35,16 @@ function killTree(child: ChildProcess, signal: NodeJS.Signals): void { return; } try { - if (process.platform === "win32") { - // No process groups to negate; Node maps this to TerminateProcess. - child.kill(signal); + if (WIN32) { + // `child` here is cmd.exe, not the dev server — we spawn with `shell`, + // so the real tree is cmd.exe -> pnpm -> node -> Vite. `child.kill()` + // maps to TerminateProcess on cmd.exe alone and leaves that tree running, + // still holding the port, outliving airship and blocking the next launch. + // taskkill /T is the only way to take the descendants with it. + spawnSync("taskkill", ["/pid", String(child.pid), "/T", "/F"], { + stdio: "ignore", + windowsHide: true, + }); return; } // Negative pid = the whole group, which is what actually stops a dev server @@ -87,12 +96,25 @@ export async function startDevServer(opts: { child.once("exit", (code, signal) => { exited = { code, signal }; }); + // A shell that will not start emits `error` and never `exit`. Without this + // the readiness loop below would wait the full 90s for a process that was + // never running — and an unheard `error` event throws. + child.once("error", () => { + exited ??= { code: null, signal: null }; + }); const stop = async (): Promise => { if (exited) { return; } killTree(child, "SIGTERM"); + // Windows has no graceful signal — killTree already went straight to + // `taskkill /F`, so there is nothing to escalate to and nothing to wait + // for. Polling for three seconds before repeating the same forced kill + // would only delay shutdown. + if (WIN32) { + return; + } for (let waited = 0; waited < STOP_GRACE_MS; waited += POLL_INTERVAL_MS) { if (exited) { return; @@ -134,26 +156,29 @@ export async function startDevServer(opts: { } } -const BROWSER_OPENERS: Record = { - darwin: "open", - win32: "start", -}; - /** Open a URL in the default browser, best-effort. */ export function openInBrowser(url: string): void { - const command = BROWSER_OPENERS[process.platform] ?? "xdg-open"; + // `cmd /c start "" ` on Windows rather than `shell: true` + `start`. + // With a shell Node does no argument escaping, so a URL carrying `&` would be + // split by cmd.exe into two commands; and `start` reads its first quoted + // argument as a window title, which is what the empty string absorbs. Same + // form as openUrl in @airship/server's open-editor. + const [command, args] = + process.platform === "win32" + ? (["cmd", ["/c", "start", "", url]] as const) + : ([process.platform === "darwin" ? "open" : "xdg-open", [url]] as const); try { // Detached and fully ignored: the browser outliving airship is the point, // and its stdio would otherwise pollute the terminal. - const child = spawn(command, [url], { + const child = spawn(command, [...args], { detached: true, - shell: process.platform === "win32", stdio: "ignore", + windowsHide: true, }); - child.unref(); child.on("error", () => { // No browser, no display, no `open` — the URL is printed either way. }); + child.unref(); } catch { // Same: --open is a convenience, never a reason to fail a launch. } diff --git a/packages/core/src/providers/opencode-server.ts b/packages/core/src/providers/opencode-server.ts index 92ea9ab..7825691 100644 --- a/packages/core/src/providers/opencode-server.ts +++ b/packages/core/src/providers/opencode-server.ts @@ -96,7 +96,29 @@ export interface OpencodeHandle { url: string; } -const BINARY = process.platform === "win32" ? "opencode.exe" : "opencode"; +/** + * The names `opencode` can go by on disk, most-specific first. + * + * Hardcoding `opencode.exe` on Windows found nothing for the install we + * ourselves recommend: `npm i -g opencode-ai` writes `opencode.cmd` and + * `opencode.ps1` shims, never a `.exe`. So a perfectly good opencode reported + * "No `opencode` binary found on PATH" and every edit through that backend + * failed. PATHEXT is the OS's own answer to which extensions are executable; + * the fallback matches its default. + */ +const BINARY_NAMES: string[] = + process.platform === "win32" + ? [ + ...(process.env.PATHEXT ?? ".COM;.EXE;.BAT;.CMD") + .split(";") + .filter(Boolean) + .map((ext) => `opencode${ext.toLowerCase()}`), + // Extension-less last, so a real launcher always wins — but still + // present, because dropping it would be its own regression for anyone + // whose opencode is a bare binary on PATH. + "opencode", + ] + : ["opencode"]; /** A cold first launch on a slow machine must not read as a broken install. */ const START_TIMEOUT_MS = 30_000; @@ -115,9 +137,11 @@ export function resolveOpencodeBinary(override?: string): string | null { if (!dir) { continue; } - const candidate = join(dir, BINARY); - if (existsSync(candidate)) { - return candidate; + for (const name of BINARY_NAMES) { + const candidate = join(dir, name); + if (existsSync(candidate)) { + return candidate; + } } } return null; @@ -255,9 +279,16 @@ function registerExitHooks(): void { return; } hooksRegistered = true; - for (const signal of ["exit", "SIGINT", "SIGTERM"] as const) { + // SIGBREAK on Windows, where SIGTERM is accepted by `process.once` but never + // delivered — Ctrl-Break would otherwise leave this child holding its port. + // "exit" remains the backstop that catches everything else. + const signals = ["exit", "SIGINT", "SIGTERM"] as const; + for (const signal of signals) { process.once(signal, () => shutdownServer()); } + if (process.platform === "win32") { + process.once("SIGBREAK", () => shutdownServer()); + } } /** diff --git a/packages/server/src/index.ts b/packages/server/src/index.ts index 072749e..aad0591 100644 --- a/packages/server/src/index.ts +++ b/packages/server/src/index.ts @@ -524,8 +524,17 @@ export async function startServer(opts: ServerOptions): Promise { socket.on("close", () => tunnelled.delete(socket as Socket)); }); - await new Promise((resolve) => { - server.listen(opts.port, () => resolve()); + // The `error` listener is what makes this rejectable. Without it an + // EADDRINUSE — or, on Windows, the EACCES you get from a port inside a + // Hyper-V/WSL2 reserved range — is emitted with nobody listening, becomes an + // uncaughtException, and escapes the caller's try/catch, so the dev server + // airship started with `--exec` is never stopped either. + await new Promise((resolve, reject) => { + server.once("error", reject); + server.listen(opts.port, () => { + server.removeListener("error", reject); + resolve(); + }); }); const addr = server.address() as AddressInfo | null; diff --git a/packages/server/src/open-editor.ts b/packages/server/src/open-editor.ts index 091c9e9..ab5b022 100644 --- a/packages/server/src/open-editor.ts +++ b/packages/server/src/open-editor.ts @@ -8,12 +8,15 @@ * a column and are awkward to detect. So: try the binary, fall back to the * scheme. */ -import { spawn, spawnSync } from "node:child_process"; +import { type ChildProcess, spawn, spawnSync } from "node:child_process"; import { existsSync } from "node:fs"; import { platform } from "node:os"; -import { resolve, sep } from "node:path"; +import { resolve } from "node:path"; +import { isPathInside, toPosixPath } from "@airship/core"; import type { Editor } from "@airship/protocol"; +const WIN32 = platform() === "win32"; + export interface OpenRequest { column?: number; editor?: Editor; @@ -48,8 +51,10 @@ const LAUNCHERS: Record = { const PREFERENCE: Editor[] = ["vscode", "cursor", "windsurf", "zed"]; /** Vite serves out-of-root files under `/@fs/`. */ -const VITE_FS_PREFIX = /^\/@fs(\/.*)$/; +const VITE_FS_PREFIX = /^\/@fs\/(.*)$/; const LEADING_SLASHES = /^\/+/; +/** `C:` — a Windows path that is already rooted at a drive. */ +const WIN32_DRIVE = /^[a-zA-Z]:/; export function openInEditor(cwd: string, req: OpenRequest): OpenResult { const root = resolve(cwd); @@ -57,7 +62,12 @@ export function openInEditor(cwd: string, req: OpenRequest): OpenResult { // This socket is unauthenticated and local, and this handler spawns // processes — without the containment check any page the browser loads could // ask the daemon to open arbitrary files on disk. - if (abs !== root && !abs.startsWith(root + sep)) { + // + // isPathInside rather than a bare startsWith: the two sides reach here by + // different routes and can disagree on symlinks (macOS `/tmp`, `/var`) or on + // case (Windows, including the drive letter), and a false "outside the + // project" on the user's own file is indistinguishable from a real refusal. + if (!isPathInside(root, abs)) { return { error: "path is outside the project", ok: false }; } if (!existsSync(abs)) { @@ -73,22 +83,13 @@ export function openInEditor(cwd: string, req: OpenRequest): OpenResult { if (!onPath(launcher.bin)) { continue; } - try { - // Detached and unref'd: the editor outlives the daemon, and inheriting - // stdio would wire its output into ours. Never `shell: true` — argv - // arrays mean a path with a space or a quote is just a path. - spawn(launcher.bin, launcher.args(target), { - detached: true, - stdio: "ignore", - }).unref(); + if (launch(launcher.bin, launcher.args(target))) { return { editor, ok: true }; - } catch { - // Fall through to the next candidate, then to the URL scheme. } } const wanted = req.editor ?? PREFERENCE[0]; - if (openUrl(`${LAUNCHERS[wanted].scheme}://file/${abs}:${line}:${column}`)) { + if (openUrl(editorUrl(LAUNCHERS[wanted].scheme, abs, line, column))) { return { editor: wanted, ok: true }; } return { @@ -111,16 +112,83 @@ export function openInEditor(cwd: string, req: OpenRequest): OpenResult { function projectPath(file: string): string { const fs = file.match(VITE_FS_PREFIX); if (fs?.[1]) { - return fs[1]; + // The remainder is already absolute. On POSIX it needs the slash the + // capture dropped; on Windows it opens with a drive letter and must NOT + // get one, because `resolve` reads a leading slash as rooted-but- + // deviceless and inherits the device from cwd — turning + // `/@fs/C:/Users/me/x.tsx` into `C:\C:\Users\me\x.tsx`. + return WIN32_DRIVE.test(fs[1]) ? fs[1] : `/${fs[1]}`; } // A real absolute path exists on disk; a URL path like `/src/App.tsx` does - // not, and is meant to be read relative to the project root. - if (file.startsWith("/") && !existsSync(file)) { + // not, and is meant to be read relative to the project root. On Windows the + // probe is skipped: a leading slash with no drive letter is never an absolute + // path there, so `existsSync` would only ever be answering about a different + // file — `:\src\App.tsx`, which routinely exists. + if (file.startsWith("/") && (WIN32 || !existsSync(file))) { return file.replace(LEADING_SLASHES, ""); } return file; } +/** + * Start a detached child, or report that it could not start. + * + * `shell` on Windows because every one of these editors ships as a `.cmd` shim + * (`code.cmd`, `cursor.cmd`, …) and libuv's PATH search only ever tries `.com` + * and `.exe`. `where code` finds the shim, so `onPath` says yes, and the bare + * spawn then fails with ENOENT — which is how this managed to be 100% broken on + * Windows while looking fine. Node applies cmd-specific argument escaping when + * `shell` is set, so a path with a space in it survives. + * + * The `error` listener is not optional. spawn reports a failure to start + * asynchronously, so the `try` here never sees it; an EventEmitter that emits + * `error` with nobody listening throws, and this runs in the daemon, so one + * click on a file it cannot open took the whole server down. + * + * `pid` is undefined when the spawn failed outright, which is the only + * synchronous signal available — and it is what lets the caller fall through to + * the URL scheme instead of reporting a success that never happened. + */ +function launch(bin: string, args: string[]): boolean { + try { + const child = spawn(bin, args, { + detached: true, + shell: WIN32, + stdio: "ignore", + windowsHide: true, + }); + detach(child); + return child.pid !== undefined; + } catch { + return false; + } +} + +/** Let the child outlive us, and never let its `error` reach the top level. */ +function detach(child: ChildProcess): void { + child.on("error", () => { + // No editor, no PATH entry, no display. The caller reports it; the daemon + // must not die of it. + }); + child.unref(); +} + +/** + * `vscode://file/...` for the fallback path. + * + * Backslashes are not legal in a URI path, so a Windows path has to be + * rewritten or the scheme handler simply ignores it. POSIX paths already open + * with `/`, which is why there is only one after `file` in the template. + */ +function editorUrl( + scheme: string, + abs: string, + line: number, + column: number +): string { + return `${scheme}://file/${toPosixPath(abs)}:${line}:${column}`; +} + function candidates(explicit?: Editor): Editor[] { if (explicit) { return [explicit]; @@ -140,10 +208,12 @@ function onPath(bin: string): boolean { if (hit !== undefined) { return hit; } - const probe = platform() === "win32" ? "where" : "which"; + const probe = WIN32 ? "where" : "which"; let found = false; try { - found = spawnSync(probe, [bin], { stdio: "ignore" }).status === 0; + found = + spawnSync(probe, [bin], { stdio: "ignore", windowsHide: true }).status === + 0; } catch { found = false; } @@ -158,12 +228,20 @@ function openUrl(url: string): boolean { if (os === "darwin") { bin = "open"; } else if (os === "win32") { + // `cmd /c start "" ` rather than `shell: true`: `start` takes a window + // title as its first quoted argument, so the empty string is what stops it + // swallowing the URL as one. cmd.exe is a real .exe, so no shell needed. bin = "cmd"; args = ["/c", "start", "", url]; } try { - spawn(bin, args, { detached: true, stdio: "ignore" }).unref(); - return true; + const child = spawn(bin, args, { + detached: true, + stdio: "ignore", + windowsHide: true, + }); + detach(child); + return child.pid !== undefined; } catch { return false; }