bun patch --commit: restore the node_modules/<pkg> link under the isolated linker

Closed
RO
24d ago
+492-34

Problem

  • With linker = "isolated", bun patch b then bun patch --commit b leaves node_modules/b as a real directory, not the link into node_modules/.bun. The dependencies of b are siblings of its store entry, so require("a") from inside b fails with MODULE_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 the bun patch copy before --commit. Nothing ends that afterwards.

Fix

  • do_patch_commit records 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 with Binary files ... differ (with a warning), and a copy with a nested node_modules folder. The relink runs in the task of the entry, before its scripts, and again after all tasks.
  • --commit refuses a folder that is a link. git diff records a link as new 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). Also bun-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 at node_modules/<pkg>. --commit diffs 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_directory renames 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 --commit would 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/b and packages/w/node_modules/b are the same folder. A plain bun install still never replaces a real directory. The copy of another package stays, and so does a second copy of the same name@version at 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 CheckIfBlocked until its dependencies are done, then runs SymlinkDependencyBinaries. The relink is the first thing in that step. The bin links of the entry resolve through the link, and the postinstall script 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 differ where 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_commit prints a warning and does not record the folder, so the copy stays as it does on main. To refuse such a commit, or to support git diff --binary, changes --commit for every linker and is not part of this PR.
  • do_patch_commit moves the nested node_modules folder of the copy out of the diff. With the isolated linker that folder can hold the bun patch copy 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 from bun patch never has the folder, because the copy from the cache skips node_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 b with no bun patch b before it (the folder is still the link) writes a patch of the link, records it in patchedDependencies, and every later bun install exits 1 with failed to parse patchfile: bad_file_mode. 1.3.13 does the same.
  • On main a second --commit after 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, then note: 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 patch edits inside the store and the link never moves. It loses the edits when globalStore = 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 patch and --commit the copy cannot load the dependencies of the package. 1.3.13 has the same behavior.
  • An abandoned edit (no --commit at all) keeps the real directory, also after bun 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 patch creates the copy at the path of the hoisted layout, for example node_modules/a or node_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 of b. With globalStore = true that entry is in the shared cache.
  • A second bun patch b@1.0.0 while the copy exists below a workspace link removes the link node_modules/w and 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 a package.json and the strategy is not ReplaceDirectory. #43188 changes how bun patch makes the copy and does not change what --commit does 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