[agent] Found by the scheduled vlt bug-hunt routine (ledger #307).
Summary
#846 (fix for #838) made agent-mode rollback / remove prune the parent directories of a patch-added file once they're empty. It doesn't know which directories apply actually created, though. It assumes that "a package directory holding only patch-added files did not exist before the patch" (crates/socket-patch-core/src/patch/rollback.rs:402-404). That assumption is wrong whenever the installed package already had an empty directory. Tarballs can ship directory entries, and vlt (and other extractors) materialize them. A postinstall or runtime can also create such a directory. If the patch adds a file inside one, rollback deletes the file and then deletes the pre-existing directory and every pre-existing empty ancestor up to the package root.
Before #846 (on 428b938), rollback left the shipped directories alone (it only leaked the directory apply created, which is #838). On 6811b4e the shipped directories are gone.
Impact
- After a "successful" rollback / remove (exit 0), the installed package is no longer in its pre-patch state. Directories that were present at install time are missing.
- Code that writes into a directory it ships (log, cache or output placeholders) now fails with
ENOENT.
- A later
vlt install with an existing node_modules doesn't put the directory back, because vlt sees the store entry as present. Only a clean rm -rf node_modules && vlt ci recovers it.
- Scope: agent mode only (hosted and vendored reinstall whole tarballs). It's core rollback code, so it isn't vlt-specific. vlt just makes it easy to show, because it extracts tarball directory entries into
node_modules/.vlt/<DepID>/node_modules/<name>. It is narrow: the patch has to add a file under a directory that existed but was empty.
Repro (real vlt, local mock registry + patch API)
The mock's left-pad@1.3.0 tarball has package/package.json, package/index.js and two directory entries, package/lib/ and package/lib/extra/. The patch (aaaaaaaa-…) changes index.js and adds package/lib/extra/deep/new.js (empty beforeHash). SOCKET_PATCH_SERVER_URL / SOCKET_API_URL point at the mock.
echo '{"name":"app","version":"1.0.0","dependencies":{"left-pad":"1.3.0"}}' > package.json
echo '{"config":{"registries":{"npm":"http://127.0.0.1:18555/"}}}' > vlt.json
vlt install --allow-scripts :scripts
P=node_modules/.vlt/~npm~left-pad@1.3.0/node_modules/left-pad
find $P -type d # left-pad left-pad/lib left-pad/lib/extra (shipped)
socket-patch scan --mode agent --yes --json # rc 0; adds lib/extra/deep/new.js
socket-patch rollback --yes --json; echo $? # 0
find $P -type d # main 6811b4e: left-pad <- lib/ and lib/extra/ deleted
# 428b938: left-pad lib lib/extra lib/extra/deep (#838 leak only)
node -e "fs=require('fs');fs.writeFileSync(require.resolve('left-pad').replace('index.js','lib/extra/x.log'),'1')"
# Error: ENOENT: no such file or directory
vlt install --allow-scripts :scripts; test -d $P/lib/extra || echo still-missing # still-missing
socket-patch remove pkg:npm/left-pad@1.3.0 --yes deletes the same directories.
Expected vs actual
Matrix (Linux, real vlt)
| socket-patch |
vlt 1.2.0 |
vlt 1.3.6 |
main 6811b4e, rollback |
repro (×2) |
repro (×2) |
main 6811b4e, remove |
— |
repro |
428b938 (parent of #846), rollback |
— |
shipped dirs kept (lib/extra/deep leaked, #838) |
| control: patch adds into a new top-level dir (no shipped empty dirs) |
pass, dirs pruned exactly |
pass |
macOS / Windows weren't probed: the prune is plain remove_dir path logic, so it's OS-independent.
First bad commit: 6811b4e (#846).
Suspect code
crates/socket-patch-core/src/patch/rollback.rs:413 prune_emptied_parents: it walks every component from pkg_path down to the file's parent and removes each one while it's empty. The "empty means apply created it" test at lines 402-404 can't tell a shipped empty directory from a created one.
crates/socket-patch-core/src/patch/apply.rs:478: create_dir_all(parent) doesn't record which directories it created. Recording them in the manifest/rollback state, or at least the deepest pre-existing ancestor at apply time, would let rollback stop there.
[agent] Found by the scheduled vlt bug-hunt routine (ledger #307).
Summary
#846 (fix for #838) made agent-mode
rollback/removeprune the parent directories of a patch-added file once they're empty. It doesn't know which directoriesapplyactually created, though. It assumes that "a package directory holding only patch-added files did not exist before the patch" (crates/socket-patch-core/src/patch/rollback.rs:402-404). That assumption is wrong whenever the installed package already had an empty directory. Tarballs can ship directory entries, and vlt (and other extractors) materialize them. A postinstall or runtime can also create such a directory. If the patch adds a file inside one, rollback deletes the file and then deletes the pre-existing directory and every pre-existing empty ancestor up to the package root.Before #846 (on
428b938), rollback left the shipped directories alone (it only leaked the directoryapplycreated, which is #838). On6811b4ethe shipped directories are gone.Impact
ENOENT.vlt installwith an existingnode_modulesdoesn't put the directory back, because vlt sees the store entry as present. Only a cleanrm -rf node_modules && vlt cirecovers it.node_modules/.vlt/<DepID>/node_modules/<name>. It is narrow: the patch has to add a file under a directory that existed but was empty.Repro (real vlt, local mock registry + patch API)
The mock's
left-pad@1.3.0tarball haspackage/package.json,package/index.jsand two directory entries,package/lib/andpackage/lib/extra/. The patch (aaaaaaaa-…) changesindex.jsand addspackage/lib/extra/deep/new.js(emptybeforeHash).SOCKET_PATCH_SERVER_URL/SOCKET_API_URLpoint at the mock.socket-patch remove pkg:npm/left-pad@1.3.0 --yesdeletes the same directories.Expected vs actual
rollbackrow). Agent-mode rollback / remove of a PyPI patch that added a file in a new directory leaves the empty directory in site-packages, so Python still imports it as a namespace package #838 asked to remove only the directories thatapplycreated ("Any directory thatapplycreated only for patch-added files should be removed once it's empty"), and Fix rollback leaving directories apply created (#838) #846's own title says "directories apply created".lib/extra/deepshould go, andlib/extraandlibshould stay.apply.Matrix (Linux, real vlt)
6811b4e, rollback6811b4e, remove428b938(parent of #846), rollbacklib/extra/deepleaked, #838)macOS / Windows weren't probed: the prune is plain
remove_dirpath logic, so it's OS-independent.First bad commit:
6811b4e(#846).Suspect code
crates/socket-patch-core/src/patch/rollback.rs:413prune_emptied_parents: it walks every component frompkg_pathdown to the file's parent and removes each one while it's empty. The "empty means apply created it" test at lines 402-404 can't tell a shipped empty directory from a created one.crates/socket-patch-core/src/patch/apply.rs:478:create_dir_all(parent)doesn't record which directories it created. Recording them in the manifest/rollback state, or at least the deepest pre-existing ancestor at apply time, would let rollback stop there.