Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand All @@ -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:
Expand All @@ -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
Expand All @@ -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"
Expand Down
40 changes: 40 additions & 0 deletions MPCAutofill/cardpicker/printing_metadata_import.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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())
Expand Down
72 changes: 66 additions & 6 deletions MPCAutofill/cardpicker/tests/test_local_calculate_verdicts.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -72,6 +73,7 @@
PrintingTagStatus,
VoteSource,
)
from cardpicker.printing_metadata_import import ensure_scryfall_cache_present
from cardpicker.tests.factories import (
CanonicalArtistFactory,
CanonicalCardFactory,
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
15 changes: 15 additions & 0 deletions docker/docker-compose.prod.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -124,3 +138,4 @@ services:
volumes:
postgres_data:
elasticsearch_data:
scryfall_cache:
45 changes: 45 additions & 0 deletions docs/troubleshooting.md
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Loading