From 8838b325fb560af70cf1a512a8e656b9470a7a27 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sat, 4 Jul 2026 20:13:53 -0400 Subject: [PATCH] fix(recapture): link on-disk images to their post (#1288) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Recapture disk-skips already-downloaded media, and upsert_post_record only writes Post fields — so a pre-existing image (e.g. one pulled under the old gallery-dl path, imported bare with no post) stays orphaned even after its post record is (re)written. Confirmed on the operator's instance: 329 pixiv images with primary_post_id NULL, 694 pixiv posts with content but no linked images, 0 duplicate posts. Fix: the recapture relink channel now carries the media's post_id (2- → 3-tuple path/url/post_id), and phase 3 calls importer.link_existing_image_to_post — match the on-disk image by path, find its Post by (source, external_post_id), upsert image_provenance + primary_post_id. Factored the provenance-linking out of _apply_sidecar into a shared _attach_provenance so the fresh-import and recapture-backlink paths can't diverge. Idempotent; generic across native platforms (no-op for already-linked Patreon/SubscribeStar). Re-running recapture now repairs orphaned images; future walks never orphan. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01CDgx8bQS5YrGRK76v8HUnM --- backend/app/services/download_service.py | 25 +++- backend/app/services/gallery_dl.py | 2 +- backend/app/services/importer.py | 172 +++++++++++++++-------- backend/app/services/ingest_core.py | 22 +-- tests/test_patreon_ingester.py | 6 +- tests/test_sidecar_import.py | 54 +++++++ 6 files changed, 205 insertions(+), 76 deletions(-) diff --git a/backend/app/services/download_service.py b/backend/app/services/download_service.py index 772fdc9..08ca44e 100644 --- a/backend/app/services/download_service.py +++ b/backend/app/services/download_service.py @@ -418,7 +418,8 @@ class DownloadService: # the duplicate file). Empty outside recapture mode. relink_pairs = getattr(dl_result, "relink_source_paths", None) or [] relinked = 0 - for rel_str, rel_url in relink_pairs: + post_linked = 0 + for rel_str, rel_url, rel_post_id in relink_pairs: rel_path = Path(rel_str) if not rel_path.exists(): # noqa: ASYNC240 continue @@ -428,13 +429,27 @@ class DownloadService: if await loop.run_in_executor(None, _relink): relinked += 1 + + # #1288: link the on-disk image to its Post. Recapture disk-skips the + # media (never re-imported), so a pre-existing image (e.g. one pulled + # under the old gallery-dl path) otherwise stays orphaned even after + # the post record is written. Idempotent for already-linked images. + def _link(p=rel_path, pid=rel_post_id): + return self.importer.link_existing_image_to_post( + p, pid, source=source_row, artist=artist, + ) + + if await loop.run_in_executor(None, _link): + post_linked += 1 if relink_pairs: # recapture diagnostic: how many on-disk images got their - # source_filehash backfilled (inline-image localization). < total is - # normal — files already carrying a filehash are skipped (NULL-only). + # source_filehash backfilled (inline-image localization) and how many + # got (re)linked to their Post. < total for source_filehash is normal + # (files already carrying a filehash are skipped, NULL-only). log.info( - "recap: relinked source_filehash on %d/%d on-disk image(s)", - relinked, len(relink_pairs), + "recap: relinked source_filehash on %d and linked %d/%d on-disk " + "image(s) to their post", + relinked, post_linked, len(relink_pairs), ) # Kick the off-platform file-host downloader for any links this run diff --git a/backend/app/services/gallery_dl.py b/backend/app/services/gallery_dl.py index 748052e..71d13af 100644 --- a/backend/app/services/gallery_dl.py +++ b/backend/app/services/gallery_dl.py @@ -157,7 +157,7 @@ class DownloadResult: # pairs for already-present media whose ImageRecord.source_filehash should be # backfilled (inline-image localization) WITHOUT re-download or unlink. Empty # on the gallery-dl path and outside recapture. - relink_source_paths: list[tuple[str, str]] = field(default_factory=list) + relink_source_paths: list[tuple[str, str, str]] = field(default_factory=list) stdout: str = "" stderr: str = "" return_code: int = 0 diff --git a/backend/app/services/importer.py b/backend/app/services/importer.py index 2459836..69be5f8 100644 --- a/backend/app/services/importer.py +++ b/backend/app/services/importer.py @@ -1313,6 +1313,113 @@ class Importer: self.session.commit() return True + def link_existing_image_to_post( + self, path: Path, external_post_id: str, *, + source: Source | None = None, artist: Artist | None = None, + ) -> bool: + """Recapture back-link: associate an ALREADY-on-disk image (matched by + its stored path) with its post — the link _apply_sidecar normally makes + at per-media import time. + + Recapture disk-skips downloaded media, so a pre-existing image (e.g. one + pulled under the old gallery-dl path, imported as a bare record with no + post) never gets its `image_provenance` row / `primary_post_id`. This + backfills it from the walk's (on-disk path, post external id) pairing: + find the ImageRecord by path, find/attach the Post by (source, + external_post_id), upsert provenance. Idempotent (issue #1288). Returns + True when a record matched and was linked; no-op when the file has no + record or no id is given.""" + if not external_post_id: + return False + record = self.session.execute( + select(ImageRecord).where(ImageRecord.path == str(path)) + ).scalar_one_or_none() + if record is None: + return False + artist_id = record.artist_id or (artist.id if artist else None) + if artist_id is None: + return False + if record.artist_id is None: + record.artist_id = artist_id + post = self._find_or_create_post( + source_id=source.id if source else None, + external_post_id=str(external_post_id), + artist_id=artist_id, + ) + self._attach_provenance( + record, post, source_id=source.id if source else None, + ) + self.session.commit() + return True + + def _attach_provenance( + self, record: ImageRecord, post: Post, *, + source_id: int | None, captured_metadata: dict | None = None, + ) -> None: + """Upsert the (image, post) `image_provenance` link + keep + `primary_post_id` and the denormalized gallery sort keys aligned. Shared + by _apply_sidecar (fresh per-media import) and link_existing_image_to_post + (recapture back-link, #1288) so the two paths can't diverge. Idempotent. + + Race-safe (image_record_id, post_id) upsert — mirrors the + _find_or_create_source/post savepoint pattern. The plain + SELECT-then-INSERT pattern lost a race when two workers ran on the same + (image, post) pair (e.g. the 5-min recovery sweep re-enqueued a + still-running long import), planting duplicates that then broke + .scalar_one_or_none() on every later deep-scan rederive + (MultipleResultsFound). Alembic 0021's uq_image_provenance_image_post + UNIQUE makes this savepoint trip on collision.""" + exists = self.session.execute( + select(ImageProvenance.id).where( + ImageProvenance.image_record_id == record.id, + ImageProvenance.post_id == post.id, + ) + ).scalar_one_or_none() + if exists is None: + sp = self.session.begin_nested() + try: + self.session.add( + ImageProvenance( + image_record_id=record.id, + post_id=post.id, + source_id=source_id, + captured_metadata=captured_metadata, + ) + ) + self.session.flush() + sp.commit() + except IntegrityError: + sp.rollback() + if record.primary_post_id is None: + record.primary_post_id = post.id + # Keep the denormalized gallery sort key (alembic 0035) aligned with + # the primary post's publish date so /scroll orders off + # ix_image_record_effective_date instead of COALESCE-ing across the + # post join. Only override when THIS post is the primary AND carries + # a date; otherwise the column keeps its created_at-equivalent server + # default (matches the old COALESCE(post_date, created_at) fallback). + if record.primary_post_id == post.id and post.post_date is not None: + record.effective_date = post.post_date + # earliest_post_date (alembic 0071) = MIN(post_date) across ALL of this + # image's provenance posts, not just the primary — so the gallery can + # sort by original publish rather than the download/repost the primary + # points at. Recompute from provenance whenever a dated post is linked; + # the provenance row for THIS post was committed above, so the MIN + # includes it. Leaves the created_at default when no linked post is dated. + if post.post_date is not None: + earliest = self.session.execute( + select(func.min(Post.post_date)) + .select_from(ImageProvenance) + .join(Post, Post.id == ImageProvenance.post_id) + .where( + ImageProvenance.image_record_id == record.id, + Post.post_date.is_not(None), + ) + ).scalar_one_or_none() + if earliest is not None: + record.earliest_post_date = earliest + self.session.flush() + def _apply_sidecar( self, record: ImageRecord, @@ -1386,65 +1493,12 @@ class Importer: if not self.post_first: self._apply_post_fields(post, sd) - # Race-safe (image_record_id, post_id) upsert — mirrors the - # _find_or_create_source/post savepoint pattern. The plain - # SELECT-then-INSERT pattern lost a race when two workers ran - # _apply_sidecar on the same (image, post) pair (e.g. the 5-min - # recovery sweep re-enqueued a still-running long import), planting - # duplicates that then broke .scalar_one_or_none() on every later - # deep-scan rederive (MultipleResultsFound). Alembic 0021 adds the - # uq_image_provenance_image_post UNIQUE so this savepoint actually - # trips on collision. - exists = self.session.execute( - select(ImageProvenance.id).where( - ImageProvenance.image_record_id == record.id, - ImageProvenance.post_id == post.id, - ) - ).scalar_one_or_none() - if exists is None: - sp = self.session.begin_nested() - try: - self.session.add( - ImageProvenance( - image_record_id=record.id, - post_id=post.id, - source_id=src.id if src else None, - captured_metadata=sd.raw, - ) - ) - self.session.flush() - sp.commit() - except IntegrityError: - sp.rollback() - if record.primary_post_id is None: - record.primary_post_id = post.id - # Keep the denormalized gallery sort key (alembic 0035) aligned with - # the primary post's publish date so /scroll orders off - # ix_image_record_effective_date instead of COALESCE-ing across the - # post join. Only override when THIS post is the primary AND carries - # a date; otherwise the column keeps its created_at-equivalent server - # default (matches the old COALESCE(post_date, created_at) fallback). - if record.primary_post_id == post.id and post.post_date is not None: - record.effective_date = post.post_date - # earliest_post_date (alembic 0071) = MIN(post_date) across ALL of this - # image's provenance posts, not just the primary — so the gallery can - # sort by original publish rather than the download/repost the primary - # points at. Recompute from provenance whenever a dated post is linked; - # the provenance row for THIS post was committed above, so the MIN - # includes it. Leaves the created_at default when no linked post is dated. - if post.post_date is not None: - earliest = self.session.execute( - select(func.min(Post.post_date)) - .select_from(ImageProvenance) - .join(Post, Post.id == ImageProvenance.post_id) - .where( - ImageProvenance.image_record_id == record.id, - Post.post_date.is_not(None), - ) - ).scalar_one_or_none() - if earliest is not None: - record.earliest_post_date = earliest - self.session.flush() + # Link the (image, post) provenance + keep primary_post_id / the gallery + # sort keys aligned — shared with the recapture back-link path (#1288). + self._attach_provenance( + record, post, source_id=src.id if src else None, + captured_metadata=sd.raw, + ) def _copy_to_library( self, source: Path, sha: str, attribution_path: Path diff --git a/backend/app/services/ingest_core.py b/backend/app/services/ingest_core.py index 42deb77..9b5d391 100644 --- a/backend/app/services/ingest_core.py +++ b/backend/app/services/ingest_core.py @@ -179,10 +179,11 @@ class Ingester: written: list[str] = [] post_records: list[str] = [] quarantined_paths: list[str] = [] - # #830 recapture: (on-disk path, CDN source_url) pairs for already-present - # media, so phase 3 can backfill the ImageRecord's source_filehash WITHOUT - # re-downloading or unlinking the file. Empty outside recapture mode. - relink: list[tuple[str, str]] = [] + # #830 recapture: (on-disk path, CDN source_url, post_id) triples for + # already-present media, so phase 3 can (a) backfill the ImageRecord's + # source_filehash and (b) link the on-disk image to its Post (#1288) — + # WITHOUT re-downloading or unlinking the file. Empty outside recapture. + relink: list[tuple[str, str, str]] = [] downloaded = 0 errors = 0 quarantined = 0 @@ -410,12 +411,15 @@ class Ingester: to_clear.append(key) skipped_count += 1 consecutive_seen += 1 - # #830 recapture: surface (on-disk path, CDN url) so phase - # 3 can backfill source_filehash for inline-image - # localization — a SEPARATE non-deleting channel, never the - # import list (which would unlink the file, per above). + # #830/#1288 recapture: surface (on-disk path, CDN url, + # post_id) so phase 3 can backfill source_filehash AND link + # the on-disk image to its Post — a SEPARATE non-deleting + # channel, never the import list (which would unlink the + # file, per above). if recapture and outcome.path is not None: - relink.append((str(outcome.path), media_item.url)) + relink.append( + (str(outcome.path), media_item.url, media_item.post_id) + ) elif outcome.status == "skipped_seen": skipped_count += 1 consecutive_seen += 1 diff --git a/tests/test_patreon_ingester.py b/tests/test_patreon_ingester.py index 5316607..9e75462 100644 --- a/tests/test_patreon_ingester.py +++ b/tests/test_patreon_ingester.py @@ -667,10 +667,12 @@ async def test_backfill_skips_already_captured_post_but_recapture_forces_it( assert downloader2.post_records == 1 assert res_recap.files_downloaded == 0 # no media re-download assert downloader2.download_calls == 0 - # The on-disk media is surfaced as a (path, CDN url) relink pair. + # The on-disk media is surfaced as a (path, CDN url, post_id) relink triple + # (post_id drives the #1288 image→post back-link in phase 3). assert len(res_recap.relink_source_paths) == 1 - rel_path, rel_url = res_recap.relink_source_paths[0] + rel_path, rel_url, rel_post_id = res_recap.relink_source_paths[0] assert rel_url == m1.url + assert rel_post_id == m1.post_id # Diagnostic summary surfaces the post-record + relink counts (operator reads # this off the event stdout to see what a recapture actually did). assert "1 post-record(s), 1 relinked" in res_recap.stdout diff --git a/tests/test_sidecar_import.py b/tests/test_sidecar_import.py index 7ec05d4..87f1408 100644 --- a/tests/test_sidecar_import.py +++ b/tests/test_sidecar_import.py @@ -158,6 +158,60 @@ def test_relink_source_filehash_no_match_is_noop(importer, import_layout): ) is False +def test_link_existing_image_to_post_backfills_provenance(importer, import_layout, db_sync): + """#1288: an on-disk image imported BARE (no sidecar → no post link, as the + pre-cutover gallery-dl pixiv images were) gets linked to its post from the + recapture walk's (path, external_post_id) pairing — image_provenance + + primary_post_id — without re-importing. Idempotent.""" + import_root, _ = import_layout + m = import_root / "Alice" / "img.jpg" + _split(m, "v") + rec = importer.session.get(ImageRecord, importer.import_one(m).image_id) + assert rec.primary_post_id is None # bare: no post link yet + artist = db_sync.get(Artist, rec.artist_id) + src = Source( + artist_id=artist.id, platform="pixiv", + url="https://www.pixiv.net/users/1", enabled=True, + ) + db_sync.add(src) + db_sync.flush() + # The post already exists (upsert_post_record wrote it earlier in recapture). + post = Post( + source_id=src.id, artist_id=artist.id, + external_post_id="146132304", post_title="t", + ) + db_sync.add(post) + db_sync.commit() + + assert importer.link_existing_image_to_post( + Path(rec.path), "146132304", source=src, artist=artist, + ) is True + importer.session.refresh(rec) + assert rec.primary_post_id == post.id + prov = db_sync.execute(select(ImageProvenance).where( + ImageProvenance.image_record_id == rec.id, + ImageProvenance.post_id == post.id, + )).scalar_one() + assert prov.source_id == src.id + + # Idempotent: a second call re-affirms without duplicating the row. + assert importer.link_existing_image_to_post( + Path(rec.path), "146132304", source=src, artist=artist, + ) is True + n = db_sync.execute(select(func.count(ImageProvenance.id)).where( + ImageProvenance.image_record_id == rec.id, + ImageProvenance.post_id == post.id, + )).scalar_one() + assert n == 1 + + +def test_link_existing_image_to_post_no_match_is_noop(importer): + """A path with no ImageRecord, or an empty external id, is a clean no-op.""" + assert importer.link_existing_image_to_post( + Path("/nonexistent/x.jpg"), "999", + ) is False + + def test_reimport_same_post_idempotent(importer, import_layout): import_root, _ = import_layout # Threshold 0: only an exact phash match collapses. Orthogonal splits