From 06b886219eae4bc31e8777d4ac53749747483d39 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Fri, 9 Oct 2026 12:43:03 -0400 Subject: [PATCH 1/3] Keep restore blobs of active patches in GC Co-Authored-By: Claude Opus 5.5 (1M context) From a6605b1467387187972ff9d6ee9712c8c1d4aa97 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Fri, 9 Oct 2026 13:07:46 -0400 Subject: [PATCH 2/3] Keep restore blobs of active patches in GC `repair` (alias `gc`) and `scan --prune` kept only the afterHash blobs of patches still in the manifest, so the first repair deleted the originals `get` stored. A later `rollback --offline` of a still active patch then failed and told the user to run `repair`, which only downloads afterHash blobs and can never bring the original back. Give ArtifactReferences one policy for a manifest's patches, `active`: afterHash and beforeHash blobs plus the diff archive of every patch. repair and scan --prune use it, and remove/rollback's `after_removal` builds on it. Delete the dead cleanup_unused_blobs, cleanup_unused_archives and format_cleanup_result. The rollback missing-blob remedy now says to re-run without --offline (or once the patch API is reachable) instead of naming repair. Fixes #893 Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/socket-patch-cli/CLI_CONTRACT.md | 4 +- .../socket-patch-cli/src/commands/repair.rs | 2 +- .../socket-patch-cli/src/commands/rollback.rs | 8 +- .../socket-patch-cli/src/commands/scan/gc.rs | 2 +- .../tests/remove_rollback_api_overrides.rs | 6 +- .../tests/repair/repair_invariants.rs | 91 ++++++ .../tests/rollback/rollback_invariants.rs | 20 +- .../tests/scan/scan_paths_e2e.rs | 55 ++++ .../src/manifest/cleanup_blobs.rs | 267 ++++++++---------- 9 files changed, 277 insertions(+), 178 deletions(-) diff --git a/crates/socket-patch-cli/CLI_CONTRACT.md b/crates/socket-patch-cli/CLI_CONTRACT.md index bb4592f4b..edd61fb1b 100644 --- a/crates/socket-patch-cli/CLI_CONTRACT.md +++ b/crates/socket-patch-cli/CLI_CONTRACT.md @@ -24,7 +24,7 @@ For task-oriented guidance, start with [usage](../../docs/usage.md), | `apply` | — | Agent mode: apply patches from the local manifest | | `rollback` | — | **Full-state rollback (v5.0, MAJOR)**: restore original files AND unwind vendored lockfile wiring / restore hosted pins to their upstream registry entries, remove the rolled-back entries from the manifest, and GC their blobs/archives; takes optional variadic positional `targets` (PURL \| UUID \| package name \| path glob). See [Rollback command contract](#rollback-command-contract-v50) | | `remove` | — | Restore and remove one patch across hosted, vendored, and agent state; requires positional `identifier`. | -| `repair` | — | Download missing agent blobs, redownload missing/corrupt vendored artifacts (never re-synthesizing a lost ledger), and clean up unused ones (refuses with `lock_held` when a live process holds the lock; see "Lock lifecycle" below) | +| `repair` | — | Download missing agent blobs, redownload missing/corrupt vendored artifacts (never re-synthesizing a lost ledger), and clean up unused ones: a blob or diff archive no manifest patch references (the afterHash and beforeHash blobs and the diff archive of every manifest patch are kept, so an offline `rollback` still has its originals; `scan --prune` keeps the same set) (refuses with `lock_held` when a live process holds the lock; see "Lock lifecycle" below) | Rows are in `--help` order (v5.0): the hosted/vendored workflow (`scan` → `vex` → `vendor`, with `list` to inspect), then the agent-mode (in-place patching) commands. @@ -144,7 +144,7 @@ For a **9.0 root lock**, the CLI ensures `pnpm-workspace.yaml` carries `trustLoc **Agent-flow run-level warnings (additive).** An agent-mode apply (`--mode agent` / `--sync`, `--json`) may add a top-level `warnings[]` array of `{code, detail}` entries to the scan envelope (absent when none fired; each is also mirrored to stderr unless `--silent`). They surface cross-mode state the apply cannot change — never a status or exit-code change (hosted refusals set the precedent: exit 0 + warning). Codes (stable; new codes are additive/MINOR): `vendored_ownership_retained` — vendor-owned package(s) were skipped before download (the per-patch `skipped`/`vendored` records in `apply.patches[]` are unchanged); the detail names the purls and the migration path (`remove `, or `vendor --revert` which unwinds every vendored package, then re-run). `hosted_wiring_retained` — the lockfiles still pin scanned package(s) to a hosted patch (the agent run does not unwind hosted wiring — as of v5.0 that is `socket-patch rollback`'s job, which restores the upstream registry entries, or `remove ` per package); the detail names the purls and the options (stay `--mode hosted`, migrate via `scan --mode vendored`, or `socket-patch rollback`). The warning keys on the hosted pins lockfile discovery finds at scan time, so a flow that restored the upstream entries retires it. The human path prints the same `hosted_wiring_retained` text to stderr after an apply; the vendored counterpart is already covered by its per-package `[skip] … (vendored …)` lines. `ownership_not_restored` (v5.0; `apply` and `rollback` `warnings[]` alike) — a file WAS patched (or restored) but its ownership could not be put back to the original uid/gid (the mode is still restored last); the detail is `: : patched, but ownership could not be restored to uid N gid M: ` and the human line `Warning: ` (stderr, muted by `--silent`); never a status or exit change. -`scan --prune` opts into garbage collection. When set, `scan` removes manifest entries for packages no longer present in the crawl, then deletes orphan blob and diff-archive files, and every legacy package archive, from `.socket/`. Off by default (v3.0) so a temporary uninstall doesn't silently destroy manifest state. Only entries whose ecosystem this run actually crawled are eligible: a `pkg:/` with no crawler in this build (a newer CLI's ecosystem in the committed manifest) is exempt — the crawl never looked for them, so their absence is not evidence of removal (same fail-safe as the `--ecosystems` filter, which narrows the query but never the prune's installed set). The pass also reconciles vendored state (runs FIRST, under ONE apply-lock acquisition shared with the manifest prune — lock contention skips the whole pass without failing the scan; `--lock-timeout` is honored and a lock I/O error is reported rather than swallowed; the existence gate — a manifest file OR a vendor ledger file, both cheap stats; an emptied ledger is deleted on save, so its presence is its content proxy — runs BEFORE the lock, so a bare project never gets a `.socket/`; in the vendored scan arms the pass runs AFTER the vendor step): (a) ledger entries still tracked by a manifest record (manifest-mode entries written by standalone `vendor`) whose patch is gone from the manifest are reverted — `detached` entries (every `scan`/`get --mode vendored` entry, v5.0) have no manifest record to lose and are exempt from this leg; (b) EVERY ledger entry whose dependency is no longer in the lockfile graph is reverted and any manifest entry it still had dropped (v5.0: the check is about the lockfile, not the manifest, so embedded-record entries are no longer exempt; a missing or undeterminable lockfile keeps the entry, fail-safe); and (c) orphan `.socket/vendor//` dirs with no ledger entry are swept. The prune never deletes a zero-patch `.socket/manifest.json` (its `{"patches": {}}` + `setup` block stay). The JSON `gc` sub-object gains `revertedVendoredEntries` + `keptVendoredEntries` + `failedVendoredEntries` + `removedVendorOrphanDirs` (wet) / `revertableVendoredEntries` + `vendorOrphanDirs` (preview), plus two ADDITIVE wet-only keys: `skipped: {code, message}` — present exactly when the pass was skipped at the lock (`lock_held` | `lock_io`; every count is then zero) — and `warnings: [{code, detail}]` — `vendor_state_write_failed` / `manifest_write_failed` (entries were reverted but the ledger or manifest rewrite failed), `cleanup_failed` (an orphan sweep failed mid-way), and the reinstall advisories of the vendored reverts (`vendor_bun_reinstall_required`, `vendor_vlt_reinstall_required`; a revert's other warnings, such as `vendor_lock_entry_removed` or a drift keep's, are not repeated here). Human mode prints `GC: skipped (): .`, one `GC: .` line per warning, and `GC: failed to revert N vendored entries: …` (singular for one) for `failedVendoredEntries`. `keptVendoredEntries` lists drift-kept entries the revert deliberately preserved (`vendor_artifact_kept` — undo the drift and re-run `vendor --revert` to finish); the preview cannot see drift (backends return before the wiring replay on dry runs), so `revertableVendoredEntries` may over-promise what a wet run will actually reclaim. +`scan --prune` opts into garbage collection. When set, `scan` removes manifest entries for packages no longer present in the crawl, then deletes orphan blob and diff-archive files, and every legacy package archive, from `.socket/`. A file is an orphan when no patch left in the manifest references it: the afterHash and beforeHash blobs and the diff archive of every remaining patch are kept, the same retention policy `repair` uses (the beforeHash blobs are an offline rollback's only restore data, and `repair` downloads afterHash blobs only). Off by default (v3.0) so a temporary uninstall doesn't silently destroy manifest state. Only entries whose ecosystem this run actually crawled are eligible: a `pkg:/` with no crawler in this build (a newer CLI's ecosystem in the committed manifest) is exempt — the crawl never looked for them, so their absence is not evidence of removal (same fail-safe as the `--ecosystems` filter, which narrows the query but never the prune's installed set). The pass also reconciles vendored state (runs FIRST, under ONE apply-lock acquisition shared with the manifest prune — lock contention skips the whole pass without failing the scan; `--lock-timeout` is honored and a lock I/O error is reported rather than swallowed; the existence gate — a manifest file OR a vendor ledger file, both cheap stats; an emptied ledger is deleted on save, so its presence is its content proxy — runs BEFORE the lock, so a bare project never gets a `.socket/`; in the vendored scan arms the pass runs AFTER the vendor step): (a) ledger entries still tracked by a manifest record (manifest-mode entries written by standalone `vendor`) whose patch is gone from the manifest are reverted — `detached` entries (every `scan`/`get --mode vendored` entry, v5.0) have no manifest record to lose and are exempt from this leg; (b) EVERY ledger entry whose dependency is no longer in the lockfile graph is reverted and any manifest entry it still had dropped (v5.0: the check is about the lockfile, not the manifest, so embedded-record entries are no longer exempt; a missing or undeterminable lockfile keeps the entry, fail-safe); and (c) orphan `.socket/vendor//` dirs with no ledger entry are swept. The prune never deletes a zero-patch `.socket/manifest.json` (its `{"patches": {}}` + `setup` block stay). The JSON `gc` sub-object gains `revertedVendoredEntries` + `keptVendoredEntries` + `failedVendoredEntries` + `removedVendorOrphanDirs` (wet) / `revertableVendoredEntries` + `vendorOrphanDirs` (preview), plus two ADDITIVE wet-only keys: `skipped: {code, message}` — present exactly when the pass was skipped at the lock (`lock_held` | `lock_io`; every count is then zero) — and `warnings: [{code, detail}]` — `vendor_state_write_failed` / `manifest_write_failed` (entries were reverted but the ledger or manifest rewrite failed), `cleanup_failed` (an orphan sweep failed mid-way), and the reinstall advisories of the vendored reverts (`vendor_bun_reinstall_required`, `vendor_vlt_reinstall_required`; a revert's other warnings, such as `vendor_lock_entry_removed` or a drift keep's, are not repeated here). Human mode prints `GC: skipped (): .`, one `GC: .` line per warning, and `GC: failed to revert N vendored entries: …` (singular for one) for `failedVendoredEntries`. `keptVendoredEntries` lists drift-kept entries the revert deliberately preserved (`vendor_artifact_kept` — undo the drift and re-run `vendor --revert` to finish); the preview cannot see drift (backends return before the wiring replay on dry runs), so `revertableVendoredEntries` may over-promise what a wet run will actually reclaim. `scan` queries the patch API in `--batch-size` chunks. Authenticated runs POST `/v0/orgs/{slug}/patches/batch`; token-less runs POST `{proxy}/patch/batch` on the public proxy and degrade to per-package `GET /patch/by-package/:purl` requests in two cases: the deployed proxy predates the batch endpoint (legacy proxies answer the POST with their `400 "Unsupported endpoint"` catch-all), or the all-or-nothing batch validation rejects the chunk (e.g. a crawled PURL type the server doesn't recognize, such as `pkg:jsr/…` — the per-package path tolerates those individually, preserving the pre-batch scan semantics). Rate limits and over-capacity 503s surface instead of silently degrading. diff --git a/crates/socket-patch-cli/src/commands/repair.rs b/crates/socket-patch-cli/src/commands/repair.rs index 345dc9355..b4099672f 100644 --- a/crates/socket-patch-cli/src/commands/repair.rs +++ b/crates/socket-patch-cli/src/commands/repair.rs @@ -633,7 +633,7 @@ 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 = ArtifactReferences::for_apply(manifest) + let sweep = ArtifactReferences::active(manifest) .sweep(&socket_dir, args.common.dry_run) .await; let passes = [ diff --git a/crates/socket-patch-cli/src/commands/rollback.rs b/crates/socket-patch-cli/src/commands/rollback.rs index 02bcdf968..0feb1dfbb 100644 --- a/crates/socket-patch-cli/src/commands/rollback.rs +++ b/crates/socket-patch-cli/src/commands/rollback.rs @@ -655,7 +655,7 @@ fn skipped_not_installed_json(purl: &str) -> serde_json::Value { /// human path gets. One failed result per affected package keeps the /// `failed` counter meaning "packages that failed" (the same per-package /// semantics as a mid-run failure) and names each missing blob hash plus -/// the `socket-patch repair` remedy in machine-readable form, using the +/// the re-run remedy in machine-readable form, using the /// engine's own `missing_blob` verify vocabulary. `reason_for` renders /// the per-hash diagnostic (offline gate vs. download failure). fn missing_blob_abort_results( @@ -2415,7 +2415,7 @@ pub(crate) async fn rollback_patches_inner( "Error: {} missing and --offline is set.", plural(missing_blobs.len(), "blob is", "blobs are") ); - eprintln!("Run \"socket-patch repair\" to download missing blobs."); + eprintln!("Re-run without --offline to download the original blobs."); } let results = missing_blob_abort_results( &abort_manifest, @@ -2424,7 +2424,7 @@ pub(crate) async fn rollback_patches_inner( |hash| { format!( "Before blob not found: {hash} and --offline prevents fetching. \ - Run \"socket-patch repair\" to download missing blobs." + Re-run without --offline to download the original blobs." ) }, ); @@ -2505,7 +2505,7 @@ pub(crate) async fn rollback_patches_inner( .unwrap_or("download failed"); format!( "Before blob could not be downloaded: {hash} - {why}. \ - Run \"socket-patch repair\" to download missing blobs." + Re-run once the patch API is reachable to download the original blobs." ) }, ); diff --git a/crates/socket-patch-cli/src/commands/scan/gc.rs b/crates/socket-patch-cli/src/commands/scan/gc.rs index 38c102440..68fd41a4c 100644 --- a/crates/socket-patch-cli/src/commands/scan/gc.rs +++ b/crates/socket-patch-cli/src/commands/scan/gc.rs @@ -158,7 +158,7 @@ async fn run_gc( socket_dir: &Path, dry_run: bool, ) -> GcSummary { - let sweep = ArtifactReferences::for_apply(manifest) + let sweep = ArtifactReferences::active(manifest) .sweep(socket_dir, dry_run) .await; let mut warnings = Vec::new(); diff --git a/crates/socket-patch-cli/tests/remove_rollback_api_overrides.rs b/crates/socket-patch-cli/tests/remove_rollback_api_overrides.rs index 168a59cf2..55082f09e 100644 --- a/crates/socket-patch-cli/tests/remove_rollback_api_overrides.rs +++ b/crates/socket-patch-cli/tests/remove_rollback_api_overrides.rs @@ -242,9 +242,9 @@ fn remove_rollback_downloads_missing_blob_via_flag_overrides() { // The blob's on-disk lifecycle: it must have LANDED in .socket/blobs for // remove to proceed (the post-download `still_missing` re-check reads the // dir; exit 0 below is unreachable otherwise), and then remove's - // unused-blob sweep deletes it again — beforeHash blobs are by design - // downloaded on-demand and never retained (`cleanup_unused_blobs` keeps - // only afterHash blobs, and this patch was just removed anyway). + // unused-blob sweep deletes it again — this patch was just removed (and + // rolled back), so nothing references its original any more; only the + // blobs of patches still in the manifest are retained. assert!( !socket.join("blobs").join(&before_hash).exists(), "remove's unused-blob sweep must not retain the on-demand before-blob.\n\ diff --git a/crates/socket-patch-cli/tests/repair/repair_invariants.rs b/crates/socket-patch-cli/tests/repair/repair_invariants.rs index 365d063e5..a32776a8f 100644 --- a/crates/socket-patch-cli/tests/repair/repair_invariants.rs +++ b/crates/socket-patch-cli/tests/repair/repair_invariants.rs @@ -1094,3 +1094,94 @@ fn repair_silent_suppresses_human_stdout() { "silent repair must still perform cleanup (orphan should be gone)" ); } + +// --------------------------------------------------------------------------- +// Retention: one policy for every patch still in the manifest (#893) +// --------------------------------------------------------------------------- + +/// `repair` (alias `gc`) must keep the beforeHash blob of a patch that is +/// still in the manifest: it is the only local restore data, and `repair` +/// cannot download it again (its fetch covers afterHash blobs only). Before +/// #893 the sweep kept afterHash blobs only, so an offline rollback of a +/// still-active patch failed after any `repair` and told the user to run +/// `repair`. A beforeHash blob that only a manifest-absent patch referenced +/// is still collected. +#[test] +fn repair_keeps_active_patch_before_blob_so_offline_rollback_still_works() { + const ORIGINAL: &[u8] = b"module.exports = 'original';\n"; + const PATCHED: &[u8] = b"module.exports = 'patched';\n"; + let before = git_sha256(ORIGINAL); + let after = git_sha256(PATCHED); + let purl = "pkg:npm/repair-retention@1.0.0"; + + let tmp = tempfile::tempdir().expect("tempdir"); + let root = tmp.path(); + std::fs::write( + root.join("package.json"), + r#"{ "name": "repair-retention-root", "version": "0.0.0" }"#, + ) + .unwrap(); + let pkg = root.join("node_modules/repair-retention"); + std::fs::create_dir_all(&pkg).unwrap(); + std::fs::write( + pkg.join("package.json"), + r#"{ "name": "repair-retention", "version": "1.0.0" }"#, + ) + .unwrap(); + std::fs::write(pkg.join("index.js"), PATCHED).unwrap(); + + let socket = root.join(".socket"); + std::fs::create_dir_all(&socket).unwrap(); + let manifest = serde_json::json!({ + "patches": { + purl: { + "uuid": "22222222-2222-4222-8222-222222222222", + "exportedAt": "2024-01-01T00:00:00Z", + "files": { + "package/index.js": { "beforeHash": before, "afterHash": after } + }, + "vulnerabilities": {}, + "description": "retention test patch", + "license": "MIT", + "tier": "free" + } + } + }); + std::fs::write( + socket.join("manifest.json"), + serde_json::to_string_pretty(&manifest).unwrap(), + ) + .unwrap(); + // The layout `get` leaves: both blobs of the active patch. + write_blob(&socket, &before, ORIGINAL); + write_blob(&socket, &after, PATCHED); + // An original only a removed patch referenced is still garbage. + let stale_original = git_sha256(b"original of a removed patch\n"); + write_blob(&socket, &stale_original, b"original of a removed patch\n"); + + let (code, stdout) = run_repair(root, &[]); + assert_eq!(code, 0, "repair must succeed; stdout=\n{stdout}"); + assert!( + socket.join("blobs").join(&before).exists(), + "repair must keep the active patch's beforeHash blob; stdout=\n{stdout}" + ); + assert!(socket.join("blobs").join(&after).exists()); + assert!( + !socket.join("blobs").join(&stale_original).exists(), + "an original no manifest patch references is still swept" + ); + + let out = socket_cmd(root) + .args(["rollback", "--offline", "--json"]) + .output() + .expect("run socket-patch"); + let stdout = String::from_utf8_lossy(&out.stdout); + assert_eq!( + out.status.code(), + Some(0), + "offline rollback after repair must succeed; stdout=\n{stdout}" + ); + let v: serde_json::Value = serde_json::from_str(&stdout).expect("envelope JSON"); + assert_eq!(v["status"], "success", "stdout=\n{stdout}"); + assert_eq!(std::fs::read(pkg.join("index.js")).unwrap(), ORIGINAL); +} diff --git a/crates/socket-patch-cli/tests/rollback/rollback_invariants.rs b/crates/socket-patch-cli/tests/rollback/rollback_invariants.rs index 55760c8b2..690906f7a 100644 --- a/crates/socket-patch-cli/tests/rollback/rollback_invariants.rs +++ b/crates/socket-patch-cli/tests/rollback/rollback_invariants.rs @@ -197,8 +197,8 @@ fn rollback_offline_with_missing_before_blob_partial_failure() { // The error names the remedy; the per-file record names the blob. let err = entry["error"].as_str().expect("error message string"); assert!( - err.contains("socket-patch repair"), - "error must carry the repair remedy; got: {err}" + err.contains("Re-run without --offline") && !err.contains("repair"), + "error must carry the re-run remedy (repair cannot fetch originals, #893); got: {err}" ); let verified = entry["filesVerified"] .as_array() @@ -221,8 +221,8 @@ fn rollback_offline_with_missing_before_blob_partial_failure() { "message must name the missing hash; got: {msg}" ); assert!( - msg.contains("--offline") && msg.contains("socket-patch repair"), - "message must name the offline gate and the repair remedy; got: {msg}" + msg.contains("--offline") && msg.contains("Re-run without --offline"), + "message must name the offline gate and the re-run remedy; got: {msg}" ); } @@ -249,8 +249,8 @@ fn rollback_offline_missing_blob_human_names_package_and_remedy() { "stderr must explain the offline gate; stderr=\n{stderr}" ); assert!( - stderr.contains("socket-patch repair"), - "stderr must carry the repair remedy; stderr=\n{stderr}" + stderr.contains("Re-run without --offline") && !stderr.contains("socket-patch repair"), + "stderr must carry the re-run remedy; stderr=\n{stderr}" ); let stdout = String::from_utf8_lossy(&out.stdout); assert!( @@ -302,8 +302,8 @@ fn rollback_undownloadable_blob_envelope_names_blob_and_remedy() { assert_eq!(entry["success"], false); let err = entry["error"].as_str().expect("error message string"); assert!( - err.contains("socket-patch repair"), - "error must carry the repair remedy; got: {err}" + err.contains("patch API is reachable") && !err.contains("repair"), + "error must carry the re-run remedy; got: {err}" ); let verified = entry["filesVerified"] .as_array() @@ -560,10 +560,10 @@ fn rollback_mixed_installed_gated_and_not_installed_entries() { "the gated package is installed — path must be reported; stdout=\n{stdout}" ); // The pinned missing-blob abort envelope survives for the installed - // package: engine vocabulary + repair remedy. + // package: engine vocabulary + re-run remedy. let err = entry["error"].as_str().expect("error message string"); assert!( - err.contains("Cannot roll back: ") && err.contains("socket-patch repair"), + err.contains("Cannot roll back: ") && err.contains("Re-run without --offline"), "pinned abort error shape; got: {err}" ); let verified = entry["filesVerified"] diff --git a/crates/socket-patch-cli/tests/scan/scan_paths_e2e.rs b/crates/socket-patch-cli/tests/scan/scan_paths_e2e.rs index 8f7846315..322060998 100644 --- a/crates/socket-patch-cli/tests/scan/scan_paths_e2e.rs +++ b/crates/socket-patch-cli/tests/scan/scan_paths_e2e.rs @@ -355,6 +355,61 @@ async fn paths_never_narrow_the_prune_universe() { assert_eq!(v["paths"], serde_json::json!(["packages/app"])); } +/// `scan --prune` keeps the beforeHash blob of every patch still in the +/// manifest (#893): it is the only local restore data for an offline +/// rollback, and `repair` cannot download it again. Only the originals of +/// pruned patches are collected. +#[tokio::test] +async fn prune_keeps_before_blobs_of_active_patches() { + let server = MockServer::start().await; + mock_batch_empty(&server).await; + + let tmp = tempfile::tempdir().unwrap(); + write_root_package_json(tmp.path()); + write_npm_package_at(tmp.path(), "", "root-dep", "1.0.0"); + + let active_after = "a".repeat(64); + let active_before = "d".repeat(64); + let gone_after = "c".repeat(64); + let gone_before = "e".repeat(64); + let blobs: Vec = [&active_after, &active_before, &gone_after, &gone_before] + .iter() + .map(|h| stage_blob(tmp.path(), h)) + .collect(); + + let mut active = manifest_entry("11111111-1111-4111-8111-111111111111", &active_after); + active["files"]["package/index.js"]["beforeHash"] = active_before.clone().into(); + let mut gone = manifest_entry("33333333-3333-4333-8333-333333333333", &gone_after); + gone["files"]["package/index.js"]["beforeHash"] = gone_before.clone().into(); + let manifest = serde_json::json!({ + "patches": { ROOT_PURL: active, "pkg:npm/gone@9.9.9": gone } + }); + std::fs::write( + tmp.path().join(".socket/manifest.json"), + serde_json::to_string_pretty(&manifest).unwrap(), + ) + .unwrap(); + + let (code, stdout, stderr) = run_scan( + tmp.path(), + &server.uri(), + &["--mode", "agent", "--prune", "--yes"], + ); + assert_eq!(code, 0, "stdout={stdout}; stderr={stderr}"); + assert!(blobs[0].exists(), "active patch's afterHash blob survives"); + assert!( + blobs[1].exists(), + "active patch's beforeHash blob must survive --prune; stdout={stdout}" + ); + assert!(!blobs[2].exists(), "pruned patch's afterHash blob is swept"); + assert!( + !blobs[3].exists(), + "pruned patch's beforeHash blob is swept" + ); + let v = parse_envelope(&stdout); + assert_eq!(v["gc"]["removedBlobs"], 2, "got {v}"); +} + // --------------------------------------------------------------------------- // 3. A scope matching nothing is a normal empty scan. // --------------------------------------------------------------------------- diff --git a/crates/socket-patch-core/src/manifest/cleanup_blobs.rs b/crates/socket-patch-core/src/manifest/cleanup_blobs.rs index 1f003c937..19f1995a4 100644 --- a/crates/socket-patch-core/src/manifest/cleanup_blobs.rs +++ b/crates/socket-patch-core/src/manifest/cleanup_blobs.rs @@ -1,9 +1,8 @@ use std::collections::HashSet; use std::path::Path; -use crate::api::blob_fetcher::{ArtifactNoun, BLOB}; -use crate::manifest::operations::get_after_hash_blobs; -use crate::manifest::schema::PatchManifest; +use crate::api::blob_fetcher::ArtifactNoun; +use crate::manifest::schema::{PatchManifest, PatchRecord}; /// Result of a blob cleanup operation. #[derive(Debug, Default)] @@ -28,28 +27,37 @@ pub struct ArtifactReferences { } impl ArtifactReferences { - /// Repair and pruning retain the bytes needed to apply active patches. - pub fn for_apply(manifest: &PatchManifest) -> Self { - Self { - blobs: get_after_hash_blobs(manifest), - patch_uuids: manifest.patches.values().map(|r| r.uuid.clone()).collect(), + /// The one retention policy for a manifest's patches: the afterHash and + /// beforeHash blobs and the diff archive of every patch in it. `repair` + /// and `scan --prune` keep exactly this. The beforeHash blobs are the + /// only local restore data: an offline rollback needs them, and + /// `repair` downloads afterHash blobs only, so it can never restore an + /// original it swept. + pub fn active(manifest: &PatchManifest) -> Self { + let mut references = Self { + blobs: HashSet::new(), + patch_uuids: HashSet::new(), + }; + for record in manifest.patches.values() { + references.retain(record); } + references } - /// Remove and rollback also retain originals for every remaining patch - /// and removed-but-not-installed patch. A crawler miss must not destroy - /// the only local restore data. Other removed patches become collectible. + /// Remove and rollback keep [`Self::active`] for the remaining + /// manifest, plus the originals of every removed-but-not-installed + /// patch: a crawler miss must not destroy the only local restore data. + /// Other removed patches become collectible. pub fn after_removal<'a>( previous: &PatchManifest, remaining: &PatchManifest, removed_not_installed: impl IntoIterator, ) -> Self { - let mut references = Self::for_apply(remaining); - for record in remaining.patches.values().chain( - removed_not_installed - .into_iter() - .filter_map(|purl| previous.patches.get(purl)), - ) { + let mut references = Self::active(remaining); + for record in removed_not_installed + .into_iter() + .filter_map(|purl| previous.patches.get(purl)) + { let mut has_original = false; for file in record.files.values() { if !file.before_hash.is_empty() { @@ -64,6 +72,18 @@ impl ArtifactReferences { references } + fn retain(&mut self, record: &PatchRecord) { + for file in record.files.values() { + for hash in [&file.after_hash, &file.before_hash] { + // Empty beforeHash is the created-by-patch sentinel. + if !hash.is_empty() { + self.blobs.insert(hash.clone()); + } + } + } + self.patch_uuids.insert(record.uuid.clone()); + } + /// Sweep each artifact directory independently so a failed pass does not /// stop another. Callers report partial counts and cleanup warnings. pub async fn sweep(&self, socket_dir: &Path, dry_run: bool) -> ArtifactSweep { @@ -86,7 +106,7 @@ pub struct ArtifactSweep { pub packages: std::io::Result, } -/// Shared core for `cleanup_unused_blobs` / `cleanup_unused_archives`. +/// Shared core of every [`ArtifactReferences::sweep`] pass. /// /// Walks `dir`, treats it as authoritative socket-patch state (so any /// regular non-hidden file is considered for removal), and asks @@ -168,42 +188,6 @@ async fn cleanup_dir bool>( Ok(result) } -/// Cleans up unused blob files from the blobs directory. -/// -/// Analyzes the manifest to determine which afterHash blobs are needed for applying patches, -/// then removes any blob files that are not needed. -/// -/// Note: beforeHash blobs are considered "unused" because they are downloaded on-demand -/// during rollback operations. This saves disk space since beforeHash blobs are only -/// needed for rollback, not for applying patches. -pub async fn cleanup_unused_blobs( - manifest: &PatchManifest, - blobs_dir: &Path, - dry_run: bool, -) -> Result { - // Only keep afterHash blobs - beforeHash blobs are downloaded on-demand during rollback - let used_blobs = get_after_hash_blobs(manifest); - cleanup_dir(blobs_dir, dry_run, |name| used_blobs.contains(name)).await -} - -/// Cleans up unused per-patch archive files from `archives_dir`. -/// -/// Archives are named `.tar.gz`. Any file matching that -/// pattern whose UUID is not present in the manifest is removed. Files -/// that do *not* end in `.tar.gz` are treated as orphans and also -/// removed — these directories are managed exclusively by socket-patch, -/// so any stray non-archive file is assumed to be left over from an -/// older socket-patch version. Subdirectories and hidden files are -/// left untouched. -pub async fn cleanup_unused_archives( - manifest: &PatchManifest, - archives_dir: &Path, - dry_run: bool, -) -> Result { - let used_uuids: HashSet = manifest.patches.values().map(|r| r.uuid.clone()).collect(); - cleanup_archives(&used_uuids, archives_dir, dry_run).await -} - async fn cleanup_archives( used_uuids: &HashSet, archives_dir: &Path, @@ -224,12 +208,6 @@ async fn cleanup_archives( .await } -/// Formats a blob cleanup result for human-readable output (see -/// [`format_cleanup_result_for`]). -pub fn format_cleanup_result(result: &CleanupResult, dry_run: bool) -> String { - format_cleanup_result_for(result, dry_run, BLOB) -} - /// Formats a cleanup result counting `noun`s: "Removed 2 unused diff /// archives (3 B freed)", and under a dry run the sorted list of what /// would go ("Unused diff archives:" then ` - ` lines; the @@ -307,6 +285,7 @@ pub fn format_bytes(bytes: u64) -> String { #[cfg(test)] mod tests { use super::*; + use crate::api::blob_fetcher::BLOB; use crate::manifest::schema::{PatchFileInfo, PatchManifest, PatchRecord}; use std::collections::HashMap; @@ -317,6 +296,26 @@ mod tests { const AFTER_HASH_2: &str = "dddddddddddddddddddddddddddddddddddddddddddddddddddddddddddd2222"; const ORPHAN_HASH: &str = "oooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo"; + /// The blob pass of [`ArtifactReferences::active`]'s sweep alone. + async fn sweep_blobs( + manifest: &PatchManifest, + dir: &Path, + dry_run: bool, + ) -> std::io::Result { + let references = ArtifactReferences::active(manifest); + cleanup_dir(dir, dry_run, |name| references.blobs.contains(name)).await + } + + /// The diff-archive pass of [`ArtifactReferences::active`]'s sweep alone. + async fn sweep_archives( + manifest: &PatchManifest, + dir: &Path, + dry_run: bool, + ) -> std::io::Result { + let references = ArtifactReferences::active(manifest); + cleanup_archives(&references.patch_uuids, dir, dry_run).await + } + fn create_test_manifest() -> PatchManifest { let mut files = HashMap::new(); files.insert( @@ -357,7 +356,7 @@ mod tests { #[tokio::test] async fn artifact_retention_covers_active_removed_and_uninstalled_patches() { for policy in [ - "apply", + "active", "remaining", "not-installed", "created-only", @@ -383,7 +382,7 @@ mod tests { } let empty = PatchManifest::default(); let references = match policy { - "apply" => ArtifactReferences::for_apply(&manifest), + "active" => ArtifactReferences::active(&manifest), "remaining" => ArtifactReferences::after_removal(&manifest, &manifest, []), "removed" => ArtifactReferences::after_removal(&manifest, &empty, []), _ => ArtifactReferences::after_removal(&manifest, &empty, ["missing-purl", purl]), @@ -409,8 +408,8 @@ mod tests { std::fs::write(diffs.join(&archive), b"diff").unwrap(); std::fs::write(diffs.join("orphan.tar.gz"), b"orphan").unwrap(); std::fs::write(packages.join(&archive), b"legacy").unwrap(); - let keep_original = matches!(policy, "remaining" | "not-installed"); - let keep_patched = matches!(policy, "apply" | "remaining"); + let keep_original = matches!(policy, "active" | "remaining" | "not-installed"); + let keep_patched = matches!(policy, "active" | "remaining"); let kept = 2 * usize::from(keep_original) + 3 * usize::from(keep_patched); let keep_archive = keep_original || keep_patched; @@ -471,9 +470,7 @@ mod tests { .await .unwrap(); - let result = cleanup_unused_blobs(&manifest, &blobs_dir, false) - .await - .unwrap(); + let result = sweep_blobs(&manifest, &blobs_dir, false).await.unwrap(); // Should remove only the orphan blob assert_eq!(result.blobs_removed, 1); @@ -493,52 +490,34 @@ mod tests { .is_err()); } + /// #893: the beforeHash blobs of a patch still in the manifest are its + /// only local restore data, so the sweep keeps them beside the + /// afterHash blobs; only unreferenced files go. #[tokio::test] - async fn test_cleanup_removes_before_hash_blobs() { + async fn test_cleanup_keeps_before_hash_blobs_of_active_patches() { let dir = tempfile::tempdir().unwrap(); let blobs_dir = dir.path().join("blobs"); tokio::fs::create_dir_all(&blobs_dir).await.unwrap(); let manifest = create_test_manifest(); + for hash in [ + BEFORE_HASH_1, + BEFORE_HASH_2, + AFTER_HASH_1, + AFTER_HASH_2, + ORPHAN_HASH, + ] { + tokio::fs::write(blobs_dir.join(hash), hash).await.unwrap(); + } - // Create both beforeHash and afterHash blobs - tokio::fs::write(blobs_dir.join(BEFORE_HASH_1), "before content 1") - .await - .unwrap(); - tokio::fs::write(blobs_dir.join(BEFORE_HASH_2), "before content 2") - .await - .unwrap(); - tokio::fs::write(blobs_dir.join(AFTER_HASH_1), "after content 1") - .await - .unwrap(); - tokio::fs::write(blobs_dir.join(AFTER_HASH_2), "after content 2") - .await - .unwrap(); - - let result = cleanup_unused_blobs(&manifest, &blobs_dir, false) - .await - .unwrap(); - - // Should remove the beforeHash blobs - assert_eq!(result.blobs_removed, 2); - assert!(result.removed_blobs.contains(&BEFORE_HASH_1.to_string())); - assert!(result.removed_blobs.contains(&BEFORE_HASH_2.to_string())); - - // afterHash blobs should still exist - assert!(tokio::fs::metadata(blobs_dir.join(AFTER_HASH_1)) - .await - .is_ok()); - assert!(tokio::fs::metadata(blobs_dir.join(AFTER_HASH_2)) - .await - .is_ok()); + let result = sweep_blobs(&manifest, &blobs_dir, false).await.unwrap(); - // beforeHash blobs should be removed - assert!(tokio::fs::metadata(blobs_dir.join(BEFORE_HASH_1)) - .await - .is_err()); - assert!(tokio::fs::metadata(blobs_dir.join(BEFORE_HASH_2)) - .await - .is_err()); + assert_eq!(result.blobs_removed, 1); + assert_eq!(result.removed_blobs, vec![ORPHAN_HASH.to_string()]); + for hash in [BEFORE_HASH_1, BEFORE_HASH_2, AFTER_HASH_1, AFTER_HASH_2] { + assert!(blobs_dir.join(hash).exists(), "{hash} must be kept"); + } + assert!(!blobs_dir.join(ORPHAN_HASH).exists()); } #[tokio::test] @@ -549,23 +528,21 @@ mod tests { let manifest = create_test_manifest(); - tokio::fs::write(blobs_dir.join(BEFORE_HASH_1), "before content 1") + tokio::fs::write(blobs_dir.join(ORPHAN_HASH), "orphan content") .await .unwrap(); tokio::fs::write(blobs_dir.join(AFTER_HASH_1), "after content 1") .await .unwrap(); - let result = cleanup_unused_blobs(&manifest, &blobs_dir, true) - .await - .unwrap(); + let result = sweep_blobs(&manifest, &blobs_dir, true).await.unwrap(); - // Should report beforeHash as would-be-removed + // Should report the orphan as would-be-removed assert_eq!(result.blobs_removed, 1); - assert!(result.removed_blobs.contains(&BEFORE_HASH_1.to_string())); + assert!(result.removed_blobs.contains(&ORPHAN_HASH.to_string())); // But both blobs should still exist - assert!(tokio::fs::metadata(blobs_dir.join(BEFORE_HASH_1)) + assert!(tokio::fs::metadata(blobs_dir.join(ORPHAN_HASH)) .await .is_ok()); assert!(tokio::fs::metadata(blobs_dir.join(AFTER_HASH_1)) @@ -574,7 +551,7 @@ mod tests { // A dry run never touches the directory either, even when every // file in it would go. - let result = cleanup_unused_blobs(&PatchManifest::new(), &blobs_dir, true) + let result = sweep_blobs(&PatchManifest::new(), &blobs_dir, true) .await .unwrap(); assert_eq!(result.blobs_removed, 2); @@ -596,9 +573,7 @@ mod tests { .await .unwrap(); - let result = cleanup_unused_blobs(&manifest, &blobs_dir, false) - .await - .unwrap(); + let result = sweep_blobs(&manifest, &blobs_dir, false).await.unwrap(); assert_eq!(result.blobs_removed, 2); // A wet sweep that orphaned everything leaves no empty `blobs/` husk @@ -635,7 +610,7 @@ mod tests { return; } - let result = cleanup_unused_blobs(&create_test_manifest(), &blobs_dir, false).await; + let result = sweep_blobs(&PatchManifest::new(), &blobs_dir, false).await; std::fs::set_permissions(&blobs_dir, std::fs::Permissions::from_mode(0o755)).unwrap(); let result = result.expect("unlink failures do not fail the pass"); @@ -677,9 +652,7 @@ mod tests { let manifest = create_test_manifest(); - let result = cleanup_unused_blobs(&manifest, &non_existent, false) - .await - .unwrap(); + let result = sweep_blobs(&manifest, &non_existent, false).await.unwrap(); assert_eq!(result.blobs_checked, 0); assert_eq!(result.blobs_removed, 0); @@ -709,7 +682,7 @@ mod tests { ..Default::default() }; assert_eq!( - format_cleanup_result(&result, false), + format_cleanup_result_for(&result, false, BLOB), "No blobs to clean up." ); } @@ -724,7 +697,7 @@ mod tests { ..Default::default() }; assert_eq!( - format_cleanup_result(&result, false), + format_cleanup_result_for(&result, false, BLOB), "Checked 5 blobs: all in use." ); } @@ -739,7 +712,7 @@ mod tests { ..Default::default() }; assert_eq!( - format_cleanup_result(&result, false), + format_cleanup_result_for(&result, false, BLOB), "Removed 2 unused blobs (2.00 KB freed)" ); } @@ -762,9 +735,7 @@ mod tests { .await .unwrap(); - let result = cleanup_unused_archives(&manifest, &archives, false) - .await - .unwrap(); + let result = sweep_archives(&manifest, &archives, false).await.unwrap(); assert_eq!(result.blobs_removed, 1); assert!(result @@ -793,9 +764,7 @@ mod tests { .await .unwrap(); - let result = cleanup_unused_archives(&manifest, &archives, true) - .await - .unwrap(); + let result = sweep_archives(&manifest, &archives, true).await.unwrap(); assert_eq!(result.blobs_removed, 1); assert!( @@ -822,9 +791,7 @@ mod tests { .await .unwrap(); - let result = cleanup_unused_archives(&manifest, &archives, false) - .await - .unwrap(); + let result = sweep_archives(&manifest, &archives, false).await.unwrap(); assert_eq!(result.blobs_removed, 1); assert!(result.removed_blobs.contains(&"stray.txt".to_string())); @@ -851,9 +818,7 @@ mod tests { .await .unwrap(); - let result = cleanup_unused_archives(&manifest, &archives, false) - .await - .unwrap(); + let result = sweep_archives(&manifest, &archives, false).await.unwrap(); assert_eq!(result.blobs_removed, 1); assert!(result.removed_blobs.contains(&TEST_UUID.to_string())); @@ -878,9 +843,7 @@ mod tests { .await .unwrap(); - let result = cleanup_unused_archives(&manifest, &archives, false) - .await - .unwrap(); + let result = sweep_archives(&manifest, &archives, false).await.unwrap(); assert_eq!(result.blobs_removed, 1); assert!(result @@ -894,9 +857,7 @@ mod tests { let archives = dir.path().join("does-not-exist"); let manifest = create_test_manifest(); - let result = cleanup_unused_archives(&manifest, &archives, false) - .await - .unwrap(); + let result = sweep_archives(&manifest, &archives, false).await.unwrap(); assert_eq!(result.blobs_checked, 0); assert_eq!(result.blobs_removed, 0); } @@ -924,9 +885,7 @@ mod tests { .await .unwrap(); - let result = cleanup_unused_blobs(&manifest, &blobs_dir, false) - .await - .unwrap(); + let result = sweep_blobs(&manifest, &blobs_dir, false).await.unwrap(); // Only the single regular, non-hidden file is checked; nothing removed. assert_eq!(result.blobs_checked, 1); @@ -946,7 +905,7 @@ mod tests { let blobs_dir = dir.path().join("blobs"); tokio::fs::create_dir_all(&blobs_dir).await.unwrap(); - let result = cleanup_unused_blobs(&create_test_manifest(), &blobs_dir, false) + let result = sweep_blobs(&create_test_manifest(), &blobs_dir, false) .await .unwrap(); @@ -980,9 +939,7 @@ mod tests { ) .unwrap(); - let result = cleanup_unused_blobs(&manifest, &blobs_dir, false) - .await - .unwrap(); + let result = sweep_blobs(&manifest, &blobs_dir, false).await.unwrap(); // The orphan is removed; the symlink is counted as neither checked nor // removed (it is not a regular file) and is left in place. @@ -1014,9 +971,7 @@ mod tests { tokio::fs::write(&outside, vec![0u8; 4096]).await.unwrap(); symlink(&outside, blobs_dir.join("link-to-outside")).unwrap(); - let result = cleanup_unused_blobs(&manifest, &blobs_dir, false) - .await - .unwrap(); + let result = sweep_blobs(&manifest, &blobs_dir, false).await.unwrap(); assert_eq!(result.blobs_checked, 0); assert_eq!(result.blobs_removed, 0); @@ -1056,9 +1011,7 @@ mod tests { let bad_path = blobs_dir.join(OsStr::from_bytes(b"orphan-\xff\xfe")); tokio::fs::write(&bad_path, "junk").await.unwrap(); - let result = cleanup_unused_blobs(&manifest, &blobs_dir, false) - .await - .unwrap(); + let result = sweep_blobs(&manifest, &blobs_dir, false).await.unwrap(); assert_eq!(result.blobs_checked, 1); assert_eq!(result.blobs_removed, 1); @@ -1074,7 +1027,7 @@ mod tests { removed_blobs: vec!["aaa".to_string(), "bbb".to_string()], ..Default::default() }; - let formatted = format_cleanup_result(&result, true); + let formatted = format_cleanup_result_for(&result, true, BLOB); assert_eq!( formatted, "Would remove 2 unused blobs (2.00 KB freed)\nUnused blobs:\n - aaa\n - bbb" @@ -1113,7 +1066,7 @@ mod tests { ..Default::default() }; assert_eq!( - format_cleanup_result(&one_in_use, false), + format_cleanup_result_for(&one_in_use, false, BLOB), "Checked 1 blob: in use." ); // Unsorted input (directory-walk order) prints sorted. @@ -1125,7 +1078,7 @@ mod tests { ..Default::default() }; assert_eq!( - format_cleanup_result(&result, true), + format_cleanup_result_for(&result, true, BLOB), "Would remove 3 unused blobs (3 B freed)\nUnused blobs:\n - a\n - b\n - c" ); } From c004390497c720401272945ed286d26bf13cb222 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Fri, 9 Oct 2026 13:46:20 -0400 Subject: [PATCH 3/3] Update repair output tests for kept originals The human repair output tests assumed repair sweeps the active patch's beforeHash blob. It now keeps it (#893), so the no-orphan case checks both blobs in use and the orphan case removes only the orphan. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../tests/cli/output_modes_e2e.rs | 40 +++++++++---------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/crates/socket-patch-cli/tests/cli/output_modes_e2e.rs b/crates/socket-patch-cli/tests/cli/output_modes_e2e.rs index 3ea873f35..8005ef442 100644 --- a/crates/socket-patch-cli/tests/cli/output_modes_e2e.rs +++ b/crates/socket-patch-cli/tests/cli/output_modes_e2e.rs @@ -314,40 +314,36 @@ fn scan_non_json_no_packages_prints_friendly_message() { fn repair_non_json_no_orphans_prints_summary() { let tmp = tempfile::tempdir().unwrap(); write_manifest(tmp.path(), "pkg:npm/repair-target@1.0.0", b"a", b"b"); - // `write_manifest` writes BOTH the beforeHash and afterHash blobs, but - // repair treats `beforeHash` blobs as unused-by-design (they are fetched - // on demand during rollback). To exercise the genuine "all in use" path - // implied by this test's name, drop the beforeHash blob so the only - // remaining blob is the in-use afterHash one. + // `write_manifest` writes BOTH the beforeHash and afterHash blobs of + // the active patch, and repair keeps both (#893: the beforeHash blob is + // an offline rollback's only restore data). let blobs = tmp.path().join(".socket/blobs"); let before_blob = blobs.join(git_sha256(b"a")); let after_blob = blobs.join(git_sha256(b"b")); - std::fs::remove_file(&before_blob).unwrap(); assert!( - after_blob.exists(), - "fixture precondition: afterHash blob present" + before_blob.exists() && after_blob.exists(), + "fixture precondition: both blobs present" ); let (code, stdout, _stderr) = common::run_with_env(tmp.path(), &["repair", "--offline"], &[]); assert_eq!(code, 0); - // With exactly one in-use blob and no orphans, repair must report the + // With two in-use blobs and no orphans, repair must report the // all-in-use status (not a removal) and finish. The old check accepted // any output containing "Repair complete.", so a repair that wrongly // deleted the in-use blob — or skipped the cleanup scan entirely — still // passed. assert!( - stdout.contains("Checked 1 blob: in use."), - "no-orphan repair must report the single blob as in-use; got: {stdout}" + stdout.contains("Checked 2 blobs: all in use."), + "no-orphan repair must report both blobs as in use; got: {stdout}" ); assert!( stdout.contains("Repair complete."), "non-JSON repair should print the completion summary; got: {stdout}" ); - // Critically: the in-use afterHash blob (the patched file content that - // `apply` needs) must NOT be deleted by repair. + // Critically: neither in-use blob may be deleted by repair. assert!( - after_blob.exists(), - "repair must preserve the in-use afterHash blob" + after_blob.exists() && before_blob.exists(), + "repair must preserve the active patch's afterHash and beforeHash blobs" ); } @@ -370,12 +366,16 @@ fn repair_non_json_with_orphans_prints_cleanup_summary() { assert_eq!(code, 0); // The test name promises a *cleanup* summary, so assert the cleanup // actually happened — both in the printed summary and on disk. Pin the - // exact count (the orphan blob + the by-design-unused beforeHash blob = - // 2) so a repair that removes too few OR too many blobs fails here; the - // old `contains("Removed")` accepted any nonzero count. + // exact count (only the orphan: the active patch's beforeHash blob is + // kept, #893) so a repair that removes too few OR too many blobs fails + // here; the old `contains("Removed")` accepted any nonzero count. + assert!( + stdout.contains("Removed 1 unused blob ("), + "repair with orphans must report exactly 1 removed unused blob; got: {stdout}" + ); assert!( - stdout.contains("Removed 2 unused blobs"), - "repair with orphans must report exactly 2 removed unused blobs; got: {stdout}" + blobs.join(git_sha256(b"a")).exists(), + "repair must keep the active patch's beforeHash blob" ); assert!( !orphan.exists(),