fix: add unique constraint on conference to prevent duplicate split#21
fix: add unique constraint on conference to prevent duplicate split#21Boanerges1996 wants to merge 3 commits intopeermetrics:masterfrom
Conversation
…ent duplicates When two participants join the same session simultaneously, both call POST /v1/initialize with the same conferenceId. Without a unique constraint, both get_or_create calls create separate conference records (race condition). This splits participants across different conference pages. Fix: - Migration 0002: merges existing duplicates (keeps oldest, moves all related events/sessions/connections/issues to it) - Migration 0003: adds unique_together on (conference_id, app) After this, concurrent get_or_create calls will correctly find the existing conference instead of creating duplicates.
Boanerges1996
left a comment
Boanerges1996
left a comment
There was a problem hiding this comment.
Overall this is a clean, well-structured fix. The root cause analysis is correct — get_or_create without a DB-level unique constraint is a textbook race condition. Splitting data + schema migrations is the right call. A few comments inline.
|
|
||
| class Meta: | ||
| db_table = 'conference' | ||
| unique_together = (('conference_id', 'app'),) |
There was a problem hiding this comment.
nit: Django docs now recommend constraints with UniqueConstraint over the older unique_together (which is soft-deprecated since Django 4.2). Since you're on Django 2.2, both work — UniqueConstraint was introduced in 2.2. Not a blocker, just flagging for when you upgrade:
class Meta:
db_table = 'conference'
constraints = [
models.UniqueConstraint(fields=['conference_id', 'app'], name='unique_conference_per_app')
]- Moved imports to top of file/function - Wrapped each duplicate group in transaction.atomic() - Added Summary model to reassignment (OneToOne FK to Conference) - Added safety check on dup.delete() to catch unexpected cascades
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55758d3a63
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
P1: Removed over-strict cascade-delete guard. Deleting a duplicate conference normally cascades to M2M join table rows (participant through table), which is expected. The guard was raising on this. P2: Replaced hasattr(keeper, 'summary') with a local boolean flag (keeper_has_summary) that updates after the first summary is moved. Avoids stale cached reverse OneToOne lookups across iterations.
2 participants
Problem
When two participants join the same telehealth session simultaneously, both call
POST /v1/initializewith the sameconferenceId. Without a database-level unique constraint on(conference_id, app_id), bothget_or_createcalls can create separate conference records — splitting participants across different conference pages.This is a race condition: both requests do
SELECT(finds nothing), then both doINSERT(both succeed). Django'sget_or_createonly retries onIntegrityError, which requires a unique constraint to trigger.Compare with Participant model which already has
unique_together = (('participant_id', 'app'),)and doesn't have this issue.Fix
Model: Added
unique_together = (('conference_id', 'app'),)to Conference.Migrations (split into two to avoid pending trigger conflict):
0002: Data migration — merges existing duplicate conferences. Keeps the oldest, moves all related events/sessions/connections/issues/participants to it, deletes the rest.0003: Schema migration — adds the unique constraint.Verified locally
Test plan