store-owned: finish Protocol-B removal (PG-free zero-batch tracking) - #3669
Closed
nicoloboschi wants to merge 13 commits into
Closed
store-owned: finish Protocol-B removal (PG-free zero-batch tracking)#3669nicoloboschi wants to merge 13 commits into
nicoloboschi wants to merge 13 commits into
Conversation
A memories store that owns its rows (external backend) retains a document in a connection-free store phase. That phase wrote each memory TWICE: insert_facts staged it without entities, then record_unit_entities read the just-written records back (full vectors) and re-upserted them with entity ids attached — because entity ids can only be resolved onto real unit ids after they exist. On an object-store-backed engine that reattach is a second full write per memory plus a read-back, doubling the round-trips on the slow path (the dominant cost of retain). It's avoidable: mint the ids without writing (insert_facts_batch with defer_index), remap the entities, then write once via index_facts with the entity ids already inline — tagged with the write-group txn so it commits atomically with the group (and, as a bonus, the postings are now witness-covered instead of riding an uncovered seam). Co-occurrence accumulation still runs (record_unit_entity_postings gains store_write=False: co-occurrence only, since the row is already correct). Streaming ext path only; the delta path is unchanged. Tests updated: the single write via index_facts is asserted connection-free and txn-tagged, and the posting runs store_write=False. No behavior change for the Postgres store (a no-op there).
A store advertising `store_owned_retain` now commits the entire retain in ONE server-side call: it resolves each fact's raw entity names against its own registry, mints ids for new names, writes the memories with ids attached, and replaces the document's prior version atomically. The orchestrator therefore: - skips Phase-1 entity resolution (no Postgres trigram scan / entity INSERTs); - routes the streaming write through a new `_streaming_store_owned_retain` that reconstructs entity names from the facts and issues the single retain — no connection phase, no `documents`/`chunks`/`entities` rows, no commit witness, no `decide_txn`. The store's single write is already atomic; - marks the document tracked so the post-loop finalizer (which would write a Postgres documents row and run handle_document_tracking, whose delete-by- document_id lands at a later seq and would delete the just-written memories) does not fire; - falls delta retain back to full retain for such a store (delta's chunk-diff still rides a Postgres connection phase) — a follow-up. Non-store-owned (Postgres) backends are unchanged: the capability gate defaults off, so the two-phase path runs exactly as before. Verified end-to-end (store-owned backend, LLM-free chunks): retain 3 docs -> recall 3/3 ranked; zero Postgres rows for the bank; an entity shared by two documents resolves to ONE id; two writes per document, no clobbering tombstones.
…t per-org The store-owned retain path was gated on a plain `store_owned_retain` attribute, which a routing memories extension (per-org: some banks on a store, some on Postgres) does not expose — so a routed store-owned bank silently fell back to the Postgres path. Add `store_owned_retain_for(bank_id)` on the base extension (defaults to the class attribute, mirroring owns_document_store_for) and consult it from the three retain gates instead of the attribute. A single-store extension needs no change; a router overrides the _for method to answer per bank.
…gate it RoutingMemories generates a delegating wrapper for every async method on the MemoriesExtension interface. The store-owned retain() lived only on the concrete store, so a routed org hit 'RoutingMemories has no attribute retain'. Declare it on the base (default raises NotImplementedError; store-owned stores override, and the orchestrator only calls it when store_owned_retain_for is true) so the router covers it automatically.
…ix include_chunks Two write paths still opened a cross-store write-group for a store-owned org even though every memory write in them lands ONLY in the store: - Consolidation (consolidator.py): minted a write-group per LLM batch and left it pending if the batch crashed/cancelled between mint and decide — the exact source of the undecided txn+upsert/txn+patch ops that stall the store's indexer and starve every namespace's fold. For a store-owned org the observations, deletes and mark_consolidated stamps are store-only, so _batch_txn=None: plain, immediately- durable, idempotent-on-retry writes, no witness, nothing left undecided. The refresh-tag bookkeeping becomes a best-effort plain write. - delete_memory_unit (memory_engine.py): the write-group wrapped only the store tombstone (the relink/prune enqueues join memory_units/unit_entities, which a store-owned org keeps no rows in — no-ops); a plain delete needs no group. Also fix include_chunks for store-owned recall: it seeded chunk metadata from the Postgres chunks table, which PG-free retain no longer writes, so no chunks came back. Derive the metadata from the hits (chunk_id + document_id) and overlay the text from the store — the store path that was already half-wired. Curation, delete-document and 0-fact retain tracking still use a write-group: they genuinely write the Postgres entities registry / documents row alongside the store, so they stay until those move off Postgres.
…hunks is empty
PG-free retain writes no SQL chunks row, so include_chunks came back empty for a
store-owned org. Query the chunks table as before; if the store owns its document
store AND the query is empty, synthesize each row from the chunk_id itself
(chunk_id = {bank}_{document}_{index}; bank_id known, index is the last segment),
then overlay the text from the store. Chunk metadata now returns; the text overlay
(list_chunk_texts) is exercised the same way as the legacy owns-docs path.
…sh Protocol-B removal) The last three write-group txn users for a store-owned org, all of which depended on Postgres rows that PG-free retain no longer writes: - delete_document: gated the store tombstone on the Postgres documents DELETE RETURNING id, which is always None under PG-free retain (no row) — so a store-owned document could NEVER be deleted (memories + doc record left behind). Now drives the deletion off the store: plain delete-by-document + doc-record delete, no write-group (store-only). - curation (edit/invalidate/revert): _curation_txn=None for store-owned — the edited memory write is atomic on its own; the Postgres entity/posting writes are unused by store-owned reads. No witness left undecided. - 0-fact retain tracking: a store-owned 0-fact (re-)ingest now does one plain delete-by-document instead of a Postgres documents row + write-group. With retain, delta, consolidation, unit-delete, delete-document, curation and 0-fact tracking all txn-free, a store-owned org opens NO Protocol-B write-group at all — nothing can be left undecided to stall the indexer.
A store that owns its document metadata keeps no rows in the SQL documents table,
so list_documents returned an empty page for it. Add a store-owned branch that
delegates to the store's own list_documents (returning the same {items,total,
limit,offset} shape), and declare list_documents on the MemoriesExtension base so
routing delegates it. Store-generic.
Bank stats read total_documents from the SQL documents table unconditionally, so a store that owns its documents (empty SQL table) reported 0 — the one count not asked of the store, unlike links/nodes. Add a store-owned branch calling the store's count_documents, and declare count_documents on the base. Store-generic.
get_entity_graph read the SQL entity_cooccurrences/entities tables, empty for a store that owns its entities. Add a store-owned branch delegating to the store's get_entity_graph, and declare it on the base. Store-generic.
When a store owns the whole retain (its own entity registry, atomic writes), a curation edit that changes entities was still minting the new entities into the host SQL 'entities' table (resolve_entities_only autocommit) and writing the unit_entities postings (reassert_entities_batch / link_units_to_entities_batch) — rows that store's own reads never consult. So a brand-new entity created by an edit landed only in SQL and was invisible to the store's entity reads. Route it the same way retain already does: hand the store the raw entity NAMES and let its apply_edit resolve + mint them against its OWN registry, rewriting the memory's entity ids from the result. apply_edit gains an 'entity_names' argument (authoritative when set; a SQL-registry store ignores it and uses the pre-resolved entity_ids). For such a store the edit path now skips the SQL entity mint, the canonical-name read-back, and the clear/reassert/link/prune postings entirely. No behavior change for a SQL store (entity_names stays None; the resolve+relink path is untouched). Companion: a store provider's apply_edit must resolve+mint entity_names. (get_entity_graph, the other store-owned entity residue, already reads from the store.)
…removal) The post-loop "no batch processed" finalizer created a Postgres documents row and opened a write-group transaction even for a store that owns retain. It is reached when a retain yields zero batches — empty/gibberish content, or a recovery where every chunk was already committed on a prior attempt — so the per-batch 0-fact branch's store-owned path never ran. Guard it the same way: for a store-owned retain, drop the document's prior memories with one plain store-side delete-by-document, no Postgres row and no write-group, so nothing is left undecided to stall the store's indexer.
nicoloboschi
force-pushed
the
store-owned-drop-cross-store-txn
branch
from
August 20, 2026 12:31
c2926d2 to
62336b4
Compare
Collaborator
Author
|
Superseded by #3696, which consolidates the store-owned writes, the delta re-ingest CAS, and the PG-free zero-batch finalizer into one branch so the store interface and its implementers version together. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Completes the store-owned / PG-free write path so a store that owns retain, entities, and document
metadata (
store_owned_retain_for(bank_id)/owns_document_store_for(bank_id)) no longer goesthrough cross-store write-group (Protocol-B) transactions for any operation.
Consolidation, curation edits, unit deletes, delete-document/where, delta re-ingest, the main
streaming write, and the per-batch 0-fact path were already converted in the preceding commits. This
adds the one remaining gap:
The fix — zero-batch document tracking
_streaming_retain_batch's post-loop "no batch processed" finalizer created a Postgresdocumentsrow and opened a
begin_txn/decide_txnwrite-group even for a store-owned retain. It isreached when a retain yields zero batches:
In that window the per-batch 0-fact branch's store-owned path never runs, so
doc_tracking_donestays False and the finalizer falls through to the legacy Postgres path. That reintroduces both a
Postgres write and an undecided write-group on the supposedly PG-free path.
The finalizer now takes the same store-owned branch the per-batch 0-fact path uses: for a
store-owned retain, drop the document's prior memories with one plain store-side
delete-by-document — no Postgres row, no write-group, nothing left undecided to stall the store's
indexer.
Also
A comments-only commit describes the store-owned delete generically (drops an internal backend name
from three comments); no behavioural change.
Status
Draft — not for merge yet. Opened to make the completed store-owned series reviewable against
mainand to continue the remaining work here.Test notes
orchestrator.pyandmemory_engine.pyparse clean.delete_document(conn=None, ...)shape).