refactor: extract evaluate-game business logic from router to service layer (#202)

Closes #202

Adds app/services/refractor_service.py with two service functions:

- ensure_variant_cards: idempotent function that creates missing variant
  cards for all tiers up to a target, with partial failure handling and
  REFRACTOR_BOOST_ENABLED kill switch support.

- evaluate_and_boost: combines evaluate_card(dry_run=True) with
  ensure_variant_cards so both tier computation and variant-card creation
  happen in one testable call without HTTP round-trips.

Updates both router endpoints to use evaluate_and_boost:

- POST /cards/{card_id}/evaluate now calls evaluate_and_boost instead of
  evaluate_card directly, which also fixes the broken variant creation on
  the manual evaluate path (variant cards are now created when a tier-up
  is detected, not just recorded).

- POST /evaluate-game/{game_id} replaces ~55 lines of inline boost
  orchestration (boost_enabled flag, tier loop, partial failure tracking,
  card_type/track null checks) with a single evaluate_and_boost call.
  Response shape is unchanged.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

app/db_engine.py

@@ -44,10 +44,6 @@ else:
         pragmas={"journal_mode": "wal", "cache_size": -1 * 64000, "synchronous": 0},
     )
 
-# Refractor stat accumulation starts at this season — stats from earlier seasons
-# are excluded from evaluation queries.  Override via REFRACTOR_START_SEASON env var.
-REFRACTOR_START_SEASON = int(os.environ.get("REFRACTOR_START_SEASON", "11"))
-
 # 2025, 2005
 ranked_cardsets = [24, 25, 26, 27, 28, 29]
 LIVE_CARDSET_ID = 27

app/routers_v2/players.py

@@ -40,7 +40,7 @@ from ..db_engine import (
 )
 from ..db_helpers import upsert_players
 from ..dependencies import oauth2_scheme, valid_token
-from ..services.card_storage import backfill_variant_image_url, upload_variant_apng
+from ..services.card_storage import backfill_variant_image_url
 from ..services.refractor_boost import compute_variant_hash
 from ..services.apng_generator import apng_cache_path, generate_animated_card
 
