diff --git a/packages/databricks-vscode/src/test/e2e/run_dbconnect.ucws.e2e.ts b/packages/databricks-vscode/src/test/e2e/run_dbconnect.ucws.e2e.ts index ce3057cc9..a4eeac71e 100644 --- a/packages/databricks-vscode/src/test/e2e/run_dbconnect.ucws.e2e.ts +++ b/packages/databricks-vscode/src/test/e2e/run_dbconnect.ucws.e2e.ts @@ -16,6 +16,7 @@ import { getBasicBundleConfig, writeRootBundleConfig, } from "./utils/dabsFixtures.ts"; +import {isTransientFileLockError, retryOnTransientError} from "../retry.ts"; const execFile = promisify(execFileCb); @@ -51,7 +52,8 @@ const DBCONNECT_VERSION_SPEC = "17.3.*"; // tests"), so the notebook cell never runs and no output file is written. We // install the deps directly to remove that dependency on the flaky UI step; the // pip call is idempotent, so on shards that already have them it is a fast -// no-op. +// no-op. The install is retried on transient Windows file locks (see +// ensureVenvHasDbConnect for why). async function ensureVenvHasKernelDeps(projectDir: string) { const python = venvPython(projectDir); try { @@ -63,13 +65,27 @@ async function ensureVenvHasKernelDeps(projectDir: string) { return; } try { - const {stdout, stderr} = await execFile(python, [ - "-m", - "pip", - "install", - ...KERNEL_DEPS, - "--disable-pip-version-check", - ]); + const {stdout, stderr} = await retryOnTransientError( + () => + execFile(python, [ + "-m", + "pip", + "install", + ...KERNEL_DEPS, + "--disable-pip-version-check", + ]), + { + attempts: 3, + delayMs: 5000, + isTransient: isTransientFileLockError, + onRetry: (attempt, e) => + console.log( + "kernel-deps install hit a transient Windows file " + + `lock (attempt ${attempt}); retrying.`, + e + ), + } + ); console.log("ensureVenvHasKernelDeps stdout:", stdout); if (stderr) { console.log("ensureVenvHasKernelDeps stderr:", stderr); @@ -89,7 +105,11 @@ async function ensureVenvHasKernelDeps(projectDir: string) { // installs it directly. The `console.warn` is deliberate: it keeps a real // Windows UX regression discoverable in nightly logs even though the direct // install turns the test green. The pip call is skipped when the package is -// already present, so healthy shards pay no reinstall cost. +// already present, so healthy shards pay no reinstall cost. The install itself +// is retried on the transient Windows file lock (WinError 32 "used by another +// process"): pip has to overwrite the venv's existing numpy, and if a scanner +// or a still-running install holds one of its files open the first attempt dies +// mid-write, but a moment later succeeds. async function ensureVenvHasDbConnect(projectDir: string) { const python = venvPython(projectDir); try { @@ -117,13 +137,27 @@ async function ensureVenvHasDbConnect(projectDir: string) { } try { - const {stdout, stderr} = await execFile(python, [ - "-m", - "pip", - "install", - `databricks-connect==${DBCONNECT_VERSION_SPEC}`, - "--disable-pip-version-check", - ]); + const {stdout, stderr} = await retryOnTransientError( + () => + execFile(python, [ + "-m", + "pip", + "install", + `databricks-connect==${DBCONNECT_VERSION_SPEC}`, + "--disable-pip-version-check", + ]), + { + attempts: 3, + delayMs: 5000, + isTransient: isTransientFileLockError, + onRetry: (attempt, e) => + console.log( + "databricks-connect install hit a transient Windows " + + `file lock (attempt ${attempt}); retrying.`, + e + ), + } + ); console.log("ensureVenvHasDbConnect stdout:", stdout); if (stderr) { console.log("ensureVenvHasDbConnect stderr:", stderr); diff --git a/packages/databricks-vscode/src/test/retry.test.ts b/packages/databricks-vscode/src/test/retry.test.ts new file mode 100644 index 000000000..790707965 --- /dev/null +++ b/packages/databricks-vscode/src/test/retry.test.ts @@ -0,0 +1,204 @@ +import {expect} from "chai"; +import {isTransientFileLockError, retryOnTransientError} from "./retry"; + +// A pip failure Node's execFile surfaces: `.message` is only "Command failed: +// ", while the diagnostic (the WinError line) lands in `.stderr`. The +// predicate has to look past `.message` or it will miss every real lock. +function pipError(stderr: string): Error { + return Object.assign(new Error("Command failed: python -m pip install"), { + stderr, + }); +} + +describe("retry", () => { + describe("isTransientFileLockError", () => { + it("matches the WinError 32 'used by another process' lock", () => { + const error = pipError( + "ERROR: Could not install packages due to an OSError: " + + "[WinError 32] The process cannot access the file because " + + "it is being used by another process: " + + "'...\\.venv\\Lib\\site-packages\\numpy\\_core\\_add_newdocs_scalars.py'" + ); + expect(isTransientFileLockError(error)).to.equal(true); + }); + + it("does not match a bare WinError 5 'Access is denied' (can be a permanent permission error)", () => { + const error = pipError( + "ERROR: Could not install packages due to an OSError: " + + "[WinError 5] Access is denied: '...\\numpy\\core.pyd'" + ); + expect(isTransientFileLockError(error)).to.equal(false); + }); + + it("does not match a longer error code that merely starts with 32", () => { + const error = pipError( + "ERROR: Could not install packages due to an OSError: " + + "[WinError 320] something unrelated" + ); + expect(isTransientFileLockError(error)).to.equal(false); + }); + + it("does not match a non-lock failure (version not found)", () => { + const error = pipError( + "ERROR: Could not find a version that satisfies the " + + "requirement databricks-connect==17.3.*" + ); + expect(isTransientFileLockError(error)).to.equal(false); + }); + }); + + describe("retryOnTransientError", () => { + const alwaysTransient = () => true; + const neverTransient = () => false; + const noopSleep = async () => {}; + + it("returns the result without sleeping when the operation succeeds", async () => { + let calls = 0; + let slept = 0; + const result = await retryOnTransientError( + async () => { + calls++; + return "ok"; + }, + { + attempts: 3, + delayMs: 5, + isTransient: alwaysTransient, + sleep: async () => { + slept++; + }, + } + ); + + expect(result).to.equal("ok"); + expect(calls).to.equal(1); + expect(slept).to.equal(0); + }); + + it("retries a transient failure until the operation succeeds", async () => { + let calls = 0; + const result = await retryOnTransientError( + async () => { + calls++; + if (calls < 3) { + throw new Error("transient"); + } + return "ok"; + }, + { + attempts: 3, + delayMs: 5, + isTransient: alwaysTransient, + sleep: noopSleep, + } + ); + + expect(result).to.equal("ok"); + expect(calls).to.equal(3); + }); + + it("waits with linear backoff between attempts", async () => { + const delays: number[] = []; + let calls = 0; + await retryOnTransientError( + async () => { + calls++; + if (calls < 3) { + throw new Error("transient"); + } + return "ok"; + }, + { + attempts: 3, + delayMs: 5, + isTransient: alwaysTransient, + sleep: async (ms) => { + delays.push(ms); + }, + } + ); + + // delayMs * attempt-number: 5 before the 2nd try, 10 before the 3rd. + expect(delays).to.deep.equal([5, 10]); + }); + + it("gives up and rethrows after exhausting attempts on a persistent transient error", async () => { + let calls = 0; + let error: unknown; + try { + await retryOnTransientError( + async () => { + calls++; + throw new Error("still locked"); + }, + { + attempts: 3, + delayMs: 5, + isTransient: alwaysTransient, + sleep: noopSleep, + } + ); + } catch (e) { + error = e; + } + + expect(calls).to.equal(3); + expect((error as Error)?.message).to.equal("still locked"); + }); + + it("rethrows a non-transient error immediately without retrying", async () => { + let calls = 0; + let error: unknown; + try { + await retryOnTransientError( + async () => { + calls++; + throw new Error("fatal"); + }, + { + attempts: 3, + delayMs: 5, + isTransient: neverTransient, + sleep: noopSleep, + } + ); + } catch (e) { + error = e; + } + + expect(calls).to.equal(1); + expect((error as Error)?.message).to.equal("fatal"); + }); + + it("reports each retry through the onRetry callback", async () => { + const seen: Array<{attempt: number; message: string}> = []; + let calls = 0; + await retryOnTransientError( + async () => { + calls++; + if (calls < 3) { + throw new Error(`fail ${calls}`); + } + return "ok"; + }, + { + attempts: 3, + delayMs: 5, + isTransient: alwaysTransient, + sleep: noopSleep, + onRetry: (attempt, e) => { + seen.push({ + attempt, + message: (e as Error).message, + }); + }, + } + ); + + expect(seen).to.deep.equal([ + {attempt: 1, message: "fail 1"}, + {attempt: 2, message: "fail 2"}, + ]); + }); + }); +}); diff --git a/packages/databricks-vscode/src/test/retry.ts b/packages/databricks-vscode/src/test/retry.ts new file mode 100644 index 000000000..bdba85943 --- /dev/null +++ b/packages/databricks-vscode/src/test/retry.ts @@ -0,0 +1,70 @@ +// Retry helper for the e2e test harness. Kept here (under `src/test/`, not +// `src/test/e2e/`) because `tsconfig.json` excludes the e2e folder from the +// unit build, so a colocated unit test only runs when the module lives outside +// it; the e2e specs import it via an explicit `.ts` extension. + +export interface RetryOptions { + // Total number of tries, including the first (so `attempts: 3` == 1 try + 2 + // retries). + attempts: number; + // Base delay; the wait before retry N is `delayMs * N` (linear backoff). + delayMs: number; + // Only failures this returns `true` for are retried; everything else + // rethrows immediately so genuine breakage still surfaces fast. + isTransient: (error: unknown) => boolean; + // Called before each retry (not on the final give-up), for logging. + onRetry?: (attempt: number, error: unknown) => void; + // Injectable so tests run without real timers. + sleep?: (ms: number) => Promise; +} + +const defaultSleep = (ms: number): Promise => + new Promise((resolve) => setTimeout(resolve, ms)); + +// Recognises the transient Windows file lock that makes a pip install fail even +// though a moment later it would succeed: another process (an AV scanner, an +// indexer, a still-running install) holding a file open, reported as WinError +// 32 "the process cannot access the file because it is being used by another +// process". We deliberately do NOT match WinError 5 "Access is denied", which +// is just as often a permanent permission failure — retrying that only delays +// the real error. Node's `execFile` surfaces only "Command failed: " in +// `.message` and puts the real OSError in `.stderr`, so we search every stream +// the failure might carry its text in. `\b` keeps "WinError 32" from matching +// unrelated codes like "WinError 320". +export function isTransientFileLockError(error: unknown): boolean { + const parts: string[] = []; + if (error instanceof Error) { + parts.push(error.message); + } + const streams = error as {stderr?: unknown; stdout?: unknown}; + if (typeof streams?.stderr === "string") { + parts.push(streams.stderr); + } + if (typeof streams?.stdout === "string") { + parts.push(streams.stdout); + } + return /being used by another process|WinError 32\b/i.test( + parts.join("\n") + ); +} + +export async function retryOnTransientError( + operation: () => Promise, + options: RetryOptions +): Promise { + const sleep = options.sleep ?? defaultSleep; + for (let attempt = 1; attempt < options.attempts; attempt++) { + try { + return await operation(); + } catch (error) { + if (!options.isTransient(error)) { + throw error; + } + options.onRetry?.(attempt, error); + await sleep(options.delayMs * attempt); + } + } + // Final attempt: nothing left to retry, so let success return or the error + // propagate directly. + return await operation(); +}