Improve error handling and reporting when removing cache blocks (awsl…

…abs#1043)

* Improve error handling when removing cache blocks

Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>

* Clean up ObjectId Debug implementation

Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>

---------

Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>

mountpoint-s3/src/data_cache/disk_data_cache.rs

@@ -335,11 +335,19 @@ impl DiskDataCache {
             let path_to_remove = self.get_path_for_block_key(&to_remove);
             trace!("evicting block at {}", path_to_remove.display());
             if let Err(remove_err) = fs::remove_file(&path_to_remove) {
-                warn!("unable to remove invalid block: {:?}", remove_err);
+                if remove_err.kind() != ErrorKind::NotFound {
+                    warn!("unable to evict block: {:?}", remove_err);
+                }
             }
         }
         Ok(())
     }
+
+    fn remove_block_from_usage(&self, block_key: &DiskBlockKey) {
+        if let Some(usage) = &self.usage {
+            usage.lock().unwrap().remove(block_key);
+        }
+    }
 }
 
 /// Hash the cache key using its fields as well as the [CACHE_VERSION].
@@ -389,12 +397,15 @@ impl DataCache for DiskDataCache {
                 metrics::counter!("disk_data_cache.block_hit").increment(0);
                 metrics::counter!("disk_data_cache.block_err").increment(1);
                 match fs::remove_file(&path) {
-                    Ok(()) => {
-                        if let Some(usage) = &self.usage {
-                            usage.lock().unwrap().remove(&block_key);
+                    Ok(()) => self.remove_block_from_usage(&block_key),
+                    Err(remove_err) => {
+                        // We failed to delete the block.
+                        if remove_err.kind() == ErrorKind::NotFound {
+                            // No need to report or try again.
+                            self.remove_block_from_usage(&block_key);
                         }
+                        warn!("unable to remove invalid block: {:?}", remove_err);
                     }
-                    Err(remove_err) => warn!("unable to remove invalid block: {:?}", remove_err),
                 }
                 Err(err)
             }

mountpoint-s3/src/object.rs

@@ -1,14 +1,25 @@
+use std::fmt::Debug;
+
 use mountpoint_s3_client::types::ETag;
 
 use crate::sync::Arc;
 
 /// Identifier for a specific version of an S3 object.
 /// Formed by the object key and etag. Holds its components in an [Arc], so it can be cheaply cloned.
-#[derive(Clone, Debug, Hash, PartialEq, Eq)]
+#[derive(Clone, Hash, PartialEq, Eq)]
 pub struct ObjectId {
     inner: Arc<InnerObjectId>,
 }
 
+impl Debug for ObjectId {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        f.debug_struct("ObjectId")
+            .field("key", &self.inner.key)
+            .field("etag", &self.inner.etag)
+            .finish()
+    }
+}
+
 #[derive(Debug, Hash, PartialEq, Eq)]
 struct InnerObjectId {
     key: String,