Share rollback artifact retention across remove and rollback

CHANGELOG.md

@@ -102,6 +102,10 @@ limits, and required install commands.
 
 ### Fixed
 
+- `remove` and `rollback` share artifact-retention rules, preserving the original
+  blobs of patches left active. Removing one patch no longer destroys another
+  patch's offline rollback data (#559).
+
 - Global mode (`-g`) finds npm, yarn, pnpm, bun, RubyGems and Composer on
   Windows, where they install as `.cmd` / `.bat` shims, instead of reporting
   an empty scan. The yarn and npm-family global lookups no longer run from the

crates/socket-patch-cli/CLI_CONTRACT.md

@@ -754,6 +754,9 @@ worse, lets a warm cache silently serve unpatched bytes):
   (parity with rollback/repair/`scan --prune`; GC errors warn and continue, repair's posture).
   Package archives (`.socket/packages/`) are legacy in v5.0: nothing writes or reads them, so
   every GC sweep removes the whole directory.
+  Like `rollback`, `remove` retains the original blobs for every patch left in the manifest
+  and for removed-but-not-installed patches, so removing one patch preserves offline rollback
+  of other active patches. Only blobs no longer referenced by that keep set are collected.
 * **remove restores hosted pins (v5.0)**: an identifier matching hosted pins in the lockfiles
   (purl or patch uuid; v5 keeps no hosted ledger) restores each matched pin to its default
   upstream registry entry — the same restore as `rollback` (see "Hosted unwind coverage"), for every
@@ -830,7 +833,7 @@ A bare `rollback` (or a scoped one, for its scope) restores the SYSTEM to unpatc
 3. **Vendored leg** — each in-scope ledger entry (embedded-record entries included) is reverted through the vendor backends: lockfile wiring restored, artifact dir deleted (and its emptied `.socket/vendor/<eco>/` husk pruned, v5.0), ledger entry dropped + persisted per purl (crash-consistent, like `vendor --revert`). A **drift-keep** (the backend refused a drifted lock) keeps the entry, the artifact, AND the manifest record (`vendoredKept`, exit 1 — the system is still patched); a failure is recorded and other entries proceed.
 4. **Hosted leg** — each in-scope hosted pin is restored to its default upstream registry entry; see "Hosted unwind coverage" below. After a hosted leg with no failure, a wet run deletes a pre-v5 `redirect-state.json` once no lockfile pins a hosted patch any more (a failed delete is the `legacy_redirect_ledger_kept` warning).
 5. **Manifest cleanup** — entries are removed ONLY for in-scope purls whose legs fully succeeded, were not-installed, or were release-variant siblings narrowed away by an attempted variant that succeeded (half a variant group never lingers — `remove` parity); drift-kept and failed purls keep their records, and a failed variant holds its whole group. No-op removals never rewrite the file. A failed write surfaces as `manifest_write_failed` (warning + `partial_failure` exit 1; GC still runs against the unchanged manifest).
-6. **GC** — `cleanup_unused_blobs` + diff/package-archive sweeps against the post-removal manifest, with beforeHash blobs pinned (synthetic afterHash-slot records) for (a) removed-but-not-installed entries (a crawler miss must not destroy the only local revert data — `remove` parity) and (b) EVERY entry remaining in the post-removal manifest — still-active patches (failed, drift-kept, eco-/path-excluded) keep their revert data, so a scoped or failed run never destroys the blobs a later rollback needs; only blobs referenced solely by genuinely-removed entries are swept. GC errors warn (`cleanup_failed`) and continue — they never affect the exit (repair's posture).
+6. **GC** — blob, diff and legacy package-archive sweeps against the post-removal manifest, using the same artifact-reference policy as `remove`, retaining beforeHash blobs for (a) removed-but-not-installed entries (a crawler miss must not destroy the only local revert data — `remove` parity) and (b) EVERY entry remaining in the post-removal manifest — still-active patches (failed, drift-kept, eco-/path-excluded) keep their revert data, so a scoped or failed run never destroys the blobs a later rollback needs; only blobs referenced solely by genuinely-removed entries are swept. GC errors warn (`cleanup_failed`) and continue — they never affect the exit (repair's posture).
 
 **Confirmation prompt.** A wet, non-preserve run with work prompts once, remove-style, composing only the clauses that apply into one English list (`a and b`, `a, b, and c`) with counted nouns: `Roll back N patches`, `remove them from the local manifest`, `delete M vendored artifacts and their ledger records`, `restore H hosted packages to the upstream registry` (e.g. `Roll back 1 patch, remove it from the local manifest, and restore 1 hosted package to the upstream registry?`) — default yes, auto-accepted under `--yes`/`--json`/non-TTY (the shared `confirm` semantics; CI unaffected). Decline prints `Rollback cancelled.` and exits 0. `--dry-run` and `--preserve-state` runs are prompt-free (they delete no local state).
 

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

@@ -1,6 +1,6 @@
 use clap::Args;
 use socket_patch_core::api::client::get_api_client_with_overrides;
-use socket_patch_core::manifest::cleanup_blobs::format_bytes;
+use socket_patch_core::manifest::cleanup_blobs::{format_bytes, ArtifactReferences};
 use socket_patch_core::manifest::operations::{read_manifest, write_manifest};
 use socket_patch_core::manifest::schema::PatchManifest;
 use socket_patch_core::patch::redirect::upstream::HostedPin;
@@ -14,8 +14,7 @@ use std::time::Duration;
 
 use super::get::short_uuid;
 use super::rollback::{
-    pin_before_hash_blobs, rollback_patches_inner, run_hosted_leg, sweep_failure,
-    sweep_unused_artifacts, HostedLegOutcome, InnerSelection,
+    rollback_patches_inner, run_hosted_leg, sweep_failure, HostedLegOutcome, InnerSelection,
 };
 use crate::commands::vendored_backend::{RevertedEntry, VendorRevertStep, VendoredBackend};
 use crate::args::{apply_env_toggles, GlobalArgs};
@@ -935,24 +934,15 @@ pub async fn run(args: RemoveArgs) -> i32 {
     }
 
     // ── GC ──────────────────────────────────────────────────────────────
-    // Clean up unused blobs (previewed, not deleted, on --dry-run). The
-    // reference manifest is the post-removal manifest PLUS one synthetic
-    // keep record per retained entry above: `cleanup_unused_blobs` keeps
-    // only afterHash blobs (beforeHash blobs are normally re-downloadable
-    // on demand), so each pinned before-hash is listed in an afterHash
-    // slot. Scoped to REVERT data only — the retained entries' real
-    // afterHash blobs stay sweepable like any other orphan.
-    let mut cleanup_reference = updated_manifest;
-    let pinned_purls: Vec<String> = retained_not_installed
-        .iter()
-        .map(|p| (*p).to_string())
-        .collect();
-    pin_before_hash_blobs(&mut cleanup_reference, &manifest, pinned_purls.iter());
+    let references = ArtifactReferences::after_removal(
+        &manifest,
+        &updated_manifest,
+        retained_not_installed.iter().copied(),
+    );
     let mut blobs_removed = 0;
     let mut archives_removed = 0;
     if !args.preserve_state {
-        let sweep =
-            sweep_unused_artifacts(&cleanup_reference, &socket_dir, args.common.dry_run).await;
+        let sweep = references.sweep(&socket_dir, args.common.dry_run).await;
         // repair's posture: a failed pass (or a pass that could not unlink
         // every orphan) warns and continues, never fatal; its partial
         // counts still stand.

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

@@ -5,7 +5,7 @@ use socket_patch_core::api::blob_fetcher::{
 };
 use socket_patch_core::api::client::{get_api_client_with_overrides, ApiClient};
 use socket_patch_core::manifest::cleanup_blobs::{
-    format_all_in_use, format_cleanup_result_for, CleanupResult,
+    format_all_in_use, format_cleanup_result_for, ArtifactReferences, CleanupResult,
 };
 use socket_patch_core::manifest::operations::read_manifest;
 use socket_patch_core::patch::apply::PatchSources;
@@ -16,7 +16,7 @@ use std::time::Duration;
 use crate::args::{apply_env_toggles, parse_bool_flag, GlobalArgs};
 use crate::commands::fetch_stage::files_diffs_cannot_cover;
 use crate::commands::lock_cli::{acquire_or_emit, error_envelope};
-use crate::commands::rollback::{sweep_failure, sweep_unused_artifacts};
+use crate::commands::rollback::sweep_failure;
 use crate::json_envelope::{Command, Envelope, PatchAction, PatchEvent, Status};
 
 #[derive(Args)]
@@ -627,7 +627,9 @@ async fn repair_inner(
     // summary prints once all three passes are in, so "nothing to clean
     // up" is only said when all three really are empty.
     if let (false, Some(manifest)) = (args.download_only, manifest.as_ref()) {
-        let sweep = sweep_unused_artifacts(manifest, &socket_dir, args.common.dry_run).await;
+        let sweep = ArtifactReferences::for_apply(manifest)
+            .sweep(&socket_dir, args.common.dry_run)
+            .await;
         let passes = [
             ("blob", BLOB, sweep.blobs),
             ("diff", DIFF_ARCHIVE, sweep.diffs),

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

@@ -2,9 +2,7 @@ use clap::Args;
 use socket_patch_core::api::blob_fetcher::{fetch_blobs_by_hash, format_fetch_result};
 use socket_patch_core::api::client::{get_api_client_with_overrides, ApiClient};
 use socket_patch_core::crawlers::{CrawlerOptions, Ecosystem};
-use socket_patch_core::manifest::cleanup_blobs::{
-    cleanup_unused_archives, cleanup_unused_blobs, CleanupResult,
-};
+use socket_patch_core::manifest::cleanup_blobs::{ArtifactReferences, CleanupResult};
 use socket_patch_core::manifest::operations::{
     get_before_hash_blobs, read_manifest, write_manifest,
 };
@@ -31,54 +29,6 @@ use crate::json_envelope::Command as EnvelopeCommand;
 use crate::looks_like_uuid;
 use crate::ui::{plural, StatusLine};
 
-/// Pin the beforeHash blobs of `purls` into `reference` as synthetic keep
-/// records: `cleanup_unused_blobs` keeps only afterHash blobs (beforeHash
-/// blobs are normally re-downloadable on demand), so each pinned
-/// before-hash is listed in an afterHash slot. Scoped to REVERT data only
-/// — the pinned entries' real afterHash blobs stay sweepable like any
-/// other orphan. Shared by rollback's default GC and `remove`'s
-/// crawler-miss guard.
-pub(crate) fn pin_before_hash_blobs<'a>(
-    reference: &mut PatchManifest,
-    source: &PatchManifest,
-    purls: impl IntoIterator<Item = &'a String>,
-) {
-    for purl in purls {
-        let Some(record) = source.patches.get(purl) else {
-            continue;
-        };
-        let pinned: HashMap<String, PatchFileInfo> = record
-            .files
-            .iter()
-            .filter(|(_, info)| !info.before_hash.is_empty())
-            .map(|(file, info)| {
-                // A synthetic key so pins never clobber the real file rows
-                // of an entry that REMAINS in the reference (whose afterHash
-                // blobs must stay kept). The sweep reads only the hash
-                // VALUES, never the keys, and this reference manifest is
-                // in-memory only.
-                (
-                    format!("{file}#beforeHash-pin"),
-                    PatchFileInfo {
-                        before_hash: String::new(),
-                        after_hash: info.before_hash.clone(),
-                    },
-                )
-            })
-            .collect();
-        if pinned.is_empty() {
-            continue; // every file was created-by-patch: no revert blobs
-        }
-        if let Some(existing) = reference.patches.get_mut(purl) {
-            existing.files.extend(pinned);
-        } else {
-            let mut keep_record = record.clone();
-            keep_record.files = pinned;
-            reference.patches.insert(purl.clone(), keep_record);
-        }
-    }
-}
-
 #[derive(Args)]
 pub struct RollbackArgs {
     /// What to roll back: a package PURL, a patch UUID, or a path glob
@@ -812,38 +762,6 @@ pub(crate) struct HostedLegOutcome {
     pub(crate) edited_files: std::collections::BTreeSet<String>,
 }
 
-/// One GC pass over `.socket/blobs`, `diffs` and `packages` against
-/// `reference` (the post-removal manifest with the revert blobs a later
-/// rollback needs pinned in). Each directory reports separately: callers
-/// own the warn-and-continue posture and the wording. The core sweep
-/// removes an emptied directory, so a fully reverted project keeps none
-/// of the three. Shared by rollback's GC, remove's post-removal sweep and
-/// repair's cleanup phase.
-pub(crate) struct ArtifactSweep {
-    pub(crate) blobs: std::io::Result<CleanupResult>,
-    pub(crate) diffs: std::io::Result<CleanupResult>,
-    pub(crate) packages: std::io::Result<CleanupResult>,
-}
-
-pub(crate) async fn sweep_unused_artifacts(
-    reference: &PatchManifest,
-    socket_dir: &Path,
-    dry_run: bool,
-) -> ArtifactSweep {
-    ArtifactSweep {
-        blobs: cleanup_unused_blobs(reference, &socket_dir.join("blobs"), dry_run).await,
-        diffs: cleanup_unused_archives(reference, &socket_dir.join("diffs"), dry_run).await,
-        // Nothing writes or reads `.socket/packages/` any more; sweep the
-        // leftover directory whole.
-        packages: cleanup_unused_archives(
-            &PatchManifest::default(),
-            &socket_dir.join("packages"),
-            dry_run,
-        )
-        .await,
-    }
-}
-
 /// The `cleanup_failed` detail for one sweep pass labelled `label`: the
 /// directory-level error that stopped the pass, or — after a pass that
 /// kept sweeping past unlink failures — the files it could not remove
@@ -1661,35 +1579,19 @@ pub async fn run(args: RollbackArgs) -> i32 {
             }
 
             // ── GC ───────────────────────────────────────────────────────
-            // Sweep against the post-removal manifest, with beforeHash
-            // blobs pinned (synthetic afterHash-slot records — the sweep
-            // keeps only afterHash blobs) for (a) removed-but-not-installed
-            // entries — a crawler miss must not destroy the only local
-            // revert data — and (b) in-scope entries that FAILED this run:
-            // their entries stay, and the blobs the gate just downloaded
-            // must survive for an offline retry.
+            // Removal retains originals for active patches and crawler misses.
             let mut gc_json: serde_json::Value = serde_json::json!({ "skipped": true });
             let mut gc_bytes_freed: u64 = 0;
             if cleanup_allowed {
-                // Pin the beforeHash blobs of EVERY entry remaining in the
-                // manifest (still-active patches keep their revert data —
-                // an eco-scoped or failed run must never destroy the blobs
-                // a later rollback needs) plus removed-but-not-installed
-                // entries (remove's crawler-miss guard). Blobs referenced
-                // only by genuinely-removed entries are what gets swept.
-                let pinned_purls: Vec<String> = removed
-                    .iter()
-                    .filter(|p| not_installed.contains(p))
-                    .chain(updated_manifest.patches.keys())
-                    .cloned()
-                    .collect();
-                // The post-removal manifest is not needed past the sweep:
-                // it becomes the (pin-augmented) reference in place.
-                let mut cleanup_reference = updated_manifest;
-                pin_before_hash_blobs(&mut cleanup_reference, &manifest, pinned_purls.iter());
-                let sweep =
-                    sweep_unused_artifacts(&cleanup_reference, &socket_dir, args.common.dry_run)
-                        .await;
+                let references = ArtifactReferences::after_removal(
+                    &manifest,
+                    &updated_manifest,
+                    removed
+                        .iter()
+                        .filter(|p| not_installed.contains(p))
+                        .map(String::as_str),
+                );
+                let sweep = references.sweep(&socket_dir, args.common.dry_run).await;
                 let mut removed_counts = [0usize; 3];
                 for (slot, (label, result)) in removed_counts.iter_mut().zip([
                     ("blob", sweep.blobs),
@@ -4418,60 +4320,6 @@ mod tests {
         );
     }
 
-    /// A purl absent from the source manifest contributes nothing to the
-    /// GC reference: the pin loop skips it (the lookup-miss `continue`)
-    /// rather than inserting an empty synthetic keep record, and present
-    /// purls around it still pin normally.
-    #[test]
-    fn pin_before_hash_blobs_skips_purls_absent_from_source() {
-        let mut present = make_record("uuid-present");
-        present.files.insert(
-            "package/index.js".to_string(),
-            PatchFileInfo {
-                before_hash: "beefbeef".to_string(),
-                after_hash: "cafecafe".to_string(),
-            },
-        );
-        let mut source = PatchManifest {
-            patches: HashMap::new(),
-            setup: None,
-        };
-        source
-            .patches
-            .insert("pkg:npm/present@1.0.0".to_string(), present);
-
-        let mut reference = PatchManifest {
-            patches: HashMap::new(),
-            setup: None,
-        };
-        let purls = [
-            "pkg:npm/ghost@9.9.9".to_string(),
-            "pkg:npm/present@1.0.0".to_string(),
-        ];
-        pin_before_hash_blobs(&mut reference, &source, purls.iter());
-
-        assert!(
-            !reference.patches.contains_key("pkg:npm/ghost@9.9.9"),
-            "a purl the source manifest does not hold must not grow a \
-             synthetic record, got {:?}",
-            reference.patches.keys().collect::<Vec<_>>()
-        );
-        let pinned = reference
-            .patches
-            .get("pkg:npm/present@1.0.0")
-            .expect("the present purl must still pin");
-        assert_eq!(pinned.files.len(), 1, "got {:?}", pinned.files);
-        assert_eq!(
-            pinned
-                .files
-                .get("package/index.js#beforeHash-pin")
-                .expect("synthetic pin key")
-                .after_hash,
-            "beefbeef",
-            "the beforeHash must be pinned in an afterHash slot"
-        );
-    }
-
     /// The vendored leg tolerates a key with no ledger entry: the scope
     /// resolver guarantees keys exist, but a divergent ledger must skip
     /// the key silently (the lookup-miss `continue`) rather than panic or