mononoke: make change_target_config return result immediately if it w…

…as computed

Summary:
# Goal of the stack

There goal of this stack is to make megarepo api safer to use. In particular, we want to achieve:
1) If the same request is executed a few times, then it won't cause corrupt repo in any way (i.e. it won't create commits that client didn't intend to create and it won't move bookmarks to unpredictable places)
2) If request finished successfully, but we failed to send the success to the client, then repeating the same request would finish successfully.

Achieving #1 is necessary because async_requests_worker might execute a few requests at the same time (though this should be rare). Achieveing #2 is necessary because if we fail to send successful response to the client (e.g. because of network issues), we want client to retry and get this successful response back, so that client can continue with their next request.

In order to achieve #1 we make all bookmark move conditional i.e. we move a bookmark only if current location of the bookmark is at the place where client expects it. This should help achieve goal #1, because even if we have two requests executing at the same time, only one of them will successfully move a bookmark.

However once we achieve #1 we have a problem with #2 - if a request was successful, but we failed to send a successful reply back to the client then client will retry the request, and it will fail, because a bookmark is already at the new location (because previous request was successful), but client expects it to be at the old location (because client doesn't know that the request was succesful). To fix this issue before executing the request we check if this request was already successful, and we do it heuristically by checking request parameters and verifying the commit remapping state. This doesn't protect against malicious clients, but it should protect from issue #2 described above.

So the whole stack of diffs is the following:
1) take a method from megarepo api
2) implement a diff that makes bookmark moves conditional
3) Fix the problem #2 by checking if a previous request was successful or not

# This diff

Same as with D29848377 - if result was already computed and client retries the
same request, then return it.

Differential Revision: D29874802

fbshipit-source-id: ebc2f709bc8280305473d6333d0725530c131872

eden/mononoke/megarepo_api/src/change_target_config.rs

@@ -176,27 +176,34 @@ impl<'a> ChangeTargetConfig<'a> {
         // Find the target config version and remapping state that was used to
         // create the latest target commit. This config version will be used to
         // as a base for comparing with new config.
-        let (target_bookmark, old_target_cs_id) =
+        let (target_bookmark, actual_target_location) =
             find_target_bookmark_and_value(&ctx, &target_repo, &target).await?;
-        if old_target_cs_id != target_location {
-            return Err(MegarepoError::request(anyhow!(
-                "Can't change target config because \
-                 target_location is set to {} which is different \
-                 from actual target location {}.",
-                target_location,
-                old_target_cs_id,
-            )));
+
+        // target doesn't point to the commit we expect - check
+        // if this method has already succeded and just immediately return the
+        // result if so.
+        if actual_target_location != target_location {
+            return self
+                .check_if_this_method_has_already_succeeded(
+                    ctx,
+                    &new_version,
+                    (target_location, actual_target_location),
+                    &changesets_to_merge,
+                    &target_repo,
+                )
+                .await;
         }
+
         let old_target_cs = &target_repo
-            .changeset(old_target_cs_id)
+            .changeset(target_location)
             .await?
             .ok_or_else(|| {
                 MegarepoError::internal(anyhow!("programming error - target changeset not found!"))
             })?;
         let (old_remapping_state, old_config) = find_target_sync_config(
             &ctx,
             target_repo.blob_repo(),
-            old_target_cs_id,
+            target_location,
             &target,
             &self.megarepo_configs,
         )
@@ -658,6 +665,47 @@ impl<'a> ChangeTargetConfig<'a> {
 
         Ok(merge.get_changeset_id())
     }
+
+    // If that change_target_config() call was successful, but failed to send
+    // successful result to the client (e.g. network issues) then
+    // client will retry a request. We need to detect this situation and
+    // send a successful response to the client.
+    async fn check_if_this_method_has_already_succeeded(
+        &self,
+        ctx: &CoreContext,
+        new_version: &SyncConfigVersion,
+        (expected_target_location, actual_target_location): (ChangesetId, ChangesetId),
+        changesets_to_merge: &BTreeMap<SourceName, ChangesetId>,
+        repo: &RepoContext,
+    ) -> Result<ChangesetId, MegarepoError> {
+        // Bookmark points a non-expected commit - let's see if changeset it points to was created
+        // by a previous change_target_config call
+
+        // Check that first parent is a target location
+        let parents = repo
+            .blob_repo()
+            .get_changeset_parents_by_bonsai(ctx.clone(), actual_target_location)
+            .await?;
+        if parents.get(0) != Some(&expected_target_location) {
+            return Err(MegarepoError::request(anyhow!(
+                "Neither {} nor its first parent {:?} point to a target location {}",
+                actual_target_location,
+                parents.get(0),
+                expected_target_location,
+            )));
+        }
+
+        self.check_if_commit_has_expected_remapping_state(
+            ctx,
+            actual_target_location,
+            new_version,
+            changesets_to_merge,
+            repo,
+        )
+        .await?;
+
+        Ok(actual_target_location)
+    }
 }
 
 #[cfg(test)]

eden/mononoke/megarepo_api/src/change_target_config_test.rs

@@ -468,3 +468,158 @@ async fn test_change_target_config_no_file_dir_conflict_2(fb: FacebookInit) -> R
 
     Ok(())
 }
