diff --git a/src/db.zig b/src/db.zig index 4ac2479..771a0e9 100644 --- a/src/db.zig +++ b/src/db.zig @@ -1,11 +1,23 @@ -//! In-memory database engine backed by the append-only log. Maps -//! db -> collection -> _id(serialized) -> owned Document. All mutations are -//! logged and synced before they become visible in memory, so a crash never -//! loses a committed write. Callers must hold the write lock (`lock`) around -//! any command that mutates state, and the read lock (`lock_read`) around -//! read-only commands so reads overlap with each other. +//! Database engine over an mmap'd data file with the append-only log in front +//! of it as the write-ahead log. Maps db -> collection -> `_id_` B+tree -> +//! absolute slab offset; documents, tree pages and overflow records all live in +//! the data file, so resident memory is the working set rather than the size of +//! the database. All mutations are logged and synced before they become visible, +//! so a crash never loses a committed write, and a checkpoint publishes the data +//! file and truncates the log so an open does not replay everything ever +//! written. Callers must hold the write lock (`lock`) around any command that +//! mutates state, and the read lock (`lock_read`) around read-only commands so +//! reads overlap with each other. +//! +//! Two invariants the rest of this file depends on. A checkpoint never renumbers +//! slab offsets, because index leaves hold them physically -- only `compact` +//! moves documents, and it rebuilds every index in the same pass. And an index +//! must never under-approximate: it generates candidates and the full filter is +//! re-applied to those, so a missing entry is a missing query result that +//! nothing else detects (see `assert_indexes_cover_every_document`). const std = @import("std"); +const builtin = @import("builtin"); const bson = @import("bson.zig"); const storage = @import("storage.zig"); const index = @import("index.zig"); @@ -374,6 +386,7 @@ pub const Engine = struct { // once replay completes (order-independent). A checkpointed open finds // them already populated, and the guard in rebuild_index skips them. try engine.build_all_indexes(); + engine.assert_indexes_cover_every_document(); // Everything replayed is durable by definition -- it was read back off // the log -- so the commit watermark starts level with the sequence. engine.committed_seq = engine.seq; @@ -1408,6 +1421,55 @@ pub const Engine = struct { /// as a candidate generator; future writes are still enforced) — the /// database always opens, leaving dropIndexes as an in-band recovery /// path. + /// Every document is reachable through every index that is supposed to cover + /// it, checked once at the end of an open. + /// + /// An index that is merely *incomplete* is the worst failure this engine can + /// have, because nothing reports it: the index only generates candidates and + /// the full filter is re-applied to those, so a missing entry is a missing + /// query result and every other check still passes. That is exactly how the + /// replay-time `createIndex` bug survived -- `countDocuments` was right, + /// `find({})` was right, and only `find({k: v})` was quietly short. + /// + /// `_id_` is exact: one entry per document, always. A secondary index is + /// checked only when its shape makes the count exact -- `sparse` omits + /// documents missing the key, and `multikey` contributes several entries for + /// one document -- so those are compared as a lower bound instead of an + /// equality. Debug and ReleaseSafe only; an open is not a hot path, but a + /// full index walk per collection is not free either. + fn assert_indexes_cover_every_document(self: *Engine) void { + if (builtin.mode == .ReleaseFast or builtin.mode == .ReleaseSmall) return; + var db_it = self.dbs.iterator(); + while (db_it.next()) |db_entry| { + var coll_it = db_entry.value_ptr.collections.iterator(); + while (coll_it.next()) |coll_entry| { + const coll = coll_entry.value_ptr.*; + assert_msg( + coll.id_index.count() == coll.doc_count, + "the _id_ index must hold exactly one entry per document after an open", + ); + assert_msg( + coll.id_index.unreachable_key_count() == 0, + "every _id_ entry must be findable by descent, not only by iteration", + ); + for (coll.indexes.items) |ix| { + // Reachability applies to every index whatever its shape: an + // entry in the leaf chain that a descent cannot find is a + // query result that silently goes missing. + if (ix.unreachable_key_count() != 0) { + ix.dbg_root(); + @panic("unreachable index entries"); + } + if (ix.sparse or ix.multikey) continue; + assert_msg( + ix.count() >= coll.doc_count, + "a non-sparse index must cover every document after an open", + ); + } + } + } + } + fn build_all_indexes(self: *Engine) !void { var db_it = self.dbs.iterator(); while (db_it.next()) |db_entry| { @@ -1812,11 +1874,15 @@ pub const Engine = struct { /// Register an (empty) index from a persisted spec document. A repeated /// create record for the same name is an idempotent no-op. + /// Register an index from a logged spec. Returns the new index, or null when + /// one of that name was already present (a re-registration is a no-op, not an + /// error). The caller needs the pointer because an index registered during + /// replay may have to be built over documents that replay will never see. fn register_index_from_spec( self: *Engine, coll: *Collection, spec_doc: *const bson.Document, - ) !void { + ) !?*index.Index { const parsed = try index.parse_spec(self.gpa, self.pager, spec_doc); const ix = self.gpa.create(index.Index) catch |err| { var dead = parsed; @@ -1829,9 +1895,10 @@ pub const Engine = struct { ix.deinit(self.gpa); self.gpa.destroy(ix); }; - if (coll.find_index(ix.name) != null) return; + if (coll.find_index(ix.name) != null) return null; try coll.indexes.append(self.gpa, ix); committed = true; + return ix; } }; @@ -1864,12 +1931,38 @@ fn apply_record(ctx: *anyopaque, record: storage.Record, doc: *bson.Document) an // after replay completes. switch (record.type) { storage.record_type_index_create => { - self.register_index_from_spec(coll, doc) catch |err| { + const registered = self.register_index_from_spec(coll, doc) catch |err| { std.debug.print("multiforadb: index create record failed to apply: {s}\n", .{ @errorName(err), }); return; }; + // A checkpointed open replays only what the watermark does not cover, + // so the documents already in the image never reach this index. Build + // it over them now, which is what the live `create_index` command + // does with pre-existing documents. + // + // Leaving it to `build_all_indexes` does not work and fails silently: + // the next upsert in the log puts one entry in, and a non-empty index + // is skipped by the `count() > 0` guard there -- so the index ends up + // holding the documents logged after its creation and none of the + // ones logged before, which is an index that under-approximates. + // + // Only for a maintaining replay. A full replay leaves every secondary + // index empty on purpose and `build_all_indexes` fills them in one + // pass at the end, which is cheaper than one pass per index here. + if (self.replay_maintains_indexes) { + if (registered) |ix| self.rebuild_index(coll, ix) catch |err| { + // The database must always open (ground rule 4). A failure + // here leaves the index short, so say so rather than leaving + // a query to be quietly wrong about it. + std.debug.print( + "multiforadb: WARNING: index '{s}' could not be built over existing " ++ + "documents during replay: {s}; drop and re-create it\n", + .{ ix.name, @errorName(err) }, + ); + }; + } return; }, storage.record_type_index_drop => { @@ -2106,6 +2199,174 @@ test "compaction reclaims garbage but leaves a garbage-free log alone" { try testing.expect(engine.log.data_bytes < after_insert * 2); } +test "a secondary index stays reachable across checkpoints, churn and a rebuild" { + // The one path the index unit tests cannot reach: copy-on-write. `test_pager` + // never publishes a watermark, so `stable_pages` is 0 there and every page is + // writable in place -- no node page is ever relocated. Through the engine a + // checkpoint makes the whole image stable, so the next tree mutation copies + // each node it touches to a fresh page and rewrites the id->page slot. + // + // Few distinct keys on purpose: ten values over thousands of documents means + // each value spans many leaves and most interior separators are duplicates, + // which is the shape the crash fuzzer fails on. + var threaded: std.Io.Threaded = .init_single_threaded; + defer threaded.deinit(); + var env = test_env(&threaded); + const io = env.io; + const gpa = testing.allocator; + + var tmp = try TmpLog.init(gpa); + defer tmp.deinit(gpa); + var engine = try Engine.open(gpa, io, tmp.path); + defer engine.deinit(); + engine.compact_threshold = 64 * 1024; // rebuild often, like --heavy + try engine.lock(); + defer engine.unlock(); + + var spec = try index_spec(gpa, "k", "k_1", false, false, null); + defer spec.deinit(); + _ = try engine.create_index("app", "c", &spec); + + const n_keys: i32 = 10; + const n: i32 = 1200; + var id: i32 = 0; + while (id < n) : (id += 1) { + var d = try make_keyed(gpa, id, @mod(id, n_keys)); + defer d.deinit(); + try engine.insert("app", "c", &d, &env.gen); + + // Checkpoint, churn and rebuild interleaved with the writes, so tree + // mutations land on pages the last checkpoint froze. + if (@mod(id, 150) == 0) { + try engine.commit(); + try engine.checkpoint(); + } + if (@mod(id, 7) == 0 and id > 20) { + _ = try engine.remove_by_id("app", "c", .{ .int32 = id - 20 }); + } + if (engine.take_compact()) try engine.compact(); + } + try engine.commit(); + try engine.checkpoint(); + + const coll = engine.get_collection("app", "c").?; + const ix = coll.find_index("k_1").?; + + // Every entry the leaf chain holds must also be findable by descending from + // the root, which is the only way a query reaches it. + try testing.expectEqual(@as(u32, 0), ix.unreachable_key_count()); + try testing.expectEqual(@as(u32, 0), coll.id_index.unreachable_key_count()); + + // And per key, the index must agree with a scan of the documents. + var k: i32 = 0; + while (k < n_keys) : (k += 1) { + var want: usize = 0; + var scan = coll.id_index.iter(); + while (scan.next()) |e| { + const kv = try bson.get_at(gpa, coll.doc_bytes(e.off), "k"); + if (kv) |v| if (v.int32 == k) { + want += 1; + }; + } + var out: std.ArrayListUnmanaged(u64) = .empty; + defer out.deinit(gpa); + try ix.lookup_eq(gpa, &.{.{ .int32 = k }}, &out); + testing.expectEqual(want, out.items.len) catch |err| { + std.debug.print(" key {d}: index {d}, scan {d}, entry_count {d}\n", .{ + k, + out.items.len, + want, + ix.count(), + }); + return err; + }; + } +} + +test "an index created after the checkpoint indexes the documents that predate it" { + // Found by tests/fuzz/crash-fuzz.js in --heavy mode, roughly once per 700 + // crash/reopen cycles, as `find({k:v})` returning nothing for a key that has + // documents. `countDocuments` and `find({})` were right, so the documents + // were there and only the index's answer about them was wrong -- an index + // that under-approximates, which is silent by construction: the index only + // generates candidates and the full filter is re-applied to those, so a + // missing entry is a missing result and nothing complains. + // + // The sequence needs three things at once: a checkpoint, a `createIndex` + // logged after it, and a write after that. + // + // 1. documents exist and a checkpoint puts them in the durable image + // 2. createIndex is logged *after* the watermark + // 3. another document is written, also after the watermark + // + // On reopen the catalog restores step 1's documents but not the index, so + // replay starts at the watermark and never sees them. Replay registers the + // index empty and -- because a checkpointed open maintains indexes as it + // replays -- step 3's document goes in. The index is now non-empty and + // incomplete, so `rebuild_index`'s `count() > 0` guard skips it and step 1's + // documents are never indexed at all. + var threaded: std.Io.Threaded = .init_single_threaded; + defer threaded.deinit(); + var env = test_env(&threaded); + const io = env.io; + const gpa = testing.allocator; + + var tmp = try TmpLog.init(gpa); + defer tmp.deinit(gpa); + { + var engine = try Engine.open(gpa, io, tmp.path); + defer engine.deinit(); + try engine.lock(); + + // 1. Two documents, made durable in the data file. + var d1 = try make_user(gpa, 1, "a@x.io"); + defer d1.deinit(); + var d2 = try make_user(gpa, 2, "b@x.io"); + defer d2.deinit(); + try engine.insert("app", "users", &d1, &env.gen); + try engine.insert("app", "users", &d2, &env.gen); + try engine.commit(); + try engine.checkpoint(); + + // 2. The index arrives after the watermark. + var spec = try index_spec(gpa, "email", "email_1", false, false, null); + defer spec.deinit(); + _ = try engine.create_index("app", "users", &spec); + + // 3. And a write after that, which is what makes the index non-empty on + // replay and so hides the two documents behind the `count() > 0` guard. + var d3 = try make_user(gpa, 3, "c@x.io"); + defer d3.deinit(); + try engine.insert("app", "users", &d3, &env.gen); + try engine.commit(); + engine.unlock(); + // No second checkpoint: the watermark still predates the createIndex. + } + + var engine2 = try Engine.open(gpa, io, tmp.path); + defer engine2.deinit(); + const coll = engine2.get_collection("app", "users").?; + const ix = coll.find_index("email_1").?; + + // One entry per document. Under-approximation is the whole failure mode, so + // the count is the assertion that matters. + try testing.expectEqual(@as(u64, 3), coll.doc_count); + try testing.expectEqual(@as(usize, 3), ix.count()); + + // And every entry resolves to a document whose email re-encodes to its key, + // so the entries are the right ones and not merely the right number. + var seen: [3]bool = .{ false, false, false }; + var it = ix.iter(); + while (it.next()) |entry| { + const doc_id = (try bson.get_at(gpa, coll.doc_bytes(entry.off), "_id")).?; + const idx: usize = @intCast(doc_id.int32 - 1); + try testing.expect(idx < seen.len); + try testing.expect(!seen[idx]); + seen[idx] = true; + } + try testing.expect(seen[0] and seen[1] and seen[2]); +} + test "reopening without a checkpoint reuses the data file instead of appending to it" { // Mutation check: delete the `loaded.generation == 0` reset of `alloc_tail` // in `Pager.open`. Red -- each reopen starts allocating above the previous @@ -2831,6 +3092,15 @@ fn index_count( return 0; } +fn make_keyed(gpa: std.mem.Allocator, id: i32, k: i32) !bson.Document { + var arena = std.heap.ArenaAllocator.init(gpa); + errdefer arena.deinit(); + const pairs = try arena.allocator().alloc(bson.Pair, 2); + pairs[0] = .{ .key = try arena.allocator().dupe(u8, "_id"), .value = .{ .int32 = id } }; + pairs[1] = .{ .key = try arena.allocator().dupe(u8, "k"), .value = .{ .int32 = k } }; + return .{ .arena = arena, .pairs = pairs }; +} + fn make_user(gpa: std.mem.Allocator, id: i32, email: []const u8) !bson.Document { var arena = std.heap.ArenaAllocator.init(gpa); errdefer arena.deinit(); diff --git a/src/index.zig b/src/index.zig index 715b8d0..afda333 100644 --- a/src/index.zig +++ b/src/index.zig @@ -798,6 +798,70 @@ pub const Index = struct { return .{ .ix = self, .leaf = self.first_leaf, .slot = 0 }; } + /// TEMPORARY: does the parent's child order match the leaf chain, and is each + /// separator really its child's first key? + pub fn dbg_root(self: *const Index) void { + const rt = self.page(self.root); + std.debug.print(" root={d} count={d} first_child={d} depth={d}\n", .{ self.root, rt.count, rt.first_child, self.depth }); + // chain position of every leaf + var pos_of = std.mem.zeroes([512]i32); + for (&pos_of) |*v| v.* = -1; + var lf = self.first_leaf; + var pos: i32 = 0; + while (lf != 0) : (pos += 1) { + if (lf < pos_of.len) pos_of[lf] = pos; + lf = self.page(lf).next; + } + var prev_pos: i32 = pos_of[rt.first_child]; + var i: u32 = 0; + while (i < rt.count) : (i += 1) { + const child = get_slot(rt, i).extra; + const cp = if (child < pos_of.len) pos_of[child] else -2; + const sep = self.key_of(self.root, i); + const cfirst = if (self.page(child).count > 0) self.key_of(child, 0) else ""; + const sep_wrong = cfirst.len > 0 and !std.mem.eql(u8, sep, cfirst); + const order_wrong = cp != prev_pos + 1; + if (sep_wrong or order_wrong) { + std.debug.print(" slot[{d}] child={d} chainpos={d} (prev {d}){s}{s}\n sep ={x}\n first={x}\n", .{ + i, child, cp, prev_pos, + if (order_wrong) " ORDER" else "", if (sep_wrong) " SEP!=FIRST" else "", sep, cfirst, + }); + } + prev_pos = cp; + } + } + + /// Debug aid: how many distinct keys are present in the leaf chain but not + /// findable by descending from the root. + /// + /// Iteration and descent are two independent ways to reach an entry, and a + /// query only ever uses descent. `count()` cannot tell them apart -- it + /// returns a stored counter -- so an index whose leaves are intact but whose + /// interior nodes no longer route to them looks perfectly healthy by every + /// other measure, and silently answers a query with fewer documents than it + /// holds. That is what the crash fuzzer caught: a handful of key values + /// returning nothing while every other value was exact. + /// + /// O(distinct keys x depth). For assertions and tests, not for the hot path. + pub fn unreachable_key_count(self: *const Index) u32 { + var bad: u32 = 0; + var it = self.iter(); + var prev: ?[]const u8 = null; + while (it.next()) |e| { + if (prev) |p| { + if (std.mem.eql(u8, p, e.key)) continue; + } + prev = e.key; + var probe = self.seek(e.key); + const first = probe.next() orelse { + bad += 1; + continue; + }; + if (cmp_prefix(e.key, first.key) != .eq) bad += 1; + } + return bad; + } + /// Reverse ordered iteration. Leaves are doubly linked and `prev` has /// always been maintained -- nothing walked it until now, so a descending /// scan had to materialize every candidate and reverse the list. This turns @@ -1242,6 +1306,36 @@ pub const Index = struct { /// Separator position in an internal node: after any equal keys, so the /// "last separator <= key" descent lands on the newest right child. + /// The slot at which a new right sibling of `left` belongs: immediately after + /// `left`'s own position among this node's children. + /// + /// Deliberately positional, not a search for the promoted key. The two agree + /// only while separators are distinct. When several children share a + /// separator -- ten distinct values across thousands of documents, so each + /// value spans dozens of leaves -- `separator_pos` returns the slot after the + /// *whole* equal-key run, which puts the new sibling at the end of that run + /// while the leaf chain has it immediately after `left`. + /// + /// Parent child order then no longer matches leaf chain order, and that is + /// the one thing a lookup cannot survive: `descend_lower` picks the last + /// child of the equal run, and `lookup_eq` walks forward from there over keys + /// that are *smaller* than the one it wants, so it stops at the first + /// mismatch and reports nothing. The entries are all present, the chain is + /// correctly ordered, `count()` is right -- and a query returns an empty + /// result. Found by tests/fuzz/crash-fuzz.js after ~700 heavy cycles as + /// `find({k: 3})` returning 0 of 401 documents while every other key was + /// exact. + fn child_slot_after(self: *const Index, node_id: u32, left: u32) u32 { + const node = self.page(node_id); + if (node.first_child == left) return 0; + var i: u32 = 0; + while (i < node.count) : (i += 1) { + if (get_slot(node, i).extra == left) return i + 1; + } + assert_msg(false, "a split's left sibling must be a child of the node taking its separator"); + return node.count; + } + fn separator_pos(self: *const Index, node_id: u32, key: []const u8) u32 { const node = self.page(node_id); var lo: u32 = 0; @@ -1352,7 +1446,7 @@ pub const Index = struct { } const child = self.descend_insert(node_id, key); const res = self.insert_rec(child, key, off) orelse return null; - return self.insert_separator(node_id, res); + return self.insert_separator(node_id, child, res); } /// Split a full leaf around the record being inserted. The new record @@ -1426,7 +1520,7 @@ pub const Index = struct { /// Insert a promoted separator into an internal node, splitting it when /// full. Returns the next promotion, or null. - fn insert_separator(self: *Index, node_id: u32, split: Split) ?Split { + fn insert_separator(self: *Index, node_id: u32, left: u32, split: Split) ?Split { // The incoming key may live in the promo buffer, which a nested // split_internal (below) would overwrite with its own promoted key; // spilled keys already live in the immutable slab. Copy inline keys @@ -1443,7 +1537,7 @@ pub const Index = struct { self.repack_keep_prefix(node_id, self.page(node_id).count); } if (self.fits(node_id, key.len)) { - self.store_record(node_id, self.separator_pos(node_id, key), .{ + self.store_record(node_id, self.child_slot_after(node_id, left), .{ .key = key, .child = split.right, .spill_off = split.spill_off, @@ -1451,7 +1545,7 @@ pub const Index = struct { self.page_mut(split.right).parent = node_id; return null; } - return self.split_internal(node_id, key, split.spill_off, split.right); + return self.split_internal(node_id, left, key, split.spill_off, split.right); } /// Split a full internal node around the separator being inserted: the @@ -1462,13 +1556,15 @@ pub const Index = struct { fn split_internal( self: *Index, node_id: u32, + left: u32, key: []const u8, spill_off: ?u64, child: u32, ) Split { const old_count = self.page(node_id).count; std.debug.assert(old_count >= 2); - const pos = self.separator_pos(node_id, key); + // Positional, for the reason `child_slot_after` documents. + const pos = self.child_slot_after(node_id, left); const n = old_count + 1; var costs: [max_slots + 1]u32 = undefined; @@ -2345,6 +2441,50 @@ fn simple_index( return Index.init(gpa, pager, "test", keys[0..paths.len], unique, sparse, null); } +test "a bulk build with many duplicate keys stays reachable for every key" { + // The shape the crash fuzzer failed on: ~4000 entries over 10 distinct key + // values, so each value spans several leaves and the interior separators + // repeat. `find({k:v})` came back empty for a few values and exactly right + // for the rest, with `count()` still reporting every entry -- entries that + // exist and cannot be reached. + const gpa = testing.allocator; + var ix = try simple_index(gpa, test_pager(), &.{"k"}, false, false); + defer ix.deinit(gpa); + + const n_docs: usize = 4000; + const n_keys: i32 = 10; + var docs: std.ArrayListUnmanaged([]u8) = .empty; + defer { + for (docs.items) |d| gpa.free(d); + docs.deinit(gpa); + } + for (0..n_docs) |i| { + const k: i32 = @intCast(@mod(@as(i32, @intCast(i)), n_keys)); + const pairs = [_]bson.Pair{.{ .key = "k", .value = .{ .int32 = k } }}; + const bytes = try bytes_of(gpa, &pairs); + try docs.append(gpa, bytes); + try ix.append_doc_entries(gpa, bytes, @intCast(i + 1)); + } + _ = try ix.finish_bulk(gpa, false); + try testing.expectEqual(n_docs, ix.count()); + + // Iteration must see every entry: that separates "never inserted" from + // "inserted and unreachable from the root". + var walked: usize = 0; + var wit = ix.iter(); + while (wit.next()) |_| walked += 1; + try testing.expectEqual(n_docs, walked); + + // And every key must be reachable by descent, which is what a query does. + var k: i32 = 0; + while (k < n_keys) : (k += 1) { + var out: std.ArrayListUnmanaged(u64) = .empty; + defer out.deinit(gpa); + try ix.lookup_eq(gpa, &.{.{ .int32 = k }}, &out); + try testing.expectEqual(n_docs / @as(usize, @intCast(n_keys)), out.items.len); + } +} + /// Look up the documents under `key` and compare with the expected offsets. /// A leaf record's payload is a slab offset now, so tests identify documents by /// small distinct numbers rather than by byte-string ids. @@ -2884,6 +3024,159 @@ test "incremental inserts and removals stay identical to a brute-force model" { } } +test "splitting inside a run of equal separators keeps every key reachable" { + // The crash fuzzer's exact shape, and the reason the two differentials above + // miss it: bulk-pack first, *then* keep inserting. + // + // A packed tree has full leaves, so the next inserts split leaves in the + // middle of a run of equal separators -- and a new right sibling placed by + // key rather than by position lands at the end of that run, so the parent's + // child order stops matching the leaf chain. A purely incremental build + // leaves leaves half full and rarely produces the geometry. + const gpa = testing.allocator; + var ix = try simple_index(gpa, test_pager(), &.{"k"}, false, false); + defer ix.deinit(gpa); + + const n_keys: i32 = 10; + const packed_docs: usize = 4000; + const grown_docs: usize = 2000; + + // Phase 1: bulk pack, which fills every leaf. + for (0..packed_docs) |i| { + const k: i32 = @intCast(@mod(@as(i32, @intCast(i)), n_keys)); + const d = try bytes_of(gpa, &.{.{ .key = "k", .value = .{ .int32 = k } }}); + defer gpa.free(d); + try ix.append_doc_entries(gpa, d, @intCast(i + 1)); + } + _ = try ix.finish_bulk(gpa, false); + ix.pager.release_reservation(&ix.hold); + + // Phase 2: grow it in random key order, which is what puts a split on the + // leaf *before* an equal-key run -- the case where the new right sibling's + // promoted key equals the run's key while its chain position is at the run's + // start. Round-robin never produces it: every insert routes to the last + // child of its run, and a split there belongs at the end of the run anyway. + var prng = std.Random.DefaultPrng.init(0xbad_5eed); + const rand = prng.random(); + for (0..grown_docs) |j| { + const k = rand.intRangeLessThan(i32, 0, n_keys); + const d = try bytes_of(gpa, &.{.{ .key = "k", .value = .{ .int32 = k } }}); + defer gpa.free(d); + _ = try ix.add_doc(gpa, d, @intCast(packed_docs + j + 1), false); + ix.pager.release_reservation(&ix.hold); + if (j % 25 == 0) { + testing.expectEqual(@as(u32, 0), ix.unreachable_key_count()) catch |err| { + std.debug.print(" went unreachable after {d} grown inserts\n", .{j}); + return err; + }; + } + } + + try testing.expectEqual(packed_docs + grown_docs, ix.count()); + // Every entry the chain holds must be findable by descent too. + try testing.expectEqual(@as(u32, 0), ix.unreachable_key_count()); + + // And a lookup must find as many entries as iteration holds for that key. + var k: i32 = 0; + while (k < n_keys) : (k += 1) { + var enc: std.ArrayListUnmanaged(u8) = .empty; + defer enc.deinit(gpa); + try bson.encode_key(bson.Value{ .int32 = k }, gpa, &enc); + var want: usize = 0; + var wit = ix.iter(); + while (wit.next()) |e| { + if (cmp_prefix(enc.items, e.key) == .eq) want += 1; + } + var out: std.ArrayListUnmanaged(u64) = .empty; + defer out.deinit(gpa); + try ix.lookup_eq(gpa, &.{.{ .int32 = k }}, &out); + testing.expectEqual(want, out.items.len) catch |err| { + std.debug.print(" key {d}: reachable {d}, iteration holds {d} (entry_count {d})\n", .{ + k, out.items.len, want, ix.count(), + }); + return err; + }; + } +} + +test "an incrementally built index with few distinct keys stays reachable" { + // The crash fuzzer's shape, which the existing differentials miss: a single + // key with only ten distinct values over thousands of documents, so each + // value spans dozens of leaves and most interior separators are duplicates. + // The compound-key differential above uses ~961 combinations over 600 + // inserts, which is almost no duplication at all. + // + // Symptom being hunted: `lookup_eq` returning nothing for a few values while + // every other value is exactly right, and `count()` still reporting every + // entry -- entries that exist and cannot be reached by descent. + const gpa = testing.allocator; + var prng = std.Random.DefaultPrng.init(0xd0_0d_1e); + const rand = prng.random(); + var ix = try simple_index(gpa, test_pager(), &.{"k"}, false, false); + defer ix.deinit(gpa); + + const n_keys: i32 = 10; + const n: usize = 3000; + var keys: std.ArrayListUnmanaged(i32) = .empty; + defer keys.deinit(gpa); + var live: std.ArrayListUnmanaged(bool) = .empty; + defer live.deinit(gpa); + + // Interleave inserts and removals, which is what a real workload does and + // what leaves half-empty leaves and one-child internal nodes behind. + for (0..n) |i| { + const k = rand.intRangeAtMost(i32, 0, n_keys - 1); + const off: u64 = @intCast(i + 1); + const d = try bytes_of(gpa, &.{.{ .key = "k", .value = .{ .int32 = k } }}); + defer gpa.free(d); + _ = try ix.add_doc(gpa, d, off, false); + // A bare index test has no engine to do this at a write boundary, and + // without it the promise accumulates and grows the shared test file + // without bound. + ix.pager.release_reservation(&ix.hold); + try keys.append(gpa, k); + try live.append(gpa, true); + + // Remove an earlier document every few inserts. + if (i > 20 and i % 3 == 0) { + const victim = rand.intRangeLessThan(usize, 0, keys.items.len); + if (live.items[victim]) { + const vd = try bytes_of(gpa, &.{.{ .key = "k", .value = .{ .int32 = keys.items[victim] } }}); + defer gpa.free(vd); + ix.remove_doc(gpa, vd, @intCast(victim + 1)); + live.items[victim] = false; + } + } + + // Every value must be reachable by descent, and the counts must match a + // brute-force pass over the model. Checked periodically rather than every + // step: this is 10 descents over a tree of thousands of entries. + if (i % 250 != 0 and i != n - 1) continue; + var k_check: i32 = 0; + while (k_check < n_keys) : (k_check += 1) { + var want: usize = 0; + for (keys.items, live.items) |kk, is_live| { + if (is_live and kk == k_check) want += 1; + } + var out: std.ArrayListUnmanaged(u64) = .empty; + defer out.deinit(gpa); + try ix.lookup_eq(gpa, &.{.{ .int32 = k_check }}, &out); + testing.expectEqual(want, out.items.len) catch |err| { + std.debug.print( + " at insert {d}: key {d} reachable {d}, expected {d} (entry_count {d})\n", + .{ i, k_check, out.items.len, want, ix.count() }, + ); + return err; + }; + } + // And iteration must see exactly as many entries as the tree claims. + var walked: usize = 0; + var wit = ix.iter(); + while (wit.next()) |_| walked += 1; + try testing.expectEqual(ix.count(), walked); + } +} + /// One document's facts in the incremental-mutation differential. const ModelFact = struct { a: i32, b: i32, off: u64 }; diff --git a/src/pager.zig b/src/pager.zig index f2f67b1..0dc1036 100644 --- a/src/pager.zig +++ b/src/pager.zig @@ -771,8 +771,20 @@ pub const Pager = struct { // Round up to a growth chunk, and to the system page size, so a // 16 KiB-page host never gets a partial mapping request. + // + // `alignForwardAnyAlign`, not `alignForward`: the chunk is a *proportion* + // of the current size once the file passes 64 MiB, and `mapped_pages / 8` + // is not a power of two. `alignForward` asserts that it is -- so this + // panicked in safe builds and, worse, in ReleaseFast (where the assert is + // compiled out) computed `(addr + align - 1) & ~(align - 1)` with a + // non-power-of-two mask, which can round *down*. A mapping longer than the + // file is the one thing this function exists to prevent: a store into a + // mapped page past end-of-file raises SIGBUS, which no error path catches. + // + // Never noticed because no unit test grew a pager past 64 MiB, which is + // where the chunk stops being `grow_chunk_pages`. const chunk = @max(grow_chunk_pages, self.mapped_pages / 8); - var new_pages = std.mem.alignForward(u32, want_pages, chunk); + var new_pages = std.mem.alignForwardAnyAlign(u32, want_pages, chunk); const sys_pages: u32 = @intCast(map_align / page_size); if (sys_pages > 1) new_pages = std.mem.alignForward(u32, new_pages, sys_pages); diff --git a/tests/fuzz/crash-fuzz.js b/tests/fuzz/crash-fuzz.js index 50cfdca..e673502 100644 --- a/tests/fuzz/crash-fuzz.js +++ b/tests/fuzz/crash-fuzz.js @@ -561,8 +561,41 @@ async function verify(client, base, r, cycleNo) { .map((d) => canon(d)) .sort(); if (got.length !== want.length || got.some((c, i) => c !== want[i])) { + // Decisive diagnostic: the same question asked without the index. `dbDocs` + // came from find({}) on this same reopened server, so filtering it here + // says whether the *documents* are wrong or only the index's answer about + // them. An index that returns fewer documents than a scan is the canonical + // under-approximation -- candidates are generated from the index and the + // full filter is only re-applied to those, so a missing entry is a + // silently missing result. + const scanGot = dbDocs.filter((d) => d.k === v).map((d) => canon(d)).sort(); throw new Fail(`cycle ${cycleNo}: find({k:${v}}) mismatch at prefix ${matched}`, { - cycleNo, v, matched, got, want, + cycleNo, + v, + matched, + verdict: + scanGot.length === want.length && scanGot.every((c, i) => c === want[i]) + ? 'INDEX under-approximates: a scan of the same server returns the expected documents' + : 'DOCUMENTS differ too: the scan does not match the model either', + indexReturned: got.length, + scanReturned: scanGot.length, + modelExpected: want.length, + // Per-key totals, so a single lost leaf is distinguishable from an + // index that came back empty. + perKeyIndexVsScan: await (async () => { + const rows = []; + for (let u = 0; u < 10; u++) { + const idx = (await coll.find({ k: u }).toArray()).length; + const scan = dbDocs.filter((d) => d.k === u).length; + rows.push({ k: u, index: idx, scan }); + } + return rows; + })(), + indexes: dbIndexes.map((i) => i.name), + totalDocs: dbDocs.length, + serverLog: serverLog.slice(-2000), + got, + want, }); } }