fix(render): schedule image_url backfill on cache hits #216

Claude · 2026-04-11T15:13:36Z

Claude commented

2026-04-11 15:13:36 +00:00

Summary

Variant card rows with image_url=NULL could stay NULL forever once their PNG was cached on disk. The cache-hit short-circuit in get_batter_card returned FileResponse immediately, bypassing the backfill BackgroundTask at the bottom of the full-render path. Any variant row recreated with image_url=NULL while its PNG was still on disk stayed NULL forever, breaking every downstream consumer that reads battingcard.image_url / pitchingcard.image_url for display.

Confirmed today on dev: variant 415156387 on player 6557 (Nick Markakis), battingcard row 6342, has image_url=NULL even though the PNG is served at https://paper-dynasty.s3.us-east-1.amazonaws.com/cards/cardset-012/player-6557/v415156387/battingcard.png. Two render endpoint 200s in dev API logs, zero backfill log lines.

Fix

Extract _schedule_variant_image_backfill helper and wire it into both the cache-hit path and the full-render path. The helper's internal guards (variant > 0, tier is None, row exists with NULL image_url) keep happy-path cache hits a no-op. Behavior-neutral for the full-render path.

One function under surgery: get_batter_card in app/routers_v2/players.py. No schema changes, no new dependencies.

Design context: docs/superpowers/plans/2026-04-11-variant-image-url-cache-bug.md

Impact

Not dev-only. Prod hits this any time a variant row is regenerated with a pre-existing cached PNG — admin corrections, manual data fixes, and (once PR #215 lands) the dev cleanup rollback endpoint followed by re-test.

Tests

5 new tests in tests/test_players_card_render.py:
- Cache hit + image_url=None → backfill scheduled
- Cache hit + image_url populated → backfill NOT scheduled
- Cache hit + card row missing (DoesNotExist) → no crash, no backfill
- Cache hit + variant=0 → backfill NOT scheduled
- Cache hit + tier query param set → backfill NOT scheduled
Full suite: 271 passing, 13 pre-existing failures unchanged (animated card, apng, postgame refractor — unrelated)

DO NOT DEPLOY — Cal is manually testing against the dev API.

## Summary Variant card rows with `image_url=NULL` could stay NULL forever once their PNG was cached on disk. The cache-hit short-circuit in `get_batter_card` returned `FileResponse` immediately, bypassing the backfill `BackgroundTask` at the bottom of the full-render path. Any variant row recreated with `image_url=NULL` while its PNG was still on disk stayed NULL forever, breaking every downstream consumer that reads `battingcard.image_url` / `pitchingcard.image_url` for display. Confirmed today on dev: variant 415156387 on player 6557 (Nick Markakis), battingcard row 6342, has `image_url=NULL` even though the PNG is served at `https://paper-dynasty.s3.us-east-1.amazonaws.com/cards/cardset-012/player-6557/v415156387/battingcard.png`. Two render endpoint 200s in dev API logs, zero backfill log lines. ## Fix Extract `_schedule_variant_image_backfill` helper and wire it into both the cache-hit path and the full-render path. The helper's internal guards (`variant > 0`, `tier is None`, row exists with NULL `image_url`) keep happy-path cache hits a no-op. Behavior-neutral for the full-render path. One function under surgery: `get_batter_card` in `app/routers_v2/players.py`. No schema changes, no new dependencies. **Design context:** `docs/superpowers/plans/2026-04-11-variant-image-url-cache-bug.md` ## Impact Not dev-only. Prod hits this any time a variant row is regenerated with a pre-existing cached PNG — admin corrections, manual data fixes, and (once PR #215 lands) the dev cleanup rollback endpoint followed by re-test. ## Tests - 5 new tests in `tests/test_players_card_render.py`: - Cache hit + `image_url=None` → backfill scheduled - Cache hit + `image_url` populated → backfill NOT scheduled - Cache hit + card row missing (`DoesNotExist`) → no crash, no backfill - Cache hit + `variant=0` → backfill NOT scheduled - Cache hit + `tier` query param set → backfill NOT scheduled - Full suite: 271 passing, 13 pre-existing failures unchanged (animated card, apng, postgame refractor — unrelated) **DO NOT DEPLOY** — Cal is manually testing against the dev API.

Claude added 1 commit 2026-04-11 15:13:38 +00:00

fix(render): schedule image_url backfill on cache hits c0f862636a

The card render endpoint's file-on-disk cache short-circuit returned
FileResponse immediately when the cached PNG existed, bypassing the
backfill BackgroundTask that populates battingcard.image_url /
pitchingcard.image_url. If a variant row was later recreated with
image_url=NULL while the PNG was still cached, the DB column stayed
NULL forever, breaking every downstream consumer that reads image_url for
display.

Extract the backfill-scheduling logic into a _schedule_variant_image_backfill
helper and call it on both the cache-hit and full-render paths. The
helper's internal guards (variant > 0, tier is None, row exists with
NULL image_url) keep the happy-path cache hit a no-op.

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

Claude added the

ai-reviewing

label 2026-04-11 15:15:52 +00:00

Claude reviewed 2026-04-11 15:18:11 +00:00

Claude left a comment

AI Code Review

Files Reviewed

app/routers_v2/players.py (modified)
tests/test_players_card_render.py (added)

Findings

Correctness

No issues found.