+
+#[fbinit::test]
+async fn test_change_target_config_repeat_same_request(fb: FacebookInit) -> Result<(), Error> {
+    let ctx = CoreContext::test_mock(fb);
+    let mut test = MegarepoTest::new(&ctx).await?;
+    let first_source_name = SourceName::new("source_1");
+    let target: Target = test.target("target".to_string());
+
+    init_megarepo(&ctx, &mut test).await?;
+
+    let first_source_cs_id =
+        resolve_cs_id(&ctx, &test.blobrepo, first_source_name.0.clone()).await?;
+    let target_cs_id = resolve_cs_id(&ctx, &test.blobrepo, "target").await?;
+
+    let version_2 = "version_2".to_string();
+    let third_source_name = SourceName::new("source_3");
+    let third_source_cs_id = CreateCommitContext::new_root(&ctx, &test.blobrepo)
+        .add_file("third", "third")
+        .commit()
+        .await?;
+
+    bookmark(&ctx, &test.blobrepo, third_source_name.to_string())
+        .set_to(third_source_cs_id)
+        .await?;
+    SyncTargetConfigBuilder::new(test.repo_id(), target.clone(), version_2.clone())
+        .source_builder(first_source_name.clone())
+        .set_prefix_bookmark_to_source_name()
+        .linkfile("first", "linkfiles/first_in_other_location")
+        .build_source()?
+        .source_builder(third_source_name.clone())
+        .set_prefix_bookmark_to_source_name()
+        .build_source()?
+        .build(&mut test.configs_storage);
+
+    let configs_storage: Arc<dyn MononokeMegarepoConfigs> = Arc::new(test.configs_storage.clone());
+    let change_target_config = ChangeTargetConfig::new(&configs_storage, &test.mononoke);
+    change_target_config
+        .run(
+            &ctx,
+            &target,
+            version_2.clone(),
+            target_cs_id,
+            btreemap! {
+                first_source_name.clone() => first_source_cs_id,
+                third_source_name.clone() => third_source_cs_id,
+            },
+            None,
+        )
+        .await?;
+
+    // Now repeat the same request - it should succeed
+    let change_target_config = ChangeTargetConfig::new(&configs_storage, &test.mononoke);
+    change_target_config
+        .run(
+            &ctx,
+            &target,
+            version_2.clone(),
+            target_cs_id,
+            btreemap! {
+                first_source_name.clone() => first_source_cs_id,
+                third_source_name.clone() => third_source_cs_id,
+            },
+            None,
+        )
+        .await?;
+
+    // Now send slightly different request - it should fail
+    let change_target_config = ChangeTargetConfig::new(&configs_storage, &test.mononoke);
+    assert!(
+        change_target_config
+            .run(
+                &ctx,
+                &target,
+                version_2,
+                target_cs_id,
+                btreemap! {
+                    first_source_name.clone() => first_source_cs_id,
+                },
+                None,
+            )
+            .await
+            .is_err()
+    );
+    Ok(())
+}
+
+#[fbinit::test]
+async fn test_change_target_config_noop_change(fb: FacebookInit) -> Result<(), Error> {
+    let ctx = CoreContext::test_mock(fb);
+    let mut test = MegarepoTest::new(&ctx).await?;
+    let first_source_name = SourceName::new("source_1");
+    let target: Target = test.target("target".to_string());
+
+    init_megarepo(&ctx, &mut test).await?;
+
+    let first_source_cs_id =
+        resolve_cs_id(&ctx, &test.blobrepo, first_source_name.0.clone()).await?;
+    let target_cs_id = resolve_cs_id(&ctx, &test.blobrepo, "target").await?;
+
+    let version_2 = "version_2".to_string();
+    let third_source_name = SourceName::new("source_3");
+    let third_source_cs_id = CreateCommitContext::new_root(&ctx, &test.blobrepo)
+        .add_file("third", "third")
+        .commit()
+        .await?;
+
+    bookmark(&ctx, &test.blobrepo, third_source_name.to_string())
+        .set_to(third_source_cs_id)
+        .await?;
+    SyncTargetConfigBuilder::new(test.repo_id(), target.clone(), version_2.clone())
+        .source_builder(first_source_name.clone())
+        .set_prefix_bookmark_to_source_name()
+        .linkfile("first", "linkfiles/first_in_other_location")
+        .build_source()?
+        .source_builder(third_source_name.clone())
+        .set_prefix_bookmark_to_source_name()
+        .build_source()?
+        .build(&mut test.configs_storage);
+
+    let configs_storage: Arc<dyn MononokeMegarepoConfigs> = Arc::new(test.configs_storage.clone());
+    let change_target_config = ChangeTargetConfig::new(&configs_storage, &test.mononoke);
+    let new_target_cs_id = change_target_config
+        .run(
+            &ctx,
+            &target,
+            version_2.clone(),
+            target_cs_id,
+            btreemap! {
+                first_source_name.clone() => first_source_cs_id,
+                third_source_name.clone() => third_source_cs_id,
+            },
+            None,
+        )
+        .await?;
+
+    // Now do a noop change on existing commit
+    let change_target_config = ChangeTargetConfig::new(&configs_storage, &test.mononoke);
+    let noop_change = change_target_config
+        .run(
+            &ctx,
+            &target,
+            version_2.clone(),
+            new_target_cs_id,
+            btreemap! {
+                first_source_name.clone() => first_source_cs_id,
+                third_source_name.clone() => third_source_cs_id,
+            },
+            None,
+        )
+        .await?;
+
+    assert_ne!(noop_change, new_target_cs_id);
+
+    Ok(())
+}