From 100b19c4a0347c906fb1f77f58bbfbd81b4ea97c Mon Sep 17 00:00:00 2001 From: "@rugpanov" Date: Mon, 24 Aug 2026 14:18:45 +0300 Subject: [PATCH 1/5] refactor(cli): unify CLI process execution behind one vscode-free seam MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit *Why* The `cli` and `python-setup` packages each carried their own child-process plumbing: `CliWrapper` had `execFile`/`waitForProcess`/`runBundleCommand` (buffered exec + bundle streaming, cancelled via an AbortController that only SIGTERMs the direct child) while `PythonSetupCliClient` had a separate, better spawn core (detached process group, whole-tree kill, buffer-then-decode). Two implementations of the same concern, and the older one leaked grandchildren (`terraform`, `uv`) on cancel. *What* - Add `src/cli/cliProcess.ts`: a single, vscode-free execution seam (`run(command, args, options)`) that every request/response CLI call flows through. Injectable spawn/terminate seams make it fully unit-testable. It spawns detached on POSIX + kills the whole process tree on cancel (taskkill /T /F on Windows), buffers-then-decodes output (no split-code-point corruption), streams decoded chunks to callbacks, and resolves on any exit (exit-code policy is the caller's). - `CliWrapper`'s `execFile`/`cancellableExecFile`/`runBundleCommand` are now thin adapters over `run()`; the Node-`execFile` error surface (stderr in `.message`, `.code`/`.stdout`/`.stderr`) is preserved so `isFileNotFound` and the profile-parse checks keep working. `waitForProcess` and the duplicated Windows escaping are removed. - `PythonSetupCliClient` delegates its spawning to `run()` and keeps only the setup-local concern (argv, JSON parse, onLog, cancel mapping); the shared process primitives (`terminateProcessTree`, `CancellationLike`, seams) now live in `cliProcess` and are re-exported for existing consumers. - The interactive pty terminals (`bundle run`, `bundle sync --watch`) are deliberately left out — they render into a live Pseudoterminal, a different execution model. - `ProcessError`/`CancellationError` stay in `CliWrapper` (they touch `vscode` for their toast), keeping `cliProcess` free of any `vscode` import. Bonus: POSIX cancel now tears down the whole tree (no orphaned terraform/uv), and the buffered path no longer has Node execFile's 1 MB stdout cap. *Verification* - New `cliProcess.test.ts` (21 cases): buffered capture, exit codes, streaming, UTF-8 straddle, partial-byte flush, closeStdin, shell passthrough, spawn error, cancel→tree-kill, Windows escaping — all via injected fakes, run under plain mocha (no extension host). - `yarn test:unit` → 930 passing, 0 failing (the one `shellUtils` real-shell timeout was a load flake; green on rerun). `yarn test:lint` → clean. Co-authored-by: Isaac --- .../src/cli/CliWrapper.test.ts | 28 -- .../databricks-vscode/src/cli/CliWrapper.ts | 238 ++++++------ .../src/cli/cliProcess.test.ts | 361 ++++++++++++++++++ .../databricks-vscode/src/cli/cliProcess.ts | 303 +++++++++++++++ .../gateways/PythonSetupCliClient.test.ts | 77 +--- .../gateways/PythonSetupCliClient.ts | 252 +++--------- 6 files changed, 829 insertions(+), 430 deletions(-) create mode 100644 packages/databricks-vscode/src/cli/cliProcess.test.ts create mode 100644 packages/databricks-vscode/src/cli/cliProcess.ts diff --git a/packages/databricks-vscode/src/cli/CliWrapper.test.ts b/packages/databricks-vscode/src/cli/CliWrapper.test.ts index 2fadb7367..0fe6e2465 100644 --- a/packages/databricks-vscode/src/cli/CliWrapper.test.ts +++ b/packages/databricks-vscode/src/cli/CliWrapper.test.ts @@ -11,7 +11,6 @@ import { CliWrapper, ProcessError, getSshConnectCommand, - waitForProcess, } from "./CliWrapper"; import path from "node:path"; import os from "node:os"; @@ -23,8 +22,6 @@ import {ProfileAuthProvider} from "../configuration/auth/AuthProvider"; import {isMatch} from "lodash"; import {removeUndefinedKeys} from "../utils/envVarGenerators"; import {writeFileSync} from "fs"; -import {ChildProcess, ChildProcessWithoutNullStreams} from "child_process"; -import {Readable} from "stream"; const execFile = promisify(execFileCb); // Mirror CliWrapper.cliPath: the bundled binary is `databricks.exe` on Windows. @@ -491,31 +488,6 @@ describe("cancellableExecFile closeStdin", () => { }); }); -describe("waitForProcess", () => { - it("should return correctly formatted stdout and stderr", async () => { - const process = new ChildProcess(); - const stdoutChunks = [`{"hello": "wor`, `ld"}`]; - const stderrChunks = [`{"error": "no`, `oo"}`]; - process.stdout = new Readable({ - read() { - this.push(stdoutChunks.shift()); - }, - }); - process.stderr = new Readable({ - read() { - this.push(stderrChunks.shift()); - }, - }); - const waitPromise = waitForProcess( - process as ChildProcessWithoutNullStreams - ); - process.emit("close", 0); - const {stdout, stderr} = await waitPromise; - assert.equal(stdout, `{"hello": "world"}`); - assert.equal(stderr, `{"error": "nooo"}`); - }); -}); - describe("ProcessError.showErrorMessage", () => { let originalShowError: typeof window.showErrorMessage; let originalExecuteCommand: typeof commands.executeCommand; diff --git a/packages/databricks-vscode/src/cli/CliWrapper.ts b/packages/databricks-vscode/src/cli/CliWrapper.ts index c3b1c9da9..92ceb6fd2 100644 --- a/packages/databricks-vscode/src/cli/CliWrapper.ts +++ b/packages/databricks-vscode/src/cli/CliWrapper.ts @@ -1,9 +1,4 @@ -import { - ChildProcessWithoutNullStreams, - SpawnOptionsWithoutStdio, - execFile as execFileCb, - spawn, -} from "child_process"; +import {SpawnOptionsWithoutStdio} from "child_process"; import { ExtensionContext, window, @@ -12,7 +7,7 @@ import { CancellationToken, } from "vscode"; import {workspaceConfigs} from "../vscode-objs/WorkspaceConfigs"; -import {promisify} from "node:util"; +import {run as runCli} from "./cliProcess"; import {logging} from "@databricks/sdk-experimental"; import {LoggerManager, Loggers} from "../logger"; import {Context, context} from "@databricks/sdk-experimental/dist/context"; @@ -33,85 +28,103 @@ import { } from "../utils/shellUtils"; const withLogContext = logging.withLogContext; -function getEscapedCommandAndAgrs( - cmd: string, - args: string[], - options: SpawnOptionsWithoutStdio -) { - if (process.platform === "win32") { - const cmdArgs = args.slice(); - args = [ - "/d", // Disables execution of AutoRun commands, which are like .bashrc commands. - "/c", // Carries out the command specified by and then exits the command processor. - `""${cmd}" ${cmdArgs.map((a) => `"${a}"`).join(" ")}"`, - ]; - cmd = "cmd.exe"; - options = {...options, windowsVerbatimArguments: true}; - } - return {cmd, args, options}; -} export interface ExecFileOptions { /** - * Close the child's stdin immediately after spawning. Node's `execFile` - * gives the child an open stdin pipe that never receives EOF, so any CLI - * command that prompts for confirmation (e.g. `aitools update`) blocks - * forever waiting on input. Ending stdin delivers EOF so the prompt - * resolves instead of hanging. Only set this for non-interactive commands - * that we never feed input to. + * Close the child's stdin immediately after spawning. Node gives the child + * an open stdin pipe that never receives EOF, so any CLI command that + * prompts for confirmation (e.g. `aitools update`) blocks forever waiting + * on input. Ending stdin delivers EOF so the prompt resolves instead of + * hanging. Only set this for non-interactive commands we never feed input to. */ closeStdin?: boolean; } +/** + * Buffered CLI execution over the shared {@link runCli} seam: run to + * completion, then resolve with the full output or throw. The thrown error + * mirrors Node's `execFile` rejection so existing callers keep working — its + * `message` includes stderr, and `.code`/`.stderr`/`.stdout` are set — which + * is what the SDK's `isFileNotFound` and the profile-parsing checks inspect. + */ +async function bufferedExec( + file: string, + args: string[], + options: Omit, + cancellationToken: CancellationToken | undefined, + execOptions: ExecFileOptions, + escapeCommandForWindows: boolean +): Promise<{stdout: string; stderr: string}> { + const result = await runCli(file, args, { + // SpawnOptions types cwd as string | URL; every caller passes a string. + cwd: options.cwd as string | undefined, + env: options.env, + shell: options.shell as boolean | undefined, + token: cancellationToken, + closeStdin: execOptions.closeStdin, + escapeCommandForWindows, + }); + + if (result.cancelled) { + throw new CancellationError(); + } + if (result.exitCode !== 0) { + const error: Error & { + code?: number | null; + stdout?: string; + stderr?: string; + } = new Error( + `Command failed: ${file} ${args.join(" ")}\n${result.stderr}` + ); + error.code = result.exitCode; + error.stdout = result.stdout; + error.stderr = result.stderr; + throw error; + } + return {stdout: result.stdout, stderr: result.stderr}; +} + +/** + * Buffered exec that spawns `file` directly (bare command names resolve via the + * PATH / shell). Used by callers that manage their own Windows quoting through + * the `shell` option (e.g. the Azure and host-CLI probes). + */ export async function cancellableExecFile( file: string, args: string[], options: Omit = {}, cancellationToken?: CancellationToken, execOptions: ExecFileOptions = {} -): Promise<{ - stdout: string; - stderr: string; -}> { - const abortController = new AbortController(); - cancellationToken?.onCancellationRequested(() => abortController.abort()); - const signal = abortController.signal; - - const promise = promisify(execFileCb)(file, args, { - ...options, - signal, - }); - if (execOptions.closeStdin) { - // `promisify(execFile)` returns a PromiseWithChild that exposes the - // spawned ChildProcess on `.child`. - promise.child.stdin?.end(); - } - const res = await promise; - return {stdout: res.stdout.toString(), stderr: res.stderr.toString()}; +): Promise<{stdout: string; stderr: string}> { + return await bufferedExec( + file, + args, + options, + cancellationToken, + execOptions, + /*escapeCommandForWindows*/ false + ); } +/** + * Buffered exec that routes the command through `cmd.exe` on Windows, so a CLI + * invoked by a bare name or a path with spaces resolves and quotes correctly. + * The default entry point for the extension's own CLI calls. + */ export const execFile = async ( file: string, args: string[], options: Omit = {}, cancellationToken?: CancellationToken, execOptions: ExecFileOptions = {} -): Promise<{ - stdout: string; - stderr: string; -}> => { - const { - cmd, - args: escapedArgs, - options: escapedOptions, - } = getEscapedCommandAndAgrs(file, args, options); - - return await cancellableExecFile( - cmd, - escapedArgs, - escapedOptions, +): Promise<{stdout: string; stderr: string}> => { + return await bufferedExec( + file, + args, + options, cancellationToken, - execOptions + execOptions, + /*escapeCommandForWindows*/ true ); }; @@ -266,42 +279,6 @@ export class CancellationError extends Error { } } -export async function waitForProcess( - p: ChildProcessWithoutNullStreams, - onStdOut?: (data: string) => void, - onStdError?: (data: string) => void -) { - const stdout: string[] = []; - p.stdout.on("data", (data) => { - stdout.push(data.toString()); - if (onStdOut) { - onStdOut(data.toString()); - } - }); - - const stderr: string[] = []; - p.stderr.on("data", (data) => { - stderr.push(data.toString()); - if (onStdError) { - onStdError(data.toString()); - } - }); - - await new Promise((resolve, reject) => { - p.on("close", (code) => { - if (code === 0) { - resolve(); - } else { - const message = onStdError ? "" : stderr.join(""); - reject(new ProcessError(message, code)); - } - }); - p.on("error", (e) => new ProcessError(e.message, null)); - }); - - return {stdout: stdout.join(""), stderr: stderr.join("")}; -} - async function runBundleCommand( bundleOpName: string, cmd: string, @@ -342,45 +319,50 @@ async function runBundleCommand( }); logger?.debug(quote([cmd, ...args]), {bundleOpName}); - const abortController = new AbortController(); - let options: SpawnOptionsWithoutStdio = { - cwd: workspaceFolder.fsPath, - env: removeUndefinedKeys(env), - signal: abortController.signal, - }; - ({cmd, args, options} = getEscapedCommandAndAgrs(cmd, args, options)); + let result; try { - const p = spawn(cmd, args, options); - cancellationToken?.onCancellationRequested(() => { - if (process.platform === "win32" && p.pid) { - // On windows aborting the signal doesn't kill the CLI. - // Use taskkill here with the "force" and "tree" flags (to kill sub-processes too) - spawn("taskkill", ["/pid", String(p.pid), "/T", "/F"]); - } else { - abortController.abort(); - } - }); - const {stdout, stderr} = await waitForProcess(p, onStdOut, onStdError); - logger?.info(displayLogs.end, { - bundleOpName, + result = await runCli(cmd, args, { + cwd: workspaceFolder.fsPath, + env: removeUndefinedKeys(env), + token: cancellationToken, + escapeCommandForWindows: true, + onStdout: onStdOut, + onStderr: onStdError, }); - logger?.debug("output", {stdout, stderr, bundleOpName}); - return {stdout, stderr}; } catch (e: any) { + // A genuine spawn/stream failure (e.g. the binary is missing). if (cancellationToken?.isCancellationRequested) { logger?.warn(`${displayLogs.error} Reason: Cancelled`, { bundleOpName, }); throw new CancellationError(); - } else { - logger?.error(`${displayLogs.error} ${e.message ?? ""}`, { - ...e, - bundleOpName, - }); - throw new ProcessError(e.message, e.code); } + logger?.error(`${displayLogs.error} ${e.message ?? ""}`, { + ...e, + bundleOpName, + }); + throw new ProcessError(e.message, e.code ?? null); + } + + if (result.cancelled) { + logger?.warn(`${displayLogs.error} Reason: Cancelled`, {bundleOpName}); + throw new CancellationError(); } + if (result.exitCode !== 0) { + // stderr was streamed to onStdError (and the logs), so the error itself + // carries no message — the detail lives in the "Show Logs" channel. + logger?.error(displayLogs.error, {bundleOpName}); + throw new ProcessError("", result.exitCode); + } + + logger?.info(displayLogs.end, {bundleOpName}); + logger?.debug("output", { + stdout: result.stdout, + stderr: result.stderr, + bundleOpName, + }); + return {stdout: result.stdout, stderr: result.stderr}; } /** * Entrypoint for all wrapped CLI commands diff --git a/packages/databricks-vscode/src/cli/cliProcess.test.ts b/packages/databricks-vscode/src/cli/cliProcess.test.ts new file mode 100644 index 000000000..28bba01b4 --- /dev/null +++ b/packages/databricks-vscode/src/cli/cliProcess.test.ts @@ -0,0 +1,361 @@ +import {expect} from "chai"; +import {EventEmitter} from "node:events"; +import { + run, + SpawnFn, + TerminatePrimitives, + terminateProcessTree, + getEscapedCommandAndArgs, +} from "./cliProcess"; + +/** + * A fake child process that emits scripted stdout/stderr then closes. Lets us + * drive {@link run} without spawning a real binary. `stdout`/`stderr` may be a + * single string or a list of raw byte chunks, so tests can reproduce a payload + * split across "data" events (e.g. a multi-byte char straddling the boundary). + */ +function fakeSpawn(script: { + stdout?: string | Buffer[]; + stderr?: string | Buffer[]; + code?: number; + spawnError?: Error; + onKill?: () => void; + onStdinEnd?: () => void; + captureArgs?: (cmd: string, args: string[], opts: any) => void; +}): SpawnFn { + const toChunks = (v?: string | Buffer[]): Buffer[] => { + if (v === undefined) { + return []; + } + return typeof v === "string" ? [Buffer.from(v)] : v; + }; + return ((cmd: string, args: string[], opts: any) => { + script.captureArgs?.(cmd, args, opts); + const child: any = new EventEmitter(); + child.stdout = new EventEmitter(); + child.stderr = new EventEmitter(); + child.stdin = {end: () => script.onStdinEnd?.()}; + child.kill = () => script.onKill?.(); + setImmediate(() => { + if (script.spawnError) { + child.emit("error", script.spawnError); + return; + } + for (const chunk of toChunks(script.stderr)) { + child.stderr.emit("data", chunk); + } + for (const chunk of toChunks(script.stdout)) { + child.stdout.emit("data", chunk); + } + child.emit("close", script.code ?? 0); + }); + return child; + }) as unknown as SpawnFn; +} + +describe("cliProcess.run", () => { + it("captures stdout, stderr and the exit code on a normal exit", async () => { + const result = await run("/fake/cli", ["version"], { + spawnFn: fakeSpawn({stdout: "hello", stderr: "warn", code: 0}), + }); + expect(result.stdout).to.equal("hello"); + expect(result.stderr).to.equal("warn"); + expect(result.exitCode).to.equal(0); + expect(result.cancelled).to.equal(false); + }); + + it("resolves (does not throw) on a non-zero exit, reporting the code", async () => { + // Policy on the exit code belongs to the caller, not the gateway: a + // failing CLI still produced stdout/stderr the adapter may want to read. + const result = await run("/fake/cli", ["bundle", "deploy"], { + spawnFn: fakeSpawn({stdout: "", stderr: "boom", code: 1}), + }); + expect(result.exitCode).to.equal(1); + expect(result.stderr).to.equal("boom"); + }); + + it("spawns the command in the given cwd and env", async () => { + let seenCmd = ""; + let seenArgs: string[] = []; + let seenOpts: any; + await run("/custom/cli", ["auth", "profiles"], { + cwd: "/my/project", + /* eslint-disable-next-line @typescript-eslint/naming-convention */ + env: {DATABRICKS_CONFIG_PROFILE: "prod"}, + spawnFn: fakeSpawn({ + captureArgs: (c, a, opts) => { + seenCmd = c; + seenArgs = a; + seenOpts = opts; + }, + }), + }); + expect(seenCmd).to.equal("/custom/cli"); + expect(seenArgs).to.deep.equal(["auth", "profiles"]); + expect(seenOpts.cwd).to.equal("/my/project"); + expect(seenOpts.env).to.deep.equal({ + /* eslint-disable-next-line @typescript-eslint/naming-convention */ + DATABRICKS_CONFIG_PROFILE: "prod", + }); + }); + + it("forwards the shell option to spawn", async () => { + let seenShell: boolean | undefined; + await run("az", ["--version"], { + shell: true, + spawnFn: fakeSpawn({ + captureArgs: (_c, _a, opts) => { + seenShell = opts.shell; + }, + }), + }); + expect(seenShell).to.equal(true); + }); + + it("spawns detached on POSIX so the whole process group can be killed", async () => { + let seenDetached: boolean | undefined; + await run("/fake/cli", [], { + spawnFn: fakeSpawn({ + captureArgs: (_c, _a, opts) => { + seenDetached = opts.detached; + }, + }), + }); + // detached everywhere except Windows (where taskkill /T handles the + // tree and detached would spawn an extra console window). + expect(seenDetached).to.equal(process.platform !== "win32"); + }); + + it("streams decoded stdout and stderr chunks to the callbacks", async () => { + const out: string[] = []; + const err: string[] = []; + await run("/fake/cli", [], { + spawnFn: fakeSpawn({stdout: "line1\n", stderr: "warn\n"}), + onStdout: (c) => out.push(c), + onStderr: (c) => err.push(c), + }); + expect(out.join("")).to.equal("line1\n"); + expect(err.join("")).to.equal("warn\n"); + }); + + it("decodes a multi-byte char split across two stdout chunks", async () => { + // "…" (U+2026) is 3 bytes in UTF-8; split it down the middle so each + // chunk holds a partial code point. Naive per-chunk toString() would + // corrupt it to U+FFFD. + const bytes = Buffer.from("a…b", "utf8"); + const cut = bytes.indexOf(0xe2) + 1; // mid-ellipsis + const result = await run("/fake/cli", [], { + spawnFn: fakeSpawn({ + stdout: [bytes.subarray(0, cut), bytes.subarray(cut)], + }), + }); + expect(result.stdout).to.equal("a…b"); + }); + + it("flushes a partial trailing stderr byte to onStderr at close", async () => { + // stderr ends mid-"…" (U+2026, 3 bytes) and the completing bytes never + // arrive: the streaming decoder holds the incomplete sequence back on + // the data event, so the tail must be flushed when the process closes. + const full = Buffer.from("done…", "utf8"); + const truncated = full.subarray(0, full.length - 1); // drop last byte + const logs: string[] = []; + await run("/fake/cli", [], { + spawnFn: fakeSpawn({stderr: [truncated]}), + onStderr: (c) => logs.push(c), + }); + const joined = logs.join(""); + expect(joined.startsWith("done")).to.equal(true); + expect(joined.length).to.be.greaterThan("done".length); + }); + + it("ends the child's stdin when closeStdin is set", async () => { + let ended = false; + await run("/fake/cli", [], { + closeStdin: true, + spawnFn: fakeSpawn({onStdinEnd: () => (ended = true)}), + }); + expect(ended).to.equal(true); + }); + + it("rejects when the process fails to spawn", async () => { + let threw = false; + try { + await run("/missing/cli", [], { + spawnFn: fakeSpawn({spawnError: new Error("spawn ENOENT")}), + }); + } catch (e) { + threw = true; + expect((e as Error).message).to.contain("ENOENT"); + } + expect(threw).to.equal(true); + }); + + // A process that hangs until terminated, then emits "close" (as a real + // SIGTERM'd process would), letting the run promise settle. + const hangingSpawn: SpawnFn = (() => { + const child: any = new EventEmitter(); + child.stdout = new EventEmitter(); + child.stderr = new EventEmitter(); + child.stdin = {end: () => {}}; + child.kill = () => child.emit("close", null); + child.pid = 1234; + return child; + }) as unknown as SpawnFn; + + // A cancellation token whose callback fires on the next tick, tracking + // whether its subscription was disposed. + function nextTickToken() { + let disposed = false; + return { + disposed: () => disposed, + token: { + isCancellationRequested: false, + onCancellationRequested: (cb: () => void) => { + setImmediate(cb); + return { + dispose() { + disposed = true; + }, + }; + }, + }, + }; + } + + it("terminates the child via the injected terminator and reports cancelled", async () => { + let terminatedChild: any; + const {token} = nextTickToken(); + const result = await run("/fake/cli", [], { + token, + spawnFn: hangingSpawn, + terminateFn: (child) => { + terminatedChild = child; + (child as any).kill(); + }, + }); + expect(terminatedChild?.pid).to.equal(1234); + expect(result.cancelled).to.equal(true); + }); + + it("disposes the cancellation subscription on a normal exit", async () => { + const {token, disposed} = nextTickToken(); + // never actually cancels (callback fires but process already closed); + // use a fast-closing spawn so the run completes normally. + await run("/fake/cli", [], { + token: { + isCancellationRequested: false, + onCancellationRequested: token.onCancellationRequested, + }, + spawnFn: fakeSpawn({stdout: "ok"}), + }); + expect(disposed()).to.equal(true); + }); + + it("does not let a throwing terminator escape cancellation", async () => { + const {token} = nextTickToken(); + // Must settle cleanly (as cancelled), not blow up synchronously. + const result = await run("/fake/cli", [], { + token, + spawnFn: hangingSpawn, + terminateFn: (child) => { + (child as any).kill(); + throw new Error("kill failed"); + }, + }); + expect(result.cancelled).to.equal(true); + }); +}); + +describe("cliProcess.getEscapedCommandAndArgs", () => { + it("passes the command through unchanged on POSIX", () => { + const {cmd, args, windowsVerbatimArguments} = getEscapedCommandAndArgs( + "/bin/databricks", + ["bundle", "deploy"], + "linux" + ); + expect(cmd).to.equal("/bin/databricks"); + expect(args).to.deep.equal(["bundle", "deploy"]); + expect(windowsVerbatimArguments).to.equal(undefined); + }); + + it("wraps the command in cmd.exe with verbatim args on Windows", () => { + const {cmd, args, windowsVerbatimArguments} = getEscapedCommandAndArgs( + "C:\\bin\\databricks.exe", + ["bundle", "deploy"], + "win32" + ); + expect(cmd).to.equal("cmd.exe"); + // /d disables AutoRun, /c runs the command; the whole thing is one + // double-quoted string so paths/args with spaces survive. + expect(args[0]).to.equal("/d"); + expect(args[1]).to.equal("/c"); + expect(args[2]).to.equal( + '""C:\\bin\\databricks.exe" "bundle" "deploy""' + ); + expect(windowsVerbatimArguments).to.equal(true); + }); +}); + +describe("terminateProcessTree", () => { + // Records what the terminator did, plus a fake child whose direct kill() + // we can observe. `killThrows` simulates the group being gone (ESRCH). + function harness(platform: NodeJS.Platform, killThrows = false) { + const calls = { + groupKill: [] as Array<[number, NodeJS.Signals]>, + spawned: [] as Array<[string, string[]]>, + directKill: [] as (NodeJS.Signals | undefined)[], + }; + const prims: TerminatePrimitives = { + platform, + kill: (pid, signal) => { + if (killThrows) { + throw new Error("ESRCH"); + } + calls.groupKill.push([pid, signal]); + }, + spawnHelper: (cmd, args) => calls.spawned.push([cmd, args]), + }; + const child: any = new EventEmitter(); + child.pid = 4321; + child.kill = (signal?: NodeJS.Signals) => calls.directKill.push(signal); + return {calls, prims, child}; + } + + it("kills the negated pid (process group) with SIGTERM on POSIX", () => { + const {calls, prims, child} = harness("linux"); + terminateProcessTree(child, prims); + expect(calls.groupKill).to.deep.equal([[-4321, "SIGTERM"]]); + expect(calls.directKill).to.be.empty; + }); + + it("falls back to a direct child kill when the group is already gone", () => { + const {calls, prims, child} = harness("darwin", /*killThrows*/ true); + terminateProcessTree(child, prims); + expect(calls.directKill).to.deep.equal(["SIGTERM"]); + }); + + it("shells taskkill /T /F with the string pid on Windows", () => { + const {calls, prims, child} = harness("win32"); + terminateProcessTree(child, prims); + expect(calls.spawned).to.deep.equal([ + ["taskkill", ["/pid", "4321", "/T", "/F"]], + ]); + expect(calls.directKill).to.be.empty; + }); + + it("falls back to a direct kill when the child has no pid", () => { + const {calls, prims, child} = harness("linux"); + child.pid = undefined; + terminateProcessTree(child, prims); + expect(calls.groupKill).to.be.empty; + expect(calls.directKill).to.deep.equal(["SIGTERM"]); + }); + + it("falls back to a direct kill on Windows when the child has no pid", () => { + const {calls, prims, child} = harness("win32"); + child.pid = undefined; + terminateProcessTree(child, prims); + expect(calls.spawned).to.be.empty; + expect(calls.directKill).to.deep.equal(["SIGTERM"]); + }); +}); diff --git a/packages/databricks-vscode/src/cli/cliProcess.ts b/packages/databricks-vscode/src/cli/cliProcess.ts new file mode 100644 index 000000000..f5cb477a5 --- /dev/null +++ b/packages/databricks-vscode/src/cli/cliProcess.ts @@ -0,0 +1,303 @@ +import { + spawn as nodeSpawn, + ChildProcessWithoutNullStreams, +} from "node:child_process"; +import {StringDecoder} from "node:string_decoder"; + +/** + * Minimal cancellation signal, structurally compatible with `vscode.Cancellation + * Token`. Declared locally so this module carries no `vscode` import and stays + * unit-testable without the extension host. + */ +export interface CancellationLike { + readonly isCancellationRequested: boolean; + onCancellationRequested(listener: () => void): {dispose(): void}; +} + +/** + * Injectable spawn seam. The real implementation is Node's `child_process. + * spawn`; tests pass a fake that emits scripted stdout/stderr. + */ +export type SpawnFn = ( + command: string, + args: string[], + options: { + cwd?: string; + env?: NodeJS.ProcessEnv; + detached?: boolean; + shell?: boolean; + windowsVerbatimArguments?: boolean; + } +) => ChildProcessWithoutNullStreams; + +/** + * Injectable process-tree terminator. The default kills the whole tree on both + * platforms (see {@link terminateProcessTree}). Injectable so cancellation is + * assertable in tests without spawning real processes. + */ +export type TerminateFn = (child: ChildProcessWithoutNullStreams) => void; + +/** + * OS primitives used by {@link terminateProcessTree}, injectable so the + * termination logic can be unit-tested without touching real processes. + */ +export interface TerminatePrimitives { + platform: NodeJS.Platform; + /** Send `signal` to `pid` (a negative pid targets the process group). */ + kill(pid: number, signal: NodeJS.Signals): void; + /** Fire-and-forget helper spawn (e.g. `taskkill`). */ + spawnHelper(command: string, args: string[]): void; +} + +const realTerminatePrimitives: TerminatePrimitives = { + platform: process.platform, + kill: (pid, signal) => process.kill(pid, signal), + spawnHelper: (command, args) => { + // Swallow the async spawn error (e.g. taskkill not on PATH) so it can't + // become an uncaught exception in the extension host during cancel. + nodeSpawn(command, args).on("error", () => {}); + }, +}; + +/** + * Kill the whole process tree, injectable primitives and all. On Windows a + * plain `SIGTERM` doesn't reach a CLI spawned via `cmd.exe`, so force-kill the + * tree with `taskkill /T /F`; on POSIX the child is a process-group leader + * (spawned detached), so signalling the negated pid tears down the whole group + * — including any grandchild a direct-child `SIGTERM` would orphan (e.g. + * `terraform`, `uv`) — with a fallback to a direct kill if the group is + * already gone. + */ +export function terminateProcessTree( + child: ChildProcessWithoutNullStreams, + prims: TerminatePrimitives = realTerminatePrimitives +): void { + const pid = child.pid; + if (prims.platform === "win32") { + if (pid) { + prims.spawnHelper("taskkill", ["/pid", String(pid), "/T", "/F"]); + return; + } + } else if (pid) { + try { + prims.kill(-pid, "SIGTERM"); + return; + } catch { + // Group already gone / not a leader — fall through to a direct kill. + } + } + child.kill("SIGTERM"); +} + +const defaultTerminate: TerminateFn = (child) => terminateProcessTree(child); + +/** + * Escape a command for a shell we hand it to. On Windows we route through + * `cmd.exe` (`/d` disables AutoRun, `/c` runs then exits) with the whole + * command as one double-quoted string plus `windowsVerbatimArguments`, so a + * path or argument containing spaces survives Node's own arg-quoting. On POSIX + * the command is passed through unchanged. + * + * `platform` is injectable so both branches are unit-testable on one host. + */ +export function getEscapedCommandAndArgs( + command: string, + args: string[], + platform: NodeJS.Platform = process.platform +): {cmd: string; args: string[]; windowsVerbatimArguments?: boolean} { + if (platform === "win32") { + return { + cmd: "cmd.exe", + args: [ + "/d", + "/c", + `""${command}" ${args.map((a) => `"${a}"`).join(" ")}"`, + ], + windowsVerbatimArguments: true, + }; + } + return {cmd: command, args}; +} + +export interface CliRunResult { + /** Full stdout, decoded once at the end (never corrupts a split code point). */ + stdout: string; + /** Full stderr, decoded once at the end. */ + stderr: string; + /** Process exit code, or `null` when the process was killed by a signal. */ + exitCode: number | null; + /** True when the run was aborted via its cancellation token. */ + cancelled: boolean; +} + +export interface CliRunOptions { + cwd?: string; + env?: NodeJS.ProcessEnv; + /** When cancelled, the whole child process tree is terminated. */ + token?: CancellationLike; + /** + * Close the child's stdin immediately after spawning. Node gives the child + * an open stdin pipe that never receives EOF, so a CLI command that prompts + * for confirmation (e.g. `aitools update`) blocks forever. Ending stdin + * delivers EOF so the prompt resolves. Only set for non-interactive + * commands we never feed input to. + */ + closeStdin?: boolean; + /** + * Run the command through the OS shell (Node's `spawn` `shell` option). + * Used by callers that resolve a bare command name on the PATH (e.g. `az`, + * the host CLI) rather than an absolute binary path. + */ + shell?: boolean; + /** + * Route the command through `cmd.exe` on Windows (see + * {@link getEscapedCommandAndArgs}). The buffered/bundle callers set this; + * callers that spawn a resolved `.exe` path directly leave it off. + */ + escapeCommandForWindows?: boolean; + /** Receives decoded stdout chunks as they arrive (e.g. logger narration). */ + onStdout?: (chunk: string) => void; + /** Receives decoded stderr chunks as they arrive (e.g. the "Show Logs" channel). */ + onStderr?: (chunk: string) => void; + /** Injectable spawn seam (defaults to Node's `spawn`). */ + spawnFn?: SpawnFn; + /** Injectable process-tree terminator (defaults to killing the whole tree). */ + terminateFn?: TerminateFn; +} + +/** + * Run a child process and resolve with its captured output and exit code. + * + * The single low-level execution seam behind every request/response CLI call + * (buffered helpers, bundle streaming, and the python-setup client). It carries + * no `vscode` import so it is fully unit-testable via the injected spawn seam. + * + * Resolves on **any** process exit — a non-zero exit is not an error here; + * policy on the exit code belongs to the caller. A cancelled run resolves with + * `cancelled: true`. Rejects only on a genuine spawn/stream failure (e.g. + * `ENOENT`, `EPIPE`). + */ +export function run( + command: string, + args: string[], + options: CliRunOptions = {} +): Promise { + const spawnFn = options.spawnFn ?? (nodeSpawn as unknown as SpawnFn); + const terminateFn = options.terminateFn ?? defaultTerminate; + const { + cmd, + args: spawnArgs, + windowsVerbatimArguments, + } = options.escapeCommandForWindows + ? getEscapedCommandAndArgs(command, args) + : {cmd: command, args, windowsVerbatimArguments: undefined}; + + return new Promise((resolve, reject) => { + let child: ChildProcessWithoutNullStreams; + try { + child = spawnFn(cmd, spawnArgs, { + cwd: options.cwd, + env: options.env, + // Give the child its own process group on POSIX so cancellation + // can kill the whole tree. On Windows `detached` opens a new + // console; taskkill /T handles the tree there, so leave it off. + detached: process.platform !== "win32", + shell: options.shell, + windowsVerbatimArguments, + }); + } catch (e) { + reject(e as Error); + return; + } + + if (options.closeStdin) { + child.stdin?.end(); + } + + // Accumulate raw bytes and decode once at the end: a multi-byte UTF-8 + // sequence can straddle two "data" events, and decoding each chunk in + // isolation would corrupt it. Chunks streamed to onStdout/onStderr go + // through a StringDecoder that holds back a trailing partial code point + // until the next chunk (or the final flush at close). + const stdoutChunks: Buffer[] = []; + const stderrChunks: Buffer[] = []; + const stdoutStreamDecoder = new StringDecoder("utf8"); + const stderrStreamDecoder = new StringDecoder("utf8"); + let settled = false; + let cancelled = false; + + const requestCancel = () => { + cancelled = true; + // Best-effort terminate; the "close"/"error" handler still fires and + // settles the promise, so cancellation never leaves it hanging. + // Guard against terminate throwing (e.g. the child is already gone) + // so it can't escape this callback. + try { + terminateFn(child); + } catch { + // ignore — the process-exit handler will settle the promise + } + }; + const cancelSub = options.token?.onCancellationRequested(requestCancel); + + const finish = (fn: () => void) => { + if (settled) { + return; + } + settled = true; + cancelSub?.dispose(); + fn(); + }; + + child.stdout.on("data", (b: Buffer) => { + stdoutChunks.push(b); + if (options.onStdout) { + const text = stdoutStreamDecoder.write(b); + if (text) { + options.onStdout(text); + } + } + }); + child.stderr.on("data", (b: Buffer) => { + stderrChunks.push(b); + if (options.onStderr) { + const text = stderrStreamDecoder.write(b); + if (text) { + options.onStderr(text); + } + } + }); + + // A stream-level error (e.g. EPIPE) would otherwise be an unhandled + // "error" event, which Node throws as an uncaught exception in the + // extension host. Route it through finish() like any other failure. + child.stdout.on("error", (err: Error) => finish(() => reject(err))); + child.stderr.on("error", (err: Error) => finish(() => reject(err))); + child.on("error", (err: Error) => finish(() => reject(err))); + + child.on("close", (code) => + finish(() => { + // Flush any partial trailing byte each streaming decoder held + // back, so the callbacks don't miss the final fragment. + if (options.onStdout) { + const tail = stdoutStreamDecoder.end(); + if (tail) { + options.onStdout(tail); + } + } + if (options.onStderr) { + const tail = stderrStreamDecoder.end(); + if (tail) { + options.onStderr(tail); + } + } + resolve({ + stdout: Buffer.concat(stdoutChunks).toString("utf8"), + stderr: Buffer.concat(stderrChunks).toString("utf8"), + exitCode: code, + cancelled, + }); + }) + ); + }); +} diff --git a/packages/databricks-vscode/src/python-setup/gateways/PythonSetupCliClient.test.ts b/packages/databricks-vscode/src/python-setup/gateways/PythonSetupCliClient.test.ts index 1b6c9e272..56e5a2cb1 100644 --- a/packages/databricks-vscode/src/python-setup/gateways/PythonSetupCliClient.test.ts +++ b/packages/databricks-vscode/src/python-setup/gateways/PythonSetupCliClient.test.ts @@ -4,8 +4,6 @@ import { PythonSetupCancelledError, PythonSetupCliClient, SpawnFn, - TerminatePrimitives, - terminateProcessTree, } from "./PythonSetupCliClient"; import { SUCCESS_DEFAULT, @@ -27,10 +25,10 @@ function fakeSpawn(script: { captureArgs?: ( cmd: string, args: string[], - cwd: string, - detached: boolean + cwd: string | undefined, + detached: boolean | undefined ) => void; - captureEnv?: (env: NodeJS.ProcessEnv) => void; + captureEnv?: (env: NodeJS.ProcessEnv | undefined) => void; }): SpawnFn { const toChunks = (v?: string | Buffer[]): Buffer[] => { if (v === undefined) { @@ -44,6 +42,7 @@ function fakeSpawn(script: { const child: any = new EventEmitter(); child.stdout = new EventEmitter(); child.stderr = new EventEmitter(); + child.stdin = {end: () => {}}; child.kill = () => script.onKill?.(); setImmediate(() => { if (script.spawnError) { @@ -93,7 +92,7 @@ describe("PythonSetupCliClient", () => { it("spawns the resolved path with setup-local argv in the given cwd", async () => { let seenCmd = ""; let seenArgs: string[] = []; - let seenCwd = ""; + let seenCwd: string | undefined = ""; const client = new PythonSetupCliClient( () => "/custom/databricks", () => ({}), @@ -161,7 +160,7 @@ describe("PythonSetupCliClient", () => { () => ({DATABRICKS_CONFIG_PROFILE: profiles[call++]}), fakeSpawn({ stdout: JSON.stringify(SUCCESS_DEFAULT), - captureEnv: (env) => seen.push(env.DATABRICKS_CONFIG_PROFILE), + captureEnv: (env) => seen.push(env?.DATABRICKS_CONFIG_PROFILE), }) ); await client.run(inv, {cwd: "/proj"}); @@ -406,67 +405,3 @@ describe("PythonSetupCliClient", () => { expect(caught).to.be.instanceOf(PythonSetupCancelledError); }); }); - -describe("terminateProcessTree", () => { - // Records what the terminator did, plus a fake child whose direct kill() - // we can observe. `killThrows` simulates the group being gone (ESRCH). - function harness(platform: NodeJS.Platform, killThrows = false) { - const calls = { - groupKill: [] as Array<[number, NodeJS.Signals]>, - spawned: [] as Array<[string, string[]]>, - directKill: [] as (NodeJS.Signals | undefined)[], - }; - const prims: TerminatePrimitives = { - platform, - kill: (pid, signal) => { - if (killThrows) { - throw new Error("ESRCH"); - } - calls.groupKill.push([pid, signal]); - }, - spawnHelper: (cmd, args) => calls.spawned.push([cmd, args]), - }; - const child: any = new EventEmitter(); - child.pid = 4321; - child.kill = (signal?: NodeJS.Signals) => calls.directKill.push(signal); - return {calls, prims, child}; - } - - it("kills the negated pid (process group) with SIGTERM on POSIX", () => { - const {calls, prims, child} = harness("linux"); - terminateProcessTree(child, prims); - expect(calls.groupKill).to.deep.equal([[-4321, "SIGTERM"]]); - expect(calls.directKill).to.be.empty; - }); - - it("falls back to a direct child kill when the group is already gone", () => { - const {calls, prims, child} = harness("darwin", /*killThrows*/ true); - terminateProcessTree(child, prims); - expect(calls.directKill).to.deep.equal(["SIGTERM"]); - }); - - it("shells taskkill /T /F with the string pid on Windows", () => { - const {calls, prims, child} = harness("win32"); - terminateProcessTree(child, prims); - expect(calls.spawned).to.deep.equal([ - ["taskkill", ["/pid", "4321", "/T", "/F"]], - ]); - expect(calls.directKill).to.be.empty; - }); - - it("falls back to a direct kill when the child has no pid", () => { - const {calls, prims, child} = harness("linux"); - child.pid = undefined; - terminateProcessTree(child, prims); - expect(calls.groupKill).to.be.empty; - expect(calls.directKill).to.deep.equal(["SIGTERM"]); - }); - - it("falls back to a direct kill on Windows when the child has no pid", () => { - const {calls, prims, child} = harness("win32"); - child.pid = undefined; - terminateProcessTree(child, prims); - expect(calls.spawned).to.be.empty; - expect(calls.directKill).to.deep.equal(["SIGTERM"]); - }); -}); diff --git a/packages/databricks-vscode/src/python-setup/gateways/PythonSetupCliClient.ts b/packages/databricks-vscode/src/python-setup/gateways/PythonSetupCliClient.ts index 6903c192c..a2cc75bdc 100644 --- a/packages/databricks-vscode/src/python-setup/gateways/PythonSetupCliClient.ts +++ b/packages/databricks-vscode/src/python-setup/gateways/PythonSetupCliClient.ts @@ -1,8 +1,9 @@ import { - spawn as nodeSpawn, - ChildProcessWithoutNullStreams, -} from "node:child_process"; -import {StringDecoder} from "node:string_decoder"; + run as runCli, + CancellationLike, + SpawnFn, + TerminateFn, +} from "../../cli/cliProcess"; import { buildSetupLocalArgs, SetupLocalInvocation, @@ -12,91 +13,10 @@ import { PythonSetupResult, } from "../models/PythonSetupResult"; -/** - * Minimal cancellation signal, structurally compatible with `vscode.Cancellation - * Token`. Declared locally so this gateway carries no `vscode` import and stays - * unit-testable without the extension host. - */ -export interface CancellationLike { - readonly isCancellationRequested: boolean; - onCancellationRequested(listener: () => void): {dispose(): void}; -} - -/** - * Injectable spawn seam. The real implementation is Node's `child_process. - * spawn`; tests pass a fake that emits scripted stdout/stderr. - */ -export type SpawnFn = ( - command: string, - args: string[], - options: {cwd: string; env: NodeJS.ProcessEnv; detached: boolean} -) => ChildProcessWithoutNullStreams; - -/** - * Injectable process-tree terminator. `setup-local` spawns `uv sync` as a - * grandchild, and killing only the direct `databricks` child orphans it: on - * Windows a plain `SIGTERM` doesn't even reach the CLI, and on POSIX a signal - * to the direct child is not propagated to grandchildren. The default kills the - * whole tree on both — `taskkill /T /F` on Windows, and (because the child is - * spawned {@link https://nodejs.org/api/child_process.html detached}, i.e. its - * own process-group leader) a `SIGTERM` to the negated pid on POSIX, which - * signals the entire group. Injectable so cancellation is assertable in tests - * without spawning real processes. - */ -export type TerminateFn = (child: ChildProcessWithoutNullStreams) => void; - -/** - * OS primitives used by {@link terminateProcessTree}, injectable so the - * termination logic can be unit-tested without touching real processes. - */ -export interface TerminatePrimitives { - platform: NodeJS.Platform; - /** Send `signal` to `pid` (a negative pid targets the process group). */ - kill(pid: number, signal: NodeJS.Signals): void; - /** Fire-and-forget helper spawn (e.g. `taskkill`). */ - spawnHelper(command: string, args: string[]): void; -} - -const realTerminatePrimitives: TerminatePrimitives = { - platform: process.platform, - kill: (pid, signal) => process.kill(pid, signal), - spawnHelper: (command, args) => { - // Swallow the async spawn error (e.g. taskkill not on PATH) so it can't - // become an uncaught exception in the extension host during cancel. - nodeSpawn(command, args).on("error", () => {}); - }, -}; - -/** - * Kill the whole `setup-local` process tree, injectable primitives and all. On - * Windows a plain `SIGTERM` doesn't reach the CLI, so force-kill the tree with - * `taskkill /T /F`; on POSIX the child is a process-group leader (spawned - * detached), so signalling the negated pid tears down the whole group — - * including the `uv` grandchild a direct-child SIGTERM would orphan — with a - * fallback to a direct kill if the group is already gone. - */ -export function terminateProcessTree( - child: ChildProcessWithoutNullStreams, - prims: TerminatePrimitives = realTerminatePrimitives -): void { - const pid = child.pid; - if (prims.platform === "win32") { - if (pid) { - prims.spawnHelper("taskkill", ["/pid", String(pid), "/T", "/F"]); - return; - } - } else if (pid) { - try { - prims.kill(-pid, "SIGTERM"); - return; - } catch { - // Group already gone / not a leader — fall through to a direct kill. - } - } - child.kill("SIGTERM"); -} - -const defaultTerminate: TerminateFn = (child) => terminateProcessTree(child); +// Re-exported so consumers (e.g. PythonSetupDriftManager) and this gateway's +// tests keep a single import site while the process primitives live in the +// shared, vscode-free cli/cliProcess module. +export type {CancellationLike, SpawnFn} from "../../cli/cliProcess"; /** * Rejection raised when a {@link PythonSetupCliClient.run} is aborted via its @@ -135,30 +55,28 @@ export type EnvFn = () => NodeJS.ProcessEnv; /** * Gateway to the `databricks environments setup-local` CLI command. * - * Runs the CLI with `--output json`, captures stdout (the single structured - * {@link PythonSetupResult}) and stderr (raw logs), and resolves with the parsed - * result on both success and failure exits — the caller branches on `result.ok` - * / `result.error`, not on the process exit code. A cancelled run rejects with - * {@link PythonSetupCancelledError} so callers can tell it apart from a genuine - * CLI failure. + * Builds the argv and drives the shared {@link runCli} process seam with + * `--output json`, capturing stdout (the single structured + * {@link PythonSetupResult}) and forwarding stderr (raw logs) to `onLog`. It + * resolves with the parsed result on both success and failure exits — the + * caller branches on `result.ok` / `result.error`, not the process exit code. + * A cancelled run rejects with {@link PythonSetupCancelledError} so callers can + * tell it apart from a genuine CLI failure. * * There is deliberately no live phase narration: under `--output json` the CLI * emits only the final JSON object on stdout (per-phase text is text-mode only, * and `uv sync` output is buffered), so callers show an indeterminate progress * indicator and read the per-phase outcomes from `result.phases` afterwards. - * - * The client itself does no I/O beyond spawning: path resolution and argv are - * pure helpers, and the spawn function is injected, so it is fully unit-testable. */ export class PythonSetupCliClient { constructor( private readonly resolvePath: () => string, private readonly envFn: EnvFn, - private readonly spawnFn: SpawnFn = nodeSpawn as unknown as SpawnFn, - private readonly terminateFn: TerminateFn = defaultTerminate + private readonly spawnFn?: SpawnFn, + private readonly terminateFn?: TerminateFn ) {} - run( + async run( invocation: SetupLocalInvocation, options: RunOptions ): Promise { @@ -166,111 +84,39 @@ export class PythonSetupCliClient { // request is already cancelled don't even start it — reject before // spawning rather than spawn-then-kill, which would let it run briefly. if (options.token?.isCancellationRequested) { - return Promise.reject(new PythonSetupCancelledError()); + throw new PythonSetupCancelledError(); } - const args = buildSetupLocalArgs(invocation); - return new Promise((resolve, reject) => { - let child: ChildProcessWithoutNullStreams; - try { - child = this.spawnFn(this.resolvePath(), args, { - cwd: options.cwd, - env: this.envFn(), - // Give the child its own process group on POSIX so cancel - // can kill the whole tree (see defaultTerminate). On Windows - // `detached` opens a new console; we use taskkill there, so - // leave it off. - detached: process.platform !== "win32", - }); - } catch (e) { - reject(e as Error); - return; - } - - // Accumulate raw bytes and decode once at the end: a multi-byte - // UTF-8 sequence can straddle two "data" chunks, and decoding each - // chunk in isolation would corrupt it (turning a valid JSON payload - // into U+FFFD garbage that fails to parse). stderr is streamed to - // onLog as it arrives, so it goes through a StringDecoder that holds - // back any trailing partial code point until the next chunk. - const stdoutChunks: Buffer[] = []; - const stderrDecoder = new StringDecoder("utf8"); - let stderr = ""; - let settled = false; - let cancelled = false; - - const requestCancel = () => { - cancelled = true; - // Best-effort terminate; the "close"/"error" handler still - // fires and settles the promise, so cancellation never leaves - // it hanging. Guard against terminate throwing (e.g. the child - // is already gone) so it can't escape this callback. - try { - this.terminateFn(child); - } catch { - // ignore — the process-exit handler will settle the promise - } - }; - const cancelSub = - options.token?.onCancellationRequested(requestCancel); - - const finish = (fn: () => void) => { - if (settled) { - return; - } - settled = true; - cancelSub?.dispose(); - fn(); - }; - - child.stdout.on("data", (b: Buffer) => stdoutChunks.push(b)); - child.stderr.on("data", (b: Buffer) => { - const text = stderrDecoder.write(b); - if (text) { - stderr += text; - options.onLog?.(text); - } - }); - - // A stream-level error (e.g. EPIPE) would otherwise be an unhandled - // "error" event, which Node throws as an uncaught exception in the - // extension host. Route it through finish() like any other failure. - child.stdout.on("error", (err: Error) => finish(() => reject(err))); - child.stderr.on("error", (err: Error) => finish(() => reject(err))); + const result = await runCli( + this.resolvePath(), + buildSetupLocalArgs(invocation), + { + cwd: options.cwd, + env: this.envFn(), + token: options.token, + onStderr: options.onLog, + spawnFn: this.spawnFn, + terminateFn: this.terminateFn, + } + ); - child.on("error", (err: Error) => finish(() => reject(err))); + if (result.cancelled) { + throw new PythonSetupCancelledError(); + } - child.on("close", () => - finish(() => { - // Flush any partial trailing byte the decoder held back, and - // forward it to onLog too so the log channel isn't missing - // the final fragment. - const tail = stderrDecoder.end(); - if (tail) { - stderr += tail; - options.onLog?.(tail); - } - if (cancelled) { - reject(new PythonSetupCancelledError()); - return; - } - const stdout = Buffer.concat(stdoutChunks).toString("utf8"); - try { - resolve(parsePythonSetupResult(stdout)); - } catch (e) { - // No parseable result on stdout. Append captured stderr - // (uv/CLI diagnostics, or an auth error printed before a - // result object was built) so the failure is actionable, - // while preserving the original error's type. - const err = e as Error; - const detail = stderr.trim(); - if (detail) { - err.message = `${err.message}\n${detail}`; - } - reject(err); - } - }) - ); - }); + try { + return parsePythonSetupResult(result.stdout); + } catch (e) { + // No parseable result on stdout. Append captured stderr (uv/CLI + // diagnostics, or an auth error printed before a result object was + // built) so the failure is actionable, while preserving the + // original error's type. + const err = e as Error; + const detail = result.stderr.trim(); + if (detail) { + err.message = `${err.message}\n${detail}`; + } + throw err; + } } } From f67b0e2648f0936c2ef6276ef5bf5f88b841a95c Mon Sep 17 00:00:00 2001 From: "@rugpanov" Date: Mon, 24 Aug 2026 14:37:08 +0300 Subject: [PATCH 2/5] fix(cli): guarantee cancel settlement and terminate on stream errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit *Why* Review of the cliProcess seam surfaced two real robustness gaps introduced by routing every call through spawn + a cancellation token: - cancel could hang: the run resolved only on "close", so a child that ignored SIGTERM (or a failed taskkill) left bundle/aitools/python-setup operations pending forever. The old buffered execFile path settled promptly on cancel. - a stdout/stderr stream error rejected without terminating the child; since POSIX children are now spawned detached, the tree kept running after the caller saw a failure. *What* - On cancel, `run()` now fires the terminate and settles immediately as cancelled instead of waiting for "close"; a late "close" is ignored. Removes the hang while keeping the whole-tree kill. - On a stdout/stderr "error", `run()` terminates the process tree before rejecting (spawn-level "error" still just rejects — there is no process yet). - Forward-declared the cancellation subscription so a synchronously-firing token can't hit a temporal-dead-zone in `finish`. *Verification* - New `cliProcess.test.ts` cases: "settles promptly on cancel even if the process never closes" and "terminates the process tree when a stdout stream error occurs". New `CliWrapper.test.ts` case pins the buffered non-zero-exit error surface (`.code`/`.stderr`/`.stdout` + stderr in `.message`) that `isFileNotFound` and the profile parser depend on. - `yarn test:unit` -> 933 passing, 0 failing. `yarn test:lint` -> clean. Co-authored-by: Isaac --- .../src/cli/CliWrapper.test.ts | 24 +++++ .../src/cli/cliProcess.test.ts | 52 +++++++++ .../databricks-vscode/src/cli/cliProcess.ts | 102 +++++++++++------- 3 files changed, 137 insertions(+), 41 deletions(-) diff --git a/packages/databricks-vscode/src/cli/CliWrapper.test.ts b/packages/databricks-vscode/src/cli/CliWrapper.test.ts index 0fe6e2465..60c81fb68 100644 --- a/packages/databricks-vscode/src/cli/CliWrapper.test.ts +++ b/packages/databricks-vscode/src/cli/CliWrapper.test.ts @@ -486,6 +486,30 @@ describe("cancellableExecFile closeStdin", () => { await settled; } }); + + // On a non-zero exit the thrown error must mirror Node's `execFile` + // rejection: stderr in `.message` (the profile parser greps it) plus + // numeric `.code` and `.stderr`/`.stdout` (what the SDK's `isFileNotFound` + // inspects). Regression guard for the spawn-based reimplementation. + it("throws a Node-execFile-shaped error on a non-zero exit", async () => { + let caught: any; + try { + await cancellableExecFile(process.execPath, [ + "-e", + "process.stderr.write('cannot parse config file'); process.stdout.write('partial'); process.exit(3);", + ]); + } catch (e) { + caught = e; + } + assert.ok(caught, "expected a rejection on non-zero exit"); + assert.strictEqual(caught.code, 3); + assert.strictEqual(caught.stderr, "cannot parse config file"); + assert.strictEqual(caught.stdout, "partial"); + assert.ok( + caught.message.includes("cannot parse config file"), + "stderr must be in the error message for the profile-parse checks" + ); + }); }); describe("ProcessError.showErrorMessage", () => { diff --git a/packages/databricks-vscode/src/cli/cliProcess.test.ts b/packages/databricks-vscode/src/cli/cliProcess.test.ts index 28bba01b4..15184bc27 100644 --- a/packages/databricks-vscode/src/cli/cliProcess.test.ts +++ b/packages/databricks-vscode/src/cli/cliProcess.test.ts @@ -264,6 +264,58 @@ describe("cliProcess.run", () => { }); expect(result.cancelled).to.equal(true); }); + + it("settles promptly on cancel even if the process never closes", async () => { + // A child that ignores termination and never emits "close". The run must + // still settle (as cancelled) rather than hang forever waiting on close. + const neverClosingSpawn: SpawnFn = (() => { + const child: any = new EventEmitter(); + child.stdout = new EventEmitter(); + child.stderr = new EventEmitter(); + child.stdin = {end: () => {}}; + child.pid = 4242; + child.kill = () => {}; // ignores the signal, never closes + return child; + }) as unknown as SpawnFn; + const {token} = nextTickToken(); + let terminated = false; + const result = await run("/fake/cli", [], { + token, + spawnFn: neverClosingSpawn, + terminateFn: () => (terminated = true), + }); + expect(result.cancelled).to.equal(true); + expect(terminated).to.equal(true); + }); + + it("terminates the process tree when a stdout stream error occurs", async () => { + // stdout errors mid-run (e.g. EPIPE). Because POSIX children are spawned + // detached, rejecting without killing would orphan the tree, so the + // terminator must run before the rejection. + let terminated = false; + const erroringSpawn: SpawnFn = (() => { + const child: any = new EventEmitter(); + child.stdout = new EventEmitter(); + child.stderr = new EventEmitter(); + child.stdin = {end: () => {}}; + child.pid = 99; + child.kill = () => {}; + setImmediate(() => child.stdout.emit("error", new Error("EPIPE"))); + return child; + }) as unknown as SpawnFn; + let threw = false; + try { + await run("/fake/cli", [], { + spawnFn: erroringSpawn, + terminateFn: () => (terminated = true), + }); + } catch (e) { + threw = true; + expect((e as Error).message).to.contain("EPIPE"); + } + expect(threw).to.equal(true); + expect(terminated).to.equal(true); + }); }); describe("cliProcess.getEscapedCommandAndArgs", () => { diff --git a/packages/databricks-vscode/src/cli/cliProcess.ts b/packages/databricks-vscode/src/cli/cliProcess.ts index f5cb477a5..61618e2f3 100644 --- a/packages/databricks-vscode/src/cli/cliProcess.ts +++ b/packages/databricks-vscode/src/cli/cliProcess.ts @@ -225,20 +225,9 @@ export function run( const stderrStreamDecoder = new StringDecoder("utf8"); let settled = false; let cancelled = false; - - const requestCancel = () => { - cancelled = true; - // Best-effort terminate; the "close"/"error" handler still fires and - // settles the promise, so cancellation never leaves it hanging. - // Guard against terminate throwing (e.g. the child is already gone) - // so it can't escape this callback. - try { - terminateFn(child); - } catch { - // ignore — the process-exit handler will settle the promise - } - }; - const cancelSub = options.token?.onCancellationRequested(requestCancel); + // Declared before `finish` (which disposes it) so a token that fires + // synchronously on subscription can't hit a temporal-dead-zone error. + let cancelSub: {dispose(): void} | undefined; const finish = (fn: () => void) => { if (settled) { @@ -249,6 +238,51 @@ export function run( fn(); }; + // Best-effort terminate that can never throw out of an event handler + // (e.g. the child is already gone). + const safeTerminate = () => { + try { + terminateFn(child); + } catch { + // ignore + } + }; + + const settleResult = (exitCode: number | null) => + finish(() => { + // Flush any partial trailing byte each streaming decoder held + // back, so the callbacks don't miss the final fragment. + if (options.onStdout) { + const tail = stdoutStreamDecoder.end(); + if (tail) { + options.onStdout(tail); + } + } + if (options.onStderr) { + const tail = stderrStreamDecoder.end(); + if (tail) { + options.onStderr(tail); + } + } + resolve({ + stdout: Buffer.concat(stdoutChunks).toString("utf8"), + stderr: Buffer.concat(stderrChunks).toString("utf8"), + exitCode, + cancelled, + }); + }); + + const requestCancel = () => { + cancelled = true; + safeTerminate(); + // Settle immediately as cancelled rather than waiting for "close": + // a process that ignores the signal (or a failed taskkill) must not + // leave the operation pending forever. The tree has been signalled; + // any late "close" is ignored once settled. + settleResult(null); + }; + cancelSub = options.token?.onCancellationRequested(requestCancel); + child.stdout.on("data", (b: Buffer) => { stdoutChunks.push(b); if (options.onStdout) { @@ -270,34 +304,20 @@ export function run( // A stream-level error (e.g. EPIPE) would otherwise be an unhandled // "error" event, which Node throws as an uncaught exception in the - // extension host. Route it through finish() like any other failure. - child.stdout.on("error", (err: Error) => finish(() => reject(err))); - child.stderr.on("error", (err: Error) => finish(() => reject(err))); + // extension host. The child is live here, and POSIX children are + // detached, so terminate the whole tree before rejecting — otherwise it + // keeps running (and mutating state) after the caller sees a failure. + const rejectFromStreamError = (err: Error) => + finish(() => { + safeTerminate(); + reject(err); + }); + child.stdout.on("error", rejectFromStreamError); + child.stderr.on("error", rejectFromStreamError); + // A spawn-level "error" (e.g. ENOENT) means no process is running, so + // there is nothing to terminate. child.on("error", (err: Error) => finish(() => reject(err))); - child.on("close", (code) => - finish(() => { - // Flush any partial trailing byte each streaming decoder held - // back, so the callbacks don't miss the final fragment. - if (options.onStdout) { - const tail = stdoutStreamDecoder.end(); - if (tail) { - options.onStdout(tail); - } - } - if (options.onStderr) { - const tail = stderrStreamDecoder.end(); - if (tail) { - options.onStderr(tail); - } - } - resolve({ - stdout: Buffer.concat(stdoutChunks).toString("utf8"), - stderr: Buffer.concat(stderrChunks).toString("utf8"), - exitCode: code, - cancelled, - }); - }) - ); + child.on("close", (code) => settleResult(code)); }); } From a1f629ac1380a79f01e6ae0d99c4ccf4a3391d8d Mon Sep 17 00:00:00 2001 From: "@rugpanov" Date: Mon, 24 Aug 2026 14:50:35 +0300 Subject: [PATCH 3/5] fix(cli): escalate cancel to SIGKILL and fix sync-token disposal *Why* Second review pass flagged two issues in the cancellation path: - settling on cancel without confirming teardown could leave a detached CLI alive (and its capture listeners buffering) after the caller moved on, while waiting only for "close" could hang if the child ignored SIGTERM. - a token that fires its callback synchronously on subscription settled the run before the subscription handle was assigned, so it was never disposed (a listener leak); the synchronous terminate could also close the child before the "close" listener was even attached, dropping the event. *What* - Cancel now terminates gracefully (SIGTERM tree), waits for "close", and after a bounded grace escalates to SIGKILL (which the process cannot ignore). The run always settles, and only after a confirmed teardown, so no orphaned tree. Grace period and both terminators are injectable seams. - Subscribe to the cancellation token only after the child listeners (especially "close") are attached, and dispose the subscription explicitly when a synchronous token already settled the run. *Verification* - New cliProcess.test.ts cases: escalates to force kill when the graceful terminate is ignored (then settles on close); does not escalate when the graceful terminate closes the process; disposes the subscription when the token fires synchronously; terminateProcessTree honours a SIGKILL signal. - yarn test:unit -> 936 passing, 0 failing. yarn test:lint -> clean. Co-authored-by: Isaac --- .../src/cli/cliProcess.test.ts | 89 +++++++++++++++---- .../databricks-vscode/src/cli/cliProcess.ts | 88 +++++++++++++----- 2 files changed, 140 insertions(+), 37 deletions(-) diff --git a/packages/databricks-vscode/src/cli/cliProcess.test.ts b/packages/databricks-vscode/src/cli/cliProcess.test.ts index 15184bc27..f2003a8ef 100644 --- a/packages/databricks-vscode/src/cli/cliProcess.test.ts +++ b/packages/databricks-vscode/src/cli/cliProcess.test.ts @@ -265,27 +265,78 @@ describe("cliProcess.run", () => { expect(result.cancelled).to.equal(true); }); - it("settles promptly on cancel even if the process never closes", async () => { - // A child that ignores termination and never emits "close". The run must - // still settle (as cancelled) rather than hang forever waiting on close. - const neverClosingSpawn: SpawnFn = (() => { - const child: any = new EventEmitter(); - child.stdout = new EventEmitter(); - child.stderr = new EventEmitter(); - child.stdin = {end: () => {}}; - child.pid = 4242; - child.kill = () => {}; // ignores the signal, never closes - return child; - }) as unknown as SpawnFn; + // A child that ignores the graceful SIGTERM (its `kill` does nothing) but + // exposes a `forceClose` we invoke to simulate SIGKILL taking effect. + function ignoresSigtermSpawn(): {spawn: SpawnFn; forceClose: () => void} { + const child: any = new EventEmitter(); + child.stdout = new EventEmitter(); + child.stderr = new EventEmitter(); + child.stdin = {end: () => {}}; + child.pid = 4242; + child.kill = () => {}; // ignores the graceful signal, never closes + return { + spawn: (() => child) as unknown as SpawnFn, + forceClose: () => child.emit("close", null), + }; + } + + it("escalates to a force kill when the graceful terminate is ignored, then settles on close", async () => { + const {spawn, forceClose} = ignoresSigtermSpawn(); const {token} = nextTickToken(); - let terminated = false; + let softTerminated = false; + let forceTerminated = false; const result = await run("/fake/cli", [], { token, - spawnFn: neverClosingSpawn, - terminateFn: () => (terminated = true), + spawnFn: spawn, + killGraceMs: 5, + terminateFn: () => (softTerminated = true), + forceTerminateFn: () => { + forceTerminated = true; + forceClose(); // SIGKILL lands: the process finally closes + }, }); + expect(softTerminated).to.equal(true); + expect(forceTerminated).to.equal(true); expect(result.cancelled).to.equal(true); - expect(terminated).to.equal(true); + }); + + it("does not escalate to a force kill when the graceful terminate closes the process", async () => { + const {token} = nextTickToken(); + let forceTerminated = false; + const result = await run("/fake/cli", [], { + token, + spawnFn: hangingSpawn, // its kill() emits "close" + killGraceMs: 5, + terminateFn: (child) => (child as any).kill(), + forceTerminateFn: () => (forceTerminated = true), + }); + expect(result.cancelled).to.equal(true); + expect(forceTerminated).to.equal(false); + }); + + it("disposes the cancellation subscription even when the token fires synchronously", async () => { + // A token whose callback runs synchronously during subscription: the run + // can settle before the subscription handle is even assigned, so the + // disposal must still happen. + let disposed = false; + const syncToken = { + isCancellationRequested: false, + onCancellationRequested: (cb: () => void) => { + cb(); + return { + dispose() { + disposed = true; + }, + }; + }, + }; + const result = await run("/fake/cli", [], { + token: syncToken, + spawnFn: hangingSpawn, + terminateFn: (child) => (child as any).kill(), + }); + expect(result.cancelled).to.equal(true); + expect(disposed).to.equal(true); }); it("terminates the process tree when a stdout stream error occurs", async () => { @@ -380,6 +431,12 @@ describe("terminateProcessTree", () => { expect(calls.directKill).to.be.empty; }); + it("signals the process group with the given signal (SIGKILL escalation)", () => { + const {calls, prims, child} = harness("linux"); + terminateProcessTree(child, prims, "SIGKILL"); + expect(calls.groupKill).to.deep.equal([[-4321, "SIGKILL"]]); + }); + it("falls back to a direct child kill when the group is already gone", () => { const {calls, prims, child} = harness("darwin", /*killThrows*/ true); terminateProcessTree(child, prims); diff --git a/packages/databricks-vscode/src/cli/cliProcess.ts b/packages/databricks-vscode/src/cli/cliProcess.ts index 61618e2f3..c845c9dee 100644 --- a/packages/databricks-vscode/src/cli/cliProcess.ts +++ b/packages/databricks-vscode/src/cli/cliProcess.ts @@ -62,15 +62,16 @@ const realTerminatePrimitives: TerminatePrimitives = { /** * Kill the whole process tree, injectable primitives and all. On Windows a * plain `SIGTERM` doesn't reach a CLI spawned via `cmd.exe`, so force-kill the - * tree with `taskkill /T /F`; on POSIX the child is a process-group leader - * (spawned detached), so signalling the negated pid tears down the whole group - * — including any grandchild a direct-child `SIGTERM` would orphan (e.g. - * `terraform`, `uv`) — with a fallback to a direct kill if the group is - * already gone. + * tree with `taskkill /T /F` (always forceful — `signal` is moot there); on + * POSIX the child is a process-group leader (spawned detached), so signalling + * the negated pid tears down the whole group — including any grandchild a + * direct-child signal would orphan (e.g. `terraform`, `uv`) — with a fallback + * to a direct kill if the group is already gone. */ export function terminateProcessTree( child: ChildProcessWithoutNullStreams, - prims: TerminatePrimitives = realTerminatePrimitives + prims: TerminatePrimitives = realTerminatePrimitives, + signal: NodeJS.Signals = "SIGTERM" ): void { const pid = child.pid; if (prims.platform === "win32") { @@ -80,17 +81,29 @@ export function terminateProcessTree( } } else if (pid) { try { - prims.kill(-pid, "SIGTERM"); + prims.kill(-pid, signal); return; } catch { // Group already gone / not a leader — fall through to a direct kill. } } - child.kill("SIGTERM"); + child.kill(signal); } +/** Graceful terminate (SIGTERM) — the first attempt on cancellation. */ const defaultTerminate: TerminateFn = (child) => terminateProcessTree(child); +/** Forceful terminate (SIGKILL) — the escalation when the graceful one is ignored. */ +const defaultForceTerminate: TerminateFn = (child) => + terminateProcessTree(child, realTerminatePrimitives, "SIGKILL"); + +/** + * How long to wait after the graceful `SIGTERM` before escalating to `SIGKILL` + * on cancellation. Bounds how long a cancel can take when a child ignores the + * first signal, so the run always settles. + */ +const DEFAULT_KILL_GRACE_MS = 5000; + /** * Escape a command for a shell we hand it to. On Windows we route through * `cmd.exe` (`/d` disables AutoRun, `/c` runs then exits) with the whole @@ -159,10 +172,17 @@ export interface CliRunOptions { onStdout?: (chunk: string) => void; /** Receives decoded stderr chunks as they arrive (e.g. the "Show Logs" channel). */ onStderr?: (chunk: string) => void; + /** + * Grace period (ms) between the graceful `SIGTERM` on cancel and the + * `SIGKILL` escalation. Defaults to {@link DEFAULT_KILL_GRACE_MS}. + */ + killGraceMs?: number; /** Injectable spawn seam (defaults to Node's `spawn`). */ spawnFn?: SpawnFn; - /** Injectable process-tree terminator (defaults to killing the whole tree). */ + /** Injectable graceful process-tree terminator (SIGTERM; the first attempt). */ terminateFn?: TerminateFn; + /** Injectable forceful process-tree terminator (SIGKILL; the escalation). */ + forceTerminateFn?: TerminateFn; } /** @@ -184,6 +204,8 @@ export function run( ): Promise { const spawnFn = options.spawnFn ?? (nodeSpawn as unknown as SpawnFn); const terminateFn = options.terminateFn ?? defaultTerminate; + const forceTerminateFn = options.forceTerminateFn ?? defaultForceTerminate; + const killGraceMs = options.killGraceMs ?? DEFAULT_KILL_GRACE_MS; const { cmd, args: spawnArgs, @@ -225,24 +247,28 @@ export function run( const stderrStreamDecoder = new StringDecoder("utf8"); let settled = false; let cancelled = false; - // Declared before `finish` (which disposes it) so a token that fires + // Declared before `finish` (which touches them) so a token that fires // synchronously on subscription can't hit a temporal-dead-zone error. let cancelSub: {dispose(): void} | undefined; + let forceKillTimer: NodeJS.Timeout | undefined; const finish = (fn: () => void) => { if (settled) { return; } settled = true; + if (forceKillTimer) { + clearTimeout(forceKillTimer); + } cancelSub?.dispose(); fn(); }; // Best-effort terminate that can never throw out of an event handler // (e.g. the child is already gone). - const safeTerminate = () => { + const safeTerminate = (fn: TerminateFn) => { try { - terminateFn(child); + fn(child); } catch { // ignore } @@ -274,15 +300,24 @@ export function run( const requestCancel = () => { cancelled = true; - safeTerminate(); - // Settle immediately as cancelled rather than waiting for "close": - // a process that ignores the signal (or a failed taskkill) must not - // leave the operation pending forever. The tree has been signalled; - // any late "close" is ignored once settled. - settleResult(null); + // Graceful first, then escalate: signal the tree with SIGTERM and + // wait for "close" (so the result reflects a confirmed teardown, and + // the capture listeners stop when the process actually exits). If the + // child ignores SIGTERM within the grace window, force-kill the tree + // with SIGKILL — which it cannot ignore — so the run always settles + // and never leaves a detached CLI alive after the caller moves on. + safeTerminate(terminateFn); + if (!settled) { + forceKillTimer = setTimeout(() => { + if (!settled) { + safeTerminate(forceTerminateFn); + } + }, killGraceMs); + // Don't let the grace timer keep the event loop (or the test + // runner) alive on its own. + forceKillTimer.unref?.(); + } }; - cancelSub = options.token?.onCancellationRequested(requestCancel); - child.stdout.on("data", (b: Buffer) => { stdoutChunks.push(b); if (options.onStdout) { @@ -309,7 +344,7 @@ export function run( // keeps running (and mutating state) after the caller sees a failure. const rejectFromStreamError = (err: Error) => finish(() => { - safeTerminate(); + safeTerminate(terminateFn); reject(err); }); child.stdout.on("error", rejectFromStreamError); @@ -319,5 +354,16 @@ export function run( child.on("error", (err: Error) => finish(() => reject(err))); child.on("close", (code) => settleResult(code)); + + // Subscribe only after the child listeners (especially "close") are + // attached: an already-cancelled token can fire `requestCancel` + // synchronously here, and its terminate may make the child close at + // once — which would be missed if "close" weren't wired up yet. + cancelSub = options.token?.onCancellationRequested(requestCancel); + // If the token fired synchronously, the run already settled before + // `cancelSub` was assigned, so `finish` couldn't dispose it — do so now. + if (settled) { + cancelSub?.dispose(); + } }); } From 7db62478d7d5a14e4a87376f811cc57f1a350fca Mon Sep 17 00:00:00 2001 From: "@rugpanov" Date: Mon, 24 Aug 2026 15:13:46 +0300 Subject: [PATCH 4/5] fix(cli): unref the Windows taskkill helper so it can't hold the event loop *Why* The fire-and-forget taskkill spawned to tear down the process tree on Windows was never unreferenced, so it could keep the event loop (and the test runner) alive across repeated cancellations. *What* - `unref()` the spawned taskkill helper in the real terminate primitives; still swallow its async spawn error as before. *Verification* - `yarn test:unit` -> 936 passing, 0 failing. `yarn test:lint` -> clean. (The helper is the real OS primitive; tests drive termination through the injected fake, so there is no unit-level behavior change to assert.) Co-authored-by: Isaac --- packages/databricks-vscode/src/cli/cliProcess.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/databricks-vscode/src/cli/cliProcess.ts b/packages/databricks-vscode/src/cli/cliProcess.ts index c845c9dee..5d8aa926b 100644 --- a/packages/databricks-vscode/src/cli/cliProcess.ts +++ b/packages/databricks-vscode/src/cli/cliProcess.ts @@ -53,9 +53,12 @@ const realTerminatePrimitives: TerminatePrimitives = { platform: process.platform, kill: (pid, signal) => process.kill(pid, signal), spawnHelper: (command, args) => { - // Swallow the async spawn error (e.g. taskkill not on PATH) so it can't - // become an uncaught exception in the extension host during cancel. - nodeSpawn(command, args).on("error", () => {}); + // Fire-and-forget: unref so the helper can't keep the event loop alive, + // and swallow the async spawn error (e.g. taskkill not on PATH) so it + // can't become an uncaught exception in the extension host during cancel. + const helper = nodeSpawn(command, args); + helper.unref(); + helper.on("error", () => {}); }, }; From 197e7345650dc909ebfbeca5cdd6ab156aff608a Mon Sep 17 00:00:00 2001 From: "@rugpanov" Date: Tue, 25 Aug 2026 15:55:32 +0300 Subject: [PATCH 5/5] refactor(cli): drop unnecessary casts (review nits) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit *Why* Review nits from @rclarey: two TypeScript casts are redundant now that `SpawnFn` has an explicit signature and catch bindings are `unknown`. *What* - `run()`: `options.spawnFn ?? nodeSpawn` — drop the `as unknown as SpawnFn`; Node's `spawn` assigns to `SpawnFn` directly. - spawn-failure catch: `reject(e)` — drop the `as Error` (reject takes `any`). *Verification* - `yarn test:unit` -> 936 passing, 0 failing (build type-checks without the casts). `yarn test:lint` -> clean. Co-authored-by: Isaac --- packages/databricks-vscode/src/cli/cliProcess.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/databricks-vscode/src/cli/cliProcess.ts b/packages/databricks-vscode/src/cli/cliProcess.ts index 5d8aa926b..d87fddc50 100644 --- a/packages/databricks-vscode/src/cli/cliProcess.ts +++ b/packages/databricks-vscode/src/cli/cliProcess.ts @@ -205,7 +205,7 @@ export function run( args: string[], options: CliRunOptions = {} ): Promise { - const spawnFn = options.spawnFn ?? (nodeSpawn as unknown as SpawnFn); + const spawnFn = options.spawnFn ?? nodeSpawn; const terminateFn = options.terminateFn ?? defaultTerminate; const forceTerminateFn = options.forceTerminateFn ?? defaultForceTerminate; const killGraceMs = options.killGraceMs ?? DEFAULT_KILL_GRACE_MS; @@ -231,7 +231,7 @@ export function run( windowsVerbatimArguments, }); } catch (e) { - reject(e as Error); + reject(e); return; }