GeezerVison is showing github.com — Bench: cover Bun isolated .bun store; fix rescan restore by mikolalysenko · Pull Request #667 · SocketDev/socket-patch
Conversation
What main changed
#496 (35de7548, "Fix npm crawler missing Bun, Deno and Yarn 4 stores") made the npm crawler walk Bun's isolated store, node_modules/.bun/<name>@<version>/node_modules/<name>, in scan, apply's resolver and VEX. Bun 1.3.2+ uses the isolated linker by default. The suite only had a hoisted Bun project (bun/*), so this path was never timed or validated.
Suite changes
- Added
bun-isolated/hosted and bun-isolated/rescan (3000 packages, 60 patched). They use the same text bun.lock generator as bun/*, with a pnpm-shaped .bun store: per-entry dependency symlinks, Bun's .bun/node_modules hoist links, and root links for direct dependencies only, so 2700 of the 3000 packages exist only in the store. The Expect matches bun/*: every package scanned, 60 redirected, bun.lock rewritten.
- Updated the README's package-manager list.
- Fixed (
93b1c929, harness): when a rescan's preparatory scan failed validation, run_once returned before restoring the tree. In compare the other binary then started from a rescanned project and was reported INVALID by mistake. The 2026-10-04 weekly A/B hit this: the pre-Fix npm crawler missing Bun, Deno and Yarn 4 stores (#366, #373, #405, #495) #496 base failed bun-isolated/rescan's prep, and the head, which validates on its own, was flagged redirect.rewrittenFiles: got [], want ["bun.lock"]. The harness now restores before returning the error.
- Removed: nothing.
Validation (4 vCPU Intel Xeon @ 2.80GHz cloud sandbox)
run on main 045d7ec7, 3 runs: bun-isolated/hosted 321.9 ms, bun-isolated/rescan 332.6 ms, 127 requests, 34.8 MiB peak RSS. Both validate.- The scenario exercises the new path: the pre-Fix npm crawler missing Bun, Deno and Yarn 4 stores (#366, #373, #405, #495) #496 binary (
1169ae68) fails it with lockfileOnlyPackages: got 2700, want 0.
- A/A
compare (default 15 pairs): hosted +0.8% [−2.1, +6.1], rescan −0.7% [−4.0, +6.4]. No regression.
strace -f -e trace=execve on a serve run shows only env → socket-patch, so no subprocesses.cargo fmt -p socket-patch-bench, cargo clippy -p socket-patch-bench --all-features --all-targets -D warnings and cargo test -p socket-patch-bench (29 passed) are all clean.- Harness fix (2026-10-04, 4 vCPU Xeon @ 2.10GHz): re-running
2463257a vs main on bun-isolated/rescan + yarn-berry/rescan now reports only the base invalid. A/A, 7 pairs: npm/rescan +1.2% [−10.2, +13.9], bun-isolated/rescan +1.2% [−7.9, +12.2], poetry/rescan −4.8% [−11.0, +12.3]. No regressions. fmt, clippy -D warnings and tests (29 passed) are clean.
Time budget
The harness fix doesn't change timing (restore runs only on a failed prep). A default (15-pair) compare of the two new scenarios took 89 s on this sandbox, fixture builds included. On this machine a 9-pair compare of the existing 39 scenarios took 8.4 min. GitHub's 4-core runners have run faster than this sandbox so far, so a full default compare should stay near the ~12 min target. If it goes over, bun/rescan is the first candidate to drop, since its code path is now covered twice.
🤖 Generated with Claude Code
https://claude.ai/code/session_016Gubp3gsnxFDvRbguaeLqT
Note Low Risk
Changes only the benchmark harness and fixtures; the rescan restore fix improves test isolation with no production runtime impact.
Overview
Adds bun-isolated hosted/rescan scenarios to socket-patch-bench so timing and validation cover Bun 1.3’s default isolated node_modules/.bun store (same bun.lock as hoisted bun, but a pnpm-shaped symlink layout with most packages store-only). The README now separates hoisted bun from bun-isolated.
Fixes a harness bug in run_once: when a rescan’s preparatory scan fails validation, the work tree is restored before returning the error so A/B compare does not leave the next binary starting from a partially rewritten fixture and failing validation incorrectly.
Reviewed by Cursor Bugbot for commit 93b1c92. Configure here.
Generated by Claude Code
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Bun 1.3.2+ installs with the isolated linker by default, keeping every
package only in node_modules/.bun/<name>@<version>/node_modules/<name>.
#496 taught the npm crawler (scan, apply's resolver, VEX) to walk that
store, but the suite only had a hoisted Bun layout, so the new walk was
never timed and a regression back to "2700 lockfile-only packages"
would have gone unnoticed.
Add bun-isolated/{hosted,rescan}: the same text bun.lock as bun/*, with
a pnpm-shaped .bun store, per-entry dependency links, Bun's
.bun/node_modules hoist links and root links for direct deps only. A
pre-#496 binary fails it (lockfileOnlyPackages: got 2700, want 0).
Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
cursor[bot] reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
cursor
Bot
left a comment
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stale Bugbot comment from a previous run.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Ready for review at 5ab8e870603f892b842bd45f4fcb5f54597145a7.
- CI: 97/97 green (3 skipped).
- Bugbot: reviewed this head, no findings.
- Reviewer focus: bench-only change (new
bun-isolated fixture); check the default compare time stays near the ~12 min budget.
Generated by Claude Code
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Codex review of 5ab8e870603f892b842bd45f4fcb5f54597145a7: ready to merge as-is from this review. No actionable findings.
Reviewed all three changed files, fixture registration, dependency/version resolution, and scoped/unscoped symlink targets. Validation passed:
- 29 repository tests in the benchmark crate and benchmark Clippy.
- Both full-size
bun-isolated/hosted and bun-isolated/rescan scenarios against the exact-head CLI: 3,000 packages and 60 patches, with all correctness assertions satisfied.
- Full fixture topology: 300 direct root links, 2,700 instances without root links, and all 9,099 symlinks valid; dependency targets match the declared package names and versions.
- A native Bun 1.3.2 isolated install confirms scoped and unscoped store keys, root/dependency/hoist links, and duplicate-version placement.
Full current-head CI is clear: 245 successful checks, six skipped; six successful workflows, two skipped. Exact-head Bugbot is clear, with no unresolved review threads. The unchanged source merges cleanly with current main 045d7ec7.
The two debug scenario runs validate correctness; they do not independently establish the author's performance comparison. No source changes were needed.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
A rescan's preparatory scan returned early on a validation failure,
skipping the tree restore. The project stayed rewritten, so in
`compare` the other binary's next run started from a rescanned tree
and was reported INVALID for the first binary's fault: on the
2026-10-04 weekly A/B a pre-#496 base failed bun-isolated/rescan's
prep and the head (valid on its own) was flagged invalid too.
Restore before returning the error, as the measured run already does.
Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_016Gubp3gsnxFDvRbguaeLqT
cursor[bot] reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 93b1c92. Configure here.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
[burn-down agent] Still Ready for review, now at 93b1c9295b938365c30be564fb21d1f7b35de1b4. The label was first applied at 5ab8e87.
- CI: 249/249 check runs green on this head (4 skipped by path filters), none failing.
- Bugbot: reviewed
93b1c92, no new issues. No unresolved review threads. Mergeable and up to date with main.
- New since the last ready comment:
93b1c92 restores the bench fixture after a failed rescan prep, so a dirty fixture no longer leaks into the next binary's run. Reviewers should check the restore path in the rescan prep.
Generated by Claude Code
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Labels
bench
socket-patch scan benchmark suite
Ready for review
Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|