shoney.arickathil
7ad52937b2
- wo_row_remove read the id map's value as a slot, but on a keys table that value is a LOG OFFSET (hput(t, id, wal_off + 1)). slot_row does no bounds check, so a delete indexed t->slabs[] with a byte offset and then called db_val_free on whatever it landed on — arbitrary frees, not a wrong answer - keys tables now take their own arm: no slab slot, no bitmap bit, no free-list entry to return. The index hook needs the row's values, so the row is borrowed from the log for exactly that long - wo_row_ptr carried the same trap and is public. It cannot refuse keys tables outright (insert legitimately calls it while the map still holds a slot), so it now detects the offset case — index past the slabs, or bitmap bit clear — and returns NULL. Callers all handle NULL - test_keys_resident_delete pins it; it SEGVs against the old code, verified by reverting the fix rather than assumed - found while auditing every hget() reader before narrowing the loader refusal to allow benchmarking. The refusal was justified in the docs by "updates are unimplemented" while actually standing in front of this too: a guard whose stated reason is narrower than its real one gets removed by someone who believes the stated reason Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 76b8fd944af9ed062467bdf9ab93c2e96dd198cf)
6 KiB
databasev2 — chain and dependency review
Reviewed 2026-08-29 against story frontmatter, the track index's sequence
table, and the code as it stands on dev. Six findings, ordered by how much
damage each could do if acted on.
The recorded picture
chain is a cross-track field (positions 1–6, defined in
docs/stories/board-views.md), not a databasev2 one. Only two databasev2
iterations carry it:
| chain | Story | status |
|---|---|---|
| 1 | language 08 shard-actor runtime, 11 fibers | done |
| 2 | language 22 durability/throughput/scale | done |
| 3 | language 31 actor lifecycle, 40 shutdown drain | done |
| 4 | language 24 chat/websocket workload | done |
| 5 | databasev2 4 io_uring group commit | in-progress |
| 6 | databasev2 3 WAL checkpoint | done |
The track's own sequence lives in 00-story.md as a Needs column plus an
ASCII graph. The two disagree with each other, with the chain field, and with
what happened.
Finding 1 — the graph contradicts the chain field and the history
00-story.md draws 3 ──▶ 4: iteration 3 before iteration 4. The chain field
says the opposite — iteration 4 is chain 5, iteration 3 is chain 6, so 4 comes
first. History settles it: 4's part A landed 2026-08-28, 3 landed
2026-08-29. The chain field and the history agree; the graph is wrong.
Worth fixing rather than shrugging at, because the graph is the artefact someone reads when choosing what to start.
Finding 2 — the graph contradicts its own prose about direction
The order rationale states "3 and 4 matter to 2" — that is, 2 depends on 3
and 4. The graph draws an edge from 2 to 3, which reads as the reverse.
One of the two is backwards, and the prose is the one that matches the code:
resident: keys needed the checkpoint, not the other way round.
Finding 3 — a retired path is still drawn
The graph still shows 2 ──▶ 5 ──▶ 6. The 2026-08-27 amendment directly below
it says iteration 6 is largely superseded by 2, and that 5 is no longer a
prerequisite for anything on the critical path. The prose retired the path;
the picture kept it.
Finding 4 — the chain metadata omits the iteration doing the work
Iteration 2 is on the critical path, is in-progress, and is where 5c/5d just
landed — and it carries no chain field. The board-views query "the
concurrency chain, in execution order" filters WHERE chain, so iteration 2 is
invisible to it. Either 2 belongs on the chain and should say so, or the chain
is genuinely a concurrency artefact that databasev2 2 sits outside — in which
case 3 and 4 carrying it while 2 does not deserves a one-line explanation.
Finding 5 — iteration 3's hazard section is stale, and was incomplete
This is the one with teeth.
03-wal-checkpoint.md carries a "Hazard: compaction invalidates every
resident: keys offset" section and a matching Outstanding entry. Both are now
stale: the Outstanding entry says "Nothing fails today because
iteration 2's storage half is unimplemented", which stopped being true when
5c/5d landed (125bd09, 08abd09, 0c97fa4, f606fc9). The hazard also
offers two shapes and says "the first is almost certainly right" — the first
was implemented, and the section should now record that as settled rather
than as an open fork.
More importantly, the recorded hazard named only half the danger. It described stored offsets becoming wrong: a pointer into a rewritten file. Implementation found a second, worse failure it did not anticipate — compaction walked the slab bitmap, and a keys-resident row has no bitmap bit, because its slot is returned to the free list when the payload is dropped. Every such row would therefore have been omitted from the new log entirely. That is silent data loss, not a bad pointer, and no amount of offset-rebuilding would have caught it.
Both failure modes are now pinned by test_keys_resident_survives_compaction,
which rewrites rows in hash order so offsets genuinely move and a missing
re-point cannot pass by luck.
Finding 6 — the coupling is now bidirectional, and undocumented in that direction
The docs record 2 depending on 3. After 5d, 3's own deliverable depends on
2's API: wo_wal_compact in database/src/wal.c now calls
wo_row_next_id, wo_row_offset1, wo_row_set_offset and
wo_table_is_keys_resident — all iteration 2 surface. Compaction can no longer
be described as a pure file operation, which is exactly what the hazard section
predicted and no dependency table records.
Minor, same family: iteration 6 is "largely superseded" and to be revisited
"only with a measurement showing the page cache insufficient" — a hold
condition — yet its status is pending while genuinely parked iterations 8, 9
and 10 are hold.
Addendum 2026-08-29 — the recommendation this review implied was wrong
This review argued the next step was to measure resident: keys before
investing further, and that measuring required narrowing the loader refusal so
a benchmark could declare such a table. Auditing the code before narrowing it
found that delete on a keys-resident table was memory corruption, not a
missing feature: wo_row_remove read the id map's value as a slot when on such
a table it is a log offset, and slot_row bounds-checks nothing.
The refusal was therefore load-bearing in a way nobody had written down. It was justified in the docs by "updates are unimplemented" — one honest gap — while actually standing in front of two, one of which frees arbitrary pointers.
Both are now closed or contained (wo_row_remove fixed, wo_row_ptr returns
NULL rather than a wild pointer), but the lesson generalises: a guard whose
stated reason is narrower than its real one will eventually be removed by
someone who believes the stated reason.
What is actually blocked
Nothing in databasev2 is blocked on anything else in databasev2. Iteration 2's remaining tasks 6 and 7 depend only on iteration 2. Iteration 4's part B is not blocked either — it is unstarted with an invalidated premise, which is a re-brainstorm, not a dependency.