Conversation
…s concurrently Every release flow in bun-upgrade.test.ts copied the test binary into its own temp dir. On Windows the first launch of each new copy is slow (about 2.4s for the 78 MB canary build on the arm64 lane, about 7s for a debug build), and the three flows at the bottom of the file ran one after another, each waiting on its own Expand-Archive. The flows that fail before the binary is replaced now share one stand-in, created once in beforeAll. The flow that replaces the binary gets one of its own. Both are hard links to bunExe() and fall back to a copy when the temp dir is on another volume. The upgrade only renames directory entries, so the linked bunExe() is never modified. All eight tests run in one concurrent batch. Each flow stages into its own BUN_TMPDIR, except the one that tests the held-open default temp dir. Assertions now pin the exact output and the state on disk: the inline snapshots and exit codes of the argument checks, the announced version and the exact last error line of every failing flow (digest mismatch with the exact asset URL, unpack failure), the staging directory contents after each run, and after a completed upgrade the exact contents of the install directory (the fake release script on POSIX, the parked .outdated image plus the recreated bunx link on Windows) together with the full "Upgraded" banner.
|
Warning Review limit reachedYour included review limit has been reached. You’re in a promotional period — use the checkbox below to run this review for free:
On-demand reviews are free for the next 31 days. After that, they cost $0.25 per reviewed file. How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset (next review available in 5 minutes), then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
|
Status: ready for review at 728d0fd. Test-only change, one file. How the slow part was measured: the old file and this file were timed on the same machines with the same binaries. Windows 11 arm64 with the canary build: 17.2s before, 3.6s to 3.8s after (18 runs). Windows Server 2019 x64: 10.2s before, 3.0s to 3.3s after (10 runs). Linux x64 with |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It's a careful, well-documented test refactor, but it leans on several platform-specific claims (Windows hard-link semantics vs. the swap path, openTempDirWithoutSharingDelete running concurrently with the other flows, the exact .outdated/bunx install-dir contents) that I can't independently verify from Linux — a quick human look from someone with the Windows lanes in mind would be worthwhile.
Checked: the hard-link-is-safe argument against renameat_concurrently_without_fallback and the Windows .outdated rename path; that every concurrent flow using the shared standInExe fails before the swap and stages into its own BUN_TMPDIR; that the new exact-string assertions (Downgrading/is out!, the Upgraded banner, Unzip failed (exit code: 9), the checksum error, bunx-debug.exe) match the source in upgrade_command.rs / install_completions_command.rs; and that releaseAsset never needs a -baseline suffix because bun upgrade doesn't select one.
Extended reasoning...
Overview
Test-only change to test/cli/install/bun-upgrade.test.ts (~200 lines). Two orthogonal changes: (1) the per-test copyFile(bunExe(), ...) becomes a hard link with a copy fallback, and the flows that never reach the swap share one stand-in from beforeAll; (2) the last three release-flow tests move to it.concurrent, each with its own BUN_TMPDIR so staging dirs don't collide. Along the way every assertion is tightened — toContain fragments become inline snapshots / exact toEndWith / readdirSync equality, and exit codes are now checked everywhere.
Security risks
None. Test file only; no production code touched. The hard link is created in a temp dir and the upgrade path renames directory entries rather than writing through the inode, so bunExe() itself is never mutated (verified against the POSIX renameat_concurrently_without_fallback path and the Windows .outdated rename in source; the PR also confirms empirically via link counts / fsutil hardlink list).
Level of scrutiny
Medium. It's test-only and the PR description is unusually thorough (per-platform timings, first-launch probes, source line references, 8–13 green runs per lane), and the bug-hunting pass found nothing. But the diff is not mechanical: it introduces concurrency across flows that previously ran serially, shares one executable across concurrent subprocesses, changes what the FILE_SHARE_DELETE test runs alongside on Windows, and pins a lot of exact runtime output (the full Upgraded banner, the Windows install-dir contents including bunx.exe/bunx-debug.exe). Those are exactly the kinds of Windows-specific interactions where a maintainer sanity check is cheap insurance.
Other factors
I cross-checked the new exact strings against src/runtime/cli/upgrade_command.rs (Downgrading from Bun {}-canary, Bun v{} is out! You're on v{}, Unzip failed (exit code: {}), Failed to verify Bun (code: {}), the checksum error, the full Upgraded. banner) and install_completions_command.rs (bunx-debug.exe in debug builds) — they all match. bun upgrade never selects a -baseline asset, so the digest test's releaseAsset URL is correct on every lane. The concurrent digest runs and the --profile flow all use the shared stand-in but each set a distinct BUN_TMPDIR, and none reaches the swap. No existing test was weakened or removed; assertions are strictly stronger. Deferring only because the change is large enough and Windows-sensitive enough that it falls outside "simple/mechanical".
|
The three Windows-specific points from the review were checked on two Windows machines, not only from the source. For the record:
|
… the first line On POSIX a piped stderr receives the "Fetching version tags" progress line when the version fetch takes longer than the progress bar's delay. Pinned to one CPU, the debug build printed it in every run and three or four tests failed. The announcement is now looked up among the lines, and the non-canary branch is matched at the end of the output. The stand-in is linked from the resolved executable path, as test/cli/bun.test.ts already does.
|
Pushed 728d0fd after a self-review of the first version. The first version asserted that the version announcement is line 0 of stderr. That is not true on POSIX when the version fetch takes longer than the progress bar's 500ms delay: a The stand-in is now linked from The self-review also noted that the Windows unpack line pins a fallthrough ( |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it restructures the test's concurrency (the openTempDirWithoutSharingDelete handle is now held while other flows run), switches to hard-linking the test binary, and tightens several assertions to exact platform-specific strings (Info-ZIP exit code 9, the Windows install-dir listing with bunx.exe/bunx-debug.exe), a human look would still be worthwhile.
What was reviewed:
- Hard-link safety:
bun upgradeonly renames the launched entry (POSIX rename/exchange, Windows.outdatedpark) —bunExe()is never written through the link; cross-volume falls back tocopyFile. - Concurrency isolation: each failing flow stages into its own
BUN_TMPDIR, so the held-open default temp dir handle and the sharedstandInExe(never swapped) don't collide. toContainAnyValuesis a real jest-extended matcher already used in the suite;expectTargetsReleasecovers both canary and release wording.
Extended reasoning...
Overview
Test-only change to test/cli/install/bun-upgrade.test.ts (~250 lines). Replaces per-test copyFile(bunExe(), ...) with a shared hard-linked stand-in from beforeAll (copy fallback for cross-volume), converts the three sequential release-flow tests to it.concurrent with per-test BUN_TMPDIR isolation, and strengthens assertions from toContain/not.toContain to inline snapshots, toEndWith on exact error strings, exit-code checks, and readdirSync disk-state checks. No production code touched.
Security risks
None. Test-only; the local TLS release server, NODE_TLS_REJECT_UNAUTHORIZED=0, and ASAN_OPTIONS handling are all pre-existing.
Level of scrutiny
Moderate-to-high for a test change. The correctness of running these flows concurrently rests on several platform-specific invariants: (1) bun upgrade only renames the launched directory entry and never writes into the inode behind bunExe(), so a hard link is safe; (2) holding the default temp dir open without FILE_SHARE_DELETE in the runner process does not block concurrent tests that create/delete subdirectories inside it; (3) the shared standInExe is only used by flows that fail before the swap. The PR description traces each of these to source (src/sys/lib.rs, self_exe_path()) and verifies empirically on Windows 11 arm64, Server 2019 x64, and Linux debug+ASAN with 8–13 green runs each, and the robobun follow-up confirms fsutil hardlink list and install-dir listings on real Windows machines. That is thorough, but the tightened assertions — exact Unzip failed (exit code: 9) (depends on Info-ZIP across all POSIX CI images), the exact Windows install-dir listing including the bunx/bunx-debug link created by completions, and the full Upgraded banner — are the kind of thing a maintainer familiar with the CI fleet should sanity-check.
Other factors
The bug-hunting system found nothing. I checked that toContainAnyValues is a real matcher (used in jest-extended.test.js and expect.test.js), that openTempDirWithoutSharingDelete opens fs::RealFS::get_default_temp_dir() (so the concurrent flows' BUN_TMPDIR redirection genuinely sidesteps it), and that closeTempDirHandle not being in a try/finally is pre-existing behavior, not a regression. The zip is now written to a separate tmpdirSync() so it doesn't appear in the readdirSync(installDir) assertion. Given the scope of the concurrency restructuring and the platform-specific assertion tightening, I'm deferring rather than auto-approving.
|
On the one new point in this review, the |
Problem
bun-upgrade.test.tstakes 20s on Windows 11 arm64 and 13s on Windows 2019 x64 (build 101560), 2s elsewhere.bunExe(). On Windows the first launch of a new copy costs 2.4s (canary build, arm64) or 7s (debug build). A hard link starts in under 20ms.Fix
beforeAll. The flow that swaps gets its own. Both are hard links tobunExe(), with a copy as the cross-volume fallback. Five copies per run become two links.src/sys/lib.rs:9060, cross-device path at:8982). It never writes into the file behindbunExe().test/cli/bun.test.ts:146already runs a linked stand-in the same way.BUN_TMPDIR. The completing flow keeps the default temp dir, the one it holds open.Background
bun upgrade --stableunpacks the asset into<tmpdir>/<version>/, runs it with--version, then moves it over its own executable. The test serves the release locally.<name>.outdatedand runscompletions, which recreates thebunxlink (install_completions_command.rs:106).Notes
Timings (same machine, same build, old file vs this file)
bun bd test(debug, 216 MB)bun bd test(debug+ASAN, 820 MB)The old file was run with
--timeout=90000on Windows, which is what the CI runner passes: on the arm64 machine two of the old tests did not fit into the default 5s per test. The new file was also run 10 times per Windows machine with the default timeout. What remains is the completing flow: the first launch of the extracted image (2.4s on arm64, which is part of what the flow verifies) plusExpand-Archive(0.7s on arm64, about 2s on Server 2019). A deflated archive was slower (0.4s to compress, 1.3s to expand), so the archive stays stored.Per run: executable materializations 5 copies -> 2 hard links, zip builds 1 -> 1,
bun upgradeprocesses 9 -> 9, sequential phases 4 -> 1. The zip is not built inbeforeAllbecause only one test needs it and it is on that test's own critical path either way.First-launch probe on the arm64 machine (78 MB canary build): fresh copy 2431ms and 2371ms, the same copy again 7ms, a hard link 18ms, a fresh copy of the small
cmd.exe26ms. The cost is per new file and grows with its size. On Server 2019, where the bootstrap removes Defender, a fresh copy costs 273ms and a hard link 6ms. The arm64 image still reports real-time protection on (it is tamper protected), which may be why that lane is the slowest.Why the links are safe: POSIX installs the new binary with a NOREPLACE rename, then an EXCHANGE, then delete plus rename (
renameat_concurrently_without_fallback). Windows renames the old image to.outdatedand moves the new one in. The cross-device fallback unlinks the destination before it writes.self_exe_path()(src/bun_core/util.rs:2745) is/proc/self/exeon Linux,_NSGetExecutablePathplusrealpathon macOS andGetModuleFileNameWplusGetFinalPathNameByHandleon Windows. All of them return the link that was launched, because a hard link is an ordinary name and not a symlink. Confirmed on Linux (link count of the debug binary was 3 after a run, the swapped-out link sat in the staging dir) and on Windows (fsutil hardlink listshowed the stand-ins and the parked.outdatedentries,bunExe()unchanged). Renaming and deleting a link to the running test binary works on Windows (probed), so temp dir cleanup is unaffected.Assertion changes
toContainlines, no exit code.--help: stdout starts with the usage line, stderr is empty, exit code 0. Before: anot.toContainof a message spelled differently from the real one (bun itself), so it could not fail.--stable --profile: one line names v9.9.9 (only profile assets are served, so this also proves--profilewas honored), stderr ends with the exact unpack error, exit code 1. Before: twonot.toContain, exit code awaited but not checked.Upgradedbanner, exit code 0. On disk: on POSIX the install dir holds exactly the executable and its content is the fake release script. On Windows it holds exactly the executable,<name>.outdatedandbunx.exe(bunx-debug.exefor debug builds). The non-canary branch (output ends with the exactCongrats! ...line, install dir unchanged) is derived from the source strings. CI andbun bdare canary builds and take the other branch, as before.readdirSyncof the staging dir is[], the mode check is unconditional on POSIX. Before:toContain("9.9.9")and twoexistsSyncchecks.[]. Match: one line names v9.9.8, stderr ends with the exact unpack error, the staging root is["9.9.8"]. Before:toContainon a fragment andtoContain("9.9.8").Expand-Archivewrites to the inherited stdout.The version line is looked up among the lines (
toContainAnyValues), not pinned to line 0. On POSIX a piped stderr receives aFetching version tagsprogress line first when the version fetch takes longer than the progress bar's 500ms delay. The first version of this PR pinned line 0: with the debug build pinned to one CPU (taskset -c <cpu> bun bd test ...) three or four tests failed in 3 of 3 runs. With the lookup, 3 of 3 starved runs and 3 normal runs pass. ThetoEndWithassertions are not affected by progress output, because the progress bar ends its line before bun prints anything else.The exact unpack line differs by platform. POSIX:
Unzip failed (exit code: 9)(Info-ZIP, whichscripts/bootstrap.shinstalls on every Linux image and which macOS ships). Windows:error: Failed to verify Bun (code: ENOENT), becauseupgrade_command.rsdoes not check the Expand-Archive exit status and notices the failure at the verify step.The Windows unpack line pins what
bun upgradeprints today. The Windows block inupgrade_command.rs(around line 957) does not look at the Expand-Archive status, unlike the POSIX block, so a corrupt download is reported as a verify failure. That is a product question outside this test-only change. A fix there changes one constant in this file.The stand-in is linked from
realpath(bunExe()), the same idiom astest/cli/bun.test.ts:146. A shared harness helper, and the other tests that copy the binary on Windows (bun-install-registry.test.tsmakes seven copies around line 9073), are candidates for a follow-up and are out of scope here. NTFS allows 1023 links per file. A machine that runs this file hundreds of times without clearing its temp dir hits that limit, and the helper then falls back to a copy.With a Windows debug build the completing flow takes about 14s, of which about 7s is the first launch of the extracted 216 MB image, so a local
bun bd teston Windows still needs--timeoutas the CI runner passes it. Before this change two tests needed it even with the release build.Open PRs #39379, #37674 and #31743 touch this file. The helpers keep their names and signatures, the argument tests keep their order, and the
startReleaseServercomment that #37674 edits is untouched. The two flows that #39379 adds can usestandInExewhen either side rebases.test/expected-durations.jsonis left for CI to regenerate.no test proof · iteration 0 · Platform-specific test-only change; deferring to CI.