diff --git a/crates/idx-core/src/investor.rs b/crates/idx-core/src/investor.rs index 3d233d1..0b34596 100644 --- a/crates/idx-core/src/investor.rs +++ b/crates/idx-core/src/investor.rs @@ -54,32 +54,23 @@ impl Investor { fn lock_file(&self, exclusive: bool) -> Result { // Every path to one database (symlinks included) shares one lock. let real = db::resolve_db_path(&self.db_path()?)?; - let mut lock_name = real.into_os_string(); - lock_name.push(".lock"); - let file = fs::OpenOptions::new() - .create(true) - .truncate(false) - .write(true) - .open(PathBuf::from(lock_name)) - .map_err(|e| IdxError::Io(format!("failed to open ownership lock: {e}")))?; - if exclusive { - file.lock() - } else { - file.lock_shared() - } - .map_err(|e| IdxError::Io(format!("failed to lock ownership database: {e}")))?; - Ok(file) + lock_at(&real, exclusive) } fn write_lock(&self) -> Result { self.lock_file(true) } - /// A read connection holding the shared lock for its lifetime. Where no - /// lock file can be created (a read-only filesystem), nothing can replace - /// the database either, so reads go ahead unlocked. + /// A read connection holding the shared lock for its lifetime. fn connect(&self) -> Result { - let lock = self.lock_file(false).ok(); + let real = db::resolve_db_path(&self.db_path()?)?; + let lock = match lock_at(&real, false) { + Ok(lock) => Some(lock), + // No lock file and none can be created (a read-only filesystem): + // no writer can lock it to replace the database either. + Err(_) if unlockable(&real) => None, + Err(err) => return Err(err), + }; Ok(ReadConnection { conn: self.open_db()?, _lock: lock, @@ -258,10 +249,7 @@ impl Investor { force: bool, ) -> Result { let manifest_source = snapshot::resolve_manifest_source(manifest)?; - let db_path = self.db_path()?; - snapshot::sync_snapshot(&manifest_source, &db_path, force, |exclusive| { - self.lock_file(exclusive) - }) + snapshot::sync_snapshot(&manifest_source, &self.db_path()?, force, lock_at) } /// Imports one report. A release already imported (same SHA-256) is @@ -459,6 +447,47 @@ pub struct OwnershipStatus { pub releases: Vec, } +fn lock_path(db_path: &Path) -> PathBuf { + let mut name = db_path.as_os_str().to_os_string(); + name.push(".lock"); + PathBuf::from(name) +} + +/// Locks `.lock` beside the resolved database path. A shared lock does +/// not need write access, so readers fall back to opening an existing lock +/// file read-only (e.g. one created by another user). +fn lock_at(db_path: &Path, exclusive: bool) -> Result { + let path = lock_path(db_path); + let opened = fs::OpenOptions::new() + .create(true) + .truncate(false) + .write(true) + .open(&path); + let file = match opened { + Ok(file) => file, + Err(_) if !exclusive && path.exists() => fs::File::open(&path) + .map_err(|e| IdxError::Io(format!("failed to open ownership lock: {e}")))?, + Err(e) => return Err(IdxError::Io(format!("failed to open ownership lock: {e}"))), + }; + if exclusive { + file.lock() + } else { + file.lock_shared() + } + .map_err(|e| IdxError::Io(format!("failed to lock ownership database: {e}")))?; + Ok(file) +} + +/// True when the lock file is absent and cannot be created. +fn unlockable(db_path: &Path) -> bool { + let path = lock_path(db_path); + !path.exists() + && matches!( + fs::OpenOptions::new().create(true).truncate(false).write(true).open(&path), + Err(e) if matches!(e.kind(), std::io::ErrorKind::PermissionDenied | std::io::ErrorKind::ReadOnlyFilesystem) + ) +} + struct ReadConnection { // Dropped (closed) before the lock it is declared ahead of. conn: Connection, diff --git a/crates/idx-core/src/ownership/snapshot.rs b/crates/idx-core/src/ownership/snapshot.rs index 781a691..9c4e6fb 100644 --- a/crates/idx-core/src/ownership/snapshot.rs +++ b/crates/idx-core/src/ownership/snapshot.rs @@ -144,14 +144,15 @@ pub fn parse_manifest(raw: &str) -> Result Ok(manifest) } -/// `lock(exclusive)` takes the database's lock: shared while inspecting the -/// local database, exclusive while rechecking and installing, and never held -/// during the downloads. +/// `lock(resolved_db_path, exclusive)` takes the database's lock: shared +/// while inspecting the local database, exclusive while rechecking and +/// installing, and never held during the downloads. The path is resolved +/// once, so the lock and the replaced file are always the same database. pub fn sync_snapshot( manifest_source: &str, db_path: &Path, force: bool, - lock: impl Fn(bool) -> Result, + lock: impl Fn(&Path, bool) -> Result, ) -> Result { // Results report the configured path; files are read and replaced at the // resolved one, so a symlinked path updates its target instead of being @@ -161,7 +162,7 @@ pub fn sync_snapshot( let db_path = resolved_db_path.as_path(); let manifest = fetch_manifest(manifest_source)?; let local_state = { - let _shared = lock(false)?; + let _shared = lock(db_path, false)?; inspect_local_db(db_path)? }; let decision = build_sync_decision(local_state.as_ref(), &manifest.snapshot, force); @@ -215,7 +216,7 @@ pub fn sync_snapshot( let install_result = (|| -> Result<(), IdxError> { validate_snapshot_db(&temp_path, &manifest.snapshot)?; - let _lock = lock(true)?; + let _lock = lock(db_path, true)?; // The decision was made before the download; an import or edit that // landed since must not be overwritten without --force. if !force && !same_local_state(local_state.as_ref(), inspect_local_db(db_path)?.as_ref()) {