diff --git a/MPCAutofill/MPCAutofill/settings.py b/MPCAutofill/MPCAutofill/settings.py index 5611cae69..356ee09ef 100755 --- a/MPCAutofill/MPCAutofill/settings.py +++ b/MPCAutofill/MPCAutofill/settings.py @@ -647,7 +647,7 @@ # constant), same convention as STAGE_E_STREAMING_ENABLED above, so a self-hoster who doesn't run # sweeps at all - and therefore never wants the warm skipped over a ledger row it doesn't # understand - can turn the gate off with one env var and no code change or redeploy. -WARM_CATALOG_STATS_SWEEP_GATE_ENABLED = env.bool("WARM_CATALOG_STATS_SWEEP_GATE_ENABLED", default=True) +WARM_CATALOG_STATS_SWEEP_GATE_ENABLED = env.bool("WARM_CATALOG_STATS_SWEEP_GATE_ENABLED", default=False) # WARM_CATALOG_STATS_SWEEP_STALE_AFTER_HOURS - how old a RUNNING PilotRunLedger row has to be # before the gate stops trusting it and lets the warm run anyway (the guard against a crashed diff --git a/MPCAutofill/cardpicker/management/commands/warm_catalog_stats.py b/MPCAutofill/cardpicker/management/commands/warm_catalog_stats.py index 83624ba00..b38f4b06c 100644 --- a/MPCAutofill/cardpicker/management/commands/warm_catalog_stats.py +++ b/MPCAutofill/cardpicker/management/commands/warm_catalog_stats.py @@ -35,36 +35,29 @@ docstring). A cron/schedule run that silently writes nowhere and reports success is exactly the bug issue #538 exists to prevent, and this command must not repeat it a second way. -**SKIPS (exit 0, never an error) while a catalog sweep is in flight (owner ruling 2026-07-29).** -A sweep (`PilotRunLedger` row with `status=RUNNING` - see that model's own docstring: "the -durable, queryable record", created RUNNING at start, updated COMPLETED/FAILED at end) genuinely -contends for the same database this command's five aggregates query, so this command checks for -one BEFORE computing anything and, if found, skips the entire run rather than compute against a -catalog mid-mutation. This is the exact same "not configured" vs "broken" split -`warm_artist_external_links` already draws between an opt-in gate (quiet, exit-0 skip) and a -genuine fetch failure (`CommandError`, non-zero) - a skip here is equally NOT an error: it is this -command correctly declining to run, not a failure to run. The cache is left completely untouched -on skip, same rule this command already follows on a genuine failure - never a partial or zeroed -blob, stale-but-correct beats fresh-but-wrong here just as much as it does on the failure path. - -Guarded against a crashed sweep that never reaches COMPLETED/FAILED and would otherwise leave a -`RUNNING` row - and therefore this gate - stuck forever: a `RUNNING` row older than -`settings.WARM_CATALOG_STATS_SWEEP_STALE_AFTER_HOURS` (default comfortably above a full sweep's -measured ~7.0h floor - see that setting's own comment in `MPCAutofill/MPCAutofill/settings.py` for the exact -number and reasoning) is ignored and does not block this run. Both the gate itself -(`settings.WARM_CATALOG_STATS_SWEEP_GATE_ENABLED`) and the staleness bound are settings-driven, -tunable without a migration or a code change - see those two settings' own comments. - -The skip message names the specific blocking run (`run_id`, `started_at`, and how long it has -been running) precisely so a frozen stats page is diagnosable from the command's own log output in -one command, without a separate query against `PilotRunLedger`. - -**Consequence, stated plainly (owner ruling 2026-07-29): gating the WHOLE warm run this way means -the stats page can be up to ~7h stale during a full sweep**, not the roughly-hourly cadence the -schedule alone implies - see `docs/features/catalog-stats.md`'s "Sweep gate" section for the full -write-up. This is an accepted trade, not an oversight: a fast/slow split (skip only the panels -that actually read tables a sweep mutates, keep warming the rest) would avoid this staleness, and -is a known, deliberately deferred follow-up - not designed or built here. +**Sweep gate (owner ruling 2026-07-29, RETIRED as default 2026-08-03).** +The original ruling gated the warm run while a catalog sweep held a `PilotRunLedger` row with +`status=RUNNING` within the staleness bound, on the premise that a heavy batch sweep contended +for the same database. That premise no longer holds under the streaming micro-batch sweep design, +where a `RUNNING` row is perpetually present — the gate was freezing the stats page indefinitely. + +**Owner's ruling, 2026-08-03 (reverses the 2026-07-29 gate ruling): the gate is RETIRED as the +default.** The command now computes all five panels on every hourly run, sweep or no sweep, +because: (a) the aggregations are MVCC-safe — plain SELECTs never block on or are blocked by the +streaming sweep's tiny micro-batch INSERTs; (b) the stats pipeline already isolates sweep +artifacts — `runHistory` filters rows with `anonymous_id=SLOW_PATH_ANONYMOUS_ID, +skip_reason=SLOW_PATH_TO_REVIEW_SKIP_REASON` (see `catalog_stats.py` lines ~391-396), and the +vote panels count only human sources, so mid-sweep numbers are stable and correct; (c) measured +full compute of all five panels takes ~9s (last ungated run 2026-08-02T16:00:22Z→16:00:31Z), +trivial load once an hour. + +**The gate is now OPT-IN**, controlled by the settings flag +`WARM_CATALOG_STATS_SWEEP_GATE_ENABLED` (default `False`). When opted in (set to `True` via +env var), the exact 2026-07-29 skip behaviour is preserved: a RUNNING row within the staleness +bound skips the entire run (exit 0, cache untouched, same warning text as before), and a RUNNING +row older than `WARM_CATALOG_STATS_SWEEP_STALE_AFTER_HOURS` (default 12h) is ignored as a +crashed-sweep guard. Both the gate flag and the staleness bound remain settings-driven, tunable +without a migration or a code change. """ from datetime import timedelta @@ -96,10 +89,11 @@ def _find_blocking_sweep() -> Optional[PilotRunLedger]: class Command(BaseCommand): help = ( "Recomputes all five Proposal F catalog-stats panels and writes the catalog-stats cache " - "blob. Intended for an hourly django-q2 schedule. Skips cleanly (exit 0) while a catalog " - "sweep is in flight (PilotRunLedger status=RUNNING within the staleness bound), leaving " - "the cache untouched - see this command's own module docstring for the accepted " - "up-to-~7h staleness trade this creates. On any other failure, also leaves the existing " + "blob. Intended for an hourly django-q2 schedule. By default (sweep gate disabled) " + "computes all five panels on every run, sweep or no sweep - see this command's own module " + "docstring for the 2026-08-03 retirement rationale. When the sweep gate is opt-in enabled " + "(WARM_CATALOG_STATS_SWEEP_GATE_ENABLED=True), skips cleanly (exit 0) while a catalog " + "sweep is in flight, leaving the cache untouched. On any failure, also leaves the existing " "cache untouched and exits non-zero." ) @@ -121,6 +115,8 @@ def handle(self, *args: Any, **kwargs: Any) -> None: ) ) return + else: + self.stdout.write("Sweep gate: disabled (default) — computing all five panels.") try: blob = warm_catalog_stats_cache() diff --git a/MPCAutofill/cardpicker/tests/test_catalog_stats.py b/MPCAutofill/cardpicker/tests/test_catalog_stats.py index f679ae43e..d832513fc 100644 --- a/MPCAutofill/cardpicker/tests/test_catalog_stats.py +++ b/MPCAutofill/cardpicker/tests/test_catalog_stats.py @@ -725,9 +725,9 @@ def test_migration_reverses_and_reapplies_cleanly(self, db): class TestWarmCatalogStatsSweepGate: """ - Tests for `warm_catalog_stats`'s sweep gate (owner ruling 2026-07-29 - see that command's own - module docstring, and `docs/features/catalog-stats.md`'s "Sweep gate" section, for the full - skip/staleness mechanism and the accepted up-to-~7h staleness trade it creates). + Tests for `warm_catalog_stats`'s sweep gate - the 2026-07-29 skip/staleness mechanism, + now opt-in (default off, 2026-08-03 retirement). See that command's own module docstring + and `docs/features/catalog-stats.md`'s "Sweep gate" section. Every scenario here calls the real management command end-to-end (`call_command`, not the gate helper directly) so a regression in either the gate check itself OR its wiring into @@ -737,7 +737,8 @@ class TestWarmCatalogStatsSweepGate: def test_running_sweep_within_staleness_bound_skips_and_leaves_cache_untouched(self, db, capsys): """The core skip guarantee: exit 0 (no exception), a clear message naming the blocking run, and the cache left byte-for-byte as it was - not merely "the command returned", the - cache must still hold exactly the PRIOR blob, proving no recompute happened at all.""" + cache must still hold exactly the PRIOR blob, proving no recompute happened at all. + Requires the gate to be explicitly enabled since it is off by default (2026-08-03).""" call_command("warm_catalog_stats") good_cache = caches[SHARED_CACHE_ALIAS].get(CACHE_KEY) assert good_cache is not None @@ -748,7 +749,8 @@ def test_running_sweep_within_staleness_bound_skips_and_leaves_cache_untouched(s status=PilotRunLedger.Status.RUNNING, ) - call_command("warm_catalog_stats") # must not raise - a skip is exit 0, never an error + with override_settings(WARM_CATALOG_STATS_SWEEP_GATE_ENABLED=True): + call_command("warm_catalog_stats") # must not raise - a skip is exit 0, never an error output = capsys.readouterr().out assert "sweep-in-flight-abc123" in output diff --git a/MPCAutofill/cardpicker/tests/test_warm_catalog_stats.py b/MPCAutofill/cardpicker/tests/test_warm_catalog_stats.py new file mode 100644 index 000000000..fce2c89ce --- /dev/null +++ b/MPCAutofill/cardpicker/tests/test_warm_catalog_stats.py @@ -0,0 +1,169 @@ +""" +Tests for `warm_catalog_stats` management command sweep gate behaviour after the +2026-08-03 retirement (gate OFF by default, opt-in via settings flag). Covers the four +scenarios the retirement PR requires plus one default-behaviour guard. + +Mirrors the existing `TestWarmCatalogStatsSweepGate` class in `test_catalog_stats.py` +and the migration-test style of `test_warm_artist_external_links_schedule.py`. +""" + +import datetime as dt +from unittest.mock import patch + +import pytest + +from django.core.cache import caches +from django.core.management import CommandError, call_command +from django.test import override_settings +from django.utils import timezone + +from cardpicker.catalog_stats import CACHE_KEY, SHARED_CACHE_ALIAS +from cardpicker.models import PilotRunLedger + +_TEST_CACHES = { + "default": {"BACKEND": "django.core.cache.backends.locmem.LocMemCache"}, + "shared": { + "BACKEND": "django.core.cache.backends.locmem.LocMemCache", + "LOCATION": "test-warm-catalog-stats-shared", + }, +} + + +@pytest.fixture(autouse=True) +def _shared_cache_configured(): + with override_settings(CACHES=_TEST_CACHES): + caches["default"].clear() + caches[SHARED_CACHE_ALIAS].clear() + yield + caches["default"].clear() + caches[SHARED_CACHE_ALIAS].clear() + + +def _create_run( + run_id: str, + command: str = "local_identify_printing_tags", + status: str = PilotRunLedger.Status.COMPLETED, + started_at: dt.datetime = None, + finished_at: dt.datetime = None, + votes_written=None, +) -> PilotRunLedger: + run = PilotRunLedger.objects.create(run_id=run_id, command=command, status=status, votes_written=votes_written) + if finished_at is not None: + run.finished_at = finished_at + run.save(update_fields=["finished_at"]) + if started_at is not None: + PilotRunLedger.objects.filter(pk=run.pk).update(started_at=started_at) + run.refresh_from_db() + return run + + +@pytest.mark.django_db +class TestWarmCatalogStatsDefaultBehaviour: + """Default behaviour: gate OFF (2026-08-03 retirement).""" + + def test_running_sweep_does_not_block_when_gate_is_off_by_default(self, capsys): + """A RUNNING PilotRunLedger within the stale bound does NOT block — all + five panels computed, generatedAt bumped, cache written. This is the core + fix for the bug that froze the stats page indefinitely under the streaming + sweep's perpetually-RUNNING ledger row.""" + _create_run( + run_id="perpetual-streaming-sweep", + command="stage_e_streaming_dispatch", + status=PilotRunLedger.Status.RUNNING, + ) + + call_command("warm_catalog_stats") + output = capsys.readouterr().out + + assert "Sweep gate: disabled (default)" in output + assert "Catalog-stats cache warmed" in output + + cached = caches[SHARED_CACHE_ALIAS].get(CACHE_KEY) + assert cached is not None + assert cached["generatedAt"] is not None + + def test_no_sweep_running_computes_normally_by_default(self, capsys): + """Absent any RUNNING row at all, default behaviour is the standard full + compute — this is the same path as when a sweep is running, but worth + guarding explicitly so a future gate change cannot silently break the + no-sweep path.""" + call_command("warm_catalog_stats") + output = capsys.readouterr().out + + assert "Sweep gate: disabled (default)" in output + assert "Catalog-stats cache warmed" in output + + cached = caches[SHARED_CACHE_ALIAS].get(CACHE_KEY) + assert cached is not None + assert cached["generatedAt"] is not None + + +@pytest.mark.django_db +class TestWarmCatalogStatsGateOptIn: + """Opt-in gate behaviour (gate ON — the explicit override restores the + 2026-07-29 skip semantics for emergency conservatism).""" + + def test_running_sweep_within_bound_skips_when_gate_enabled(self, capsys): + """Opt-in: a RUNNING row within the staleness bound blocks the run, + cache untouched, exit 0 — exact 2026-07-29 behaviour preserved.""" + call_command("warm_catalog_stats") + good_cache = caches[SHARED_CACHE_ALIAS].get(CACHE_KEY) + assert good_cache is not None + + _create_run( + run_id="opt-in-blocking-sweep", + command="local_identify_printing_tags", + status=PilotRunLedger.Status.RUNNING, + ) + + with override_settings(WARM_CATALOG_STATS_SWEEP_GATE_ENABLED=True): + call_command("warm_catalog_stats") + output = capsys.readouterr().out + + assert "opt-in-blocking-sweep" in output + assert "skip" in output.lower() + assert caches[SHARED_CACHE_ALIAS].get(CACHE_KEY) == good_cache + + def test_stale_sweep_does_not_block_when_gate_enabled(self): + """Opt-in staleness guard preserved: a RUNNING row older than the + staleness bound is ignored and the warm computes normally. This is the + crashed-sweep guard that prevents a RUNNING row left behind by a crash + from freezing the page permanently.""" + from django.conf import settings + + stale_started_at = timezone.now() - dt.timedelta(hours=settings.WARM_CATALOG_STATS_SWEEP_STALE_AFTER_HOURS + 1) + _create_run( + run_id="crashed-sweep-nobody-finished", + command="local_identify_printing_tags", + status=PilotRunLedger.Status.RUNNING, + started_at=stale_started_at, + ) + + with override_settings(WARM_CATALOG_STATS_SWEEP_GATE_ENABLED=True): + call_command("warm_catalog_stats") + + cached = caches[SHARED_CACHE_ALIAS].get(CACHE_KEY) + assert cached is not None + assert cached["generatedAt"] is not None + + +@pytest.mark.django_db +class TestWarmCatalogStatsFailureSemantics: + """Failure semantics independent of gate state.""" + + def test_computation_failure_leaves_cache_untouched_and_raises_command_error(self): + """If compute_catalog_stats raises, the previous cache blob survives + byte-for-byte and the command exits with CommandError — same contract + regardless of gate state.""" + call_command("warm_catalog_stats") + good_cache = caches[SHARED_CACHE_ALIAS].get(CACHE_KEY) + assert good_cache is not None + + with patch( + "cardpicker.catalog_stats.compute_catalog_stats", + side_effect=RuntimeError("boom"), + ): + with pytest.raises(CommandError, match="left untouched"): + call_command("warm_catalog_stats") + + assert caches[SHARED_CACHE_ALIAS].get(CACHE_KEY) == good_cache diff --git a/docs/features/catalog-stats.md b/docs/features/catalog-stats.md index 0b2a34333..325568faf 100644 --- a/docs/features/catalog-stats.md +++ b/docs/features/catalog-stats.md @@ -397,49 +397,60 @@ live-skew `null` guard, both pure-function level - see "The gated human-progress series" above). Playwright: `frontend/tests/Stats.spec.ts` and `frontend/tests/ParticipationGraph.spec.ts`. -## Sweep gate (`warm_catalog_stats`, 2026-07-29 owner ruling) - -**New section - added standalone at the end of this file so it merges -cleanly alongside PR #566's own edits elsewhere in this document; not -interleaved into any paragraph above.** - -`warm_catalog_stats` now skips its entire run, cleanly (exit 0, cache left -completely untouched), while a catalog sweep is in flight - defined as any -`PilotRunLedger` row with `status=RUNNING` (see that model's own -docstring: created `RUNNING` at start, updated `COMPLETED`/`FAILED` at -end). A sweep genuinely contends for the same database this command's -five aggregates query, so the warm command checks for one BEFORE -computing anything, the same way it already leaves the cache untouched on -any other failure. - -**Guard against a crashed sweep**: a `RUNNING` row does not block forever - -- one older than `settings.WARM_CATALOG_STATS_SWEEP_STALE_AFTER_HOURS` - (default comfortably above a full sweep's measured ~7.0h floor; see that - setting's own comment in `MPCAutofill/MPCAutofill/settings.py` for the exact number - and its reasoning) is ignored, so a crashed sweep that never updated its - ledger row to `COMPLETED`/`FAILED` cannot permanently freeze the stats - page. The skip log line names the specific blocking run (`run_id`, - `started_at`, and how long it has been running) so a frozen page is - diagnosable from the command's own output in one command. - -**Both the gate and the staleness bound are settings-driven** -(`WARM_CATALOG_STATS_SWEEP_GATE_ENABLED`, -`WARM_CATALOG_STATS_SWEEP_STALE_AFTER_HOURS` - see +## Sweep gate (`warm_catalog_stats`, retired as default 2026-08-03) + +**2026-07-29 ruling → retired 2026-08-03. The gate is now OFF by default; the +command computes all five panels on every hourly run, sweep or no sweep.** + +### Why it was retired + +The original ruling (2026-07-29) gated the warm run while any +`PilotRunLedger` row with `status=RUNNING` was present within the +staleness bound, on the premise that a heavy batch sweep contended for +the same database. That premise no longer holds under the streaming +micro-batch sweep design (Stage E), where a `RUNNING` row is perpetually +present — the gate was freezing the stats page indefinitely. + +**The owner's 2026-08-03 ruling reverses the gate with three-part +rationale, verified against the live system:** + +1. **MVCC safety.** The five aggregations are plain `SELECT` queries — + they never block on or are blocked by the streaming sweep's tiny + micro-batch `INSERT`s (25 cards, max 3 concurrent). + +2. **Sweep artifacts already filtered.** The `runHistory` panel filters + rows with `anonymous_id=SLOW_PATH_ANONYMOUS_ID, skip_reason=SLOW_PATH_TO_REVIEW_SKIP_REASON` + (`catalog_stats.py` lines ~391–396), and the vote panels + (`participation`, `contributionsOverTime`) count only human sources + (`HUMAN_SOURCES = USER/ADMIN/FEDERATED`), so mid-sweep numbers are + stable and correct regardless of whether a sweep is in flight. + +3. **~9s measured compute.** Full compute of all five panels was measured + at ~9s (last ungated run 2026-08-02T16:00:22Z → 16:00:31Z), trivial + load once an hour. + +The staleness bound (`WARM_CATALOG_STATS_SWEEP_STALE_AFTER_HOURS`, +default 12h) and the `_find_blocking_sweep` helper are preserved unchanged +as a crashed-sweep guard in the opt-in path below. + +### How the gate works now (opt-in) + +- **Default (gate off):** The command computes all five panels on every + hourly run. `_find_blocking_sweep` is never consulted. The stdout log + prints `"Sweep gate: disabled (default) — computing all five panels."` + so operators can see which mode ran. + +- **Opt-in (gate on):** Set `WARM_CATALOG_STATS_SWEEP_GATE_ENABLED=true` + in the environment. This restores the exact 2026-07-29 behaviour: a + `RUNNING` row within the staleness bound skips the entire run (exit 0, + cache untouched, same warning text naming the blocking run by its + `run_id`/`started_at`/running-time). A `RUNNING` row older than the + staleness bound is ignored (crashed-sweep guard). + +Both settings remain settings-driven (`WARM_CATALOG_STATS_SWEEP_GATE_ ENABLED`, `WARM_CATALOG_STATS_SWEEP_STALE_AFTER_HOURS` in `MPCAutofill/MPCAutofill/settings.py`), tunable without a migration or a code change. -**Consequence, stated plainly (accepted trade, not an oversight)**: -because the gate skips the ENTIRE warm run rather than any individual -panel, the stats page can go up to ~7h stale during a full sweep - -noticeably worse than the hourly schedule's cadence alone implies. A -fast/slow split (skip only the panels that read tables a sweep actually -mutates, keep warming the rest on schedule) would avoid this staleness -and is a known follow-up - deliberately deferred, not designed here. This -document intentionally does not restate that design; see the command's -own module docstring for the up-to-date statement of the trade if this -section and the code ever drift. - **Index**: `CardScanLog(anonymous_id, skip_reason)` (migration `0096`) - added because `compute_skip_breakdown`'s per-engine panel (and any future query shaped the same way) filters/groups `CardScanLog` on those two