From 68a6a75749ba700f9688f9821352b0e78438576f Mon Sep 17 00:00:00 2001 From: "@rugpanov" Date: Fri, 14 Aug 2026 15:49:43 +0200 Subject: [PATCH] test(e2e): retry the dbconnect pip fallback through transient Windows file locks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit *Why* The `run_dbconnect` e2e test's `should setup virtual environment` step flakes on the Windows shard. When the extension's install path leaves databricks-connect missing, the test's own `ensureVenvHasDbConnect` fallback runs `pip install databricks-connect`. While pip overwrites the venv's existing numpy it can hit `[WinError 32] The process cannot access the file because it is being used by another process` on a numpy file — a transient lock (a scanner or a still-running install holding it open). The fallback swallowed that error and moved on, so databricks-connect stayed missing and the downstream "Databricks Connect:" tree-item check timed out with "Setup confirmation failed". `ensureVenvHasKernelDeps` had the same single-shot-then-swallow shape. *What* - Add `src/test/retry.ts` — a pure `retryOnTransientError` helper plus an `isTransientFileLockError` predicate (matches WinError 32 "used by another process" and WinError 5 "Access is denied", searching `.message`/`.stderr`/ `.stdout` since Node's execFile hides the OSError in stderr). Lives under `src/test/` (not `src/test/e2e/`, which tsconfig excludes from the unit build) so it can carry a colocated unit test; e2e imports it via `.ts`. - Add `src/test/retry.test.ts` — unit coverage for success, retry-then-succeed, linear backoff, give-up-after-N, non-transient-rethrow, and the predicate. - Wrap both fallback pip installs in `run_dbconnect.ucws.e2e.ts` with the helper (3 attempts, 5s linear backoff), logging each retry so a real Windows UX regression stays visible in nightly logs. The final swallow+log and the ground-truth tree-item assertion are unchanged. Test-only: no production code, settings, persisted state, telemetry, or when-clause flags — zero user-facing impact. *Verification* - `retry.test.ts`: 9 passing (ts-mocha, written red-first then green). - e2e file type-checks under `src/test/e2e/tsconfig.json` (the sole error is a pre-existing `assert {type: "json"}` in `wdio.conf.ts`, untouched here). - `yarn build` compiles both new files into `out/` so the unit-suite glob picks up the test; eslint + `prettier -c` clean on all touched files. Co-authored-by: Isaac --- .../src/test/e2e/run_dbconnect.ucws.e2e.ts | 66 ++++-- .../databricks-vscode/src/test/retry.test.ts | 204 ++++++++++++++++++ packages/databricks-vscode/src/test/retry.ts | 70 ++++++ 3 files changed, 324 insertions(+), 16 deletions(-) create mode 100644 packages/databricks-vscode/src/test/retry.test.ts create mode 100644 packages/databricks-vscode/src/test/retry.ts 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(); +}