fix: add unique index on subscriptions to prevent duplicate entries#2553
Draft
mroderick wants to merge 2 commits intocodebar:masterfrom
Draft
fix: add unique index on subscriptions to prevent duplicate entries#2553mroderick wants to merge 2 commits intocodebar:masterfrom
mroderick wants to merge 2 commits intocodebar:masterfrom
Conversation
Members subscribed to multiple groups within a chapter were being processed multiple times when sending invitations with audience='everyone', causing 'Member has already been taken' validation errors. Root Cause: - Member.in_group() uses JOIN on subscriptions, returning duplicate rows when a member is subscribed to the same group type multiple times - 336 members in London chapter affected (subscribed to both Students and Coaches) - 35 members in Berlin chapter affected - 57 members in Brighton chapter affected Impact: - Workshop invitation batches failed mid-process (e.g., 517 of 7000+ invitations sent before failure) - InvitationLogEntry validates uniqueness of member_id per invitation, causing validation errors on second processing attempt - Admins had to manually retry batch sends multiple times Affected Chapters: - 46 of 54 chapters (85%) have members with duplicate subscriptions - ~1,200 members affected across all chapters - Top 5: London (336), West London (79), Brighton (57), South London (57), Barcelona (55), Edinburgh (37), Berlin (35) Timeline: - Original code (Jan 2017, commit 448acc7) included .uniq for deduplication - Refactoring (May 2018, commit d7a463d) extracted to scopes but accidentally removed the .uniq call Fix: Add .distinct to chapter_students and chapter_coaches methods, restoring the deduplication behavior that existed before the refactoring. Tests: - Add spec for unique members when subscribed to same group multiple times - Add spec for unique invitations per role when member is in both groups
mroderick
force-pushed
the
fix/subscription-unique-index
branch
2 times, most recently
from
c62c0bd to
86f89d3
Compare
- Uses disable_ddl_transaction! (https://api.rubyonrails.org/classes/ActiveRecord/Migration.html#method-i-disable_ddl_transaction-21) - Uses algorithm: :concurrently (https://api.rubyonrails.org/classes/ActiveRecord/ConnectionAdapters/SchemaStatements.html#method-i-add_index) - Removes 31 duplicate subscriptions from production database - Prevents race conditions from causing duplicate entries in future Why (member_id, group_id) not (member_id, chapter_id)? - Index on (member_id, group_id) allows a member to be both student AND coach in the same chapter - Each chapter has separate "Students" and "Coaches" groups - different group_id values - This index prevents duplicate subscriptions to the SAME group (the race condition bug) - But allows valid dual membership: member can subscribe to group_id=1 (Students) AND group_id=2 (Coaches)
mroderick
force-pushed
the
fix/subscription-unique-index
branch
from
86f89d3 to
d881296
Compare
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.
Please review #2551 first, and then this one ... or roll a 1d20 sanity check 🎲
Summary
Adds a database-level unique index on
(member_id, group_id)in the subscriptions table to prevent race conditions that cause duplicate subscriptions.Problem
The Rails model validation
validates :group, uniqueness: { scope: :member_id }can be bypassed by race conditions when two concurrent requests both pass validation before either saves to the database.Evidence from Production
Analysis of production data revealed 25 unique member/group combinations with duplicate subscriptions created within 3ms to 2.5 seconds of each other - classic race condition pattern spanning 2015-2023.
Solution
(member_id, group_id)to enforce at database levelWhy (member_id, group_id) not (member_id, chapter_id)?
The index on
(member_id, group_id)allows a member to be both a student AND a coach in the same chapter:group_idgroup_id=1(London Students) ANDgroup_id=2(London Coaches)Using
(member_id, chapter_id)would incorrectly prevent the student+coach combination.Migration Details
disable_ddl_transaction!to allow concurrent index creationalgorithm: :concurrentlyto avoid table locks during production deploymentsTesting
Related