Repository navigation
fix(update): check and pin the npm cache root before staging (#6288) #6395
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,17 @@ | ||
| import { lstatSync, readdirSync, realpathSync, statSync } from "node:fs"; | ||
| import { spawnSync } from "node:child_process"; | ||
| import { homedir } from "node:os"; | ||
| import { dirname, isAbsolute, resolve } from "node:path"; | ||
| import { fileURLToPath } from "node:url"; | ||
| import { npmInvocation } from "./npm-invocation.mjs"; | ||
|
|
||
| const WORKER_ARG = "--ocx-npm-cache-preflight-worker"; | ||
| const ROOT_ONLY_ARG = "--root-only"; | ||
| const PROTOCOL_VERSION = 1; | ||
| // Windows starts npm through cmd.exe and is routinely slowed by on-access scanning, so the | ||
| // same `npm config get cache` that takes well under a second on POSIX can take several there. | ||
| const WORKER_TIMEOUT_MS = process.platform === "win32" ? 25_000 : 10_000; | ||
| const NPM_CONFIG_TIMEOUT_MS = process.platform === "win32" ? 15_000 : 5_000; | ||
| const INSPECTION_TIMEOUT_MS = 7_500; | ||
| const MAX_ENTRIES = 100_000; | ||
| const MAX_DEPTH = 64; | ||
|
|
@@ -17,6 +21,8 @@ const RESULT_REASONS = new Set([ | |
| "cache_entry_foreign_owner", | ||
| "cache_entry_inaccessible", | ||
| "cache_path_malformed", | ||
| "cache_root_dangling_link", | ||
| "cache_root_not_directory", | ||
| "inspection_incomplete", | ||
| "npm_config_failed", | ||
| "npm_unavailable", | ||
|
|
@@ -29,12 +35,58 @@ function inaccessibleByMode(stat) { | |
| return (ownerBits & 0o400) === 0; | ||
| } | ||
|
|
||
| /** | ||
| * Check that npm can create or use its cache ROOT (#6288). This is the part of the inspection | ||
| * that is meaningful on every platform: npm's first action is `mkdir -p <cache>`, and it fails | ||
| * with ENOTDIR when the root — or the nearest existing ancestor npm would create it under — is | ||
| * a file, or a link/junction whose target is gone. A Windows directory junction relocated to | ||
| * another volume is ordinary configuration; only an unresolvable one is a problem. | ||
| */ | ||
| export function inspectNpmCacheRoot(cachePath, options = {}) { | ||
| const lstat = options.lstatFn ?? lstatSync; | ||
| const stat = options.statFn ?? statSync; | ||
| let current = resolve(cachePath); | ||
| let entry; | ||
| for (;;) { | ||
| try { | ||
| entry = lstat(current); | ||
| break; | ||
| } catch (error) { | ||
| // A file somewhere above the root: POSIX reports ENOTDIR here, which is exactly the | ||
| // ENOTDIR npm's own mkdir would hit. Windows reports ENOENT and the walk reaches the file. | ||
| if (error?.code === "ENOTDIR") return { ok: false, reason: "cache_root_not_directory" }; | ||
| if (error?.code !== "ENOENT") return { ok: false, reason: "cache_entry_inaccessible" }; | ||
|
There was a problem hiding this comment.
When a missing cache suffix is beneath a regular-file ancestor (for example, Useful? React with 👍 / 👎. |
||
| const parent = dirname(current); | ||
| // Nothing on the path exists at all (a missing drive root); npm cannot mkdir there either. | ||
| if (parent === current) return { ok: false, reason: "cache_root_not_directory" }; | ||
| current = parent; | ||
| } | ||
| } | ||
| let target = entry; | ||
| if (entry.isSymbolicLink()) { | ||
| try { | ||
| target = stat(current); | ||
| } catch (error) { | ||
| return { | ||
| ok: false, | ||
| reason: error?.code === "ENOENT" || error?.code === "ENOTDIR" || error?.code === "ELOOP" | ||
| ? "cache_root_dangling_link" | ||
| : "cache_entry_inaccessible", | ||
| }; | ||
| } | ||
| } | ||
| if (!target.isDirectory()) return { ok: false, reason: "cache_root_not_directory" }; | ||
| return { ok: true, reason: "cache_accessible" }; | ||
| } | ||
|
|
||
| /** | ||
| * Inspect an existing Unix npm cache without following symlinks. The limits are | ||
| * deliberately part of the result contract: an incomplete inspection cannot prove | ||
| * that replacing the live package will succeed. | ||
| */ | ||
| export function inspectNpmCacheDirectory(cachePath, options = {}) { | ||
| const root = inspectNpmCacheRoot(cachePath, options); | ||
| if (!root.ok) return root; | ||
| const expectedUid = options.expectedUid ?? process.getuid?.(); | ||
| const deadline = (options.nowMs ?? Date.now)() + (options.timeoutMs ?? INSPECTION_TIMEOUT_MS); | ||
| const maxEntries = options.maxEntries ?? MAX_ENTRIES; | ||
|
|
@@ -118,22 +170,59 @@ export function inspectNpmCacheDirectory(cachePath, options = {}) { | |
| return { ok: true, reason: "cache_accessible" }; | ||
| } | ||
|
|
||
| // cmd.exe re-parses npm.cmd's %* after our quoting, so on Windows a pinned path must not carry | ||
| // characters that quoting cannot neutralize there. Such a path is refused, never escaped. | ||
| const WINDOWS_CMD_METACHARACTERS = /["%!^&|<>]/; | ||
|
|
||
| function validCachePath(value, platform = process.platform) { | ||
| return typeof value === "string" && value.length > 0 && value.length <= 4096 | ||
| && !value.includes("\0") && !/[\r\n]/.test(value) && isAbsolute(value) | ||
| && !(platform === "win32" && WINDOWS_CMD_METACHARACTERS.test(value)); | ||
| } | ||
|
|
||
| /** | ||
| * Resolve the cache npm itself uses, with the operator's full npmrc chain. The transactional | ||
| * stage pins this path with `--cache`: its `--prefix <stage>` moves npm's globalconfig to | ||
| * `<stage>/etc/npmrc`, so without the pin a `cache=` from the global npmrc is silently dropped | ||
| * and staging falls back to npm's default root — not the root this preflight checked (#6288). | ||
| */ | ||
| export function resolveNpmCachePath(options = {}) { | ||
| const env = options.env ?? process.env; | ||
| const platform = options.platform ?? process.platform; | ||
| // Global mode, from the home directory: the staged install is `npm install -g`, which never | ||
| // reads a project .npmrc, so a repository the user happens to run `ocx update` in must not be | ||
| // able to choose the cache that install is pinned to. | ||
| const invocation = (options.invocationFn ?? npmInvocation)(["config", "get", "cache", "--global"], platform, env); | ||
| if (!invocation) return { ok: false, reason: "npm_unavailable" }; | ||
| const npm = (options.spawnSyncFn ?? spawnSync)(invocation.file, invocation.args, { | ||
| encoding: "utf8", | ||
| timeout: options.timeoutMs ?? NPM_CONFIG_TIMEOUT_MS, | ||
| windowsHide: true, | ||
| cwd: options.cwd ?? homedir(), | ||
| ...invocation.options, | ||
| env: invocation.options?.env ?? env, | ||
| }); | ||
| if (npm?.status !== 0) return { ok: false, reason: "npm_config_failed" }; | ||
| const output = typeof npm.stdout === "string" ? npm.stdout.trim() : ""; | ||
| if (!validCachePath(output, platform)) return { ok: false, reason: "cache_path_malformed" }; | ||
| return { ok: true, path: output }; | ||
| } | ||
|
|
||
| function workerResult(argv) { | ||
| const rootOnly = argv.includes(ROOT_ONLY_ARG); | ||
| const supplied = argv.find(arg => arg !== ROOT_ONLY_ARG); | ||
| let cachePath; | ||
| if (supplied !== undefined) { | ||
| if (!validCachePath(supplied)) return { ok: false, reason: "cache_path_malformed" }; | ||
| cachePath = supplied; | ||
| } else { | ||
| const resolved = resolveNpmCachePath(); | ||
| if (!resolved.ok) return resolved; | ||
| cachePath = resolved.path; | ||
| } | ||
| // Windows has no uid and no Unix owner bits, so the deep ownership/mode walk proves nothing | ||
| // there; the root check is the part of the contract that holds on every platform. | ||
| return rootOnly ? inspectNpmCacheRoot(cachePath) : inspectNpmCacheDirectory(cachePath); | ||
| } | ||
|
|
||
| // Reasons that legitimately accompany `ok: true`. The parser below cross-checks the flag against | ||
|
|
@@ -161,15 +250,33 @@ function parseWorkerOutput(stdout) { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Run the bounded cache inspection in an isolated, synchronously-timeboxed worker. Pass | ||
| * `cachePath` (from {@link resolveNpmCachePath}) to inspect exactly the root a later stage pins; | ||
| * without it the worker resolves npm's cache itself. | ||
| */ | ||
| export function runNpmCachePreflight(options = {}) { | ||
| const result = runPreflightWorker(options); | ||
| // Before #6288 Windows skipped this gate entirely. Only the root shapes that make npm's own | ||
| // mkdir fail with ENOTDIR may now refuse an update there; a slow or unavailable npm, a worker | ||
| // timeout, or any other inconclusive result keeps the previous behavior instead of blocking. | ||
| if ((options.platform ?? process.platform) === "win32" && !result.ok | ||
| && !WINDOWS_BLOCKING_REASONS.has(result.reason)) { | ||
| return { ok: true, reason: "windows_skip" }; | ||
|
Comment on lines
+263
to
265
There was a problem hiding this comment.
This Windows fallback still returns Useful? React with 👍 / 👎. |
||
| } | ||
| return result; | ||
| } | ||
|
|
||
| const WINDOWS_BLOCKING_REASONS = new Set(["cache_root_dangling_link", "cache_root_not_directory"]); | ||
|
|
||
| function runPreflightWorker(options) { | ||
| const workerArgs = [fileURLToPath(import.meta.url), WORKER_ARG]; | ||
| if ((options.platform ?? process.platform) === "win32") workerArgs.push(ROOT_ONLY_ARG); | ||
| if (options.cachePath !== undefined) workerArgs.push(options.cachePath); | ||
| const spawn = options.spawnSyncFn ?? spawnSync; | ||
| const result = spawn( | ||
| options.execPath ?? process.execPath, | ||
| workerArgs, | ||
| { | ||
| encoding: "utf8", | ||
| timeout: options.timeoutMs ?? WORKER_TIMEOUT_MS, | ||
|
|
@@ -182,8 +289,14 @@ export function runNpmCachePreflight(options = {}) { | |
| return parseWorkerOutput(result.stdout) ?? { ok: false, reason: "worker_output_malformed" }; | ||
| } | ||
|
|
||
| /** Fixed operator guidance; worker/npm output and the cache path are intentionally never interpolated. */ | ||
| export function npmCachePreflightFailureMessage(reason) { | ||
| if (reason === "cache_root_dangling_link") { | ||
| return `npm cache access pre-flight failed (${reason}); npm's cache folder (see 'npm config get cache') is a link or junction whose target is missing — recreate the target folder or remove the link, then retry`; | ||
|
Contributor
There was a problem hiding this comment. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Match cache diagnostics to the updater’s global query. The updater resolves
Update the message assertions in As per path instructions, “Check that user-facing docs stay in sync with actual CLI/API behavior.” 📍 Affects 2 files
🤖 Prompt for AI AgentsSource: Path instructions |
||
| } | ||
| if (reason === "cache_root_not_directory") { | ||
| return `npm cache access pre-flight failed (${reason}); npm's cache folder (see 'npm config get cache') or a folder above it is a file or an unavailable drive, so npm would fail with ENOTDIR on mkdir — fix that path or set a different cache, then retry`; | ||
| } | ||
| return `npm cache access pre-flight failed (${reason}); fix cache ownership and permissions, then retry`; | ||
| } | ||
|
|
||
|
|
@@ -193,7 +306,7 @@ const isWorker = process.argv[1] | |
| if (isWorker) { | ||
| let result; | ||
| try { | ||
| result = workerResult(process.argv.slice(3)); | ||
| } catch { | ||
| result = { ok: false, reason: "cache_entry_inaccessible" }; | ||
| } | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restore
"windows_skip"toNpmCachePreflightReason.src/update/npm-cache-preflight.mjs, Line 265, still returns{ ok: true, reason: "windows_skip" }for inconclusive Windows results. The declaration now excludes that reachable result.A typed caller that checks
result.reason === "windows_skip"receives a non-overlapping comparison error, although the branch remains valid at runtime. TypeScript documents this behavior for literal unions. (typescriptlang.org)Restore the literal to the union.
Proposed correction
| "cache_root_not_directory" + | "windows_skip"📝 Committable suggestion
🤖 Prompt for AI Agents