CI & Build / Python lint (push) Successful in 5s
CI & Build / Plugin hooks (push) Successful in 16s
CI & Build / TypeScript typecheck (push) Successful in 37s
CI & Build / integration (push) Successful in 38s
CI & Build / Python tests (push) Failing after 55s
CI & Build / Build & push image (push) Skipped
An embedding refresh replaces a record's vectors as delete-then-insert, which takes the chunk rows first and the parent row second (via the insert's foreign key). A cascading delete of the parent takes exactly those two locks in the other order. Postgres calls the cycle a deadlock and kills one side: sometimes the detached embedder, silently, and sometimes the user's delete, as a 500 on an operation that should have worked. Both upserts now claim the parent row with FOR KEY SHARE NOWAIT before touching any chunk row. That removes the cycle instead of narrowing it — either the embedder is first and the delete queues behind it, or the delete already holds the row and the embedder loses at once, which is the side designed to lose. FOR KEY SHARE is the lock the insert would take anyway, so an ordinary edit is unaffected. The note twin, recorded as unverified on the issue, has the same shape and the same fix; a trash purge is the hard delete that reaches it. Unit tests pin the ORDER and the lock mode by compiling the statement; the integration pair holds a real delete open in one transaction and proves the embedder returns having written nothing, with a deadline so a regression fails instead of hanging.
119 lines
5.1 KiB
Python
119 lines
5.1 KiB
Python
"""The embedding refresh must LOSE to a delete, not race it (#3262).
|
|
|
|
Both upserts replace a record's vectors as delete-then-insert. That takes two
|
|
row locks — the chunk rows, then the parent row via the insert's foreign key —
|
|
in the exact reverse of the order a cascading delete of the parent takes them.
|
|
Postgres calls that a deadlock and kills one side at random, which sometimes
|
|
means killing the user's delete.
|
|
|
|
These pin the ORDER, not the outcome: the claim on the parent goes first, and
|
|
when the claim fails nothing else in the transaction runs. Compiling the
|
|
statement is the only way to assert on a lock mode without a database — the
|
|
integration twin (test_integration_embedding_yields_to_delete.py) proves the
|
|
behaviour against a real one.
|
|
"""
|
|
from unittest.mock import AsyncMock, MagicMock, patch
|
|
|
|
from sqlalchemy.dialects import postgresql
|
|
from sqlalchemy.exc import OperationalError
|
|
|
|
from scribe.services import embeddings as emb
|
|
from tests.helpers import compiled_sql
|
|
|
|
ONE_VECTOR = [[0.0] * 384]
|
|
|
|
# The lock mode is a Postgres extension — the generic dialect renders a plain
|
|
# FOR UPDATE and would pass an assertion that proves nothing.
|
|
PG = postgresql.dialect()
|
|
|
|
|
|
def _mock_session(lock_result: object = 7, execute_side_effect=None):
|
|
"""A session stand-in whose first execute answers the parent-row claim."""
|
|
session = MagicMock()
|
|
claimed = MagicMock()
|
|
claimed.scalar_one_or_none.return_value = lock_result
|
|
if execute_side_effect is not None:
|
|
session.execute = AsyncMock(side_effect=execute_side_effect)
|
|
else:
|
|
session.execute = AsyncMock(return_value=claimed)
|
|
session.commit = AsyncMock()
|
|
session.add = MagicMock()
|
|
ctx = MagicMock()
|
|
ctx.__aenter__ = AsyncMock(return_value=session)
|
|
ctx.__aexit__ = AsyncMock(return_value=False)
|
|
return session, ctx
|
|
|
|
|
|
def _lock_unavailable() -> OperationalError:
|
|
"""What asyncpg raises through SQLAlchemy when NOWAIT can't take the row."""
|
|
return OperationalError("SELECT ...", {}, Exception("lock not available"))
|
|
|
|
|
|
async def test_a_note_refresh_claims_the_row_before_rewriting_its_vectors():
|
|
"""The claim is FIRST, and it is FOR KEY SHARE NOWAIT.
|
|
|
|
FOR KEY SHARE because that is exactly the lock the insert's foreign key
|
|
takes anyway — it conflicts with a delete of the note and with nothing
|
|
else, so an ordinary edit is unaffected. NOWAIT because the whole point is
|
|
to lose immediately rather than queue up behind the delete and hold the
|
|
chunk rows while doing it.
|
|
"""
|
|
session, ctx = _mock_session()
|
|
with (
|
|
patch.object(emb, "async_session", return_value=ctx),
|
|
patch.object(emb, "get_embeddings", AsyncMock(return_value=ONE_VECTOR)),
|
|
):
|
|
await emb.upsert_note_embedding(7, 42, "T", "a short body")
|
|
|
|
claim, replace = [c.args[0] for c in session.execute.call_args_list][:2]
|
|
assert compiled_sql(claim, dialect=PG).startswith("SELECT notes.id")
|
|
assert "FOR KEY SHARE NOWAIT" in compiled_sql(claim, dialect=PG)
|
|
assert compiled_sql(replace, dialect=PG).startswith("DELETE FROM note_embeddings")
|
|
session.add.assert_called()
|
|
|
|
|
|
async def test_a_rule_refresh_claims_the_row_before_rewriting_its_vectors():
|
|
"""The rule twin — the path the reported deadlock actually took."""
|
|
session, ctx = _mock_session()
|
|
with (
|
|
patch.object(emb, "async_session", return_value=ctx),
|
|
patch.object(emb, "get_embeddings", AsyncMock(return_value=ONE_VECTOR)),
|
|
):
|
|
await emb.upsert_rule_embedding(9, "T", "a short statement", "on write")
|
|
|
|
claim, replace = [c.args[0] for c in session.execute.call_args_list][:2]
|
|
assert compiled_sql(claim, dialect=PG).startswith("SELECT rules.id")
|
|
assert "FOR KEY SHARE NOWAIT" in compiled_sql(claim, dialect=PG)
|
|
assert compiled_sql(replace, dialect=PG).startswith("DELETE FROM rule_embeddings")
|
|
session.add.assert_called()
|
|
|
|
|
|
async def test_a_record_being_deleted_is_left_alone_rather_than_raced():
|
|
"""The claim failing ends the write — it does not fall through to the
|
|
delete-and-insert that would take the locks in the losing order."""
|
|
session, ctx = _mock_session(execute_side_effect=_lock_unavailable())
|
|
with (
|
|
patch.object(emb, "async_session", return_value=ctx),
|
|
patch.object(emb, "get_embeddings", AsyncMock(return_value=ONE_VECTOR)),
|
|
):
|
|
await emb.upsert_rule_embedding(9, "T", "a short statement", "on write")
|
|
|
|
assert session.execute.await_count == 1, "it stopped at the claim"
|
|
session.add.assert_not_called()
|
|
session.commit.assert_not_awaited()
|
|
|
|
|
|
async def test_a_record_already_gone_is_not_re_embedded():
|
|
"""A vector inserted for a row that no longer exists is either a foreign
|
|
key violation or, worse, a resurrected chunk. Nothing to refresh."""
|
|
session, ctx = _mock_session(lock_result=None)
|
|
with (
|
|
patch.object(emb, "async_session", return_value=ctx),
|
|
patch.object(emb, "get_embeddings", AsyncMock(return_value=ONE_VECTOR)),
|
|
):
|
|
await emb.upsert_note_embedding(7, 42, "T", "a short body")
|
|
|
|
assert session.execute.await_count == 1
|
|
session.add.assert_not_called()
|
|
session.commit.assert_not_awaited()
|