diff --git a/alembic/versions/0028_collapse_sidecar_synthetics_into_real_sources.py b/alembic/versions/0028_collapse_sidecar_synthetics_into_real_sources.py new file mode 100644 index 0000000..5ec9267 --- /dev/null +++ b/alembic/versions/0028_collapse_sidecar_synthetics_into_real_sources.py @@ -0,0 +1,190 @@ +"""collapse-sidecar-synthetic: repoint Posts/ImageProvenance/DownloadEvents +from `sidecar::` synthetic Source anchors onto the real +Source for the same (artist, platform) when one exists, then delete the +synthetic. + +Revision ID: 0028 +Revises: 0027 +Create Date: 2026-05-31 + +Background: alembic 0022 (2026-05-26) consolidated the old per-post-URL +Source rows into one canonical Source per (artist, platform). When NO +real campaign URL was salvageable among the candidates, it rewrote the +canonical row to url='sidecar::' enabled=false as a +disabled anchor for any Posts already attached. + +That was fine while it was the only Source for that artist+platform. +But: the unique constraint on Source is (artist_id, platform, url), not +(artist_id, platform). When the operator later added the real +subscription via the UI / extension / etc., a SECOND row landed — +the real one — with id > the synthetic. Both coexisted. + +Two follow-on problems surfaced 2026-05-31: + + 1. The Subscriptions UI listed both rows. The synthetic was disabled + so the scheduler never polled it, but it looked like a phantom + subscription. (Fixed in same commit by SourceService.list filter.) + 2. importer._source_for_sidecar picked Source by `ORDER BY id ASC + LIMIT 1`, so EVERY gallery-dl download since the real Source was + added attached its Post to the SYNTHETIC anchor, not the real + Source. (Fixed in same commit by preferring non-sidecar URLs.) + +This migration is the data half of the cleanup: for every (artist, +platform) with both a synthetic AND a real Source, repoint the +synthetic's children (Posts, ImageProvenance, DownloadEvents) onto the +real Source and delete the synthetic. Reuses the same epid/provenance +collision dance from alembic 0022 because the same uniqueness +constraints fire row-by-row during bulk UPDATEs. + +Lone synthetic anchors — those where no real Source for the same +(artist, platform) exists (e.g., filesystem-imported artist with no +subscription added) — are LEFT INTACT. They anchor real imported +content; deleting them would CASCADE-delete the Posts the operator +imported. The SourceService.list filter hides them from the UI; the +operator can delete them by hand if they want the underlying imports +gone. +""" +from typing import Sequence, Union + +from alembic import op +from sqlalchemy import text + +revision: str = "0028" +down_revision: Union[str, None] = "0027" +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + conn = op.get_bind() + + # Find (artist_id, platform) groups where BOTH a sidecar synthetic + # and at least one real Source exist. + groups = conn.execute(text(""" + SELECT artist_id, platform + FROM source + GROUP BY artist_id, platform + HAVING bool_or(url LIKE 'sidecar:%') + AND bool_or(url NOT LIKE 'sidecar:%') + """)).fetchall() + + for artist_id, platform in groups: + rows = conn.execute( + text(""" + SELECT id, url FROM source + WHERE artist_id = :a AND platform = :p + ORDER BY id ASC + """), + {"a": artist_id, "p": platform}, + ).fetchall() + + synthetic_ids = [sid for sid, url in rows if url.startswith("sidecar:")] + real_rows = [(sid, url) for sid, url in rows if not url.startswith("sidecar:")] + if not synthetic_ids or not real_rows: + continue # belt+suspenders; the GROUP BY already filtered + + # Canonical real: lowest-id non-sidecar Source. + canonical_id = real_rows[0][0] + + # STEP A: PRE-merge Post collisions on (canonical, external_post_id). + # Mirror alembic 0022's pre-merge logic — when synth has Post X + # epid=N and real has Post Y epid=N, the bulk UPDATE below would + # trip uq_post_source_external_id row-by-row. Group all Posts + # under (canonical + synthetics) by epid; for any group >1, + # pick a keep (prefer one already under canonical, else lowest + # id) and merge the rest into it. + all_posts = conn.execute( + text(""" + SELECT external_post_id, id, source_id + FROM post + WHERE source_id = :canonical OR source_id = ANY(:synths) + ORDER BY external_post_id, id + """), + {"canonical": canonical_id, "synths": synthetic_ids}, + ).fetchall() + by_epid: dict = {} + for epid, post_id, src_id in all_posts: + by_epid.setdefault(epid, []).append((post_id, src_id)) + for _epid, posts in by_epid.items(): + if len(posts) <= 1: + continue + canonical_side = [p for p in posts if p[1] == canonical_id] + keep_id = canonical_side[0][0] if canonical_side else posts[0][0] + drop_ids = [p[0] for p in posts if p[0] != keep_id] + for drop_id in drop_ids: + # Pre-delete image_provenance rows under drop_ whose + # image_record_id already has provenance under keep — + # avoids tripping uq_image_provenance_image_post (0021) + # row-by-row during the repoint UPDATE. + conn.execute( + text(""" + DELETE FROM image_provenance + WHERE post_id = :drop_ + AND image_record_id IN ( + SELECT image_record_id FROM image_provenance + WHERE post_id = :keep + ) + """), + {"keep": keep_id, "drop_": drop_id}, + ) + conn.execute( + text(""" + UPDATE image_provenance SET post_id = :keep + WHERE post_id = :drop_ + """), + {"keep": keep_id, "drop_": drop_id}, + ) + conn.execute( + text(""" + UPDATE image_record SET primary_post_id = :keep + WHERE primary_post_id = :drop_ + """), + {"keep": keep_id, "drop_": drop_id}, + ) + conn.execute( + text("DELETE FROM post WHERE id = :drop_"), + {"drop_": drop_id}, + ) + + # STEP B: Bulk reparent the remaining Posts off the synthetics. + conn.execute( + text(""" + UPDATE post SET source_id = :canonical + WHERE source_id = ANY(:synths) + """), + {"canonical": canonical_id, "synths": synthetic_ids}, + ) + + # STEP C: Reparent ImageProvenance.source_id (denormalized FK; + # no UNIQUE on source_id, safe bulk). + conn.execute( + text(""" + UPDATE image_provenance SET source_id = :canonical + WHERE source_id = ANY(:synths) + """), + {"canonical": canonical_id, "synths": synthetic_ids}, + ) + + # STEP D: Reparent any DownloadEvent.source_id. Synthetics are + # enabled=false so the scheduler never created events for them; + # this is belt+suspenders for any rows planted by manual force + # or older code paths. + conn.execute( + text(""" + UPDATE download_event SET source_id = :canonical + WHERE source_id = ANY(:synths) + """), + {"canonical": canonical_id, "synths": synthetic_ids}, + ) + + # STEP E: Drop the now-empty synthetics. + conn.execute( + text("DELETE FROM source WHERE id = ANY(:synths)"), + {"synths": synthetic_ids}, + ) + + +def downgrade() -> None: + # Lossy migration — synthetic Sources deleted, Posts repointed and + # potentially merged. No safe downgrade. + pass diff --git a/backend/app/services/gallery_dl.py b/backend/app/services/gallery_dl.py index c356908..3e19a69 100644 --- a/backend/app/services/gallery_dl.py +++ b/backend/app/services/gallery_dl.py @@ -42,13 +42,16 @@ class ErrorType(StrEnum): UNKNOWN_ERROR = "unknown_error" -# Pinned to download_source's Celery soft_time_limit (900s, see -# tasks/download.py:32). Anything larger and Celery kills the task before -# subprocess.run can raise TimeoutExpired — leaving the DownloadEvent -# stranded for the recovery sweep instead of capturing a clean timeout -# error. Per-source bumps live in source.config_overrides for legitimately -# long syncs. Operator-confirmed 2026-05-30 (~40-min hang investigation). -_DEFAULT_GDL_TIMEOUT_SECONDS = 900 +# 30 seconds shy of download_source's Celery soft_time_limit (900s, see +# tasks/download.py:32). subprocess.run MUST raise TimeoutExpired before +# Celery raises SoftTimeLimitExceeded — otherwise Celery wins the race, +# SIGKILLs the worker, in-memory stdout/stderr is lost, and the +# DownloadEvent ends up empty-logged with "stranded by recovery sweep" +# 18 minutes later (operator-flagged 2026-05-31, Knuxy event #38275). +# The 30s buffer absorbs scheduler jitter / GC pauses without making +# legitimately-long-running syncs timeout-friendlier. Per-source bumps +# still live in source.config_overrides for legitimately long syncs. +_DEFAULT_GDL_TIMEOUT_SECONDS = 870 @dataclass @@ -369,7 +372,17 @@ class GalleryDLService: if return_code in (1, 4) and (skip_line_count > 0 or has_skip_text) and not has_actual_error: return ErrorType.NO_NEW_CONTENT, "No new content to download" - if return_code in (1, 4) and not has_actual_error: + # Tier-gated classification used to require `return_code in (1, 4)`, + # which silently fell through to UNKNOWN_ERROR when gallery-dl + # returned a different exit code for mixed-failure runs (e.g. + # paywall warnings + a missing yt-dlp dep flipping the exit bits). + # The artist then surfaced as "needs attention" purely because a + # paywall blocked posts the operator wasn't paying to see — + # operator-flagged 2026-05-31. Now: if no source-level error + # category fired AND tier-gated warnings are present, classify + # as TIER_LIMITED regardless of return code. Same priority order + # as before (auth/rate/access/not_found/network/http still win). + if not has_actual_error: tier_gated_lines = [ line for line in combined.split("\n") if "][warning]" in line and "not allowed to view post" in line diff --git a/backend/app/services/importer.py b/backend/app/services/importer.py index b722f67..c66caa8 100644 --- a/backend/app/services/importer.py +++ b/backend/app/services/importer.py @@ -261,24 +261,44 @@ class Importer: def _source_for_sidecar( self, *, artist_id: int, platform: str, artist_slug: str, ) -> Source: - """Filesystem-import sidecar Source resolver. + """Sidecar-import Source resolver. Used by both filesystem imports + and gallery-dl downloads (both write sidecar JSON, both flow through + _apply_sidecar / _capture_attachment). Source represents a subscription feed (one per artist+platform — the - gallery-dl URL polled by the FC-3 downloader). The filesystem importer - used to call _find_or_create_source(url=sd.post_url), which created - one Source row per post URL — 100s of junk Sources per artist, all - with enabled=True, polluting the artist detail page and tricking the + URL polled by the FC-3 downloader). The filesystem importer used to + call _find_or_create_source(url=sd.post_url), creating one Source + row per post URL — 100s of junk Sources per artist, all with + enabled=True, polluting the artist detail page and tricking the subscription checker into trying to poll patreon post URLs as feeds. - Operator-flagged 2026-05-26. + Operator-flagged 2026-05-26; consolidated via alembic 0022. - New behaviour: if any Source row exists for (artist_id, platform), - reuse it regardless of its URL — the artist's real subscription Source - (created by the downloader / extension / UI) is the canonical - attachment point for filesystem-imported posts. If none exists, create - ONE synthetic anchor with url='sidecar::' and - enabled=False (so the subscription checker doesn't poll it). + Resolution order: prefer a real (non-sidecar) Source over a + synthetic anchor. When alembic 0022 ran, it may have rewritten + per-post Sources into `sidecar::` synthetic + anchors. If the operator later added the real subscription, both + rows now coexist. A naive `ORDER BY id ASC LIMIT 1` lookup would + pick the older synthetic and silently attach every gallery-dl + download to the wrong Source — operator-flagged 2026-05-31 after + the Subscriptions UI surfaced the phantom anchors. Pick the real + one when one exists; fall back to the synthetic; only create a + new synthetic when nothing exists for (artist, platform). """ - stmt = ( + real_stmt = ( + select(Source) + .where( + Source.artist_id == artist_id, + Source.platform == platform, + ~Source.url.like("sidecar:%"), + ) + .order_by(Source.id.asc()) + .limit(1) + ) + real = self.session.execute(real_stmt).scalar_one_or_none() + if real is not None: + return real + + any_stmt = ( select(Source) .where( Source.artist_id == artist_id, @@ -288,7 +308,7 @@ class Importer: .limit(1) ) return self._get_or_create( - stmt, + any_stmt, lambda: Source( artist_id=artist_id, platform=platform, diff --git a/backend/app/services/source_service.py b/backend/app/services/source_service.py index 85a6564..57a9571 100644 --- a/backend/app/services/source_service.py +++ b/backend/app/services/source_service.py @@ -151,10 +151,18 @@ class SourceService: async def list( self, artist_id: int | None = None, failing: bool = False, + include_synthetic: bool = False, ) -> list[SourceRecord]: stmt = select(Source, Artist).join(Artist, Artist.id == Source.artist_id) if artist_id is not None: stmt = stmt.where(Source.artist_id == artist_id) + if not include_synthetic: + # Filesystem-import sidecar anchors (importer._source_for_sidecar) + # have url='sidecar::' and exist only to give + # imported Posts a NOT-NULL Source FK. They aren't pollable + # feeds; the Subscriptions UI used to render them as phantom + # subscriptions. Hide by default. + stmt = stmt.where(~Source.url.like("sidecar:%")) if failing: # Worst-first so the rollup card surfaces the loudest failures. stmt = stmt.where(Source.consecutive_failures > 0).order_by( diff --git a/requirements.txt b/requirements.txt index a6e009b..dc76496 100644 --- a/requirements.txt +++ b/requirements.txt @@ -22,6 +22,11 @@ imagehash>=4.3,<4.4 # Gallery-dl wrapper (lands in FC-3) gallery-dl>=1.32,<1.33 +# Video extractor backend for gallery-dl. Without it, every video post +# attachment fails with `[downloader.ytdl][error] Cannot import yt-dlp` +# → `[download][error] Failed to download NN_video.mp4`. Operator-flagged +# 2026-05-31 after Patreon video posts produced empty downloads. +yt-dlp>=2025.1 # Utilities python-dotenv>=1.2,<2.0 diff --git a/tests/test_gallery_dl_service.py b/tests/test_gallery_dl_service.py index 35d2d04..af490d1 100644 --- a/tests/test_gallery_dl_service.py +++ b/tests/test_gallery_dl_service.py @@ -164,6 +164,58 @@ async def test_validation_quarantines_truncated_files(tmp_path, monkeypatch): assert result.error_type == ErrorType.VALIDATION_FAILED +@pytest.mark.asyncio +async def test_download_tier_limited_classified_as_success(gdl, monkeypatch): + """Pure tier-gated stderr (paywall warnings only, no source-level + errors). Should be TIER_LIMITED with success=True so the + DownloadEvent doesn't surface as 'needs attention' for an operator + who's just not paying the top tier. Pre-fix: this fell through to + UNKNOWN_ERROR when the return code wasn't 1 or 4.""" + stderr = ( + "[patreon][warning] Not allowed to view post 159421258\n" + "[patreon][warning] Not allowed to view post 159247879\n" + "[patreon][warning] Not allowed to view post 158111214\n" + ) + # Use a non-(1,4) return code to prove the gate widened. + monkeypatch.setattr( + "backend.app.services.gallery_dl.subprocess.run", + lambda *a, **k: _proc(stdout="", stderr=stderr, returncode=128), + ) + result = await gdl.download( + url="https://patreon.com/alice", artist_slug="alice", platform="patreon", + source_config=SourceConfig(), + ) + assert result.error_type == ErrorType.TIER_LIMITED + assert result.success is True + assert "3 posts" in (result.error_message or "") + + +@pytest.mark.asyncio +async def test_download_tier_limited_with_per_item_video_failures(gdl, monkeypatch): + """Real operator log shape (2026-05-31): hundreds of paywall warnings + + a handful of per-item video errors caused by missing yt-dlp. The + video errors are classified as per-item (not source-level), so the + classifier should still pick TIER_LIMITED. Once yt-dlp is installed + the video errors stop firing; until then, the source-health doesn't + flap to 'needs attention' over paywalled content.""" + stderr = ( + "[patreon][warning] Not allowed to view post 159421258\n" + "[patreon][warning] Not allowed to view post 159247879\n" + "[downloader.ytdl][error] Cannot import yt-dlp or youtube-dl\n" + "[download][error] Failed to download 02_video.mp4\n" + ) + monkeypatch.setattr( + "backend.app.services.gallery_dl.subprocess.run", + lambda *a, **k: _proc(stdout="", stderr=stderr, returncode=1), + ) + result = await gdl.download( + url="https://patreon.com/alice", artist_slug="alice", platform="patreon", + source_config=SourceConfig(), + ) + assert result.error_type == ErrorType.TIER_LIMITED + assert result.success is True + + def test_compute_run_stats(gdl): stats = gdl._compute_run_stats( return_code=0, diff --git a/tests/test_importer_upsert_helpers.py b/tests/test_importer_upsert_helpers.py index 8472031..7f8882e 100644 --- a/tests/test_importer_upsert_helpers.py +++ b/tests/test_importer_upsert_helpers.py @@ -210,3 +210,36 @@ def test_source_for_sidecar_distinct_platforms_distinct_anchors( assert p.id != x.id assert p.platform == "patreon" assert x.platform == "pixiv" + + +def test_source_for_sidecar_prefers_real_over_synthetic_when_both_exist( + importer, artist_row, db_sync, +): + """When BOTH a synthetic anchor AND a real Source exist for the same + (artist, platform), the resolver must return the REAL one. This is the + fix for the 2026-05-31 phantom-subscription bug: alembic 0022 had + rewritten an older per-post Source row into a sidecar synthetic, and + the operator later added the real subscription. The old `ORDER BY + id ASC LIMIT 1` lookup picked the older synthetic (lower id), + silently attaching every gallery-dl download to the wrong Source. + """ + synthetic = Source( + artist_id=artist_row.id, platform="patreon", + url=f"sidecar:patreon:{artist_row.slug}", enabled=False, + ) + db_sync.add(synthetic) + db_sync.flush() + real = Source( + artist_id=artist_row.id, platform="patreon", + url="https://www.patreon.com/testartist", enabled=True, + ) + db_sync.add(real) + db_sync.flush() + assert synthetic.id < real.id # ordering precondition + + resolved = importer._source_for_sidecar( + artist_id=artist_row.id, platform="patreon", + artist_slug=artist_row.slug, + ) + assert resolved.id == real.id + assert resolved.url == "https://www.patreon.com/testartist" diff --git a/tests/test_source_service.py b/tests/test_source_service.py index aa310ef..f6df9f3 100644 --- a/tests/test_source_service.py +++ b/tests/test_source_service.py @@ -1,7 +1,7 @@ import pytest from sqlalchemy import select -from backend.app.models import Artist +from backend.app.models import Artist, Source from backend.app.services.source_service import ( KNOWN_PLATFORMS, ArtistNotFoundError, @@ -133,3 +133,39 @@ async def test_update_changes_fields(db): updated = await svc.update(rec.id, enabled=False, config_overrides={"videos": False}) assert updated.enabled is False assert updated.config_overrides == {"videos": False} + + +@pytest.mark.asyncio +async def test_list_hides_sidecar_synthetic_anchors(db): + """Filesystem-import synthetic Sources (url='sidecar::', + enabled=False — see importer._source_for_sidecar) used to leak into the + Subscriptions UI as phantom subscriptions because list() didn't filter + them. They aren't pollable feeds; hide by default.""" + artist = await _artist(db, "Alice") + real = Source( + artist_id=artist.id, platform="patreon", + url="https://patreon.com/alice", enabled=True, config_overrides={}, + ) + synthetic = Source( + artist_id=artist.id, platform="patreon", + url="sidecar:patreon:alice", enabled=False, config_overrides={}, + ) + db.add_all([real, synthetic]) + await db.commit() + + svc = SourceService(db) + visible = await svc.list() + visible_urls = {s.url for s in visible} + assert "https://patreon.com/alice" in visible_urls + assert "sidecar:patreon:alice" not in visible_urls + + # Same filter applies to the artist-scoped list path (the artist detail + # page hits /api/sources?artist_id=N). + artist_scoped = await svc.list(artist_id=artist.id) + assert {s.url for s in artist_scoped} == {"https://patreon.com/alice"} + + # include_synthetic=True opts back in for admin tooling. + everything = await svc.list(include_synthetic=True) + assert {s.url for s in everything} >= { + "https://patreon.com/alice", "sidecar:patreon:alice", + }