fix(opensearch): rank the wildcard graph view from a complete candidate set by YashvantHange · Pull Request #3707 · HKUDS/LightRAG

YashvantHange · 2026-08-23T19:36:20Z

Description

OpenSearchGraphStorage ranked the * graph view by degree read from two terms aggregations on the edge index, one per endpoint field, each capped at its own size. The union of two separately-truncated top-N lists is not the global top-N: an entity whose in-degree and out-degree both fall outside their respective lists never reaches the sort, however high its undirected degree is. The max_nodes cutoff normally lands in the degree-1/degree-2 band, so an entity with one in-edge and one out-edge outside both lists is an ordinary shape for a large graph, not a pathological one.

Edge documents now carry endpoints, a multi-valued keyword holding both ids, so a single terms aggregation returns one bucket per entity holding true undirected degree. The candidate set the ranking sorts is then the global top by degree.

Of the two storage shapes proposed in the issue, this takes the endpoints field rather than materializing per-node degree onto the node document. Materialized degree would have to be maintained on every edge write and delete, adding a hot-path cost and a correctness surface that the field approach does not need.

Related Issues

Closes #3613. Follows #3593, which changed only how the existing ranking broke ties.

Changes Made

endpoints keyword field on the edges mapping, written by both upsert_edge and upsert_edges_batch. A self-loop stores a single value, which also makes the ranking agree with node_degree: that method counts the loop's one document once, while the pair of aggregations counted it on both the source and the target side.
_ensure_edge_endpoints_ready lands the mapping and backfills existing indexes with a scripted update_by_query, guarded by an index _meta flag so a completed run costs one mapping read per startup rather than a rescan. The same put_mapping + value-backfill pattern as _ensure_scheduling_tiebreaker_ready.
_degree_map_from_edge_index is the single ranking source for both _get_knowledge_graph_all and get_popular_labels, which ranked from the same two aggregations (checklist item 4 in the issue).
The aggregation sets an explicit shard_size and logs when the response still reports a non-zero doc_count_error_upper_bound.
BaseGraphStorage.get_knowledge_graph contract updated.

The backfill degrades rather than failing startup

Worth a look during review, since it differs from the canonical-edge-id migration next door. That one is fail-fast because it has no correct fallback. This one does: the legacy aggregations are exactly the behaviour that shipped, so a cluster that cannot complete the backfill (permissions, a transport error, a large index mid-reindex) keeps serving the approximate ranking and retries on the next startup, rather than refusing to start over a ranking-quality issue. The _meta flag is set only after a recount confirms no documents still lack the field, so a partial backfill cannot be mistaken for a complete one.

On the second approximation

Terms aggregations are count-approximate across shards independently of the truncation above. Degree is exact at the single-shard default (OPENSEARCH_NUMBER_OF_SHARDS=1). On a multi-shard index this sets an explicit shard_size to shrink the error and logs the residual bound when the cluster reports one, rather than hiding it. Removing it entirely would need a composite aggregation paging every term, which cannot order by count. The contract docstring says this rather than claiming the caveat is fully retired (checklist item 5).

Checklist

Changes tested locally
Code reviewed
Documentation updated (if necessary)
Unit tests added (if applicable)

Additional Notes

New tests/kg/opensearch_impl/test_opensearch_degree_ranking.py uses a stub with faithful terms semantics (buckets ordered by doc_count descending then key ascending, only the top size returned) so one edge fixture drives both aggregation shapes. It pins the issue's minimal case in both directions: the legacy union omits A, the endpoints aggregation returns it with its true degree 2. Write-path, backfill-completion, partial-backfill and degradation-on-error cases are covered too.

Thirteen existing stubs in test_opensearch_storage.py hard-coded the two-aggregation response shape and were updated to answer either shape from the same described graph.

tests/kg/opensearch_impl passes (375) and ruff check . is clean. The wider tests/kg run shows 11 failures on my machine that are Windows-only and predate this branch (module 'signal' has no attribute 'SIGKILL' in the lock and reservation tests); none of those files touch OpenSearch.

