From d1f2ad43e785a44805ba273859ec1eddcaafc937 Mon Sep 17 00:00:00 2001 From: David Harks Date: Thu, 10 Sep 2026 12:14:07 -0500 Subject: [PATCH 1/3] Handle a race condition which can occur when multiple `ozy`s run in parallel. --- Cargo.lock | 2 +- Cargo.toml | 2 +- src/app.rs | 238 ++++++++++++++++++++++++++++++++++++++++++++++------- 3 files changed, 208 insertions(+), 34 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a061b0b..cc521d9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1245,7 +1245,7 @@ dependencies = [ [[package]] name = "ozy" -version = "0.2.0" +version = "0.2.2" dependencies = [ "anyhow", "bzip2 0.6.1", diff --git a/Cargo.toml b/Cargo.toml index be2499c..35e1d8e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "ozy" -version = "0.2.1" +version = "0.2.2" edition = "2024" # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html diff --git a/src/app.rs b/src/app.rs index e809c38..bd55f1b 100644 --- a/src/app.rs +++ b/src/app.rs @@ -2,7 +2,9 @@ use anyhow::{Context, Error, Result, anyhow}; use file_lock::{FileLock, FileOptions}; use crate::config::{apply_overrides, resolve}; -use crate::files::{delete_if_exists, get_ozy_cache_dir, remove_installed_version, versions_in}; +use crate::files::{ + delete_if_exists, get_ozy_cache_dir, remove_installed_version, trash_and_remove, versions_in, +}; use crate::installers::conda::Conda; use crate::installers::file::File; use crate::installers::installer::Installer; @@ -121,24 +123,6 @@ impl App { } pub fn ensure_installed(&self) -> Result<()> { - let result = self.ensure_installed_internal(); - if result.is_err() { - delete_if_exists(self.get_install_path()?.parent().unwrap())?; - } - - result - } - - fn lock_version(&self, version: &str) -> Result { - let lock_for_writing = FileOptions::new().create(true).write(true).read(true); - let lockfile_path = self.get_app_base()?.join(format!("{}.lock", version)); - let _lock = - FileLock::lock(lockfile_path, true, lock_for_writing).context("Locking file")?; - - Ok(_lock) - } - - fn ensure_installed_internal(&self) -> Result<()> { if self.is_installed().context("Checking if it's installed")? { return Ok(()); } @@ -146,7 +130,11 @@ impl App { std::fs::create_dir_all(install_dir.parent().unwrap()) .context("While creating parent directory of install directory")?; - let _lock = self.lock_version(&self.version); + let _lock = self.lock_version(&self.version)?; + // Someone else may have installed it while we were waiting for the lock. + if self.is_installed().context("Checking if it's installed")? { + return Ok(()); + } // In Python we generate a UUID here; is that necessary? let uniq_install_dir = self @@ -154,20 +142,54 @@ impl App { .context("Checking its install path")?; delete_if_exists(uniq_install_dir.as_path())?; + let result = self.install_and_link(&uniq_install_dir, &install_dir); + if result.is_err() { + // Tidy up our own half-finished work, and only that: anything already installed + // belongs to whoever successfully put it there. We still hold the lock, so nobody + // else is using this directory. + let _ = delete_if_exists(uniq_install_dir.as_path()); + } + + result.with_context(|| format!("While installing {} v{}", self.name, self.version)) + } + + /// Run the installer into `uniq_install_dir`, then put it in place at `install_dir` - by + /// renaming for a relocatable app, and by symlinking for one that has to stay put. + fn install_and_link( + &self, + uniq_install_dir: &std::path::Path, + install_dir: &std::path::Path, + ) -> Result<()> { self.installer - .install(uniq_install_dir.as_path()) - .context("While running the Installer") - .and_then(|_| { - match self.relocatable { - true => std::fs::rename(&uniq_install_dir, &install_dir).context("Renaming")?, - false => std::os::unix::fs::symlink(&uniq_install_dir, &install_dir) - .context("Symlinking")?, - }; - Ok(()) - }) - .with_context(|| format!("While installing {} v{}", self.name, self.version))?; + .install(uniq_install_dir) + .context("While running the Installer")?; - Ok(()) + let put_in_place = match self.relocatable { + true => std::fs::rename(uniq_install_dir, install_dir).context("Renaming"), + false => { + std::os::unix::fs::symlink(uniq_install_dir, install_dir).context("Symlinking") + } + }; + + // if put_in_place contains Err, it means another process (probably a parallel ozy) + // installed the package we were working on. Trash our work-in-progress one since theirs + // completed successfully; if the app is not relocatable then do nothing, since the + // equivalent install is completed. + match put_in_place { + Err(err) if !self.is_installed().unwrap_or(false) => Err(err), + Err(_) if self.relocatable => trash_and_remove(&get_ozy_cache_dir()?, uniq_install_dir) + .context("While discarding a redundant install"), + _ => Ok(()), + } + } + + fn lock_version(&self, version: &str) -> Result { + let lock_for_writing = FileOptions::new().create(true).write(true).read(true); + let lockfile_path = self.get_app_base()?.join(format!("{}.lock", version)); + let _lock = + FileLock::lock(lockfile_path, true, lock_for_writing).context("Locking file")?; + + Ok(_lock) } pub fn get_absolute_executable_path(&self) -> Result { @@ -388,4 +410,156 @@ mod tests { "symlink installer for symlink_app v5.6.7" ); } + + /// A fake installer that leaves a marker behind. `delay` widens the window in which two + /// racing processes are both mid-install. + struct FakeInstaller { + delay: std::time::Duration, + } + + impl Installer for FakeInstaller { + fn install(&self, to_dir: &std::path::Path) -> Result<()> { + std::fs::create_dir_all(to_dir)?; + std::fs::write(to_dir.join("marker"), "installed")?; + std::thread::sleep(self.delay); + Ok(()) + } + + fn describe(&self) -> String { + "fake installer".to_string() + } + } + + /// A fake installer that half-populates the directory and then gives up. + struct FailingInstaller; + + impl Installer for FailingInstaller { + fn install(&self, to_dir: &std::path::Path) -> Result<()> { + std::fs::create_dir_all(to_dir)?; + std::fs::write(to_dir.join("half-written"), "junk")?; + Err(anyhow!("installer blew up")) + } + + fn describe(&self) -> String { + "failing installer".to_string() + } + } + + fn fake_app(name: &str, version: &str, installer: Box) -> App { + App { + name: name.to_string(), + version: version.to_string(), + installer, + relocatable: true, + executable_path: format!("bin/{}", name), + } + } + + /// Point ozy's cache at `home`. $HOME is process-global, so tests that call this have to + /// hold `HOME_LOCK` for as long as they use it. + fn set_home(home: &std::path::Path) { + // SAFETY: nothing else in this test binary reads $HOME, and HOME_LOCK keeps the tests + // that do from overlapping. + unsafe { std::env::set_var("HOME", home) }; + } + + static HOME_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + + /// The other half of `concurrent_installs_do_not_clobber`; a no-op in a normal test run. + /// ozy locks with fcntl, which is per-process, so the race only reproduces across processes. + #[test] + fn concurrent_install_child() { + if let Ok(home) = std::env::var("OZY_TEST_CHILD_HOME") { + set_home(std::path::Path::new(&home)); + fake_app( + "racy_app", + "1.0", + Box::new(FakeInstaller { + delay: std::time::Duration::from_millis(500), + }), + ) + .ensure_installed() + .expect("child install failed"); + } + } + + /// Two processes installing the same app at once: the loser of the lock must notice the + /// winner already installed it and leave it be, rather than reinstalling and renaming on top + /// of it (which fails with ENOTEMPTY, and then took the whole app directory down with it). + #[test] + fn concurrent_installs_do_not_clobber() { + let _guard = HOME_LOCK.lock().unwrap(); + let home = tempfile::tempdir().expect("tempdir"); + let home_str = home.path().to_str().unwrap().to_string(); + + let mut child = std::process::Command::new(std::env::current_exe().unwrap()) + .args([ + "--exact", + "--nocapture", + "app::tests::concurrent_install_child", + ]) + .env("OZY_TEST_CHILD_HOME", &home_str) + .spawn() + .expect("spawning child test process"); + + set_home(home.path()); + let ours = fake_app( + "racy_app", + "1.0", + Box::new(FakeInstaller { + delay: std::time::Duration::from_millis(500), + }), + ) + .ensure_installed(); + let theirs = child.wait().expect("waiting for child test process"); + + ours.expect("install in this process failed"); + assert!(theirs.success(), "install in the child process failed"); + assert!( + home.path().join(".cache/ozy/racy_app/1.0/marker").is_file(), + "the installed app was clobbered by the racing install" + ); + } + + /// A failed install must clean up after itself without taking the rest of the app's cache + /// directory - other installed versions, and the lock files - with it. + #[test] + fn failed_install_leaves_the_rest_of_the_app_alone() { + let _guard = HOME_LOCK.lock().unwrap(); + let home = tempfile::tempdir().expect("tempdir"); + let cache = home.path().join(".cache/ozy"); + + set_home(home.path()); + fake_app( + "versioned_app", + "1.0", + Box::new(FakeInstaller { + delay: std::time::Duration::ZERO, + }), + ) + .ensure_installed() + .expect("installing v1.0"); + + let err = fake_app("versioned_app", "2.0", Box::new(FailingInstaller)) + .ensure_installed() + .expect_err("v2.0 should have failed to install"); + assert!( + format!("{:#}", err).contains("installer blew up"), + "{:#}", + err + ); + + assert!( + cache.join("versioned_app/1.0/marker").is_file(), + "the failed install deleted a good one" + ); + assert!( + cache.join("versioned_app/1.0.lock").is_file(), + "the failed install deleted the lock files" + ); + assert!( + !cache.join("internal_install/versioned_app/2.0").exists(), + "the failed install left its half-written directory behind" + ); + } } From 5449863dcc71b88d8f557c7bf4fa87d98d2c0b1f Mon Sep 17 00:00:00 2001 From: David Harks Date: Thu, 10 Sep 2026 13:20:44 -0500 Subject: [PATCH 2/3] Fix a few edge cases --- src/app.rs | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 81 insertions(+), 6 deletions(-) diff --git a/src/app.rs b/src/app.rs index bd55f1b..c044058 100644 --- a/src/app.rs +++ b/src/app.rs @@ -136,6 +136,11 @@ impl App { return Ok(()); } + // Nothing valid is installed, and we hold the lock, so anything sitting at install_dir + // is junk from an earlier run - a dangling symlink, a stray file. Clear it, or the + // rename/symlink below fails with ENOTDIR/EEXIST on every future run. + delete_if_exists(install_dir.as_path())?; + // In Python we generate a UUID here; is that necessary? let uniq_install_dir = self .get_internal_install_path() @@ -177,8 +182,14 @@ impl App { // equivalent install is completed. match put_in_place { Err(err) if !self.is_installed().unwrap_or(false) => Err(err), - Err(_) if self.relocatable => trash_and_remove(&get_ozy_cache_dir()?, uniq_install_dir) - .context("While discarding a redundant install"), + Err(_) => { + if self.relocatable { + // Best-effort: the app is installed, so failing to bin our redundant copy + // is not worth failing the user's command over. + let _ = trash_and_remove(&get_ozy_cache_dir()?, uniq_install_dir); + } + Ok(()) + } _ => Ok(()), } } @@ -458,17 +469,45 @@ mod tests { /// Point ozy's cache at `home`. $HOME is process-global, so tests that call this have to /// hold `HOME_LOCK` for as long as they use it. fn set_home(home: &std::path::Path) { - // SAFETY: nothing else in this test binary reads $HOME, and HOME_LOCK keeps the tests - // that do from overlapping. + // SAFETY: every test that sets or reads the environment holds HOME_LOCK for the + // duration, so no other thread is in getenv/setenv concurrently. unsafe { std::env::set_var("HOME", home) }; } static HOME_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + /// Take HOME_LOCK, ignoring poisoning so a panicking test doesn't mask its sibling's + /// assertion failure with a PoisonError. Restores $HOME when dropped, so a test's tempdir + /// doesn't outlive it as a dangling $HOME for the rest of the process. + fn lock_home() -> HomeGuard { + HomeGuard { + was: std::env::var_os("HOME"), + _lock: HOME_LOCK.lock().unwrap_or_else(|e| e.into_inner()), + } + } + + struct HomeGuard { + was: Option, + _lock: std::sync::MutexGuard<'static, ()>, + } + + impl Drop for HomeGuard { + fn drop(&mut self) { + // SAFETY: runs before _lock drops, so we still hold HOME_LOCK - see set_home. + match &self.was { + Some(home) => unsafe { std::env::set_var("HOME", home) }, + None => unsafe { std::env::remove_var("HOME") }, + } + } + } + + const CHILD_RAN: &str = "child-ran"; + /// The other half of `concurrent_installs_do_not_clobber`; a no-op in a normal test run. /// ozy locks with fcntl, which is per-process, so the race only reproduces across processes. #[test] fn concurrent_install_child() { + let _guard = lock_home(); if let Ok(home) = std::env::var("OZY_TEST_CHILD_HOME") { set_home(std::path::Path::new(&home)); fake_app( @@ -480,6 +519,9 @@ mod tests { ) .ensure_installed() .expect("child install failed"); + // Proof for the parent that this test actually ran: a filter that matches nothing + // also exits 0, so the child's exit status alone doesn't say the race was exercised. + std::fs::write(std::path::Path::new(&home).join(CHILD_RAN), "").expect("sentinel"); } } @@ -488,7 +530,7 @@ mod tests { /// of it (which fails with ENOTEMPTY, and then took the whole app directory down with it). #[test] fn concurrent_installs_do_not_clobber() { - let _guard = HOME_LOCK.lock().unwrap(); + let _guard = lock_home(); let home = tempfile::tempdir().expect("tempdir"); let home_str = home.path().to_str().unwrap().to_string(); @@ -515,6 +557,10 @@ mod tests { ours.expect("install in this process failed"); assert!(theirs.success(), "install in the child process failed"); + assert!( + home.path().join(CHILD_RAN).is_file(), + "the child test never ran, so nothing raced - has it been renamed?" + ); assert!( home.path().join(".cache/ozy/racy_app/1.0/marker").is_file(), "the installed app was clobbered by the racing install" @@ -525,7 +571,7 @@ mod tests { /// directory - other installed versions, and the lock files - with it. #[test] fn failed_install_leaves_the_rest_of_the_app_alone() { - let _guard = HOME_LOCK.lock().unwrap(); + let _guard = lock_home(); let home = tempfile::tempdir().expect("tempdir"); let cache = home.path().join(".cache/ozy"); @@ -562,4 +608,33 @@ mod tests { "the failed install left its half-written directory behind" ); } + + /// A dangling symlink at the install path - left by a non-relocatable install whose target + /// went away - used to wedge the app forever: it isn't a valid install, so the rename onto + /// it failed with ENOTDIR on every subsequent run. + #[test] + fn a_wedged_install_path_recovers() { + let _guard = lock_home(); + let home = tempfile::tempdir().expect("tempdir"); + let install_dir = home.path().join(".cache/ozy/wedged_app/1.0"); + + std::fs::create_dir_all(install_dir.parent().unwrap()).expect("cache dir"); + std::os::unix::fs::symlink(home.path().join("gone"), &install_dir).expect("symlink"); + + set_home(home.path()); + fake_app( + "wedged_app", + "1.0", + Box::new(FakeInstaller { + delay: std::time::Duration::ZERO, + }), + ) + .ensure_installed() + .expect("installing over a dangling symlink"); + + assert!( + install_dir.join("marker").is_file(), + "the install did not replace the dangling symlink" + ); + } } From e2e09a241766994ae4a8acc6169230ee45904ce7 Mon Sep 17 00:00:00 2001 From: David Harks Date: Thu, 10 Sep 2026 13:36:18 -0500 Subject: [PATCH 3/3] Update release notes --- RELEASE_NOTES.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 014bfaa..e0dd79a 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -2,6 +2,11 @@ All changes are in the [Releases](https://github.com/aquanauts/ozy/releases). +### 0.2.2 +* Properly re-check install after acquiring a lock +* Ensure we avoid a situation where manual intervention would be required to recover a failed install +* Improved test hygiene + ### [0.2.1](https://github.com/aquanauts/ozy/releases/tag/v0.2.1) * Fix up CI - no code change