Merge pull request #58 from MilesCranmer/MilesCranmer/issue56

Fix #56 – race condition while writing `.record` from two concurrent processes

Cargo.toml

@@ -22,6 +22,7 @@ clap = { version = "4.4", features = ["derive"] }
 clap_complete = "4.4"
 clap_complete_nushell = "4.4"
 dunce = "1.0.4"
+fs4 = { version = "0.10.0", features = ["sync"] }
 fs_extra = "1.3"
 walkdir = "1"
 
@@ -30,6 +31,7 @@ assert_cmd = "1.0"
 lazy_static = "1.4"
 predicates = "3.0"
 rand = "0.8"
+regex = "1.11.0"
 rstest = "0.18"
 tempfile = "3"
 

README.md

@@ -22,9 +22,10 @@ This version, "rip2", is a fork-of-a-fork:
     - Added support for: Windows, NixOS
     - Cleanup: refactoring to modern rust, merging PRs from original repo
     - Testing: add full test suite and coverage monitoring
-    - Features: colorful output, datetime info in seance, 
+    - Features: colorful output, datetime info in seance
     - Bug fixes: FIFO files, issue with seance
     - Shell completions for bash, elvish, fish, powershell, zsh, and nushell (via clap)
+    - Thread safety for deletion record
 
 ## ⚰️ Installation
 

src/lib.rs

@@ -19,7 +19,7 @@ pub mod record;
 pub mod util;
 
 use args::Args;
-use record::{Record, RecordItem};
+use record::{Record, RecordItem, DEFAULT_FILE_LOCK};
 
 const LINES_TO_INSPECT: usize = 6;
 const FILES_TO_INSPECT: usize = 6;
@@ -42,7 +42,7 @@ pub fn run(cli: Args, mode: impl util::TestingMode, stream: &mut impl Write) ->
     }
 
     // Stores the deleted files
-    let record = Record::new(graveyard);
+    let record = Record::<DEFAULT_FILE_LOCK>::new(graveyard);
     let cwd = &env::current_dir()?;
 
     // If the user wishes to restore everything
@@ -119,10 +119,10 @@ pub fn run(cli: Args, mode: impl util::TestingMode, stream: &mut impl Write) ->
     Ok(())
 }
 
