Skip to content

feat: Implement atomic transaction for activating broadcast messages … #11

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mihf05 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: Implement atomic transaction for activating broadcast messages … #11

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8f3b242
feat: Implement atomic transaction for activating broadcast messages …
mihf05 Dec 19, 2025
d7f77f6
refactor: Optimize atomic transaction handling in BroadcastMessage mo…
mihf05 Dec 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions server/broadcast/api_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from rest_framework.decorators import action, api_view, permission_classes
from rest_framework.response import Response
from rest_framework.permissions import IsAuthenticated, AllowAny
from django.db import transaction
from .models import BroadcastMessage
from .serializers import BroadcastMessageSerializer
from dashboard.models import UserDetails
Expand Down Expand Up @@ -46,8 +47,17 @@ def set_active(self, request, pk=None):
if message.user != request.user and not request.user.is_staff:
return Response({'error': 'Permission denied'}, status=status.HTTP_403_FORBIDDEN)

message.active = True
message.save()
# Use atomic transaction with row locking to prevent race conditions
with transaction.atomic():
# Lock and deactivate all user's messages
BroadcastMessage.objects.select_for_update().filter(
user=request.user, active=True
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an inconsistency in which user's messages are being deactivated. The permission check on line 47 validates that message.user matches request.user (or user is staff), but line 54 deactivates messages for request.user. If a staff user is activating another user's message, this will deactivate the staff user's messages instead of the message owner's messages.

The filter should use message.user instead of request.user to ensure the correct user's messages are deactivated.

Suggested change
# Lock and deactivate all user's messages
BroadcastMessage.objects.select_for_update().filter(
user=request.user, active=True
# Lock and deactivate all messages for the message owner
BroadcastMessage.objects.select_for_update().filter(
user=message.user, active=True

Copilot uses AI. Check for mistakes.
).update(active=False)

# Activate the selected message
message.active = True
message.save()
Comment on lines +50 to +58
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The atomic transaction implementation for preventing race conditions in the set_active endpoint lacks test coverage. Consider adding tests that verify: 1) concurrent set_active requests only result in one active message, 2) staff users can properly activate messages for other users, and 3) the transaction properly handles edge cases like activating an already-active message.

Copilot uses AI. Check for mistakes.
Comment on lines +50 to +58
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The message object was retrieved outside the transaction, so it may be stale by the time the transaction executes. If another request modified this message between retrieval and the transaction, those changes would be lost.

The message should be refetched within the transaction using select_for_update() to ensure you're working with the current state of the object.

Copilot uses AI. Check for mistakes.

return Response({'message': 'Message set as active'})


Expand Down
14 changes: 11 additions & 3 deletions server/broadcast/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from django.db import models
from django.db import models, transaction
from django.conf import settings


Expand All @@ -10,8 +10,16 @@ class BroadcastMessage(models.Model):

def save(self, *args, **kwargs):
if self.active:
BroadcastMessage.objects.filter(user=self.user, active=True).update(active=False)
super().save(*args, **kwargs)
# Use atomic transaction with row locking to prevent race conditions
with transaction.atomic():
# Lock and deactivate all user's active messages
BroadcastMessage.objects.select_for_update().filter(
user=self.user, active=True
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The select_for_update() call will lock rows that match the filter, but when saving a new message (pk is None), there's no row for the current message to lock yet. This creates a potential race condition where two concurrent requests could both deactivate existing messages and then both save as active, resulting in multiple active messages.

Consider locking all messages for the user first, or use a get_or_create pattern with locking.

Suggested change
# Lock and deactivate all user's active messages
BroadcastMessage.objects.select_for_update().filter(
user=self.user, active=True
# Lock and deactivate all user's messages to ensure only one remains active
BroadcastMessage.objects.select_for_update().filter(
user=self.user

Copilot uses AI. Check for mistakes.
).update(active=False)
# Save this message as active
super().save(*args, **kwargs)
Comment thread
mihf05 marked this conversation as resolved.
else:
super().save(*args, **kwargs)

def __str__(self):
return f'{self.user.username}: {self.message[:20]}'