Fix superseded rollback skipping still-patched copies (#1084)

crates/socket-patch-cli/src/commands/rollback.rs

@@ -2656,10 +2656,29 @@ pub(crate) async fn rollback_patches_inner(
             continue;
         }
         let files = target.files.as_ref().unwrap_or(&patch.files);
+        let mut result = result;
         if let Some(warning) = superseded_record_skip(target, &result, files, superseded).await {
-            warnings.push(warning);
-            superseded_left.push(purl.clone());
-            continue;
+            // The superseded primary never reached its store copies; one
+            // still at this record's patched bytes (Bun's orphaned
+            // isolated-store entry, #1084) is restored here, or fails the
+            // run and keeps the record, before the record is dropped.
+            match socket_patch_core::patch::rollback::rollback_store_copies_holding_patch(
+                purl,
+                pkg_path,
+                files,
+                &blobs_path,
+                common.dry_run,
+            )
+            .await
+            {
+                Some(copies) if !copies.success => result = copies,
+                restored => {
+                    warnings.push(warning);
+                    superseded_left.push(purl.clone());
+                    results.extend(restored);
+                    continue;
+                }
+            }
         }
         if !result.success {
             has_errors = true;

crates/socket-patch-cli/tests/in_process_rollback_hosted.rs

@@ -1570,6 +1570,157 @@ async fn remove_unhosts_a_package_whose_agent_record_is_superseded() {
     );
 }
 
+/// Re-shape the superseded fixture as Bun's isolated linker leaves it
+/// after `bun install` of the hosted pin (#1084): `node_modules/<name>`
+/// links to the new store entry holding B's bytes, and the old
+/// `node_modules/.bun/<name>@<version>` entry Bun never prunes still holds
+/// agent record A's patched bytes. Returns the orphan's `index.js`.
+#[cfg(unix)]
+fn orphan_bun_store_copy(root: &Path) -> std::path::PathBuf {
+    let nm = root.join("node_modules");
+    let top = nm.join(NAME);
+    let store = nm.join(".bun");
+    let live = store
+        .join(format!("{NAME}@http+++patch.test+bbbbbbbb"))
+        .join("node_modules")
+        .join(NAME);
+    let orphan = store
+        .join(format!("{NAME}@{VERSION}"))
+        .join("node_modules")
+        .join(NAME);
+    for (dir, index) in [(&live, B_PATCHED_INDEX), (&orphan, A_PATCHED_INDEX)] {
+        std::fs::create_dir_all(dir).unwrap();
+        std::fs::copy(top.join("package.json"), dir.join("package.json")).unwrap();
+        std::fs::write(dir.join("index.js"), index).unwrap();
+    }
+    std::fs::remove_dir_all(&top).unwrap();
+    std::os::unix::fs::symlink(&live, &top).unwrap();
+    orphan.join("index.js")
+}
+
+/// #1084: the live copy holds B's bytes, but an orphaned Bun store copy
+/// still holds A's. Rollback restores that copy before dropping record A,
+/// so the next `bun install` cannot relink agent patch A unrecorded.
+#[cfg(unix)]
+#[tokio::test]
+#[serial]
+async fn rollback_restores_an_orphaned_store_copy_of_a_superseded_record() {
+    let server = MockServer::start().await;
+    let tmp = tempfile::tempdir().unwrap();
+    let pristine = write_superseded_agent_fixture(tmp.path(), &server, B_PATCHED_INDEX).await;
+    let orphan = orphan_bun_store_copy(tmp.path());
+
+    let (code, envelope) = run_rollback_subprocess_online(tmp.path(), &server, &[]);
+    assert_eq!(code, 0, "{envelope:#}");
+    assert_eq!(
+        std::fs::read(&orphan).unwrap(),
+        ORIGINAL_INDEX,
+        "the orphaned store copy at A's patched bytes is restored:\n{envelope:#}"
+    );
+    let live = tmp.path().join("node_modules").join(NAME).join("index.js");
+    assert_eq!(
+        std::fs::read(live).unwrap(),
+        B_PATCHED_INDEX,
+        "B's live copy is still the reinstall's to replace"
+    );
+    assert!(
+        warning_codes(&envelope).contains(&"rollback_record_superseded".to_string()),
+        "{envelope:#}"
+    );
+    let restored = std::fs::read_to_string(tmp.path().join("package-lock.json")).unwrap();
+    assert_eq!(restored, pristine);
+    assert!(manifest_patch_keys(tmp.path()).is_empty());
+}
+
+/// #1084: `remove <purl>` restores the orphaned copy the same way.
+#[cfg(unix)]
+#[tokio::test]
+#[serial]
+async fn remove_restores_an_orphaned_store_copy_of_a_superseded_record() {
+    let server = MockServer::start().await;
+    let tmp = tempfile::tempdir().unwrap();
+    let pristine = write_superseded_agent_fixture(tmp.path(), &server, B_PATCHED_INDEX).await;
+    let orphan = orphan_bun_store_copy(tmp.path());
+
+    let (code, envelope) = run_remove_subprocess_online(tmp.path(), &server, PURL);
+    assert_eq!(code, 0, "{envelope:#}");
+    assert_eq!(
+        std::fs::read(&orphan).unwrap(),
+        ORIGINAL_INDEX,
+        "the orphaned store copy at A's patched bytes is restored:\n{envelope:#}"
+    );
+    let restored = std::fs::read_to_string(tmp.path().join("package-lock.json")).unwrap();
+    assert_eq!(restored, pristine);
+    assert!(manifest_patch_keys(tmp.path()).is_empty());
+}
+
+/// #1084: a store copy that holds A's bytes but cannot be restored (its
+/// before-blob is gone) fails the run and keeps record A, instead of
+/// dropping the only data that could restore it.
+#[cfg(unix)]
+#[tokio::test]
+#[serial]
+async fn an_unrestorable_orphaned_store_copy_keeps_the_superseded_record() {
+    let server = MockServer::start().await;
+    let tmp = tempfile::tempdir().unwrap();
+    write_superseded_agent_fixture(tmp.path(), &server, B_PATCHED_INDEX).await;
+    orphan_bun_store_copy(tmp.path());
+    let before = socket_patch_core::hash::git_sha256::compute_git_sha256_from_bytes(ORIGINAL_INDEX);
+    std::fs::remove_file(tmp.path().join(".socket/blobs").join(before)).unwrap();
+
+    let (code, envelope) = run_rollback_subprocess_online(tmp.path(), &server, &[]);
+    assert_eq!(
+        code, 1,
+        "a copy still holding A's bytes that cannot be restored fails:\n{envelope:#}"
+    );
+    assert_eq!(
+        manifest_patch_keys(tmp.path()),
+        vec![PURL.to_string()],
+        "record A stays for a later rollback"
+    );
+}
+
+/// #1084 review: a store copy of the superseding patch B that shares a
+/// file with record A (B's `lib.js` is A's patched `lib.js`, its
+/// `index.js` is B's own) is not A's copy. It is left, as the primary is,
+/// and the run drops record A as before instead of failing on it forever.
+#[cfg(unix)]
+#[tokio::test]
+#[serial]
+async fn a_superseding_store_copy_sharing_a_file_with_the_record_is_left() {
+    let server = MockServer::start().await;
+    let tmp = tempfile::tempdir().unwrap();
+    write_superseded_agent_fixture(tmp.path(), &server, B_PATCHED_INDEX).await;
+    let copy = orphan_bun_store_copy(tmp.path());
+    // Record A patched lib.js too; B carries the same patched lib.js.
+    let before = socket_patch_core::hash::git_sha256::compute_git_sha256_from_bytes(ORIGINAL_INDEX);
+    let after = socket_patch_core::hash::git_sha256::compute_git_sha256_from_bytes(A_PATCHED_INDEX);
+    let manifest_path = tmp.path().join(".socket/manifest.json");
+    let mut manifest: Value =
+        serde_json::from_str(&std::fs::read_to_string(&manifest_path).unwrap()).unwrap();
+    manifest["patches"][PURL]["files"]["package/lib.js"] =
+        serde_json::json!({ "beforeHash": before, "afterHash": after });
+    std::fs::write(&manifest_path, manifest.to_string()).unwrap();
+    std::fs::write(&copy, B_PATCHED_INDEX).unwrap();
+    std::fs::write(copy.with_file_name("lib.js"), A_PATCHED_INDEX).unwrap();
+    let live = tmp.path().join("node_modules").join(NAME).join("lib.js");
+    std::fs::write(live, A_PATCHED_INDEX).unwrap();
+
+    let (code, envelope) = run_rollback_subprocess_online(tmp.path(), &server, &[]);
+    assert_eq!(code, 0, "{envelope:#}");
+    assert!(
+        warning_codes(&envelope).contains(&"rollback_record_superseded".to_string()),
+        "{envelope:#}"
+    );
+    assert_eq!(std::fs::read(&copy).unwrap(), B_PATCHED_INDEX);
+    assert_eq!(
+        std::fs::read(copy.with_file_name("lib.js")).unwrap(),
+        A_PATCHED_INDEX,
+        "B's copy is the reinstall's to replace"
+    );
+    assert!(manifest_patch_keys(tmp.path()).is_empty());
+}
+
 /// B07: a remove/rollback identifier that names the superseded record's
 /// uuid (A) selects the whole owned pin, so the hosted pin of the same
 /// release under the superseding uuid (B) is unwound too, instead of being

crates/socket-patch-core/src/patch/rollback.rs

@@ -371,6 +371,87 @@ pub async fn rollback_package_patch(
     .await
 }
 
+/// Roll back the other pnpm/vlt/Bun store copies of `pkg_path` that still
+/// hold this record's patched bytes, when the primary copy was left to a
+/// superseding hosted pin (#1084).
+///
+/// [`rollback_package_patch`] visits a package's store copies only after
+/// the primary succeeds. A superseded primary fails (it holds the hosted
+/// patch's bytes), so its copies are never reached. On Bun's isolated
+/// linker the store entry the hosted reinstall orphaned is such a copy:
+/// it still holds the agent patch, and the next `bun install` can link it
+/// again. Each copy [`holds_this_patch`] is rolled back with the
+/// single-copy engine and folded into the returned result, which fails if
+/// any copy fails. A copy with any file at bytes the record never wrote
+/// (the superseding patch's own copies, which can share a file with this
+/// record) is left, as the primary is. `None` when no copy holds this
+/// record's patch (or `package_key` is not npm).
+pub async fn rollback_store_copies_holding_patch(
+    package_key: &str,
+    pkg_path: &Path,
+    files: &HashMap<String, PatchFileInfo>,
+    blobs_path: &Path,
+    dry_run: bool,
+) -> Option<RollbackResult> {
+    if !package_key.starts_with("pkg:npm/") {
+        return None;
+    }
+    let mut folded: Option<RollbackResult> = None;
+    for copy in crate::crawlers::npm_crawler::find_store_peer_variant_copies(pkg_path).await {
+        if !holds_this_patch(&copy, files).await {
+            continue;
+        }
+        let copy_result =
+            rollback_package_patch_at(package_key, &copy, files, blobs_path, dry_run).await;
+        let result = folded.get_or_insert_with(|| RollbackResult {
+            package_key: package_key.to_string(),
+            package_path: pkg_path.display().to_string(),
+            success: true,
+            files_verified: Vec::new(),
+            files_rolled_back: Vec::new(),
+            error: None,
+            sidecar: None,
+        });
+        crate::patch::store_copies::fold(result, &copy, copy_result);
+    }
+    folded
+}
+
+/// Whether `dir` is a copy this record patched: every file is at the
+/// record's patched or original bytes (a new file may be absent), and at
+/// least one is at the patched bytes or cannot be checked (an unsafe key,
+/// a read error other than "not found"), which may hide them. A file at
+/// any other bytes, or a missing original file, marks a copy the record
+/// does not own, just as it marks the primary superseded.
+async fn holds_this_patch(dir: &Path, files: &HashMap<String, PatchFileInfo>) -> bool {
+    let mut patched = false;
+    for (file, info) in files {
+        let rel = normalize_file_path(file);
+        if !crate::patch::apply::is_safe_relative_subpath(rel) {
+            patched = true;
+            continue;
+        }
+        // FIFO-safe: a FIFO or device at the leaf is refused, not opened.
+        match crate::utils::fs::read_regular_to_bytes(&dir.join(rel)).await {
+            Ok(bytes) => {
+                let hash = crate::hash::git_sha256::compute_git_sha256_from_bytes(&bytes);
+                if hash == info.after_hash {
+                    patched = true;
+                } else if hash != info.before_hash {
+                    return false;
+                }
+            }
+            Err(e) if e.kind() == std::io::ErrorKind::NotFound => {
+                if !info.before_hash.is_empty() {
+                    return false;
+                }
+            }
+            Err(_) => patched = true,
+        }
+    }
+    patched
+}
+
 impl crate::patch::store_copies::CopyFold for RollbackResult {
     const VERB: &'static str = "roll back";