feat(pyramid): add fast multi-layer root routing and accelerate graph builds

[jac0626]

Change Type

• Bug fix
• New feature
• Improvement/Refactor
• Documentation
• CI/Build/Infra

Linked Issue

• Fixes: [feat](pyramid): align Pyramid no-path retrieval with HGraph factor control and multi-layer root graph #2712

What Changed

• Add per-hierarchy root_graph_type with backward-compatible single_layer and opt-in multi_layer values.
• Implement the multi-layer root as a dense flat bottom graph plus sparse routing graphs, built jointly with HGraph-style route/bottom insertion instead of a post-build overlay. graph_type continues to control non-root Pyramid nodes.
• Align Pyramid construction with the high-throughput HGraph flow: pre-encode supported data cells, use fixed workers with dynamic blocks, and build bottom/routing edges in the same insertion pass.
• Accelerate HGraph cold builds with fixed workers, 64-point dynamic blocks, safe parallel RaBitQ/SQ8 pre-encoding, worker-local search scratch allocation, and lower candidate-copy/allocation overhead. Incremental, deduplicated, extra-info, force-remove, split-storage, and unsupported storage paths retain the compatible fallback.
• Align explicit Pyramid factor semantics with HGraph while keeping RaBitQ lower-bound-safe reorder candidates separate from the requested final TopK.
• Apply hops_limit to non-root graph searches, preserve route graphs and format metadata through binary/streaming serialization, expose routing statistics, and populate raw-vector storage on cache-assisted builds.
• Add regression coverage for Build/Add, duplicate representatives, concurrent Add/Search, factor boundaries, hops, cache, serialization compatibility, RaBitQ 3-bit + SQ8 code validity, non-contiguous IDs, and force-remove ID reuse.
• Update English and Chinese Pyramid documentation.

Construction Design

For multi_layer, the Pyramid root owns:

• a dense flat bottom graph containing every root vector; and
• sparse upper routing graphs sampled from the same level assignment used during construction.

Each vector searches the existing route hierarchy, connects to the bottom graph, then connects to its eligible route levels. This follows HGraph's construction order inside Pyramid without extracting or refactoring a shared graph builder in this PR.

Test Evidence

• make fmt equivalent (clang-format 15 dry run across all changed C++ files)
• make lint equivalent (clang-tidy 15 on changed production .cpp files)
• make test
• make cov, run tests, and collect coverage
• Other (Debug build plus focused unit/functional suites)

cmake --build build --target unittests functests -j96
# passed

./build/tests/unittests '[ut][pyramid]' -r compact
# 23 test cases, 339 assertions passed

./build/tests/unittests '[ut][hgraph]' -r compact
# 41 test cases, 605 assertions passed

./build/tests/functests '*ForceRemove*,HGraph Sequential Add Remove ReAdd,HGraph reuses a removed route entry point without a self-edge' -r compact
# 9 test cases, 992 assertions passed

./build/tests/unittests '[ut][PyramidParameters],[ut][PyramidParameter]' -r compact
# 18 test cases, 171 assertions passed

./build/tests/unittests '[ut][BasicSearcher],[ut][ParallelSearcher]' -r compact
./build/tests/unittests '[ut][pruning_strategy]' -r compact
# 9 test cases, 65 assertions passed

./build/tests/functests '[ft][pyramid]' -r compact
# 56 test cases, 320781 assertions passed

clang-format-15 --dry-run --Werror <all changed C++ files>
clang-tidy-15 -p build <changed production .cpp files>
git diff --check
# passed

The final head must still pass the new PR CI/TSAN checks. Full make test and coverage were not run locally.

SIFT1M Build Comparison

Configuration: SIFT1M, 1,000,000 x 128-d L2 vectors, RaBitQ 3-bit base + SQ8 precise reorder, NSW, max_degree=64, ef_construction=400, alpha=1.0, 96 build threads pinned to CPUs 0-95, ef_search=100, TopK 10.

Index	Build wall	CPU time	Avg cores	CPU util.	Build throughput	Memory	Recall@10	1-thread QPS
HGraph	38.337 s	3391.929 s	88.476	92.162%	26,084 vec/s	0.582 GiB	0.980	1009.412
Pyramid multi-layer root	27.750 s	2244.320 s	80.875	84.245%	36,036 vec/s	0.614 GiB	0.981	1038.384

