Skip to content

Agent-mode rollback and remove delete empty directories the package shipped when a patch added a file under them (regression from #846) #862

Description

[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.

No activity

Activity on this issue will appear here.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions