From a9a86464e98b87f0b7e563db0b6cb47505b8a21c Mon Sep 17 00:00:00 2001 From: Mateusz Kwapich Date: Mon, 18 Mar 2024 04:44:23 -0700 Subject: [PATCH] validate working copy equivalence instead of assuming it Summary: This diff is basically defensive programming but the stakes are high enough that I think is justified. In the D54877757 I'm assuming that current target_bookmark commit is working copy equivalent to the parent of the commit we're trying to sync. In this commit I'm acutally checking the mapping to verify that fact. Differential Revision: D54965081 fbshipit-source-id: 3c52fe33b86de9c4946b5ae3637d562fec9f41bf --- .../cross_repo_sync/src/commit_syncers_lib.rs | 57 +++++++++++++------ 1 file changed, 40 insertions(+), 17 deletions(-) diff --git a/eden/mononoke/commit_rewriting/cross_repo_sync/src/commit_syncers_lib.rs b/eden/mononoke/commit_rewriting/cross_repo_sync/src/commit_syncers_lib.rs index f4e084977a451..24eee69a391b9 100644 --- a/eden/mononoke/commit_rewriting/cross_repo_sync/src/commit_syncers_lib.rs +++ b/eden/mononoke/commit_rewriting/cross_repo_sync/src/commit_syncers_lib.rs @@ -70,6 +70,7 @@ use crate::types::Repo; use crate::types::Source; use crate::types::SubmoduleDeps; use crate::types::Target; +use crate::CommitSyncContext; const LEASE_WARNING_THRESHOLD: Duration = Duration::from_secs(60); @@ -193,8 +194,8 @@ pub(crate) async fn remap_parents<'a, M: SyncedCommitMapping + Clone + 'static, pub struct SyncedAncestorsVersions { // Versions of all synced ancestors pub versions: HashSet, - // Rewritten ancestors - pub rewritten_ancestors: HashMap, + // Rewritten ancestors: source_cs_id -> (rewritten_cs_id, version) + pub rewritten_ancestors: HashMap, } impl SyncedAncestorsVersions { @@ -283,11 +284,11 @@ where synced_ancestors_versions.versions.insert(version); } RewrittenAs(cs_ids_versions) => { - for (cs_id, version) in cs_ids_versions { + for (rewritten_cs_id, version) in cs_ids_versions { synced_ancestors_versions.versions.insert(version.clone()); synced_ancestors_versions .rewritten_ancestors - .insert(cs_id, version); + .insert(cs_id, (rewritten_cs_id, version)); } } EquivalentWorkingCopyAncestor(_, version) => { @@ -381,12 +382,14 @@ where Some(plural) => { use PluralCommitSyncOutcome::*; match plural { - NotSyncCandidate(version) => Ok(vec![(None, version)]), + NotSyncCandidate(version) => Ok(vec![(*cs_id, (None, version))]), RewrittenAs(cs_ids_versions) => Ok(cs_ids_versions .into_iter() - .map(|(cs_id, v)| (Some(cs_id), v)) + .map(|(rewritten_cs_id, v)| (*cs_id, (Some(rewritten_cs_id), v))) .collect()), - EquivalentWorkingCopyAncestor(_, version) => Ok(vec![(None, version)]), + EquivalentWorkingCopyAncestor(equivalent_cs_id, version) => { + Ok(vec![(*cs_id, (Some(equivalent_cs_id), version))]) + } } } None => Err(anyhow!("Failed to get config version from synced ancestor")), @@ -397,16 +400,17 @@ where .await? .into_iter() .flatten() - .collect::>(); + .collect::, CommitSyncConfigVersion))>>(); let synced_ancestors_versions = synced_ancestors_list .iter() - .cloned() - .map(|(_, v)| v) + .map(|(_source, (_target, v))| v.clone()) .collect(); let rewritten_ancestors = synced_ancestors_list .into_iter() - .filter_map(|(maybe_cs_id, version)| maybe_cs_id.map(|cs_id| (cs_id, version))) + .filter_map(|(source, (maybe_target, version))| { + maybe_target.map(|target| (source, (target, version))) + }) .collect(); // Get the oldest unsynced ancestors by getting the difference between the @@ -483,19 +487,38 @@ where return Ok((parent_version, HashMap::new())); } - // Let's see if we can find a better ancestor to use for the sync. + // Let's first validate that the target bookmark is still working-copy equivalent to what the + // parent of the commit we'd like to sync + let backsyncer = commit_syncer.reverse()?; + let small_csid_equivalent_to_target_bookmark = + if let Some(small_csid_equivalent_to_target_bookmark) = backsyncer + .sync_commit( + ctx, + target_bookmark_csid, + CandidateSelectionHint::Only, + CommitSyncContext::XRepoSyncJob, + false, + ) + .await? + { + small_csid_equivalent_to_target_bookmark + } else { + return Ok((parent_version, HashMap::new())); + }; + let mut parent_mapping = HashMap::new(); - for (parent, _version) in synced_ancestors_versions.rewritten_ancestors.iter() { + for (source_parent_csid, (target_parent_csid, _version)) in + synced_ancestors_versions.rewritten_ancestors.iter() + { // If the bookmark value is descendant of our parent it should have equivalent working // copy. - // TODO(mitrandir): we could actually trigger a sync and validate such equivalence instead - // of assuming it. if target_repo .commit_graph() - .is_ancestor(ctx, *parent, target_bookmark_csid) + .is_ancestor(ctx, *target_parent_csid, target_bookmark_csid) .await? + && small_csid_equivalent_to_target_bookmark == *source_parent_csid { - parent_mapping.insert(*parent, target_bookmark_csid); + parent_mapping.insert(*target_parent_csid, target_bookmark_csid); } }