Bug fix logic is sound. The cache-hit path previously returned FileResponse immediately without scheduling the backfill, leaving image_url=NULL permanently for any variant row recreated while its PNG was cached. The fix correctly calls _schedule_variant_image_backfill before returning.
Helper guards are logically equivalent to the old inline block. Old: if variant > 0 and tier is None. New: if variant == 0 or tier is not None: return False. De Morgan's law — identical semantics.
cached_path extraction is consistent. Both the os.path.isfile() check and the FileResponse call now use the same variable — eliminates the subtle dual-format-string duplication that was in the old code.
Full-render path is behavior-neutral. The old inline block and the new helper call produce identical outcomes.
CardModel.player_id query field correct. Matches the existing pattern at line 1063-1064 in the pre-PR file.
tier=tier in the cache-hit call is redundant but harmless. The outer tier is None condition guarantees tier is None at the call site, so the helper's tier is not None guard is always False there. Not a bug.

Security

No issues found. All query fields (player_id, variant, card_type, tier) are FastAPI-validated typed parameters — no raw user input in ORM queries.

Style & Conventions

Logging on every cache hit is new behavior. The old cache-hit path had no logging at all. The logging.info(...) now fires on every cache hit — including base card (variant=0) hits where backfill_scheduled=False. For a high-traffic endpoint, this could generate significant log volume. Consider downgrading to logging.debug or guarding: if backfill_scheduled: logging.info(...).
Return value of _schedule_variant_image_backfill discarded in full-render path. Minor asymmetry with the cache-hit path, which captures backfill_scheduled for logging. Not a bug — full-render path never logged this before.

Suggestions

The DB lookup on every variant cache hit (when image_url is already populated) is an extra round-trip per request. For popular variant cards that are fully backfilled, every cache hit now queries the DB to confirm image_url is not None. A future optimization could pass the image_url value through to avoid the lookup entirely, but this is out of scope for a bug-fix PR.

Verdict: COMMENT

Fix is correct, extraction is clean, and the five new tests cover all key scenarios (null backfill, populated no-op, missing row, variant=0, tier override). No blocking issues. Posted as COMMENT due to self-review restriction (PR author = reviewer).

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/routers_v2/players.py` (modified) - `tests/test_players_card_render.py` (added) ### Findings #### Correctness No issues found. - **Bug fix logic is sound.** The cache-hit path previously returned `FileResponse` immediately without scheduling the backfill, leaving `image_url=NULL` permanently for any variant row recreated while its PNG was cached. The fix correctly calls `_schedule_variant_image_backfill` before returning. - **Helper guards are logically equivalent to the old inline block.** Old: `if variant > 0 and tier is None`. New: `if variant == 0 or tier is not None: return False`. De Morgan's law — identical semantics. - **`cached_path` extraction is consistent.** Both the `os.path.isfile()` check and the `FileResponse` call now use the same variable — eliminates the subtle dual-format-string duplication that was in the old code. - **Full-render path is behavior-neutral.** The old inline block and the new helper call produce identical outcomes. - **`CardModel.player_id` query field correct.** Matches the existing pattern at line 1063-1064 in the pre-PR file. - **`tier=tier` in the cache-hit call is redundant but harmless.** The outer `tier is None` condition guarantees `tier` is `None` at the call site, so the helper's `tier is not None` guard is always False there. Not a bug. #### Security No issues found. All query fields (`player_id`, `variant`, `card_type`, `tier`) are FastAPI-validated typed parameters — no raw user input in ORM queries. #### Style & Conventions - **Logging on every cache hit is new behavior.** The old cache-hit path had no logging at all. The `logging.info(...)` now fires on every cache hit — including base card (`variant=0`) hits where `backfill_scheduled=False`. For a high-traffic endpoint, this could generate significant log volume. Consider downgrading to `logging.debug` or guarding: `if backfill_scheduled: logging.info(...)`. - **Return value of `_schedule_variant_image_backfill` discarded in full-render path.** Minor asymmetry with the cache-hit path, which captures `backfill_scheduled` for logging. Not a bug — full-render path never logged this before. #### Suggestions - The DB lookup on every variant cache hit (when `image_url` is already populated) is an extra round-trip per request. For popular variant cards that are fully backfilled, every cache hit now queries the DB to confirm `image_url is not None`. A future optimization could pass the `image_url` value through to avoid the lookup entirely, but this is out of scope for a bug-fix PR. ### Verdict: COMMENT Fix is correct, extraction is clean, and the five new tests cover all key scenarios (null backfill, populated no-op, missing row, variant=0, tier override). No blocking issues. Posted as COMMENT due to self-review restriction (PR author = reviewer). --- *Automated review by Claude PR Reviewer*

Claude added

ai-reviewed

and removed

ai-reviewing

labels 2026-04-11 15:18:35 +00:00

cal merged commit 1c2243602d into main

2026-04-11 15:24:34 +00:00

cal deleted branch fix/variant-image-url-cache

2026-04-11 15:24:34 +00:00

cal referenced this issue from a commit

2026-04-11 15:24:36 +00:00

Merge pull request 'fix(render): schedule image_url backfill on cache hits' (#216) from fix/variant-image-url-cache into main

Sign in to join this conversation.

No reviewers

No Milestone

No project

No Assignees

1 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: cal/paper-dynasty-database#216