diff --git a/CHANGELOG.md b/CHANGELOG.md index 4f65734..8a8a51e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,19 @@ ## Unreleased +### A dropped `FileEditor` releases its lock at once (2026-09-28) +- `FileEditor`'s `flock` could outlive the editor for a moment when + another thread forked to spawn a process: the child shared the locked + descriptor until it exec'd, so reopening the file right after the drop + was sometimes refused with `Error::Locked` (seen once as a failure of + `edit_interop::editor_locks_the_file` in a parallel test run). The drop + now unlocks before closing, which releases the lock for every descriptor + that shares it. Reproducer + `edit_tests::drop_releases_the_lock_while_other_threads_spawn_processes` + (tank, 2026-09-28): 1483 of 2000 reopens refused before, 0 in 30 runs + after. The agent store's lock file unlocks on drop the same way (its + 250 ms retry on open had hidden the race). `docs/known-issues.md`. + ### `ObjectHeader::parse` back at its pre-M2/M3 speed (2026-09-27) - Parsing a version-1 object header was 4% slower than before range-read M2/M3 (`docs/known-issues.md`). The cause was the call to the per-chunk diff --git a/crates/clawhdf5-agent/src/store_lock.rs b/crates/clawhdf5-agent/src/store_lock.rs index 2813f5b..40e720f 100644 --- a/crates/clawhdf5-agent/src/store_lock.rs +++ b/crates/clawhdf5-agent/src/store_lock.rs @@ -20,7 +20,7 @@ const LOCK_RETRY_DELAY: std::time::Duration = std::time::Duration::from_millis(1 /// never leaves a stale lock behind; the empty lock file itself is harmless). #[derive(Debug)] pub(crate) struct StoreLock { - _file: File, + file: File, } impl StoreLock { @@ -42,7 +42,7 @@ impl StoreLock { let mut attempts_left = LOCK_RETRIES; loop { match file.try_lock() { - Ok(()) => return Ok(Self { _file: file }), + Ok(()) => return Ok(Self { file }), Err(TryLockError::WouldBlock) if attempts_left > 0 => { attempts_left -= 1; std::thread::sleep(LOCK_RETRY_DELAY); @@ -60,6 +60,16 @@ impl StoreLock { } } +impl Drop for StoreLock { + /// Unlocks before the file is closed: a process another thread forks + /// inherits the descriptor until it execs, and a `flock` lasts while any + /// descriptor of the open file does, so closing alone could keep the + /// store locked for a moment after the drop (see `FileEditor`'s `Drop`). + fn drop(&mut self) { + let _ = self.file.unlock(); + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/crates/clawhdf5/src/edit/mod.rs b/crates/clawhdf5/src/edit/mod.rs index 455c77a..28e1fa6 100644 --- a/crates/clawhdf5/src/edit/mod.rs +++ b/crates/clawhdf5/src/edit/mod.rs @@ -61,6 +61,9 @@ const MSG_FLAG_DONTSHARE: u8 = 0x04; /// Opening takes an exclusive advisory lock on the file (`flock`, the lock /// libhdf5 itself takes when file locking is on), so a second editor, or /// h5py opening the file for writing, fails until the editor is dropped. +/// The drop unlocks the file before closing it, so the lock is gone as soon +/// as the drop returns, even in a program whose other threads spawn +/// processes (a forked child shares the locked descriptor until it execs). /// Readers that do not lock ([`File`]) can still open it, but see a file /// that may be mid-update. /// @@ -114,6 +117,19 @@ pub struct FileEditor { free: FreeList, } +impl Drop for FileEditor { + /// Releases the lock explicitly before the file is closed. A `flock` + /// belongs to the open file description and lasts until every + /// descriptor of it is closed; a process another thread forks (any + /// `std::process::Command`) inherits the descriptor and keeps it until + /// it execs, so closing alone could leave the file locked for a moment + /// after the drop. Unlocking through our descriptor releases the lock + /// for all of them. + fn drop(&mut self) { + let _ = self.file.unlock(); + } +} + /// Where a layout message keeps the fields an edit may change (offsets in /// the message body). #[derive(Debug, Default, Clone, Copy)] diff --git a/crates/clawhdf5/tests/edit_tests.rs b/crates/clawhdf5/tests/edit_tests.rs index 1f83d74..d55578f 100644 --- a/crates/clawhdf5/tests/edit_tests.rs +++ b/crates/clawhdf5/tests/edit_tests.rs @@ -218,3 +218,49 @@ fn edits_go_to_the_file_held_not_the_path() { assert_eq!(f.dataset("big").unwrap().read_f64().unwrap(), [1.5; 5000]); assert!(matches!(f.root().attr("note").unwrap(), Some(AttrValue::F64Array(v)) if v == vals)); } + +/// Dropping an editor releases its lock at once, even while other threads +/// keep spawning processes: a child inherits the locked descriptor between +/// `fork` and `exec` (close-on-exec closes it only at `exec`), and a +/// `flock` lasts while any descriptor of the open file does, so without the +/// explicit unlock in `Drop` a reopen right after the drop was sometimes +/// refused with `Error::Locked`. +#[cfg(unix)] +#[test] +fn drop_releases_the_lock_while_other_threads_spawn_processes() { + use std::sync::Arc; + use std::sync::atomic::{AtomicBool, Ordering}; + let dir = tempfile::tempdir().unwrap(); + let path = sample(dir.path()); + let stop = Arc::new(AtomicBool::new(false)); + let spawners: Vec<_> = (0..4) + .map(|_| { + let stop = Arc::clone(&stop); + std::thread::spawn(move || { + let mut n = 0u64; + while !stop.load(Ordering::Relaxed) { + let _ = std::process::Command::new("true").status(); + n += 1; + } + n + }) + }) + .collect(); + let rounds = 2000; + let mut refused = 0; + // Every open but the first follows the drop of the previous editor. + for _ in 0..rounds { + match FileEditor::open(&path) { + Ok(ed) => drop(ed), + Err(Error::Locked(_)) => refused += 1, + Err(e) => panic!("{e}"), + } + } + stop.store(true, Ordering::Relaxed); + let spawned: u64 = spawners.into_iter().map(|t| t.join().unwrap()).sum(); + assert!(spawned > 0); + assert_eq!( + refused, 0, + "{refused} of {rounds} reopens refused ({spawned} processes spawned)" + ); +} diff --git a/docs/known-issues.md b/docs/known-issues.md index b50deff..69039ab 100644 --- a/docs/known-issues.md +++ b/docs/known-issues.md @@ -448,6 +448,35 @@ Newest first. "Before any release" means no tagged release (v2.7.0 and earlier) contains the bug. Full detail is in `CHANGELOG.md` under the date given. +## A dropped `FileEditor` could keep its file locked for a moment + +**Status:** fixed 2026-09-28 (`fix/editor-lock-fork-race`), before any release +(`FileEditor` dates from 2026-09-26). Spurious `Error::Locked` only; no +data was affected. Nothing for users to do. + +`FileEditor` locks its file with `flock`, which belongs to the open file +description and lasts until every descriptor of it is closed. When another +thread of the program forked to spawn a process (any +`std::process::Command`), the child inherited the descriptor and kept it +until it exec'd (close-on-exec acts only at `exec`), so an `open` of the +same file right after the editor was dropped could be refused with +`Error::Locked`. It showed up as a one-off failure of +`edit_interop::editor_locks_the_file` in a full parallel `cargo test`; +the deliberate reproducer +(`edit_tests::drop_releases_the_lock_while_other_threads_spawn_processes`: +four threads running `true` in a loop while the main thread opens and drops +an editor 2000 times) had 1483 of 2000 reopens refused on tank. The drop +now unlocks the file explicitly before closing it, which releases the lock +for every descriptor of the description: 0 refused in 30 runs of the same +test. The agent store's `.h5.lock` (`HDF5Memory`, since v2.3.0) +had the same pattern, hidden by its 250 ms retry on open; it unlocks on +drop too. + +An `fcntl` open-file-description lock (`F_OFD_SETLK`) would not have +helped: it is inherited across `fork` the same way, and on Linux it does +not conflict with `flock`, so libhdf5 (h5py), which locks with `flock`, +would no longer be refused while an editor holds the file. + ## `ObjectHeader::parse` 4% slower after range-read M2/M3 **Status:** fixed 2026-09-27 (`96086ad`, PR #21), before any release.