Skip to content

perf(graph): batch edge/node writes and bare-endpoint resolution - #858

Closed
skyhancloud wants to merge 2 commits into
tirth8205:stagingfrom
skyhancloud:fix/721-batched-edge-writes
Closed

skyhancloud wants to merge 2 commits into
tirth8205:stagingfrom
skyhancloud:fix/721-batched-edge-writes

Conversation

@skyhancloud

Copy link
Copy Markdown
Contributor

Fixes #721

Root cause

GraphStore opens its SQLite connection in autocommit mode
(isolation_level=None), but three write paths issued one SQL statement per
row without opening an explicit transaction:

  1. store_file_nodes_edges / store_file_batch called upsert_node /
    upsert_edge once per symbol — a SELECT + INSERT (or SELECT + UPDATE +
    last_insert_rowid) round trip per row. At 142k nodes / 1.14M edges that
    is ~3.4M statements, each maintaining 9 indexes on a multi-GB database.

  2. _resolve_bare_endpoints ran conn.execute(update_sql, ...) per bare edge
    and only called commit() afterwards. Because the connection is in
    autocommit mode, every UPDATE committed on its own — one WAL commit +
    fsync per row. With hundreds of thousands of bare CALLS / TESTED_BY
    endpoints this is effectively a hang: the exact "freezes at
    conn.execute(update_sql, ...)" report in [Bug]: Hang at "INFO: Spring event resolver: indexed 0 events and emitted 0 CALLS edges" #721.

  3. Signature computation looped update_node_signature per node — one
    autocommitted UPDATE per node.

resolve_cpp_scoped_call_targets had the same per-row autocommit pattern for
its call-target and TESTED_BY-mirror updates.

Changes

  • store_file_nodes_edges / store_file_batch now delete the file's rows and
    re-insert everything with executemany inside the existing single
    transaction. Duplicate call sites (same kind/source/target/file/line) are
    collapsed in Python with the previous last-write-wins semantics, so a plain
    INSERT batch is safe; nodes keep their ON CONFLICT(qualified_name) upsert.
  • _resolve_bare_endpoints collects every mutation and applies it with a
    chunked executemany inside one BEGIN IMMEDIATE transaction instead of
    one autocommitted UPDATE per edge. The candidate scan now streams instead of
    materialising the full edge set, and an EXISTS guard short-circuits the
    no-op case without a full fetch.
  • resolve_cpp_scoped_call_targets gets the same single-transaction batching
    for its call-target and TESTED_BY-mirror updates.
  • Signature computation (postprocessing and MCP build paths) batches via the
    new GraphStore.update_node_signatures().
  • New regression test
    test_store_file_nodes_edges_collapses_duplicate_call_sites.

No schema change. Resolution counts and final DB state are identical to the
previous per-row paths (verified old-vs-new on identical synthetic databases).

Verification

Full build: 4889 files, 149803 nodes, 1161152 edges (postprocess=full)
Resolved 77934 evidence-backed bare CALLS targets
Resolved 33112 evidence-backed bare TESTED_BY sources
FTS index rebuilt: 142649 rows indexed

Notes

  • Large graphs will still be I/O-bound on the candidate scan (it reads every
    matching edge), but the write side is now one transaction per stage, not one
    commit per row.
  • _UPDATE_BATCH = 50_000 chunks executemany to bound peak memory while
    keeping a single commit/checkpoint.

