feat: a paywalled creator is no longer indistinguishable from a silent one (milestone 387 step A2)
CI / extension-version (push) Successful in 5s
Build images / sign-extension (push) Successful in 5s
CI / lint (push) Successful in 6s
Build images / build-agent (push) Successful in 12s
CI / frontend-build (push) Successful in 36s
CI / backend-lint-and-test (push) Successful in 42s
Build images / build-web (push) Successful in 1m24s
Build images / smoke-web (push) Skipped
Build images / build-ml (push) Successful in 2m30s
Build images / promote (push) Skipped
CI / integration (push) Successful in 2m34s
CI / extension-version (push) Successful in 5s
Build images / sign-extension (push) Successful in 5s
CI / lint (push) Successful in 6s
Build images / build-agent (push) Successful in 12s
CI / frontend-build (push) Successful in 36s
CI / backend-lint-and-test (push) Successful in 42s
Build images / build-web (push) Successful in 1m24s
Build images / smoke-web (push) Skipped
Build images / build-ml (push) Successful in 2m30s
Build images / promote (push) Skipped
CI / integration (push) Successful in 2m34s
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LNXXULQDjVZmbuNa2G9mD9
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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) -------------------------------
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user