diff --git a/.forgejo/workflows/build.yml b/.forgejo/workflows/build.yml index ce9d042..49c1bfe 100644 --- a/.forgejo/workflows/build.yml +++ b/.forgejo/workflows/build.yml @@ -10,9 +10,12 @@ on: # 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. + # BOTH images build on dev, deliberately: a `:dev` web image paired + # with a stale `:dev` agent is a worse trap than no dev channel at all, + # since the mismatch only shows up as a runtime failure. (It was three + # until #4311 — `fabledcurator-ml` was the same bytes as `fabledcurator` + # under a second name, and the stack that needed the second name is being + # collapsed onto the consolidated image.) branches: [main, dev] # # NO tag trigger (milestone 318 step 2). A `v*` tag names a commit `main` @@ -35,8 +38,8 @@ on: # workflow is not shipped bytes, so it is in no artifact's path set. Putting # it in one would re-version every artifact for a comment change. # - # ONE input, not one per artifact. Forcing all three is cheap once the - # registry cache is warm (#3114), and three booleans is an interface nobody + # ONE input, not one per artifact. Forcing both is cheap once the registry + # cache is warm (#3114), and a boolean per artifact is an interface nobody # remembers the meaning of. workflow_dispatch: inputs: @@ -787,8 +790,8 @@ jobs: # # build-web additionally exposes this as `outputs.candidate`, which is # what gates the `promote` job — a job's `if:` cannot read `env`, and - # one flag is enough because all three derive it from the same - # IS_REFRESH. ml and agent do not re-emit it; a second copy nothing + # one flag is enough because both jobs derive it from the same + # IS_REFRESH. build-agent does not re-emit it; a second copy nothing # reads is the kind of thing that later reads as load-bearing. if [ "${IS_REFRESH:-}" = "true" ]; then echo "build_ref=$IMAGE:refresh-candidate" >> "$GITHUB_OUTPUT" @@ -1523,18 +1526,19 @@ jobs: # a FAILED gate blocks would have published unverified images while reporting # success. Not running is not the same as passing. # - # All three images promote TOGETHER, or none do. They are one stack: build.yml - # already refuses to publish a :dev web image beside a stale :dev ml, because - # the mismatch only shows up as a runtime failure. A refresh that published ml - # and withheld web would be that same trap, arrived at through the gate. + # BOTH images promote TOGETHER, or neither does. They are one stack: + # build.yml already refuses to publish a :dev web image beside a stale :dev + # agent, because the mismatch only shows up as a runtime failure. A refresh + # that published the agent and withheld web would be that same trap, arrived + # at through the gate. # # The gate covers the web image only (milestone 362 step 3 scoped it there), - # so ml and agent are being held to web's verdict rather than their own. That - # is deliberate and it is the conservative direction — they ship together, so - # the weakest evidence should govern all three — but it is not the same as - # having smoked them, and it should not be read as if it were. + # so the agent is being held to web's verdict rather than its own. That is + # deliberate and it is the conservative direction — they ship together, so + # the weakest evidence should govern both — but it is not the same as having + # smoked it, and it should not be read as if it were. promote: - needs: [build-web, build-ml, build-agent, smoke-web] + needs: [build-web, build-agent, smoke-web] # Only a refresh publishes through a candidate; a push writes its channel # tag directly from the build. Reads the same reuse-step decision the build # took, via a job output — a job's `if:` cannot see the `env` context. @@ -1555,7 +1559,7 @@ jobs: TAG=latest FAILED="" - for NAME in fabledcurator fabledcurator-ml fabledcurator-agent; do + for NAME in fabledcurator fabledcurator-agent; do REPO="bvandeusen/$NAME" echo "promote: $REPO" @@ -1616,144 +1620,7 @@ jobs: echo "promote: idempotent." >&2 exit 1 fi - echo "promote: all three channel tags moved" - # NOT A BUILD. `fabledcurator-ml` is the SAME IMAGE as `fabledcurator` and - # has been since milestone 422 step 6 merged the ML layers into the one - # Dockerfile — this job built `file: Dockerfile, context: .`, byte for byte - # what build-web builds, and published it under a second name. - # - # So it was doing the whole build twice. Measured on run 7282, a cold cache: - # build-web 1m54s, build-ml 1m55s, for identical output — plus a second - # push of a few hundred MB. Operator, 2026-09-22: *"please fix the CI so it - # doesn't do this superfluous work."* - # - # It now publishes NOTHING OF ITS OWN. It re-tags the manifest build-web - # already pushed, so the second name goes on working for the operator's - # Swarm stack — which still references it — while CI stops building twice. - # - # The name is retired entirely in #4311, once that stack points its - # ml-worker service at `fabledcurator:latest` with `command: ["ml-worker"]`. - # Nothing here is load-bearing after that: the whole job goes, rather than - # this comment growing another paragraph. - build-ml: - # Was parallel with build-web. It cannot be any more — there is nothing to - # copy until build-web has decided what this run publishes. That is the - # cost of not building twice, and it is seconds: this job transfers no - # layers the runner does not already have. - needs: [build-web] - runs-on: python-ci - container: - image: git.fabledsword.com/bvandeusen/ci-python:3.14 - steps: - - uses: actions/checkout@v4 - with: - ref: ${{ env.BUILD_REF }} - # Shallow is enough now. This job derives no version from history: - # it publishes no content, so it has no artifact identity of its own - # and `artifacts.sh revision ml` decides nothing here any more. - - # See sign-extension's copy for why this guard exists. - - name: Guard — a scheduled run must have checked out main - if: env.IS_REFRESH == 'true' - run: | - set -eu - BRANCH=$(git rev-parse --abbrev-ref HEAD) - echo "schedule: HEAD is $BRANCH ($(git rev-parse --short HEAD))" - if [ "$BRANCH" != "main" ]; then - echo "schedule: expected main, got '$BRANCH'." >&2 - echo "schedule: BUILD_REF was not honoured by the runner." >&2 - echo "schedule: refusing to publish a channel tag from it." >&2 - exit 1 - 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 - env: - TOKEN: ${{ secrets.RELEASE_TOKEN }} - ACTOR: ${{ github.actor }} - run: echo "$TOKEN" | docker login git.fabledsword.com -u "$ACTOR" --password-stdin - - - name: Point fabledcurator-ml at the image fabledcurator published - env: - SRC: git.fabledsword.com/bvandeusen/fabledcurator - DST: git.fabledsword.com/bvandeusen/fabledcurator-ml - BUILT_DIGEST: ${{ needs.build-web.outputs.digest }} - PUBLISHED_DIGEST: ${{ needs.build-web.outputs.published_digest }} - run: | - set -eu - # WHERE THE BYTES COME FROM. build-web either pushed a manifest this - # run, or it hit reuse — in which case the digest its channel tag - # already names is the right source, because that is what "hit" - # MEANS: the reuse step read this commit's fc.revision off that tag. - DIGEST="${BUILT_DIGEST:-}" - [ -n "$DIGEST" ] || DIGEST="${PUBLISHED_DIGEST:-}" - if [ -z "$DIGEST" ]; then - echo "alias: FAILED — build-web neither built an image nor" >&2 - echo "alias: resolved a published one, so there is nothing for" >&2 - echo "alias: fabledcurator-ml to be pointed at." >&2 - exit 1 - fi - - # WHICH TAGS, mirroring exactly what build-web wrote for itself. - # - # A refresh writes the CANDIDATE tag and nothing else, so `promote` - # moves it to :latest after smoke-web passes. That keeps this name - # under the same gate it has today and needs no change to promote, - # which already loops over all three image names. - SHORT_SHA=$(printf '%s' "$GITHUB_SHA" | cut -c1-7) - if [ "${IS_REFRESH:-}" = "true" ]; then - TAGS="refresh-candidate" - elif [ "${GITHUB_REF##*/}" = "main" ]; then - TAGS="latest c-${SHORT_SHA}" - else - TAGS="dev" - fi - - # Pull, tag, push — deliberately NOT `imagetools create`. - # - # That wraps its source in an INDEX, and `.Image.Config.Labels` does - # not resolve through one (#3183, run 4751). Worse here than it was - # there: promote asks the registry for IMAGE manifest media types - # ONLY, so an index sitting at :refresh-candidate would fail its - # `test -n "$CT"` and break the weekly refresh for this name. - # - # MEASURED on run 7300, the first execution: the push moved ZERO - # layer bytes. Every blob came back either "Layer already exists" or - # "Mounted from bvandeusen/fabledcurator" — the registry does - # cross-repository blob mounts between two repos of one owner, so - # this copies a manifest and nothing else. 22s, against the ~2min - # the duplicate build cost (run 7282: build-ml 1m55s). - # - # The pushed manifest gets its OWN digest (b979b145… where the - # source was aca4c75…) because docker re-serialises it. Same layers, - # same config, same fc.revision; only the manifest bytes differ. - # Nothing compares the two, and promote reads whatever is at the tag - # — but it is worth saying, because the digests looking different is - # the first thing someone will notice and misread as a second build. - docker pull "$SRC@$DIGEST" - for t in $TAGS; do - echo "alias: $DST:$t -> $SRC@$DIGEST" - docker tag "$SRC@$DIGEST" "$DST:$t" - docker push "$DST:$t" - done - - # Read it back. A push that reported success but left the tag - # elsewhere is exactly the silent-and-plausible failure this - # pipeline keeps producing, and the check costs one request. - for t in $TAGS; do - NOW=$(docker buildx imagetools inspect "$DST:$t" \ - --format '{{ index .Image.Config.Labels "fc.revision" }}' 2>/dev/null || echo "") - echo "alias: $DST:$t now carries fc.revision=${NOW:-}" - if [ -z "$NOW" ]; then - echo "alias: FAILED — $DST:$t has no readable fc.revision." >&2 - echo "alias: The label resolves through a plain image manifest" >&2 - echo "alias: and not through an index, so this means the tag is" >&2 - echo "alias: index-wrapped — which breaks promote's refresh path." >&2 - exit 1 - fi - done - + echo "promote: both channel tags moved" build-agent: runs-on: python-ci container: @@ -1961,8 +1828,8 @@ jobs: # # build-web additionally exposes this as `outputs.candidate`, which is # what gates the `promote` job — a job's `if:` cannot read `env`, and - # one flag is enough because all three derive it from the same - # IS_REFRESH. ml and agent do not re-emit it; a second copy nothing + # one flag is enough because both jobs derive it from the same + # IS_REFRESH. build-agent does not re-emit it; a second copy nothing # reads is the kind of thing that later reads as load-bearing. if [ "${IS_REFRESH:-}" = "true" ]; then echo "build_ref=$IMAGE:refresh-candidate" >> "$GITHUB_OUTPUT" diff --git a/README.md b/README.md index 0e0cf55..7916abc 100644 --- a/README.md +++ b/README.md @@ -260,12 +260,11 @@ commits since the previous tag; it builds no image. ## What's in here -Five deployable pieces, built by `.forgejo/workflows/build.yml`: +Four 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` | `fabledcurator-ml` | The same image as the web service since milestone 422 — one image serves every lane. Published under this name too, for stacks that still reference it. | +| **Web / workers** | `Dockerfile` | `fabledcurator` | Quart API + the built Vue SPA in one image. `entrypoint.sh` picks the role: `web`, `worker`, `scheduler`, `ml-worker`, or `all` (every lane under supervisord, the single-container layout). The `maintenance-long` service is a second `worker` pinned to the long-running maintenance queue. | | **GPU agent** | `agent/Dockerfile` | `fabledcurator-agent` | Optional desktop-GPU worker (`agent/`). Leases jobs over **HTTP only** — never touches the database or Redis. 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. | diff --git a/docker-compose.yml b/docker-compose.yml index e7d919d..2248734 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -233,7 +233,7 @@ services: redis: { condition: service_healthy } ml-worker: - image: git.fabledsword.com/bvandeusen/fabledcurator-ml:latest + image: git.fabledsword.com/bvandeusen/fabledcurator:latest command: ["ml-worker"] # A single GPU inference pass can run tens of seconds — let it finish. stop_grace_period: 120s diff --git a/scripts/artifacts.sh b/scripts/artifacts.sh index 6a3df2f..4b6cde8 100755 --- a/scripts/artifacts.sh +++ b/scripts/artifacts.sh @@ -8,7 +8,7 @@ # never reaches the image — see DERIVER below, and #3156 for the same finding # about packaging.sh. # -# Four artifacts, four independent versions. An artifact whose shipped files +# Three artifacts, three independent versions. An artifact whose shipped files # did not change keeps its version and does not rebuild — that is the whole # point, and it is why each path set must match its Dockerfile rather than # being a plausible guess. Getting a set wrong is quiet in BOTH directions: @@ -51,21 +51,6 @@ ROOT=$(git rev-parse --show-toplevel) # rather than restated — one definition, per #2397. WEB_PATHS='Dockerfile requirements.txt requirements-ml.txt backend alembic alembic.ini entrypoint.sh frontend :(exclude)frontend/test :(exclude)frontend/test/**' -# ml — THE SAME IMAGE as web since milestone 422 step 6, built from the same -# Dockerfile with the same context and published under a second name only -# because the operator's Swarm stack still references it. -# -# So it is the same path set, by assignment rather than by a copy that would -# drift. This was two lists describing one image for about an hour, and the -# artifact-paths guard caught it immediately: the merged Dockerfile carries -# the frontend-builder stage, so the ml image copies `frontend/package.json`, -# and the old ml list did not cover it — its version would not have moved when -# the frontend changed, and a pinned build would serve stale bytes (#3202). -# -# `requirements-ml.txt` is in WEB_PATHS for the mirror-image reason: the web -# image now installs the ML requirements, so changing them changes it. -ML_PATHS="$WEB_PATHS" - # agent (agent/Dockerfile, context `agent`) — copies requirements.txt and # fc_agent only. agent/README.md, agent/docker-compose.yml and agent/ruff.toml # live in the directory but never reach the image, so they must not re-version @@ -78,7 +63,7 @@ AGENT_PATHS='agent/Dockerfile agent/requirements.txt agent/fc_agent' # # Why web and nothing else. Every artifact stamps `fc.revision`, but only web # also stamps a version (build.yml line ~488 feeds `version web` to the -# FC_VERSION build arg; ml and agent ask for `revision` alone, and the +# FC_VERSION build arg; the agent asks for `revision` alone, and the # extension takes its version from packaging.sh). For a revision-only artifact # this file needs no entry: any change to how the revision is COMPUTED changes # the derived value, which then disagrees with the label on the published image @@ -101,7 +86,7 @@ DERIVER='scripts/artifacts.sh' usage() { - echo "usage: artifacts.sh {paths|revision|version|epoch} {web|ml|agent|extension}" >&2 + echo "usage: artifacts.sh {paths|revision|version|epoch} {web|agent|extension}" >&2 exit 2 } @@ -115,9 +100,6 @@ ext_paths() { cmd_paths() { case "$1" in web) echo "$WEB_PATHS $DERIVER $(ext_paths)" ;; - # Identical to web, deliberately: it IS web's image. That includes the - # deriver and the bundled extension — the XPI is in these bytes too. - ml) echo "$ML_PATHS $DERIVER $(ext_paths)" ;; agent) echo "$AGENT_PATHS" ;; extension) ext_paths ;; *) usage ;; @@ -128,8 +110,21 @@ cmd_paths() { # Unquoted on purpose: the pathspec must word-split into separate args. # Globbing is already off script-wide. newest() { - # shellcheck disable=SC2046 - set -- "$(cd "$ROOT" && git log --format='%ct %H' HEAD -- $(cmd_paths "$1") \ + # Resolve the path set into a variable rather than inlining it as + # `git log ... -- $(cmd_paths "$1")`. An empty substitution there leaves + # `git log HEAD --` with NO pathspec, which answers with the newest commit + # in the whole repository instead of failing — see the dispatch guard at the + # foot of this file for what that cost and why the real check lives there. + # + # This `|| exit 2` cannot be that check (it exits a subshell, since every + # caller wraps `newest` in one) — it is belt and braces for a future caller + # that reaches `newest` without passing the guard. + paths=$(cmd_paths "$1") || exit 2 + + # Unquoted on purpose: the pathspec must word-split into separate args. + # Globbing is off script-wide (`set -f`). + # shellcheck disable=SC2086 + set -- "$(cd "$ROOT" && git log --format='%ct %H' HEAD -- $paths \ | sort -n | tail -1)" if [ -z "$1" ]; then echo "artifacts.sh: no commit touches this artifact's shipped files" >&2 @@ -226,6 +221,28 @@ cmd_version() { } [ $# -ge 2 ] || usage + +# Validate the ARTIFACT here, in the main shell, before anything dispatches. +# +# It cannot be done deeper down. `cmd_revision`, `cmd_version` and `cmd_epoch` +# all call `newest` inside a command substitution, and `newest` resolves the +# path set inside another one — so an `exit` from either kills only that +# subshell. The script carried on, printed nothing, and exited 0. +# +# Before this, `revision ml` was worse than nothing: the empty substitution +# left `git log HEAD --` with no pathspec, so it answered with the newest +# commit in the WHOLE REPOSITORY — a real-looking 12-char sha on stdout, exit +# 0, and the usage line on stderr where no caller reads it. The reuse check +# would have compared that against a published label, missed, and rebuilt +# every push forever without a red run anywhere. +# +# Latent while every name callers passed was valid. #4311 removed `ml` from +# the set, which made a name that used to work start taking that path. +case "$2" in + web|agent|extension) ;; + *) usage ;; +esac + case "$1" in paths) cmd_paths "$2" ;; revision) cmd_revision "$2" ;; diff --git a/scripts/release_notes.py b/scripts/release_notes.py index 70487ad..9964a04 100644 --- a/scripts/release_notes.py +++ b/scripts/release_notes.py @@ -83,7 +83,6 @@ API = "https://git.fabledsword.com/api/v1/repos/bvandeusen/FabledCurator" IMAGES = ( "git.fabledsword.com/bvandeusen/fabledcurator", - "git.fabledsword.com/bvandeusen/fabledcurator-ml", "git.fabledsword.com/bvandeusen/fabledcurator-agent", ) diff --git a/tests/test_artifact_paths.py b/tests/test_artifact_paths.py index 22b4c51..8214014 100644 --- a/tests/test_artifact_paths.py +++ b/tests/test_artifact_paths.py @@ -31,7 +31,6 @@ ROOT = Path(__file__).resolve().parent.parent # artifact -> (dockerfile, build context relative to the repo root) ARTIFACTS = { "web": ("Dockerfile", ""), - "ml": ("Dockerfile", ""), "agent": ("agent/Dockerfile", "agent"), } @@ -133,7 +132,7 @@ DERIVERS = [ # orphaned. Guarded for web too, since web bundles what the extension makes. ("extension/scripts/packaging.sh", ("extension", "web")), # artifacts.sh decides the FC_VERSION baked into the web image (#3202). - # Web only, and deliberately: ml and agent ask this script for `revision` + # Web only, and deliberately: the agent asks this script for `revision` # alone, so they are covered by the self-correcting path above, and the # extension takes its version from packaging.sh. Milestone 318 step 5 is # the worked instance — b3989d0 and 5771fd5 share revision fb2c4d5b80be @@ -197,3 +196,30 @@ def test_files_that_never_reach_an_image_do_not_version_it(artifact, path): f"image — it would re-version and rebuild {artifact} for a change it " f"does not ship." ) + + +def test_an_unknown_artifact_fails_instead_of_answering(): + """It used to answer — with the newest commit in the whole repository. + + `newest()` inlined the path set as `git log ... -- $(cmd_paths "$1")`. + `usage` exits from the command SUBSHELL, so an unknown name made the + substitution come back empty and `git log HEAD --` walked everything: + stdout got a real-looking 12-char sha, the exit code was 0, and the usage + line went to stderr where no caller reads it. A wrong answer shaped + exactly like a right one, which the reuse check would have compared + against a published label and quietly rebuilt on forever. + + Latent while every name callers passed was valid. #4311 removed `ml` from + that set, which is what made a name that used to work start taking the + silent path. + """ + out = subprocess.run( + ["sh", str(ROOT / "scripts" / "artifacts.sh"), "revision", "ml"], + capture_output=True, text=True, cwd=ROOT, + ) + assert out.returncode != 0, ( + f"an unknown artifact exited 0 and printed {out.stdout!r}" + ) + assert not out.stdout.strip(), ( + f"an unknown artifact printed {out.stdout!r} on stdout" + ) diff --git a/tests/test_release_notes.py b/tests/test_release_notes.py index 34b354c..8bc1862 100644 --- a/tests/test_release_notes.py +++ b/tests/test_release_notes.py @@ -195,13 +195,13 @@ def test_a_non_tag_ref_renders_but_refuses_to_claim_it_published(): body_of(out) -def test_the_rollback_refs_name_all_three_images(): - """Rule 145: `:c-` is the rollback unit, and the three images move +def test_the_rollback_refs_name_both_images(): + """Rule 145: `:c-` is the rollback unit, and both images move together. A release listing only the web image sends an operator into a - rollback that leaves ml and agent on the newer build — the exact mismatch - build.yml builds all three on every push to avoid.""" + rollback that leaves the agent on the newer build — the exact mismatch + build.yml builds both on every push to avoid.""" body = body_of(notes("HEAD")) - for image in ("fabledcurator", "fabledcurator-ml", "fabledcurator-agent"): + for image in ("fabledcurator", "fabledcurator-agent"): assert f"bvandeusen/{image}:c-" in body, f"{image} missing from the rollback refs"