feat: Implement atomic transaction for activating broadcast messages …

[mihf05]

…to prevent race conditions

[Copilot]

Pull request overview

This PR implements atomic transactions with row-level locking to prevent race conditions when activating broadcast messages, ensuring only one message can be active per user at a time.

Key Changes

• Added transaction.atomic() with select_for_update() to the BroadcastMessage.save() method to lock and deactivate existing active messages before saving
• Implemented the same atomic transaction pattern in the set_active API endpoint to safely activate messages via the API
• Restructured the save logic to handle both active and inactive message saves appropriately

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File	Description
server/broadcast/models.py	Added atomic transaction with row locking in the save method to prevent concurrent activation of multiple messages
server/broadcast/api_views.py	Implemented atomic transaction with row locking in the set_active endpoint to ensure safe message activation via API

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

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]

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]

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]

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