dev/MultiforaDB
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

db/storage: close three data-loss paths in commit and compaction

Browse Source 
Follow-up hardening on the group-commit work from ecd28d9 and c8d547f. The
signal -> broadcast fix and the append-drained wakeup under commit_lock were
correct but incomplete; each of the three defects below could lose or corrupt
data that had already been acknowledged.

- log_append's cleanup defer took commit_lock with `lock(...) catch {}` and
  then unlocked unconditionally. Mutex.lock is Cancelable!void and
  Mutex.unlock treats an already-unlocked mutex as `unreachable`, so a
  cancellation there (client disconnect, shutdown) released a mutex the fiber
  never held: a panic in ReleaseSafe and silent memory corruption in the
  default ReleaseFast build. A cleanup path must not be a cancellation point,
  so it uses lockUncancelable.

- Engine.commit waited with `catch return`, which returns *success* from
  !void. A failed wait therefore told the caller its write was on disk and the
  dispatch epilogue replied ok without a seal or an fsync -- the same failure
  class c8d547f fixed, reached through the error path instead of the happy
  one. The follower wait now propagates; the leader's drain is uncancelable,
  since once `committing` is set every other writer is parked behind it and
  the drain is bounded anyway.

- Two compactions could run at once. They share one `<log>.tmp` path and each
  ends in a rename onto the log, so one truncates and rewrites the file the
  other is about to publish, and then that one renames whatever it finds over
  the live log. compact now claims an atomic `compacting` slot and a second
  caller returns; the guard sits on the resource rather than in take_compact,
  so direct callers (tests included) are covered too. compact_pending became
  atomic while we were there: note_compact sets it under a *collection* lock
  and the epilogue read it under none.

Also in compaction: the tmp file is opened with a new Log.create that
truncates. Log.open keeps an existing file's bytes and only rewinds end_pos to
the header, so a longer tmp left by a crashed or retried rewrite kept its
tail -- and those trailing blocks are intact and hash-correct, so replay
applied them as live records once the rename published the file, resurrecting
deleted documents. The `deleteFile ... catch {}` that used to stand in for
this is gone, and with it a swallowed error the invariant rested on. The
retry loop is bounded at 8 attempts (each one rewrites the whole log before
the seq check can reject it, so an unbounded retry livelocks under sustained
writes); giving up re-arms the request instead of failing the write.

A compaction failure no longer fails the write whose epilogue triggered it:
the write is durable by then, so the error is reported and the request
re-armed rather than turned into an error the client retries.

Assertions: db.zig, commands.zig and storage.zig had none, which is why every
defect in this series was found by a stress run rather than at the moment of
corruption. src/assert.zig adds an assert that survives ReleaseFast --
std.debug.assert lowers to `unreachable`, which in this project's default
build is not a skipped check but a promise to the optimizer, exactly the wrong
lowering for a durability invariant that might be false. Eleven of them now
cover the commit watermark, the in-flight append count, the compaction
snapshot, and the live/dead counters (whose u64 subtraction would otherwise
underflow into a live count that suppresses compaction forever).

cmd_find asserts that a missing collection implies no matches instead of
silently emitting an empty page for a query that did match.

Dead state removed: Log.defer_sync was never read (only written once by
compact), yet its doc comment instructed callers to follow a defer_sync
protocol that no longer exists and has no effect if followed. Engine's
begin_batch/end_batch were both `_ = self`, so three call sites announced a
batch boundary that wasn't there. Both are gone and the comments now describe
the real contract: appends never sync, Log.sync is the only commit point.

Tests: Log.create's truncation is pinned by a storage test that replays after
reusing a path, and the compaction guard by a db test that drives the flag
directly -- both confirmed to fail without their fix. The threaded test added
alongside them is a smoke test only, and says so: both races have windows too
narrow to hit reliably (compact_snapshot_coll holds each collection's write
lock while snapshotting, so two compactions serialize there and an insert
cannot re-arm compact_pending meanwhile), and it passes with the guard
removed.

Includes an unrelated fix that was already in the working tree: slab_append
held a pointer into slab.items across an append to that same list, which
could dangle after a realloc and corrupt the new segment's start offset.

Verified: unit suite in ReleaseFast/ReleaseSafe/Debug; e2e, e2e2 concurrent,
e2e3, e2e4, e2e5, e2e6 (72/72) and the kill -9 crash pair, with no assertion
firing anywhere.
This commit is contained in:
Alexey Shakhmatov
2026-08-03 11:55:52 +03:00
parent c8d547fef5
commit 720540860a
5 changed files with 499 additions and 93 deletions
Download Patch File Download Diff File Expand all files Collapse all files

83
src/commands.zig
View File