@@ -740,7 +740,6 @@ async def get_one_player(player_id: int, csv: Optional[bool] = False):
 @router.get("/{player_id}/{card_type}card/{d}/{variant}/animated")
 async def get_animated_card(
     request: Request,
-    background_tasks: BackgroundTasks,
     player_id: int,
     card_type: Literal["batting", "pitching"],
     variant: int,
@@ -861,16 +860,6 @@ async def get_animated_card(
     finally:
         await page.close()
 
-    if tier is None:
-        background_tasks.add_task(
-            upload_variant_apng,
-            player_id=player_id,
-            variant=variant,
-            card_type=card_type,
-            cardset_id=this_player.cardset.id,
-            apng_path=cache_path,
-        )
-
     return FileResponse(path=cache_path, media_type="image/apng", headers=headers)
 
 

app/routers_v2/refractor.py

@@ -1,6 +1,3 @@
-import os
-from datetime import date
-
 from fastapi import APIRouter, Depends, HTTPException, Query
 import logging
 from typing import Optional
@@ -339,7 +336,7 @@ async def evaluate_card(card_id: int, token: str = Depends(oauth2_scheme)):
         raise HTTPException(status_code=401, detail="Unauthorized")
 
     from ..db_engine import Card
-    from ..services.refractor_evaluator import evaluate_card as _evaluate
+    from ..services.refractor_service import evaluate_and_boost
 
     try:
         card = Card.get_by_id(card_id)
@@ -347,7 +344,7 @@ async def evaluate_card(card_id: int, token: str = Depends(oauth2_scheme)):
         raise HTTPException(status_code=404, detail=f"Card {card_id} not found")
 
     try:
-        result = _evaluate(card.player_id, card.team_id)
+        result = evaluate_and_boost(card.player_id, card.team_id)
     except ValueError as exc:
         raise HTTPException(status_code=404, detail=str(exc))
 
@@ -369,8 +366,7 @@ async def evaluate_game(game_id: int, token: str = Depends(oauth2_scheme)):
         raise HTTPException(status_code=401, detail="Unauthorized")
 
     from ..db_engine import RefractorCardState, Player, StratPlay
-    from ..services.refractor_boost import apply_tier_boost
-    from ..services.refractor_evaluator import evaluate_card
+    from ..services.refractor_service import evaluate_and_boost
 
     plays = list(StratPlay.select().where(StratPlay.game == game_id))
 
@@ -384,8 +380,6 @@ async def evaluate_game(game_id: int, token: str = Depends(oauth2_scheme)):
     evaluated = 0
     tier_ups = []
 
-    boost_enabled = os.environ.get("REFRACTOR_BOOST_ENABLED", "true").lower() != "false"
-
     for player_id, team_id in pairs:
         try:
             state = RefractorCardState.get_or_none(
@@ -406,16 +400,11 @@ async def evaluate_game(game_id: int, token: str = Depends(oauth2_scheme)):
                     continue
 
             old_tier = state.current_tier
-            # Use dry_run=True so that current_tier is NOT written here.
-            # apply_tier_boost() writes current_tier + variant atomically on
-            # tier-up.  If no tier-up occurs, apply_tier_boost is not called
-            # and the tier stays at old_tier (correct behaviour).
-            result = evaluate_card(player_id, team_id, dry_run=True)
+            result = evaluate_and_boost(player_id, team_id)
             evaluated += 1
 
-            # Use computed_tier (what the formula says) to detect tier-ups.
-            computed_tier = result.get("computed_tier", old_tier)
-            if computed_tier > old_tier:
+            new_tier = result.get("current_tier", old_tier)
+            if new_tier > old_tier:
                 player_name = "Unknown"
                 try:
                     p = Player.get_by_id(player_id)
@@ -423,69 +412,19 @@ async def evaluate_game(game_id: int, token: str = Depends(oauth2_scheme)):
                 except Exception:
                     pass
 
-                # Phase 2: Apply rating boosts for each tier gained.
-                # apply_tier_boost() writes current_tier + variant atomically.
-                # If it fails, current_tier stays at old_tier — automatic retry next game.
-                boost_result = None
-                if not boost_enabled:
-                    # Boost disabled via REFRACTOR_BOOST_ENABLED=false.
-                    # Skip notification — current_tier was not written (dry_run),
-                    # so reporting a tier-up would be a false notification.
-                    continue
-
-                card_type = state.track.card_type if state.track else None
-                if card_type:
-                    last_successful_tier = old_tier
-                    failing_tier = old_tier + 1
-                    try:
-                        for tier in range(old_tier + 1, computed_tier + 1):
-                            failing_tier = tier
-                            boost_result = apply_tier_boost(
-                                player_id, team_id, tier, card_type
-                            )
-                            last_successful_tier = tier
-                    except Exception as boost_exc:
-                        logger.warning(
-                            f"Refractor boost failed for player={player_id} "
-                            f"team={team_id} tier={failing_tier}: {boost_exc}"
-                        )
-                        # Report only the tiers that actually succeeded.
-                        # If none succeeded, skip the tier_up notification entirely.
-                        if last_successful_tier == old_tier:
-                            continue
-                        # At least one intermediate tier was committed; report that.
-                        computed_tier = last_successful_tier
-                else:
-                    # No card_type means no track — skip boost and skip notification.
-                    # A false tier-up notification must not be sent when the boost
-                    # was never applied (current_tier was never written to DB).
-                    logger.warning(
-                        f"Refractor boost skipped for player={player_id} "
-                        f"team={team_id}: no card_type on track"
-                    )
-                    continue
-
                 tier_up_entry = {
                     "player_id": player_id,
                     "team_id": team_id,
                     "player_name": player_name,
                     "old_tier": old_tier,
-                    "new_tier": computed_tier,
+                    "new_tier": new_tier,
                     "current_value": result.get("current_value", 0),
                     "track_name": state.track.name if state.track else "Unknown",
                 }
 
-                # Non-breaking addition: include boost info when available.
-                if boost_result:
-                    variant_num = boost_result.get("variant_created")
-                    tier_up_entry["variant_created"] = variant_num
-                    if computed_tier >= 3 and variant_num and card_type:
-                        d = date.today().strftime("%Y-%m-%d")
-                        api_base = os.environ.get("API_BASE_URL", "").rstrip("/")
-                        tier_up_entry["animated_url"] = (
-                            f"{api_base}/api/v2/players/{player_id}/{card_type}card"
-                            f"/{d}/{variant_num}/animated"
-                        )
+                variants_created = result.get("variants_created") or []
+                if variants_created:
+                    tier_up_entry["variant_created"] = variants_created[-1]
 
                 tier_ups.append(tier_up_entry)
 

app/services/card_storage.py

@@ -8,10 +8,7 @@ get_s3_client()
     (environment variables or instance profile).
 
 build_s3_key(cardset_id, player_id, variant, card_type)
-    Construct the S3 object key for a variant card PNG image.
-
-build_apng_s3_key(cardset_id, player_id, variant, card_type)
-    Construct the S3 object key for a variant animated card APNG.
+    Construct the S3 object key for a variant card image.
 
 build_s3_url(s3_key, render_date)
     Return the full HTTPS S3 URL with a cache-busting date query param.
@@ -19,19 +16,11 @@ build_s3_url(s3_key, render_date)
 upload_card_to_s3(s3_client, png_bytes, s3_key)
     Upload raw PNG bytes to S3 with correct ContentType and CacheControl headers.
 
-upload_apng_to_s3(s3_client, apng_bytes, s3_key)
-    Upload raw APNG bytes to S3 with correct ContentType and CacheControl headers.
-
 backfill_variant_image_url(player_id, variant, card_type, cardset_id, png_path)
     End-to-end: read PNG from disk, upload to S3, update BattingCard or
     PitchingCard.image_url in the database. All exceptions are caught and
     logged; this function never raises (safe to call as a background task).
 
-upload_variant_apng(player_id, variant, card_type, cardset_id, apng_path)
-    End-to-end: read APNG from disk and upload to S3. No DB update (no
-    animated_url column exists yet). All exceptions are caught and logged;
-    this function never raises (safe to call as a background task).
-
 Design notes
 ------------
 - S3 credentials are resolved from the environment by boto3 at call time;
@@ -108,29 +97,6 @@ def build_s3_url(s3_key: str, render_date: date) -> str:
     return f"{base_url}/{s3_key}?d={date_str}"
 
 
-def build_apng_s3_key(
-    cardset_id: int, player_id: int, variant: int, card_type: str
-) -> str:
-    """Construct the S3 object key for a variant animated card APNG.
-
-    Key format:
-        cards/cardset-{csid:03d}/player-{pid}/v{variant}/{card_type}card.apng
-
-    Args:
-        cardset_id: Numeric cardset ID (zero-padded to 3 digits).
-        player_id:  Player ID.
-        variant:    Variant number (1-4 = refractor tiers).
-        card_type:  Either "batting" or "pitching".
-
-    Returns:
-        The S3 object key string.
-    """
-    return (
-        f"cards/cardset-{cardset_id:03d}/player-{player_id}"
-        f"/v{variant}/{card_type}card.apng"
-    )
-
-
 def upload_card_to_s3(s3_client, png_bytes: bytes, s3_key: str) -> None:
     """Upload raw PNG bytes to S3 with the standard card image headers.
 
@@ -230,81 +196,3 @@ def backfill_variant_image_url(
             variant,
             card_type,
         )
-
-
-def upload_apng_to_s3(s3_client, apng_bytes: bytes, s3_key: str) -> None:
-    """Upload raw APNG bytes to S3 with the standard animated card headers.
-
-    Sets ContentType=image/apng and CacheControl=public, max-age=86400 (1 day)
-    matching the animated endpoint's own Cache-Control header.
-
-    Args:
-        s3_client:  A boto3 S3 client (from get_s3_client).
-        apng_bytes: Raw APNG image bytes.
-        s3_key:     S3 object key (from build_apng_s3_key).
-
-    Returns:
-        None
-    """
-    s3_client.put_object(
-        Bucket=S3_BUCKET,
-        Key=s3_key,
-        Body=apng_bytes,
-        ContentType="image/apng",
-        CacheControl="public, max-age=86400",
-    )
-
-
-def upload_variant_apng(
-    player_id: int,
-    variant: int,
-    card_type: str,
-    cardset_id: int,
-    apng_path: str,
-) -> None:
-    """Read a rendered APNG from disk and upload it to S3.
-
-    Intended to be called as a background task after a new animated card is
-    rendered. No DB update is performed (no animated_url column exists yet).
-
-    All exceptions are caught and logged — this function is intended to be
-    called as a background task and must never propagate exceptions.
-
-    Args:
-        player_id:  Player ID used for the S3 key.
-        variant:    Variant number (matches the refractor tier variant).
-        card_type:  "batting" or "pitching" — selects the S3 key.
-        cardset_id: Cardset ID used for the S3 key.
-        apng_path:  Absolute path to the rendered APNG file on disk.
-
-    Returns:
-        None
-    """
-    try:
-        with open(apng_path, "rb") as f:
-            apng_bytes = f.read()
-
-        s3_key = build_apng_s3_key(
-            cardset_id=cardset_id,
-            player_id=player_id,
-            variant=variant,
-            card_type=card_type,
-        )
-        s3_client = get_s3_client()
-        upload_apng_to_s3(s3_client, apng_bytes, s3_key)
-
-        logger.info(
-            "upload_variant_apng: uploaded %s animated card player=%s variant=%s key=%s",
-            card_type,
-            player_id,
-            variant,
-            s3_key,
-        )
-
-    except Exception:
-        logger.exception(
-            "upload_variant_apng: failed for player=%s variant=%s card_type=%s",
-            player_id,
-            variant,
-            card_type,
-        )

app/services/refractor_evaluator.py

@@ -148,11 +148,10 @@ def evaluate_card(
             strikeouts=sum(r.strikeouts for r in rows),
         )
     else:
-        from app.db_engine import (  # noqa: PLC0415
+        from app.db_engine import (
             BattingSeasonStats,
             PitchingSeasonStats,
-            REFRACTOR_START_SEASON,
-        )
+        )  # noqa: PLC0415
 
         card_type = card_state.track.card_type
         if card_type == "batter":
@@ -160,7 +159,6 @@ def evaluate_card(
                 BattingSeasonStats.select().where(
                     (BattingSeasonStats.player == player_id)
                     & (BattingSeasonStats.team == team_id)
-                    & (BattingSeasonStats.season >= REFRACTOR_START_SEASON)
                 )
             )
             totals = _CareerTotals(
@@ -177,7 +175,6 @@ def evaluate_card(
                 PitchingSeasonStats.select().where(
                     (PitchingSeasonStats.player == player_id)
                     & (PitchingSeasonStats.team == team_id)
-                    & (PitchingSeasonStats.season >= REFRACTOR_START_SEASON)
                 )
             )
             totals = _CareerTotals(

app/services/refractor_service.py

@@ -0,0 +1,200 @@
+"""Refractor service layer — shared orchestration across router endpoints.
+
+Provides ``ensure_variant_cards`` and ``evaluate_and_boost`` so that both
+the evaluate-game endpoint and the manual card-evaluate endpoint share the
+same boost orchestration logic without requiring HTTP round-trips in tests.
+"""
+
+import logging
+import os
+
+logger = logging.getLogger(__name__)
+
+
+def ensure_variant_cards(
+    player_id: int,
+    team_id: int,
+    target_tier: int | None = None,
+    card_type: str | None = None,
+    *,
+    _state_model=None,
+) -> dict:
+    """Ensure variant cards exist for all tiers up to target_tier.
+
+    Idempotent — safe to call multiple times.  If a variant card already
+    exists for a given tier it is skipped.  Partial failures are tolerated:
+    lower tiers that were committed are reported even if a higher tier fails.
+
+    Args:
+        player_id: Player primary key.
+        team_id: Team primary key.
+        target_tier: Highest tier to ensure variants for.  If None, uses
+            ``state.current_tier`` (backfill mode — creates missing variants
+            for tiers already recorded in the DB).
+        card_type: One of 'batter', 'sp', 'rp'.  If None, derived from
+            ``state.track.card_type``.
+        _state_model: Dependency-injection override for RefractorCardState
+            (used in tests).
+
+    Returns:
+        Dict with keys:
+          - ``current_tier`` (int): highest tier confirmed in the DB after
+            this call (equal to state.current_tier if no new tiers created).
+          - ``variants_created`` (list[int]): variant hashes newly created.
+          - ``boost_results`` (list[dict]): raw return values from
+            apply_tier_boost for each newly created variant.
+    """
+    if _state_model is None:
+        from app.db_engine import RefractorCardState as _state_model  # noqa: PLC0415
+
+    state = _state_model.get_or_none(
+        (_state_model.player_id == player_id) & (_state_model.team_id == team_id)
+    )
+    if state is None:
+        return {"current_tier": 0, "variants_created": [], "boost_results": []}
+
+    if target_tier is None:
+        target_tier = state.current_tier
+
+    if target_tier == 0:
+        return {
+            "current_tier": state.current_tier,
+            "variants_created": [],
+            "boost_results": [],
+        }
+
+    # Resolve card_type from track if not provided.
+    resolved_card_type = card_type
+    if resolved_card_type is None:
+        resolved_card_type = state.track.card_type if state.track else None
+    if resolved_card_type is None:
+        logger.warning(
+            "ensure_variant_cards: no card_type for player=%s team=%s — skipping",
+            player_id,
+            team_id,
+        )
+        return {
+            "current_tier": state.current_tier,
+            "variants_created": [],
+            "boost_results": [],
+        }
+
+    # Respect kill switch.
+    boost_enabled = os.environ.get("REFRACTOR_BOOST_ENABLED", "true").lower() != "false"
+    if not boost_enabled:
+        return {
+            "current_tier": state.current_tier,
+            "variants_created": [],
+            "boost_results": [],
+        }
+
+    from app.services.refractor_boost import (  # noqa: PLC0415
+        apply_tier_boost,
+        compute_variant_hash,
+    )
+
+    is_batter = resolved_card_type == "batter"
+    if is_batter:
+        from app.db_engine import BattingCard as CardModel  # noqa: PLC0415
+    else:
+        from app.db_engine import PitchingCard as CardModel  # noqa: PLC0415
+
+    variants_created = []
+    boost_results = []
+    last_successful_tier = state.current_tier
+
+    for tier in range(1, target_tier + 1):
+        variant_hash = compute_variant_hash(player_id, tier)
+        existing = CardModel.get_or_none(
+            (CardModel.player == player_id) & (CardModel.variant == variant_hash)
+        )
+        if existing is not None:
+            # Already exists — idempotent skip.
+            continue
+        try:
+            boost_result = apply_tier_boost(
+                player_id, team_id, tier, resolved_card_type
+            )
+            variants_created.append(variant_hash)
+            boost_results.append(boost_result)
+            last_successful_tier = tier
+        except Exception as exc:
+            logger.warning(
+                "ensure_variant_cards: boost failed for player=%s team=%s tier=%s: %s",
+                player_id,
+                team_id,
+                tier,
+                exc,
+            )
+            # Don't attempt higher tiers if a lower one failed; the missing
+            # lower-tier variant would cause apply_tier_boost to fail for T+1
+            # anyway (it reads from the previous tier's variant as its source).
+            break
+
+    return {
+        "current_tier": last_successful_tier,
+        "variants_created": variants_created,
+        "boost_results": boost_results,
+    }
+
+
+def evaluate_and_boost(
+    player_id: int,
+    team_id: int,
+    *,
+    _state_model=None,
+    _stats_model=None,
+) -> dict:
+    """Full evaluation: recompute tier from career stats, then ensure variant cards exist.
+
+    Combines evaluate_card (dry_run=True) with ensure_variant_cards so that
+    both tier computation and variant-card creation happen in a single call.
+    Handles both tier-up cases (computed_tier > stored) and backfill cases
+    (tier already in DB but variant card was never created).
+
+    Args:
+        player_id: Player primary key.
+        team_id: Team primary key.
+        _state_model: DI override for RefractorCardState (used in tests).
+        _stats_model: DI override for stats model passed to evaluate_card.
+
+    Returns:
+        Dict containing all fields from evaluate_card plus:
+          - ``current_tier`` (int): highest tier confirmed in the DB after
+            this call (may be higher than eval_result["current_tier"] if a
+            tier-up was committed here).
+          - ``variants_created`` (list[int]): variant hashes newly created.
+          - ``boost_results`` (list[dict]): raw boost result dicts.
+
+    Raises:
+        ValueError: If no RefractorCardState exists for (player_id, team_id).
+    """
+    from app.services.refractor_evaluator import evaluate_card  # noqa: PLC0415
+
+    eval_kwargs: dict = {"dry_run": True}
+    if _state_model is not None:
+        eval_kwargs["_state_model"] = _state_model
+    if _stats_model is not None:
+        eval_kwargs["_stats_model"] = _stats_model
+
+    eval_result = evaluate_card(player_id, team_id, **eval_kwargs)
+
+    computed_tier = eval_result.get("computed_tier", 0)
+    stored_tier = eval_result.get("current_tier", 0)
+    # target_tier is the higher of formula result and stored value:
+    # - tier-up case: computed > stored, creates new variant(s)
+    # - backfill case: stored > computed (stale), creates missing variants
+    target_tier = max(computed_tier, stored_tier)
+
+    ensure_kwargs: dict = {"target_tier": target_tier}
+    if _state_model is not None:
+        ensure_kwargs["_state_model"] = _state_model
+
+    ensure_result = ensure_variant_cards(player_id, team_id, **ensure_kwargs)
+
+    return {
+        **eval_result,
+        "current_tier": ensure_result["current_tier"],
+        "variants_created": ensure_result["variants_created"],
+        "boost_results": ensure_result["boost_results"],
+    }