Skip to content

fix(retain): globalise memory_links lock order on the insert path (#3396) - #3406

Merged
nicoloboschi merged 1 commit into
mainfrom
fix-memory-links-lock-order-3396
Aug 12, 2026
Merged

fix(retain): globalise memory_links lock order on the insert path (#3396)#3406
nicoloboschi merged 1 commit into
mainfrom
fix-memory-links-lock-order-3396

Conversation

@nicoloboschi

Copy link
Copy Markdown
Collaborator

Fixes #3396.

The deadlock #2570 targeted still fired a few times a day (per @kubaodias in #3387): DeadlockDetectedError from link_utils during temporal-link insertion, absorbed by the worker retry. Reading both write paths, the insert-side lock ordering was only partial in two ways.

What was wrong

1. The insert sort key covered only half the unique key. _bulk_insert_links sorted on (from_unit_id, to_unit_id) — two of the four columns in the unique index (from, to, link_type, COALESCE(entity_id, nil)). A temporal and a semantic edge on the same (from, to) pair compared equal; Python's stable sort then left them in input order, which isn't stable across transactions, so two concurrent inserts could take the two index entries in opposite orders.

2. Insert and delete used different total orders. chunk_storage.delete_chunks_by_ids orders by (LEAST(from, to), GREATEST(from, to), link_type, COALESCE(entity_id, nil)) — direction-normalised so (A,B) and (B,A) sort adjacent. The insert sorted on the raw (from, to), placing them far apart. Both orders are internally total but different, and a consistent lock order requires every writer to share one.

The fix

Both call sites now sort on one canonical key — the full, direction-normalised unique key. Extracted _lock_order_key and pointed the insert sort at it; the delete side already uses exactly this order and is unchanged.

(LEAST(from, to), GREATEST(from, to), link_type, COALESCE(entity_id, nil))

UUID string ordering matches Postgres uuid byte ordering because the ids are canonical lowercase-hex (the same assumption the old str() sort already relied on).

Tests

TestLockOrderKey in tests/test_link_utils.py — 4 pure unit tests: direction normalisation, link_type disambiguation (mechanism 1), None → nil-UUID mapping, and that a mixed batch reproduces the delete's ORDER BY. Full file: 31/31 pass; ruff + ty clean.

Note

As the issue says, this is derived from reading the two paths, not a reproduction — it closes both ordering gaps rather than betting on which one fires in production. Severity is low (a few/day, fully absorbed by the worker retry); this makes the #2570 ordering guarantee actually global.

)

The deadlock #2570 targeted still fired a few times a day because the
insert-side lock ordering was only partial:

1. _bulk_insert_links sorted on (from, to) — two of the four columns in
   the unique index (from, to, link_type, COALESCE(entity_id, nil)). A
   temporal and a semantic edge on the same pair compared equal, so a
   stable sort left them in input order and concurrent inserts could take
   the two index entries in opposite orders.

2. That order also disagreed with chunk_storage.delete_chunks_by_ids,
   which normalises direction via LEAST/GREATEST. Two different total
   orders can still cycle.

Sort both paths on one canonical key — the full, direction-normalised
unique key — by extracting _lock_order_key and pointing the insert sort
at it. The delete side already uses exactly this order and is unchanged.
@kubaodias

Copy link
Copy Markdown
Contributor

Heads-up: this crossed with new evidence on #3396 — server-log DETAIL blocks plus a local repro show the firing cycle waits on memory_units tuples (delete-side X locks from earlier statements in the retain txn vs the locked CTE's FOR KEY SHARE), not on memory_links index entries, so the sort-key change here won't reduce the observed deadlocks. Details in #3396 (comment). The change itself looks harmless and the ordering unification is reasonable hygiene — but it may be worth softening "Fixes #3396" to "Refs #3396" so the issue stays open for the cross-statement cycle.

@nicoloboschi
nicoloboschi merged commit 83a080f into main Aug 12, 2026
103 of 107 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

memory_links lock ordering is still not global: insert sorts on half the unique key, and disagrees with the delete's order

2 participants