EphemeralFile: panic if removal fails

This requires us to be disciplined about dropping
all EphemeralFile objects (=> InMemoryLayer objects)
before removing the timeline / tenant dir on disk.

If we don't do that, we'll panic inside the EphemeralFile::drop.

We know that detach doesn't honor this, so, we cannot
ship this patch just yet. But it's good to have it as an
aspirational goal.

pageserver/src/tenant/ephemeral_file.rs

@@ -39,7 +39,7 @@ pub struct EphemeralFile {
     file_id: u64,
     _tenant_id: TenantId,
     _timeline_id: TimelineId,
-    file: Arc<VirtualFile>,
+    file: Option<Arc<VirtualFile>>,
 
     pub size: u64,
 }
@@ -73,11 +73,20 @@ impl EphemeralFile {
         let file_rc = Arc::new(file);
         l.files.insert(file_id, file_rc.clone());
 
+        #[cfg(debug_assertions)]
+        debug!(
+            "created ephemeral file {}\n{}",
+            filename.display(),
+            std::backtrace::Backtrace::force_capture()
+        );
+        #[cfg(not(debug_assertions))]
+        debug!("created ephemeral file {}", filename.display());
+
         Ok(EphemeralFile {
             file_id,
             _tenant_id: tenant_id,
             _timeline_id: timeline_id,
-            file: file_rc,
+            file: Some(file_rc),
             size: 0,
         })
     }
@@ -87,6 +96,8 @@ impl EphemeralFile {
         while off < PAGE_SZ {
             let n = self
                 .file
+                .as_ref()
+                .unwrap()
                 .read_at(&mut buf[off..], blkno as u64 * PAGE_SZ as u64 + off as u64)?;
 
             if n == 0 {
@@ -269,17 +280,43 @@ impl Drop for EphemeralFile {
         cache.drop_buffers_for_ephemeral(self.file_id);
 
         // remove entry from the hash map
-        EPHEMERAL_FILES.write().unwrap().files.remove(&self.file_id);
+        let virtual_file = EPHEMERAL_FILES
+            .write()
+            .unwrap()
+            .files
+            .remove(&self.file_id)
+            .unwrap();
+
+        // remove file from self
+        let self_file = self.file.take().unwrap();
+
+        assert_eq!(
+            Arc::as_ptr(&virtual_file) as *const (),
+            Arc::as_ptr(&self_file) as *const ()
+        );
+        drop(self_file);
+
+        // XXX once we upgrade to Rust 1.70, use Arc::into_inner.
+        // It does the following checks atomically.
+        assert_eq!(Arc::weak_count(&virtual_file), 0);
+        let virtual_file = Arc::try_unwrap(virtual_file).expect(
+            "we are being dropped and EPHEMERAL_FILES is the only other place where we put the Arc",
+        );
 
         // unlink the file
-        let res = std::fs::remove_file(&self.file.path);
-        if let Err(e) = res {
-            warn!(
-                "could not remove ephemeral file '{}': {}",
-                self.file.path.display(),
-                e
-            );
-        }
+        // TODO: we should be able to unwrap here, but, timeline delete and tenant detach do
+        //       std::fs::remove_dir_all without dropping all InMemoryLayer => EphemeralFile
+        //       of the tenant => need to fix that first.
+        match virtual_file.remove() {
+            Ok(()) => (),
+            Err((virtual_file, e)) => {
+                warn!(
+                    "could not remove ephemeral file '{}': {}",
+                    virtual_file.path.display(),
+                    e
+                );
+            }
+        };
     }
 }
 

pageserver/src/tenant/storage_layer/delta_layer.rs

@@ -917,7 +917,7 @@ impl Drop for DeltaLayerWriter {
     fn drop(&mut self) {
         if let Some(inner) = self.inner.take() {
             match inner.blob_writer.into_inner().into_inner() {
-                Ok(vfile) => vfile.remove(),
+                Ok(vfile) => vfile.remove().unwrap(),
                 Err(err) => warn!(
                     "error while flushing buffer of image layer temporary file: {}",
                     err

pageserver/src/tenant/storage_layer/image_layer.rs

@@ -709,7 +709,7 @@ impl ImageLayerWriter {
 impl Drop for ImageLayerWriter {
     fn drop(&mut self) {
         if let Some(inner) = self.inner.take() {
-            inner.blob_writer.into_inner().remove();
+            inner.blob_writer.into_inner().remove().unwrap();
         }
     }
 }

pageserver/src/virtual_file.rs

@@ -324,16 +324,8 @@ impl VirtualFile {
         Ok(result)
     }
 
-    pub fn remove(self) {
-        let path = self.path.clone();
-        drop(self);
-        std::fs::remove_file(path).expect("failed to remove the virtual file");
-    }
-}
-
-impl Drop for VirtualFile {
-    /// If a VirtualFile is dropped, close the underlying file if it was open.
-    fn drop(&mut self) {
+    /// Idempotently close the file descriptor we might have or have not open for this VirtualFile.
+    pub fn close(&mut self) {
         let handle = self.handle.get_mut().unwrap();
 
         // We could check with a read-lock first, to avoid waiting on an
@@ -351,6 +343,26 @@ impl Drop for VirtualFile {
                 .observe_closure_duration(|| slot_guard.file.take());
         }
     }
+
+    /// Caller can retry if we return an `Err`.
+    #[allow(clippy::result_large_err)]
+    pub fn remove(mut self) -> Result<(), (Self, std::io::Error)> {
+        // close our fd before unlink system call, so that the unlink actually performs the removal
+        self.close();
+        // Try to remove file on disk.
+        // If it fails, we idempotently closed the fd, but the caller can choose to retry.
+        match std::fs::remove_file(&self.path) {
+            Ok(()) => Ok(()),
+            Err(e) => Err((self, e)),
+        }
+    }
+}
+
+impl Drop for VirtualFile {
+    /// If a VirtualFile is dropped, close the underlying file if it was open.
+    fn drop(&mut self) {
+        self.close();
+    }
 }
 
 impl Read for VirtualFile {