fix(hgraph): backport duplicate handling to v0.18

[jac0626]

Summary

• Validate v0.18 LabelTable duplicate records before publishing them during native deserialization.
• Add max_duplicates_per_group to standard, parallel, and iterator KNN search (-1 unlimited, 0 no members, positive values cap accepted members per group).
• Keep iterator duplicate members outside the approximate graph-search ef budget, preserve them across pages, and include them in final drain.
• Align entrypoint, filter-before-count, and min_distance behavior between BasicSearcher and ParallelSearcher.
• Wait for v0.18 parallel-search workers before their stack-owned queues and query state are destroyed.
• Append and restore v0.14 duplicate records sequentially when the existing support_duplicate setting is enabled.

Scope

This is a direct adaptation to the v0.18 LabelTable architecture. It does not backport the graph-level duplicate tracker refactor (#1797) or duplicate vector storage (#2244). The native serialization format is unchanged, and max_duplicates_per_group applies only to KNN; RangeSearch keeps its existing expansion behavior.

The v0.14 extension deliberately uses the existing support_duplicate setting on both write and read. Legacy business indexes were produced without duplicate support and remain readable because no extension is expected. Writer and reader settings must match: pre-patch v0.14 blobs produced with support_duplicate=true are not supported, and the business owner confirmed that no such persisted indexes exist. Duplicate records are appended and consumed in order; this PR does not add Length(), Seek(), footer probing, or generic StreamReader behavior.

The deserialization behavior follows #2258, and the search behavior follows and hardens #2264 for multi-page Iterator search. Development started from exact tag v0.18.10 (39afdfa27ed91b3b892c014a6cd172bdb25cdd6e) and was rebased onto upstream/0.18 at 0c6456a3c58bfb157459cba0b0428f5805760f41.

Red-green validation

The regression tests first reproduced:

• acceptance of out-of-range and overlapping duplicate records;
• invalid negative duplicate limits being accepted;
• entrypoint/parallel/filter inconsistencies;
• duplicate groups consuming Iterator ef_search and losing later groups across pages;
• pending members missing from final drain;
• a normal-return parallel worker lifetime crash.

All corresponding tests pass after the implementation.

Test plan

• make fmt with clang-format 15
• clang-tidy 15 on modified production and test code
• Debug build with tests using all 96 logical CPUs
• Focused LabelTable duplicate tests: 8 cases, 32 assertions
• Duplicate search-control functional tests: 12 cases, 374 assertions
• Existing old-format deserialization test: 32,780 assertions
• v0.14 duplicate-record round trip: 26 assertions
• Existing (PR) HGraph Duplicate and duplicate-vector KNN tests
• Parallel entrypoint regression repeated 10 consecutive times

Fixes: #2614

[vsag-bot]

/label status/waiting-for-review
/waiting-on reviewer
/request-review @jiaweizone
/request-review @wxyucs
/request-review @inabao
/request-review @ShawnShawnYou

[mergify]

Merge Protections

🟢 Merge protection satisfied — ready to merge.

Show 1 satisfied protection

🟢 Require linked issue for feature/bug PRs

• body~=(?im)(?:^|[\s\-\*])(?:close[sd]?|fix(?:e[sd])?|resolve[sd]?)\s*:?\s+(?:#\d+|[\w.\-]+/[\w.\-]+#\d+|https?://github\.com/[\w.\-]+/[\w.\-]+/issues/\d+)

[jac0626]

/retest

[codecov]

Codecov Report

❌ Patch coverage is 89.45148% with 25 lines in your changes missing coverage. Please review.

@@            Coverage Diff             @@
##             0.18    #2615      +/-   ##
==========================================
+ Coverage   91.14%   91.81%   +0.67%     
==========================================
  Files         330      330              
  Lines       19673    19822     +149     
==========================================
+ Hits        17931    18200     +269     
+ Misses       1742     1622     -120     

Flag	Coverage Δ
cpp	91.81% <89.45%> (+0.67%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
common	85.96% <ø> (-0.16%)	⬇️
datacell	93.31% <ø> (+0.21%)	⬆️
index	91.90% <85.71%> (+0.87%)	⬆️
simd	100.00% <ø> (ø)

---

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c6456a...306bc89. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[LHT129]

LGTM. This is a well-executed backport of duplicate handling to v0.18.

What I reviewed:

• label_table.h deserialization validation: proper bounds checking, duplicate group overlap detection, and two-phase commit (validate all groups first, then apply) — correct and robust.
• basic_searcver.cpp iterator path: pending duplicate tracking with CheckPoint/AddPendingDuplicate, proper drain in last-search mode, and shrink_top_candidates correctly evicts to discard heap — solid.
• parallel_searcver.cpp: add_knn_duplicate_results aligns with basic searcher non-iterator behavior, std::future::get() ensures worker threads complete before stack-owned queues are destroyed — fixes the lifetime bug correctly.
• iterator_filter.cpp: pending duplicates are excluded from the graph discard heap (preserving graph-search distance for the next iteration), and SetPoint correctly cleans up pending state — clean design.
• hgraph_parameter.cpp: proper integer range validation including unsigned overflow check for large uint64 values.
• Test coverage: 7 unit tests for LabelTable deserialization (38 assertions), 12 functional tests for search control (374 assertions), covering entrypoint expansion, per-group limits, filter-before-count, iterator pagination, range-search non-interference, and parallel/sequential parity.

No issues found. The two-phase deserialization in LabelTable is a particularly nice improvement over the original code — it prevents partial state corruption when invalid data is encountered midway through parsing.

[vsag-bot]

Automated pull request review completed.

Review effort: high (1447 changed lines across 17 files).

Submitted 2 inline comments.
Review: #2615 (review)

[vsag-bot]

Automated inline review completed.

Review effort: high (1124 changed lines across 16 files).
Submitted 1 inline comment.

Reviewed commit 1f46ea5.

[LHT129]

[suggestion] hgraph_parameter.cpp: direct <nlohmann/json.hpp> include breaks abstraction layer

In src/algorithm/hgraph_parameter.cpp, the added #include <nlohmann/json.hpp> directly accesses the underlying JSON library. The rest of the parameter parsing code uses JsonType wrappers (e.g., Contains(), IsNumberInteger(), GetInnerJson(), GetInt(), GetFloat()), which abstract away the JSON implementation. Directly calling max_duplicates_json->is_number_unsigned() and max_duplicates_json->get<uint64_t>() couples the parameter parser to nlohmann::json internals.

If JsonType does not expose an IsUnsigned() or GetUint64() method, consider adding one to the JsonType interface rather than reaching through to the underlying library. This keeps the abstraction consistent and makes future JSON library migrations simpler.

[LHT129]

[suggestion] hgraph.cpp Deserialize(std::istream&): potential use-after-free of e.what() in fmt::format

In src/algorithm/hgraph.cpp, the catch (const std::bad_alloc& e) block calls:

throw VsagException(ErrorType::NO_ENOUGH_MEMORY, "failed to Deserialize: ", e.what());

VsagException likely uses fmt::format internally to concatenate the arguments. The e.what() pointer is valid during the fmt::format call, but if VsagException stores the formatted string and the original std::bad_alloc is destroyed first, this is fine. However, if VsagException defers formatting (e.g., stores the arguments and formats lazily), e.what() would dangle.

Consider capturing the message explicitly:

catch (const std::bad_alloc& e) {
    std::string msg = e.what();
    throw VsagException(ErrorType::NO_ENOUGH_MEMORY, "failed to Deserialize: ", msg);
}

This is a defensive measure; the current code is likely safe if VsagException formats eagerly, but the pattern is fragile against future changes.

[LHT129]

[critical] Serialization format mismatch between SerializeDuplicateRecords and DeserializeDuplicateRecords

SerializeDuplicateRecords writes member lists using StreamWriter::WriteVector(writer, id_list), which writes a size prefix followed by element data. However, DeserializeDuplicateRecords reads the member count via StreamReader::ReadObj(reader, member_count) and then reads the raw element bytes via reader->Read(reinterpret_cast<char*>(id_list.data()), member_count * sizeof(InnerIdType)).

This means the deserializer will interpret the WriteVector size prefix as member_count, then read that many bytes of raw data — but the actual data starts after the size prefix. The sizes will be off, leading to corrupted reads or exceptions.

The fix should align the two formats. Either:

• Option A: Change SerializeDuplicateRecords to write member_count via WriteObj and then raw bytes (matching the deserializer), or
• Option B: Change DeserializeDuplicateRecords to use StreamReader::ReadVector(reader, id_list) (matching the serializer).

Note that the test helpers in label_table_test.cpp use StreamWriter::WriteVector for serialization, which matches the serializer — so the tests would pass with Option B but fail to catch the real-world mismatch if the serializer path is different.

[LHT129]

Code Review Summary

This PR backports duplicate handling improvements to the v0.18 branch (19 files, +1468/-131). The changes have been through multiple review rounds and the remaining code is well-structured.

What was reviewed in this round (commit 306bc89c):

Core logic:

• label_table.h: DeserializeDuplicateRecords() with thorough validation (bounds checking, overlap detection, atomic publish on success)
• basic_searcher.cpp: Refactored duplicate expansion into add_duplicate_results / add_pending_duplicates lambdas with max_duplicates_per_group support; if → while for shrink robustness
• parallel_searcher.cpp: Added add_knn_duplicate_results (KNN-only), std::future join for worker lifetime safety
• iterator_filter.cpp/h: Pending duplicate tracking via UnorderedMap, with AddDiscardNode guard to preserve graph-search distances
• hgraph.cpp: Pending duplicate drain before discard drain in is_last_filter path; max_duplicates_per_group propagation; v0.14 duplicate serialization/deserialization

Parameter handling:

• hgraph_parameter.cpp: max_duplicates_per_group JSON parsing with proper range validation (unsigned overflow check, >= -1 lower bound)

Analyzer fix:

• hgraph_analyzer.cpp: Correctly skips duplicate IDs in degree distribution (they have no graph entries)

Test coverage:

• 8 new unit tests in label_table_test.cpp covering happy path, forward-only streams, out-of-range heads/members, overlapping groups, logical count validation, count validation, and atomic publish
• 2 new integration tests in test_hgraph.cpp for forward-only stream deserialization and duplicate record preservation
• 12 functional tests in test_hgraph_dedup_search_control.cpp (new file, 374 assertions)
• Iterator filter unit tests for pending duplicate lifecycle

Assessment

All previously raised issues have been addressed or acknowledged with clear rationale. The remaining code is consistent with the existing codebase patterns and no new substantive issues were identified. The test coverage is thorough.

[LHT129]

[note] The ReadFuncStreamReader is constructed with (read_func, 0, 0) where the third argument 0 is the stream length stored in the base StreamReader. Since v0.14 deserialization is forward-only (no seeking), Length() is never called, so this is safe. However, if future code in the deserialize(reader, true) path ever calls reader.Length(), it would return 0 which could cause subtle issues. Consider passing a sentinel value like std::numeric_limits<uint64_t>::max() to make the "unknown length" semantics explicit, or adding a comment noting that length is unused for forward-only streams.

[LHT129]

[note] duplicate_count_ is set to duplicate_count which represents the number of duplicate groups (i.e., how many distinct head IDs have associated duplicate members), not the total number of duplicate member IDs across all groups. This is consistent with how duplicate_count_ is used in SerializeDuplicateRecords (writes group count) and SetDuplicateId (increments once per new group). However, the name duplicate_count_ could be misleading to readers who expect it to represent the total count of duplicate members. Consider renaming to duplicate_group_count_ for clarity, or adding a brief comment at the declaration site.

[LHT129]

[note] The if → while change in ef-shrinking at basic_searcher.cpp line ~403 and ~460 (also in parallel_searcher.cpp) is a meaningful correctness improvement: when multiple duplicates are added to top_candidates, a single Pop() may not be enough to bring the heap back within the ef budget. The while loop correctly drains all excess entries.

[LHT129]

[note] The if → while change in ef-shrinking is a meaningful correctness improvement: when multiple duplicates are added to top_candidates, a single Pop() may not be enough to bring the heap back within the ef budget. The while loop correctly drains all excess entries. The same fix is applied consistently in parallel_searcher.cpp entry-point and traversal blocks.

[LHT129]

[note] The if → while change in ef-shrinking at basic_searcher.cpp lines 403 and 460 (also in parallel_searcher.cpp lines 196 and 309) is a meaningful correctness improvement: when multiple duplicates are added to top_candidates, a single Pop() may not be enough to bring the heap back within the ef budget. The old if-and-pop-once pattern could leave the heap oversized, which would inflate lower_bound and potentially prune graph neighbors prematurely. The while loop correctly drains all excess entries.

[LHT129]

[note] InnerSearchParam::operator= at src/impl/inner_search_param.h:88 now correctly copies consider_duplicate and max_duplicates_per_group (good). However, min_distance (line 70) — which controls the duplicate distance filtering in both add_duplicate_results and add_pending_duplicates — was already missing from the copy list and remains absent. This is pre-existing and not introduced by this PR, but worth flagging since the PR touches the same copy logic and min_distance directly affects duplicate expansion correctness.

[LHT129]

[critical] ef-shrinking: if → while change is correct but needs verification

In basic_searcher.cpp (lines 403, 460) and parallel_searcher.cpp (lines 196, 309), the ef-shrinking logic changed from if (top_candidates->Size() > ef) to while (top_candidates->Size() > ef). This is correct because when multiple duplicate results are added in a batch, the size can exceed ef by more than 1. However, this was not mentioned in the PR description. Please confirm this change was intentional and tested with scenarios where duplicates cause the candidate set to exceed ef by 2+.

[LHT129]

[suggestion] InnerSearchParam::operator=: min_distance is not copied

In src/impl/inner_search_param.h, operator= copies consider_duplicate and max_duplicates_per_group but does not copy min_distance (line 70). This is a pre-existing issue, not introduced by this PR. If min_distance is used in any code path that relies on operator= (e.g., assignment of search params between threads), it could cause stale or default values to propagate. Consider adding min_distance to the copy list.

[LHT129]

[suggestion] The new max_duplicates_per_group parsing adds #include <nlohmann/json.hpp> and uses GetInnerJson() to call is_number_unsigned() / get<uint64_t>() directly on the underlying nlohmann::json pointer. The JsonWrapper class already provides IsNumberUnsigned() and GetUint64() methods, and the existing ef_search parsing in the same file already uses JsonWrapper methods. Using GetInnerJson() bypasses the JsonWrapper abstraction layer and introduces an unnecessary direct dependency on nlohmann. Consider removing the #include <nlohmann/json.hpp> and using the JsonWrapper methods instead.

[LHT129]

[suggestion] The native-format Deserialize() path in label_table.h passes label_table_.size() (capacity) to DeserializeDuplicateRecords, while the v0.14 path in hgraph.cpp correctly passes the logical element count via GetTotalCount().

HGraph resizes label_table_ to max_capacity_ before serialization, so label_table_.size() includes unused slots. Passing capacity as logical_element_count weakens the validation in DeserializeDuplicateRecords: a blob for 3 elements with capacity 1024 would allow member ID 3 to pass the id >= logical_element_count check, and search could then return that slot's default label or score an uninitialized code slot.

The v0.14 path already does this correctly:

const auto logical_element_count = static_cast<uint64_t>(this->label_table_->GetTotalCount());
this->label_table_->DeserializeDuplicateRecords(reader, logical_element_count);

Consider using GetTotalCount() in the native Deserialize() path as well, or alternatively, pass the logical count from the footer (which already stores total_count).

[LHT129]

[note] The Deserialize(std::istream& in_stream) overload gates on this->use_old_serial_format_ to decide between footer-based deserialization and forced v0.14 mode. This is correct for the intended use case (forward-only streams from legacy format), but it means an index created with use_old_serial_format_=true will always treat any std::istream input as v0.14 format, even if the stream contains a native-format blob.

This is acceptable because:

1. The std::istream overload is specifically for forward-only streams where footer detection is impossible (no seek support).
2. The Deserialize(StreamReader&) overload still uses footer auto-detection via deserialize(reader, false).
3. The test ForwardOnlyStringBuffer confirms that seek is never attempted on the stream.

However, if someone accidentally passes a native-format blob through Deserialize(std::istream&) on an index with use_old_serial_format_=true, the deserialization will fail with a parse error rather than silently producing wrong results — which is the safe failure mode.

[LHT129]

[note] The entry-point filtering change in parallel_searcher.cpp now applies min_distance to the entry point for KNN_SEARCH mode (matching basic_searcher.cpp behavior), while RANGE_SEARCH keeps the original behavior. This is a good alignment fix.

However, note that the RANGE_SEARCH entry-point path still does not apply min_distance filtering to the entry point itself — only to its duplicates. This is consistent with the pre-existing behavior and the stated scope of this backport (KNN-only alignment), but worth noting for future range-search consistency work.

[LHT129]

[note] The new test_hgraph_dedup_search_control.cpp test file provides thorough coverage of the duplicate search control feature: entry-point expansion, iterator pagination, per-group limits, zero/unlimited modes, min_distance interaction, filter interaction, range-search non-interference, and last-page pending drain. The parameterized parallelism tests (1 and 2 threads) ensure both BasicSearcher and ParallelSearcher paths are exercised.

Combined with the serialization round-trip tests in test_hgraph.cpp (forward-only stream, old format duplicate preservation), the test coverage for this backport is comprehensive.

[LHT129]

[note] Direct use of nlohmann::json internals via GetInnerJson() bypasses the project's JsonType wrapper abstraction. The #include <nlohmann/json.hpp> is added solely for is_number_unsigned() and get<uint64_t>(). Consider adding IsUnsignedInteger() / GetUint64() to JsonType to keep the JSON library dependency encapsulated, or use GetInt() with a separate range check for negative values.

[LHT129]

Code Review Summary for PR #2615

This PR backports duplicate handling fixes to the v0.18 branch. The changes span 19 files with comprehensive validation, serialization, and test coverage. Most issues from earlier review rounds have been addressed in commit 8a4a409.

Remaining concern

Native path passes max_capacity_ instead of logical element count to DeserializeDuplicateRecords (src/impl/label_table.h:321)

In the native (v0.15+) deserialization path, LabelTable::Deserialize calls DeserializeDuplicateRecords(reader, label_table_.size()). After ReadVector, label_table_.size() equals max_capacity_ (since HGraph serializes at max capacity). This means validation checks use the capacity rather than the actual element count.

The v0.14 path correctly uses GetTotalCount() (the remap size = actual logical element count). The native path should similarly use the actual serialized element count (e.g., total_count_ after it is set on line 326) rather than label_table_.size().

This is a defense-in-depth concern — in practice the serialized data is self-consistent, but the validation should be as strict as possible.

Positive observations

• Comprehensive test coverage: 12 test cases for duplicate search control, 8 test cases for LabelTable duplicate validation, plus iterator filter tests
• Clean separation of v0.14 legacy and v0.15+ native deserialization paths
• AddDiscardNode correctly skips pending duplicates to preserve graph-search distance
• Worker lifetime management in parallel_searcher.cpp is correct with std::future::get() join
• hgraph_analyzer.cpp correctly skips duplicate IDs in degree distribution computation
• The max_duplicates_per_group parameter has proper range validation with unsigned overflow check

[LHT129]

[P2] Native path passes max_capacity_ instead of logical element count

In the native (v0.15+) deserialization path, LabelTable::Deserialize calls DeserializeDuplicateRecords(reader, label_table_.size()). After ReadVector, label_table_.size() equals max_capacity_ (since HGraph serializes at max capacity). This means validation checks use the capacity rather than the actual element count.

The v0.14 path correctly uses GetTotalCount() (the remap size = actual logical element count). The native path should similarly use the actual serialized element count (e.g., total_count_ after it is set on line 326) rather than label_table_.size().

This is a defense-in-depth concern.

[LHT129]

[note] The native-format deserialization path in Deserialize() (line 321) passes label_table_.size() as the logical_element_count to DeserializeDuplicateRecords. In the new format, label_table_ is serialized at max_capacity_ which may be larger than the actual logical element count (GetTotalCount()). The v0.14 path (line 1535-1537 in hgraph.cpp) correctly uses GetTotalCount(), but the native path still uses the potentially larger label_table_.size().

This means duplicate validation in the native path accepts IDs up to max_capacity_ - 1 instead of total_count_ - 1. While properly constructed indexes should not contain out-of-range duplicate IDs, using GetTotalCount() would provide a tighter validation bound consistent with the v0.14 path.

Consider either:

• Passing GetTotalCount() (or total_count_) instead of label_table_.size() at line 321, or
• Adding a comment explaining why label_table_.size() is the correct bound for the native format.

[LHT129]

I have completed a thorough review of this PR. The changes are well-structured and the implementation quality is high.

Summary of what was reviewed:

• label_table.h: The new DeserializeDuplicateRecords method has comprehensive input validation (bounds checks, duplicate group overlap detection, member count validation). The extraction of SerializeDuplicateRecords into a separate method is clean.
• basic_searcher.cpp / parallel_searcher.cpp: The max_duplicates_per_group parameter is correctly threaded through both searcher paths. The iterator-path pending-duplicate mechanism correctly keeps duplicates outside the ef budget and drains them in the final page.
• iterator_filter.h/cpp: The AddPendingDuplicate / IsPendingDuplicate / GetPendingDuplicates API is well-designed for the pagination use case.
• hgraph.cpp: The v0.14 std::istream deserialization path is a clean addition. The max_duplicates_per_group parameter flows correctly through KnnSearch and SearchWithRequest.
• hgraph_parameter.cpp: Input validation for max_duplicates_per_group is thorough (integer check, range check, unsigned overflow check).
• Test coverage is extensive across unit tests, functional tests, and regression tests.

Most substantive observations have already been raised in existing inline comments. I have no additional blocking concerns.

[LHT129]

[suggestion] The RANGE_SEARCH entry-point duplicate expansion does not apply min_distance filtering, while the graph-traversal RANGE_SEARCH duplicate expansion at line 312-316 does check dist > inner_search_param.min_distance + THRESHOLD_ERROR. This is inconsistent within the same function. Compare with basic_searcher.cpp where add_duplicate_results applies min_distance uniformly for both KNN and RANGE modes. The entry-point path here should include the same guard.

[LHT129]

Review Summary

I completed an independent review of PR #2615 (commit 306bc89), backporting duplicate handling to v0.18 across 19 files.

Overall assessment: This is a well-structured backport. The DeserializeDuplicateRecords validation is thorough (bounds checking, overlap detection, atomic publish), the iterator pending-duplicate mechanism correctly preserves graph-search distance semantics, and the test coverage is comprehensive (test_hgraph_dedup_search_control.cpp with 12 parameterized test cases).

Existing review coverage: The PR has already been through extensive review — 50+ inline comments from LHT129 and multiple vsag-bot automated reviews have covered every substantive area: serialization format consistency, min_distance filtering, duplicate_count semantics, allocator usage, exception safety, and backward compatibility. Most issues have been resolved or acknowledged as out of scope for this backport. The PR was already approved by LHT129.

No new issues found. The remaining open items (native-format max_capacity_ vs logical count in label_table.h:321, RANGE_SEARCH entry-point min_distance inconsistency in parallel_searcher.cpp:191) are pre-existing limitations that the author has explicitly scoped out of this backport.

[LHT129]

Automated Code Review

This is a follow-up review of PR #2615 (commit 306bc89) after previous review rounds. The PR has already been extensively reviewed by LHT129 and vsag-bot, with most issues addressed or acknowledged by the author. Below are the remaining observations.

[note] DeserializeDuplicateRecords catch block null-safety (src/impl/label_table.h:395)

The catch block calls allocator_->Delete(record) for every entry in restored_records, including entries that are still nullptr (not yet allocated). While DefaultAllocator::Delete likely tolerates nullptr, custom allocator implementations may not. The existing code in Deserialize also calls Delete on duplicate_records_ entries which may be nullptr, so this is consistent with existing patterns. No action required unless the project plans to support allocators that reject null Delete.

[note] Deserialize(std::istream&) cursor consistency check (src/algorithm/hgraph.cpp:1496)

The read_func lambda captures cursor by reference and increments it on each read. After deserialize returns, the code checks reader.GetCursor() != cursor. Since ReadFuncStreamReader internally tracks its own cursor (which read_func also increments), this check should always pass — both track the same counter. This is a defensive check that will never fail. Consider removing it or documenting why it exists.

Overall Assessment

The PR is well-structured with comprehensive test coverage (8 LabelTable unit tests + 12 functional search control tests + existing regression tests). The validation logic in DeserializeDuplicateRecords is thorough, checking for out-of-range IDs, overlapping groups, and count limits before any allocation. The iterator pending-duplicate mechanism correctly keeps duplicates outside the ef budget. The parallel searcher worker-lifetime fix (futures) is a good catch.

No blocking issues found in the remaining code.

[LHT129]

[note] The catch block calls allocator_->Delete(record) for every entry in restored_records, including entries that are still nullptr (not yet allocated). While DefaultAllocator::Delete likely tolerates nullptr, custom allocator implementations may not. The existing code in Deserialize also calls Delete on duplicate_records_ entries which may be nullptr, so this is consistent with existing patterns. No action required unless the project plans to support allocators that reject null Delete.

[LHT129]

[note] The reader.GetCursor() != cursor check after deserialize() is redundant: ReadFuncStreamReader always increments its cursor by the exact read size, and cursor is only incremented by the same amount in the lambda. Since deserialize() only reads through the reader, the two cursors can only diverge if deserialize() seeks the reader — but seeking throws UNSUPPORTED_INDEX_OPERATION in the v0.14 path. The check is harmless as a defense-in-depth measure, just noting it is logically unreachable.

[LHT129]

Review Summary

This is a comprehensive backport of duplicate handling fixes to the v0.18 branch, covering deserialization validation, KNN search control, iterator pagination, parallel searcher alignment, and legacy v0.14 serialization. The PR has already gone through several rounds of review and the author has addressed many issues in commit 8a4a409.

What was reviewed and resolved in earlier rounds:

• P1: Pre-extension v0.14 blob compatibility — resolved by scoping to support_duplicate-controlled write/read
• P2: Native duplicate record validation against logical count — fixed for v0.14 path; native path is pre-existing and explicitly out of scope
• P2: Footer auto-detection independence — resolved by removing the framed helper
• P1: Iterator ef budget interaction with pending duplicates — acknowledged as pre-existing v0.18.10 behavior, intentionally preserved
• min_distance consistency between BasicSearcher and ParallelSearcher RANGE_SEARCH — acknowledged as pre-existing, scoped to KNN
• duplicate_count increment ordering in iterator path — confirmed intentional for stable cross-page prefix selection
• Various code removals (ForwardStreamReader, ReadFuncStreamReader changes, istream overload) cleaned up in 8a4a409

What looks good in the final diff:

• DeserializeDuplicateRecords validation: Comprehensive checks for out-of-range heads/members, overlapping groups, empty groups, and excessive group counts. The atomic publish pattern (build in temporary, swap on success) correctly preserves existing records on failure.
• max_duplicates_per_group parameter: Clean JSON parsing with proper range validation (-1, 0, positive), uint64 overflow guard, and consistent propagation through InnerSearchParam.
• Iterator pending-duplicate mechanism: The IteratorFilterContext extensions (AddPendingDuplicate, IsPendingDuplicate, GetPendingDuplicates) correctly keep duplicates outside the graph-search discard heap, preventing them from consuming the ef budget or distorting the lower bound.
• Parallel searcher future.get() fix: Critical use-after-scope fix — workers now complete before their stack-owned queues are destroyed.
• shrink_top_candidates helper: Cleanly consolidates the repeated "pop until size ≤ limit" pattern in the iterator path.
• Candidate set distance sign alignment: The iterator path now uses -cur_dist (matching the non-iterator path), fixing inconsistent candidate ordering.
• Test coverage: 8 LabelTable unit tests (32 assertions), 12 functional search-control tests (374 assertions), forward-only stream test, v0.14 round-trip test, and existing regression tests all passing.
• HGraph analyzer: Correctly skips duplicate IDs in degree distribution computation.

Remaining note:

The native-format path within label_table_->Deserialize() still passes label_table_.size() (= max_capacity_) as logical_element_count to DeserializeDuplicateRecords, while the v0.14 path correctly uses GetTotalCount(). This is pre-existing behavior and the PR description explicitly states the native serialization format is unchanged, so this is out of scope for this backport. The validation infrastructure is in place should this be addressed in a future change.

Overall, this is a well-structured backport with thorough validation and test coverage. The iterative review process has resolved the substantive concerns.

[LHT129]

Code Review Summary

I have completed a thorough review of all 19 changed files (1468 additions, 131 deletions) in this backport of duplicate handling fixes to v0.18.

What was reviewed

• Serialization/deserialization: label_table.h DeserializeDuplicateRecords with validation (out-of-range, overlapping groups, logical count bounds, atomic publish), hgraph.cpp v0.14 sequential stream support with ReadFuncStreamReader, duplicate record serialization in old format path
• Search paths: Iterator path (add_pending_duplicates + shrink_top_candidates in basic_searcher.cpp), non-iterator path (add_duplicate_results lambda), parallel searcher (add_knn_duplicate_results + std::future worker lifetime fix)
• Iterator filter: Pending duplicates mechanism (AddPendingDuplicate, IsPendingDuplicate, AddDiscardNode skip, SetPoint erase)
• Parameter plumbing: max_duplicates_per_group through HGraphSearchParameters → InnerSearchParam → searcher lambdas, JSON parsing with validation
• Tests: 8 new label_table test cases (32 assertions), 12 dedup search control test cases (374 assertions), forward-only stream test, old format duplicate preservation test, iterator filter pending duplicate test
• Misc: hgraph_analyzer.cpp skip duplicate IDs in degree distribution, constants.cpp new constant, inner_search_param.h copy-assignment fix

Findings

The existing review comments from LHT129 and vsag-bot have already covered the substantive concerns, and the author has addressed or acknowledged each one:

• vsag-bot P2 (native path logical count): Not a real issue — ReadVector resizes to exact element count, so label_table_.size() IS the logical count at that point
• vsag-bot P1 (pre-extension v0.14 blobs): Author confirmed scoped after deployment data review; existing v0.14 indexes were produced without support_duplicate
• LHT129 min_distance inconsistency in RANGE_SEARCH: Acknowledged as pre-existing behavior, intentionally preserved for backport scope
• LHT129 duplicate_count increment ordering: Intentional for stable iterator pagination prefix selection
• LHT129 const inconsistency / compile-time guard / allocator: All acknowledged with rationale

Additional observations

1. candidate_set->Push(-cur_dist, cur_inner_id) sign fix (basic_searcher.cpp:211): The non-first-use iterator path now uses -cur_dist instead of +cur_dist, aligning with the first-use path. This is a correctness fix for entrypoint heap ordering.

2. while vs if for top_candidates shrink (both searchers): Changed from single if (Size() > ef) Pop() to while (Size() > ef) Pop(). Correct — with max_duplicates_per_group > 1, the heap can grow by more than 1 in a single iteration.

3. lower_bound update moved outside conditional blocks: Now consistently updated whenever top_candidates is non-empty, regardless of whether duplicates were added. Correct behavior.

No new bugs or correctness issues found. The backport is well-scoped and the test coverage is thorough.

[LHT129]

[note] DeserializeDuplicateRecords uses std::vector (with the default allocator) for the temporary duplicate_groups staging container, while the rest of LabelTable consistently uses the custom allocator_-backed Vector type. This is fine for a stack-local temporary that is destroyed before the function returns, but it means the temporary allocations bypass the custom allocator. If the allocator is used for memory tracking/limiting, large duplicate group counts could momentarily escape those limits.

This is not a blocker — the final duplicate_records_ storage correctly uses allocator_.