From 1355bd0ac58fed09a31ff984d7f72977c1526847 Mon Sep 17 00:00:00 2001 From: arpad-m Date: Wed, 12 Jul 2023 15:52:14 +0200 Subject: [PATCH] layer deletion: Improve a comment and fix TOCTOU (#4673) The comment referenced an issue that was already closed. Remove that reference and replace it with an explanation why we already don't print an error. See discussion in https://github.com/neondatabase/neon/issues/2934#issuecomment-1626505916 For the TOCTOU fixes, the two calls after the `.exists()` both didn't handle the situation well where the file was deleted after the initial `.exists()`: one would assume that the path wasn't a file, giving a bad error, the second would give an accurate error but that's not wanted either. We remove both racy `exists` and `is_file` checks, and instead just look for errors about files not being found. --- libs/remote_storage/src/local_fs.rs | 15 ++++++--------- .../src/tenant/remote_timeline_client/delete.rs | 7 ++++--- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/libs/remote_storage/src/local_fs.rs b/libs/remote_storage/src/local_fs.rs index ca5fbd5de5..36fd2647c5 100644 --- a/libs/remote_storage/src/local_fs.rs +++ b/libs/remote_storage/src/local_fs.rs @@ -7,6 +7,7 @@ use std::{ borrow::Cow, future::Future, + io::ErrorKind, path::{Path, PathBuf}, pin::Pin, }; @@ -343,18 +344,14 @@ impl RemoteStorage for LocalFs { async fn delete(&self, path: &RemotePath) -> anyhow::Result<()> { let file_path = path.with_base(&self.storage_root); - if !file_path.exists() { + match fs::remove_file(&file_path).await { + Ok(()) => Ok(()), + // The file doesn't exist. This shouldn't yield an error to mirror S3's behaviour. // See https://docs.aws.amazon.com/AmazonS3/latest/API/API_DeleteObject.html // > If there isn't a null version, Amazon S3 does not remove any objects but will still respond that the command was successful. - return Ok(()); + Err(e) if e.kind() == ErrorKind::NotFound => Ok(()), + Err(e) => Err(anyhow::anyhow!(e)), } - - if !file_path.is_file() { - anyhow::bail!("{file_path:?} is not a file"); - } - Ok(fs::remove_file(file_path) - .await - .map_err(|e| anyhow::anyhow!(e))?) } async fn delete_objects<'a>(&self, paths: &'a [RemotePath]) -> anyhow::Result<()> { diff --git a/pageserver/src/tenant/remote_timeline_client/delete.rs b/pageserver/src/tenant/remote_timeline_client/delete.rs index 9f6732fbff..3f505d45ab 100644 --- a/pageserver/src/tenant/remote_timeline_client/delete.rs +++ b/pageserver/src/tenant/remote_timeline_client/delete.rs @@ -19,9 +19,10 @@ pub(super) async fn delete_layer<'a>( let path_to_delete = conf.remote_path(local_layer_path)?; - // XXX: If the deletion fails because the object already didn't exist, - // it would be good to just issue a warning but consider it success. - // https://github.com/neondatabase/neon/issues/2934 + // We don't want to print an error if the delete failed if the file has + // already been deleted. Thankfully, in this situation S3 already + // does not yield an error. While OS-provided local file system APIs do yield + // errors, we avoid them in the `LocalFs` wrapper. storage.delete(&path_to_delete).await.with_context(|| { format!("Failed to delete remote layer from storage at {path_to_delete:?}") })