fix(cache): surface corrupt semantic cache entries instead of swallowing them (#2405) - #2683
fix(cache): surface corrupt semantic cache entries instead of swallowing them (#2405)#2683ousamabenyounes wants to merge 1 commit into
Conversation
…ing them (Graphify-Labs#2405) load_cached() caught json.JSONDecodeError together with OSError and returned None, so a corrupt (invalid-JSON) cache entry was indistinguishable from a miss. Left in place it fails to parse on every future run, silently re-extracting — and, for semantic kinds, re-billing the LLM — the file forever, with no diagnostic (a real vault had 138/249 entries dead). Count JSONDecodeError failures in a process-global (mirroring the existing _legacy_semantic_hits counter) and emit one aggregate RuntimeWarning from check_semantic_cache naming how many entries could not be parsed and how to clear them. OSError stays a silent miss (transient read failure, not corruption). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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 4 advisory finding(s) below merit a look before merge.
Graphify review — findings
This PR changes how the semantic cache handles cache entries that fail to parse as JSON. Previously invalid-JSON entries were caught alongside OSError and returned as a silent miss; now load_cached distinguishes json.JSONDecodeError, increments a module-level _corrupt_cache_entries counter, and check_semantic_cache emits one aggregate RuntimeWarning naming how many entries failed to parse. It touches graphify/cache.py (new counter, changed exception handling, new warning block), adds a corresponding test in tests/test_cache.py, and adds a changelog entry. The test the diff references in the changed-symbols list is broader than the single new test shown in the diff, but the visible surface area is the cache corruption-handling path, the new warning, and its test plus changelog note.
Worth a look
- Module-level corrupt-entry counter is not thread-safe; concurrent semantic cache scans corrupt the delta —
graphify/cache.py:924· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Corrupt-cache warning message never matches test regex 'corrupt' —
graphify/cache.py:1197· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Corrupt-entry count delta is process-global, not per-call, giving cross-contaminated warnings under concurrency —
graphify/cache.py:1197· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Warning message does not contain the word 'corrupt' the test matches on —
graphify/cache.py:1199· 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 — 1378 functions depend on the 393 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
save_semantic_cache()— 51 callers, 9 callees - worse:
file_hash()— 40 callers, 6 callees
Verification — 1378 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: 943 function(s) in the blast radius were not formally verified this run
· 2 more finding(s) on lines outside this diff (see the check run).
|
Shipped in v0.9.42 ( |
Summary
Fix #2405
load_cached()caughtjson.JSONDecodeErrorin the sameexceptasOSErrorand returnedNone, so a corrupt (invalid-JSON) cache entry was indistinguishable from a cache miss. A corrupt entry stays on disk and fails to parse on every future run, so the file is silently re-extracted each time — and for semantic kinds that means re-billing the LLM — with nothing in the output hinting why. The reporter found 138 of 249 entries dead in a real vault, paying that extraction cost on every incremental run.Fix
load_cached()now countsJSONDecodeErrorfailures in a process-global_corrupt_cache_entries, mirroring the existing_legacy_semantic_hitscounter, and still returnsNone(miss → re-extract, self-healing).check_semantic_cache()emits one aggregateRuntimeWarningper run naming how many entries could not be parsed and how to clear them (--force/ clear cache), matching the existing legacy-vintage warning it already raises.OSErrorstays a silent miss — a transient read failure is not corruption.Scope is the consumer side only, as the issue requests; producers that write bad entries are tracked separately (#2380 / #2197).
Test verification (RED → GREEN)
New test
test_corrupt_semantic_entry_warns_and_is_a_misssaves a semantic entry, corrupts it on disk, then assertscheck_semantic_cacheboth treats it as a miss and warns.RED (unmodified
v8, test only):GREEN (with fix):
No regression
uv run --frozen pytest tests/ -q→4329 passed, 3 skipped, plus the 3 pre-existingtest_ollama.pybackend-detection failures that also fail on a cleanv8checkout (unrelated to this change).python -m tools.skillgen --check→check OK.