From 1559ef36afb92342b888a9a4e6a20cb9fe0be022 Mon Sep 17 00:00:00 2001
From: Joonas Koivunen <joonas@neon.tech>
Date: Thu, 24 Aug 2023 22:21:49 +0300
Subject: [PATCH] fix: rename the written out file in Layer ctor

---
 pageserver/src/tenant/storage_layer/delta_layer.rs | 7 +------
 pageserver/src/tenant/storage_layer/image_layer.rs | 8 +-------
 pageserver/src/tenant/storage_layer/layer.rs       | 9 +++++++--
 3 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/pageserver/src/tenant/storage_layer/delta_layer.rs b/pageserver/src/tenant/storage_layer/delta_layer.rs
index 1afdfbd30a..42bd8fb67f 100644
--- a/pageserver/src/tenant/storage_layer/delta_layer.rs
+++ b/pageserver/src/tenant/storage_layer/delta_layer.rs
@@ -561,12 +561,7 @@ impl DeltaLayerWriterInner {
         // fsync the file
         file.sync_all()?;
 
-        let layer = Layer::for_written(self.conf, timeline, desc)?;
-        // Rename the file to its final name
-        //
-        // Note: This overwrites any existing file. There shouldn't be any.
-        // FIXME: throw an error instead?
-        std::fs::rename(self.path, layer.local_path())?;
+        let layer = Layer::for_written_tempfile(self.conf, timeline, desc, &self.path)?;
 
         trace!("created delta layer {}", layer.local_path().display());
 
diff --git a/pageserver/src/tenant/storage_layer/image_layer.rs b/pageserver/src/tenant/storage_layer/image_layer.rs
index c64c114a9c..acaf473041 100644
--- a/pageserver/src/tenant/storage_layer/image_layer.rs
+++ b/pageserver/src/tenant/storage_layer/image_layer.rs
@@ -495,13 +495,7 @@ impl ImageLayerWriterInner {
         // fsync the file
         file.sync_all()?;
 
-        let layer = Layer::for_written(self.conf, timeline, desc)?;
-
-        // Rename the file to its final name
-        //
-        // Note: This overwrites any existing file. There shouldn't be any.
-        // FIXME: throw an error instead?
-        std::fs::rename(self.path, layer.local_path())?;
+        let layer = Layer::for_written_tempfile(self.conf, timeline, desc, &self.path)?;
 
         trace!("created image layer {}", layer.local_path().display());
 
diff --git a/pageserver/src/tenant/storage_layer/layer.rs b/pageserver/src/tenant/storage_layer/layer.rs
index 9d9dc4e352..a54608aa2b 100644
--- a/pageserver/src/tenant/storage_layer/layer.rs
+++ b/pageserver/src/tenant/storage_layer/layer.rs
@@ -120,10 +120,13 @@ impl Layer {
         ResidentLayer { downloaded, owner }
     }
 
-    pub(crate) fn for_written(
+    /// Creates a Layer value for freshly written out new layer file by renaming it from a
+    /// temporary path.
+    pub(crate) fn for_written_tempfile(
         conf: &'static PageServerConf,
         timeline: &Arc<Timeline>,
         desc: PersistentLayerDesc,
+        temp_path: &Path,
     ) -> anyhow::Result<ResidentLayer> {
         let mut resident = None;
 
@@ -143,7 +146,9 @@ impl Layer {
 
         let downloaded = resident.expect("just initialized");
 
-        // FIXME: should we handle the rename?
+        // if the rename works, the path is as expected
+        std::fs::rename(temp_path, owner.local_path())
+            .context("rename temporary file as correct path for {owner}")?;
 
         Ok(ResidentLayer { downloaded, owner })
     }