index/pager: place a split's new sibling positionally, and fix mmap growth alignment
Two bugs, both of which the crash fuzzer surfaced and neither of which any
existing test could see.
**A split put the new sibling in the wrong slot when separators repeat.**
`split_leaf` located the new right sibling with `separator_pos(node, key)`, a
search for the promoted key. That agrees with "immediately after `left`" only
while separators are distinct. When several children share one -- 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 sibling at the end of that run while the leaf chain has it right after
`left`.
Parent child order then stops matching 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 *smaller* than the one
it wants, stops at the first mismatch, and reports nothing. Every entry is
present, the chain is correctly ordered, `count()` is right -- and the query
returns empty. `crash-fuzz.js` found it after ~700 heavy cycles as
`find({k: 3})` returning 0 of 401 documents while every other key was exact.
Fixed by `child_slot_after`, which is positional by construction.
**mmap growth rounded with a non-power-of-two alignment.** Past 64 MiB the
growth chunk becomes a proportion of the current size (`mapped_pages / 8`),
which is not a power of two -- and `std.mem.alignForward` asserts that it is.
In safe builds that panicked; in ReleaseFast, where the assert is compiled out,
it computed `(addr + align - 1) & ~(align - 1)` with a non-power-of-two mask,
which can round *down*. A mapping shorter than intended is survivable, but a
mapping longer than the file is exactly what this function exists to prevent: a
store into a mapped page past end-of-file raises SIGBUS, which no error path
catches. `alignForwardAnyAlign` instead. Never noticed because no unit test grew
a pager past 64 MiB.
Also here, because both bugs were invisible rather than merely unfixed:
- `assert_indexes_cover_every_document` (db.zig) checks the index invariant
directly -- an index generates candidates and the full filter is re-applied to
those, so a missing entry is a missing query result nothing else detects.
- `Index.unreachable_key_count` counts keys present in the leaf chain but not
reachable by descending from the root, which is precisely the state above:
healthy by every other measure.
- `Index.dbg_root` dumps parent/chain agreement. Marked TEMPORARY; drop it once
the invariant checks have earned their keep.
- `crash-fuzz.js` now asks the same question without the index, so a failure
says whether the documents are wrong or only the index's answer about them,
and reports per-key totals so one lost leaf is distinguishable from an empty
index.
Verified: `zig build test` in ReleaseFast and ReleaseSafe, and seeded fuzzer
runs that previously reproduced the split bug.
This commit is contained in:
@@ -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,
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user