…te set Degree came from two terms aggregations on the edge index, one per endpoint field, each capped at its own size. An entity whose in-degree and out-degree both fell outside their respective top-N never reached the ranking however high its undirected degree was. That is not a pathological shape: the max_nodes cutoff normally lands in the degree-1/degree-2 band, so an entity with one in-edge and one out-edge falling outside both lists is ordinary for a large graph. Edge documents now carry `endpoints`, a multi-valued keyword holding both ids, so one terms aggregation returns one bucket per entity with its true undirected degree. A self-loop stores a single value, which also makes the ranking agree with node_degree, where the loop's one document is counted once. Existing indexes get the mapping and a scripted backfill at startup, guarded by an index _meta flag so a completed run costs one mapping read rather than a rescan. Unlike the canonical-edge-id migration next door, this one degrades instead of failing startup: the legacy aggregations are exactly what shipped before, so a cluster that cannot complete the backfill keeps serving the approximate ranking and retries on the next startup. get_popular_labels ranked from the same two aggregations and shares the new path. The aggregation now sets an explicit shard_size and logs when the response still reports a non-zero doc_count_error_upper_bound, so residual multi-shard count error is surfaced rather than hidden. Closes HKUDS#3613.

danielaskdd · 2026-08-23T21:39:33Z

@codex review

chatgpt-codex-connector · 2026-08-23T21:42:10Z

Codex Review: Didn't find any major issues. Delightful!

Reviewed commit: e1d4c57ba1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

danielaskdd · 2026-08-24T05:55:18Z

Reviewed this branch merged onto current main (no conflicts): tests/kg/opensearch_impl passes (375) and ruff check . / ruff format --check are clean. The approach looks right — I walked every edge-document write path and only upsert_edge and upsert_edges_batch write at runtime, both of which now set endpoints, while the migration-time writes (_merge_into_canonical_edge, the canonical-id bulk create) all run before _ensure_edge_endpoints_ready in initialize(), so the backfill covers them. The _meta flag being set only after a recount confirms zero remaining docs is the right gate, and the fallback asking each side for size keeps the legacy pool no smaller than what shipped.

Three things to fix before this merges.

1. `endpoints` leaks into the API/WebUI edge properties

_construct_graph_edge filters internal fields out of KnowledgeGraphEdge.properties, but the new field is not in the exclusion tuple:

properties={
    k: v
    for k, v in edge.items()
    if k not in ("_id", "source_node_id", "target_node_id", "relationship", "source_ids")
},

Confirmed against this branch:

doc = {"source_node_id": "A", "target_node_id": "B", "endpoints": ["A", "B"],
       "relationship": "r", "weight": 1.0, "description": "d", "source_ids": ["c1"]}
_construct_graph_edge("edge-1", doc).properties
# {'endpoints': ['A', 'B'], 'weight': 1.0, 'description': 'd'}

So every edge in a /graphs response now carries a redundant endpoints duplicating the top-level source/target, and it shows up in the WebUI's edge property panel. Not a data-correctness problem — upsert_edge recomputes the field on every write, so a round-trip through an edit cannot corrupt it — but keeping storage-internal fields out of the response is exactly what that exclusion list is for. Please add _EDGE_ENDPOINTS_FIELD to it, with a test asserting it is absent from the returned properties.

2. `_edge_endpoints_ready` is not persisted on a freshly created index

_create_indices_if_not_exist sets the in-memory flag when it creates the edges index, but the create body carries no _meta, so the promise is never written down. The next startup re-reads the mapping, finds no _EDGE_ENDPOINTS_META_FLAG, and runs the missing-doc count again — on an index that was created from the current mapping and can never have a doc without the field. Cheap, but it means the steady-state cost the docstring advertises ("one mapping read per startup") is only reached on the second startup, and the in-memory flag and the persisted one can disagree for the whole lifetime of the process that created the index.