Fixes the build/postprocess hang on large graphs (issue tirth8205#721).

Root cause: GraphStore opens the SQLite connection in autocommit mode
(isolation_level=None), but several write paths issued one UPDATE (or one
SELECT+INSERT round trip) per row without opening an explicit transaction.
On graphs with 10^5+ nodes and 10^6+ edges that means one WAL commit +
fsync per row and ~3 SQL round trips per edge, which can take tens of
minutes or appear permanently hung (142k nodes / 1.14M edges in the report).

Changes:
- store_file_nodes_edges / store_file_batch now delete the file's rows and
  re-insert everything with executemany in the existing single transaction.
  Duplicate call sites (same kind/source/target/file/line) are collapsed in
  Python with the previous last-write-wins semantics, so one plain INSERT
  batch is safe. Nodes keep their ON CONFLICT(qualified_name) upsert.
- _resolve_bare_endpoints collects every mutation and applies it with a
  chunked executemany inside one BEGIN IMMEDIATE transaction instead of
  one autocommitted UPDATE per edge; the candidate read now streams instead
  of materialising the full edge set, and an EXISTS guard short-circuits
  the no-op case without a full fetch.
- resolve_cpp_scoped_call_targets gets the same single-transaction
  batching for its call-target and TESTED_BY-mirror updates.
- Signature computation (postprocessing and MCP build paths) batches via a
  new GraphStore.update_node_signatures() instead of per-row updates.
- New regression test for duplicate call-site collapse in the batched store.

No schema change; result counts and post-state are identical to the
previous per-row paths (verified old-vs-new on identical synthetic DBs).
@tirth8205

Copy link
Copy Markdown
Owner

The batching itself looks right. I checked atomicity under mid-batch failure, duplicate call-site collapse, the ON CONFLICT path against stale qualified-name rows, chunk boundaries with _UPDATE_BATCH forced small, unicode paths, and idempotent re-runs of the resolvers; everything matches the old per-row semantics and rolls back cleanly.

One thing blocks merge: the suite is red. tests/test_transactions.py line 84, test_rollback_on_failure_in_batch_ops, patches store.upsert_node to simulate a failure inside store_file_nodes_edges, but _replace_file_data no longer calls upsert_node, so the patch never fires and pytest fails with DID NOT RAISE. Please update that test to inject the failure into the new path, for example patch _replace_file_data or pass a node whose extra contains a non-JSON-serializable value, and keep the rollback assertions.

Minor: sync_tested_by in resolve_cpp_scoped_call_targets still returns changed_mirror but nothing reads it now; drop the dead bookkeeping.

Ruff and mypy are clean, and #835 and #861 both merge cleanly on top of this.

@skyhancloud

Copy link
Copy Markdown
Contributor Author

The batching itself looks right. I checked atomicity under mid-batch failure, duplicate call-site collapse, the ON CONFLICT path against stale qualified-name rows, chunk boundaries with _UPDATE_BATCH forced small, unicode paths, and idempotent re-runs of the resolvers; everything matches the old per-row semantics and rolls back cleanly.

One thing blocks merge: the suite is red. tests/test_transactions.py line 84, test_rollback_on_failure_in_batch_ops, patches store.upsert_node to simulate a failure inside store_file_nodes_edges, but _replace_file_data no longer calls upsert_node, so the patch never fires and pytest fails with DID NOT RAISE. Please update that test to inject the failure into the new path, for example patch _replace_file_data or pass a node whose extra contains a non-JSON-serializable value, and keep the rollback assertions.

Minor: sync_tested_by in resolve_cpp_scoped_call_targets still returns changed_mirror but nothing reads it now; drop the dead bookkeeping.

Ruff and mypy are clean, and #835 and #861 both merge cleanly on top of this.

Thanks. Fixed both points:

  • Rewrote test_rollback_on_failure_in_batch_ops. Went with your second suggestion, pass a node whose extra isn't JSON-serializable, so the failure happens inside _replace_file_data while it builds the batch. I also seed fail.py with a real node first, so the test actually proves the old data survives the failed overwrite (the old "fail.py has nothing" assertion would have passed even without a rollback).
  • Dropped the unused changed_mirror return in sync_tested_by.

Local run: tests/test_transactions.py 5 passed, tests/test_graph.py 71 passed, ruff clean on code_review_graph/. Should be good to go.

@tirth8205

Copy link
Copy Markdown
Owner

Integrated on integration/token-efficiency-hardening as 546d96a. Batched writes retain transaction rollback behaviour and reduce per-row SQL and commit overhead. Targeted graph and transaction checks passed, and the combined writer also populates the symbol column introduced by #942.

@tirth8205

Copy link
Copy Markdown
Owner

Merged into staging through the integration PR #988 (merge commit 3030ca8). Your change is carried by commit 546d96a, which keeps you as the author (Gökhan Bulut). It will be promoted staging → testing → main and released from there. Thank you for the contribution! Closing this PR since its content is now on staging.

@tirth8205 tirth8205 closed this Sep 15, 2026
Bakul2006 pushed a commit to Bakul2006/code-review-graph that referenced this pull request Sep 15, 2026
…are-endpoint resolution

Port the reviewed token-efficiency implementation with targeted regression coverage.

Source-PR: tirth8205#858
Maintainer corrections and scope extraction applied where needed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Hang at "INFO: Spring event resolver: indexed 0 events and emitted 0 CALLS edges"

2 participants