diff --git a/PLAN.md b/PLAN.md index e0f2270..713fd7c 100644 --- a/PLAN.md +++ b/PLAN.md @@ -631,6 +631,96 @@ The pattern worth noting for M1: both were found by *running* the suites, not by reading them, and the second was only visible because the first stopped masking it. +### Before the free list: eight bugs the M1 design work turned up + +The doc-level free list multiplies traffic through exactly the reclamation +paths, so those paths were read closely before anything was built. Three of the +eight were found that way, by reading. Five were found by the tests written for +the other three — the same lesson M0's gate taught, arriving one milestone +early: a reclamation bug is invisible until something runs long enough, or +concurrently enough, to reach the state that exposes it. + +1. **`write_freelist` counted its entries before allocating its own pages.** + The allocation goes through `take_free`, which `swapRemove`s an exact-fit + entry, so the loop wrote one entry fewer than the count it had already + committed to and the hash landed eight bytes short. `read_freelist` then + declared the list corrupt and dropped all of it. A one-page freelist stream + and a one-page hole are both the common case, so this fired at essentially + every reopen: the free list has been discarded on restart since it existed. + +2. **The free lists were read and written with no lock.** `free_pages` appended + to `free_pending` while `publish` rotated the three lists under `alloc_lock`, + and `write_freelist` walked all three while a concurrent `free_pages` could + reallocate them. Found by the test written for (1). + +3. **A checkpoint never gave back the generation it replaced.** The catalog + stream and the freelist stream are allocated fresh every publish and were + never freed, so an idle server grew forever. + +4. **A checkpoint could publish a watermark above the durable log tail.** The + snapshot's `seq` check does not catch a writer that appended *before* the + walk started and has not committed yet; the window is as wide as an fsync. + This was an assertion, so the failure mode was a server abort under exactly + the load that makes checkpoints frequent — and without the assertion it is + the loss of an acknowledged write, since the truncation that follows a + checkpoint would discard the record. Now a retry: seal and re-snapshot. + +5. **A document append could land in the published image.** `slab_reserve` + checks that the append cursor is writable; `publish` can clear the + unpublished set between that check and `slab_append`'s copy, because the log + append and its fsync sit in between. SIGBUS where `protect_stable` is + compiled in, a silent overwrite of durable data in ReleaseFast, where it is + not. Fixed with a pager-level append lock, held shared by appenders and + exclusively by `publish`; measured at no cost on the write path (8 clients × + 1500 inserts at `{w:1,j:true}`: 23779–24879 docs/s before, 23823–24452 + after). + +6. **`write_catalog` read `slab_extents` under only the shared catalog lock**, + while `slab_reserve` appended to that ArrayList under the collection's. + +7. **The engine's counters were shared by writers holding no lock in common.** + `live_docs`, `dead_docs`, `live_bytes` and `dead_bytes` are updated by a + writer holding its own collection's lock and the catalog's shared — so two + writers on different collections lose each other's updates, and a reader had + no way to see the totals consistently with the per-collection figures they + are supposed to equal. Now `counter_lock`, a leaf, with a `Counters` + snapshot for the two readers that compare them. The only one of the eight + that is **not** mutation-checked: it aborted three of eight ReleaseSafe runs + once (8) made the checkpoint's consistency check reachable, and then would + not re-trigger in 34 further runs, on the reverted fix and on the pre-fix + revision alike. The rate depends on machine load. It stands on inspection, + and the concurrency test now asserts the identity once everything is quiet + rather than relying on catching the race in the act. + +8. **The slab did not count what the appender skips.** `slab_used` only ever + grew by a document's length, so the gap left when a checkpoint pushes the + append cursor up to a system page, and the tail of an extent abandoned for a + document that no longer fits, were counted nowhere — real garbage, + invisible to the trigger that decides whether a rebuild is worth doing, and + part of the 1.65×/2.47× the churn gate measured. + +Accounting is now an identity rather than four independent counters: +`dead_bytes` is the sum of `slab_used - live_bytes` over the collections that +exist. `read_catalog` recomputes it on open instead of trusting the watermark's +hint, `compact` recomputes it from what a rebuild leaves behind instead of +zeroing it, and `write_catalog` returns both sums for the checkpoint to assert +under the quiescence condition it already had. That assertion is what surfaced +(7) and what mutation-checks (5) and (8). + +Two of the eight — the drop that charged a dropped collection's live bytes to +`dead_bytes`, and the counters — also changed what the watermark is for: its +`dead_bytes` field is now a hint for anything inspecting the header, not a +source of truth, because a collection dropped after the last checkpoint is gone +from the catalog and would still be charged for in the hint. + +**Still open, deliberately.** `rebuild_collection` frees the pages it abandoned +while holding only the collection's lock, and a concurrent `checkpoint` may +already have snapshotted a catalog that claims them. The `seq` retry does not +see it, because a rebuild appends no log record. The fix is mutual exclusion +between `compact` and `checkpoint`; it is out of this scope because it wants +its own design pass, and because the free list must not add a second instance +of the same shape. + --- ## 6. Deferred designs (grill each at its milestone) @@ -670,7 +760,9 @@ it. caught by draining a collection being updated underneath. Still open in M1: the doc-level free list, sessions plumbing (`lsid` - accepted), and command-monitoring (`expectEvents`) in the spec runner. + accepted), and command-monitoring (`expectEvents`) in the spec runner. The + eight reclamation bugs above were cleared first, as preconditions for the + free list rather than as work of their own. **A prerequisite the free list must honour**, recorded here while it is still being designed: *an offset that was ever a record start must remain a record start.* `doc_bytes` reads a `u32` length prefix in place, so an