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

[sammywachtel]

Review copy. On the personal fork for review before it goes to basicmachines-co/basic-memory. Merging it here is not the goal.

Proposed upstream title: fix(core): serialize search-index rewrites of one entity on Postgres. The body below is the draft upstream description.

This fork's main is behind upstream, so the Files tab here also shows upstream commits. The change under review is the one commit on top: 03ef4ba

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 basicmachines-co#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.

[sammywachtel]

Opened upstream as basicmachines-co#1665; this review copy is no longer needed. The branch stays, because the upstream PR is opened from it.