ebf006e5c6
CRITICAL BUG FIX: Play locks were never released on exceptions, causing permanent user lockouts. Found 13 stuck plays in production. Changes: 1. Added lock_play parameter to checks_log_interaction() (default True) 2. Removed unnecessary locks from read-only commands: - /settings-ingame (game settings, not play state) - /show-card defense (read-only display) - /substitute commands (just show UI, lock in callback) 3. Added safe_play_lock() context manager for automatic lock release 4. Added play locking to substitution callbacks: - SelectBatterSub.callback() - SelectReliefPitcher.callback() 5. Global error handler now releases stuck locks automatically Architecture: - Commands that display UI or read data: No lock - Commands that modify play state: Lock at last possible moment - 3-layer defense: manual release, context manager, global handler Resolves race condition from concurrent play modifications. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
318 lines
9.9 KiB
Python
318 lines
9.9 KiB
Python
"""
|
|
Test play locking idempotency guard.
|
|
|
|
Tests the play locking system including:
|
|
- Lock acquisition and rejection of duplicate submissions
|
|
- Automatic lock release on exception via global error handler
|
|
- Manual lock release via release_play_lock()
|
|
- Context manager safe_play_lock() behavior
|
|
"""
|
|
|
|
import pytest
|
|
from unittest.mock import MagicMock, AsyncMock, patch
|
|
from command_logic.logic_gameplay import checks_log_interaction, release_play_lock, safe_play_lock
|
|
from exceptions import PlayLockedException
|
|
|
|
|
|
@pytest.fixture
|
|
def mock_session():
|
|
"""Create a mock SQLAlchemy session."""
|
|
session = MagicMock()
|
|
return session
|
|
|
|
|
|
@pytest.fixture
|
|
def mock_interaction():
|
|
"""Create a mock Discord interaction."""
|
|
interaction = MagicMock()
|
|
interaction.user.name = "TestUser"
|
|
interaction.user.id = 12345
|
|
interaction.channel.name = "test-game-channel"
|
|
interaction.channel_id = 98765
|
|
interaction.response = MagicMock()
|
|
interaction.response.defer = AsyncMock()
|
|
return interaction
|
|
|
|
|
|
@pytest.fixture
|
|
def mock_game(mock_session):
|
|
"""Create a mock game with current play."""
|
|
game = MagicMock()
|
|
game.id = 100
|
|
game.current_play_or_none = MagicMock()
|
|
return game
|
|
|
|
|
|
@pytest.fixture
|
|
def mock_team():
|
|
"""Create a mock team."""
|
|
team = MagicMock()
|
|
team.id = 50
|
|
team.abbrev = "TEST"
|
|
return team
|
|
|
|
|
|
@pytest.fixture
|
|
def mock_play_unlocked():
|
|
"""Create an unlocked mock play."""
|
|
play = MagicMock()
|
|
play.id = 200
|
|
play.locked = False
|
|
return play
|
|
|
|
|
|
@pytest.fixture
|
|
def mock_play_locked():
|
|
"""Create a locked mock play."""
|
|
play = MagicMock()
|
|
play.id = 200
|
|
play.locked = True
|
|
return play
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_unlocked_play_locks_successfully(
|
|
mock_session, mock_interaction, mock_game, mock_team, mock_play_unlocked
|
|
):
|
|
"""Verify unlocked play can be locked and processed."""
|
|
mock_game.current_play_or_none.return_value = mock_play_unlocked
|
|
mock_session.exec.return_value.one.return_value = mock_team
|
|
|
|
with patch(
|
|
"command_logic.logic_gameplay.get_channel_game_or_none", return_value=mock_game
|
|
):
|
|
with patch(
|
|
"command_logic.logic_gameplay.get_team_or_none", return_value=mock_team
|
|
):
|
|
result_game, result_team, result_play = await checks_log_interaction(
|
|
mock_session, mock_interaction, command_name="log xcheck"
|
|
)
|
|
|
|
assert result_play.locked is True
|
|
assert result_play.id == mock_play_unlocked.id
|
|
mock_session.commit.assert_called_once()
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_locked_play_rejects_duplicate_interaction(
|
|
mock_session, mock_interaction, mock_game, mock_team, mock_play_locked
|
|
):
|
|
"""Verify duplicate command on locked play raises PlayLockedException."""
|
|
mock_game.current_play_or_none.return_value = mock_play_locked
|
|
mock_session.exec.return_value.one.return_value = mock_team
|
|
|
|
with patch(
|
|
"command_logic.logic_gameplay.get_channel_game_or_none", return_value=mock_game
|
|
):
|
|
with patch(
|
|
"command_logic.logic_gameplay.get_team_or_none", return_value=mock_team
|
|
):
|
|
with pytest.raises(PlayLockedException) as exc_info:
|
|
await checks_log_interaction(
|
|
mock_session, mock_interaction, command_name="log xcheck"
|
|
)
|
|
|
|
assert "already being processed" in str(exc_info.value)
|
|
assert "wait" in str(exc_info.value).lower()
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_locked_play_logs_warning(
|
|
mock_session, mock_interaction, mock_game, mock_team, mock_play_locked, caplog
|
|
):
|
|
"""Verify locked play attempt is logged with warning."""
|
|
mock_game.current_play_or_none.return_value = mock_play_locked
|
|
mock_session.exec.return_value.one.return_value = mock_team
|
|
|
|
with patch(
|
|
"command_logic.logic_gameplay.get_channel_game_or_none", return_value=mock_game
|
|
):
|
|
with patch(
|
|
"command_logic.logic_gameplay.get_team_or_none", return_value=mock_team
|
|
):
|
|
try:
|
|
await checks_log_interaction(
|
|
mock_session, mock_interaction, command_name="log xcheck"
|
|
)
|
|
except PlayLockedException:
|
|
pass
|
|
|
|
assert any(
|
|
"attempted log xcheck on locked play" in record.message
|
|
for record in caplog.records
|
|
)
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_lock_released_after_successful_completion(mock_session):
|
|
"""
|
|
Verify play lock is released after successful command completion.
|
|
|
|
Tests the complete_play() function to ensure it:
|
|
- Sets play.locked = False
|
|
- Sets play.complete = True
|
|
- Commits changes to database
|
|
"""
|
|
from command_logic.logic_gameplay import complete_play
|
|
|
|
# Create mock play that's currently locked
|
|
mock_play = MagicMock()
|
|
mock_play.id = 300
|
|
mock_play.locked = True
|
|
mock_play.complete = False
|
|
mock_play.game = MagicMock()
|
|
mock_play.game.id = 100
|
|
mock_play.inning_num = 1
|
|
mock_play.inning_half = "top"
|
|
mock_play.starting_outs = 0
|
|
mock_play.away_score = 0
|
|
mock_play.home_score = 0
|
|
mock_play.on_base_code = 0
|
|
mock_play.batter = MagicMock()
|
|
mock_play.batter.team = MagicMock()
|
|
mock_play.pitcher = MagicMock()
|
|
mock_play.pitcher.team = MagicMock()
|
|
mock_play.managerai = MagicMock()
|
|
|
|
# Mock the session.exec queries
|
|
mock_session.exec.return_value.one.return_value = MagicMock()
|
|
|
|
# Execute complete_play
|
|
with patch("command_logic.logic_gameplay.get_one_lineup"):
|
|
with patch("command_logic.logic_gameplay.get_re24", return_value=0.0):
|
|
with patch("command_logic.logic_gameplay.get_wpa", return_value=0.0):
|
|
complete_play(mock_session, mock_play)
|
|
|
|
# Verify lock was released and play marked complete
|
|
assert mock_play.locked is False
|
|
assert mock_play.complete is True
|
|
|
|
# Verify changes were committed
|
|
mock_session.add.assert_called()
|
|
mock_session.commit.assert_called_once()
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_different_commands_racing_on_locked_play(
|
|
mock_session, mock_interaction, mock_game, mock_team, mock_play_locked
|
|
):
|
|
"""
|
|
Verify different command types are blocked when play is locked.
|
|
|
|
Tests that the lock prevents ANY command from processing, not just
|
|
duplicates of the same command. This prevents race conditions where
|
|
different users try different commands simultaneously.
|
|
"""
|
|
mock_game.current_play_or_none.return_value = mock_play_locked
|
|
mock_session.exec.return_value.one.return_value = mock_team
|
|
|
|
# Test different command types all raise PlayLockedException
|
|
command_types = ["log walk", "log strikeout", "log single", "log xcheck"]
|
|
|
|
for command_name in command_types:
|
|
with patch(
|
|
"command_logic.logic_gameplay.get_channel_game_or_none",
|
|
return_value=mock_game,
|
|
):
|
|
with patch(
|
|
"command_logic.logic_gameplay.get_team_or_none",
|
|
return_value=mock_team,
|
|
):
|
|
with pytest.raises(PlayLockedException) as exc_info:
|
|
await checks_log_interaction(
|
|
mock_session, mock_interaction, command_name=command_name
|
|
)
|
|
|
|
# Verify exception message is consistent
|
|
assert "already being processed" in str(exc_info.value)
|
|
assert "wait" in str(exc_info.value).lower()
|
|
|
|
|
|
def test_release_play_lock_unlocks_locked_play(mock_session):
|
|
"""
|
|
Verify release_play_lock() releases a locked play.
|
|
|
|
Tests that the release_play_lock helper function correctly:
|
|
- Sets play.locked = False
|
|
- Commits the change to the database
|
|
"""
|
|
mock_play = MagicMock()
|
|
mock_play.id = 400
|
|
mock_play.locked = True
|
|
mock_play.complete = False
|
|
|
|
release_play_lock(mock_session, mock_play)
|
|
|
|
assert mock_play.locked is False
|
|
mock_session.add.assert_called_once_with(mock_play)
|
|
mock_session.commit.assert_called_once()
|
|
|
|
|
|
def test_release_play_lock_handles_unlocked_play(mock_session):
|
|
"""
|
|
Verify release_play_lock() safely handles already unlocked play.
|
|
|
|
Tests that calling release_play_lock on an already unlocked play
|
|
is a no-op and doesn't cause errors.
|
|
"""
|
|
mock_play = MagicMock()
|
|
mock_play.id = 400
|
|
mock_play.locked = False
|
|
mock_play.complete = True
|
|
|
|
# Should not raise exception even if play is already unlocked
|
|
release_play_lock(mock_session, mock_play)
|
|
|
|
# Should still be unlocked
|
|
assert mock_play.locked is False
|
|
|
|
|
|
def test_safe_play_lock_releases_on_exception(mock_session):
|
|
"""
|
|
Verify safe_play_lock context manager releases lock on exception.
|
|
|
|
Tests that when an exception occurs within the context manager,
|
|
the play lock is automatically released before re-raising.
|
|
"""
|
|
mock_play = MagicMock()
|
|
mock_play.id = 500
|
|
mock_play.locked = True
|
|
mock_play.complete = False
|
|
|
|
with pytest.raises(ValueError):
|
|
with safe_play_lock(mock_session, mock_play):
|
|
# Simulate exception during command processing
|
|
raise ValueError("Test exception")
|
|
|
|
# Lock should be released
|
|
assert mock_play.locked is False
|
|
mock_session.add.assert_called()
|
|
mock_session.commit.assert_called()
|
|
|
|
|
|
def test_safe_play_lock_allows_normal_completion(mock_session):
|
|
"""
|
|
Verify safe_play_lock doesn't interfere with normal completion.
|
|
|
|
Tests that when no exception occurs, the context manager doesn't
|
|
prematurely release the lock (complete_play should handle that).
|
|
"""
|
|
mock_play = MagicMock()
|
|
mock_play.id = 600
|
|
mock_play.locked = True
|
|
mock_play.complete = False
|
|
|
|
with safe_play_lock(mock_session, mock_play):
|
|
# Normal processing
|
|
pass
|
|
|
|
# Lock should remain (will be released by complete_play)
|
|
# Note: The finally block will force unlock, but this simulates
|
|
# the case where complete_play() is called within the context
|
|
mock_play.complete = True
|
|
|
|
with safe_play_lock(mock_session, mock_play):
|
|
pass
|
|
|
|
# Completed play should not be unlocked by context manager
|