SDSTOR-22729: index/wb_cache: Fix recovery corruption after root-split crash

[JacksonYao287]

A B-tree root split (tree height N → N+1) proceeds in three steps:

1. Allocate new_root and call on_root_changed(new_root), which updates
   the in-memory superblock (SB) and links meta_buf → new_root_buf in
   the CP flush DAG.
2. split_node(new_root, old_root) modifies old_root in memory
   (edge_info=EMPTY, next_bnode=child_node2) and calls
   transact_nodes({child_node2}, {}, old_root, new_root), which invokes
   link_buf(new_root_buf, old_root_buf).
3. The on-disk SB is written at the very end of CP flush, after all node
   buffers complete.

A SIGKILL landing after step 2 but before the SB write exposed three
latent bugs that combined to corrupt the tree.

Bug A — link_buf Condition 1 created a flat flush DAG

Condition 1 bypassed new_root_buf whenever it was newly created in the
current CP, regardless of whether old_root_buf was new or old. This
caused old_root_buf to link directly to meta_buf, making new_root_buf
and old_root_buf siblings with no ordering between them. old_root could
therefore reach disk in its transient split state (edge_info=EMPTY,
next_bnode=child_node2) before new_root, widening the crash window.

Bug B — Recovery discarded a committed new_root

When the crash left old_root on disk in split state and new_root durable
but the SB unwritten, the recovery loop could not reliably identify
new_root as the intended root. Existing logic either discarded the
committed new_root candidate or had no mechanism to promote it,
leaving the persisted SB still pointing to old_root.

Bug C — repair_root_node applied an unsafe edge repair

With old_root as the recovered tree root (per the stale SB),
repair_root_node read old_root.next_bnode (= child_node2, level N) and
set it as old_root's edge child (also level N). This violated the
B-tree invariant child.level == parent.level - 1, causing validate_node
to abort with "Child node level mismatch" on the next B-tree access.

Fix 1 — Preserve root-transition DAG topology (Bug A)

Changed link_buf Condition 1 to bypass up_buf only when BOTH up_buf AND
down_buf were created in the current CP:

Before: if (up_buf->m_created_cp_id == icp_ctx->id())
After: if (up_buf->m_created_cp_id == icp_ctx->id() &&
down_buf->m_created_cp_id == icp_ctx->id())

When down_buf is an older node (e.g. old_root), the dependency chain
through up_buf (new_root) is preserved so journal recovery rebuilds the
same root-transition topology deterministically. In the normal DAG,
old_root remains a down-buffer of new_root and still completes before
new_root; this does not by itself prevent old_root from reaching disk
before new_root. Physical durability of new_root and its new-node
dependencies before the modified old_root is written is provided by the
pre-flush barrier in Fix 2.

Because new_root waits for its downs, a committed new_root also implies
that old_root and child_node2 have completed their flush, which makes
Fix 2's root promotion safe once the candidate is durable.

The mirror fix is applied in index_cp.cpp process_txn_record so that
journal recovery rebuilds an identical DAG. The sanity check is
tightened accordingly: only a buffer that was itself created in the
current CP must not point to another same-CP-new up_buffer.

Fix 2 — Pre-flush new-root nodes and promote on recovery (Bug B)

Added a pre-flush phase in async_cp_flush: before the normal DAG flush
starts, all newly-created nodes belonging to ordinals that had a root
change are written to disk via async_write. Only after these writes
complete does the normal DAG flush begin. This ensures new_root (and
child_node2) are always durable before old_root can be written in its
transient split state.

During recovery, the journal is parsed to identify the final intended
new-root BlkId per ordinal (m_recovered_root_ids). A candidate is
promoted when:

• The journal identifies it as the final root for its ordinal
• was_node_committed() confirms it is durable on disk
• persisted_root_was_committed() confirms the old (persisted SB) root
  was also written in this CP, meaning the tree is in post-split state

This check is stronger than simply testing whether SB root is non-empty:
if old_root was not yet written in split state, the tree is still in a
pre-split consistent state and the new_root candidate is discarded.
set_root_from_committed_buf() updates the in-memory SB (root_node,
root_link_version, btree_depth) and the btree's root pointer before the
forced recovery CP writes the corrected SB to disk.

The first-CP path (SB root still empty) is excluded so that
recovery_completed() can build a fresh root as before.

Fix 3 — Harden repair_root_node (Bug C)

Added guards before applying the edge repair:

a. Buffer validity: skip if raw_buffer is null, node magic is invalid,
or node_id does not match blkid — the buffer was never written.
b. next_bnode empty: skip if next_bnode is already empty_bnodeid,
meaning new_root was already promoted and nothing needs repairing.
c. Level invariant: read the candidate next_bnode from disk; if its
level >= old_root's level (partial root-split state), skip the
repair rather than corrupting the edge.

