From ffbf5805e75166fccc4dafcc2872acce48207329 Mon Sep 17 00:00:00 2001 From: Rabindra Dhakal Date: Wed, 11 Feb 2026 20:58:51 +0545 Subject: [PATCH] feat(lock): add locking for concurrent process safety --- Cargo.toml | 2 +- crates/soar-cli/src/install.rs | 70 +++++++++++- crates/soar-utils/src/error.rs | 23 ++++ crates/soar-utils/src/fs.rs | 2 +- crates/soar-utils/src/lib.rs | 1 + crates/soar-utils/src/lock.rs | 193 +++++++++++++++++++++++++++++++++ 6 files changed, 283 insertions(+), 8 deletions(-) create mode 100644 crates/soar-utils/src/lock.rs diff --git a/Cargo.toml b/Cargo.toml index 10e96f21..5f9f4d6f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -37,7 +37,7 @@ image = { version = "0.25.9", default-features = false, features = ["png"] } landlock = "0.4.4" libsqlite3-sys = { version = ">=0.30.1,<0.36.0", features = [ "bundled" ]} miette = { version = "7.6.0", features = ["fancy"] } -nix = { version = "0.30.1", features = ["ioctl", "term", "user"] } +nix = { version = "0.30.1", features = ["fs", "ioctl", "term", "user"] } percent-encoding = "2.3.2" rayon = "1.11.0" regex = { version = "1.12.2", default-features = false, features = [ diff --git a/crates/soar-cli/src/install.rs b/crates/soar-cli/src/install.rs index 892fbdeb..049e2aae 100644 --- a/crates/soar-cli/src/install.rs +++ b/crates/soar-cli/src/install.rs @@ -7,6 +7,7 @@ use std::{ atomic::{AtomicU64, Ordering}, Arc, Mutex, }, + time::Duration, }; use indicatif::{MultiProgress, ProgressBar, ProgressStyle}; @@ -25,13 +26,14 @@ use soar_core::{ SoarResult, }; use soar_db::repository::{ - core::{CoreRepository, SortDirection}, + core::{CoreRepository, InstalledPackageWithPortable, SortDirection}, metadata::MetadataRepository, }; use soar_dl::types::Progress; use soar_package::integrate_package; use soar_utils::{ hash::{calculate_checksum, hash_string}, + lock::FileLock, pattern::apply_sig_variants, }; use tabled::{ @@ -1038,11 +1040,14 @@ async fn spawn_installation_task( match result { Ok((install_dir, symlinks)) => { - installed_indices - .lock() - .unwrap() - .insert(idx, (install_dir, symlinks)); - installed_count.fetch_add(1, Ordering::Relaxed); + // Only count as installed if actually installed (not skipped) + if !install_dir.as_os_str().is_empty() { + installed_indices + .lock() + .unwrap() + .insert(idx, (install_dir, symlinks)); + installed_count.fetch_add(1, Ordering::Relaxed); + } total_pb.inc(1); let _ = remove_old_versions(&target.package, &core_db, false); @@ -1085,6 +1090,59 @@ pub async fn install_single_package( target.package.repo_name, target.package.version ); + + let mut lock_attempts = 0; + let _package_lock = loop { + match FileLock::try_acquire(&target.package.pkg_name) { + Ok(Some(lock)) => break Ok(lock), + Ok(None) => { + lock_attempts += 1; + if lock_attempts == 1 { + info!( + "Waiting for lock on '{}' (another process is installing)...", + target.package.pkg_name + ); + } + tokio::time::sleep(Duration::from_millis(500)).await; + } + Err(err) => { + break Err(err); + } + } + } + .map_err(|e| SoarError::Custom(format!("Failed to acquire package lock: {}", e)))?; + + debug!( + "acquired lock for '{}' after {} attempts", + target.package.pkg_name, lock_attempts + ); + + // Re-check if package is already installed after acquiring lock + let freshly_installed: Option = core_db + .with_conn(|conn| { + CoreRepository::list_filtered( + conn, + Some(&target.package.repo_name), + Some(&target.package.pkg_name), + Some(&target.package.pkg_id), + Some(&target.package.version), + Some(true), + None, + None, + Some(SortDirection::Asc), + ) + })? + .into_iter() + .find(|ip| ip.is_installed); + + if let Some(ref pkg) = freshly_installed { + info!( + "{}#{}:{} ({}) is already installed - skipping", + pkg.pkg_name, pkg.pkg_id, pkg.repo_name, pkg.version + ); + return Ok((PathBuf::new(), Vec::new())); + } + let bin_dir = get_config().get_bin_path()?; let dir_suffix: String = target diff --git a/crates/soar-utils/src/error.rs b/crates/soar-utils/src/error.rs index b995b5f0..01916e30 100644 --- a/crates/soar-utils/src/error.rs +++ b/crates/soar-utils/src/error.rs @@ -31,6 +31,24 @@ pub enum HashError { }, } +/// Errors that can occur when working with locks. +#[derive(Debug, Diagnostic, Error)] +pub enum LockError { + #[error(transparent)] + #[diagnostic( + code(soar_utils::lock::io), + help("Check if you have write permissions to the lock directory") + )] + Io(#[from] std::io::Error), + + #[error("Failed to acquire lock for '{0}'")] + #[diagnostic( + code(soar_utils::lock::acquire_failed), + help("Check if the lock directory exists and you have write permissions") + )] + AcquireFailed(String), +} + /// Error type for path operations. #[derive(Error, Diagnostic, Debug)] pub enum PathError { @@ -360,6 +378,10 @@ pub enum UtilsError { #[diagnostic(transparent)] Bytes(#[from] BytesError), + #[error(transparent)] + #[diagnostic(transparent)] + Lock(#[from] LockError), + #[error(transparent)] #[diagnostic(transparent)] Path(#[from] PathError), @@ -376,6 +398,7 @@ pub enum UtilsError { pub type BytesResult = std::result::Result; pub type FileSystemResult = std::result::Result; pub type HashResult = std::result::Result; +pub type LockResult = std::result::Result; pub type PathResult = std::result::Result; pub type UtilsResult = std::result::Result; diff --git a/crates/soar-utils/src/fs.rs b/crates/soar-utils/src/fs.rs index 93fdb1f4..3a72166b 100644 --- a/crates/soar-utils/src/fs.rs +++ b/crates/soar-utils/src/fs.rs @@ -40,7 +40,7 @@ pub fn safe_remove>(path: P) -> FileSystemResult<()> { return Err(FileSystemError::RemoveFile { path: path.to_path_buf(), source: e, - }) + }); } }; diff --git a/crates/soar-utils/src/lib.rs b/crates/soar-utils/src/lib.rs index 72d85f9b..640b1e88 100644 --- a/crates/soar-utils/src/lib.rs +++ b/crates/soar-utils/src/lib.rs @@ -2,6 +2,7 @@ pub mod bytes; pub mod error; pub mod fs; pub mod hash; +pub mod lock; pub mod path; pub mod pattern; pub mod system; diff --git a/crates/soar-utils/src/lock.rs b/crates/soar-utils/src/lock.rs new file mode 100644 index 00000000..584354d8 --- /dev/null +++ b/crates/soar-utils/src/lock.rs @@ -0,0 +1,193 @@ +//! File-based locking mechanism for preventing concurrent operations. +//! +//! This module provides a simple file-based lock using a `.lock` file to ensure +//! that only one process can operate on a specific resource at a time. + +use std::{ + fs::{self, File, OpenOptions}, + path::{Path, PathBuf}, +}; + +use crate::error::{LockError, LockResult}; + +/// A file-based lock using `flock`. +/// +/// The lock is automatically released when `FileLock` is dropped. +pub struct FileLock { + _file: nix::fcntl::Flock, + path: PathBuf, +} + +impl FileLock { + /// Get the default lock directory for soar. + /// + /// Uses `$XDG_RUNTIME_DIR/soar/locks` or falls back to `/tmp/soar-locks`. + fn lock_dir() -> LockResult { + let xdg_runtime = std::env::var("XDG_RUNTIME_DIR").ok(); + let base = if let Some(ref runtime) = xdg_runtime { + PathBuf::from(runtime) + } else { + std::env::temp_dir() + }; + + let lock_dir = base.join("soar").join("locks"); + + if !lock_dir.exists() { + fs::create_dir_all(&lock_dir)?; + } + + Ok(lock_dir) + } + + /// Generate a lock file path for a package. + fn lock_path(name: &str) -> LockResult { + let lock_dir = Self::lock_dir()?; + + // Sanitize the package name to ensure a valid filename + let sanitize = |s: &str| { + s.chars() + .map(|c| { + if c.is_alphanumeric() || c == '-' || c == '_' || c == '.' { + c + } else { + '_' + } + }) + .collect::() + }; + + let filename = format!("{}.lock", sanitize(name)); + Ok(lock_dir.join(filename)) + } + + /// Acquire an exclusive lock on a package. + /// + /// This will block until the lock can be acquired. + /// + /// # Arguments + /// + /// * `name` - Package name + /// + /// # Returns + /// + /// Returns a `FileLock` that will automatically release the lock when dropped. + pub fn acquire(name: &str) -> LockResult { + let lock_path = Self::lock_path(name)?; + + let file = OpenOptions::new() + .write(true) + .create(true) + .truncate(false) + .open(&lock_path)?; + + let file = nix::fcntl::Flock::lock(file, nix::fcntl::FlockArg::LockExclusive).map_err( + |(_, err)| LockError::AcquireFailed(format!("{}: {}", lock_path.display(), err)), + )?; + + Ok(FileLock { + path: lock_path, + _file: file, + }) + } + + /// Try to acquire an exclusive lock without blocking. + /// + /// Returns `None` if the lock is already held by another process. + /// + /// # Arguments + /// + /// * `name` - Package name + pub fn try_acquire(name: &str) -> LockResult> { + let lock_path = Self::lock_path(name)?; + + let file = OpenOptions::new() + .write(true) + .create(true) + .truncate(false) + .open(&lock_path)?; + + match nix::fcntl::Flock::lock(file, nix::fcntl::FlockArg::LockExclusiveNonblock) { + Ok(file) => { + Ok(Some(FileLock { + path: lock_path, + _file: file, + })) + } + Err((_, err)) => { + if matches!(err, nix::errno::Errno::EWOULDBLOCK) { + return Ok(None); + } + Err(LockError::AcquireFailed(format!( + "{}: {}", + lock_path.display(), + err + ))) + } + } + } + + /// Get the path to the lock file. + pub fn path(&self) -> &Path { + &self.path + } +} + +#[cfg(test)] +mod tests { + use std::{thread, time::Duration}; + + use super::*; + + #[test] + fn test_lock_path_generation() { + let path = FileLock::lock_path("test-pkg").unwrap(); + assert!(path.to_string_lossy().ends_with("test-pkg.lock")); + } + + #[test] + fn test_lock_sanitization() { + let path = FileLock::lock_path("test/pkg").unwrap(); + assert!(path.to_string_lossy().contains("test_pkg")); + } + + #[test] + fn test_exclusive_lock() { + let lock1 = FileLock::acquire("test-exclusive").unwrap(); + + let lock2 = FileLock::try_acquire("test-exclusive").unwrap(); + assert!(lock2.is_none(), "Should not be able to acquire lock"); + + drop(lock1); + + let lock3 = FileLock::try_acquire("test-exclusive").unwrap(); + assert!( + lock3.is_some(), + "Should be able to acquire lock after release" + ); + } + + #[test] + fn test_concurrent_locks_different_packages() { + let lock1 = FileLock::acquire("pkg-a").unwrap(); + let lock2 = FileLock::acquire("pkg-b").unwrap(); + + assert!(lock1.path() != lock2.path()); + } + + #[test] + fn test_lock_blocks_until_released() { + let lock1 = FileLock::acquire("test-block").unwrap(); + let path = lock1.path().to_path_buf(); + + let handle = thread::spawn(move || { + let lock2 = FileLock::acquire("test-block").unwrap(); + assert_eq!(lock2.path(), &path); + }); + + thread::sleep(Duration::from_millis(100)); + + drop(lock1); + + handle.join().unwrap(); + } +}