mirror of
https://github.com/pnpm/pnpm.git
synced 2026-07-28 08:27:30 -04:00
node-runtime-fix
11458 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
dependabot[bot]
|
8e737f86ba |
chore(cargo): bump rusqlite from 0.38.0 to 0.39.0 (#318)
Bumps [rusqlite](https://github.com/rusqlite/rusqlite) from 0.38.0 to 0.39.0. - [Release notes](https://github.com/rusqlite/rusqlite/releases) - [Changelog](https://github.com/rusqlite/rusqlite/blob/master/Changelog.md) - [Commits](https://github.com/rusqlite/rusqlite/compare/v0.38.0...v0.39.0) --- updated-dependencies: - dependency-name: rusqlite dependency-version: 0.39.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> |
||
|
dependabot[bot]
|
70eb8538ea |
chore(github-actions): bump softprops/action-gh-release from 1 to 3 (#317)
Bumps [softprops/action-gh-release](https://github.com/softprops/action-gh-release) from 1 to 3. - [Release notes](https://github.com/softprops/action-gh-release/releases) - [Changelog](https://github.com/softprops/action-gh-release/blob/master/CHANGELOG.md) - [Commits](https://github.com/softprops/action-gh-release/compare/v1...v3) --- updated-dependencies: - dependency-name: softprops/action-gh-release dependency-version: '3' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> |
||
|
dependabot[bot]
|
79f3f00264 |
chore(github-actions): bump peter-evans/find-comment from 2 to 4 (#316)
Bumps [peter-evans/find-comment](https://github.com/peter-evans/find-comment) from 2 to 4. - [Release notes](https://github.com/peter-evans/find-comment/releases) - [Commits](https://github.com/peter-evans/find-comment/compare/v2...v4) --- updated-dependencies: - dependency-name: peter-evans/find-comment dependency-version: '4' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> |
||
|
dependabot[bot]
|
b2aef41c80 |
chore(github-actions): bump actions/cache from 4 to 5 (#315)
Bumps [actions/cache](https://github.com/actions/cache) from 4 to 5. - [Release notes](https://github.com/actions/cache/releases) - [Changelog](https://github.com/actions/cache/blob/main/RELEASES.md) - [Commits](https://github.com/actions/cache/compare/v4...v5) --- updated-dependencies: - dependency-name: actions/cache dependency-version: '5' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> |
||
|
dependabot[bot]
|
6a54795682 |
chore(github-actions): bump actions/download-artifact from 4 to 8 (#314)
Bumps [actions/download-artifact](https://github.com/actions/download-artifact) from 4 to 8. - [Release notes](https://github.com/actions/download-artifact/releases) - [Commits](https://github.com/actions/download-artifact/compare/v4...v8) --- updated-dependencies: - dependency-name: actions/download-artifact dependency-version: '8' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> |
||
|
Khải
|
ec6b632151 |
refactor: drop unnecessary PathBuf::from allocations (#319)
* perf: drop unnecessary PathBuf::from allocations PathBuf::from(&str) heap-allocates a new buffer; Path::new is a free borrow. Replace the call sites where Path::new compiles instead, and collapse PathBuf::from_str(&s).map_err(...)? into PathBuf::from(s) (the String is consumed without copying its buffer, and from_str returns Infallible so the error mapping was dead code). * style: use pipe-trait per CODE_STYLE_GUIDE.md Address review feedback on PR #319: use the pipe chain in the npmrc deserializer and restore the method chain in registry-mock dirs. * fix(npmrc): gate Path import to non-Windows test target The Path import is only used in a cfg-gated non-Windows test; on Windows clippy --deny warnings would fail on the unused import. --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Zoltan Kochan
|
4d073bec44 |
fix(tarball): drop DashMap shard guard before awaiting in mem cache (#313)
## Summary `run_with_mem_cache` deadlocks under enough concurrency: a DashMap `Ref` (which holds a synchronous shard read guard) is held across two `.await` points, so another task on the same worker can call `mem_cache.insert(...)` for a same-shard key, block on the write side, and starve the runtime. Fix: clone the inner `Arc` out of the `Ref` and drop the `Ref` before any `.await` so the shard guard's lifetime stays inside one synchronous step. ## Reproduce `pacquet install` against the `alot` benchmark workspace (~1300 packages) hangs at 0% CPU. `sample` against the hung process shows every tokio worker parked in `RawRwLock::lock_exclusive_slow`, with the main thread stuck inside `pacquet_tarball::DownloadTarballToStore::run_with_mem_cache` → `dashmap::insert` → `lock_exclusive_slow`. The pattern was pre-existing — likely intermittent before, reliably reproducible at higher concurrency. ## What changed `crates/tarball/src/lib.rs` — `mem_cache.get(package_url)` is now consumed by `.map(|r| Arc::clone(r.value()))`, returning `Option<Arc<RwLock<CacheValue>>>` with no shard guard held. The rest of the if-let block is unchanged. |
||
|
Zoltan Kochan
|
15fa5bee6c | chore(release): 0.2.2-4 | ||
|
Zoltan Kochan
|
2e7e5f7b5c |
fix(npm): preserve executable bit on platform binaries
Per-platform packages were published with the binary at mode 0644, so pacquet's JS shim's spawnSync failed with EACCES on install. The chmodSync(0o755) in generate-packages.mjs ran on the publish runner, but npm/pnpm pack normalizes file modes for deterministic tarballs: files referenced in the manifest's bin field become 0755 and everything else 0644. publishConfig.executableFiles is the npm-native way to keep additional files at 0755 in the published tarball without overloading bin's "this is a CLI entry point" semantics. Both npm and pnpm honor it. |
||
|
Zoltan Kochan
|
da1cc43090 | chore(release): 0.2.2-3 | ||
|
Zoltan Kochan
|
72db883995 |
chore(workspace): point Cargo.toml metadata at pnpm/pacquet
The workspace homepage and repository URLs still pointed at
anonrig/pacquet, the project's pre-donation home. The repo now lives
under pnpm/pacquet, so update the metadata to match. This only affects
crates.io display fields; the npm provenance fix was handled separately
in
|
||
|
Zoltan Kochan
|
9d0d3f235c |
fix(npm): point generated packages at pnpm/pacquet repo
The publish job uses npm trusted publishing with --provenance. npm's provenance verifier compares package.json's repository.url against the GitHub repo recorded in the OIDC-signed provenance bundle and rejects the publish when they disagree: Error verifying sigstore provenance bundle: Failed to validate repository information: package.json: "repository.url" is "https://github.com/anonrig/pacquet", expected to match "https://github.com/pnpm/pacquet" from provenance The main npm/pacquet/package.json was already updated to pnpm/pacquet, but the per-platform packages emitted by generate-packages.mjs still hardcoded the old anonrig URL. Update the URL there, and fix the same stale reference in the bin shim's "no prebuilt binary" error message so users land on the right issue tracker. |
||
|
Zoltan Kochan
|
4d4631f36a | chore(release): 0.2.2-2 | ||
|
Zoltan Kochan
|
eb98c99b03 |
fix(ci): give each matrix build a unique artifact name
actions/upload-artifact@v4 makes artifact names immutable within a run: when multiple matrix jobs upload to the same name, only the first succeeds and the others 409. With every platform uploading to 'binaries', only one file ended up downloadable, so the publish job saw no .zip and the unzip step failed with "No zipfiles found". Upload per-target as binaries-<code-target>, then download with pattern + merge-multiple to reassemble them in one directory. |
||
|
Zoltan Kochan
|
63c3049a9d | chore(release): 0.2.2-1 | ||
|
Zoltan Kochan
|
fb6de3e7ad |
fix(ci): replace retired ubuntu-20.04 runner with ubuntu-latest
The linux-arm64 build job was pinned to ubuntu-20.04, which GitHub retired from hosted runners. Jobs requesting that label hang in the queue forever. Match the other ubuntu entries by using ubuntu-latest. |
||
|
Zoltan Kochan
|
edafa10ff4 | chore(release): 0.2.2-0 | ||
|
Zoltan Kochan
|
a223c6d85b |
fix(ci): use correct package name pacquet-cli in release workflow
Cargo treats `_` and `-` as distinct in `-p` specs, so `cross build -p pacquet_cli` failed with "package ID specification did not match any packages". The crate is named `pacquet-cli` in crates/cli/Cargo.toml. |
||
|
Zoltan Kochan
|
8db71537b2 | chore(release): 0.2.2 | ||
|
Zoltan Kochan
|
b8f910374f |
ci(release): publish via pnpm using npm trusted publishing (#310)
Switch the release workflow's publish step from `npm publish` (with a long-lived `NPM_TOKEN` secret) to `pnpm publish` authenticated via GitHub OIDC, matching pnpm/pnpm's release flow. Setup uses `pnpm/action-setup@v6` with `standalone: true` and pins pnpm to the same `11.0.0-rc.5` used in CI; Node is provisioned via `pnpm runtime -g set node 22` so the package-generation script still has `node` on PATH. With `id-token: write` already granted, the registry verifies the workflow's OIDC token against each package's trusted-publisher config, so no token is needed for auth — the same token also backs the `--provenance` attestation. `--no-git-checks` is added because the unzipped binary artifacts leave the working tree dirty by the time publish runs. Drops the unused `discussions: write` permission. |
||
|
Zoltan Kochan
|
e8a8eaf352 |
docs: reframe project as the official pnpm rewrite (#308)
* docs: reframe project as the official pnpm rewrite - rewrite the top-level README around the pnpm-rewrite framing and a two-phase roadmap, dropping the stale TODO list - move debugging, testing, and benchmarking sections into CONTRIBUTING.md - add a README to the `npm/pacquet` package and update its description and keywords - update the `AGENTS.md` cross-reference for the relocated benchmark instructions * docs: surface "not production-ready" warning at the top of READMEs Per review feedback, lift the in-development warning into the first paragraph of `npm/pacquet/README.md` (with bold emphasis so it stays visible on the npm registry page) and add a matching admonition near the top of the workspace `README.md`. The goal is to make the project's status unmissable for anyone landing on either page. * docs(npm): mark the npm package description as a preview * docs(npm): note the package is not production-ready in its description |
||
|
Zoltan Kochan
|
56fff6bc14 |
fix(tarball): retry transient fetch errors with exponential backoff (#301)
## Summary A single transient network blip during a `pacquet install` (connection reset, TLS error, timeout, DNS hiccup, registry 5xx) used to abort the entire install. pnpm has retried these for years; pacquet now does too, with the same retry classification pnpm uses. ## What changes - **`fetch_and_extract_with_retry`** wraps the *full* tarball pipeline — request + body buffer + integrity check + gzip decode + tar extract — in one retried closure. Mirrors pnpm's [`remoteTarballFetcher.ts`](https://github.com/pnpm/pnpm/blob/main/fetching/tarball-fetcher/src/remoteTarballFetcher.ts): the body fetch and `addFilesFromTarball` (integrity + extraction) share a single retry boundary, so a flaky transfer that survives TCP framing but fails the SHA-512 hash or trips gzip / tar parsing recovers via re-fetch instead of aborting the install. - **`is_transient_error`** matches pnpm's policy exactly: only HTTP 401, 403, 404 fail fast (per `remoteTarballFetcher.ts:77-85`). Everything else — arbitrary 4xx (e.g. 410), 5xx, network reset, timeout, integrity mismatch, gzip / tar parse error, allocator refusal — retries. - **`RetryOpts`** mirrors `@zkochan/retry`'s formula (`min(min_timeout * factor.pow(attempt), max_timeout)`, `randomize: false`) with pnpm's defaults: 2 retries, factor 10, 10s floor, 60s cap. Same values as [`network/fetch/src/fetch.ts`](https://github.com/pnpm/pnpm/blob/main/network/fetch/src/fetch.ts#L29-L38) and [`config/config/src/index.ts`](https://github.com/pnpm/pnpm/blob/main/config/config/src/index.ts#L165-L168). - **HTTP status check** added to the fetch path. Previously a 4xx error body was downloaded and handed to the integrity check as if it were a tarball; now non-2xx surfaces as `TarballError::HttpStatus`. - **Config plumbing**: `Npmrc` gains `fetch_retries`, `fetch_retry_factor`, `fetch_retry_mintimeout`, `fetch_retry_maxtimeout`. Read **only** from `pnpm-workspace.yaml` as camelCase keys (`fetchRetries`, `fetchRetryFactor`, …) — pnpm 11s [`isIniConfigKey`](https://github.com/pnpm/pnpm/blob/main/config/config/src/auth.ts#L93-L94) excludes the `fetch-retry*` family from `NPM_AUTH_SETTINGS`, so a `fetch-retries=…` line in `.npmrc` is ignored upstream and is ignored here too. Conversion to `RetryOpts` is centralised in `package-manager::retry_config::retry_opts_from_config`. - **Permit lifecycle**: the post-download semaphore permit is acquired *inside* each attempt so a backoff sleep doesnt park one of the small pool of permits and block other downloads. The store-index writers `queue` call moves *outside* the retry loop so transient failures dont enqueue a half-built row that a successful retry would duplicate. Closes #259. |
||
|
Khải
|
34a17056bf |
docs: pin github upstream links to commit shas (#304)
Branch-tip links such as `github.com/pnpm/pnpm/blob/main/...` drift as the branch moves and 404 once a referenced file is renamed or deleted (two of the existing pacquet links were already pointing at moved paths). Replace every `blob/<branch>/` link in the repo with a `blob/<10-char-sha>/` permalink so the references stay meaningful, and update the AI-facing guidance in AGENTS.md to require permanent links going forward. Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Zoltan Kochan
|
ae4b1c8431 |
fix(network): match pnpm HTTP behaviour to fix tarball fetches (#302)
## Summary
A `pacquet install` was failing on `pump-2.0.1.tgz` with the opaque `error sending request for url` while a parallel `pnpm` on the same network downloaded it cleanly. Walking that down surfaced a series of independent parity gaps where pacquet's reqwest client diverged from how pnpm fetches from the registry — each one capable of producing the same generic "request failed" surface, and none of them visible in the rendered error. This PR closes all of them and adds a flattened source-chain renderer plus regression test so the next regression is debuggable.
## What changes
### `crates/network` — align the install client with pnpm's wire behaviour
- **`User-Agent: pnpm`** is set as a default header on the throttled client, matching the literal string pnpm v11 sends in [`network/fetch/src/fetchFromRegistry.ts`](https://github.com/pnpm/pnpm/blob/main/network/fetch/src/fetchFromRegistry.ts#L9). A default `reqwest::Client` sends no UA, which some registry CDNs and corporate WAFs treat as bot traffic and either reject at the edge or RST mid-handshake. Sending the literal `pnpm` (not `pacquet/<version>`) is deliberate: pacquet is a port, so any UA-keyed allow / rate-limit rule that lets pnpm through has to let pacquet through.
- **No default `Accept` header.** Pnpm attaches `application/vnd.npm.install-v1+json; q=1.0, application/json; q=0.8, */*` to every fetch including tarballs, but that's an upstream quirk we have no reason to copy. `crates/registry`'s metadata calls already set the npm-specific `Accept` per-request; tarballs send no `Accept` and the registry serves them fine.
- **Pool idle timeout 30s → 4s**, matching [`agentkeepalive`'s](https://github.com/node-modules/agentkeepalive/blob/master/lib/agent.js#L39-L41) default `freeSocketTimeout` (the agent pnpm's connection pool sits on top of). CDN edges in front of `registry.npmjs.org` close idle sockets after 5–15s without sending a FIN that hyper notices, so a longer pool TTL lets pacquet reuse a half-dead socket and surface the next request as a generic "error sending request for url" — same symptom the user hit. 4s keeps the pool useful for back-to-back downloads (pacquet runs hundreds of fetches in seconds) while staying well below typical edge keepalive.
- **`hickory-dns` resolver** in place of reqwest's default. The default routes lookups through tokio's `lookup_host`, which calls the platform's blocking `getaddrinfo`. On macOS that goes through `mDNSResponder`, which spuriously returns `EAI_NONAME` ("nodename nor servname provided") for valid hostnames when many concurrent lookups pile up — e.g. the dozens of simultaneous tarball connections this client opens. pnpm doesn't hit it because Node's `dns.lookup` runs on libuv's 4-thread pool, throttling concurrent `getaddrinfo` naturally. `hickory-dns` queries DNS over UDP/TCP directly and bypasses `mDNSResponder` entirely. Enabled via the `hickory-dns` reqwest feature in `Cargo.toml`.
- **Concurrency now follows pnpm's `networkConcurrency` formula** instead of being hardcoded at 50. Mirrors [`installing/package-requester/src/packageRequester.ts:97`](
|
||
|
Zoltan Kochan
|
5862180535 |
perf(store-dir): batch the prefetch SELECT to remove cold-cache regression (#298)
## Summary Closes #294. Replaces the per-key SELECT loop in `prefetch_cas_paths` with a single batched `SELECT key, data FROM package_index WHERE key IN (?, ?, …)`, chunked at 999 placeholders. SQLite walks the `package_index` PK B-tree once per chunk instead of once per key, collapsing N round-trips across the SQLite mutex into one query. This drops the ~50 ms of all-miss query overhead the empty-store install paid after #292 introduced the install-wide prefetch — the cold-cache regression the issue reports. ## Changes - `StoreIndex::get_many(&[String]) -> Result<HashMap<String, PackageFilesIndex>, StoreIndexError>` — batched lookup, chunked at `GET_MANY_CHUNK = 999` (safe vs. legacy `SQLITE_MAX_VARIABLE_NUMBER` builds; rusqlite's bundled SQLite caps at 32766). - `prefetch_cas_paths` Phase 1 now calls `guard.get_many(&cache_keys)` instead of looping `guard.get(&cache_key)` per key. ## Behaviour notes - Decode failures on a row are logged at `debug!` and skipped, matching `load_cached_cas_paths`'s `.ok()?` stance on the per-key path — a malformed row is a cache miss, not a batch failure. - A SQLite error during the batched read bubbles up as `StoreIndexError::Read`; `prefetch_cas_paths` already turns that into a soft fallback (per-snapshot lookups). |
||
|
Khải
|
e32e582627 |
ci(benchmark): remove exit_codes fields (#300)
|
||
|
Zoltan Kochan
|
569bdfd158 |
fix(fs): retry ensure_file opens on EMFILE/ENFILE (#290)
Resolves #289. ## Summary A cold install against an empty store on macOS dies with `Too many open files (os error 24)` because the spawned process's `RLIMIT_NOFILE` (10240 soft on macOS by default) doesn't survive pacquet's concurrent fan-out: - `post_download_semaphore` allows `num_cpus * 2` in-flight tarballs. - Each tarball's rayon entry-loop opens fds for write + occasional `verify_or_rewrite` byte-compare reads. - Inter-tarball content overlap (LICENSE / empty `.npmignore` / shared assets) sends multiple workers down the `AlreadyExists` → `file_equals_bytes` path simultaneously, each holding a read fd. `ulimit -n` reporting `unlimited` is a zsh display quirk on macOS — `getrlimit(RLIMIT_NOFILE)` for the spawned process is still the 10240 soft default. So a user with no shell tweaks just sees the install fail. ## What this PR does Match pnpm's `graceful-fs` shape: a `retry_on_fd_pressure` helper that wraps the three open sites in `crates/fs/src/ensure_file.rs`: 1. `ensure_file`'s EAFP `O_CREAT|O_EXCL` open 2. `file_equals_bytes`'s `File::open` (the one that surfaced the bug) 3. `write_atomic`'s temp-file `O_CREAT|O_EXCL` open Helper catches `EMFILE` / `ENFILE` (POSIX errno 24 / 23, hardcoded under `cfg(unix)` rather than pulling in `libc` for two stable integers), sleeps with exponential backoff (2 ms doubling, capped at 200 ms), retries up to 32 times before a final attempt propagates the error. Roughly a 5–6 s budget. Steady-state happy-path cost is one branch. Windows: pass-through. Different errno namespace, different fd model, not where users are hitting this. ## Why retries instead of a bounded-concurrency semaphore A semaphore is the more "principled" fix — proactive vs reactive — but threading one through every fs call site is a real structural change. Retry-on-EMFILE matches pnpm's production behaviour, the scope is one file, and it unblocks the immediate failure. A future PR can layer a semaphore on top if convoy-effect sleeps show up in profiling under sustained pressure. |
||
|
Zoltan Kochan
|
5f01470381 |
fix(registry-mock): walk parent chain iteratively, with cycle guard (#297)
`is_descent_of` recursed up `Process::parent()` and overflowed the default 1 MiB stack on `windows-latest` during the post-test verdaccio cleanup (`pacquet-registry-mock end`). Test runs that otherwise passed all 200 tests still failed the job because the cleanup step crashed with `STATUS_STACK_OVERFLOW` (seen on #296 CI). Convert to a loop and add a `HashSet<Pid>` cycle guard. Two reasons: 1. **Stack safety.** Iterating uses constant stack regardless of process-tree depth, so deep ancestor chains can't take cleanup down again. 2. **Cycle safety.** `Process::parent()` on Windows returns `dwParentProcessID` as recorded at process-create time and is never updated when the parent dies, so the recorded parent PID can be reused by an unrelated later process and observably "loop back" through a snapshot. The `visited` set bounds the walk in that case too. |
||
|
Zoltan Kochan
|
0d9b140c31 |
test(integrated-benchmark): add headless-install-with-hot-cache scenario (#296)
* bench(integrated-benchmark): add headless-install-with-hot-cache scenario Mirrors pnpm v11's `Headless install (frozen lockfile, warm store+cache)` benchmark (`pnpm/v11/benchmarks/bench.sh`): the common "CI install" / "fresh clone in an existing developer checkout" path where the lockfile is present, `node_modules` is empty, but the store has been populated by a prior install and should be reused. The existing `frozen-lockfile` scenario stays as-is and remains cold-cache (each timed run wipes both `node_modules` and the per-revision `store-dir`). The new `frozen-lockfile-hot-cache` scenario only wipes `node_modules` between iterations; hyperfine's warmup run is what populates the store on the first iteration so every timed run sees a hot store. Per-revision cleanup paths now come from `BenchmarkScenario::cleanup_paths`. The CI workflow gains a second `Benchmark: …` step + summary section so PRs see both cold-cache and hot-cache numbers side by side. * refactor(integrated-benchmark): pre-wipe store-dir before hyperfine The hot-cache scenario's per-iteration `--prepare` intentionally preserves `store-dir` so subsequent iterations can reuse it. That means whatever a previous run / scenario / partial invocation left in `store-dir` would otherwise carry into the warmup, and the warmup wouldn't actually be the priming run. The CI workflow runs the cold-cache step before the hot-cache step in the same `bench-work-env`, which would have hit exactly this case — the cold-cache step ends with a populated `store-dir` from its last timed iteration, and the hot-cache step would inherit it. Wipe `node_modules` and `store-dir` for every benchmark target once at the top of `benchmark()`, regardless of scenario. For cold-cache scenarios this is redundant with the per-iteration wipe but harmless; for hot-cache it makes the warmup the priming run no matter what state the work-env was in (Copilot review on #296). * refactor(integrated-benchmark): drop dead `store-dir` line from generated .npmrc Pacquet's `.npmrc` parser (`crates/npmrc/src/npmrc_auth.rs`) explicitly ignores `store-dir`; pinned by the `ignores_non_auth_keys` test there. Project-structural settings like `storeDir` only come from `pnpm-workspace.yaml` now (true for pnpm too). The static fixture's `storeDir: ./store-dir` already resolves to `{bench_dir}/store-dir` under each per-revision CWD, so removing the `.npmrc` line doesn't change behaviour — it just deletes a dead config that was misleading anyone reading the generated `.npmrc`. * fix(integrated-benchmark): always ensure storeDir is set in pnpm-workspace.yaml Without a top-level `storeDir:` in the per-revision `pnpm-workspace.yaml`, both pnpm and pacquet route to their global default store (pacquet since `npmrc_auth.rs` explicitly ignores `store-dir` from `.npmrc`). The benchmark's cleanup machinery then wipes `{bench_dir}/store-dir`, which the install never wrote to, and state from previous runs leaks into the warmup. That silently invalidates the cold-cache scenario's per-iteration reset and the hot-cache scenario's "warmup is the priming run" guarantee. The default fixture has always shipped `storeDir: ./store-dir`, but custom `--fixture-dir` runs let users supply a workspace file that might omit it (or supply no workspace file at all, which the helper previously treated as fine). Now `create_pnpm_workspace` scans for a top-level `storeDir:` and prepends `storeDir: ./store-dir\n` if one isn't already there. If the user explicitly declares a different `storeDir`, we leave it alone — that's an intentional override (e.g. sharing a store across revisions to bench a specific scenario). Copilot review on #296. |
||
|
Khải
|
b4310a71b1 |
docs(dev/guide): more style and contribution guides (#255)
* docs: fix bugs and typos in CODE_STYLE_GUIDE.md
- Correct push_path/push_back function name mismatch
- Fix non-compiling call sites in the &Path example so the snippet
type-checks against the stated signature
- Add missing `move` and trailing `;` to tokio::task::spawn examples
so they compile
- Rename "Cloning an atomic counter" section to "Cloning `Arc` and `Rc`"
since Arc/Rc are reference counters, not atomic counters
- Fix missing semicolon in an assert_eq! example
- Grammar fixes ("could easily be refactored", "Explicitly marking
the cloned type aids", "as more pull requests are reviewed")
* docs: add CONTRIBUTING.md and AGENTS.md
Adapted from KSXGitHub/parallel-disk-usage's CONTRIBUTING.md, with
pacquet-specific changes:
- No version-release exception in the commit message convention
- Setup and automated-check sections rewritten around `just init`,
`just install`, and `just ready` instead of `test.sh`
- Dropped the squashfs/fuse external-dependency section (not relevant
to pacquet); kept the registry-mock note instead
- Examples reworked to use pacquet types (manifests, store, etc.)
- Cross-links between CONTRIBUTING.md and CODE_STYLE_GUIDE.md so the
two documents are discovered together
AGENTS.md instructs AI agents to follow both guides.
* docs: pass &mut my_list in push_path examples
Addresses copilot review comment on PR #255: `push_path` takes
`&mut Vec<PathBuf>`, so the call sites need `&mut my_list` rather
than `my_list` to compile.
* chore: temporarily remove AGENTS.md before merging main
main already has an AGENTS.md (added in #268) with content unrelated
to my edits. Removing the duplicate so the merge from main pulls in
that file cleanly; the AI-guide references will be re-added on top.
* docs(agents): point at CONTRIBUTING.md and CODE_STYLE_GUIDE.md
Adds an explicit "Follow the project guides" section near the top of
AGENTS.md and lists CONTRIBUTING.md alongside CODE_STYLE_GUIDE.md in
the repo-layout overview, so both guides are surfaced as required
reading.
* docs: move code-style sections from CONTRIBUTING.md to CODE_STYLE_GUIDE.md
Per option A of the contributing/style split discussion: keep
CONTRIBUTING.md focused on process (commit message format, writing
style, setup, automated checks), and have CODE_STYLE_GUIDE.md own
every code-level convention.
Moved into CODE_STYLE_GUIDE.md (under the existing `## Guides`
section, in thematic order):
- Module Organization
- Import Organization
- Derive Macro Ordering
- Generic Parameter Naming
- Variable and Closure Parameter Naming (with the single-letter rules)
- Trait Bounds
- Pattern Matching
- Using `pipe-trait` (with when-to-use and when-not-to-use)
- Error Handling
- Conditional Test Skipping (`#[cfg]` vs `#[cfg_attr(..., ignore)]`)
- Unit test file layout (was the top-level `## Unit Tests` section)
The intra-doc link from "Where the external file sits" to
"Module Organization" still resolves because both sections now live
in the same file.
Updated `AGENTS.md`'s descriptions for both guides to reflect the new
split. Per the review thread on this PR, no other prose in
`AGENTS.md` was rewritten for writing-style conformance; that is
deferred to the maintainer.
* docs(style): drop "Derive Macro Ordering" section
The codebase does not follow the rules this section prescribed, so
the section was aspirational rather than descriptive of pacquet's
actual conventions:
- "Split across multiple #[derive(...)] lines" — never done. Across
52 files with derives, zero stack two #[derive(...)] attributes.
- Prescribed standard → comparison → Hash → derive_more order —
frequently violated. The dominant pattern places Display right
after Debug (e.g. crates/lockfile/src/comver.rs:8,
crates/lockfile/src/pkg_ver_peer.rs:12).
- #[cfg_attr(feature = "...", derive(...))] split — zero
occurrences anywhere in crates/.
The codebase's actual convention is too loose to codify usefully
(one #[derive(...)] line, Debug first when present), so the section
is removed rather than weakened to a near-tautology.
* docs(style): use .filter on both sides of the parameter-naming example
The pair was meant to contrast the closure parameter name (`entry`
vs. `x`), but using `.filter_map` on the good side and `.filter` on
the bad side muddled that with a method-choice difference. The
mismatch was carried over from the upstream parallel-disk-usage
guide. Use `.filter` on both sides so the example isolates the rule
under discussion.
* docs(style): fix four porting quirks in CODE_STYLE_GUIDE.md examples
Sweep for the same class of issue as the .filter / .filter_map
mismatch (#255, KSXGitHub review): examples that were degraded when
ported from upstream parallel-disk-usage.
- show_path with `path: &Path`: passing `my_path_buf` directly
doesn't compile because `PathBuf` does not auto-coerce to `&Path`
in argument position. Use `&my_path_buf` (same fix family as the
earlier push_path call sites).
- "Avoiding deeply nested function calls": the pre-pipe version was
a single non-nested call, so the rule's motivation wasn't visible.
Replaced with a genuinely nested constructor chain that piping
flattens.
- "pipe_as_ref" example lost its contrast in the port; restored the
"without pipe" companion line so the rule's payoff is shown.
- Picked a custom-looking pacquet function name (is_within_store)
for that example instead of `Path::exists`, since nobody uses
`path_buf.pipe_as_ref(Path::exists)` over `path_buf.exists()`.
- Renamed `rows.zip(cols)` to `left_indices.zip(right_indices)` so
the `(i, j)` closure parameters line up with the rule about index
variables.
* docs: address review-agent findings on the docs branch
- AGENTS.md: drop the stale "derives" topic from both descriptions
of CODE_STYLE_GUIDE.md, since the Derive Macro Ordering section
was removed in 06c08ff for not matching the codebase.
- AGENTS.md: replace the inaccurate "pre-commit checks" wording in
the repo-layout entry for CONTRIBUTING.md. The pre-push hook only
runs format checks; the full `just ready` suite is a manual step
before submitting, not an automated pre-commit hook.
- CODE_STYLE_GUIDE.md: fix subjunctive grammar — "should a test
fails" should be "should a test fail" (predates the recent move,
but caught while reviewing the section).
* Revert "docs(style): fix four porting quirks in CODE_STYLE_GUIDE.md examples"
This reverts commit 0d1e4348f0e8f66e552a7d8eb03db701c6fc0fcd.
* docs(style): make the pipe_as_ref example actually mid-chain
The text says "to pass a reference mid-chain", but the example was
a single `path_buf.pipe_as_ref(Path::exists)` call with no chain at
all. Replaced with a four-step chain that has methods before and
after `pipe_as_ref`, so the rule's wording matches the demonstration.
Audited every other pipe example in the section: each one is either
already part of a chain (dependencies, into_iter, summarize, etc.)
or is intentionally chain-free in the "When NOT to use pipe"
section, so no other rectification was needed.
Addresses KSXGitHub review on PR #255.
* docs(contributing): describe `just install` generically
Replace "Install the registry-mock dependencies" with "Install the
test dependencies". The previous wording named an internal component
(registry-mock) and would drift if `just install` ever did more or
different work. The new wording defers to `just install` as the
source of truth for what gets installed.
Per KSXGitHub review on PR #255.
* docs(style): drop `idx` from the index-naming alternatives
Per @zkochan review on PR #255: reduce the number of alternatives
listed for index variables. Keep `index` and `*_index` (such as
`row_index`); drop `idx`.
Applied to both places where the triple appeared: the "Conventional
single-letter names" bullet and the "Index variables" rule, so the
two stay consistent.
---------
Co-authored-by: Claude <noreply@anthropic.com>
|
||
|
Khải
|
4041a89468 |
ci(benchmark): skip benchmark workflows on PRs that can't affect timings (#295)
* ci(benchmark): skip benchmark workflows on PRs that can't affect timings Both Integrated-Benchmark and Micro-Benchmark used to run on every pull_request, even when the diff was docs-only or touched only the npm wrapper / other workflows. Add a `paths:` allow-list so the jobs trigger only on changes that can actually move benchmark numbers: Rust sources, Cargo manifests / lockfile, the toolchain pin, the shared CI actions, and the workflow file itself. Integrated-Benchmark additionally watches `justfile` (used by `just install` / `just integrated-benchmark`) and the registry-mock manifest files (which decide what verdaccio serves). Micro-Benchmark additionally watches its local `fixtures/` tarball. Resolves https://github.com/pnpm/pacquet/issues/293. * docs: remove unnecessary comments Co-authored-by: Khải <hvksmr1996@gmail.com> * ci(integrated-benchmark): trigger on fixture file changes The benchmark workload (package.json, pnpm-lock.yaml, pnpm-workspace.yaml under tasks/integrated-benchmark/src/fixtures/) is embedded into the binary via include_str!. Updating any of those files changes what the benchmark installs and therefore what it times, so add the directory to the paths allow-list. --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Zoltan Kochan
|
e7ce0b9155 |
perf: batch store-index prefetch + drive warm link phase on rayon (#292)
## Summary
Two structural changes that together cut warm-cache install wall time on a **1352-package lockfile** from ~10.85s → ~6.5s, beating pnpm v11.0.0-rc.5 (~8.5s on the same machine) by ~23%.
| | median wall |
|---|---|
| **this PR** | **6.52s** |
| pnpm v11.0.0-rc.5 | 8.51s |
| `main` baseline | 10.85s |
Measured on macOS (10-core M-class, APFS, store on same volume), warm cache, 8 runs each, `ulimit -n 65536`.
## Changes
### 1. Batched store-index prefetch (`crates/tarball/src/lib.rs::prefetch_cas_paths`)
Walks every `(integrity, pkg_id)` the lockfile mentions in one `spawn_blocking` task at install start, runs all integrity checks, and returns a `cache_key → cas_paths` HashMap. `DownloadTarballToStore` consults this map before falling through to the per-call `load_cached_cas_paths`.
This eliminates 1352 separate `spawn_blocking` round-trips a warm install was doing. Sample-profiling showed those each accumulating 20-60 ms of OS scheduling overhead while their actual work (≈40 µs SELECT + per-file integrity stats) is sub-ms — the queue across the default 512-thread blocking pool was dwarfing the work itself.
### 2. Warm batch via one global `rayon::par_iter` (`crates/package-manager/src/create_virtual_store.rs`)
Partition snapshots into "warm" (prefetched) and "cold" (cache miss). The warm batch bypasses tokio futures entirely:
```rust
warm.par_iter().try_for_each(|(snapshot_key, snapshot, cas_paths)| {
CreateVirtualDirBySnapshot { ... }.run().map_err(...)
})?;
```
The cold batch keeps the existing `try_join_all` + download path (rare on warm install, common on cold).
**Why this matters:** previously every per-snapshot tokio future called sync `rayon::join` from a tokio worker. With up to 10 such futures progressing concurrently and each one's inner `par_iter` saturating the rayon pool, the pool ended up processing **one snapshot at a time** despite tokio's worker count — `sum-of-link ≈ wall`, i.e. effectively 1× parallelism. Going straight to one big `par_iter` lets rayon schedule across all 1352 snapshots as one work-stealing graph, the shape pnpm's piscina pool gives implicitly.
### 3. Rayon pool sized at `availableParallelism * 2` (`crates/cli/src/lib.rs::configure_rayon_pool`)
The link phase is dominated by `clonefile` syscalls that block on the kernel's metadata journal, not CPU work. Oversubscribing CPUs gives more in-flight syscalls without losing wall time. Sweep on alot7:
| RAYON_NUM_THREADS | wall |
|---|---|
| 4 | 9.5s |
| 10 (default) | 9.4s |
| **20** | **9.25s** |
| 30 | 10.0s |
| 50 | 9.4s |
| 100 | 10.2s |
2x was the knee. Honours an explicit `RAYON_NUM_THREADS` env var.
|
||
|
Zoltan Kochan
|
8729c0c913 |
perf(sha2): enable asm feature for hardware SHA-512 (#282)
* perf(sha2): enable `asm` feature for hardware SHA-512 Ports item 5 of #280. pacquet's two SHA-512 call sites (`StoreDir::write_cas_file` for per-file CAFS digests and the per-tarball integrity check fed by `ssri::IntegrityChecker`) both go through the `sha2` crate. Without the `asm` feature, `sha2`'s module-picker at `sha512.rs:21` falls through to `mod soft` on every architecture — pure-Rust software SHA-512, no hardware path. Enabling the feature turns on: * x86_64 AVX / AVX-512 hand-written intrinsics (via `cpufeatures`- gated runtime detection of `SHA-NI` + related flags). * aarch64 NEON intrinsics using `sha512h`, `sha512h2`, `sha512su0`, `sha512su1` (ARMv8.2 FEAT_SHA512), runtime-gated on the `"sha3"` target-feature string which `cpufeatures` maps to the logical-OR of `hw.optional.armv8_2_sha512` and `hw.optional.armv8_2_sha3` on Apple Silicon and the `HWCAP_SHA3` bit on Linux aarch64. Both gates fall back to `soft::compress` at runtime when the feature isn't present, so this is transparent on targets that don't have hardware SHA-512. Measured on M3 with a 32 MiB single-shot benchmark (worst case for our use — our typical input is 100 KB – 5 MB, so the constant per-call overhead is a larger fraction of smaller hashes than this measurement suggests): * `default-features = false` (soft): 535 MiB/s * `features = ["asm"]` (hardware): 1355 MiB/s 2.5× on the hash itself. How much of that survives into install wall time depends on how much of the install is hashing vs network vs FS — #280 flagged SHA-512 as one hypothesis for the macOS pacquet-vs-pnpm gap since pnpm's hashing goes through Node's `crypto.hash` → OpenSSL → ARMv8 FEAT_SHA512 hardware, and pacquet was leaving that path unwired. This commit closes that specific gap; the wall-time impact will show in the CI bench and in a macOS profile. * chore(cargo): move sha2 caveat comment onto its own line Taplo's `column_width = 120` in `.taplo.toml` was breaking the sha2 line's inline-table across multiple lines once the trailing `#` comment pushed the full line past 120 chars, and that in turn cascaded into dropping the `align_entries = true` padding for the whole `[workspace.dependencies]` block — the CI format step on PR #282 was failing on the resulting diff. Moving the "0.11 removes the LowerHex impl on Output; revisit" caveat onto a preceding `#` line keeps the sha2 entry itself short enough that taplo leaves the inline-table alone, restoring the aligned format. Pure formatting; no semantic change. * fix(store-dir): gate sha2 `asm` feature off on MSVC CI Windows build on #282 failed at `cargo clippy` when the `asm` feature pulled in `sha2-asm`, whose build script compiles `src/sha512_x64.S` via `cc-rs`. MSVC's `cl.exe` doesn't parse GAS-syntax assembly, so the build logged: warning: sha2-asm@0.6.4: cl : Command line warning D9027 : source file 'src/sha512_x64.S' ignored ... LINK : fatal error LNK1181: cannot open input file '...\\sha512_x64.o' Move the `features = ["asm"]` activation from the workspace dep (unconditional, fails on MSVC) into a target-gated `[target.'cfg(not(target_env = "msvc"))'.dependencies]` block in the one crate that actually hashes (`pacquet-store-dir`). Cargo merges feature sets additively, so every non-MSVC target — Linux x86_64, macOS aarch64, etc. — still gets the hardware SHA-512 path, and MSVC Windows falls back to the pure-Rust `soft` implementation the crate defaults to. `windows-gnu` (mingw) would compile `sha2-asm` fine since the GNU toolchain understands `.S` syntax; the cfg gate is `target_env = "msvc"`, not `target_os = "windows"`, so that variant keeps the fast path if anyone builds it. |
||
|
Zoltan Kochan
|
00e1d3da2f |
perf(network,tarball): port pnpm v11 HTTP topology for tarball fetch (#281)
* perf(network,tarball): port pnpm v11 HTTP topology for tarball fetch Three HTTP/fetch-layer ports from pnpm v11, tracked in #280. All three are decisions pnpm landed upstream with explicit benchmark evidence, and all three are cases where a default `reqwest::Client` diverges from pnpm's chosen shape. 1. **HTTP/1.1 only** (`.http1_only()`). A default `reqwest::Client` negotiates HTTP/2 via ALPN whenever the registry advertises it, and registry.npmjs.org does. Pnpm v11's `network/fetch/src/dispatcher.ts:17-22` documents why they disabled H2: > With HTTP/2, undici multiplexes many streams over 1-2 TCP > connections sharing a single congestion window. In benchmarks > this was slower than opening ~50 independent HTTP/1.1 > connections that each get their own congestion window and can > saturate bandwidth in parallel. 2. **50 concurrent sockets**, matching pnpm's `DEFAULT_MAX_SOCKETS`. The previous `num_cpus.max(16)` semaphore under-subscribed on every machine benchmarked — on a 4-core GHA runner pacquet had 1/3 of pnpm's concurrent-fetch budget; even on a 10-core M3 the floor of 16 left bandwidth on the table. 3. **Pre-allocate the tarball buffer from `Content-Length`**. Ports `fetching/tarball-fetcher/src/remoteTarballFetcher.ts:148-164`. The old `response_head.bytes().await` let reqwest/hyper grow an internal `BytesMut` by doubling when CL wasn't used to pre-size — on a 1352-tarball cold install that's a lot of wasted realloc + copy per tarball. Switch to `bytes_stream()`, check `content_length()` on the response head, pre-allocate a `Vec<u8>::with_capacity(cl)` when known, and `extend_from_slice` each chunk in sequentially. Also catches size-mismatch via a new `TarballError::BadTarballSize` variant — pnpm surfaces the equivalent `BadTarballError` for the same case. An upstream bug or middleware truncation that produces fewer (or more) bytes than `Content-Length` now fails fast at the byte-accounting layer instead of silently succeeding until the ssri integrity check fails with a less-specific diagnostic. Requires adding `reqwest`'s `stream` feature (for `bytes_stream`) and pulling `futures-util` into the tarball crate (for `StreamExt::next`). Both are already in the workspace; this just scopes them to the tarball crate. Items 4 (post-download concurrency cap) and 5 (hardware SHA-512) from #280 are deliberately deferred: (4) wants Apple Silicon before/after numbers since the current cap was picked for a Linux CI failure, (5) wants a macOS profile to confirm SHA-512 is actually in the hot path before a crypto-crate swap. * style(cargo): reflow taplo-aligned columns Taplo recomputes column widths across the file when anything else in the block changes length. The 7ddb561 edit added `stream` to reqwest's feature list (longer line than the previous max in Cargo.toml) and `futures-util` to pacquet-tarball (a new longest name in that block), so taplo tightens the comment padding on `sha2` and widens the trailing padding on the surrounding deps. Ran `just fmt`; no logic changes. * fix(network,tarball): OOM-safe buffer alloc; rename constructor Three review fixes for #281: 1. **OOM protection on `Content-Length`.** Replaced the infallible `Vec::with_capacity(size as usize)` with a new helper, `allocate_tarball_buffer`, that guards the header as untrusted input. Two layers: * `usize::try_from(size)` — a `u64` CL may exceed `usize::MAX` on 32-bit targets. * `Vec::try_reserve_exact(capacity)` — refuses the allocation gracefully when memory pressure or an absurd claim would otherwise abort the process via the infallible path. Both failures surface as a new `TarballError::TarballTooLarge` variant so the install can reject the one offending package and continue instead of OOM-killing the whole process. 2. **Rename `ThrottledClient::new_from_cpu_count` → `new_for_installs`.** The previous commit dropped CPU-count sizing in favor of a fixed 50-socket cap matching pnpm's `DEFAULT_MAX_SOCKETS`, so the old name described a behavior the method no longer has. `new_for_installs` reads as "the default client tuned for pacquet install traffic", which is what it actually returns. Callers updated: `package-manager`'s `install_package_from_registry`, `cli::state`, and the `micro-benchmark` task. 3. **Dropped `BadTarballSize` variant and its checks.** Reviewer asked for a test exercising the variant; on closer look the checks can't actually fire behind reqwest/hyper, which enforces `Content-Length` framing on the receive side itself (a body shorter than CL errors the stream before our code sees a chunk sequence ending early; a body longer gets truncated or queued as the next request on a keep-alive connection). Pnpm's equivalent `BadTarballError` exists because undici in Node.js doesn't always enforce this. Keeping the variant as dead defense-in-depth on our side would just mean an unreachable branch and an untestable error path, so remove both. Adds three unit tests for `allocate_tarball_buffer` covering the chunked-transfer fallback, reasonable pre-sizing, and the `u64::MAX`-rejection case. The rejection test exercises both guards depending on target word size: 64-bit targets pass `try_from` and fail on `try_reserve_exact`; 32-bit targets fail on `try_from`. Either way the observable result is `TarballTooLarge`. |
||
|
Zoltan Kochan
|
3179c9a2f5 |
perf(fs): port pnpm v11 writeBufferToCafs into ensure_file (#279)
* perf(fs): drop exists() stat in ensure_file; use O_CREAT|O_EXCL
Ports pnpm v11's `writeFileExclusive` shape (`store/cafs/src/writeFile.ts:21-28`
and the `EEXIST` branch in `writeBufferToCafs.ts:57-68`) into pacquet's
CAFS writer. The old shape paid a `file_path.exists()` stat on every
call before the open; on a cold install where essentially every CAS
file is new the stat always returned `false`, so it was pure waste.
Replace it with `OpenOptions::create_new(true)` (`O_CREAT|O_EXCL` on
Unix, equivalent disposition on Windows) and map `ErrorKind::AlreadyExists`
to `Ok(())`. The warm-cache case (file already at its hash-derived
path) and the concurrent-writer race (another install process on the
same store got to this entry first) are both correct no-ops because
the path itself is the integrity assertion — contents at `files/XX/…`
are by definition the sha512 digest recorded in the filename, so a
present file has the bytes we were about to write.
pnpm still stats first because it runs `verifyFileIntegrity` on the
existing file to detect torn mid-write blobs from a crashed prior
install; pacquet doesn't re-verify individual CAS files on write (the
tarball-level ssri check has already passed for the batch these bytes
came from), so the stat has no job to do and can go. Torn-blob recovery
remains an open concern tracked in `investigations/pacquet-macos-perf.md`
§5 — the fix there is temp+rename, orthogonal to this syscall shape.
Also flips `ensure_file` from the pre-PR `OpenOptions::create(true)`
(which silently overwrites) to `create_new(true)` (refuses to
overwrite, surfaces as AlreadyExists → Ok). A concurrent writer now
can't silently clobber our partially-written bytes, which is a
strictly better safety property even though the observable behavior
doesn't change on success paths.
Adds four targeted tests pinning:
- new-file write puts bytes on disk (with mode on Unix);
- pre-existing file is preserved byte-for-byte (not overwritten);
- missing parent dir surfaces as CreateFile/NotFound so callers
can't accidentally bypass the `ensure_parent_dir` contract.
* perf(fs): verify & self-heal pre-existing CAS files in ensure_file
First pass of this PR skipped integrity verification on the
`AlreadyExists` branch on the theory that the hash-derived CAS path
itself is enough of an assertion. That misses torn-blob recovery —
a prior install that crashed mid-write leaves a file at the right
path with wrong bytes, and a new install would happily skip over it,
silently serving corrupt bytes downstream.
Port pnpm v11's `writeBufferToCafs` full shape instead:
1. `O_CREAT|O_EXCL` open (unchanged from first pass) — success means
we wrote the bytes ourselves.
2. `AlreadyExists` → re-read the file and byte-compare with `content`.
CAS paths are hash-derived, so matching bytes imply matching digest;
this is pnpm's `verifyFileIntegrity(fileDest, integrity)` check,
cheaper because we already hold the expected bytes and can skip
the re-hash.
3. Match → `Ok(())`. The file is a live CAS entry.
4. Mismatch (or `NotFound` on re-read from a concurrent cleaner) →
`write_atomic`: open a unique temp path next to the target, write
`content`, `fs::rename` over the target. Rename is atomic on Unix
(`rename(2)`) and replaces-in-place on Windows, so observers
never see a half-written blob. Matches pnpm's `writeFileAtomic` +
`renameOverwriteSync`.
Temp path scheme mirrors pnpm's `pathTemp`:
`{basename with -suffix stripped}{pid}{counter}`, where counter is a
process-local monotonic `AtomicU64` instead of pnpm's Node.js
`workerThreads.threadId`. Strips `-exec` → appends `x` the same way
pnpm's `removeSuffix` does, so temp files don't look like executable
CAS entries to an observer scanning the shard.
Deliberate divergences from pnpm v11 documented inline:
- No upfront stat. `create_new`/`AlreadyExists` gives us the
exists-signal for free; stat would be redundant.
- Byte-compare instead of `crypto.hash`. We hold the buffer, so
equality gives the same correctness guarantee without a second
full-buffer walk.
- No `locker: Map<string, number>` process-local cache. Pacquet's
hot path calls `ensure_file` at most once per CAS file per install
(the `StoreIndex` decides whether we even get here), so the locker
would be mostly empty work.
Adds three tests covering the new paths — matching-content
short-circuit, mismatched-content atomic rewrite, temp-name scheme
preserves pnpm's `-exec` → `x` suffix transformation.
* perf(fs): retry rename on Windows AV transient errors
Port the `EPERM|EACCES|EBUSY` arm of pnpm's `rename-overwrite`
library (zkochan/packages/rename-overwrite/index.js) into the
torn-blob-recovery `rename` call inside `ensure_file`. That arm is
the one retry family that actually hits pacquet in practice:
Windows Defender and other AV / file-indexer tools briefly hold the
destination open when we try to rename our temp file over it, which
fails with `ERROR_ACCESS_DENIED` or `ERROR_SHARING_VIOLATION`. Both
surface through Rust's `io::ErrorKind` as `PermissionDenied` or
`ResourceBusy`, and both clear on their own within tens to hundreds
of milliseconds.
Loop shape matches pnpm exactly: 60-second total budget, start at
zero sleep, grow the per-attempt sleep by 10 ms each iteration,
cap at 100 ms. Any non-transient error kind propagates immediately.
Other retry arms in `rename-overwrite` (`ENOTEMPTY`/`EEXIST`/
`ENOTDIR` swap-rename, `ENOENT` mkdir-and-recurse, `EXDEV` copy-
and-delete) are deliberately **not** ported — see the doc comment
on `rename_with_retry` for the case-by-case reasoning. Short
version: for this specific call site (temp and target in the same
CAS shard dir, both files, no live readers), those arms have no
work to do.
Adds two tests pinning the behavior:
- `transient_rename_error_classifier` rejects any `ErrorKind` other
than `PermissionDenied` / `ResourceBusy`. A regression that
classified e.g. `NotFound` as transient would pathologically spin
for 60 seconds on a legitimately missing source.
- `rename_with_retry_succeeds_fast_when_no_error` pins that the
happy path returns immediately without sleeping — the first
attempt's `Ok(())` short-circuits, which matters since the retry
loop starts with a 10 ms step.
* fix(fs): reject non-regular dirents in ensure_file; fix flaky mode test
Three review fixes for `ensure_file`:
1. **Symlink scrub on AlreadyExists.** `open(O_CREAT|O_EXCL)` returns
EEXIST on Unix even when the dirent is a symlink, so a tampered
or backup-restored store could route a symlinked dirent into this
function. The old `AlreadyExists` branch called `fs::read` (which
follows symlinks) and byte-compared — a symlink pointing at a
file with matching bytes would return Ok and leak into the CAS,
where downstream `fs::hard_link` (which does *not* follow links
on Linux) would hardlink the symlink itself rather than the
target. Add a `fs::symlink_metadata` guard: anything that isn't a
regular file (symlink, dir, fifo, socket, device) goes straight
to `write_atomic`, whose `rename` replaces it with a real file.
Two new tests pin the live-symlink and dangling-symlink paths.
2. **Stale doc comment on `writes_a_new_file`.** Said "with the
requested mode" but the call passed `None`. Mode coverage lives
in `unix_mode_is_applied_on_new_files`, so just drop the stray
mode claim.
3. **Umask-flaky mode assertion.** `OpenOptionsExt::mode` runs the
requested bits through the process umask; a restrictive shell
(e.g. `umask 0o077`) would strip the group/other bits of `0o755`
and fail `assert_eq!(mode, 0o755)`. Mask to `0o700` before
comparing so we pin only the owner bits, which every sensible
umask preserves. The observable property we care about (the
owner-exec bit surviving for executable CAS blobs) is still
covered.
* fix(fs): stream byte-compare, O_EXCL temp, Windows-only rename retry
Five review fixes for `ensure_file` hardening:
1. **Streaming byte-compare.** Old `AlreadyExists` branch called
`fs::read` and did a `Vec<u8>` equality check, doubling peak
memory for every CAS entry it verified. For a 30 MB tarball entry
that was 30 MB extra heap per concurrent worker. Replaced with
`file_equals_bytes` — open, read 8 KB stack-buffer chunks,
compare each against the corresponding slice of `content`, bail
on first mismatch. Fixed-size stack buffer regardless of file
size. Added a length short-circuit (`meta.len() != content.len()`)
before we open the file at all.
2. **`O_EXCL` on the atomic-write temp path.** Old `write_atomic`
opened with `create+truncate`, which follows symlinks and
truncates whatever's there. An attacker (or a stale temp from a
crashed prior install) pre-seeding the predicted temp path could
cause us to truncate a file outside the store shard. Switch to
`create_new(true)`; on `AlreadyExists` advance the pid + counter
suffix and retry, up to 16 fresh names. Collisions are already
vanishingly rare given the pid + per-process `AtomicU64` scheme,
but the retry closes the adversarial / shared-store loophole
without panicking.
3. **Windows-only rename-retry classifier.** Old
`is_transient_rename_error` treated `PermissionDenied` and
`ResourceBusy` as transient on every platform. On Unix, `rename`
returning those codes is essentially always a permanent
permission or mount-point issue, so the 60 s retry loop just
stretches out the failure. Gate the classifier on
`cfg!(windows)`; non-Windows returns `false` and propagates
immediately. Doc comment and the classifier test updated to
reflect the new contract.
4. **Split open / write error classification in `write_atomic`.**
Previous shape wrapped both in a single `WriteFile` variant,
making "couldn't create the temp file" and "couldn't write to it"
indistinguishable. Separate the two call sites: open errors map
to `CreateFile`, write errors stay as `WriteFile`.
5. **Drop flaky 10 ms timing assertion in
`rename_with_retry_succeeds_when_no_error`.** On loaded CI /
slow filesystems a successful first-attempt rename can exceed
10 ms of scheduling jitter even without the retry path firing.
Correctness assertions only — the classifier test already pins
the semantic property we cared about.
Also file handle is now explicitly `drop`ped before `rename` to keep
the sharing-violation window as small as possible on Windows.
Two new tests cover the streaming compare:
- `file_equals_bytes_classifies_match_mismatch_and_length_mismatch`
pins the exact-match / content-diff / length-diff branches.
- `file_equals_bytes_handles_multi_chunk_files` uses a 20 KB payload
to exercise the inner `read_exact` loop across multiple chunks
and plants a last-chunk mismatch to catch "first chunk matched, so
I returned true" regressions.
* docs(store-dir): refresh shard-cache test comments for verify_or_rewrite
The `shard_cache_populates_on_first_write_and_skips_mkdir_thereafter`
test comments still described the pre-review version of `ensure_file`,
which swallowed `AlreadyExists` as `Ok(())` without verifying the
existing bytes. Since b027412 (symlink-safety guard) and 1835847
(streaming byte-compare), the `AlreadyExists` branch routes through
`verify_or_rewrite` and returns `Ok(())` only after byte-matching the
existing file against the buffer; a mismatch takes the
`write_atomic` torn-blob-recovery path instead.
Update the doc block and the inline comment in the test to describe
this actual flow, and point to the cross-crate test that pins the
mismatch branch so a reader can follow the whole contract.
|
||
|
Zoltan Kochan
|
21e1bb7eea |
perf(package-manager): parallelise symlink layout with tarball fetch (#277)
* perf(package-manager): parallelise symlink layout with tarball fetch `CreateVirtualStore::run` used to do per-snapshot `download → mkdir → import-CAS → intra-pkg-symlinks` serially inside each tokio task. The symlink step doesn't need the tarball contents — only the resolved dep graph, which is already in the lockfile — so holding it behind the fetch meant cold installs couldn't start laying out `.pacquet/` until tarballs landed. Fork the two phases into a `tokio::try_join!`: one branch mkdirs every `.pacquet/pkg@ver/node_modules/` and creates the intra-package symlinks straight from `snapshots`, the other downloads + extracts + imports the CAS files. Matches pnpm's `headless` installer pattern (`Promise.all([linkAllModules(...), linkAllPkgs(...)])` in `pkg-manager/core/src/install/link.ts`), which is why pnpm has `node_modules` shape visible on disk seconds before the downloads finish. Drops the `snapshot` field from `InstallPackageBySnapshot` (now only fetch + import) and shrinks `CreateVirtualDirBySnapshot` to the CAS import step. `link_file` already creates parent dirs for fetches, so no explicit ordering between the two branches is required. * perf(package-manager): wrap sync symlink body in block_in_place `symlink_fut`'s per-snapshot future is entirely synchronous (mkdir + rayon-parallel `create_symlink_layout`) with no `.await` points. `futures::try_join_all` polls its children on the same task, so without a blocking hint each child ran to completion on the worker thread before the runtime got a chance to poll `fetch_fut` — coupling download progress to symlink wall-time on the multi-threaded runtime and undoing the parallelism the previous commit was aiming for. Wrap the body in `tokio::task::block_in_place` so the executor can shift `fetch_fut` and other tasks onto sibling workers while this thread blocks. Pnpm's JS equivalent gets the same effect for free via `@pnpm/worker`, which offloads symlink work to a worker thread pool. The existing `#[tokio::test]` that reaches `CreateVirtualStore::run` uses an empty snapshot set, so the map closure is never invoked and `block_in_place` is never called from current-thread tokio — no panic. * refactor(package-manager): run symlink pass in one spawn_blocking Replace the per-snapshot `block_in_place` pattern from the previous commit with a single `tokio::task::spawn_blocking` that owns the symlink work and distributes it across rayon's pool via a flat `par_iter`. Three wins: 1. No runtime-flavor fragility. `block_in_place` panics on current- thread tokio; `spawn_blocking` works everywhere. Nothing in the codebase relies on this today, but the previous shape was a trap for a future test that adds non-empty snapshots under `#[tokio::test]` without `flavor = "multi_thread"`. 2. Clean error propagation. `create_symlink_layout` now returns `Result<(), SymlinkPackageError>` (serial `try_for_each` instead of `par_iter().for_each` with an `.expect`), and the error threads up through a new `CreateVirtualStoreError::SymlinkPackage` variant. A filesystem failure during the symlink sweep now surfaces as a structured miette diagnostic instead of crashing the process mid-install. 3. Better rayon work queue. One `par_iter` over the full snapshot vector lets rayon schedule 1352 items across its workers in one pass, instead of 1352 ramp-up/ramp-down cycles of the previous nested `par_iter`-per-snapshot pattern. `snapshot.dependencies` (a small `HashMap<PkgName, SnapshotDepRef>`) is cloned once per snapshot so the task owns its data. On the 1352-package fixture this is a few tens of ms of one-off work vs. the per-iteration savings and the robustness wins; big-lockfile integration test runs in 47–50s either way. Also corrects the stale doc on `CreateVirtualDirBySnapshot` — after the split, the virtual-store package directory may or may not exist when CAS import runs, and `link_file` creates missing parents on demand. * fix(package-manager): always await symlink task before returning `tokio::try_join!` drops the losing future on the first error. With the symlink branch wrapped in a `spawn_blocking` `JoinHandle`, dropping that future dropped the handle too — and tokio cannot abort a blocking task. The closure kept running on the blocking pool, mutating `.pacquet/*/node_modules/` in the background after `CreateVirtualStore::run` had already returned the fetch error. Stop using `try_join!` on the symlink branch. Launch the blocking task, race it against the fetch `try_join_all` via a plain `.await`, then explicitly `symlink_handle.await` on every exit path. The symlink work runs concurrently with fetches exactly as before — it's on the blocking pool from the moment `spawn_blocking` returns — but now it is guaranteed to be finished before `CreateVirtualStore::run` returns, even on fetch error. Fetch errors surface first when both branches fail; pure symlink failures still surface on success of the fetch branch. No behavioral change on the success path. Big-lockfile integration still runs in ~46 s. * docs(package-manager): refresh stale try_join! reference in comment The `StoreIndexWriter` rationale block still described the old flow where the symlink and fetch branches were joined via `tokio::try_join!`. That macro was removed in 9cf5c09 when the symlink branch was split into a `spawn_blocking` handle awaited separately after the fetch `try_join_all`. Update the comment to describe the actual sequencing. * perf(package-manager): run symlink pass serially on one thread The previous version fanned the symlink pass out over rayon via `par_iter` inside a `spawn_blocking`. On cold-cache installs this stole rayon workers from the fetch branch's concurrent `create_cas_files` calls during the hot part of the install and showed up as ~4% regression on the 1352-package FrozenLockfile benchmark relative to main. Pnpm's JS equivalent already solved this: `symlinkAllModules` (`worker/src/start.ts:493-501`) runs as a plain nested `for` loop on a single `@pnpm/worker` thread — no internal parallelism, no contention with the main-thread `linkAllPkgs` work. Port that pattern: drop the outer `par_iter`, iterate the snapshot vec serially in the `spawn_blocking` closure. On Linux with num_cpus workers, the cold install has ~3 s of network and tarball-extract work; ~6-7k symlink syscalls serial on one CPU costs ~200-300 ms, which sits comfortably inside the fetch budget and frees the whole rayon pool for CAS imports. No change to the symlink/fetch concurrency contract — `spawn_blocking` still runs on its own thread concurrently with the fetch `try_join_all`. * docs(package-manager): correct create_symlink_layout doc comments Two stale bits called out in review: 1. The "virtual_node_modules_dir must already exist" note was misleading — `symlink_package` calls `fs::create_dir_all` on the symlink path's parent before each link, so the function works regardless. Callers that mkdir beforehand (as `CreateVirtualStore::run` does) just pay redundant stat syscalls. 2. The serial-iteration rationale referenced a "caller parallelises across snapshots" expectation that stopped being true in 131e89c, when the caller switched to a serial `for` loop inside `spawn_blocking`. Reword to reflect the single-threaded policy. * refactor(package-manager): overlap CAS import with symlinks per-snapshot Previous commits on this branch hoisted the whole symlink pass into a separate `spawn_blocking` task that ran concurrently with the fetch `try_join_all`. On the 1352-package cold-install benchmark that design shipped a ~3% regression vs. main, and I couldn't find a safe way to eliminate the root cause (an upfront `snapshot.dependencies.clone()` loop on the reactor thread that required ~50 k heap allocations before fetches could start). Go back to baseline's per-snapshot tokio-task shape and add the parallelism one level deeper instead: inside each snapshot's post- extract step, `CreateVirtualDirBySnapshot::run` now uses `rayon::join` to run `create_cas_files` and `create_symlink_layout` concurrently. Neither closure needs data the other produces — CAS import reads `cas_paths`, symlink layout reads `snapshot.dependencies` — so overlapping them saves the serial symlink time per snapshot (~1-3 ms on a typical package) without any cross-thread data marshaling. Both closures borrow from the same stack frame. Cross-snapshot concurrency keeps streaming through tokio's `try_join_all`, same as main. Pnpm's `deps-restorer` installer is structured along the same axis once you stop conflating its "symlink all / fetch all" worker-thread plumbing with wall-time parallelism — the real overlap that matters is symlink-with-import, not symlink-with-download. Keeps the error-propagation cleanup from earlier commits on this branch: `create_symlink_layout` still returns `Result`, and `CreateVirtualDirBySnapshot` grows a `SymlinkPackage` variant so a filesystem failure during the symlink sweep no longer panics the process mid-install. |
||
|
Zoltan Kochan
|
30ca1e25f4 |
perf(store-dir): pre-create CAFS shards at install bootstrap (#276)
Port pnpm's eager shard pre-creation plus its lazy per-directory cache into pacquet's CAFS write path so per-file `write_cas_file` calls don't pay `create_dir_all` (and its backing `stat`) on every entry of every tarball. |
||
|
Zoltan Kochan
|
99a790100c |
perf(store-dir): port pnpm's checkPkgFilesIntegrity model for warm-cache lookup (#271)
* perf(store-dir): port pnpm's checkPkgFilesIntegrity model for the warm-cache lookup Ports pnpm v11's [`store/cafs/src/checkPkgFilesIntegrity.ts`]( |
||
|
Zoltan Kochan
|
fcdfcff331 | chore: add build script to package.json (#275) | ||
|
Zoltan Kochan
|
a9018643a0 |
refactor(tarball): extract per-entry loop; propagate tar errors (#272)
* refactor(tarball): extract per-entry loop; propagate tar errors Closes #270. The per-entry loop inside `DownloadTarballToStore::run_without_mem_cache` was a pile of `.unwrap()` / `.expect()` calls that turned any tar-side failure — corrupt header, short body read, path decode, entries-iterator error — into a panic on a blocking-pool thread. A malformed or truncated tarball from the registry should surface as a `TarballError`, not unwind the install. Move the loop into a free-standing `extract_tarball_entries` function (also addresses the pre-existing `TODO: move tarball extraction to its own function`) and rewrite every `.unwrap()` / `.expect()` inside it to propagate via `TarballError::ReadTarballEntries`: * `.filter(|entry| !entry.as_ref().unwrap()...)` → match Ok/Err; Err entries fall through so the `?` inside the loop propagates them. * `entry.unwrap()` → `entry.map_err(TarballError::ReadTarballEntries)?`. * `entry.header().mode().expect("get mode")` → same propagation pattern. * `entry.read_to_end(...).unwrap()` → same. * `entry.path().unwrap()` → same. * `.into_string().expect("entry path must be valid UTF-8")` → `.to_string_lossy().into_owned()`. Lossy coercion matches pnpm's string-based path handling (Node.js's string layer does the same), so a mixed install against the shared `index.db` stays consistent. Add a unit test that feeds 1 KiB of 0xFF (not a valid tar archive — header checksum at bytes 148..156 can't possibly validate) and asserts `extract_tarball_entries` returns `TarballError::ReadTarballEntries` rather than panicking. That's the acceptance case called out on #270. * fix(tarball): cap entry pre-alloc; use fully-qualified doc link Two Copilot review nits on #272: * `Vec::with_capacity(entry.size() as usize)` trusted the tar-header size — a corrupt or malicious tarball claiming gigabytes could turn that into an OOM abort before `read_to_end` had a chance to surface the real error. Clamp the pre-allocation hint to 64 MiB (generous for any legitimate single-file entry in an npm tarball) and use `try_reserve` so an allocation failure comes back as an I/O error instead of aborting. Content beyond the clamp is still read via `Vec`'s geometric growth. * `[Path::to_string_lossy]` in the doc comment didn't have `Path` in scope (only `PathBuf` is imported), so `cargo doc -D warnings` could reject it. Use the fully qualified `[std::path::Path::to_string_lossy]`. * fix(tarball): reject directory-traversal paths; tighten filter to is_file Two Copilot review nits on #272: * `cleaned_entry_path` derives directly from tar entry path components and gets joined onto the CAFS extraction root by `create_cas_files` later. Without validation a hostile tarball could carry `..`, absolute roots, or Windows prefix components that land files outside the store. Validate every component (after dropping the top-level package directory) is `Component::Normal` — reject loudly as `TarballError::ReadTarballEntries(InvalidData)` instead of normalizing silently. Also reject the empty-cleaned-path case. Added a unit test that bypasses `tar::Header::set_path`'s own `..` validation via `as_mut_bytes()` + checksum recompute, the same shape a non-Rust tool could trivially produce. * The per-entry filter was `!entry_type().is_dir()` which kept symlinks, hardlinks, PAX / GNU extension headers, and device files — the doc comment claimed "regular-file entry", which didn't match. Tighten the filter to `entry_type().is_file()`. Silently reading a symlink's 0-byte body into the CAFS as if it were a file would just corrupt the store; npm-publish tarballs only carry regular files in practice. Updated the docstring to call out the filter explicitly. |
||
|
Zoltan Kochan
|
348795fc7a |
fix(tarball): run post-download pipeline on blocking pool (#269)
* fix(tarball): run post-download pipeline on blocking pool
The `frozen-lockfile` benchmark job has been flaky on CI with the 10-minute
step timeout firing during `Benchmark 1: pacquet@HEAD`. The logs show all
downloads completing from the network's point of view but ~1115 tarballs
stuck between "Download completed" and "Checksum verified", then ~3m46s
of total tracing silence before the runner kills the process.
Cause: the post-download work (SHA-512 of the full tarball, gzip inflate,
per-file SHA-512, `write_cas_file`, SQLite index INSERT) ran inside
`tokio::task::spawn(async move { ... })`. The body contains no `.await`
until the final nested `spawn_blocking` for the SQLite write, so each
tarball pins a tokio reactor worker for its whole duration. On a 2-core
GHA runner that caps throughput at ~2 tarballs in flight, and the tail
of large packages (100-200 KB compressed, MB+ inflated, dozens of files
each) overruns the CI step budget.
Swap the outer `spawn` to `spawn_blocking` so the work runs on tokio's
blocking pool (default 512 threads) instead. The nested `spawn_blocking`
around the SQLite write is no longer useful once we're already on the
blocking pool, so inline it.
* style(tarball): apply rustfmt
* fix(tarball): cap post-download concurrency; propagate JoinError
Two changes addressing Copilot review comments on #269:
* Gate the post-download `spawn_blocking` body with a global
`Semaphore(num_cpus * 2, floor 4)`. The body is CPU-bound (SHA-512
over the whole tarball + per-file SHA-512 + gzip inflate) with some
blocking FS I/O, and the default 512-thread blocking pool let
`try_join_all` fan out hundreds of these at once — good for I/O
wait, disastrous for CPU work on a 2-core runner. The permit is
held across the `spawn_blocking.await` so no task enters the body
until one is free.
* Replace `.expect(\"tarball-processing blocking task panicked\")` on
the spawn_blocking `JoinHandle` with
`.map_err(TarballError::TaskJoin)?`. Runtime cancellation or a
blocking-task panic now surfaces as an error rather than unwinding
the whole install. `TarballError::TaskJoin` already exists for this.
* fix(tarball): acquire post-download permit before buffering body
Addresses Copilot review on #269: `post_download_semaphore().acquire()`
used to run *after* `.bytes().await`, so under `try_join_all` fan-out a
fast registry could leave hundreds of fully-buffered tarball bodies
sitting in RAM waiting for a permit to process — a significant peak-RSS
risk on large installs.
Split the HTTP call into `send()` (headers) and `bytes()` (body) and
acquire the permit between them. The permit now covers both buffering
and processing, so the number of concurrently-buffered bodies is
bounded by the post-download cap (`num_cpus * 2`, floor 4). HTTP
headers-only concurrency is still gated separately by `ThrottledClient`
so the cheap head phase can still fan out.
|
||
|
Zoltan Kochan
|
e19163c07d |
refactor(store-dir): batch store-index writes via a single writer task (#265)
* refactor(store-dir): batch store-index writes via a single writer task Port pnpm v11's `queueWrites` / `flush` / `setRawMany` pattern from `store/index/src/index.ts` so the per-tarball store-index INSERT stops opening a fresh SQLite connection and pulling its own blocking-pool thread. * Add `StoreIndex::set_many`, which wraps a batch in `BEGIN IMMEDIATE; INSERT OR REPLACE x N; COMMIT;`. One WAL fsync amortizes across the batch, with a rollback on per-batch error so a partial apply never leaves the index in a half-written state. Encoding happens on the caller's thread before the transaction begins, so the writer only pays SQLite work. * Introduce `StoreIndexWriter`, an `Arc`-cloneable handle around a tokio unbounded channel. Producers call `queue(key, value)` — no SQLite, no `spawn_blocking`, no per-row lock. A single blocking task drains the channel, collects each non-blocking burst into a batch (cap 256), and flushes via `set_many`. * `CreateVirtualStore` and `InstallWithoutLockfile` spawn the writer at the top of the run, thread the `Arc<StoreIndexWriter>` through `InstallPackageBySnapshot` / `InstallPackageFromRegistry` / `DownloadTarballToStore`, drop the orchestration's clone after `try_join_all`, then `await` the writer task so the final batch flushes before the install returns. * `DownloadTarballToStore::run_without_mem_cache` drops its per-tarball `spawn_blocking` for the index write entirely. Batch errors, `JoinError`s, and writer-open failures are all downgraded to `warn!` and "skip the row", matching the read side's graceful-degradation stance from #261. Tests pass against the existing `pacquet-registry-mock` harness. Context: opened while investigating #263. On macOS I haven't been able to demonstrate a wall-time win from this change yet — the measurable bottleneck in pacquet's cold-install sys time sits elsewhere (most likely tarball extraction / `write_cas_file` and sha512 per file), see the comment thread on #263 for the investigation notes and sample profile. The change still stands as structural cleanup and the pnpm-model alignment, and it will compound with whatever fix the real bottleneck gets. * refactor(store-dir): skip per-row encode failures in `set_many` `collect::<Result<_, _>>()?` turned a single `encode_package_files_index` failure into "drop the whole batch" — up to 256 otherwise-valid rows lost because of one malformed `PackageFilesIndex`. That's stricter than the "best-effort index" contract the writer task and the read-side already follow. Encode in a loop, log the failing key at `warn!`, and keep the rest of the batch alive for commit. SQLite errors inside the transaction still roll back the whole batch (that's how atomicity is supposed to work); only the per-row encoding step is skipped. Addresses Copilot review on #265. * refactor(store-dir,tarball): reword #263 ref; quiet `queue` warn on first failure; fix private-doc-link Three follow-up nits from the Copilot review on #265: * The comment in `tarball/src/lib.rs` described #263 as "fixed" — it isn't, this PR is one step toward fixing it. Reworded to "the per-write blocking thread churn described in #263" so the citation survives the landing of both. * `StoreIndexWriter::queue` logged a `warn!` on every `send` failure. When the writer task exits early (open failure, panic) every subsequent `queue` call fails — on a 1352-snapshot install that's a thousand identical warnings drowning everything else out. Add a `warn_on_send_failure: AtomicBool` one-shot guard: first failure logs (with "further failures silenced" hint), subsequent failures go silent. The observable signal the user actually cares about — "this install's rows aren't in the index, the next install will re-download" — surfaces on the next run regardless. * `StoreIndexWriter`'s doc block linked to the private constant `MAX_BATCH_SIZE`. `RUSTDOCFLAGS=-D warnings` in CI rejects the private-intra-doc-link lint, so replace the link with a literal "256 entries" plus a `see MAX_BATCH_SIZE` pointer in prose. Addresses Copilot review on #265. |
||
|
Zoltan Kochan
|
f33481ec83 |
docs: add AGENTS.md with upstream-parity guidance (#268)
* docs: add AGENTS.md with upstream-parity guidance Documents the cardinal rule (mirror pnpm/pnpm main), build/test/lint commands, test targeting, code-reuse expectations, and Conventional Commits conventions for AI coding agents working in this repo. * docs: symlink CLAUDE.md to AGENTS.md So Claude Code picks up the same guidance automatically without duplicating the content. * docs(agents): fix fixture path and relax "just only" guidance - Shared test fixtures live under crates/testing-utils/src/fixtures/, not under __fixtures__ directories (that's a pnpm-ism). - Reword the commands section to allow dropping to cargo/taplo directly when the just recipe doesn't expose the flag you need, matching the later narrow-test guidance that uses `cargo nextest -p <crate>`. |
||
|
Zoltan Kochan
|
4fa08d5495 |
bench(integrated-benchmark): level cold-install comparison on macOS and across platforms (#264)
* bench(integrated-benchmark): pin storeDir in pnpm-workspace.yaml The fixture's `.npmrc` set `store-dir=<bench-dir>/store-dir`, but pacquet reads `.npmrc` for auth/registry only — project-structural settings like `storeDir` must come from `pnpm-workspace.yaml` (see `crates/npmrc/src/lib.rs:173-178`, matching pnpm 11). With no `storeDir` in the yaml the benchmark silently fell through to pacquet's default (`~/Library/pnpm/store` on macOS, `~/.local/share/pnpm/store` on Linux), so `hyperfine --prepare`'s `rm -rf <bench-dir>/store-dir` never actually wiped the store the install read from. On a developer machine whose default store already holds the fixture's 1352 snapshots — any prior `pnpm install` of a similar project — every bench iteration hit the warm-store cache-lookup path instead of the cold-install path. The measured wall time was dominated by the per-file `symlink_metadata` pass of #260, not by the fetch+extract+write pipeline the benchmark was designed to stress. Adding `storeDir: ./store-dir` makes the bench honour the prepared per-revision store. On CI this is a no-op (the default `~/.local/share/pnpm/store` is empty anyway on a fresh runner) — the fix prevents developer-local runs from producing meaningless numbers. * bench(integrated-benchmark): silence pnpm fsevents build warning on macOS `fsevents` is a macOS-only optional dependency of `chokidar` (via several fixture entries). On Linux CI it isn't installed and pnpm never complains; on a developer macOS machine it installs and pnpm emits `ERR_PNPM_IGNORED_BUILDS: fsevents@1.2.13` because the fixture's `.npmrc` sets `ignore-scripts=true`. That error exits pnpm with status 1, which hyperfine flags via the benchmark harness's exit-code check and takes the whole `just integrated-benchmark` run down on the first warmup. Adding `fsevents: false` to `allowBuilds` silences the warning specifically for this package without re-enabling script execution elsewhere. Matches what pnpm emits when you run `pnpm approve-builds` and decline fsevents. * bench(integrated-benchmark): force pnpm to install cross-platform optionals Pacquet doesn't filter optional dependencies by `os` / `cpu` / `libc` at the moment — `create_virtual_store` iterates every snapshot in the lockfile unconditionally, so `fsevents` (and every other platform-specific optional) ends up in the install payload regardless of runner. pnpm does filter, so on Linux CI it skips `fsevents` and friends entirely. That's a real payload difference: pnpm's install does strictly less tarball fetching / extraction / CAS writing than pacquet's on the same lockfile. Small in absolute terms for this fixture, but a hidden handicap in a benchmark whose whole point is to measure the fetch+extract+write pipeline. Set `supportedArchitectures` to cover every OS / CPU / libc combo we care about so pnpm pulls cross-platform optionals on every runner, matching pacquet's payload. The fix belongs on the pnpm side of the fixture because pacquet's filter-by-platform is a future code change with larger blast radius; bringing the two tools to the same install set in one-line config here is the lower-risk intermediate. |
||
|
Zoltan Kochan
|
968bfc7270 |
fix(network): set explicit timeouts on default reqwest client; enable CI tracing (#267)
* fix(network): set explicit timeouts on the default reqwest client `ThrottledClient::new_from_cpu_count` handed back `Client::new()`, which has no connect / request / pool-idle timeouts at all. The existing docstring on the sibling `from_client` already called this out as a footgun — that note stays relevant for tests, but the production path can't afford "wait forever". On CI this is how `integrated-benchmark` hangs at "Benchmark 1: pacquet@HEAD" until the 10-min GHA step cap kicks in: the bench's single-process verdaccio occasionally stalls (GC pause, uplink hiccup, TCP loss without RST) and pacquet sits on the half-open socket indefinitely, emitting nothing even under `--show-output` (see #263 investigation notes). Set explicit defaults: * `connect_timeout(10s)` — generous for localhost, snaps on resource-exhausted runners. * `timeout(60s)` — per-request deadline. Typical npm tarball is under 5 MB, so 60 s on localhost is comfortable; a real hang turns into `TarballError::FetchTarball` and hyperfine reports the non-zero exit instead of sitting silent. * `pool_idle_timeout(30s)` — evict conns verdaccio closed but we didn't notice. Retries on transient failures are tracked separately in #259. Also enable pacquet tracing on the CI bench step via `TRACE=pacquet=info`, so the next time one of these stalls makes it through despite the new timeouts the step log shows which phase pacquet was in (download, extraction, import, index write) before it went silent. `--show-output` was already in place; without a tracing filter it only ever captured an empty stream. Refs #263. * fix(network): use `Self::new_from_cpu_count` in intra-doc link `[`new_from_cpu_count`]` doesn't resolve from inside the `impl` block because rustdoc needs a path; `Self::new_from_cpu_count` does. The Doc CI job runs with `RUSTDOCFLAGS=-D warnings` so the broken link was a hard error. No behavioural change. * fix(network): bump default request timeout from 60s to 5 min 60 seconds is too aggressive for a default that governs real registry traffic, not just the bench's localhost verdaccio. Large npm tarballs can legitimately take more than a minute over a slow network, and pacquet has no retry-with-backoff yet (#259) — a 60-second timeout would turn slow-but-progressing downloads into outright install failures. 5 minutes keeps the \"catch truly stuck sockets\" property (still well inside the bench's step budget and the CLI's reasonable-user patience) while comfortably accommodating slow real-world connections. Also reword the doc to call out that the value is the default for registry traffic too, not just the bench, and that making it user-configurable is the natural next step once retries exist. Addresses Copilot review on #267. |
||
|
Alessio Attilio
|
1819226b51 | fix: ignore uuid vulnerability (GHSA-w5hq-g745-h8pq) (#11344) | ||
|
Zoltan Kochan
|
890efaf472 |
chore: use @zkochan/git-wt package for worktree creation (#11359)
* chore: use @zkochan/git-wt package for worktree creation Replace the in-repo `worktree:new` script and `shell/wt.*` helpers with the published `@zkochan/git-wt` package. Contributors now install it globally (`pnpm add -g @zkochan/git-wt`) and enable the `wt` shell function via `git-wt init <shell>`, which also makes `git wt <branch>` available as a native git subcommand. * chore: remove shell/cleanup-worktrees.sh Its functionality is now available as `git-wt cleanup` in the @zkochan/git-wt package, which contributors are already being directed to install in CONTRIBUTING.md. * docs: give copy-paste install commands for the wt shell function Previously CONTRIBUTING.md said "add this line to your config" and showed the snippet, making contributors open the rc file themselves. Replace with a one-liner per shell that appends to the rc file and activates `wt` in the current session in one go. |
||
|
Zoltan Kochan
|
0ba3ea0e4b |
bench(integrated-benchmark): replace 26-snapshot fixture with 1352-snapshot one (#262)
* bench(integrated-benchmark): replace 26-snapshot fixture with 1352-snapshot one
The previous `alotta-modules` fixture had 7 direct deps and 26
snapshots, small enough that a frozen-lockfile install completed in
~50 ms. At that scale the wall-time is dominated by per-process
overhead (binary startup, lockfile parse), so scaling wins in the
install pipeline — shared SQLite connections, import-method changes,
symlink-pass parallelism — don't show up.
Swap in a realistic 1352-snapshot lockfile of `alotta-modules` (389 KB
vs 13 KB) so the benchmark reflects a workload where the install
pipeline actually matters. First CI run will need to repopulate the
cached verdaccio storage; subsequent runs are served from the existing
cache step.
* bench(integrated-benchmark): add pnpm-workspace.yaml to the fixture
`allowBuilds: {core-js: false, es5-ext: false}` silences pnpm v11's
`ERR_PNPM_IGNORED_BUILDS` warnings for two packages whose postinstalls
would fire in this 1352-snapshot fixture. Pacquet's benchmark-side
`.npmrc` already sets `ignore-scripts=true` so nothing actually runs,
but pnpm still prints to stderr about the ignored builds and that
noise can reach hyperfine.
Stage the new file the same way `package.json` is staged — embedded
via `include_str!` for the default fixture, copied from `--fixture-dir`
when set. The `--fixture-dir` copy is gated on the file's existence
so existing fixture directories that predate this change don't start
erroring out.
|
||
|
Zoltan Kochan
|
6544b3ace5 |
perf(store-dir): share one read-only StoreIndex across cache lookups (#261)
* perf(store-dir): share one read-only StoreIndex across cache lookups `DownloadTarballToStore::load_cached_cas_paths` previously opened a fresh SQLite `Connection::open_with_flags` + `PRAGMA busy_timeout` on every snapshot, so a 1352-package frozen-lockfile install paid 1352 connection-opens just to read cache hints — all serialized on tokio's blocking pool because `spawn_blocking` can't parallelize opens against one file. Open a `StoreIndex` once per install, wrap it in `Arc<Mutex<…>>` (rusqlite `Connection` is `Send` but not `Sync`), and thread the handle through `CreateVirtualStore`, `InstallWithoutLockfile`, and `DownloadTarballToStore`. Each lookup now takes the mutex just for the sub-millisecond `SELECT`, releases it before the per-file validation pass, and frees the Arc when the install finishes. Measured on the #260 repro (1352 packages, release build, macOS APFS, warm CAS store): `rm -rf node_modules && pacquet install --frozen- lockfile` drops from ~17.4 s to ~13.9 s, with CPU utilization rising from ~135 % to ~160 % as the serialized-open bottleneck clears. * address review feedback * `create_virtual_store.rs`: bind `store_index.as_ref()` to a local and use `async move` so the closure's copy of the `Option<&…>` is moved into each future, rather than borrowing from the closure's per-iteration scope. * `load_cached_cas_paths`: treat a poisoned index mutex as a cache miss rather than propagating the panic. The `SELECT` is stateless, so the prior panic can't have left the index in an inconsistent shape, and cache lookups are a best-effort hint — falling through to a fresh download is strictly safer than crashing every subsequent snapshot. * park StoreIndex open on the blocking pool Copilot flagged that `StoreIndex::shared_readonly_in` does synchronous SQLite work (`Connection::open_with_flags` + `PRAGMA busy_timeout`) and calling it directly in an `async fn` stalls the tokio reactor thread for the duration of the open. It's once per install, not a hot spot, but matches the pattern the store-index writer already uses and costs us nothing to keep consistent. Wrap both call sites (frozen-lockfile and no-lockfile install paths) in `tokio::task::spawn_blocking` and await the result before kicking off concurrent package work. Also moves `tokio` from `dev-dependencies` into `dependencies` in package-manager's `Cargo.toml` since it's now a runtime dep for the library code, not just tests. * degrade JoinError on StoreIndex open to a cache miss `spawn_blocking(...).await` returns `Result<_, JoinError>` which fires on blocking-task panic or on cancellation during runtime shutdown. Panicking on that turns a transient runtime issue into a hard crash for an install that could otherwise succeed — cache lookups just miss. Drop the `.expect(...)` and degrade to `None` via `.ok().flatten()`. `shared_readonly_in` already yields `None` for a first-time install against an empty store, and every downstream caller already handles that shape, so this is the same failure mode the code already copes with — just reached from a different source. * use async move in install_dependencies_from_registry map closure Makes the recursive no-lockfile closure's captures explicit: bind `&node_modules_path` to a local `node_modules_path_ref` (Copy) and switch to `async move` so every captured variable is moved (copied, for all the `&T` and `Option<&T>` captures) into the future instead of borrowing the closure's per-iteration stack frame. Matches the pattern already used in `create_virtual_store.rs` and in the outer no-lockfile closure — keeps all three parallel-map sites consistent. * log JoinError before degrading to cache miss on StoreIndex open The silent `.ok().flatten()` hid two failure modes behind a cache miss: a panic inside the blocking task, and task cancellation during runtime shutdown. Neither is a crash-worthy error, but a blocking-task panic during install really should be visible somewhere. Promote the two call sites to a `match` and emit a `tracing::warn!` on the `Err(JoinError)` branch before falling back to `None`. Uses the `pacquet::install` target in common with the rest of this crate's tracing. Degradation path is unchanged. * chore: cargo fmt * log JoinError in tarball cache lookup, refresh stale comment Mirror the tracing change from the index-open call sites: when the blocking cache-lookup task returns `Err(JoinError)`, emit a `tracing::warn!` with the error and the `cache_key` before degrading to `None`. Panic / cancellation stays diagnosable; the caller still falls through to a fresh download. Also drop the stale "msgpackr-records decoding is a follow-up" wording from the cache-lookup rationale — `StoreIndex::get` has decoded pnpm-written msgpackr-records since |
||
|
Zoltan Kochan
|
1325c6eac9 |
feat(lockfile): support npm-alias dependencies in snapshots (#258)
* feat(lockfile): support npm-alias dependencies in snapshots Snapshot dependency values can be either a plain version (e.g. `5.1.2`) or an npm-alias reference (e.g. `string-width-cjs: string-width@4.2.3`, produced by `"string-width-cjs": "npm:string-width@^4.2.0"` in a consumer's package.json). Parsing the alias form as `PkgVerPeer` failed because it is not a bare semver. Introduce `SnapshotDepRef` — `Plain(PkgVerPeer)` or `Alias(PkgNameVerPeer)` — mirroring pnpm's `refToRelative` alias detection, and thread it through the snapshot → virtual-store symlink path: alias entries use the entry key as the link name under `node_modules/` and the aliased target for the virtual-store lookup. * chore: cargo fmt |