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

conanfile.py

@@ -9,7 +9,7 @@
 
 class HomestoreConan(ConanFile):
     name = "homestore"
-    version = "7.5.15"
+    version = "7.5.16"
 
     homepage = "https://github.com/eBay/Homestore"
     description = "HomeStore Storage Engine"

src/include/homestore/btree/detail/btree_common.ipp

@@ -292,7 +292,10 @@ void Btree<K, V>::validate_node_child_relation(BtreeNodePtr node, BtreeNodePtr&
 
         if(ind > 0){
             if (previous_child->next_bnode()!= child_node->node_id())     {
-                throw std::runtime_error(fmt::format("Broken child linkage: {}-th Child node [{}] node id is not equal to previous child node [{}] next node",ind, child_node->to_string(), child_node->node_id(), previous_child->to_string()));
+                throw std::runtime_error(fmt::format(
+                    "Broken child linkage: {}-th Child node [{}] node id {} is not equal to previous child node [{}] next node {}",
+                    ind, child_node->to_string(), child_node->node_id(), previous_child->to_string(),
+                    previous_child->next_bnode()));
             }
             K last_parent_key = node->get_nth_key< K >(ind-1, false /* copy */);
             K previous_child_last_key = previous_child->get_last_key< K >();
@@ -370,7 +373,8 @@ template <typename K, typename V>
 void Btree<K, V>::validate_node(const bnodeid_t& bnodeid) const {
     BtreeNodePtr node;
     if (auto ret = read_node_impl(bnodeid, node); ret != btree_status_t::success) {
-        throw std::runtime_error(fmt::format("node read failed for bnodeid: {} reason: {}", bnodeid, ret));
+        throw std::runtime_error(
+            fmt::format("node read failed for bnodeid: {} reason: {}", bnodeid, enum_name(ret)));
     } else {
         try {
             if (node->is_node_deleted()) { return; }
@@ -402,7 +406,9 @@ void Btree<K, V>::validate_node(const bnodeid_t& bnodeid) const {
             if (neighbor_id != empty_bnodeid) {
                 BtreeNodePtr neighbor_node;
                 if (auto ret = read_node_impl(neighbor_id, neighbor_node); ret != btree_status_t::success) {
-					throw std::runtime_error(fmt::format("reading neighbor node of [{}] failed for bnodeid: {} reason : {}", node->to_string(), neighbor_id, ret));
+					throw std::runtime_error(fmt::format(
+                        "reading neighbor node of [{}] failed for bnodeid: {} reason : {}", node->to_string(),
+                        neighbor_id, enum_name(ret)));
                 }
                 validate_next_node_relation(node, neighbor_node, last_child_node);
             }

src/include/homestore/index/index_internal.hpp

@@ -94,6 +94,10 @@ class IndexTableBase {
     virtual void stop() = 0;
     virtual void repair_node(IndexBufferPtr const& buf) = 0;
     virtual void repair_root_node(IndexBufferPtr const& buf) = 0;
+    // Returns true if SB root was updated to the committed buffer.
+    virtual bool set_root_from_committed_buf(IndexBufferPtr const& buf) = 0;
+    // True when on-disk SB already has a non-empty root (not first-CP create crash).
+    virtual bool has_persisted_root() const = 0;
     virtual void delete_stale_children(IndexBufferPtr const& buf) = 0;
     virtual void audit_tree() const = 0;
     virtual void update_sb() = 0;

src/include/homestore/index/index_table.hpp

@@ -211,36 +211,128 @@ class IndexTable : public IndexTableBase, public Btree< K, V > {
         if (m_sb->root_node == idx_buf->blkid().to_integer()) {
             // This is the root node, we need to update the root node in superblk
             LOGTRACEMOD(wbcache, "{} is old root so we need to update the meta node ", idx_buf->to_string());
+            if (idx_buf->raw_buffer() == nullptr ||
+                !BtreeNode::is_valid_node(sisl::blob{idx_buf->raw_buffer(), this->m_bt_cfg.node_size()}) ||
+                r_cast< persistent_hdr_t const* >(idx_buf->raw_buffer())->node_id != idx_buf->blkid().to_integer()) {
+                LOGERROR("repair_root_node: skip invalid/unwritten buf {}", idx_buf->to_string());
+                return;
+            }
+
+            // Already a proper root (e.g. new_root just promoted by set_root_from_committed_buf):
+            // valid edge, empty next. recover_buf may still call update_root because this buf sits
+            // under meta after Condition-1 linking — nothing to convert. Check header before
+            // allocating a BtreeNode to avoid an unnecessary (and previously leaked) object.
+            auto const* phdr = r_cast< persistent_hdr_t const* >(idx_buf->raw_buffer());
+            if (phdr->next_node == empty_bnodeid) {
+                LOGTRACEMOD(wbcache, "repair_root_node: buf={} already has empty next_bnode; nothing to repair",
+                            idx_buf->to_string());
+                return;
+            }
+
             BtreeNode* n = this->init_node(idx_buf->raw_buffer(), idx_buf->blkid().to_integer(), false /* init_buf */,
                                            BtreeNode::identify_leaf_node(idx_buf->raw_buffer()));
             static_cast< IndexBtreeNode* >(n)->attach_buf(idx_buf);
-            auto edge_id = n->next_bnode();
+            // Own via intrusive_ptr so every early-return path releases the node (and its
+            // attached IndexBuffer) — otherwise LSan reports the node plus the recovery DAG.
+            BtreeNodePtr bn{n};
+            auto edge_id = bn->next_bnode();
+
+            // In a partial root-split crash old_root's next_bnode points to child_node2 which is
+            // at the same level as old_root (not one level below as a valid edge child must be). Applying
+            // the edge repair would violate the level invariant. next_bnode may also be unwritten
+            // (flat flush DAG); read_node_impl fails safely in that case — skip repair rather than
+            // corrupting the node.
+            {
+                BtreeNodePtr candidate;
+                auto const ret = read_node_impl(edge_id, candidate);
+                if (ret != btree_status_t::success) {
+                    LOGERROR("repair_root_node: SKIP buf={} next_bnode={} unreadable (ret={}); "
+                             "partial root-split / unflushed sibling, not repairing",
+                             idx_buf->to_string(), edge_id, enum_name(ret));
+                    return;
+                }
+                if (candidate->level() >= bn->level()) {
+                    LOGERROR("repair_root_node: SKIP buf={} next_bnode={} level={} >= parent level={}; "
+                             "partial root-split state, not repairing",
+                             idx_buf->to_string(), edge_id, candidate->level(), bn->level());
+                    return;
+                }
+            }
 
-            if (n->has_valid_edge() && hs()->has_fc_service()) {
+            if (bn->has_valid_edge() && hs()->has_fc_service()) {
                 auto const reason =
                     fmt::format("root {} already has a valid edge {}, so we should have found the new root node",
-                                n->to_string(), n->get_edge_value().bnode_id());
+                                bn->to_string(), bn->get_edge_value().bnode_id());
                 hs()->fc_service().trigger_fc(FaultContainmentEvent::ENTER, static_cast< void* >(&(m_sb->parent_uuid)),
                                               reason);
                 return;
             } else {
-                BT_REL_ASSERT(!n->has_valid_edge(),
+                BT_REL_ASSERT(!bn->has_valid_edge(),
                               "root {} already has a valid edge {}, so we should have found the new root node",
-                              n->to_string(), n->get_edge_value().bnode_id());
+                              bn->to_string(), bn->get_edge_value().bnode_id());
             }
-            n->set_next_bnode(empty_bnodeid);
-            n->set_edge_value(BtreeLinkInfo{edge_id, 0});
-            LOGTRACEMOD(wbcache, "change root node {}: edge updated to {} and invalidate the next node! ", n->node_id(),
-                        edge_id);
+            bn->set_next_bnode(empty_bnodeid);
+            bn->set_edge_value(BtreeLinkInfo{edge_id, 0});
+            LOGTRACEMOD(wbcache, "change root node {}: edge updated to {} and invalidate the next node! ",
+                        bn->node_id(), edge_id);
             auto cpg = cp_mgr().cp_guard();
-            write_node_impl(n, (void*)cpg.context(cp_consumer_t::INDEX_SVC));
+            write_node_impl(bn, (void*)cpg.context(cp_consumer_t::INDEX_SVC));
 
         } else {
             LOGTRACEMOD(wbcache, "This is not the root node, so we can ignore this repair call for buf {}",
                         idx_buf->to_string());
         }
     }
 
+    bool has_persisted_root() const override { return m_sb->root_node != empty_bnodeid; }
+
+    // companion: called during recovery when new_root was committed but the SB write crashed.
+    // Updates in-memory SB and btree root info so the next forced CP persists the correct root.
+    // Returns true only when the SB root was actually updated.
+    bool set_root_from_committed_buf(IndexBufferPtr const& idx_buf) override {
+        if (idx_buf->raw_buffer() == nullptr ||
+            !BtreeNode::is_valid_node(sisl::blob{idx_buf->raw_buffer(), this->m_bt_cfg.node_size()}) ||
+            r_cast< persistent_hdr_t const* >(idx_buf->raw_buffer())->node_id != idx_buf->blkid().to_integer()) {
+            LOGERROR("set_root_from_committed_buf: skip invalid/unwritten buf {}", idx_buf->to_string());
+            return false;
+        }
+
+        auto const cand_level = r_cast< persistent_hdr_t const* >(idx_buf->raw_buffer())->level;
+
+        // First-CP crash before any SB root was persisted: keep root empty so
+        // recovery_completed() creates a fresh root (CrashBeforeFirstCp).
+        if (m_sb->root_node == empty_bnodeid) {
+            LOGINFOMOD(wbcache,
+                       "set_root_from_committed_buf: SB root empty, skip promote of {} level={} (first-CP path)",
+                       idx_buf->blkid().to_integer(), cand_level);
+            return false;
+        }
+
+        // Only adopt a height-increasing new root (level > persisted depth). Same-or-lower
+        // level nodes under meta are split siblings / old roots from Condition-1 flattening,
+        // not the new root.
+        if (cand_level <= m_sb->btree_depth) {
+            LOGINFOMOD(wbcache, "set_root_from_committed_buf: skip {} level={} <= sb depth={}",
+                       idx_buf->blkid().to_integer(), cand_level, m_sb->btree_depth);
+            return false;
+        }
+
+        BtreeNode* n = this->init_node(idx_buf->raw_buffer(), idx_buf->blkid().to_integer(), false /* init_buf */,
+                                       BtreeNode::identify_leaf_node(idx_buf->raw_buffer()));
+        static_cast< IndexBtreeNode* >(n)->attach_buf(idx_buf);
+        BtreeNodePtr bn{n};
+
+        LOGINFOMOD(wbcache, "set_root_from_committed_buf: updating SB root {} -> {} level={}", m_sb->root_node,
+                   idx_buf->blkid().to_integer(), bn->level());
+
+        m_sb->root_node = idx_buf->blkid().to_integer();
+        m_sb->root_link_version = bn->link_version();
+        m_sb->btree_depth = bn->level();
+        this->m_btree_depth = bn->level();
+        this->set_root_node_info(BtreeLinkInfo{m_sb->root_node, m_sb->root_link_version});
+        return true;
+    }
+
     void delete_stale_children(IndexBufferPtr const& idx_buf) override {
         if (!idx_buf->is_meta_buf() && idx_buf->m_created_cp_id == -1) {
             BtreeNode* n = this->init_node(idx_buf->raw_buffer(), idx_buf->blkid().to_integer(), false /* init_buf */,
@@ -266,6 +358,13 @@ class IndexTable : public IndexTableBase, public Btree< K, V > {
                         this->root_node_id());
             return;
         }
+        // Same-CP new nodes that never flushed are zero-filled; init_node(init_buf=false) would assert.
+        if (idx_buf->raw_buffer() == nullptr ||
+            !BtreeNode::is_valid_node(sisl::blob{idx_buf->raw_buffer(), this->m_bt_cfg.node_size()}) ||
+            r_cast< persistent_hdr_t const* >(idx_buf->raw_buffer())->node_id != idx_buf->blkid().to_integer()) {
+            LOGERROR("repair_node: skip invalid/unwritten buf {}", idx_buf->to_string());
+            return;
+        }
         BtreeNode* n = this->init_node(idx_buf->raw_buffer(), idx_buf->blkid().to_integer(), false /* init_buf */,
                                        BtreeNode::identify_leaf_node(idx_buf->raw_buffer()));
         static_cast< IndexBtreeNode* >(n)->attach_buf(idx_buf);
@@ -307,7 +406,7 @@ class IndexTable : public IndexTableBase, public Btree< K, V > {
         node->set_checksum();
         auto prev_state = idx_node->m_idx_buf->m_state.exchange(index_buf_state_t::DIRTY);
         LOGTRACEMOD(wbcache, "write_node_impl: node_id={} cp_id={} prev_state={} -> DIRTY", node->node_id(),
-                    cp_ctx->id(), static_cast<int>(prev_state));
+                    cp_ctx->id(), static_cast< int >(prev_state));
         idx_node->m_idx_buf->m_node_level = node->level();
         if (prev_state == index_buf_state_t::CLEAN) {
             // It was clean before, dirtying it first time, add it to the wb_cache list to flush

src/include/homestore/index_service.hpp

@@ -98,6 +98,8 @@ class IndexService {
     void repair_index_node(uint32_t ordinal, IndexBufferPtr const& node_buf);
     void parent_recover(uint32_t ordinal, IndexBufferPtr const& node_buf);
     void update_root(uint32_t ordinal, IndexBufferPtr const& node_buf);
+    // Returns true if SB root was updated to the committed buffer.
+    bool set_root_from_committed_buf(uint32_t ordinal, IndexBufferPtr const& node_buf);
 
     IndexWBCacheBase& wb_cache() {
         if (!m_wb_cache) { throw std::runtime_error("Attempted to access a null pointer wb_cache"); }

src/lib/index/index_cp.cpp

@@ -281,7 +281,7 @@ std::map< BlkId, IndexBufferPtr > IndexCPContext::recover(sisl::byte_view sb) {
     //    LOGTRACEMOD(wbcache,"\n\n\nAFTER modify : \n ");
     //    dag_print(buf_map, "After: ");
 
-    auto sanityCheck = [](const std::map< BlkId, IndexBufferPtr >& dags) {
+    auto sanityCheck = [cp_id = id()](const std::map< BlkId, IndexBufferPtr >& dags) {
         for (const auto& [blkid, bufferPtr] : dags) {
             auto up_buffer = bufferPtr->m_up_buffer;
             if (up_buffer) {
@@ -290,9 +290,16 @@ std::map< BlkId, IndexBufferPtr > IndexCPContext::recover(sisl::byte_view sb) {
                     "Sanity check failed: Buffer {} blkdid {} has an up_buffer {} blkid that is marked as freed.",
                     bufferPtr->to_string(), blkid.to_integer(), up_buffer->to_string(),
                     up_buffer->blkid().to_integer());
-                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);
+                // link_buf Condition 1 bypasses new→new links, so a buffer created in this CP must not
+                // still point at another same-CP-new up. Older downs may depend on a newly created up
+                // (e.g. old_root → new_root during root split). During journal recovery, buffers are
+                // fresh objects: -1 means not marked created-in-this-CP (equivalent to != cp_id).
+                if (bufferPtr->m_created_cp_id == cp_id) {
+                    HS_REL_ASSERT(
+                        up_buffer->m_created_cp_id != cp_id,
+                        "Sanity check failed: new Buffer {} has an up_buffer {} that just created (created_cp_id={})",
+                        bufferPtr->to_string(), up_buffer->to_string(), up_buffer->m_created_cp_id);
+                }
                 HS_REL_ASSERT(up_buffer->m_index_ordinal == bufferPtr->m_index_ordinal,
                               "Sanity check failed: Buffer {} has an up_buffer {} with different index_ordinal "
                               "(up_ordinal={}, buf_ordinal={})",
@@ -331,7 +338,8 @@ void IndexCPContext::process_txn_record(txn_record const* rec, std::map< BlkId,
     auto cpg = cp_mgr().cp_guard();
 
     auto const rec_to_buf = [&buf_map, &cpg](txn_record const* rec, bool is_meta, BlkId const& bid,
-                                             IndexBufferPtr const& up_buf) -> IndexBufferPtr {
+                                             IndexBufferPtr const& up_buf,
+                                             bool mark_created_in_cp = false) -> IndexBufferPtr {
         IndexBufferPtr buf;
         // MetaIndexBuffer always has blkid={0,0,0,0} regardless of which BTree table it belongs to.
         // When multiple tables have a root split in the same CP, all their MetaBufs share the same blkid
@@ -356,10 +364,17 @@ void IndexCPContext::process_txn_record(txn_record const* rec, std::map< BlkId,
             buf = it->second;
         }
 
+        // Mark before linking so Condition 1 matches IndexWBCache::link_buf (both ends new).
+        if (mark_created_in_cp) { buf->m_created_cp_id = cpg->id(); }
+
         if (up_buf) {
             auto real_up_buf = up_buf;
-            if (up_buf->m_created_cp_id == cpg->id()) {
+            // Mirror link_buf Condition 1: bypass up_buf only when BOTH up and down were created
+            // in this CP. If down is an older node, keep the dependency through up_buf.
+            if (up_buf->m_created_cp_id == cpg->id() && buf->m_created_cp_id == cpg->id()) {
                 real_up_buf = up_buf->m_up_buffer;
+                HS_DBG_ASSERT(real_up_buf,
+                              "Up buffer is newly created in this cp, but it doesn't have its own up_buffer");
             } else if (up_buf->m_node_freed) {
                 real_up_buf = up_buf->m_up_buffer;
                 LOGTRACEMOD(wbcache, "\n\n change upbuffer from {} to {}\n\n", up_buf->to_string(),
@@ -391,9 +406,8 @@ void IndexCPContext::process_txn_record(txn_record const* rec, std::map< BlkId,
     }
 
     for (uint8_t idx{0}; idx < rec->num_new_ids; ++idx) {
-        auto new_buf = rec_to_buf(rec, false /* is_meta */, rec->blk_id(cur_idx++),
-                                  inplace_child_buf ? inplace_child_buf : parent_buf);
-        new_buf->m_created_cp_id = cpg->id();
+        rec_to_buf(rec, false /* is_meta */, rec->blk_id(cur_idx++),
+                   inplace_child_buf ? inplace_child_buf : parent_buf, true /* mark_created_in_cp */);
     }
 
     for (uint8_t idx{0}; idx < rec->num_freed_ids; ++idx) {