Skip to content

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

Open
JacksonYao287 wants to merge 1 commit into
eBay:stable/v7.xfrom
JacksonYao287:SDSTOR-22729
Open

SDSTOR-22729: index/wb_cache: Fix recovery corruption after root-split crash#900
JacksonYao287 wants to merge 1 commit into
eBay:stable/v7.xfrom
JacksonYao287:SDSTOR-22729

Conversation

@JacksonYao287

@JacksonYao287 JacksonYao287 commented Jul 23, 2026

Copy link
Copy Markdown
Member

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

codecov-commenter commented Jul 23, 2026

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

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

Files with missing lines Patch % Lines
src/lib/index/wb_cache.cpp 32.00% 2 Missing and 49 partials ⚠️
src/include/homestore/index/index_table.hpp 32.35% 27 Missing and 19 partials ⚠️
src/lib/index/index_cp.cpp 35.71% 2 Missing and 16 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.26%           
==============================================
  Files                  ?      110           
  Lines                  ?    13118           
  Branches               ?     6324           
==============================================
  Hits                   ?     6332           
  Misses                 ?     2560           
  Partials               ?     4226           

☔ 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
JacksonYao287 force-pushed the SDSTOR-22729 branch 4 times, most recently from 96f4aad to dd527a7 Compare July 27, 2026 06:54
…t crash

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 — Restore correct flush-DAG ordering (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. The key guarantee this
establishes is: if new_root is committed (on disk), then old_root and
child_node2 are also committed, making Fix 2's root promotion safe.

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).

**Additional hardening**

- skip load_buf for meta_buf in recover(): MetaIndexBuffer has
  blkid={0,0,0,0}; calling load_buf on it reads garbage from vdev and
  corrupts m_dirtied_cp_id.
- recover_buf() returns early for meta_buf: meta durability is tracked
  via the new-root candidate path, not via was_node_committed.
- read_buf() validates node magic and node_id before init_node to catch
  corrupt or unwritten blocks early.
- to_string() replaces unescaped inner braces {{{}}} with [{}] to avoid
  fmt::format_error when HS_*_ASSERT_CMP re-parses the message string.

Added two regression tests to IndexCrashTest:

CrashAtMetaBufOnSecondRootSplit: Establishes a durable level-1 tree,
then sets crash_flush_on_meta and inserts enough keys to force a second
root split (level-1 → level-2). The crash fires after new_root and
old_root are both durable but before meta_buf writes. Fix 2 detects
new_root and promotes it; Fix 3 would catch the level invariant
violation if Fix 2 had not applied. Tree integrity is verified via
reapply_after_crash and get_all.

CrashAfterOldRootFlushOnSecondRootSplit: Same setup, but uses
crash_flush_on_root to crash specifically when old_root is flushed
(after the pre-flush has already made new_root durable). Also sets
skip_cp_after_index_root_recovery to keep the original journal current,
then performs a second restart to verify that replaying an
already-promoted candidate is idempotent.
auto record_size = txn_record::size_for_num_ids(created_bufs.size() + freed_bufs.size() + (left_child_buf ? 1 : 0) +
(parent_buf ? 1 : 0));
std::unique_lock< iomgr::FiberManagerLib::mutex > lg{m_txn_journal_mtx};
if (parent_buf && parent_buf->is_meta_buf() && !left_child_buf && !created_bufs.empty()) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this indicates that a new root is found, which also means root split

}
}

IndexBufferPtrList IndexCPContext::root_change_preflush_bufs() {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// Root-change records contain no split/merge side effects. Retaining the last record per ordinal identifies
// the final intended root even if the same CP grows and then collapses the tree.
if (rec->is_parent_meta && rec->num_freed_ids == 0) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

HS_REL_ASSERT(up_buffer->m_created_cp_id == -1,
"Sanity check failed: Buffer {} has an up_buffer {} that just created (created_cp_id={})",
bufferPtr->to_string(), up_buffer->to_string(), up_buffer->m_created_cp_id);
if (bufferPtr->m_created_cp_id == cp_id) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (up_buf->m_created_cp_id == icp_ctx->id()) {
// Condition 1: Flatten only new-to-new links. An existing node modified by a root split must retain the new root
// as its up-buffer so recovery reconstructs the same root transition deterministically.
if (up_buf->m_created_cp_id == icp_ctx->id() && down_buf->m_created_cp_id == icp_ctx->id()) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is one of the important change. we only keep flaten dag for newly created btree node. for the existing btree node, we keep the dependency

}
}

for (auto const& [ordinal, candidate] : new_root_candidates) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change the meta_buf , pointing to the identified root buf

preflush_futures.reserve(preflush_bufs.size());
for (auto const& buf : preflush_bufs) {
LOGTRACEMOD(wbcache, "Preflushing root-transition node {}", buf->to_string());
preflush_futures.emplace_back(

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is another import change. if root split happens, we first flush all the preflush buffer, and then flush all the buffers in the cp.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants