diff --git a/server/src/api/public.rs b/server/src/api/public.rs index c76691d..9162d0c 100644 --- a/server/src/api/public.rs +++ b/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 {}", diff --git a/server/src/api/user.rs b/server/src/api/user.rs index d984a94..9c176d5 100644 --- a/server/src/api/user.rs +++ b/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 diff --git a/server/src/api/worker.rs b/server/src/api/worker.rs index 57c9be6..d9b3eaa 100644 --- a/server/src/api/worker.rs +++ b/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 {}", diff --git a/server/src/main.rs b/server/src/main.rs index 2cbbb74..7484645 100644 --- a/server/src/main.rs +++ b/server/src/main.rs @@ -619,15 +619,33 @@ impl Central { &self, job: JobId, file: JobFileId, - ) -> Result { + ) -> Result> { 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(