send empty bundle-list upon failure

mzr · facebook-github-bot · commit a9566d777a3d · 2025-04-25T02:54:22.000-07:00
Summary: When we fail to generate a bundle-list, the whole git command fails, but when we send an empty bundle list, nothing happens - git just sees it's empty and does nothing. In case of a sever-side failure, let's just fail silently and send an empty bundle-list to the client. When we fail silently we have to log it, otherwise there's no way of knowing we're failing, so I'm also adding logging here.

Reviewed By: RajivTS

Differential Revision: D73188354

fbshipit-source-id: ebc80c2a2b423e640fe175d4c8b067b9cc7db9d7
diff --git a/eden/mononoke/git_server/src/main.rs b/eden/mononoke/git_server/src/main.rs
@@ -9,6 +9,7 @@
 #![feature(never_type)]
 #![feature(let_chains)]
 #![feature(cursor_split)]
+#![feature(try_blocks)]
 
 use std::fs::File;
 use std::io::Write;
diff --git a/eden/mononoke/git_server/src/model.rs b/eden/mononoke/git_server/src/model.rs
@@ -13,6 +13,7 @@ pub use context::GitServerContext;
 pub use context::RepositoryRequestContext;
 use gotham_derive::StateData;
 use gotham_derive::StaticResponseExtender;
+pub use method::BundleUriOutcome;
 pub use method::GitMethod;
 pub use method::GitMethodInfo;
 pub use method::PushValidationErrors;
diff --git a/eden/mononoke/git_server/src/model/method.rs b/eden/mononoke/git_server/src/model/method.rs
@@ -180,6 +180,12 @@ impl Display for PushValidationErrors {
     }
 }
 
+#[derive(Clone, StateData)]
+pub enum BundleUriOutcome {
+    Success(String),
+    Error(String),
+}
+
 #[cfg(test)]
 mod tests {
     use anyhow::Result;
diff --git a/eden/mononoke/git_server/src/read/upload_pack.rs b/eden/mononoke/git_server/src/read/upload_pack.rs
@@ -62,6 +62,7 @@ use crate::command::Command;
 use crate::command::FetchArgs;
 use crate::command::LsRefsArgs;
 use crate::command::RequestCommand;
+use crate::model::BundleUriOutcome;
 use crate::model::GitMethodInfo;
 use crate::model::RepositoryParams;
 use crate::model::RepositoryRequestContext;
@@ -339,7 +340,7 @@ pub async fn upload_pack(state: &mut State) -> Result<Response<Body>, HttpError>
         }
         Command::BundleUri => {
             let git_host = GitHost::from_state_mononoke_host(state)?;
-            let output = bundle_uri(&request_context, git_host).await;
+            let output = bundle_uri(state, &request_context, git_host).await;
             let output = output.map_err(HttpError::e500)?.try_into_response(state);
             output.map_err(HttpError::e500)
         }
