Skip to content

fix: survive the re-warm on every manager, and drop Yarn PnP - #65

Merged
zkochan merged 3 commits into
mainfrom
fix-rewarm-yarn
Aug 19, 2026
Merged

fix: survive the re-warm on every manager, and drop Yarn PnP#65
zkochan merged 3 commits into
mainfrom
fix-rewarm-yarn

Conversation

@zkochan

@zkochan zkochan commented Aug 19, 2026

Copy link
Copy Markdown
Member

Two things, per review direction, in one PR.

1. The re-warm fix (second weekly-run failure, on Yarn)

Error: The nearest package directory (.../yarn/alotta-files/.rewarm) doesn't seem to be part of the project declared in .../yarn/alotta-files.

Berry walks up from wherever it runs and refuses to install in a directory nested under a project that doesn't declare it. Two changes, both reusing patterns the repo already has:

  • The throwaway re-warm copy now lives as a sibling of the project (<fixture>.rewarm), where nothing sits above it for Berry's walk to find.
  • It gets the empty-lockfile project-root marker every scenario already relies on — cleanLockfile's nodetouch trick — a no-op for managers that don't need it.

Validation, done properly this time

Both weekly failures were managers the earlier smoke tests didn't cover (pnpm 11 → dangling symlink; Yarn → project-root walk), so this round ran the real scenario chain end to end for exactly those, with Yarn 6.0.0-rc.19 — the benchmark's own version — provisioned the way the benchmark provisions it:

manager update row through the re-warm
yarn 1958ms
yarn_pnp 2262ms
pnpm 11 1954ms
bun 677ms

Every manager completes. (pnpm 12 and npm were validated in earlier rounds and don't touch the changed paths: pnpm 12 reaches the cache through absolute env paths, npm accepts any directory as a root.)

2. Yarn PnP removed from the benchmark

The page compares installation into node_modules; PnP is a different product with a different on-disk contract, most rows never applied to it (n/a across every node_modules column), and the intro already points at Yarn's own benchmarks for the modes this page doesn't carry. The column, its command, its linker case, and its recorded results go together; the intro now names PnP explicitly among the modes covered there instead.

Also adds .claude to .gitignore.

🤖 Generated with Claude Code

…oot marker

The weekly run's second failure, this time on Yarn: Berry walks up from
wherever it runs, finds the fixture project above `.rewarm`, and
refuses to install in a directory that project doesn't declare. The
throwaway copy now lives as a sibling of the project, where nothing
sits above it, and gets the same empty-lockfile project-root marker
every scenario already relies on (`cleanLockfile`'s nodetouch trick) —
a no-op for the managers that don't need it.

Validated by running the real scenario chain end to end for the
managers the previous smoke tests missed: yarn 1958ms, yarn_pnp 2262ms,
pnpm11 1954ms, bun 677ms on the update row — every one through the
re-warm, none through a wall.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 19, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: cbab791b-8ea4-423b-b198-f483c0c98dd7

📥 Commits

Reviewing files that changed from the base of the PR and between 5d611d9 and db41e0a.

📒 Files selected for processing (1)
  • benchmarkFixture.js

Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review.

📜 Recent review details
🔇 Additional comments (2)
benchmarkFixture.js (2)

393-398: LGTM!


414-417: LGTM!


📝 Walkthrough

Walkthrough

The rewarm flow now creates its temporary project beside the fixture. It also cleans the lockfile before installation, including for package managers that treat the operation as a no-op.

Changes

Rewarm flow

Layer / File(s) Summary
Rewarm project setup
benchmarkFixture.js
The temporary rewarm project uses a sibling <fixture>.rewarm directory. The setup calls cleanLockfile before installation.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to db41e

The PR makes a localized fixture-path and project-root marker change, with the affected manager scenarios completing successfully; no actionable merge-blocking risk remains beyond normal checks and review.

Possibly related PRs

Poem

A rabbit hops past Yarn’s old wall,
“A sibling path will solve it all!”
The lockfile rests, clean and light,
Then installs start just right.
Squeak, the rewarm path is bright!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Title check ⚠️ Warning The title describes the re-warm fix but incorrectly states that the change drops Yarn PnP, which the PR validates instead. Remove “and drop Yarn PnP” and state that the change fixes re-warm handling across package managers, including Yarn PnP.
✅ Passed checks (4 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-rewarm-yarn

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Keep cache re-warm project outside Yarn fixture

🐞 Bug fix 🕐 10-20 Minutes

Grey Divider

AI Description

• Moves the re-warm copy beside the fixture to avoid Yarn Berry ancestry rejection.
• Creates Yarn’s empty lockfile root marker before the untimed cache warm-up.
Diagram

graph TD
  A["Benchmark fixture"] --> B["Sibling re-warm"] --> C["Root marker"] --> D["Manager install"] --> E[("Shared cache")] --> F["Update scenario"]
Loading
High-Level Assessment

The current approach is appropriate: placing the throwaway project beside the fixture avoids Yarn’s ancestor-project detection, while reusing cleanLockfile preserves established manager-specific behavior. A fully independent temporary directory was considered, but would add path and lifecycle management without improving isolation for this benchmark.

Files changed (1) +10 / -1

Bug fix (1) +10 / -1
benchmarkFixture.jsIsolate the re-warm project from Yarn’s fixture ancestry +10/-1

Isolate the re-warm project from Yarn’s fixture ancestry

• Moves the temporary re-warm project from a nested '.rewarm' directory to a sibling path, preventing Yarn Berry from associating it with the fixture project. Calls 'cleanLockfile' before installation so Yarn receives an empty lockfile project-root marker while other managers remain unaffected.

benchmarkFixture.js

zkochan and others added 2 commits August 19, 2026 16:29
The page compares installation into `node_modules`; PnP is a different
product with a different on-disk contract, most of the table's rows
never applied to it (`n/a` across every `node_modules` column), and the
intro already points at Yarn's own benchmarks for the modes this page
doesn't carry. The column, its command, its linker case, and its
recorded results go together.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@zkochan zkochan changed the title fix: keep the re-warm project out of the fixture, and give Yarn its root marker fix: survive the re-warm on every manager, and drop Yarn PnP Aug 19, 2026
@zkochan
zkochan merged commit 0b65469 into main Aug 19, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant