Skip to content

fix(core): serialize search-index rewrites of one entity on Postgres - #1665

Open
sammywachtel wants to merge 1 commit into
basicmachines-co:mainfrom
sammywachtel:fix/search-projection-overlap
Open

sammywachtel wants to merge 1 commit into
basicmachines-co:mainfrom
sammywachtel:fix/search-projection-overlap

Conversation

@sammywachtel

@sammywachtel sammywachtel commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

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.;
  • the same on a relation row, Key (<relation id>, relation, <project>);
  • asyncpg.exceptions.DeadlockDetectedError: deadlock detected, with both processes on DELETE 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_index row 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:

  1. The second DELETE waits on the first's row locks, then re-checks rows the first has replaced, and with more than two writers they wait on each other: a deadlock.
  2. When the second DELETE finds nothing to wait on (the old rows were already gone), both transactions reach their INSERTs together. The upsert absorbs a conflict only on its arbiter, the partial index on (permalink, type, project_id). If the second insert passes the arbiter check before the first commits, its insert into search_index_pkey then conflicts with the first's row, and Postgres raises the violation, because a conflict on a non-arbiter unique index is never resolved by ON 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:

  • 4 concurrent saves per round, 3 rounds: 2 of 12 saves answered 500 (deadlock).
  • 12 saves one after another, each waiting for the previous answer: 1 answered 500 (deadlock), because a save's own follow-up reindex was still running when the next save arrived.
  • The Postgres log over these runs held 14 deadlock detected / duplicate key value violates unique constraint "search_index_pkey" errors, including Key (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

  • New repository/search_projection_lock.py: lock_entity_search_projection(session, project_id=, entity_id=) takes pg_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_data takes 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_entity and delete_entity take 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_index rows. 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 with search_index_pkey or deadlock detected.
  • test_concurrent_refreshes_keep_relation_rows_single and its negative control: 6 concurrent refreshes with content in hand, as the batch indexer calls it. Without the lock they fail with search_index_pkey on 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.py reverted, 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

  • Rewrites of one entity's search rows now queue. A long note re-indexed by several writers at once is indexed once per writer, one after another, rather than failing; the total work is unchanged.
  • The lock covers the two rewrite paths above. SearchRepository.delete_by_entity_id called on its own (note delete) and the bulk project reindex do not take it; they did not show up in the failures.
  • A refresh handed its text by the caller (content given) 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.

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant