mirror of
https://github.com/tiennm99/DocsGPT.git
synced 2026-10-17 12:15:43 +00:00
fix(graphrag): review fixes — replay-safe writes, strict count, zero weights
Three findings from review, all on code this branch introduced.
apply_chunk was not replay-safe. commit() can report connection loss *after*
Postgres committed, and the reconnect retry then replays the write: _upsert_node
bumps doc_freq a second time and _add_edge inserts another row, since
graph_edges has no uniqueness constraint for a logical edge. The chunk's
graph_ingest_progress row is now written in the same transaction as the rows it
describes, and a replay that finds it already "done" returns (0, 0) without
touching the graph. Extraction drops its separate mark_chunk("done"): the
checkpoint and the graph can no longer disagree.
count_nodes swallows every query failure and answers 0, so extraction's
"fall back to the write count" handler could never run — a failed count after a
successful build reported an empty graph. count_nodes grows a strict mode that
re-raises; retrieval keeps the swallow, which is what routes a source to
ClassicRAG.
A zero edge weight was read as a full-strength link: `or 1.0` rewrote an
explicit 0 before the <= 0 filter. Only missing and null weights default now.
The same coercion sat in _ppr_scores, where it would have kept the ranker's
rule unreachable from the product path, so it is fixed there too.
This commit is contained in:
1 parent
9e9f130ed0
commit
3f774d813c
6 files changed
+274
-9
No files matched your search
@@ -624,6 +624,42 @@ class TestSummaryNodeCount:
|
||||
store.delete_by_source(source_id)
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
class TestSummaryCountFailure:
|
||||
"""A broken count query must not be reported as an empty graph."""
|
||||
|
||||
def test_a_failed_count_reports_the_write_count(
|
||||
self, monkeypatch, stub_embedding
|
||||
):
|
||||
from unittest.mock import MagicMock
|
||||
|
||||
store = MagicMock(name="GraphStore")
|
||||
store.pending_chunks.return_value = ["c1"]
|
||||
store.apply_chunk.return_value = (2, 1)
|
||||
store.count_nodes.side_effect = RuntimeError("count query failed")
|
||||
monkeypatch.setattr(
|
||||
"docsgpt.graphrag.store.GraphStore", lambda *a, **k: store
|
||||
)
|
||||
_install_stub_llm(
|
||||
monkeypatch,
|
||||
_StubLLM([_extraction_json([{"name": "Ada"}], [])]),
|
||||
)
|
||||
|
||||
summary = extract_graph_for_source(
|
||||
str(uuid.uuid4()),
|
||||
user="owner-1",
|
||||
chunks=[_chunk("c1", "Ada.")],
|
||||
config=SourceConfig(),
|
||||
request_id="req-1",
|
||||
)
|
||||
|
||||
# Falls back to what was actually written, not to zero.
|
||||
assert summary["nodes"] == 2
|
||||
# And it asked for a count that raises rather than one that returns 0,
|
||||
# or the fallback above could never run.
|
||||
assert store.count_nodes.call_args.kwargs.get("strict") is True
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
class TestParsing:
|
||||
def test_parses_embedded_json(self):
|
||||
|
||||
@@ -1002,3 +1002,115 @@ class TestWritesSurviveALostConnection:
|
||||
|
||||
assert handed == [broken]
|
||||
broken.rollback.assert_called_once()
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
class TestCountNodesFailureModes:
|
||||
"""Retrieval wants a swallowed count; extraction wants to hear about it."""
|
||||
|
||||
def _store_with_failing_cursor(self):
|
||||
store = GraphStore.__new__(GraphStore)
|
||||
store._tables_ensured = True
|
||||
cursor = MagicMock()
|
||||
cursor.execute.side_effect = RuntimeError("relation does not exist")
|
||||
conn = MagicMock()
|
||||
conn.cursor.return_value = cursor
|
||||
store._connection = conn
|
||||
store._get_connection = lambda: conn
|
||||
return store
|
||||
|
||||
def test_default_reports_zero_to_drive_the_classic_fallback(self):
|
||||
store = self._store_with_failing_cursor()
|
||||
|
||||
assert store.count_nodes(str(uuid.uuid4())) == 0
|
||||
|
||||
def test_strict_surfaces_the_query_failure(self):
|
||||
"""A caller reporting graph size must not read a broken query as empty."""
|
||||
store = self._store_with_failing_cursor()
|
||||
|
||||
with pytest.raises(RuntimeError):
|
||||
store.count_nodes(str(uuid.uuid4()), strict=True)
|
||||
|
||||
|
||||
@pytest.mark.integration
|
||||
class TestApplyChunkIsReplaySafe:
|
||||
"""A retry after an ambiguous commit must not apply a chunk twice.
|
||||
|
||||
``_write_with_reconnect`` replays the write when the connection dies, and
|
||||
``commit()`` itself can raise connection loss *after* the server committed.
|
||||
Replaying then bumps ``doc_freq`` a second time and inserts a second
|
||||
logical edge (``graph_edges`` has no uniqueness constraint), so the chunk's
|
||||
own progress row is written in the same transaction and short-circuits it.
|
||||
"""
|
||||
|
||||
@pytest.fixture
|
||||
def store(self, postgresql):
|
||||
store = GraphStore(connection_string=_ephemeral_dsn(postgresql.info))
|
||||
try:
|
||||
store._ensure_tables()
|
||||
except Exception as exc:
|
||||
pytest.skip(f"pgvector extension unavailable: {exc}")
|
||||
yield store
|
||||
store.close()
|
||||
|
||||
def test_a_replayed_chunk_is_not_applied_twice(self, store):
|
||||
source_id = str(uuid.uuid4())
|
||||
entities = [
|
||||
{
|
||||
"name": "Ada",
|
||||
"normalized_name": "ada",
|
||||
"type": "person",
|
||||
"description": "d",
|
||||
}
|
||||
]
|
||||
relationships = [
|
||||
{
|
||||
"source": "Ada",
|
||||
"target": "Engine",
|
||||
"type": "worked_on",
|
||||
"description": "x",
|
||||
"weight": 2.0,
|
||||
}
|
||||
]
|
||||
embeddings = {"ada": _embedding(0.1), "engine": _embedding(0.2)}
|
||||
try:
|
||||
first = store.apply_chunk(
|
||||
source_id, "c1", entities, relationships, embeddings
|
||||
)
|
||||
replay = store.apply_chunk(
|
||||
source_id, "c1", entities, relationships, embeddings
|
||||
)
|
||||
|
||||
assert first == (1, 1)
|
||||
assert replay == (0, 0)
|
||||
node = store.get_node_by_normalized(source_id, "ada")
|
||||
assert node["doc_freq"] == 1
|
||||
overview = store.get_graph_overview(source_id)
|
||||
assert len(overview["edges"]) == 1
|
||||
# The write records its own progress, so the caller's checkpoint
|
||||
# and the rows it describes commit together.
|
||||
assert store.get_progress(source_id)["c1"] == "done"
|
||||
finally:
|
||||
store.delete_by_source(source_id)
|
||||
|
||||
def test_a_different_chunk_still_applies(self, store):
|
||||
"""The guard is per chunk, not a blanket 'already saw this source'."""
|
||||
source_id = str(uuid.uuid4())
|
||||
entities = [
|
||||
{
|
||||
"name": "Ada",
|
||||
"normalized_name": "ada",
|
||||
"type": "person",
|
||||
"description": "d",
|
||||
}
|
||||
]
|
||||
embeddings = {"ada": _embedding(0.1)}
|
||||
try:
|
||||
store.apply_chunk(source_id, "c1", entities, [], embeddings)
|
||||
second = store.apply_chunk(source_id, "c2", entities, [], embeddings)
|
||||
|
||||
assert second == (1, 0)
|
||||
node = store.get_node_by_normalized(source_id, "ada")
|
||||
assert node["doc_freq"] == 2
|
||||
finally:
|
||||
store.delete_by_source(source_id)
|
||||
Reference in new issue
Block a user