return 404 errors when a file is not found in S3

server/src/api/public.rs

@@ -25,17 +25,21 @@ pub(crate) async fn public_file_download(
 
     let p = path.into_inner();
 
+    let not_found = || {
+        HttpError::for_client_error(
+            None,
+            StatusCode::NOT_FOUND,
+            "published file not found".into(),
+        )
+    };
+
     /*
      * Load the user from the database.
      */
     let u = if let Some(au) = c.db.user_by_name(&p.username).or_500()? {
         au.id
     } else {
-        return Err(HttpError::for_client_error(
-            None,
-            StatusCode::NOT_FOUND,
-            "published file not found".into(),
-        ));
+        return Err(not_found());
     };
 
     let pf = if let Some(pf) =
@@ -44,17 +48,16 @@ pub(crate) async fn public_file_download(
     {
         pf
     } else {
-        return Err(HttpError::for_client_error(
-            None,
-            StatusCode::NOT_FOUND,
-            "published file not found".into(),
-        ));
+        return Err(not_found());
     };
 
     let mut res = Response::builder();
     res = res.header(CONTENT_TYPE, "application/octet-stream");
 
-    let fr = c.file_response(pf.job, pf.file).await.or_500()?;
+    let Some(fr) = c.file_response(pf.job, pf.file).await.or_500()? else {
+        return Err(not_found());
+    };
+
     info!(
         log,
         "published file: user {} series {} version {} name {} is in the {}",

server/src/api/user.rs

@@ -152,7 +152,13 @@ pub(crate) async fn job_output_download(
     let mut res = Response::builder();
     res = res.header(CONTENT_TYPE, "application/octet-stream");
 
-    let fr = c.file_response(t.id, o.id).await.or_500()?;
+    let Some(fr) = c.file_response(t.id, o.id).await.or_500()? else {
+        return Err(HttpError::for_client_error(
+            None,
+            hyper::StatusCode::NOT_FOUND,
+            "output file not found".into(),
+        ));
+    };
     info!(
         log,
         "job {} output {} path {:?} is in the {}", t.id, o.id, o.path, fr.info

server/src/api/worker.rs

@@ -237,10 +237,15 @@ pub(crate) async fn worker_job_input_download(
     let mut res = Response::builder();
     res = res.header(CONTENT_TYPE, "application/octet-stream");
 
-    let fr = c
+    let Some(fr) = c
         .file_response(i.other_job.unwrap_or(i.job), i.id.unwrap())
         .await
-        .or_500()?;
+        .or_500()?
+    else {
+        return Err(HttpError::for_internal_error(
+            "input file not found".to_string(),
+        ));
+    };
     info!(
         log,
         "worker {} job {} input {} name {:?} is in the {}",

server/src/main.rs

@@ -619,15 +619,33 @@ impl Central {
         &self,
         job: JobId,
         file: JobFileId,
-    ) -> Result<FileResponse> {
+    ) -> Result<Option<FileResponse>> {
         let op = self.file_path(job, file)?;
 
-        Ok(if op.is_file() {
+        Ok(Some(if op.is_file() {
             /*
              * The file exists locally.
              */
             let info = format!("local file system at {:?}", op);
-            let f = tokio::fs::File::open(op).await?;
+            let f = match tokio::fs::File::open(&op).await {
+                Ok(f) => f,
+                // The the path doesn't exist. Return `None` rather than an
+                // error, so that the API can respond with 404 rather than 500.
+                //
+                // N.B. that this isn't a particularly common case, since we
+                // just checked that the path exists and will only attempt to
+                // open it if it does. However, the file *could* have been
+                // removed since when we checked if it exists, so do the right
+                // thing in this case anyway.
+                Err(e) if e.kind() == std::io::ErrorKind::NotFound => {
+                    return Ok(None);
+                }
+                Err(e) => {
+                    return Err(e).with_context(|| {
+                        format!("job {job} file {file}: failed to open {op:?}")
+                    })
+                }
+            };
             let md = f.metadata().await?;
             assert!(md.is_file());
             let fbs = FileBytesStream::new(f);
@@ -639,20 +657,36 @@ impl Central {
              */
             let key = self.file_object_key(job, file);
             let info = format!("object store at {}", key);
-            let obj = self
+            let res = self
                 .s3
                 .get_object()
                 .bucket(&self.config.storage.bucket)
                 .key(key)
                 .send()
-                .await?;
-
+                .await;
+            let obj = match res {
+                // The object doesn't exist. Return `None` rather than an error
+                // so that the API can respond with a 404 rather than 500.
+                Err(aws_smithy_http::result::SdkError::ServiceError(e))
+                    if e.err().is_no_such_key() =>
+                {
+                    return Ok(None);
+                }
+                Err(e) => {
+                    return Err(e).with_context(|| {
+                        format!(
+                        "job {job} file {file}: failed to get file {op:?} from S3"
+                    )
+                    })
+                }
+                Ok(obj) => obj,
+            };
             FileResponse {
                 info,
                 size: obj.content_length.try_into().unwrap(),
                 body: Body::wrap_stream(obj.body),
             }
-        })
+        }))
     }
 
     fn complete_job(