-fn bury_target(
+fn bury_target<const FILE_LOCK: bool>(
     target: &PathBuf,
     graveyard: &PathBuf,
-    record: &Record,
+    record: &Record<FILE_LOCK>,
     cwd: &Path,
     inspect: bool,
     mode: &impl util::TestingMode,

src/record.rs

@@ -1,5 +1,6 @@
 use chrono::Local;
-use std::io::{BufRead, BufReader, Error, ErrorKind, Write};
+use fs4::fs_std::FileExt;
+use std::io::{BufRead, BufReader, Error, ErrorKind, Read, Write};
 use std::path::{Path, PathBuf};
 use std::{fs, io};
 
@@ -29,13 +30,27 @@ impl RecordItem {
     }
 }
 
+/// A record of file operations maintained in the graveyard directory
+///
+/// # Type Parameters
+///
+/// * `FILE_LOCK` - When `true`, exclusive file locks are acquired when opening
+///   the record file for reading or writing. This prevents concurrent access from multiple
+///   processes. When `false`, no file locking is performed - which is used for testing.
 #[derive(Debug)]
-pub struct Record {
+pub struct Record<const FILE_LOCK: bool> {
     path: PathBuf,
 }
 
-impl Record {
-    pub fn new(graveyard: &Path) -> Record {
+#[cfg(not(target_os = "windows"))]
+pub const DEFAULT_FILE_LOCK: bool = true;
+
+#[cfg(target_os = "windows")]
+pub const DEFAULT_FILE_LOCK: bool = false;
+// TODO: Investigate why this is needed. Does Windows not support file locks?
+
+impl<const FILE_LOCK: bool> Record<FILE_LOCK> {
+    pub fn new(graveyard: &Path) -> Record<FILE_LOCK> {
         let path = graveyard.join(RECORD);
         // Create the record file if it doesn't exist
         if !path.exists() {
@@ -46,6 +61,9 @@ impl Record {
                 .write(true)
                 .open(&path)
                 .expect("Failed to open record file");
+            if FILE_LOCK {
+                record_file.lock_exclusive().unwrap();
+            }
             record_file
                 .write_all(b"Time\tOriginal\tDestination\n")
                 .expect("Failed to write header to record file");
@@ -54,8 +72,12 @@ impl Record {
     }
 
     pub fn open(&self) -> Result<fs::File, Error> {
-        fs::File::open(&self.path)
-            .map_err(|_| Error::new(ErrorKind::NotFound, "Failed to read record!"))
+        let file = fs::File::open(&self.path)
+            .map_err(|_| Error::new(ErrorKind::NotFound, "Failed to read record!"))?;
+        if FILE_LOCK {
+            file.lock_exclusive().unwrap();
+        }
+        Ok(file)
     }
 
     /// Return the path in the graveyard of the last file to be buried.
@@ -64,10 +86,8 @@ impl Record {
     pub fn get_last_bury(&self) -> Result<PathBuf, Error> {
         // record: impl AsRef<Path>
         let record_file = self.open()?;
-        let contents = {
-            let path_f = PathBuf::from(&self.path);
-            fs::read_to_string(path_f)?
-        };
+        let mut contents = String::new();
+        BufReader::new(&record_file).read_to_string(&mut contents)?;
 
         // This will be None if there is nothing, or Some
         // if there is items in the vector
@@ -109,10 +129,18 @@ impl Record {
             .map_while(Result::ok)
             .filter(|line| !graves.iter().any(|y| *y == RecordItem::new(line).dest))
             .collect();
-        let mut mutable_record_file = fs::File::create(record_path)?;
-        writeln!(mutable_record_file, "{}", header)?; // Write the header back
+        // let mut new_record_file = fs::File::create(record_path)?;
+        let mut new_record_file = fs::OpenOptions::new()
+            .create(true)
+            .truncate(true)
+            .write(true)
+            .open(record_path)?;
+        if FILE_LOCK {
+            new_record_file.lock_exclusive().unwrap();
+        }
+        writeln!(new_record_file, "{}", header)?; // Write the header back
         for line in lines_to_write {
-            writeln!(mutable_record_file, "{}", line)?;
+            writeln!(new_record_file, "{}", line)?;
         }
         Ok(())
     }
@@ -160,18 +188,28 @@ impl Record {
     pub fn write_log(&self, source: impl AsRef<Path>, dest: impl AsRef<Path>) -> io::Result<()> {
         let (source, dest) = (source.as_ref(), dest.as_ref());
 
-        // Check if record exists. If not, create it and write the header.
-        // TODO: Is this actually necessary?
-        if !self.path.exists() {
-            let mut record_file = fs::OpenOptions::new()
+        let already_existed = self.path.exists();
+
+        // TODO: The tiny amount of time between the check and the open
+        //       could allow for a race condition. But maybe I'm being overkill.
+
+        let mut record_file = if already_existed {
+            fs::OpenOptions::new().append(true).open(&self.path)?
+        } else {
+            fs::OpenOptions::new()
                 .create(true)
                 .truncate(true)
                 .write(true)
-                .open(&self.path)?;
-            writeln!(record_file, "Time\tOriginal\tDestination")?;
+                .open(&self.path)?
+        };
+
+        if FILE_LOCK {
+            record_file.lock_exclusive().unwrap();
         }
 
-        let mut record_file = fs::OpenOptions::new().append(true).open(&self.path)?;
+        if !already_existed {
+            writeln!(record_file, "Time\tOriginal\tDestination")?;
+        }
 
         writeln!(
             record_file,
@@ -190,3 +228,11 @@ impl Record {
         Ok(())
     }
 }
+
+impl<const FILE_LOCK: bool> Clone for Record<FILE_LOCK> {
+    fn clone(&self) -> Self {
+        Record {
+            path: self.path.clone(),
+        }
+    }
+}