From 72a3b94ce73cb53032e827bf3514a460aafd7c3d Mon Sep 17 00:00:00 2001 From: Cal Corum Date: Wed, 19 Nov 2025 16:39:01 -0600 Subject: [PATCH] CLAUDE: Add transaction handling for multi-step DB operations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #4 from code review - multi-step database operations were not wrapped in transactions, risking partial state on failure. Changes: - Modified save_play() and update_game_state() in DatabaseOperations to accept optional session parameter for transaction grouping - Wrapped resolve_play() DB operations in single atomic transaction - Wrapped resolve_manual_play() DB operations in single atomic transaction - Transaction commits atomically or rolls back entirely on failure Pattern: When session provided, use flush() for IDs without committing; caller controls transaction. When no session, create internal transaction. All 739 unit tests passing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- backend/.claude/CODE_REVIEW_GAME_ENGINE.md | 6 +- backend/app/core/game_engine.py | 180 +++++++++++++-------- backend/app/database/operations.py | 107 +++++++----- 3 files changed, 182 insertions(+), 111 deletions(-) diff --git a/backend/.claude/CODE_REVIEW_GAME_ENGINE.md b/backend/.claude/CODE_REVIEW_GAME_ENGINE.md index 979824b..9902ec5 100644 --- a/backend/.claude/CODE_REVIEW_GAME_ENGINE.md +++ b/backend/.claude/CODE_REVIEW_GAME_ENGINE.md @@ -282,7 +282,11 @@ Some methods seem designed for testing but are public API. - Issue #1: Re-raise exception in `_batch_save_inning_rolls` - Issue #2: Added `_game_locks` dict and `_get_game_lock()` method, wrapped decision submissions with `async with` - Issue #3: Added `_cleanup_game_resources()` method, called on game completion in `resolve_play`, `resolve_manual_play`, and `end_game` -- [ ] High issues fixed +- [x] High issue #4 fixed (2025-01-19) + - Added optional `session` parameter to `save_play` and `update_game_state` in DatabaseOperations + - Wrapped multi-step DB ops in `resolve_play` and `resolve_manual_play` with single transaction + - Transaction commits atomically or rolls back entirely on failure +- [ ] High issues #5-11 pending - [ ] Medium issues addressed - [ ] Low issues addressed diff --git a/backend/app/core/game_engine.py b/backend/app/core/game_engine.py index bc9b9c9..f3a67e4 100644 --- a/backend/app/core/game_engine.py +++ b/backend/app/core/game_engine.py @@ -22,6 +22,8 @@ from app.core.validators import game_validator, ValidationError from app.core.dice import dice_system from app.core.ai_opponent import ai_opponent from app.database.operations import DatabaseOperations +from app.database.session import AsyncSessionLocal +from sqlalchemy.ext.asyncio import AsyncSession from app.models.game_models import ( GameState, DefensiveDecision, OffensiveDecision ) @@ -474,9 +476,6 @@ class GameEngine: self._rolls_this_inning[game_id] = [] self._rolls_this_inning[game_id].append(result.ab_roll) - # STEP 2: Save play to DB (uses snapshot from GameState) - await self._save_play_to_db(state, result) - # Capture state before applying result state_before = { 'inning': state.inning, @@ -486,41 +485,60 @@ class GameEngine: 'status': state.status } - # STEP 3: Apply result to state (outs, score, runners) + # STEP 3: Apply result to state (outs, score, runners) - before transaction self._apply_play_result(state, result) - # STEP 4: Update game state in DB only if something changed - if (state.inning != state_before['inning'] or - state.half != state_before['half'] or - state.home_score != state_before['home_score'] or - state.away_score != state_before['away_score'] or - state.status != state_before['status']): + # STEP 2, 4, 5: Database operations in single transaction + async with AsyncSessionLocal() as session: + try: + # STEP 2: Save play to DB (uses snapshot from GameState) + await self._save_play_to_db(state, result, session=session) - await self.db_ops.update_game_state( - game_id=state.game_id, - inning=state.inning, - half=state.half, - home_score=state.home_score, - away_score=state.away_score, - status=state.status - ) - logger.info("Updated game state in DB - score/inning/status changed") - else: - logger.debug("Skipped game state update - no changes to persist") + # STEP 4: Update game state in DB only if something changed + if (state.inning != state_before['inning'] or + state.half != state_before['half'] or + state.home_score != state_before['home_score'] or + state.away_score != state_before['away_score'] or + state.status != state_before['status']): - # STEP 5: Check for inning change + await self.db_ops.update_game_state( + game_id=state.game_id, + inning=state.inning, + half=state.half, + home_score=state.home_score, + away_score=state.away_score, + status=state.status, + session=session + ) + logger.info("Updated game state in DB - score/inning/status changed") + else: + logger.debug("Skipped game state update - no changes to persist") + + # STEP 5: Check for inning change + if state.outs >= 3: + await self._advance_inning(state, game_id) + # Update DB again after inning change + await self.db_ops.update_game_state( + game_id=state.game_id, + inning=state.inning, + half=state.half, + home_score=state.home_score, + away_score=state.away_score, + status=state.status, + session=session + ) + + # Commit entire transaction + await session.commit() + logger.debug("Committed play transaction successfully") + + except Exception as e: + await session.rollback() + logger.error(f"Transaction failed, rolled back: {e}") + raise + + # Batch save rolls at half-inning boundary (separate transaction - audit data) if state.outs >= 3: - await self._advance_inning(state, game_id) - # Update DB again after inning change - await self.db_ops.update_game_state( - game_id=state.game_id, - inning=state.inning, - half=state.half, - home_score=state.home_score, - away_score=state.away_score, - status=state.status - ) - # Batch save rolls at half-inning boundary await self._batch_save_inning_rolls(game_id) # STEP 6: Prepare next play or clean up if game completed @@ -612,9 +630,6 @@ class GameEngine: self._rolls_this_inning[game_id] = [] self._rolls_this_inning[game_id].append(ab_roll) - # STEP 2: Save play to DB - await self._save_play_to_db(state, result) - # Capture state before applying result state_before = { 'inning': state.inning, @@ -624,41 +639,60 @@ class GameEngine: 'status': state.status } - # STEP 3: Apply result to state + # STEP 3: Apply result to state - before transaction self._apply_play_result(state, result) - # STEP 4: Update game state in DB only if something changed - if (state.inning != state_before['inning'] or - state.half != state_before['half'] or - state.home_score != state_before['home_score'] or - state.away_score != state_before['away_score'] or - state.status != state_before['status']): + # STEP 2, 4, 5: Database operations in single transaction + async with AsyncSessionLocal() as session: + try: + # STEP 2: Save play to DB + await self._save_play_to_db(state, result, session=session) - await self.db_ops.update_game_state( - game_id=state.game_id, - inning=state.inning, - half=state.half, - home_score=state.home_score, - away_score=state.away_score, - status=state.status - ) - logger.info("Updated game state in DB - score/inning/status changed") - else: - logger.debug("Skipped game state update - no changes to persist") + # STEP 4: Update game state in DB only if something changed + if (state.inning != state_before['inning'] or + state.half != state_before['half'] or + state.home_score != state_before['home_score'] or + state.away_score != state_before['away_score'] or + state.status != state_before['status']): - # STEP 5: Check for inning change + await self.db_ops.update_game_state( + game_id=state.game_id, + inning=state.inning, + half=state.half, + home_score=state.home_score, + away_score=state.away_score, + status=state.status, + session=session + ) + logger.info("Updated game state in DB - score/inning/status changed") + else: + logger.debug("Skipped game state update - no changes to persist") + + # STEP 5: Check for inning change + if state.outs >= 3: + await self._advance_inning(state, game_id) + # Update DB again after inning change + await self.db_ops.update_game_state( + game_id=state.game_id, + inning=state.inning, + half=state.half, + home_score=state.home_score, + away_score=state.away_score, + status=state.status, + session=session + ) + + # Commit entire transaction + await session.commit() + logger.debug("Committed manual play transaction successfully") + + except Exception as e: + await session.rollback() + logger.error(f"Manual play transaction failed, rolled back: {e}") + raise + + # Batch save rolls at half-inning boundary (separate transaction - audit data) if state.outs >= 3: - await self._advance_inning(state, game_id) - # Update DB again after inning change - await self.db_ops.update_game_state( - game_id=state.game_id, - inning=state.inning, - half=state.half, - home_score=state.home_score, - away_score=state.away_score, - status=state.status - ) - # Batch save rolls at half-inning boundary await self._batch_save_inning_rolls(game_id) # STEP 6: Prepare next play or clean up if game completed @@ -924,12 +958,22 @@ class GameEngine: # Rolls are still in _rolls_this_inning for retry on next inning boundary raise - async def _save_play_to_db(self, state: GameState, result: PlayResult) -> None: + async def _save_play_to_db( + self, + state: GameState, + result: PlayResult, + session: Optional[AsyncSession] = None + ) -> None: """ Save play to database using snapshot from GameState. Uses the pre-calculated snapshot fields (no database lookbacks). + Args: + state: Current game state + result: Play result to save + session: Optional external session for transaction grouping + Raises: ValueError: If required player IDs are missing """ @@ -1034,7 +1078,7 @@ class GameEngine: # Add stat fields to play_data play_data.update(stats) - await self.db_ops.save_play(play_data) + await self.db_ops.save_play(play_data, session=session) logger.debug(f"Saved play {state.play_count}: batter={batter_id}, on_base={on_base_code}") async def get_game_state(self, game_id: UUID) -> Optional[GameState]: diff --git a/backend/app/database/operations.py b/backend/app/database/operations.py index 5b051e3..c4eaaa2 100644 --- a/backend/app/database/operations.py +++ b/backend/app/database/operations.py @@ -15,6 +15,7 @@ import logging from typing import Optional, List, Dict from uuid import UUID from sqlalchemy import select +from sqlalchemy.ext.asyncio import AsyncSession from app.database.session import AsyncSessionLocal from app.models.db_models import Game, Play, Lineup, GameSession, RosterLink, Roll @@ -113,7 +114,8 @@ class DatabaseOperations: half: str, home_score: int, away_score: int, - status: Optional[str] = None + status: Optional[str] = None, + session: Optional[AsyncSession] = None ) -> None: """ Update game state fields using direct UPDATE (no SELECT). @@ -125,44 +127,51 @@ class DatabaseOperations: home_score: Home team score away_score: Away team score status: Game status if updating + session: Optional external session for transaction grouping Raises: ValueError: If game not found """ from sqlalchemy import update - async with AsyncSessionLocal() as session: - try: - # Build update values - update_values = { - "current_inning": inning, - "current_half": half, - "home_score": home_score, - "away_score": away_score - } + async def _do_update(sess: AsyncSession) -> None: + # Build update values + update_values = { + "current_inning": inning, + "current_half": half, + "home_score": home_score, + "away_score": away_score + } - if status: - update_values["status"] = status + if status: + update_values["status"] = status - # Direct UPDATE statement (no SELECT needed) - result = await session.execute( - update(Game) - .where(Game.id == game_id) - .values(**update_values) - ) + # Direct UPDATE statement (no SELECT needed) + result = await sess.execute( + update(Game) + .where(Game.id == game_id) + .values(**update_values) + ) - await session.commit() + if result.rowcount == 0: + raise ValueError(f"Game {game_id} not found for update") - # Check if game was found - if result.rowcount == 0: - raise ValueError(f"Game {game_id} not found") + # Use provided session or create new one + if session: + await _do_update(session) + # Don't commit - caller controls transaction + logger.debug(f"Updated game {game_id} state in external transaction") + else: + async with AsyncSessionLocal() as new_session: + try: + await _do_update(new_session) + await new_session.commit() + logger.debug(f"Updated game {game_id} state (inning {inning}, {half})") - logger.debug(f"Updated game {game_id} state (inning {inning}, {half})") - - except Exception as e: - await session.rollback() - logger.error(f"Failed to update game {game_id} state: {e}") - raise + except Exception as e: + await new_session.rollback() + logger.error(f"Failed to update game {game_id} state: {e}") + raise async def add_pd_lineup_card( self, @@ -402,12 +411,17 @@ class DatabaseOperations: logger.debug(f"Retrieved {len(subs)} eligible substitutes for team {team_id}") return subs - async def save_play(self, play_data: dict) -> int: + async def save_play( + self, + play_data: dict, + session: Optional[AsyncSession] = None + ) -> int: """ Save play to database. Args: play_data: Dictionary with play data matching Play model fields + session: Optional external session for transaction grouping Returns: Play ID (primary key) @@ -415,20 +429,29 @@ class DatabaseOperations: Raises: SQLAlchemyError: If database operation fails """ - async with AsyncSessionLocal() as session: - try: - play = Play(**play_data) - session.add(play) - await session.commit() - # Note: play.id is available after commit without refresh - play_id = play.id - logger.info(f"Saved play {play.play_number} for game {play.game_id}") - return play_id # type: ignore + async def _do_save(sess: AsyncSession) -> int: + play = Play(**play_data) + sess.add(play) + await sess.flush() # Get ID without committing + play_id = play.id + logger.info(f"Saved play {play.play_number} for game {play.game_id}") + return play_id # type: ignore - except Exception as e: - await session.rollback() - logger.error(f"Failed to save play: {e}") - raise + # Use provided session or create new one + if session: + return await _do_save(session) + # Don't commit - caller controls transaction + else: + async with AsyncSessionLocal() as new_session: + try: + play_id = await _do_save(new_session) + await new_session.commit() + return play_id + + except Exception as e: + await new_session.rollback() + logger.error(f"Failed to save play: {e}") + raise async def get_plays(self, game_id: UUID) -> List[Play]: """