Merge pull request 'clawhdf5: a dropped FileEditor releases its lock at once' (#23) from fix/editor-lock-fork-race into main

Reviewed-on: #23

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

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::*;

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)]

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)"
+    );
+}

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 `<store>.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.