From 2ce467e347b4a453331d9ff6363ccc0d352094db Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 26 Aug 2026 22:23:29 -0400 Subject: [PATCH] fix(cleanup): artist cascade preview counts posts and attachments (#3067) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `project_artist_cascade` is documented as "a read-only projection of what delete_artist_cascade would touch" and drives the Tier-C confirm dialog, but it counted only images, sources, thumbs, import_tasks and bytes. It never counted posts or attachments, both of which the apply destroys. That is silent in the worst case. `Post.artist_id` is ondelete=CASCADE, so every post goes whether or not it carried an image — and FC has a large body-only post population (#1288 measured 694 pixiv posts with text and no images). Such an artist previewed as `images: 0`, reading as "empty, safe to remove", while the apply destroyed every captured body, description, external-link set and raw_metadata snapshot. The danger-zone card already promised "every image, source, post, and attachment" — the copy was honest and the numbers were not. Root of the drift: the preview re-derived its own predicates instead of sharing the apply's, the same shape as the 2026-06-08 fandom-tag deletion that rule 93 exists for. So rather than bolt on two counts, both halves now build from shared `_artist_{images,posts,attachments}_conditions` helpers, following the `_unused_tag_conditions` / `_bare_post_conditions` style already in the file. Sources keep no helper — the apply doesn't query them either, it gets them from the Artist.sources ORM cascade. The apply also now reports `posts_deleted` (counted before the delete, since the CASCADE leaves nothing to count after). Rule 93's second half asks for the apply to be tested, and parity is only assertable if both halves state the number. Adds a preview/apply parity test that runs both against one artist and asserts the three pairs agree AND that the rows actually went, plus a body-only-artist test covering the case that motivated this. The confirm dialog's counts grid renders every key, so posts and attachments surface there automatically; the prose summary line names posts explicitly, since that is the number that changes how an artist reads. Co-Authored-By: Claude Opus 5 (1M context) --- backend/app/services/cleanup_service.py | 103 ++++++++++++++---- .../components/artist/ArtistDangerZone.vue | 21 ++-- tests/test_cleanup_service.py | 83 ++++++++++++++ 3 files changed, 176 insertions(+), 31 deletions(-) diff --git a/backend/app/services/cleanup_service.py b/backend/app/services/cleanup_service.py index 2a34509..d39cdb6 100644 --- a/backend/app/services/cleanup_service.py +++ b/backend/app/services/cleanup_service.py @@ -48,6 +48,47 @@ log = logging.getLogger(__name__) _VIDEO_DURATION_UNKNOWN = -1.0 +# -- artist-cascade predicates (rule 93: ONE definition, preview + apply) --- +# project_artist_cascade (preview) and delete_artist_cascade (apply) both build +# their queries from these. The preview used to re-derive its own — which is how +# it came to count images and stay silent about posts and attachments while the +# apply destroyed both. Same failure shape as the 2026-06-08 fandom-tag +# deletion, where a re-implemented delete predicate diverged from the preview's. +# Returned as condition LISTS spread into `.where(*conds)`, matching +# _unused_tag_conditions / _bare_post_conditions below. + + +def _artist_images_conditions(artist_id: int) -> list: + """Images the cascade deletes (rows AND their on-disk files).""" + return [ImageRecord.artist_id == artist_id] + + +def _artist_posts_conditions(artist_id: int) -> list: + """Posts the cascade destroys. The apply never names these — post.artist_id + is ondelete=CASCADE, so Postgres takes them when the artist row goes — which + is exactly why the preview has to name them: an artist whose posts are + body-only (no images) otherwise previews as `images: 0` and reads as an + empty artist, while every captured body/description/external-link set is + destroyed.""" + return [Post.artist_id == artist_id] + + +def _artist_attachments_conditions(artist_id: int) -> list: + """Attachments the cascade deletes. Matched by artist_id OR by the owning + post's artist: artist_id is nullable (_capture_attachment leaves it NULL + when no artist resolved), so neither arm alone covers every row. The + sha-addressed blobs are NOT unlinked (one blob backs many rows) — these are + row counts, and the bytes are not part of this operation's footprint.""" + return [ + or_( + PostAttachment.artist_id == artist_id, + PostAttachment.post_id.in_( + select(Post.id).where(*_artist_posts_conditions(artist_id)) + ), + ) + ] + + def project_artist_cascade(session: Session, *, slug: str) -> dict: """Read-only projection of what delete_artist_cascade would touch. @@ -56,12 +97,17 @@ def project_artist_cascade(session: Session, *, slug: str) -> dict: "artist": {"id": int, "name": str, "slug": str}, "projected": { "images": int, + "posts": int, # hard-deleted by the post.artist_id CASCADE + "attachments": int, # rows deleted; the sha-addressed blobs stay "sources": int, "thumbs": int, # images with a thumbnail_path set "import_tasks": int, # ImportTask rows referencing the artist's images "bytes_on_disk": int, # SUM(image_record.size_bytes) — column is NOT NULL }, } + Every count is built from the shared `_artist_*_conditions` predicates the + apply uses, so the two halves cannot drift (rule 93). + Raises LookupError if slug not found. No mutations. """ from ..models.import_task import ImportTask @@ -73,36 +119,49 @@ def project_artist_cascade(session: Session, *, slug: str) -> dict: if artist is None: raise LookupError(f"artist slug not found: {slug!r}") + images_conds = _artist_images_conditions(artist.id) + images_count = session.execute( - select(func.count(ImageRecord.id)) - .where(ImageRecord.artist_id == artist.id) + select(func.count(ImageRecord.id)).where(*images_conds) ).scalar_one() + posts_count = session.execute( + select(func.count(Post.id)) + .where(*_artist_posts_conditions(artist.id)) + ).scalar_one() + attachments_count = session.execute( + select(func.count(PostAttachment.id)) + .where(*_artist_attachments_conditions(artist.id)) + ).scalar_one() + # Sources have no shared predicate: the apply never queries them either, it + # gets them from the Artist.sources ORM cascade. Counted directly here. sources_count = session.execute( select(func.count(Source.id)) .where(Source.artist_id == artist.id) ).scalar_one() thumbs_count = session.execute( select(func.count(ImageRecord.id)) - .where(ImageRecord.artist_id == artist.id) + .where(*images_conds) .where(ImageRecord.thumbnail_path.is_not(None)) ).scalar_one() import_tasks_count = session.execute( select(func.count(ImportTask.id)) .where( ImportTask.result_image_id.in_( - select(ImageRecord.id).where(ImageRecord.artist_id == artist.id) + select(ImageRecord.id).where(*images_conds) ) ) ).scalar_one() bytes_on_disk = session.execute( select(func.coalesce(func.sum(ImageRecord.size_bytes), 0)) - .where(ImageRecord.artist_id == artist.id) + .where(*images_conds) ).scalar_one() return { "artist": {"id": artist.id, "name": artist.name, "slug": artist.slug}, "projected": { "images": images_count, + "posts": posts_count, + "attachments": attachments_count, "sources": sources_count, "thumbs": thumbs_count, "import_tasks": import_tasks_count, @@ -291,12 +350,22 @@ def delete_artist_cascade( "files_deleted": 0, "thumbs_deleted": 0, "import_tasks_nulled": 0, + "posts_deleted": 0, "attachments_deleted": 0, "files_failed": 0, }, } artist_info = {"id": artist.id, "name": artist.name, "slug": artist.slug} + # Counted BEFORE the delete: Postgres takes these via the post.artist_id + # CASCADE when the artist row goes, so afterwards there is nothing left to + # count. Reported so the summary can be checked against the preview's + # `posts` — the parity rule 93 asks for is only testable if both halves + # actually state the number. + posts_deleted = session.execute( + select(func.count(Post.id)).where(*_artist_posts_conditions(artist.id)) + ).scalar_one() + images_deleted = 0 files_deleted = 0 thumbs_deleted = 0 @@ -305,7 +374,7 @@ def delete_artist_cascade( while True: rows = session.execute( select(ImageRecord) - .where(ImageRecord.artist_id == artist.id) + .where(*_artist_images_conditions(artist.id)) .limit(500) ).scalars().all() if not rows: @@ -342,24 +411,11 @@ def delete_artist_cascade( # _repoint_post_links guards the identical collision class in the reconcile # path; this is its artist-cascade counterpart. # - # Matched by artist_id OR by the owning post's artist: artist_id is nullable - # and _capture_attachment leaves it NULL when no artist resolved, so neither - # predicate alone covers every row this cascade is about to strand. - # - # The sha-addressed BLOBS are deliberately left on disk. One blob backs many - # rows (attachment_store.store is sha-addressed + idempotent), so unlinking - # needs a refcount pass over the surviving rows — that belongs to the - # attachment-reclamation sweep, not here, and this Tier-C op must not delete - # bytes its own preview never disclosed. + # Which rows count as the artist's — and why the blobs are left on disk — + # is _artist_attachments_conditions, shared with the preview. attachments_deleted = session.execute( - delete(PostAttachment).where( - or_( - PostAttachment.artist_id == artist.id, - PostAttachment.post_id.in_( - select(Post.id).where(Post.artist_id == artist.id) - ), - ) - ) + delete(PostAttachment) + .where(*_artist_attachments_conditions(artist.id)) ).rowcount or 0 session.commit() @@ -373,6 +429,7 @@ def delete_artist_cascade( "files_deleted": files_deleted, "thumbs_deleted": thumbs_deleted, "import_tasks_nulled": import_tasks_nulled, + "posts_deleted": posts_deleted, "attachments_deleted": attachments_deleted, "files_failed": files_failed, }, diff --git a/frontend/src/components/artist/ArtistDangerZone.vue b/frontend/src/components/artist/ArtistDangerZone.vue index 275c4a4..f4da126 100644 --- a/frontend/src/components/artist/ArtistDangerZone.vue +++ b/frontend/src/components/artist/ArtistDangerZone.vue @@ -50,14 +50,19 @@ const projected = ref(null) const projectedCounts = computed(() => projected.value?.projected || null) -const modalDescription = computed( - () => projected.value - ? `Artist “${props.artistName}” — ` - + `${projected.value.projected.images} images, ` - + `${projected.value.projected.sources} sources, ` - + `${Math.round(projected.value.projected.bytes_on_disk / 1_048_576)} MiB on disk` - : '', -) +// `posts` is named here, not left to the counts grid below it: an artist whose +// posts are body-only previews as `images: 0`, and a summary line that says +// only "0 images" reads as "this artist is empty" while the apply destroys +// every captured post body (#3067). Attachments stay in the grid — the grid +// renders every key, so this line carries only what changes the read. +const modalDescription = computed(() => { + const p = projectedCounts.value + return p + ? `Artist “${props.artistName}” — ${p.images} images, ` + + `${p.posts} posts, ${p.sources} sources, ` + + `${Math.round(p.bytes_on_disk / 1_048_576)} MiB on disk` + : '' +}) async function onClick() { loading.value = true diff --git a/tests/test_cleanup_service.py b/tests/test_cleanup_service.py index a9f3d00..7303a2f 100644 --- a/tests/test_cleanup_service.py +++ b/tests/test_cleanup_service.py @@ -68,6 +68,8 @@ def test_project_artist_cascade_returns_zeroes_for_empty_artist(db_sync): assert result["artist"]["slug"] == "empty" assert result["projected"] == { "images": 0, + "posts": 0, + "attachments": 0, "sources": 0, "thumbs": 0, "import_tasks": 0, @@ -92,6 +94,87 @@ def test_project_artist_cascade_counts_images_and_thumbs_and_bytes(db_sync, tmp_ assert result["projected"]["bytes_on_disk"] == 3500 +def test_project_artist_cascade_counts_posts_and_attachments(db_sync, tmp_path): + """The body-only artist: zero images, but posts and attachments that the + apply destroys. Previewing this as `images: 0` alone is what made a + content-only artist read as an empty one (#3067).""" + a = _make_artist(db_sync, slug="bodyonly") + p1 = Post(artist_id=a.id, external_post_id="bo-1", description="a body") + p2 = Post(artist_id=a.id, external_post_id="bo-2", description="another") + db_sync.add_all([p1, p2]) + db_sync.flush() + db_sync.add(PostAttachment( + post_id=p1.id, artist_id=a.id, sha256="b0d1".ljust(64, "0"), + path="/store/b0d1/a.pdf", original_filename="a.pdf", + ext=".pdf", size_bytes=5, + )) + # artist_id NULL, reachable only through its post — the second arm of + # _artist_attachments_conditions. + db_sync.add(PostAttachment( + post_id=p2.id, artist_id=None, sha256="b0d2".ljust(64, "0"), + path="/store/b0d2/b.pdf", original_filename="b.pdf", + ext=".pdf", size_bytes=5, + )) + db_sync.commit() + + projected = cleanup_service.project_artist_cascade( + db_sync, slug="bodyonly", + )["projected"] + assert projected["images"] == 0 + assert projected["posts"] == 2 + assert projected["attachments"] == 2 + + +def test_artist_cascade_preview_matches_apply(db_sync, tmp_path): + """Rule 93: the preview's numbers must be what the apply actually does. + + Guards the drift directly rather than trusting that both halves happen to + use the same predicate — the preview and apply are asserted against each + other on one artist carrying all three row kinds. + """ + a = _make_artist(db_sync, slug="parity") + for i in range(3): + f = tmp_path / f"par{i}.jpg" + f.write_bytes(b"x") + _make_image( + db_sync, artist=a, path=str(f), sha256=f"{i:064x}", size=10, + ) + posts = [ + Post(artist_id=a.id, external_post_id=f"par-{i}") for i in range(4) + ] + db_sync.add_all(posts) + db_sync.flush() + for i, p in enumerate(posts[:2]): + db_sync.add(PostAttachment( + post_id=p.id, artist_id=a.id, sha256=f"par{i}".ljust(64, "0"), + path=f"/store/par{i}/f.zip", original_filename="f.zip", + ext=".zip", size_bytes=9, + )) + db_sync.commit() + artist_id = a.id + + projected = cleanup_service.project_artist_cascade( + db_sync, slug="parity", + )["projected"] + summary = cleanup_service.delete_artist_cascade( + db_sync, artist_id=artist_id, images_root=tmp_path, + )["summary"] + + assert projected["images"] == summary["images_deleted"] == 3 + assert projected["posts"] == summary["posts_deleted"] == 4 + assert projected["attachments"] == summary["attachments_deleted"] == 2 + + # And the apply really did remove them — a matching pair of numbers is + # worth nothing if neither half touched the DB. + assert db_sync.execute( + select(func.count(Post.id)).where(Post.artist_id == artist_id) + ).scalar_one() == 0 + assert db_sync.execute( + select(func.count(PostAttachment.id)) + .where(PostAttachment.artist_id == artist_id) + ).scalar_one() == 0 + + def test_project_artist_cascade_raises_on_unknown_slug(db_sync): with pytest.raises(LookupError): cleanup_service.project_artist_cascade(db_sync, slug="nope")