Repository navigation
fix(retain): two concurrent retains of the same content store each fact once (#6337) - #6433
Merged
Merged
Conversation
…i-batch document's facts twice
…ct once (#6337) Each batch checked "is the document still mine" by comparing the document's content hash with its own. Two retains of the same content (a client retry, a task run again while the first still writes) share that hash, so both wrote every batch after the first and stored its facts twice. Under the document lock, a batch now drops the chunks already stored with the same text, together with their facts. A chunk commits in the same transaction as its facts, so a stored chunk means its facts are stored. Whichever run reaches a chunk first writes it, so the document ends complete if either run finishes. The delta write gets the same guard. Phase 1 entity resolution runs before the lock and is keyed by fact position, so the kept facts are renumbered and the phase-1 result is rebuilt to match.
- renumber causal relations when facts are dropped (they pointed at the wrong fact) - un-queue the entity mention counts of dropped facts, so mentions are not counted twice - read only this batch's chunk rows under the lock (new chunk_indices filter on load_existing_chunks), not the whole document per batch - unit tests for the drop helper (entities, ANN links, causal links, mention counts) - replace the timing-based staggered and delta tests with one that pauses the runs at exact points; its delta variant now really writes through the delta guard - system test asserts the exact set of facts, one per chunk - comment: completeness depends on the run that last cleared the document; the store-owned paths write atomically and need no guard - types on the drop helper
- chunk filter casts to integer[], which the Oracle rewrite turns into IN (...); int[] left broken SQL on Oracle for every multi-batch retain - overtaken test: bounded wait for A's first batch, and both variants assert B reached the chunk guard (and the recovery variant that B did not take delta) - type the drop helper's resolver; correct the forget_pending_mentions docstring
…nistic Slices of one chunk let B's first slice be fully stored already; B then only touched the document row and A stopped as overtaken by a newer retain, so the guard was never reached. Slices of several chunks keep B's first slice with chunks to write.
Replaces the _try_delta_retain wrapper, which test_named_result_stubs flags because it returned a variable instead of the named result. The drop spy now records its caller, which shows directly whether B's guard ran in the delta write or a streaming batch.
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.
Fixes #6337.
Problem
Two retains of the same document with the same content, running at once (a client retrying a slow retain, or a task run again while its first run is still writing), stored the facts of a multi-batch document twice.
Each batch checked "is the document still mine?" by comparing the document's content hash with its own. Two runs with the same content carry the same hash, so the check passed for both, and both wrote every batch after the first. Chunks were deduplicated by their fixed ids, facts were not. Recall, consolidation and everything else reading the bank saw the duplicates.
The same weak check guarded the two other routes the issue names: a second retain that starts after the first has committed a batch (the streaming recovery path for a sliced document, and the delta path for one that fits a single batch).
Fix
Under the per-document row lock, a batch now drops the chunks already stored with the same text, together with their facts. A chunk commits in the same transaction as its facts, so "chunk stored" means "its facts are stored". Whichever run reaches a chunk first writes it, so the document ends complete once the run that last cleared it finishes. The delta write gets the same guard.
This is the approach the reporter proposed. Their first idea, aborting a run when it finds an overlap, leaves gaps when both runs abort, so it is not used.
Details:
_drop_committed_chunks).forget_pending_mentions), so mentions are not counted twice either.chunk_indicesfilter onload_existing_chunks,= ANY($3::integer[]), which the Oracle rewrite turns intoIN (...)).What changes
Only same-content overlaps. Runs with different content still hand over as before (the later one wins, the other stops). Every batch except a fresh first one does one more small read: its own chunks' hashes.
Tests
test_concurrent_retains_of_the_same_content_store_each_fact_once: the reporter's test (kept as their commit). Failed 3/3 before, passes after.test_concurrent_same_content_retains_out_of_order_store_each_chunk_once(12 seeds): chunks finish extracting in random order, so the runs batch different chunks. 12/12 failed before, pass after, and the document always ends complete.test_same_content_retain_overtaken_between_load_and_write_stores_each_chunk_once(delta and recovery variants): pauses the runs at exact points so B reads the half-written document and A writes the rest before B does; asserts B really reached the guard. Both failed before.test_drop_committed_chunks.py: unit tests for the renumbering (entities, ANN links, causal links, mention counts).test_92b_concurrent_same_retain.py: two identical retains at once through the real server; the exact set of facts, one per chunk. Failed 2/2 before.Locally: 1589 tests across the 60 retain-related files pass; ruff, ty and
lint.share clean.Thanks to @skybleak333 for the precise report, the failing test, and the analysis of why the obvious fix is wrong.