From 1f5825aa5e2977d5496e607156a79a41f8919eac Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Tue, 18 Aug 2026 13:29:47 +0000 Subject: [PATCH] fix(storage): fetch full closure during repair Use Git's noop negotiation algorithm only for identifier-family integrity fetches. A corrupt family can retain refs whose objects are absent; normal negotiation treats those refs as local have tips, requests an empty pack, and then fails connectivity even when the remote advertises the missing commit.\n\nThe dedicated integrity fetch role forces the accepted clone source to send the requested object's complete closure into the shared family object directory. Primary and hedged proactive fetches deliberately retain normal negotiation so routine transfers stay incremental.\n\nValidated against a quarantined production canary object, with strict all-target Clippy, all git::integrity unit tests, and a regression test for integrity-only noop selection. --- src/git/integrity.rs | 9 ++++++++- src/purgatory/sync/context.rs | 31 +++++++++++++++++++++++++++++++ src/purgatory/sync/mod.rs | 2 +- 3 files changed, 40 insertions(+), 2 deletions(-) diff --git a/src/git/integrity.rs b/src/git/integrity.rs index 9693a2f..e70cdfa 100644 --- a/src/git/integrity.rs +++ b/src/git/integrity.rs @@ -446,7 +446,14 @@ impl FamilyRepairSource for crate::purgatory::sync::RealSyncContext { url: &str, oids: &[String], ) -> Result> { - crate::purgatory::sync::SyncContext::fetch_oids(self, target_view, url, oids).await + crate::purgatory::sync::SyncContext::fetch_oids_with_role( + self, + target_view, + url, + oids, + crate::purgatory::sync::GitFetchRole::Integrity, + ) + .await } } diff --git a/src/purgatory/sync/context.rs b/src/purgatory/sync/context.rs index 90d6a35..9cebcea 100644 --- a/src/purgatory/sync/context.rs +++ b/src/purgatory/sync/context.rs @@ -21,6 +21,7 @@ use std::time::{Duration, Instant}; pub enum GitFetchRole { Primary, Hedge, + Integrity, } impl GitFetchRole { @@ -28,6 +29,7 @@ impl GitFetchRole { match self { Self::Primary => "primary", Self::Hedge => "hedge", + Self::Integrity => "integrity", } } } @@ -465,6 +467,7 @@ fn hardened_git_command( resolve_pin: Option<&str>, auth_header: Option<&str>, object_directory: Option<&Path>, + negotiation_algorithm: Option<&str>, args: &[String], ) -> Command { let mut command = Command::new("git"); @@ -475,6 +478,11 @@ fn hardened_git_command( .arg("http.proxy=") .arg("-c") .arg("credential.helper="); + if let Some(algorithm) = negotiation_algorithm { + command + .arg("-c") + .arg(format!("fetch.negotiationAlgorithm={algorithm}")); + } if let Some(pin) = resolve_pin { command.arg("-c").arg(format!("http.curloptResolve={pin}")); } @@ -831,6 +839,15 @@ fn is_object_missing_error(stderr: &str) -> bool { || stderr.contains("Server does not allow request for unadvertised object") } +/// Integrity repair starts from a repository whose refs may name objects that +/// are already missing. Normal fetch negotiation walks those refs as local +/// "have" tips, which can make Git request no pack and then fail its own +/// connectivity check. Repair fetches therefore advertise no local haves and +/// ask the source for the complete requested object closure. +fn fetch_negotiation_algorithm(role: GitFetchRole) -> Option<&'static str> { + (role == GitFetchRole::Integrity).then_some("noop") +} + /// Record a remote failure with the naughty list tracker (only error /// categories the tracker classifies as persistent are recorded). fn record_remote_failure( @@ -1076,6 +1093,7 @@ impl SyncContext for RealSyncContext { resolve_pin.as_deref(), fresh_auth_header().as_deref(), None, + None, &ls_remote_args, ), &domain, @@ -1147,6 +1165,7 @@ impl SyncContext for RealSyncContext { resolve_pin.as_deref(), fresh_auth_header().as_deref(), family_objects, + fetch_negotiation_algorithm(role), &args, ), &domain, @@ -1218,6 +1237,7 @@ impl SyncContext for RealSyncContext { resolve_pin.as_deref(), fresh_auth_header().as_deref(), family_objects, + fetch_negotiation_algorithm(role), &args, ), &domain, @@ -1754,6 +1774,17 @@ mod fetch_helper_tests { )); } + #[test] + fn integrity_fetches_disable_local_have_negotiation() { + assert_eq!(fetch_negotiation_algorithm(GitFetchRole::Primary), None); + assert_eq!(fetch_negotiation_algorithm(GitFetchRole::Hedge), None); + assert_eq!( + fetch_negotiation_algorithm(GitFetchRole::Integrity), + Some("noop") + ); + assert_eq!(GitFetchRole::Integrity.as_str(), "integrity"); + } + #[test] fn miss_memo_is_stable_order_independent_invalidated_and_bounded() { let first = HashSet::from(["b".to_string(), "a".to_string()]); diff --git a/src/purgatory/sync/mod.rs b/src/purgatory/sync/mod.rs index 022a556..e46b29a 100644 --- a/src/purgatory/sync/mod.rs +++ b/src/purgatory/sync/mod.rs @@ -13,7 +13,7 @@ mod r#loop; mod queue; mod throttle; -pub use context::{ProcessResult, RealSyncContext, SyncContext}; +pub use context::{GitFetchRole, ProcessResult, RealSyncContext, SyncContext}; pub use functions::{ get_throttled_domains_with_untried_urls, sync_identifier, sync_identifier_from_url, sync_identifier_next_url, ThrottledDomainInfo,