From 51c331f26ad3c8c422b41267e3695c8f2295510e Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Tue, 27 Jan 2026 21:05:50 +0000 Subject: feat: implement OID retry logic for 'not our ref' errors Add retry loop in fetch_oids that handles git's behavior of stopping at the first missing OID. When a 'not our ref' error occurs: - Parse the missing OID from stderr - Remove it from the fetch list and track it as missing - Retry with remaining OIDs until success or all OIDs exhausted This ensures we fetch all available OIDs even when some are missing from the remote, rather than failing the entire batch. Also improves error reporting: - Include URL in all error messages for easier debugging - Log stderr even when domain is already on naughty list --- src/purgatory/sync/context.rs | 163 +++++++++++++++++++++++++----------------- 1 file changed, 96 insertions(+), 67 deletions(-) (limited to 'src/purgatory/sync') diff --git a/src/purgatory/sync/context.rs b/src/purgatory/sync/context.rs index 33c2d12..0df8be0 100644 --- a/src/purgatory/sync/context.rs +++ b/src/purgatory/sync/context.rs @@ -361,59 +361,60 @@ impl SyncContext for RealSyncContext { let naughty_list = self.git_naughty_list.clone(); tokio::task::spawn_blocking(move || -> Result> { - // git fetch ... - fetch all OIDs with full history - let mut args = vec!["fetch", &url]; - args.extend(missing_oids.iter().map(|s| s.as_str())); - - let output = Command::new("git") - .args(&args) - .current_dir(&repo_path) - .output(); - - match output { - Ok(result) if result.status.success() => { - // Count how many OIDs we now have - let fetched: Vec = missing_oids - .iter() - .filter(|oid| crate::git::oid_exists(&repo_path, oid)) - .cloned() - .collect(); - - debug!(fetched_count = fetched.len(), "Successfully fetched OIDs"); - - Ok(fetched) + let mut remaining_oids = missing_oids.clone(); + let mut missing_from_remote: Vec = Vec::new(); + + // Retry loop: keep fetching until success or no OIDs left + loop { + if remaining_oids.is_empty() { + // All OIDs were missing from remote + debug!( + url = %url, + missing_count = missing_from_remote.len(), + "All requested OIDs missing from remote" + ); + return Ok(vec![]); } - Ok(result) => { - let stderr = String::from_utf8_lossy(&result.stderr); - - // Extract domain and classify error for naughty list - if let Some(domain) = extract_domain(&url) { - if let Some(category) = NaughtyListTracker::classify_error(&stderr) { - let is_new = naughty_list.record(&domain, category, stderr.to_string()); - - if is_new { - tracing::warn!( - domain = %domain, - category = %category, - error = %stderr, - "Git remote domain added to naughty list" - ); - } else { - debug!( - domain = %domain, - category = %category, - "Git remote domain still on naughty list" - ); - } + + // git fetch ... - fetch all OIDs with full history + let mut args = vec!["fetch".to_string(), url.clone()]; + args.extend(remaining_oids.iter().cloned()); + + let output = Command::new("git") + .args(&args) + .current_dir(&repo_path) + .output(); + + match output { + Ok(result) if result.status.success() => { + // Fetch succeeded - count how many OIDs we now have + let fetched: Vec = missing_oids + .iter() + .filter(|oid| crate::git::oid_exists(&repo_path, oid)) + .cloned() + .collect(); + + if !missing_from_remote.is_empty() { + debug!( + url = %url, + fetched_count = fetched.len(), + missing_count = missing_from_remote.len(), + missing_oids = ?missing_from_remote, + "Fetch completed after retries - some OIDs were missing from remote" + ); + } else { + debug!(fetched_count = fetched.len(), "Successfully fetched OIDs"); } + + return Ok(fetched); } + Ok(result) => { + let stderr = String::from_utf8_lossy(&result.stderr); - // Check for "not our ref" errors and provide a clearer error message - let error_msg = if stderr.contains("upload-pack: not our ref") { - // Parse out the missing OID from stderr (git only reports one at a time) - let missing_oid = stderr - .lines() - .find_map(|line| { + // Check for "not our ref" error - this is retryable + if stderr.contains("upload-pack: not our ref") { + // Parse out the missing OID from stderr + let missing_oid = stderr.lines().find_map(|line| { if line.contains("not our ref") { // Extract the OID from lines like: // "fatal: remote error: upload-pack: not our ref " @@ -423,32 +424,60 @@ impl SyncContext for RealSyncContext { } }); - let total_requested = missing_oids.len(); + if let Some(ref oid) = missing_oid { + // Remove the missing OID and retry with remaining + remaining_oids.retain(|o| o != oid); + missing_from_remote.push(oid.clone()); - if let Some(oid) = missing_oid { - if total_requested > 1 { - // BUG: Git stops at first missing OID, so we don't know if the others exist - // We need retry logic to fetch remaining OIDs individually - tracing::warn!( + debug!( url = %url, missing_oid = %oid, - total_requested = total_requested, - "Git fetch failed on first missing OID - other requested OIDs may exist but were not fetched. Retry logic needed." + remaining_count = remaining_oids.len(), + "OID not found on remote, retrying with remaining OIDs" ); - format!("remote missing oid {} (BUG: {} other oids not attempted)", oid, total_requested - 1) - } else { - format!("remote missing only oid requested: {}", oid) + + continue; // Retry with remaining OIDs + } + } + + // Non-retryable error - record to naughty list and return error + if let Some(domain) = extract_domain(&url) { + if let Some(category) = NaughtyListTracker::classify_error(&stderr) { + let is_new = + naughty_list.record(&domain, category, stderr.to_string()); + + if is_new { + tracing::warn!( + domain = %domain, + category = %category, + error = %stderr, + "Git remote domain added to naughty list" + ); + } else { + debug!( + domain = %domain, + category = %category, + error = %stderr, + "Git fetch failed (domain on naughty list)" + ); + } } - } else { - format!("git fetch failed: {}", stderr) } - } else { - format!("git fetch failed: {}", stderr) - }; - Err(anyhow::anyhow!("{}", error_msg)) + return Err(anyhow::anyhow!( + "git fetch failed for {}: {}", + url, + stderr + )); + } + Err(e) => { + return Err(anyhow::anyhow!( + "git fetch command error for {}: {}", + url, + e + )) + } } - Err(e) => Err(anyhow::anyhow!("git fetch command error: {}", e)), } }) .await -- cgit v1.2.3 From 847acdecb9c28a5307123b9ee685b769a598cfc1 Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Tue, 27 Jan 2026 21:40:46 +0000 Subject: fix: distinguish 0 OIDs fetched from successful fetch in logging When fetch_oids returns Ok(vec![]) (all requested OIDs missing from remote), the log message now says 'Fetch returned no OIDs (not available on remote)' instead of the misleading 'Fetch succeeded' with oids_fetched=0. --- src/purgatory/sync/functions.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) (limited to 'src/purgatory/sync') diff --git a/src/purgatory/sync/functions.rs b/src/purgatory/sync/functions.rs index 65d29af..2b7e71f 100644 --- a/src/purgatory/sync/functions.rs +++ b/src/purgatory/sync/functions.rs @@ -369,7 +369,7 @@ pub async fn sync_identifier_from_url( throttle_manager.complete_request(&domain); let oids_fetched = match fetch_result { - Ok(fetched) => { + Ok(fetched) if !fetched.is_empty() => { debug!( identifier = %identifier, url = %url, @@ -378,6 +378,14 @@ pub async fn sync_identifier_from_url( ); fetched.len() } + Ok(_) => { + debug!( + identifier = %identifier, + url = %url, + "Fetch returned no OIDs (not available on remote)" + ); + 0 + } Err(e) => { debug!( identifier = %identifier, -- cgit v1.2.3 From 6d920cae2704016869500889a92b358d845b69e1 Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Tue, 27 Jan 2026 21:42:25 +0000 Subject: improve logging --- src/purgatory/sync/context.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) (limited to 'src/purgatory/sync') diff --git a/src/purgatory/sync/context.rs b/src/purgatory/sync/context.rs index 0df8be0..904f8af 100644 --- a/src/purgatory/sync/context.rs +++ b/src/purgatory/sync/context.rs @@ -403,7 +403,7 @@ impl SyncContext for RealSyncContext { "Fetch completed after retries - some OIDs were missing from remote" ); } else { - debug!(fetched_count = fetched.len(), "Successfully fetched OIDs"); + debug!(url = %url, fetched_count = fetched.len(), "Successfully fetched OIDs"); } return Ok(fetched); @@ -418,7 +418,9 @@ impl SyncContext for RealSyncContext { if line.contains("not our ref") { // Extract the OID from lines like: // "fatal: remote error: upload-pack: not our ref " - line.split("not our ref").nth(1).map(|s| s.trim().to_string()) + line.split("not our ref") + .nth(1) + .map(|s| s.trim().to_string()) } else { None } @@ -464,11 +466,7 @@ impl SyncContext for RealSyncContext { } } - return Err(anyhow::anyhow!( - "git fetch failed for {}: {}", - url, - stderr - )); + return Err(anyhow::anyhow!("git fetch failed for {}: {}", url, stderr)); } Err(e) => { return Err(anyhow::anyhow!( -- cgit v1.2.3 From efc3da477d4edb9d1334718e3e20d197ba711468 Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Tue, 27 Jan 2026 21:55:34 +0000 Subject: fix: pass actually fetched OIDs to process_newly_available_git_data Previously, sync_identifier_from_url passed all needed OIDs to process_newly_available_git_data, not just the OIDs that were successfully fetched. This caused incorrect logging (new_oids_count would show all needed OIDs, not just fetched ones). While this didn't break functionality (the actual processing uses can_apply_state which checks the repository on disk), it made debugging confusing. Changes: - Rename oids_fetched to fetched_oids and change type from usize to Vec - Return Vec from match arms instead of counts - Pass fetched_oids (not needed_oids) to process_newly_available_git_data - Return fetched_oids.len() at the end This ensures logging accurately reflects which OIDs were actually fetched from the remote. --- src/purgatory/sync/functions.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) (limited to 'src/purgatory/sync') diff --git a/src/purgatory/sync/functions.rs b/src/purgatory/sync/functions.rs index 2b7e71f..9207d58 100644 --- a/src/purgatory/sync/functions.rs +++ b/src/purgatory/sync/functions.rs @@ -368,7 +368,7 @@ pub async fn sync_identifier_from_url( let fetch_result = ctx.fetch_oids(&target_repo, url, &needed_oids).await; throttle_manager.complete_request(&domain); - let oids_fetched = match fetch_result { + let fetched_oids = match fetch_result { Ok(fetched) if !fetched.is_empty() => { debug!( identifier = %identifier, @@ -376,7 +376,7 @@ pub async fn sync_identifier_from_url( oids_fetched = fetched.len(), "Fetch succeeded" ); - fetched.len() + fetched } Ok(_) => { debug!( @@ -384,7 +384,7 @@ pub async fn sync_identifier_from_url( url = %url, "Fetch returned no OIDs (not available on remote)" ); - 0 + vec![] } Err(e) => { debug!( @@ -393,13 +393,13 @@ pub async fn sync_identifier_from_url( error = %e, "Fetch failed" ); - 0 + vec![] } }; // Try to process any events that can now be satisfied - if oids_fetched > 0 { - let new_oids: HashSet = needed_oids.into_iter().collect(); + if !fetched_oids.is_empty() { + let new_oids: HashSet = fetched_oids.iter().cloned().collect(); if let Err(e) = ctx .process_newly_available_git_data(&target_repo, &new_oids) .await @@ -412,7 +412,7 @@ pub async fn sync_identifier_from_url( } } - oids_fetched + fetched_oids.len() } /// Sync git data for an identifier. -- cgit v1.2.3