Skip to content

Let PUT cards swap fronts and lock cards in id order - #101

Merged
lesnik512 merged 1 commit into
mainfrom
fix/card-update-locking
Oct 17, 2026
Merged

lesnik512 merged 1 commit into
mainfrom
fix/card-update-locking

Conversation

@lesnik512

Copy link
Copy Markdown
Member

Fixes two bugs in PUT /decks/{id}/cards/. Each has a test that failed on main first.

Swapping fronts in one PUT

Swapping front between two cards of a deck failed with a duplicate-key error. card_deck_id_front_uc was not deferrable, and Postgres checks a non-deferrable unique constraint after each row, so the first row's new value collided with the second row's old one. The constraint is now DEFERRABLE INITIALLY IMMEDIATE, with a migration that drops and recreates it. Postgres then checks it at the end of the statement, and a real duplicate still fails.

  • A real duplicate still raises 23505 with the same message. advanced-alchemy matches that message, so a duplicate front still maps to DuplicateKeyError, and test_update_cards_to_a_duplicate_front still passes.
  • Postgres rejects a deferrable constraint as an ON CONFLICT arbiter. The only ON CONFLICT in the code is on decks.idempotency_key, so nothing is affected.

Deadlock between concurrent PUTs

The UPDATE plan is a nested loop driven by unnest, so rows were locked in the order the client sent them. Two PUTs that touch the same cards in different orders could each hold a row the other needs. Sorting the input would not be enough, because the planner can pick a different join order.

The UPDATE now joins a CTE:

WITH locked AS (
  SELECT id FROM cards WHERE id = ANY(:ids) AND deck_id = :deck_id ORDER BY id FOR UPDATE
)
UPDATE cards ... FROM unnest(...) AS updates(...), locked WHERE cards.id = updates.id AND cards.id = locked.id ...

Every updated row has to match a locked row. The CTE locks rows in id order before each one is updated (it runs lazily, so the locks aren't all taken up front), and that is enough to keep the lock order the same across concurrent requests. It is still one statement and one round trip. ids is bound once and used twice, so the SQL text stays the same for any number of cards.

Tests

  • test_update_cards_swaps_fronts: a PUT that swaps two fronts returns 200 with the swapped values.
  • tests/test_card_locking.py uses a real engine, because the interleaving needs separate connections:
    • A third connection holds a lock on card 3.
    • PUT A sends [1, 3, 2] and waits on card 3. PUT B sends [2, 1]. The test polls pg_stat_activity on a fresh connection each time, because the view is snapshotted per transaction. Then it releases the lock.
    • On main this raises DeadlockDetectedError. It does the same with the locked join removed from this branch. With the join, both PUTs return 200, and the test passed 3 runs out of 3.
    • On any failure path the test cancels its requests and deletes the rows it committed.
  • The real-engine fixture from test_autocommit_writes.py moved to conftest.py as committing_engine, and both tests use it.
  • just test passes 87 tests at 100% coverage, just test-migrations passes, and just lint is clean.

Make card_deck_id_front_uc DEFERRABLE INITIALLY IMMEDIATE so uniqueness is checked at the end of the statement and one PUT can swap two cards' fronts. Join the UPDATE to a locked CTE that selects the rows ORDER BY id FOR UPDATE, so concurrent PUTs on overlapping cards lock them in the same order instead of deadlocking.
@lesnik512
lesnik512 merged commit 1cc521f into main Oct 17, 2026
3 checks passed
@lesnik512
lesnik512 deleted the fix/card-update-locking branch October 17, 2026 13:18
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.

1 participant