diff --git a/.forgejo/workflows/build.yml b/.forgejo/workflows/build.yml index a63cee9..43ab953 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 @@ -25,52 +33,135 @@ 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 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 @@ -108,6 +199,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: | @@ -134,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) @@ -141,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 @@ -184,19 +303,31 @@ 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: 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 - # 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 @@ -208,12 +339,18 @@ 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: | 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. @@ -279,29 +416,56 @@ 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- # 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 + # 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 @@ -310,6 +474,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 @@ -339,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 @@ -377,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 diff --git a/.forgejo/workflows/ci.yml b/.forgejo/workflows/ci.yml index fccfe19..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,108 +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; } - - # --- 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. - 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. - # 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/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/README.md b/README.md index 225f318..915ffa5 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 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 @@ -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 check, 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. 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/api/extension.py b/backend/app/api/extension.py index 20b1b30..868bab2 100644 --- a/backend/app/api/extension.py +++ b/backend/app/api/extension.py @@ -6,6 +6,8 @@ from __future__ import annotations import asyncio import hashlib +import hmac +import os import re from pathlib import Path @@ -30,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 @@ -41,7 +49,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: @@ -124,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/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/cleanup_service.py b/backend/app/services/cleanup_service.py index cd7ef99..89afd51 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, @@ -277,6 +336,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,11 +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 @@ -300,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: @@ -323,6 +397,28 @@ 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. + # + # 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(*_artist_attachments_conditions(artist.id)) + ).rowcount or 0 + session.commit() + session.delete(artist) session.commit() @@ -333,6 +429,8 @@ 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, }, } @@ -1494,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/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/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/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/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/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/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/ci-requirements.md b/ci-requirements.md index 2d69ef4..0be64a8 100644 --- a/ci-requirements.md +++ b/ci-requirements.md @@ -54,13 +54,34 @@ 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 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. + 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. +- **`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 5220623..6dc6db1 100644 --- a/extension/README.md +++ b/extension/README.md @@ -1,13 +1,14 @@ # 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) -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,61 @@ 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). + +## 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 -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/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/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/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/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() { 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/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+)*$/) }) 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/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/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/BrowserExtensionCard.vue b/frontend/src/components/settings/BrowserExtensionCard.vue index 0424975..4226eba 100644 --- a/frontend/src/components/settings/BrowserExtensionCard.vue +++ b/frontend/src/components/settings/BrowserExtensionCard.vue @@ -4,12 +4,22 @@ · Firefox · v{{ manifest.version }} + + {{ manifest.channel }}

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/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/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/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/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/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/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/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/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/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_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_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} diff --git a/tests/test_api_extension.py b/tests/test_api_extension.py index d3010e0..65a8a77 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( @@ -367,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/ ----------------------------------------- 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_cleanup_service.py b/tests/test_cleanup_service.py index e2a89f6..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 @@ -68,6 +69,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 +95,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") @@ -294,6 +378,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 -------------------------------------------------- @@ -916,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_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_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, []) 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 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_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 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