From a6d485abcac778010e4aafadcb55eba89ccb7915 Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Mon, 15 Jun 2026 01:53:56 +0200 Subject: [PATCH] fix: stabilize Windows pacquet install tests (#12410) - Replace lockfile env-document stream scanning with a FileHandle read loop that closes deterministically, including split-BOM handling. - Align pacquet's default `virtualStoreDirMaxLength` with pnpm's Windows default. - Forward pnpm's effective virtual store max length to delegated pacquet installs through `PNPM_CONFIG_VIRTUAL_STORE_DIR_MAX_LENGTH`, so currently published pacquet versions do not write mismatched `.modules.yaml` on Windows. --- .changeset/slow-windows-lockfile-streams.md | 3 +- installing/commands/src/installDeps.ts | 2 + installing/commands/src/runPacquet.ts | 24 +++++- installing/commands/test/runPacquet.ts | 1 + lockfile/fs/src/yamlDocuments.ts | 82 ++++++++++--------- lockfile/fs/test/yamlDocuments.test.ts | 24 ++++++ pacquet/crates/config/src/defaults.rs | 13 +-- pacquet/crates/config/src/lib.rs | 6 +- pacquet/crates/config/src/tests.rs | 12 ++- .../package-manager/src/install/tests.rs | 5 +- .../src/virtual_store_layout.rs | 5 +- 11 files changed, 110 insertions(+), 67 deletions(-) diff --git a/.changeset/slow-windows-lockfile-streams.md b/.changeset/slow-windows-lockfile-streams.md index 87bed57978..00c909c6dd 100644 --- a/.changeset/slow-windows-lockfile-streams.md +++ b/.changeset/slow-windows-lockfile-streams.md @@ -1,6 +1,7 @@ --- "@pnpm/lockfile.fs": patch +"@pnpm/installing.commands": patch "pnpm": patch --- -Wait for early-exited lockfile read streams to close before rewriting lockfiles. +Close lockfile reads deterministically before rewriting lockfiles and keep pacquet's virtual store directory length aligned with pnpm on Windows. diff --git a/installing/commands/src/installDeps.ts b/installing/commands/src/installDeps.ts index c4649ddfe5..b203f1e414 100644 --- a/installing/commands/src/installDeps.ts +++ b/installing/commands/src/installDeps.ts @@ -119,6 +119,7 @@ export type InstallDepsOptions = Pick & Pick // surface closely enough on that command that they're safe to pass // along. From `add`/`update`/`dedupe` we don't forward anything: those // commands carry flags pacquet's `install` doesn't recognize - // (`--save-dev`, `--save-peer`, etc.) which clap would reject. Either - // way pacquet picks up the settings users care about from - // `pnpm-workspace.yaml` / `.npmrc` on its own, so a non-install - // delegation isn't broken by the omission. + // (`--save-dev`, `--save-peer`, etc.) which clap would reject. const forwardedFlags = opts.isInstallCommand ? collectForwardedFlags(opts.argv) : [] // In resolve mode pacquet does the resolution itself, so it must not // be pinned to the existing lockfile — drop both injected flags. @@ -160,6 +164,7 @@ function makeRun (opts: MakeRunPacquetOpts): (callOpts?: RunPacquetCallOpts) => logger.info({ message: banner, prefix: opts.lockfileDir }) const child = spawn(pacquetBin, args, { cwd: opts.lockfileDir, + env: makePacquetEnv(opts), stdio: ['ignore', 'inherit', 'pipe'], }) const filterResolved = callOpts?.filterResolvedProgress === true @@ -197,6 +202,17 @@ function makeRun (opts: MakeRunPacquetOpts): (callOpts?: RunPacquetCallOpts) => } } +function makePacquetEnv (opts: MakeRunPacquetOpts): NodeJS.ProcessEnv { + const env = { ...process.env } + for (const key of Object.keys(env)) { + if (key.toLowerCase() === 'pnpm_config_virtual_store_dir_max_length') { + delete env[key] + } + } + env.PNPM_CONFIG_VIRTUAL_STORE_DIR_MAX_LENGTH = String(opts.virtualStoreDirMaxLength) + return env +} + /** * Path of the platform-specific native pacquet binary for the host. The * pacquet npm package ships a Node wrapper at `bin/pacquet` that uses diff --git a/installing/commands/test/runPacquet.ts b/installing/commands/test/runPacquet.ts index 655cb44506..1662919f4f 100644 --- a/installing/commands/test/runPacquet.ts +++ b/installing/commands/test/runPacquet.ts @@ -23,6 +23,7 @@ function makeEngine (lockfileDir: string, packageName: 'pacquet' | '@pnpm/pacque packageName, argv: { original: [], remain: [] }, isInstallCommand: true, + virtualStoreDirMaxLength: process.platform === 'win32' ? 60 : 120, }) } diff --git a/lockfile/fs/src/yamlDocuments.ts b/lockfile/fs/src/yamlDocuments.ts index 7fac5c2a7f..0c35a3bd10 100644 --- a/lockfile/fs/src/yamlDocuments.ts +++ b/lockfile/fs/src/yamlDocuments.ts @@ -1,4 +1,5 @@ -import { createReadStream, type ReadStream } from 'node:fs' +import { type FileHandle, open } from 'node:fs/promises' +import { StringDecoder } from 'node:string_decoder' import util from 'node:util' import stripBom from 'strip-bom' @@ -6,69 +7,70 @@ import stripBom from 'strip-bom' export const YAML_DOCUMENT_SEPARATOR = '\n---\n' export const YAML_DOCUMENT_START = '---\n' +const READ_BUFFER_SIZE = 64 * 1024 + /** * Reads the first YAML document from a multi-document YAML file using streaming. * The file must start with "---\n" to indicate it contains an env lockfile document. * Stops reading as soon as the second document separator is found. * Returns null if the file doesn't exist or doesn't start with "---\n". */ -export async function streamReadFirstYamlDocument (filePath: string): Promise { - const stream = createReadStream(filePath, { encoding: 'utf8' }) - const chunks = stream[Symbol.asyncIterator]() +export async function streamReadFirstYamlDocument (filePath: string, readBufferSize = READ_BUFFER_SIZE): Promise { + let fileHandle: FileHandle | undefined let buffer = '' + let firstChunk = true try { - // Phase 1: verify the file starts with "---\n" - - for (let chunk = await chunks.next(); !chunk.done; chunk = await chunks.next()) { // eslint-disable-line no-await-in-loop - if (buffer.length === 0) { - // Strip BOM from the first chunk. Safe because the stream uses utf8 encoding, - // so the 3-byte BOM is decoded into a single \uFEFF character in the first chunk. - buffer = stripBom(chunk.value as string) - } else { - buffer += chunk.value + fileHandle = await open(filePath, 'r') + const decoder = new StringDecoder('utf8') + const readBuffer = Buffer.allocUnsafe(normalizeReadBufferSize(readBufferSize)) + let position = 0 + while (true) { + const { bytesRead } = await fileHandle.read(readBuffer, 0, readBuffer.length, position) // eslint-disable-line no-await-in-loop + if (bytesRead === 0) break + position += bytesRead + let chunk = decoder.write(readBuffer.subarray(0, bytesRead)) + if (firstChunk && chunk.length > 0) { + // Strip BOM from the first chunk. Safe because the decoder uses utf8, + // so the 3-byte BOM is decoded into a single \uFEFF character. + chunk = stripBom(chunk) + firstChunk = false } + buffer += chunk // Normalize CRLF (Windows) to LF so document separator detection works. buffer = buffer.replace(/\r\n/g, '\n') - if (buffer.length >= YAML_DOCUMENT_START.length) break - } - if (!buffer.startsWith(YAML_DOCUMENT_START)) { - await closeStream(stream) - return null - } - // Phase 2: find the second "---" separator - let firstDocument: string | undefined - while (true) { + if (canRejectDocumentStart(buffer)) { + return null + } const sep = buffer.indexOf(YAML_DOCUMENT_SEPARATOR, YAML_DOCUMENT_START.length) if (sep !== -1) { - firstDocument = buffer.slice(YAML_DOCUMENT_START.length, sep) - break + return buffer.slice(YAML_DOCUMENT_START.length, sep) } - const chunk = await chunks.next() // eslint-disable-line no-await-in-loop - if (chunk.done) break - // Normalize CRLF (Windows) to LF so the separator search matches on Windows-checked-out files. - buffer = (buffer + chunk.value).replace(/\r\n/g, '\n') } - if (firstDocument != null) { - await closeStream(stream) - return firstDocument + const remainder = decoder.end() + if (remainder.length > 0) { + buffer += firstChunk ? stripBom(remainder) : remainder + buffer = buffer.replace(/\r\n/g, '\n') } + return null } catch (err: unknown) { - await closeStream(stream) if (util.types.isNativeError(err) && 'code' in err && err.code === 'ENOENT') { return null } throw err + } finally { + await fileHandle?.close().catch(() => {}) } - await closeStream(stream) - return null } -async function closeStream (stream: ReadStream): Promise { - if (stream.closed) return - await new Promise((resolve) => { - stream.once('close', resolve) - stream.destroy() - }) +function canRejectDocumentStart (buffer: string): boolean { + if (buffer.length < YAML_DOCUMENT_START.length) return false + if (buffer === '---\r') return false + return !buffer.startsWith(YAML_DOCUMENT_START) +} + +function normalizeReadBufferSize (readBufferSize: number): number { + const size = Number.isFinite(readBufferSize) ? Math.floor(readBufferSize) : READ_BUFFER_SIZE + return size > 0 ? size : READ_BUFFER_SIZE } /** diff --git a/lockfile/fs/test/yamlDocuments.test.ts b/lockfile/fs/test/yamlDocuments.test.ts index c27a2735e6..af79445a8c 100644 --- a/lockfile/fs/test/yamlDocuments.test.ts +++ b/lockfile/fs/test/yamlDocuments.test.ts @@ -61,6 +61,22 @@ describe('streamReadFirstYamlDocument', () => { expect(result).toBe('foo: bar') }) + test('handles BOM split across reads', async () => { + const dir = temporaryDirectory() + const filePath = path.join(dir, 'test.yaml') + fs.writeFileSync(filePath, '\uFEFF---\nfoo: bar\n---\nlockfileVersion: 9.0\n') + const result = await streamReadFirstYamlDocument(filePath, 2) + expect(result).toBe('foo: bar') + }) + + test.each([0, -1, Number.NaN])('falls back to default read buffer size for %p', async (readBufferSize) => { + const dir = temporaryDirectory() + const filePath = path.join(dir, 'test.yaml') + fs.writeFileSync(filePath, '---\nfoo: bar\n---\nlockfileVersion: 9.0\n') + const result = await streamReadFirstYamlDocument(filePath, readBufferSize) + expect(result).toBe('foo: bar') + }) + test('returns null for empty file', async () => { const dir = temporaryDirectory() const filePath = path.join(dir, 'test.yaml') @@ -88,6 +104,14 @@ describe('streamReadFirstYamlDocument', () => { expect(result).toBe(envContent) }) + test('handles CRLF document start split across reads', async () => { + const dir = temporaryDirectory() + const filePath = path.join(dir, 'test.yaml') + fs.writeFileSync(filePath, '---\r\nfoo: bar\r\n---\r\nlockfileVersion: 9.0\r\n') + const result = await streamReadFirstYamlDocument(filePath, 4) + expect(result).toBe('foo: bar') + }) + test('handles BOM with CRLF line endings', async () => { const dir = temporaryDirectory() const filePath = path.join(dir, 'test.yaml') diff --git a/pacquet/crates/config/src/defaults.rs b/pacquet/crates/config/src/defaults.rs index fe9ef48fe6..52896c9aa9 100644 --- a/pacquet/crates/config/src/defaults.rs +++ b/pacquet/crates/config/src/defaults.rs @@ -217,17 +217,12 @@ pub fn default_modules_cache_max_age() -> u64 { 10080 } -/// Default `virtualStoreDirMaxLength` matching pnpm's fallback at -/// . -/// -/// Kept as a free function (not a re-export of -/// `pacquet_modules_yaml::DEFAULT_VIRTUAL_STORE_DIR_MAX_LENGTH`) so -/// `pacquet-config` doesn't pull in the modules-yaml crate just for one -/// integer. Both copies must agree; the modules-yaml side carries the -/// same upstream link. +/// Default `virtualStoreDirMaxLength` matching pnpm's platform-aware +/// config default at +/// . #[must_use] pub fn default_virtual_store_dir_max_length() -> u64 { - 120 + if cfg!(windows) { 60 } else { 120 } } /// Default `peersSuffixMaxLength` matching pnpm's fallback at diff --git a/pacquet/crates/config/src/lib.rs b/pacquet/crates/config/src/lib.rs index dd97635009..4fa8a07d37 100644 --- a/pacquet/crates/config/src/lib.rs +++ b/pacquet/crates/config/src/lib.rs @@ -500,8 +500,8 @@ pub struct Config { /// flat name would exceed this many bytes, the tail is replaced /// with a 32-char sha256 hash so the path stays within filesystem /// limits (macOS / ext4 cap component names at 255 bytes; pnpm - /// defaults to 120 to leave headroom for `node_modules/` - /// suffixes appended below). + /// defaults to 60 on Windows and 120 elsewhere to leave headroom + /// for `node_modules/` suffixes appended below). /// /// Configurable via `virtualStoreDirMaxLength` in /// `pnpm-workspace.yaml`, global `config.yaml`, or @@ -511,7 +511,7 @@ pub struct Config { /// The same value is persisted into `node_modules/.modules.yaml` /// so subsequent installs see the user's pick. /// - /// Default value is 120. + /// Default value is 60 on Windows and 120 otherwise. #[default(_code = "default_virtual_store_dir_max_length()")] pub virtual_store_dir_max_length: u64, diff --git a/pacquet/crates/config/src/tests.rs b/pacquet/crates/config/src/tests.rs index bdfebcfaf9..f3bdbd2dfd 100644 --- a/pacquet/crates/config/src/tests.rs +++ b/pacquet/crates/config/src/tests.rs @@ -1504,16 +1504,14 @@ pub fn pnpm_config_hoist_false_clears_hoist_pattern() { ); } -/// `virtualStoreDirMaxLength` defaults to 120 — same value pnpm -/// writes when nothing is configured. The constant lives in -/// `pacquet-modules-yaml`; this asserts the config side carries -/// the matching default so a fresh install produces the same -/// virtual-store dirnames as pnpm. +/// `virtualStoreDirMaxLength` defaults to the same platform-aware value +/// pnpm uses when nothing is configured. #[test] -pub fn virtual_store_dir_max_length_defaults_to_120() { +pub fn virtual_store_dir_max_length_matches_pnpm_default() { let tmp = tempdir().unwrap(); let config = Config::new().current::(tmp.path()).expect("loads"); - assert_eq!(config.virtual_store_dir_max_length, 120); + let expected = if cfg!(windows) { 60 } else { 120 }; + assert_eq!(config.virtual_store_dir_max_length, expected); } /// `virtualStoreDirMaxLength` in `pnpm-workspace.yaml` overrides diff --git a/pacquet/crates/package-manager/src/install/tests.rs b/pacquet/crates/package-manager/src/install/tests.rs index 959bfec0a7..b569809939 100644 --- a/pacquet/crates/package-manager/src/install/tests.rs +++ b/pacquet/crates/package-manager/src/install/tests.rs @@ -888,7 +888,10 @@ async fn install_writes_modules_yaml() { // `modules_dir`, so a relative on-disk value round-trips back // to the absolute install-time path. assert_eq!(emitted_virtual_store_dir, virtual_store_dir.to_string_lossy()); - assert_eq!(virtual_store_dir_max_length, DEFAULT_VIRTUAL_STORE_DIR_MAX_LENGTH); + assert_eq!( + virtual_store_dir_max_length, + pacquet_config::default_virtual_store_dir_max_length(), + ); assert_eq!( registries.as_ref().and_then(|r| r.get("default")).map(String::as_str), Some(config.registry.as_str()), diff --git a/pacquet/crates/package-manager/src/virtual_store_layout.rs b/pacquet/crates/package-manager/src/virtual_store_layout.rs index 77b91272cd..6e9e0f4288 100644 --- a/pacquet/crates/package-manager/src/virtual_store_layout.rs +++ b/pacquet/crates/package-manager/src/virtual_store_layout.rs @@ -79,8 +79,9 @@ pub struct VirtualStoreLayout { /// the escaped filename exceeds this many bytes, the tail is /// replaced with a 32-char sha256 hash so the directory name fits /// within filesystem limits (macOS / ext4 cap component names at - /// 255 bytes, but pnpm defaults to 120 to leave headroom for the - /// `@/` suffix appended below). + /// 255 bytes, but pnpm defaults to 60 on Windows and 120 elsewhere + /// to leave headroom for the `@/` suffix appended + /// below). /// /// [`PkgNameVerPeer::to_virtual_store_name`]: pacquet_lockfile::PkgNameVerPeer::to_virtual_store_name virtual_store_dir_max_length: usize,