fixup

The Problem
-----------

Before this patch, the following could happen.
* read_dir().next() returns the unint mark entry, we delete the timeline dir and the mark
* read_dir().next() returns the timeline dir entry
  * this is totally normal, a directory iterator is not invalidated by directory modifiction
  * we see that there's no uninit mark
  * so we try to load the timeline
  * but actually, the timeline dir is gone, so we fail the load with an error like
    ```
    2023-06-09T18:43:41.664247Z ERROR load{tenant_id=X}: load failed, setting tenant state to Broken: failed to load metadata

    Caused by:
        0: Failed to read metadata bytes from path .neon/tenants/X/timelines/Y/metadata
        1: No such file or directory (os error 2)
    ```

The Fix
-------

Turn the purging of temp entries into a fix-point iteration that restarts after every purge.
Expressive, but less efficient. I'm ok with the inefficiency until it becomes a problem.

After fix-point iteration, do read-only iteration where we expect there to only
be dir entries that are valid timeline dirs.

I also took the liberty to drive-by fix two issues that have been bugging me
for some time:

1. precise error when extracting TimelineId from uninit mark file
   refs https://github.com/neondatabase/neon/issues/3488
2. Bail out instead of WARN-logging if there is directory entry in the
   timelines dir that is not a valid TimelineId.
   Bailing out means that the tenant will fail to load, i.e., it will be `Broken`.
   The situation can be fixed by operator using ignore+fix+load.
   Before this patch, we would just log a warning and continue.
   In my opinion, being strict about this is the better choice, because,
   if we somehow miss a timeline's existence, we make incorrect GC decisions.

pageserver/src/lib.rs

@@ -24,7 +24,9 @@ pub mod walredo;
 use std::path::Path;
 
 use crate::task_mgr::TaskKind;
+use anyhow::Context;
 use tracing::info;
+use utils::id::TimelineId;
 
 /// Current storage format version
 ///
@@ -123,13 +125,31 @@ pub fn is_temporary(path: &Path) -> bool {
     }
 }
 
