From 7751715b83915f1abf1bc214051c598fd7fe0a0f Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 9 Sep 2026 15:40:19 -0400 Subject: [PATCH] feat: a paywalled creator is no longer indistinguishable from a silent one (milestone 387 step A2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A2 of milestone 387. A1 made the gated-post count true; this makes it mean something. A native walk that reached the bottom returned `error_type=None` whether the creator had posted nothing or every post sat behind a tier we don't hold. `source.error_type` stayed NULL and the source read healthy and quiet. gallery-dl has classified this as TIER_LIMITED since the paywall-as-"needs attention" complaint; the native path never did. Two things had to move that the plan didn't foresee, both found by reading the consumers rather than by testing afterwards: The backfill lifecycle's completion test required `error_type is None`. Returning TIER_LIMITED naively would have dropped a fully-paywalled backfill into the not-finished branch — zero downloads means no progress, two strikes marks it "stalled" — so the creator we can see least would become the one we re-walk most. `walk_completed` now admits informational classes. `_update_source_health` only stamps `error_type` on status "error" and CLEARS it on "ok". Since TIER_LIMITED is a success, the chip was wiped by the very run that produced it — which is why FailingSourcesCard's `tier_limited` palette entry has never been reachable. An "ok" run now keeps an informational class while failures stay 0 and last_error stays clear: the run did not fail and must not earn a backoff. Deviation from the plan, deliberate: the filed step said classify only when `downloaded == 0`. gallery-dl doesn't condition on that, and diverging the two backends over the same concept is what rule 169 forbids — so the native path mirrors it. "There is content here you aren't paying for" is equally true in a week we also got the cheap posts. Pinned by a test, since the stricter rule looks more correct. The predicate, the wording and the completion test are defined once in gallery_dl.py and spread into both backends (snippet 3087), rather than re-derived per half — which is exactly how they drifted apart before. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01LNXXULQDjVZmbuNa2G9mD9 --- backend/app/services/download_service.py | 27 ++++-- backend/app/services/gallery_dl.py | 78 +++++++++++++++-- backend/app/services/ingest_core.py | 42 ++++++--- tests/test_download_service.py | 103 ++++++++++++++++++++++- tests/test_patreon_ingester.py | 55 ++++++++++++ 5 files changed, 281 insertions(+), 24 deletions(-) diff --git a/backend/app/services/download_service.py b/backend/app/services/download_service.py index 08ca44e..67a5690 100644 --- a/backend/app/services/download_service.py +++ b/backend/app/services/download_service.py @@ -34,7 +34,9 @@ from .gallery_dl import ( GalleryDLService, SourceConfig, extract_errors_warnings, + is_informational, truncate_log, + walk_completed, ) from .importer import Importer from .platforms import auth_type_for @@ -552,11 +554,13 @@ class DownloadService: # page is no longer double-counted. new_overrides (read fresh above) # carries the ingester's committed value forward untouched. - completed = ( - dl_result.success - and dl_result.error_type is None - and dl_result.return_code == 0 - ) + # Shared with the result path so the two halves can't disagree about + # what "finished" means. Note it admits an INFORMATIONAL error_type: a + # fully-paywalled creator's backfill really did reach the bottom, and + # treating it as unfinished would re-walk that wall every chunk until + # the stall counter tripped — the creator we can see least becoming the + # one we fetch most. + completed = walk_completed(dl_result) if completed: new_overrides["_backfill_state"] = "complete" new_overrides.pop("_backfill_cursor", None) @@ -625,8 +629,17 @@ class DownloadService: if status == "ok": source.consecutive_failures = 0 source.last_error = None - # alembic 0032 — clear the failure-class chip on success. - source.error_type = None + # alembic 0032 — clear the failure-class chip on success, EXCEPT an + # informational class. tier_limited rides an otherwise-successful + # run: failures stay 0 and last_error stays clear (the run did not + # fail and must not earn a backoff), but "there is content here we + # aren't allowed to see" is a durable fact about the SOURCE, not + # about this run. Clearing it here is what left FailingSourcesCard's + # `tier_limited` palette entry unreachable — the chip was wiped by + # the very success that produced it. + source.error_type = ( + error_type if is_informational(error_type) else None + ) elif status == "error": source.consecutive_failures = (source.consecutive_failures or 0) + 1 source.last_error = error_message diff --git a/backend/app/services/gallery_dl.py b/backend/app/services/gallery_dl.py index 78abd9b..ddaf9f7 100644 --- a/backend/app/services/gallery_dl.py +++ b/backend/app/services/gallery_dl.py @@ -261,6 +261,72 @@ def make_run_stats( } +# --- tier-gated classification, shared by BOTH backends --------------------- +# +# These three live together because the native ingester and the gallery-dl +# subprocess must reach the same verdict from the same number. They did not: +# gallery-dl classified TIER_LIMITED while ingest_core counted gated posts and +# threw the count away, so the platforms FC owns reported a paywalled creator as +# a silent one (#874 follow-up). One predicate, spread into both, rather than +# the condition re-derived per backend. + + +def classify_tier_gated(tier_gated_count: int) -> ErrorType | None: + """TIER_LIMITED when a walk saw tier-gated posts and nothing else failed. + + Deliberately NOT conditioned on `downloaded == 0`. A creator whose top-tier + posts we cannot see is tier-limited even in a week we did get their cheaper + ones — the fact the operator needs ("there is content here you are not + paying for") is true either way. gallery-dl has classified it this way since + the paywall-as-"needs attention" complaint (see `_categorize_error`), and the + native path now matches rather than inventing a stricter rule. + + Callers must apply this only AFTER the real error categories (auth, rate + limit, drift, …) have had their turn; tier-gating is the weakest signal and + must never mask a genuine failure. + """ + return ErrorType.TIER_LIMITED if tier_gated_count else None + + +def tier_gated_message(count: int) -> str: + """The one wording for the tier-gated verdict, so the two backends can't + describe the same state differently in the Logs UI.""" + return ( + f"Subscription tier does not grant access to " + f"{count} post{'s' if count != 1 else ''}" + ) + + +# `Source.error_type` doubles as the failure-class chip, and a status of "ok" +# CLEARS it (alembic 0032). TIER_LIMITED breaks that assumption: it rides an +# otherwise-successful run, so without an exemption the chip is wiped the moment +# it is set and `FailingSourcesCard`'s `tier_limited` palette entry can never +# render. Informational classes are the exemption — they describe the source, +# not a failure of the run. +INFORMATIONAL_ERROR_TYPES = frozenset({ErrorType.TIER_LIMITED.value}) + + +def is_informational(error_type) -> bool: + """True for a class that reports a state rather than a failure. Accepts an + ErrorType or the plain string persisted on Source.error_type.""" + return error_type is not None and str(error_type) in INFORMATIONAL_ERROR_TYPES + + +def walk_completed(result: DownloadResult) -> bool: + """Did this walk reach the bottom cleanly? + + The backfill lifecycle's completion test. An informational error_type still + counts as complete: a fully-paywalled creator's backfill DID finish, and + treating it as unfinished re-walks the same wall until the stall counter + trips — the creator we can see least becoming the one we fetch most. + """ + return ( + result.success + and result.return_code == 0 + and (result.error_type is None or is_informational(result.error_type)) + ) + + class GalleryDLService: """Service for executing gallery-dl downloads.""" @@ -531,12 +597,12 @@ class GalleryDLService: line for line in combined.split("\n") if "][warning]" in line and "not allowed to view post" in line ] - if tier_gated_lines: - count = len(tier_gated_lines) - return ( - ErrorType.TIER_LIMITED, - f"Subscription tier does not grant access to {count} post{'s' if count != 1 else ''}", - ) + # Same predicate + wording the native path uses, so the two backends + # can't drift on what counts as tier-gated or how it reads. + count = len(tier_gated_lines) + gated = classify_tier_gated(count) + if gated is not None: + return (gated, tier_gated_message(count)) # Partial-success: the subprocess exited non-zero (typically because # the wall-clock timeout fired mid-walk), but it had downloaded ≥1 diff --git a/backend/app/services/ingest_core.py b/backend/app/services/ingest_core.py index 2da2b21..ead2f1e 100644 --- a/backend/app/services/ingest_core.py +++ b/backend/app/services/ingest_core.py @@ -35,7 +35,13 @@ from collections.abc import Callable from sqlalchemy import delete, func, select, text from sqlalchemy.dialects.postgresql import insert as pg_insert -from .gallery_dl import DownloadResult, ErrorType, make_run_stats +from .gallery_dl import ( + DownloadResult, + ErrorType, + classify_tier_gated, + make_run_stats, + tier_gated_message, +) from .native_ingest_common import NativeAuthError, NativeDriftError log = logging.getLogger(__name__) @@ -574,17 +580,33 @@ class Ingester: error_type=ErrorType.API_DRIFT, error_message=msg, ) - # Normal success: reached the bottom, or a tick that early-outed. rc 0 + - # error_type None is REQUIRED for a backfill/recovery walk that reached - # the bottom to be marked COMPLETE by - # download_service._apply_backfill_lifecycle — so we return None even - # when downloaded == 0 (a re-confirming walk that found nothing new still - # completed). success=True maps to status "ok" regardless. A tick that - # early-outed also returns here; ticks never set backfill state so the - # lifecycle is a no-op for them. + # Normal success: reached the bottom, or a tick that early-outed. A + # zero-download walk still returns success here — a re-confirming walk + # that found nothing new genuinely completed. A tick that early-outed + # also lands here; ticks never set backfill state so the lifecycle is a + # no-op for them. + # + # success=True and return_code=0 are load-bearing, not cosmetic. They + # are what make this a COMPLETE walk for + # download_service._apply_backfill_lifecycle (via walk_completed) and + # what map it to status "ok", so a walk that fetched nothing doesn't + # accrue consecutive_failures or a backoff it hasn't earned. + # + # #874 follow-up: "nothing new" and "everything sat behind a tier you + # don't hold" are different facts, and returning None for both made a + # paywalled creator indistinguishable from a silent one. TIER_LIMITED is + # classified LAST — every real failure has already returned above — + # because tier-gating is the weakest signal and must never mask a + # genuine error. It is informational, so walk_completed still counts + # this walk as finished (see that predicate for why re-walking a + # paywalled creator forever is the bug being avoided). + gated_error = classify_tier_gated(gated_skipped) return _result( success=True, return_code=0, - error_type=None, error_message=None, + error_type=gated_error, + error_message=( + tier_gated_message(gated_skipped) if gated_error else None + ), ) # -- failure mapping (adapter overrides) ------------------------------- diff --git a/tests/test_download_service.py b/tests/test_download_service.py index 2866531..7533dcb 100644 --- a/tests/test_download_service.py +++ b/tests/test_download_service.py @@ -370,7 +370,9 @@ async def test_run_download_native_unresolvable_fails_loud( # --- FC-3d: finalize hook updates Source health columns ------------------- -async def _seed_source_with_health(db, *, failures=0, last_error=None, suffix="fz"): +async def _seed_source_with_health( + db, *, failures=0, last_error=None, suffix="fz", error_type=None, +): artist = Artist(name=f"alice-{suffix}", slug=f"alice-{suffix}") db.add(artist) await db.flush() @@ -378,6 +380,7 @@ async def _seed_source_with_health(db, *, failures=0, last_error=None, suffix="f artist_id=artist.id, platform="patreon", url=f"https://patreon.com/alice-{suffix}", enabled=True, consecutive_failures=failures, last_error=last_error, + error_type=error_type, ) db.add(source) await db.flush() @@ -588,6 +591,104 @@ async def test_backfill_state_running_selects_backfill_mode_and_resumes( assert co.get("_backfill_cursor") == "03:RESUME2:next" +@pytest.mark.asyncio +async def test_backfill_completes_when_every_post_was_tier_gated( + db, db_sync, tmp_path, seed_artist_and_source, +): + """A fully-paywalled backfill chunk still COMPLETES. + + The completion test used to require `error_type is None`, so classifying a + gated walk as TIER_LIMITED would have dropped it into the not-finished + branch: zero files downloaded means `advanced` is False, two strikes marks + it 'stalled', and the creator we can see least becomes the one we re-walk + most. `walk_completed` admits informational classes for exactly this. + """ + from backend.app.services.gallery_dl import ErrorType + + _artist, source = seed_artist_and_source + source.config_overrides = { + "_backfill_state": "running", "_backfill_cursor": "03:NEAR:bottom", + } + source.backfill_runs_remaining = 5 + await db.commit() + + svc, _ = _backfill_svc(db, db_sync, tmp_path, _make_fake_dl_result( + success=True, written_paths=[], files_downloaded=0, + error_type=ErrorType.TIER_LIMITED, + error_message="Subscription tier does not grant access to 9 posts", + )) + await svc.download_source(source.id) + + co = (await db.execute( + select(Source.config_overrides).where(Source.id == source.id) + )).scalar_one() + assert co.get("_backfill_state") == "complete" + assert "_backfill_cursor" not in co + + +@pytest.mark.asyncio +async def test_ok_run_keeps_the_tier_limited_chip_but_not_the_failure_state( + db, +): + """An informational class survives a successful run; a failure class doesn't. + + `status == "ok"` clears `error_type` (alembic 0032) because it is the + failure-class chip. tier_limited rides an OK run, so that clear is what made + FailingSourcesCard's `tier_limited` palette entry unreachable — the chip was + wiped by the same success that produced it. It must persist while the + failure bookkeeping stays clean: no accrued failures, no last_error. + """ + from backend.app.services.download_service import DownloadService + + source_id, event_id = await _seed_source_with_health( + db, failures=3, last_error="prev", suffix="gated", + ) + svc = DownloadService( + async_session=db, sync_session=None, + gdl=None, importer=None, cred_service=None, + ) + await svc._update_source_health( + source_id=source_id, status="ok", error_message=None, + error_type="tier_limited", + ) + + row = (await db.execute( + select( + Source.error_type, Source.consecutive_failures, Source.last_error, + ).where(Source.id == source_id) + )).one() + assert row.error_type == "tier_limited" + assert row.consecutive_failures == 0 + assert row.last_error is None + + +@pytest.mark.asyncio +async def test_ok_run_still_clears_a_real_failure_class(db): + """The exemption is narrow: a genuine failure class is still cleared on OK, + so a recovered source stops showing a stale chip.""" + from backend.app.services.download_service import DownloadService + + source_id, _event_id = await _seed_source_with_health( + db, failures=2, last_error="boom", suffix="recovered", + error_type="auth_error", + ) + svc = DownloadService( + async_session=db, sync_session=None, + gdl=None, importer=None, cred_service=None, + ) + await svc._update_source_health( + source_id=source_id, status="ok", error_message=None, + error_type=None, + ) + + row = (await db.execute( + select(Source.error_type, Source.consecutive_failures) + .where(Source.id == source_id) + )).one() + assert row.error_type is None + assert row.consecutive_failures == 0 + + @pytest.mark.asyncio async def test_backfill_clean_exit_marks_complete( db, db_sync, tmp_path, seed_artist_and_source, diff --git a/tests/test_patreon_ingester.py b/tests/test_patreon_ingester.py index 7ff5d68..59c05b7 100644 --- a/tests/test_patreon_ingester.py +++ b/tests/test_patreon_ingester.py @@ -711,6 +711,61 @@ async def test_gated_posts_are_reported_in_run_stats( assert result.run_stats["downloaded_count"] == 1 +@pytest.mark.asyncio +async def test_fully_gated_walk_classifies_tier_limited_but_still_succeeds( + source_id, sync_engine, tmp_path, +): + """A walk where everything was paywalled is a SUCCESS with a reason. + + Before this, it returned `error_type=None` — identical to a creator who + simply hadn't posted. success/return_code are asserted deliberately: they + are what keep the walk "completed" for the backfill lifecycle and status + "ok" for source health, so a source we can't see never accrues + consecutive_failures or a backoff it hasn't earned. + """ + gated = [(f"g{i}", [_media(f"g{i}", 1)]) for i in range(2)] + client = _FakeClient([(None, gated)], gated={"g0", "g1"}) + ing = _ingester(sync_engine, tmp_path, client, _FakeDownloader(tmp_path)) + + result = ing.run( + source_id=source_id, campaign_id="c1", artist_slug="ingest", + url="https://patreon.com/ingest", mode="backfill", + ) + + assert result.error_type == ErrorType.TIER_LIMITED + assert result.success is True + assert result.return_code == 0 + assert "2 posts" in result.error_message + + +@pytest.mark.asyncio +async def test_tier_limited_fires_even_when_some_media_downloaded( + source_id, sync_engine, tmp_path, +): + """Gated posts classify TIER_LIMITED even on a walk that DID download. + + Mirrors gallery-dl's long-standing rule (`_categorize_error`), which does + not condition on a zero download count either. The fact worth surfacing is + "there is content here you aren't paying for", and that is equally true in a + week we also got the cheaper posts. Pinned because the obvious-looking + stricter rule (`downloaded == 0`) would silently diverge the two backends. + """ + client = _FakeClient( + [(None, [("g0", [_media("g0", 1)]), ("open", [_media("open", 1)])])], + gated={"g0"}, + ) + ing = _ingester(sync_engine, tmp_path, client, _FakeDownloader(tmp_path)) + + result = ing.run( + source_id=source_id, campaign_id="c1", artist_slug="ingest", + url="https://patreon.com/ingest", mode="backfill", + ) + + assert result.files_downloaded == 1 + assert result.error_type == ErrorType.TIER_LIMITED + assert result.success is True + + @pytest.mark.asyncio async def test_run_stats_reports_zero_gated_when_nothing_is_gated( source_id, sync_engine, tmp_path,