From e3b1a31b43706b24d02b469eab92ab55ff39c5f5 Mon Sep 17 00:00:00 2001
From: Rajiv Sharma <rajshar@meta.com>
Date: Fri, 20 Dec 2024 04:48:16 -0800
Subject: [PATCH] Ensure expired and non-existing bubbles don't result in 500
 status code responses

Summary:
Due to incorrect error handling, we get a lot of 500s in Eden API for requests that are invalid to begin with. Either they refer to an non-existent bubble or they refer to a bubble that has already expired.
{F1974017446}

Which leads to logs like above in our scuba table. This diff makes the necessary changes to ensure that we return such cases with 4xx error codes so they don't get classified as server errors

Reviewed By: lmvasquezg

Differential Revision: D67458467

fbshipit-source-id: ff5ae4c8c48a260a5b8d4c218156f30821212788
---
 .../edenapi_service/src/handlers/commit.rs    | 35 +++++++++++++++----
 1 file changed, 29 insertions(+), 6 deletions(-)

diff --git a/eden/mononoke/edenapi_service/src/handlers/commit.rs b/eden/mononoke/edenapi_service/src/handlers/commit.rs
index c37216218048a..00e9487a9f5cc 100644
--- a/eden/mononoke/edenapi_service/src/handlers/commit.rs
+++ b/eden/mononoke/edenapi_service/src/handlers/commit.rs
@@ -652,19 +652,37 @@ impl SaplingRemoteApiHandler for FetchSnapshotHandler {
             .bubble_from_changeset(repo.ctx(), &cs_id)
             .await
             .context("Failure in fetching bubble from changeset")?
-            .context("Snapshot not in a bubble")?;
+            .ok_or_else(|| {
+                HttpError::e404(MononokeError::NotAvailable(format!(
+                    "Snapshot for changeset {} not found in bubble",
+                    cs_id
+                )))
+            })?;
         let labels = repo
             .ephemeral_store()
             .labels_from_bubble(repo.ctx(), &bubble_id)
             .await
             .context("Failed to fetch labels associated with the snapshot")?;
         let blobstore = repo.bubble_blobstore(Some(bubble_id)).await?;
-        let cs = cs_id
+        let fallible_cs = cs_id
             .load(repo.ctx(), &blobstore)
             .await
-            .context("Failed to load bonsai changeset through bubble blobstore")
-            .map_err(MononokeError::from)?
-            .into_mut();
+            .context("Failed to load bonsai changeset through bubble blobstore");
+        let cs = match fallible_cs {
+            Ok(cs) => cs.into_mut(),
+            Err(e) => {
+                let bubble_expired = format!("{:?}", e).contains("has expired");
+                let err = if bubble_expired {
+                    HttpError::e400(MononokeError::NotAvailable(format!(
+                        "Snapshot for changeset {} with bubble ID {} expired",
+                        cs_id, bubble_id
+                    )))
+                } else {
+                    HttpError::e500(MononokeError::from(e))
+                };
+                return Err(err.into());
+            }
+        };
         let time = cs.author_date.timestamp_secs();
         let tz = cs.author_date.tz_offset_secs();
         let response = FetchSnapshotResponse {
@@ -744,7 +762,12 @@ impl SaplingRemoteApiHandler for AlterSnapshotHandler {
             .ephemeral_store()
             .bubble_from_changeset(repo.ctx(), &cs_id)
             .await?
-            .context("Snapshot does not exist or has already expired")?;
+            .ok_or_else(|| {
+                HttpError::e404(MononokeError::NotAvailable(format!(
+                    "Snapshot for changeset {} not found in bubble",
+                    cs_id
+                )))
+            })?;
         let (label_addition, label_removal) = (
             !request.labels_to_add.is_empty(),
             !request.labels_to_remove.is_empty(),