fix(engine): drop per-bank vector indexes before bank delete data (#3485)

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"):

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.

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).
     """

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.

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."""