bun patch --commit: restore the node_modules/<pkg> link under the isolated linker
Problem
- With
linker = "isolated",bun patch bthenbun patch --commit bleavesnode_modules/bas a real directory, not the link intonode_modules/.bun. The dependencies ofbare siblings of its store entry, sorequire("a")from insidebfails withMODULE_NOT_FOUND, and no later install repairs it. - Since #29489 (1.3.14),
Symlinker::ensure_symlink(src/install/isolated_install/Symlinker.rs:78) keeps a real directory where a link belongs, to protect thebun patchcopy before--commit. Nothing ends that afterwards.
Fix
do_patch_commitrecords the real path of the folder it diffed (CommittedPatch). The root or workspace entry whose link is that folder writes the link again (Strategy::ReplaceDirectory): the copy moves aside, the link is written, the copy is deleted.- The relink needs the
.bun-tag-<hash>file of the new patch in the store entry, so a patch that does not apply keeps the copy. So does a diff withBinary files ... differ(with a warning), and a copy with a nestednode_modulesfolder. The relink runs in the task of the entry, before its scripts, and again after all tasks. --commitrefuses a folder that is a link.git diffrecords a link asnew file mode 120000, and no install can apply that patch.- Verified:
test/cli/install/bun-patch.test.ts, block "bun patch --commit with the isolated linker" (7 of 9 fail on main). Alsobun-install-patch.test.ts,isolated-install.test.ts,isolated-relink.test.ts.
Background
- The isolated linker installs each package at
node_modules/.bun/<name>@<version>/node_modules/<name>, with its dependencies as links next to it.node_modules/<name>of the root or a workspace is a link to that entry. bun patch <pkg>puts a detached, editable copy atnode_modules/<pkg>.--commitdiffs it against the cache, writes the patch file, and installs.- That install rebuilds the store entry at the same path. The file
.bun-tag-<patch hash>marks the applied patch.
Notes
Output of the repro (loopback registry, b depends on a, linker = "isolated").
Before (release build of main, 1.4.3-canary.1+b52d51348):
$ bun install -> exit 0
node_modules/b is a symlink into node_modules/.bun; require("a") from inside b -> a@1.0.0
$ bun patch b -> exit 0
$ bun patch --commit b -> exit 0
node_modules/b is a REAL directory; require("a") from inside b -> MODULE_NOT_FOUND
$ bun install -> exit 0
node_modules/b is a REAL directory; require("a") from inside b -> MODULE_NOT_FOUND
After:
$ bun patch --commit b -> exit 0
node_modules/b is a symlink into node_modules/.bun; require("a") from inside b -> a@1.0.0
1.3.13 gives the "after" output. It wrote the link in SymlinkDependencies and replaced whatever was there.
How the fix works:
Symlinker::replace_directoryrenames the copy to a hidden sibling (.<name>.old-<hex>) before it writes the link. If the rename fails the copy is whole, and if the link fails the copy is renamed back. A delete that stops part-way leaves nothing partial where the next--commitwould diff it.- The folder is found by its real path (open the directory, read the path of the handle), so
node_modules/w/node_modules/bandpackages/w/node_modules/bare the same folder. A plainbun installstill never replaces a real directory. The copy of another package stays, and so does a second copy of the samename@versionat another link, because neither was diffed. - When the diff is empty (
No changes detected) the copy equals the package and no patch is recorded. The link goes back when the store entry exists. No tag is needed. - A root or workspace task waits in
CheckIfBlockeduntil its dependencies are done, then runsSymlinkDependencyBinaries. The relink is the first thing in that step. The bin links of the entry resolve through the link, and thepostinstallscript of a workspace package that loads the patched package works. On main that script fails the commit:Cannot find package 'no-deps',postinstall script from "w" exited with 1. - An entry in a dependency cycle does not wait for its dependencies, so the same relink runs once more on the main thread after all tasks. One case stays as on main: the workspace and the patched package are in one cycle, and a script of that workspace loads the package during the install of
--commit. - git prints
Binary files a/x and b/x differwhere a text diff cannot hold a change, and the patch parser drops that file. The copy then has edits that the patch lacks.do_patch_commitprints a warning and does not record the folder, so the copy stays as it does on main. To refuse such a commit, or to supportgit diff --binary, changes--commitfor every linker and is not part of this PR. do_patch_commitmoves the nestednode_modulesfolder of the copy out of the diff. With the isolated linker that folder can hold thebun patchcopy of a package that the hoisted layout nests under this one (node_modules/b/node_modules/a), with edits that are not committed. When the copy has that folder, the commit does not record it, so the copy stays as it does on main. A fresh copy frombun patchnever has the folder, because the copy from the cache skipsnode_modules. #43178 changes that for bundled dependencies, so the two PRs need one more condition when both are in.- When the link cannot be written and the copy cannot move back, the error of the move back is returned, and it names the path of the copy. A folder whose real path cannot be read fails the commit with that error.
- The patch failure test adds a symlink to the copy, which the patch parser rejects with
bad_file_mode. I removed the tag check once to confirm that this test then fails. - With the hoisted linker the install replaces
node_modules/<pkg>with the patched package, and a patch that does not apply leaves the folder alone. The isolated linker now behaves the same.
The --commit guard:
- On main,
bun patch --commit bwith nobun patch bbefore it (the folder is still the link) writes a patch of the link, records it inpatchedDependencies, and every laterbun installexits 1 withfailed to parse patchfile: bad_file_mode. 1.3.13 does the same. - On main a second
--commitafter the first is harmless, only because the copy is still there. With the link back it would overwrite the good patch file. The guard exits 1 before it writes anything:error: node_modules/b is not a folder that bun patch prepared, thennote: Run 'bun patch b' first.
A tree that an older build left broken: a plain bun install keeps the real directory (by design, it can hold edits). bun patch --commit <pkg> on this build, or rm -rf node_modules/<pkg> && bun install, puts the link back. I checked the first one.
Self-review before the PR: 10 concerns raised, 8 addressed. The two not taken:
- A redesign where
bun patchedits inside the store and the link never moves. It loses the edits whenglobalStore = true, it needs a rule to pick among the peer variants of one package, and nobody has built it. This PR restores what 1.3.13 did after--commit. - Tests parametrized by linker for the whole patch flow. That is a follow-up for the test file, not for this fix.
What stays as it is (not in this PR):
- Between
bun patchand--committhe copy cannot load the dependencies of the package. 1.3.13 has the same behavior. - An abandoned edit (no
--commitat all) keeps the real directory, also afterbun install --force. - A package with no link of its own (a transitive dependency, or a dependency of a workspace that the root does not depend on):
bun patchcreates the copy at the path of the hoisted layout, for examplenode_modules/aornode_modules/w/node_modules/b. The patch is applied in the store, but the copy stays after--commit. When that path goes through a link (node_modules/b/node_modules/a), the copy is written into the store entry ofb. WithglobalStore = truethat entry is in the shared cache. - A second
bun patch b@1.0.0while the copy exists below a workspace link removes the linknode_modules/wand leaves a real directory. - #41453 edits the same branch of
Symlinker::ensure_symlink. After it, the merge is one condition: keep the directory when it has apackage.jsonand the strategy is notReplaceDirectory. #43188 changes howbun patchmakes the copy and does not change what--commitdoes with it.
no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/install/bun-patch.test.ts