From 39cf81aea6bee0c77f01b3d56151e9a7ac935a08 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 26 Aug 2026 22:02:51 -0400 Subject: [PATCH 01/16] fix(cleanup): clear an artist's attachments before the cascade delete (#3066) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `delete_artist_cascade` could abort partway through, and it aborted after the irreversible half. Deleting an artist CASCADEs to Post (post.artist_id is ondelete=CASCADE), which SET NULLs post_attachment.post_id — and `uq_post_attachment_null_post_sha` is a partial UNIQUE on sha256 ALONE WHERE post_id IS NULL. So any two of that artist's attachments sharing a sha collapse onto one another and raise. That shape is ordinary, not corrupt: `_capture_attachment` deliberately writes one row per post over a single sha-addressed blob, so a creator who attaches the same pdf to two posts already has two such rows. A pre-existing filesystem-import row (post_id NULL) with the same sha collides on its own. The images and their on-disk files are deleted and committed in 500-row batches BEFORE the artist row is touched, so the failure landed after them: images gone, artist and posts alive, files unrecoverable. Fix: delete the artist's post_attachment rows explicitly first, matched by artist_id OR by the owning post's artist (artist_id is nullable, so neither arm alone covers every row). `_repoint_post_links` already guards the identical collision class in the reconcile path; this is its artist-cascade counterpart. Migration 0043 reasoned only about upgrade-time safety and never about this later SET NULL. The sha-addressed blobs are deliberately left on disk: one blob backs many rows, so unlinking needs a refcount pass, and this Tier-C op must not delete bytes its own preview never disclosed. Adds `attachments_deleted` to the summary, and two regression tests — the same sha on two posts, and an unrelated NULL-post row that must survive. Co-Authored-By: Claude Opus 5 (1M context) --- backend/app/services/cleanup_service.py | 41 +++++++++++++ tests/test_cleanup_service.py | 82 +++++++++++++++++++++++++ 2 files changed, 123 insertions(+) diff --git a/backend/app/services/cleanup_service.py b/backend/app/services/cleanup_service.py index cd7ef99..2a34509 100644 --- a/backend/app/services/cleanup_service.py +++ b/backend/app/services/cleanup_service.py @@ -277,6 +277,10 @@ def delete_artist_cascade( series_page / tag_suggestion_rejection from ImageRecord delete, and source / post / download_event / etc. from Artist delete (via Artist.sources cascade="all, delete-orphan"). + + The artist's post_attachment rows are cleared EXPLICITLY before the + artist row goes — see the comment at that step; leaving them to the + cascade aborts the whole delete on a unique violation. """ artist = session.get(Artist, artist_id) if artist is None: @@ -287,6 +291,7 @@ def delete_artist_cascade( "files_deleted": 0, "thumbs_deleted": 0, "import_tasks_nulled": 0, + "attachments_deleted": 0, "files_failed": 0, }, } @@ -323,6 +328,41 @@ def delete_artist_cascade( # source_path_prefix matching that's out of scope here. import_tasks_nulled = 0 + # Clear the artist's attachments BEFORE the artist row, or the delete below + # aborts. Deleting an artist CASCADEs to Post (post.artist_id is + # ondelete=CASCADE), which SET NULLs post_attachment.post_id — and + # `uq_post_attachment_null_post_sha` is a partial UNIQUE on sha256 ALONE + # WHERE post_id IS NULL, so any two of this artist's attachments sharing a + # sha collapse onto one another and raise. That is an ORDINARY shape, not a + # corrupt one: _capture_attachment deliberately writes one row per post over + # a single sha-addressed blob (a creator who attaches the same pdf to two + # posts has two rows), and a pre-existing filesystem-import row with the same + # sha and a NULL post_id collides on its own. Migration 0043 reasoned only + # about upgrade-time safety and never about this later SET NULL. + # _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. + 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) + ), + ) + ) + ).rowcount or 0 + session.commit() + session.delete(artist) session.commit() @@ -333,6 +373,7 @@ def delete_artist_cascade( "files_deleted": files_deleted, "thumbs_deleted": thumbs_deleted, "import_tasks_nulled": import_tasks_nulled, + "attachments_deleted": attachments_deleted, "files_failed": files_failed, }, } diff --git a/tests/test_cleanup_service.py b/tests/test_cleanup_service.py index e2a89f6..a9f3d00 100644 --- a/tests/test_cleanup_service.py +++ b/tests/test_cleanup_service.py @@ -294,6 +294,88 @@ def test_delete_artist_cascade_idempotent_on_missing(db_sync, tmp_path): assert result["summary"]["images_deleted"] == 0 +def test_delete_artist_cascade_survives_same_sha_on_two_posts(db_sync, tmp_path): + """Same file attached to two of the artist's posts must not abort the delete. + + Left to the cascade this raises: artist delete CASCADEs to Post, which SET + NULLs post_attachment.post_id, and `uq_post_attachment_null_post_sha` + (sha256 alone, WHERE post_id IS NULL) then rejects the second row. That's an + ordinary shape — _capture_attachment writes one row per post over one + sha-addressed blob by design. Also covers the NULL-artist_id arm of the + delete predicate: the second row has no artist_id, only a post that does. + """ + a = _make_artist(db_sync, slug="casatt") + p1 = Post(artist_id=a.id, external_post_id="att-p1") + p2 = Post(artist_id=a.id, external_post_id="att-p2") + db_sync.add_all([p1, p2]) + db_sync.flush() + + shared_sha = "ca5a".ljust(64, "0") + db_sync.add(PostAttachment( + post_id=p1.id, artist_id=a.id, sha256=shared_sha, + path="/store/ca5a/bundle.zip", original_filename="bundle.zip", + ext=".zip", size_bytes=7, + )) + db_sync.add(PostAttachment( + post_id=p2.id, artist_id=None, sha256=shared_sha, + path="/store/ca5a/bundle.zip", original_filename="bundle.zip", + ext=".zip", size_bytes=7, + )) + db_sync.commit() + artist_id = a.id + + result = cleanup_service.delete_artist_cascade( + db_sync, artist_id=artist_id, images_root=tmp_path, + ) + + assert result["summary"]["attachments_deleted"] == 2 + assert db_sync.execute( + select(func.count(Artist.id)).where(Artist.id == artist_id) + ).scalar_one() == 0 + assert db_sync.execute( + select(func.count(PostAttachment.id)) + .where(PostAttachment.sha256 == shared_sha) + ).scalar_one() == 0 + + +def test_delete_artist_cascade_keeps_unrelated_null_post_attachment( + db_sync, tmp_path, +): + """A filesystem-import row (post_id NULL) sharing the sha is the other way + this collides — and it must SURVIVE: it belongs to no artist, so the + cascade has no claim on it.""" + a = _make_artist(db_sync, slug="casorph") + p = Post(artist_id=a.id, external_post_id="orph-p1") + db_sync.add(p) + db_sync.flush() + + sha = "0rfa".ljust(64, "0") + standalone = PostAttachment( + post_id=None, artist_id=None, sha256=sha, + path="/store/0rfa/manual.pdf", original_filename="manual.pdf", + ext=".pdf", size_bytes=3, + ) + db_sync.add(standalone) + db_sync.add(PostAttachment( + post_id=p.id, artist_id=a.id, sha256=sha, + path="/store/0rfa/manual.pdf", original_filename="manual.pdf", + ext=".pdf", size_bytes=3, + )) + db_sync.commit() + artist_id, standalone_id = a.id, standalone.id + + result = cleanup_service.delete_artist_cascade( + db_sync, artist_id=artist_id, images_root=tmp_path, + ) + + assert result["summary"]["attachments_deleted"] == 1 + surviving = db_sync.execute( + select(PostAttachment.id, PostAttachment.post_id) + .where(PostAttachment.sha256 == sha) + ).all() + assert surviving == [(standalone_id, None)] + + # --- delete_images -------------------------------------------------- From 2ce467e347b4a453331d9ff6363ccc0d352094db Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 26 Aug 2026 22:23:29 -0400 Subject: [PATCH 02/16] 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") From 2e0f8f8c611c28eed4265b884c63443eac9954c5 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 26 Aug 2026 22:37:24 -0400 Subject: [PATCH 03/16] =?UTF-8?q?feat(cleanup):=20reclaim=20orphaned=20att?= =?UTF-8?q?achments=20=E2=80=94=20rows=20and=20store=20blobs=20(#3068)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PostAttachment's two FKs are both ON DELETE SET NULL, so a deleted post or artist left the row behind rather than taking it. Nothing ever pruned those rows, and nothing in the repo had ever unlinked a file under the attachment store — so both rows and bytes accumulated permanently, invisible to every existing diagnostic. Why a disk->DB reconciliation rather than a row sweep: the store is sha-addressed and idempotent, so ONE blob backs MANY rows. Deleting a row does not free its blob, and since the artist cascade (#3066) now deletes its attachment rows outright, a freed blob has no DB pointer left to find it by. Walking the store and asking "does any row still reference this sha?" catches orphans from every cause, including ones no future delete path will think to report. Preview and apply share `_orphan_attachment_conditions` (rule 93). The dry-run derives its surviving-sha set by NEGATING that same predicate, so it is honest about blobs the delete would free rather than counting them as still-referenced — the one place this was easy to get backwards, so it has its own parity test. Guards, each with a reason: - A blob is written before its row commits, so a just-stored file legitimately has no referencing row. Files under 6h are never judged — same guard and reasoning as ORPHAN_TEMP_MIN_AGE_HOURS. - `.partial` staging files belong to cleanup_orphaned_temp_files; skipped rather than raced. - The sha is parsed as the first 64 chars, not via Path.stem: store() takes the extension from the source filename, and a URL-encoded basename yields a multi-dot suffix that would make stem eat part of the sha. - A 900s walk budget reports partial=True instead of running to the task's hard limit (rule 89). - TASK_STUCK_THRESHOLD_MINUTES override at 30 (= time_limit 25 + 5). Without it a healthy 20-minute walk is phantom-flagged 'RecoverySweep' at the bare 5-min default — the #883 failure class; its invariant test is mirrored here. Defaults to the safe preview at both the task and the route, unlike the other maintenance triggers: this apply unlinks files. Operator-triggered only, never on a beat. Ships with its UI (rule 27): AttachmentReclaimCard in Cleanup → Duplicates & leftovers, built on the existing useMaintenanceTask/MaintenanceTile shapes, so a run survives navigating away. Surfaces files_failed and partial explicitly, since both change what the numbers mean. Also promotes humanBytes to utils/bytes.js — it was byte-identical in VideoDedupCard and GatedPurgeCard and this card would have been the third copy. The three divergent `formatBytes` helpers are deliberately left alone. Co-Authored-By: Claude Opus 5 (1M context) --- backend/app/api/admin.py | 16 ++ backend/app/services/cleanup_service.py | 152 +++++++++++++++ backend/app/tasks/admin.py | 28 +++ backend/app/tasks/maintenance.py | 6 + .../settings/AttachmentReclaimCard.vue | 127 +++++++++++++ .../components/settings/GatedPurgeCard.vue | 9 +- .../components/settings/VideoDedupCard.vue | 9 +- frontend/src/utils/bytes.js | 17 ++ frontend/src/views/CleanupView.vue | 7 +- tests/test_api_admin.py | 31 ++++ tests/test_cleanup_service.py | 174 ++++++++++++++++++ tests/test_maintenance.py | 14 ++ 12 files changed, 572 insertions(+), 18 deletions(-) create mode 100644 frontend/src/components/settings/AttachmentReclaimCard.vue create mode 100644 frontend/src/utils/bytes.js diff --git a/backend/app/api/admin.py b/backend/app/api/admin.py index e1ef468..9f676be 100644 --- a/backend/app/api/admin.py +++ b/backend/app/api/admin.py @@ -459,6 +459,22 @@ async def trigger_prune_missing_files(): return _queued(async_result) +@admin_bp.route("/maintenance/reclaim-attachments", methods=["POST"]) +async def trigger_reclaim_attachments(): + """Reclaim orphaned attachments (#3068). Body {"dry_run": bool}: dry_run + (the DEFAULT here) projects the orphan rows and unreferenced store blobs + without touching either; dry_run=false deletes the rows then unlinks every + blob no surviving row references. Maintenance queue; operator-triggered + only — never an unattended sweep, since the apply unlinks files. Returns the + Celery task id — poll /maintenance/task-result/ for the summary.""" + from ..tasks.admin import reclaim_orphaned_attachments_task + + body = await request.get_json(silent=True) or {} + dry_run = bool(body.get("dry_run", True)) # default to the SAFE preview + async_result = reclaim_orphaned_attachments_task.delay(dry_run=dry_run) + return _queued(async_result) + + @admin_bp.route("/maintenance/dedup-videos", methods=["POST"]) async def trigger_dedup_videos(): """Tier-1 video dedup (#871). Body {"dry_run": bool}: dry_run=true previews diff --git a/backend/app/services/cleanup_service.py b/backend/app/services/cleanup_service.py index d39cdb6..89afd51 100644 --- a/backend/app/services/cleanup_service.py +++ b/backend/app/services/cleanup_service.py @@ -1592,3 +1592,155 @@ def purge_gated_previews( "ledger_cleared": ledger_cleared, "posts_deleted": posts_deleted, } + + +# -- orphaned attachment reclamation --------------------------------------- +# PostAttachment's two FKs are both ON DELETE SET NULL, so a deleted post or +# artist leaves the row behind rather than taking it. Nothing ever pruned those +# rows, and nothing has ever unlinked a file under the attachment store — so +# both rows and bytes accumulated permanently and were invisible to every +# existing diagnostic. +# +# Why this is a DISK->DB reconciliation rather than a row sweep: the store is +# sha-addressed and idempotent (attachment_store.store), so ONE blob backs MANY +# rows. Deleting a row therefore does not free its blob, and — since the artist +# cascade now deletes its attachment rows outright — a freed blob has no DB +# pointer left to find it by. Walking the store and asking "does any row still +# reference this sha?" catches orphans from every cause, including ones no +# future delete path will think to report. + +# A blob is written by attachment_store.store BEFORE its row is inserted and +# committed, so a just-stored file legitimately has no referencing row for a +# moment. Same guard, same reasoning as ORPHAN_TEMP_MIN_AGE_HOURS in +# tasks/maintenance.py: never judge a file younger than this. +_ATTACHMENT_ORPHAN_MIN_AGE_HOURS = 6 + +# Wall-clock budget for the store walk (rule 89). A library with a large +# attachment store shouldn't be able to run this past its soft time limit; on +# exhaustion it reports partial=True and the operator re-runs to finish. +_ATTACHMENT_RECLAIM_BUDGET_SECONDS = 900 + +# The store names files ``. Parse the sha as the first 64 chars +# rather than via Path.stem: store() takes the extension straight from the +# source filename, and a URL-encoded basename yields a multi-dot "suffix" +# (see [[reference_url_encoded_basename_suffix]]) that would make stem eat part +# of the sha. Validating the 64 chars as hex also skips anything else in the +# tree that isn't a stored blob. +_SHA256_HEX_LEN = 64 + + +def _orphan_attachment_conditions() -> list: + """PostAttachment rows belonging to nothing: both FKs nulled by a deleted + post AND a deleted artist. A row with post_id NULL but an artist_id is the + deliberate filesystem-import case (importer._capture_attachment writes it + that way) and is NOT an orphan — it is still attributed.""" + return [ + PostAttachment.post_id.is_(None), + PostAttachment.artist_id.is_(None), + ] + + +def _is_sha_named(name: str) -> bool: + """True when `name` starts with a 64-char lowercase-hex sha256.""" + if len(name) < _SHA256_HEX_LEN: + return False + head = name[:_SHA256_HEX_LEN] + return all(c in "0123456789abcdef" for c in head) + + +def reclaim_orphaned_attachments( + session: Session, *, images_root: Path, dry_run: bool = False, +) -> dict: + """Prune unattributed PostAttachment rows, then unlink store blobs that no + surviving row references. + + Returns (same discovery keys either way, so the UI renders one shape): + {"rows": int, # orphan rows found / deleted + "files": int, # unreferenced blobs found / unlinked + "bytes": int, # their total size + "scanned": int, # blobs examined + "skipped_recent": int, # blobs under the min-age guard + "files_failed": int, # unlink raised (apply only) + "partial": bool} # walk hit the time budget + + dry_run computes exactly what the apply would do and mutates nothing — the + surviving-sha set is derived by NEGATING the same orphan predicate the + delete uses, so the preview cannot disagree with the apply (rule 93). + """ + started = time.monotonic() + orphan_conds = _orphan_attachment_conditions() + + if dry_run: + rows = session.execute( + select(func.count(PostAttachment.id)).where(*orphan_conds) + ).scalar_one() + else: + rows = session.execute( + delete(PostAttachment).where(*orphan_conds) + ).rowcount or 0 + session.commit() + + # Shas that still have a home. In the apply path the orphan rows are already + # gone, so `NOT orphan` is redundant but harmless; in the dry-run path it is + # what makes the projection honest about blobs the delete would free. One + # predicate, one query, both modes. + surviving_shas = set(session.execute( + select(PostAttachment.sha256).where(~and_(*orphan_conds)).distinct() + ).scalars()) + + root = Path(images_root) / "attachments" + cutoff = ( + datetime.now(UTC).timestamp() + - _ATTACHMENT_ORPHAN_MIN_AGE_HOURS * 3600 + ) + files = 0 + freed_bytes = 0 + scanned = 0 + skipped_recent = 0 + files_failed = 0 + partial = False + + if root.is_dir(): + for path in root.rglob("*"): + if time.monotonic() - started >= _ATTACHMENT_RECLAIM_BUDGET_SECONDS: + partial = True + break + # .partial staging files belong to cleanup_orphaned_temp_files — + # leave them alone rather than racing an in-flight store(). + if path.suffix in (".part", ".partial") or not path.is_file(): + continue + if not _is_sha_named(path.name): + continue + scanned += 1 + sha = path.name[:_SHA256_HEX_LEN] + if sha in surviving_shas: + continue + try: + st = path.stat() + if st.st_mtime >= cutoff: + skipped_recent += 1 + continue + size = st.st_size + if not dry_run: + path.unlink() + files += 1 + freed_bytes += size + except OSError as exc: + files_failed += 1 + log.warning("reclaim_orphaned_attachments: %s: %s", path, exc) + + if not dry_run and (rows or files): + log.info( + "attachment reclaim: %d orphan row(s) deleted, %d blob(s) unlinked " + "(%d bytes), %d failed, partial=%s", + rows, files, freed_bytes, files_failed, partial, + ) + return { + "rows": rows, + "files": files, + "bytes": freed_bytes, + "scanned": scanned, + "skipped_recent": skipped_recent, + "files_failed": files_failed, + "partial": partial, + } diff --git a/backend/app/tasks/admin.py b/backend/app/tasks/admin.py index 784b4da..73ed1d7 100644 --- a/backend/app/tasks/admin.py +++ b/backend/app/tasks/admin.py @@ -409,3 +409,31 @@ def rescan_series_suggestions_task(self, after_post_id: int = 0) -> dict: ) rescan_series_suggestions_task.delay(summary["resume_after_id"]) return summary + + +@celery.task( + name="backend.app.tasks.admin.reclaim_orphaned_attachments_task", + bind=True, + autoretry_for=(OperationalError, DBAPIError), + retry_backoff=15, retry_backoff_max=180, max_retries=1, + # The service stops walking at its own 900s budget and reports partial, so + # these limits are the backstop for a wedged filesystem (NFS stall), not the + # expected exit. Comfortably above the budget so a normal run always returns + # its summary rather than being killed mid-walk. + soft_time_limit=1200, time_limit=1500, # 20 min / 25 min +) +def reclaim_orphaned_attachments_task(self, dry_run: bool = True) -> dict: + """Reclaim unattributed PostAttachment rows and the store blobs nothing + references any more (#3068). dry_run (the default) returns the projection + without touching rows or files; apply deletes the orphan rows, then unlinks + every blob no surviving row references. + + Defaults to the SAFE preview — unlike the other tasks here, whose apply is + reversible-ish or scoped; this one deletes files. Operator-triggered only, + never on a beat: an unattended sweep that unlinks blobs is not something to + run without someone reading the projection first.""" + SessionLocal = _sync_session_factory() + with SessionLocal() as session: + return cleanup_service.reclaim_orphaned_attachments( + session, images_root=IMAGES_ROOT, dry_run=dry_run, + ) diff --git a/backend/app/tasks/maintenance.py b/backend/app/tasks/maintenance.py index 67432c2..10b01ec 100644 --- a/backend/app/tasks/maintenance.py +++ b/backend/app/tasks/maintenance.py @@ -173,6 +173,12 @@ TASK_STUCK_THRESHOLD_MINUTES: dict[str, int] = { # task-name override beats the queue threshold whatever queue the row records # (it recorded 'default' before the celery_signals fix → download). 65 = 60+5. "backend.app.tasks.external.fetch_external_link": 65, + # Attachment reclaim walks the whole sha-addressed store; the service caps + # itself at a 900s budget and reports partial, but the task's hard limit is + # 25 min for a wedged filesystem (NFS stall). Same phantom-flag class as the + # external-fetch entry above — without an override a healthy in-flight walk + # is swept 'RecoverySweep' at the bare 5-min default. 30 = 25 + 5. + "backend.app.tasks.admin.reclaim_orphaned_attachments_task": 30, } diff --git a/frontend/src/components/settings/AttachmentReclaimCard.vue b/frontend/src/components/settings/AttachmentReclaimCard.vue new file mode 100644 index 0000000..9e7824f --- /dev/null +++ b/frontend/src/components/settings/AttachmentReclaimCard.vue @@ -0,0 +1,127 @@ + + + diff --git a/frontend/src/components/settings/GatedPurgeCard.vue b/frontend/src/components/settings/GatedPurgeCard.vue index e1e0a30..8776533 100644 --- a/frontend/src/components/settings/GatedPurgeCard.vue +++ b/frontend/src/components/settings/GatedPurgeCard.vue @@ -102,6 +102,7 @@ import { computed, ref } from 'vue' import { useMaintenanceTask } from '../../composables/useMaintenanceTask.js' +import { humanBytes } from '../../utils/bytes.js' import MaintenanceTile from '../common/MaintenanceTile.vue' import QueueStatusBar from './QueueStatusBar.vue' @@ -122,14 +123,6 @@ const summaryType = computed(() => { return summary.value && summary.value.matched > 0 ? 'info' : 'success' }) -function humanBytes (n) { - const b = Number(n || 0) - if (b >= 1 << 30) return (b / (1 << 30)).toFixed(1) + ' GB' - if (b >= 1 << 20) return (b / (1 << 20)).toFixed(1) + ' MB' - if (b >= 1 << 10) return (b / (1 << 10)).toFixed(1) + ' KB' - return b + ' B' -} - // The confirm dialog gates the destructive apply; close it, then run. function apply () { confirmOpen.value = false diff --git a/frontend/src/components/settings/VideoDedupCard.vue b/frontend/src/components/settings/VideoDedupCard.vue index 9e26862..219b5c2 100644 --- a/frontend/src/components/settings/VideoDedupCard.vue +++ b/frontend/src/components/settings/VideoDedupCard.vue @@ -78,6 +78,7 @@ import { computed, ref } from 'vue' import { useMaintenanceTask } from '../../composables/useMaintenanceTask.js' +import { humanBytes } from '../../utils/bytes.js' import MaintenanceTile from '../common/MaintenanceTile.vue' import QueueStatusBar from './QueueStatusBar.vue' @@ -98,14 +99,6 @@ const summaryType = computed(() => { return summary.value && summary.value.redundant > 0 ? 'info' : 'success' }) -function humanBytes (n) { - const b = Number(n || 0) - if (b >= 1 << 30) return (b / (1 << 30)).toFixed(1) + ' GB' - if (b >= 1 << 20) return (b / (1 << 20)).toFixed(1) + ' MB' - if (b >= 1 << 10) return (b / (1 << 10)).toFixed(1) + ' KB' - return b + ' B' -} - // The confirm dialog gates the destructive apply; close it, then run. function apply () { confirmOpen.value = false diff --git a/frontend/src/utils/bytes.js b/frontend/src/utils/bytes.js new file mode 100644 index 0000000..c83a916 --- /dev/null +++ b/frontend/src/utils/bytes.js @@ -0,0 +1,17 @@ +// Human-readable byte sizes for maintenance summaries ("2.4 GB reclaimable"). +// +// Promoted out of the cleanup cards, which had grown byte-identical private +// copies (VideoDedupCard, GatedPurgeCard) and were about to grow a third for +// the attachment reclaim. Binary units (1 KB = 1024 B) — these numbers come +// from st_size / SUM(size_bytes), so they describe disk, not marketing. +// +// NOT the same shape as the `formatBytes` helpers in SystemStatsCards, +// BackupRunsTable and PostCard — those differ in units, precision and +// zero-handling. Left alone deliberately rather than force-fitted here. +export function humanBytes (n) { + const b = Number(n || 0) + if (b >= 1 << 30) return (b / (1 << 30)).toFixed(1) + ' GB' + if (b >= 1 << 20) return (b / (1 << 20)).toFixed(1) + ' MB' + if (b >= 1 << 10) return (b / (1 << 10)).toFixed(1) + ' KB' + return b + ' B' +} diff --git a/frontend/src/views/CleanupView.vue b/frontend/src/views/CleanupView.vue index 999d5fe..72a3c6f 100644 --- a/frontend/src/views/CleanupView.vue +++ b/frontend/src/views/CleanupView.vue @@ -19,14 +19,16 @@
-

Duplicates & posts

+

Duplicates & leftovers

- Tidy post records, duplicates and locked-preview leftovers. + Tidy post records, duplicates, locked-preview leftovers and attachments + that outlived what they belonged to.

+
@@ -60,6 +62,7 @@ import SingleColorAuditCard from '../components/cleanup/SingleColorAuditCard.vue import PostMaintenanceCard from '../components/settings/PostMaintenanceCard.vue' import VideoDedupCard from '../components/settings/VideoDedupCard.vue' import GatedPurgeCard from '../components/settings/GatedPurgeCard.vue' +import AttachmentReclaimCard from '../components/settings/AttachmentReclaimCard.vue' import TagMaintenanceCard from '../components/settings/TagMaintenanceCard.vue' import DangerZoneCard from '../components/settings/DangerZoneCard.vue' diff --git a/tests/test_api_admin.py b/tests/test_api_admin.py index 230e27e..0ff580f 100644 --- a/tests/test_api_admin.py +++ b/tests/test_api_admin.py @@ -677,3 +677,34 @@ async def test_reset_content_tagging_apply_requires_confirm_token(client, db): ) assert resp.status_code == 200 assert (await resp.get_json())["deleted"] == 1 + + +@pytest.mark.asyncio +async def test_trigger_reclaim_attachments_defaults_to_preview(client, monkeypatch): + """Unlike the other maintenance triggers, this one's apply unlinks FILES — + so an empty body must mean preview, not apply.""" + from backend.app.tasks import admin as admin_tasks + + calls = [] + monkeypatch.setattr( + admin_tasks.reclaim_orphaned_attachments_task, "delay", _fake_delay(calls) + ) + resp = await client.post("/api/admin/maintenance/reclaim-attachments", json={}) + assert resp.status_code == 202 + assert (await resp.get_json())["task_id"] == "task-xyz" + assert calls[0][1] == {"dry_run": True} + + +@pytest.mark.asyncio +async def test_trigger_reclaim_attachments_threads_apply(client, monkeypatch): + from backend.app.tasks import admin as admin_tasks + + calls = [] + monkeypatch.setattr( + admin_tasks.reclaim_orphaned_attachments_task, "delay", _fake_delay(calls) + ) + resp = await client.post( + "/api/admin/maintenance/reclaim-attachments", json={"dry_run": False}, + ) + assert resp.status_code == 202 + assert calls[0][1] == {"dry_run": False} diff --git a/tests/test_cleanup_service.py b/tests/test_cleanup_service.py index 7303a2f..d98c212 100644 --- a/tests/test_cleanup_service.py +++ b/tests/test_cleanup_service.py @@ -5,6 +5,7 @@ side effects use tmp_path. Assertions on mutated rows use COLUMN SELECTS per reference_async_coredml_test_assertions — never re-read ORM attributes after a service mutates and re-fetches. """ +import os from datetime import UTC, datetime import pytest @@ -1081,3 +1082,176 @@ def test_reconcile_preserves_from_attachment_on_provenance_collision(db_sync, tm .where(ImageProvenance.image_record_id == img_id) ).all() assert rows == [(native_id, att_id)] + + +# --- reclaim_orphaned_attachments ----------------------------------- + + +def _store_blob(root, sha, *, ext=".pdf", age_hours=48, data=b"blob"): + """Write a file into the sha-addressed attachment store, aged past the + min-age guard by default.""" + d = root / "attachments" / sha[:3] + d.mkdir(parents=True, exist_ok=True) + p = d / f"{sha}{ext}" + p.write_bytes(data) + old = datetime.now(UTC).timestamp() - age_hours * 3600 + os.utime(p, (old, old)) + return p + + +def _attachment(db_sync, *, sha, post=None, artist=None): + att = PostAttachment( + post_id=post.id if post else None, + artist_id=artist.id if artist else None, + sha256=sha, path=f"/store/{sha[:3]}/f.pdf", + original_filename="f.pdf", ext=".pdf", size_bytes=4, + ) + db_sync.add(att) + db_sync.flush() + return att + + +def test_reclaim_attachments_dry_run_projects_without_mutating(db_sync, tmp_path): + a = _make_artist(db_sync, slug="recl-dry") + p = Post(artist_id=a.id, external_post_id="rd-1") + db_sync.add(p) + db_sync.flush() + kept_sha, orphan_sha = "aa11".ljust(64, "0"), "bb22".ljust(64, "0") + _attachment(db_sync, sha=kept_sha, post=p, artist=a) + _attachment(db_sync, sha=orphan_sha) # both FKs NULL → orphan + db_sync.commit() + kept_blob = _store_blob(tmp_path, kept_sha) + orphan_blob = _store_blob(tmp_path, orphan_sha) + + result = cleanup_service.reclaim_orphaned_attachments( + db_sync, images_root=tmp_path, dry_run=True, + ) + assert result["rows"] == 1 + assert result["files"] == 1 + assert result["bytes"] == orphan_blob.stat().st_size + + # Nothing actually happened. + assert kept_blob.exists() and orphan_blob.exists() + assert db_sync.execute( + select(func.count(PostAttachment.id)) + ).scalar_one() == 2 + + +def test_reclaim_attachments_apply_deletes_rows_and_unlinks_blobs(db_sync, tmp_path): + a = _make_artist(db_sync, slug="recl-apply") + p = Post(artist_id=a.id, external_post_id="ra-1") + db_sync.add(p) + db_sync.flush() + kept_sha, orphan_sha = "cc33".ljust(64, "0"), "dd44".ljust(64, "0") + _attachment(db_sync, sha=kept_sha, post=p, artist=a) + _attachment(db_sync, sha=orphan_sha) + db_sync.commit() + kept_blob = _store_blob(tmp_path, kept_sha) + orphan_blob = _store_blob(tmp_path, orphan_sha) + + result = cleanup_service.reclaim_orphaned_attachments( + db_sync, images_root=tmp_path, dry_run=False, + ) + assert result["rows"] == 1 + assert result["files"] == 1 + + assert kept_blob.exists() # still referenced + assert not orphan_blob.exists() # nothing points at it any more + surviving = db_sync.execute(select(PostAttachment.sha256)).scalars().all() + assert surviving == [kept_sha] + + +def test_reclaim_attachments_preview_matches_apply(db_sync, tmp_path): + """Rule 93 — the dry-run's numbers are what the apply does. The projection + has to negate the orphan predicate to be honest about blobs the delete is + about to free, so this is the assertion that catches getting that backwards. + """ + orphan_sha = "ee55".ljust(64, "0") + _attachment(db_sync, sha=orphan_sha) + db_sync.commit() + _store_blob(tmp_path, orphan_sha) + + projected = cleanup_service.reclaim_orphaned_attachments( + db_sync, images_root=tmp_path, dry_run=True, + ) + applied = cleanup_service.reclaim_orphaned_attachments( + db_sync, images_root=tmp_path, dry_run=False, + ) + for key in ("rows", "files", "bytes"): + assert projected[key] == applied[key], key + assert applied["rows"] == 1 and applied["files"] == 1 + + +def test_reclaim_attachments_keeps_shared_blob_while_any_row_remains(db_sync, tmp_path): + """The refcount case this whole sweep exists for: one sha-addressed blob + backs several rows, so deleting SOME of them must not free the file.""" + a = _make_artist(db_sync, slug="recl-shared") + p = Post(artist_id=a.id, external_post_id="rs-1") + db_sync.add(p) + db_sync.flush() + sha = "ff66".ljust(64, "0") + _attachment(db_sync, sha=sha, post=p, artist=a) # attributed — survives + _attachment(db_sync, sha=sha) # orphan — deleted + db_sync.commit() + blob = _store_blob(tmp_path, sha) + + result = cleanup_service.reclaim_orphaned_attachments( + db_sync, images_root=tmp_path, dry_run=False, + ) + assert result["rows"] == 1 # the orphan row went + assert result["files"] == 0 # the blob did NOT + assert blob.exists() + + +def test_reclaim_attachments_spares_filesystem_import_rows(db_sync, tmp_path): + """post_id NULL with an artist_id is the deliberate filesystem-import shape + (importer._capture_attachment), not an orphan — it is still attributed.""" + a = _make_artist(db_sync, slug="recl-fsimport") + sha = "1177".ljust(64, "0") + _attachment(db_sync, sha=sha, artist=a) # post NULL, artist set + db_sync.commit() + blob = _store_blob(tmp_path, sha) + + result = cleanup_service.reclaim_orphaned_attachments( + db_sync, images_root=tmp_path, dry_run=False, + ) + assert result["rows"] == 0 + assert result["files"] == 0 + assert blob.exists() + assert db_sync.execute( + select(func.count(PostAttachment.id)) + ).scalar_one() == 1 + + +def test_reclaim_attachments_skips_recent_and_staging_files(db_sync, tmp_path): + """A blob is written BEFORE its row commits, so a just-stored file with no + row is in-flight, not orphaned. `.partial` staging files belong to + cleanup_orphaned_temp_files and must be left alone either way.""" + fresh_sha, staged_sha = "2288".ljust(64, "0"), "3399".ljust(64, "0") + fresh = _store_blob(tmp_path, fresh_sha, age_hours=0) + staged = _store_blob(tmp_path, staged_sha, ext=".pdf.partial") + db_sync.commit() + + result = cleanup_service.reclaim_orphaned_attachments( + db_sync, images_root=tmp_path, dry_run=False, + ) + assert result["files"] == 0 + assert result["skipped_recent"] == 1 + assert fresh.exists() and staged.exists() + + +def test_reclaim_attachments_ignores_non_sha_named_files(db_sync, tmp_path): + """The walk must only judge files it can identify as store blobs — anything + else under the root is none of its business.""" + d = tmp_path / "attachments" / "zzz" + d.mkdir(parents=True) + stray = d / "notes.txt" + stray.write_text("not a blob") + old = datetime.now(UTC).timestamp() - 48 * 3600 + os.utime(stray, (old, old)) + + result = cleanup_service.reclaim_orphaned_attachments( + db_sync, images_root=tmp_path, dry_run=False, + ) + assert result["files"] == 0 + assert stray.exists() diff --git a/tests/test_maintenance.py b/tests/test_maintenance.py index 1972667..5f7e3ed 100644 --- a/tests/test_maintenance.py +++ b/tests/test_maintenance.py @@ -778,3 +778,17 @@ def test_vacuum_analyze_runs_over_high_churn_tables(): result = vacuum_analyze.apply().get() assert result["vacuumed"] == list(VACUUM_TABLES) + + +def test_reclaim_attachments_stuck_threshold_exceeds_hard_time_limit(): + """#883's invariant, applied to the attachment reclaim: a task whose stall + threshold is under its own hard limit gets phantom-flagged 'RecoverySweep' + while it is still healthily running.""" + from backend.app.tasks.admin import reclaim_orphaned_attachments_task + from backend.app.tasks.maintenance import TASK_STUCK_THRESHOLD_MINUTES + + hard_minutes = reclaim_orphaned_attachments_task.time_limit / 60 + override = TASK_STUCK_THRESHOLD_MINUTES[ + "backend.app.tasks.admin.reclaim_orphaned_attachments_task" + ] + assert override >= hard_minutes From ddf896078c3a25cdcffe7652b2897fac4db834bc Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 27 Aug 2026 07:24:08 -0400 Subject: [PATCH 04/16] refactor(platforms): retire deviantart end-to-end (#3069) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Executes the 2026-07-05 product decision (FC downloaders = art-dedicated services only), which removed Twitter/X and Bluesky but left deviantart fully wired for seven weeks — the half-retired state rule 22 exists to prevent. Removed: the PlatformInfo module and its registry entry, the gallery-dl extractor block, extension_service's artist-page pattern, the extension's PLATFORMS + PLATFORM_ARTIST_PATTERNS entries, its manifest host permission and content-script match, the frontend icon/colour/label, and the operator- facing "supported platforms" list that still advertised it. Two judgment calls, both recorded in migration 0088: * existing `source` rows are DISABLED, not deleted. The row is the only record of the artist's DeviantArt URL. Disabling is also required for correctness rather than tidiness: with the platform unregistered the download path falls through to gallery-dl, which carries its OWN deviantart extractor, so an enabled row would have kept downloading from a dropped platform. * the `credential` row IS deleted — a live session cookie for a site FC will never call again. Adds the invariant whose absence is why manifest.json drifted in the first place: nothing tied its domain lists back to the platform table. The extension suite now asserts both directions, plus that no host permission belongs to an unclaimed domain (`*://*/*` exempted — FC is self-hosted at an operator-chosen URL the extension cannot enumerate). Extension version 1.0.10 -> 1.0.11: ci.yml's guard hard-fails a packaged extension change without a bump. No release is cut — build.yml's sign-extension job only runs on main. Co-Authored-By: Claude Opus 5 --- alembic/versions/0088_retire_deviantart.py | 70 ++++++++++++++++ backend/app/services/credential_service.py | 2 +- backend/app/services/download_backends.py | 5 +- backend/app/services/extension_service.py | 6 -- backend/app/services/gallery_dl.py | 14 +--- backend/app/services/platforms/__init__.py | 7 +- backend/app/services/platforms/base.py | 2 +- backend/app/services/platforms/deviantart.py | 23 ------ extension/README.md | 6 +- extension/lib/platforms.js | 11 --- extension/manifest.json | 4 +- extension/package.json | 2 +- extension/test/platforms.spec.js | 79 +++++++++++++++++-- .../settings/BrowserExtensionCard.vue | 2 +- frontend/src/utils/platformColor.js | 11 ++- tests/test_api_extension.py | 18 ++++- tests/test_api_platforms.py | 5 +- tests/test_artist_directory_service.py | 2 +- tests/test_download_backends.py | 2 +- tests/test_platform_lock.py | 2 +- tests/test_platforms_registry.py | 13 ++- tests/test_post_feed_service.py | 4 +- tests/test_source_service.py | 6 +- 23 files changed, 202 insertions(+), 94 deletions(-) create mode 100644 alembic/versions/0088_retire_deviantart.py delete mode 100644 backend/app/services/platforms/deviantart.py diff --git a/alembic/versions/0088_retire_deviantart.py b/alembic/versions/0088_retire_deviantart.py new file mode 100644 index 0000000..171e722 --- /dev/null +++ b/alembic/versions/0088_retire_deviantart.py @@ -0,0 +1,70 @@ +"""retire deviantart (#3069) — quiesce the rows the dropped platform leaves behind + +`deviantart` is no longer a registered platform, so nothing can create or edit a +source with that key any more. Existing rows are a different question, and the +two tables want opposite treatment: + +* `source` rows are DISABLED, not deleted. The row is the only place the + artist's DeviantArt URL is recorded, and losing it is unrecoverable — whereas + a disabled row is visible in the UI and reversible by hand. Disabling is also + required for correctness, not just tidiness: with the platform unregistered + the download path falls through to gallery-dl, which carries its OWN built-in + deviantart extractor, so an enabled row would have gone on downloading from a + platform the product dropped. + +* the `credential` row IS deleted. It is an encrypted DeviantArt session cookie + for a site FC will never call again — keeping a live credential we have no + use for is strictly worse than dropping it, and re-exporting from the browser + is the recovery path if that judgment is ever wrong. + +A no-op on an instance that never had a DeviantArt source, which is the +expected case. + +Revision ID: 0088 +Revises: 0087 +Create Date: 2026-08-27 +""" +from typing import Sequence, Union + +import sqlalchemy as sa +from alembic import op + +revision: str = "0088" +down_revision: Union[str, None] = "0087" +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + +_RETIRED = "deviantart" + +# Written into last_error so the disabled row explains itself in the UI rather +# than looking like an unexplained toggle someone flipped. +_REASON = ( + "Platform retired: FabledCurator no longer supports DeviantArt " + "(dropped 2026-08-27, #3069). This source was disabled automatically; " + "the URL is kept for reference and the row can be deleted by hand." +) + + +def upgrade() -> None: + conn = op.get_bind() + disabled = conn.execute( + sa.text( + "UPDATE source SET enabled = false, last_error = :reason " + "WHERE platform = :p AND enabled = true" + ), + {"reason": _REASON, "p": _RETIRED}, + ).rowcount + creds = conn.execute( + sa.text("DELETE FROM credential WHERE platform = :p"), {"p": _RETIRED} + ).rowcount + print(f"0088: disabled {disabled} deviantart source(s), removed {creds} credential(s)") + + +def downgrade() -> None: + # Re-enabling is deliberately NOT done: the platform is gone from the + # registry, so a re-enabled source would still have no backend to run on. + # Clearing the stamped reason is the only half that means anything. + op.get_bind().execute( + sa.text("UPDATE source SET last_error = NULL WHERE platform = :p AND last_error = :reason"), + {"p": _RETIRED, "reason": _REASON}, + ) diff --git a/backend/app/services/credential_service.py b/backend/app/services/credential_service.py index a8f6d8c..42c8a54 100644 --- a/backend/app/services/credential_service.py +++ b/backend/app/services/credential_service.py @@ -181,7 +181,7 @@ def _augment_cookies(platform: str, netscape: str) -> str: """Delegate to the platform's `augment_cookies` hook if one is registered (subscribestar, hentaifoundry, etc. — see `services/platforms/.py`). No-op when the platform doesn't - register a hook (Patreon, DeviantArt). Centralizing the + register a hook (Patreon, Discord). Centralizing the quirks-per-platform in the platforms package means adding a new platform's cookie quirks doesn't require touching this file.""" info = PLATFORMS.get(platform) diff --git a/backend/app/services/download_backends.py b/backend/app/services/download_backends.py index c6e1d2e..49c510a 100644 --- a/backend/app/services/download_backends.py +++ b/backend/app/services/download_backends.py @@ -31,9 +31,8 @@ from .pixiv_ingester import PixivIngester from .subscribestar_ingester import SubscribeStarIngester # Platforms whose download + verify go through the native ingester rather than -# gallery-dl. gallery-dl still serves the rest (hentaifoundry, discord, -# deviantart — the latter slated for retirement, not migration) until they -# migrate too. +# gallery-dl. gallery-dl still serves the rest (hentaifoundry, discord) until +# they migrate too. NATIVE_INGESTER_PLATFORMS = frozenset({"patreon", "subscribestar", "pixiv"}) # Mirrors patreon_resolver._CAMPAIGNS_URL — surfaced in resolution-failure diff --git a/backend/app/services/extension_service.py b/backend/app/services/extension_service.py index 25cb03a..7a629ab 100644 --- a/backend/app/services/extension_service.py +++ b/backend/app/services/extension_service.py @@ -55,12 +55,6 @@ _PLATFORM_PATTERNS: list[tuple[str, re.Pattern[str]]] = [ r"^https?://(?:www\.)?hentai-foundry\.com/user/(?P[^/?#]+)", re.IGNORECASE, )), - ("deviantart", re.compile( - r"^https?://(?:www\.)?deviantart\.com/" - r"(?!home$|watch\b|tag\b|browse\b)" - r"(?P[^/?#]+)/?$", - re.IGNORECASE, - )), ("pixiv", re.compile( r"^https?://(?:www\.)?pixiv\.net/(?:en/)?users/(?P\d+)", re.IGNORECASE, diff --git a/backend/app/services/gallery_dl.py b/backend/app/services/gallery_dl.py index 71d13af..78abd9b 100644 --- a/backend/app/services/gallery_dl.py +++ b/backend/app/services/gallery_dl.py @@ -299,8 +299,9 @@ class GalleryDLService: # (services/patreon_ingester.py), not gallery-dl. PLATFORM_DEFAULTS = { # subscribestar removed — native-ingester platform now (#71); pixiv - # removed likewise (#129). The remaining entries are the gallery-dl - # platforms not yet migrated. + # removed likewise (#129); deviantart removed at #3069 as a dropped + # platform, not a migrated one. The remaining entries are the + # gallery-dl platforms not yet migrated. "hentaifoundry": { "content_types": ["all"], "directory": [], @@ -316,15 +317,6 @@ class GalleryDLService: "reactions": False, "threads": True, }, - "deviantart": { - "content_types": ["all"], - "directory": [], - "filename": "{index:>03}_{title[:50]}.{extension}", - "flat": True, - "original": True, - "mature": True, - "metadata": True, - }, } def __init__( diff --git a/backend/app/services/platforms/__init__.py b/backend/app/services/platforms/__init__.py index 822276a..be94f3d 100644 --- a/backend/app/services/platforms/__init__.py +++ b/backend/app/services/platforms/__init__.py @@ -8,9 +8,10 @@ PLATFORMS below. Sidecar parsing, cookie materialization, and Lifted from GallerySubscriber's ~/Nextcloud/Projects/GallerySubscriber/backend/app/api/platforms.py -and ~/.../extension/lib/platforms.js. Six platforms; auth_type and +and ~/.../extension/lib/platforms.js. Five platforms; auth_type and URL patterns match GS exactly so the existing browser extension -hits FC unmodified. +hits FC unmodified. deviantart was dropped at #3069 (2026-08-27) — +FC downloaders are art-dedicated services only. """ from .base import ( @@ -18,7 +19,6 @@ from .base import ( DEFAULT_EXTERNAL_POST_ID_KEYS, PlatformInfo, ) -from .deviantart import INFO as _DEVIANTART from .discord import INFO as _DISCORD from .hentaifoundry import INFO as _HENTAIFOUNDRY from .patreon import INFO as _PATREON @@ -33,7 +33,6 @@ PLATFORMS: dict[str, PlatformInfo] = { _HENTAIFOUNDRY, _DISCORD, _PIXIV, - _DEVIANTART, ) } diff --git a/backend/app/services/platforms/base.py b/backend/app/services/platforms/base.py index 737b529..ce6ae48 100644 --- a/backend/app/services/platforms/base.py +++ b/backend/app/services/platforms/base.py @@ -63,7 +63,7 @@ class PlatformInfo: # Synthesize a post permalink from sidecar data. Required when # gallery-dl's `url` field is the file/CDN URL rather than the post # permalink (subscribestar/pixiv/hf/discord). None = trust the bare - # `url` field (patreon, deviantart). + # `url` field (patreon). derive_post_url: Callable[[dict], str | None] | None = None # Post-process the materialized cookies.txt for gallery-dl. Used by diff --git a/backend/app/services/platforms/deviantart.py b/backend/app/services/platforms/deviantart.py deleted file mode 100644 index e41fc3b..0000000 --- a/backend/app/services/platforms/deviantart.py +++ /dev/null @@ -1,23 +0,0 @@ -"""DeviantArt — no exercised quirks yet. - -No operator-owned DeviantArt archive existed at the 2026-05-27 sidecar -audit, so we don't know yet whether DA's gallery-dl sidecars are -well-behaved or have their own quirks. When DA gets exercised for the -first time, add `derive_post_url` / `augment_cookies` here as needed. -""" - -from .base import GD_DEFAULTS, PlatformInfo - -INFO = PlatformInfo( - key="deviantart", - name="DeviantArt", - description="Download artwork from DeviantArt artists", - auth_type="cookies", - requires_auth=False, - url_pattern=r"^https?://(www\.)?deviantart\.com/", - url_examples=[ - "https://www.deviantart.com/example-artist", - "https://www.deviantart.com/example-artist/gallery", - ], - default_config={**GD_DEFAULTS, "content_types": ["gallery"]}, -) diff --git a/extension/README.md b/extension/README.md index 5220623..e780d95 100644 --- a/extension/README.md +++ b/extension/README.md @@ -1,9 +1,9 @@ # FabledCurator Firefox Extension Self-hosted Firefox extension that pushes session cookies from supported -platforms (Patreon, SubscribeStar, Hentai-Foundry, Discord, Pixiv, -DeviantArt) into FabledCurator, and lets you add a creator as a Source -from their page in one click. +platforms (Patreon, SubscribeStar, Hentai-Foundry, Discord, Pixiv) +into FabledCurator, and lets you add a creator as a Source from their +page in one click. ## Install (operator) diff --git a/extension/lib/platforms.js b/extension/lib/platforms.js index 2f37d93..c3f0c5a 100644 --- a/extension/lib/platforms.js +++ b/extension/lib/platforms.js @@ -68,16 +68,6 @@ const PLATFORMS = { urlPattern: /^https?:\/\/(www\.)?pixiv\.net/, note: 'Click to authenticate via OAuth', }, - deviantart: { - name: 'DeviantArt', - domains: ['.deviantart.com', 'www.deviantart.com', 'deviantart.com'], - authType: 'cookies', - color: '#05CC47', - urlPattern: /^https?:\/\/(www\.)?deviantart\.com/, - // DA's logged-in-only endpoints sit behind their internal _napi - // namespace which shifts; skipping verify until a stable check - // surfaces. Same posture as SubscribeStar. - }, }; /** @@ -98,7 +88,6 @@ const PLATFORM_ARTIST_PATTERNS = { patreon: /^https?:\/\/(www\.)?patreon\.com\/(?:cw\/|c\/)?(?!(?:home|search|messages|notifications|library|settings|posts)(?:[\/?#]|$))[^/?#]+/i, subscribestar: /^https?:\/\/(www\.)?subscribestar\.(com|adult)\/(?!feed$|messages$|library$)[^/?#]+\/?$/i, hentaifoundry: /^https?:\/\/(www\.)?hentai-foundry\.com\/user\/[^/?#]+/i, - deviantart: /^https?:\/\/(www\.)?deviantart\.com\/(?!home$|watch\b|tag\b|browse\b)[^/?#]+\/?$/i, pixiv: /^https?:\/\/(www\.)?pixiv\.net\/(en\/)?users\/\d+/i, }; diff --git a/extension/manifest.json b/extension/manifest.json index e763e3f..b488988 100644 --- a/extension/manifest.json +++ b/extension/manifest.json @@ -1,7 +1,7 @@ { "manifest_version": 3, "name": "FabledCurator", - "version": "1.0.10", + "version": "1.0.11", "description": "Export cookies from supported platforms to FabledCurator and add creators as sources in one click.", "browser_specific_settings": { @@ -33,7 +33,6 @@ "*://*.hentai-foundry.com/*", "*://*.discord.com/*", "*://*.pixiv.net/*", - "*://*.deviantart.com/*", "*://app-api.pixiv.net/*", "*://oauth.secure.pixiv.net/*", "*://*/*" @@ -61,7 +60,6 @@ "*://*.subscribestar.com/*", "*://*.subscribestar.adult/*", "*://*.hentai-foundry.com/*", - "*://*.deviantart.com/*", "*://*.pixiv.net/*" ], "js": ["lib/platforms.js", "content/content-script.js"], diff --git a/extension/package.json b/extension/package.json index 66fa304..638582e 100644 --- a/extension/package.json +++ b/extension/package.json @@ -1,6 +1,6 @@ { "name": "fabledcurator-extension", - "version": "1.0.10", + "version": "1.0.11", "private": true, "description": "Firefox extension for FabledCurator", "comment_ignore_files": "The --ignore-files list comes from scripts/packaging.sh, the single source of truth shared with ci.yml's guard and the derived-version patch count. `set -f` is REQUIRED before the substitution: without it the shell globs `test/**` against the working tree and silently narrows the pattern to whatever files happen to exist.", diff --git a/extension/test/platforms.spec.js b/extension/test/platforms.spec.js index deb9f97..df1e53a 100644 --- a/extension/test/platforms.spec.js +++ b/extension/test/platforms.spec.js @@ -1,6 +1,12 @@ import { describe, it, expect } from 'vitest' +import { readFileSync } from 'node:fs' +import { fileURLToPath } from 'node:url' +import path from 'node:path' import { loadLib } from './helpers/loadLib.js' +const EXT_DIR = path.join(path.dirname(fileURLToPath(import.meta.url)), '..') +const manifest = JSON.parse(readFileSync(path.join(EXT_DIR, 'manifest.json'), 'utf8')) + const { getPlatformFromUrl, isArtistPage, PLATFORMS, PLATFORM_ARTIST_PATTERNS } = loadLib( 'platforms.js', ['getPlatformFromUrl', 'isArtistPage', 'PLATFORMS', 'PLATFORM_ARTIST_PATTERNS'] @@ -13,7 +19,6 @@ describe('getPlatformFromUrl', () => { expect(getPlatformFromUrl('https://www.hentai-foundry.com/user/someone')).toBe('hentaifoundry') expect(getPlatformFromUrl('https://discord.com/channels/@me')).toBe('discord') expect(getPlatformFromUrl('https://www.pixiv.net/en/users/123')).toBe('pixiv') - expect(getPlatformFromUrl('https://www.deviantart.com/someone')).toBe('deviantart') }) it('accepts http as well as https, with or without www', () => { @@ -26,6 +31,15 @@ describe('getPlatformFromUrl', () => { expect(getPlatformFromUrl('https://not-patreon.com/Atole')).toBe(null) expect(getPlatformFromUrl('')).toBe(null) }) + + it('returns null for deviantart, retired at #3069', () => { + // The 2026-07-05 product decision (FC downloaders = art-dedicated services + // only) left deviantart wired for seven weeks. Asserting the negative is + // what keeps a partial retirement from being re-completed by accident. + expect(getPlatformFromUrl('https://www.deviantart.com/someone')).toBe(null) + expect(PLATFORMS.deviantart).toBeUndefined() + expect(PLATFORM_ARTIST_PATTERNS.deviantart).toBeUndefined() + }) }) describe('isArtistPage', () => { @@ -72,12 +86,6 @@ describe('isArtistPage', () => { expect(isArtistPage('https://www.pixiv.net/en/artworks/999', 'pixiv')).toBe(false) }) - it('excludes DeviantArt navigation roots', () => { - expect(isArtistPage('https://www.deviantart.com/someone', 'deviantart')).toBe(true) - expect(isArtistPage('https://www.deviantart.com/home', 'deviantart')).toBe(false) - expect(isArtistPage('https://www.deviantart.com/watch', 'deviantart')).toBe(false) - }) - it('returns false for a platform with no artist pattern (discord)', () => { expect(isArtistPage('https://discord.com/channels/@me', 'discord')).toBe(false) }) @@ -116,7 +124,6 @@ describe('platform table integrity', () => { patreon: 'https://www.patreon.com/cw/Atole', subscribestar: 'https://subscribestar.adult/someone', hentaifoundry: 'https://www.hentai-foundry.com/user/someone', - deviantart: 'https://www.deviantart.com/someone', pixiv: 'https://www.pixiv.net/en/users/12345' } for (const [key, url] of Object.entries(samples)) { @@ -125,3 +132,59 @@ describe('platform table integrity', () => { } }) }) + +describe('manifest.json agrees with the platform table', () => { + // #3069: deviantart was dropped from the product in July but survived in + // manifest.json until late August, because NOTHING tied the manifest's + // domain lists back to PLATFORMS. These two specs are that tie. Both + // directions matter: a stale match ships host access the product decided + // not to use, and a missing one silently kills the Add-to-FC button. + const matches = manifest.content_scripts[0].matches + // '*://*.patreon.com/*' -> '.patreon.com', the form PLATFORMS.domains uses. + const hostOf = (m) => m.replace(/^\*:\/\/\*/, '').replace(/\/\*$/, '') + + it('injects the content script only on domains a platform claims', () => { + for (const m of matches) { + const host = hostOf(m) + const owner = Object.entries(PLATFORMS).find( + ([, p]) => p.domains.includes(host) + ) + expect(owner, `no platform claims content-script match "${m}"`).toBeTruthy() + // The content script exists to draw the Add-as-source button, so a + // platform with no artist pattern (discord) has no business here. + expect( + PLATFORM_ARTIST_PATTERNS[owner[0]], + `"${m}" injects for ${owner[0]}, which has no artist pattern` + ).toBeTruthy() + } + }) + + it('injects on every platform that has an artist pattern', () => { + const covered = new Set( + matches + .map(hostOf) + .map((h) => Object.entries(PLATFORMS).find(([, p]) => p.domains.includes(h))) + .filter(Boolean) + .map(([key]) => key) + ) + for (const key of Object.keys(PLATFORM_ARTIST_PATTERNS)) { + expect(covered, `${key} has an artist pattern but no content-script match`).toContain(key) + } + }) + + it('requests no host permission for a domain no platform claims', () => { + // '*://*/*' is the deliberate exception: FC is self-hosted at an arbitrary + // operator-chosen URL, so the extension cannot enumerate its own backend. + // Every OTHER entry is a platform domain and must still have an owner. + for (const h of manifest.host_permissions) { + if (h === '*://*/*') continue + const host = hostOf(h) + // pixiv's OAuth/API hosts are pixiv infrastructure, not creator pages, + // so they are matched by suffix rather than by the domains list. + const claimed = Object.values(PLATFORMS).some( + (p) => p.domains.includes(host) || p.domains.some((d) => host.endsWith(d)) + ) + expect(claimed, `host permission "${h}" belongs to no platform`).toBe(true) + } + }) +}) diff --git a/frontend/src/components/settings/BrowserExtensionCard.vue b/frontend/src/components/settings/BrowserExtensionCard.vue index 0424975..2dfbd38 100644 --- a/frontend/src/components/settings/BrowserExtensionCard.vue +++ b/frontend/src/components/settings/BrowserExtensionCard.vue @@ -9,7 +9,7 @@

Pushes session cookies from supported platforms - (patreon, subscribestar, hentaifoundry, discord, pixiv, deviantart) + (patreon, subscribestar, hentaifoundry, discord, pixiv) into FabledCurator, and lets you add a creator as a source from their page in one click.

diff --git a/frontend/src/utils/platformColor.js b/frontend/src/utils/platformColor.js index 654619d..ef759b6 100644 --- a/frontend/src/utils/platformColor.js +++ b/frontend/src/utils/platformColor.js @@ -1,8 +1,10 @@ // Single source of truth for platform → color + icon mapping. Used by -// PlatformChip and any other GS-style platform-tagged surface. The six +// PlatformChip and any other GS-style platform-tagged surface. The five // platforms FC supports map 1:1 to the GS palette; unknown platforms fall -// back to grey + mdi-web. Operator-confirmed scope 2026-05-27. The ICONS key -// set is pinned against backend known_platform_keys() by +// back to grey + mdi-web — which is deliberately what a retired platform +// hits: a pre-#3069 deviantart source row still renders, as its raw key on +// a grey chip. Operator-confirmed scope 2026-05-27. The ICONS key set is +// pinned against backend known_platform_keys() by // tests/test_fe_be_contract.py. const ICONS = { @@ -11,7 +13,6 @@ const ICONS = { hentaifoundry: 'mdi-palette', discord: 'mdi-discord', pixiv: 'mdi-alpha-p-box', - deviantart: 'mdi-deviantart', } const COLORS = { @@ -20,7 +21,6 @@ const COLORS = { hentaifoundry: 'purple', discord: 'indigo', pixiv: 'blue', - deviantart: 'green', } const LABELS = { @@ -29,7 +29,6 @@ const LABELS = { hentaifoundry: 'HentaiFoundry', discord: 'Discord', pixiv: 'Pixiv', - deviantart: 'DeviantArt', } export function platformIcon(platform) { diff --git a/tests/test_api_extension.py b/tests/test_api_extension.py index d3010e0..b2885de 100644 --- a/tests/test_api_extension.py +++ b/tests/test_api_extension.py @@ -129,7 +129,6 @@ async def test_resolve_artist_name_dispatches_per_platform(db, monkeypatch): ("https://www.subscribestar.com/foobar", "subscribestar", "foobar"), ("https://subscribestar.adult/foobar", "subscribestar", "foobar"), ("https://www.hentai-foundry.com/user/Foo/profile", "hentaifoundry", "Foo"), - ("https://www.deviantart.com/baz", "deviantart", "baz"), ("https://www.pixiv.net/users/12345", "pixiv", "12345"), ("https://www.pixiv.net/en/users/12345", "pixiv", "12345"), ]) @@ -160,6 +159,23 @@ async def test_quick_add_source_unknown_url_400(client, ext_key): assert "known" in body +@pytest.mark.asyncio +async def test_quick_add_source_rejects_retired_deviantart(client, ext_key): + """#3069: a DeviantArt creator URL used to derive cleanly. Now that the + platform is retired, the extension's own gate should never offer the + button — but a stale content script on an un-updated browser still can, + so the backend has to refuse it rather than create an unusable source.""" + resp = await client.post( + "/api/extension/quick-add-source", + json={"url": "https://www.deviantart.com/baz"}, + headers={"X-Extension-Key": ext_key}, + ) + assert resp.status_code == 400 + body = await resp.get_json() + assert body["error"] == "unknown_platform" + assert "deviantart" not in body["known"] + + @pytest.mark.asyncio async def test_quick_add_source_invalid_url_400(client, ext_key): resp = await client.post( diff --git a/tests/test_api_platforms.py b/tests/test_api_platforms.py index 1c76b90..f65817a 100644 --- a/tests/test_api_platforms.py +++ b/tests/test_api_platforms.py @@ -6,16 +6,17 @@ pytestmark = pytest.mark.integration @pytest.mark.asyncio -async def test_platforms_returns_gs_six(client): +async def test_platforms_returns_gs_five(client): resp = await client.get("/api/platforms") assert resp.status_code == 200 body = await resp.get_json() platforms = body["platforms"] assert set(platforms.keys()) == { "patreon", "subscribestar", "hentaifoundry", - "discord", "pixiv", "deviantart", + "discord", "pixiv", } assert "fanbox" not in platforms + assert "deviantart" not in platforms # retired at #3069 @pytest.mark.asyncio diff --git a/tests/test_artist_directory_service.py b/tests/test_artist_directory_service.py index 56f490e..e08e311 100644 --- a/tests/test_artist_directory_service.py +++ b/tests/test_artist_directory_service.py @@ -154,7 +154,7 @@ async def test_list_platform_filter_excludes_no_source(db): @pytest.mark.asyncio async def test_list_platform_filter_excludes_wrong_platform(db): a = await _seed_artist(db, "alice-wplat") - await _seed_source(db, a.id, "deviantart", "https://d/alice-wp") + await _seed_source(db, a.id, "discord", "https://d/alice-wp") await db.commit() page = await ArtistDirectoryService(db).list_artists( diff --git a/tests/test_download_backends.py b/tests/test_download_backends.py index b53da45..ce7dda1 100644 --- a/tests/test_download_backends.py +++ b/tests/test_download_backends.py @@ -19,7 +19,7 @@ def test_native_platforms(): def test_gallery_dl_platforms_are_not_native(): # The platforms still served by gallery-dl must NOT route to the native # ingester — guards an accidental over-broad migration. - for platform in ("hentaifoundry", "discord", "deviantart"): + for platform in ("hentaifoundry", "discord"): assert uses_native_ingester(platform) is False diff --git a/tests/test_platform_lock.py b/tests/test_platform_lock.py index ce97ae1..b677e1e 100644 --- a/tests/test_platform_lock.py +++ b/tests/test_platform_lock.py @@ -9,8 +9,8 @@ pytestmark = pytest.mark.integration def test_non_serialized_platform_has_no_lock(): # gallery-dl platforms aren't capped — they get no lock at all. - assert platform_lock("deviantart", ttl_seconds=60) is None assert platform_lock("hentaifoundry", ttl_seconds=60) is None + assert platform_lock("discord", ttl_seconds=60) is None def test_subscribestar_is_serialized(): diff --git a/tests/test_platforms_registry.py b/tests/test_platforms_registry.py index c100792..c94341c 100644 --- a/tests/test_platforms_registry.py +++ b/tests/test_platforms_registry.py @@ -11,10 +11,10 @@ from backend.app.services.platforms import ( ) -def test_known_platform_keys_is_gs_six(): +def test_known_platform_keys_is_gs_five(): assert known_platform_keys() == frozenset({ "patreon", "subscribestar", "hentaifoundry", - "discord", "pixiv", "deviantart", + "discord", "pixiv", }) @@ -23,6 +23,15 @@ def test_fanbox_not_in_registry(): assert "fanbox" not in PLATFORMS +def test_deviantart_is_retired(): + # #3069 executed the 2026-07-05 drop decision (FC downloaders = ART- + # DEDICATED services only). The registry is what /api/platforms, the + # source validator and the credential validator all read, so its absence + # here is what actually retires the platform everywhere else. + assert "deviantart" not in PLATFORMS + assert auth_type_for("deviantart") is None + + def test_auth_type_for_known_and_unknown(): assert auth_type_for("patreon") == "cookies" assert auth_type_for("discord") == "token" diff --git a/tests/test_post_feed_service.py b/tests/test_post_feed_service.py index 87d0de5..8b544ce 100644 --- a/tests/test_post_feed_service.py +++ b/tests/test_post_feed_service.py @@ -169,7 +169,7 @@ async def test_scroll_filters_by_artist(db): async def test_scroll_filters_by_platform(db): artist = await _seed_artist(db, "alice-platf") src_p = await _seed_source(db, artist.id, "patreon", "https://p/alice-pp") - src_d = await _seed_source(db, artist.id, "deviantart", "https://d/alice-dd") + src_d = await _seed_source(db, artist.id, "discord", "https://d/alice-dd") now = datetime.now(UTC) pp = await _seed_post(db, src_p.id, external_id="PP", post_date=now) await _seed_post(db, src_d.id, external_id="PD", post_date=now) @@ -234,7 +234,7 @@ async def test_scroll_combined_artist_and_platform(db): alice = await _seed_artist(db, "alice-combo") bob = await _seed_artist(db, "bob-combo") src_alice_patreon = await _seed_source(db, alice.id, "patreon", "https://p/alice-c") - src_alice_da = await _seed_source(db, alice.id, "deviantart", "https://d/alice-c") + src_alice_da = await _seed_source(db, alice.id, "discord", "https://d/alice-c") src_bob_patreon = await _seed_source(db, bob.id, "patreon", "https://p/bob-c") now = datetime.now(UTC) target = await _seed_post( diff --git a/tests/test_source_service.py b/tests/test_source_service.py index a9bdc28..17acecd 100644 --- a/tests/test_source_service.py +++ b/tests/test_source_service.py @@ -23,12 +23,14 @@ async def _artist(db, name="Alice"): @pytest.mark.asyncio -async def test_known_platforms_is_gs_six(db): +async def test_known_platforms_is_gs_five(db): assert KNOWN_PLATFORMS == frozenset({ "patreon", "subscribestar", "hentaifoundry", - "discord", "pixiv", "deviantart", + "discord", "pixiv", }) assert "fanbox" not in KNOWN_PLATFORMS + # Retired at #3069 — a source can no longer be created on it. + assert "deviantart" not in KNOWN_PLATFORMS @pytest.mark.asyncio From 516521e7b02b26da081c9c78c17d795e9c3f22f3 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 27 Aug 2026 07:26:02 -0400 Subject: [PATCH 05/16] =?UTF-8?q?refactor(platforms):=20drop=20migration?= =?UTF-8?q?=200088=20=E2=80=94=20no=20deviantart=20rows=20exist=20(#3069)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Operator confirms the instance has never used DeviantArt, so there is nothing for 0088 to quiesce. The migration only ever had two jobs — disable leftover `source` rows and delete a stale `credential` row — and both were guards against data that does not exist here. Removing it rather than keeping a no-op: a migration that runs on every deploy to touch zero rows is a permanent cost paid for a hypothetical, and it would read to a future reader as evidence that DeviantArt sources once existed. `platform` has no CHECK constraint, so retiring the key needs no schema change of its own. alembic head returns to 0087. Co-Authored-By: Claude Opus 5 --- alembic/versions/0088_retire_deviantart.py | 70 ---------------------- 1 file changed, 70 deletions(-) delete mode 100644 alembic/versions/0088_retire_deviantart.py diff --git a/alembic/versions/0088_retire_deviantart.py b/alembic/versions/0088_retire_deviantart.py deleted file mode 100644 index 171e722..0000000 --- a/alembic/versions/0088_retire_deviantart.py +++ /dev/null @@ -1,70 +0,0 @@ -"""retire deviantart (#3069) — quiesce the rows the dropped platform leaves behind - -`deviantart` is no longer a registered platform, so nothing can create or edit a -source with that key any more. Existing rows are a different question, and the -two tables want opposite treatment: - -* `source` rows are DISABLED, not deleted. The row is the only place the - artist's DeviantArt URL is recorded, and losing it is unrecoverable — whereas - a disabled row is visible in the UI and reversible by hand. Disabling is also - required for correctness, not just tidiness: with the platform unregistered - the download path falls through to gallery-dl, which carries its OWN built-in - deviantart extractor, so an enabled row would have gone on downloading from a - platform the product dropped. - -* the `credential` row IS deleted. It is an encrypted DeviantArt session cookie - for a site FC will never call again — keeping a live credential we have no - use for is strictly worse than dropping it, and re-exporting from the browser - is the recovery path if that judgment is ever wrong. - -A no-op on an instance that never had a DeviantArt source, which is the -expected case. - -Revision ID: 0088 -Revises: 0087 -Create Date: 2026-08-27 -""" -from typing import Sequence, Union - -import sqlalchemy as sa -from alembic import op - -revision: str = "0088" -down_revision: Union[str, None] = "0087" -branch_labels: Union[str, Sequence[str], None] = None -depends_on: Union[str, Sequence[str], None] = None - -_RETIRED = "deviantart" - -# Written into last_error so the disabled row explains itself in the UI rather -# than looking like an unexplained toggle someone flipped. -_REASON = ( - "Platform retired: FabledCurator no longer supports DeviantArt " - "(dropped 2026-08-27, #3069). This source was disabled automatically; " - "the URL is kept for reference and the row can be deleted by hand." -) - - -def upgrade() -> None: - conn = op.get_bind() - disabled = conn.execute( - sa.text( - "UPDATE source SET enabled = false, last_error = :reason " - "WHERE platform = :p AND enabled = true" - ), - {"reason": _REASON, "p": _RETIRED}, - ).rowcount - creds = conn.execute( - sa.text("DELETE FROM credential WHERE platform = :p"), {"p": _RETIRED} - ).rowcount - print(f"0088: disabled {disabled} deviantart source(s), removed {creds} credential(s)") - - -def downgrade() -> None: - # Re-enabling is deliberately NOT done: the platform is gone from the - # registry, so a re-enabled source would still have no backend to run on. - # Clearing the stamped reason is the only half that means anything. - op.get_bind().execute( - sa.text("UPDATE source SET last_error = NULL WHERE platform = :p AND last_error = :reason"), - {"p": _RETIRED, "reason": _REASON}, - ) From 89155478a8cc4d5a62cb41c46d20dd254a9431e2 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 27 Aug 2026 07:33:14 -0400 Subject: [PATCH 06/16] test(refetch): cover the Layer-2 auto-refetch remediation (#3071) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit refetch_service was the only module under backend/app/services/ with no test file — and not an inert one: it runs unattended off the recovery sweep and deletes a file from disk before asking a downloader to replace it. The frontend cites it by name as the reason the Import tab could be retired ("imports heal themselves"). The ticket described it as having zero direct coverage. That is true of the module, but not of the code: test_api_import_admin.py already drives the happy path end-to-end through the refetch route — file deleted, task flagged, one dispatch, second attempt a no-op. These 17 tests therefore target what the route tests cannot reach rather than restating them: * every branch of resolve_refetch_source — disabled Source, a `sidecar::` synthetic anchor, a platform mismatch, the gallery-dl `NN_` numbering-prefix sidecar, the lowest-id pick among several candidates, and each of the five ways it declines (no sidecar, unreadable JSON, non-object JSON, no platform, no artist folder / no matching Artist row). * that the file SURVIVES when nothing re-pollable resolves. This is the assertion the module exists for: `no_source` is the common case on a filesystem-only library, where the file on disk is the operator's only copy. The route-level no_source test cannot catch a regression here — its path never existed, so an unconditional unlink would pass it. * that the `refetched` bound is checked BEFORE the unlink, so a second sweep leaves the re-downloaded file alone rather than deleting it again. * that an unlink failure is logged and stepped over, not raised — a raise would abort the whole sweep for every other poison-pill row in the batch. Exercised with a real IsADirectoryError rather than a patched pathlib. Co-Authored-By: Claude Opus 5 --- tests/test_refetch_service.py | 324 ++++++++++++++++++++++++++++++++++ 1 file changed, 324 insertions(+) create mode 100644 tests/test_refetch_service.py diff --git a/tests/test_refetch_service.py b/tests/test_refetch_service.py new file mode 100644 index 0000000..59811a7 --- /dev/null +++ b/tests/test_refetch_service.py @@ -0,0 +1,324 @@ +"""Layer-2 auto-refetch remediation — `services/refetch_service.py` (#3071). + +This module was the only one under `backend/app/services/` with no test +file, which matters more than a coverage gap normally would: it runs +UNATTENDED off the recovery sweep (`tasks/maintenance.py`, gated by +FC_AUTO_REFETCH_CORRUPT) and it DELETES a file from disk before asking a +downloader for a fresh copy. The frontend cites it by name as the reason +the Import tab could be retired at all (`stores/import.js`: imports +"heal themselves"). + +`test_api_import_admin.py` already drives the happy path end-to-end +through `POST /api/import/tasks//refetch` — file deleted, task +flagged, one dispatch, second attempt a no-op. What it CANNOT reach is +the branching inside `resolve_refetch_source`, and it never proves the +negative that actually protects the operator's data: that a file whose +source does NOT resolve is still on disk afterwards. Its `no_source` +case points at a path that never existed, so nothing survives to check. + +Those two things are what this module covers. +""" + +import json +from pathlib import Path + +import pytest +from sqlalchemy import select + +from backend.app.models import Artist, ImportBatch, ImportTask, Source +from backend.app.services.refetch_service import ( + attempt_refetch, + resolve_refetch_source, +) + +pytestmark = pytest.mark.integration + + +# --- fixtures / helpers ---------------------------------------------------- + +@pytest.fixture +def import_root(tmp_path): + root = tmp_path / "import" + root.mkdir() + return root + + +def _media(import_root: Path, artist_dir: str, name: str = "post.jpg") -> Path: + """A corrupt-import stand-in at import_root//.""" + d = import_root / artist_dir if artist_dir else import_root + d.mkdir(parents=True, exist_ok=True) + m = d / name + m.write_bytes(b"corrupt-bytes") + return m + + +def _sidecar(media: Path, payload) -> Path: + """gallery-dl writes `.json` beside the media file.""" + sc = media.with_suffix(".json") + sc.write_text(payload if isinstance(payload, str) else json.dumps(payload)) + return sc + + +def _artist(session, name: str) -> Artist: + a = Artist(name=name, slug=name.lower()) + session.add(a) + session.flush() + return a + + +def _source(session, artist, platform="patreon", url=None, enabled=True) -> Source: + s = Source( + artist_id=artist.id, + platform=platform, + url=url if url is not None else f"https://www.{platform}.com/{artist.slug}", + enabled=enabled, + config_overrides={}, + ) + session.add(s) + session.flush() + return s + + +def _task(session, media: Path, refetched: bool = False) -> ImportTask: + batch = ImportBatch( + triggered_by="manual", source_path=str(media.parent), scan_mode="quick", + ) + session.add(batch) + session.flush() + t = ImportTask( + batch_id=batch.id, source_path=str(media), task_type="media", + status="failed", refetched=refetched, + ) + session.add(t) + session.flush() + return t + + +@pytest.fixture +def no_dispatch(monkeypatch): + """Capture download_source.delay instead of queueing a real re-check. + + refetch_service imports the task lazily (inside attempt_refetch, to + dodge a tasks->services->tasks cycle), so patching the attribute on + the module is enough — the import resolves at call time. + """ + from backend.app.tasks import download as download_mod + + calls = [] + monkeypatch.setattr(download_mod.download_source, "delay", calls.append) + return calls + + +# --- resolve_refetch_source: what counts as re-pollable -------------------- + +def test_resolve_finds_enabled_source_matching_the_sidecar_platform(db_sync, import_root): + m = _media(import_root, "Alice") + _sidecar(m, {"category": "patreon", "post_id": 1}) + artist = _artist(db_sync, "Alice") + src = _source(db_sync, artist) + + assert resolve_refetch_source(db_sync, str(m), import_root).id == src.id + + +def test_resolve_skips_a_disabled_source(db_sync, import_root): + """A disabled Source is not re-pollable: the operator turned it off, + and a sweep must not reach past that to delete their file.""" + m = _media(import_root, "Alice") + _sidecar(m, {"category": "patreon"}) + _source(db_sync, _artist(db_sync, "Alice"), enabled=False) + + assert resolve_refetch_source(db_sync, str(m), import_root) is None + + +def test_resolve_rejects_a_synthetic_sidecar_anchor_url(db_sync, import_root): + """`sidecar::` is a bookkeeping anchor for files that + arrived on disk, not a feed. Re-polling it is impossible, so it must + not qualify — otherwise the file is deleted for a fetch that can + never happen.""" + m = _media(import_root, "Alice") + _sidecar(m, {"category": "patreon"}) + _source(db_sync, _artist(db_sync, "Alice"), url="sidecar:patreon:alice") + + assert resolve_refetch_source(db_sync, str(m), import_root) is None + + +def test_resolve_needs_a_source_on_the_sidecars_own_platform(db_sync, import_root): + m = _media(import_root, "Alice") + _sidecar(m, {"category": "patreon"}) + _source(db_sync, _artist(db_sync, "Alice"), platform="pixiv") + + assert resolve_refetch_source(db_sync, str(m), import_root) is None + + +def test_resolve_picks_the_lowest_id_when_several_sources_qualify(db_sync, import_root): + m = _media(import_root, "Alice") + _sidecar(m, {"category": "patreon"}) + artist = _artist(db_sync, "Alice") + first = _source(db_sync, artist, url="https://www.patreon.com/alice-one") + _source(db_sync, artist, url="https://www.patreon.com/alice-two") + + # Deterministic choice, not "whichever the planner returned first" — + # the pick decides which downloader runs. + assert resolve_refetch_source(db_sync, str(m), import_root).id == first.id + + +def test_resolve_reads_the_gallery_dl_numbered_sidecar(db_sync, import_root): + """gallery-dl prefixes media with `NN_` for in-post ordering but + writes the sidecar under the UNPREFIXED stem. Refetch resolves real + downloaded files, so it has to follow that convention.""" + m = _media(import_root, "Alice", name="01_post.jpg") + (import_root / "Alice" / "post.json").write_text(json.dumps({"category": "patreon"})) + src = _source(db_sync, _artist(db_sync, "Alice")) + + assert resolve_refetch_source(db_sync, str(m), import_root).id == src.id + + +# --- resolve_refetch_source: every way it declines ------------------------- + +def test_resolve_declines_without_a_sidecar(db_sync, import_root): + m = _media(import_root, "Alice") + _source(db_sync, _artist(db_sync, "Alice")) + + assert resolve_refetch_source(db_sync, str(m), import_root) is None + + +def test_resolve_declines_on_unreadable_sidecar_json(db_sync, import_root): + m = _media(import_root, "Alice") + _sidecar(m, "{not valid json") + _source(db_sync, _artist(db_sync, "Alice")) + + assert resolve_refetch_source(db_sync, str(m), import_root) is None + + +def test_resolve_declines_when_the_sidecar_is_not_an_object(db_sync, import_root): + # A bare JSON list parses fine but has no `category` to read. + m = _media(import_root, "Alice") + _sidecar(m, ["patreon"]) + _source(db_sync, _artist(db_sync, "Alice")) + + assert resolve_refetch_source(db_sync, str(m), import_root) is None + + +def test_resolve_declines_when_the_sidecar_names_no_platform(db_sync, import_root): + m = _media(import_root, "Alice") + _sidecar(m, {"post_id": 1}) + _source(db_sync, _artist(db_sync, "Alice")) + + assert resolve_refetch_source(db_sync, str(m), import_root) is None + + +def test_resolve_declines_for_a_file_sitting_directly_in_import_root(db_sync, import_root): + """No artist folder means no artist bucket to resolve — the + filesystem-only drop case.""" + m = _media(import_root, "") + _sidecar(m, {"category": "patreon"}) + _source(db_sync, _artist(db_sync, "Alice")) + + assert resolve_refetch_source(db_sync, str(m), import_root) is None + + +def test_resolve_declines_when_no_artist_row_matches_the_folder(db_sync, import_root): + m = _media(import_root, "Nobody") + _sidecar(m, {"category": "patreon"}) + _source(db_sync, _artist(db_sync, "Alice")) + + assert resolve_refetch_source(db_sync, str(m), import_root) is None + + +# --- attempt_refetch: the destructive half --------------------------------- + +def test_attempt_refetch_deletes_the_file_and_queues_one_recheck( + db_sync, import_root, no_dispatch, +): + m = _media(import_root, "Alice") + _sidecar(m, {"category": "patreon"}) + src = _source(db_sync, _artist(db_sync, "Alice")) + task = _task(db_sync, m) + + result = attempt_refetch(db_sync, task, import_root) + + assert result == {"status": "refetch_queued", "source_id": src.id} + assert not m.exists() # the bad copy is gone... + assert no_dispatch == [src.id] # ...and exactly one re-check was queued + assert task.refetched is True + + +def test_attempt_refetch_leaves_the_file_alone_when_nothing_resolves( + db_sync, import_root, no_dispatch, +): + """THE assertion this module exists for. `no_source` is the common + case on a filesystem-only library, and the file on disk is then the + operator's ONLY copy — deleting it without a downloader that can + replace it destroys the thing the remediation was meant to repair. + + The route-level `no_source` test cannot catch a regression here: its + path never existed, so an unconditional unlink would pass it. + """ + m = _media(import_root, "Alice") + _sidecar(m, {"category": "patreon"}) + _source(db_sync, _artist(db_sync, "Alice"), enabled=False) + task = _task(db_sync, m) + + assert attempt_refetch(db_sync, task, import_root) == {"status": "no_source"} + assert m.exists() + assert m.read_bytes() == b"corrupt-bytes" + assert no_dispatch == [] + assert task.refetched is False # not consumed — a real fix can still run + + +def test_attempt_refetch_is_bounded_to_a_single_attempt( + db_sync, import_root, no_dispatch, +): + """The `refetched` bound is what stops SOURCE-side corruption from + looping: re-downloading a file that is broken upstream returns the + same bytes forever. The check must come FIRST — a second call has to + leave the (re-downloaded) file untouched, not delete it again. + """ + m = _media(import_root, "Alice") + _sidecar(m, {"category": "patreon"}) + _source(db_sync, _artist(db_sync, "Alice")) + task = _task(db_sync, m, refetched=True) + + assert attempt_refetch(db_sync, task, import_root) == {"status": "already_refetched"} + assert m.exists() + assert no_dispatch == [] + + +def test_attempt_refetch_proceeds_when_the_file_is_already_gone( + db_sync, import_root, no_dispatch, +): + """`missing_ok=True`: the sweep races an operator who deleted the bad + file by hand. The re-check is still the right next move.""" + m = _media(import_root, "Alice") + _sidecar(m, {"category": "patreon"}) + src = _source(db_sync, _artist(db_sync, "Alice")) + task = _task(db_sync, m) + m.unlink() + + assert attempt_refetch(db_sync, task, import_root)["status"] == "refetch_queued" + assert no_dispatch == [src.id] + + +def test_attempt_refetch_survives_an_unlink_failure( + db_sync, import_root, no_dispatch, +): + """An unremovable path is logged and stepped over, not raised — this + runs unattended, and a raise would abort the whole recovery sweep for + every OTHER poison-pill row in the batch. + + A directory standing where the media file should be produces a + genuine IsADirectoryError (an OSError) without patching pathlib, so + the handler is exercised rather than simulated. + """ + d = import_root / "Alice" / "post.jpg" + d.mkdir(parents=True) + (import_root / "Alice" / "post.json").write_text(json.dumps({"category": "patreon"})) + src = _source(db_sync, _artist(db_sync, "Alice")) + task = _task(db_sync, d) + + assert attempt_refetch(db_sync, task, import_root)["status"] == "refetch_queued" + assert d.exists() # removal genuinely failed... + assert no_dispatch == [src.id] # ...and the sweep carried on anyway + assert db_sync.execute( + select(ImportTask.refetched).where(ImportTask.id == task.id) + ).scalar_one() is True From bfc5135f1927da2946655e3a8c9efb8e3974ec26 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 27 Aug 2026 07:39:49 -0400 Subject: [PATCH 07/16] =?UTF-8?q?docs:=20true=20up=20README=20=E2=80=94=20?= =?UTF-8?q?status,=20the=20five=20pieces,=20CI=20(#3070)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit README.md was last touched in 4aff9c5 (2026-05-14) and several of its most visible lines had gone false: - "Pre-v1. Not yet functional." — FC has been continuously deployed for months. Replaced with what main/dev actually mean for what's running. - "Node 22 pre-installed" — ci-requirements.md and extension.yml both say node 24, and frontend/package.json requires >=24. - "Runner label python-ci — a runner with Python 3.14, ruff and Node 22 pre-installed ... The runner image (runner-base:python-ci) is built from CI-Runner/CI-python/" — describes runs-on as selecting the toolchain. It doesn't: runs-on is a scheduling label, and every job names its own container.image (ci-python:3.14, or node:24-bookworm-slim for the extension lane). - "Both ci.yml and build.yml use this label" — there are three workflows. The CI section now points at ci-requirements.md rather than restating it, so the two can't drift apart again; that file is current and is the one the CI-runner process expects. Added a "What's in here" table for the five deployable pieces — the extension, the GPU agent and the ML image were unmentioned, three of the five. Also corrected the RELEASE_TOKEN write:release scope, which is no longer "for future release-cutting workflows": it backs the ext- releases that cache the signed XPI. Refs #3070 --- README.md | 41 +++++++++++++++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 225f318..c13f2f6 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,21 @@ Combines what was [ImageRepo](https://git.fabledsword.com/bvandeusen/ImageRepo) ## Status -Pre-v1. Not yet functional. +In production. `main` is continuously deployed — every merge to `main` builds +and publishes `:latest` images, so whatever is on `main` is what is running. +Day-to-day work happens on `dev`, which publishes `:dev` images. + +## What's in here + +Five deployable pieces, built by `.forgejo/workflows/build.yml`: + +| Piece | Built from | Image | Role | +| --- | --- | --- | --- | +| **Web / workers** | `Dockerfile` | `fabledcurator` | Quart API + the built Vue SPA in one image. `entrypoint.sh` picks the role: `web`, `worker`, `scheduler`. The `maintenance-long` service is a second `worker` pinned to the long-running maintenance queue. | +| **ML worker** | `Dockerfile.ml` | `fabledcurator-ml` | Same app, plus `requirements-ml.txt` — tagging and embedding models that run in-container. | +| **GPU agent** | `agent/Dockerfile` | `fabledcurator-agent` | Optional desktop-GPU worker (`agent/`). Leases jobs over **HTTP only** — never touches the database or Redis. Run it for a burst, stop it to reclaim the card. See `agent/README.md`. | +| **Firefox extension** | `extension/` | signed XPI | MV3 extension: pushes platform session cookies into FC and adds a creator as a Source in one click. AMO-signed on `main` only, then bundled into the web image and served from Settings → Maintenance. See `extension/README.md`. | +| **Data** | — | `pgvector/pgvector:pg16`, `redis:7-alpine` | Postgres with pgvector for embeddings; Redis as the Celery broker. | ## Quick start @@ -29,22 +43,37 @@ docker compose -f docker-compose.yml up -d # (skips the override so containers pull registry images) ``` +The GPU agent is deployed separately, on the machine with the card — +`agent/docker-compose.yml`, not this stack. + ## Deployment posture FabledCurator is designed to run inside a self-hosted homelab environment over plain HTTP. If you want TLS, terminate it at your reverse proxy. The app does not generate certificates, redirect to HTTPS, or set HSTS. ## CI / Forgejo setup -The repo's workflows expect: +Three workflows: `ci.yml` (lint, extension-version guard, backend unit tests, +frontend build, integration), `extension.yml` (extension lint, vitest, XPI +content verification), and `build.yml` (sign + publish). -- **Runner label `python-ci`** — a Forgejo runner with Python 3.14, ruff, and Node 22 pre-installed. Both `ci.yml` and `build.yml` use this label. The runner image (`runner-base:python-ci`) is built from `CI-Runner/CI-python/` in the operator's workspace; `make push` from that directory builds and pushes a new image when toolchain pins change. -- **Repo secret `RELEASE_TOKEN`** — a Forgejo PAT with the following scopes: +**The toolchain each job runs in is its `container.image`, not its `runs-on` +label.** `runs-on: python-ci` only schedules the job onto a runner; every job +then names the image it actually wants. `ci-requirements.md` is the current, +authoritative list of images and per-job installs — read that rather than a +copy here, so the two can't drift. + +The repo expects one secret: + +- **`RELEASE_TOKEN`** — a Forgejo PAT with: - `write:package` + `read:package` — for `docker push` to `git.fabledsword.com` - - `write:release` — for future release-cutting workflows - - `write:issue` — for future issue-management automation + - `write:release` — for the `ext-` releases that cache the signed XPI + - `write:issue` — for issue-management automation Generate at https://git.fabledsword.com/user/settings/applications. The injected `GITHUB_TOKEN` cannot be used because it lacks `write:package`. +AMO signing additionally needs `MOZILLA_AMO_JWT_KEY` / `MOZILLA_AMO_JWT_SECRET`; it runs on +`main` only and is cached per version, since AMO rejects a re-signed version. + ## License Personal project; use at your own discretion. From 1ac448d881eb9a94bdf7b75be16df702d323075c Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 27 Aug 2026 07:48:06 -0400 Subject: [PATCH 08/16] refactor: four small cleanups from the review pass (#3072) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Items 2-5 of #3072. Item 1 (the per-row sweep inserts) is separate. 2. .fc-bad was not merely duplicated — it is .fc-weak under a second name. Both local definitions were `color: rgb(var(--v-theme-error))`, identical to the global .fc-weak, and GpuAgentCard was already using .fc-weak to colour exactly what GpuActivityPanel coloured .fc-bad (an errored count, red when non-zero). So rather than promoting a synonym to app.css, both call sites now use .fc-weak and the local defs are gone. app.css's status-colour comment records why there is no .fc-bad, next to the existing note on why .fc-ok is deliberately NOT global. 3. GalleryItem.vue's obsidian literals now use --v-theme-background, which IS obsidian (vuetify-theme.js maps background -> surfaces. obsidian). Preferred over --fc-chrome-rgb: same value, but that variable is named for the nav fade, not for the palette entry. The ticket said these were the only three real uses in the tree. They are not — GalleryItem itself had two more in the artist-label gradient (fixed here, so the file is now consistent), and ~13 more live in SeriesView, SeriesReaderView, ImageViewer, ArtistHeader, ExploreView and GalleryFilterBar. Those are a separate sweep, filed rather than folded in here. 4. The attachment download path had two hand-formatted copies. One definition now, `attachment_download_url`, next to the model both serializers already import. The test pins it by MATCHING the built path against the app's real URL map rather than comparing to a literal — a string-equality test would still pass after someone renamed the route, which is the drift the helper exists to prevent. 5. Extension API key now compares with hmac.compare_digest. Compared as BYTES, not str: compare_digest's str form raises TypeError on non-ASCII, and this value comes straight from an attacker-controlled header, so the str form would turn a junk key into a 500 instead of a 403. Low stakes either way — the API is unauthenticated-by-design on a LAN — but it costs nothing. Refs #3072 --- backend/app/api/extension.py | 11 ++++++++++- backend/app/models/__init__.py | 3 ++- backend/app/models/post_attachment.py | 12 ++++++++++++ backend/app/services/post_feed_service.py | 3 ++- backend/app/services/provenance_service.py | 3 ++- frontend/src/components/gallery/GalleryItem.vue | 9 +++++---- .../settings/DownloadsActivityPanel.vue | 3 +-- .../src/components/settings/GpuActivityPanel.vue | 3 +-- frontend/src/styles/app.css | 7 ++++++- tests/test_api_attachments.py | 15 ++++++++++++++- 10 files changed, 55 insertions(+), 14 deletions(-) diff --git a/backend/app/api/extension.py b/backend/app/api/extension.py index 20b1b30..082bc1e 100644 --- a/backend/app/api/extension.py +++ b/backend/app/api/extension.py @@ -6,6 +6,7 @@ from __future__ import annotations import asyncio import hashlib +import hmac import re from pathlib import Path @@ -41,7 +42,15 @@ async def _ext_key_required(session) -> bool: stored = (await session.execute( select(AppSetting.value).where(AppSetting.key == "extension_api_key") )).scalar_one_or_none() - return stored is not None and supplied == stored + if stored is None: + return False + # compare_digest, not `==`: the stored key is a shared secret, and a + # short-circuiting compare leaks its prefix through timing. Costs nothing + # here — it is not that this route is exposed (#3072). Compared as BYTES: + # compare_digest's str form rejects non-ASCII with TypeError, and this + # header is attacker-supplied, so a str compare would turn a junk key into + # a 500 instead of a 403. + return hmac.compare_digest(supplied.encode("utf-8"), stored.encode("utf-8")) def _extract_version(xpi_name: str) -> str: diff --git a/backend/app/models/__init__.py b/backend/app/models/__init__.py index f06d74d..a11d267 100644 --- a/backend/app/models/__init__.py +++ b/backend/app/models/__init__.py @@ -27,7 +27,7 @@ from .patreon_seen_media import PatreonSeenMedia from .pixiv_failed_media import PixivFailedMedia from .pixiv_seen_media import PixivSeenMedia from .post import Post -from .post_attachment import PostAttachment +from .post_attachment import PostAttachment, attachment_download_url from .presentation_review import PresentationReview from .series_chapter import SeriesChapter from .series_page import SeriesPage @@ -58,6 +58,7 @@ __all__ = [ "SubscribeStarSeenMedia", "Post", "PostAttachment", + "attachment_download_url", "PresentationReview", "SeriesChapter", "SeriesPage", diff --git a/backend/app/models/post_attachment.py b/backend/app/models/post_attachment.py index 1edb7f3..f522cdf 100644 --- a/backend/app/models/post_attachment.py +++ b/backend/app/models/post_attachment.py @@ -65,3 +65,15 @@ class PostAttachment(Base): captured_at: Mapped[datetime] = mapped_column( DateTime(timezone=True), nullable=False, server_default=func.now() ) + + +def attachment_download_url(attachment_id: int) -> str: + """The path that streams this attachment's bytes. + + Both serializers that expose an attachment to the frontend + (`provenance_service`, `post_feed_service`) built this literal themselves, + so changing the route in `api/attachments.py` meant two edits and only one + would be remembered (#3072). `test_attachment_download_url` pins it against + the app's registered rule, so the drift is caught rather than trusted to. + """ + return f"/api/attachments/{attachment_id}/download" diff --git a/backend/app/services/post_feed_service.py b/backend/app/services/post_feed_service.py index c1304ac..d4aec83 100644 --- a/backend/app/services/post_feed_service.py +++ b/backend/app/services/post_feed_service.py @@ -24,6 +24,7 @@ from ..models import ( Post, PostAttachment, Source, + attachment_download_url, ) from ..utils.html_sanitize import ( extract_img_srcs, @@ -360,7 +361,7 @@ class PostFeedService: "ext": att.ext, "mime": att.mime, "size_bytes": att.size_bytes, - "download_url": f"/api/attachments/{att.id}/download", + "download_url": attachment_download_url(att.id), }) return out diff --git a/backend/app/services/provenance_service.py b/backend/app/services/provenance_service.py index 6fee555..02f1a64 100644 --- a/backend/app/services/provenance_service.py +++ b/backend/app/services/provenance_service.py @@ -16,6 +16,7 @@ from ..models import ( Post, PostAttachment, Source, + attachment_download_url, ) from ..utils.html_sanitize import sanitize_post_html @@ -53,7 +54,7 @@ def _attachment_dict(a: PostAttachment) -> dict: "original_filename": a.original_filename, "size_bytes": a.size_bytes, "ext": a.ext, - "download_url": f"/api/attachments/{a.id}/download", + "download_url": attachment_download_url(a.id), } diff --git a/frontend/src/components/gallery/GalleryItem.vue b/frontend/src/components/gallery/GalleryItem.vue index cab8b9e..f5fd815 100644 --- a/frontend/src/components/gallery/GalleryItem.vue +++ b/frontend/src/components/gallery/GalleryItem.vue @@ -139,9 +139,9 @@ function onThumbError() { thumbError.value = true } position: absolute; top: 8px; left: 8px; width: 22px; height: 22px; border-radius: 4px; border: 2px solid rgba(232, 228, 216, 0.8); - background: rgba(20, 23, 26, 0.45); + background: rgba(var(--v-theme-background), 0.45); display: grid; place-items: center; - color: #14171A; z-index: 11; + color: rgb(var(--v-theme-background)); z-index: 11; } .fc-gallery-item__checkbox.on { background: rgb(var(--v-theme-accent)); @@ -152,7 +152,7 @@ function onThumbError() { thumbError.value = true } min-width: 22px; height: 22px; padding: 0 5px; border-radius: 11px; background: rgb(var(--v-theme-accent)); - color: #14171A; font-size: 12px; font-weight: 700; + color: rgb(var(--v-theme-background)); font-size: 12px; font-weight: 700; display: grid; place-items: center; z-index: 11; pointer-events: none; } @@ -160,7 +160,8 @@ function onThumbError() { thumbError.value = true } position: absolute; left: 0; right: 0; bottom: 0; padding: 14px 8px 6px; background: linear-gradient( - to top, rgba(20, 23, 26, 0.78), rgba(20, 23, 26, 0) + to top, rgba(var(--v-theme-background), 0.78), + rgba(var(--v-theme-background), 0) ); font-size: 12px; line-height: 1.2; white-space: nowrap; overflow: hidden; text-overflow: ellipsis; diff --git a/frontend/src/components/settings/DownloadsActivityPanel.vue b/frontend/src/components/settings/DownloadsActivityPanel.vue index d8f9d16..380b44a 100644 --- a/frontend/src/components/settings/DownloadsActivityPanel.vue +++ b/frontend/src/components/settings/DownloadsActivityPanel.vue @@ -35,7 +35,7 @@ All subscription sources healthy.

- {{ failing.length }} failing source(s): + {{ failing.length }} failing source(s): {{ failingNames }}

@@ -72,5 +72,4 @@ onUnmounted(() => { if (pollId) clearInterval(pollId) }) diff --git a/frontend/src/components/settings/GpuActivityPanel.vue b/frontend/src/components/settings/GpuActivityPanel.vue index 39bba29..1f85a5b 100644 --- a/frontend/src/components/settings/GpuActivityPanel.vue +++ b/frontend/src/components/settings/GpuActivityPanel.vue @@ -25,7 +25,7 @@
done
-
{{ q.error }}
+
{{ q.error }}
errored
@@ -104,5 +104,4 @@ onUnmounted(() => { if (pollId) clearInterval(pollId) }) font-size: 11px; text-transform: uppercase; letter-spacing: 0.04em; color: rgb(var(--v-theme-on-surface-variant)); } -.fc-bad { color: rgb(var(--v-theme-error)); } diff --git a/frontend/src/styles/app.css b/frontend/src/styles/app.css index 3e24a75..fbd9380 100644 --- a/frontend/src/styles/app.css +++ b/frontend/src/styles/app.css @@ -50,7 +50,12 @@ /* Status text colours (DRY pass #161): fc-good = success, fc-weak = error, consolidated from the GPU / heads cards. fc-ok is intentionally NOT global — - it means on-surface in HeadsCard but success in QueuesTable. */ + it means on-surface in HeadsCard but success in QueuesTable. + + No `.fc-bad` (#3072): it was defined locally and identically in the Downloads + and GPU activity panels, and it is fc-weak under a second name — GpuAgentCard + and GpuActivityPanel were colouring the same "errored" count with different + class names. Both now use fc-weak. Reach for fc-weak, not a new synonym. */ .fc-good { color: rgb(var(--v-theme-success)); } .fc-weak { color: rgb(var(--v-theme-error)); } diff --git a/tests/test_api_attachments.py b/tests/test_api_attachments.py index 95e31f2..7918008 100644 --- a/tests/test_api_attachments.py +++ b/tests/test_api_attachments.py @@ -1,6 +1,6 @@ import pytest -from backend.app.models import Artist, PostAttachment +from backend.app.models import Artist, PostAttachment, attachment_download_url pytestmark = pytest.mark.integration @@ -33,3 +33,16 @@ async def test_download_streams_with_disposition(client, db, tmp_path): async def test_download_404(client): resp = await client.get("/api/attachments/999999/download") assert resp.status_code == 404 + + +@pytest.mark.asyncio +async def test_attachment_download_url_routes_to_the_download_endpoint(app): + """The two serializers no longer hand-format this path (#3072) — but a + single definition is only worth having if it still matches the route. Pin + it by MATCHING against the real URL map rather than comparing to a literal: + a string equality test would pass just as happily after someone renamed the + route, which is the exact drift the helper exists to prevent.""" + built = attachment_download_url(4242) + endpoint, args = app.url_map.bind("localhost").match(built) + assert endpoint == "attachments.download" + assert args == {"attachment_id": 4242} From 5a0e1bbd03c8a8f7f80658d2e4e7120baca38455 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 27 Aug 2026 07:51:23 -0400 Subject: [PATCH 09/16] perf(ml): batch the auto-apply sweeps' image_tag inserts (#3072) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Item 1 of #3072. Both sweeps issued a single-row pg_insert(image_tag) from inside their per-image loop. Steady state that is nothing; a first sweep over a back-catalogue is one round-trip per applied tag, tens of thousands of them. Each chunk now collects its rows and writes them in one statement. The ticket suggested one insert per chunk PER TAG. A single multi-row VALUES carries every tag at once, so it is one statement per chunk full stop — and the sweeps already accumulate across all heads before they commit, so nothing had to be restructured to allow it. Not a new helper: wip_title.apply_wip_image_tags was already doing the chunked ON CONFLICT DO NOTHING insert, so that shape is extracted to services/image_tag_apply.insert_image_tags and all three writers share it. The extraction deliberately leaves wip_title's pre-SELECT behind rather than pulling it into the shared function — the sweeps don't need it (their `skip` sets already exclude applied and rejected images) and it exists only to produce an accurate count, which the sweeps also compute themselves. So the shared primitive returns nothing: psycopg reports rowcount -1 for a multi-row ON CONFLICT DO NOTHING insert, and a count taken from the statement would be a lie rather than an approximation. Ordering note for the system-tag sweep: tag rows are now written after that chunk's PresentationReview rows rather than interleaved before them. Safe — PresentationReview FKs to image_record and tag, not to image_tag. Chunk size stays 5000: 5000 rows x 3 bound params = 15000, inside Postgres' 65535-parameter ceiling with room to spare. tests/test_image_tag_apply.py covers the primitive directly, since it is now the single place three writers can be wrong at once — most importantly that a re-run never restamps a hand-applied tag's source, which would silently poison head training (it excludes the auto sources). Left alone: _insert_presentation_review is still per-row, and the retract path still deletes per-row. Both operate on sets that are small by construction, unlike the apply path. Refs #3072 --- backend/app/services/image_tag_apply.py | 51 ++++++++++ backend/app/services/ml/heads.py | 33 ++++--- backend/app/services/wip_title.py | 14 +-- tests/test_image_tag_apply.py | 119 ++++++++++++++++++++++++ 4 files changed, 195 insertions(+), 22 deletions(-) create mode 100644 backend/app/services/image_tag_apply.py create mode 100644 tests/test_image_tag_apply.py diff --git a/backend/app/services/image_tag_apply.py b/backend/app/services/image_tag_apply.py new file mode 100644 index 0000000..65257f4 --- /dev/null +++ b/backend/app/services/image_tag_apply.py @@ -0,0 +1,51 @@ +"""Bulk, idempotent writes to the ``image_tag`` association table. + +Three writers attach tags to images in bulk: the WIP-title backfill +(`wip_title.apply_wip_image_tags`), the concept-head auto-apply sweep and the +system-tag auto-apply sweep (both in `ml/heads.py`). The two sweeps used to +issue ONE INSERT PER ROW from inside their per-image loop — fine in steady +state, but a first pass over a back-catalogue is tens of thousands of +individual round-trips (#3072). All three share this one chunked multi-row +insert now. + +Sync only: every caller runs on a sync ``Session`` (the Celery task path). No +async service writes image_tag in bulk, so there is no async sibling to keep in +step — unlike `db_helpers.get_or_create`, which does have one. +""" + +from __future__ import annotations + +from sqlalchemy.dialects.postgresql import insert as pg_insert +from sqlalchemy.orm import Session + +from ..models.tag import image_tag + +# 5000 rows x 3 bound params = 15000, comfortably inside Postgres' 65535-param +# ceiling for a single statement. Raising this past ~21000 rows would exceed it. +INSERT_CHUNK = 5000 + + +def insert_image_tags( + session: Session, rows: list[dict], *, chunk: int = INSERT_CHUNK +) -> None: + """Attach ``rows`` to their images, skipping any tag already on one. + + Each row is ``{"image_record_id": int, "tag_id": int, "source": str}``. + Does NOT commit — the caller owns the transaction. + + ON CONFLICT DO NOTHING against the (image_record_id, tag_id) primary key, + so an existing tag keeps its ORIGINAL ``source``: re-running a sweep can + never re-stamp a tag the operator applied by hand as machine-applied. + + Returns nothing on purpose. psycopg reports ``rowcount`` -1 for a multi-row + ON CONFLICT DO NOTHING insert (it runs via an executemany path), so a count + taken from the statement would be a lie rather than an approximation. + Callers that need an accurate count derive it themselves — see + `wip_title.apply_wip_image_tags`' pre-SELECT, and the sweeps' `skip` sets. + """ + for start in range(0, len(rows), chunk): + session.execute( + pg_insert(image_tag) + .values(rows[start:start + chunk]) + .on_conflict_do_nothing(index_elements=["image_record_id", "tag_id"]) + ) diff --git a/backend/app/services/ml/heads.py b/backend/app/services/ml/heads.py index dbaa8cf..c6d2a02 100644 --- a/backend/app/services/ml/heads.py +++ b/backend/app/services/ml/heads.py @@ -41,6 +41,7 @@ from ...models import ( TagSuggestionRejection, ) from ...models.tag import CHROME_SYSTEM_TAGS, PROCESS_SYSTEM_TAGS, image_tag +from ..image_tag_apply import insert_image_tags from .training_data import ( _AUTO_SOURCES, _applied_or_rejected, @@ -757,6 +758,10 @@ def auto_apply_sweep( Xn = _l2norm(np.vstack([emb[i] for i in cids]).astype(np.float32), np) probs = _sigmoid(Xn @ W.T + b, np) # (N, H) scanned += len(cids) + # Collected across every head, then written as ONE insert below. Was an + # insert per applied tag from inside this loop, which on a first sweep + # over a back-catalogue is tens of thousands of round-trips (#3072). + pending: list[dict] = [] for h in range(len(rows)): tid = tag_ids[h] for idx in np.where(probs[:, h] >= thr[h])[0]: @@ -766,12 +771,12 @@ def auto_apply_sweep( skip[tid].add(iid) applied[h] += 1 if not dry_run: - session.execute( - pg_insert(image_tag) - .values(image_record_id=iid, tag_id=tid, source="head_auto") - .on_conflict_do_nothing() - ) + pending.append({ + "image_record_id": iid, "tag_id": tid, + "source": "head_auto", + }) if not dry_run: + insert_image_tags(session, pending) session.commit() run.last_progress_at = datetime.now(UTC) session.commit() @@ -913,6 +918,11 @@ def system_tag_auto_apply_sweep( if Wc is not None: max_c, arg_c = _conflict_scores(Xn, Wc, bc, np) # (N,), (N,) scanned += len(cids) + # Same batching as auto_apply_sweep (#3072): collect the chunk's rows + # and write them once, below. The PresentationReview rows stay per-row — + # they FK to image_record/tag, not to image_tag, so writing the tags + # after them is safe, and a flagged conflict is rare by construction. + pending: list[dict] = [] for p in range(len(pres)): tid = pres_tag_ids[p] for idx in np.where(probs[:, p] >= thr)[0]: @@ -922,14 +932,10 @@ def system_tag_auto_apply_sweep( skip[tid].add(iid) applied[p] += 1 if not dry_run: - session.execute( - pg_insert(image_tag) - .values( - image_record_id=iid, tag_id=tid, - source=source, - ) - .on_conflict_do_nothing() - ) + pending.append({ + "image_record_id": iid, "tag_id": tid, + "source": source, + }) # Guard 2: also looks like real content → still apply, but flag it # for the review strip instead of silently marking (chrome hides, # process stays visible — either way the operator gets a heads-up). @@ -944,6 +950,7 @@ def system_tag_auto_apply_sweep( mode=mode, ) if not dry_run: + insert_image_tags(session, pending) session.commit() concepts = [ diff --git a/backend/app/services/wip_title.py b/backend/app/services/wip_title.py index 49a18a4..8608cab 100644 --- a/backend/app/services/wip_title.py +++ b/backend/app/services/wip_title.py @@ -20,10 +20,10 @@ family gains one member. import re from sqlalchemy import select -from sqlalchemy.dialects.postgresql import insert as pg_insert from sqlalchemy.orm import Session from ..models.tag import WIP_SYSTEM_TAG, Tag, image_tag +from .image_tag_apply import insert_image_tags # image_tag.source stamped on title-heuristic WIP tags — distinct from the other # apply sources so provenance stays legible and a future undo can target only these. @@ -113,13 +113,9 @@ def apply_wip_image_tags( to_insert = [iid for iid in chunk if iid not in already] if not to_insert: continue - session.execute( - pg_insert(image_tag) - .values([ - {"image_record_id": iid, "tag_id": tag_id, "source": source} - for iid in to_insert - ]) - .on_conflict_do_nothing(index_elements=["image_record_id", "tag_id"]) - ) + insert_image_tags(session, [ + {"image_record_id": iid, "tag_id": tag_id, "source": source} + for iid in to_insert + ]) inserted += len(to_insert) return inserted diff --git a/tests/test_image_tag_apply.py b/tests/test_image_tag_apply.py new file mode 100644 index 0000000..797414d --- /dev/null +++ b/tests/test_image_tag_apply.py @@ -0,0 +1,119 @@ +"""`insert_image_tags` — the shared bulk write behind the WIP-title backfill and +both auto-apply sweeps (#3072). + +The sweeps previously issued one INSERT per applied tag from inside their +per-image loop; they now hand this helper a chunk's worth of rows. That makes +this function the single place three writers can be wrong at once, so it is +tested directly rather than only through its callers. +""" +import pytest +from sqlalchemy import select + +from backend.app.models import ImageRecord, Tag, TagKind +from backend.app.models.tag import image_tag +from backend.app.services.image_tag_apply import insert_image_tags + +pytestmark = pytest.mark.integration + +_N = 0 + + +def _img(db_sync): + global _N + _N += 1 + rec = ImageRecord( + path=f"/images/ita/{_N}.jpg", sha256=f"a{_N:063d}", + size_bytes=1, mime="image/jpeg", width=1, height=1, + origin="imported_filesystem", integrity_status="unknown", + ) + db_sync.add(rec) + db_sync.flush() + return rec + + +def _tag(db_sync, name): + t = Tag(name=name, kind=TagKind.general) + db_sync.add(t) + db_sync.flush() + return t + + +def _rows(db_sync, tag_id): + """(image_record_id, source) pairs currently carrying `tag_id`.""" + return dict(db_sync.execute( + select(image_tag.c.image_record_id, image_tag.c.source) + .where(image_tag.c.tag_id == tag_id) + ).all()) + + +def _row(image_record_id, tag_id, source): + return { + "image_record_id": image_record_id, "tag_id": tag_id, "source": source, + } + + +def test_inserts_every_row_in_one_call(db_sync): + t = _tag(db_sync, "ita-basic") + imgs = [_img(db_sync) for _ in range(3)] + insert_image_tags( + db_sync, [_row(i.id, t.id, "head_auto") for i in imgs] + ) + assert _rows(db_sync, t.id) == {i.id: "head_auto" for i in imgs} + + +def test_spans_several_tags_in_a_single_call(db_sync): + """The sweeps accumulate across ALL heads before flushing, so one call + carries rows for different tags. A per-tag implementation would drop all + but the first.""" + t1, t2 = _tag(db_sync, "ita-multi-1"), _tag(db_sync, "ita-multi-2") + a, b = _img(db_sync), _img(db_sync) + insert_image_tags(db_sync, [ + _row(a.id, t1.id, "head_auto"), _row(b.id, t1.id, "head_auto"), + _row(a.id, t2.id, "head_auto"), + ]) + assert _rows(db_sync, t1.id) == {a.id: "head_auto", b.id: "head_auto"} + assert _rows(db_sync, t2.id) == {a.id: "head_auto"} + + +def test_an_existing_tag_keeps_its_original_source(db_sync): + """THE assertion this helper exists for. A sweep re-running over an image + the operator tagged by hand must not restamp it as machine-applied — that + would silently poison the head's own training data, which excludes the + auto sources. ON CONFLICT DO NOTHING, never DO UPDATE.""" + t = _tag(db_sync, "ita-manual") + rec = _img(db_sync) + insert_image_tags(db_sync, [_row(rec.id, t.id, "manual")]) + + insert_image_tags(db_sync, [_row(rec.id, t.id, "head_auto")]) + + assert _rows(db_sync, t.id) == {rec.id: "manual"} + + +def test_a_repeat_within_one_call_does_not_raise(db_sync): + """Two heads can both fire on the same (image, tag) inside one chunk. The + conflict is resolved by the statement, not by the caller de-duplicating.""" + t = _tag(db_sync, "ita-dupe") + rec = _img(db_sync) + insert_image_tags(db_sync, [ + _row(rec.id, t.id, "head_auto"), _row(rec.id, t.id, "head_auto"), + ]) + assert _rows(db_sync, t.id) == {rec.id: "head_auto"} + + +def test_more_rows_than_the_chunk_size_all_land(db_sync): + """The chunk exists to stay under Postgres' 65535 bound-parameter ceiling. + Driven with a tiny chunk so the split is real rather than theoretical — at + the 5000 default no test would ever reach a second statement.""" + t = _tag(db_sync, "ita-chunked") + imgs = [_img(db_sync) for _ in range(7)] + insert_image_tags( + db_sync, [_row(i.id, t.id, "head_auto") for i in imgs], chunk=2 + ) + assert _rows(db_sync, t.id) == {i.id: "head_auto" for i in imgs} + + +def test_no_rows_is_a_no_op(db_sync): + """A dry-run chunk, or a chunk where every candidate was already skipped, + hands over an empty list. `.values([])` is a SQL error, so the empty case + must never reach the statement.""" + insert_image_tags(db_sync, []) From cd5444e3aefc681b21b7b775c6727218591b8ca1 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 27 Aug 2026 09:26:58 -0400 Subject: [PATCH 10/16] ci(extension): derive the version from commit TIME, not commit count (#3092) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rule 149: an artifact's ordering key must be time-derived, never a commit count. packaging.sh's cmd_patch was a count. Why that matters here rather than in the abstract. A count is per-branch: dev and main count different histories of the SAME code. Today only main signs, so nothing has ordered the two against each other and the fault is invisible. The moment dev also publishes an extension, the two versions order by which branch accumulated more commits rather than by which is newer — and a squash-merge makes it permanent, because main gains one commit where dev gained five. dev then climbs away from main and a dev install can never cross back. That is Roundtable's 2026-08-24 incident (Scribe #2993) in a different repo: their versionCode was the branch's commit count, and it produced a channel you could enter and not leave. Measured on this repo today the old formula gives main=23, dev=24 — one apart, which is exactly how the inversion stays invisible until it strands somebody. New formula: minutes since 2020-01-01 of the LATEST commit touching a packaged extension file. Same anchor and unit Roundtable settled on. Commit time, not build time, and the difference is load-bearing: - stable while the extension is unchanged, so the ext- signature cache still hits and AMO is called once per extension CHANGE rather than once per push. Build-time minutes would re-sign on every push and never let two channels share a signature. - after a merge, main sees the same commit and derives the same number, so :latest reuses the signature :dev already produced for byte-identical code. Same code, same version, one signing. - monotonic: max() over a set that only gains members. Verified across all 24 extension-touching commits, zero non-monotonic steps. - reproducible from any checkout. Derives 1.0.3499884 on dev, 1.0.3465860 on main — both far above the last hand-set 1.0.11, so milestone 271's backfill guard is satisfied by construction rather than by an offset. Still shadow-only: nothing reads the derived value yet. Both shadow steps log it, and ci.yml's runs on dev too, so both channels' numbers are visible — that is the pair that has to stay ordered. Prior shadow observations describe the OLD formula and prove nothing about this one, so the window restarts; ci.yml says so at the step. New requirement recorded in ci-requirements.md: a depth-1 clone derives a wrong, too-low value rather than failing, so fetch-depth: 0 is load-bearing wherever packaging.sh version is called. Refs #3092, milestone 271 --- .forgejo/workflows/ci.yml | 6 +++++ ci-requirements.md | 14 ++++++---- extension/scripts/packaging.sh | 49 +++++++++++++++++++++++++++------- 3 files changed, 55 insertions(+), 14 deletions(-) diff --git a/.forgejo/workflows/ci.yml b/.forgejo/workflows/ci.yml index fccfe19..e9514a7 100644 --- a/.forgejo/workflows/ci.yml +++ b/.forgejo/workflows/ci.yml @@ -84,6 +84,12 @@ jobs: # can't be reclaimed. Logging it against real pushes first is the only # way to validate it at zero cost. # Placed before every early-exit path so it reports on all runs. + # + # The formula CHANGED on 2026-08-27 (commit count -> commit time, per + # rule 149), so observations logged before that date describe the old + # one and prove nothing about this. The window restarts here. Unlike + # build.yml's copy this runs on dev too, so both channels' numbers are + # visible — which is the pair that has to stay ordered. DERIVED=$(sh extension/scripts/packaging.sh version 2>&1 || echo "UNAVAILABLE") echo "shadow: manual=$PKG derived=$DERIVED" # ----------------------------------------------------------------- diff --git a/ci-requirements.md b/ci-requirements.md index 2d69ef4..620da37 100644 --- a/ci-requirements.md +++ b/ci-requirements.md @@ -56,11 +56,15 @@ per `docs/process.md`'s "add deps to the image when used by >1 project". - **`extension/scripts/packaging.sh` is the single definition of what ships inside the XPI.** Three consumers read from it rather than keeping their own copy: web-ext's `--ignore-files` (`extension/package.json`), the `:(exclude)` - pathspec in `ci.yml`'s `extension-version` guard, and the commit count that - derives the extension version. Three hand-kept copies of that one fact is - what allowed issue #2397. -- `build.yml`'s `sign-extension` checks out with `fetch-depth: 0` — the derived - extension version is a commit count, which a shallow clone cannot produce. + pathspec in `ci.yml`'s `extension-version` guard, and the `git log` pathspec + that derives the extension version. Three hand-kept copies of that one fact + is what allowed issue #2397. +- Jobs that derive the extension version check out with `fetch-depth: 0`. The + version is the commit TIME of the newest packaged-extension change (minutes + since 2020-01-01, per family rule 149 — never a commit count, which orders + by branch rather than by recency). A depth-1 clone sees one commit and + derives a wrong, too-low value rather than failing, so the full-history + checkout is load-bearing wherever `packaging.sh version` is called. - Callers MUST `set -f` before substituting the script's output. Without it the shell expands `test/**` against the working tree and silently narrows the pattern to whatever files exist at that moment — a failure that looks like diff --git a/extension/scripts/packaging.sh b/extension/scripts/packaging.sh index 523b871..a8aa969 100755 --- a/extension/scripts/packaging.sh +++ b/extension/scripts/packaging.sh @@ -7,7 +7,7 @@ # # 1. web-ext's --ignore-files (extension/package.json's four scripts) # 2. the :(exclude) pathspec (ci.yml's extension-version guard) -# 3. the rev-list pathspec (the derived version, below) +# 3. the git-log pathspec (the derived version, below) # # They now all read from here. POSIX sh only — CI's run shell is busybox. # @@ -68,20 +68,51 @@ cmd_major_minor() { | sed -E 's/.*"version"[[:space:]]*:[[:space:]]*"([0-9]+)\.([0-9]+).*/\1.\2/' } -# Count of commits that touched a PACKAGED extension file. Monotonic on a -# branch (the count only grows), which is a correctness requirement, not a -# nicety: Firefox refuses to install a version lower than the one present. +# 2020-01-01T00:00:00Z — the anchor for the derived patch component. Fixed +# forever; moving it would renumber every version downwards. +VERSION_EPOCH=1577836800 + +# Minutes since VERSION_EPOCH of the LATEST commit that touched a PACKAGED +# extension file. # -# Merge commits need no special handling — git's history simplification already -# prunes merges that don't change the pathspec, so --no-merges is a no-op here -# (verified on main: both forms return the same count). +# Time-derived, per family rule 149: an artifact's ordering key must never be a +# commit count. A count is per-branch — `dev` and `main` count different +# histories of the same code — so the moment BOTH channels publish, their +# versions order by which branch accumulated more commits rather than by which +# is newer. A squash-merge makes that permanent: main gains one commit where dev +# gained five, so dev climbs away from main and a dev install can never cross +# back. That is Roundtable's 2026-08-24 incident (`versionCode` was the branch's +# commit count) in a different repo. Measured here on 2026-08-27: main=23, +# dev=24 under the old formula — one apart, which is exactly how the inversion +# stays invisible until it strands somebody. +# +# Why the commit's time and not the build's: +# * MONOTONIC — max() over a set that only ever gains members. Verified +# across all 24 extension-touching commits: zero non-monotonic steps. +# * STABLE while the extension is unchanged, so an unchanged extension keeps +# its version, the ext- signature cache still hits, and AMO is +# called once per extension CHANGE rather than once per push. Build-time +# minutes would re-sign on every push and never let two channels share a +# signature. +# * SHARED ACROSS CHANNELS — after a merge, `main` sees the same commit and +# derives the same number, so `:latest` reuses the signature `:dev` already +# produced for byte-identical code. Same code, same version, one signing. +# * REPRODUCIBLE — any checkout of a commit yields that commit's version. +# +# Requires real history: a depth-1 clone sees one commit and will derive a wrong +# (too low) value. Every consumer must check out with fetch-depth: 0. cmd_patch() { root=$(git rev-parse --show-toplevel) # Unquoted on purpose: the pathspec must word-split into separate args. # Globbing is already off script-wide (set -euf above). # shellcheck disable=SC2046 - count=$(cd "$root" && git rev-list --count HEAD -- extension/ $(cmd_pathspec)) - echo "$count" + ts=$(cd "$root" && git log --format=%ct HEAD -- extension/ $(cmd_pathspec) \ + | sort -n | tail -1) + if [ -z "$ts" ]; then + echo "packaging.sh: no commit touches a packaged extension file" >&2 + exit 1 + fi + echo $(( (ts - VERSION_EPOCH) / 60 )) } cmd_version() { From 239b1ed8d90713213bd7c12ec0611a807d5b1f5a Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 27 Aug 2026 09:26:58 -0400 Subject: [PATCH 11/16] ci: build :dev images again so the dev channel can carry a build MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit build.yml triggered on main and tags only. The 2026-05-26 comment gave the reason: "operator tests from :latest after merge-to-main, not from the dev branch image. Saves one full docker build per dev push." That trade has since been named as a fault. Family rule 147 — main IS production, test on :dev, never by shipping — and rule 146 — a rolling channel refreshes itself, and a channel that can only be refreshed by shipping is not a channel. 146's note on 147 describes this exact shape: the pressure to test by shipping does not come from carelessness, it comes from :dev being unable to carry the build. Two live consequences, not hypotheticals: - docker-compose.yml pins fabledcurator:dev, an image nothing has published since May. The registry-image path of the documented quick-start could not have worked. - trying an extension change required merging to main, because sign-extension is gated to main and :dev did not exist to carry an XPI. Shipping was the only way to test. All three images build on dev. Deliberate: a :dev web image paired with a stale :dev ml or agent is a worse trap than no dev channel, because the mismatch surfaces as a runtime failure rather than a missing tag. The cost the 2026-05-26 note was avoiding is real and is now paid on every dev push — layer reuse should keep ml's cost to the COPY layers, but if it bites, narrowing is a `paths:` filter away. :dev only. The dev path never writes :c-: that is the rollback unit (rule 145), and a rolling tag may legitimately carry newer contents than the :c- of the same commit. This does NOT yet put an XPI on :dev — sign-extension is still gated to main, and ungating it has to wait for the derived version to control publishing, or dev would sign the hand-set 1.0.11, hit the existing cache and ship main's stale XPI. That is the next step. --- .forgejo/workflows/build.yml | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/.forgejo/workflows/build.yml b/.forgejo/workflows/build.yml index a63cee9..e5ffbd6 100644 --- a/.forgejo/workflows/build.yml +++ b/.forgejo/workflows/build.yml @@ -2,10 +2,18 @@ name: Build images on: push: - # `:dev` builds dropped 2026-05-26 — operator tests from `:latest` after - # merge-to-main, not from the dev branch image. Saves one full docker - # build per dev push. - branches: [main] + # `:dev` builds were dropped 2026-05-26 to save a docker build per dev + # push, on the reasoning that "operator tests from `:latest` after + # merge-to-main". Restored 2026-08-27: that is testing by shipping, and + # family rules 146/147 now name it directly — `main` IS production, and a + # channel that can only be refreshed by shipping is not a channel. The + # pressure to merge in order to try something does not come from + # carelessness; it comes from `:dev` being unable to carry the build. + # + # All three images build on dev, deliberately: a `:dev` web image paired + # with a stale `:dev` ml or agent is a worse trap than no dev channel at + # all, since the mismatch only shows up as a runtime failure. + branches: [main, dev] # Tag-push triggers an immutable per-version image build (e.g. # `:v26.05.26.5`) — gives a real rollback story alongside the floating # `:main` / `:latest`. Layer reuse keeps the registry-storage cost @@ -279,9 +287,11 @@ jobs: # rollback unit"). Rollback to any commit # becomes `docker pull …:c-` without a # release ceremony. - # anything else → safety net; shouldn't fire given the `on:` - # config above. Tag :dev to surface the - # unexpected run in the registry. + # refs/heads/dev → push to dev: publish :dev, the rolling test + # channel (family rule 146). Rolling means it may + # carry newer contents than the :c- of the + # same commit; it never writes :c- itself, + # because that is the rollback unit (rule 145). # POSIX-safe substring (the runner shell is dash/BusyBox sh, not # bash — `${var:0:7}` errors with "Bad substitution"; cut works # everywhere). Operator-flagged 2026-06-01 after first :c- From 5447a40e974db30731d764a9dfa4b48ed6c37ff3 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 27 Aug 2026 10:45:17 -0400 Subject: [PATCH 12/16] ci(extension): the derived version drives signing (milestone 271 step 4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cutover. sign-extension no longer reads the version out of the repo — it runs packaging.sh version and stamps the result into manifest.json and package.json in the working tree before web-ext sees them. Never committed back: the commit carrying the bump would itself be a change to the extension and would move the version again. Shadow mode ends here, in both build.yml and ci.yml. It had one job — validate the formula at zero cost before a real AMO version was burned — and CI confirmed it on 239b1ed: shadow: manual=1.0.11 derived=1.0.3499884. build-web re-derives rather than being handed the value, so it gains fetch-depth: 0. It was the outstanding landmine: a depth-1 clone derives a WRONG, too-low version rather than failing, and would then 404 fetching a release that exists under its real name. sign-extension and extension-version already had full history. New guard, and it stays permanently: refuse to sign when the derived version is strictly OLDER than the highest ext-* release already signed. Firefox rejects a downgrade and AMO never releases a burned version, so backwards is unrecoverable — it strands every install that took the higher one. Strictly older, not older-or-equal: equality is the ordinary case, an unchanged extension deriving the same version it did last build, which is exactly what makes the ext- cache hit and holds AMO to one call per extension CHANGE rather than per push. The release list is paginated because ext-* shares it with the v* tags, and the bound fails rather than calling the highest it happened to see the highest there is. First derived value is 1.0.3499884 against a highest-signed ext-1.0.10, so the backfill direction is right by six orders of magnitude. 1.0.11 sits in the repo and was never signed; nothing is stranded by skipping past it. Still main-only. Step 6 ungates sign-extension to dev, which is what actually puts an XPI on :dev. Note for step 5: ci.yml's manual-bump guard is now false. It still demands a hand bump when a packaged file changes, and that bump no longer decides anything — the derived value overwrites it at build time. Harmless but pointless, and it should be retired before the next extension change. --- .forgejo/workflows/build.yml | 151 +++++++++++++++++++++++++++++------ .forgejo/workflows/ci.yml | 17 ---- 2 files changed, 127 insertions(+), 41 deletions(-) diff --git a/.forgejo/workflows/build.yml b/.forgejo/workflows/build.yml index e5ffbd6..f63df7d 100644 --- a/.forgejo/workflows/build.yml +++ b/.forgejo/workflows/build.yml @@ -49,36 +49,104 @@ jobs: steps: - uses: actions/checkout@v4 with: - # Full history: the shadow-mode step below derives a version from a - # commit count, which a depth-1 clone cannot produce. Harmless for - # everything else in this job. + # Full history is load-bearing, not a convenience: the version this + # job signs is derived from the commit TIME of the newest packaged + # extension change. A depth-1 clone sees one commit and derives a + # wrong, too-low value rather than failing (ci-requirements.md). fetch-depth: 0 - - name: Resolve extension version + # The version is DERIVED, not read from the repo (milestone 271 step 4, + # cut over 2026-08-27). `packaging.sh version` returns MAJOR.MINOR from + # manifest.json plus a patch component that is the commit TIME of the + # newest change to a PACKAGED extension file, in minutes since + # 2020-01-01 — family rule 149, never a commit count, which orders by + # branch rather than by recency. + # + # The committed "version" in manifest.json / package.json no longer + # decides anything: the stamp step below overwrites it in the working + # tree before web-ext ever reads it. It is deliberately NOT committed + # back — the commit carrying the bump would itself be a change to the + # extension and would move the version again. The repo holds the source; + # the build derives the label. + - name: Derive extension version id: extver run: | - VERSION=$(grep -E '"version"' extension/package.json | head -1 | sed -E 's/.*"version"[[:space:]]*:[[:space:]]*"([^"]+)".*/\1/') + set -eu + VERSION=$(sh extension/scripts/packaging.sh version) echo "version=$VERSION" >> "$GITHUB_OUTPUT" - echo "Resolved extension version: $VERSION" + echo "Derived extension version: $VERSION" - # --- shadow mode (milestone #271, step 2) --------------------------- - # Informational ONLY — nothing downstream reads this, and it must never - # fail the build. This is THE place the derived formula gets validated: - # `sign-extension` only runs on main, so main pushes are the sole source - # of truth for whether the derived version moves exactly when the shipped - # extension changes. Compare these lines across several main builds - # before step 4 lets the derived value control publishing. - - name: Shadow — derived version (informational) + # Firefox refuses a downgrade and AMO never releases a burned version, + # so a version that moves BACKWARDS is unrecoverable: it strands every + # install that already took the higher one. Two ways it could happen — + # a checkout without full history (derives too low), or a rewritten + # history that drops the newest packaged commit. + # + # The test is `derived < highest already signed`, strictly. Equality is + # the ORDINARY case, not a fault: an unchanged extension derives the same + # version it did last build, which is exactly what lets the ext- + # cache hit and holds AMO to one call per extension CHANGE. Only moving + # backwards is a failure, so this runs on every path — cache hit + # included — rather than only before a sign. + - name: Guard — the derived version must never go backwards + env: + TOKEN: ${{ secrets.RELEASE_TOKEN }} + DERIVED: ${{ steps.extver.outputs.version }} run: | - set -u - DERIVED=$(sh extension/scripts/packaging.sh version 2>&1 || echo "UNAVAILABLE") - MANUAL=${{ steps.extver.outputs.version }} - echo "shadow: manual=$MANUAL derived=$DERIVED sha=$GITHUB_SHA" - if [ "$MANUAL" = "$DERIVED" ]; then - echo "shadow: manual and derived agree" - else - echo "shadow: DIVERGENT — expected until step 4 cuts over; derived is authoritative-to-be" - fi + python3 - <<'PY' + import json, os, sys, urllib.request + + API = ("https://git.fabledsword.com/api/v1/repos/" + "bvandeusen/FabledCurator/releases") + headers = {"Authorization": "token " + os.environ["TOKEN"]} + + # Paginated rather than first-page-only: ext-* releases share this + # list with the v* release tags, so one page would start missing them + # as those accumulate. The bound FAILS rather than silently scanning + # part of the list and calling the highest it saw the highest there is. + tags = [] + for page in range(1, 21): + req = urllib.request.Request( + f"{API}?limit=50&page={page}", headers=headers) + with urllib.request.urlopen(req, timeout=30) as resp: + batch = json.load(resp) + if not batch: + break + tags += [r.get("tag_name", "") for r in batch] + else: + sys.exit("guard: >1000 releases — pagination bound reached") + + def parse(v): + try: + return tuple(int(part) for part in v.split(".")) + except ValueError: + return None + + derived_s = os.environ["DERIVED"] + derived = parse(derived_s) + if derived is None: + sys.exit(f"guard: derived version {derived_s!r} is not numeric") + + signed = sorted( + (v, t) for t in tags if t.startswith("ext-") + for v in [parse(t[4:])] if v + ) + if not signed: + print("guard: no ext-* release yet — nothing to go backwards from") + raise SystemExit(0) + + hi, hi_tag = signed[-1] + print(f"guard: derived={derived_s} highest already signed={hi_tag}") + if derived < hi: + sys.exit( + f"REFUSING TO SIGN: derived {derived_s} is OLDER than the " + f"already-signed {hi_tag}. Firefox would reject it as a " + f"downgrade, and AMO will not release the burned version. " + f"First thing to check: did this job check out with " + f"fetch-depth: 0?" + ) + print("guard: ok") + PY - name: Check Forgejo release-asset cache id: cache @@ -116,6 +184,29 @@ jobs: # removal — sign-extension's job is just to ensure the cache # exists on Forgejo; the build-web side reads it independently). + # web-ext signs whatever manifest.json says, so the derived value has to + # reach the tree before signing. package.json is written too: the two are + # required to agree (ci.yml's guard), and a local `npm run build` reads + # it. Working tree only — never committed, per the note on the derive + # step. + - name: Stamp the derived version into manifest.json + package.json + env: + DERIVED: ${{ steps.extver.outputs.version }} + run: | + python3 - <<'PY' + import json, os + + version = os.environ["DERIVED"] + for path in ("extension/manifest.json", "extension/package.json"): + with open(path) as fh: + doc = json.load(fh) + doc["version"] = version + with open(path, "w") as fh: + json.dump(doc, fh, indent=2) + fh.write("\n") + print(f"{path}: version -> {version}") + PY + - name: Sign via AMO (cache miss) if: steps.cache.outputs.cached != 'true' run: | @@ -199,6 +290,12 @@ jobs: image: git.fabledsword.com/bvandeusen/ci-python:3.14 steps: - uses: actions/checkout@v4 + with: + # Full history: this job RE-DERIVES the extension version rather than + # being handed it, and a depth-1 clone derives a wrong, too-low value + # rather than failing — which would 404 the download of a release + # that exists perfectly well under its real name. + fetch-depth: 0 - name: Download signed XPI from Forgejo release asset (main + tags) # Fires on main-push AND on tag-push. Tag-push builds re-package the @@ -221,7 +318,13 @@ jobs: TOKEN: ${{ secrets.RELEASE_TOKEN }} run: | set -eux - VERSION=$(grep -E '"version"' extension/package.json | head -1 | sed -E 's/.*"version"[[:space:]]*:[[:space:]]*"([^"]+)".*/\1/') + # Re-derived, not read from the repo: sign-extension published + # ext-, and the committed version has been inert since + # milestone 271 step 4. Both jobs run `packaging.sh version` over the + # same commit, so they agree by construction — and if they ever + # didn't, this download 404s and the build fails loudly instead of + # shipping a stale XPI. + VERSION=$(sh extension/scripts/packaging.sh version) # Poll for the ext- release. main-push's sign-extension # step (AMO round-trip, 1-5min) needs to finish + upload before # tag-push can fetch. 30s * 20 = up to 10min wait, then hard-fail. diff --git a/.forgejo/workflows/ci.yml b/.forgejo/workflows/ci.yml index e9514a7..71e5caa 100644 --- a/.forgejo/workflows/ci.yml +++ b/.forgejo/workflows/ci.yml @@ -77,23 +77,6 @@ jobs: test -n "$PKG" || { echo "ERROR: no version found in extension/package.json"; exit 1; } test -n "$MAN" || { echo "ERROR: no version found in extension/manifest.json"; exit 1; } - # --- shadow mode (milestone #271, step 2) ------------------------- - # Informational ONLY: nothing below reads DERIVED, and this must never - # fail the job. `web-ext sign` is one-shot per version (AMO 409s on a - # repeat), so a wrong formula would burn a real version number that - # can't be reclaimed. Logging it against real pushes first is the only - # way to validate it at zero cost. - # Placed before every early-exit path so it reports on all runs. - # - # The formula CHANGED on 2026-08-27 (commit count -> commit time, per - # rule 149), so observations logged before that date describe the old - # one and prove nothing about this. The window restarts here. Unlike - # build.yml's copy this runs on dev too, so both channels' numbers are - # visible — which is the pair that has to stay ordered. - DERIVED=$(sh extension/scripts/packaging.sh version 2>&1 || echo "UNAVAILABLE") - echo "shadow: manual=$PKG derived=$DERIVED" - # ----------------------------------------------------------------- - # (1) Unconditional: the two version strings must agree. `web-ext sign` # reads manifest.json (package.json sits in --ignore-files and isn't # even inside the XPI), so AMO signs MAN and Firefox installs MAN. From 9eb946b21bd3d1ed394054b54dd7be4894b4660f Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 27 Aug 2026 10:56:58 -0400 Subject: [PATCH 13/16] ci(extension): sign on dev too, and bundle the XPI into :dev (step 6) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The step the milestone exists for. sign-extension ungates from main-only to main-or-dev, and build-web downloads the XPI on dev as well, so a dev push produces an image carrying the extension that is being developed rather than requiring a merge to try one. Not two signatures. The version is the commit TIME of the newest packaged extension change, so dev and main derive the SAME number for the same source. A dev push that changes the extension signs it; the merge to main finds the ext- release already there, hits the cache, and bundles the byte-identical XPI into :latest with no second AMO call. One signature per extension CHANGE, shared by both channels. That property is what makes two channels affordable at all, and it is why step 4 had to land first: ungating this while the version was still the hand-set 1.0.11 would have found the existing ext-1.0.11 release, skipped AMO, and bundled main's stale XPI into :dev — a dev channel confidently serving old code. Tags stay excluded. The tag path deliberately skips signing and polls for the release instead (the 2026-05-27 race). The ext- release's target_commitish moves from the literal "main" to $GITHUB_SHA. Either branch can create that release now, and tagging a dev-signed XPI against a main commit that need not even contain the source it was built from is a lie that costs nothing to avoid. Known, not addressed here: two concurrent builds that both derive the same unsigned version will both call AMO and the loser gets a 409. The window already existed between main and tag pushes; dev signing widens it. It fails loudly rather than shipping anything wrong, and the rollback trap cleans up the empty release. Filed separately. Also unchanged here: ci.yml's manual-bump guard is still in place and still false. It does not fire on this commit — nothing packaged changed — but it will fail the lane on the next extension change, demanding a bump that no longer decides anything. Step 5 next. --- .forgejo/workflows/build.yml | 54 ++++++++++++++++++++++++++---------- 1 file changed, 40 insertions(+), 14 deletions(-) diff --git a/.forgejo/workflows/build.yml b/.forgejo/workflows/build.yml index f63df7d..ca758ff 100644 --- a/.forgejo/workflows/build.yml +++ b/.forgejo/workflows/build.yml @@ -33,16 +33,31 @@ jobs: # Forgejo release exists yet, otherwise downloads the cached signed XPI. # Result is uploaded as an Actions artifact for build-web to consume. # - # Why this lives in build.yml (not a separate workflow): the merge-commit's - # docker image tagged `:latest` MUST carry the XPI. A separate sign workflow - # racing build.yml leaves `:latest` without the XPI for ~5min (until the - # commit-back triggers another build). Inline ordering eliminates the race. + # Why this lives in build.yml (not a separate workflow): the image a push + # publishes MUST carry the XPI. A separate sign workflow racing build.yml + # leaves that image without one for ~5min (until the commit-back triggers + # another build). Inline ordering eliminates the race. # Cache strategy: Forgejo Release Assets — picked 2026-05-25 over Generic # Packages (cleaner API surface) and commit-back-to-side-branch (no extra # branch to manage). AMO blocks re-signing the same version (returns 409), - # so signing is intentionally one-shot per version bump. + # so signing is intentionally one-shot per version. + # + # BOTH branches sign (milestone 271 step 6, 2026-08-27). Not two signatures: + # the version is the commit TIME of the newest packaged-extension change, so + # dev and main derive the SAME number for the same extension source. A dev + # push that changes the extension signs it; the merge to main then finds the + # ext- release already there, hits the cache, and bundles the + # byte-identical XPI into `:latest` with no second AMO call. One signature + # per extension CHANGE, shared by both channels — that is what makes two + # channels affordable, and it is why step 4 (derived version) had to land + # first. Ungating this while the version was still the hand-set 1.0.11 would + # have hit the existing ext-1.0.11 cache and bundled MAIN's stale XPI into + # `:dev` — a dev channel confidently serving old code. + # + # Tags stay excluded: the tag path deliberately skips signing and polls for + # the release instead (see build-web's race note, 2026-05-27). sign-extension: - if: github.ref == 'refs/heads/main' + if: github.ref == 'refs/heads/main' || github.ref == 'refs/heads/dev' runs-on: python-ci container: image: git.fabledsword.com/bvandeusen/ci-python:3.14 @@ -233,6 +248,11 @@ jobs: # created it so an upload failure below can roll back (don't # leave an empty release tombstone that the next run's # cache-check mistakes for a partial-failure state). + # + # target_commitish is the signing commit, not a branch name: since + # step 6 either branch can create this release, and hard-coding + # `main` would tag a dev-signed XPI against a main commit that may + # not even contain the extension source it was built from. STATUS=$(curl -s -o release.json -w "%{http_code}" \ -H "Authorization: token $TOKEN" \ "https://git.fabledsword.com/api/v1/repos/bvandeusen/FabledCurator/releases/tags/ext-$VERSION" || echo 000) @@ -240,7 +260,7 @@ jobs: CREATED_BY_US=false else curl -s -X POST -H "Authorization: token $TOKEN" -H "Content-Type: application/json" \ - -d "{\"tag_name\":\"ext-$VERSION\",\"name\":\"Extension $VERSION (signed XPI cache)\",\"body\":\"Internal cache for the signed XPI consumed by build.yml's build-web job. Not a user-facing FC release.\",\"target_commitish\":\"main\"}" \ + -d "{\"tag_name\":\"ext-$VERSION\",\"name\":\"Extension $VERSION (signed XPI cache)\",\"body\":\"Internal cache for the signed XPI consumed by build.yml's build-web job. Not a user-facing FC release.\",\"target_commitish\":\"$GITHUB_SHA\"}" \ -o release.json \ "https://git.fabledsword.com/api/v1/repos/bvandeusen/FabledCurator/releases" CREATED_BY_US=true @@ -283,7 +303,10 @@ jobs: build-web: needs: [sign-extension] - # sign-extension is main-only; on dev it's skipped, build-web still runs. + # sign-extension runs on main and dev, and is skipped on a tag push (which + # polls for the release instead). Either is fine to build on; a FAILED sign + # is not — this condition lets success and skipped through, so a failure + # skips build-web rather than shipping an image without the XPI. if: always() && (needs.sign-extension.result == 'success' || needs.sign-extension.result == 'skipped') runs-on: python-ci container: @@ -297,11 +320,14 @@ jobs: # that exists perfectly well under its real name. fetch-depth: 0 - - name: Download signed XPI from Forgejo release asset (main + tags) - # Fires on main-push AND on tag-push. Tag-push builds re-package the - # same source code as the preceding main-push build but with an - # immutable version tag — they need the XPI too, otherwise the - # versioned image ships without the signed extension. + - name: Download signed XPI from Forgejo release asset + # Fires on every trigger shape. dev and main each bundle the XPI their + # own sign-extension just published — that is the whole point of the + # channel work (milestone 271 step 6): the dev image carries the + # extension being developed, rather than requiring a merge to try it. + # Tag-push builds re-package the same source as the preceding main-push + # build but with an immutable version tag — they need the XPI too, + # otherwise the versioned image ships without the signed extension. # # Tag-push vs main-push race (operator-flagged 2026-05-27 after # v26.05.27.0 hit it): a release cut fires BOTH workflows almost @@ -313,7 +339,7 @@ jobs: # for up to 10min total) before giving up. Main-push's signing # eventually wins and tag-push picks the release up on a later # iteration. - if: github.ref == 'refs/heads/main' || startsWith(github.ref, 'refs/tags/') + if: github.ref == 'refs/heads/main' || github.ref == 'refs/heads/dev' || startsWith(github.ref, 'refs/tags/') env: TOKEN: ${{ secrets.RELEASE_TOKEN }} run: | From fe48e77821f4b2ccd4cd93647b20f8223392836d Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 27 Aug 2026 11:29:44 -0400 Subject: [PATCH 14/16] ci(extension): retire the manual-bump guard, true up the docs (step 5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The guard asked whether a packaged extension file changed without the version moving. Since step 4 nobody moves the version by hand, so it was checking a fact that had stopped existing — and it was not merely dead weight: it would have failed the lane on every real extension change, demanding a bump that decides nothing. Removed rather than left running beside the new mechanism (rule 22). What replaces it is thinner and true. The extension-version lane now asserts the derivation resolves on this commit, that the derived value is the plain dotted-numeric shape AMO accepts, and that MAJOR.MINOR agrees between manifest.json and package.json. MAJOR.MINOR is the one part still hand-set, and packaging.sh reads it from manifest.json ALONE, so a divergence ships a version package.json disagrees with. The lane keeps fetch-depth: 0 — checking that the derivation survives a real checkout is half its remaining value. Deliberately not checked there: that the derived value beats what is already signed. That guard belongs in build.yml, where it compares against the real ext-* releases. Comparing against origin/main in a lane would be wrong, because dev legitimately derives a LOWER value whenever main is ahead on the extension, and a lane that fails for being behind is a lane people learn to ignore. packaging.sh is down to two consumers from three. version.spec.js's "ci.yml derives its pathspec" test would have gone red on that, so it is rewritten to assert the property rather than the consumer: no workflow inlines an :(exclude)extension/ literal, across all three. That keeps the #2397 anti-regression value while surviving consumers coming and going. A second test pins build.yml to packaging.sh version and fails if it goes back to grepping the committed value — which is not a style regression but the #3092 bug itself. build.yml joins extension.yml's trigger paths, since the suite now asserts against it. The lockstep test narrows from the whole version string to MAJOR.MINOR. The committed patch numbers are inert now; asserting on them would fail for a difference that changes nothing. Docs. extension/README.md's Release section described extension.yml signing on main and committing the XPI into frontend/public/ — untrue since 2026-05-25, and it told the reader to hand-bump both files, which is now exactly the wrong instruction. Rewritten, with a Versioning section that says plainly that editing the patch number does nothing and why the key is commit time rather than a count. ci-requirements.md drops the third packaging.sh consumer and names every job that needs full history. Root README no longer claims the extension is signed on main only. --- .forgejo/workflows/ci.yml | 141 ++++++++++--------------------- .forgejo/workflows/extension.yml | 11 ++- README.md | 4 +- ci-requirements.md | 30 ++++--- extension/README.md | 46 ++++++++-- extension/test/version.spec.js | 51 +++++++++-- 6 files changed, 158 insertions(+), 125 deletions(-) diff --git a/.forgejo/workflows/ci.yml b/.forgejo/workflows/ci.yml index 71e5caa..17a6c9f 100644 --- a/.forgejo/workflows/ci.yml +++ b/.forgejo/workflows/ci.yml @@ -2,7 +2,7 @@ name: CI # CI lanes per FabledRulebook/forgejo.md "CI philosophy": # - lint: ruff only, no dep install — fast-fail for the common lint bounce. -# - extension-version: guards the extension publish path (see the job). +# - extension-version: the derived version resolves and MAJOR.MINOR agrees. # - backend-lint-and-test: `pytest -m "not integration"`, no service containers. # - frontend-build: vitest unit + vite build. # - integration: pgvector + redis service containers; alembic + `pytest -m integration`. @@ -42,18 +42,29 @@ jobs: # catching syntax errors before the image build. run: python -m compileall -q agent/fc_agent - # Guards the extension publish path, which has no self-correcting behavior. + # The extension version is DERIVED, not hand-maintained (milestone 271 step + # 4): build.yml computes it from the commit TIME of the newest packaged + # extension change and stamps it into manifest.json / package.json at build + # time. The guard that used to live here — "packaged files changed but nobody + # bumped the version" — was therefore checking a fact that had stopped + # existing. Worse than useless: it would have failed this lane on every real + # extension change, demanding a bump that decides nothing. Retired 2026-08-27 + # rather than left running beside the new mechanism (rule 22). # - # build.yml's sign-extension job keys its AMO-signing cache purely on the - # version string in extension/package.json: if an `ext-` Forgejo - # release already carries an XPI, signing is SKIPPED and that old signed XPI - # is what build-web bakes into `:latest`. Nothing in that path inspects - # whether extension/ actually changed — so a forgotten version bump ships a - # stale extension on a fully green build, silently. (AMO can't help: it 409s - # on re-signing a version, which is exactly why the cache exists.) + # Two things are still worth asserting, and this is the only lane that can: + # the extension.yml suite runs on node:24-slim, which is exactly why + # version.spec.js sticks to packaging.sh's git-free subcommands. + # 1. the derivation actually resolves on this commit + # 2. MAJOR.MINOR agrees between the two files — the one part still hand-set, + # and packaging.sh reads it from manifest.json ALONE, so a divergence + # ships a version package.json disagrees with # - # This job makes that case loud, on the dev push, instead of invisible at - # merge-to-main. It is pure git + text work — no deps, no services. + # Deliberately NOT checked here: that the derived value beats what has already + # been signed. That guard belongs in build.yml, where it compares against the + # real ext-* releases. Comparing against origin/main here would be wrong — + # dev legitimately derives a LOWER value whenever main is ahead on the + # extension, and a lane that fails for being behind is a lane people learn to + # ignore. extension-version: runs-on: python-ci container: @@ -61,97 +72,37 @@ jobs: steps: - uses: actions/checkout@v4 with: - # Full history: the check diffs against the push's `before` SHA (or - # the PR base), which a depth-1 clone wouldn't contain. + # The derivation needs real history: a depth-1 clone sees one commit + # and produces a wrong, too-low value RATHER THAN FAILING. Checking + # that here is half the point of the lane. fetch-depth: 0 - - name: Extension version guard - env: - BEFORE: ${{ github.event.before }} - PR_BASE: ${{ github.event.pull_request.base.sha }} + - name: Extension version derives cleanly run: | set -eu # busybox sh on the act_runner — no bashisms (family rule). - ver() { grep -E '"version"' "$1" | head -1 | sed -E 's/.*"version"[[:space:]]*:[[:space:]]*"([^"]+)".*/\1/'; } - PKG=$(ver extension/package.json) - MAN=$(ver extension/manifest.json) - test -n "$PKG" || { echo "ERROR: no version found in extension/package.json"; exit 1; } - test -n "$MAN" || { echo "ERROR: no version found in extension/manifest.json"; exit 1; } - - # (1) Unconditional: the two version strings must agree. `web-ext sign` - # reads manifest.json (package.json sits in --ignore-files and isn't - # even inside the XPI), so AMO signs MAN and Firefox installs MAN. - # build.yml keys its cache, release tag, XPI filename — and therefore - # the version /api/extension/manifest reports to the update prompt — - # on PKG. Divergence either hard-fails at AMO or ships a mislabelled - # XPI whose update prompt lies about what's installed. + VERSION=$(sh extension/scripts/packaging.sh version) + echo "derived: $VERSION" + # The shape AMO accepts, and the shape build.yml will stamp. + if ! echo "$VERSION" | grep -qE '^[0-9]+(\.[0-9]+)*$'; then + echo "ERROR: derived version '$VERSION' is not plain dotted-numeric." + echo "AMO would reject it, and build.yml stamps it verbatim." + exit 1 + fi + mm() { grep -E '"version"' "$1" | head -1 | sed -E 's/.*"version"[[:space:]]*:[[:space:]]*"([0-9]+\.[0-9]+).*/\1/'; } + MAN=$(mm extension/manifest.json) + PKG=$(mm extension/package.json) + test -n "$MAN" || { echo "ERROR: no parseable version in extension/manifest.json"; exit 1; } + test -n "$PKG" || { echo "ERROR: no parseable version in extension/package.json"; exit 1; } if [ "$MAN" != "$PKG" ]; then - echo "ERROR: extension version mismatch." - echo " extension/manifest.json = $MAN <- what AMO signs / Firefox installs" - echo " extension/package.json = $PKG <- what CI caches, names, and reports" - echo "Set both to the same value." + echo "ERROR: MAJOR.MINOR disagrees between the two files." + echo " extension/manifest.json = $MAN <- packaging.sh reads MAJOR.MINOR from here" + echo " extension/package.json = $PKG" + echo "Only MAJOR.MINOR is hand-set. The patch component is derived from" + echo "commit time and overwritten at build time, so the committed patch" + echo "numbers are inert — but MAJOR.MINOR still ships. Set both the same." exit 1 fi - - # (2) If the SHIPPED extension changed, the version must have moved. - # - # Compare against MAIN, not against the previous push. The publish - # decision is made at merge-to-main against whatever ext- - # already exists, so "differs from main" is the question that matters. - # Diffing against the previous dev push instead would demand a fresh - # bump on every iteration — push, tweak the extension again, and CI - # would insist on a second bump that buys nothing, inflating the - # version for no reason. On a main push there is no "main to compare - # to" yet, so fall back to that push's own before-SHA. - if [ "${GITHUB_REF##*/}" = "main" ]; then - BASE="${BEFORE:-}" - else - BASE=$(git rev-parse --verify -q origin/main 2>/dev/null || git rev-parse --verify -q main 2>/dev/null || echo "") - # PR base is the fallback when main isn't in the clone at all. - [ -n "$BASE" ] || BASE="${PR_BASE:-}" - fi - case "$BASE" in - ''|0000000000000000000000000000000000000000) - echo "No usable base ref (no main in clone / first push) — skipping the bump check." - echo "OK: extension version $PKG" - exit 0 - ;; - esac - if ! git cat-file -e "$BASE^{commit}" 2>/dev/null; then - echo "Base commit $BASE not in this clone — skipping the bump check." - echo "OK: extension version $PKG" - exit 0 - fi - # The exclusion list is NOT written out here — it comes from - # extension/scripts/packaging.sh, the one definition of what ships, - # shared with web-ext's --ignore-files and the derived-version patch - # count. Three hand-kept copies of that fact is how #2397 happened. - # - # `set -f` is required around the substitution: without it the shell - # globs `test/**` against the working tree and silently narrows it. - set -f - CHANGED=$(git diff --name-only "$BASE" HEAD -- extension/ $(sh extension/scripts/packaging.sh pathspec)) - set +f - if [ -z "$CHANGED" ]; then - echo "No packaged extension files changed since $BASE — nothing to guard." - echo "OK: extension version $PKG" - exit 0 - fi - echo "Packaged extension files changed since $BASE:" - echo "$CHANGED" | sed 's/^/ /' - PKG_OLD=$(git show "$BASE:extension/package.json" 2>/dev/null | grep -E '"version"' | head -1 | sed -E 's/.*"version"[[:space:]]*:[[:space:]]*"([^"]+)".*/\1/') - if [ -z "$PKG_OLD" ]; then - echo "Could not read the base version — skipping the bump check." - echo "OK: extension version $PKG" - exit 0 - fi - if [ "$PKG_OLD" = "$PKG" ]; then - echo "ERROR: packaged extension files changed but the version is still $PKG." - echo "build.yml would find the existing ext-$PKG release, skip AMO signing," - echo "and bake the OLD signed XPI into :latest — a green build shipping stale code." - echo "Bump the version in BOTH extension/package.json and extension/manifest.json." - exit 1 - fi - echo "OK: extension version $PKG_OLD -> $PKG" + echo "OK: MAJOR.MINOR $MAN, derived version $VERSION" backend-lint-and-test: runs-on: python-ci diff --git a/.forgejo/workflows/extension.yml b/.forgejo/workflows/extension.yml index c933028..f29cad9 100644 --- a/.forgejo/workflows/extension.yml +++ b/.forgejo/workflows/extension.yml @@ -10,15 +10,20 @@ on: paths: - 'extension/**' - '.forgejo/workflows/extension.yml' - # test/version.spec.js asserts ci.yml's extension-version guard never - # ignores a file web-ext actually packages, so a ci.yml-only edit can - # break this suite and must trigger it. + # test/version.spec.js asserts things ABOUT the other two workflows — + # that neither inlines the packaged-file set, and that build.yml derives + # the shipped version rather than reading it out of the repo. A + # workflow-only edit can therefore break this suite, so it has to trigger + # it. build.yml joined the list at milestone 271 step 5, when the spec + # started asserting against it. - '.forgejo/workflows/ci.yml' + - '.forgejo/workflows/build.yml' pull_request: branches: [main] paths: - 'extension/**' - '.forgejo/workflows/ci.yml' + - '.forgejo/workflows/build.yml' workflow_dispatch: jobs: diff --git a/README.md b/README.md index c13f2f6..915ffa5 100644 --- a/README.md +++ b/README.md @@ -19,7 +19,7 @@ Five deployable pieces, built by `.forgejo/workflows/build.yml`: | **Web / workers** | `Dockerfile` | `fabledcurator` | Quart API + the built Vue SPA in one image. `entrypoint.sh` picks the role: `web`, `worker`, `scheduler`. The `maintenance-long` service is a second `worker` pinned to the long-running maintenance queue. | | **ML worker** | `Dockerfile.ml` | `fabledcurator-ml` | Same app, plus `requirements-ml.txt` — tagging and embedding models that run in-container. | | **GPU agent** | `agent/Dockerfile` | `fabledcurator-agent` | Optional desktop-GPU worker (`agent/`). Leases jobs over **HTTP only** — never touches the database or Redis. Run it for a burst, stop it to reclaim the card. See `agent/README.md`. | -| **Firefox extension** | `extension/` | signed XPI | MV3 extension: pushes platform session cookies into FC and adds a creator as a Source in one click. AMO-signed on `main` only, then bundled into the web image and served from Settings → Maintenance. See `extension/README.md`. | +| **Firefox extension** | `extension/` | signed XPI | MV3 extension: pushes platform session cookies into FC and adds a creator as a Source in one click. AMO-signed on both `dev` and `main` (one signature per extension change, shared by the two channels), bundled into that channel's web image and served from Settings → Maintenance. See `extension/README.md`. | | **Data** | — | `pgvector/pgvector:pg16`, `redis:7-alpine` | Postgres with pgvector for embeddings; Redis as the Celery broker. | ## Quick start @@ -52,7 +52,7 @@ FabledCurator is designed to run inside a self-hosted homelab environment over p ## CI / Forgejo setup -Three workflows: `ci.yml` (lint, extension-version guard, backend unit tests, +Three workflows: `ci.yml` (lint, extension-version check, backend unit tests, frontend build, integration), `extension.yml` (extension lint, vitest, XPI content verification), and `build.yml` (sign + publish). diff --git a/ci-requirements.md b/ci-requirements.md index 620da37..4d0f3c4 100644 --- a/ci-requirements.md +++ b/ci-requirements.md @@ -54,17 +54,25 @@ per `docs/process.md`'s "add deps to the image when used by >1 project". shims to production code — the libs ship as `background.scripts`, not ES modules, so the specs exercise exactly the bytes packaged into the XPI. - **`extension/scripts/packaging.sh` is the single definition of what ships - inside the XPI.** Three consumers read from it rather than keeping their own - copy: web-ext's `--ignore-files` (`extension/package.json`), the `:(exclude)` - pathspec in `ci.yml`'s `extension-version` guard, and the `git log` pathspec - that derives the extension version. Three hand-kept copies of that one fact - is what allowed issue #2397. -- Jobs that derive the extension version check out with `fetch-depth: 0`. The - version is the commit TIME of the newest packaged-extension change (minutes - since 2020-01-01, per family rule 149 — never a commit count, which orders - by branch rather than by recency). A depth-1 clone sees one commit and - derives a wrong, too-low value rather than failing, so the full-history - checkout is load-bearing wherever `packaging.sh version` is called. + inside the XPI.** Two consumers read from it rather than keeping their own + copy: web-ext's `--ignore-files` (`extension/package.json`), and the `git log` + pathspec inside the script's own version derivation. It was three until + 2026-08-27 — `ci.yml`'s `extension-version` guard held the third and went when + the manual bump it guarded did (milestone 271 step 5). Hand-kept copies of + that one fact is what allowed issue #2397, so `extension/test/version.spec.js` + asserts no workflow has reintroduced a literal `:(exclude)extension/…`. +- **The shipped extension version is derived, not committed.** It is the commit + TIME of the newest packaged-extension change (minutes since 2020-01-01, per + family rule 149 — never a commit count, which orders by branch rather than by + recency). `build.yml`'s `sign-extension` computes it and stamps it into + `extension/manifest.json` + `package.json` in the working tree before signing; + the stamp is never committed. Treat the version in the repo as a base: only + its MAJOR.MINOR is read, and its patch component is inert. +- Every job that calls `packaging.sh version` checks out with `fetch-depth: 0` — + `build.yml`'s `sign-extension` and `build-web`, and `ci.yml`'s + `extension-version`. A depth-1 clone sees one commit and derives a wrong, + too-low value **rather than failing**, so the full-history checkout is + load-bearing rather than incidental. - Callers MUST `set -f` before substituting the script's output. Without it the shell expands `test/**` against the working tree and silently narrows the pattern to whatever files exist at that moment — a failure that looks like diff --git a/extension/README.md b/extension/README.md index e780d95..22db07c 100644 --- a/extension/README.md +++ b/extension/README.md @@ -7,7 +7,8 @@ page in one click. ## Install (operator) -The signed XPI is bundled into the FC Docker image. Open FC → +The signed XPI is bundled into the FC Docker image — `:dev` and +`:latest` each carry their own channel's build. Open FC → Settings → Maintenance → Browser extension → click "Install Firefox extension". Firefox shows its native install prompt. After installing, open the extension's options page (about:addons → FabledCurator → @@ -20,6 +21,7 @@ same card. cd extension/ npm install --no-save # web-ext only npm run lint # web-ext lint +npm run test:unit # vitest — lib/ logic + packaging/version checks npm run start # launches Firefox with extension loaded npm run build # unsigned XPI in web-ext-artifacts/ ``` @@ -36,10 +38,42 @@ npm run build # unsigned XPI in web-ext-artifacts/ - [ ] Subscriptions list: popup → "Sources" tab → list renders - [ ] Check now: click play icon on source row → no error toast +## Versioning — don't hand-edit the patch number + +The shipped version is **derived**, not committed. `scripts/packaging.sh +version` returns `MAJOR.MINOR` from `manifest.json` plus a patch component +that is the commit *time* of the newest change to a packaged extension file, +in minutes since 2020-01-01. `build.yml` computes it and stamps it into both +`manifest.json` and `package.json` at build time. The stamp is never +committed — the commit carrying it would itself be a change to the extension, +which would move the version again. + +So: + +- **Editing the patch number does nothing.** It is overwritten before web-ext + ever reads it. There is no bump to make, and none to forget. +- **MAJOR.MINOR is still yours.** It carries the deliberate meaning, it is read + from `manifest.json` alone, and CI fails the `extension-version` lane if the + two files disagree on it. +- `npm run build` locally produces an XPI labelled with the *committed* + version, since nothing stamped it. Fine for loading into a test profile; not + what ships. + +Why commit time and not a commit count: a count is per-branch, so `dev` and +`main` count different histories of the same code and their versions end up +ordered by which branch accumulated more commits rather than by which is newer. +Commit time gives both branches the same number for the same source — which is +exactly what lets one AMO signature serve both channels (family rule 149, FC +issue #3092). + ## Release -Bump `manifest.json` + `package.json` SemVer (both files) and commit -under `extension/**`. The `.forgejo/workflows/extension.yml` workflow -runs `web-ext sign` on main, commits the signed XPI to -`frontend/public/extension/`, and the next FC server build bundles it -into the Docker image. +Nothing to do by hand. Push to `dev`: `build.yml` signs the extension if this +change moved the version, caches the signed XPI as a Forgejo `ext-` +release, and bundles it into `fabledcurator:dev`. Merging to `main` derives the +same version, hits that cache, and bundles the byte-identical XPI into +`:latest` with no second AMO call. + +AMO refuses to re-sign a version it has already issued, so signing is one-shot +per version — which is why the cache exists and why the version must never move +backwards. diff --git a/extension/test/version.spec.js b/extension/test/version.spec.js index d39a41e..4e69a69 100644 --- a/extension/test/version.spec.js +++ b/extension/test/version.spec.js @@ -77,21 +77,56 @@ describe('consumers delegate rather than keeping their own copy', () => { } }) - it('ci.yml derives its pathspec from the script and hardcodes none', () => { - const ci = readText('..', '.forgejo', 'workflows', 'ci.yml') - expect(ci).toContain('extension/scripts/packaging.sh pathspec') - // A literal :(exclude)extension/... in the workflow means someone bypassed - // the shared definition. - expect(ci).not.toMatch(/:\(exclude\)extension\//) + const WORKFLOWS = ['ci.yml', 'build.yml', 'extension.yml'] + + it('no workflow hardcodes the packaged-file set', () => { + // ci.yml used to substitute `packaging.sh pathspec` directly, for the + // manual-bump guard that milestone 271 step 5 retired. Nothing inlines the + // set today, and nothing should start to: a literal :(exclude)extension/... + // in a workflow means someone bypassed the shared definition, which is + // exactly the drift #2397 was about. Asserted across all three rather than + // against one named consumer, so it keeps holding as consumers come and go. + for (const wf of WORKFLOWS) { + const text = readText('..', '.forgejo', 'workflows', wf) + expect(text, `${wf} inlines an :(exclude) literal`).not.toMatch(/:\(exclude\)extension\//) + } + }) + + it('build.yml takes the shipped version from the script, not from the repo', () => { + // The version is DERIVED from commit time (#3092, milestone 271 step 4). + // Going back to reading the committed value is not a style regression, it + // is the bug: a hand-set version makes dev and main sign the same number + // for different code, and the ext- cache then serves one channel + // the other's XPI. + const build = readText('..', '.forgejo', 'workflows', 'build.yml') + expect(build).toContain('packaging.sh version') + expect(build, 'build.yml re-reads the committed version instead of deriving it') + .not.toMatch(/grep[^\n]*'"version"'[^\n]*package\.json/) }) }) describe('extension version', () => { - it('keeps manifest.json and package.json in lockstep', () => { - expect(read('manifest.json').version).toBe(read('package.json').version) + const majorMinor = (v) => v.split('.').slice(0, 2).join('.') + + it('keeps the hand-set MAJOR.MINOR in lockstep across both files', () => { + // Narrowed from full-string equality at milestone 271 step 5. Since step 4 + // the patch component is derived from commit time and stamped into both + // files at build time, so the committed patch numbers are inert — nothing + // reads them and they are not what ships. Asserting on them would fail for + // a difference that changes nothing. + // + // MAJOR.MINOR is the opposite: still hand-set, still shipped, and + // packaging.sh reads it from manifest.json ALONE. Let the two diverge and + // the extension ships a version package.json disagrees with, with no other + // signal. + expect(majorMinor(read('manifest.json').version)) + .toBe(majorMinor(read('package.json').version)) }) it('uses a plain dotted numeric version AMO will accept', () => { + // The committed value seeds MAJOR.MINOR, so it still has to parse even + // though its patch component never ships. ci.yml asserts the same shape on + // the DERIVED value, which is the one AMO actually sees. expect(read('package.json').version).toMatch(/^\d+(\.\d+)*$/) }) From a7e626a67a795ac7c803df0c4ee9cda6c0b2a864 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 27 Aug 2026 11:47:30 -0400 Subject: [PATCH 15/16] feat(extension): report the channel beside the version (step 7) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the half of the ask the signing work didn't: a way to tell a dev build from a main one. FC_CHANNEL is baked into the web image at build time and /api/extension/manifest reports it as its own key, next to version — the popup banner, the toolbar tooltip and the Settings card all name it. Beside the version, never inside it. A `1.0.3499884-dev` suffix is the obvious shortcut and it is the exact failure this design comes from: versionIsNewer parses each dotted segment with parseInt, so a suffixed segment reads as 0, every dev build compares equal to every other, and "no update available" stops being distinguishable from "I cannot read this version". The comparator already degrades rather than discarding (rule 150), which is a reason not to NEED the suffix, not a licence to add one. Two tests hold the line — one backend, asserting version and channel are separate keys; one frontend, asserting the rendered version text stays the bare derived number. Optional on the read side, and absent rather than defaulted. An image built before this field says nothing by not having the key; an image built without a channel now says nothing the same way, so there is one absence to handle instead of a second spelling of "unknown". Every reader drops the label entirely when it is missing and reads exactly as it did before. Reported verbatim rather than validated against {dev, main}: if an image declares something else, showing what it claims helps whoever is debugging more than dropping it would. FC_CHANNEL is declared LAST in the Dockerfile. An ARG invalidates every layer below it, and this is the one value that differs between the dev and main builds of identical source — earlier, and the two channels could never share a cached pip install. A tag push counts as main: a vYY.MM.DD tag is cut from main, so that image is a main-channel artifact wearing an immutable name. No channel switcher, deliberately. background.js:34 already records that Firefox's static update_url cannot apply, because every FC instance is a different host — so the extension asks its configured backend, and the channel IS the instance it points at. Switching is repointing apiUrl and reinstalling from that host. A separate setting would contradict each server build shipping its own extension. This commit touches packaged extension files, so it moves the derived version and will sign a new one via AMO — the first push to exercise the extension-changed path from dev end to end. --- .forgejo/workflows/build.yml | 12 ++++ Dockerfile | 17 +++++ backend/app/api/extension.py | 26 ++++++- ci-requirements.md | 9 +++ extension/README.md | 19 +++++ extension/background/background.js | 27 ++++++- extension/popup/popup.js | 6 +- .../settings/BrowserExtensionCard.vue | 10 +++ .../components/browserExtensionCard.spec.js | 72 +++++++++++++++++++ tests/test_api_extension.py | 47 ++++++++++++ 10 files changed, 241 insertions(+), 4 deletions(-) create mode 100644 frontend/test/components/browserExtensionCard.spec.js diff --git a/.forgejo/workflows/build.yml b/.forgejo/workflows/build.yml index ca758ff..5551d68 100644 --- a/.forgejo/workflows/build.yml +++ b/.forgejo/workflows/build.yml @@ -426,13 +426,20 @@ jobs: # everywhere). Operator-flagged 2026-06-01 after first :c- # main-push build failed at this step. SHORT_SHA=$(printf '%s' "$GITHUB_SHA" | cut -c1-7) + # `channel` is baked into the image as FC_CHANNEL and reported by + # /api/extension/manifest (milestone 271 step 7). A tag-push counts as + # `main`: a vYY.MM.DD tag is cut from main, so that image is a + # main-channel artifact wearing an immutable name. if [ "${GITHUB_REF#refs/tags/}" != "${GITHUB_REF}" ]; then TAG_NAME="${GITHUB_REF#refs/tags/}" echo "tags=git.fabledsword.com/bvandeusen/fabledcurator:${TAG_NAME}" >> "$GITHUB_OUTPUT" + echo "channel=main" >> "$GITHUB_OUTPUT" elif [ "${GITHUB_REF##*/}" = "main" ]; then echo "tags=git.fabledsword.com/bvandeusen/fabledcurator:main,git.fabledsword.com/bvandeusen/fabledcurator:latest,git.fabledsword.com/bvandeusen/fabledcurator:c-${SHORT_SHA}" >> "$GITHUB_OUTPUT" + echo "channel=main" >> "$GITHUB_OUTPUT" else echo "tags=git.fabledsword.com/bvandeusen/fabledcurator:dev" >> "$GITHUB_OUTPUT" + echo "channel=dev" >> "$GITHUB_OUTPUT" fi - name: Login to Forgejo registry @@ -449,6 +456,11 @@ jobs: file: Dockerfile push: true tags: ${{ steps.tag.outputs.tags }} + # Only the web image carries a channel: it is the one that serves + # /api/extension/manifest. The ml and agent images have nothing to + # report it to. + build-args: | + FC_CHANNEL=${{ steps.tag.outputs.channel }} build-ml: runs-on: python-ci diff --git a/Dockerfile b/Dockerfile index 6c9da51..050cc04 100644 --- a/Dockerfile +++ b/Dockerfile @@ -47,6 +47,23 @@ RUN chmod +x entrypoint.sh COPY --from=frontend-builder /build/dist ./frontend/dist +# Which channel this image belongs to — `dev` or `main` (milestone 271 step 7). +# build.yml passes it; /api/extension/manifest reports it beside the version so +# an operator can tell which channel an install came from without the channel +# ever touching the version string. +# +# Empty by default, deliberately: a locally-built image then reports NO channel +# rather than claiming to be one, and the manifest omits the field entirely — +# indistinguishable from an image built before the field existed, which is +# exactly the shape every reader already has to handle. +# +# Declared LAST on purpose. An ARG/ENV invalidates every layer below it, and +# this is the one value that differs between the dev and main builds of +# identical source — put it any earlier and the two channels could never share +# a cached pip install. +ARG FC_CHANNEL="" +ENV FC_CHANNEL=${FC_CHANNEL} + EXPOSE 8080 ENTRYPOINT ["./entrypoint.sh"] diff --git a/backend/app/api/extension.py b/backend/app/api/extension.py index 082bc1e..868bab2 100644 --- a/backend/app/api/extension.py +++ b/backend/app/api/extension.py @@ -7,6 +7,7 @@ from __future__ import annotations import asyncio import hashlib import hmac +import os import re from pathlib import Path @@ -31,6 +32,12 @@ XPI_DIR = Path("/app/frontend/dist/extension") _XPI_VERSION_RE = re.compile(r"fabledcurator-(?P[\w.-]+)\.xpi$") +# Which channel this image belongs to — "dev" or "main" — baked in at build +# time from the FC_CHANNEL build arg (milestone 271 step 7). Empty for a local +# build, or for any image predating the field. Tests override by monkeypatching +# this constant, same as XPI_DIR above. +FC_CHANNEL = os.environ.get("FC_CHANNEL", "").strip() + async def _ext_key_required(session) -> bool: """Unlike /api/credentials (which accepts the browser path with no @@ -133,13 +140,30 @@ def _read_manifest_sync() -> dict | None: return None versioned.sort(key=lambda p: p.stat().st_mtime) latest = versioned[-1] - return { + info = { "installed": True, "version": _extract_version(latest.name), "xpi_url": f"/extension/{latest.name}", "latest_url": "/extension/fabledcurator-latest.xpi", "sha256": _sha256(latest), } + # The channel goes BESIDE the version, never inside it. A `-dev` suffix is + # what silently disabled the dev channel in the sibling project this design + # comes from: the comparator returned nothing for a non-integer segment, so + # every dev version compared equal and "no update available" became + # indistinguishable from "I cannot read this version". + # + # Omitted rather than defaulted when unset. Absence already has a meaning + # every reader must handle — an image built before this field existed says + # exactly the same thing by not having the key — so a blank channel reuses + # that path instead of inventing a second "unknown" spelling. + # + # Reported verbatim, not validated against {"dev", "main"}: if an image + # declares something else, showing what it actually claims is more useful + # to whoever is debugging it than dropping the value on the floor. + if FC_CHANNEL: + info["channel"] = FC_CHANNEL + return info @extension_bp.route("/manifest", methods=["GET"]) diff --git a/ci-requirements.md b/ci-requirements.md index 4d0f3c4..0be64a8 100644 --- a/ci-requirements.md +++ b/ci-requirements.md @@ -73,6 +73,15 @@ per `docs/process.md`'s "add deps to the image when used by >1 project". `extension-version`. A depth-1 clone sees one commit and derives a wrong, too-low value **rather than failing**, so the full-history checkout is load-bearing rather than incidental. +- **`FC_CHANNEL` is a build arg, not a runtime setting.** `build.yml` passes + `dev` / `main` to the web image only (the ml and agent images have nothing to + report it to), and `/api/extension/manifest` reports it beside the version so + an install can be traced to a channel. It is declared LAST in the Dockerfile + on purpose: an ARG invalidates every layer below it, and this is the one value + that differs between the dev and main builds of identical source, so placing + it earlier would stop the two channels ever sharing a cached `pip install`. + Empty by default — a local build then reports no channel at all rather than + claiming one. - Callers MUST `set -f` before substituting the script's output. Without it the shell expands `test/**` against the working tree and silently narrows the pattern to whatever files exist at that moment — a failure that looks like diff --git a/extension/README.md b/extension/README.md index 22db07c..6dc6db1 100644 --- a/extension/README.md +++ b/extension/README.md @@ -66,6 +66,25 @@ Commit time gives both branches the same number for the same source — which is exactly what lets one AMO signature serve both channels (family rule 149, FC issue #3092). +## Channels + +`dev` and `main` each build and sign their own extension, and an install is +tied to whichever FC instance it points at — Firefox's static `update_url` +cannot apply here, since every FC install is a different host, so the extension +asks its configured backend. **The channel therefore IS the instance.** +Switching channel means repointing the FC URL in options and reinstalling from +that host; there is no separate channel setting, and adding one would +contradict each server build shipping its own extension. + +The channel is reported *beside* the version, never inside it: +`/api/extension/manifest` answers `{"version": "...", "channel": "dev"}`. It is +optional — an instance that declares none simply omits the key, and the popup, +the toolbar tooltip and the Settings card all read exactly as they did before +the field existed. Do not be tempted to make it a `-dev` version suffix: the +comparator parses each dotted segment with `parseInt`, so a suffixed segment +reads as 0 and every dev build compares equal to every other, collapsing "no +update available" and "I cannot read this version" into one answer. + ## Release Nothing to do by hand. Push to `dev`: `build.yml` signs the extension if this diff --git a/extension/background/background.js b/extension/background/background.js index 9fc8f59..edbc2a0 100644 --- a/extension/background/background.js +++ b/extension/background/background.js @@ -37,7 +37,16 @@ ensureInitialized().catch(e => console.error('init failed:', e)); // configured backend for the latest published version and nudge the operator to // reinstall the freshly-signed XPI — surfaced as a popup banner (on demand) and // a toolbar badge (daily). /api/extension/manifest is public and returns -// {version, latest_url, sha256}; the XPI is served from the web root (not /api). +// {version, latest_url, sha256} plus an OPTIONAL {channel} naming which channel +// that instance serves ("dev"/"main", #3113); the XPI is served from the web +// root (not /api). +// +// The channel IS the instance: Firefox's static update_url cannot apply here +// because every FC install is a different host, so the extension asks its +// configured backend — which means switching channel is repointing apiUrl in +// options and reinstalling from that host. There is no separate channel +// setting to build, and building one would contradict each server build +// shipping its own extension. function versionIsNewer(candidate, current) { // Dotted numeric compare so 1.0.10 > 1.0.9 (a plain string compare wouldn't). @@ -60,12 +69,22 @@ async function checkForUpdateInfo() { } const currentVersion = browser.runtime.getManifest().version; const latestVersion = info && info.version ? info.version : null; + // Which channel the configured instance serves — reported ALONGSIDE the + // version, never folded into it. A `-dev` suffix would have to survive + // versionIsNewer's parseInt above, and it wouldn't: the segment would read + // as 0 and every dev build would compare equal to every other. + // + // null is a normal answer, not a failure — an instance built before the + // field existed, or one built locally with no channel declared. Nothing + // below branches on it except the label. + const channel = info && info.channel ? info.channel : null; // latest_url is served from the web root, not the JSON API. const base = api.webRoot(); return { updateAvailable: !!latestVersion && versionIsNewer(latestVersion, currentVersion), currentVersion, latestVersion, + channel, xpiUrl: info && info.latest_url ? `${base}${info.latest_url}` : null, }; } @@ -77,7 +96,11 @@ async function refreshUpdateBadge() { await browser.action.setBadgeText({ text: r.updateAvailable ? '↑' : '' }); if (r.updateAvailable) { await browser.action.setBadgeBackgroundColor({ color: '#F4BA7A' }); - await browser.action.setTitle({ title: `FabledCurator — update available (v${r.latestVersion})` }); + // Channel first, version second, and the channel dropped entirely when + // the instance doesn't report one — so the tooltip reads exactly as it + // did before the field existed rather than saying "(unknown ...)". + const label = r.channel ? `${r.channel} v${r.latestVersion}` : `v${r.latestVersion}`; + await browser.action.setTitle({ title: `FabledCurator — update available (${label})` }); } else { await browser.action.setTitle({ title: 'FabledCurator' }); } diff --git a/extension/popup/popup.js b/extension/popup/popup.js index 4ffcd8e..771bb6a 100644 --- a/extension/popup/popup.js +++ b/extension/popup/popup.js @@ -81,8 +81,12 @@ async function checkForUpdate() { } function showUpdateBanner(r) { + // The channel names itself beside the version, never inside it (#3113). + // Absent when the instance doesn't report one, and the banner then reads + // exactly as it did before the field existed. + const channel = r.channel ? ` (${r.channel})` : ''; document.getElementById('update-text').textContent = - `Update available — v${r.latestVersion} (installed v${r.currentVersion})`; + `Update available${channel} — v${r.latestVersion} (installed v${r.currentVersion})`; // Opening the signed XPI triggers Firefox's native install prompt. document.getElementById('update-btn').addEventListener('click', () => { browser.tabs.create({ url: r.xpiUrl }); diff --git a/frontend/src/components/settings/BrowserExtensionCard.vue b/frontend/src/components/settings/BrowserExtensionCard.vue index 2dfbd38..4226eba 100644 --- a/frontend/src/components/settings/BrowserExtensionCard.vue +++ b/frontend/src/components/settings/BrowserExtensionCard.vue @@ -4,6 +4,16 @@ · Firefox · v{{ manifest.version }} + + {{ manifest.channel }} diff --git a/frontend/test/components/browserExtensionCard.spec.js b/frontend/test/components/browserExtensionCard.spec.js new file mode 100644 index 0000000..b406bb5 --- /dev/null +++ b/frontend/test/components/browserExtensionCard.spec.js @@ -0,0 +1,72 @@ +// @vitest-environment happy-dom +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' +import { nextTick } from 'vue' + +import BrowserExtensionCard from '../../src/components/settings/BrowserExtensionCard.vue' +import { freshPinia, mountComponent } from '../support/mountComponent.js' + +// useApi is a thin fetch wrapper, so the seam is fetch itself (same shape as +// showcase.spec.js) rather than a module mock. +function stubApi(manifest) { + globalThis.fetch = vi.fn(async (url) => { + const payload = String(url).includes('/api/extension/manifest') + ? manifest + : { key: 'test-key' } + return { + ok: true, status: 200, statusText: '200', + text: async () => JSON.stringify(payload), + } + }) +} + +async function mountCard(manifest) { + stubApi(manifest) + const w = mountComponent(BrowserExtensionCard, { pinia: freshPinia() }) + // onMounted fires two fetches (manifest + key) and each resolves through a + // chain of microtasks. Yielding to a macrotask drains the whole queue, which + // a fixed number of nextTicks would only do by luck. + await new Promise((resolve) => setTimeout(resolve, 0)) + await nextTick() + return w +} + +const INSTALLED = { + installed: true, + version: '1.0.3499884', + xpi_url: '/extension/fabledcurator-1.0.3499884.xpi', + latest_url: '/extension/fabledcurator-latest.xpi', + sha256: 'abc', +} + +describe('BrowserExtensionCard — channel', () => { + beforeEach(() => { vi.restoreAllMocks() }) + afterEach(() => { delete globalThis.fetch }) + + it('names the channel the instance reports', async () => { + // The point of the whole channel scheme: an operator can tell a dev + // instance from a main one without installing anything. + const w = await mountCard({ ...INSTALLED, channel: 'dev' }) + expect(w.text()).toContain('dev') + }) + + it('shows the version and the channel as SEPARATE text, never merged', async () => { + // Regression guard with teeth: the tempting shortcut is a `-dev` version + // suffix, and that is precisely what breaks the extension's comparator — + // it parses each dotted segment with parseInt, so a suffixed segment reads + // as 0 and every dev build compares equal to every other. If someone ever + // "simplifies" by folding the channel into the version, the version text + // stops being the bare derived number and this fails. + const w = await mountCard({ ...INSTALLED, channel: 'dev' }) + expect(w.text()).toContain('v1.0.3499884') + expect(w.text()).not.toContain('1.0.3499884-dev') + }) + + it('renders no channel when the instance declares none', async () => { + // A locally-built image, or one predating the field. The card must read + // exactly as it did before the channel existed rather than inventing an + // "unknown" badge — absence is a normal answer here, not a fault. + const w = await mountCard(INSTALLED) + expect(w.text()).toContain('v1.0.3499884') + expect(w.findAll('v-chip')).toHaveLength(0) + }) +}) diff --git a/tests/test_api_extension.py b/tests/test_api_extension.py index b2885de..65a8a77 100644 --- a/tests/test_api_extension.py +++ b/tests/test_api_extension.py @@ -383,6 +383,53 @@ async def test_extension_manifest_returns_metadata_when_xpi_present(client, monk assert body["sha256"] == hashlib.sha256(b"fake-xpi-content").hexdigest() +@pytest.mark.asyncio +async def test_extension_manifest_reports_the_channel_the_image_declares( + client, monkeypatch, tmp_path +): + """The channel travels BESIDE the version, never inside it. + + Folding it in as a `1.0.3499884-dev` suffix is the failure this design + exists to avoid: the extension's comparator parses each dotted segment as + an integer, so a suffixed segment collapses to 0 and every dev build + compares equal to every other — "no update available" and "I cannot read + this version" stop being distinguishable. Asserting the two are separate + keys is what keeps a future edit from merging them. + """ + (tmp_path / "fabledcurator-1.2.3.xpi").write_bytes(b"x") + monkeypatch.setattr(extension_module, "XPI_DIR", tmp_path) + monkeypatch.setattr(extension_module, "FC_CHANNEL", "dev") + resp = await client.get("/api/extension/manifest") + assert resp.status_code == 200 + body = await resp.get_json() + assert body["channel"] == "dev" + assert body["version"] == "1.2.3" + + +@pytest.mark.asyncio +async def test_extension_manifest_omits_the_channel_when_the_image_declares_none( + client, monkeypatch, tmp_path +): + """A local build, or any image from before the field existed. + + The key must be ABSENT rather than present-and-empty: absence is the state + every consumer already handles (an older image conveys it by not having the + key at all), so a blank channel reuses that path instead of introducing a + second spelling of "unknown" for each reader to special-case. + """ + (tmp_path / "fabledcurator-1.2.3.xpi").write_bytes(b"x") + monkeypatch.setattr(extension_module, "XPI_DIR", tmp_path) + monkeypatch.setattr(extension_module, "FC_CHANNEL", "") + resp = await client.get("/api/extension/manifest") + assert resp.status_code == 200 + body = await resp.get_json() + assert "channel" not in body + # Everything else still answers — an image with no channel is not a + # degraded one, it just cannot say which channel it came from. + assert body["installed"] is True + assert body["latest_url"] == "/extension/fabledcurator-latest.xpi" + + # --- /extension/ ----------------------------------------- From 0db38cc11192a112fcba2f04d52012bf99e91a94 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 27 Aug 2026 12:08:46 -0400 Subject: [PATCH 16/16] ci: log in to the registry with the docker CLI, not docker/login-action MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit build-ml failed at the login step twice on a7e626a, five seconds in, with MODULE_NOT_FOUND on the action's own dist/index.js. Not the token — the secret resolved to *** and the action never ran far enough to use it. The cause is a race in act_runner's shared action cache, not corruption. A remote action is cached at one /root/.cache/act/ per runner, and build-web, build-ml and build-agent all start in the same second and all want docker/login-action@v3. One job re-clones that directory — emptying and repopulating it — while another walks it to copy into its container, and the walker lstat()s a file that just vanished. The two failures named DIFFERENT missing files, eslint.config.mjs then jest.config.ts, which is what rules out a bad cache entry and points at the race: a dangling entry would name the same file every time. Re-running does not help, because the re-run starts the three jobs simultaneously again. It reproduced immediately. Dropping the action removes FC from that race for this step. Logging in is one command, the docker CLI is already in the CI image per ci-requirements.md, and the same reasoning as family rule 5 applies: a marketplace action buys nothing when the tool is baked into the image the workflow already selected. Password on stdin, never as an argument — an argument lands in the process table and draws docker's own deprecation warning. This narrows the exposure rather than closing it. All three jobs also share docker/build-push-action@v5 and can race on it the same way; that one has not lost yet, and replacing it means hand-rolling buildx invocation including the build-args and provenance handling, which is a bigger change than this failure justifies. Recorded on #3118. Live consequence being cleared: fabledcurator-ml:dev was left a commit behind fabledcurator:dev, which is the stale-pairing trap the trigger comment on 239b1ed warns about. --- .forgejo/workflows/build.yml | 50 +++++++++++++++++++++++++----------- 1 file changed, 35 insertions(+), 15 deletions(-) diff --git a/.forgejo/workflows/build.yml b/.forgejo/workflows/build.yml index 5551d68..43ab953 100644 --- a/.forgejo/workflows/build.yml +++ b/.forgejo/workflows/build.yml @@ -442,12 +442,30 @@ jobs: echo "channel=dev" >> "$GITHUB_OUTPUT" fi + # A shell step, not docker/login-action@v3, because the action's shared + # cache races itself (#3118). act_runner caches a remote action under one + # /root/.cache/act/ per runner, and build-web, build-ml and + # build-agent all start in the same second and all want this same action. + # One job re-clones the directory — which empties and repopulates it — + # while another is walking it to copy into its container, and the walker + # lstat()s a file that has just vanished. It failed twice on 2026-08-27, + # naming a DIFFERENT missing file each time (`eslint.config.mjs`, then + # `jest.config.ts`), which is what rules out a corrupt cache and points at + # a race. The loser dies with MODULE_NOT_FOUND on dist/index.js before the + # action runs at all, so the secret is never even reached. + # + # Nothing is lost by dropping it: logging in is one command, the docker + # CLI is already in the CI image (ci-requirements.md), and the same + # reasoning as family rule 5 applies — a marketplace action buys nothing + # when the tool is baked into the image the workflow already selected. + # + # Password on stdin, never as an argument: an argument lands in the + # process table and draws docker's own deprecation warning. - name: Login to Forgejo registry - uses: docker/login-action@v3 - with: - registry: git.fabledsword.com - username: ${{ github.actor }} - password: ${{ secrets.RELEASE_TOKEN }} + env: + TOKEN: ${{ secrets.RELEASE_TOKEN }} + ACTOR: ${{ github.actor }} + run: echo "$TOKEN" | docker login git.fabledsword.com -u "$ACTOR" --password-stdin - name: Build and push web image uses: docker/build-push-action@v5 @@ -490,12 +508,13 @@ jobs: echo "tags=git.fabledsword.com/bvandeusen/fabledcurator-ml:dev" >> "$GITHUB_OUTPUT" fi + # Shell step rather than docker/login-action — see build-web's note on + # the shared action-cache race (#3118). - name: Login to Forgejo registry - uses: docker/login-action@v3 - with: - registry: git.fabledsword.com - username: ${{ github.actor }} - password: ${{ secrets.RELEASE_TOKEN }} + env: + TOKEN: ${{ secrets.RELEASE_TOKEN }} + ACTOR: ${{ github.actor }} + run: echo "$TOKEN" | docker login git.fabledsword.com -u "$ACTOR" --password-stdin - name: Build and push ml image uses: docker/build-push-action@v5 @@ -528,12 +547,13 @@ jobs: echo "tags=git.fabledsword.com/bvandeusen/fabledcurator-agent:dev" >> "$GITHUB_OUTPUT" fi + # Shell step rather than docker/login-action — see build-web's note on + # the shared action-cache race (#3118). - name: Login to Forgejo registry - uses: docker/login-action@v3 - with: - registry: git.fabledsword.com - username: ${{ github.actor }} - password: ${{ secrets.RELEASE_TOKEN }} + env: + TOKEN: ${{ secrets.RELEASE_TOKEN }} + ACTOR: ${{ github.actor }} + run: echo "$TOKEN" | docker login git.fabledsword.com -u "$ACTOR" --password-stdin - name: Build and push agent image uses: docker/build-push-action@v5