Repository navigation
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 35 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 673ece0a1a
ℹ️ 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".
| return ( | ||
| f"MATCH (a:{from_label} {{{from_key}: row.from_val}}), " | ||
| f"(b:{to_label} {{{to_key}: row.to_val}})\n" | ||
| f"CREATE (a)-[r:{rel_type}]->(b)\n" |
There was a problem hiding this comment.
When two ingestors concurrently write the same previously absent site-less endpoint pair, both update statements can observe no matching edge and then both execute this unconditional CREATE, leaving duplicate site-less relationships. The two statements run as separate auto-commit transactions, and _conn_lock only serializes one ingestor instance, so it does not prevent this race against another writer to the shared graph; preserve the prior idempotence with an atomic database operation or cross-ingestor serialization.
Useful? React with 👍 / 👎.
 [High impact] The PR appears safe to merge; no concrete blocking bugs were found. SummaryThis PR keeps relationship rows without a site separate from located edges. Trace confirmation now writes the full runtime observation directly onto existing static edges.
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Relationship row] --> B{Has site keys?}
B -->|Yes| C[Merge by endpoints and site]
B -->|No| D[Update edge without site keys]
D --> E{Edge found?}
E -->|No| F[Create one edge for the pair]
E -->|Yes| G[Keep located edges unchanged]
H[Runtime observation] --> I{Static pair exists?}
I -->|Yes| J[Write observation onto static edges]
I -->|No| A
Reviews (1) · Last reviewed commit: "Update codebase_rag/services/graph_servi..." · Reviewed by Greptile |
vitali87
changed the title
|
Summary
MERGEon its endpoints alone. That matched every located edge of the pair and set the row's properties,resolutionamong them, on each one. Such rows come from returned and factory closures, the callable-parameter fallback, libclang macro uses, Roslyn facts and inferred C# namespace imports. A row like that now lands on the pair's site-less edge, created when there is none, which is howindex.binalready keeps it.resolutionrather than reading asexact, socgr renameno longer rewrites a call on an unrelated object. The graph andindex.binalso hold the same CALLS edges._confirm_staticset onlyresolutionthrough its own query, and the rest of the observation (dynamic,dynamic_call_count,dynamic_stale, ...) rode on a site-less row that the merge stamped onto every static site. The confirm query now sets the whole observation on every static site itself, and a confirmed pair writes no site-less row. Runtime-only edges are written as before. What [Bug]: any incremental re-parse of a file silently discards its trace-derived (dynamic) CALLS edges; dead-code flips back after a comment edit #2429 specifies is unchanged: a confirmed pair carries the observation on each of its sites, with no site-less duplicate.Demo
The fixture from the issue:
make(obj)callsobj.inner()on an untyped object and returns its owninner. It is indexed into an embedded graph, first importingcodebase_ragfrommain, then from this branch. Onmainthe guessed line-4 call reads asexactand rename would rewrite it. Here it staysheuristic, and the returned-closure edge stands alone with no site.That edge is why rename now refuses this fixture: "1 graph-known site(s) carry no rewrite location". Rename already refused a closure that is only returned for the same reason. A closure that is both called and returned went through on
mainonly because its site-less edge was merged into the located call, which is this bug. It is now refused the same way. The refusal comes from rename counting a reachability edge as an occurrence of the name, which #3531 describes.How
graph_service: site-less rows of a site-keyed type take a two-step write. The first step updates the pair's site-less edge (WHEREevery site keyIS NULL). The second creates one for the pairs that had none, one edge per pair however many rows, with later rows' properties winning as repeated writes to one edge do. Rows still count once each in the flush summary. The Rust extension does not take this case, so these rows run Cypher; every other write is unchanged.build_trace_confirm_calls_querysets each property inTRACE_CONFIRMED_PROPSwithcoalesce($key, r.key), so a key the observation lacks keeps the edge's value. A test pins the keys both trace writers produce to that list.write_trace_edgereturns after confirming.docs/architecture/graph-schema.mdno longer say site-less rows collapse onto located edges.Type of Change
Related Issues
Closes #2879. The embedded-engine tests are in #3530, which is stacked on this PR.
Test Plan
make test-paralleloruv run pytest -n auto -m "not integration")make test-integration, requires Docker)New tests here:
integration/test_siteless_rel_rows_memgraph.py(Memgraph 3.3.0): fails with the routing to the new path disabled (the located edges readexact), passes with it.test_node_relationship_coverage.py::test_a_row_without_a_site_flushes_in_two_queries: every site-keyed type writes a site-less row as an update, then a create, and never as a MERGE.test_dynamic_trace_ingest.py::test_the_confirm_query_names_every_key_an_observation_carries: fails when a key is dropped fromTRACE_CONFIRMED_PROPS.Negative tests:
heuristicedge keepsheuristicafter a site-lessexactrow for its pair, and two site-less rows make one edge, not two.Existing tests changed:
test_graph_service_calls_failure_logging.py,test_failed_rel_rows_logged.py,test_links_after_targets.py,test_graph_service_node_retry.py,test_graph_service.py::TestCreateMode::test_flush_relationships_uses_merge_query_by_defaultandtest_node_relationship_coverage.py::test_each_relationship_type_can_be_flushed: their rows now carry a site, as real rows of those types do. Their fakes stand in for the MERGE query's count, and rows without a site have their own path, covered by the new tests.test_dynamic_trace_ingest.py,test_trace_carry.py,test_edge_resolution.pyandtest_trace_confirm_labelled.py: a confirmed pair's observation is now read from the confirm write's parameters, and each asserts that no relationship row was written for that pair.Runs, on this stack's top before the split:
main.Mutations, each failing the test meant to catch it:
test_trace_edges_survive_reparse_e2e.py.TRACE_CONFIRMED_PROPS.Checklist
!in the title and its release notes inchanges/breaking/<PR number>.md(see Breaking changes)make pre-commit)# type: ignore,cast(),Any, orobjecttype hints