Skip to content

Commit

Permalink
slapi: defend against incomplete server response for bookmarks
Browse files Browse the repository at this point in the history
Summary:
Sometimes the request:

  api.bookmarks(['master'])

got a HTTP 200 response that decodes to:

  {}

In some code paths, this is then interpreted as `{'master': None}`, meaning the
deletion of the `master` bookmark. That's not ideal. So let's defend against
the error by checking the response length.

Normally, the server reports all bookmark names as requested with values
being either `None` or the commit hash.

Reviewed By: muirdm

Differential Revision: D65855783

fbshipit-source-id: 304ba8e5d72bfe52fd215cf67002a4f11f9005b4
  • Loading branch information
quark-zju authored and facebook-github-bot committed Nov 13, 2024
1 parent 28a52c2 commit 35e6d82
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 2 deletions.
17 changes: 15 additions & 2 deletions eden/scm/lib/edenapi/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1410,16 +1410,29 @@ impl SaplingRemoteApi for Client {
&self,
bookmarks: Vec<String>,
) -> Result<Vec<BookmarkEntry>, SaplingRemoteApiError> {
let request_len = bookmarks.len();
tracing::info!("Requesting {} bookmarks", bookmarks.len());
let url = self.build_url(paths::BOOKMARKS)?;
let bookmark_req = BookmarkRequest { bookmarks };
self.log_request(&bookmark_req, "bookmarks");
let bookmarks_wire = bookmark_req.to_wire();
let req = self
.configure_request(self.inner.client.post(url))?
.cbor(&bookmark_req.to_wire())
.cbor(&bookmarks_wire)
.map_err(SaplingRemoteApiError::RequestSerializationFailed)?;

self.fetch_vec_with_retry::<BookmarkEntry>(vec![req]).await
let response = self
.fetch_vec_with_retry::<BookmarkEntry>(vec![req])
.await?;
if response.len() != request_len {
let bookmarks = bookmarks_wire.bookmarks;
let message = format!(
"Requested bookmarks {:?} but only got {:?}.",
bookmarks, &response
);
return Err(SaplingRemoteApiError::IncompleteResponse(message));
}
Ok(response)
}

async fn set_bookmark(
Expand Down
3 changes: 3 additions & 0 deletions eden/scm/lib/edenapi/trait/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ pub enum SaplingRemoteApiError {
NotSupported,
#[error(transparent)]
MissingCerts(#[from] auth::MissingCerts),
#[error("IncompeteResponse: {0}")]
IncompleteResponse(String),
}

#[derive(Debug, Error)]
Expand Down Expand Up @@ -133,6 +135,7 @@ impl SaplingRemoteApiError {
| HttpError { .. }
| ServerError(_)
| NoResponse
| IncompleteResponse(_)
| ParseResponse(_)
| Other(_) => true,

Expand Down

0 comments on commit 35e6d82

Please sign in to comment.