diff --git a/hindsight-api-slim/hindsight_api/engine/db/ops_postgresql.py b/hindsight-api-slim/hindsight_api/engine/db/ops_postgresql.py index 7ba7c773c1..4d345901ad 100644 --- a/hindsight-api-slim/hindsight_api/engine/db/ops_postgresql.py +++ b/hindsight-api-slim/hindsight_api/engine/db/ops_postgresql.py @@ -1078,8 +1078,8 @@ async def drop_bank_vector_indexes( # EXCLUSIVE, on the shared memory_units table. A plain DROP INDEX blocks # (and deadlocks with) every other bank's concurrent reads/writes on the # table; CONCURRENTLY does not conflict with DML. The caller - # (delete_bank) runs this on an autocommit connection after its delete - # transaction has committed — CONCURRENTLY cannot run inside a tx. + # (delete_bank) runs this on an autocommit connection before its delete + # transaction starts — CONCURRENTLY cannot run inside a tx. # The lock key must match create_bank_vector_indexes', whose `table` # is the fq name this reconstructs from `schema`. async with self._index_ddl_lock(f"{schema}.memory_units"): diff --git a/hindsight-api-slim/hindsight_api/engine/memory_engine.py b/hindsight-api-slim/hindsight_api/engine/memory_engine.py index bdcb69070d..81d5dd185b 100644 --- a/hindsight-api-slim/hindsight_api/engine/memory_engine.py +++ b/hindsight-api-slim/hindsight_api/engine/memory_engine.py @@ -7871,6 +7871,10 @@ async def delete_bank( - All entities for this bank (if deleting all memory units) - All associated links, unit-entity associations, and co-occurrences + The bank's per-bank vector indexes are dropped BEFORE the data delete: + with enough banks, planning any statement against memory_units can + exhaust Postgres' lock table, while DROP INDEX still works (issue #3485). + Args: bank_id: bank ID to delete fact_type: Optional fact type filter (world, experience). If provided, only deletes memories of that type. @@ -7890,10 +7894,35 @@ async def delete_bank( backend = await self._get_backend() invalidated_obs = 0 result: dict[str, int] = {} - bank_internal_id: str | None = None async with acquire_with_retry(backend) as conn: # Ensure connection is not in read-only mode (can happen with connection poolers) await conn.execute("SET SESSION CHARACTERISTICS AS TRANSACTION READ WRITE") + + # Drop this bank's per-bank vector indexes BEFORE the delete + # transaction (issue #3485): with enough banks, planning any + # statement against memory_units exhausts Postgres' lock table, + # while DROP INDEX (CONCURRENTLY) still works — dropping first + # keeps deletion usable and restores an API-driven recovery path. + # Only the full-delete path drops indexes: fact_type-scoped + # deletes and delete_bank_profile=False keep them. + if not fact_type and delete_bank_profile: + internal_id = await conn.fetchval( + f"SELECT internal_id FROM {fq_table('banks')} WHERE bank_id = $1", bank_id + ) + if internal_id: + # Retry the transient deadlock a concurrent index + # build/drop on the shared memory_units table can still + # trigger (40P01 / ORA-00060). Sized well above the + # defaults: a many-process delete storm (CI teardown ran + # 8 workers at once) drains at ~1 deadlock victim per + # deadlock_timeout, so ~30s of jittered backoff outlasts + # any realistic pile-up. + await retry_with_backoff( + lambda: bank_utils.drop_bank_vector_indexes(conn, str(internal_id), ops=self._backend.ops), + max_retries=7, + max_delay=10.0, + ) + async with conn.transaction(): try: if fact_type: @@ -8024,36 +8053,14 @@ async def delete_bank( } if delete_bank_profile: - # Delete the bank profile and retrieve internal_id for HNSW index cleanup - internal_id = await conn.fetchval( - f"DELETE FROM {fq_table('banks')} WHERE bank_id = $1 RETURNING internal_id", bank_id - ) - if internal_id: - bank_internal_id = str(internal_id) + # Delete the bank profile (its vector indexes were + # already dropped above, before the delete tx). + await conn.execute(f"DELETE FROM {fq_table('banks')} WHERE bank_id = $1", bank_id) result["bank_deleted"] = True except Exception as e: raise Exception(f"Failed to delete agent data: {str(e)}") - # Drop per-bank vector indexes AFTER the transaction commits: the - # drop runs CONCURRENTLY (see ops.drop_bank_vector_indexes), which - # cannot run inside a transaction block. Same-process drops are - # serialized by the ops-level DDL lock; retry_with_backoff absorbs - # the residual cross-process deadlock a concurrent index build/drop - # on the shared memory_units table can still trigger (sqlstate - # 40P01 / ORA-00060) so a delete is never lost to a transient lock - # cycle. Sized well above the defaults: a many-process delete storm - # (CI teardown ran 8 workers' drops at once) drains at roughly one - # deadlock victim per deadlock_timeout (1s), so the default ~2.4s - # of backoff lost every retry; ~30s of jittered backoff outlasts - # any realistic pile-up. - if bank_internal_id: - await retry_with_backoff( - lambda: bank_utils.drop_bank_vector_indexes(conn, bank_internal_id, ops=self._backend.ops), - max_retries=7, - max_delay=10.0, - ) - # A store that keeps memories outside SQL leaves memory_units empty, so every DELETE # above was a no-op on its data — it must be told to drop the bank's memories too, or # they are orphaned. Runs after the transaction: it is an external-store call, not SQL. diff --git a/hindsight-api-slim/hindsight_api/engine/retain/bank_utils.py b/hindsight-api-slim/hindsight_api/engine/retain/bank_utils.py index 304366c2d9..7ba1feb777 100644 --- a/hindsight-api-slim/hindsight_api/engine/retain/bank_utils.py +++ b/hindsight-api-slim/hindsight_api/engine/retain/bank_utils.py @@ -78,8 +78,9 @@ async def create_bank_vector_indexes(conn, bank_id: str, internal_id: str, ops=N async def drop_bank_vector_indexes(conn, internal_id: str, ops=None) -> None: """Drop per-(bank, fact_type) partial vector indexes for a bank being deleted. - Called before the bank row is deleted so internal_id is still known. - Idempotent via DROP INDEX IF EXISTS. + Called before the bank's data is deleted: delete_bank runs the drop on an + autocommit connection ahead of its delete transaction, so the bank row (and + its internal_id) still exists. Idempotent via DROP INDEX IF EXISTS. On Oracle, this is a no-op (uses single global vector index). """ diff --git a/hindsight-api-slim/tests/test_bank_lifecycle_deadlock_retry.py b/hindsight-api-slim/tests/test_bank_lifecycle_deadlock_retry.py index ccb39a437f..bc2cea3b86 100644 --- a/hindsight-api-slim/tests/test_bank_lifecycle_deadlock_retry.py +++ b/hindsight-api-slim/tests/test_bank_lifecycle_deadlock_retry.py @@ -4,9 +4,10 @@ (``public`` schema), so every bank create/delete does index DDL on the same ``memory_units`` table other workers are writing. A fresh bank builds its partial indexes with a plain ``CREATE INDEX`` inside the bank-create tx -(ShareLock), and ``delete_bank`` drops them (CONCURRENTLY, post-commit); both -can be chosen as a deadlock victim. These are the exact production paths that -flaked in CI — they must retry the transient deadlock, not surface it. +(ShareLock), and ``delete_bank`` drops them (CONCURRENTLY, on an autocommit +connection before its delete tx); both can be chosen as a deadlock victim. +These are the exact production paths that flaked in CI — they must retry the +transient deadlock, not surface it. The deadlock is injected via monkeypatch (one-shot ``DeadlockDetectedError``) so the retry path is exercised deterministically, without racing real workers. diff --git a/hindsight-api-slim/tests/test_hnsw_indexes.py b/hindsight-api-slim/tests/test_hnsw_indexes.py index cf9ff9bd5c..ad950d23d3 100644 --- a/hindsight-api-slim/tests/test_hnsw_indexes.py +++ b/hindsight-api-slim/tests/test_hnsw_indexes.py @@ -118,6 +118,47 @@ async def test_delete_bank_drops_vector_indexes(memory, request_context): assert indexes_after == [], f"Indexes should be dropped after bank deletion, got: {indexes_after}" +@pytest.mark.asyncio +async def test_delete_bank_drops_indexes_before_data(memory, request_context, monkeypatch): + """delete_bank must drop the per-bank vector indexes BEFORE the delete transaction. + + Regression test for #3485: when the cluster-wide index count approaches the + Postgres lock-table budget, planning ANY statement against memory_units + fails — including the delete DML. DROP INDEX (a utility statement) only + locks its own index and the table, so it keeps working; dropping first is + what keeps bank deletion usable at high bank counts. + """ + bank_id = f"test_hnsw_order_{uuid.uuid4().hex[:8]}" + await memory.retain_async( + bank_id=bank_id, + content="Dana is a research scientist.", + request_context=request_context, + ) + + backend = await memory._get_backend() + real = backend.ops.drop_bank_vector_indexes + drop_rows_present: bool | None = None + drop_profile_present: bool | None = None + + async def recording_drop(conn, schema, internal_id, fact_types): + # At drop time the bank's memory rows and profile must still exist — + # the drop runs before the delete transaction, not after it. + nonlocal drop_rows_present, drop_profile_present + drop_rows_present = (await conn.fetchval("SELECT COUNT(*) FROM memory_units WHERE bank_id = $1", bank_id)) > 0 + drop_profile_present = ( + await conn.fetchval("SELECT internal_id FROM banks WHERE bank_id = $1", bank_id) + ) is not None + return await real(conn, schema, internal_id, fact_types) + + monkeypatch.setattr(backend.ops, "drop_bank_vector_indexes", recording_drop) + + await memory.delete_bank(bank_id, request_context=request_context) + + assert drop_rows_present is True, "indexes must be dropped while the bank's rows still exist" + assert drop_profile_present is True, "indexes must be dropped before the bank profile is deleted" + assert await _get_bank_vector_indexes(memory._pool, bank_id) == [] + + @pytest.mark.asyncio async def test_retain_idempotent_bank_creation(memory, request_context): """Retaining into the same bank twice must not error and still have exactly 3 indexes."""