From 398649a25c8e27c47796272819fb517252e07b51 Mon Sep 17 00:00:00 2001 From: Jepson Taylor <130782313+jepsontaylor@users.noreply.github.com> Date: Fri, 25 Sep 2026 08:01:09 +0000 Subject: [PATCH] Install a leaf page only after its WAL record is appended. A non-splitting insert stages the new page off the live frame, appends IndexInsert, then swaps that page in and sets its LSN while holding the frame lock. A crash at index::insert therefore leaves the leaf without the key, and a flush cannot persist the new bytes under the previous LSN. Splits and durability mode are unchanged. Claim: AGENT_CHAT.md 2026-09-25T01:20Z fix/index-wal-before-page. Acceptance: cargo test -p redlinedb-bench --offline --test index_insert_publish; cargo test -p redlinedb-kernel --offline --lib direct_insert_matches direct_unique_conflict; cargo test -p redlinedb-kernel --offline --test index_tests --- AGENT_CHAT.md | 4 ++ crates/bench/tests/index_insert_publish.rs | 45 ++++++++++++++++++++++ crates/kernel/src/index/mutate/insert.rs | 25 ++++++------ crates/kernel/src/storage/buffer.rs | 16 ++++++++ 4 files changed, 78 insertions(+), 12 deletions(-) create mode 100644 crates/bench/tests/index_insert_publish.rs diff --git a/AGENT_CHAT.md b/AGENT_CHAT.md index 8c1038df8..8e620d8ff 100644 --- a/AGENT_CHAT.md +++ b/AGENT_CHAT.md @@ -1,5 +1,9 @@ # RedlineDB Agent Chat +## 2026-09-25T01:20Z fix/index-wal-before-page + +Claim: canonical checkout `/home/ubuntu/redlineDB`, branch `fix/index-wal-before-page` from `origin/main` `28196cdf31eebc1f466920aa0f2b6200d85e8720`. A non-splitting leaf insert stages the new page privately, appends the WAL record, then installs that page and its LSN under one frame lock. A crash at `index::insert` leaves the leaf unchanged. No worktree. + ## 2026-09-25T00:40Z perf/leaf-direct-default Claim: canonical checkout `/home/ubuntu/redlineDB`, branch `perf/leaf-direct-default` from `origin/main` `75ea89c204acd8cd988138815bf88a609e19b7b2`. Non-splitting leaf insert places one cell and checksums once. `REDLINEDB_LEAF_DIRECT_INSERT=0` keeps the full-page rebuild. Durability stays Strict. No worktree. diff --git a/crates/bench/tests/index_insert_publish.rs b/crates/bench/tests/index_insert_publish.rs new file mode 100644 index 000000000..f9bfda839 --- /dev/null +++ b/crates/bench/tests/index_insert_publish.rs @@ -0,0 +1,45 @@ +//! A crash at `index::insert` must not leave the new key on the leaf. +//! +//! The insert stages the page privately, hits this failpoint, and only +//! then appends the WAL record and installs the page. The bench crate +//! builds the kernel with failpoints, so this runs in `tests (bench)`. + +use std::panic::{AssertUnwindSafe, catch_unwind}; +use std::sync::Arc; + +use redlinedb_kernel::failpoints; +use redlinedb_kernel::format::{PageGeneration, PageId, TuplePtr, TxId}; +use redlinedb_kernel::index::{BtreeIndex, IndexDescriptor, IndexId, IndexRowRef, IndexUniqueness}; +use redlinedb_kernel::storage::{BufferPool, PageFile}; + +#[test] +fn crash_before_wal_leaves_the_leaf_without_the_key() { + let dir = tempfile::tempdir().expect("temp dir"); + let page_file = + Arc::new(PageFile::create(dir.path().join("data.redline"), 4096).expect("page file")); + let buffer = Arc::new(BufferPool::new(Arc::clone(&page_file), 64).expect("buffer")); + let index = BtreeIndex::create( + buffer, + IndexDescriptor::new( + IndexId(9), + redlinedb_kernel::format::RelId(1), + IndexUniqueness::NonUnique, + ), + ) + .expect("index"); + let row = IndexRowRef::new(TuplePtr::new_with_generation( + PageId(3), + 1, + PageGeneration::ONE, + )); + failpoints::cfg("index::insert", "panic").expect("arm failpoint"); + let panicked = catch_unwind(AssertUnwindSafe(|| { + index.insert_tx(TxId(1), b"staged", row).expect("insert"); + })); + failpoints::cfg("index::insert", "off").expect("disarm failpoint"); + assert!(panicked.is_err(), "index::insert must panic before publish"); + assert!( + index.point_lookup(b"staged").expect("lookup").is_empty(), + "the live leaf must not contain a key whose WAL record was not appended" + ); +} diff --git a/crates/kernel/src/index/mutate/insert.rs b/crates/kernel/src/index/mutate/insert.rs index d65084908..3098115c7 100644 --- a/crates/kernel/src/index/mutate/insert.rs +++ b/crates/kernel/src/index/mutate/insert.rs @@ -114,11 +114,14 @@ impl BtreeIndex { )?; return Ok(()); } + // Stage off the live frame. A flush during the WAL append still + // sees the previous leaf bytes and the previous page LSN. + let mut staged = page_ref.clone(); if super::insert_leaf::direct_leaf_insert_enabled() { - super::insert_leaf::insert_one_cell(page_ref, slot, &entries[slot])?; + super::insert_leaf::insert_one_cell(&mut staged, slot, &entries[slot])?; } else { Self::rewrite_leaf( - page_ref, + &mut staged, self.descriptor().index_id, &entries, header.left, @@ -127,13 +130,11 @@ impl BtreeIndex { )?; } drop(page); - if emit_wal { - // Lane E failpoint: armed before the leaf-write becomes - // visible (mark_dirty + WAL append). A crash here proves the - // index entry is either fully reflected post-recovery or - // absent. + let publish_lsn = if emit_wal { + // Armed before the staged leaf is installed. A crash here + // leaves the on-buffer leaf unchanged, so the key is absent. crate::fail_point!("index::insert"); - let end_lsn = self.append_index_delta( + self.append_index_delta( tx_id, WalPayload::IndexInsert { tx_id, @@ -141,11 +142,11 @@ impl BtreeIndex { logical_key: logical_key.to_vec(), row, }, - )?; - guard.mark_dirty(end_lsn)?; + )? } else { - guard.mark_dirty(lsn)?; - } + lsn + }; + guard.install_dirty(staged, publish_lsn)?; drop(leaf_write); return Ok(()); } diff --git a/crates/kernel/src/storage/buffer.rs b/crates/kernel/src/storage/buffer.rs index f11b3ece6..07797432b 100644 --- a/crates/kernel/src/storage/buffer.rs +++ b/crates/kernel/src/storage/buffer.rs @@ -850,6 +850,22 @@ impl PageGuard { Ok(()) } + /// Replace the resident page and mark it dirty while holding the frame + /// lock. Callers stage bytes privately, append the WAL record, then + /// publish here so a flush cannot observe the new bytes under the + /// previous page LSN. + pub(crate) fn install_dirty(&self, new_page: Page, lsn: Lsn) -> Result<()> { + let mut frame = self.mutable_frame()?; + let page = frame + .page + .as_mut() + .ok_or(Error::CorruptPage("resident frame missing page"))?; + *page = new_page; + page.set_page_lsn(lsn)?; + frame.dirty = true; + Ok(()) + } + fn frame(&self) -> Result> { self.frame .state