diff --git a/MPCAutofill/cardpicker/management/commands/local_calculate_verdicts.py b/MPCAutofill/cardpicker/management/commands/local_calculate_verdicts.py index 3c3b2a6d9..8c332a4f1 100644 --- a/MPCAutofill/cardpicker/management/commands/local_calculate_verdicts.py +++ b/MPCAutofill/cardpicker/management/commands/local_calculate_verdicts.py @@ -22,6 +22,7 @@ merge_counters, resilient_terminal_output, ) +from cardpicker.printing_metadata_import import ensure_scryfall_cache_present from cardpicker.utils import find_stale_applied_migrations, get_baked_git_sha @@ -40,7 +41,9 @@ class Command(BaseCommand): "and requires an explicit --write to actually write, matching local_residual_classify's " "own convention. --write also requires a matching COMPLETED dry-run PilotRunLedger row " "from the last --dry-run-window-hours (forced-dry-run guard, issue #362) - see " - "--skip-dryrun-check to override." + "--skip-dryrun-check to override. Refuses to start at all if the Scryfall bulk-data cache " + "(scryfall_cache/default_cards.json) is missing (issue #402) unless " + "--allow-missing-scryfall-cache is passed." ) def add_arguments(self, parser: Any) -> None: @@ -56,6 +59,16 @@ def add_arguments(self, parser: Any) -> None: parser.add_argument( "--chunk-size", type=int, default=500, help="Queryset .iterator() chunk size. Default: 500." ) + parser.add_argument( + "--allow-missing-scryfall-cache", + action="store_true", + default=False, + help="Explicitly accept a missing Scryfall bulk-data cache (scryfall_cache/" + "default_cards.json) instead of refusing to start (issue #402's fail-loud guard - " + "see printing_metadata_import.ensure_scryfall_cache_present). Without this flag, a " + "missing cache is a hard CommandError, not the silent degraded-to-empty back-face " + "lookup this command used to run with.", + ) # Forced-dry-run guard (issue #362, Phase 0 rails): this command has no caller-chosen # cohort narrower than "whatever's currently eligible" (unlike reparse_collector_evidence's # --selector or retract_stage_d_by_run_id's --run-id), so the guard below always passes @@ -72,6 +85,13 @@ def handle(self, *args: Any, **kwargs: Any) -> None: "code before running this command." ) + # Fail-loud staleness guard (issue #402): must run before any card-by-card work below, + # which otherwise silently degrades to an empty back-face lookup (get_back_face_names' + # own soft-fail path) if the cache file is missing - see + # ensure_scryfall_cache_present's own docstring. + if not kwargs["allow_missing_scryfall_cache"]: + ensure_scryfall_cache_present() + run_id = kwargs["run_id"] or generate_run_id() dry_run = not kwargs["write"] mode = "WRITE" if kwargs["write"] else "DRY RUN" diff --git a/MPCAutofill/cardpicker/printing_metadata_import.py b/MPCAutofill/cardpicker/printing_metadata_import.py index 1fd8c0e84..d308ceedc 100644 --- a/MPCAutofill/cardpicker/printing_metadata_import.py +++ b/MPCAutofill/cardpicker/printing_metadata_import.py @@ -12,6 +12,7 @@ from pydantic import BaseModel, ValidationError from django.conf import settings +from django.core.management.base import CommandError from cardpicker.integrations.game.mtg import Scryfall from cardpicker.models import CanonicalCard, CanonicalPrintingMetadata @@ -86,6 +87,45 @@ def _is_stale(path: Path) -> bool: return not path.exists() or time.time() - path.stat().st_mtime > 7 * 24 * 3600 +def ensure_scryfall_cache_present(default_cards_path: Path | None = None) -> None: + """ + Fail-loud staleness guard (issue #402). The Scryfall bulk-data cache + (`scryfall_cache/default_cards.json`, ~558MB) lived as a plain file inside the django/worker + container filesystem with no persistent volume mounted over it - every image rebuild silently + destroyed it. Deploy-2 (2026-07-23) did exactly that, and `local_calculate_verdicts` went on + to run degraded: `get_back_face_names`/`is_back_face` (see their own docstrings) treat a + missing file as "no back faces known yet" - logging one warning and returning an empty + `frozenset()` - which is the right behaviour for THAT function (a per-card lookup has no + business raising), but left the whole run silently degraded with nothing loud enough to catch + in a routine log skim. + + This is the loud counterpart: call it ONCE, at a long-running command's own start (see + `local_calculate_verdicts`'s `Command.handle()`), BEFORE any card-by-card work begins. Raises + `CommandError` immediately if the cache file does not exist. Callers that have deliberately + decided to accept the degraded (empty back-face lookup) mode - e.g. a fresh bootstrap that + hasn't run `import_scryfall_printing_metadata`/`import_canonical_card_data` yet - should catch + this by not calling the guard at all (an explicit `--allow-missing-scryfall-cache`-style flag + at the call site), rather than this function growing its own bypass flag. + + Deliberately checks EXISTENCE only, not staleness - `_is_stale`'s 7-day weekly re-download + window is a completely separate, already-working concern + (`import_scryfall_printing_metadata`/`import_canonical_card_data` both already re-fetch a + stale-but-present file on their own next run); this guard only exists to catch the file being + GONE. + """ + path = default_cards_path or _cache_path() + if not path.exists(): + raise CommandError( + f"SCRYFALL CACHE MISSING: {path} does not exist. This is the persistent " + "scryfall_cache volume (docker/docker-compose.prod.yml) backing " + "get_back_face_names/is_back_face's back-face lookup - proceeding without it " + "silently degrades that lookup to an empty set (see printing_metadata_import." + "ensure_scryfall_cache_present's own docstring), not a loud failure. Populate it by " + "running import_scryfall_printing_metadata or import_canonical_card_data first, or " + "pass --allow-missing-scryfall-cache to explicitly accept the degraded mode." + ) + + @section_timer(name="get default_cards bulk data URL") def _get_default_cards_url() -> str: response = requests.get("https://api.scryfall.com/bulk-data", headers=Scryfall.get_headers()) diff --git a/MPCAutofill/cardpicker/tests/test_local_calculate_verdicts.py b/MPCAutofill/cardpicker/tests/test_local_calculate_verdicts.py index 6030ef3cc..badc8a0f9 100644 --- a/MPCAutofill/cardpicker/tests/test_local_calculate_verdicts.py +++ b/MPCAutofill/cardpicker/tests/test_local_calculate_verdicts.py @@ -24,6 +24,7 @@ import imagehash import pytest +from django.core.management import CommandError from django.db import connection from cardpicker.local_calculate_verdicts import ( @@ -72,6 +73,7 @@ PrintingTagStatus, VoteSource, ) +from cardpicker.printing_metadata_import import ensure_scryfall_cache_present from cardpicker.tests.factories import ( CanonicalArtistFactory, CanonicalCardFactory, @@ -1617,20 +1619,24 @@ def test_a_card_the_fallback_calculator_only_scanned_is_still_routed(self, db): class TestCommandLedgerHardeningAndDryRunGuard: """Phase 0 rails (issues #362/#153's milestone): the Command.handle() lifecycle itself, on an empty (zero-eligible-card) DB - the calculator behaviour above is already exhaustively covered - by the pure-function tests in this file; these exercise the ledger/guard WIRING around it.""" + by the pure-function tests in this file; these exercise the ledger/guard WIRING around it. + + None of these tests are exercising the scryfall-cache guard (issue #402, see + TestScryfallCacheGuard below) - they all pass --allow-missing-scryfall-cache since the test + environment has no real scryfall_cache/default_cards.json on disk.""" def test_write_refused_without_a_prior_matching_dry_run(self, db): from django.core.management import CommandError, call_command with pytest.raises(CommandError, match="FORCED DRY-RUN GUARD"): - call_command("local_calculate_verdicts", "--write") + call_command("local_calculate_verdicts", "--write", "--allow-missing-scryfall-cache") assert not PilotRunLedger.objects.filter(command="local_calculate_verdicts").exists() def test_write_succeeds_after_a_matching_dry_run(self, db): from django.core.management import call_command - call_command("local_calculate_verdicts") # dry-run (default) - call_command("local_calculate_verdicts", "--write") + call_command("local_calculate_verdicts", "--allow-missing-scryfall-cache") # dry-run (default) + call_command("local_calculate_verdicts", "--write", "--allow-missing-scryfall-cache") ledgers = list(PilotRunLedger.objects.filter(command="local_calculate_verdicts").order_by("started_at")) assert len(ledgers) == 2 @@ -1641,7 +1647,7 @@ def test_write_succeeds_after_a_matching_dry_run(self, db): def test_skip_dryrun_check_bypasses_the_guard_and_is_recorded(self, db, capsys): from django.core.management import call_command - call_command("local_calculate_verdicts", "--write", "--skip-dryrun-check") + call_command("local_calculate_verdicts", "--write", "--skip-dryrun-check", "--allow-missing-scryfall-cache") printed = capsys.readouterr().out assert "SKIP-DRYRUN-CHECK" in printed @@ -1668,8 +1674,62 @@ def raising_print(*args: Any, **kwargs: Any) -> None: # No exception escapes call_command - resilient_terminal_output swallows the simulated # BrokenPipeError from the terminal summary print. - call_command("local_calculate_verdicts", "--write", "--skip-dryrun-check") + call_command("local_calculate_verdicts", "--write", "--skip-dryrun-check", "--allow-missing-scryfall-cache") ledger = PilotRunLedger.objects.get(command="local_calculate_verdicts") assert ledger.status == PilotRunLedger.Status.COMPLETED assert ledger.finished_at is not None + + +class TestScryfallCacheGuard: + """Issue #402's fail-loud staleness guard: `ensure_scryfall_cache_present` (unit-level, pure + file-existence check - see TestGetBackFaceNames above for the sibling soft-fail lookup it's + deliberately NOT replacing) plus its wiring into `local_calculate_verdicts`'s own + Command.handle().""" + + def test_raises_when_the_cache_file_is_missing(self, tmp_path): + missing_path = tmp_path / "does_not_exist.json" + + with pytest.raises(CommandError, match="SCRYFALL CACHE MISSING"): + ensure_scryfall_cache_present(default_cards_path=missing_path) + + def test_does_not_raise_when_the_cache_file_is_present(self, tmp_path): + path = _write_bulk_data_file(tmp_path, []) + + ensure_scryfall_cache_present(default_cards_path=path) # no exception + + def test_command_refuses_to_start_when_the_cache_is_missing(self, db, monkeypatch, tmp_path): + from django.core.management import call_command + + import cardpicker.printing_metadata_import as printing_metadata_import_module + + monkeypatch.setattr(printing_metadata_import_module, "_cache_path", lambda: tmp_path / "does_not_exist.json") + + with pytest.raises(CommandError, match="SCRYFALL CACHE MISSING"): + call_command("local_calculate_verdicts") + assert not PilotRunLedger.objects.filter(command="local_calculate_verdicts").exists() + + def test_command_proceeds_when_the_flag_overrides_a_missing_cache(self, db, monkeypatch, tmp_path): + from django.core.management import call_command + + import cardpicker.printing_metadata_import as printing_metadata_import_module + + monkeypatch.setattr(printing_metadata_import_module, "_cache_path", lambda: tmp_path / "does_not_exist.json") + + call_command("local_calculate_verdicts", "--allow-missing-scryfall-cache") # no exception (dry-run) + + ledger = PilotRunLedger.objects.get(command="local_calculate_verdicts") + assert ledger.status == PilotRunLedger.Status.COMPLETED + + def test_command_proceeds_when_the_cache_is_actually_present(self, db, monkeypatch, tmp_path): + from django.core.management import call_command + + import cardpicker.printing_metadata_import as printing_metadata_import_module + + real_path = _write_bulk_data_file(tmp_path, []) + monkeypatch.setattr(printing_metadata_import_module, "_cache_path", lambda: real_path) + + call_command("local_calculate_verdicts") # no exception (dry-run), no override flag needed + + ledger = PilotRunLedger.objects.get(command="local_calculate_verdicts") + assert ledger.status == PilotRunLedger.Status.COMPLETED diff --git a/docker/docker-compose.prod.yml b/docker/docker-compose.prod.yml index 1da712c04..2f7ad4c54 100644 --- a/docker/docker-compose.prod.yml +++ b/docker/docker-compose.prod.yml @@ -47,6 +47,14 @@ services: - SESSION_COOKIE_SAMESITE=${SESSION_COOKIE_SAMESITE} - SESSION_COOKIE_SECURE=${SESSION_COOKIE_SECURE} - CORS_ALLOW_CREDENTIALS=${CORS_ALLOW_CREDENTIALS} + volumes: + # scryfall_cache/default_cards.json (~558MB) is the Scryfall bulk-data cache that + # printing_metadata_import.py/mtg.py's import_canonical_card_data read - a plain + # container-filesystem file with no persistent volume was silently destroyed by every + # image rebuild (issue #402; deploy-2 2026-07-23 lost it and local_calculate_verdicts + # ran degraded with an empty back-face lookup, no error). Named volume mirrors + # postgres_data/elasticsearch_data below so a rebuild (`up --build`) no longer touches it. + - scryfall_cache:/MPCAutofill/MPCAutofill/scryfall_cache worker: image: mpcautofill_worker container_name: mpcautofill_worker @@ -83,6 +91,12 @@ services: - SESSION_COOKIE_SAMESITE=${SESSION_COOKIE_SAMESITE} - SESSION_COOKIE_SECURE=${SESSION_COOKIE_SECURE} - CORS_ALLOW_CREDENTIALS=${CORS_ALLOW_CREDENTIALS} + volumes: + # Same persistent scryfall_cache as the django service above (shared, same named + # volume) - the weekly import_canonical_card_data/update_dfcs schedule runs on THIS + # container via manage.py qcluster (see docs/infrastructure.md's "Startup vs. + # scheduled catalog sync"), so it reads/refreshes the same on-disk bulk-data cache. + - scryfall_cache:/MPCAutofill/MPCAutofill/scryfall_cache # nginx serving the frontend nginx: @@ -124,3 +138,4 @@ services: volumes: postgres_data: elasticsearch_data: + scryfall_cache: diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 14db0adf1..80f2aeb71 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -1670,3 +1670,48 @@ Verify any future fix here the same way: `--shard=1/4` (not the file in isolation) against a cold `.next` cache, since shard composition (not just raw worker count) drives how much dev-server contention this spec actually sees. + +## `local_calculate_verdicts` silently runs with an empty back-face lookup after an image rebuild (Scryfall cache lost) + +**Symptom**: `manage.py local_calculate_verdicts --write` runs +cleanly, finishes `COMPLETED`, but back-face-only cards (e.g. a card +uploaded under a DFC's second-face name) that should resolve via +`_resolve_candidates_for_card`'s DFCPair fallback all come back +`printing_pk=None`/no vote. Nothing errors or warns loudly - the only +tell (if you go looking) is a per-card `logger.warning` from +`_load_back_face_names` ("Scryfall bulk-data file not found... back- +face lookup returning an empty set"), easy to miss in routine log +output. Observed live: deploy-2 (2026-07-23) rebuilt the django/worker +images, and the run that followed was silently degraded this way. + +**Cause**: `scryfall_cache/default_cards.json` (the Scryfall bulk-data +cache `printing_metadata_import.py`/`mtg.py`'s +`import_canonical_card_data` both read/refresh, ~558MB) lived as a +plain file inside the django/worker container filesystem with no +persistent volume mounted over it (`docker/docker-compose.prod.yml` had +no `volumes:` entry for either service). Every `up --build` image +rebuild throws away the old container filesystem, taking the cache with +it. `get_back_face_names`/`is_back_face` (see their own docstrings) +correctly treat a missing file as "no back faces known yet" - one +warning, `frozenset()` returned, never a raise - which is the right +behaviour for a per-card lookup, but left the whole `local_calculate_ verdicts` run silently degraded with no failure signal anywhere in its +own `COMPLETED` ledger row or terminal summary. + +**Fix** (issue #402, two parts): (1) `scryfall_cache` is now a named, +persistent Docker volume (`docker/docker-compose.prod.yml`, mirroring +`postgres_data`/`elasticsearch_data`'s own pattern) mounted on BOTH the +`django` service (manual `exec`-run commands) and the `worker` service +(the weekly `import_canonical_card_data`/`update_dfcs` schedule that +runs via its own `manage.py qcluster` process, per "Startup vs. +scheduled catalog sync" above) - a rebuild no longer touches it. +Compose changes only take effect at the NEXT deploy (`up --build -d`), +not retroactively on already-running containers. (2) +`printing_metadata_import.ensure_scryfall_cache_present()` is a new +fail-loud guard, called at the very start of `local_calculate_verdicts`'s +`Command.handle()` (before any card-by-card work) - it raises a +`CommandError` naming the missing path if the cache file doesn't exist, +distinct from `get_back_face_names`'s existing soft "empty set" path, +unless `--allow-missing-scryfall-cache` is passed explicitly. This +catches the failure mode structurally even if the volume mount is ever +missed again (e.g. a fresh box rebuild that skips the compose file, or +a manual `docker run` bypassing compose entirely).