From b8c55b57231e120e814f3e427c9b9a979a95d3dd Mon Sep 17 00:00:00 2001 From: Cal Corum Date: Wed, 18 Mar 2026 00:04:04 -0500 Subject: [PATCH] fix: address PR #104 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Correct idempotency guard docstring in update_season_stats() to accurately describe the last_game FK check limitation: only detects replay of the most-recently-processed game; out-of-order re-delivery (game G after G+1) bypasses the guard. References issue #105 for the planned ProcessedGame ledger fix. - Fix migration card_type comment: 'batting' or 'pitching' → 'batter', 'sp', or 'rp' to match actual seeded values. - Remove local rarity fixture in test_season_stats_update.py that shadowed the conftest.py fixture; remove unused rarity parameter from player_batter and player_pitcher fixtures. - Update test_double_count_prevention docstring to note the known out-of-order re-delivery limitation. Co-Authored-By: Claude Sonnet 4.6 --- app/services/season_stats.py | 29 ++++++++++++++----- .../2026-03-17_add_evolution_tables.sql | 2 +- tests/test_season_stats_update.py | 24 +++++++-------- 3 files changed, 34 insertions(+), 21 deletions(-) diff --git a/app/services/season_stats.py b/app/services/season_stats.py index 46d7e13..c9b01a1 100644 --- a/app/services/season_stats.py +++ b/app/services/season_stats.py @@ -2,9 +2,15 @@ season_stats.py — Incremental PlayerSeasonStats update logic. Called once per completed StratGame to accumulate batting and pitching -statistics into the player_season_stats table. The update is idempotent: -if this game_id has already been processed (detected via last_game FK), -the function returns early without double-counting. +statistics into the player_season_stats table. + +Idempotency limitation: re-delivery of a game is detected by checking +whether any PlayerSeasonStats row still carries that game_id as last_game. +This guard only works if no later game has been processed for the same +players — if game G+1 is processed first, a re-delivery of game G will +bypass the guard and double-count stats. A persistent processed-game +ledger is needed for full idempotency across out-of-order re-delivery +(see issue #105). Peewee upsert strategy: - SQLite: on_conflict_replace() — simplest path, deletes + re-inserts @@ -364,8 +370,15 @@ def update_season_stats(game_id: int) -> dict: Accumulate per-game batting and pitching stats into PlayerSeasonStats. This function is safe to call exactly once per game. If called again - for the same game_id (detected by checking last_game FK), it returns - immediately without modifying any data. + for the same game_id while it is still the most-recently-processed + game for at least one affected player (detected by checking last_game + FK), it returns early without modifying any data. + + Limitation: the guard only detects re-delivery if no later game has + been processed for the same players. Out-of-order re-delivery (e.g. + game G re-delivered after game G+1 was already processed) will not be + caught and will silently double-count stats. See issue #105 for the + planned ProcessedGame ledger fix. Algorithm: 1. Fetch StratGame to get the season. @@ -396,8 +409,10 @@ def update_season_stats(game_id: int) -> dict: season = game.season with db.atomic(): - # Step 2 — Double-count prevention: check if any row already - # carries this game_id as last_game + # Step 2 — Double-count prevention: check if any row still + # carries this game_id as last_game. Note: only detects replay + # of the most-recently-processed game; out-of-order re-delivery + # bypasses this guard (see issue #105). already_processed = ( PlayerSeasonStats.select() .where(PlayerSeasonStats.last_game == game_id) diff --git a/migrations/2026-03-17_add_evolution_tables.sql b/migrations/2026-03-17_add_evolution_tables.sql index 5ab57aa..e084dce 100644 --- a/migrations/2026-03-17_add_evolution_tables.sql +++ b/migrations/2026-03-17_add_evolution_tables.sql @@ -90,7 +90,7 @@ CREATE INDEX IF NOT EXISTS player_season_stats_player_season_idx CREATE TABLE IF NOT EXISTS evolution_track ( id SERIAL PRIMARY KEY, name VARCHAR(255) UNIQUE NOT NULL, - card_type VARCHAR(50) NOT NULL, -- 'batting' or 'pitching' + card_type VARCHAR(50) NOT NULL, -- 'batter', 'sp', or 'rp' formula VARCHAR(255) NOT NULL, -- e.g. 'hr', 'k_per_9', 'ops' t1_threshold INTEGER NOT NULL, t2_threshold INTEGER NOT NULL, diff --git a/tests/test_season_stats_update.py b/tests/test_season_stats_update.py index cfa0dcf..6919bea 100644 --- a/tests/test_season_stats_update.py +++ b/tests/test_season_stats_update.py @@ -156,11 +156,6 @@ def make_play(game, play_num, batter, batter_team, pitcher, pitcher_team, **stat # --------------------------------------------------------------------------- -@pytest.fixture -def rarity(): - return Rarity.create(value=1, name="Common", color="#ffffff") - - @pytest.fixture def team_a(): return _make_team("TMA", gmid=1001) @@ -172,13 +167,13 @@ def team_b(): @pytest.fixture -def player_batter(rarity): +def player_batter(): """A batter-type player for team A.""" return _make_player("Batter One", pos="CF") @pytest.fixture -def player_pitcher(rarity): +def player_pitcher(): """A pitcher-type player for team B.""" return _make_player("Pitcher One", pos="SP") @@ -399,15 +394,18 @@ def test_decision_integration(team_a, team_b, player_batter, player_pitcher, gam def test_double_count_prevention(team_a, team_b, player_batter, player_pitcher, game): """Calling update_season_stats() twice for the same game must not double the stats. - What: Process a game once (pa=3), then call the function again. The - second call should detect the already-processed state via the - PlayerSeasonStats.last_game FK check and return early with 'skipped'=True. + What: Process a game once (pa=3), then immediately call the function + again with the same game_id. The second call detects via the + PlayerSeasonStats.last_game FK check that this game is still the + most-recently-processed game and returns early with 'skipped'=True. The resulting pa should still be 3, not 6. Why: The bot infrastructure may deliver game-complete events more than - once (network retries, message replays). Without idempotency, stats - would accumulate incorrectly and could not be corrected without a full - reset. + once (network retries, message replays). The guard prevents + double-counting when the replayed game is still the last game + processed for those players. Note: this test only covers same-game + immediate replay — out-of-order re-delivery (game G after G+1) is a + known limitation tracked in issue #105. """ for i in range(3): make_play(