From 7459c34a2eef8dbd0be94fdb8eed7cdfd6f2867b Mon Sep 17 00:00:00 2001 From: Kris Jenkins Date: Wed, 19 Aug 2026 14:47:05 +0100 Subject: [PATCH] Bugfix: Acknowledged transactions can be lost when the commitlog rotates or compresses a segment. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Imagine this sequence: 1. A database has been running long enough that its commitlog passes `max_segment_size` — 1 GiB by default — and rolls over to a new segment. 2. A client calls a reducer over a v2 WebSocket, which is to say with confirmed reads on: the strongest durability promise SpacetimeDB makes. 3. The transaction is written into the new segment, `fdatasync`ed, the durable offset advances, and the acknowledgement is released. 4. A moment later the machine loses power. The client was told the write was durable, so it should be there on restart, right? It won't be, and neither will anything else that landed in that segment. `fs::create_segment` builds a new segment by writing the header to a temporary file and renaming it into place: ```rust let mut tmp = tempfile::Builder::new().make_in(&self.root.0, |tmp_path| { File::options().read(true).write(true).create_new(true).open(tmp_path) })?; header.write(&mut tmp)?; tmp.as_file_mut().sync_all()?; let segment = tmp.persist(path)?; // rename(2) ``` The `sync_all` makes the file's *contents* durable. It says nothing about the `rename`, which is a modification of the enclosing directory, and on *nix a directory's entries are only durable once the directory itself is `fsync`ed. Nothing ever `fsync`s the commitlog root. So the segment file can be on disk, complete and fully synced, with no name pointing at it. `existing_offsets` finds segments by reading the directory, so on restart the log simply ends at the previous segment. Every transaction written into the orphan — each one `fdatasync`ed and acknowledged — is gone, and the database comes back up reporting no problem at all. That breaks the contract `Repo::create_segment` states directly above the offending code ("the `header` **must** have been durably written to the segment") and the one `spacetimedb-durability` states for the whole layer, that a higher durable offset implies durability of every offset below it. `compress_segment_with` has the same hole and a worse blast radius. It renames a compressed copy over a segment that is *already* durable, and never syncs the copy's contents either: ```rust let mut dst = NamedTempFile::new_in(&self.root)?; let stats = f.compress(&mut src, &mut dst)?; dst.persist(self.segment_path(offset))?; ``` A crash in that window can leave the segment's name resolving to a truncated or empty compressed file while the original — holding committed, acknowledged transactions — has already been unlinked. Rotation can lose a new segment; compression can lose an old one that was safe until we touched it. Two things kept this quiet. It needs a crash inside a narrow window at a segment boundary, and on ext4 with `data=ordered` the next data `fsync` usually drags the pending rename along with it — which makes the bug look like correct behaviour on the most common Linux configuration, even though it is a filesystem accident rather than a guarantee, and does not carry over to XFS, APFS or btrfs. Beyond that, an `fsync` leaves no trace a test can observe from outside the process, so no amount of ordinary testing would have noticed the missing one. The `snapshot` crate, doing the same rename dance a few directories away, gets it right and has done all along. Both call sites now go through a single `Fs::persist_durably`, which syncs the file, renames it, and syncs the root. Making it one method rather than two fixes is the point: the invariant is easy to forget, and the next segment installation added to this repo gets it for free. It takes a `SegmentFile` rather than any path, because it syncs the repository root and nothing else -- installing a file outside that directory has to say so explicitly, rather than silently syncing the wrong one. - Add `sync_dir`, mirroring `snapshot`'s `FileOrDirPath::sync_all`, including its no-op on Windows, where opening a directory as a file is an error. - Add the missing `sync_all` on the compressed segment's contents, which was absent independently of the directory problem. # API and ABI breaking changes None. # Expected complexity level and risk 1. Two extra `fsync`s on two cold paths — one per segment rotation, so once per 1 GiB of log by default, and one per segment compression. Both paths already `fsync` a file and perform a rename, so the added cost is noise against what they do anyway. # Testing - [x] `cargo test -p spacetimedb-commitlog`: 67 unit + 11 integration tests pass. - [x] `cargo clippy -p spacetimedb-commitlog --all-targets` clean. - [ ] Reviewer: there is deliberately no test for this, for the same reason the bug survived so long. An `fsync` has no effect that is observable without a crash, so a test can only assert that the call was made -- a restatement of the line it is testing -- and demonstrating the actual property needs crash injection (`dm-log-writes`, or an `LD_PRELOAD` that swallows `fsync`) which this crate has no harness for. What guards the invariant instead is structural: `.persist(` now appears exactly once in the crate, inside `persist_durably`, whose doc comment spells out both failure modes and whose `SegmentFile` parameter keeps the destination inside the directory it syncs. Please review that funnel rather than look for a test. The added `sync_all` on the compressed file's contents rests on review for the same reason. --- crates/commitlog/src/repo/fs.rs | 46 ++++++++++++++++++++++++++++++--- 1 file changed, 43 insertions(+), 3 deletions(-) diff --git a/crates/commitlog/src/repo/fs.rs b/crates/commitlog/src/repo/fs.rs index c0456a9fe56..56b06ff27dd 100644 --- a/crates/commitlog/src/repo/fs.rs +++ b/crates/commitlog/src/repo/fs.rs @@ -130,6 +130,47 @@ impl Fs { Ok(size) } + + /// Atomically install the temporary file `tmp` at `path`, ensuring that + /// both its contents and the directory entry naming it are fsync'd durably. + /// + /// Note that just fsyncing the file is not sufficient. A durable file in a not-yet-durable + /// directory would be reliably on disk, but not reliably reachable. + /// + /// Only the repository root is fsync'd, so `path` must name an entry directly + /// within it -- as [`Self::segment_path`] guarantees. Installing a file + /// elsewhere would need to sync that file's own parent directory instead. + fn persist_durably(&self, mut tmp: NamedTempFile, path: &SegmentFile) -> io::Result { + tmp.as_file_mut().sync_all()?; + let file = tmp.persist(path)?; + sync_dir(&self.root.0)?; + + Ok(file) + } +} + +/// `fsync` the directory at `path`, making durable any directory entries +/// created or removed within it (e.g. by `rename`). +/// +/// On *nix, `fsync`ing a file does **not** make the directory entry pointing +/// at it durable -- the enclosing directory must be `fsync`ed separately. +/// +/// On Windows, directories cannot be opened as files and `fsync`ing one is an +/// error, so this is a no-op there. +#[cfg_attr(target_os = "windows", allow(unused_variables))] +fn sync_dir(path: &std::path::Path) -> io::Result<()> { + #[cfg(not(target_os = "windows"))] + File::open(path) + .map_err(|e| { + io::Error::new( + e.kind(), + format!("failed to open directory {} for fsync: {}", path.display(), e), + ) + })? + .sync_all() + .map_err(|e| io::Error::new(e.kind(), format!("failed to fsync directory {}: {}", path.display(), e)))?; + + Ok(()) } impl fmt::Display for Fs { @@ -280,8 +321,7 @@ impl Repo for Fs { File::options().read(true).write(true).create_new(true).open(tmp_path) })?; header.write(&mut tmp)?; - tmp.as_file_mut().sync_all()?; - let segment = tmp.persist(path)?; + let segment = self.persist_durably(tmp, &path)?; // Notify subscribers. if let Some(on_new_segment) = self.on_new_segment.as_ref() { @@ -330,7 +370,7 @@ impl Repo for Fs { let mut dst = NamedTempFile::new_in(&self.root)?; let stats = f.compress(&mut src, &mut dst)?; - dst.persist(self.segment_path(offset))?; + self.persist_durably(dst, &self.segment_path(offset))?; Ok(stats) }