From ef5df40f22a622a5fa53de9d37d496a199e263da Mon Sep 17 00:00:00 2001 From: Tolo Palmer Date: Wed, 17 Jun 2026 14:46:06 +0100 Subject: [PATCH] fix: add Release fence to seqlock writer-enter to prevent torn reads (#40) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit write_lock() published the odd ("writer active") sequence number with a Release store and then mutated data. A Release store orders only the operations that *precede* it, so on weak-memory hardware (ARM64) the subsequent data writes can float ahead of the odd-seq store. A reader can then observe mutated data while seq still reads even at both read_begin and read_validate, and falsely validate a torn read — wrong results returned through the safe cross-process backend, not just an extra retry. (x86 is TSO, so it cannot manifest there.) Add `atomic::fence(Release)` immediately after the odd-seq store — the textbook seqlock writer-enter. The release fence orders the odd publish before the following data writes; paired with the reader's existing `fence(Acquire)` in read_validate, a reader that observes any mutated data is guaranteed to also observe the odd seq and retry. The exit side (write_unlock's Release store) was already correct but only covers the even publish, not the entry side. Add a loom model of the seqlock's reader/writer ordering (gated behind `cfg(loom)`, so loom is not a normal/release/CI dependency). Loom exhaustively explores interleavings and reorderings: without the fence it finds an execution that validates a torn read (d0=0, d1=1); with the fence, no such execution exists. Run with `RUSTFLAGS="--cfg loom" cargo test --lib seqlock_ordering`. Document the invariant in ARCHITECTURE. Co-Authored-By: Claude Opus 4.8 (1M context) --- Cargo.lock | 222 +++++++++++++++++++++++++++++++++++++++++++ Cargo.toml | 7 +- docs/ARCHITECTURE.md | 7 ++ src/shm/lock.rs | 68 +++++++++++++ 4 files changed, 303 insertions(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index dca3b55..683e8d0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -15,6 +15,15 @@ dependencies = [ "zerocopy", ] +[[package]] +name = "aho-corasick" +version = "1.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ddd31a130427c27518df266943a5308ed92d4b226cc639f5a8f1002816174301" +dependencies = [ + "memchr", +] + [[package]] name = "allocator-api2" version = "0.2.21" @@ -27,6 +36,16 @@ version = "2.10.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "812e12b5285cc515a9c72a5c1d3b6d46a19dac5acfef5265968c166106e31dd3" +[[package]] +name = "cc" +version = "1.2.64" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "dad887fd958be91b5098c0248def011f4523ab786cd411be668777e55063501f" +dependencies = [ + "find-msvc-tools", + "shlex", +] + [[package]] name = "cfg-if" version = "1.0.4" @@ -39,12 +58,33 @@ version = "1.0.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "877a4ace8713b0bcf2a4e7eec82529c029f1d0619886d18145fea96c3ffe5c0f" +[[package]] +name = "find-msvc-tools" +version = "0.1.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5baebc0774151f905a1a2cc41989300b1e6fbb29aff0ceffa1064fdd3088d582" + [[package]] name = "foldhash" version = "0.1.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d9c4f5dac5e15c24eb999c26181a6ca40b39fe946cbe4c263c7209467bc83af2" +[[package]] +name = "generator" +version = "0.8.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b3b854b0e584ead1a33f18b2fcad7cf7be18b3875c78816b753639aa501513ae" +dependencies = [ + "cc", + "cfg-if", + "libc", + "log", + "rustversion", + "windows-link", + "windows-result", +] + [[package]] name = "getrandom" version = "0.3.4" @@ -74,6 +114,12 @@ version = "0.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2304e00983f87ffb38b55b444b5e3b60a884b5d30c0fca7d82fe33449bbe55ea" +[[package]] +name = "lazy_static" +version = "1.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bbd2bcb4c963f2ddae06a2efc7e9f3591312473c50c6685e1f298068316e66fe" + [[package]] name = "libc" version = "0.2.181" @@ -89,6 +135,40 @@ dependencies = [ "scopeguard", ] +[[package]] +name = "log" +version = "0.4.32" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "953f07c43838f8e6f9758cab68bf5bed85465e7587ebe0b823f1bcd81978ad3a" + +[[package]] +name = "loom" +version = "0.7.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "419e0dc8046cb947daa77eb95ae174acfbddb7673b4151f56d1eed8e93fbfaca" +dependencies = [ + "cfg-if", + "generator", + "scoped-tls", + "tracing", + "tracing-subscriber", +] + +[[package]] +name = "matchers" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d1525a2a28c7f4fa0fc98bb91ae755d1e2d1505079e05539e35bc876b5d65ae9" +dependencies = [ + "regex-automata", +] + +[[package]] +name = "memchr" +version = "2.8.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "88904434abc2901f197fe8cc55f0445e7ded921dba5911dad2e2b39b48e663c4" + [[package]] name = "memmap2" version = "0.9.9" @@ -98,6 +178,15 @@ dependencies = [ "libc", ] +[[package]] +name = "nu-ansi-term" +version = "0.50.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7957b9740744892f114936ab4a57b3f487491bbeafaf8083688b16841a4240e5" +dependencies = [ + "windows-sys", +] + [[package]] name = "once_cell" version = "1.21.3" @@ -127,6 +216,12 @@ dependencies = [ "windows-link", ] +[[package]] +name = "pin-project-lite" +version = "0.2.17" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a89322df9ebe1c1578d689c92318e070967d1042b512afbe49518723f4e6d5cd" + [[package]] name = "portable-atomic" version = "1.13.1" @@ -223,12 +318,56 @@ dependencies = [ "bitflags", ] +[[package]] +name = "regex-automata" +version = "0.4.14" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6e1dd4122fc1595e8162618945476892eefca7b88c52820e74af6262213cae8f" +dependencies = [ + "aho-corasick", + "memchr", + "regex-syntax", +] + +[[package]] +name = "regex-syntax" +version = "0.8.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d6f6ff9a378485b298a5286656da665ba74413d36db0979633275d2e708145d4" + +[[package]] +name = "rustversion" +version = "1.0.22" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b39cdef0fa800fc44525c84ccb54a029961a8215f9619753635a9c0d2538d46d" + +[[package]] +name = "scoped-tls" +version = "1.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e1cf6437eb19a8f4a6cc0f7dca544973b0b78843adbfeb3683d1a94a0024a294" + [[package]] name = "scopeguard" version = "1.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "94143f37725109f92c262ed2cf5e59bce7498c01bcc1502d7b9afe439a4e9f49" +[[package]] +name = "sharded-slab" +version = "0.1.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f40ca3c46823713e0d4209592e8d6e826aa57e928f09752619fc696c499637f6" +dependencies = [ + "lazy_static", +] + +[[package]] +name = "shlex" +version = "2.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f8fadd59c855ef2080decdef8ff161eb6661b86933c9d82e5ba29dc602a55aba" + [[package]] name = "smallvec" version = "1.15.1" @@ -252,12 +391,76 @@ version = "0.13.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b1dd07eb858a2067e2f3c7155d54e929265c264e6f37efe3ee7a8d1b5a1dd0ba" +[[package]] +name = "thread_local" +version = "1.1.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f60246a4944f24f6e018aa17cdeffb7818b76356965d03b07d6a9886e8962185" +dependencies = [ + "cfg-if", +] + +[[package]] +name = "tracing" +version = "0.1.44" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "63e71662fa4b2a2c3a26f570f037eb95bb1f85397f3cd8076caed2f026a6d100" +dependencies = [ + "pin-project-lite", + "tracing-core", +] + +[[package]] +name = "tracing-core" +version = "0.1.36" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "db97caf9d906fbde555dd62fa95ddba9eecfd14cb388e4f491a66d74cd5fb79a" +dependencies = [ + "once_cell", + "valuable", +] + +[[package]] +name = "tracing-log" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ee855f1f400bd0e5c02d150ae5de3840039a3f54b025156404e34c23c03f47c3" +dependencies = [ + "log", + "once_cell", + "tracing-core", +] + +[[package]] +name = "tracing-subscriber" +version = "0.3.23" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cb7f578e5945fb242538965c2d0b04418d38ec25c79d160cd279bf0731c8d319" +dependencies = [ + "matchers", + "nu-ansi-term", + "once_cell", + "regex-automata", + "sharded-slab", + "smallvec", + "thread_local", + "tracing", + "tracing-core", + "tracing-log", +] + [[package]] name = "unicode-ident" version = "1.0.23" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "537dd038a89878be9b64dd4bd1b260315c1bb94f4d784956b81e27a088d9a09e" +[[package]] +name = "valuable" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ba73ea9cf16a25df0c8caa16c51acb937d5712a8429db78a3ee29d5dcacd3a65" + [[package]] name = "version_check" version = "0.9.5" @@ -271,6 +474,7 @@ dependencies = [ "ahash", "hashbrown", "libc", + "loom", "memmap2", "parking_lot", "pyo3", @@ -291,6 +495,24 @@ version = "0.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f0805222e57f7521d6a62e36fa9163bc891acd422f971defe97d64e70d0a4fe5" +[[package]] +name = "windows-result" +version = "0.4.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7781fa89eaf60850ac3d2da7af8e5242a5ea78d1a11c49bf2910bb5a73853eb5" +dependencies = [ + "windows-link", +] + +[[package]] +name = "windows-sys" +version = "0.61.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ae137229bcbd6cdf0f7b80a31df61766145077ddf49416a728b02cb3921ff3fc" +dependencies = [ + "windows-link", +] + [[package]] name = "wit-bindgen" version = "0.51.0" diff --git a/Cargo.toml b/Cargo.toml index 1b833ac..c9ccf29 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -17,8 +17,13 @@ memmap2 = "0.9" libc = "0.2" ahash = "0.8" +# Loom is only pulled in for the seqlock memory-ordering model (issue #40), built +# solely under `RUSTFLAGS="--cfg loom"`. It is absent from normal/release builds and CI. +[target.'cfg(loom)'.dependencies] +loom = "0.7" + [lints.rust] -unexpected_cfgs = { level = "warn", check-cfg = ['cfg(Py_GIL_DISABLED)'] } +unexpected_cfgs = { level = "warn", check-cfg = ['cfg(Py_GIL_DISABLED)', 'cfg(loom)'] } [profile.release] lto = "fat" diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 1d67b73..70410e2 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -68,6 +68,13 @@ For day-to-day workflow and commands, see [`CLAUDE.md`](../CLAUDE.md) and `hash & (capacity - 1)`. Always use `.next_power_of_two()`. - **`#[repr(C)]` struct field ordering** — place u64 fields before u32 to avoid implicit alignment padding; affects `size_of` assertions in `layout.rs`. +- **Seqlock writer ordering (issue #40).** The writer must publish the odd ("writer active") + sequence number *before* its data mutations become visible: `write_lock` does the odd store + then an `atomic::fence(Release)` (a Release store alone orders only *prior* ops). This pairs + with the reader's `atomic::fence(Acquire)` in `read_validate` — without the writer fence, on + weak-memory hardware a data write can float ahead of the odd publish and a reader can validate + a torn read against a stale even seq. The ordering is model-checked under `loom` (run + `RUSTFLAGS="--cfg loom" cargo test --lib seqlock_ordering`; loom is a `cfg(loom)`-only dep). - **Cross-process timestamps must use a system-wide clock (issue #32).** `created_at_nanos` is written into shared memory by one process and compared against `now` in another, so `shm::current_time_nanos` uses `CLOCK_MONOTONIC` (process-independent on Linux, macOS, and diff --git a/src/shm/lock.rs b/src/shm/lock.rs index 47d28d8..0c708e7 100644 --- a/src/shm/lock.rs +++ b/src/shm/lock.rs @@ -99,6 +99,15 @@ impl ShmSeqLock { let seq = unsafe { &*self.seq_ptr }; let prev = seq.load(Ordering::Relaxed); seq.store(prev + 1, Ordering::Release); + // Release fence so the odd-seq publish is ordered BEFORE the data + // mutations that follow (#40). A Release store alone orders only the ops + // *preceding* it, so without this fence the data writes can float ahead of + // the odd store on weak-memory hardware (ARM64) — a reader could then see + // mutated data while seq still reads even at both read_begin and + // read_validate, falsely validating a torn read. This is the textbook + // seqlock writer-enter construction (the exit-side Release at write_unlock + // orders data writes before going even, but cannot cover the entry side). + std::sync::atomic::fence(Ordering::Release); } /// Release the write lock. @@ -113,3 +122,62 @@ impl ShmSeqLock { unsafe { &*self.write_lock_ptr }.store(0, Ordering::Release); } } + +/// Loom model of the seqlock's reader/writer memory ordering (issue #40). +/// +/// The real `ShmSeqLock` reads/writes atomics through raw mmap pointers, which loom +/// cannot track, so the *algorithm* is replicated here with loom atomics. The orderings +/// mirror the real code exactly: writer goes odd (Release store) + Release fence, mutates +/// data, goes even (Release store); reader loads seq (Acquire), reads data, Acquire fence, +/// re-loads seq, and only trusts the data if the seq is unchanged and even. +/// +/// Run with: `RUSTFLAGS="--cfg loom" cargo test --lib seqlock_ordering` +/// +/// Deleting the `fence(Release)` below makes loom find an execution where a reader +/// validates a torn read (one data word from the old write, one from the new) — the #40 +/// bug. With the fence, no such execution exists. +#[cfg(loom)] +mod loom_tests { + use loom::sync::atomic::{fence, AtomicU64, Ordering}; + use loom::sync::Arc; + use loom::thread; + + #[test] + fn seqlock_ordering_no_torn_validated_read() { + loom::model(|| { + // `d0`/`d1` are two data words the writer always keeps equal; if a reader + // ever validates a read with d0 != d1, it observed a torn write. + let seq = Arc::new(AtomicU64::new(0)); + let d0 = Arc::new(AtomicU64::new(0)); + let d1 = Arc::new(AtomicU64::new(0)); + + let writer = { + let (seq, d0, d1) = (seq.clone(), d0.clone(), d1.clone()); + thread::spawn(move || { + // write_lock: go odd (spinlock omitted — single writer). + let prev = seq.load(Ordering::Relaxed); + seq.store(prev + 1, Ordering::Release); + fence(Ordering::Release); // #40 fix — delete to see loom fail + // data mutation, kept internally consistent + d0.store(1, Ordering::Relaxed); + d1.store(1, Ordering::Relaxed); + // write_unlock: go even + let prev = seq.load(Ordering::Relaxed); + seq.store(prev + 1, Ordering::Release); + }) + }; + + // reader: one optimistic pass (read_begin + read_validate). + let s1 = seq.load(Ordering::Acquire); + let v0 = d0.load(Ordering::Relaxed); + let v1 = d1.load(Ordering::Relaxed); + fence(Ordering::Acquire); + let s2 = seq.load(Ordering::Relaxed); + if s1 & 1 == 0 && s1 == s2 { + assert_eq!(v0, v1, "torn read validated as consistent (#40)"); + } + + writer.join().unwrap(); + }); + } +}