Also changed the raw pointer n to BtreeNodePtr bn to prevent a
memory leak on the early-return paths added by this fix.

The same validity guard is applied to repair_node to protect against
same-CP new nodes that were never flushed (all-zero on disk).

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 31.57895% with 117 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (stable/v7.x@0d15abb). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
src/lib/index/wb_cache.cpp	29.33%	2 Missing and 51 partials ⚠️
src/include/homestore/index/index_table.hpp	32.35%	27 Missing and 19 partials ⚠️
src/lib/index/index_cp.cpp	37.03%	0 Missing and 17 partials ⚠️
src/lib/index/index_service.cpp	0.00%	0 Missing and 1 partial ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@              Coverage Diff               @@
##             stable/v7.x     #900   +/-   ##
==============================================
  Coverage               ?   48.24%           
==============================================
  Files                  ?      110           
  Lines                  ?    13114           
  Branches               ?     6323           
==============================================
  Hits                   ?     6327           
  Misses                 ?     2558           
  Partials               ?     4229           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[JacksonYao287]

this collects all the bufs , excluding meta_buf and freed buf, in the cp where a new root if found(root split). we need to flush these bufs for new root recovery if crash happens.

[JacksonYao287]

this part is used to identify what is the final root according to txn_journal

[JacksonYao287]

we have linked the old root to new root(new root is created in the cp , but it is now a valid up_buffer) for root split case, so change this check.

[JacksonYao287]

Fix 1(the changes here) tightens the Condition 1 guard in link_buf:

// Before: bypass up_buf whenever it was created in the current CP
if (up_buf->m_created_cp_id == icp_ctx->id())

// After: bypass only when BOTH up and down were created in the current CP
if (up_buf->m_created_cp_id == icp_ctx->id() &&
down_buf->m_created_cp_id == icp_ctx->id())

---

How the DAG Is Built for a Root Split

transact_bufs makes two link_buf calls:

// parent_buf = new_root_buf, child_buf = old_root_buf, new_node_bufs = {child_node2_buf}

link_buf(new_root_buf, old_root_buf, is_sibling=false); // ← Fix 1 affects this call
link_buf(old_root_buf, child_node2_buf, is_sibling=true); // ← Fix 1 does NOT affect this call

Second call (up=old_root_buf, down=child_node2_buf): old_root is an existing node from a previous CP, so old_root->m_created_cp_id != current. Condition 1 never fires here regardless of Fix 1. child_node2_buf->m_up_buffer is always set to old_root_buf — child_node2 is always a down-buffer of old_root.

First call is where Fix 1 matters. With up=new_root_buf (newly created) and down=old_root_buf (old):

• Without Fix 1: Condition 1 fires (new_root->m_created_cp_id == current), bypassing new_root_buf. old_root_buf->m_up_buffer becomes meta_buf.
• With Fix 1: Condition 1 requires both to be new, but old_root is old — does not fire. old_root_buf->m_up_buffer becomes new_root_buf.

---

The Two Resulting DAGs

Without Fix 1 (broken):
meta_buf (wait=2)
├── new_root_buf (new, wait=0) ← sibling of old_root, no ordering guarantee
└── old_root_buf (old, wait=1)
└── child_node2_buf (new, wait=0)

With Fix 1 (correct linear chain):
meta_buf (wait=1)
└── new_root_buf (new, wait=1)
└── old_root_buf (old, wait=1)
└── child_node2_buf (new, wait=0)

Fix 1 changes only one edge: old_root's up-buffer moves from meta_buf to new_root_buf. The child_node2 → old_root dependency is present in both cases and is unaffected by Fix 1.

---

What Goes Wrong Without Fix 1

During normal CP flush

In the broken flat DAG, new_root_buf and old_root_buf are siblings under meta_buf with no ordering between them. old_root can reach disk in its transient split state (edge_info=EMPTY, next_bnode=child_node2) before new_root is written. If a crash lands in that window:

• old_root is on disk in split state
• new_root is not on disk
• The SB still points to old_root

pre-flush compensates for this by writing new_root to disk before the DAG flush starts. But without Fix 1, the recovery DAG is also broken, which causes the second problem below.

During crash recovery (the critical failure)

Fix 1 has a mirror change in process_txn_record so that journal replay reconstructs the identical DAG. Without the mirror:

