Draft for issue kstrafe#17

src/lib.rs

@@ -394,6 +394,25 @@ pub struct FileRotate<S: SuffixScheme> {
     mode: Option<u32>,
 }
 
+/// Error for the function `move_file_with_suffix` - in order to
+#[derive(Debug)]
+enum MoveFileError {
+    Io(io::Error),
+    /// Safeguard for the assumption that the rotated files will not be moved/deleted
+    /// by an external process / user:
+    /// If the condition is true, that has likely happened and thus we need to call
+    /// `scan_suffixes` before trying again.
+    ///
+    /// If we were to assume said assumption, we would only call `scan_suffixes`
+    /// in FileRotate::new().
+    SuffixScanNeeded,
+}
+impl From<io::Error> for MoveFileError {
+    fn from(e: io::Error) -> Self {
+        Self::Io(e)
+    }
+}
+
 impl<S: SuffixScheme> FileRotate<S> {
     /// Create a new [FileRotate].
     ///
@@ -507,17 +526,23 @@ impl<S: SuffixScheme> FileRotate<S> {
 
     /// Recursive function that keeps moving files if there's any file name collision.
     /// If `suffix` is `None`, it moves from basepath to next suffix given by the SuffixScheme
-    /// Assumption: Any collision in file name is due to an old log file.
     ///
     /// Returns the suffix of the new file (the last suffix after possible cascade of renames).
     fn move_file_with_suffix(
         &mut self,
         old_suffix_info: Option<SuffixInfo<S::Repr>>,
-    ) -> io::Result<SuffixInfo<S::Repr>> {
+    ) -> Result<SuffixInfo<S::Repr>, MoveFileError> {
+        let old_path = match old_suffix_info {
+            Some(ref suffix) => suffix.to_path(&self.basepath),
+            None => self.basepath.clone(),
+        };
+        if !old_path.exists() {
+            return Err(MoveFileError::SuffixScanNeeded);
+        }
+
         // NOTE: this newest_suffix is there only because AppendTimestamp specifically needs
         // it. Otherwise it might not be necessary to provide this to `rotate_file`. We could also
         // have passed the internal BTreeMap itself, but it would require to make SuffixInfo `pub`.
-
         let newest_suffix = self.suffixes.iter().next().map(|info| &info.suffix);
 
         let new_suffix = self.suffix_scheme.rotate_file(
@@ -551,10 +576,9 @@ impl<S: SuffixScheme> FileRotate<S> {
             new_suffix_info
         };
 
-        let old_path = match old_suffix_info {
-            Some(suffix) => suffix.to_path(&self.basepath),
-            None => self.basepath.clone(),
-        };
+        if new_path.exists() {
+            return Err(MoveFileError::SuffixScanNeeded);
+        }
 
         // Do the move
         assert!(old_path.exists());
@@ -569,8 +593,20 @@ impl<S: SuffixScheme> FileRotate<S> {
 
         let _ = self.file.take();
 
-        // This function will always create a new file. Returns suffix of that file
-        let new_suffix_info = self.move_file_with_suffix(None)?;
+        // `move_file_with_suffix` will always (on success) create a new file and return the suffix
+        // of that file.
+        // We need a loop here in case it fails for the specific reason that `self.suffixes` is not
+        // up-to-date, and we need another scan before moving on.
+        // The only reason for such a discrepancy is external interventions in the filesystem.
+        let new_suffix_info = loop {
+            match self.move_file_with_suffix(None) {
+                Ok(x) => break x,
+                Err(MoveFileError::SuffixScanNeeded) => {
+                    self.scan_suffixes();
+                }
+                Err(MoveFileError::Io(e)) => return Err(e),
+            }
+        };
         self.suffixes.insert(new_suffix_info);
 
         self.open_file();

src/tests.rs

@@ -1,4 +1,5 @@
 use super::{suffix::*, *};
+use std::iter::FromIterator;
 #[cfg(unix)]
 use std::os::unix::fs::PermissionsExt;
 use tempdir::TempDir;
@@ -371,6 +372,84 @@ fn unix_file_permissions() {
     }
 }
 
+#[test]
+fn user_moves_log_files() {
+    // FileRotate will be able to recover if its memory of what files exist (`self.suffixes`) turns
+    // out to not be consistent with the files that are actually on disk... User or external
+    // process has changed the file listing.
+
+    let initial_suffixes = [1, 2, 3];
+
+    #[derive(Debug)]
+    enum Action {
+        Create(usize),
+        Remove(usize),
+    }
+    use Action::*;
+
+    let actions = [Create(4), Remove(1), Remove(2), Remove(3)];
+
+    for action in actions {
+        println!("{:?}", action);
+        let tmp_dir = TempDir::new("file-rotate-test").unwrap();
+        let dir = tmp_dir.path();
+        let log_path = dir.join("log");
+        for suffix in initial_suffixes {
+            File::create(dir.join(format!("log.{}", suffix))).unwrap();
+        }
+
+        let mut log = FileRotate::new(
+            &log_path,
+            AppendCount::new(5),
+            ContentLimit::Bytes(1),
+            Compression::None,
+            #[cfg(unix)]
+            None,
+        );
+
+        match action {
+            Create(suffix) => {
+                let name = format!("log.{}", suffix);
+                File::create(dir.join(name)).unwrap();
+            }
+            Remove(suffix) => {
+                let name = format!("log.{}", suffix);
+                std::fs::remove_file(dir.join(name)).unwrap();
+            }
+        }
+
+        write!(log, "12").unwrap();
+
+        let mut expected_set = BTreeSet::from_iter(initial_suffixes.iter().map(|x| SuffixInfo {
+            suffix: *x,
+            compressed: false,
+        }));
+        match action {
+            Create(4) => {
+                // The one that was created
+                expected_set.insert(SuffixInfo {
+                    suffix: 4,
+                    compressed: false,
+                });
+                // An additional one due to rotation
+                expected_set.insert(SuffixInfo {
+                    suffix: 5,
+                    compressed: false,
+                });
+                assert_eq!(log.suffixes, expected_set);
+            }
+            // There is only one Create(_) action that is reasonable to test
+            Create(_) => unreachable!(),
+            Remove(_) => {
+                // No matter which file we remove, we would expect rotation to create one
+                // additional file such that we end up with the same files as we started.
+                assert_eq!(expected_set, log.suffixes);
+            }
+        }
+        println!("OK");
+    }
+}
+
 #[quickcheck_macros::quickcheck]
 fn arbitrary_lines(count: usize) {
     let tmp_dir = TempDir::new("file-rotate-test").unwrap();