fix(recapture): link on-disk images to their post (#1288)
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CDgx8bQS5YrGRK76v8HUnM
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user