CLAUDE: Add transaction handling for multi-step DB operations

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 <noreply@anthropic.com>

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
 

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,9 +485,15 @@ 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 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)
+
                 # STEP 4: Update game state in DB only if something changed
                 if (state.inning != state_before['inning'] or
                     state.half != state_before['half'] or
@@ -502,7 +507,8 @@ class GameEngine:
                         half=state.half,
                         home_score=state.home_score,
                         away_score=state.away_score,
-                status=state.status
+                        status=state.status,
+                        session=session
                     )
                     logger.info("Updated game state in DB - score/inning/status changed")
                 else:
@@ -518,9 +524,21 @@ class GameEngine:
                         half=state.half,
                         home_score=state.home_score,
                         away_score=state.away_score,
-                status=state.status
+                        status=state.status,
+                        session=session
                     )
-            # Batch save rolls at half-inning boundary
+
+                # 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._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,9 +639,15 @@ 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 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)
+
                 # STEP 4: Update game state in DB only if something changed
                 if (state.inning != state_before['inning'] or
                     state.half != state_before['half'] or
@@ -640,7 +661,8 @@ class GameEngine:
                         half=state.half,
                         home_score=state.home_score,
                         away_score=state.away_score,
-                status=state.status
+                        status=state.status,
+                        session=session
                     )
                     logger.info("Updated game state in DB - score/inning/status changed")
                 else:
@@ -656,9 +678,21 @@ class GameEngine:
                         half=state.half,
                         home_score=state.home_score,
                         away_score=state.away_score,
-                status=state.status
+                        status=state.status,
+                        session=session
                     )
-            # Batch save rolls at half-inning boundary
+
+                # 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._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]:

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,14 +127,14 @@ 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:
+        async def _do_update(sess: AsyncSession) -> None:
-            try:
             # Build update values
             update_values = {
                 "current_inning": inning,
@@ -145,22 +147,29 @@ class DatabaseOperations:
                 update_values["status"] = status
 
             # Direct UPDATE statement (no SELECT needed)
-                result = await session.execute(
+            result = await sess.execute(
                 update(Game)
                 .where(Game.id == game_id)
                 .values(**update_values)
             )
 
-                await session.commit()
-
-                # Check if game was found
             if result.rowcount == 0:
-                    raise ValueError(f"Game {game_id} not found")
+                raise ValueError(f"Game {game_id} not found for update")
 
+        # 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})")
 
                 except Exception as e:
-                await session.rollback()
+                    await new_session.rollback()
                     logger.error(f"Failed to update game {game_id} state: {e}")
                     raise
 
@@ -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,18 +429,27 @@ class DatabaseOperations:
         Raises:
             SQLAlchemyError: If database operation fails
         """
-        async with AsyncSessionLocal() as session:
+        async def _do_save(sess: AsyncSession) -> int:
-            try:
             play = Play(**play_data)
-                session.add(play)
+            sess.add(play)
-                await session.commit()
+            await sess.flush()  # Get ID without committing
-                # 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
 
+        # 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 session.rollback()
+                    await new_session.rollback()
                     logger.error(f"Failed to save play: {e}")
                     raise