Repository navigation
fix(retain): make a store-owned append conditional on the document's content hash - #5389
Merged
Merged
Conversation
…content hash A store-owned append's write was conditional on a store-wide watermark, which any unrelated write to the bank moves. On a busy bank the append lost its race on every retry until it gave up and the turn was dropped. The write is now conditional on the content hash the append read (empty when the document did not exist), passed as `expect_content_hash` on put_document and on the retain session's part. A session commit that loses the race is redone in-process, the same way a lost race inside the pipeline already is. The concurrent-append test now runs against every memories store and adds writes to other documents while the appends race; two system stories cover queued and synchronous appends.
Review fixes: - the in-memory test store's session now enforces expect_content_hash, so the suite exercises the precondition instead of ignoring it; - a test drives an append whose session commit loses its race and asserts it is redone in a fresh session with both turns kept (fails with the redo removed); - the sessionless store-owned retain's docstring no longer calls the store-side compare-and-set a follow-up: it exists, on the session path.
nicoloboschi
force-pushed
the
per-document-append-precondition
branch
from
October 7, 2026 10:17
8083206 to
efcbb1b
Compare
test_retain_store_owned_no_connection still passed the removed doc_watermark_at_load. It now passes doc_hash_at_load, and the fallback test also asserts the fence is conditional on the hash the delta planned against.
nicoloboschi
added a commit
that referenced
this pull request
Oct 7, 2026
…keep (#5392) Follow-up to #5389, which shipped three gaps. The engine-level redo had no test at all. `RetainSession` has no in-repo implementation, so nothing exercised the `StoreWriteConflict` branch of `_run_retain_execution` — the one that redoes an append whose session commit lost its compare-and-set, after `_retain_batch_with_append_retry` has already returned. `test_retain_outbox_session.py` grows one, built on the store/session doubles that file already has: the append is redone on a fresh session, from a pristine copy of the submission (the pipeline consumes its input), the losing attempts deliver no completion event and leak no session, and the attempt-limit conflict raises. The two new system stories asserted only that each turn appears in the document's text, and nothing about the memories under it — which is the other half of the bug they guard: a losing writer used to cascade-delete the winner's units, so the full text could stand with nothing remembered beneath it. They now also assert every memory of the appended document is the stubbed fact, and that there is at least one. How many survive dedup is not pinnable (the queue may fold several turns into one execution), but "only this fact, and some of it" is. Exhausting the redo attempts raised with nothing logged, so a dropped turn was indistinguishable from an ordinary store error. Warn with the bank and the document, as `_retain_batch_with_append_retry` already does for the same event one layer down. `put_documents` documented a per-document `expect_content_hash` the engine can never send: `flush_document_bodies` routes every guarded write as a single-document call, precisely so one document losing its race cannot fail the batch. Say that instead — the same docstring warns two lines later that "a method a store must declare is one the engine can never call, and that has shipped twice", and an unreachable precondition path is the same trap.
This was referenced Oct 8, 2026
Merged
This was referenced Oct 16, 2026
Merged
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
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.
Problem
For a store that owns its documents, an append's write was conditional on a store-wide watermark. Any write to the bank moves it — retains of other documents, consolidation — and an append's read-to-write window includes its LLM extraction. On a busy bank the append lost its race on every retry, gave up after three attempts, and the turn was dropped.
Change
put_document(expect_content_hash=…)replacesexpect_watermark. The write commits only if the stored document still holds that content hash (""= it does not exist). Only a write to this document can fail it.put_documentstakes it per document dict.RetainDocumentPart.expect_content_hashcarries the same condition into a retain session.DocumentBase.watermark/DocumentChunkState.watermarkare gone._append_expectation), and the delta path passes the hash it planned against._retain_batch_with_append_retryalready gives a lost race inside the pipeline. The redo clears deferred outbox events so a lost attempt delivers nothing.Tests
test_concurrent_appends_keep_every_turnonly asserts on public reads, so it is no longer markedmemory_backend_incompatible. It now also retains other documents into the same bank while the appends race.expect_content_hash.test_12_append_document.py): 8 queued appends through the worker beside 8 other-document retains, and 3 truly concurrent synchronous appends. Every operation must complete and every turn must be kept.Local: the touched api-slim test files pass (461), and
test_12_append_document.pypasses 8/8, including against a store-owned backend.Merge order
A store-owned memories extension implementing the seam has to accept
expect_content_hashbefore this lands. Its change is ready alongside this one.