From 7b36bc8beb5809562abe3120bf28188bd1a7190d Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Wed, 10 Jun 2026 16:45:56 +0200 Subject: [PATCH 1/3] Prevent stale fs-store writes after lock cleanup Reserve write versions while holding the per-path lock map mutex so cleanup cannot remove the version state between version allocation and lock reference acquisition. Add a regression test for the ordering invariant. Co-Authored-By: HAL 9000 This finding was discovered by Project Loupe --- lightning-persister/src/fs_store/common.rs | 44 +++++++++++++++++++++- 1 file changed, 42 insertions(+), 2 deletions(-) diff --git a/lightning-persister/src/fs_store/common.rs b/lightning-persister/src/fs_store/common.rs index 885f806b344..5c73ad1afe7 100644 --- a/lightning-persister/src/fs_store/common.rs +++ b/lightning-persister/src/fs_store/common.rs @@ -91,14 +91,17 @@ impl FilesystemStoreState { } fn get_new_version_and_lock_ref(&self, dest_file_path: PathBuf) -> (Arc>, u64) { + let mut outer_lock = self.inner.locks.lock().unwrap(); + let version = self.next_version.fetch_add(1, Ordering::Relaxed); if version == u64::MAX { panic!("FilesystemStore version counter overflowed"); } // Get a reference to the inner lock. We do this early so that the arc can double as an in-flight counter for - // cleaning up unused locks. - let inner_lock_ref = self.inner.get_inner_lock_ref(dest_file_path); + // cleaning up unused locks. Allocate the version while holding the lock map mutex so that clean_locks cannot + // remove the entry after a version has been reserved but before its lock reference is cloned. + let inner_lock_ref = Arc::clone(&outer_lock.entry(dest_file_path).or_default()); (inner_lock_ref, version) } @@ -851,3 +854,40 @@ pub(crate) fn get_key_from_dir_entry_path( }, } } + +#[cfg(test)] +mod tests { + use super::*; + + use std::sync::Arc; + use std::thread; + use std::time::Duration; + + #[test] + fn version_is_not_reserved_before_lock_ref() { + let mut temp_path = std::env::temp_dir(); + temp_path.push("test_version_is_not_reserved_before_lock_ref"); + let state = Arc::new(FilesystemStoreState::new(temp_path)); + let path = + state.get_checked_dest_file_path("ns", "sub", Some("key"), "write", false).unwrap(); + + let outer_lock = state.inner.locks.lock().unwrap(); + let state_for_thread = Arc::clone(&state); + let path_for_thread = path.clone(); + let handle = + thread::spawn(move || state_for_thread.get_new_version_and_lock_ref(path_for_thread)); + + thread::sleep(Duration::from_millis(50)); + assert_eq!( + state.next_version.load(Ordering::Relaxed), + 1, + "version allocation must wait until the lock reference can be cloned" + ); + + drop(outer_lock); + + let (inner_lock_ref, version) = handle.join().unwrap(); + assert_eq!(version, 1); + state.inner.clean_locks(&inner_lock_ref, path); + } +} From 7106181ad1d7b7a33837efb251f871d44fc5664a Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 11 Jun 2026 10:31:38 +0200 Subject: [PATCH 2/3] f - Prevent stale fs-store writes Move the version-allocation ordering note to the allocation it describes so the cleanup invariant is easier to follow. Co-Authored-By: HAL 9000 --- lightning-persister/src/fs_store/common.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lightning-persister/src/fs_store/common.rs b/lightning-persister/src/fs_store/common.rs index 5c73ad1afe7..16a135221c4 100644 --- a/lightning-persister/src/fs_store/common.rs +++ b/lightning-persister/src/fs_store/common.rs @@ -93,14 +93,15 @@ impl FilesystemStoreState { fn get_new_version_and_lock_ref(&self, dest_file_path: PathBuf) -> (Arc>, u64) { let mut outer_lock = self.inner.locks.lock().unwrap(); + // Allocate the version while holding the lock map mutex so that clean_locks cannot remove the entry after a + // version has been reserved but before its lock reference is cloned. let version = self.next_version.fetch_add(1, Ordering::Relaxed); if version == u64::MAX { panic!("FilesystemStore version counter overflowed"); } // Get a reference to the inner lock. We do this early so that the arc can double as an in-flight counter for - // cleaning up unused locks. Allocate the version while holding the lock map mutex so that clean_locks cannot - // remove the entry after a version has been reserved but before its lock reference is cloned. + // cleaning up unused locks. let inner_lock_ref = Arc::clone(&outer_lock.entry(dest_file_path).or_default()); (inner_lock_ref, version) From 2c09a2610d1231b78704ae3e26d42136e8e89e90 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 11 Jun 2026 11:00:03 +0200 Subject: [PATCH 3/3] f - Prevent stale fs-store writes Exercise the stale-write race through the stored filesystem bytes so the regression test covers the user-visible overwrite bug. Co-Authored-By: HAL 9000 --- lightning-persister/src/fs_store/common.rs | 85 +++++++++++++++++----- 1 file changed, 65 insertions(+), 20 deletions(-) diff --git a/lightning-persister/src/fs_store/common.rs b/lightning-persister/src/fs_store/common.rs index 16a135221c4..96e58945f84 100644 --- a/lightning-persister/src/fs_store/common.rs +++ b/lightning-persister/src/fs_store/common.rs @@ -11,7 +11,11 @@ use std::collections::HashMap; use std::fs; use std::io::{ErrorKind, Read, Write}; use std::path::{Path, PathBuf}; +#[cfg(test)] +use std::sync::atomic::AtomicBool; use std::sync::atomic::{AtomicU64, AtomicUsize, Ordering}; +#[cfg(test)] +use std::sync::mpsc; use std::sync::{Arc, Mutex, RwLock}; #[cfg(target_os = "windows")] @@ -99,6 +103,8 @@ impl FilesystemStoreState { if version == u64::MAX { panic!("FilesystemStore version counter overflowed"); } + #[cfg(test)] + maybe_pause_after_version_allocation(&self.inner, &dest_file_path); // Get a reference to the inner lock. We do this early so that the arc can double as an in-flight counter for // cleaning up unused locks. @@ -856,39 +862,78 @@ pub(crate) fn get_key_from_dir_entry_path( } } +#[cfg(test)] +struct VersionAllocatedHook { + dest_file_path: PathBuf, + version_allocated: mpsc::Sender<()>, + continue_write: Mutex>, + fired: AtomicBool, +} + +#[cfg(test)] +static VERSION_ALLOCATED_HOOK: Mutex>> = Mutex::new(None); + +#[cfg(test)] +fn maybe_pause_after_version_allocation(inner: &FilesystemStoreInner, dest_file_path: &Path) { + let hook = VERSION_ALLOCATED_HOOK.lock().unwrap().clone(); + if let Some(hook) = hook { + if hook.dest_file_path.as_path() != dest_file_path + || hook.fired.swap(true, Ordering::AcqRel) + { + return; + } + + let version_allocation_holds_lock = inner.locks.try_lock().is_err(); + hook.version_allocated.send(()).unwrap(); + if !version_allocation_holds_lock { + hook.continue_write.lock().unwrap().recv().unwrap(); + } + } +} + #[cfg(test)] mod tests { use super::*; use std::sync::Arc; use std::thread; - use std::time::Duration; #[test] - fn version_is_not_reserved_before_lock_ref() { + fn stale_write_after_lock_cleanup_does_not_overwrite_newer_write() { let mut temp_path = std::env::temp_dir(); - temp_path.push("test_version_is_not_reserved_before_lock_ref"); - let state = Arc::new(FilesystemStoreState::new(temp_path)); + temp_path.push("test_stale_write_after_lock_cleanup"); + let _ = std::fs::remove_dir_all(&temp_path); + + let state = Arc::new(FilesystemStoreState::new(temp_path.clone())); let path = state.get_checked_dest_file_path("ns", "sub", Some("key"), "write", false).unwrap(); + let (version_allocated, wait_for_version) = mpsc::channel(); + let (continue_write, wait_to_continue) = mpsc::channel(); + *VERSION_ALLOCATED_HOOK.lock().unwrap() = Some(Arc::new(VersionAllocatedHook { + dest_file_path: path.clone(), + version_allocated, + continue_write: Mutex::new(wait_to_continue), + fired: AtomicBool::new(false), + })); - let outer_lock = state.inner.locks.lock().unwrap(); let state_for_thread = Arc::clone(&state); let path_for_thread = path.clone(); - let handle = - thread::spawn(move || state_for_thread.get_new_version_and_lock_ref(path_for_thread)); - - thread::sleep(Duration::from_millis(50)); - assert_eq!( - state.next_version.load(Ordering::Relaxed), - 1, - "version allocation must wait until the lock reference can be cloned" - ); - - drop(outer_lock); - - let (inner_lock_ref, version) = handle.join().unwrap(); - assert_eq!(version, 1); - state.inner.clean_locks(&inner_lock_ref, path); + let stale_write = thread::spawn(move || { + let (inner_lock_ref, version) = + state_for_thread.get_new_version_and_lock_ref(path_for_thread.clone()); + state_for_thread + .inner + .write_version(inner_lock_ref, path_for_thread, b"stale".to_vec(), version, false) + .unwrap(); + }); + + wait_for_version.recv().unwrap(); + state.write_impl("ns", "sub", "key", b"newer".to_vec(), false).unwrap(); + continue_write.send(()).unwrap(); + stale_write.join().unwrap(); + *VERSION_ALLOCATED_HOOK.lock().unwrap() = None; + + assert_eq!(state.read_impl("ns", "sub", "key", false).unwrap(), b"newer"); + let _ = std::fs::remove_dir_all(temp_path); } }