From 38b370312cd51a2df2baf156bca5537a41afe88e Mon Sep 17 00:00:00 2001 From: amkram Date: Mon, 13 Jul 2026 16:32:36 -0700 Subject: [PATCH 1/3] fix(index): collapse per-node syncmer change record to one entry per position A node can touch the same syncmer position more than once -- e.g. a nuc mutation SUBs/ADDs a syncmer, then that syncmer's block is dropped, DEL'ing it. The change record then carries a stale non-DEL entry whose position a later DEL already erased from the map. Three consumers assume one entry per position: - index_single_mode::computeNewKminmerRanges: find() == end() -> "syncmer position not found" abort (the placement/.idx build). - mgsr::computeNewKminmerRanges: the found position's refOnSyncmers optional is empty -> std::bad_optional_access (the --meta/.midx build); mgsr's block-deletion loop also dereferences an already-deleted position's optional. - the backtrack restores the wrong pre-node value. Collapse each node's change record to one NET entry per position (net type from final map membership, restore rsyncmer from the first touch) before it is consumed, in both the sequential and parallel index_single_mode builders and in mgsr; also guard mgsr's block-deletion loop against already-deleted positions. No-op for panmans where no position repeats (rsv/sars/tb/ecoli/HIV/mtDNA): all unit tests pass and sars's .midx is byte-identical before/after. Fixes indexing of klebs_1000 (both .idx at k=51 and --meta), which deterministically hit these crashes. --- src/index_single_mode.cpp | 44 +++++++++++++++++++++++++++++++++++++++ src/mgsr.cpp | 44 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+) diff --git a/src/index_single_mode.cpp b/src/index_single_mode.cpp index 6497ca9f..9eb8dbef 100644 --- a/src/index_single_mode.cpp +++ b/src/index_single_mode.cpp @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -697,6 +698,47 @@ index_single_mode::IndexBuilder::computeNewKminmerRanges( return newKminmerRanges; } +namespace { +// Collapse a node's change record to one net entry per position. A node that both edits a +// seed and deletes that seed's block records the position twice (SUB/ADD, then DEL), but the +// consumers assume one entry -- the stale non-DEL entry names a position the DEL already +// erased, so computeNewKminmerRanges' find() fails and the backtrack restores a wrong value. +// Net kind = whether the position survives to the node's end (final map); restore value = the +// first (pre-node) entry. No-op when no position repeats. +void dedupeSyncmerChangeRecord( + std::vector>& rec, + const index_single_mode::SyncmerSet& finalMap) { + if (rec.size() < 2) return; + std::unordered_map> firstTouch; + firstTouch.reserve(rec.size()); + std::vector order; + order.reserve(rec.size()); + bool anyRepeat = false; + for (const auto& [pos, type, rsync] : rec) { + auto [it, inserted] = firstTouch.try_emplace(pos, type, rsync); + if (inserted) + order.push_back(pos); + else + anyRepeat = true; + } + if (!anyRepeat) return; + std::vector> out; + out.reserve(order.size()); + for (uint64_t pos : order) { + const auto& [firstType, firstRsync] = firstTouch[pos]; + const bool finalHas = finalMap.find(pos) != finalMap.end(); + if (firstType == panmapUtils::seedChangeType::ADD) { // parent lacked pos; keep only if it survives + if (finalHas) out.emplace_back(pos, panmapUtils::seedChangeType::ADD, seeding::rsyncmer_t()); + } else { // parent had pos; restore its pre-node value + out.emplace_back(pos, + finalHas ? panmapUtils::seedChangeType::SUB : panmapUtils::seedChangeType::DEL, + firstRsync); + } + } + rec = std::move(out); +} +} // namespace + void index_single_mode::IndexBuilder::buildIndexHelper(panmanUtils::Node* node, std::unordered_set& emptyNodes, panmapUtils::BlockSequences& blockSequences, @@ -929,6 +971,7 @@ void index_single_mode::IndexBuilder::buildIndexHelper(panmanUtils::Node* node, } } + dedupeSyncmerChangeRecord(refOnSyncmersChangeRecord, refOnSyncmersMap); std::vector> newKminmerRanges = computeNewKminmerRanges(refOnSyncmersChangeRecord, dfsIndex); @@ -1916,6 +1959,7 @@ void index_single_mode::IndexBuilder::processNode(panmanUtils::Node* node, std::vector addedSeedHashes; std::vector> substitutedSeedHashes; // + dedupeSyncmerChangeRecord(refOnSyncmersChangeRecord, state.refOnSyncmersMap); std::vector> newKminmerRanges = computeNewKminmerRanges(refOnSyncmersChangeRecord, state, dfsIndex); diff --git a/src/mgsr.cpp b/src/mgsr.cpp index 3c62e493..17d3aa4d 100644 --- a/src/mgsr.cpp +++ b/src/mgsr.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -3259,6 +3260,45 @@ std::vector mgsr::mgsrIndexBuilder::computeNewSync return newSyncmerRanges; } +namespace { +// Collapse a node's change record to one net entry per position (see the twin in +// index_single_mode.cpp for the full rationale). The same node-edits-a-seed-then-deletes-its- +// block case leaves a stale entry that here derefs an already-cleared refOnSyncmers optional +// -> bad_optional_access. Net kind = final-map membership; restore = first (pre-node) entry. +void dedupeSyncmerChangeRecord( + std::vector>& rec, + const std::set& finalMap) { + if (rec.size() < 2) return; + std::unordered_map> firstTouch; + firstTouch.reserve(rec.size()); + std::vector order; + order.reserve(rec.size()); + bool anyRepeat = false; + for (const auto& [pos, type, rsync] : rec) { + auto [it, inserted] = firstTouch.try_emplace(pos, type, rsync); + if (inserted) + order.push_back(pos); + else + anyRepeat = true; + } + if (!anyRepeat) return; + std::vector> out; + out.reserve(order.size()); + for (uint64_t pos : order) { + const auto& [firstType, firstRsync] = firstTouch[pos]; + const bool finalHas = finalMap.find(pos) != finalMap.end(); + if (firstType == panmapUtils::seedChangeType::ADD) { + if (finalHas) out.emplace_back(pos, panmapUtils::seedChangeType::ADD, seeding::rsyncmer_t()); + } else { + out.emplace_back(pos, + finalHas ? panmapUtils::seedChangeType::SUB : panmapUtils::seedChangeType::DEL, + firstRsync); + } + } + rec = std::move(out); +} +} // namespace + std::vector::iterator, std::set::iterator>> mgsr::mgsrIndexBuilder::computeNewKminmerRanges( std::vector>& refOnSyncmersChangeRecord, @@ -3584,6 +3624,9 @@ void mgsr::mgsrIndexBuilder::buildIndexHelper(panmanUtils::Node* node, if (oldExists && !newExists) { if (blockOnSyncmers.find(blockId) != blockOnSyncmers.end()) { for (uint64_t pos : blockOnSyncmers[blockId]) { + // Skip: this position was already deleted earlier in this node (its + // blockOnSyncmers cleanup is deferred), so refOnSyncmers is nullopt here. + if (!refOnSyncmers[pos].has_value()) continue; refOnSyncmersChangeRecord.emplace_back( pos, panmapUtils::seedChangeType::DEL, refOnSyncmers[pos].value()); blockOnSyncmersChangeRecord.emplace_back(blockId, pos, panmapUtils::seedChangeType::DEL); @@ -3596,6 +3639,7 @@ void mgsr::mgsrIndexBuilder::buildIndexHelper(panmanUtils::Node* node, } } + dedupeSyncmerChangeRecord(refOnSyncmersChangeRecord, refOnSyncmersMap); std::vector::iterator, std::set::iterator>> newKminmerRanges = computeNewKminmerRanges(refOnSyncmersChangeRecord, dfsIndex); From b77967f2e9b235fba8437e1297615e40ded7c8cd Mon Sep 17 00:00:00 2001 From: Alex Kramer Date: Mon, 13 Jul 2026 19:02:02 -0700 Subject: [PATCH 2/3] Refactor comments --- src/index_single_mode.cpp | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/index_single_mode.cpp b/src/index_single_mode.cpp index 9eb8dbef..604fb6ed 100644 --- a/src/index_single_mode.cpp +++ b/src/index_single_mode.cpp @@ -699,12 +699,8 @@ index_single_mode::IndexBuilder::computeNewKminmerRanges( } namespace { -// Collapse a node's change record to one net entry per position. A node that both edits a -// seed and deletes that seed's block records the position twice (SUB/ADD, then DEL), but the -// consumers assume one entry -- the stale non-DEL entry names a position the DEL already -// erased, so computeNewKminmerRanges' find() fails and the backtrack restores a wrong value. -// Net kind = whether the position survives to the node's end (final map); restore value = the -// first (pre-node) entry. No-op when no position repeats. +// Collapse a node's change record to one net entry per position, to handle a node that both edits a +// seed and deletes that seed's block. void dedupeSyncmerChangeRecord( std::vector>& rec, const index_single_mode::SyncmerSet& finalMap) { From fef79c907f6853fd64823676a858637b4b51aa7d Mon Sep 17 00:00:00 2001 From: Alex Kramer Date: Mon, 13 Jul 2026 19:19:57 -0700 Subject: [PATCH 3/3] Simplify comments --- src/mgsr.cpp | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/mgsr.cpp b/src/mgsr.cpp index 17d3aa4d..5707be28 100644 --- a/src/mgsr.cpp +++ b/src/mgsr.cpp @@ -3261,10 +3261,7 @@ std::vector mgsr::mgsrIndexBuilder::computeNewSync } namespace { -// Collapse a node's change record to one net entry per position (see the twin in -// index_single_mode.cpp for the full rationale). The same node-edits-a-seed-then-deletes-its- -// block case leaves a stale entry that here derefs an already-cleared refOnSyncmers optional -// -> bad_optional_access. Net kind = final-map membership; restore = first (pre-node) entry. +// Collapse a node's change record to one net entry per position void dedupeSyncmerChangeRecord( std::vector>& rec, const std::set& finalMap) { @@ -8308,4 +8305,4 @@ std::vector mgsr::getNearestNodes(mgsr::MgsrLiteNode* node, } return result; -} \ No newline at end of file +}