From d165b36e0369ffe9dd3c33147275dbf02254bd09 Mon Sep 17 00:00:00 2001
From: DJO <790521+Alenar@users.noreply.github.com>
Date: Fri, 26 Jan 2024 17:43:05 +0100
Subject: [PATCH 1/3] Strengthen immutable file listing

* It first find the "immutable" directory (and fails if it can't find
  it).
* It also check the file extension (for either "chunk", "primary" or
  "secondary").
---
 .../src/digesters/immutable_file.rs           | 86 +++++++++++++++----
 1 file changed, 71 insertions(+), 15 deletions(-)

diff --git a/mithril-common/src/digesters/immutable_file.rs b/mithril-common/src/digesters/immutable_file.rs
index 810438774bb..932ac0f6a66 100644
--- a/mithril-common/src/digesters/immutable_file.rs
+++ b/mithril-common/src/digesters/immutable_file.rs
@@ -1,9 +1,9 @@
 use crate::entities::{ImmutableFileName, ImmutableFileNumber};
 
+use crate::digesters::ImmutableFileListingError::MissingImmutableFolder;
 use digest::{Digest, Output};
 use std::{
     cmp::Ordering,
-    ffi::OsStr,
     fs::File,
     io,
     num::ParseIntError,
@@ -12,9 +12,23 @@ use std::{
 use thiserror::Error;
 use walkdir::WalkDir;
 
-fn is_immutable(path: &Path) -> bool {
-    let immutable = OsStr::new("immutable");
-    path.iter().any(|component| component == immutable)
+const IMMUTABLE_FILE_EXTENSIONS: [&str; 3] = ["chunk", "primary", "secondary"];
+
+fn is_immutable(entry: &walkdir::DirEntry) -> bool {
+    let is_file = entry.file_type().is_file();
+    let extension = entry.path().extension().map(|e| e.to_string_lossy());
+
+    is_file && extension.is_some_and(|e| IMMUTABLE_FILE_EXTENSIONS.contains(&e.as_ref()))
+}
+
+/// Walk the given path and return the first directory named "immutable" it finds
+fn find_immutables_dir(path_to_walk: &Path) -> Option<PathBuf> {
+    WalkDir::new(path_to_walk)
+        .into_iter()
+        .filter_entry(|e| e.file_type().is_dir())
+        .filter_map(|e| e.ok())
+        .find(|f| f.file_name() == "immutable")
+        .map(|e| e.into_path())
 }
 
 /// Represent an immutable file in a Cardano node database directory
@@ -62,6 +76,10 @@ pub enum ImmutableFileListingError {
     /// Raised when [ImmutableFile::new] fails.
     #[error("immutable file creation error")]
     ImmutableFileCreation(#[from] ImmutableFileCreationError),
+
+    /// Raised when the "immutable" folder could not be found in a file structure.
+    #[error("Couldn't find the 'immutable' folder in '{0:?}'")]
+    MissingImmutableFolder(PathBuf),
 }
 
 impl ImmutableFile {
@@ -116,18 +134,19 @@ impl ImmutableFile {
     pub fn list_completed_in_dir(
         dir: &Path,
     ) -> Result<Vec<ImmutableFile>, ImmutableFileListingError> {
+        let immutable_dir =
+            find_immutables_dir(dir).ok_or(MissingImmutableFolder(dir.to_path_buf()))?;
         let mut files: Vec<ImmutableFile> = vec![];
 
-        for path in WalkDir::new(dir)
+        for path in WalkDir::new(immutable_dir)
+            .min_depth(1)
+            .max_depth(1)
             .into_iter()
+            .filter_entry(is_immutable)
             .filter_map(|file| file.ok())
-            .map(|f| f.path().to_owned())
         {
-            let metadata = path.metadata()?;
-            if metadata.is_file() && is_immutable(&path) {
-                let immutable_file = ImmutableFile::new(path)?;
-                files.push(immutable_file);
-            }
+            let immutable_file = ImmutableFile::new(path.into_path())?;
+            files.push(immutable_file);
         }
         files.sort();
 
@@ -179,6 +198,23 @@ mod tests {
         }
     }
 
+    fn extract_filenames(immutables: &[ImmutableFile]) -> Vec<String> {
+        immutables
+            .iter()
+            .map(|i| i.path.file_name().unwrap().to_str().unwrap().to_owned())
+            .collect()
+    }
+
+    #[test]
+    fn list_immutable_file_fail_if_not_in_immutable_dir() {
+        let target_dir = get_test_dir("list_immutable_file_fail_if_not_in_immutable_dir/invalid");
+        let entries = vec![];
+        create_fake_files(&target_dir, &entries);
+
+        ImmutableFile::list_completed_in_dir(target_dir.parent().unwrap())
+            .expect_err("ImmutableFile::list_in_dir should have Failed");
+    }
+
     #[test]
     fn list_immutable_file_should_skip_last_number() {
         let target_dir = get_test_dir("list_immutable_file_should_skip_last_number/immutable");
@@ -260,12 +296,32 @@ mod tests {
         create_fake_files(&target_dir, &entries);
         let immutables = ImmutableFile::list_completed_in_dir(target_dir.parent().unwrap())
             .expect("ImmutableFile::list_in_dir Failed");
-        let immutables_names: Vec<String> = immutables
-            .into_iter()
-            .map(|i| i.path.file_name().unwrap().to_str().unwrap().to_owned())
-            .collect();
+        let immutables_names: Vec<String> = extract_filenames(&immutables);
 
         let expected: Vec<&str> = entries.into_iter().rev().skip(3).rev().collect();
         assert_eq!(expected, immutables_names);
     }
+
+    #[test]
+    fn list_immutable_file_should_work_with_non_immutable_files() {
+        let target_dir =
+            get_test_dir("list_immutable_file_should_work_with_non_immutable_files/immutable");
+        let entries = vec![
+            "123.chunk",
+            "123.primary",
+            "123.secondary",
+            "124.chunk",
+            "124.primary",
+            "124.secondary",
+            "README.md",
+            "124.secondary.back",
+        ];
+        create_fake_files(&target_dir, &entries);
+        let immutables = ImmutableFile::list_completed_in_dir(target_dir.parent().unwrap())
+            .expect("ImmutableFile::list_in_dir Failed");
+        let immutables_names: Vec<String> = extract_filenames(&immutables);
+
+        let expected: Vec<&str> = entries.into_iter().rev().skip(5).rev().collect();
+        assert_eq!(expected, immutables_names);
+    }
 }

From 50759c86b4f0caa853be17e460e2f5524c3ac2f3 Mon Sep 17 00:00:00 2001
From: DJO <790521+Alenar@users.noreply.github.com>
Date: Fri, 26 Jan 2024 18:03:07 +0100
Subject: [PATCH 2/3] Strengthen skipping last trio step when listing immutable
 files

It will skip the file(s) with the highest number instead of simply
skipping the last three files.
---
 .../src/digesters/immutable_file.rs           | 41 ++++++++++++++++++-
 1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/mithril-common/src/digesters/immutable_file.rs b/mithril-common/src/digesters/immutable_file.rs
index 932ac0f6a66..02fcdb54d71 100644
--- a/mithril-common/src/digesters/immutable_file.rs
+++ b/mithril-common/src/digesters/immutable_file.rs
@@ -150,8 +150,18 @@ impl ImmutableFile {
         }
         files.sort();
 
-        // @todo: make the skip of the last 'trio' more robust
-        Ok(files.into_iter().rev().skip(3).rev().collect())
+        match files.last() {
+            // empty list
+            None => Ok(files),
+            // filter out the last immutable file(s)
+            Some(last_file) => {
+                let last_number = last_file.number;
+                Ok(files
+                    .into_iter()
+                    .filter(|f| f.number < last_number)
+                    .collect())
+            }
+        }
     }
 }
 
@@ -324,4 +334,31 @@ mod tests {
         let expected: Vec<&str> = entries.into_iter().rev().skip(5).rev().collect();
         assert_eq!(expected, immutables_names);
     }
+
+    #[test]
+    fn list_immutable_file_can_list_incomplete_trio() {
+        let target_dir = get_test_dir("list_immutable_file_can_list_incomplete_trio/immutable");
+        let entries = vec![
+            "21.chunk",
+            "21.primary",
+            "21.secondary",
+            "123.chunk",
+            "123.secondary",
+            "124.chunk",
+            "124.primary",
+            "125.primary",
+            "125.secondary",
+            "223.chunk",
+            "224.primary",
+            "225.secondary",
+            "226.chunk",
+        ];
+        create_fake_files(&target_dir, &entries);
+        let immutables = ImmutableFile::list_completed_in_dir(target_dir.parent().unwrap())
+            .expect("ImmutableFile::list_in_dir Failed");
+        let immutables_names: Vec<String> = extract_filenames(&immutables);
+
+        let expected: Vec<&str> = entries.into_iter().rev().skip(1).rev().collect();
+        assert_eq!(expected, immutables_names);
+    }
 }

From 909936ea79a8163d91741198028c950462d49208 Mon Sep 17 00:00:00 2001
From: DJO <790521+Alenar@users.noreply.github.com>
Date: Mon, 29 Jan 2024 17:37:39 +0100
Subject: [PATCH 3/3] Update mithril-common crate version

---
 Cargo.lock                | 2 +-
 mithril-common/Cargo.toml | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Cargo.lock b/Cargo.lock
index 031f2b302e8..ca6f23efff3 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -3426,7 +3426,7 @@ dependencies = [
 
 [[package]]
 name = "mithril-common"
-version = "0.2.155"
+version = "0.2.156"
 dependencies = [
  "anyhow",
  "async-trait",
diff --git a/mithril-common/Cargo.toml b/mithril-common/Cargo.toml
index 130dac6382d..5314ea24f8f 100644
--- a/mithril-common/Cargo.toml
+++ b/mithril-common/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "mithril-common"
-version = "0.2.155"
+version = "0.2.156"
 description = "Common types, interfaces, and utilities for Mithril nodes."
 authors = { workspace = true }
 edition = { workspace = true }