fix(graph-maintenance): sort unit_ids to eliminate concurrent-insert deadlock#2353
Conversation
…deadlock
`enqueue_graph_maintenance` is called inside the same transaction as the
mutation that produced its `unit_ids` list (see `enqueue_relink_victims`
after a memory update, document delete, etc.). The INSERT it issues takes
a short-lived row-level lock per `(bank_id, unit_id)` for the
unique-key check (`ON CONFLICT DO NOTHING` on Postgres, the
`IGNORE_ROW_ON_DUPKEY_INDEX` hint on Oracle).
Under load, two concurrent transactions on the same bank can produce
overlapping `unit_ids` sets in different orders — most easily reproduced
by two concurrent `PATCH /v1/default/banks/{bank_id}/memories/{id}`
requests where the victim sets (surviving units linking to the patched
unit) intersect. When the two transactions try to acquire their per-row
locks in opposite orders, Postgres detects the cycle and aborts one
transaction with `asyncpg.exceptions.DeadlockDetectedError`, which the
FastAPI layer surfaces as an opaque 500.
Fix: sort `unit_ids` inside both `PostgreSQLOps.enqueue_graph_maintenance`
and `OracleOps.enqueue_graph_maintenance` before issuing the INSERT.
With a total order over the lock set, deadlock is mathematically
impossible — both transactions queue cleanly on the first conflicting
row, then proceed in lockstep.
The only public caller (`enqueue_relink_victims` in
`hindsight_api/engine/graph_maintenance.py`) doesn't rely on insertion
order, so this is a pure correctness improvement with no API-visible
effect. The abstract contract docstring already said "Order is
unspecified" — implementations now happen to pick a deterministic
order, but that's an internal invariant, not part of the public
contract.
Tests:
- `tests/test_enqueue_graph_maintenance_ordered.py` (new):
- `test_pg_enqueue_graph_maintenance_inserts_in_sorted_order` —
captures the array passed to `conn.execute` from a deliberately
shuffled input and asserts it is sorted.
- `test_oracle_enqueue_graph_maintenance_inserts_in_sorted_order` —
same assertion against `conn.executemany`'s tuples.
- Two empty-input tests pin the early-return short-circuit (no INSERT
when `unit_ids == []`).
- Verified existing `tests/test_graph_maintenance.py` still passes
(14/14) — the relink-victim enqueue and drain semantics are unchanged.
Compatibility: identical on both dialects. No schema changes. No
externally-visible behavior change beyond the deadlock no longer
firing.
nicoloboschi
left a comment
nicoloboschi
left a comment
There was a problem hiding this comment.
Approving. I reproduced the deadlock against real Postgres with two concurrent transactions inserting overlapping keys in opposite order (PG aborts one with DeadlockDetectedError after deadlock_timeout), and confirmed that a shared sort order eliminates the cycle — so the fix is correct for the queue-insert path.
One caveat for the record: the PR description's "deadlock is mathematically impossible" is overstated. The same PATCH transaction also issues an unsorted entities ... ON CONFLICT ... DO UPDATE per resolved entity (entity_resolver.py:936) before reaching the queue insert, which can still deadlock on overlapping entity sets. This PR correctly fixes the queue insert; the entity-upsert path is a separate follow-up (a deadlock-retry around the transaction would cover all sites at once, since acquire_with_retry deliberately does not retry in-transaction errors).
|
Correction to my approval note above: I went and audited the full The
So Follow-up: I'll open a small PR to delete the dead |
2 participants
Summary
enqueue_graph_maintenance(in both the PostgreSQL and Oracle data-accessops impls) issues an
INSERT INTO graph_maintenance_queue (bank_id, unit_id)with
ON CONFLICT DO NOTHING(PG) /IGNORE_ROW_ON_DUPKEY_INDEX(Oracle).The unique-key check takes a short-lived row-level lock per
(bank_id, unit_id)being inserted.When two transactions on the same bank execute concurrently with
overlapping but differently-ordered
unit_idsarrays, the per-row locksget acquired in opposite orders. Postgres detects the cycle and aborts
one transaction with
asyncpg.exceptions.DeadlockDetectedError, whichthe FastAPI layer surfaces as an opaque 500.
This is straightforward to hit from two concurrent
PATCH /v1/default/banks/{bank_id}/memories/{id}requests where therespective victim sets (surviving units whose outgoing links pointed at
the patched units) intersect —
enqueue_relink_victimsthen enqueueseach transaction's victim list in arbitrary
SELECT DISTINCTorder.Fix
Sort
unit_idsinside bothPostgreSQLOps.enqueue_graph_maintenanceandOracleOps.enqueue_graph_maintenancebefore issuing the INSERT. With atotal order over the lock set, deadlock is mathematically impossible —
both transactions queue cleanly on the first conflicting row, then
proceed in lockstep.
The only public caller (
enqueue_relink_victimsinhindsight_api/engine/graph_maintenance.py) doesn't rely on insertionorder, and the abstract method docstring already documents "Order is
unspecified". This change picks a deterministic implementation-internal
order without strengthening the public contract.
Test plan
hindsight-api-slim/tests/test_enqueue_graph_maintenance_ordered.py(new file, 4 tests):
test_pg_enqueue_graph_maintenance_inserts_in_sorted_order—captures the array passed to
conn.executefrom a deliberatelyshuffled input and asserts it is sorted.
test_oracle_enqueue_graph_maintenance_inserts_in_sorted_order—same assertion against
conn.executemany's tuples.test_pg_empty_unit_ids_short_circuits/test_oracle_empty_unit_ids_short_circuits— pin the existingearly-return when
unit_ids == [].Regression sweep on the existing graph-maintenance suite —
14/14 passing in
tests/test_graph_maintenance.py(relink semanticsunchanged).
ruff check/ruff formatclean. Pre-commit hooks pass.Compatibility
determinism. No schema changes.
ON CONFLICT DO NOTHINGsemanticsunchanged.
IGNORE_ROW_ON_DUPKEY_INDEXhint unchanged.no longer firing under concurrent load.