Repository navigation
fix(serve): stop a graph reload from stalling every MCP request - #5181
ayushcodes10 wants to merge 2 commits into
Conversation
_GraphContextCache.load() ran the whole rebuild, parsing graph.json and building its trigram index, inside the cache's single lock. That one lock covers the pinned default graph and every project graph, so while a changed graph.json was reloaded every request on the server waited, including requests for a different graph and requests that could have been answered from the graph already sitting in memory. On a large graph the rebuild alone can take upward of fifteen seconds. The rebuild now runs outside the lock, single flighted per resolved path so concurrent callers never start a second, redundant rebuild of the same path. A caller that already holds a stale entry for that path gets it back immediately while someone else's rebuild is in flight, rather than queuing behind it. A caller with no entry yet, the path's first ever load, has nothing stale to serve and waits for the one in flight build instead of racing a second one. Since the lock is held only for the stat and the small dict bookkeeping around it, a slow rebuild of one path no longer blocks a request for an unrelated one.
Covers a caller getting its stale entry back immediately while another thread's reload of the same path is in flight, a reload of one path never blocking a request for a different path, concurrent first time loads of a brand new path still single flighting to one build rather than each paying for their own, and a later call seeing the new graph once an in flight rebuild finishes.
|
Thanks for the pull request, @ayushcodes10. A maintainer will review it soon. Want to talk it through while it is in review? Come join us on our Discord server. For longer-form discussion there is also GitHub Discussions. A couple of things that speed up review: make sure the test suite passes on Python 3.10 and 3.13, and that the change keeps extraction deterministic. |
github-actions
Bot
added
the
contributor
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.
Not checked: tests were not run; formal verification proved 0 of 2 changed function(s) (2 not verified).
Formal verification. PR-changed functions: 0/2 verified (0 proven, 0 may-equivalent, 0 distinguished) · 2 not verified (2 vacuous).
Not verified on this run: \_\_init\_\_ (vacuous: never exercised), load (vacuous: never exercised).
Graphify review — findings
Fixes a server-wide stall in _GraphContextCache.load by moving the graph.json parse and trigram-index rebuild out of the cache lock, so a slow reload of one path no longer blocks requests for other paths. Rebuilds are single-flighted per (path, pinned): a caller holding a stale entry gets it back immediately while a rebuild runs, and a first-time caller waits on the one in-flight build instead of starting its own. The hand-off from the in-flight event to the stored entry is where this can go wrong, since waiters are released and the new entry is published in separate lock sections, and builds for distinct paths now run in parallel with eviction applied only after each finishes.
Worth a look
- Single-flight event is signalled before the built entry is stored, allowing redundant rebuilds and stale overwrites —
graphify/serve.py:233· Escalate · medium- NOT verified (no proof, no reproducing execution) — needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 849 functions depend on the 220 functions this change touches.
Health — grade B; 10 existing hotspot(s) in the area this change touches (pre-existing, not introduced here):
dispatch_command()— 8 callers, 149 callees (high)main()— 126 callers, 3 callees (high)_query_graph_text()— 53 callers, 7 callees (high)_score_query()— 24 callers, 8 callees (high)_shortest_path_text()— 25 callers, 7 callees (high)resolve_seed()— 30 callers, 5 callees (high)_resolve_path_endpoint()— 14 callers, 7 callees (high)_find_node_tiers()— 10 callers, 9 callees (high)- …and 2 more
Verification — 849 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 623 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
27 of 560 test file(s) selected (5%) via static blast radius.
tests/test_4874_jvm_resolution.py— impacttests/test_affected_cli.py— impacttests/test_affected_path_file_node.py— impacttests/test_affected_seed_reference_forms.py— impacttests/test_benchmark.py— impacttests/test_benchmark_raw_graph.py— impacttests/test_benchmark_real_query.py— impacttests/test_confidence_score_output.py— impacttests/test_edge_also_seen.py— impacttests/test_explain_same_file_tie.py— impacttests/test_extract.py— impacttests/test_file_label_disambiguation.py— impacttests/test_graph_context_cache_reload.py— impact, changed-testtests/test_mcp_graph_view_cache.py— impacttests/test_path_cli.py— impacttests/test_path_confidence.py— impacttests/test_path_type_member.py— impacttests/test_query_induced_edges.py— impacttests/test_query_mcp_direction.py— impacttests/test_query_names_its_graph.py— impacttests/test_query_seed_order.py— impacttests/test_query_seeds.py— impacttests/test_query_source_first.py— impacttests/test_serve.py— impacttests/test_serve_http.py— impacttests/test_terraform.py— impacttests/test_truncation_full_answer.py— impact
Selection is safe under the controlled-regression assumption; always-run tests + a periodic full run are the backstops. Advisory — it never changes the check verdict.
Docs that may be stale (advisory)
CHANGELOG.md§ 0.9.24 (2026-07-22) (lines 1289-1303): references changed symbolscallergraphify/skill-agents.md§ Step 9 - Save manifest, update cost tracker, clean up, and report (lines 639-749): references changed symbolsgatedgraphify/skill-amp.md§ Step 9 - Save manifest, update cost tracker, clean up, and report (lines 639-749): references changed symbolsgatedgraphify/skill-claw.md§ Step 9 - Save manifest, update cost tracker, clean up, and report (lines 642-752): references changed symbolsgatedgraphify/skill-codex.md§ Step 9 - Save manifest, update cost tracker, clean up, and report (lines 643-753): references changed symbolsgatedgraphify/skill-copilot.md§ Step 9 - Save manifest, update cost tracker, clean up, and report (lines 642-752): references changed symbolsgatedgraphify/skill-devin.md§ Part B - Semantic extraction (parallel subagents) (lines 240-439): references changed symbolscallergraphify/skill-droid.md§ Step 9 - Save manifest, update cost tracker, clean up, and report (lines 639-749): references changed symbolsgatedgraphify/skill-kilo.md§ Step 9 - Save manifest, update cost tracker, clean up, and report (lines 642-752): references changed symbolsgatedgraphify/skill-kiro.md§ Step 9 - Save manifest, update cost tracker, clean up, and report (lines 642-752): references changed symbolsgated
…and 10 more.
Formal verification
Could not verify: Could not verify \_\_init\_\_.
The verifier did not have enough to check \_\_init\_\_, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: no capturable inputs from the test suite; property tier: not verifiable: all 180 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly __init__ raised on the synthesized receiver args — names the real obstacle, not a sampling gap)
Could not verify: Could not verify load.
The verifier did not have enough to check load, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: no capturable inputs from the test suite; property tier: not verifiable: all 86 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly __init__ raised on the synthesized receiver args — names the real obstacle, not a sampling gap)
… into the schema note serve: collapse the two lock blocks in _GraphContextCache.load so the in-flight marker clears and waiters wake in the SAME lock block that stores the new entry. As landed (#5181) these ran in separate blocks, leaving a window where a woken waiter saw neither a stored entry nor an in-flight build and kicked off a second full rebuild, defeating the single-flight guarantee the fix exists to provide. On build failure the marker is still released via `except BaseException`. cache: extend the schema-45 note to cover the Oracle [NON]EDITIONABLE recovery (#5167); it rides the same 0.9.108 bump as the other SQL work. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Landed on v8 as abeb4b8 via authorship-preserving cherry-pick, released in graphify 0.9.108. Thanks @ayushcodes10. |
Fixes #5177.
Bug
graphify.servehot-reloadsgraph.jsonwhen its(mtime_ns, size)changes, but the whole reload (_load_graphplus building the trigram index) runs inside_GraphContextCache's single lock. There is one cache instance per server, so that one lock covers the pinned default graph and every project graph. While a changedgraph.jsonis parsed and its index rebuilt, every request on the server waits — including requests for a different graph and requests that could have been answered from the graph already sitting in memory. On a large graph the reload alone can take close to twenty seconds, and a nightly rebuild that rewritesgraph.jsonseveral times turns each rewrite into a full-server stall.Fix
_GraphContextCache.load()now only holdsself._lockfor the stat and the small dict bookkeeping around it; the actual rebuild (_load_entry) runs outside the lock. Rebuilds are single-flighted per(resolved_path, pinned)key via athreading.Event, so concurrent callers never start a second, redundant rebuild of the same path. A caller that already holds a stale entry for that path gets it back immediately while someone else's rebuild is in flight (stale-while-revalidate), rather than queuing behind it. A caller with no entry yet — the path's first ever load — has nothing stale to serve, so it waits for the one in-flight build instead of racing a second one. Because the lock is held so briefly, a slow rebuild of one path no longer blocks a request for an unrelated one either.Tests
Added
tests/test_graph_context_cache_reload.py, exercising_GraphContextCachedirectly with a monkeypatched, artificially slow_load_entry:test_stale_entry_served_while_a_reload_is_in_flight— of three concurrent callers against a now-stale entry, exactly one pays the rebuild cost and the other two get the old entry back immediately, with only one rebuild call total.test_reload_of_one_path_does_not_block_another_path— a slow rebuild of one path does not delay a concurrent load of a different, unrelated path.test_concurrent_first_load_of_new_path_builds_once(control) — with no stale entry to fall back to, concurrent first-time loads of the same brand-new path still single-flight to one build.test_reload_eventually_serves_the_new_graph(control) — once an in-flight rebuild finishes, a later call sees the new data, not the stale one forever.Verification
upstream/v8version ofgraphify/serve.py(restored byte-identical afterward) and pass against the fix; the two controls pass on both.3 failed, 11002 passed, 202 skipped, 1 xfailed— all 3 are pre-existing known flakes (test_erlang_extractor.py,test_r_extractor.py, the skill backtick test), unrelated to this change.python3 -m tools.skillgen --check:check OK: 134 artifact(s) match committed output and expected/.🤖 Generated with Claude Code
https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh