diff --git a/crates/socket-patch-cli/src/commands/rollback.rs b/crates/socket-patch-cli/src/commands/rollback.rs index b51a058f4..e42f8a78f 100644 --- a/crates/socket-patch-cli/src/commands/rollback.rs +++ b/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; diff --git a/crates/socket-patch-cli/tests/in_process_rollback_hosted.rs b/crates/socket-patch-cli/tests/in_process_rollback_hosted.rs index 75b5baeea..c1fa514dd 100644 --- a/crates/socket-patch-cli/tests/in_process_rollback_hosted.rs +++ b/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/` +/// links to the new store entry holding B's bytes, and the old +/// `node_modules/.bun/@` 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 ` 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(©, 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(©).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 diff --git a/crates/socket-patch-core/src/patch/rollback.rs b/crates/socket-patch-core/src/patch/rollback.rs index 764d5da7f..0c7177d49 100644 --- a/crates/socket-patch-core/src/patch/rollback.rs +++ b/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, + blobs_path: &Path, + dry_run: bool, +) -> Option { + if !package_key.starts_with("pkg:npm/") { + return None; + } + let mut folded: Option = None; + for copy in crate::crawlers::npm_crawler::find_store_peer_variant_copies(pkg_path).await { + if !holds_this_patch(©, files).await { + continue; + } + let copy_result = + rollback_package_patch_at(package_key, ©, 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_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) -> 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";