Prevent stale fs-store writes after lock cleanup#4678
Conversation
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
|
👋 Thanks for assigning @benthecarman as a reviewer! |
|
The diff matches my prior review exactly — nothing has changed. The production change at common.rs:97-114 is correct (version allocation and lock-ref clone now atomic under the I re-verified the test for self-induced races: in the fixed path both threads proceed concurrently, but No issues found. |
| fn get_new_version_and_lock_ref(&self, dest_file_path: PathBuf) -> (Arc<RwLock<u64>>, u64) { | ||
| let mut outer_lock = self.inner.locks.lock().unwrap(); | ||
|
|
||
| let version = self.next_version.fetch_add(1, Ordering::Relaxed); |
There was a problem hiding this comment.
I don't think this needs to be atomic anymore now, and can be included in the mutex?
There was a problem hiding this comment.
I'd rather not make this a much more invasive change, risking to introduce further edge cases tbh.
There was a problem hiding this comment.
Hm, to me it seems more risky to leave the option open to modify the version outside of the lock.
Move the version-allocation ordering note to the allocation it describes so the cleanup invariant is easier to follow. Co-Authored-By: HAL 9000
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
tnull
force-pushed
the
2026-06-fs-store-version-lock-cleanup
branch
from
f8b7ca1 to
2c09a26
Compare
|
Updated to line-wrap commit messages. |
5 participants
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