Skip to content

Avoid retrying object_exists 404 responses #458

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
durch merged 1 commit into from
May 4, 2026
Merged

Avoid retrying object_exists 404 responses #458

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 66 additions & 11 deletions s3/src/bucket.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1025,17 +1025,8 @@ impl Bucket {
pub async fn object_exists<S: AsRef<str>>(&self, path: S) -> Result<bool, S3Error> {
let command = Command::HeadObject;
let request = RequestImpl::new(self, path.as_ref(), command).await?;
let response_data = match request.response_data(false).await {
Ok(response_data) => response_data,
Err(S3Error::HttpFailWithBody(status_code, error)) => {
if status_code == 404 {
return Ok(false);
}
return Err(S3Error::HttpFailWithBody(status_code, error));
}
Err(e) => return Err(e),
};
Ok(response_data.status_code() != 404)
let status_code = request.response_status().await?;
Ok(status_code != 404)
Comment on lines +1028 to +1029

@coderabbitai coderabbitai Bot May 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Only treat successful HEAD responses as existence.

response_status() returns raw non-2xx statuses when fail-on-err is disabled, so this now reports Ok(true) for cases like 403, 500, or a redirect. That hides auth/server problems as “object exists”. Please keep 404 => false, return true only for success statuses, and surface every other status as an error.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@s3/src/bucket.rs` around lines 1028 - 1029, The current check treats any
non-404 status as existence because it returns Ok(status_code != 404); change
this to return Ok(true) only when the response status is a success (2xx) and
return Ok(false) for 404, but surface any other status as an error. Locate the
call to request.response_status().await? and replace the boolean expression with
a branch that uses status.is_success() (or checks 200..=299) to return Ok(true),
explicitly handle 404 to return Ok(false), and convert all other status codes
into an Err (propagating or mapping into the function’s error type) so
auth/server errors aren’t masked.

}

#[maybe_async::maybe_async]
Expand Down Expand Up @@ -3113,11 +3104,75 @@ mod test {
use crate::{Bucket, PostPolicy};
use http::header::{CACHE_CONTROL, HeaderMap, HeaderName, HeaderValue};
use std::env;
#[cfg(all(not(feature = "sync"), feature = "with-tokio"))]
use std::io::{Read, Write};
#[cfg(all(not(feature = "sync"), feature = "with-tokio"))]
use std::net::TcpListener;
#[cfg(all(not(feature = "sync"), feature = "with-tokio"))]
use std::sync::{
Arc,
atomic::{AtomicUsize, Ordering},
};
#[cfg(all(not(feature = "sync"), feature = "with-tokio"))]
use std::thread;

fn init() {
let _ = env_logger::builder().is_test(true).try_init();
}

#[cfg(all(not(feature = "sync"), feature = "with-tokio"))]
#[tokio::test]
async fn test_object_exists_404_does_not_retry() {
init();

let listener = TcpListener::bind("127.0.0.1:0").unwrap();
let endpoint = format!("http://{}", listener.local_addr().unwrap());
let requests = Arc::new(AtomicUsize::new(0));
let request_count = Arc::clone(&requests);

let server = thread::spawn(move || {
let (mut stream, _) = listener.accept().unwrap();
request_count.fetch_add(1, Ordering::SeqCst);

let mut buffer = [0; 2048];
let _ = stream.read(&mut buffer).unwrap();
stream
.write_all(
b"HTTP/1.1 404 Not Found\r\nContent-Length: 0\r\nConnection: close\r\n\r\n",
)
.unwrap();
});

crate::set_retries(1);

let credentials = Credentials::new(
Some("test_access_key"),
Some("test_secret_key"),
None,
None,
None,
)
.unwrap();
let bucket = Bucket::new(
"test-bucket",
Region::Custom {
region: "us-east-1".to_owned(),
endpoint,
},
credentials,
)
.unwrap()
.with_path_style();

let exists = bucket.object_exists("/missing.txt").await.unwrap();

crate::set_retries(1);
server.join().unwrap();

assert!(!exists);
assert_eq!(requests.load(Ordering::SeqCst), 1);
}
Comment on lines +3123 to +3174

@coderabbitai coderabbitai Bot May 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

LGTM — test correctly enforces the no-retry invariant.

The single-accept server design is intentional and effective: if object_exists retried, the second attempt would receive ECONNREFUSED, causing .unwrap() at line 3167 to panic and failing the test. The requests counter then provides a precise assertion that only one request was made.

One note: crate::set_retries(1) at line 3169 is global state and does not restore the previous default — it just sets the same value that was already set at line 3146. If the library's default retry count is greater than 1, tests running after this one in the same process will unexpectedly inherit retries = 1. Consider saving and restoring the original value, or using a test-local mechanism if the API supports it.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@s3/src/bucket.rs` around lines 3123 - 3174, The test
test_object_exists_404_does_not_retry modifies global retry state via
crate::set_retries(1) without restoring it, which can leak into other tests;
change the test to capture the current retry count before setting (e.g., let
prev = crate::get_retries() or otherwise read the current value), call
crate::set_retries(1) for the duration of the test, and then restore the
original value at the end (crate::set_retries(prev)) — ensure restoration runs
even if the test panics by using a guard/RAII pattern or a finally-style drop
guard inside test_object_exists_404_does_not_retry so the global retries are
returned to their original value.


fn test_aws_credentials() -> Credentials {
Credentials::new(
Some(&env::var("EU_AWS_ACCESS_KEY_ID").unwrap()),
Expand Down
41 changes: 41 additions & 0 deletions s3/src/request/async_std_backend.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,47 @@ impl<'a> Request for SurfRequest<'a> {
Ok(response)
}

async fn response_status(&self) -> Result<u16, S3Error> {
crate::retry! {
async {
let headers = self.headers().await?;

let request = match self.command.http_verb() {
HttpMethod::Get => surf::Request::builder(Method::Get, self.url()?),
HttpMethod::Delete => surf::Request::builder(Method::Delete, self.url()?),
HttpMethod::Put => surf::Request::builder(Method::Put, self.url()?),
HttpMethod::Post => surf::Request::builder(Method::Post, self.url()?),
HttpMethod::Head => surf::Request::builder(Method::Head, self.url()?),
};

let mut request = request.body(self.request_body()?);

for (name, value) in headers.iter() {
request = request.header(
HeaderName::from_bytes(AsRef::<[u8]>::as_ref(&name).to_vec())
.expect("Could not parse heaeder name"),
HeaderValue::from_bytes(AsRef::<[u8]>::as_ref(&value).to_vec())
.expect("Could not parse header value"),
);
}

let response = request
.send()
.await
.map_err(|e| S3Error::Surf(e.to_string()))?;
let status = u16::from(response.status());

if status == 404 {
Ok(status)
} else if cfg!(feature = "fail-on-err") && !response.status().is_success() {
Err(S3Error::HttpFail)
} else {
Ok(status)
}
}.await
}
}

async fn response_data(&self, etag: bool) -> Result<ResponseData, S3Error> {
let mut response = crate::retry! {self.response().await}?;
let status_code = response.status();
Expand Down
37 changes: 37 additions & 0 deletions s3/src/request/blocking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,43 @@ impl<'a> Request for AttoRequest<'a> {
Ok(response)
}

fn response_status(&self) -> Result<u16, S3Error> {
crate::retry! {
{
let headers = self.headers()?;
let mut session = attohttpc::Session::new();

for (name, value) in headers.iter() {
session.header(HeaderName::from_bytes(name.as_ref())?, value.to_str()?);
}

if let Some(timeout) = self.bucket.request_timeout {
session.timeout(timeout)
}

let request = match self.command.http_verb() {
HttpMethod::Get => session.get(self.url()?),
HttpMethod::Delete => session.delete(self.url()?),
HttpMethod::Put => session.put(self.url()?),
HttpMethod::Post => session.post(self.url()?),
HttpMethod::Head => session.head(self.url()?),
};

let response = request.bytes(&self.request_body()?).send()?;
let status = response.status().as_u16();

if status == 404 {
Ok(status)
} else if cfg!(feature = "fail-on-err") && !response.status().is_success() {
let text = response.text()?;
Err(S3Error::HttpFailWithBody(status, text))
} else {
Ok(status)
}
}
}
}

fn response_data(&self, etag: bool) -> Result<ResponseData, S3Error> {
let response = crate::retry! {self.response()}?;
let status_code = response.status().as_u16();
Expand Down
4 changes: 4 additions & 0 deletions s3/src/request/request_trait.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,10 @@ pub trait Request {
#[cfg(any(feature = "with-async-std", feature = "with-tokio"))]
async fn response_data_to_stream(&self) -> Result<ResponseDataStream, S3Error>;
async fn response_header(&self) -> Result<(Self::HeaderMap, u16), S3Error>;
async fn response_status(&self) -> Result<u16, S3Error> {
let (_, status_code) = self.response_header().await?;
Ok(status_code)
}
fn datetime(&self) -> OffsetDateTime;
fn bucket(&self) -> Bucket;
fn command(&self) -> Command<'_>;
Expand Down
50 changes: 50 additions & 0 deletions s3/src/request/tokio_backend.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,56 @@ impl<'a> Request for ReqwestRequest<'a> {
Ok(response)
}

async fn response_status(&self) -> Result<u16, S3Error> {
retry! {
async {
let headers = self
.headers()
.await?
.iter()
.map(|(k, v)| {
(
reqwest::header::HeaderName::from_str(k.as_str()),
reqwest::header::HeaderValue::from_str(v.to_str().unwrap_or_default()),
)
})
.filter(|(k, v)| k.is_ok() && v.is_ok())
.map(|(k, v)| (k.unwrap(), v.unwrap()))
.collect();

let client = self.bucket.http_client();

let method = match self.command.http_verb() {
HttpMethod::Delete => reqwest::Method::DELETE,
HttpMethod::Get => reqwest::Method::GET,
HttpMethod::Post => reqwest::Method::POST,
HttpMethod::Put => reqwest::Method::PUT,
HttpMethod::Head => reqwest::Method::HEAD,
};

let request = client
.request(method, self.url()?.as_str())
.headers(headers)
.body(self.request_body()?);

let request = request.build()?;
let response = client.execute(request).await?;
let status = response.status().as_u16();

if status == 404 {
return Ok(status);
}

if cfg!(feature = "fail-on-err") && !response.status().is_success() {
let text = response.text().await?;
return Err(S3Error::HttpFailWithBody(status, text));
}

Ok(status)
}.await
}
}

async fn response_data(&self, etag: bool) -> Result<ResponseData, S3Error> {
let response = retry! {self.response().await }?;
let status_code = response.status().as_u16();
Expand Down
Loading