Skip to content

Commit 63f3471

Browse files
cursoragentcursoragent
committed
Fix blob gates to check all package copies, not just first
The before-blob gate (rollback) and after-blob gate (apply) were only checking the first physical copy of each PURL, while the actual rollback/apply loops process ALL copies (nested dupes, diamonds, file: dups). This meant a later copy could need a blob even when the first copy was already in the correct state, causing the blob to not be fetched and the later copy to fail. Fixed by: - Updated mismatch_blob_gaps() to accept HashMap<String, Vec<PathBuf>> and iterate over all paths for each PURL - Updated rollback blob gate to iterate over all paths in rollback_targets instead of just using all_packages (first path only) - Updated all test fixtures to use vec![path] instead of single path Need-blob is per on-disk state, not per hash.
1 parent 063e79a commit 63f3471

2 files changed

Lines changed: 64 additions & 56 deletions

File tree

‎crates/socket-patch-cli/src/commands/apply.rs‎

Lines changed: 41 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ fn warn_mismatch_overwrites(result: &ApplyResult, common: &GlobalArgs) {
6868
async fn ensure_blobs_for_mismatches(
6969
args: &ApplyArgs,
7070
manifest: &PatchManifest,
71-
all_packages: &HashMap<String, PathBuf>,
71+
all_packages: &HashMap<String, Vec<PathBuf>>,
7272
vendored_purls: &HashSet<String>,
7373
staged: &mut StagedSources,
7474
) {
@@ -142,13 +142,13 @@ async fn ensure_blobs_for_mismatches(
142142
/// queue fetches either.
143143
async fn mismatch_blob_gaps(
144144
manifest: &PatchManifest,
145-
all_packages: &HashMap<String, PathBuf>,
145+
all_packages: &HashMap<String, Vec<PathBuf>>,
146146
vendored_purls: &HashSet<String>,
147147
blobs_path: &Path,
148148
force: bool,
149149
) -> HashSet<String> {
150150
let mut needed: HashSet<String> = HashSet::new();
151-
for (purl, pkg_path) in all_packages {
151+
for (purl, pkg_paths) in all_packages {
152152
let variant_eco = Ecosystem::from_purl(purl).is_some_and(|e| e.supports_release_variants());
153153
let stripped = strip_purl_qualifiers(purl);
154154
let records: Vec<(&String, &PatchRecord)> = manifest
@@ -164,34 +164,39 @@ async fn mismatch_blob_gaps(
164164
{
165165
continue;
166166
}
167-
let gated = variant_eco
168-
&& !force
169-
&& (records.len() > 1
170-
|| records
171-
.first()
172-
.is_some_and(|(key, _)| key.as_str() != stripped));
173-
for (_, record) in records {
174-
if gated {
175-
if let Some((file_name, file_info)) = representative_file(&record.files) {
176-
let status = verify_file_patch(pkg_path, file_name, file_info)
177-
.await
178-
.status;
179-
if !variant_matches_installed(Some(&status)) {
180-
continue;
167+
// Check every copy: each copy's on-disk state is independent, so a
168+
// nested copy can have a HashMismatch even when the root copy is
169+
// Ready/AlreadyPatched. Need-blob is per copy, not per hash.
170+
for pkg_path in pkg_paths {
171+
let gated = variant_eco
172+
&& !force
173+
&& (records.len() > 1
174+
|| records
175+
.first()
176+
.is_some_and(|(key, _)| key.as_str() != stripped));
177+
for (_, record) in &records {
178+
if gated {
179+
if let Some((file_name, file_info)) = representative_file(&record.files) {
180+
let status = verify_file_patch(pkg_path, file_name, file_info)
181+
.await
182+
.status;
183+
if !variant_matches_installed(Some(&status)) {
184+
continue;
185+
}
181186
}
182187
}
183-
}
184-
for (file_name, info) in &record.files {
185-
if info.before_hash.is_empty() {
186-
continue;
187-
}
188-
let verify = verify_file_patch(pkg_path, file_name, info).await;
189-
if verify.status == VerifyStatus::HashMismatch
190-
&& tokio::fs::metadata(blobs_path.join(&info.after_hash))
191-
.await
192-
.is_err()
193-
{
194-
needed.insert(info.after_hash.clone());
188+
for (file_name, info) in &record.files {
189+
if info.before_hash.is_empty() {
190+
continue;
191+
}
192+
let verify = verify_file_patch(pkg_path, file_name, info).await;
193+
if verify.status == VerifyStatus::HashMismatch
194+
&& tokio::fs::metadata(blobs_path.join(&info.after_hash))
195+
.await
196+
.is_err()
197+
{
198+
needed.insert(info.after_hash.clone());
199+
}
195200
}
196201
}
197202
}
@@ -1083,14 +1088,6 @@ async fn apply_patches_inner(
10831088
)
10841089
.await;
10851090

1086-
// One representative path per PURL, for the mismatch-blob gate and the
1087-
// pre-attempt checks that only need "is it installed / a sample copy"
1088-
// (the gate's fetch decision is per-hash, identical across copies).
1089-
let first_paths: HashMap<String, PathBuf> = all_packages
1090-
.iter()
1091-
.filter_map(|(purl, paths)| paths.first().map(|p| (purl.clone(), p.clone())))
1092-
.collect();
1093-
10941091
if all_packages.is_empty() && partitioned.is_empty() {
10951092
// Nothing in scope: the manifest lists no patches (or every patch was
10961093
// filtered out by `--ecosystems`). There is genuinely no work to do,
@@ -1128,7 +1125,7 @@ async fn apply_patches_inner(
11281125
}
11291126

11301127
// Apply patches
1131-
ensure_blobs_for_mismatches(args, &manifest, &first_paths, &vendored_purls, &mut staged).await;
1128+
ensure_blobs_for_mismatches(args, &manifest, &all_packages, &vendored_purls, &mut staged).await;
11321129
let sources = staged.as_patch_sources();
11331130
let policy = mismatch_policy(args.force, args.common.strict);
11341131
let mut has_errors = false;
@@ -1751,7 +1748,7 @@ mod tests {
17511748
files,
17521749
);
17531750
let mut all_packages = HashMap::new();
1754-
all_packages.insert("pkg:pypi/foo@1.0.0".to_string(), pkg.clone());
1751+
all_packages.insert("pkg:pypi/foo@1.0.0".to_string(), vec![pkg.clone()]);
17551752

17561753
let needed =
17571754
mismatch_blob_gaps(&manifest, &all_packages, &HashSet::new(), &blobs, false).await;
@@ -1821,7 +1818,7 @@ mod tests {
18211818
},
18221819
);
18231820
let mut all_packages = HashMap::new();
1824-
all_packages.insert("pkg:pypi/foo@1.0.0".to_string(), pkg.clone());
1821+
all_packages.insert("pkg:pypi/foo@1.0.0".to_string(), vec![pkg.clone()]);
18251822

18261823
let needed =
18271824
mismatch_blob_gaps(&manifest, &all_packages, &HashSet::new(), &blobs, false).await;
@@ -1867,7 +1864,7 @@ mod tests {
18671864
);
18681865
let manifest = manifest_with_record("pkg:gem/foo@1.0.0", files);
18691866
let mut all_packages = HashMap::new();
1870-
all_packages.insert("pkg:gem/foo@1.0.0".to_string(), pkg.clone());
1867+
all_packages.insert("pkg:gem/foo@1.0.0".to_string(), vec![pkg.clone()]);
18711868

18721869
let needed =
18731870
mismatch_blob_gaps(&manifest, &all_packages, &HashSet::new(), &blobs, false).await;
@@ -1907,7 +1904,7 @@ mod tests {
19071904
);
19081905
let manifest = manifest_with_record("pkg:gem/foo@1.0.0?platform=x86_64-linux", files);
19091906
let mut all_packages = HashMap::new();
1910-
all_packages.insert("pkg:gem/foo@1.0.0".to_string(), pkg.clone());
1907+
all_packages.insert("pkg:gem/foo@1.0.0".to_string(), vec![pkg.clone()]);
19111908

19121909
let needed =
19131910
mismatch_blob_gaps(&manifest, &all_packages, &HashSet::new(), &blobs, false).await;
@@ -1954,7 +1951,7 @@ mod tests {
19541951
);
19551952
let manifest = manifest_with_record("pkg:gem/foo@1.0.0", files);
19561953
let mut all_packages = HashMap::new();
1957-
all_packages.insert("pkg:gem/foo@1.0.0".to_string(), pkg.clone());
1954+
all_packages.insert("pkg:gem/foo@1.0.0".to_string(), vec![pkg.clone()]);
19581955

19591956
let needed =
19601957
mismatch_blob_gaps(&manifest, &all_packages, &HashSet::new(), &blobs, false).await;
@@ -2002,7 +1999,7 @@ mod tests {
20021999
);
20032000
let manifest = manifest_with_record("pkg:npm/foo@1.0.0", files);
20042001
let mut all_packages = HashMap::new();
2005-
all_packages.insert("pkg:npm/foo@1.0.0".to_string(), pkg.clone());
2002+
all_packages.insert("pkg:npm/foo@1.0.0".to_string(), vec![pkg.clone()]);
20062003

20072004
let needed =
20082005
mismatch_blob_gaps(&manifest, &all_packages, &HashSet::new(), &blobs, false).await;

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

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -742,9 +742,9 @@ async fn rollback_patches_inner(
742742
)
743743
.await;
744744

745-
// One representative path per PURL for the "is it installed" checks and
746-
// the before-blob gate (the gate's fetch decision is per-hash, identical
747-
// across copies). The per-copy restore uses `all_packages_multi`.
745+
// One representative path per PURL for the "is it installed" checks.
746+
// The before-blob gate now checks every copy (need-blob is per on-disk
747+
// state, not per hash), so it uses `rollback_targets` directly.
748748
let all_packages: HashMap<String, PathBuf> = all_packages_multi
749749
.iter()
750750
.filter_map(|(purl, paths)| paths.first().map(|p| (purl.clone(), p.clone())))
@@ -931,22 +931,33 @@ async fn rollback_patches_inner(
931931
// it must not abort the run or trigger a download; the rollback loop's
932932
// own per-file verification still reports those states honestly
933933
// (already_original / not_found / hash_mismatch).
934+
//
935+
// Check every copy: each copy's on-disk state is independent, so a
936+
// nested copy can be patched while the root copy is already original.
937+
// Need-blob is per copy, not per hash — the gate must probe every
938+
// physical copy that will be attempted, or a later copy can be missing
939+
// its blob even when an online run fetches for the first copy.
934940
let absent_blobs = get_missing_before_blobs(&gate_manifest, &blobs_path).await;
935941
let mut missing_blobs: HashSet<String> = HashSet::new();
936942
let mut blob_gated_purls: HashSet<String> = HashSet::new();
937943
if !absent_blobs.is_empty() {
938944
for (purl, patch) in &gate_manifest.patches {
939-
let pkg_path = all_packages
940-
.get(purl)
941-
.expect("gate manifest holds only attempted targets, which the crawler discovered");
942-
for (file, info) in &patch.files {
943-
if info.before_hash.is_empty() || !absent_blobs.contains(&info.before_hash) {
945+
// Probe every copy of this PURL that will be attempted. The
946+
// rollback loop below restores every copy in `rollback_targets`,
947+
// so the gate must verify all of them too.
948+
for (target_purl, pkg_path) in &rollback_targets {
949+
if *target_purl != purl {
944950
continue;
945951
}
946-
let v = verify_file_rollback(pkg_path, file, info, &blobs_path).await;
947-
if v.status == VerifyRollbackStatus::MissingBlob {
948-
missing_blobs.insert(info.before_hash.clone());
949-
blob_gated_purls.insert(purl.clone());
952+
for (file, info) in &patch.files {
953+
if info.before_hash.is_empty() || !absent_blobs.contains(&info.before_hash) {
954+
continue;
955+
}
956+
let v = verify_file_rollback(pkg_path, file, info, &blobs_path).await;
957+
if v.status == VerifyRollbackStatus::MissingBlob {
958+
missing_blobs.insert(info.before_hash.clone());
959+
blob_gated_purls.insert(purl.clone());
960+
}
950961
}
951962
}
952963
}

0 commit comments

Comments
 (0)