On the latest direct run, Pyramid used 27.62% less wall time and delivered 38.15% more build throughput than HGraph. Across three runs, Pyramid averaged 27.674 s versus 39.355 s (-29.68%) and 36,135 versus 25,425 vectors/s (+42.13%), with equal 0.981 average Recall@10. Its lower utilization reflects less total CPU work: three-run CPU time was 35.48% lower while wall time was also lower.

The HGraph optimization reduced its three-run average wall time by 39.20% and increased throughput by 64.57% versus the pre-optimization 64.728 s / 15,449 vectors/s baseline, with Recall@10 unchanged at 0.981.

Both final runs returned 10/10 self-nearest-neighbor checks. This final 1M run did not recollect P90/P99 latency, hops, distance-computation counts, serialized size, or a current single-layer Pyramid result; no claim is made for those issue acceptance measurements.

Compatibility Impact

• API/ABI compatibility: additive only; no public signature changes.
• Configuration: existing configurations remain single_layer. multi_layer requires a built root. We intentionally continue to allow explicit single_layer with no_build_levels: [0]; only multi_layer with an unbuilt root is rejected. This is narrower than the issue's broad wording and preserves valid rootless single-layer configurations.
• Search behavior: Pyramid now honors explicit reorder factor and applies hops_limit to graph nodes below the root.
• Serialization: released single-layer indexes remain loadable. New multi-layer payloads carry pyramid_root_storage_format_version=2; sparse multi-layer indexes produced by earlier unmerged revisions of this PR must be rebuilt.

Performance and Concurrency Impact

• Pyramid and HGraph cold construction use fixed workers and dynamic work blocks to avoid one-future-per-vector overhead and improve CPU occupancy.
• Parallel code insertion is enabled only for pre-sized, non-split storage that explicitly reports concurrent insertion support; all other configurations use the existing safe path.
• Node/point locking still protects graph publication, Add/Search concurrency, resize, cache refinement, and serialization. Force-remove builds retain self-candidate filtering when an internal ID can be reused.

Documentation Impact

• No docs update needed
• Updated docs:
  • README.md
  • DEVELOPMENT.md
  • CONTRIBUTING.md
  • Other: docs/docs/{en,zh}/src/indexes/pyramid.md

Risk and Rollback

• Risk level: medium
• Main risks: graph-construction concurrency, candidate-budget semantics, and new multi-layer serialization.
• Rollback plan: revert the feature/performance commits. Indexes configured with multi_layer format v2 would need to be rebuilt as single_layer after rollback.

Checklist

• I have linked the relevant issue
• I have added/updated tests for new behavior
• I have considered API compatibility impact
• I have updated docs if behavior/workflow changed
• My commit message follows project conventions

[vsag-bot]

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

[mergify]

Merge Protections

🟢 All 3 merge protections satisfied — ready to merge.

Show 3 satisfied protections

🟢 Require kind label

• label~=^kind/

🟢 Require version label

• label~=^version/

🟢 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+)

[Copilot]

Pull request overview

This PR adds an opt-in multi-layer routing overlay for Pyramid hierarchy roots (HGraph-style sparse route layers + existing complete bottom graph) and aligns Pyramid’s search-time reorder behavior with the common factor parameter, including additional stats and serialization support.

Changes:

• Add per-hierarchy root_graph_type (single_layer default, multi_layer opt-in) and implement root route-graph build/search + persistence.
• Apply factor to cap reorder candidate count (while preserving requested final TopK) and expose reorder_candidate_count in query statistics.
• Enforce hops_limit in ParallelSearcher and extend Pyramid’s hop limiting behavior to non-root graph nodes (while excluding route graphs).

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tests/test_pyramid.cpp	Adds functional coverage for multi-layer root behavior and streaming/binary-set serialization.
src/query_context.h	Adds reorder_candidate_count to search statistics JSON and storage.
src/impl/searcher/parallel_searcher.cpp	Enforces hops_limit during parallel graph traversal.
src/impl/searcher/parallel_searcher_test.cpp	Adds unit test validating hops_limit behavior.
src/impl/reorder/flatten_reorder.cpp	Records reorder candidate counts for flatten reorder paths.
src/impl/reorder/bucket_reorder.cpp	Records reorder candidate counts for bucket reorder.
src/constants.cpp	Introduces Pyramid root_graph_type constants.
src/analyzer/pyramid_analyzer.cpp	Updates analyzer search call to pass entry point explicitly.
src/algorithm/pyramid/pyramid.h	Adds multi-layer root routing structures, APIs, and memory reporting declarations.
src/algorithm/pyramid/pyramid.cpp	Implements root route graph planning/build/search, factor-based reorder limiting, stats, memory reporting, and serialization hooks.
src/algorithm/pyramid/pyramid_zparameters.h	Adds root_graph_type to parameter structs.
src/algorithm/pyramid/pyramid_zparameters.cpp	Validates root_graph_type and explicit factor; wires config mapping/compat checks.
src/algorithm/pyramid/pyramid_zparameters_test.cpp	Adds tests for root_graph_type validation and explicit factor validation.
src/algorithm/pyramid/pyramid_test.cpp	Adds unit tests for routing, serialization survival, factor semantics, duplicates, and hop limiting behavior.
src/algorithm/index_search_parameter.h	Tracks whether factor was explicitly provided (has_topk_factor).
include/vsag/constants.h	Exposes new Pyramid constants in the public header.
docs/docs/zh/src/indexes/pyramid.md	Documents root_graph_type, factor, updated hops_limit semantics, and new stats fields (ZH).
docs/docs/en/src/indexes/pyramid.md	Documents root_graph_type, factor, updated hops_limit semantics, and new stats fields (EN).

