Skip to content

Bugfix: Acknowledged transactions can be lost when the commitlog rotates or compresses a segment. - #5785

Merged
bfops merged 1 commit into
clockworklabs:masterfrom
krisajenkins:kris/commitlog-durable-rename
Aug 24, 2026
Merged

Bugfix: Acknowledged transactions can be lost when the commitlog rotates or compresses a segment.#5785
bfops merged 1 commit into
clockworklabs:masterfrom
krisajenkins:kris/commitlog-durable-rename

Conversation

@krisajenkins

Copy link
Copy Markdown
Contributor

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, fdatasynced, 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:

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 fsynced.
Nothing ever fsyncs 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 fdatasynced 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:

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 fsyncs 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

  • cargo test -p spacetimedb-commitlog: 67 unit + 11 integration tests
    pass.
  • 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.

@bfops
bfops enabled auto-merge August 24, 2026 16:28
@bfops
bfops disabled auto-merge August 24, 2026 16:44
…tes or compresses a segment.

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.
@clockwork-labs-bot
clockwork-labs-bot force-pushed the kris/commitlog-durable-rename branch from b2084c4 to 7459c34 Compare August 24, 2026 16:47
@bfops
bfops enabled auto-merge August 24, 2026 16:48
@bfops
bfops added this pull request to the merge queue Aug 24, 2026
Merged via the queue into clockworklabs:master with commit ee0892a Aug 24, 2026
61 of 62 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants