Stop recycling game ids and lock the game row on delete - #1313
Open
johnpooch wants to merge 2 commits into
Open
Conversation
A game id was the slug of its name, and only fell back to a uuid-suffixed form when the bare slug was already taken. Deleting a game therefore freed its id, and the next game with that name reclaimed it. A request holding the freed id then acted on an unrelated row: DELETE /game/<id>/delete/ collected one game's members, deleted the row a later game had created under the same id, and left that game's members behind, so the deferred foreign key rejected the transaction at COMMIT with a ForeignKeyViolation. Always suffix the slug, so an id is issued once and never reissued. The availability check and the IntegrityError retry it needed both go away. Delete also ran unlocked and outside a transaction of its own: Django cascades in Python, collecting children in one snapshot and removing them in another. Re-fetch the game under select_for_update() inside the transaction that deletes it, and re-check permissions against the locked row. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KrbHq68GmJquPDib3fYkc3
…44911237-cwb4ck # Conflicts: # .claude/rules/backend/models.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Fixes the
ForeignKeyViolationraised at COMMIT byDELETE /game/{game_id}/delete/(Sentry DIPLICITY-API-AA):update or delete on table "game_game" violates foreign key constraint "member_member_game_id_f16d3bd2_fk_game_game_id".A game id was the slug of its name (
service/game/models.py:506), falling back to a uuid-suffixed form only when the bare slug was already taken. Deleting a game therefore freed its id, and the next game with the same name reclaimed it.GameCloneToSandboxSerializermakes that churn constant: it deletes the user's oldest sandbox and then creates a new game namedf"{source_game.name} (Sandbox)"(service/game/serializers.py:637-641), i.e. the exact name whose slug it just freed.A request holding the freed id then acts on an unrelated row.
perform_destroycollected one game's members, deleted whichever row now held that id, and left the newer game's members behind.member_member.game_idisDEFERRABLE INITIALLY DEFERREDwithON DELETE NO ACTION(verified against productionpg_constraint), so Postgres checks at COMMIT, finds no game row for the key and 25 orphaned members, and rejects the transaction.Two changes:
service/game/models.py). An id is issued once and never reissued, so a stale id can never resolve to a different game. The availability check and theIntegrityErrorretry it needed both go away — which is also why the create query-count assertions drop by 3.service/game/views.py). Django cascades in Python: the unlocked, autocommit delete collected children in one snapshot and removed them in another, leaving a window for concurrent writes.perform_destroynow re-fetches the game underselect_for_update()inside the transaction that deletes it, and re-checks permissions against the locked row — the patternSeatClaimMixinalready uses.A concurrent delete of a game that is already gone now returns 404 rather than 500.
Checklist
/review-pragainst this PR in Claude Code and addressed (or responded to) its findingsTests: full backend suite green (2350 passed, 8 skipped). New coverage —
test_recreated_sandbox_game_does_not_reuse_deleted_idreproduces the production sequence (create sandbox → delete → recreate under the same name) over HTTP, andtest_delete_sandbox_game_twice_returns_404pins the repeat-delete behaviour.🤖 Generated with Claude Code
https://claude.ai/code/session_01KrbHq68GmJquPDib3fYkc3
Generated by Claude Code