Reconstructed DAG in recovery (without Fix 1's mirror):
meta_buf (wait=2)
├── new_root_buf (new, up=meta_buf) ← correctly identified as root candidate
└── old_root_buf (old, up=meta_buf) ← Condition 1 fired, bypassed new_root_buf
└── child_node2_buf (new, up=old_root_buf)

The recovery main loop processes child_node2_buf (new, up=old_root_buf, not meta):

// child_node2 path: up_buffer is old_root (not meta), both committed
→ commit_recovered_blk(child_node2)
→ pending_bufs.push_back(old_root_buf)

And new_root_buf (new, up=meta_buf) goes through new_root_candidates → set_root_from_committed_buf(new_root) correctly promotes the root.

But then recover_buf(old_root_buf) is called from pending_bufs:

was_node_committed(old_root_buf) → true (on disk in split state)
buf->m_up_buffer->is_meta_buf() → true (old_root links to meta_buf in the broken DAG)
→ update_root(ordinal, old_root_buf) // ← called AFTER root was already promoted to new_root!

This call to update_root(old_root) is at best a harmless no-op and at worst silently reverts the correctly promoted root back to old_root, undoing the
entire recovery.

With Fix 1's mirror, the reconstructed recovery DAG is the correct linear chain:
meta_buf (wait=1)
└── new_root_buf (new, up=meta_buf, wait=1)
└── old_root_buf (old, up=new_root_buf, wait=1) ← links to new_root, not meta_buf
└── child_node2_buf (new, up=old_root_buf, wait=0)

The recovery loop still pushes old_root_buf into pending_bufs via the child_node2 path. But now recover_buf(old_root_buf) sees old_root->m_up_buffer =
new_root_buf (not meta_buf), so it does not call update_root(old_root). Instead it calls recover_buf(new_root_buf), which finds new_root->m_up_buffer
= meta_buf and calls update_root(new_root) — idempotent, since the root is already new_root.

[JacksonYao287]

change the meta_buf , pointing to the identified root buf

[JacksonYao287]

the newly added code is mostly comments , which helps you to better understand what the code does. no big logic changes, pls feel free to keep reviewing.

cc @shosseinimotlagh @nnastonen

[shosseinimotlagh]

The three-fix approach (DAG condition narrowing, pre-flush barrier + journal-based root promotion, repair_root_node hardening) is architecturally sound, each fix targets a distinct causal layer, and the 6 new crash tests cover all relevant crash windows including double-crash idempotency.

[shosseinimotlagh]

External Code Review

The bug analysis is correct and the direction of all three fixes is right. But there are real correctness concerns and one likely regression that should be addressed before merge.

---

CRITICAL

1. Silent behavioral regression: prune_up_buffers removed without justification

wb_cache.cpp, non-root-split new-node discard path:

// OLD (when parent is uncommitted):
buf->m_up_buffer->remove_down_buffer(buf);
prune_up_buffers(buf, pruned_bufs_to_repair);  // traverses up, adds to repair list
bufs_to_skip_sanity_check.insert(buf);

// NEW:
prune_from_up_buffer(buf);                     // only disconnects immediate parent
bufs_to_skip_sanity_check.insert(buf);

prune_up_buffers traversed up the buffer chain and populated pruned_bufs_to_repair. That list drives the repair path for parent nodes whose child pointers became stale when a new child was discarded. The new code only calls prune_from_up_buffer, which disconnects buf from its immediate parent but never adds the uncommitted parent to the repair list. If that parent is an old-CP node that was modified in the crashed CP (insert dirtied an interior node, crash happened before both the new leaf and its parent flushed), the parent's stale child pointer is never repaired. The PR description does not mention this change at all.

---

2. root_change_preflush_bufs() over-collects — wrong scope for the pre-flush barrier

index_cp.cpp:

m_dirty_buf_list.foreach_entry([...](IndexBufferPtr const& buf) {
    if (buf->is_meta_buf() || buf->m_node_freed || buf->m_created_cp_id != id() ||
        !m_root_changed_ordinals.contains(buf->m_index_ordinal)) {
        return;
    }
    if (selected_blkids.insert(buf->blkid()).second) { bufs.push_back(buf); }
});

This collects all new-CP nodes for any ordinal that had a root change, not just new_root_buf and child_node2. If a heavily-inserted tree also had a root split in the same CP, every new interior node created by those inserts gets pre-flushed. The safety argument only requires that new_root_buf (and child_node2) are durable before old_root can be written in split state — regular new nodes are already correctly ordered relative to their own ancestors in the DAG. The over-collection inflates I/O proportionally to tree mutations, not to the number of root splits.

---

3. commit_blk idempotency is assumed but not verified

wb_cache.cpp, commit_recovered_blk lambda:

HS_REL_ASSERT(status == BlkAllocStatus::SUCCESS, "Failed to commit recovered index block {}",
              buf->m_blkid.to_string());

In Case B (SB was already written before the crash), commit_recovered_blk is called for new_root_buf. After a crash that occurs post-SB-write, the block allocator may have persisted the allocation to disk, meaning the block is already marked allocated when the recovery call happens. If commit_blk returns anything other than SUCCESS for an already-allocated block, this assert terminates the process on a legitimate recovery path. The PR should either document that commit_blk is idempotent with a SUCCESS return for already-committed blocks, or guard against ALREADY_EXISTS status.

---

HIGH

4. Fragile null-pointer dependency on short-circuit evaluation in repair_root_node

index_table.hpp:

if (ret != btree_status_t::success || edge_node->level() >= root->level()) {

When ret != success, edge_node may be null — the LOGERROR a few lines later acknowledges this with edge_node ? static_cast<int>(edge_node->level()) : -1. The condition is only safe because || short-circuits. Any future maintainer who reorders conditions or refactors will introduce a segfault. Split into two explicit checks.

---

5. set_root_from_committed_buf defensive false returns are fatal in the caller

index_table.hpp + wb_cache.cpp:

// caller:
HS_REL_ASSERT(table && table->set_root_from_committed_buf(candidate), ...);

The function has multiple guards (null raw_buf, invalid node magic, node_id != blkid, validate_node failure) that return false with a LOGERROR. But the caller treats any false as a process-aborting fatal assert. This creates a contract conflict: if these guards are truly defensive, returning false is wrong and they should assert internally. If they represent legitimate recovery failures, the caller should not crash. As written, a validate_node failure on a partially-written node converts a potentially recoverable situation into a process abort.

---

6. Journal root-record parsing has undocumented layout coupling

index_cp.cpp:

bool const is_root_split = !rec->has_inplace_child && rec->num_new_ids == 1;
bool const is_root_collapse = rec->has_inplace_child && rec->num_new_ids == 0;
auto const root_idx = rec->has_inplace_parent ? 1 : 0;
m_recovered_root_ids[rec->index_ordinal] = rec->blk_id(root_idx);

root_idx encodes the assumption that blk_ids are laid out as [inplace_child?, inplace_parent?, new_ids...]. For a root split with is_parent_meta=true, has_inplace_parent is presumably false, so root_idx=0 lands on the first new_id. There is no cross-reference between this parsing and the add_to_txn_journal call site, no assertion that the blkid at root_idx is the new root, and no unit test verifying this mapping. A silent mismatch here would store the wrong blkid in m_recovered_root_ids and promote the wrong node as root.

---

MEDIUM

7. No test for multiple root splits in the same CP

The PR description explicitly calls out the multi-root-split scenario: Condition 1 flattening, intermediate roots committed but not promoted, m_recovered_root_ids retaining only the last journal entry. This is the most novel aspect of the fix and has no dedicated TYPED_TEST. A test that forces two root splits within one CP, crashes at the meta_buf write, and verifies only the final root is promoted is needed.

8. was_node_committed called redundantly in recovery

In the root-split branch, was_node_committed(buf) is called for buf_was_committed, then persisted_root_was_committed(ordinal) calls it again on the persisted root's buf — which in Case B is the same buf. Each call may invoke load_buf. There is no explicit memoization. At minimum a comment confirming load_buf is idempotent (does not re-read if already cached) is needed.

---

LOW / STYLE

9. Duplicate error messages for distinct failure conditions

Both repair_root_node and set_root_from_committed_buf use identical LOGERROR text for two structurally different failures (is_valid_node failure vs. node_id != blkid mismatch). These should produce distinct log messages for debuggability.

10. CrashAfterOldRootFlushOnFirstRootSplit test comment is inaccurate

The comment says "crash after old_root is flushed but before new_root_buf is written." Fix 2's pre-flush has already written new_root_buf before the DAG starts, so new_root_buf IS on disk when this crash fires. The comment should say "before the DAG's second write of new_root_buf" and note that new_root_buf is already durable from the pre-flush — otherwise the scenario as described appears to contradict the fix it's testing.

11. m_sb->root_node == empty_bnodeid guard in set_root_from_committed_buf is unreachable dead code

persisted_root_was_committed() already returns false when persisted_root_node_id() == empty_bnodeid, so no candidate with an empty SB root can enter new_root_candidates. The guard at line 127 is dead. If somehow reached, the caller's HS_REL_ASSERT would crash the process. Dead defensive code that would crash the process is more harmful than no guard at all.

12. Pre-flush crash simulator calls cp_ctx->complete(true) after crashing

After hs()->crash_simulator().crash(), the code calls cp_ctx->complete(true), signaling successful CP completion immediately after a simulated crash. Needs a comment confirming the test infrastructure handles this sequencing correctly and this does not race with crash unwinding.

---

Summary

Severity	Issues
Critical	3 (regression, pre-flush over-collection, commit_blk idempotency)
High	3 (null-ptr short-circuit, fatal/defensive contract, journal layout coupling)
Medium	2 (missing multi-split test, redundant was_node_committed)
Low	4 (dup error messages, inaccurate test comment, dead guard, crash simulator sequencing)

Must fix before merge: issues 1, 2, 3, 5, 6. Issues 4 and 7 should be addressed in this PR rather than deferred.