Repository navigation
fix(core): serialize search-index rewrites of one entity on Postgres - #1665
Open
sammywachtel wants to merge 1 commit into
Open
sammywachtel wants to merge 1 commit into
sammywachtel wants to merge 1 commit into
Conversation
Every refresh of an entity's search rows is delete-then-insert in one transaction. Two of them for the same entity at once either deadlocked on the DELETE or hit search_index_pkey on the entity row or a relation row: the upsert's conflict target is the permalink index, and a violation on any other unique index is raised. Two quick saves of a long note are enough, and the second save answers 500. Each rewrite of an entity's projection now takes a transaction-scoped advisory lock keyed on (project, entity) before it touches any row: the indexer's refresh (SearchService.index_entity_data) and the accepted-note write (AcceptedNoteSearchRepository.refresh_entity, delete_entity). A refresh that reads its text from storage reads it after taking the lock, so the last refresh indexes the file as it is now. SQLite is unchanged. Signed-off-by: sammywachtel <subp@wachtel.us>
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.
Why
On Postgres, saving a long note twice in quick succession can fail. The second save answers 500, and the log shows one of:
asyncpg.exceptions.UniqueViolationError: duplicate key value violates unique constraint "search_index_pkey",DETAIL: Key (id, type, project_id)=(<entity id>, entity, <project>) already exists.;Key (<relation id>, relation, <project>);asyncpg.exceptions.DeadlockDetectedError: deadlock detected, with both processes onDELETE FROM search_index WHERE entity_id = $1 AND project_id = $2(while rechecking updated tuple ... in relation "search_index").Every refresh of an entity's search rows is delete-then-insert in one transaction (since the fix for #1621): delete every
search_indexrow the entity owns, then insert the entity row, its observation rows and its relation rows again. Under READ COMMITTED, two such transactions for the same entity collide:(permalink, type, project_id). If the second insert passes the arbiter check before the first commits, its insert intosearch_index_pkeythen conflicts with the first's row, and Postgres raises the violation, because a conflict on a non-arbiter unique index is never resolved byON CONFLICT.A long note makes the window wide: re-indexing tens of KB takes longer than the gap between two saves. One save already runs more than one refresh of the same entity (the accepted write's
refresh_entity, the freshen-before-write reindex, and the follow-up relation resolution), so two saves a few seconds apart overlap even when the client waits for each answer.Evidence
Reproduced locally with synthetic notes, on Postgres (pgvector pg17), through
PUT /v2/projects/{project}/knowledge/entities/{entity}on a note of about 80 KB with 12 relations and 12 observations, file watcher on:deadlock detected/duplicate key value violates unique constraint "search_index_pkey"errors, includingKey (id, type, project_id)=(1, relation, 2) already exists.With this change, the same harness: 4×3, 12 sequential, 6×4, 8×3 and 3×5 rounds, every save answered 202, the Postgres log held no errors, and the file, the accepted note content and the entity's search row all named the same last version, with exactly one relation row per relation.
What changed
repository/search_projection_lock.py:lock_entity_search_projection(session, project_id=, entity_id=)takespg_advisory_xact_lock(hashtextextended('basic-memory:search-projection:<project>:<entity>', 0)). The lock is released by the commit or rollback that ends the transaction. It is a no-op on SQLite, which already allows one writer at a time. Refreshes of different entities do not wait on each other.SearchService.index_entity_datatakes the lock first in its transaction, before the delete. When the refresh reads its text from storage (content is None), it reads after taking the lock, so the last refresh to run indexes the file as it is now, not as it was when it started waiting. A read failure still raises before any row is deleted, and the rollback keeps the previous projection.AcceptedNoteSearchRepository.refresh_entityanddelete_entitytake the same lock before their delete, inside the accepted-note transaction.Lock order: a transaction takes this lock before it touches any of the entity's
search_indexrows. Every writer that rewrites an entity's projection does, so a holder of the lock never waits on another writer of those rows. Neither caller holds another session's lock on these rows when it takes this one.Testing
New
tests/services/test_search_refresh_concurrency.py(Postgres; skipped on SQLite):test_concurrent_saves_of_a_long_note_all_succeed_and_the_last_one_wins: three rounds of 6 concurrent "write the 80 KB file, then refresh from storage". Every refresh succeeds, the entity keeps exactly its one entity row, two observation rows and two relation rows, and the entity row's content equals the file on disk at the end.test_negative_control_without_the_lock_overlapping_saves_fail: the same with the lock patched out; the refreshes fail withsearch_index_pkeyordeadlock detected.test_concurrent_refreshes_keep_relation_rows_singleand its negative control: 6 concurrent refreshes with content in hand, as the batch indexer calls it. Without the lock they fail withsearch_index_pkeyon relation rows.test_an_accepted_write_and_an_index_refresh_take_turns: accepted-note writes and index refreshes of one entity interleaved, all succeed, one entity row.To make the race certain rather than likely, every refresh in these tests pauses for 50 ms between its delete and its insert, and the collision rounds start from an entity whose rows were just deleted. Both changes are applied equally to the tests and their negative controls. With the change to
search_service.pyreverted, the first and third tests fail.tests/repository/test_accepted_note_search_repository.py: the recorded statements on Postgres now start with the lock.Runs:
tests/services,tests/repository,tests/indexing,tests/index,tests/api/v2, SQLite (macOS): 3004 passed, 50 skipped.tests/services/test_search_refresh_concurrency.py,tests/services/test_search_refresh_atomicity.py,tests/repository/test_accepted_note_search_repository.py, Postgres 17: 15 passed.ruff check,ruff format --check,ty check src tests test-int: clean.Risks / follow-ups
SearchRepository.delete_by_entity_idcalled on its own (note delete) and the bulk project reindex do not take it; they did not show up in the failures.contentgiven) indexes that text even if a later save has since changed the file; the later save's own refresh follows it and wins. Only refreshes that read from storage read the newest text by construction.