From bdc68a4cf1205beed07469e6fd3664c802ff75a3 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Sat, 29 Aug 2026 22:52:52 -0700 Subject: [PATCH 1/9] fix(drivers): load a driver from the location the failing runtime named MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A globally installed CLI (`npm install -g`, package tree under `/usr/lib/node_modules/altimate-code`) run from an unrelated working directory could not load any warehouse driver — 8 of 8 trials on a cold VM: DuckDB driver found at duckdb but failed to load: ENOENT: no such file or directory, open '/usr/lib/node_modules/altimate-code/node_modules/duckdb/package.json' The file exists at that path without the `` prefix. Ambient resolution concatenated the working directory onto an already-absolute path, which is a runtime behaviour we cannot change — but it names the correct location in the error, and that is better evidence than anything we can infer. `searchRootsFromError` now mines the paths an ambient failure quotes, repairs a concatenated working directory, and searches the enclosing `node_modules` first. `repairCwdPrefixedPath` is deliberately conservative: it fires only when the named path is absent, is genuinely prefixed by the working directory, and the de-prefixed remainder exists on disk, so a legitimately nested `/node_modules/…` is left alone. The message was the second half of the bug. `found at duckdb` named the bare specifier as though it were a location, so a package that had never been located anywhere read as a load failure at a known path — which is why this was diagnosed as a load bug rather than a search-coverage one. Both sites that produced it now say what actually happened: the default module resolution failed, here is where we looked, and here is the on-disk copy we also tried. Verified in the shape it occurs in, not in unit tests alone: a probe compiled with the production `Bun.build` options (`external`, `autoloadPackageJson`), laid out as a real `npm install -g` tree with a real `npm install duckdb`, run from an unrelated cwd with no `NODE_PATH` and no `ALTIMATE_BIN_DIR`. before: 8/8 fail, reproducing the reported message verbatim after: 8/8 load the driver Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VqnuBDGkh1ZT65Ti7e6DHZ --- packages/drivers/src/resolve.ts | 108 ++++++++++++++- .../drivers/test/resolve-cwd-prefix.test.ts | 127 ++++++++++++++++++ 2 files changed, 230 insertions(+), 5 deletions(-) create mode 100644 packages/drivers/test/resolve-cwd-prefix.test.ts diff --git a/packages/drivers/src/resolve.ts b/packages/drivers/src/resolve.ts index 1d061ca7e..0d4d75774 100644 --- a/packages/drivers/src/resolve.ts +++ b/packages/drivers/src/resolve.ts @@ -156,6 +156,60 @@ function nodeModulesUpward(start: string): string[] { return found } +/** + * Repair a path that had the working directory concatenated onto an + * already-absolute path, e.g. `/usr/lib/node_modules/…` for a package + * that really lives at `/usr/lib/node_modules/…`. + * + * Observed from a globally installed CLI (`npm install -g`, package tree under + * `/usr/lib/node_modules/altimate-code`) run from an unrelated directory: the + * runtime's own resolution reported + * `ENOENT … open '/usr/lib/node_modules/altimate-code/node_modules/duckdb/package.json'` + * while the file existed at that path without the `` prefix. + * + * Deliberately conservative — it fires only when the named path is absent, is + * genuinely prefixed by the working directory, and the de-prefixed remainder + * exists. That last check is what keeps a legitimately nested + * `/node_modules/…` from being mangled. + */ +export function repairCwdPrefixedPath(candidate: string, cwd = process.cwd()): string | undefined { + if (!candidate || fs.existsSync(candidate)) return undefined + if (!candidate.startsWith(cwd + path.sep)) return undefined + const remainder = candidate.slice(cwd.length) + if (!path.isAbsolute(remainder)) return undefined + return fs.existsSync(remainder) ? remainder : undefined +} + +/** + * `node_modules` directories named by an error the runtime raised while trying + * to resolve a package itself. + * + * When ambient resolution fails it often names the exact absolute location it + * was reaching for. That location is better evidence than anything we can + * infer, so it is worth searching — after repairing a concatenated working + * directory, which is the failure this exists for. Returns roots only when they + * exist on disk, so a nonsense path contributes nothing. + */ +export function searchRootsFromError(error: unknown): string[] { + const message = error instanceof Error ? error.message : String(error) + const roots: string[] = [] + // Absolute paths the runtime quoted, POSIX or Windows. + for (const match of message.matchAll(/['"`]((?:\/|[A-Za-z]:[\\/])[^'"`\n]+)['"`]/g)) { + const named = match[1] + if (!named) continue + for (const candidate of [named, repairCwdPrefixedPath(named)]) { + if (!candidate) continue + // Walk back to the enclosing node_modules directory. + const marker = `${path.sep}node_modules${path.sep}` + const at = candidate.lastIndexOf(marker) + if (at === -1) continue + const root = candidate.slice(0, at + marker.length - 1) + if (isDirectory(root) && !roots.includes(root)) roots.push(root) + } + } + return roots +} + /** * Directories to search for an optional SDK, most specific first. * @@ -317,12 +371,19 @@ export async function loadOptionalDriver( return await importer(specifier) } catch (ambientError) { const ambientBroken = !isModuleNotFound(ambientError, specifier) - const roots = driverSearchRoots() + // Search the location the runtime itself named first. When ambient + // resolution fails it frequently quotes the absolute path it was reaching + // for, and that beats anything we can infer — including the case where it + // concatenated the working directory onto an already-absolute path. + const roots = [...searchRootsFromError(ambientError), ...driverSearchRoots()] const resolved = resolveOptionalPackage(specifier, roots) if (!resolved) { - // A broken ambient copy is a load failure, not an absence. - if (ambientBroken) throw loadFailure(driver, specifier, ambientError) + // Nothing was found anywhere, so there is no location to name. Saying + // "found at " here would report the bare specifier as though + // it were a place on disk, which reads as a load failure at a known path + // and sends the reader looking for a file that was never located. + if (ambientBroken) throw ambientLoadFailure(driver, ambientError, describeSearched(roots)) throw new DriverNotInstalledError(driver, DRIVER_PACKAGES[driver], roots) } @@ -332,7 +393,18 @@ export async function loadOptionalDriver( // On disk but will not load — a half-installed copy, or a native addon // built for another platform. When an ambient copy was also broken, // report that one: it is the copy the runtime would normally pick. - throw loadFailure(driver, ambientBroken ? specifier : resolved, ambientBroken ? ambientError : loadError) + if (ambientBroken) { + // Both copies are unusable. Lead with the ambient one — it is the copy + // the runtime would normally pick — but name the on-disk path we also + // tried, rather than passing the specifier off as a location. + throw ambientLoadFailure( + driver, + ambientError, + `A copy at ${resolved} was also tried and failed to load: ` + + (loadError instanceof Error ? loadError.message : String(loadError)), + ) + } + throw loadFailure(driver, resolved, loadError) } } } @@ -368,6 +440,29 @@ function loadFailure(driver: DriverName, where: string, error: unknown): Error { ) } +/** + * The ambient import failed for a reason other than "not installed". + * + * We do not know where that copy lives — the runtime resolved it, not us — so + * the specifier must not be reported as though it were a location on disk. The + * previous wording, `found at duckdb but failed to load`, read as a load + * failure at a known path for a package that had in fact never been located, + * and sent readers looking for a file that was not there. + */ +function ambientLoadFailure(driver: DriverName, error: unknown, detail: string): Error { + return new Error( + `${DRIVER_LABELS[driver]} driver failed to load from the default module resolution: ` + + `${error instanceof Error ? error.message : String(error)}\n${detail}`, + ) +} + +function describeSearched(searched: readonly string[]): string { + return searched.length + ? `It was not found in any searchable location. Searched ${searched.length} ` + + `location${searched.length === 1 ? "" : "s"}: ${searched.join(", ")}` + : "It was not found in any searchable location, and no driver directory exists yet." +} + /** * Import an optional package that is not a warehouse driver, returning * undefined when it is unavailable. @@ -381,7 +476,10 @@ export async function loadOptionalPackage(specifier: string): Promise/usr/lib/node_modules/altimate-code/node_modules/duckdb/package.json' +// +// Two defects in one line. The runtime concatenated the working directory onto +// an already-absolute path, and our message then named the bare specifier as +// though it were a location on disk — so it read as "found it, could not load +// it" when in fact nothing had been found at all. +// +// Real directories on disk here, because the whole mechanism is path existence. + +let root = "" +let pkgRoot = "" +let nodeModules = "" + +/** Build a minimal but real installed package tree. */ +function writePackage(dir: string, name: string, main: string, body: string) { + const pkgDir = path.join(dir, name) + fs.mkdirSync(pkgDir, { recursive: true }) + fs.writeFileSync(path.join(pkgDir, "package.json"), JSON.stringify({ name, version: "1.0.0", main })) + fs.writeFileSync(path.join(pkgDir, main), body) + return pkgDir +} + +describe("cwd concatenated onto an absolute path", () => { + beforeAll(() => { + root = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "resolve-cwd-"))) + pkgRoot = path.join(root, "lib", "node_modules", "altimate-code") + nodeModules = path.join(pkgRoot, "node_modules") + fs.mkdirSync(nodeModules, { recursive: true }) + writePackage(nodeModules, "duckdb", "index.js", "module.exports = { Database: function () {} }\n") + }) + + afterAll(() => { + if (root) fs.rmSync(root, { recursive: true, force: true }) + }) + + test("repairs a path whose absolute form exists", () => { + const real = path.join(nodeModules, "duckdb", "package.json") + const mangled = process.cwd() + real + expect(repairCwdPrefixedPath(mangled)).toBe(real) + }) + + test("leaves a path that exists alone", () => { + const real = path.join(nodeModules, "duckdb", "package.json") + expect(repairCwdPrefixedPath(real)).toBeUndefined() + }) + + test("leaves a legitimately nested path under cwd alone", () => { + // `/node_modules/x` de-prefixes to `/node_modules/x`, which does not + // exist — so the repair must decline rather than invent a root. + const nested = path.join(process.cwd(), "node_modules", "definitely-not-here", "package.json") + expect(repairCwdPrefixedPath(nested)).toBeUndefined() + }) + + test("declines when the de-prefixed path does not exist either", () => { + const mangled = process.cwd() + path.join(root, "nope", "package.json") + expect(repairCwdPrefixedPath(mangled)).toBeUndefined() + }) + + test("harvests the node_modules root the runtime named", () => { + const mangled = process.cwd() + path.join(nodeModules, "duckdb", "package.json") + const error = Object.assign(new Error(`ENOENT: no such file or directory, open '${mangled}'`), { + code: "ENOENT", + }) + expect(searchRootsFromError(error)).toContain(nodeModules) + }) + + test("harvests nothing from an error naming no usable path", () => { + expect(searchRootsFromError(new Error("something went wrong"))).toEqual([]) + expect(searchRootsFromError(new Error("open '/no/such/place/pkg/package.json'"))).toEqual([]) + }) + + test("loads the driver from the location the failing runtime named", async () => { + // Reproduces the reported failure exactly: ambient resolution throws ENOENT + // naming the correct absolute path with cwd concatenated on, and nothing + // else on this machine can see that tree. + const mangled = process.cwd() + path.join(nodeModules, "duckdb", "package.json") + const ambient = Object.assign(new Error(`ENOENT: no such file or directory, open '${mangled}'`), { + code: "ENOENT", + }) + + let call = 0 + const importer = async (spec: string) => { + call++ + if (call === 1) throw ambient + return await import(/* @vite-ignore */ spec) + } + + const mod: any = await loadOptionalDriver("duckdb", "duckdb", importer) + const duckdb = mod.default ?? mod + expect(typeof duckdb.Database).toBe("function") + }) + + test("does not claim a location when nothing was found", async () => { + const ambient = Object.assign(new Error("ENOENT: no such file or directory, open '/nowhere/pkg.json'"), { + code: "ENOENT", + }) + const importer = async () => { + throw ambient + } + + let message = "" + try { + await loadOptionalDriver("duckdb", "duckdb", importer) + } catch (e) { + message = e instanceof Error ? e.message : String(e) + } + // The old text was `found at duckdb but failed to load: …`, naming the bare + // specifier as a place on disk. + expect(message).not.toContain("found at duckdb") + expect(message).toContain("failed to load from the default module resolution") + }) +}) From 16ac5e5e035350056fec5bffa42443b7e132f482 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Sat, 29 Aug 2026 23:42:37 -0700 Subject: [PATCH 2/9] fix(drivers): make on-demand driver installs safe across processes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The managed driver directory is shared by every CLI process on the machine, and `installsInFlight` is an in-process Map — it cannot see other processes. Eight CLIs starting together each ran `npm install` over the same tree: npm install failed (exit 217) … ENOTEMPTY … rmdir /root/.local/share/altimate-code/drivers/node_modules/duckdb/… That is not benchmark-specific. Any concurrent use of the CLI hits it, and the result is a driver that appears broken on a machine where nothing is wrong. `withInstallLock` takes a cross-process lock before mutating the directory. `mkdir` is atomic and fails EEXIST when the directory exists on both POSIX and Windows, which makes a lock directory the portable primitive; it lives beside the install directory so npm never sees it as stray package content. Stale locks are broken two ways, because neither alone is sufficient: a dead owner on this host, and age, which is the only signal available for a lock left by another host sharing a home directory. After acquiring, readiness is re-checked. The peer that held the lock has usually just installed the very thing we queued for, so most contenders return "already present" rather than running a second npm over the same tree. On timeout the install proceeds unlocked rather than failing: a racing install is recoverable and the readiness check afterwards is authoritative, whereas refusing to install because a peer is slow turns contention into a hard error. Also adds cwd/execPath and a cwd-prefix note to driver load failures. Two separate investigations have now diagnosed a load failure from the error text alone and got it wrong, because the text named a path without saying what the process's own view of the filesystem was. The exclusion claim is about separate processes, so the test spawns separate processes — an in-process test cannot establish it. The control confirms the test bites: the same four processes without the lock interleave completely (four enters before any exit). 10/10 repeat runs green. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VqnuBDGkh1ZT65Ti7e6DHZ --- packages/drivers/src/resolve.ts | 172 ++++++++++++++++++++- packages/drivers/test/install-lock.test.ts | 159 +++++++++++++++++++ 2 files changed, 329 insertions(+), 2 deletions(-) create mode 100644 packages/drivers/test/install-lock.test.ts diff --git a/packages/drivers/src/resolve.ts b/packages/drivers/src/resolve.ts index 0d4d75774..730081933 100644 --- a/packages/drivers/src/resolve.ts +++ b/packages/drivers/src/resolve.ts @@ -452,10 +452,37 @@ function loadFailure(driver: DriverName, where: string, error: unknown): Error { function ambientLoadFailure(driver: DriverName, error: unknown, detail: string): Error { return new Error( `${DRIVER_LABELS[driver]} driver failed to load from the default module resolution: ` + - `${error instanceof Error ? error.message : String(error)}\n${detail}`, + `${error instanceof Error ? error.message : String(error)}\n${detail}${loadDiagnostics(error)}`, ) } +/** + * Context a reader needs to tell a resolution fault apart from a broken + * package, appended to load failures. + * + * Driver-load failures have twice been diagnosed from the error text alone and + * twice been diagnosed wrong, because the text named a path without saying what + * the process's own view of the filesystem was. The expensive question each + * time was "is this absolute path being re-anchored to the working directory?" + * — which is answerable on the spot, and only from inside the failing process. + */ +function loadDiagnostics(error: unknown): string { + const lines = [`cwd=${process.cwd()}`, `execPath=${process.execPath}`] + const message = error instanceof Error ? error.message : String(error) + for (const match of message.matchAll(/['"`]((?:\/|[A-Za-z]:[\\/])[^'"`\n]+)['"`]/g)) { + const named = match[1] + if (!named) continue + const repaired = repairCwdPrefixedPath(named) + if (repaired) { + lines.push( + `NOTE: "${named}" does not exist, but "${repaired}" does — the working ` + + `directory appears to have been concatenated onto an absolute path.`, + ) + } + } + return `\n(${lines.join("; ")})` +} + function describeSearched(searched: readonly string[]): string { return searched.length ? `It was not found in any searchable location. Searched ${searched.length} ` + @@ -810,7 +837,16 @@ async function installOptionalDriverInternal( if (!options.force && installed(driver)) { return { driver, packages, dir, installed: true, alreadyPresent: true } } - return performInstall(driver, packages, dir, options) + // Take the cross-process lock, then check readiness again. The peer that + // held it has usually just installed the very thing we queued for, so + // most contenders return "already present" instead of running a second + // npm over the same tree — which is what produced the ENOTEMPTY races. + return withInstallLock(dir, async (acquired) => { + if (acquired && !options.force && installed(driver)) { + return { driver, packages, dir, installed: true, alreadyPresent: true } + } + return performInstall(driver, packages, dir, options) + }) }) installsInFlight.set(dir, run) try { @@ -823,6 +859,138 @@ async function installOptionalDriverInternal( /** In-flight installs keyed by target directory (see the note above). */ const installsInFlight = new Map>() +// --------------------------------------------------------------------------- +// Cross-process install lock +// --------------------------------------------------------------------------- + +/** + * `installsInFlight` serialises installs inside one process. It cannot see + * other processes, and the managed driver directory is shared by all of them, + * so N CLIs starting together each run `npm install` over the same tree: + * + * npm install failed (exit 217) … ENOTEMPTY … + * rmdir /root/.local/share/altimate-code/drivers/node_modules/duckdb/… + * + * That is not benchmark-specific — any concurrent use of the CLI hits it. + * + * `mkdir` is atomic and fails with EEXIST when the directory exists, on both + * POSIX and Windows, which makes a lock directory the portable primitive here. + * The lock lives beside the install directory rather than inside it so npm + * never sees it as stray package content. + */ +function installLockPath(dir: string): string { + return `${dir.replace(/[\\/]+$/, "")}.lock` +} + +interface LockHolder { + pid: number + hostname: string + /** Absent when the owner file is malformed; age then falls back to mtime. */ + startedAt?: number +} + +function readLockHolder(lockDir: string): LockHolder | undefined { + try { + const raw = fs.readFileSync(path.join(lockDir, "owner.json"), "utf8") + const parsed: unknown = JSON.parse(raw) + if (!parsed || typeof parsed !== "object") return undefined + const holder = parsed as Partial + if (typeof holder.pid !== "number") return undefined + return { + pid: holder.pid, + hostname: typeof holder.hostname === "string" ? holder.hostname : "", + startedAt: typeof holder.startedAt === "number" ? holder.startedAt : undefined, + } + } catch { + return undefined + } +} + +/** + * True when a lock cannot belong to a live install any more: its owner is gone, + * or it has outlived any plausible npm run. Both checks are needed — a killed + * process leaves no signal beyond its absence, and a lock from another host + * (shared home directory) can only be judged by age. + */ +function isStaleLock(lockDir: string, holder: LockHolder | undefined, maxAgeMs: number): boolean { + if (holder && holder.hostname === os.hostname() && !processExists(holder.pid)) return true + const startedAt = holder?.startedAt + if (typeof startedAt === "number" && Date.now() - startedAt > maxAgeMs) return true + try { + return Date.now() - fs.statSync(lockDir).mtimeMs > maxAgeMs + } catch { + // Vanished between checks — someone else released it, so it is not stale. + return false + } +} + +/** + * Run `fn` while holding an exclusive lock on `dir`, across processes. + * + * On timeout the work runs anyway rather than failing. A driver install that + * races is recoverable — npm is largely idempotent here and the readiness check + * afterwards is authoritative — whereas refusing to install because a lock + * could not be taken turns a slow peer into a hard failure. + */ +export async function withInstallLock( + dir: string, + fn: (acquired: boolean) => Promise, + options: { timeoutMs?: number; staleAfterMs?: number; pollMs?: number } = {}, +): Promise { + const lockDir = installLockPath(dir) + const timeoutMs = options.timeoutMs ?? 240_000 + const staleAfterMs = options.staleAfterMs ?? 300_000 + const pollMs = options.pollMs ?? 100 + const deadline = Date.now() + timeoutMs + let acquired = false + + for (;;) { + try { + fs.mkdirSync(lockDir, { recursive: false }) + acquired = true + break + } catch (e) { + // Anything but "already held" — an unwritable parent, say — means we + // cannot lock at all, so proceed unlocked rather than block forever. + const code = e && typeof e === "object" && "code" in e ? (e as { code?: unknown }).code : undefined + if (code !== "EEXIST") break + if (isStaleLock(lockDir, readLockHolder(lockDir), staleAfterMs)) { + try { + fs.rmSync(lockDir, { recursive: true, force: true }) + } catch { + // Another process won the cleanup; fall through and retry. + } + continue + } + if (Date.now() >= deadline) break + await sleep(pollMs) + } + } + + if (acquired) { + try { + fs.writeFileSync( + path.join(lockDir, "owner.json"), + JSON.stringify({ pid: process.pid, hostname: os.hostname(), startedAt: Date.now() } satisfies LockHolder), + ) + } catch { + // Diagnostics only — the lock is the directory, not the file in it. + } + } + + try { + return await fn(acquired) + } finally { + if (acquired) { + try { + fs.rmSync(lockDir, { recursive: true, force: true }) + } catch { + // Leaving it behind is safe: the next contender ages it out as stale. + } + } + } +} + async function performInstall( driver: DriverName, packages: readonly string[], diff --git a/packages/drivers/test/install-lock.test.ts b/packages/drivers/test/install-lock.test.ts new file mode 100644 index 000000000..482062564 --- /dev/null +++ b/packages/drivers/test/install-lock.test.ts @@ -0,0 +1,159 @@ +import { afterEach, beforeEach, describe, expect, test } from "bun:test" +import fs from "node:fs" +import os from "node:os" +import path from "node:path" +import { fileURLToPath } from "node:url" + +import { withInstallLock } from "../src/resolve" + +// The managed driver directory is shared by every CLI process on the machine, +// and `installsInFlight` only serialises within one process. Eight CLIs +// starting together each ran `npm install` over the same tree: +// +// npm install failed (exit 217) … ENOTEMPTY … +// rmdir /root/.local/share/altimate-code/drivers/node_modules/duckdb/… +// +// The exclusion claim is about separate processes, so the central test spawns +// separate processes. An in-process test cannot establish it. + +const resolveModule = fileURLToPath(new URL("../src/resolve.ts", import.meta.url)) + +let dir = "" + +beforeEach(() => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), "install-lock-")) +}) + +afterEach(() => { + if (dir) fs.rmSync(dir, { recursive: true, force: true }) + fs.rmSync(`${dir}.lock`, { recursive: true, force: true }) +}) + +describe("cross-process install lock", () => { + test("excludes concurrent processes from the critical section", async () => { + const target = path.join(dir, "drivers") + fs.mkdirSync(target, { recursive: true }) + const log = path.join(dir, "log.txt") + + // Each child holds the lock briefly and brackets its critical section. + // Overlapping brackets in the log mean the lock did not hold. + const child = path.join(dir, "child.ts") + fs.writeFileSync( + child, + `import fs from "node:fs" +import { withInstallLock } from ${JSON.stringify(resolveModule)} +const [target, log, id] = process.argv.slice(2) +await withInstallLock(target, async (acquired) => { + if (!acquired) { fs.appendFileSync(log, \`timeout \${id}\\n\`); return } + fs.appendFileSync(log, \`enter \${id}\\n\`) + await new Promise((r) => setTimeout(r, 120)) + fs.appendFileSync(log, \`exit \${id}\\n\`) +}, { timeoutMs: 30000 }) +process.exit(0) +`, + ) + + const kids = Array.from({ length: 4 }, (_, i) => + Bun.spawn(["bun", child, target, log, String(i)], { stdout: "ignore", stderr: "ignore" }), + ) + const codes = await Promise.all(kids.map((k) => k.exited)) + expect(codes).toEqual([0, 0, 0, 0]) + + const events = fs.readFileSync(log, "utf8").trim().split("\n").filter(Boolean) + expect(events).not.toContain("timeout 0") + // Every enter must be followed by its own exit before the next enter. + let inside: string | undefined + for (const line of events) { + const [kind, id] = line.split(" ") + if (kind === "enter") { + expect(inside).toBeUndefined() + inside = id + } else if (kind === "exit") { + expect(inside).toBe(id) + inside = undefined + } + } + expect(events.filter((e) => e.startsWith("enter")).length).toBe(4) + }, 60_000) + + test("reports the section ran unlocked when the lock cannot be taken in time", async () => { + const target = path.join(dir, "drivers") + fs.mkdirSync(target, { recursive: true }) + // Hold the lock with a live owner so it cannot be judged stale. + fs.mkdirSync(`${target}.lock`) + fs.writeFileSync( + path.join(`${target}.lock`, "owner.json"), + JSON.stringify({ pid: process.pid, hostname: os.hostname(), startedAt: Date.now() }), + ) + + let sawAcquired: boolean | undefined + await withInstallLock( + target, + async (acquired) => { + sawAcquired = acquired + }, + { timeoutMs: 200, pollMs: 20 }, + ) + // The work still runs — refusing to install because a peer is slow would + // turn contention into a hard failure — but it knows it was unlocked. + expect(sawAcquired).toBe(false) + // A lock we did not take must not be deleted on the way out. + expect(fs.existsSync(`${target}.lock`)).toBe(true) + }) + + test("breaks a lock whose owner is gone", async () => { + const target = path.join(dir, "drivers") + fs.mkdirSync(target, { recursive: true }) + fs.mkdirSync(`${target}.lock`) + fs.writeFileSync( + path.join(`${target}.lock`, "owner.json"), + // PID 0x7FFFFFFF is not a live process on any platform we run on. + JSON.stringify({ pid: 0x7fffffff, hostname: os.hostname(), startedAt: Date.now() }), + ) + + let sawAcquired: boolean | undefined + await withInstallLock( + target, + async (acquired) => { + sawAcquired = acquired + }, + { timeoutMs: 5000, pollMs: 20 }, + ) + expect(sawAcquired).toBe(true) + }) + + test("breaks a lock that has outlived any plausible install", async () => { + const target = path.join(dir, "drivers") + fs.mkdirSync(target, { recursive: true }) + fs.mkdirSync(`${target}.lock`) + fs.writeFileSync( + path.join(`${target}.lock`, "owner.json"), + JSON.stringify({ pid: process.pid, hostname: os.hostname(), startedAt: Date.now() - 10 * 60_000 }), + ) + + let sawAcquired: boolean | undefined + await withInstallLock( + target, + async (acquired) => { + sawAcquired = acquired + }, + { timeoutMs: 5000, staleAfterMs: 60_000, pollMs: 20 }, + ) + expect(sawAcquired).toBe(true) + }) + + test("releases the lock when the critical section throws", async () => { + const target = path.join(dir, "drivers") + fs.mkdirSync(target, { recursive: true }) + let thrown = "" + try { + await withInstallLock(target, async () => { + throw new Error("boom") + }) + } catch (e) { + thrown = e instanceof Error ? e.message : String(e) + } + expect(thrown).toBe("boom") + expect(fs.existsSync(`${target}.lock`)).toBe(false) + }) +}) From d13f070dbfc67e4c81187ba95bb4bc9b4fa755d8 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Sun, 30 Aug 2026 01:05:54 -0700 Subject: [PATCH 3/9] fix(drivers): keep harvested roots inside the trust boundary, and make the install lock hold MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings on #1201, grouped by what they actually break. **Harvested roots crossed a deliberate security boundary.** `driverSearchRoots()` refuses project and ancestor `node_modules` because importing a workspace- controlled SDK during a warehouse read/test bypasses the permission boundary and can expose resolved credentials. `searchRootsFromError()` mined any absolute path an error quoted and prepended it, so an ambient failure naming a project-local `node_modules` routed straight around that invariant. Harvested roots are now filtered through the same exclusion. **They also preempted the managed installation.** Prepending meant a stale or broken copy the runtime happened to name won over the driver we installed, which inverts the documented "managed install dir comes first" ordering. Trusted roots now come first and harvested roots are appended, which still recovers the original failure: a harvested root is reached whenever the roots ahead of it resolve nothing. **Windows never harvested anything.** The extraction regex accepts `C:/…` and `C:\…`, but the `node_modules` marker is built from `path.sep`, so a forward-slash path could never match a backslash marker. `enclosingNodeModulesRoot` normalises separators, and takes `sep` as a parameter so the Windows behaviour is tested from a POSIX host rather than asserted. `repairCwdPrefixedPath` now also handles the two Windows concatenation shapes, which have no separator to carry. **The lock silently degraded to unlocked in exactly the cold-start case it exists for.** `.lock` sits beside the managed directory, and on a fresh machine nothing has created the XDG data directory yet — `performInstall` is the first thing that does, and it runs after the lock attempt. The non-recursive `mkdir` failed ENOENT, took the "cannot lock" branch, and dropped every concurrent CLI into an unlocked install. The parent is now created first. **Stale-lock recovery could admit two installers.** Two processes could both judge a lock stale, and deleting by pathname let the loser delete the winner's fresh lock. Claiming is now a `rename`, which exactly one process can win. **A lock could be released out from under its successor.** An owner whose lock was broken as stale would, on the way out, delete the lock a peer had since taken. Release now only removes a lock still carrying its own token. **Age aged out live installs.** `isStaleLock` applied the age check unconditionally, so a live same-host owner running a slow native build past `staleAfterMs` had its lock broken and a peer entered — reintroducing the very ENOTEMPTY race. Where liveness is decidable (owner on this host) it is now the only signal; age applies only where it cannot be (no readable owner, or another host sharing a home directory). **The lock path was outside the approved permission pattern.** The tool asks for `external_directory` on `/*`, but the lock is the sibling `.lock` — so creating, writing and removing it mutated an external path the user never approved. It is now included in the request. **Two unrelated cwd faults on the failure path.** `process.cwd()` throws once the working directory is removed, and both `loadDiagnostics` and — pre-existing — `resolveOptionalPackage`'s `createRequire` anchor called it unguarded while a driver failure was being formatted, replacing the real diagnosis with an unrelated `uv_cwd` ENOENT. Both are guarded; the anchor never needed cwd, since resolution is driven by the explicit `paths`. Tests. The two end-to-end resolution tests used the specifier `duckdb`, which the repository's own `packages/drivers/node_modules` can satisfy no matter what the harvesting code does — they passed while proving nothing, and would have kept passing after the reordering above. They now use specifiers that exist nowhere but the tree the test builds, and assert on an export marker, so they establish which root actually satisfied the load. The cross-process exclusion test gained a start barrier: without one, a scheduler that ran the four children serially satisfied the no-overlap assertion even with a completely broken lock. The control confirms the barriered test still bites — four unlocked children interleave completely (enter 1, enter 2, enter 3, enter 0) and the assertion rejects on the second enter. 9 repeat runs, 72/72 green. Not verified: Windows on hardware. The separator handling is now unit-tested through the `sep` parameter, but nothing here ran on Windows. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VqnuBDGkh1ZT65Ti7e6DHZ --- packages/drivers/src/resolve.ts | 250 +++++++++++++++--- packages/drivers/test/install-lock.test.ts | 103 +++++++- .../drivers/test/resolve-cwd-prefix.test.ts | 228 ++++++++++++++-- .../tools/warehouse-install-driver.ts | 15 +- ...arehouse-install-driver-permission.test.ts | 12 +- 5 files changed, 535 insertions(+), 73 deletions(-) diff --git a/packages/drivers/src/resolve.ts b/packages/drivers/src/resolve.ts index 730081933..fa77994ab 100644 --- a/packages/drivers/src/resolve.ts +++ b/packages/drivers/src/resolve.ts @@ -172,12 +172,83 @@ function nodeModulesUpward(start: string): string[] { * exists. That last check is what keeps a legitimately nested * `/node_modules/…` from being mangled. */ -export function repairCwdPrefixedPath(candidate: string, cwd = process.cwd()): string | undefined { - if (!candidate || fs.existsSync(candidate)) return undefined - if (!candidate.startsWith(cwd + path.sep)) return undefined - const remainder = candidate.slice(cwd.length) - if (!path.isAbsolute(remainder)) return undefined - return fs.existsSync(remainder) ? remainder : undefined +export function repairCwdPrefixedPath(candidate: string, cwd = safeCwd()): string | undefined { + if (!candidate || !cwd || fs.existsSync(candidate)) return undefined + if (!candidate.startsWith(cwd)) return undefined + // POSIX concatenation yields `/usr/…`, whose remainder carries the + // separator. Windows has no separator to carry: `C:\work` + `C:\global\…` + // concatenates to `C:\workC:\global\…`, and a join-shaped `C:\work\C:\global\…` + // leaves a stray leading separator on the remainder. Try each shape and + // accept only a remainder that is absolute and exists, so a near-miss such + // as cwd `/work` against `/workspace/…` contributes nothing. + const rest = candidate.slice(cwd.length) + for (const remainder of rest.startsWith(path.sep) ? [rest, rest.slice(1)] : [rest]) { + if (!remainder || !path.isAbsolute(remainder)) continue + if (fs.existsSync(remainder)) return remainder + } + return undefined +} + +/** + * `process.cwd()` throws ENOENT when the working directory has been removed out + * from under the process. Every caller here is formatting an error or repairing + * a path, where throwing would replace the driver failure the caller is trying + * to report with an unrelated ENOENT — losing the actual diagnosis. + */ +function safeCwd(): string | undefined { + try { + return process.cwd() + } catch { + return undefined + } +} + +/** + * The `node_modules` content `driverSearchRoots()` deliberately refuses to + * search: the working directory tree, and the project and ancestor + * `node_modules` above it. + */ +function workspaceScope(): { cwd: string | undefined; ancestors: string[] } { + const cwd = safeCwd() + if (!cwd) return { cwd: undefined, ancestors: [] } + const resolved = path.resolve(cwd) + return { cwd: resolved, ancestors: nodeModulesUpward(resolved).map((dir) => path.resolve(dir)) } +} + +/** + * Preserve order, drop repeats. A harvested root often names a directory the + * inferred roots already cover, and listing it twice makes the searched-location + * count in the failure message overstate where we actually looked. + */ +function dedupeRoots(roots: readonly string[]): string[] { + return [...new Set(roots)] +} + +/** + * The `node_modules` directory enclosing `candidate`, or undefined when there + * is none. + * + * `sep` is a parameter so the Windows behaviour is testable from a POSIX host. + * It matters because Windows quotes both `C:\…` and `C:/…` in errors, while the + * marker is built from the platform separator — a forward-slash path would + * never match a backslash marker, and the root would silently not be harvested. + * Rewriting separators is length-preserving, so the slice offsets still hold. + */ +export function enclosingNodeModulesRoot(candidate: string, sep: string = path.sep): string | undefined { + const normalized = sep === "\\" ? candidate.replace(/\//g, "\\") : candidate + const marker = `${sep}node_modules${sep}` + const at = normalized.lastIndexOf(marker) + if (at === -1) return undefined + return normalized.slice(0, at + marker.length - 1) +} + +/** True when `root` is workspace-controlled and must not be imported from. */ +function isWorkspaceRoot(root: string, scope: { cwd: string | undefined; ancestors: string[] }): boolean { + const resolved = path.resolve(root) + if (scope.ancestors.includes(resolved)) return true + if (!scope.cwd) return false + const rel = path.relative(scope.cwd, resolved) + return rel === "" || (!rel.startsWith("..") && !path.isAbsolute(rel)) } /** @@ -189,9 +260,16 @@ export function repairCwdPrefixedPath(candidate: string, cwd = process.cwd()): s * infer, so it is worth searching — after repairing a concatenated working * directory, which is the failure this exists for. Returns roots only when they * exist on disk, so a nonsense path contributes nothing. + * + * Workspace-controlled roots are never returned. `driverSearchRoots()` refuses + * project and ancestor `node_modules` because importing a matching SDK during a + * warehouse read/test would bypass the permission boundary and can expose + * resolved credentials; mining a path out of an error message must not become a + * way around that invariant. */ export function searchRootsFromError(error: unknown): string[] { const message = error instanceof Error ? error.message : String(error) + const scope = workspaceScope() const roots: string[] = [] // Absolute paths the runtime quoted, POSIX or Windows. for (const match of message.matchAll(/['"`]((?:\/|[A-Za-z]:[\\/])[^'"`\n]+)['"`]/g)) { @@ -200,11 +278,10 @@ export function searchRootsFromError(error: unknown): string[] { for (const candidate of [named, repairCwdPrefixedPath(named)]) { if (!candidate) continue // Walk back to the enclosing node_modules directory. - const marker = `${path.sep}node_modules${path.sep}` - const at = candidate.lastIndexOf(marker) - if (at === -1) continue - const root = candidate.slice(0, at + marker.length - 1) - if (isDirectory(root) && !roots.includes(root)) roots.push(root) + const root = enclosingNodeModulesRoot(candidate) + if (!root || !isDirectory(root)) continue + if (isWorkspaceRoot(root, scope)) continue + if (!roots.includes(root)) roots.push(root) } } return roots @@ -278,7 +355,13 @@ export function packageNameOf(specifier: string): string { */ export function resolveOptionalPackage(specifier: string, roots = driverSearchRoots()): string | undefined { const pkg = packageNameOf(specifier) - const require = createRequire(pathToFileURL(path.join(process.cwd(), "noop.js")).href) + // The anchor only has to be some absolute file URL — resolution is driven by + // the explicit `paths` below, not by this base. So it must not be the one + // thing that can throw: process.cwd() raises ENOENT when the working + // directory has been removed, and letting that escape here replaces every + // driver diagnosis with an unrelated uv_cwd error. + const anchor = safeCwd() ?? os.tmpdir() + const require = createRequire(pathToFileURL(path.join(anchor, "noop.js")).href) for (const root of roots) { const pkgDir = path.join(root, pkg) @@ -371,11 +454,18 @@ export async function loadOptionalDriver( return await importer(specifier) } catch (ambientError) { const ambientBroken = !isModuleNotFound(ambientError, specifier) - // Search the location the runtime itself named first. When ambient - // resolution fails it frequently quotes the absolute path it was reaching - // for, and that beats anything we can infer — including the case where it - // concatenated the working directory onto an already-absolute path. - const roots = [...searchRootsFromError(ambientError), ...driverSearchRoots()] + // Trusted roots first, then the location the runtime itself named. When + // ambient resolution fails it frequently quotes the absolute path it was + // reaching for — including the case where it concatenated the working + // directory onto an already-absolute path — and that is the only evidence + // available when nothing else resolves. But it is evidence about wherever + // the runtime happened to point, which may be a stale or broken copy, so it + // must not preempt the managed installation: `driverSearchRoots()` puts the + // driver we installed first precisely so it wins over a stale copy + // elsewhere. Appending keeps that order and still recovers the failure this + // exists for, because a harvested root is reached whenever the roots ahead + // of it resolve nothing. + const roots = dedupeRoots([...driverSearchRoots(), ...searchRootsFromError(ambientError)]) const resolved = resolveOptionalPackage(specifier, roots) if (!resolved) { @@ -467,7 +557,11 @@ function ambientLoadFailure(driver: DriverName, error: unknown, detail: string): * — which is answerable on the spot, and only from inside the failing process. */ function loadDiagnostics(error: unknown): string { - const lines = [`cwd=${process.cwd()}`, `execPath=${process.execPath}`] + // A deleted working directory makes process.cwd() throw. This runs while a + // driver failure is being formatted, so letting that escape would replace the + // fault the reader needs with an unrelated ENOENT from the reporting path. + const cwd = safeCwd() + const lines = [`cwd=${cwd ?? ""}`, `execPath=${process.execPath}`] const message = error instanceof Error ? error.message : String(error) for (const match of message.matchAll(/['"`]((?:\/|[A-Za-z]:[\\/])[^'"`\n]+)['"`]/g)) { const named = match[1] @@ -503,10 +597,10 @@ export async function loadOptionalPackage(specifier: string): Promise>() * The lock lives beside the install directory rather than inside it so npm * never sees it as stray package content. */ -function installLockPath(dir: string): string { +export function installLockPath(dir: string): string { return `${dir.replace(/[\\/]+$/, "")}.lock` } @@ -887,6 +981,8 @@ interface LockHolder { hostname: string /** Absent when the owner file is malformed; age then falls back to mtime. */ startedAt?: number + /** Identifies one acquisition, so a holder only ever releases its own lock. */ + token?: string } function readLockHolder(lockDir: string): LockHolder | undefined { @@ -900,6 +996,7 @@ function readLockHolder(lockDir: string): LockHolder | undefined { pid: holder.pid, hostname: typeof holder.hostname === "string" ? holder.hostname : "", startedAt: typeof holder.startedAt === "number" ? holder.startedAt : undefined, + token: typeof holder.token === "string" ? holder.token : undefined, } } catch { return undefined @@ -907,15 +1004,25 @@ function readLockHolder(lockDir: string): LockHolder | undefined { } /** - * True when a lock cannot belong to a live install any more: its owner is gone, - * or it has outlived any plausible npm run. Both checks are needed — a killed - * process leaves no signal beyond its absence, and a lock from another host - * (shared home directory) can only be judged by age. + * True when a lock cannot belong to a live install any more. + * + * The two signals are not interchangeable, and which one applies depends on + * whether liveness is decidable at all: + * + * - **Owner on this host.** Liveness is decidable, so it is the only thing that + * counts. Age must *not* also apply here: npm can legitimately run longer + * than any duration we pick — a native build such as `oracledb` or `duckdb`, + * or a caller that raised its own install timeout — and breaking a live + * owner's lock puts two `npm install` runs over the same tree, which is the + * exact corruption this lock exists to prevent. + * - **No readable owner, or an owner on another host** (a shared home + * directory). Liveness cannot be established, so age is the only signal + * available and the lock ages out. */ function isStaleLock(lockDir: string, holder: LockHolder | undefined, maxAgeMs: number): boolean { - if (holder && holder.hostname === os.hostname() && !processExists(holder.pid)) return true + if (holder && holder.hostname === os.hostname()) return !processExists(holder.pid) const startedAt = holder?.startedAt - if (typeof startedAt === "number" && Date.now() - startedAt > maxAgeMs) return true + if (typeof startedAt === "number") return Date.now() - startedAt > maxAgeMs try { return Date.now() - fs.statSync(lockDir).mtimeMs > maxAgeMs } catch { @@ -924,6 +1031,56 @@ function isStaleLock(lockDir: string, holder: LockHolder | undefined, maxAgeMs: } } +/** + * Take ownership of a lock judged stale, atomically, and remove it. + * + * Two processes can both judge the same lock stale. If each simply deleted the + * pathname, the first would delete the dead lock and acquire a fresh one, and + * the second would then delete *that* live lock and acquire its own — putting + * both inside the critical section, which is the failure the lock exists to + * prevent. `rename` is atomic: exactly one process can move a given directory, + * and only that process goes on to delete it. The loser's rename fails and it + * simply retries against whatever state now exists. + */ +function claimStaleLock(lockDir: string): void { + const claimed = `${lockDir}.stale-${process.pid}-${Date.now()}` + try { + fs.renameSync(lockDir, claimed) + } catch { + // Another process claimed it first, or the owner released it. Either way + // there is nothing of ours to clean up; retry the acquire. + return + } + try { + fs.rmSync(claimed, { recursive: true, force: true }) + } catch { + // The rename already made the lock unreachable, so a leftover directory + // beside it costs nothing but disk. + } +} + +/** + * Release a lock this process acquired, but only while it is still ours. + * + * A lock we hold can be broken as stale and re-taken by a peer while `fn` is + * still running — an install that outlives `staleAfterMs` on a machine whose + * owner record is unreadable, say. Removing it by pathname would then delete + * the successor's live lock and admit a third process. The token is written + * when the lock is taken, so a mismatch means the directory is somebody else's. + */ +function releaseInstallLock(lockDir: string, token: string): void { + const holder = readLockHolder(lockDir) + // An unreadable owner file leaves nothing to compare against; the lock is + // most likely still ours (we wrote it) and leaking it would wedge every later + // install behind a lock nobody releases, so remove it. + if (holder?.token !== undefined && holder.token !== token) return + try { + fs.rmSync(lockDir, { recursive: true, force: true }) + } catch { + // Leaving it behind is safe: the next contender ages it out as stale. + } +} + /** * Run `fn` while holding an exclusive lock on `dir`, across processes. * @@ -942,8 +1099,22 @@ export async function withInstallLock( const staleAfterMs = options.staleAfterMs ?? 300_000 const pollMs = options.pollMs ?? 100 const deadline = Date.now() + timeoutMs + const token = `${process.pid}-${Date.now().toString(36)}-${Math.random().toString(36).slice(2, 10)}` let acquired = false + // The lock's parent must exist before the atomic mkdir can land. On a cold + // machine nothing has created the XDG data directory yet — `performInstall` + // is the first thing that does, and it runs *after* this — so a non-recursive + // mkdir would fail ENOENT, take the "cannot lock" branch, and drop every + // caller straight to an unlocked install. That is precisely the cold-start + // stampede this lock exists to prevent, so create the parent first. + try { + fs.mkdirSync(path.dirname(lockDir), { recursive: true }) + } catch { + // Genuinely unwritable. The acquire below then fails too and we proceed + // unlocked, which is the documented degradation. + } + for (;;) { try { fs.mkdirSync(lockDir, { recursive: false }) @@ -955,11 +1126,7 @@ export async function withInstallLock( const code = e && typeof e === "object" && "code" in e ? (e as { code?: unknown }).code : undefined if (code !== "EEXIST") break if (isStaleLock(lockDir, readLockHolder(lockDir), staleAfterMs)) { - try { - fs.rmSync(lockDir, { recursive: true, force: true }) - } catch { - // Another process won the cleanup; fall through and retry. - } + claimStaleLock(lockDir) continue } if (Date.now() >= deadline) break @@ -971,7 +1138,12 @@ export async function withInstallLock( try { fs.writeFileSync( path.join(lockDir, "owner.json"), - JSON.stringify({ pid: process.pid, hostname: os.hostname(), startedAt: Date.now() } satisfies LockHolder), + JSON.stringify({ + pid: process.pid, + hostname: os.hostname(), + startedAt: Date.now(), + token, + } satisfies LockHolder), ) } catch { // Diagnostics only — the lock is the directory, not the file in it. @@ -981,13 +1153,7 @@ export async function withInstallLock( try { return await fn(acquired) } finally { - if (acquired) { - try { - fs.rmSync(lockDir, { recursive: true, force: true }) - } catch { - // Leaving it behind is safe: the next contender ages it out as stale. - } - } + if (acquired) releaseInstallLock(lockDir, token) } } diff --git a/packages/drivers/test/install-lock.test.ts b/packages/drivers/test/install-lock.test.ts index 482062564..b4c9ba31d 100644 --- a/packages/drivers/test/install-lock.test.ts +++ b/packages/drivers/test/install-lock.test.ts @@ -4,7 +4,7 @@ import os from "node:os" import path from "node:path" import { fileURLToPath } from "node:url" -import { withInstallLock } from "../src/resolve" +import { installLockPath, withInstallLock } from "../src/resolve" // The managed driver directory is shared by every CLI process on the machine, // and `installsInFlight` only serialises within one process. Eight CLIs @@ -34,15 +34,25 @@ describe("cross-process install lock", () => { const target = path.join(dir, "drivers") fs.mkdirSync(target, { recursive: true }) const log = path.join(dir, "log.txt") - - // Each child holds the lock briefly and brackets its critical section. - // Overlapping brackets in the log mean the lock did not hold. + const ready = path.join(dir, "ready") + fs.mkdirSync(ready) + + // A start barrier, because without one the test can pass vacuously: if the + // scheduler happens to run the children serially — each acquiring, holding, + // and exiting before the next starts — the "no overlapping bracket" check + // is satisfied even by a completely broken lock. Every child announces + // itself and waits until all four are ready, so they contend for real. const child = path.join(dir, "child.ts") fs.writeFileSync( child, `import fs from "node:fs" import { withInstallLock } from ${JSON.stringify(resolveModule)} -const [target, log, id] = process.argv.slice(2) +const [target, log, ready, id] = process.argv.slice(2) +fs.writeFileSync(\`\${ready}/\${id}\`, "1") +const deadline = Date.now() + 20000 +while (fs.readdirSync(ready).length < 4 && Date.now() < deadline) { + await new Promise((r) => setTimeout(r, 5)) +} await withInstallLock(target, async (acquired) => { if (!acquired) { fs.appendFileSync(log, \`timeout \${id}\\n\`); return } fs.appendFileSync(log, \`enter \${id}\\n\`) @@ -54,13 +64,15 @@ process.exit(0) ) const kids = Array.from({ length: 4 }, (_, i) => - Bun.spawn(["bun", child, target, log, String(i)], { stdout: "ignore", stderr: "ignore" }), + Bun.spawn(["bun", child, target, log, ready, String(i)], { stdout: "ignore", stderr: "ignore" }), ) const codes = await Promise.all(kids.map((k) => k.exited)) expect(codes).toEqual([0, 0, 0, 0]) const events = fs.readFileSync(log, "utf8").trim().split("\n").filter(Boolean) - expect(events).not.toContain("timeout 0") + // All four contended and all four got in; nobody fell through unlocked. + expect(events.filter((e) => e.startsWith("timeout")).length).toBe(0) + expect(events.filter((e) => e.startsWith("enter")).length).toBe(4) // Every enter must be followed by its own exit before the next enter. let inside: string | undefined for (const line of events) { @@ -73,9 +85,29 @@ process.exit(0) inside = undefined } } - expect(events.filter((e) => e.startsWith("enter")).length).toBe(4) }, 60_000) + test("takes the lock when its parent directory does not exist yet", async () => { + // The cold-start shape this change exists for. `.lock` sits beside the + // managed directory, and on a fresh machine nothing has created the XDG data + // directory yet — `performInstall` is the first thing that does, and it runs + // *after* the lock is taken. A non-recursive mkdir would fail ENOENT, take + // the "cannot lock" branch, and drop every concurrent CLI into an unlocked + // install: exactly the stampede the lock is meant to stop. + const target = path.join(dir, "fresh", "xdg", "altimate-code", "drivers") + expect(fs.existsSync(path.dirname(installLockPath(target)))).toBe(false) + + let sawAcquired: boolean | undefined + await withInstallLock( + target, + async (acquired) => { + sawAcquired = acquired + }, + { timeoutMs: 5000, pollMs: 20 }, + ) + expect(sawAcquired).toBe(true) + }) + test("reports the section ran unlocked when the lock cannot be taken in time", async () => { const target = path.join(dir, "drivers") fs.mkdirSync(target, { recursive: true }) @@ -122,13 +154,15 @@ process.exit(0) expect(sawAcquired).toBe(true) }) - test("breaks a lock that has outlived any plausible install", async () => { + test("breaks a lock left by another host once it has outlived any plausible install", async () => { + // Age is the only signal available for a lock written by a different host + // sharing a home directory, because its pid means nothing here. const target = path.join(dir, "drivers") fs.mkdirSync(target, { recursive: true }) fs.mkdirSync(`${target}.lock`) fs.writeFileSync( path.join(`${target}.lock`, "owner.json"), - JSON.stringify({ pid: process.pid, hostname: os.hostname(), startedAt: Date.now() - 10 * 60_000 }), + JSON.stringify({ pid: process.pid, hostname: `${os.hostname()}-other`, startedAt: Date.now() - 10 * 60_000 }), ) let sawAcquired: boolean | undefined @@ -142,6 +176,55 @@ process.exit(0) expect(sawAcquired).toBe(true) }) + test("does not age out a live owner on this host", async () => { + // npm can legitimately run longer than any age we pick — a native build such + // as oracledb, or a caller that raised its own install timeout. Breaking a + // live owner's lock would put two npm runs over the same tree, which is the + // corruption this lock exists to prevent. Where liveness is decidable it is + // the only thing that counts. + const target = path.join(dir, "drivers") + fs.mkdirSync(target, { recursive: true }) + fs.mkdirSync(`${target}.lock`) + fs.writeFileSync( + path.join(`${target}.lock`, "owner.json"), + JSON.stringify({ pid: process.pid, hostname: os.hostname(), startedAt: Date.now() - 60 * 60_000 }), + ) + + let sawAcquired: boolean | undefined + await withInstallLock( + target, + async (acquired) => { + sawAcquired = acquired + }, + { timeoutMs: 200, staleAfterMs: 1_000, pollMs: 20 }, + ) + // Waited, then proceeded unlocked rather than stealing a running install. + expect(sawAcquired).toBe(false) + expect(fs.existsSync(`${target}.lock`)).toBe(true) + }) + + test("does not delete a lock that has been re-taken by a peer", async () => { + // A lock we hold can be broken as stale and re-acquired by someone else + // while our critical section is still running. Releasing by pathname would + // then delete the successor's live lock and admit a third process, so the + // release only removes a lock still carrying our own token. + const target = path.join(dir, "drivers") + fs.mkdirSync(target, { recursive: true }) + const lockDir = installLockPath(target) + + await withInstallLock(target, async (acquired) => { + expect(acquired).toBe(true) + // A peer breaks our lock and takes its own. + fs.writeFileSync( + path.join(lockDir, "owner.json"), + JSON.stringify({ pid: process.pid, hostname: os.hostname(), startedAt: Date.now(), token: "successor" }), + ) + }) + + expect(fs.existsSync(lockDir)).toBe(true) + fs.rmSync(lockDir, { recursive: true, force: true }) + }) + test("releases the lock when the critical section throws", async () => { const target = path.join(dir, "drivers") fs.mkdirSync(target, { recursive: true }) diff --git a/packages/drivers/test/resolve-cwd-prefix.test.ts b/packages/drivers/test/resolve-cwd-prefix.test.ts index 26692e724..7aa82dd9e 100644 --- a/packages/drivers/test/resolve-cwd-prefix.test.ts +++ b/packages/drivers/test/resolve-cwd-prefix.test.ts @@ -1,9 +1,10 @@ -import { afterAll, beforeAll, describe, expect, test } from "bun:test" +import { afterAll, afterEach, beforeAll, describe, expect, test } from "bun:test" import fs from "node:fs" import os from "node:os" import path from "node:path" import { + enclosingNodeModulesRoot, loadOptionalDriver, repairCwdPrefixedPath, searchRootsFromError, @@ -21,11 +22,22 @@ import { // it" when in fact nothing had been found at all. // // Real directories on disk here, because the whole mechanism is path existence. +// +// The end-to-end tests deliberately use specifiers that exist *nowhere* except +// the tree the test builds. `driverSearchRoots()` includes roots derived from +// execPath and this module's own location, which in-tree reach the repository's +// own `packages/drivers/node_modules` — where a real `duckdb` is installed. A +// test asking for `duckdb` would therefore resolve it from the repository no +// matter what the harvesting code did, and pass while proving nothing. A unique +// specifier cannot be satisfied by any root but the harvested one. let root = "" let pkgRoot = "" let nodeModules = "" +const HARVESTED_PKG = "altimate-harvest-probe" +const ABSENT_PKG = "altimate-absent-probe" + /** Build a minimal but real installed package tree. */ function writePackage(dir: string, name: string, main: string, body: string) { const pkgDir = path.join(dir, name) @@ -35,6 +47,13 @@ function writePackage(dir: string, name: string, main: string, body: string) { return pkgDir } +/** An ENOENT of the reported shape, naming `real` with the cwd concatenated on. */ +function cwdPrefixedEnoent(real: string) { + return Object.assign(new Error(`ENOENT: no such file or directory, open '${process.cwd()}${real}'`), { + code: "ENOENT", + }) +} + describe("cwd concatenated onto an absolute path", () => { beforeAll(() => { root = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "resolve-cwd-"))) @@ -42,6 +61,7 @@ describe("cwd concatenated onto an absolute path", () => { nodeModules = path.join(pkgRoot, "node_modules") fs.mkdirSync(nodeModules, { recursive: true }) writePackage(nodeModules, "duckdb", "index.js", "module.exports = { Database: function () {} }\n") + writePackage(nodeModules, HARVESTED_PKG, "index.js", "module.exports = { marker: 'from-harvested-root' }\n") }) afterAll(() => { @@ -71,11 +91,54 @@ describe("cwd concatenated onto an absolute path", () => { expect(repairCwdPrefixedPath(mangled)).toBeUndefined() }) + test("reports the driver failure when the working directory is unavailable", async () => { + // process.cwd() throws ENOENT once the working directory is removed out from + // under the process. Repair and diagnostics both run *while a driver failure + // is being formatted*, so an unavailable cwd must degrade to "no repair" + // rather than replace the fault the reader needs with an unrelated ENOENT + // raised by the reporting path itself. + // + // The condition is forced rather than staged: deleting a real working + // directory does not make process.cwd() throw on macOS, so a test that + // removed a directory would silently assert nothing on this platform. + const realCwd = process.cwd.bind(process) + process.cwd = () => { + throw Object.assign(new Error("ENOENT: no such file or directory, uv_cwd"), { code: "ENOENT" }) + } + try { + const real = path.join(nodeModules, "duckdb", "package.json") + expect(repairCwdPrefixedPath(`/somewhere${real}`)).toBeUndefined() + + const ambient = Object.assign(new Error("ENOENT: no such file or directory, open '/nowhere/pkg.json'"), { + code: "ENOENT", + }) + let message = "" + try { + await loadOptionalDriver("duckdb", ABSENT_PKG, async () => { + throw ambient + }) + } catch (e) { + message = e instanceof Error ? e.message : String(e) + } + // The driver fault survives, and the diagnostics say plainly that the + // process could not see its own working directory. + expect(message).toContain("failed to load from the default module resolution") + expect(message).toContain("cwd=") + expect(message).not.toContain("uv_cwd") + } finally { + process.cwd = realCwd + } + }) + + test("does not mistake a sibling directory sharing a prefix for the cwd", () => { + // cwd `/a/work` against `/a/workspace/…` shares a textual prefix but is a + // different directory; de-prefixing there would invent a nonsense path. + const sibling = `${process.cwd()}space` + expect(repairCwdPrefixedPath(path.join(sibling, "lib", "pkg.json"))).toBeUndefined() + }) + test("harvests the node_modules root the runtime named", () => { - const mangled = process.cwd() + path.join(nodeModules, "duckdb", "package.json") - const error = Object.assign(new Error(`ENOENT: no such file or directory, open '${mangled}'`), { - code: "ENOENT", - }) + const error = cwdPrefixedEnoent(path.join(nodeModules, "duckdb", "package.json")) expect(searchRootsFromError(error)).toContain(nodeModules) }) @@ -84,14 +147,11 @@ describe("cwd concatenated onto an absolute path", () => { expect(searchRootsFromError(new Error("open '/no/such/place/pkg/package.json'"))).toEqual([]) }) - test("loads the driver from the location the failing runtime named", async () => { + test("loads a package from the location the failing runtime named", async () => { // Reproduces the reported failure exactly: ambient resolution throws ENOENT // naming the correct absolute path with cwd concatenated on, and nothing // else on this machine can see that tree. - const mangled = process.cwd() + path.join(nodeModules, "duckdb", "package.json") - const ambient = Object.assign(new Error(`ENOENT: no such file or directory, open '${mangled}'`), { - code: "ENOENT", - }) + const ambient = cwdPrefixedEnoent(path.join(nodeModules, HARVESTED_PKG, "package.json")) let call = 0 const importer = async (spec: string) => { @@ -100,9 +160,9 @@ describe("cwd concatenated onto an absolute path", () => { return await import(/* @vite-ignore */ spec) } - const mod: any = await loadOptionalDriver("duckdb", "duckdb", importer) - const duckdb = mod.default ?? mod - expect(typeof duckdb.Database).toBe("function") + const mod: any = await loadOptionalDriver("duckdb", HARVESTED_PKG, importer) + // Identity, not shape: proves the harvested root is what satisfied the load. + expect((mod.default ?? mod).marker).toBe("from-harvested-root") }) test("does not claim a location when nothing was found", async () => { @@ -115,13 +175,151 @@ describe("cwd concatenated onto an absolute path", () => { let message = "" try { - await loadOptionalDriver("duckdb", "duckdb", importer) + await loadOptionalDriver("duckdb", ABSENT_PKG, importer) } catch (e) { message = e instanceof Error ? e.message : String(e) } // The old text was `found at duckdb but failed to load: …`, naming the bare // specifier as a place on disk. - expect(message).not.toContain("found at duckdb") + expect(message).not.toContain(`found at ${ABSENT_PKG}`) expect(message).toContain("failed to load from the default module resolution") }) }) + +describe("enclosing node_modules root", () => { + test("finds the root in a platform-native path", () => { + expect(enclosingNodeModulesRoot("/a/node_modules/pkg/index.js", "/")).toBe("/a/node_modules") + }) + + test("finds the last root when the path nests several", () => { + expect(enclosingNodeModulesRoot("/a/node_modules/b/node_modules/c/index.js", "/")).toBe( + "/a/node_modules/b/node_modules", + ) + }) + + test("returns nothing when the path names no node_modules", () => { + expect(enclosingNodeModulesRoot("/a/b/index.js", "/")).toBeUndefined() + }) + + test("handles a Windows path quoted with forward slashes", () => { + // Windows runtimes quote both shapes. The marker is built from the platform + // separator, so without normalisation a forward-slash path would never match + // a backslash marker and the root would silently not be harvested. + expect(enclosingNodeModulesRoot("C:/app/node_modules/pkg/index.js", "\\")).toBe("C:\\app\\node_modules") + }) + + test("handles a Windows path quoted with backslashes", () => { + expect(enclosingNodeModulesRoot("C:\\app\\node_modules\\pkg\\index.js", "\\")).toBe("C:\\app\\node_modules") + }) + + test("handles a Windows path with mixed separators", () => { + expect(enclosingNodeModulesRoot("C:\\app/node_modules\\pkg/index.js", "\\")).toBe("C:\\app\\node_modules") + }) +}) + +describe("harvested roots do not preempt the managed installation", () => { + const PRIORITY_PKG = "altimate-priority-probe" + let managedRoot = "" + let strayRoot = "" + let savedDriverDir: string | undefined + + beforeAll(() => { + savedDriverDir = process.env["ALTIMATE_DRIVER_DIR"] + managedRoot = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "resolve-managed-"))) + strayRoot = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "resolve-stray-"))) + writePackage(path.join(managedRoot, "node_modules"), PRIORITY_PKG, "index.js", "module.exports={marker:'managed'}\n") + writePackage(path.join(strayRoot, "node_modules"), PRIORITY_PKG, "index.js", "module.exports={marker:'stray'}\n") + process.env["ALTIMATE_DRIVER_DIR"] = managedRoot + }) + + afterAll(() => { + if (savedDriverDir === undefined) delete process.env["ALTIMATE_DRIVER_DIR"] + else process.env["ALTIMATE_DRIVER_DIR"] = savedDriverDir + for (const dir of [managedRoot, strayRoot]) if (dir) fs.rmSync(dir, { recursive: true, force: true }) + }) + + test("prefers the driver we installed over a copy the error happened to name", async () => { + // A harvested root is evidence about wherever the runtime pointed, which may + // be a stale or broken copy. driverSearchRoots() puts the managed install + // first precisely so a driver we installed wins; harvesting must not undo + // that by jumping the queue. + const ambient = Object.assign( + new Error( + `ENOENT: no such file or directory, open '${path.join(strayRoot, "node_modules", PRIORITY_PKG, "package.json")}'`, + ), + { code: "ENOENT" }, + ) + let call = 0 + const importer = async (spec: string) => { + call++ + if (call === 1) throw ambient + return await import(/* @vite-ignore */ spec) + } + + const mod: any = await loadOptionalDriver("duckdb", PRIORITY_PKG, importer) + expect((mod.default ?? mod).marker).toBe("managed") + }) +}) + +describe("harvested roots respect the workspace boundary", () => { + let workspace = "" + let outside = "" + let outsideModules = "" + const originalCwd = process.cwd() + + beforeAll(() => { + // Its own tree: the first describe's afterAll has already removed that one + // by the time this block runs. + outside = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "resolve-outside-"))) + outsideModules = path.join(outside, "node_modules") + writePackage(outsideModules, "duckdb", "index.js", "module.exports = {}\n") + }) + + afterAll(() => { + if (outside) fs.rmSync(outside, { recursive: true, force: true }) + }) + + afterEach(() => { + process.chdir(originalCwd) + if (workspace) fs.rmSync(workspace, { recursive: true, force: true }) + workspace = "" + }) + + test("refuses a node_modules inside the working directory", () => { + // driverSearchRoots() deliberately never searches project node_modules: + // importing a workspace-controlled SDK during a warehouse read/test would + // bypass the permission boundary and can expose resolved credentials. + // Mining a path out of an error message must not route around that. + workspace = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "resolve-ws-"))) + const projectModules = path.join(workspace, "node_modules") + writePackage(projectModules, "duckdb", "index.js", "module.exports = {}\n") + process.chdir(workspace) + + const error = new Error(`ENOENT: no such file or directory, open '${path.join(projectModules, "duckdb", "package.json")}'`) + expect(searchRootsFromError(error)).not.toContain(projectModules) + expect(searchRootsFromError(error)).toEqual([]) + }) + + test("refuses a node_modules in an ancestor of the working directory", () => { + workspace = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "resolve-ws-"))) + const ancestorModules = path.join(workspace, "node_modules") + writePackage(ancestorModules, "duckdb", "index.js", "module.exports = {}\n") + const nested = path.join(workspace, "packages", "app") + fs.mkdirSync(nested, { recursive: true }) + process.chdir(nested) + + const error = new Error(`ENOENT: no such file or directory, open '${path.join(ancestorModules, "duckdb", "package.json")}'`) + expect(searchRootsFromError(error)).not.toContain(ancestorModules) + }) + + test("still harvests a root outside the workspace", () => { + // The exclusion must not swallow the case the harvesting exists for. + workspace = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "resolve-ws-"))) + process.chdir(workspace) + + const error = new Error( + `ENOENT: no such file or directory, open '${path.join(outsideModules, "duckdb", "package.json")}'`, + ) + expect(searchRootsFromError(error)).toContain(outsideModules) + }) +}) diff --git a/packages/opencode/src/altimate/tools/warehouse-install-driver.ts b/packages/opencode/src/altimate/tools/warehouse-install-driver.ts index 7b52b523b..7a1387129 100644 --- a/packages/opencode/src/altimate/tools/warehouse-install-driver.ts +++ b/packages/opencode/src/altimate/tools/warehouse-install-driver.ts @@ -8,6 +8,7 @@ import { loadOptionalDriver, driverInstallDir, driverLabel, + installLockPath, installOptionalDriver, isDriverInstalled, npmInstallArgs, @@ -85,6 +86,14 @@ export const WarehouseInstallDriverTool = Tool.define("warehouse_install_driver" const packages = DRIVER_PACKAGES[driver].join(" ") const externalPattern = FSUtil.normalizePathPattern(path.join(dir, "*")) + // The cross-process install lock is a sibling of the managed directory, not + // a child of it — it lives outside so npm never treats it as stray package + // content. That puts it outside the pattern above, so it has to be approved + // explicitly: this tool creates, writes and removes that directory, and + // asking for `/*` alone would mutate an external path the user never + // agreed to. + const lockPattern = FSUtil.normalizePathPattern(path.join(installLockPath(dir), "*")) + const externalPatterns = [externalPattern, lockPattern] const installCommand = ["npm", ...npmInstallArgs(DRIVER_PACKAGES[driver])].join(" ") // This tool bypasses the bash and edit tools, so it must broker the same @@ -93,9 +102,9 @@ export const WarehouseInstallDriverTool = Tool.define("warehouse_install_driver" // choose only a driver enum, never shell text or package names. await ctx.ask({ permission: "external_directory", - patterns: [externalPattern], - always: [externalPattern], - metadata: { driver, dir }, + patterns: externalPatterns, + always: externalPatterns, + metadata: { driver, dir, lockDir: installLockPath(dir) }, }) await ctx.ask({ permission: "bash", diff --git a/packages/opencode/test/altimate/warehouse-install-driver-permission.test.ts b/packages/opencode/test/altimate/warehouse-install-driver-permission.test.ts index d4b6bf549..a844ad11a 100644 --- a/packages/opencode/test/altimate/warehouse-install-driver-permission.test.ts +++ b/packages/opencode/test/altimate/warehouse-install-driver-permission.test.ts @@ -51,13 +51,19 @@ describe("warehouse_install_driver permissions", () => { ) const dir = DriverResolve.driverInstallDir() + // The cross-process install lock is a sibling of the managed directory, not + // a child of it, so `/*` does not cover it. The tool creates, writes + // and removes that directory, so it has to be approved explicitly rather + // than mutated as an unapproved external path. + const lockDir = DriverResolve.installLockPath(dir) + expect(lockDir).toBe(`${dir}.lock`) expect(events).toEqual(["ask:external_directory", "ask:bash", "install"]) expect(requests).toEqual([ { permission: "external_directory", - patterns: [path.join(dir, "*")], - always: [path.join(dir, "*")], - metadata: { driver: "postgres", dir }, + patterns: [path.join(dir, "*"), path.join(lockDir, "*")], + always: [path.join(dir, "*"), path.join(lockDir, "*")], + metadata: { driver: "postgres", dir, lockDir }, }, { permission: "bash", From 53a196de0979242a99413b5085d9342c8ec1770a Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Sun, 30 Aug 2026 01:28:53 -0700 Subject: [PATCH 4/9] fix(drivers): bound the stale-lock retry, and harvest every enclosing root MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second review round. One of these is a bug the previous commit introduced. **The stale-claim branch could spin forever.** `claimStaleLock` returned without reporting failure and the loop `continue`d unconditionally, skipping both the deadline check and the sleep. A claim that fails persistently — a lock owned by another user, or a container that permits inspection but not rename — meant `mkdir` EEXIST, judged stale, claim fails, repeat, at full CPU with no timeout. The claim now reports success, and only a successful claim skips the wait; a failed one falls through to the normal deadline-and-sleep path. The regression test hangs rather than fails if that bound is lost again. **Stale recovery could still move a live lock aside.** The rename made two cleaners safe against each other, but not against the stale→fresh transition: a peer could release and re-acquire between the staleness verdict and the rename. The claim now re-reads the owner record before renaming, and — because nothing makes "read the owner" and "rename" one operation — verifies what it actually moved afterwards, restoring it if a peer had re-taken the lock. That narrows the window rather than closing it, which is stated plainly rather than implied. **Liveness-only staleness could wedge the lock permanently.** Making a live same-host owner immune to age fixed the interrupted-install bug but introduced a worse one: `processExists` answers "some process holds this pid", so a crashed owner whose pid is recycled by an unrelated long-lived process would hold the lock forever, with every later install waiting out its timeout and then running unlocked. A live owner is now protected only up to a backstop far beyond any real npm run (1h default), which bounds the wedge without interrupting an install. **Release could delete a successor's lock during a window the token cannot cover.** The directory is created before `owner.json` is written, so a successor that re-took the lock in that window holds a live lock carrying no token, and the token check let it be removed. Release now also compares the lock directory's inode, captured at acquire. **Every path this mechanism writes now sits under one approved prefix.** Stale recovery renamed the lock to a sibling of the container, which the tool's `.lock/*` permission does not cover. The atomic lock moved inside the container as `.lock/held`, and claims rename to `.lock/stale-…`, so both are covered by the pattern already brokered. **The lock wait could be shorter than the install it waits on.** The two timeouts were independent constants: raising the install timeout past the lock wait meant a contender gave up while the holder's npm was still running and then installed unlocked over the same tree. The lock wait is now derived from the install timeout. **Nested dependency paths harvested the wrong root.** A quoted path such as `/opt/node_modules/duckdb/node_modules/node-addon-api/…` yielded only the innermost `node_modules`, which holds the dependency — while the driver being looked for sits in the outer one, so it was never found. All enclosing roots are now harvested, innermost first, each subject to the same workspace exclusion. **The workspace exclusion compared lexical paths while resolution followed symlinks.** A link whose lexical path sits outside the working directory but whose target sits inside it passed the check and would have been imported. Containment now compares real paths, falling back to lexical when the link cannot be followed. Gates: typecheck 13/13 (forced, not cached); marker check ok; lint 5903 warnings and 1 error, byte-identical to `main`'s own baseline (the error is the known pre-existing `consistent-return` in `packages/http-recorder/test/record-replay.test.ts`); `packages/drivers` 255 pass; `test/altimate` 4226 pass. Lock tests 99/99 across 9 repeat runs. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VqnuBDGkh1ZT65Ti7e6DHZ --- packages/drivers/src/resolve.ts | 223 ++++++++++++++---- packages/drivers/test/install-lock.test.ts | 129 +++++++--- .../drivers/test/resolve-cwd-prefix.test.ts | 50 +++- 3 files changed, 306 insertions(+), 96 deletions(-) diff --git a/packages/drivers/src/resolve.ts b/packages/drivers/src/resolve.ts index fa77994ab..b05e63ceb 100644 --- a/packages/drivers/src/resolve.ts +++ b/packages/drivers/src/resolve.ts @@ -211,8 +211,26 @@ function safeCwd(): string | undefined { function workspaceScope(): { cwd: string | undefined; ancestors: string[] } { const cwd = safeCwd() if (!cwd) return { cwd: undefined, ancestors: [] } - const resolved = path.resolve(cwd) - return { cwd: resolved, ancestors: nodeModulesUpward(resolved).map((dir) => path.resolve(dir)) } + const resolved = realPath(cwd) + return { cwd: resolved, ancestors: nodeModulesUpward(resolved).map(realPath) } +} + +/** + * Absolute *real* path, falling back to the lexical one when the link cannot be + * followed. + * + * The containment check below must compare real paths. `isDirectory` follows + * symlinks, so a symlinked `node_modules` whose lexical path sits outside the + * working directory but whose target sits inside it would pass a lexical + * exclusion and be imported — reintroducing the workspace-controlled code the + * exclusion exists to keep out. + */ +function realPath(candidate: string): string { + try { + return fs.realpathSync(path.resolve(candidate)) + } catch { + return path.resolve(candidate) + } } /** @@ -225,26 +243,36 @@ function dedupeRoots(roots: readonly string[]): string[] { } /** - * The `node_modules` directory enclosing `candidate`, or undefined when there - * is none. + * Every `node_modules` directory enclosing `candidate`, innermost first. + * + * All of them, not just the innermost: a quoted path often runs through a + * driver's own dependency — `/opt/node_modules/duckdb/node_modules/node-addon-api/…` + * — where the innermost root holds the dependency and the *outer* one holds the + * driver we are actually looking for. Returning only the innermost left the + * driver unfindable in exactly the nested case. * * `sep` is a parameter so the Windows behaviour is testable from a POSIX host. * It matters because Windows quotes both `C:\…` and `C:/…` in errors, while the * marker is built from the platform separator — a forward-slash path would - * never match a backslash marker, and the root would silently not be harvested. + * never match a backslash marker, and nothing would be harvested at all. * Rewriting separators is length-preserving, so the slice offsets still hold. */ -export function enclosingNodeModulesRoot(candidate: string, sep: string = path.sep): string | undefined { +export function enclosingNodeModulesRoots(candidate: string, sep: string = path.sep): string[] { const normalized = sep === "\\" ? candidate.replace(/\//g, "\\") : candidate const marker = `${sep}node_modules${sep}` - const at = normalized.lastIndexOf(marker) - if (at === -1) return undefined - return normalized.slice(0, at + marker.length - 1) + const roots: string[] = [] + for (let at = normalized.lastIndexOf(marker); at !== -1; at = normalized.lastIndexOf(marker, at - 1)) { + roots.push(normalized.slice(0, at + marker.length - 1)) + if (at === 0) break + } + return roots } /** True when `root` is workspace-controlled and must not be imported from. */ function isWorkspaceRoot(root: string, scope: { cwd: string | undefined; ancestors: string[] }): boolean { - const resolved = path.resolve(root) + // Real paths on both sides: a symlink pointing into the workspace must not + // slip past a purely lexical comparison. + const resolved = realPath(root) if (scope.ancestors.includes(resolved)) return true if (!scope.cwd) return false const rel = path.relative(scope.cwd, resolved) @@ -277,11 +305,12 @@ export function searchRootsFromError(error: unknown): string[] { if (!named) continue for (const candidate of [named, repairCwdPrefixedPath(named)]) { if (!candidate) continue - // Walk back to the enclosing node_modules directory. - const root = enclosingNodeModulesRoot(candidate) - if (!root || !isDirectory(root)) continue - if (isWorkspaceRoot(root, scope)) continue - if (!roots.includes(root)) roots.push(root) + // Walk back to every enclosing node_modules directory, innermost first. + for (const root of enclosingNodeModulesRoots(candidate)) { + if (!isDirectory(root)) continue + if (isWorkspaceRoot(root, scope)) continue + if (!roots.includes(root)) roots.push(root) + } } } return roots @@ -935,12 +964,21 @@ async function installOptionalDriverInternal( // held it has usually just installed the very thing we queued for, so // most contenders return "already present" instead of running a second // npm over the same tree — which is what produced the ENOTEMPTY races. - return withInstallLock(dir, async (acquired) => { - if (acquired && !options.force && installed(driver)) { - return { driver, packages, dir, installed: true, alreadyPresent: true } - } - return performInstall(driver, packages, dir, options) - }) + return withInstallLock( + dir, + async (acquired) => { + if (acquired && !options.force && installed(driver)) { + return { driver, packages, dir, installed: true, alreadyPresent: true } + } + return performInstall(driver, packages, dir, options) + }, + // Outlast one peer's install. A lock wait shorter than the install it is + // waiting on means a contender gives up while the holder's npm is still + // running and then installs unlocked over the same tree, which is the + // race this lock exists to stop. The two timeouts were independent + // constants, so raising the install timeout alone silently broke it. + { timeoutMs: (options.timeoutMs ?? 180_000) + 60_000 }, + ) }) installsInFlight.set(dir, run) try { @@ -976,6 +1014,20 @@ export function installLockPath(dir: string): string { return `${dir.replace(/[\\/]+$/, "")}.lock` } +/** + * The atomically-created lock itself, which lives *inside* the container + * `installLockPath()` names. + * + * Two directories rather than one so every path this mechanism writes sits + * under a single approved prefix. Stale recovery renames the held lock aside + * before deleting it, and that destination has to be somewhere the install tool + * asked permission for; a sibling of the container would be outside the + * `.lock/*` pattern the tool brokers. + */ +function heldLockPath(dir: string): string { + return path.join(installLockPath(dir), "held") +} + interface LockHolder { pid: number hostname: string @@ -1019,18 +1071,47 @@ function readLockHolder(lockDir: string): LockHolder | undefined { * directory). Liveness cannot be established, so age is the only signal * available and the lock ages out. */ -function isStaleLock(lockDir: string, holder: LockHolder | undefined, maxAgeMs: number): boolean { - if (holder && holder.hostname === os.hostname()) return !processExists(holder.pid) +function isStaleLock( + lockDir: string, + holder: LockHolder | undefined, + maxAgeMs: number, + hardMaxAgeMs: number, +): boolean { + const age = lockAgeMs(lockDir, holder) + if (holder && holder.hostname === os.hostname()) { + if (!processExists(holder.pid)) return true + // `processExists` answers "some process holds this pid", not "our installer + // is still running": a crashed owner's pid can be recycled by an unrelated + // long-lived process, and liveness alone would then keep the lock forever, + // making every later install wait out its timeout and run unlocked. So a + // live same-host owner is protected, but only up to a backstop far beyond + // any real npm run — long enough never to interrupt an install, short + // enough that a recycled pid cannot wedge the directory permanently. + return age !== undefined && age > hardMaxAgeMs + } + // No readable owner, or an owner on another host sharing a home directory. + // Liveness is not decidable, so age is the only signal there is. + return age !== undefined && age > maxAgeMs +} + +/** How long the lock has been held, by owner record or directory mtime. */ +function lockAgeMs(lockDir: string, holder: LockHolder | undefined): number | undefined { const startedAt = holder?.startedAt - if (typeof startedAt === "number") return Date.now() - startedAt > maxAgeMs + if (typeof startedAt === "number") return Date.now() - startedAt try { - return Date.now() - fs.statSync(lockDir).mtimeMs > maxAgeMs + return Date.now() - fs.statSync(lockDir).mtimeMs } catch { // Vanished between checks — someone else released it, so it is not stale. - return false + return undefined } } +/** True when two owner records describe the same acquisition. */ +function sameHolder(a: LockHolder | undefined, b: LockHolder | undefined): boolean { + if (!a || !b) return a === b + return a.pid === b.pid && a.hostname === b.hostname && a.startedAt === b.startedAt && a.token === b.token +} + /** * Take ownership of a lock judged stale, atomically, and remove it. * @@ -1042,21 +1123,42 @@ function isStaleLock(lockDir: string, holder: LockHolder | undefined, maxAgeMs: * and only that process goes on to delete it. The loser's rename fails and it * simply retries against whatever state now exists. */ -function claimStaleLock(lockDir: string): void { - const claimed = `${lockDir}.stale-${process.pid}-${Date.now()}` +function claimStaleLock(lockDir: string, judged: LockHolder | undefined): boolean { + // The staleness verdict was formed before this call, and the owner can have + // released the lock and a peer re-taken it since. Renaming blindly would move + // a *live* lock aside and put two installs over the same tree. Check the owner + // record still matches the one judged stale before touching anything. + if (!sameHolder(judged, readLockHolder(lockDir))) return false + + const claimed = path.join(path.dirname(lockDir), `stale-${process.pid}-${Date.now().toString(36)}`) try { fs.renameSync(lockDir, claimed) } catch { - // Another process claimed it first, or the owner released it. Either way - // there is nothing of ours to clean up; retry the acquire. - return + // Another process claimed it first, the owner released it, or the parent + // does not permit rename. Nothing of ours to clean up. + return false + } + + // The check above narrows the window but cannot close it — nothing makes + // "read the owner" and "rename" one operation. So confirm what was actually + // moved, and put it back if a peer had re-taken the lock in between. + if (!sameHolder(judged, readLockHolder(claimed))) { + try { + fs.renameSync(claimed, lockDir) + return false + } catch { + // Cannot restore — a third process has already re-created the lock. Fall + // through and remove what we moved rather than leaking it. + } } + try { fs.rmSync(claimed, { recursive: true, force: true }) } catch { // The rename already made the lock unreachable, so a leftover directory - // beside it costs nothing but disk. + // costs nothing but disk. } + return true } /** @@ -1068,12 +1170,22 @@ function claimStaleLock(lockDir: string): void { * the successor's live lock and admit a third process. The token is written * when the lock is taken, so a mismatch means the directory is somebody else's. */ -function releaseInstallLock(lockDir: string, token: string): void { +function releaseInstallLock(lockDir: string, token: string, ino: number | undefined): void { const holder = readLockHolder(lockDir) - // An unreadable owner file leaves nothing to compare against; the lock is - // most likely still ours (we wrote it) and leaking it would wedge every later - // install behind a lock nobody releases, so remove it. - if (holder?.token !== undefined && holder.token !== token) return + if (holder?.token !== undefined) { + if (holder.token !== token) return + } else if (ino !== undefined) { + // No token to compare against. There is a real window for this: the lock + // directory is created before `owner.json` is written, so a successor that + // re-took the lock in between holds a live lock carrying no token, and + // removing it by pathname would admit a third process. The directory's own + // identity settles it — a different inode is a different lock. + try { + if (fs.statSync(lockDir).ino !== ino) return + } catch { + return + } + } try { fs.rmSync(lockDir, { recursive: true, force: true }) } catch { @@ -1092,22 +1204,24 @@ function releaseInstallLock(lockDir: string, token: string): void { export async function withInstallLock( dir: string, fn: (acquired: boolean) => Promise, - options: { timeoutMs?: number; staleAfterMs?: number; pollMs?: number } = {}, + options: { timeoutMs?: number; staleAfterMs?: number; hardStaleAfterMs?: number; pollMs?: number } = {}, ): Promise { - const lockDir = installLockPath(dir) + const lockDir = heldLockPath(dir) const timeoutMs = options.timeoutMs ?? 240_000 const staleAfterMs = options.staleAfterMs ?? 300_000 + const hardStaleAfterMs = options.hardStaleAfterMs ?? Math.max(staleAfterMs, 3_600_000) const pollMs = options.pollMs ?? 100 const deadline = Date.now() + timeoutMs const token = `${process.pid}-${Date.now().toString(36)}-${Math.random().toString(36).slice(2, 10)}` let acquired = false - - // The lock's parent must exist before the atomic mkdir can land. On a cold - // machine nothing has created the XDG data directory yet — `performInstall` - // is the first thing that does, and it runs *after* this — so a non-recursive - // mkdir would fail ENOENT, take the "cannot lock" branch, and drop every - // caller straight to an unlocked install. That is precisely the cold-start - // stampede this lock exists to prevent, so create the parent first. + let ino: number | undefined + + // The container must exist before the atomic mkdir inside it can land. On a + // cold machine nothing has created the XDG data directory yet — + // `performInstall` is the first thing that does, and it runs *after* this — so + // a non-recursive mkdir would fail ENOENT, take the "cannot lock" branch, and + // drop every caller straight into an unlocked install. That is precisely the + // cold-start stampede this lock exists to prevent. try { fs.mkdirSync(path.dirname(lockDir), { recursive: true }) } catch { @@ -1119,15 +1233,24 @@ export async function withInstallLock( try { fs.mkdirSync(lockDir, { recursive: false }) acquired = true + try { + ino = fs.statSync(lockDir).ino + } catch { + // Identity check is skipped on release; the token check still applies. + } break } catch (e) { // Anything but "already held" — an unwritable parent, say — means we // cannot lock at all, so proceed unlocked rather than block forever. const code = e && typeof e === "object" && "code" in e ? (e as { code?: unknown }).code : undefined if (code !== "EEXIST") break - if (isStaleLock(lockDir, readLockHolder(lockDir), staleAfterMs)) { - claimStaleLock(lockDir) - continue + const holder = readLockHolder(lockDir) + if (isStaleLock(lockDir, holder, staleAfterMs, hardStaleAfterMs)) { + // A claim can fail persistently — a lock owned by another user, or a + // container that permits inspection but not rename. Retrying such a + // claim without yielding spins at full CPU and never reaches the + // deadline, so only a claim that actually succeeded skips the wait. + if (claimStaleLock(lockDir, holder)) continue } if (Date.now() >= deadline) break await sleep(pollMs) @@ -1153,7 +1276,7 @@ export async function withInstallLock( try { return await fn(acquired) } finally { - if (acquired) releaseInstallLock(lockDir, token) + if (acquired) releaseInstallLock(lockDir, token, ino) } } diff --git a/packages/drivers/test/install-lock.test.ts b/packages/drivers/test/install-lock.test.ts index b4c9ba31d..75071e664 100644 --- a/packages/drivers/test/install-lock.test.ts +++ b/packages/drivers/test/install-lock.test.ts @@ -18,6 +18,17 @@ import { installLockPath, withInstallLock } from "../src/resolve" const resolveModule = fileURLToPath(new URL("../src/resolve.ts", import.meta.url)) +/** The atomic lock inside the container, which is what contention is fought over. */ +const heldPath = (target: string) => path.join(installLockPath(target), "held") + +/** Plant a lock with a given owner record, as a peer process would leave it. */ +function plantLock(target: string, holder: Record) { + const held = heldPath(target) + fs.mkdirSync(held, { recursive: true }) + fs.writeFileSync(path.join(held, "owner.json"), JSON.stringify(holder)) + return held +} + let dir = "" beforeEach(() => { @@ -26,7 +37,6 @@ beforeEach(() => { afterEach(() => { if (dir) fs.rmSync(dir, { recursive: true, force: true }) - fs.rmSync(`${dir}.lock`, { recursive: true, force: true }) }) describe("cross-process install lock", () => { @@ -87,15 +97,15 @@ process.exit(0) } }, 60_000) - test("takes the lock when its parent directory does not exist yet", async () => { - // The cold-start shape this change exists for. `.lock` sits beside the + test("takes the lock when its container does not exist yet", async () => { + // The cold-start shape this change exists for. The lock sits beside the // managed directory, and on a fresh machine nothing has created the XDG data // directory yet — `performInstall` is the first thing that does, and it runs // *after* the lock is taken. A non-recursive mkdir would fail ENOENT, take // the "cannot lock" branch, and drop every concurrent CLI into an unlocked // install: exactly the stampede the lock is meant to stop. const target = path.join(dir, "fresh", "xdg", "altimate-code", "drivers") - expect(fs.existsSync(path.dirname(installLockPath(target)))).toBe(false) + expect(fs.existsSync(installLockPath(target))).toBe(false) let sawAcquired: boolean | undefined await withInstallLock( @@ -112,11 +122,7 @@ process.exit(0) const target = path.join(dir, "drivers") fs.mkdirSync(target, { recursive: true }) // Hold the lock with a live owner so it cannot be judged stale. - fs.mkdirSync(`${target}.lock`) - fs.writeFileSync( - path.join(`${target}.lock`, "owner.json"), - JSON.stringify({ pid: process.pid, hostname: os.hostname(), startedAt: Date.now() }), - ) + const held = plantLock(target, { pid: process.pid, hostname: os.hostname(), startedAt: Date.now() }) let sawAcquired: boolean | undefined await withInstallLock( @@ -130,18 +136,14 @@ process.exit(0) // turn contention into a hard failure — but it knows it was unlocked. expect(sawAcquired).toBe(false) // A lock we did not take must not be deleted on the way out. - expect(fs.existsSync(`${target}.lock`)).toBe(true) + expect(fs.existsSync(held)).toBe(true) }) test("breaks a lock whose owner is gone", async () => { const target = path.join(dir, "drivers") fs.mkdirSync(target, { recursive: true }) - fs.mkdirSync(`${target}.lock`) - fs.writeFileSync( - path.join(`${target}.lock`, "owner.json"), - // PID 0x7FFFFFFF is not a live process on any platform we run on. - JSON.stringify({ pid: 0x7fffffff, hostname: os.hostname(), startedAt: Date.now() }), - ) + // PID 0x7FFFFFFF is not a live process on any platform we run on. + plantLock(target, { pid: 0x7fffffff, hostname: os.hostname(), startedAt: Date.now() }) let sawAcquired: boolean | undefined await withInstallLock( @@ -159,11 +161,11 @@ process.exit(0) // sharing a home directory, because its pid means nothing here. const target = path.join(dir, "drivers") fs.mkdirSync(target, { recursive: true }) - fs.mkdirSync(`${target}.lock`) - fs.writeFileSync( - path.join(`${target}.lock`, "owner.json"), - JSON.stringify({ pid: process.pid, hostname: `${os.hostname()}-other`, startedAt: Date.now() - 10 * 60_000 }), - ) + plantLock(target, { + pid: process.pid, + hostname: `${os.hostname()}-other`, + startedAt: Date.now() - 10 * 60_000, + }) let sawAcquired: boolean | undefined await withInstallLock( @@ -181,14 +183,14 @@ process.exit(0) // as oracledb, or a caller that raised its own install timeout. Breaking a // live owner's lock would put two npm runs over the same tree, which is the // corruption this lock exists to prevent. Where liveness is decidable it is - // the only thing that counts. + // what counts. const target = path.join(dir, "drivers") fs.mkdirSync(target, { recursive: true }) - fs.mkdirSync(`${target}.lock`) - fs.writeFileSync( - path.join(`${target}.lock`, "owner.json"), - JSON.stringify({ pid: process.pid, hostname: os.hostname(), startedAt: Date.now() - 60 * 60_000 }), - ) + const held = plantLock(target, { + pid: process.pid, + hostname: os.hostname(), + startedAt: Date.now() - 60 * 60_000, + }) let sawAcquired: boolean | undefined await withInstallLock( @@ -196,13 +198,55 @@ process.exit(0) async (acquired) => { sawAcquired = acquired }, - { timeoutMs: 200, staleAfterMs: 1_000, pollMs: 20 }, + { timeoutMs: 200, staleAfterMs: 1_000, hardStaleAfterMs: 24 * 60 * 60_000, pollMs: 20 }, ) // Waited, then proceeded unlocked rather than stealing a running install. expect(sawAcquired).toBe(false) - expect(fs.existsSync(`${target}.lock`)).toBe(true) + expect(fs.existsSync(held)).toBe(true) }) + test("breaks a live-looking lock once past the hard backstop", async () => { + // `processExists` answers "some process holds this pid", not "our installer + // is still running". A crashed owner's pid can be recycled by an unrelated + // long-lived process, and liveness alone would then keep that lock forever — + // every later install waiting out its timeout and running unlocked. The + // backstop bounds that without interrupting any real install. + const target = path.join(dir, "drivers") + fs.mkdirSync(target, { recursive: true }) + plantLock(target, { pid: process.pid, hostname: os.hostname(), startedAt: Date.now() - 48 * 60 * 60_000 }) + + let sawAcquired: boolean | undefined + await withInstallLock( + target, + async (acquired) => { + sawAcquired = acquired + }, + { timeoutMs: 5000, staleAfterMs: 1_000, hardStaleAfterMs: 60_000, pollMs: 20 }, + ) + expect(sawAcquired).toBe(true) + }) + + test("gives up by the deadline when a stale lock cannot be claimed", async () => { + // A claim can fail persistently: a lock owned by another user, or a + // container that permits inspection but not rename. Retrying that without + // yielding spins at full CPU and never reaches the deadline, so this test + // hangs rather than fails if the bound is lost. + const target = path.join(dir, "drivers") + fs.mkdirSync(target, { recursive: true }) + plantLock(target, { pid: 0x7fffffff, hostname: os.hostname(), startedAt: Date.now() }) + const container = installLockPath(target) + fs.chmodSync(container, 0o555) + + const started = Date.now() + try { + await withInstallLock(target, async () => {}, { timeoutMs: 300, pollMs: 20 }) + } finally { + fs.chmodSync(container, 0o755) + } + // Bounded either way: root can still rename and simply acquires the lock. + expect(Date.now() - started).toBeLessThan(10_000) + }, 30_000) + test("does not delete a lock that has been re-taken by a peer", async () => { // A lock we hold can be broken as stale and re-acquired by someone else // while our critical section is still running. Releasing by pathname would @@ -210,19 +254,36 @@ process.exit(0) // release only removes a lock still carrying our own token. const target = path.join(dir, "drivers") fs.mkdirSync(target, { recursive: true }) - const lockDir = installLockPath(target) + const held = heldPath(target) await withInstallLock(target, async (acquired) => { expect(acquired).toBe(true) // A peer breaks our lock and takes its own. fs.writeFileSync( - path.join(lockDir, "owner.json"), + path.join(held, "owner.json"), JSON.stringify({ pid: process.pid, hostname: os.hostname(), startedAt: Date.now(), token: "successor" }), ) }) - expect(fs.existsSync(lockDir)).toBe(true) - fs.rmSync(lockDir, { recursive: true, force: true }) + expect(fs.existsSync(held)).toBe(true) + }) + + test("does not delete a successor lock that has no owner record yet", async () => { + // The lock directory is created before `owner.json` is written, so a + // successor can hold a live lock carrying no token at all. Identity of the + // directory itself is what settles ownership in that window. + const target = path.join(dir, "drivers") + fs.mkdirSync(target, { recursive: true }) + const held = heldPath(target) + + await withInstallLock(target, async (acquired) => { + expect(acquired).toBe(true) + // Replace the lock with a different directory carrying no owner record. + fs.rmSync(held, { recursive: true, force: true }) + fs.mkdirSync(held, { recursive: true }) + }) + + expect(fs.existsSync(held)).toBe(true) }) test("releases the lock when the critical section throws", async () => { @@ -237,6 +298,6 @@ process.exit(0) thrown = e instanceof Error ? e.message : String(e) } expect(thrown).toBe("boom") - expect(fs.existsSync(`${target}.lock`)).toBe(false) + expect(fs.existsSync(heldPath(target))).toBe(false) }) }) diff --git a/packages/drivers/test/resolve-cwd-prefix.test.ts b/packages/drivers/test/resolve-cwd-prefix.test.ts index 7aa82dd9e..cf86ac742 100644 --- a/packages/drivers/test/resolve-cwd-prefix.test.ts +++ b/packages/drivers/test/resolve-cwd-prefix.test.ts @@ -4,7 +4,7 @@ import os from "node:os" import path from "node:path" import { - enclosingNodeModulesRoot, + enclosingNodeModulesRoots, loadOptionalDriver, repairCwdPrefixedPath, searchRootsFromError, @@ -186,34 +186,38 @@ describe("cwd concatenated onto an absolute path", () => { }) }) -describe("enclosing node_modules root", () => { +describe("enclosing node_modules roots", () => { test("finds the root in a platform-native path", () => { - expect(enclosingNodeModulesRoot("/a/node_modules/pkg/index.js", "/")).toBe("/a/node_modules") + expect(enclosingNodeModulesRoots("/a/node_modules/pkg/index.js", "/")).toEqual(["/a/node_modules"]) }) - test("finds the last root when the path nests several", () => { - expect(enclosingNodeModulesRoot("/a/node_modules/b/node_modules/c/index.js", "/")).toBe( - "/a/node_modules/b/node_modules", - ) + test("returns every enclosing root, innermost first", () => { + // A quoted path often runs through a driver's own dependency. The innermost + // root holds that dependency; the *outer* one holds the driver being looked + // for, so returning only the innermost left the driver unfindable. + expect(enclosingNodeModulesRoots("/opt/node_modules/duckdb/node_modules/node-addon-api/x.js", "/")).toEqual([ + "/opt/node_modules/duckdb/node_modules", + "/opt/node_modules", + ]) }) test("returns nothing when the path names no node_modules", () => { - expect(enclosingNodeModulesRoot("/a/b/index.js", "/")).toBeUndefined() + expect(enclosingNodeModulesRoots("/a/b/index.js", "/")).toEqual([]) }) test("handles a Windows path quoted with forward slashes", () => { // Windows runtimes quote both shapes. The marker is built from the platform // separator, so without normalisation a forward-slash path would never match - // a backslash marker and the root would silently not be harvested. - expect(enclosingNodeModulesRoot("C:/app/node_modules/pkg/index.js", "\\")).toBe("C:\\app\\node_modules") + // a backslash marker and nothing would be harvested at all. + expect(enclosingNodeModulesRoots("C:/app/node_modules/pkg/index.js", "\\")).toEqual(["C:\\app\\node_modules"]) }) test("handles a Windows path quoted with backslashes", () => { - expect(enclosingNodeModulesRoot("C:\\app\\node_modules\\pkg\\index.js", "\\")).toBe("C:\\app\\node_modules") + expect(enclosingNodeModulesRoots("C:\\app\\node_modules\\pkg\\index.js", "\\")).toEqual(["C:\\app\\node_modules"]) }) test("handles a Windows path with mixed separators", () => { - expect(enclosingNodeModulesRoot("C:\\app/node_modules\\pkg/index.js", "\\")).toBe("C:\\app\\node_modules") + expect(enclosingNodeModulesRoots("C:\\app/node_modules\\pkg/index.js", "\\")).toEqual(["C:\\app\\node_modules"]) }) }) @@ -312,6 +316,28 @@ describe("harvested roots respect the workspace boundary", () => { expect(searchRootsFromError(error)).not.toContain(ancestorModules) }) + test("refuses a symlinked root whose target is inside the working directory", () => { + // The containment check must compare real paths. `isDirectory` follows + // symlinks, so a link whose lexical path sits outside the workspace but + // whose target sits inside it would otherwise pass a lexical exclusion and + // import workspace-controlled code anyway. + workspace = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "resolve-ws-"))) + const real = path.join(workspace, "inside") + writePackage(path.join(real, "node_modules"), "duckdb", "index.js", "module.exports = {}\n") + // The link lives outside the workspace and points back into it. + const linkHome = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "resolve-link-"))) + const link = path.join(linkHome, "node_modules") + fs.symlinkSync(path.join(real, "node_modules"), link, "dir") + process.chdir(workspace) + + try { + const error = new Error(`ENOENT: no such file or directory, open '${path.join(link, "duckdb", "package.json")}'`) + expect(searchRootsFromError(error)).toEqual([]) + } finally { + fs.rmSync(linkHome, { recursive: true, force: true }) + } + }) + test("still harvests a root outside the workspace", () => { // The exclusion must not swallow the case the harvesting exists for. workspace = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "resolve-ws-"))) From 836faa7e1f90c5f14f1e55e7c47c7708512b7d65 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Sun, 30 Aug 2026 01:47:42 -0700 Subject: [PATCH 5/9] fix(drivers): fail closed on an unavailable cwd, and keep a root path absolute MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two narrow fixes from the latest review round. **The workspace exclusion failed open when `process.cwd()` was unavailable.** `isWorkspaceRoot` returned false with no working directory to compare against, so every `node_modules` root an error happened to name was admitted — turning the one case where the process cannot see its own filesystem into the case with no boundary at all. It now fails closed: no cwd means no harvested roots. This was introduced by the guard added two commits ago, which is exactly the sort of thing a fail-open default hides. **A filesystem root lost its leading separator when deriving the lock path.** `installLockPath` stripped every trailing separator, so `/` became the relative `.lock` and `C:\` the drive-relative `C:.lock`. Two processes started from different working directories would then take different locks while installing into the same directory, which is the concurrent-npm mutation the lock exists to prevent. Roots are now left intact; trailing separators are still stripped everywhere else so `/` and `` agree on one lock. Gates: typecheck 13/13 (forced); marker check ok; lint 5903 warnings and 1 error, identical to `main`'s baseline; `packages/drivers` 258 pass; `test/altimate` 4226 pass. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VqnuBDGkh1ZT65Ti7e6DHZ --- packages/drivers/src/resolve.ts | 15 +++++++++++++-- packages/drivers/test/install-lock.test.ts | 15 +++++++++++++++ .../drivers/test/resolve-cwd-prefix.test.ts | 19 +++++++++++++++++++ 3 files changed, 47 insertions(+), 2 deletions(-) diff --git a/packages/drivers/src/resolve.ts b/packages/drivers/src/resolve.ts index b05e63ceb..29dd69e14 100644 --- a/packages/drivers/src/resolve.ts +++ b/packages/drivers/src/resolve.ts @@ -274,7 +274,11 @@ function isWorkspaceRoot(root: string, scope: { cwd: string | undefined; ancesto // slip past a purely lexical comparison. const resolved = realPath(root) if (scope.ancestors.includes(resolved)) return true - if (!scope.cwd) return false + // Fail closed. With no working directory there is nothing to compare against, + // and treating that as "not workspace content" would admit every root an error + // happens to name — turning the one case where the process cannot see its own + // filesystem into the case with no boundary at all. + if (!scope.cwd) return true const rel = path.relative(scope.cwd, resolved) return rel === "" || (!rel.startsWith("..") && !path.isAbsolute(rel)) } @@ -1011,7 +1015,14 @@ const installsInFlight = new Map>() * never sees it as stray package content. */ export function installLockPath(dir: string): string { - return `${dir.replace(/[\\/]+$/, "")}.lock` + const trimmed = dir.replace(/[\\/]+$/, "") + // Trailing separators are stripped so `/` and `` agree on one lock. + // A filesystem root is the exception: stripping there destroys the root — "/" + // would become the *relative* ".lock", and "C:\" the drive-relative "C:.lock" + // — so two processes started from different working directories would take + // different locks while installing into the same place. + const base = trimmed === "" || /^[A-Za-z]:$/.test(trimmed) ? dir : trimmed + return `${base}.lock` } /** diff --git a/packages/drivers/test/install-lock.test.ts b/packages/drivers/test/install-lock.test.ts index 75071e664..149859b23 100644 --- a/packages/drivers/test/install-lock.test.ts +++ b/packages/drivers/test/install-lock.test.ts @@ -39,6 +39,21 @@ afterEach(() => { if (dir) fs.rmSync(dir, { recursive: true, force: true }) }) +describe("install lock path", () => { + test("agrees on one lock whether or not the directory has a trailing separator", () => { + expect(installLockPath("/a/drivers/")).toBe(installLockPath("/a/drivers")) + }) + + test("keeps a filesystem root intact", () => { + // Stripping the separator from a root turns the lock into a *relative* + // path, so processes with different working directories would take + // different locks while installing into the same directory. + expect(installLockPath("/")).toBe("/.lock") + expect(path.isAbsolute(installLockPath("/"))).toBe(true) + expect(installLockPath("C:\\")).toBe("C:\\.lock") + }) +}) + describe("cross-process install lock", () => { test("excludes concurrent processes from the critical section", async () => { const target = path.join(dir, "drivers") diff --git a/packages/drivers/test/resolve-cwd-prefix.test.ts b/packages/drivers/test/resolve-cwd-prefix.test.ts index cf86ac742..2045e38a6 100644 --- a/packages/drivers/test/resolve-cwd-prefix.test.ts +++ b/packages/drivers/test/resolve-cwd-prefix.test.ts @@ -338,6 +338,25 @@ describe("harvested roots respect the workspace boundary", () => { } }) + test("harvests nothing at all when the working directory cannot be established", () => { + // Fail closed. With no cwd there is nothing to compare a root against, and + // treating that as "not workspace content" would admit every root an error + // names — turning the one case where the process cannot see its own + // filesystem into the case with no boundary at all. + const realCwd = process.cwd.bind(process) + process.cwd = () => { + throw Object.assign(new Error("ENOENT: no such file or directory, uv_cwd"), { code: "ENOENT" }) + } + try { + const error = new Error( + `ENOENT: no such file or directory, open '${path.join(outsideModules, "duckdb", "package.json")}'`, + ) + expect(searchRootsFromError(error)).toEqual([]) + } finally { + process.cwd = realCwd + } + }) + test("still harvests a root outside the workspace", () => { // The exclusion must not swallow the case the harvesting exists for. workspace = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "resolve-ws-"))) From 8e4cdc24f91226929168aee89a581a110c43f100 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Sun, 30 Aug 2026 09:48:15 -0700 Subject: [PATCH 6/9] test(drivers): add a chdir arm, so a green suite means something about `--dir` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six pilots were spent on a driver-load failure that only appeared under `--dir`, which calls `process.chdir()` (cli/cmd/run.ts) before any driver is loaded. Every local verification — and the rig's own pre-flight probe — ran without a chdir, so a green suite said nothing about the configuration that actually failed. This is the same class as an `--instance-dir` arm that never exercised the code it existed to test. Four tests: resolve before and after a chdir and compare, resolve from a directory unrelated to the install tree, assert a package present only under the working directory is never resolved out of it, and check that a chdir between two resolutions does not change the answer. They pass on the current resolver, which is the point — this is a guard, not a bug report. `resolveOptionalPackage` drives resolution from the explicit `paths` argument rather than from the anchor's base, so the working directory does not currently reach the result. That is a property worth pinning, because it is invisible in review and its absence is expensive to diagnose. Measured, not assumed: on Debian 12, with a binary cross-compiled using the production compile options against a real `npm install -g` tree and a real `npm install duckdb`, run as root from an unrelated directory, adding a `process.chdir()` between process start and driver load changed nothing. Bare import, `import(file://)`, `createRequire(abs)`, the `__dirname` a loaded CJS module sees, and `loadOptionalDriver` were byte-identical with and without it, and all succeeded. So on Bun 1.3.14 a compiled binary does not re-anchor absolute module paths after a chdir, and chdir alone does not explain the field failure. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VqnuBDGkh1ZT65Ti7e6DHZ --- packages/drivers/test/resolve-chdir.test.ts | 93 +++++++++++++++++++++ 1 file changed, 93 insertions(+) create mode 100644 packages/drivers/test/resolve-chdir.test.ts diff --git a/packages/drivers/test/resolve-chdir.test.ts b/packages/drivers/test/resolve-chdir.test.ts new file mode 100644 index 000000000..64ec5d33b --- /dev/null +++ b/packages/drivers/test/resolve-chdir.test.ts @@ -0,0 +1,93 @@ +import { afterEach, beforeEach, describe, expect, test } from "bun:test" +import fs from "node:fs" +import os from "node:os" +import path from "node:path" + +import { driverSearchRoots, resolveOptionalPackage } from "../src/resolve" + +// Six pilots were spent on a driver-load failure that only appeared under +// `--dir`, which calls `process.chdir()` (cli/cmd/run.ts) before any driver is +// loaded. Every local verification — and the rig's own pre-flight probe — ran +// without a chdir, so a green suite said nothing about the configuration that +// actually failed. +// +// This arm exists so that stops being true. Resolution must not depend on the +// working directory the process happens to hold when a driver is loaded, and a +// regression that reintroduces a cwd anchor has to fail here. + +let root = "" +let pkgRoot = "" +let nodeModules = "" +let elsewhere = "" +const originalCwd = process.cwd() + +function writePackage(dir: string, name: string, main: string, body: string) { + const pkgDir = path.join(dir, name) + fs.mkdirSync(pkgDir, { recursive: true }) + fs.writeFileSync(path.join(pkgDir, "package.json"), JSON.stringify({ name, version: "1.0.0", main })) + fs.writeFileSync(path.join(pkgDir, main), body) + return pkgDir +} + +beforeEach(() => { + root = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "resolve-chdir-"))) + pkgRoot = path.join(root, "lib", "node_modules", "altimate-code") + nodeModules = path.join(pkgRoot, "node_modules") + fs.mkdirSync(nodeModules, { recursive: true }) + writePackage(nodeModules, "duckdb", "index.js", "module.exports = { Database: function () {} }\n") + elsewhere = path.join(root, "unrelated-run-dir") + fs.mkdirSync(elsewhere, { recursive: true }) +}) + +afterEach(() => { + process.chdir(originalCwd) + if (root) fs.rmSync(root, { recursive: true, force: true }) +}) + +describe("resolution does not depend on the working directory", () => { + test("resolves the same package before and after a chdir", () => { + const before = resolveOptionalPackage("duckdb", [nodeModules]) + expect(before).toBeDefined() + + process.chdir(elsewhere) + const after = resolveOptionalPackage("duckdb", [nodeModules]) + expect(after).toBe(before) + }) + + test("resolves from a directory that is not an ancestor of the package", () => { + // `elsewhere` shares only the temp root with the package tree, so nothing + // about it can contribute to resolution. This is the rig's shape: the run + // directory and the install tree are unrelated. + process.chdir(elsewhere) + const resolved = resolveOptionalPackage("duckdb", [nodeModules]) + expect(resolved).toBeDefined() + expect(resolved!.startsWith(nodeModules)).toBe(true) + expect(fs.existsSync(resolved!)).toBe(true) + }) + + test("does not resolve out of the working directory's own node_modules", () => { + // A package present only under cwd must stay invisible: project trees are + // workspace-controlled executable content and are deliberately not searched. + const cwdModules = path.join(elsewhere, "node_modules") + fs.mkdirSync(cwdModules, { recursive: true }) + writePackage(cwdModules, "pg", "index.js", "module.exports = { Client: function () {} }\n") + + process.chdir(elsewhere) + // Assert on provenance, not absence: this repo legitimately has `pg` under + // its own tree, which driverSearchRoots finds. What must never happen is a + // resolution out of the working directory. + const resolved = resolveOptionalPackage("pg", driverSearchRoots()) + if (resolved !== undefined) expect(resolved.startsWith(elsewhere)).toBe(false) + }) + + test("a chdir between resolve and re-resolve does not change the answer", () => { + process.chdir(elsewhere) + const first = resolveOptionalPackage("duckdb", [nodeModules]) + process.chdir(originalCwd) + const second = resolveOptionalPackage("duckdb", [nodeModules]) + process.chdir(root) + const third = resolveOptionalPackage("duckdb", [nodeModules]) + expect(second).toBe(first) + expect(third).toBe(first) + }) +}) From cd25034b848cf0423cfdc8570ab6815cb82b7897 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Sun, 30 Aug 2026 10:02:14 -0700 Subject: [PATCH 7/9] fix(drivers): handle UNC paths, and give the install lock a per-holder budget MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review-round gaps, two of them Windows shapes that a macOS suite cannot notice on its own. **`installLockPath` broke on a UNC share root.** It special-cased the POSIX and drive-letter roots but not `\\server\share`, which stripped to `\\server\share.lock` — naming a *different network share* rather than anything inside the directory being locked. Two processes installing into a share would then take different locks, which is the concurrent npm mutation the lock exists to prevent. Root detection now covers all three shapes and the lock always lands inside the root. **The harvesting regex accepted no UNC path.** A Windows error quoting `\\server\share\node_modules\duckdb\package.json` yielded no roots at all, so a driver on a share stayed unfindable even though the error named its exact location. The pattern is now a named export, so the claim is testable from any platform instead of resting on inspection. **The lock wait outlasted only one peer.** Every process counts its deadline from its own start, so a single budget covers a single holder; with three or more contenders the last one's deadline expired part-way through somebody else's install and it fell through to an unlocked `performInstall`. The budget is now per holder — seeing the lock change hands is proof the queue is moving rather than wedged — with a bounded number of extensions so a machine that keeps feeding in contenders cannot block a caller indefinitely. Each test fails with its own fix reverted and passes with it: 1 of 10 for each Windows shape, 1 of 14 for the multi-peer wait. The multi-peer test spawns real processes, holds 400ms against a 600ms budget, and asserts none of the three ran unlocked. 12/12 repeat runs green. Also hardens the chdir arm per review. It asked for `duckdb`, which the repo's own `packages/drivers/node_modules` satisfies through the execPath and module-location roots no matter what the resolver does — it would have passed while proving nothing. It now uses specifiers that exist nowhere but the tree each test builds and asserts on an export marker, so a pass establishes which root satisfied the load. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VqnuBDGkh1ZT65Ti7e6DHZ --- packages/drivers/src/resolve.ts | 93 ++++++++++++++++--- packages/drivers/test/install-lock.test.ts | 37 ++++++++ packages/drivers/test/resolve-chdir.test.ts | 42 ++++++--- .../test/resolve-windows-shapes.test.ts | 81 ++++++++++++++++ 4 files changed, 228 insertions(+), 25 deletions(-) create mode 100644 packages/drivers/test/resolve-windows-shapes.test.ts diff --git a/packages/drivers/src/resolve.ts b/packages/drivers/src/resolve.ts index 29dd69e14..2259518de 100644 --- a/packages/drivers/src/resolve.ts +++ b/packages/drivers/src/resolve.ts @@ -299,14 +299,29 @@ function isWorkspaceRoot(root: string, scope: { cwd: string | undefined; ancesto * resolved credentials; mining a path out of an error message must not become a * way around that invariant. */ +/** + * Absolute paths a runtime quoted inside an error message. + * + * Three absolute shapes, and the third is the one that is easy to leave out: + * POSIX `/…`, a drive letter `C:\…`, and a UNC share `\\server\share\…`. A + * Windows error naming a driver on a share yields no roots at all without it, + * so a driver stays unfindable even though the error named its exact location. + */ +export function quotedAbsolutePaths(message: string): string[] { + const found: string[] = [] + const pattern = /['"`]((?:\/|[A-Za-z]:[\\/]|\\\\[^\\/'"`\n]+[\\/])[^'"`\n]+)['"`]/g + for (const match of message.matchAll(pattern)) { + const named = match[1] + if (named) found.push(named) + } + return found +} + export function searchRootsFromError(error: unknown): string[] { const message = error instanceof Error ? error.message : String(error) const scope = workspaceScope() const roots: string[] = [] - // Absolute paths the runtime quoted, POSIX or Windows. - for (const match of message.matchAll(/['"`]((?:\/|[A-Za-z]:[\\/])[^'"`\n]+)['"`]/g)) { - const named = match[1] - if (!named) continue + for (const named of quotedAbsolutePaths(message)) { for (const candidate of [named, repairCwdPrefixedPath(named)]) { if (!candidate) continue // Walk back to every enclosing node_modules directory, innermost first. @@ -1016,13 +1031,32 @@ const installsInFlight = new Map>() */ export function installLockPath(dir: string): string { const trimmed = dir.replace(/[\\/]+$/, "") + const separator = dir.includes("\\") ? "\\" : "/" // Trailing separators are stripped so `/` and `` agree on one lock. - // A filesystem root is the exception: stripping there destroys the root — "/" - // would become the *relative* ".lock", and "C:\" the drive-relative "C:.lock" - // — so two processes started from different working directories would take - // different locks while installing into the same place. - const base = trimmed === "" || /^[A-Za-z]:$/.test(trimmed) ? dir : trimmed - return `${base}.lock` + // A filesystem root is the exception: stripping there destroys the root, and + // the result names something *outside* the directory being locked, so two + // processes installing into the same place take different locks. Roots keep + // their separator and the lock goes inside them. + // + // / -> /.lock not the relative .lock + // C:\ -> C:\.lock not drive-relative C:.lock + // \\server\share\ -> \\server\share\.lock not a *different share* + if (isFilesystemRoot(trimmed)) return `${trimmed}${separator}.lock` + return `${trimmed}.lock` +} + +/** + * True when `candidate` — trailing separators already stripped — names a + * filesystem root rather than a directory inside one. + * + * The UNC case is the one that is easy to miss: a share root is a root in + * exactly the way a drive letter is, and appending `.lock` to it names an + * unrelated share rather than anything under the directory being locked. + */ +function isFilesystemRoot(candidate: string): boolean { + if (candidate === "") return true // POSIX "/" strips to "" + if (/^[A-Za-z]:$/.test(candidate)) return true // "C:\" strips to "C:" + return /^\\\\[^\\/]+\\[^\\/]+$/.test(candidate) // "\\server\share" } /** @@ -1204,6 +1238,24 @@ function releaseInstallLock(lockDir: string, token: string, ino: number | undefi } } +/** + * A value that changes when the lock changes hands. + * + * The owner's token when it published one; otherwise the lock directory's own + * identity, which a fresh `mkdir` changes even when the owner file is missing + * or unreadable. `undefined` means we could not tell, and the caller then + * treats the holder as unchanged rather than inventing progress. + */ +function lockIdentity(lockDir: string, holder: LockHolder | undefined): string | undefined { + if (holder?.token) return holder.token + try { + const stat = fs.statSync(lockDir) + return `${stat.ino}:${stat.mtimeMs}` + } catch { + return undefined + } +} + /** * Run `fn` while holding an exclusive lock on `dir`, across processes. * @@ -1222,7 +1274,18 @@ export async function withInstallLock( const staleAfterMs = options.staleAfterMs ?? 300_000 const hardStaleAfterMs = options.hardStaleAfterMs ?? Math.max(staleAfterMs, 3_600_000) const pollMs = options.pollMs ?? 100 - const deadline = Date.now() + timeoutMs + let deadline = Date.now() + timeoutMs + // The budget is per *holder*, not per wait. Every process counts its deadline + // from its own start, so a single budget only ever outlasts one peer: with + // three or more contenders the last one's deadline expires part-way through + // somebody else's install and it falls through to an unlocked performInstall + // — the concurrent npm mutation this lock exists to prevent. Seeing the lock + // change hands is proof the queue is moving rather than wedged, so each new + // holder gets its own budget. Extensions are capped so a machine that keeps + // feeding in contenders cannot block a caller indefinitely. + const maxHandovers = 32 + let handovers = 0 + let lastHolder: string | undefined const token = `${process.pid}-${Date.now().toString(36)}-${Math.random().toString(36).slice(2, 10)}` let acquired = false let ino: number | undefined @@ -1256,6 +1319,14 @@ export async function withInstallLock( const code = e && typeof e === "object" && "code" in e ? (e as { code?: unknown }).code : undefined if (code !== "EEXIST") break const holder = readLockHolder(lockDir) + const identity = lockIdentity(lockDir, holder) + if (identity !== undefined && identity !== lastHolder) { + if (lastHolder !== undefined && handovers < maxHandovers) { + handovers++ + deadline = Date.now() + timeoutMs + } + lastHolder = identity + } if (isStaleLock(lockDir, holder, staleAfterMs, hardStaleAfterMs)) { // A claim can fail persistently — a lock owned by another user, or a // container that permits inspection but not rename. Retrying such a diff --git a/packages/drivers/test/install-lock.test.ts b/packages/drivers/test/install-lock.test.ts index 149859b23..de3f5dcd9 100644 --- a/packages/drivers/test/install-lock.test.ts +++ b/packages/drivers/test/install-lock.test.ts @@ -133,6 +133,43 @@ process.exit(0) expect(sawAcquired).toBe(true) }) + test("waits behind more than one peer without falling through unlocked", async () => { + // Every process counts its deadline from its own start, so a single budget + // only ever outlasts ONE holder. With three contenders the last one's + // deadline expires part-way through somebody else's install and it runs + // performInstall unlocked — the concurrent npm mutation the lock exists to + // prevent. The hold below is deliberately longer than the timeout so the + // test fails unless the wait is extended each time the lock changes hands. + const target = path.join(dir, "drivers") + fs.mkdirSync(target, { recursive: true }) + const log = path.join(dir, "peers.txt") + + const child = path.join(dir, "peer.ts") + fs.writeFileSync( + child, + `import fs from "node:fs" +import { withInstallLock } from ${JSON.stringify(resolveModule)} +const [target, log, id] = process.argv.slice(2) +await withInstallLock(target, async (acquired) => { + fs.appendFileSync(log, \`\${acquired ? "locked" : "UNLOCKED"} \${id}\\n\`) + if (acquired) await new Promise((r) => setTimeout(r, 400)) +}, { timeoutMs: 600, pollMs: 20 }) +process.exit(0) +`, + ) + + const kids = Array.from({ length: 3 }, (_, i) => + Bun.spawn(["bun", child, target, log, String(i)], { stdout: "ignore", stderr: "ignore" }), + ) + expect(await Promise.all(kids.map((k) => k.exited))).toEqual([0, 0, 0]) + + const events = fs.readFileSync(log, "utf8").trim().split("\n").filter(Boolean) + expect(events.length).toBe(3) + // Three holds of 400ms against a 600ms budget: the third can only succeed + // if watching the lock change hands renewed its wait. + expect(events.filter((e) => e.startsWith("UNLOCKED"))).toEqual([]) + }, 60_000) + test("reports the section ran unlocked when the lock cannot be taken in time", async () => { const target = path.join(dir, "drivers") fs.mkdirSync(target, { recursive: true }) diff --git a/packages/drivers/test/resolve-chdir.test.ts b/packages/drivers/test/resolve-chdir.test.ts index 64ec5d33b..65afa4f83 100644 --- a/packages/drivers/test/resolve-chdir.test.ts +++ b/packages/drivers/test/resolve-chdir.test.ts @@ -3,8 +3,20 @@ import fs from "node:fs" import os from "node:os" import path from "node:path" +import { createRequire } from "node:module" +import { pathToFileURL } from "node:url" + import { driverSearchRoots, resolveOptionalPackage } from "../src/resolve" +// Specifiers that exist nowhere but the tree each test builds. Asking for a +// real driver name would let the repo's own `packages/drivers/node_modules` +// satisfy the lookup through the execPath and module-location roots — which no +// environment isolation can suppress — so the test would pass while proving +// nothing about which root actually won. +const FIXTURE = "altimate-chdir-fixture" +const CWD_ONLY = "altimate-cwd-only-fixture" +const MARKER = "resolved-from-the-fixture-tree" + // Six pilots were spent on a driver-load failure that only appeared under // `--dir`, which calls `process.chdir()` (cli/cmd/run.ts) before any driver is // loaded. Every local verification — and the rig's own pre-flight probe — ran @@ -34,7 +46,7 @@ beforeEach(() => { pkgRoot = path.join(root, "lib", "node_modules", "altimate-code") nodeModules = path.join(pkgRoot, "node_modules") fs.mkdirSync(nodeModules, { recursive: true }) - writePackage(nodeModules, "duckdb", "index.js", "module.exports = { Database: function () {} }\n") + writePackage(nodeModules, FIXTURE, "index.js", `module.exports = { marker: ${JSON.stringify(MARKER)} }\n`) elsewhere = path.join(root, "unrelated-run-dir") fs.mkdirSync(elsewhere, { recursive: true }) }) @@ -46,11 +58,11 @@ afterEach(() => { describe("resolution does not depend on the working directory", () => { test("resolves the same package before and after a chdir", () => { - const before = resolveOptionalPackage("duckdb", [nodeModules]) + const before = resolveOptionalPackage(FIXTURE, [nodeModules]) expect(before).toBeDefined() process.chdir(elsewhere) - const after = resolveOptionalPackage("duckdb", [nodeModules]) + const after = resolveOptionalPackage(FIXTURE, [nodeModules]) expect(after).toBe(before) }) @@ -59,10 +71,13 @@ describe("resolution does not depend on the working directory", () => { // about it can contribute to resolution. This is the rig's shape: the run // directory and the install tree are unrelated. process.chdir(elsewhere) - const resolved = resolveOptionalPackage("duckdb", [nodeModules]) + const resolved = resolveOptionalPackage(FIXTURE, [nodeModules]) expect(resolved).toBeDefined() expect(resolved!.startsWith(nodeModules)).toBe(true) - expect(fs.existsSync(resolved!)).toBe(true) + // Load it and read the marker, so the test reports which root satisfied the + // lookup rather than merely that something was found. + const loaded = createRequire(pathToFileURL(resolved!).href)(resolved!) + expect(loaded.marker).toBe(MARKER) }) test("does not resolve out of the working directory's own node_modules", () => { @@ -70,23 +85,22 @@ describe("resolution does not depend on the working directory", () => { // workspace-controlled executable content and are deliberately not searched. const cwdModules = path.join(elsewhere, "node_modules") fs.mkdirSync(cwdModules, { recursive: true }) - writePackage(cwdModules, "pg", "index.js", "module.exports = { Client: function () {} }\n") + writePackage(cwdModules, CWD_ONLY, "index.js", `module.exports = { marker: ${JSON.stringify(MARKER)} }\n`) process.chdir(elsewhere) - // Assert on provenance, not absence: this repo legitimately has `pg` under - // its own tree, which driverSearchRoots finds. What must never happen is a - // resolution out of the working directory. - const resolved = resolveOptionalPackage("pg", driverSearchRoots()) - if (resolved !== undefined) expect(resolved.startsWith(elsewhere)).toBe(false) + // The specifier exists nowhere else on the machine, so this is absence with + // a known cause: anything but undefined means the lookup reached into the + // working directory. + expect(resolveOptionalPackage(CWD_ONLY, driverSearchRoots())).toBeUndefined() }) test("a chdir between resolve and re-resolve does not change the answer", () => { process.chdir(elsewhere) - const first = resolveOptionalPackage("duckdb", [nodeModules]) + const first = resolveOptionalPackage(FIXTURE, [nodeModules]) process.chdir(originalCwd) - const second = resolveOptionalPackage("duckdb", [nodeModules]) + const second = resolveOptionalPackage(FIXTURE, [nodeModules]) process.chdir(root) - const third = resolveOptionalPackage("duckdb", [nodeModules]) + const third = resolveOptionalPackage(FIXTURE, [nodeModules]) expect(second).toBe(first) expect(third).toBe(first) }) diff --git a/packages/drivers/test/resolve-windows-shapes.test.ts b/packages/drivers/test/resolve-windows-shapes.test.ts new file mode 100644 index 000000000..c36e0d426 --- /dev/null +++ b/packages/drivers/test/resolve-windows-shapes.test.ts @@ -0,0 +1,81 @@ +import { describe, expect, test } from "bun:test" + +import { enclosingNodeModulesRoots, installLockPath, quotedAbsolutePaths } from "../src/resolve" + +// Windows path shapes, unit-testable from any platform because none of these +// touch the filesystem. UNC is the shape that keeps getting left out: a share +// root behaves like a drive root, and code that only special-cases `C:` gets it +// wrong in a way that is invisible until someone runs from a network share. + +describe("installLockPath keeps a root intact", () => { + test("puts the lock inside a POSIX root", () => { + // Stripping the separator would leave "", making the lock the *relative* + // ".lock" — a different file for every working directory. + expect(installLockPath("/")).toBe("/.lock") + }) + + test("puts the lock inside a drive root", () => { + // "C:.lock" is drive-relative, not the root of C:. + expect(installLockPath("C:\\")).toBe("C:\\.lock") + }) + + test("puts the lock inside a UNC share root", () => { + // The bug this pins: "\\\\server\\share" + ".lock" names the *share* + // "\\\\server\\share.lock", a different network location entirely, so two + // processes installing into the share would not share a lock. + expect(installLockPath("\\\\server\\share\\")).toBe("\\\\server\\share\\.lock") + expect(installLockPath("\\\\server\\share")).toBe("\\\\server\\share\\.lock") + }) + + test("still strips a trailing separator on an ordinary directory", () => { + expect(installLockPath("/home/u/drivers/")).toBe("/home/u/drivers.lock") + expect(installLockPath("/home/u/drivers")).toBe("/home/u/drivers.lock") + expect(installLockPath("\\\\server\\share\\drivers\\")).toBe("\\\\server\\share\\drivers.lock") + }) + + test("a directory deeper than the share root is not treated as a root", () => { + expect(installLockPath("\\\\server\\share\\a")).toBe("\\\\server\\share\\a.lock") + }) +}) + +describe("quotedAbsolutePaths covers every absolute shape", () => { + test("POSIX", () => { + expect(quotedAbsolutePaths(`open '/usr/lib/node_modules/x/package.json'`)).toEqual([ + "/usr/lib/node_modules/x/package.json", + ]) + }) + + test("drive letter, both separators", () => { + expect(quotedAbsolutePaths(`open "C:\\app\\node_modules\\x\\package.json"`)).toEqual([ + "C:\\app\\node_modules\\x\\package.json", + ]) + expect(quotedAbsolutePaths(`open "C:/app/node_modules/x/package.json"`)).toEqual([ + "C:/app/node_modules/x/package.json", + ]) + }) + + test("UNC share", () => { + // Without the UNC alternative this yields nothing, so a driver on a share + // stays unfindable even though the error named its exact location. + expect(quotedAbsolutePaths(`ENOENT: open '\\\\server\\share\\node_modules\\duckdb\\package.json'`)).toEqual([ + "\\\\server\\share\\node_modules\\duckdb\\package.json", + ]) + }) + + test("ignores relative paths and bare words", () => { + expect(quotedAbsolutePaths(`Cannot find package 'duckdb' from 'lib/x.js'`)).toEqual([]) + }) +}) + +describe("enclosingNodeModulesRoots on Windows separators", () => { + test("walks back through a UNC path, innermost first", () => { + const roots = enclosingNodeModulesRoots( + "\\\\server\\share\\app\\node_modules\\a\\node_modules\\duckdb\\package.json", + "\\", + ) + expect(roots).toEqual([ + "\\\\server\\share\\app\\node_modules\\a\\node_modules", + "\\\\server\\share\\app\\node_modules", + ]) + }) +}) From 6ba55f3e78a52cf78b0a71dcf0fcaff38ee46669 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Sun, 30 Aug 2026 18:56:41 -0700 Subject: [PATCH 8/9] fix(drivers): load a driver from its own directory, not through the process cwd MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Measured twice independently, from a global install run under `--dir`: the package manifest consulted while loading a resolved driver is looked up at the working directory concatenated onto the driver's absolute path. ENOENT ... open '/usr/lib/.../duckdb/package.json' while the file exists at that path without the `` prefix. Materialising that concatenated path makes the failing invocation pass, and so does running with cwd `/`, which makes the concatenation a no-op. Both directions agree, so the working directory is an input to a lookup that has no business consulting it. There is no invocation-level fix: `--dir` is itself what sets the working directory (`cli/cmd/run.ts`), so cwd `/` and `--dir ` are mutually exclusive. `import(pathToFileURL(abs))` is not enough, because it is the ESM loader's own manifest lookup that goes through cwd. Loading through a CommonJS require anchored at the resolved file makes every nested lookup — the manifest included — relative to the driver's own directory. The drivers loaded this way are CommonJS; an ESM-only package still needs the loader, so that path remains and is taken only for the error that specifically means "this is ESM". `process.chdir()` around the load would also neutralise the concatenation and is deliberately not used. It is global mutable state, these loads happen under concurrency, and it would corrupt resolution for unrelated work non-deterministically — worse than the fault it patches. The regression test loads a fixture that reports its own `__dirname` while the working directory is elsewhere, so the assertion is about which directory the module came from rather than that it merely loaded. With the load site reverted it fails, and it fails with the field's exact shape: "failed to load from the default module resolution: ENOENT ... package.json / A copy at ... was also tried and failed to load". Note what that test does and does not establish. It proves the load no longer routes through the ESM loader, which is the fix's mechanism. It does not simulate the runtime's cwd-joining itself — no environment outside the affected rig has reproduced that, across eight eliminated hypotheses — so the end-to-end confirmation has to come from a build run there. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VqnuBDGkh1ZT65Ti7e6DHZ --- packages/drivers/src/resolve.ts | 58 ++++++++++++++++++++- packages/drivers/test/resolve-chdir.test.ts | 31 ++++++++++- 2 files changed, 86 insertions(+), 3 deletions(-) diff --git a/packages/drivers/src/resolve.ts b/packages/drivers/src/resolve.ts index 2259518de..d7bc7144c 100644 --- a/packages/drivers/src/resolve.ts +++ b/packages/drivers/src/resolve.ts @@ -493,6 +493,60 @@ function entryFromManifest(pkgDir: string, specifier: string, pkg: string): stri * * @throws {DriverNotInstalledError} when the package is genuinely absent. */ +/** + * Load an already-resolved package from its own absolute location. + * + * `import(pathToFileURL(abs))` is not enough. The ESM loader reads the + * package's manifest to decide the module's type and exports, and in a compiled + * binary that lookup has been observed resolving against the *process working + * directory* rather than the module's own directory — measured twice + * independently, from a global install run under `--dir`: + * + * ENOENT ... open '/usr/lib/.../duckdb/package.json' + * + * while the file exists at that path without the `` prefix. Supplying that + * concatenated path is sufficient to make the load succeed, and running with + * cwd `/` — which makes the concatenation a no-op — clears it too. + * + * A CommonJS require anchored at the resolved file makes every nested lookup, + * the manifest included, relative to the driver's own directory, so the working + * directory is never an input. The drivers we load this way are CommonJS; an + * ESM-only package still needs the loader, so that path remains as a fallback + * and is taken only for the error that specifically means "this is ESM". + * + * `process.chdir()` around the load would also neutralise the concatenation and + * is deliberately not used: it is global mutable state, and these loads happen + * under concurrency, so it would corrupt resolution for unrelated work + * non-deterministically — worse than the fault it patches. + */ +function requireFromLocation(resolved: string): unknown { + const requireFrom = createRequire(pathToFileURL(resolved).href) + return requireFrom(resolved) +} + +/** True when a require failed only because the target is an ES module. */ +function isRequireOfEsm(error: unknown): boolean { + const code = error && typeof error === "object" && "code" in error ? (error as { code?: unknown }).code : undefined + if (code === "ERR_REQUIRE_ESM") return true + const message = error instanceof Error ? error.message : String(error) + return /require\(\) of ES Module|Cannot use import statement outside a module|Unexpected token 'export'/i.test(message) +} + +/** + * Load from `resolved`, preferring the cwd-independent path. + * + * The injected `importer` is still used for the ESM fallback so tests keep a + * seam over the loader. + */ +async function loadFromLocation(resolved: string, importer: (spec: string) => Promise): Promise { + try { + return requireFromLocation(resolved) + } catch (requireError) { + if (!isRequireOfEsm(requireError)) throw requireError + return await importer(pathToFileURL(resolved).href) + } +} + export async function loadOptionalDriver( driver: DriverName, specifier: string, @@ -526,7 +580,7 @@ export async function loadOptionalDriver( } try { - return await importer(pathToFileURL(resolved).href) + return await loadFromLocation(resolved, importer) } catch (loadError) { // On disk but will not load — a half-installed copy, or a native addon // built for another platform. When an ambient copy was also broken, @@ -650,7 +704,7 @@ export async function loadOptionalPackage(specifier: string): Promise import(/* @vite-ignore */ spec)) } } diff --git a/packages/drivers/test/resolve-chdir.test.ts b/packages/drivers/test/resolve-chdir.test.ts index 65afa4f83..650ea0464 100644 --- a/packages/drivers/test/resolve-chdir.test.ts +++ b/packages/drivers/test/resolve-chdir.test.ts @@ -6,7 +6,7 @@ import path from "node:path" import { createRequire } from "node:module" import { pathToFileURL } from "node:url" -import { driverSearchRoots, resolveOptionalPackage } from "../src/resolve" +import { driverSearchRoots, loadOptionalDriver, resolveOptionalPackage } from "../src/resolve" // Specifiers that exist nowhere but the tree each test builds. Asking for a // real driver name would let the repo's own `packages/drivers/node_modules` @@ -94,6 +94,35 @@ describe("resolution does not depend on the working directory", () => { expect(resolveOptionalPackage(CWD_ONLY, driverSearchRoots())).toBeUndefined() }) + test("loads from the package's own directory while cwd is somewhere else", async () => { + // Resolution being cwd-independent is not enough: the *load* consults the + // package manifest too, and in a compiled binary that lookup was observed + // resolving against the process working directory — + // `ENOENT ... open '/usr/lib/.../duckdb/package.json'` for a file that + // exists at that path without the prefix. This pins the load itself. + // + // The fixture reports its own __dirname, so the assertion is about which + // directory the module was loaded from rather than merely that it loaded. + const pkgDir = path.join(nodeModules, FIXTURE) + fs.writeFileSync(path.join(pkgDir, "index.js"), "module.exports = { dir: __dirname }\n") + + process.chdir(elsewhere) + + // The only route to the fixture root is the path named in the ambient + // failure, which is how the real failure surfaces it. + const named = path.join(pkgDir, "package.json") + const ambient = Object.assign(new Error(`ENOENT: no such file or directory, open '${named}'`), { + code: "ENOENT", + }) + const importer = async () => { + throw ambient + } + + const loaded: any = await loadOptionalDriver("duckdb", FIXTURE, importer) + const mod = loaded?.default ?? loaded + expect(mod.dir).toBe(fs.realpathSync(pkgDir)) + }) + test("a chdir between resolve and re-resolve does not change the answer", () => { process.chdir(elsewhere) const first = resolveOptionalPackage(FIXTURE, [nodeModules]) From 59a9874f50d3859324197c46853795669ab48571 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Sun, 30 Aug 2026 19:51:54 -0700 Subject: [PATCH 9/9] fix(drivers): keep our command line out of a driver's own resolution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `@mapbox/node-pre-gyp`, which packages several native modules including DuckDB, resolves a module's manifest by parsing the **host application's** `process.argv`. `find()` passes `argv: process.argv` into its own `Run`, `nopt` abbreviation-matches whatever it finds against node-pre-gyp's option list, and `node-pre-gyp.js:164` then does: package_json_path = path.join(this.opts.directory, package_json_path) `path.join`, not `path.resolve` — so an absolute manifest path is not discarded. Our `--dir` abbreviates to node-pre-gyp's `--directory`, so `altimate-code run --dir ` made the driver look for its manifest at `` concatenated with the manifest's own absolute path, and the load failed with an ENOENT naming a path that had never existed. That the failing path also looked cwd-prefixed was a coincidence: `--dir` is what sets the working directory, so the two values were always equal. Driver loads now run with `process.argv` trimmed to `[argv[0], argv[1]]`. Swapping a global is safe here in a way `process.chdir()` would not be: the load is a synchronous `require`, JavaScript is single-threaded, and there is no `await` between the swap and the restore, so no concurrent work can observe it. The asynchronous ESM fallback is deliberately left alone for that reason. Considered and rejected: renaming `--dir`. The collision is real, but the flag is public, renaming breaks every existing invocation, and it would fix only the one option name that happens to collide today rather than the mechanism. Not specific to us, to DuckDB, or to a compiled binary — any CLI embedding a node-pre-gyp-packaged module and accepting a flag that abbreviates to `--directory` is exposed. A long argv in a compiled binary only made it visible. Of the drivers installed here, only `duckdb` pulls in node-pre-gyp today; the fix is applied to every driver load rather than to DuckDB, because the exposure is a property of the packaging tool, not of the driver. The regression test reproduces the arithmetic of the one line that broke: a fixture that reads `--dir` out of `process.argv` and joins it onto its own absolute manifest path. With the neutralisation reverted it fails, reporting the flag as seen. Two further tests pin that the command line is restored afterwards, including when the load throws. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VqnuBDGkh1ZT65Ti7e6DHZ --- packages/drivers/src/resolve.ts | 43 ++++++- .../test/resolve-argv-isolation.test.ts | 109 ++++++++++++++++++ 2 files changed, 151 insertions(+), 1 deletion(-) create mode 100644 packages/drivers/test/resolve-argv-isolation.test.ts diff --git a/packages/drivers/src/resolve.ts b/packages/drivers/src/resolve.ts index d7bc7144c..6e5c1329d 100644 --- a/packages/drivers/src/resolve.ts +++ b/packages/drivers/src/resolve.ts @@ -519,9 +519,50 @@ function entryFromManifest(pkgDir: string, specifier: string, pkg: string): stri * under concurrency, so it would corrupt resolution for unrelated work * non-deterministically — worse than the fault it patches. */ +/** + * Run `fn` with this process's command line hidden from it. + * + * `@mapbox/node-pre-gyp`, which packages several native drivers, resolves a + * module's manifest by parsing the **host application's** `process.argv` — + * `find()` passes `argv: process.argv` into its own `Run`, `nopt` + * abbreviation-matches whatever it sees against node-pre-gyp's option list, and + * `node-pre-gyp.js:164` then does: + * + * package_json_path = path.join(this.opts.directory, package_json_path) + * + * `path.join`, not `path.resolve`, so an absolute manifest path is **not** + * discarded. Our `--dir` abbreviates to node-pre-gyp's `--directory`, so + * `altimate-code run --dir ` made the driver look for its manifest at + * `` + the manifest's own absolute path, and the load failed with an + * ENOENT naming a path that had never existed. + * + * Nothing about that is specific to us, to DuckDB, or to a compiled binary: any + * CLI that embeds a node-pre-gyp-packaged module and accepts a flag + * abbreviating to `--directory` is exposed. Reported upstream; this keeps our + * users working in the meantime. + * + * **Why swapping a global here is safe when `process.chdir()` would not be.** + * `fn` is a synchronous `require`. JavaScript is single-threaded and there is + * no `await` between the swap and the restore, so no other task can run while + * the command line is hidden and no concurrent load can observe it. The same + * trick around an awaited dynamic `import()` would be a genuine hazard, and is + * deliberately not done below. + */ +function withNeutralArgv(fn: () => T): T { + const saved = process.argv + // Keep argv[0] and argv[1] — the executable and the entry script. nopt only + // parses what follows, and node-pre-gyp expects those two to be present. + process.argv = saved.slice(0, 2) + try { + return fn() + } finally { + process.argv = saved + } +} + function requireFromLocation(resolved: string): unknown { const requireFrom = createRequire(pathToFileURL(resolved).href) - return requireFrom(resolved) + return withNeutralArgv(() => requireFrom(resolved)) } /** True when a require failed only because the target is an ES module. */ diff --git a/packages/drivers/test/resolve-argv-isolation.test.ts b/packages/drivers/test/resolve-argv-isolation.test.ts new file mode 100644 index 000000000..196738c14 --- /dev/null +++ b/packages/drivers/test/resolve-argv-isolation.test.ts @@ -0,0 +1,109 @@ +import { afterEach, beforeEach, describe, expect, test } from "bun:test" +import fs from "node:fs" +import os from "node:os" +import path from "node:path" + +import { loadOptionalDriver } from "../src/resolve" + +// `@mapbox/node-pre-gyp` resolves a native module's manifest by parsing the +// HOST APPLICATION's `process.argv`. `find()` passes `argv: process.argv` into +// its own `Run`, `nopt` abbreviation-matches our flags against node-pre-gyp's +// option list, and `node-pre-gyp.js:164` then does +// +// package_json_path = path.join(this.opts.directory, package_json_path) +// +// `path.join`, not `path.resolve` — so an absolute manifest path is not +// discarded. Our `--dir` abbreviates to `--directory`, and the driver ended up +// looking for its manifest at `<--dir value>` + the manifest's absolute path. +// +// The fixture below reproduces exactly that arithmetic. It does not stand in +// for node-pre-gyp in general; it stands in for the one line that broke. + +const FIXTURE = "altimate-argv-fixture" +const savedArgv = process.argv + +let root = "" +let nodeModules = "" +let pkgDir = "" + +beforeEach(() => { + root = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "resolve-argv-"))) + nodeModules = path.join(root, "node_modules") + pkgDir = path.join(nodeModules, FIXTURE) + fs.mkdirSync(pkgDir, { recursive: true }) + fs.writeFileSync( + path.join(pkgDir, "package.json"), + JSON.stringify({ name: FIXTURE, version: "1.0.0", main: "index.js" }), + ) + fs.writeFileSync( + path.join(pkgDir, "index.js"), + [ + "const path = require('path')", + "const i = process.argv.indexOf('--dir')", + "const directory = i !== -1 ? process.argv[i + 1] : undefined", + "const manifest = path.join(__dirname, 'package.json')", + "module.exports = {", + " sawDirFlag: directory !== undefined,", + " manifestPath: directory ? path.join(directory, manifest) : manifest,", + "}", + "", + ].join("\n"), + ) +}) + +afterEach(() => { + process.argv = savedArgv + if (root) fs.rmSync(root, { recursive: true, force: true }) +}) + +/** Reach the fixture the way the real failure does: via the harvested root. */ +function importerThrowingAt(target: string) { + const ambient = Object.assign(new Error(`ENOENT: no such file or directory, open '${target}'`), { code: "ENOENT" }) + return async () => { + throw ambient + } +} + +describe("a driver load does not see the host's command line", () => { + test("the loaded module cannot observe --dir", async () => { + process.argv = [savedArgv[0], savedArgv[1], "run", "--dir", "/some/project", "--print-logs"] + + const loaded: any = await loadOptionalDriver( + "duckdb", + FIXTURE, + importerThrowingAt(path.join(pkgDir, "package.json")), + ) + const mod = loaded?.default ?? loaded + + // Without argv neutralisation the fixture sees the flag and joins, which is + // precisely what sent the driver after a manifest that never existed. + expect(mod.sawDirFlag).toBe(false) + expect(mod.manifestPath).toBe(path.join(pkgDir, "package.json")) + expect(mod.manifestPath.startsWith("/some/project")).toBe(false) + }) + + test("restores the command line afterwards", async () => { + const argv = [savedArgv[0], savedArgv[1], "run", "--dir", "/some/project"] + process.argv = argv + + await loadOptionalDriver("duckdb", FIXTURE, importerThrowingAt(path.join(pkgDir, "package.json"))) + + expect(process.argv).toEqual(argv) + }) + + test("restores the command line even when the load throws", async () => { + fs.writeFileSync(path.join(pkgDir, "index.js"), "throw new Error('broken driver')\n") + const argv = [savedArgv[0], savedArgv[1], "run", "--dir", "/some/project"] + process.argv = argv + + let failed = false + try { + await loadOptionalDriver("duckdb", FIXTURE, importerThrowingAt(path.join(pkgDir, "package.json"))) + } catch { + failed = true + } + + expect(failed).toBe(true) + expect(process.argv).toEqual(argv) + }) +})