-pub fn is_uninit_mark(path: &Path) -> bool {
+pub fn is_uninit_mark(path: &Path) -> anyhow::Result<Option<TimelineId>> {
     match path.file_name() {
-        Some(name) => name
-            .to_string_lossy()
-            .ends_with(TIMELINE_UNINIT_MARK_SUFFIX),
-        None => false,
+        Some(name) => {
+            if !name
+                .to_str()
+                .context("file name is not valid utf8")?
+                .ends_with(TIMELINE_UNINIT_MARK_SUFFIX)
+            {
+                return Ok(None);
+            } else {
+                // fallthrough
+            }
+        }
+        None => return Ok(None),
     }
+    let stem = path
+        .file_stem()
+        .context("uninit mark file has no file stem")?;
+    let stem = stem
+        .to_str()
+        .context("uninit mark file stem is not valid utf8")?;
+    let timeline_id = stem
+        .parse::<TimelineId>()
+        .context("uninit mark file stem is not a valid timeline id")?;
+    Ok(Some(timeline_id))
 }
 
 /// During pageserver startup, we need to order operations not to exhaust tokio worker threads by

pageserver/src/tenant.rs

@@ -30,6 +30,7 @@ use std::collections::BTreeSet;
 use std::collections::HashMap;
 use std::ffi::OsStr;
 use std::fs;
+use std::fs::DirEntry;
 use std::fs::File;
 use std::fs::OpenOptions;
 use std::io;
@@ -964,95 +965,84 @@ impl Tenant {
         let conf = self.conf;
         let span = info_span!("blocking");
 
+        let myself = Arc::clone(self);
         let sorted_timelines: Vec<(_, _)> = tokio::task::spawn_blocking(move || {
             let _g = span.entered();
-            let mut timelines_to_load: HashMap<TimelineId, TimelineMetadata> = HashMap::new();
             let timelines_dir = conf.timelines_path(&tenant_id);
 
-            for entry in
-                std::fs::read_dir(&timelines_dir).context("list timelines directory for tenant")?
-            {
-                let entry = entry.context("read timeline dir entry")?;
-                let timeline_dir = entry.path();
-
-                if crate::is_temporary(&timeline_dir) {
-                    info!(
-                        "Found temporary timeline directory, removing: {}",
-                        timeline_dir.display()
-                    );
-                    if let Err(e) = std::fs::remove_dir_all(&timeline_dir) {
-                        error!(
-                            "Failed to remove temporary directory '{}': {:?}",
-                            timeline_dir.display(),
-                            e
-                        );
-                    }
-                } else if is_uninit_mark(&timeline_dir) {
-                    let timeline_uninit_mark_file = &timeline_dir;
-                    info!(
-                        "Found an uninit mark file {}, removing the timeline and its uninit mark",
-                        timeline_uninit_mark_file.display()
-                    );
-                    let timeline_id = timeline_uninit_mark_file
-                        .file_stem()
-                        .and_then(OsStr::to_str)
-                        .unwrap_or_default()
-                        .parse::<TimelineId>()
-                        .with_context(|| {
-                            format!(
-                            "Could not parse timeline id out of the timeline uninit mark name {}",
-                            timeline_uninit_mark_file.display()
+            // Purge temporary directory entries until we end up with just initialized timelines.
+            let entries: Vec<DirEntry> = 'read_dir_invalidated: loop {
+                let mut entries = Vec::new();
+                'next_entry: for entry in std::fs::read_dir(&timelines_dir).with_context(|| {
+                    format!(
+                        "start timelines directory iteration for tenant {}",
+                        myself.tenant_id
+                    )
+                })? {
+                    let entry = entry.with_context(|| {
+                        format!(
+                            "continue timelines directory iteration for tenant {}",
+                            myself.tenant_id
                         )
-                        })?;
-                    let timeline_dir = conf.timeline_path(&timeline_id, &tenant_id);
-                    if let Err(e) =
-                        remove_timeline_and_uninit_mark(&timeline_dir, timeline_uninit_mark_file)
-                    {
-                        error!("Failed to clean up uninit marked timeline: {e:?}");
-                    }
-                } else {
-                    let timeline_id = timeline_dir
-                        .file_name()
-                        .and_then(OsStr::to_str)
-                        .unwrap_or_default()
-                        .parse::<TimelineId>()
-                        .with_context(|| {
-                            format!(
-                                "Could not parse timeline id out of the timeline dir name {}",
-                                timeline_dir.display()
-                            )
-                        })?;
-                    let timeline_uninit_mark_file =
-                        conf.timeline_uninit_mark_file_path(tenant_id, timeline_id);
-                    if timeline_uninit_mark_file.exists() {
+                    })?;
+                    let timeline_dir = entry.path();
+                    if crate::is_temporary(&timeline_dir) {
                         info!(
-                            %timeline_id,
-                            "Found an uninit mark file, removing the timeline and its uninit mark",
+                            "Found temporary timeline directory, removing: {}",
+                            timeline_dir.display()
                         );
-                        if let Err(e) = remove_timeline_and_uninit_mark(
-                            &timeline_dir,
-                            &timeline_uninit_mark_file,
-                        ) {
-                            error!("Failed to clean up uninit marked timeline: {e:?}");
+                        if let Err(e) = std::fs::remove_dir_all(&timeline_dir) {
+                            error!(
+                                "Failed to remove temporary directory '{}': {:?}",
+                                timeline_dir.display(),
+                                e
+                            );
                         }
-                        continue;
-                    }
-
-                    let file_name = entry.file_name();
-                    if let Ok(timeline_id) =
-                        file_name.to_str().unwrap_or_default().parse::<TimelineId>()
+                        continue 'read_dir_invalidated;
+                    } else if let Some(timeline_id) =
+                        is_uninit_mark(&timeline_dir).with_context(|| {
+                            format!(
+                                "check if dir entry is an uninit mark file: {:?}",
+                                timeline_dir
+                            )
+                        })?
                     {
-                        let metadata = load_metadata(conf, timeline_id, tenant_id)
-                            .context("failed to load metadata")?;
-                        timelines_to_load.insert(timeline_id, metadata);
-                    } else {
-                        // A file or directory that doesn't look like a timeline ID
-                        warn!(
-                            "unexpected file or directory in timelines directory: {}",
-                            file_name.to_string_lossy()
+                        let uninit_mark = timeline_dir;
+                        info!(
+                            timeline_id=%timeline_id,
+                            "Found an uninit mark file removing the timeline and its uninit mark",
                         );
+                        let timeline_dir =
+                            myself.conf.timeline_path(&timeline_id, &myself.tenant_id);
+                        remove_timeline_and_uninit_mark(&timeline_dir, &uninit_mark)?;
+                        continue 'read_dir_invalidated;
+                    } else {
+                        entries.push(entry);
+                        continue 'next_entry;
                     }
                 }
+                break entries;
+            };
+
+            let mut timelines_to_load: HashMap<TimelineId, TimelineMetadata> = HashMap::new();
+            for entry in entries {
+                let timeline_dir = entry.path();
+                assert!(!crate::is_temporary(&timeline_dir), "removed above");
+                assert!(is_uninit_mark(&timeline_dir)?.is_none(), "removed above");
+                let timeline_id = timeline_dir
+                    .file_name()
+                    .and_then(OsStr::to_str)
+                    .unwrap_or_default()
+                    .parse::<TimelineId>()
+                    .with_context(|| {
+                        format!(
+                            "Could not parse timeline id out of the timeline dir name {}",
+                            timeline_dir.display()
+                        )
+                    })?;
+                let metadata = load_metadata(myself.conf, timeline_id, myself.tenant_id)
+                    .context("failed to load metadata")?;
+                timelines_to_load.insert(timeline_id, metadata);
             }
 
             // Sort the array of timeline IDs into tree-order, so that parent comes before