Skip to content

Commit

Permalink
Ensure expired and non-existing bubbles don't result in 500 status co…
Browse files Browse the repository at this point in the history
…de 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
  • Loading branch information
RajivTS authored and facebook-github-bot committed Dec 20, 2024
1 parent 4493daf commit e3b1a31
Showing 1 changed file with 29 additions and 6 deletions.
35 changes: 29 additions & 6 deletions eden/mononoke/edenapi_service/src/handlers/commit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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(),
Expand Down

0 comments on commit e3b1a31

Please sign in to comment.