Skip to content

fix(serve): revalidate graph identity before caching contexts - #3656

Open
diskude wants to merge 1 commit into
Graphify-Labs:v8from
diskude:fix/graph-cache-identity-20260918
Open

diskude wants to merge 1 commit into
Graphify-Labs:v8from
diskude:fix/graph-cache-identity-20260918

Conversation

@diskude

@diskude diskude commented Sep 18, 2026

Copy link
Copy Markdown

An atomic replacement of graph.json can keep its byte length and restored mtime, causing a warm _GraphContextCache to return the old graph. This change includes device, inode and ctime in the cache identity and rechecks that identity after loading. It retries one changed-identity load, then rejects a second race while preserving the previous entry and its LRU state.

The patch adds 21 regression tests covering same-size/restored-mtime replacement, cold and warm load races, bounded retries, failed refresh, pinned entries and LRU behavior. It remains a metadata check: identical five-field collisions, hidden in-place rewrites, filesystem writer coordination and replacement after the final stat are outside its guarantee. Learning-sidecar invalidation in #3044 remains separate.

Validation on macOS / CPython 3.14.7 with frozen upstream dependencies:

  • Complete pytest tests/ -q --tb=short: 5,784 passed, 14 skipped, 0 failures/errors. Existing platform, optional-service and feature-condition skips remain visible.
  • Focused cache/serve/HTTP tests: 204 passed, no skips.
  • Earlier standalone pilot (CPython 3.11.15): 10 baseline assertion failures among 21 tests; candidate/restoration: all 21 pass. Eight named pilot mutants were killed.
  • Configured Ruff checks passed for both changed files. The required code-only graph update completed, with its parser/coverage limitations retained in the local evidence.

The initial full run exposed an existing backend test's dependence on inherited GRAPHIFY_GEMINI_MODEL. The unchanged test failed with that override, passed through the existing environment scrub, failed when the override was reintroduced, and passed again after removal. The full passing run used that scrub; no test expectation, source file, dependency pin or skip condition was changed to obtain it.

The complete GitHub CI and Python/platform matrix remain for CI to evaluate. This change has only been exercised in an isolated checkout; it has not been adopted into an existing managed installation or attested as loaded by a running server.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@graphify-labs graphify-labs Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Graphify reviewed this change.

Worth a look — the grounded gate found no coupling regressions or blocking issues, but 2 advisory finding(s) below merit a look before merge.

Formal verification. No changes could be formally verified in this run.


Graphify review — findings

Strengthens _GraphContextCache's file-identity key from (mtime_ns, size) to a five-field tuple of (st_dev, st_ino, st_ctime_ns, st_mtime_ns, st_size) via a new _file_key, so an equal-length, mtime-restored atomic replacement is detected instead of served stale. On a cold miss, load now builds the entry privately and re-stats after reading, retrying once if identity changed and raising RuntimeError("graph.json changed while loading") if the file keeps churning — with the old cache entry and LRU order preserved on failure. Adds tests/test_serve_cache_identity.py with in-process contract tests covering the load-time race, bounded churn, LRU eviction, pinned isolation, and parse/absence error mapping.

Worth a look

  • File I/O performed while holding the cache lock serializes all loadsgraphify/serve.py:180 · Escalate · medium
    • agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
  • Graph reload can now raise uncaught RuntimeError during file churngraphify/serve.py:184 · Escalate · medium
    • agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification

Impact & health

Graphify review

Impact — 586 functions depend on the 199 functions this change touches.

Health — this change adds coupling hotspots:

  • new: dispatch_command() — 2 callers, 125 callees
  • new: _query_graph_text() — 25 callers, 10 callees
  • new: _score_query() — 15 callers, 6 callees
  • new: _query_terms() — 20 callers, 3 callees
  • new: run_benchmark() — 16 callers, 3 callees
  • new: _load_graph() — 14 callers, 3 callees
  • new: _build_server() — 2 callers, 16 callees
  • new: _query_subgraph_tokens() — 7 callers, 3 callees
  • …and 9 more — each is listed as a finding

Verification — 586 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: 401 function(s) in the blast radius were not formally verified this run

Test selection

Test selection

9 of 287 test file(s) selected (3%) via static blast radius.

  • tests/test_benchmark.py — impact
  • tests/test_benchmark_raw_graph.py — impact
  • tests/test_file_label_disambiguation.py — impact
  • tests/test_query_induced_edges.py — impact
  • tests/test_query_mcp_direction.py — impact
  • tests/test_query_names_its_graph.py — impact
  • tests/test_serve.py — impact
  • tests/test_serve_cache_identity.py — impact, changed-test
  • tests/test_serve_http.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.

Formal verification

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 69 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\_entry.

The verifier did not have enough to check \_load\_entry, 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 200 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)

· 17 more finding(s) on lines outside this diff (see the check run).

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.

1 participant