Please stamp "_meta": {_EDGE_ENDPOINTS_META_FLAG: True} into the indices.create body alongside the mappings, so the flag is durable from the moment the index exists. (The canonical-edge-id migration has the same gap today, so if you would rather keep the two consistent, say so and we can track it separately — but the new field is the one this PR's ranking depends on.)

3. `test_edges_index_mapping_declares_endpoints_as_keyword` asserts on source text

source = inspect.getsource(OpenSearchGraphStorage._create_indices_if_not_exist)
assert '_EDGE_ENDPOINTS_FIELD: {"type": "keyword"}' in source

This passes or fails on formatting rather than behaviour: renaming the constant, reordering the dict, or a ruff format reflow breaks it while the mapping stays correct, and conversely it would keep passing if the literal moved into a branch that never executes. The property it is defending is real and worth pinning (a dynamic: true index would map the array as text with a .keyword subfield, which cannot be aggregated under the bare name) — please pin it on the request the storage actually sends: call _create_indices_if_not_exist against a mocked client whose indices.exists returns False, then assert indices.create's body has mappings.properties[_EDGE_ENDPOINTS_FIELD] == {"type": "keyword"}. That also covers fix #2 in the same assertion, since the _meta flag would live in that body.

Everything else I looked at is fine, including some things worth recording as deliberate rather than missed: self-loops now counting once (agrees with node_degree, as the PR says), the backfill degrading instead of failing startup, and a rolling deploy where an old-version process keeps writing edges after the flag is set — that one only costs ranking quality, and the canonical-migration docstring already states the same assumption. One non-blocking note: update_by_query(wait_for_completion=True) on a very large legacy edge index can exceed the client timeout, which lands in the OpenSearchException handler and degrades correctly, but unlike the canonical migration next door there is no progress logging while it runs, so an operator watching a long backfill sees nothing between the "Backfilling N edge docs" line and the outcome.

Generated by Claude Code

…creation Review follow-ups on the endpoints degree ranking. `endpoints` duplicates the top-level source/target and is storage-internal, so exclude it from `KnowledgeGraphEdge.properties` alongside the other fields `_construct_graph_edge` already filters, instead of surfacing it on every edge in a `/graphs` response and in the WebUI edge panel. A freshly created edges index set the in-memory readiness flag but wrote no `_meta`, so the next startup recounted missing-field docs on an index that cannot hold one. Stamp the flag into the `indices.create` body, making it durable from the moment the index exists. The mapping test asserted on the source text of `_create_indices_if_not_exist`, so a rename or a reflow broke it while the mapping stayed correct, and a literal moved into dead code would still pass. Assert on the request the storage sends instead, which covers the `_meta` stamp in the same body.

YashvantHange · 2026-08-29T17:50:17Z

Thanks — all three fixed in 144ccf9.

1. endpoints in the edge properties. Added _EDGE_ENDPOINTS_FIELD to the exclusion tuple in _construct_graph_edge. That is the only KnowledgeGraphEdge construction site in the file, so it covers every /graphs response and the WebUI panel.

I deliberately left get_edge alone. It returns the raw _source including _id, source_node_id, target_node_id and source_ids, and its callers are internal (operate.py, utils_graph.py) rather than the API surface. Filtering there would be a different decision about what the storage-level read contract is, not an extension of this one — say the word if you would rather it went too.

2. _meta on a freshly created index. Stamped "_meta": {_EDGE_ENDPOINTS_META_FLAG: True} into the indices.create body, so the flag is durable from the moment the index exists and the in-memory and persisted views cannot disagree. The comment explaining why an index created from the current mapping can never hold a doc without the field moved onto the _meta line, where it now justifies the stamp rather than a bare assignment.

I checked it cannot clobber the canonical-edge-id flag on the same index: that migration writes {**meta, _EDGE_ID_CANONICAL_META_FLAG: True}, a spread-merge, so the two coexist whichever order they land in. I left the canonical migration's own gap alone as you suggested, since it is the same shape but not what this PR's ranking depends on.

3. The source-text assertion. Replaced with test_created_edges_index_maps_endpoints_and_stamps_the_flag, which drives _create_indices_if_not_exist against a mocked client whose indices.exists returns False and asserts on the indices.create body — both mappings.properties[_EDGE_ENDPOINTS_FIELD] == {"type": "keyword"} and the _meta flag, as you suggested, so fix 2 is pinned in the same request. Added test_graph_edge_properties_omit_the_endpoints_field for the first point. Both fail against the pre-fix source.

On the progress-logging note. I left it. That backfill is a single update_by_query(wait_for_completion=True) — one blocking request — so logging progress would mean moving to the async task API and polling it, which adds a failure surface to the one path whose whole design point is that it degrades quietly. The canonical migration next door can log per batch because it runs its own scroll-and-bulk loop. Happy to revisit if you would rather have the progress than the simplicity.

danielaskdd · 2026-08-30T00:09:24Z

@codex review

chatgpt-codex-connector · 2026-08-30T00:09:39Z

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-01T05:45:30.675753Z	`d9e6313`	Manual request

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 144ccf93f3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

…e flag The index _meta flag was read as a durable guarantee that every edge doc carries `endpoints`. It cannot be one: nothing fences writes against an old-version process, so during a rolling deploy a worker on the previous release can index an edge without the field after this one has stamped the flag. Every later startup then returned on the flag alone, leaving that edge outside the degree aggregation permanently, with no operation short of clearing the flag by hand to bring it back. The flag now records that a backfill completed, and each startup that finds an existing index revalidates coverage with a `terminate_after` count before trusting it; a dirty index re-runs the backfill and re-stamps. That closes the window rather than eliminating it -- an edge written by an old worker after the last startup stays uncovered until the next one -- which is as far as this goes without a write fence, and it costs ranking quality alone. A failed revalidation falls back to the flag rather than below it. Degrading a healthy migrated index to the approximate ranking for the life of the process would make this check the likeliest cause of the degradation it exists to prevent. The missing-docs query is now defined once and shared by the probe and the backfill, so the two cannot drift on what "missing" means.

YashvantHange · 2026-08-30T07:47:27Z

Fixed in bd05a2e.

The report is right, and it lands on something you and I both noted last round as an accepted cost — an old-version worker writing edges after the flag is set. What neither of us followed through on is that the flag makes the damage permanent: without a coverage check, no later startup ever looks again, so that edge stays outside the aggregation until someone clears the mapping flag by hand. A window that heals on the next restart and one that never heals are different bugs.

So the flag no longer means "the field is universal", only "a backfill completed". Every startup that finds an existing index revalidates coverage with a terminate_after: 1 count before trusting it; a dirty index re-runs the backfill and re-stamps. The flag still saves the update_by_query and the _meta write on the clean path — not the query that asks, and the docstring now says so rather than promising a rescan-free startup it no longer delivers.

Two things worth your eye:

The guard must not degrade what it guards. A search rejection or a transport error from the probe would otherwise fall into the existing OpenSearchException handler and leave _edge_endpoints_ready false for the whole process — turning the coverage check into the likeliest cause of the approximate ranking, on the large population of indexes that are perfectly clean. The probe has its own handler and falls back to the flag, which is exactly the behaviour that shipped before this commit.

The re-stamp is now reachable, and _meta is shared. A flagged index re-running the backfill writes _meta again, and a real one carries the canonical-edge-id flag in the same object. It is a spread-merge, so it preserves it, but nothing pinned that before — clobbering it would silently re-trigger that whole migration on every startup. The migration-stub helper now seeds both flags and the test asserts both survive.

The window is narrowed, not closed: an edge written by an old-version worker after the last startup stays uncovered until the next one, and there is no write fence available to do better. That residual is stated in the docstring rather than left implicit, and it costs ranking quality alone — never a wrong edge, only an entity ranked from a degree short of its real one. The startup that creates the index is exempt from the probe and safely so, since it holds the data-init lock and the index it stamps is empty; that is now written down too.

Four new tests: the probe's shape and bound, a flagged-but-dirty index being backfilled again with the sibling flag preserved, a repair that does not finish keeping the legacy ranking, and probe failure falling back to the flag. Three fail against the pre-fix source.

danielaskdd · 2026-08-30T20:03:05Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bd05a2efbe

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

The coverage-revalidation probe added last round guarded against a rolling-deploy writer dirtying a flagged index, but its own failure path defeated that guard: a search rejection or transport error on the probe set _edge_endpoints_ready=True outright, trusting the flag unverified on exactly the index most likely to actually be dirty. Fall through to the backfill path instead. It costs one recount: a transient failure on a genuinely clean index re-stamps the flag in the same startup, a genuinely dirty index gets repaired in the same startup instead of waiting for the next one, and a store that is still unreachable degrades to the legacy ranking through the same outer handler every other failure in this method already falls back to.

YashvantHange · 2026-08-31T05:18:13Z

Confirmed and fixed in d9e6313.

The finding is right, and reproducing it shows exactly why: with the index flagged and the probe (client.count) raising OpenSearchException, the exception handler set _edge_endpoints_ready = True unconditionally — trusting the flag with zero verification, on precisely the index a rolling deploy is most likely to have dirtied since it was stamped. That's the one branch in this method that broke its own stated rule; every other failure path here (the outer handler, an incomplete backfill) degrades to the legacy ranking rather than claiming an exactness it can't back up.

Fixed by falling through to the backfill path instead of returning early on probe failure. That path already knows how to answer the question the probe couldn't:

If the failure was transient and the index is actually clean, the recount comes back zero-hit and the flag is re-stamped in the same startup — no need to wait for a retry.
If the index is genuinely dirty, the same recount finds the backlog and repairs it in the same startup, instead of leaving it degraded until the next one.
If the store is still unreachable, the recount fails too, and the outer except OpenSearchException handler already in this method degrades to the legacy ranking — the same safe default every other failure here uses.

Three new tests pin this: probe failure never sets the ready flag on its own, a transient failure on a clean index self-heals within the same startup, and a transient failure on a genuinely dirty index gets repaired within the same startup. Two of the three fail against the pre-fix code (confirmed by reverting locally and re-running) — the third (self-heal on a clean index) happened to pass either way since a clean recount is harmless regardless of what the probe-failure branch does with the flag, so it's there to pin the good path going forward rather than to prove the bug.

YashvantHange · 2026-08-31T05:20:49Z

@danielaskdd Fixed the revalidation-flag finding in d9e6313 (details above). Could you re-review when you have a moment?

danielaskdd · 2026-09-01T05:42:01Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d9e6313d29

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-09-01T05:45:28Z

+                "must": [
+                    {"exists": {"field": "source_node_id"}},
+                    {"exists": {"field": "target_node_id"}},
+                ],


Backfill the surviving endpoint of partial edge documents

When a legacy or malformed edge contains only source_node_id or only target_node_id, this must clause excludes it from both the backfill and readiness checks, allowing the index to be marked ready while that document has no endpoints value. The new aggregation then ignores the edge entirely, even though the previous per-field aggregation and node_degree() count its surviving endpoint, so wildcard and popular-label rankings can silently demote that valid entity; backfill the available endpoint or keep the legacy path while such documents remain.

Useful? React with 👍 / 👎.

Uh oh!

Conversation

YashvantHange commented Aug 23, 2026

Description

Related Issues

Changes Made

The backfill degrades rather than failing startup

On the second approximation

Checklist

Additional Notes

Uh oh!

danielaskdd commented Aug 23, 2026

Uh oh!

chatgpt-codex-connector Bot commented Aug 23, 2026

Uh oh!

danielaskdd commented Aug 24, 2026

1. endpoints leaks into the API/WebUI edge properties

2. _edge_endpoints_ready is not persisted on a freshly created index

3. test_edges_index_mapping_declares_endpoints_as_keyword asserts on source text

Uh oh!

YashvantHange commented Aug 29, 2026

Uh oh!

danielaskdd commented Aug 30, 2026

Uh oh!

chatgpt-codex-connector Bot commented Aug 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

YashvantHange commented Aug 30, 2026

Uh oh!

danielaskdd commented Aug 30, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

YashvantHange commented Aug 31, 2026

Uh oh!

YashvantHange commented Aug 31, 2026

Uh oh!

danielaskdd commented Sep 1, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Sep 1, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

1. `endpoints` leaks into the API/WebUI edge properties

2. `_edge_endpoints_ready` is not persisted on a freshly created index

3. `test_edges_index_mapping_declares_endpoints_as_keyword` asserts on source text

chatgpt-codex-connector Bot commented Aug 30, 2026 •

edited

Loading