Suppressed comments (3)

src/algorithm/pyramid/pyramid.cpp:428

• IndexNode::Search reads entry_point_ outside the node mutex and passes it into search_func. Since entry_point_ is written under node->mutex_ during insert/promote, this introduces a data race and can pass a torn/stale value to graph search. Capture entry_point_ while holding the shared_lock and pass the snapshot to search_func.

    bool has_index = false;
    {
        std::shared_lock lock(mutex_);
        has_index = status_ != IndexNode::Status::NO_INDEX;
    }

src/algorithm/pyramid/pyramid.cpp:1396

• rebuild_root_routes_by_nsw() calls plan_root_route_ids(), which assigns hierarchy.root->entry_point_. That write currently happens while only holding a shared_lock on root->mutex_, which violates the lock contract and can race with concurrent readers/writers. Use an exclusive lock here (or stop mutating entry_point_ inside route planning).

                    std::unique_lock route_lock(h_ptr->root_routing_mutex);
                    if (not h_ptr->root_routes_initialized) {
                        std::shared_lock root_lock(h_ptr->root->mutex_);
                        rebuild_root_routes_by_nsw(*h_ptr);
                        h_ptr->root_routes_initialized = true;

src/algorithm/pyramid/pyramid.cpp:239

• add_to_root_routes updates hierarchy.root->entry_point_ without holding IndexNode::mutex_. Since entry_point_ is guarded by node->mutex_ elsewhere (e.g., add_one_point), this write can race with concurrent reads/writes. Update entry_point_ under an exclusive lock on hierarchy.root->mutex_ (or make entry_point_ atomic / store a separate routing entry point in Hierarchy).

    for (int route_level = current_top + 1; route_level <= level; ++route_level) {
        auto graph = make_root_route_graph(hierarchy);
        graph->InsertNeighborsById(inner_id, Vector<InnerIdType>(allocator_));
        hierarchy.root_route_graphs.push_back(std::move(graph));
        hierarchy.root->entry_point_ = inner_id;
    }

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

src/algorithm/pyramid/pyramid.cpp:770

• search_param.topk is used as the topk argument to reorder_->Reorder(...), but when factor is set this value represents the reorder candidate limit, not the requested final TopK. Passing the larger value increases reorder heap work unnecessarily; the final result size is already enforced by final_topk later in this function.

        search_result = this->reorder_->Reorder(search_result,
                                                query->GetFloat32Vectors(),
                                                search_param.topk,
                                                ctx,

docs/docs/en/src/indexes/pyramid.md:160

• The docs describe factor as a general search parameter, but the implementation only applies it in KnnSearch (RangeSearch ignores it). Clarify that factor affects reorder candidate limiting for KnnSearch so users don’t assume it impacts RangeSearch.

| `factor` | float | unset | Reorder candidate multiplier. When set to `<= 1`, reorder up to `max(ef_search, topk)` candidates; when greater than `1`, reorder up to `min(max(ef_search, topk), floor(topk * factor))`. It must be finite and positive. It has no effect when reorder is disabled. |

docs/docs/zh/src/indexes/pyramid.md:154

• 文档将 factor 描述为通用检索参数，但实现仅在 KnnSearch 中应用（RangeSearch 会忽略）。建议注明 factor 仅影响 KnnSearch 的重排候选数，避免用户误以为范围检索也会生效。

| `factor` | float | 未设置 | 重排候选倍率。值 `<= 1` 时最多重排 `max(ef_search, topk)` 个候选；值大于 `1` 时最多重排 `min(max(ef_search, topk), floor(topk * factor))` 个候选。必须为有限正数；关闭重排时不生效。 |

[Copilot]

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/algorithm/pyramid/pyramid_zparameters.cpp:226

• root_graph_type is currently allowed when no_build_levels contains level 0 as long as the value is single_layer. The linked issue/PR description calls out rejecting configs that specify a root graph type while level 0 is disabled; allowing this makes misconfigurations harder to catch (the root graph type is meaningless when the root graph is not built).

    if (json.Contains(PYRAMID_ROOT_GRAPH_TYPE)) {
        CHECK_ARGUMENT(json[PYRAMID_ROOT_GRAPH_TYPE].IsString(),
                       "root_graph_type must be a string");
        this->root_graph_type = json[PYRAMID_ROOT_GRAPH_TYPE].GetString();
    }
    validate_root_graph_config(this->root_graph_type, this->no_build_levels, "Pyramid");

src/analyzer/pyramid_analyzer.cpp:1082

• node->entry_point_ is read without holding IndexNode::mutex_ when calling Pyramid::search_node(). Other call paths snapshot the entry point under the node lock to avoid races with concurrent add/promote/route updates, so this analyzer path can observe a torn/stale value.

                                           node->entry_point_);

src/algorithm/pyramid/pyramid.cpp:1241

• Binary-set Pyramid::Deserialize() does not validate the stored INDEX_PARAM against create_param_ptr_ before conditionally deserializing root_route_graphs. Because route-graph payload presence depends on the configured root_graph_type, loading with a mismatched config can misalign the stream and produce confusing failures (or attempt to deserialize graphs from non-graph bytes). Streaming deserialization already performs a CheckCompatibility() guard; the non-streaming path should do the same before reading hierarchy payloads.

            deserialize_root_routes(buffer_reader, *h_iter->second);

src/algorithm/pyramid/pyramid_zparameters.cpp:131

• Per-hierarchy parsing has the same gap: an explicitly provided root_graph_type is accepted even when no_build_levels disables level 0, as long as the value is single_layer. That contradicts the stated requirement to reject specifying a root graph type when level 0 is not built.

This issue also appears on line 221 of the same file.

    if (json.Contains(PYRAMID_ROOT_GRAPH_TYPE)) {
        CHECK_ARGUMENT(json[PYRAMID_ROOT_GRAPH_TYPE].IsString(),
                       fmt::format("hierarchy {} root_graph_type must be a string", name));
        root_graph_type = json[PYRAMID_ROOT_GRAPH_TYPE].GetString();
    }

[Copilot]

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

src/algorithm/pyramid/pyramid.cpp:779

• In search_impl(), the reorder stage is invoked with topk=search_param.topk, but search_param.topk is now overloaded to mean the reorder candidate limit when factor is provided. This makes reorder maintain a larger heap than necessary (often ef), increasing CPU/memory cost without affecting the final TopK (which is later trimmed to final_topk). Pass final_topk to Reorder() and keep the candidate cap via reorder_candidate_limit.

        search_result = this->reorder_->Reorder(search_result,
                                                query->GetFloat32Vectors(),
                                                search_param.topk,
                                                ctx,
                                                nullptr,
                                                rabitq_lower_bound_candidates);

src/algorithm/index_search_parameter.h:44

• IndexSearchParameter::FromJson sets has_topk_factor=true when factor is present, but it never resets has_topk_factor (or topk_factor) when parsing JSON that omits the field. If the same IndexSearchParameter instance is reused across requests, a previously provided factor will incorrectly persist into later searches.

        if (json.Contains(SEARCH_PARAM_FACTOR)) {
            topk_factor = json[SEARCH_PARAM_FACTOR].GetFloat();
            has_topk_factor = true;
        }

[jac0626]

Addressed the remaining suppressed review notes as well:

• 4609071 rejects an explicitly configured root_graph_type when the effective no_build_levels disables level 0, for both top-level and named-hierarchy configurations. An omitted root_graph_type still defaults to single_layer, preserving existing no-root configurations and serialized parameter round trips.
• The English and Chinese docs now state that factor controls KNN reorder candidates and does not affect range search.
• 87f7684 already passes final_topk to Reorder and validates legacy StreamReader loads before reading route payloads.
• No reset was added to IndexSearchParameter::FromJson: every current parser constructs a fresh parameter object before calling it, and the base parser consistently applies only fields present in the JSON, so the reported stale-factor path is not reachable in the current design.

[Copilot]

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/algorithm/pyramid/pyramid.cpp:631

• search_impl() is currently passed parsed_param.has_topk_factor ? search_param.topk : -1, but after separating the reorder candidate limit from search_param.topk, this should pass the computed reorder_candidate_limit so factor only caps the heap entering reorder.

                          k,
                          parsed_param.has_topk_factor ? search_param.topk : -1,
                          base_computer,

src/algorithm/pyramid/pyramid.cpp:559

• The factor handling mutates search_param.topk to the reorder-candidate limit. However, search_param.topk is also used later in search_node() (via get_suitable_ef_search(search_param.topk, ...)) to tune non-root graph ef, so specifying factor can unintentionally change leaf graph exploration work/recall. Keep search_param.topk as the requested TopK (or threshold-expanded TopK) and compute a separate reorder_candidate_limit for search_impl().

This issue also appears on line 629 of the same file.

    if (use_reorder_ and parsed_param.has_topk_factor) {
        if (parsed_param.topk_factor <= 1.0F) {
            search_param.topk = static_cast<int64_t>(search_param.ef);
        } else {
            const auto amplified_topk = static_cast<uint64_t>(

[Copilot]

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.

Suppressed comments (8)

src/impl/searcher/parallel_searcher.cpp:243

• The hops_limit check is likely off-by-one: with the current ordering (hops++ then if (hops >= limit) break), the loop stops before performing work for hop limit, effectively allowing at most limit - 1 expansions. If the intended meaning is “allow up to hops_limit hops”, switch to if (hops > inner_search_param.hops_limit) (or move the check before incrementing) and ensure stats.hops matches the intended semantics.

    while (not candidate_set->Empty()) {
        hops++;
        if (hops >= inner_search_param.hops_limit) {
            break;
        }

src/algorithm/index_search_parameter.h:44

• has_topk_factor is only set to true when factor exists, but it is never reset to false when factor is absent. If IndexSearchParameter::FromJson can be called multiple times on the same instance, stale state can incorrectly treat a missing factor as explicitly provided. Setting has_topk_factor = false at the start of FromJson (or in an else branch) would make the parsing idempotent.

        if (json.Contains(SEARCH_PARAM_FACTOR)) {
            topk_factor = json[SEARCH_PARAM_FACTOR].GetFloat();
            has_topk_factor = true;
        }

src/algorithm/index_search_parameter.h:61

• has_topk_factor is only set to true when factor exists, but it is never reset to false when factor is absent. If IndexSearchParameter::FromJson can be called multiple times on the same instance, stale state can incorrectly treat a missing factor as explicitly provided. Setting has_topk_factor = false at the start of FromJson (or in an else branch) would make the parsing idempotent.

    // for reorder, controls the number of candidates to reorder
    float topk_factor{0.0F};
    bool has_topk_factor{false};
    bool enable_reorder{true};

src/algorithm/pyramid/pyramid.cpp:136

• build_routes_by_odescent() mutates node.entry_point_ (via plan_route_ids) and node.routing_->graphs without acquiring node.mutex_ / node.routing_->mutex internally. The current call sites appear to lock externally, but this function is easy to misuse later and would then become racy. Consider taking the appropriate locks inside this function (or adding assertions that the caller already holds them) to make the synchronization contract explicit and safer.

void
Pyramid::build_routes_by_odescent(const Hierarchy& hierarchy,
                                  IndexNode& node,
                                  const FlattenInterfacePtr& codes,
                                  bool use_thread_pool) {
    if (not node.has_routing() || node.status_ != IndexNode::Status::GRAPH) {
        return;
    }
    auto route_ids = plan_route_ids(node);
    node.routing_->graphs.clear();
    node.routing_->graphs.reserve(route_ids.size());

src/algorithm/pyramid/pyramid.cpp:456

• get_memory_usage_detail() holds a node-level shared lock while recursively traversing children (and each child acquires its own lock). For large trees this can create long lock chains and increase contention with concurrent Add/Search operations, especially if GetMemoryUsage*() is called frequently (e.g., autotune/telemetry). A safer pattern is to snapshot child pointers/keys under the lock, release it, and then recurse without holding the parent lock.

std::pair<uint64_t, uint64_t>
IndexNode::get_memory_usage_detail() const {
    uint64_t memory = sizeof(IndexNode);
    uint64_t routing_memory = 0;

src/algorithm/pyramid/pyramid.cpp:456

• get_memory_usage_detail() holds a node-level shared lock while recursively traversing children (and each child acquires its own lock). For large trees this can create long lock chains and increase contention with concurrent Add/Search operations, especially if GetMemoryUsage*() is called frequently (e.g., autotune/telemetry). A safer pattern is to snapshot child pointers/keys under the lock, release it, and then recurse without holding the parent lock.

    std::shared_lock lock(mutex_);

src/algorithm/pyramid/pyramid.cpp:456

• get_memory_usage_detail() holds a node-level shared lock while recursively traversing children (and each child acquires its own lock). For large trees this can create long lock chains and increase contention with concurrent Add/Search operations, especially if GetMemoryUsage*() is called frequently (e.g., autotune/telemetry). A safer pattern is to snapshot child pointers/keys under the lock, release it, and then recurse without holding the parent lock.

    for (const auto& [key, child] : children_) {
        memory += key.capacity() + 1;
        const auto [child_memory, child_routing_memory] = child->get_memory_usage_detail();
        memory += child_memory;
        routing_memory += child_routing_memory;
    }

src/query_context.h:262

• reorder_candidate_count is stored as uint32_t and incremented via static_cast<uint32_t>(count) in reorder implementations. If the candidate list can exceed UINT32_MAX (e.g., very large ef_search / topk), this will silently wrap and produce misleading statistics. Consider using std::atomic<uint64_t> (and emitting it as uint64 in JSON) or explicitly clamping/saturating the counter.

    std::atomic<uint32_t> io_cnt{0};
    std::atomic<uint32_t> io_time_ms{0};
    std::atomic<uint32_t> reorder_distance_count{0};
    std::atomic<uint32_t> reorder_candidate_count{0};

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/algorithm/pyramid/pyramid.cpp:1031

• Pyramid::Serialize() writes the node snapshot (IndexNode::Serialize) and the routing overlay (IndexNode::serialize_routing) under two separate node mutex acquisitions. Because root Add/search can mutate entry_point_ and routing_->graphs under IndexNode::mutex_, serialization can capture an inconsistent combination (e.g., entry_point_ from before a route update, but route graphs from after). Consider holding a single shared lock across both payloads (e.g., move routing serialization into IndexNode::Serialize under the same lock, or have Pyramid::Serialize take a shared_lock on root->mutex_ around both calls).

        for (const auto& [hname, h_ptr] : hierarchies_) {
            StreamWriter::WriteString(writer, hname);
            h_ptr->root->Serialize(writer);
            h_ptr->root->serialize_routing(writer);
        }

src/impl/searcher/parallel_searcher.cpp:245

• ParallelSearcher enforces hops_limit, but it never reports the local hops/dist_cmp counters into QueryContext::stats (unlike BasicSearcher, which fetch_adds them). As a result, callers observing SearchStatistics (including the new unit test) will see hops remain 0 even when work was performed, and the reported stats can’t be used to validate that hops limiting actually occurred. Add the same stats accounting as BasicSearcher before returning.

    while (not candidate_set->Empty()) {
        hops++;
        if (hops >= inner_search_param.hops_limit) {
            break;
        }

src/impl/searcher/parallel_searcher_test.cpp:179

• This test asserts stats.hops <= param.hops_limit, but ParallelSearcher currently does not update stats.hops, so the assertion can pass vacuously (hops stays 0) even if the search loop ignored the hop cap. After fixing ParallelSearcher to report hops, strengthen the test to ensure hops is actually incremented (e.g., REQUIRE(stats.hops.load() > 0) and/or assert it equals the cap for this chain graph).

    SearchStatistics stats;
    QueryContext context{.stats = &stats};
    auto vl = pool->TakeOne();
    auto result = ParallelSearcher(common, SafeThreadPool::FactoryDefaultThreadPool())
                      .Search(graph, flatten, vl, &query, param, nullptr, &context);
    pool->ReturnOne(vl);
    REQUIRE(stats.hops.load() <= param.hops_limit);
    REQUIRE(result->Top().second != 5);

[Copilot]

Pull request overview

Copilot reviewed 29 out of 29 changed files in this pull request and generated 1 comment.