@@ -350,6 +351,7 @@ pub async fn upload_pack(state: &mut State) -> Result<Response<Body>, HttpError>
 }
 
 async fn bundle_uri(
+    state: &mut State,
     request_context: &RepositoryRequestContext,
     git_host: GitHost,
 ) -> Result<impl TryIntoResponse, Error> {
@@ -360,15 +362,41 @@ async fn bundle_uri(
         let bundle_list = bundle_uri.get_latest_bundle_list().await?;
 
         if let Some(bundle_list) = bundle_list {
-            for bundle in bundle_list.bundles.iter() {
-                let uri = bundle_uri
-                    .get_url_for_bundle_handle(&request_context.ctx, &git_host, 60, &bundle.handle)
-                    .await?;
+            let bundle_list_out: Result<Vec<u8>, anyhow::Error> = try {
+                let mut bundle_list_out_buf: Vec<u8> = vec![];
+                for bundle in bundle_list.bundles.iter() {
+                    let uri = bundle_uri
+                        .get_url_for_bundle_handle(
+                            &request_context.ctx,
+                            &git_host,
+                            60,
+                            &bundle.handle,
+                        )
+                        .await?;
 
-                let str = format!("\nbundle.bundle_{}.uri={}", bundle.fingerprint, uri,);
-                out.extend_from_slice(str.as_bytes());
+                    let str = format!("\nbundle.bundle_{}.uri={}", bundle.fingerprint, uri,);
+                    bundle_list_out_buf.extend_from_slice(str.as_bytes());
+                }
+                bundle_list_out_buf
+            };
+            match bundle_list_out {
+                Ok(blo) => {
+                    state.put(BundleUriOutcome::Success(format!(
+                        "advertised {} bundles from bundle list {}",
+                        bundle_list.bundles.len(),
+                        bundle_list.bundle_list_num
+                    )));
+                    out.extend_from_slice(&blo[..])
+                }
+                Err(err) => {
+                    state.put(BundleUriOutcome::Error(format!("{:?}", err)));
+                }
             }
+        } else {
+            state.put(BundleUriOutcome::Success("bundle-list empty".to_string()));
         }
+    } else {
+        state.put(BundleUriOutcome::Success("client untrusted".to_string()));
     }
 
     let mut output = Vec::new();
diff --git a/eden/mononoke/git_server/src/scuba.rs b/eden/mononoke/git_server/src/scuba.rs
@@ -14,6 +14,7 @@ use gotham_ext::middleware::request_context::RequestContext;
 use permission_checker::MononokeIdentitySetExt;
 use scuba_ext::MononokeScubaSampleBuilder;
 
+use crate::model::BundleUriOutcome;
 use crate::model::GitMethodInfo;
 use crate::model::PushValidationErrors;
 
@@ -26,6 +27,8 @@ pub enum MononokeGitScubaKey {
     Error,
     ErrorCount,
     PushValidationErrors,
+    BundleUriError,
+    BundleUriSuccess,
     PackfileReadError,
     PackfileSize,
     PackfileCommitCount,
@@ -43,6 +46,8 @@ impl AsRef<str> for MononokeGitScubaKey {
             Self::Error => "error",
             Self::ErrorCount => "error_count",
             Self::PushValidationErrors => "push_validation_errors",
+            Self::BundleUriError => "bundle_uri_error_msg",
+            Self::BundleUriSuccess => "bundle_uri_success_msg",
             Self::PackfileReadError => "packfile_read_error",
             Self::PackfileSize => "packfile_size",
             Self::PackfileCommitCount => "packfile_commit_count",
@@ -63,6 +68,7 @@ pub struct MononokeGitScubaHandler {
     request_context: Option<RequestContext>,
     method_info: Option<GitMethodInfo>,
     push_validation_errors: Option<PushValidationErrors>,
+    bundle_uri_outcome: Option<BundleUriOutcome>,
     client_username: Option<String>,
 }
 
@@ -101,6 +107,7 @@ impl MononokeGitScubaHandler {
         Self {
             request_context: state.try_borrow::<RequestContext>().cloned(),
             method_info: state.try_borrow::<GitMethodInfo>().cloned(),
+            bundle_uri_outcome: state.try_borrow::<BundleUriOutcome>().cloned(),
             push_validation_errors: state.try_borrow::<PushValidationErrors>().cloned(),
             client_username: state
                 .try_borrow::<MetadataState>()
@@ -130,6 +137,16 @@ impl MononokeGitScubaHandler {
                 push_validation_errors.to_string(),
             );
         }
+        if let Some(outcome) = self.bundle_uri_outcome {
+            match outcome {
+                BundleUriOutcome::Success(success_msg) => {
+                    scuba.add(MononokeGitScubaKey::BundleUriSuccess, success_msg);
+                }
+                BundleUriOutcome::Error(error_msg) => {
+                    scuba.add(MononokeGitScubaKey::BundleUriError, error_msg);
+                }
+            }
+        }
         if let Some(err) = info.first_error() {
             scuba.add(MononokeGitScubaKey::Error, format!("{:?}", err));
         }

Original file line number	Diff line number	Diff line change
`@@ -180,6 +180,12 @@ impl Display for PushValidationErrors {`
`180`	`180`	`}`
`181`	`181`	`}`
`182`	`182`
	`183`	`+#[derive(Clone, StateData)]`
	`184`	`+pub enum BundleUriOutcome {`
	`185`	`+ Success(String),`
	`186`	`+ Error(String),`
	`187`	`+}`
	`188`	`+`
`183`	`189`	`#[cfg(test)]`
`184`	`190`	`mod tests {`
`185`	`191`	`use anyhow::Result;`