CLAUDE: Fix double play bug after state recovery
Critical bug fix for issue where Groundball A with runner on first would fail to execute a double play after game recovery from database. Root cause: current_on_base_code field was not recalculated during state recovery, defaulting to 0 (empty bases) even when runners were on base. This caused runner advancement logic to select Result 1 (batter out, runners hold) instead of Result 2 (double play). Changes: - Added calculate_on_base_code() helper method to GameState model - Updated _prepare_next_play() to use helper (eliminates duplication) - Fixed state recovery to calculate current_on_base_code from runners - Fixed X-Check G1 mapping (was GROUNDBALL_B, should be GROUNDBALL_A) - Added 5 regression tests to prevent recurrence Testing: - All 359 unit tests passing - New regression tests verify fix and demonstrate bug scenario - Tested in network dev environment - double plays now work correctly Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This commit is contained in:
parent
ae728fd052
commit
dd1b087af1
152
BUGFIX_DOUBLE_PLAY_RECOVERY.md
Normal file
152
BUGFIX_DOUBLE_PLAY_RECOVERY.md
Normal file
@ -0,0 +1,152 @@
|
|||||||
|
# Bug Fix: Double Play Not Working After Game Recovery
|
||||||
|
|
||||||
|
**Issue**: [Gitea #3](https://git.manticorum.com/cal/strat-gameplay-webapp/issues/3)
|
||||||
|
|
||||||
|
**Reported By**: User during test game
|
||||||
|
**Date**: 2025-02-07
|
||||||
|
|
||||||
|
## Problem Description
|
||||||
|
|
||||||
|
When a user rejoined a game (after the game state was evicted from memory and recovered from the database), rolling a Groundball A with 0 outs and a runner on first base incorrectly resulted in:
|
||||||
|
|
||||||
|
- ❌ Runner on first out at second (correct)
|
||||||
|
- ❌ Batter **safe** at first (WRONG - should be out)
|
||||||
|
- ❌ **1 out recorded** (WRONG - should be 2 outs)
|
||||||
|
|
||||||
|
**Expected Behavior**: Double play - runner out at second, batter out at first, 2 outs recorded.
|
||||||
|
|
||||||
|
## Root Cause
|
||||||
|
|
||||||
|
The `current_on_base_code` field in `GameState` is a **snapshot field** that stores the base situation as a bit field (1=first, 2=second, 4=third, 7=loaded). This field is used by the runner advancement logic to determine which result to apply.
|
||||||
|
|
||||||
|
During normal gameplay, this field is set by `_prepare_next_play()` in the game engine. However, during **state recovery** from the database, it was not being recalculated, leaving it at the default value of `0` (empty bases).
|
||||||
|
|
||||||
|
This caused the runner advancement logic to select:
|
||||||
|
- **Result 1** (batter out, runners hold) instead of
|
||||||
|
- **Result 2** (double play at second and first)
|
||||||
|
|
||||||
|
## Fix Summary
|
||||||
|
|
||||||
|
### 1. Created Helper Method (PR #1)
|
||||||
|
|
||||||
|
Added `calculate_on_base_code()` method to `GameState` model to centralize the bit field calculation:
|
||||||
|
|
||||||
|
**File**: `backend/app/models/game_models.py`
|
||||||
|
|
||||||
|
```python
|
||||||
|
def calculate_on_base_code(self) -> int:
|
||||||
|
"""
|
||||||
|
Calculate on-base code from current runner positions.
|
||||||
|
|
||||||
|
Returns bit field where:
|
||||||
|
- Bit 0 (value 1): runner on first
|
||||||
|
- Bit 1 (value 2): runner on second
|
||||||
|
- Bit 2 (value 4): runner on third
|
||||||
|
- Value 7: bases loaded (1 + 2 + 4)
|
||||||
|
"""
|
||||||
|
code = 0
|
||||||
|
if self.on_first:
|
||||||
|
code |= 1
|
||||||
|
if self.on_second:
|
||||||
|
code |= 2
|
||||||
|
if self.on_third:
|
||||||
|
code |= 4
|
||||||
|
return code
|
||||||
|
```
|
||||||
|
|
||||||
|
### 2. Updated Game Engine (PR #1)
|
||||||
|
|
||||||
|
Refactored `_prepare_next_play()` to use the helper method:
|
||||||
|
|
||||||
|
**File**: `backend/app/core/game_engine.py` (line ~1014)
|
||||||
|
|
||||||
|
```python
|
||||||
|
# Before:
|
||||||
|
state.current_on_base_code = 0
|
||||||
|
if state.on_first:
|
||||||
|
state.current_on_base_code |= 1
|
||||||
|
if state.on_second:
|
||||||
|
state.current_on_base_code |= 2
|
||||||
|
if state.on_third:
|
||||||
|
state.current_on_base_code |= 4
|
||||||
|
|
||||||
|
# After:
|
||||||
|
state.current_on_base_code = state.calculate_on_base_code()
|
||||||
|
```
|
||||||
|
|
||||||
|
### 3. Fixed State Recovery (PR #1)
|
||||||
|
|
||||||
|
Added calculation of `current_on_base_code` at the end of state recovery:
|
||||||
|
|
||||||
|
**File**: `backend/app/core/state_manager.py` (line ~608)
|
||||||
|
|
||||||
|
```python
|
||||||
|
# CRITICAL FIX: Calculate current_on_base_code from actual runner positions
|
||||||
|
# This field is used by runner advancement logic to determine double plays
|
||||||
|
# Without this, recovered games default to on_base_code=0 (empty bases)
|
||||||
|
# even when runners are on base, breaking double play logic
|
||||||
|
state.current_on_base_code = state.calculate_on_base_code()
|
||||||
|
logger.debug(f"Recovery: Set current_on_base_code = {state.current_on_base_code}")
|
||||||
|
```
|
||||||
|
|
||||||
|
### 4. Fixed X-Check G1 Mapping (PR #2)
|
||||||
|
|
||||||
|
Also discovered and fixed a separate bug where X-Check G1 results were incorrectly mapped to GROUNDBALL_B instead of GROUNDBALL_A:
|
||||||
|
|
||||||
|
**File**: `backend/app/core/play_resolver.py` (line ~1450)
|
||||||
|
|
||||||
|
```python
|
||||||
|
# Before:
|
||||||
|
"G1": PlayOutcome.GROUNDBALL_B, # WRONG
|
||||||
|
|
||||||
|
# After:
|
||||||
|
"G1": PlayOutcome.GROUNDBALL_A, # Fixed: G1 is fast grounder (double play capable)
|
||||||
|
```
|
||||||
|
|
||||||
|
## Testing
|
||||||
|
|
||||||
|
### Regression Test Added
|
||||||
|
|
||||||
|
**File**: `backend/tests/unit/core/test_recovery_double_play_fix.py`
|
||||||
|
|
||||||
|
- ✅ Test verify `calculate_on_base_code()` for all base combinations
|
||||||
|
- ✅ Test recovered state with runner on first executes double play
|
||||||
|
- ✅ Test demonstrates bug behavior without fix
|
||||||
|
|
||||||
|
### Test Results
|
||||||
|
|
||||||
|
```
|
||||||
|
359 unit tests passing
|
||||||
|
5 new regression tests passing
|
||||||
|
```
|
||||||
|
|
||||||
|
## Impact
|
||||||
|
|
||||||
|
**Before Fix**:
|
||||||
|
- Games recovered from database would have incorrect on_base_code (always 0)
|
||||||
|
- Double plays would not work correctly after rejoining a game
|
||||||
|
- Force outs and other runner advancement scenarios also affected
|
||||||
|
|
||||||
|
**After Fix**:
|
||||||
|
- State recovery correctly calculates on_base_code from runner positions
|
||||||
|
- Double plays work correctly regardless of whether state was recovered
|
||||||
|
- All runner advancement logic functions properly after rejoin
|
||||||
|
|
||||||
|
## Related Issues
|
||||||
|
|
||||||
|
- This also fixes any other runner advancement issues that depend on `current_on_base_code` being accurate
|
||||||
|
- Prevents similar issues with Result 4 (force out), Result 10 (home-to-first DP), etc.
|
||||||
|
|
||||||
|
## Deployment Notes
|
||||||
|
|
||||||
|
No database migration required. The fix is purely in the application logic.
|
||||||
|
|
||||||
|
Users who experienced this bug should:
|
||||||
|
1. Refresh their browser after deployment
|
||||||
|
2. Previous games will work correctly after next state recovery
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
**Fixed**: 2025-02-07
|
||||||
|
**Tested**: ✅ All unit tests passing
|
||||||
|
**Status**: Ready for deployment
|
||||||
@ -1011,13 +1011,7 @@ class GameEngine:
|
|||||||
state.current_catcher = None
|
state.current_catcher = None
|
||||||
|
|
||||||
# Calculate on_base_code from current runners (bit field)
|
# Calculate on_base_code from current runners (bit field)
|
||||||
state.current_on_base_code = 0
|
state.current_on_base_code = state.calculate_on_base_code()
|
||||||
if state.on_first:
|
|
||||||
state.current_on_base_code |= 1 # Bit 0: first base
|
|
||||||
if state.on_second:
|
|
||||||
state.current_on_base_code |= 2 # Bit 1: second base
|
|
||||||
if state.on_third:
|
|
||||||
state.current_on_base_code |= 4 # Bit 2: third base
|
|
||||||
|
|
||||||
logger.info(
|
logger.info(
|
||||||
f"Prepared next play: batter lineup_id={state.current_batter.lineup_id}, "
|
f"Prepared next play: batter lineup_id={state.current_batter.lineup_id}, "
|
||||||
|
|||||||
@ -1447,7 +1447,7 @@ class PlayResolver:
|
|||||||
"DO2": PlayOutcome.DOUBLE_2,
|
"DO2": PlayOutcome.DOUBLE_2,
|
||||||
"DO3": PlayOutcome.DOUBLE_3,
|
"DO3": PlayOutcome.DOUBLE_3,
|
||||||
"TR3": PlayOutcome.TRIPLE,
|
"TR3": PlayOutcome.TRIPLE,
|
||||||
"G1": PlayOutcome.GROUNDBALL_B,
|
"G1": PlayOutcome.GROUNDBALL_A, # Fixed: G1 is fast grounder (double play capable)
|
||||||
"G2": PlayOutcome.GROUNDBALL_B,
|
"G2": PlayOutcome.GROUNDBALL_B,
|
||||||
"G3": PlayOutcome.GROUNDBALL_C,
|
"G3": PlayOutcome.GROUNDBALL_C,
|
||||||
"F1": PlayOutcome.FLYOUT_A,
|
"F1": PlayOutcome.FLYOUT_A,
|
||||||
|
|||||||
@ -605,6 +605,14 @@ class StateManager:
|
|||||||
logger.info(
|
logger.info(
|
||||||
f"Rebuilt state for game {state.game_id}: {state.play_count} plays, {runners_on_base} runners"
|
f"Rebuilt state for game {state.game_id}: {state.play_count} plays, {runners_on_base} runners"
|
||||||
)
|
)
|
||||||
|
|
||||||
|
# CRITICAL FIX: Calculate current_on_base_code from actual runner positions
|
||||||
|
# This field is used by runner advancement logic to determine double plays
|
||||||
|
# Without this, recovered games default to on_base_code=0 (empty bases)
|
||||||
|
# even when runners are on base, breaking double play logic
|
||||||
|
state.current_on_base_code = state.calculate_on_base_code()
|
||||||
|
logger.debug(f"Recovery: Set current_on_base_code = {state.current_on_base_code}")
|
||||||
|
|
||||||
return state
|
return state
|
||||||
|
|
||||||
async def evict_idle_games(self) -> list[UUID]:
|
async def evict_idle_games(self) -> list[UUID]:
|
||||||
|
|||||||
@ -588,6 +588,31 @@ class GameState(BaseModel):
|
|||||||
"""Check if there's a runner on third base"""
|
"""Check if there's a runner on third base"""
|
||||||
return self.on_third is not None
|
return self.on_third is not None
|
||||||
|
|
||||||
|
def calculate_on_base_code(self) -> int:
|
||||||
|
"""
|
||||||
|
Calculate on-base code from current runner positions.
|
||||||
|
|
||||||
|
Returns bit field where:
|
||||||
|
- Bit 0 (value 1): runner on first
|
||||||
|
- Bit 1 (value 2): runner on second
|
||||||
|
- Bit 2 (value 4): runner on third
|
||||||
|
- Value 7: bases loaded (1 + 2 + 4)
|
||||||
|
|
||||||
|
Examples:
|
||||||
|
0 = empty bases
|
||||||
|
1 = runner on first only
|
||||||
|
3 = runners on first and second
|
||||||
|
7 = bases loaded
|
||||||
|
"""
|
||||||
|
code = 0
|
||||||
|
if self.on_first:
|
||||||
|
code |= 1 # Bit 0
|
||||||
|
if self.on_second:
|
||||||
|
code |= 2 # Bit 1
|
||||||
|
if self.on_third:
|
||||||
|
code |= 4 # Bit 2
|
||||||
|
return code
|
||||||
|
|
||||||
def get_runner_at_base(self, base: int) -> LineupPlayerState | None:
|
def get_runner_at_base(self, base: int) -> LineupPlayerState | None:
|
||||||
"""Get runner at specified base (1, 2, or 3)"""
|
"""Get runner at specified base (1, 2, or 3)"""
|
||||||
if base == 1:
|
if base == 1:
|
||||||
|
|||||||
201
backend/tests/unit/core/test_recovery_double_play_fix.py
Normal file
201
backend/tests/unit/core/test_recovery_double_play_fix.py
Normal file
@ -0,0 +1,201 @@
|
|||||||
|
"""
|
||||||
|
Test for state recovery double play bug fix.
|
||||||
|
|
||||||
|
Regression test for issue where recovering a game with runner on first
|
||||||
|
would fail to execute a double play on Groundball A because
|
||||||
|
current_on_base_code was not recalculated during recovery.
|
||||||
|
|
||||||
|
GitHub Issue: https://git.manticorum.com/cal/strat-gameplay-webapp/issues/3
|
||||||
|
"""
|
||||||
|
from unittest.mock import Mock
|
||||||
|
from uuid import uuid4
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
|
||||||
|
from app.config.result_charts import PlayOutcome
|
||||||
|
from app.core.runner_advancement import GroundballResultType, RunnerAdvancement
|
||||||
|
from app.models.game_models import DefensiveDecision, GameState, LineupPlayerState
|
||||||
|
|
||||||
|
|
||||||
|
def create_lineup_player(lineup_id: int, batting_order: int, position: str = "CF"):
|
||||||
|
"""Helper to create a LineupPlayerState."""
|
||||||
|
return LineupPlayerState(
|
||||||
|
lineup_id=lineup_id,
|
||||||
|
card_id=lineup_id + 100,
|
||||||
|
position=position,
|
||||||
|
batting_order=batting_order,
|
||||||
|
is_active=True,
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
class TestRecoveryDoublePla:
|
||||||
|
"""Test suite for state recovery double play fix."""
|
||||||
|
|
||||||
|
def test_calculate_on_base_code_empty(self):
|
||||||
|
"""Verify calculate_on_base_code returns 0 for empty bases."""
|
||||||
|
state = GameState(
|
||||||
|
game_id=uuid4(),
|
||||||
|
league_id="sba",
|
||||||
|
home_team_id=1,
|
||||||
|
away_team_id=2,
|
||||||
|
home_team_is_ai=False,
|
||||||
|
away_team_is_ai=False,
|
||||||
|
current_batter=create_lineup_player(1, 1),
|
||||||
|
on_first=None,
|
||||||
|
on_second=None,
|
||||||
|
on_third=None,
|
||||||
|
)
|
||||||
|
|
||||||
|
assert state.calculate_on_base_code() == 0
|
||||||
|
|
||||||
|
def test_calculate_on_base_code_runner_on_first(self):
|
||||||
|
"""Verify calculate_on_base_code returns 1 for runner on first."""
|
||||||
|
state = GameState(
|
||||||
|
game_id=uuid4(),
|
||||||
|
league_id="sba",
|
||||||
|
home_team_id=1,
|
||||||
|
away_team_id=2,
|
||||||
|
home_team_is_ai=False,
|
||||||
|
away_team_is_ai=False,
|
||||||
|
current_batter=create_lineup_player(1, 1),
|
||||||
|
on_first=create_lineup_player(2, 2),
|
||||||
|
on_second=None,
|
||||||
|
on_third=None,
|
||||||
|
)
|
||||||
|
|
||||||
|
assert state.calculate_on_base_code() == 1
|
||||||
|
|
||||||
|
def test_calculate_on_base_code_bases_loaded(self):
|
||||||
|
"""Verify calculate_on_base_code returns 7 for bases loaded."""
|
||||||
|
state = GameState(
|
||||||
|
game_id=uuid4(),
|
||||||
|
league_id="sba",
|
||||||
|
home_team_id=1,
|
||||||
|
away_team_id=2,
|
||||||
|
home_team_is_ai=False,
|
||||||
|
away_team_is_ai=False,
|
||||||
|
current_batter=create_lineup_player(1, 1),
|
||||||
|
on_first=create_lineup_player(2, 2),
|
||||||
|
on_second=create_lineup_player(3, 3),
|
||||||
|
on_third=create_lineup_player(4, 4),
|
||||||
|
)
|
||||||
|
|
||||||
|
assert state.calculate_on_base_code() == 7
|
||||||
|
|
||||||
|
def test_recovered_state_double_play_scenario(self):
|
||||||
|
"""
|
||||||
|
Regression test: Groundball A with runner on first after recovery.
|
||||||
|
|
||||||
|
This reproduces the exact bug scenario reported:
|
||||||
|
- Game recovered from database with runner on first
|
||||||
|
- User rolls dice and enters Groundball A
|
||||||
|
- Should record 2 outs (double play), not 1 out (force out)
|
||||||
|
|
||||||
|
The bug was that current_on_base_code defaulted to 0 during recovery,
|
||||||
|
causing the runner advancement logic to use Result 1 (batter out, runners hold)
|
||||||
|
instead of Result 2 (double play).
|
||||||
|
"""
|
||||||
|
# Simulate recovered state (as if loaded from database)
|
||||||
|
state = GameState(
|
||||||
|
game_id=uuid4(),
|
||||||
|
league_id="sba",
|
||||||
|
home_team_id=1,
|
||||||
|
away_team_id=2,
|
||||||
|
home_team_is_ai=False,
|
||||||
|
away_team_is_ai=False,
|
||||||
|
inning=3,
|
||||||
|
half="top",
|
||||||
|
outs=0, # 0 outs (critical for double play)
|
||||||
|
home_score=2,
|
||||||
|
away_score=1,
|
||||||
|
on_first=create_lineup_player(5, 2), # Runner on first
|
||||||
|
on_second=None,
|
||||||
|
on_third=None,
|
||||||
|
current_batter=create_lineup_player(6, 3),
|
||||||
|
)
|
||||||
|
|
||||||
|
# CRITICAL: Simulate what state recovery now does
|
||||||
|
state.current_on_base_code = state.calculate_on_base_code()
|
||||||
|
|
||||||
|
# Verify on_base_code is correct
|
||||||
|
assert state.current_on_base_code == 1, "on_base_code should be 1 (runner on first)"
|
||||||
|
|
||||||
|
# Now test the double play logic
|
||||||
|
advancement = RunnerAdvancement()
|
||||||
|
normal_defense = DefensiveDecision(
|
||||||
|
infield_depth="normal",
|
||||||
|
outfield_depth="normal",
|
||||||
|
defensive_substitutions={},
|
||||||
|
)
|
||||||
|
|
||||||
|
result = advancement.advance_runners(
|
||||||
|
outcome=PlayOutcome.GROUNDBALL_A,
|
||||||
|
hit_location="SS",
|
||||||
|
state=state,
|
||||||
|
defensive_decision=normal_defense,
|
||||||
|
)
|
||||||
|
|
||||||
|
# Assertions
|
||||||
|
assert result.outs_recorded == 2, "Should record 2 outs (double play)"
|
||||||
|
assert (
|
||||||
|
result.result_type == GroundballResultType.DOUBLE_PLAY_AT_SECOND
|
||||||
|
), "Should use Result 2 (DOUBLE_PLAY_AT_SECOND)"
|
||||||
|
|
||||||
|
# Verify batter is out
|
||||||
|
batter_movement = next((m for m in result.movements if m.from_base == 0), None)
|
||||||
|
assert batter_movement is not None, "Should have batter movement"
|
||||||
|
assert batter_movement.is_out, "Batter should be out"
|
||||||
|
|
||||||
|
# Verify runner on first is out
|
||||||
|
runner_movement = next((m for m in result.movements if m.lineup_id == 5), None)
|
||||||
|
assert runner_movement is not None, "Should have runner movement"
|
||||||
|
assert runner_movement.is_out, "Runner on first should be out"
|
||||||
|
|
||||||
|
def test_recovered_state_without_fix_would_fail(self):
|
||||||
|
"""
|
||||||
|
Demonstrates the bug: without setting current_on_base_code, wrong result.
|
||||||
|
|
||||||
|
This test shows what WOULD happen without the fix.
|
||||||
|
"""
|
||||||
|
# Simulate recovered state WITHOUT setting current_on_base_code
|
||||||
|
state = GameState(
|
||||||
|
game_id=uuid4(),
|
||||||
|
league_id="sba",
|
||||||
|
home_team_id=1,
|
||||||
|
away_team_id=2,
|
||||||
|
home_team_is_ai=False,
|
||||||
|
away_team_is_ai=False,
|
||||||
|
inning=3,
|
||||||
|
half="top",
|
||||||
|
outs=0,
|
||||||
|
home_score=2,
|
||||||
|
away_score=1,
|
||||||
|
on_first=create_lineup_player(5, 2),
|
||||||
|
on_second=None,
|
||||||
|
on_third=None,
|
||||||
|
current_batter=create_lineup_player(6, 3),
|
||||||
|
)
|
||||||
|
|
||||||
|
# BUG: current_on_base_code defaults to 0
|
||||||
|
assert state.current_on_base_code == 0, "Default is 0 (empty bases)"
|
||||||
|
|
||||||
|
# This causes wrong behavior
|
||||||
|
advancement = RunnerAdvancement()
|
||||||
|
normal_defense = DefensiveDecision(
|
||||||
|
infield_depth="normal",
|
||||||
|
outfield_depth="normal",
|
||||||
|
defensive_substitutions={},
|
||||||
|
)
|
||||||
|
|
||||||
|
result = advancement.advance_runners(
|
||||||
|
outcome=PlayOutcome.GROUNDBALL_A,
|
||||||
|
hit_location="SS",
|
||||||
|
state=state,
|
||||||
|
defensive_decision=normal_defense,
|
||||||
|
)
|
||||||
|
|
||||||
|
# Without the fix, this would incorrectly use Result 1
|
||||||
|
assert (
|
||||||
|
result.result_type == GroundballResultType.BATTER_OUT_RUNNERS_HOLD
|
||||||
|
), "Bug: Uses Result 1 instead of Result 2"
|
||||||
|
assert result.outs_recorded == 1, "Bug: Only 1 out instead of 2"
|
||||||
Loading…
Reference in New Issue
Block a user