fix(knowledge): key RAG chunk memo entries on the chunker source, not just the shim - #3487
Conversation
…t just the shim `_extract_symbols_for_memo` is a one-line delegate to `parse_file_symbols`, so its body is byte-identical before and after any rewrite of `ast_symbol_graph`. `fingerprint` hashes only the decorated function's own source, so the memo store kept serving symbol data built by the previous parser for every file whose bytes had not changed - the recent import-resolution change did not take effect on already-indexed files. Add `memoize_persistent(..., depends_on=...)`: declared callables and modules contribute a source digest to the key. The knowledge-graph site declares the whole `ast_symbol_graph` module rather than `parse_file_symbols` alone, so the key also moves when a private helper changes (the import-resolution rules live in one) and when the `FileSymbols` dataclass gains a field - the latter would otherwise be absent from an unpickled entry and raise `AttributeError` at the access site instead of falling back to the default. Omitting `depends_on` leaves keys byte-identical to the previous scheme, so the other memo sites keep their caches. Correct the shim docstring, which claimed the fingerprint already captured changes to `parse_file_symbols`.
… just the shim `_chunk_for_memo` only dispatches to `_extract_python_chunks` or `_line_chunks`, so its body is byte-identical before and after any rewrite of either. `fingerprint` hashes only the decorated function's own source, so the memo store kept serving chunks shaped by the previous chunker for every file whose bytes had not changed - a fix to chunk shaping never reached an already-indexed file. Declare the module as a memo dependency. The chunkers live beside the shim rather than in another file, so the site names its own module via `sys.modules[__name__]` instead of importing itself, which keeps the reference on the canonical module object whichever import path loaded it first. Module granularity also covers the syntax-error fallback edge between the two chunkers and the chunk-size and overlap defaults, none of which a hash of the entry point alone can see. Correct the shim docstring, which claimed the fingerprint already captured chunker changes, and the key column in the memoization concept doc, which listed an `embedder_id` component the key never had. The regression test gets its own file: `tests/unit/test_rag.py` is skipped wholesale while the FTS5 indexer leaks memory, so a test added there would never run.
Review-bot acknowledgement summary
Review coverage for head
|
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
…oves bot-ack: 3740340955 Keying the memo store on the chunker source fixes the layer below the one that decides whether to call it. `_build_inner` re-reads a file only when its mtime moved, so for an unchanged file the chunker is never invoked and the memo key never consulted - the rows the previous chunker wrote stay in FTS until that file happens to be edited, and searches keep returning them. Record `code_digest` of the chunker module in a new `index_meta` table and reindex every file when it stops matching. It is the same digest that keys the memo store, so the two layers cannot disagree about which chunker is current. An index written before this change has no stored revision, so the first build after upgrading reprocesses everything once. Files are re-read but their chunks still come from the memo store when the chunker source itself has not moved, so that sweep costs IO, not re-chunking.
bot-ack: 3740356220 The revision read ran on a deferred transaction, which SQLite only escalates to a write lock at the first DELETE further down. Two processes can hold different chunker revisions - an upgrade while a long-lived process is still running - so the older one could read the revision the newer one had just committed, conclude the chunker changed, reindex with its own older chunker and record that as current. The newer build is undone and the pair thrash until the older process exits. Take the lock with BEGIN IMMEDIATE before the read, so the whole read-decide-write step is serialized. Lock duration is unchanged for a real build - the DELETE loop already held it across the chunking pass - and a no-op build now takes and releases it in microseconds. WAL keeps readers unblocked either way. Roll back explicitly if the build raises, so a failed run cannot leave the lock held while recording a revision the index does not reflect.
|
Your organization's Advanced Security usage limit has been reached. To continue using Advanced Security reviews, please upgrade your plan or increase your usage limits in your account settings. |
Baz dismissed its prior approval because a re-review found new findings.
| _memoized_extract = memoize_persistent( | ||
| _kg_memo_store, | ||
| site="knowledge_graph", | ||
| depends_on=(_ast_symbol_graph,), | ||
| )(_extract_symbols_for_memo) |
There was a problem hiding this comment.
Fresh graph masks parser revisions
depends_on=(_ast_symbol_graph,) invalidates memo entries only after build_knowledge_graph runs, so get_or_build_knowledge_graph can serve a graph with stale symbols/import edges for up to 30 minutes through query_impact and export_graph_summary after an ast_symbol_graph fix — should we persist an extractor revision/digest and require it to match before the built_at fast path?
Baz can fix this - reply apply commit / apply pr to fix without changes, or comment to modify the plan.
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`src/bernstein/core/knowledge/knowledge_graph.py` around lines 114-118, update the
`get_or_build_knowledge_graph`/graph caching logic so `depends_on=(_ast_symbol_graph,)`
invalidation also invalidates recently built graph databases. Persist the current
extractor/module digest or revision in the graph metadata when building, and require it
to match before taking the 30-minute fresh-enough fast path; otherwise force a rebuild
so `query_impact` and `export_graph_summary` cannot serve stale symbols or import edges
after parser changes.
There was a problem hiding this comment.
Commit 45b4c13 addressed this comment by persisting an extractor revision in graph metadata and requiring it to match before the fresh-enough fast path. A mismatched or legacy revision now forces the graph to rebuild.
`fingerprint.py` landed on main with the stat-token freshness check from that branch's review, which is a strict superset of the copy here - the shared change plus re-hashing a module whose file moves under a running interpreter. Main's `fingerprint.py`, `test_fingerprint.py` and `test_knowledge_graph.py` taken wholesale; main's fingerprint tests also cover an in-process reload case this branch's did not. The RAG work is preserved: the `knowledge/rag.py` key row, the `sys.modules[__name__]` guidance for a shim whose delegates sit beside it, and the limitation about a cache upstream of the memo layer needing its own revision gate. `_edit_chunker_source` wrote a bare digest into `_SOURCE_DIGEST_CACHE`, which is now `(freshness_token, digest)`. It writes the pair and keeps the existing token, so the substituted digest still reads as current - a fresh token would send the next lookup back to the unedited file and quietly undo the simulated edit, leaving the three invalidation tests asserting nothing.
…sion bot-ack: 3740379624 `get_or_build_knowledge_graph` returned the cached database whenever `built_at` was inside the age window, without consulting the extractor. `depends_on` invalidates memo entries only when the memoised extractor is actually called, and this fast path returns before that happens - so an `ast_symbol_graph` fix was masked for up to the whole window while `query_impact` and `export_graph_summary` kept serving the symbols and import edges the fix was meant to correct. This is the limitation this branch already documents - a cache upstream of the memo layer needs its own revision gate - present one layer up from the site it was written about. The gate is the same one `knowledge/rag.py` applies to its index: record `code_digest` over the extractor module at build time, and require it to match before taking the fast path. Both layers read the same digest, so they cannot disagree about which extractor is current. A database written before the key existed reads as `None` and rebuilds once, which is the safe direction. Verified: with the gate removed, a graph inside the window is served after the extractor revision moves, and a database with no recorded revision is trusted. Both now rebuild, and a current graph inside the window is still not rebuilt.
|
Verdict: fixed in
The gate is the one A database written before the key existed reads as Verified by removing the gate rather than by reading it:
The first column matters as much as the others — a gate that rebuilt every time would pass the staleness assertions while destroying the point of the window. |
| connection.execute( | ||
| "INSERT INTO metadata(key, value) VALUES(?, ?)", | ||
| ("extractor_revision", _extractor_revision()), | ||
| ) |
There was a problem hiding this comment.
Freshness gate trusts graph from older extractor
_write_graph computes extractor_revision only after build_knowledge_graph has run build_semantic_graph and _collect_file_nodes_and_edges, so a mid-build change can produce graph data with the old extractor while code_digest(_ast_symbol_graph) records the new one. Then get_or_build_knowledge_graph sees a matching revision and reuses stale output — should we capture the revision before parsing, verify it before commit, and abort/retry if it changed?
Baz can fix this - reply apply commit / apply pr to fix without changes, or comment to modify the plan.
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`src/bernstein/core/knowledge/knowledge_graph.py` around lines 221-224, fix
`_write_graph` so it does not compute and persist the extractor revision only after
graph extraction has completed. Capture the revision before
`build_semantic_graph`/`_collect_file_nodes_and_edges`, pass that captured value into
the write path, and verify the current revision still matches immediately before
committing; abort or retry the build if it changed, rather than stamping the database
with a digest that did not produce its nodes and edges.
There was a problem hiding this comment.
Commit cf7da9c addressed this comment by capturing the extractor revision before parsing and passing that value into _write_graph, preventing a mid-build edit from stamping the newer revision onto older graph data. It does not add the requested pre-commit verification or retry.
bot-ack: 3740567138 The revision was computed inside `_write_graph`, after `build_semantic_graph` and `_collect_file_nodes_and_edges` had already produced the nodes and edges. `code_digest` re-reads the extractor file, so an edit landing mid-build gave the new digest, while the graph itself came from the module already resident in memory. The gate added in the previous commit would then see a matching revision and trust that stale output indefinitely - the very failure the gate exists to prevent, moved into the write path. Captured once before any parsing starts and threaded through to the write. A build overtaken by an edit now records the revision it parsed under, which no longer matches the current one, so the next call rebuilds. No abort or retry is needed: the mismatch already means exactly "rebuild". Verified by computing it at write time again - the test then observes the post-edit revision stamped onto pre-edit output and fails.
|
Verdict: fixed in
Captured once before any parsing starts and threaded through to the write. On the abort/retry you suggested: not needed, and I would rather not add a retry loop that has to be reasoned about separately. A build overtaken by an edit now records the revision it parsed under, which no longer equals the current one — so the next call mismatches and rebuilds. The mismatch already means "rebuild"; recording honestly is enough to reach it, and there is no window where the wrong answer is served twice. Verified by putting the computation back at write time: the test then observes the post-edit revision recorded against pre-edit output and fails. The three assertions from the previous round still hold — a current graph inside the window is not rebuilt, a moved revision is, and a database with no recorded revision is. |
1 participant
User description
knowledge/rag.pycarries the same memo-key defect #3486 fixes for the knowledge-graph site. Review surfaced two more problems in the same path; all three are fixed here.1. The memo key does not move when the chunker does
The shim only dispatches:
fingerprinthashes only the decorated function's own source, so that body is byte-identical before and after any rewrite of either chunker. The docstring claimed the opposite:It does not. Declare the module as a memo dependency at the
_get_memoized_chunkercall site, using thedepends_onparameter added in #3486.The chunkers live beside the shim rather than in another file, so the site names its own module through
sys.modules[__name__]instead of importing itself. That keeps the reference on the canonical module object whichever import path loaded it first -bernstein.core.ragis served by the legacy redirect finder.Module granularity rather than
depends_on=(_extract_python_chunks, _line_chunks)also covers the syntax-error fallback edge between the two chunkers, and thechunk_sizeandoverlapdefaults.2. The index never reaches the memo layer
Fixing the memo key alone does not deliver the outcome it exists for.
_build_innerre-reads a file only when its mtime moved:For a file whose bytes have not changed the chunker is never called and the memo key is never consulted, so the FTS rows the previous chunker wrote stay in the index and searches keep returning them.
code_digestof the chunker module now goes into a newindex_metatable, and every file is reprocessed when it stops matching. It is deliberately the same digest that backsdepends_on, so the two layers cannot disagree about which chunker is current.An index written before this change has no stored revision, so the first build after upgrading reprocesses everything once. Those files are re-read, but their chunks still come from the memo store when the chunker source itself has not moved - the sweep costs IO, not re-chunking. There is a test for exactly that layering.
3. The rebuild decision was not atomic
The revision read ran on a deferred transaction, which SQLite escalates to a write lock only at the first
DELETEfurther down. Two processes can hold different chunker revisions - an upgrade while a long-lived process is still running - so the older one could read the revision the newer one had just committed, conclude the chunker changed, reindex with its own older chunker and record that as current. The newer build is undone, and the pair thrash until the older process exits.BEGIN IMMEDIATEbefore the read makes the whole read-decide-write step serialized. Lock duration is unchanged for a real build, since theDELETEloop already held it across the chunking pass; a no-op build now takes and releases it in microseconds, and WAL keeps readers unblocked. A failed build rolls back explicitly rather than holding the lock while recording a revision the index does not reflect.Thanks to
baz-reviewer[bot]for catching 2 and 3.Test placement
tests/unit/test_rag.pyis skipped wholesale (pytestmark = pytest.mark.skipif(True, ...)) while the FTS5 indexer leaks memory, so a regression test added there would never run. The new tests get their own file and index one file each.Verification
Reverting each fix alone, with everything else in place, fails the tests that cover it:
depends_onargumentTestChunkerMemoInvalidationchunker_changedin the mtime gateTestIndexChunkerRevisionreindex testsBEGIN IMMEDIATEtest_write_lock_is_taken_before_the_revision_is_readtest_a_second_builder_cannot_write_while_a_build_is_in_flightpasses on the pre-fix code too - by the time chunking runs, theDELETEs have already opened the transaction. It is there to guard the atomicity property against a later move to autocommit or per-file commits, not as a regression test for 3.Docs
docs/concepts/fingerprint-memoization.mdgains the self-declaring-module shape and a limitation note on caches sitting above the memo layer. The key column forknowledge/rag.pyis corrected - it listed anembedder_idcomponent the key never had.Base
Stacked on #3486, which adds
depends_on. GitHub retargets this tomainwhen that merges.Acknowledged, not fixed here
current_pathsis collected before the write lock (3740365491). Accurate as a description, but not a defect this PR should close:DELETE, two SELECTs after the walk. NowBEGIN IMMEDIATEruns immediately after the walk, so less can happen in between.Closing this properly means deciding what freshness the index promises, which is a larger change than a chunker-invalidation fix should carry.
Generated description
Below is a concise technical summary of the changes proposed in this PR:
Invalidate RAG chunk memo entries and incremental index rows when chunker code changes, using
code_digest,depends_on, and atomic SQLite locking inCodebaseIndexer. Track extractor revisions in the knowledge graph cache soget_or_build_knowledge_graphrebuilds stale graphs even within the freshness window, with regression coverage and updated memoization documentation.Modified files (3)
Latest Contributors(2)
BEGIN IMMEDIATE.Modified files (2)
Latest Contributors(2)