@@ -10,6 +10,9 @@ const Collection = db.Collection;
const query = @import("query.zig");
const update = @import("update.zig");
const index = @import("index.zig");
// Always active, including in the default ReleaseFast build -- see assert.zig.
const assert = @import("assert.zig").assert;
const assert_msg = @import("assert.zig").assert_msg;
pub const Context = struct {
gpa: std.mem.Allocator,
@@ -153,9 +156,24 @@ pub fn dispatch(ctx: *Context, msg: *wire.Message, reply: *wire.Reply) !void {
if (catalog_held) ctx.engine.unlock_catalog(cmd.locks.catalog == .exclusive);
catalog_held = false;
if (cmd.locks.coll == .exclusive) {
// Durability (seal + fsync) coalesces across concurrent writers.
// Durability (seal + fsync) coalesces across concurrent writers. A
// commit error deliberately wins over the handler's captured `result`:
// whether the write reached disk matters more to the client than why
// the write itself was unhappy.
// commit() asserts its own postcondition (committed_seq >= this
// command's seq) internally. Re-checking it here is not possible
// without the log lock, and taking it just to assert would add a real
// race in exchange for a weaker check than the one already made.
try ctx.engine.commit();
if (ctx.engine.take_compact()) try ctx.engine.compact();
// The write is durable by now, so a compaction failure is a maintenance
// problem and not the client's. Report it and hand the request back
// rather than turning an applied write into an error the client retries.
if (ctx.engine.take_compact()) {
ctx.engine.compact() catch |err| {
std.debug.print("mongo-lite: compaction failed: {s}\n", .{@errorName(err)});
ctx.engine.request_compact();
};
}
}
return result;
}
@@ -561,12 +579,11 @@ fn cmd_insert(ctx: *Context, msg: *wire.Message, reply: *wire.Reply) !void {
var write_errors: std.ArrayListUnmanaged(bson.Value) = .empty;
defer write_errors.deinit(reply.arena_alloc());
// Group commit: one fsync for the whole batch instead of one per
// document. end_batch runs on every return path, so even a failed doc
// (writeErrors) or a hard error still syncs what was appended.
ctx.engine.begin_batch();
defer ctx.engine.end_batch() catch {};
// Group commit: one fsync for the whole batch instead of one per document.
// Nothing to open or close here -- appends never sync, and the dispatch
// epilogue is the single commit point. It runs on every return path, so a
// failed doc (writeErrors) or a hard error still syncs what was appended,
// and does it after the collection lock is released.
for (docs, 0..) |*doc, i| {
if (ctx.engine.insert(db_name, coll_name, doc, ctx.oid_gen)) |_| {
inserted += 1;
@@ -624,19 +641,24 @@ fn cmd_find(ctx: *Context, msg: *wire.Message, reply: *wire.Reply) !void {
// Sorting and emitting need the documents as trees; materialize the
// matched page into the reply arena (the slab itself is never copied).
const coll = ctx.engine.get_collection(db_name, coll_name) orelse {
// A find on a namespace that does not exist is an empty cursor, not
// an error -- and above all not a reply with no `ok` at all, which
// is what returning here without one sends.
const none: []const *const bson.Document = &.{};
try emit_docs_tree(reply, db_name, coll_name, proj_pairs, none);
return reply.put_ok();
};
// A find on a namespace that does not exist is an empty cursor, not an
// error and not a reply missing `ok`: the scan above matched nothing, so
// falling through to the emit at the end of this function says exactly
// that without a second exit path to keep in step with it.
const coll = ctx.engine.get_collection(db_name, coll_name);
// The scan above ran against this same collection with the catalog lock
// held, so a missing collection means nothing matched. Asserted rather than
// left implicit: if that ever stops holding, the loop below silently emits
// an empty page for a query that did match, which is the hardest kind of
// wrong answer to notice.
if (coll == null) assert_msg(matched.items.len == 0, "find matched documents in a collection that does not exist");
// Lives in the reply arena; freed with it.
var tree_docs: std.ArrayListUnmanaged(*const bson.Document) = .empty;
const arena = reply.arena_alloc();
for (matched.items) |off| {
try tree_docs.append(arena, try doc_tree(arena, coll, off));
if (coll) |c| {
for (matched.items) |off| {
try tree_docs.append(arena, try doc_tree(arena, c, off));
}
}
if (sort_keys.len > 0 and !index_sorted) {
// Selecting the page is much cheaper than ordering everything when
@@ -771,10 +793,8 @@ fn cmd_update(ctx: *Context, msg: *wire.Message, reply: *wire.Reply) !void {
var write_errors: std.ArrayListUnmanaged(bson.Value) = .empty;
defer write_errors.deinit(reply.arena_alloc());
// Group commit for multi-document updates: one fsync per command.
ctx.engine.begin_batch();
defer ctx.engine.end_batch() catch {};
// Group commit for multi-document updates: one fsync per command, issued
// by the dispatch epilogue once the collection lock is released.
for (specs, 0..) |*spec, si| {
const q = doc_arg(spec.get("q")) orelse return bad_value(reply, "update spec requires q");
const u_doc = doc_arg(spec.get("u")) orelse return bad_value(reply, "update spec requires u");
@@ -850,10 +870,8 @@ fn cmd_delete(ctx: *Context, msg: *wire.Message, reply: *wire.Reply) !void {
const specs = try batch_arg(msg, reply, "delete", "deletes") orelse return;
var n_deleted: i64 = 0;
// Group commit for multi-document deletes: one fsync per command.
ctx.engine.begin_batch();
defer ctx.engine.end_batch() catch {};
// Group commit for multi-document deletes: one fsync per command, issued
// by the dispatch epilogue once the collection lock is released.
for (specs) |*spec| {
const q = doc_arg(spec.get("q")) orelse return bad_value(reply, "delete spec requires q");
const limit = int_value(spec.get("limit")) orelse 1;
@@ -1478,9 +1496,9 @@ fn duplicate_key_message(ctx: *Context, reply: *wire.Reply, db_name: []const u8,
const coll = ctx.engine.get_collection(db_name, coll_name);
if (coll) |c| {
if (c.dup_index) |name| {
index_name = name;
key_text = try render_dup_key(ctx, reply, db_name, coll_name, name, doc);
return e11000_message(reply, db_name, coll_name, index_name, key_text);
index_name = name;
key_text = try render_dup_key(ctx, reply, db_name, coll_name, name, doc);
return e11000_message(reply, db_name, coll_name, index_name, key_text);
}
}
key_text = try serialize_value_compact(reply, doc.get("_id") orelse bson.Value.null);
@@ -2098,7 +2116,6 @@ test "unique index constraint returns 11000 through insert and update" {
try testing.expectEqual(@as(i64, 11000), bson.get_pair(up_errs[0].doc, "code").?.int32);
}
/// Free a list of serialized ids (each element is gpa-owned).
fn free_id_list(gpa: std.mem.Allocator, list: *std.ArrayListUnmanaged([]u8)) void {
for (list.items) |id| gpa.free(id);
@@ -2184,7 +2201,7 @@ test "indexed queries are equivalent to scans over a mixed corpus" {
.{ .key = "_id", .value = .{ .int32 = 3 } },
.{ .key = "a", .value = .{ .double = 30.0 } },
.{ .key = "b", .value = .null },
.{ .key = "tags", .value = .{ .array = &.{ .{ .string = "a" } } } },
.{ .key = "tags", .value = .{ .array = &.{.{ .string = "a" }} } },
} },
.{ .doc = &.{
.{ .key = "_id", .value = .{ .string = "s4" } },
@@ -2228,13 +2245,13 @@ test "indexed queries are equivalent to scans over a mixed corpus" {
.{ .pairs = &.{.{ .key = "a", .value = .{ .doc = &.{.{ .key = "$lt", .value = .{ .int32 = 30 } }} } }} },
.{ .pairs = &.{.{ .key = "a", .value = .{ .doc = &.{.{ .key = "$lte", .value = .{ .int32 = 30 } }} } }} },
.{ .pairs = &.{.{ .key = "a", .value = .{ .doc = &.{.{ .key = "$in", .value = .{ .array = &.{ .{ .int32 = 10 }, .{ .int32 = 30 } } } }} } }} },
.{ .pairs = &.{.{ .key = "a", .value = .{ .doc = &.{ .{ .key = "$gt", .value = .{ .int32 = 5 } }, .{ .key = "$lt", .value = .{ .int32 = 25 } } } }} } },
.{ .pairs = &.{.{ .key = "a", .value = .{ .doc = &.{ .{ .key = "$gt", .value = .{ .int32 = 5 } }, .{ .key = "$lt", .value = .{ .int32 = 25 } } } } }} },
.{ .pairs = &.{.{ .key = "b", .value = .{ .string = "x" } }} },
.{ .pairs = &.{.{ .key = "b", .value = .null }} },
.{ .pairs = &.{ .{ .key = "a", .value = .{ .int32 = 10 } }, .{ .key = "b", .value = .{ .string = "x" } } } },
.{ .pairs = &.{.{ .key = "tags", .value = .{ .string = "a" } }} },
.{ .pairs = &.{.{ .key = "tags", .value = .{ .array = &.{ .{ .string = "a" }, .{ .string = "b" } } } }} },
.{ .pairs = &.{.{ .key = "tags", .value = .{ .doc = &.{.{ .key = "$all", .value = .{ .array = &.{ .{ .string = "a" } } } }} } }} },
.{ .pairs = &.{.{ .key = "tags", .value = .{ .doc = &.{.{ .key = "$all", .value = .{ .array = &.{.{ .string = "a" }} } }} } }} },
.{ .pairs = &.{.{ .key = "a", .value = .{ .doc = &.{.{ .key = "$regex", .value = .{ .string = "^1" } }} } }} },
.{ .pairs = &.{.{ .key = "c", .value = .{ .doc = &.{.{ .key = "$exists", .value = .{ .bool = false } }} } }} },
.{ .pairs = &.{.{ .key = "a", .value = .null }} },