Answer 415 for a source whose first bytes do not match its Content-Type (closes #253)

A body whose first bytes are not those of the format its Content-Type
names answered 500 on both image routes, was logged as pixa's own
error, and was fetched again on every request. Both routes now answer
415 for each error the first-bytes check returns, and the image service
logs the refused source at warning level. The failure also gets a
negative cache entry, as an upstream error status does, so the source
URL is not fetched for 5 minutes and answers 502 meanwhile. README.md
says so.

TestService_Get_ConcurrentMissesShareAFailure still expects such a
source to be fetched again and fails until its case is changed.

Model: opus-5-5

.dockerignore

@@ -13,21 +13,12 @@
 # `/myapp`, never `**/myapp`, which also matches `cmd/myapp/` and
 # deletes the package directory from the context.
 
-# .git is sent without its config. Without a VERSION build argument the
-# stage that compiles runs `git describe --tags --always` on .git, which
-# does not need .git/config; that file can hold a credential, such as a
+# Unlike the standard file, which leaves out all of .git, pixa sends
+# .git without its config. Without a VERSION build argument the stage
+# that compiles runs `git describe --tags --always` on .git, which does
+# not need .git/config; that file can hold a credential, such as a
 # password in a remote URL or the token the CI checkout step stores there.
-# Each submodule keeps a config with the same exposure in its git directory
-# under .git/modules/, nested again for a submodule's own submodules, or in
-# its own .git directory when it keeps one.
-# KNOWN GAP: a submodule whose name has a `config` segment (`config`,
-# `deploy/config`, `config/lib`) loses its whole git directory, because
-# `**/.git/modules/**/config` also matches that segment's directory
-# under .git/modules/. Go's version stamping then fails the build;
-# nothing leaks. Name such a submodule without that segment:
-# `git submodule add --name`.
-**/.git/config
-**/.git/modules/**/config
+.git/config
 
 # Agent scratch: one full checkout of the repo per in-flight agent.
 # Anchored because it occurs once where agents run at the repo root.
@@ -51,9 +42,7 @@
 **/[iI][dD]_[rR][sS][aA]
 **/[iI][dD]_[dD][sS][aA]
 **/[iI][dD]_[eE][cC][dD][sS][aA]
-**/[iI][dD]_[eE][cC][dD][sS][aA]_[sS][kK]
 **/[iI][dD]_[eE][dD]25519
-**/[iI][dD]_[eE][dD]25519_[sS][kK]
 
 # Dependencies: restored inside the image, never copied in.
 **/node_modules
@@ -71,8 +60,10 @@
 **/.vscode
 **/*.sublime-*
 
-# pixa's own entries. On the host, `make build` writes bin/pixad, and the
-# example config keeps its state directory in data/.
+# pixa's own entries. Nothing in the build reads .gitignore. On the
+# host, `make build` writes bin/pixad, and the example config keeps its
+# state directory in data/.
+.gitignore
 /bin
 /data
 

.editorconfig

@@ -13,6 +13,3 @@ indent_style = tab
 
 [*.go]
 indent_style = tab
-
-# This repository's own sections, such as one for another language it
-# uses, go below this comment, and a re-vendor keeps them.

.gitea/workflows/check.yml

@@ -1,21 +1,15 @@
 name: check
 on: [push]
-# Free the shared runner: a new push cancels only the same branch's older run.
-concurrency:
-    group: ${{ github.workflow }}-${{ github.ref }}
-    cancel-in-progress: true
 jobs:
     check:
         runs-on: ubuntu-latest
-        # Free the shared runner from a hung build.
-        timeout-minutes: 20
         steps:
             # actions/checkout v4.2.2, 2026-02-22
             - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683
-              # script/cibuild needs no token, so none is left in .git/config.
+              # The default clone is shallow and has no tags, so the
+              # version the build takes from `git describe` would be a
+              # bare commit; this fetches the whole history with its tags.
               with:
-                  persist-credentials: false
-                  # All history and tags, so git describe finds the version tag.
                   fetch-depth: 0
             - run: script/cibuild
             - run: script/docker-smoke

.gitignore

@@ -17,46 +17,15 @@ Thumbs.db
 # entry and must not be given a `**/` prefix on the way into one.
 .claude/
 
-# Node
-node_modules/
-
-# Secrets. Unanchored like every entry above, so each matches at every
-# depth. Matching is case-sensitive on Linux, so names use character
-# ranges rather than a lowercase form that misses `Server.Key`.
-
-# Environment files. `*.env` covers bare `.env` and the `prod.env`
-# convention. Only the templates `example.env` and `sample.env` are
-# re-included below. A repository that commits any other template adds
-# its own negation at the end of this file, for example `!.env.example`.
-*.[eE][nN][vV]
-.[eE][nN][vV].*
-.[eE][nN][vV][rR][cC]
-!example.env
-!sample.env
-
-# Private keys and the bundles carrying them.
-*.[pP][eE][mM]
-*.[kK][eE][yY]
-*.[pP]12
-*.[pP][fF][xX]
-[iI][dD]_[rR][sS][aA]
-[iI][dD]_[dD][sS][aA]
-[iI][dD]_[eE][cC][dD][sS][aA]
-[iI][dD]_[eE][cC][dD][sS][aA]_[sS][kK]
-[iI][dD]_[eE][dD]25519
-[iI][dD]_[eE][dD]25519_[sS][kK]
-
-# This repository's own entries, such as its build outputs, go below
-# this comment, and a re-vendor keeps them. Anchor a binary built at the
-# root: `/myapp`, never `myapp`, which also ignores `cmd/myapp/`.
-
-# Go
-*.log
-*.out
-*.test
+# Environment / secrets
+.env
+.env.*
+*.pem
+*.key
 
 # Dependencies
 vendor/
+node_modules/
 
 # Build output
 /bin/

.golangci.yml

@@ -17,7 +17,6 @@ linters:
   disable:
     # Genuinely incompatible with project patterns
     - exhaustruct      # Requires all struct fields
-    - exhaustruct_v5   # Requires all struct fields (successor to exhaustruct)
     - godot            # Requires comments to end with periods
     - wrapcheck        # Too verbose for internal packages
     - varnamelen       # Short names like db, id are idiomatic Go
@@ -25,8 +24,6 @@ linters:
     # silenced by disabling that name, not by enabling the successor.
     - wsl              # Deprecated, replaced by wsl_v5
     - gomodguard       # Deprecated, replaced by gomodguard_v2
-    # Misses findings at random in v2.14.0; back once a pinned release fixes it
-    - canonicalheader
   settings:
     lll:
       line-length: 88

.prettierignore

@@ -1,2 +1,7 @@
 node_modules/
 yarn.lock
+
+# A byte-for-byte copy of the one in sneak/prompts.
+REPO_POLICIES.md
+
+vendor/

Dockerfile

@@ -1,12 +1,7 @@
-# Every stage declares ARG CACHEBUST first: script/ passes a new value
-# on every build, which reruns every RUN step after it. The steps that
-# compile mount Go's build cache, which survives from build to build.
-
 # Lint phase. script/lint builds it alone. The linter is run directly:
 # `make lint` and script/lint are themselves a docker build.
-# golangci/golangci-lint:v2.14.0, 2026-10-15
-FROM golangci/golangci-lint@sha256:ad862ba6b3798cbe0fd9fd7408d498fd74fbd2623a92406b2fd3898faf0bf98f AS lint
-ARG CACHEBUST
+# golangci/golangci-lint:v2.12.2, 2026-10-04
+FROM golangci/golangci-lint@sha256:5cceeef04e53efe1470638d4b4b4f5ceefd574955ab3941b2d9a68a8c9ad5240 AS lint
 
 # The linter compiles every package, and govips needs the libvips
 # headers for that. REPO_POLICIES.md has the lint phase install them
@@ -19,20 +14,17 @@ WORKDIR /src
 COPY go.mod go.sum ./
 RUN go mod download
 COPY . .
-RUN --mount=type=cache,target=/root/.cache/go-build \
-    golangci-lint run --config .golangci.yml ./...
+RUN golangci-lint run --config .golangci.yml ./...
 
-# Test phase. script/test builds it alone. -race needs cgo and so a C
-# compiler, which the Debian Go image ships and the alpine one does not.
-# golang:1.25.4-trixie, 2026-10-15
-FROM golang@sha256:a02d35efc036053fdf0da8c15919276bf777a80cbfda6a35c5e9f087e652adfc AS test
-ARG CACHEBUST
+# Test phase. script/test builds it alone.
+# golang:1.25.4-alpine3.22, 2026-02-25; the runtime stage uses Alpine 3.22 too
+FROM golang:1.25.4-alpine3.22@sha256:d3f0cf7723f3429e3f9ed846243970b20a2de7bae6a5b66fc5914e228d831bbb AS test
 
 WORKDIR /src
 
-# script/bootstrap --cgo installs the build dependencies (the libvips
-# and libheif headers, and libvips' JPEG XL support, which the tests
-# need) and downloads the Go modules.
+# script/bootstrap --cgo installs the build dependencies (a C compiler,
+# the libvips and libheif headers, and libvips' JPEG XL support, which
+# the tests need) and downloads the Go modules.
 COPY script/ ./script/
 COPY go.mod go.sum ./
 RUN script/bootstrap --cgo
@@ -40,14 +32,11 @@ RUN script/bootstrap --cgo
 COPY . .
 
 # Without -v first; on a failure, again with -v for the details, and
-# the step fails even if the second run passes. -count=1 because the
-# mounted cache also holds test results, which go test would otherwise
-# replay. -parallel 4: by default a package runs as many of its tests at
-# once as the host has CPUs, and on a busy host they then wait so long
-# to be scheduled that a test that times a request can see it return a
-# second late.
-RUN --mount=type=cache,target=/root/.cache/go-build \
-    go test -count=1 -timeout 90s -race -parallel 4 -cover ./... || \
+# the step fails even if the second run passes. -parallel 4: by default
+# a package runs as many of its tests at once as the host has CPUs, and
+# on a busy host they then wait so long to be scheduled that a test
+# that times a request can see it return a second late.
+RUN go test -count=1 -timeout 90s -race -parallel 4 -cover ./... || \
     { echo "--- Rerunning with -v for details ---"; \
       go test -count=1 -timeout 90s -race -parallel 4 -v ./...; exit 1; }
 
@@ -56,44 +45,38 @@ RUN --mount=type=cache,target=/root/.cache/go-build \
 # test passed.
 # golang:1.25.4-alpine3.22, 2026-02-25; the runtime stage uses Alpine 3.22 too
 FROM golang:1.25.4-alpine3.22@sha256:d3f0cf7723f3429e3f9ed846243970b20a2de7bae6a5b66fc5914e228d831bbb AS builder
-ARG CACHEBUST
 
 COPY --from=lint /src/go.sum /dev/null
 COPY --from=test /src/go.sum /dev/null
 
 WORKDIR /src
 
-# Build dependencies, git among them, and Go modules, as in the test
-# phase
+# Build dependencies and Go modules, as in the test phase
 COPY script/ ./script/
 COPY go.mod go.sum ./
 RUN script/bootstrap --cgo
-# A tar-stream context keeps the sender's file owners, which git refuses.
-RUN git config --system --add safe.directory /src
 
 # Copy source code
 COPY . .
 
-# The VERSION build arg when one is given, otherwise
-# `git describe --tags --always` on the .git in the build context: the
-# tag on a tagged commit, tag-N-gHASH after one, the short commit when
-# no tag is reachable. With .git present, a version that is still empty,
-# dev or unknown fails the build: git is missing or could not read the
-# checkout. A context without .git, as from a source tarball, stamps an
-# empty version. CGO stays enabled for govips; -trimpath keeps build
-# paths out of the binary, and -s -w leave out the symbol table and
-# debug information.
+# VERSION is declared here, not earlier: a new value reruns only the
+# build, not script/bootstrap. Given none, the version is
+# `git describe --tags --always` of the .git in the build context (git
+# comes from script/bootstrap): the tag on a tagged commit, tag-N-gHASH
+# after one, the short commit when no tag is reachable. A context that
+# carries .git and still yields no version fails the build; one without
+# .git, as from a source tarball, stamps an empty version. CGO stays
+# enabled for govips; -trimpath keeps build paths out of the binary, and
+# -s -w leave out the symbol table and debug information.
 ARG VERSION
-RUN --mount=type=cache,target=/root/.cache/go-build \
-    VERSION="${VERSION:-$(git describe --tags --always)}"; \
-    if [ -e .git ]; then \
-        case "$VERSION" in ""|dev|unknown) \
-            echo "version is '$VERSION' although .git is present" >&2; \
-            exit 1 ;; \
-        esac; \
+RUN version="${VERSION:-$(git describe --tags --always)}"; \
+    if [ -e .git ] && { [ -z "$version" ] || [ "$version" = dev ] || \
+        [ "$version" = unknown ]; }; then \
+        echo "the build context carries .git but yields no version" >&2; \
+        exit 1; \
     fi; \
     CGO_ENABLED=1 GOTOOLCHAIN=auto go build -trimpath \
-        -ldflags="-s -w -X main.Version=${VERSION}" \
+        -ldflags "-s -w -X main.Version=${version}" \
         -o /pixad ./cmd/pixad
 
 # Runtime stage, and the last one: a plain `docker build .` builds this
@@ -102,7 +85,6 @@ RUN --mount=type=cache,target=/root/.cache/go-build \
 # the libvips and musl it was built with.
 # alpine:3.22, 2026-10-08
 FROM alpine:3.22@sha256:5291449c3df73caf6ed85e649dec1b9e818b39a5d8c871e97afc13e9cd5e8fa8
-ARG CACHEBUST
 
 # Install runtime dependencies only. vips-jxl is libvips' JPEG XL
 # support, without which pixad does not start.

README.md

@@ -71,8 +71,7 @@ The state directory (`state_dir`, `/var/lib/pixa` in the container) holds the
 database and the disk cache:
 
 - It needs a persistent volume: without one, every restart starts with an empty
-  cache. Losing the directory also ends every login session (see Encrypted
-  URLs). In the container, the startup script gives the directory to the user
+  cache. In the container, the startup script gives the directory to the user
   pixa runs as (uid 65532) and sets its mode to `750`; outside it, that user
   must be able to write the directory.
 - `cache_max_bytes` limits the source and transformed images together. The
@@ -124,12 +123,8 @@ What the [upaas](https://git.eeqj.de/sneak/upaas) app for pixa needs:
       comma-separated; default none. Every image on a listed host is served to
       anyone without a signature, so list only hosts you control, where nobody
       else can put images.
-    - `PIXA_CACHE_MAX_BYTES`: disk cache limit in bytes; `0` disables it. Set
-      it: the volume is a directory on the host's filesystem, shared with the
-      host and every other app. The default is 75% of the sum of that
-      filesystem's free space and what the cache holds, and at least 500 MiB, so
-      the cache can grow to three quarters or more of the space they all have
-      left, and to all of it when that is 500 MiB or less.
+    - `PIXA_CACHE_MAX_BYTES`: disk cache limit in bytes; `0` disables it;
+      default 75% of (free space + what the cache holds)
     - the rest are in the table under Configuration below
 - **Health check:** the image's `HEALTHCHECK` requests
   `/.well-known/healthcheck.json`. upaas reads the container's health 60 seconds
@@ -165,14 +160,6 @@ name.
 Multiple source paths may reference the same content blob; the database tracks
 references rather than using filesystem refcounting.
 
-Source and transformed image files, and the `.meta` file beside each transformed
-image, are written under a temporary name, synced to disk, and only then renamed
-into place, so a crash leaves no file under its own name with part of its data.
-When pixa starts, and once a day after, it removes each cached source or
-transformed image whose file is empty, or not the size the database records for
-it, with a warning naming the file; the next request for it fetches the source
-or converts the image again.
-
 pixa's target is 1-5k r/s, which has not been measured at that rate (see Load
 Test). Toward it, pixa keeps in memory the content types of the 10,000
 transformed images most recently cached or served, so a cache hit on one of them
@@ -197,11 +184,7 @@ path under `/v1/` answers 200, in maintenance mode too.
   session it answers 303 to `/`. Answers: 200 with the page showing the URL; 400
   with the page naming a field that is not valid; 500 when the URL cannot be
   made.
-- `POST /logout` — log out with the generator page's Logout button: end every
-  login session, in every browser (see Encrypted URLs), and remove the session
-  cookie. Needs: a login session and the generator page's form (below); without
-  a login session it only removes the cookie. Answers: 303 to `/`; 500 when the
-  logout cannot be saved in the database.
+- `GET /logout` — end the login session. Needs: nothing. Answers: 303 to `/`.
 - `GET` or `HEAD` `/v1/image/<host>/<path>/<size>.<format>`, or
   `/v1/image/<host>/<path>/<size>` with no format — an image, fetched, resized
   and converted (below). Needs: a signature, unless the host is allowlisted (see
@@ -215,12 +198,13 @@ path under `/v1/` answers 200, in maintenance mode too.
   redirects to, is `localhost`, ends in `.localhost` or `.local`, or has an
   address in a blocked network (see `blocked_networks`); 429 over
   `image_requests_per_minute` (below); 415 when the upstream answers with a
-  `Content-Type` that is not one of the source formats (below), or with an image
-  whose first bytes libvips reads as another format; 502 when the upstream
-  answered with an error status, and for 5 minutes after that for an image of
-  the same source URL that pixa can neither serve from its cache nor make from
-  the source image still in its cache; 503 when pixa is busy or in maintenance
-  mode; 500 for any other failure.
+  `Content-Type` that is not one of the source formats (below), with a body
+  whose first bytes are not those of the format its `Content-Type` names, or
+  with an image whose first bytes libvips reads as another format; 502 when the
+  upstream answered with an error status, and for 5 minutes after that, or after
+  a body whose first bytes are not those of its `Content-Type`, for the same
+  source URL; 503 when pixa is busy or in maintenance mode; 500 for any other
+  failure.
 - `GET` or `HEAD` `/v1/e/<token>/<name>` — an image through an encrypted URL
   (see Encrypted URLs). Needs: nothing but the URL. Answers: 200; 304 when
   `If-None-Match` matches the image's `ETag`; 400 for a token that does not
@@ -249,7 +233,7 @@ the request carries the same ID as `request_id`, and so do the lines it logs
 when it fetches, converts and serves an image; the fetch sends it to the
 upstream host as `X-Request-ID`.
 
-The `POST` routes accept only a form that pixa's own page served: the page puts
+Both `POST` routes accept only a form that pixa's own page served: the page puts
 a token in the form and sets a cookie to match, and a request without both is
 refused with 403, so another site cannot submit the form from a visitor's
 browser. The login and generator pages are meant to be opened over HTTPS: while
@@ -297,8 +281,12 @@ above 8192.
 The source image may be JPEG, PNG, GIF, WebP, AVIF or JPEG XL. The upstream's
 `Content-Type` must name its format, and the image's first bytes must match it.
 An upstream answering with any other `Content-Type`, SVG included, is refused
-with 415, and so is an image whose first bytes libvips reads as another format,
-such as an SVG whose first bytes pass the check for AVIF.
+with 415. So is a body whose first bytes are not those of the format its
+`Content-Type` names, such as a JPEG sent as `image/png`, text sent as an image,
+or an empty body. pixa then does not fetch that source URL again for 5 minutes,
+and answers 502 for it meanwhile, as after an upstream error status. An image
+whose first bytes libvips reads as another format, such as an SVG whose first
+bytes pass the check for AVIF, is refused with 415 too.
 
 With the format `auto`, pixa chooses the format for each request from its
 `Accept` header, in this order:
@@ -331,26 +319,9 @@ cache at once, they share one upstream fetch (or one read of the cached source)
 and one transcode: the first request does the work, and the others wait for its
 image or its error, holding no upstream connection or processing slot of their
 own. A waiting request stops waiting when its own client goes away. The work
-goes on for the others even if the first request's client goes away. The shared
-fetch sends the first request's ID upstream, and the lines logged for the fetch
-and the transcode carry that ID.
-
-A transcode whose request's `downstream_timeout` has ended (for a shared one,
-the first request's) stops waiting for a processing slot, or, once it has one,
-gives it back without beginning to encode. A request waiting for a shared
-transcode that stops this way is answered 503 with the error
-`server busy, try again later`, though its own `downstream_timeout` may not have
-ended. govips gives pixa no way to stop libvips once an encode has begun, so an
-encode keeps its slot until it finishes, which can be after
-`downstream_timeout`. The limit of 8192 on each side of the source and of the
-output bounds how long that takes. With one libvips thread, as pixad runs it,
-encoding 8192x8192 random pixels takes up to about 1 second as JPEG, 15 as PNG,
-16 as GIF, 63 as WebP, 110 as AVIF and 96 as JPEG XL, the slowest of 8 or 16
-bits per sample, with or without an alpha channel. WebP, AVIF and JPEG XL are
-that slow only with an alpha channel: 8-bit RGB takes about 44, 53 and 28
-seconds. Decoding such a source adds up to about 11 seconds. A WebP, AVIF or
-JPEG XL encode can therefore hold its slot past the default `downstream_timeout`
-of 60 seconds.
+goes on for the others even if the first request's client goes away, until that
+request's `downstream_timeout` ends. The shared fetch sends the first request's
+ID upstream, and the lines logged for the fetch and the transcode carry that ID.
 
 The login form (`POST /`) is limited to 5 attempts per minute per client
 address, counting an IPv6 client by its /64; an attempt over the limit is
@@ -387,15 +358,7 @@ nor change what it asks for.
 
 1. Open `/` in a browser over HTTPS (or over plain HTTP while `debug` is on, see
    Routes) and log in with the signing key (`signing_key`). The login session
-   lasts 30 days, or until someone logs out with the generator page's Logout
-   button. Every login is made with the one signing key, so logging out ends
-   every login session, in every browser, a copy of the session cookie included.
-   pixa keeps a random value in its database, which it makes when the database
-   has none and replaces at each logout. A login session carries the value
-   current at its login and is refused once the value has changed, also after a
-   restart (see Deployment). The check for a logout compares no time, so a login
-   after a logout works whatever the host's clock says; the 30 days are still
-   measured by the host's clock.
+   lasts 30 days, or until `/logout`.
 2. On the generator page, give the source image's URL, the width and height, the
    format, quality and fit, and how long the URL lasts, then submit the form
    (`POST /generate`). The format is JPEG XL unless another is chosen; a form
@@ -461,9 +424,7 @@ HMAC-SHA256(secret, "host:path:query:width:height:format:expiration:quality:fit"
 
 Where:
 
-- `host` — source host as the URL writes it up to the first `/`, in lower case,
-  keeping an IPv6 address's brackets and any `:` and port after it (e.g.
-  `cdn.example.com`, `cdn.example.com:8443`, `[2001:db8::1]:8443`)
+- `host` — source origin hostname (e.g. `cdn.example.com`)
 - `path` — source path (e.g. `/photos/cat.jpg`), with each `%` written as `%25`
   and each `:` as `%3A`, so `/photos/a:b.jpg` is signed as `/photos/a%3Ab.jpg`
 - `query` — source query string, empty string if none, with `%` and `:` written
@@ -518,12 +479,6 @@ entry that is neither a host name (letters, digits, hyphens, underscores and
 dots, with at most one leading dot) nor an IP address, such as one with a port
 or a `*.` wildcard, aborts startup.
 
-pixa lower-cases the host of a `/v1/image/` URL before anything else uses it, so
-`CDN.Example.com` is matched, fetched, cached and counted against
-`upstream_connections_per_host` as `cdn.example.com`. Such a URL whose host has
-a port, as in `cdn.example.com:8443`, or a `:` with no port, needs a signature
-even when the host is allowlisted. A host holding `@` is refused with 400.
-
 ### Configuration
 
 Every setting can be given as an environment variable, in a YAML config file
@@ -682,12 +637,7 @@ See `configs/config.example.yml` for all options with defaults.
 - **Metrics**: Prometheus, at `/metrics`: generic HTTP request metrics
   (duration, response size, requests in flight) and the Go runtime and process
   metrics; requests are measured and `/metrics` is served only when
-  `metrics.username` and `metrics.password` are set. A request's `handler` label
-  is the pattern of the route it matches, such as `/v1/image/*`, never its path.
-  Requests that match no route share the label `unmatched`: those the router
-  answers 404 or 405, and CORS preflights. A request's `method` label is its
-  method when that is `GET`, `HEAD`, `POST`, `PUT`, `PATCH`, `DELETE`,
-  `OPTIONS`, `CONNECT` or `TRACE`, and `other` for any other method
+  `metrics.username` and `metrics.password` are set
 - **Logging**: stdlib slog
 
 ## Entrypoints
@@ -706,9 +656,9 @@ provide:
   install-precommit)
 - `script/projectname` — output the project name ("pixa")
 - `script/test` — run the test suite: build the `test` phase of the
-  `Dockerfile`, writing no image
+  `Dockerfile`, tagged `pixa-test`
 - `script/lint` — run golangci-lint: build the `lint` phase of the `Dockerfile`,
-  writing no image; the linter never runs on the host
+  tagged `pixa-lint`; the linter never runs on the host
 - `script/fmt` — format the Go code with gofmt and the markdown with prettier
   (writes)
 - `script/fmt-check` — check the same formatting (read-only), on the host
@@ -726,20 +676,15 @@ provide:
 - `script/install-precommit` — install the git pre-commit hook that runs
   `script/precommit`
 
-Every `docker build` in these scripts passes a new value of the `CACHEBUST`
-build argument, which every stage of the `Dockerfile` declares first, so every
-`RUN` step, the lint and test phases included, runs on every build instead of
-coming from the build cache. Go's build cache is kept from build to build: the
-steps that compile mount it, so a package whose inputs are unchanged is not
-compiled again, and both `go test` runs pass `-count=1` so that no stored test
-result is replayed. The `test` phase runs on the Debian Go image, with Debian's
-libvips and libheif; the image pixa ships is built and runs on Alpine.
+Every `docker build` in these scripts passes `--no-cache`, so the lint and test
+phases run on every build instead of coming from the build cache.
 `script/check`, `script/cibuild`, `script/docker`, `script/lint`, `script/test`,
 `script/setup` and `script/install-precommit` are the standard copies from
 `sneak/prompts`, kept identical to them. `script/fmt` and `script/fmt-check` are
 the standard copies with pixa's `gofmt` step kept before prettier. prettier
-formats the markdown only, not the HTML templates, as it cannot parse a Go
-template action inside a tag.
+formats the markdown only: not the HTML templates, as it cannot parse a Go
+template action inside a tag, and not `REPO_POLICIES.md` (see
+`.prettierignore`), a copy of the one in `sneak/prompts`.
 
 ## Load Test
 

REPO_POLICIES.md

@@ -1,6 +1,6 @@
 ---
 title: Repository Policies
-last_modified: 2026-10-15
+last_modified: 2026-09-08
 ---
 
 This document covers repository structure, tooling, and workflow standards. Code
@@ -54,39 +54,34 @@ style conventions are in separate documents:
   `cibuild`. `script/bootstrap` installs all dependencies idempotently and
   assumes nothing is present: base tools come from nix, apt, brew, or apk
   (detected in that order; apt runs noninteractive). For node it uses the
-  installed node only when its major version is the pinned one; otherwise (node
-  missing, or another major, such as the node 24 the CI runner image ships) it
-  installs the PINNED node version via nvm and installs yarn and the packages
-  under it. nvm itself is installed first if missing, from a hash-verified
-  GitHub release archive (never `curl | sh`), with bash installed as an explicit
-  prerequisite since nvm requires bash. Only the major version is compared
-  because the `Dockerfile` stages start from a node image whose exact version is
-  not the pin, and nvm cannot install a prebuilt node on alpine. yarn is pinned
-  via `corepack prepare yarn@<version> --activate`. Never install "latest" or
-  "lts"; always exact versions. `script/cibuild` runs the CI build: it changes
-  to the repo root, runs `script/bootstrap`, runs `script/check`, and builds the
-  image with the version; the Gitea workflow calls it. **`script/cibuild` runs
+  installed node if present; otherwise it installs a PINNED node version via
+  nvm, first installing nvm itself if missing — from a hash-verified GitHub
+  release archive (never `curl | sh`), with bash installed as an explicit
+  prerequisite since nvm requires bash. yarn is then pinned via
+  `corepack prepare yarn@<version> --activate`. Never install "latest" or "lts";
+  always exact versions. `script/cibuild` runs the CI build: it changes to the
+  repo root, runs `script/bootstrap`, runs `script/check`, and builds the image
+  with the version; the Gitea workflow calls it. **`script/cibuild` runs
   `script/bootstrap` first**, because the workflow checks out the repo and runs
   nothing else, while `script/fmt-check` runs the formatter on the host: on a
   pristine checkout with nothing installed the run dies there, after the
-  containerised gates have passed. **The bootstrap alone is not enough**: when
-  `script/bootstrap` installs node and yarn under nvm it leaves neither on the
-  `PATH` of the shell that called it, so a bare `yarn` either exits 127 or runs
-  under another node. The host entrypoints that need yarn — `script/fmt` and
-  `script/fmt-check` — therefore use the same test as `script/bootstrap`: when
-  the node on `PATH` has the pinned major version they run the `yarn` on `PATH`,
-  and otherwise they load nvm through `$HOME/.nvm/nvm.sh` and run yarn under the
-  pinned node. A runner carrying nothing but docker and git then gets through
-  `script/check`. Four further scripts are our own extensions to the standard:
-  `script/check` runs `script/test`, `script/lint` and `script/fmt-check`;
-  `script/precommit` is what the git pre-commit hook runs, and it calls
-  `script/check`; `script/install-precommit` installs the git pre-commit hook
-  (the `make hooks` target shims to it); and `script/projectname` (literally
-  that filename) simply outputs the project's name. Scripts that need the name
-  call `script/projectname` — e.g. `script/docker` assembles its image tag from
-  it — so those scripts stay byte-identical across all repos. Repo-type-specific
-  pre-commit extras (e.g. `go mod tidy` verification in Go repos) belong in
-  `script/precommit`, not in the hook itself. Model scripts are at
+  containerised gates have passed. **The bootstrap alone is not enough**:
+  `script/bootstrap` installs node and yarn under nvm and leaves neither on the
+  `PATH` of the shell that called it, so a bare `yarn` still exits 127. The host
+  entrypoints that need yarn — `script/fmt` and `script/fmt-check` — therefore
+  source nvm for the pinned node version before invoking it, exactly as
+  `script/bootstrap`'s own install step does. A runner carrying nothing but
+  docker and git then gets through `script/check`. Four further scripts are our
+  own extensions to the standard: `script/check` runs `script/test`,
+  `script/lint` and `script/fmt-check`; `script/precommit` is what the git
+  pre-commit hook runs, and it calls `script/check`; `script/install-precommit`
+  installs the git pre-commit hook (the `make hooks` target shims to it); and
+  `script/projectname` (literally that filename) simply outputs the project's
+  name. Scripts that need the name call `script/projectname` — e.g.
+  `script/docker` assembles its image tag from it — so those scripts stay
+  byte-identical across all repos. Repo-type-specific pre-commit extras (e.g.
+  `go mod tidy` verification in Go repos) belong in `script/precommit`, not in
+  the hook itself. Model scripts are at
   `https://git.eeqj.de/sneak/prompts/raw/branch/main/script/<name>`. The README
   must document the provided scripts in an **Entrypoints** section (see the
   README requirements below).
@@ -109,24 +104,18 @@ style conventions are in separate documents:
   `lint` phase and a `test` phase, with the final stage depending on both so the
   image cannot be built unless they pass. For non-server repos the final stage
   brings up a development environment; for server repos it is the runtime image.
-  The gate phases and the build stage start from their pinned base images and
-  install what those images lack either inline, as the canonical Go `Dockerfile`
-  below does for `git`, or by running `script/bootstrap`, as the `prompts`
-  repo's own `Dockerfile` does for its yarn packages. The development
-  environment stage installs development prerequisites by running
-  `script/bootstrap` rather than duplicating its installs inline. A stage that
-  runs `script/bootstrap` COPYs `script/` and the dependency manifests
-  (`package.json` + `yarn.lock`, `go.mod` + `go.sum`, etc.) before running it.
+  Dockerfiles install development prerequisites by running `script/bootstrap`
+  rather than duplicating installs inline; COPY `script/` and the dependency
+  manifests (`package.json` + `yarn.lock`, `go.mod` + `go.sum`, etc.) before
+  running it.
 
 - **Linting and testing run in Docker, as phases of the `Dockerfile`.** There is
   no separate lint file. `script/lint` and `script/test` each build one phase
   and nothing else:
 
     ```sh
-    docker build --build-arg CACHEBUST="$(date +%s)-$$" \
-        --target lint --output type=cacheonly .
-    docker build --build-arg CACHEBUST="$(date +%s)-$$" \
-        --target test --output type=cacheonly .
+    docker build --no-cache --target lint -t "$(script/projectname)-lint" .
+    docker build --no-cache --target test -t "$(script/projectname)-test" .
     ```
 
     **A stage that is not the last one in the file is built only when the final
@@ -136,15 +125,10 @@ style conventions are in separate documents:
     plain `docker build .` builds the last stage alone and exits 0 having linted
     and tested nothing.
 
-    **The gate builds write no image.** With `--output type=cacheonly` the phase
-    runs and a failing step fails the build, but the result is not exported.
-    Nothing uses those images, and writing one out is slow: a Go test phase's
-    image holds the toolchain. A build given neither `--output` nor `-t` writes
-    an untagged image and leaves it dangling, on every developer host and every
-    CI runner. `script/cibuild` and `script/docker` build the image that ships
-    and tag it, so each build replaces the previous image; each assigns the tag
-    on its own line before the build, so `set -e` stops it where
-    `script/projectname` fails.
+    **Every `docker build` in `script/` is tagged**, here and in
+    `script/cibuild` and `script/docker`. An untagged build leaves a dangling
+    image behind on every invocation, on every developer host and every CI
+    runner; a tagged one replaces the previous image.
 
     Inside a phase the tool is invoked directly — `golangci-lint`, `go test`,
     `eslint`, `prettier` — never through `make lint` or `script/test`, which are
@@ -159,107 +143,66 @@ style conventions are in separate documents:
     one's findings, and a host-global lock in `$TMPDIR` makes concurrent runs
     exit non-zero with `parallel golangci-lint is running` — a status a caller
     cannot tell from real findings. Both have produced wrong verdicts in this
-    org, in both directions. A container has its own golangci-lint cache, its
-    own `TMPDIR` and a digest-pinned binary, so neither is reachable.
+    org, in both directions. A container has its own cache, its own `TMPDIR` and
+    a digest-pinned binary, so neither is reachable.
 
-- **Any build that runs checks is given a new `CACHEBUST` value.** Docker
-  invalidates a `COPY` layer only when the copied content changes, so on an
-  unchanged tree the check `RUN` is served from cache, nothing executes, and the
-  build still exits 0. Every `docker build` in `script/` therefore passes
-  `--build-arg CACHEBUST="$(date +%s)-$$"`, the time and the script's process
-  id, so no two builds share a value, not even the three in one
-  `script/cibuild`. `script/lint`, `script/test`, `script/cibuild` and
-  `script/docker` are the four, and there is no fifth — `script/check` runs the
-  two gate phases and `script/fmt-check`, and builds no image of its own. Every
-  stage of the `Dockerfile` declares `ARG CACHEBUST` as its first line after
-  `FROM`, and a new value reruns every `RUN` step after that line; `COPY` steps
-  are compared by content and need nothing. A stage without the `ARG` line is
-  served from cache on an unchanged tree, so a repository re-vendors the scripts
-  and adds the `ARG` lines in one commit. The scripts do not pass `--no-cache`:
-  it reruns every step too, but it also empties every cache mount, and Go's
-  build cache, which the canonical Go `Dockerfile` below mounts so that compiled
-  packages survive between check builds, would start empty every time. A bare
-  `docker build .` is not evidence that anything ran: a sub-second build
-  reporting success is a cache hit, not a result. Never invalidate by pruning —
-  `docker builder prune` and friends destroy a build cache shared with every
-  other build on the host. When a check is added or changed, prove it works by
-  planting a defect it must catch and watching the run fail on it, then revert
-  the defect. A green run alone shows neither that the check ran nor that it
-  covers what it should.
+- **Any build that runs checks is built with `--no-cache`.** Docker invalidates
+  a `COPY` layer only when the copied content changes, so on an unchanged tree
+  the check `RUN` is served from cache, nothing executes, and the build still
+  exits 0. Every `docker build` in `script/` therefore passes `--no-cache`:
+  `script/lint`, `script/test`, `script/cibuild` and `script/docker` are the
+  four, and there is no fifth — `script/check` runs the two gate phases and
+  `script/fmt-check`, and builds no image of its own. A bare `docker build .` is
+  not evidence that anything ran: a sub-second build reporting success is a
+  cache hit, not a result. Never invalidate by pruning — `docker builder prune`
+  and friends destroy a build cache shared with every other build on the host.
 
 - **The gate phases are separate stages, and the build stage depends on both.**
   The lint phase is based on the `golangci/golangci-lint` image (pinned by
   hash), so lint failures surface in seconds rather than after a full compile,
-  and the test phase is based on the Debian Go image. The canonical Go repo
+  and the test phase is based on the Go image. The canonical Go repo
   `Dockerfile`:
 
     ```dockerfile
-    # Every stage declares ARG CACHEBUST first: script/ passes a new value
-    # on every build, which reruns every RUN step after it. The steps that
-    # compile mount Go's build cache, which survives from build to build.
-
     # Lint phase
     # golangci/golangci-lint:v2.x.x, YYYY-MM-DD
     FROM golangci/golangci-lint@sha256:... AS lint
-    ARG CACHEBUST
     WORKDIR /src
     COPY go.mod go.sum ./
     RUN go mod download
     COPY . .
-    RUN --mount=type=cache,target=/root/.cache/go-build \
-        golangci-lint run --config .golangci.yml ./...
+    RUN golangci-lint run --config .golangci.yml ./...
 
-    # Test phase. -race needs cgo and so a C compiler, which the Debian Go
-    # image ships and the alpine one does not. -count=1 because the mounted
-    # cache also holds test results, which go test would otherwise replay.
-    # golang:1.x, YYYY-MM-DD
+    # Test phase
+    # golang:1.x-alpine, YYYY-MM-DD
     FROM golang@sha256:... AS test
-    ARG CACHEBUST
     WORKDIR /src
     COPY go.mod go.sum ./
     RUN go mod download
     COPY . .
-    RUN --mount=type=cache,target=/root/.cache/go-build \
-        go test -count=1 -timeout 90s -race -cover ./... || \
+    RUN go test -timeout 90s -race -cover ./... || \
         { echo "--- Rerunning with -v for details ---"; \
-          go test -count=1 -timeout 90s -race -v ./...; exit 1; }
+          go test -timeout 90s -race -v ./...; exit 1; }
 
     # Build stage. Nothing is wanted from either phase above; the copies
     # are what make BuildKit build them first, so this stage cannot run
     # unless lint and test passed.
     # golang:1.x-alpine, YYYY-MM-DD
     FROM golang@sha256:... AS builder
-    ARG CACHEBUST
     COPY --from=lint /src/go.sum /dev/null
     COPY --from=test /src/go.sum /dev/null
-    RUN apk add --no-cache git
-    # A tar-stream context keeps the sender's file owners, which git refuses.
-    RUN git config --system --add safe.directory /src
     WORKDIR /src
     COPY go.mod go.sum ./
     RUN go mod download
     COPY . .
 
-    # The VERSION build arg when one is given, otherwise
-    # `git describe --tags --always` on the .git in the build context. With
-    # .git present, a version that is still empty, dev or unknown fails the
-    # build: git is missing or could not read the checkout.
-    ARG VERSION
-    RUN --mount=type=cache,target=/root/.cache/go-build \
-        VERSION="${VERSION:-$(git describe --tags --always)}"; \
-        if [ -e .git ]; then \
-            case "$VERSION" in ""|dev|unknown) \
-                echo "version is '$VERSION' although .git is present" >&2; \
-                exit 1 ;; \
-            esac; \
-        fi; \
-        CGO_ENABLED=0 go build -trimpath \
-            -ldflags="-s -w -X main.Version=${VERSION}" \
-            -o /app ./cmd/app/
+    ARG VERSION=dev
+    RUN CGO_ENABLED=0 go build -trimpath \
+        -ldflags="-s -w -X main.Version=${VERSION}" \
+        -o /app ./cmd/app/
 
     # Runtime stage, and the last one
     FROM alpine@sha256:...
-    ARG CACHEBUST
     COPY --from=builder /app /usr/local/bin/app
     ENTRYPOINT ["app"]
     ```
@@ -274,88 +217,23 @@ style conventions are in separate documents:
     - Keep the runtime stage last, and if you add a stage after it, give it the
       same two copies. A plain `docker build .` builds the last stage's chain
       and nothing else.
-    - Only Go's build cache is mounted (`/root/.cache/go-build`, where Go keeps
-      it for root), and only on the steps that compile: `golangci-lint run`,
-      both `go test` runs and `go build`. It keeps compiled packages between
-      check builds, and Go reuses one only when all its inputs are unchanged, so
-      it saves the compile without skipping a check. Nothing else is kept:
-      golangci-lint's own cache would replay stored findings in place of a lint
-      run, and Go builds from whatever files a kept module cache holds without
-      checking them again (only `go mod verify` does), so a file changed there
-      would go unnoticed. Go's build cache also holds test results, which is why
-      both `go test` runs carry `-count=1`.
     - If the project uses `//go:embed` directives that reference build artifacts
       (e.g. a web frontend compiled in a separate stage), the lint phase must
       create placeholder files so the embed directives resolve. Example:
       `RUN mkdir -p web/dist && touch web/dist/index.html web/dist/style.css`.
-    - If the project requires CGO or system libraries for linting, install them
-      in the lint phase. The `golangci/golangci-lint` image is Debian-based and
-      has no `apk`, so install with `apt-get` under the Debian package name
-      (`libvips-dev`, where alpine says `vips-dev`), and delete the package
-      lists in the same `RUN`, so the layer does not keep them:
-
-        ```dockerfile
-        RUN apt-get update \
-            && apt-get install -y --no-install-recommends libvips-dev \
-            && rm -rf /var/lib/apt/lists/*
-        ```
-
-    - `.dockerignore` lets `.git` into the build context. It keeps out every git
-      `config` at any depth (`**/.git/config`, `**/.git/modules/**/config`): the
-      repository's own, each submodule's under `.git/modules/`, and that of a
-      submodule keeping its own `.git` directory. `git describe` does not need
-      them, and each can hold a credential: a password in a remote URL, or the
-      token the CI checkout step stores there. A submodule whose name has a
-      `config` segment (`config`, `deploy/config`, `config/lib`) loses its whole
-      git directory to `**/.git/modules/**/config`, and Go's version stamping
-      then fails the build: give it a name without that segment
-      (`git submodule add --name`). The stage that compiles has `git` (the
-      Debian Go image has it; an alpine one needs `apk add --no-cache git`) and
-      takes the version from the `VERSION` build argument when one is given,
-      otherwise from `git describe --tags --always`. That gives the tag on a
-      tagged commit; on a later commit, the tag, the number of commits since it
-      and the short commit (`v1.2.3-4-gabc1234`); and the short commit when no
-      tag is reachable. The stage that compiles also marks its working directory
-      safe for git (`git config --system --add safe.directory /src`): a context
-      sent as a tar stream keeps the sender's file owners, and git refuses a
-      checkout owned by another user, so the version would come out empty.
-      `ARG VERSION` has no default, and the build fails if the context carries
-      `.git` and the version still comes out empty, `dev` or `unknown`. A plain
-      `docker build .` with no build arguments must succeed; a Dockerfile that
-      refuses an empty build argument drops that refusal and keeps the argument.
-      A checkout whose `.git` is a file (a linked worktree, or a repository
-      checked out as a submodule) is the exception: that file points to a git
-      directory outside the build context, so the build cannot read the version
-      and a plain `docker build .` fails; pass the version with
-      `--build-arg VERSION=...`, as `script/docker` and `script/cibuild` already
-      do.
+    - If the project requires CGO or system libraries for linting (e.g.
+      `vips-dev`), install them in the lint phase with `apk add`.
+    - `ARG VERSION=dev` is declared in the stage that compiles and supplied by
+      `script/docker` and `script/cibuild`; no stage may call `git describe`.
 
 - Every repo should have a Gitea Actions workflow (`.gitea/workflows/`) that
-  runs `script/cibuild` on push, and checks out the repo as its only other step,
-  with `persist-credentials: false`: `script/cibuild` needs no token, and
-  without it the checkout leaves the job's token in `.git/config` for every
-  later step. The checkout step also sets `fetch-depth: 0`, which fetches the
-  tags `git describe` needs: by default it clones shallow with no tags, and a
-  tagged repository's CI build would stamp a bare short commit id. The
-  workflow's `concurrency` block groups runs by workflow and branch
-  (`${{ github.workflow }}-${{ github.ref }}`) with `cancel-in-progress: true`,
-  so a new push cancels the older run on the same branch, queued or running, and
-  no other: runs for replaced commits do not hold up the shared runner.
-  `script/cibuild` bootstraps, runs the gate phases, and then builds the image,
-  so a successful run means every check passed; a bare `docker build .` does not
+  runs `script/cibuild` on push, and checks out the repo as its only other step.
+  That script bootstraps, runs the gate phases, and then builds the image, so a
+  successful run means every check passed; a bare `docker build .` does not
   carry the same guarantee, because its gate phases may come from the cache. The
-  image build gets a `CACHEBUST` value of its own and so runs the gate phases a
-  second time. That is the price of the rule above, and it is worth paying: the
-  image that ships is built from a run of its own gates rather than from a cache
-  entry. The `check` job sets `timeout-minutes: 20`, so a hung build frees the
-  shared runner after 20 minutes. That allows for the three Docker builds
-  described above (the test phase, the lint phase, then the image), each held to
-  the 5-minute Docker build limit below, plus the bootstrap. A separate workflow
-  limited to `main` by a `branches` list under `on: push` cannot be checked by
-  review: to try a change to it, add the feature branch to that list and push,
-  then remove the branch from the list again before merging. Keep any job in it
-  that publishes behind `if: github.ref_name == 'main'`, so the run from the
-  feature branch publishes nothing.
+  image build is uncached and so runs the gate phases a second time. That is the
+  price of the rule above, and it is worth paying: the image that ships is built
+  from a run of its own gates rather than from a cache entry.
 
 - Use platform-standard formatters: `black` for Python, `prettier` for
   JS/CSS/Markdown/HTML, `go fmt` for Go. Always use default configuration with
@@ -408,18 +286,17 @@ style conventions are in separate documents:
     ```
 
     `-count=1` is required on both invocations: it defeats Go's test _result_
-    cache, so neither run can report a stored pass in place of running the
-    tests. It leaves the build cache alone, so it costs the runtime of the suite
-    and no recompilation.
+    cache, so the target cannot report a pass it did not earn, and the rerun
+    reproduces a failure instead of replaying it. It leaves the build cache
+    alone, so it costs the runtime of the suite and no recompilation.
 
-    That cache is Go's own, separate from Docker's layer cache. Go stores a
-    passing result in its cache directory (`GOCACHE`), and when the same tests
-    run again on unchanged code it prints that result, marked `(cached)`,
-    without running them. The directory lasts from one run to the next on a
-    developer's machine, where this target runs, and in the `test` phase of the
-    `Dockerfile`, which mounts it to keep compiled packages between check
-    builds, so the phase's `go test` runs carry `-count=1` too. A new
-    `CACHEBUST` value (above) is what makes that step run on an unchanged tree.
+    Note that this is a second, independent cache, stacked below the Docker
+    layer cache that [issue #26](https://git.eeqj.de/sneak/prompts/issues/26)
+    addresses. `CHECK_EPOCH` guarantees the `RUN make test` _step_ re-executes;
+    it does not guarantee `go test` inside that step does any work, because the
+    `GOCACHE` baked into earlier image layers survives into the re-executed
+    step. They are two separate defects requiring two separate fixes, and a fix
+    for one must not be recorded as covering the other.
 
     Python example:
 
@@ -447,12 +324,9 @@ style conventions are in separate documents:
 
 - `.gitignore` should be comprehensive from the start: OS files (`.DS_Store`),
   editor files (`.swp`, `*~`), in-repo agent scratch directories (`.claude/`),
-  `node_modules/`, and the repo's own build outputs. Fetch the standard
-  `.gitignore` from
-  `https://git.eeqj.de/sneak/prompts/raw/branch/main/.gitignore` when setting up
-  a new repo. A repo's `.gitignore` is the standard file followed by the repo's
-  own entries, such as its binaries; a re-vendor replaces the standard part and
-  keeps those entries. These patterns are written to `.gitignore`'s own
+  language build artifacts, and `node_modules/`. Fetch the standard `.gitignore`
+  from `https://git.eeqj.de/sneak/prompts/raw/branch/main/.gitignore` when
+  setting up a new repo. These patterns are written to `.gitignore`'s own
   semantics, in which an unanchored pattern already matches at every depth; they
   are not a `.dockerignore` and must not be transplanted into one unmodified.
 
@@ -466,7 +340,7 @@ style conventions are in separate documents:
   — which is more dangerous than a short file with no secret patterns at all,
   because it reads as solved and stops anyone looking. Give every
   depth-independent pattern the `**/` prefix and leave only genuinely
-  root-anchored entries unprefixed: `.claude`, and the repo's own host-built
+  root-anchored entries unprefixed: `.git`, and the repo's own host-built
   binary, written `/myapp` and never `**/myapp`, which would also match
   `cmd/myapp/` and delete the package directory from the context. Matching is
   case-sensitive, and an ALL-CAPS twin per pattern still misses `Server.Key`, so
@@ -491,24 +365,21 @@ style conventions are in separate documents:
   directory, so a repo running agents in subdirectories still ships
   `services/api/.claude/` and must add its own anchored entry there.
 
-- **A plain `docker build .` of a clone stamps the version that
-  `git describe --tags --always` gives**, derived from the `.git` in the build
-  context as the canonical `Dockerfile` above shows. Without its failure check,
-  a missing `git` or an unreadable checkout would leave `-X main.Version=` empty
-  and the build would still exit 0. `script/docker` and `script/cibuild` pass
-  the version they compute on the host; it takes precedence. They do this
-  byte-identically across repos:
+- **Excluding `.git` means `git describe` cannot run inside any build stage, and
+  it fails quietly there.** In a build stage there is no repository, so
+  `git describe` writes nothing to stdout, `-X main.Version=` comes out empty,
+  the binary reports no version at all, and the build still exits 0. Compute the
+  version on the host and thread it in as a build arg. `script/docker` and
+  `script/cibuild` do this, byte-identically across repos:
 
     ```sh
-    # The version and the tag each get their own line: a failing command
-    # substitution inside an argument does not trip `set -e`, so the inline
-    # form degrades to an empty constant.
+    # Own line: a failing command substitution inside an argument does not
+    # trip `set -e`, so the inline form degrades to an empty constant.
     version="$(git describe --tags --always --dirty 2>/dev/null || true)"
     [ -n "$version" ] || version="unknown"
-    tag="$(script/projectname)"
-    docker build --build-arg CACHEBUST="$(date +%s)-$$" \
+    docker build --no-cache \
         --build-arg VERSION="$version" \
-        -t "$tag" .
+        -t "$(script/projectname)" .
     ```
 
     `--always` makes an untagged repo yield an abbreviated commit hash rather
@@ -516,12 +387,12 @@ style conventions are in separate documents:
     fallback is applied — a live check that fires on a build from an export with
     no `.git` and on a repository with no commits yet. Do not fold it into the
     substitution as `|| echo unknown`, which makes the guard unreachable. The
-    Dockerfile's side is `ARG VERSION` in the stage that compiles, declared
+    Dockerfile's side is `ARG VERSION=dev` in the stage that compiles, declared
     there because `ARG` is stage-scoped; passing `VERSION` to a repo whose
     Dockerfile declares no such `ARG` is ignored and costs nothing, which is why
     the scripts stay byte-identical. One consequence for CI: the standard
-    checkout action clones shallow and fetches no tags, so the canonical
-    `.gitea/workflows/check.yml` sets `fetch-depth: 0` on its checkout step.
+    checkout action clones shallow and fetches no tags, so a repo that embeds a
+    tag-derived version must set `fetch-depth: 0` on its checkout step.
 
 - **Verify `.dockerignore` by enumerating the image, not by reading the
   patterns.** Plant files at the root _and_ at least two directories deep, build
@@ -555,18 +426,12 @@ style conventions are in separate documents:
   `test-support` depguard rule, where a repo names its own test-support packages
   by full import path. A repo adds entries there and changes nothing else, and a
   re-vendor carries its entries forward. The canonical golangci-lint version is
-  v2.14.0 (released 2026-09-24), pinned as the digest of the lint phase's base
+  v2.12.2 (released 2026-05-06), pinned as the digest of the lint phase's base
   image
-  (`golangci/golangci-lint@sha256:ad862ba6b3798cbe0fd9fd7408d498fd74fbd2623a92406b2fd3898faf0bf98f`,
-  which reports `2.14.0 built with go1.27.0 from 114493f9`). A module's `go`
-  directive must not name a newer Go minor version than the one golangci-lint
-  was built with, or golangci-lint refuses to lint it: this release lints
-  `go 1.27.1` but not `go 1.28`. That digest is the only pin, since no repo
-  installs golangci-lint on the host. A repo sets the lint phase digest to the
-  one named here and re-vendors `.golangci.yml` in the same commit, whichever of
-  the two prompted the change: the canonical copy can name linters that an older
-  golangci-lint rejects, and a newer golangci-lint can add linters that
-  `default: all` switches on until the canonical copy disables them.
+  (`golangci/golangci-lint@sha256:5cceeef04e53efe1470638d4b4b4f5ceefd574955ab3941b2d9a68a8c9ad5240`,
+  which reports `2.12.2 built with go1.26.2 from c0d3ddc9`). That digest is the
+  only pin, since no repo installs golangci-lint on the host: bumping the
+  version means changing it and nothing else.
 
 - **`script/bootstrap` installs a pinned tool by comparing versions, never by
   testing presence.** An `if ! command -v <tool>; then install; fi` guard tests
@@ -588,18 +453,8 @@ style conventions are in separate documents:
       function defined and never invoked has the same exit status and the same
       empty output as one that worked.
 
-    Node is the exception to the whole-token comparison: the canonical
-    `script/bootstrap` compares only its major version, because the node 22
-    alpine image the `Dockerfile` stages start from is not the exact pin, and
-    nvm cannot install a prebuilt node on alpine.
-
     Keep it POSIX sh: no arrays, no `[[`, no `grep -P`.
 
-    A Go tool a repo needs on the host is installed with `go install` pinned to
-    a commit hash (`go install <package>@<commit hash>`). It is never tracked as
-    a `go.mod` tool dependency or through a `tools.go` file, either of which
-    pulls the tool's own dependencies into the repo's `go.mod` and `go.sum`.
-
 - When pinning images or packages by hash, add a comment above the reference
   with the version and date (YYYY-MM-DD).
 
@@ -709,17 +564,13 @@ style conventions are in separate documents:
       Never edit existing migrations after release.
 
 - All repos should have an `.editorconfig` enforcing the project's indentation
-  settings: the standard file from
-  `https://git.eeqj.de/sneak/prompts/raw/branch/main/.editorconfig`, which sets
-  tabs for `Makefile` and Go files, followed by the repo's own sections, such as
-  one for another language it uses. A re-vendor replaces the standard part and
-  keeps those sections.
+  settings.
 
 - Avoid putting files in the repo root unless necessary. Root should contain
-  only project-level config files (`README.md`, `AGENTS.md`, `Makefile`,
-  `Dockerfile`, `LICENSE`, `.gitignore`, `.editorconfig`, `REPO_POLICIES.md`,
-  and language-specific config). Everything else goes in a subdirectory.
-  Canonical subdirectory names:
+  only project-level config files (`README.md`, `Makefile`, `Dockerfile`,
+  `LICENSE`, `.gitignore`, `.editorconfig`, `REPO_POLICIES.md`, and
+  language-specific config). Everything else goes in a subdirectory. Canonical
+  subdirectory names:
     - `bin/` — executable scripts and tools
     - `cmd/` — Go command entrypoints; thin only: one `main.go` per binary whose
       body is a single call into `internal/` or `pkg/`, no project logic in
@@ -750,7 +601,3 @@ style conventions are in separate documents:
     - Go: `go.mod`, `go.sum`, `.golangci.yml`
     - JS: `package.json`, `yarn.lock`, `.prettierrc`, `.prettierignore`
     - Python: `pyproject.toml`
-
-- Guidance for coding agents lives in one `AGENTS.md` at the repository root. It
-  is never committed under a file or directory named after one agent tool, such
-  as `CLAUDE.md` or `.claude/`, and never split into separate memory files.

TODO.md

@@ -18,7 +18,7 @@
 
 pre-1.0. No git tags exist. The `1.0.0` milestone is in progress; work lands on
 `next`, and `main` receives only the milestone PR that the owner merges. `next`
-is at the canonical `golangci-lint` v2.14.0 config and is green. Recent work
+is at the canonical `golangci-lint` v2.12.2 config and is green. Recent work
 extracted the internal/magic, internal/allowlist, internal/httpfetcher, and
 internal/signature packages. The gosec findings from the 2026-07-06 survey are
 resolved. The disk cache is now size-bounded with LRU eviction
@@ -30,140 +30,15 @@ P2: security: per-origin rate limiting
 
 # Completed Steps
 
-- 2026-10-15 the reconciliation pass runs at startup and then once a day (closes
-  #250): it ran on every eviction tick, every 5 minutes in pixad, and makes a
-  database query for every cached file, on the one connection requests also use.
-  On a large cache a pass could take longer than 5 minutes, and the next one
-  then started at once, so the passes never stopped. The next pass now starts a
-  day after the previous one ends, so passes never run back to back. Eviction
-  and the removal of expired negative cache rows keep their 5-minute tick, and
-  eviction also runs after each reconciliation pass. A variant file whose
-  database row failed to be written is now counted in the cache size up to about
-  a day later, rather than 5 minutes.
-- 2026-10-15 the canonical files are re-vendored from `sneak/prompts` commit
-  `e3f79c4` (closes #221): `.dockerignore`, `.editorconfig`,
-  `.gitea/workflows/check.yml`, `.gitignore`, `.golangci.yml`,
-  `.prettierignore`, `.prettierrc`, `REPO_POLICIES.md` and the standard scripts
-  are its copies. pixa's own entries follow the standard part: `/bin`, `/data`
-  and the local config files in `.dockerignore`; Go's `*.log`, `*.out` and
-  `*.test`, `vendor/`, the binaries, `/data/`, `*.sqlite3` and the local config
-  files in `.gitignore`. The workflow keeps its `script/docker-smoke` step while
-  https://git.eeqj.de/sneak/pixa/issues/203 is open. The scripts pass a new
-  `CACHEBUST` build argument in place of `--no-cache`, and every `Dockerfile`
-  stage declares it first, so every `RUN` step still runs on every build while
-  Go's build cache, mounted on the steps that compile, is kept; both `go test`
-  runs keep `-count=1`. The lint and test builds write no image. The lint phase
-  runs golangci-lint v2.14.0, the test phase runs on the Debian Go image, and
-  the build stage marks `/src` safe for git. `script/bootstrap`, `script/fmt`
-  and `script/fmt-check` use the node on the `PATH` only when it has the pinned
-  major version. Of golangci-lint v2.14.0's two findings, an unused `nolint`
-  directive is removed, and gosec's new path check is marked as not applying to
-  the fixed config file locations. The agent guidance moves unchanged from
-  `CLAUDE.md` to a root `AGENTS.md`, where `REPO_POLICIES.md` keeps it.
-- 2026-10-15 the request metrics record any method that is not a standard one as
-  `other` (closes #274): their `method` label was each request's method as sent,
-  and Go's HTTP server accepts any method name, so a client sending made-up
-  methods added series to the Prometheus registry that were never removed. The
-  label is now the request's method only for `GET`, `HEAD`, `POST`, `PUT`,
-  `PATCH`, `DELETE`, `OPTIONS`, `CONNECT` and `TRACE`. The metrics library reads
-  the method from the request it is handed, so the metrics middleware hands it a
-  copy of the request carrying the label; the handlers still see the method the
-  client sent.
-- 2026-10-15 logging out ends every login session (closes #256): a login session
-  was a cookie with nothing kept on the server, so `/logout` removed it from one
-  browser and a copy of it kept working for its 30 days. Every login is made
-  with the one signing key, so logging out now ends them all: the session
-  manager keeps a random value, made with `crypto/rand` when the new `logout`
-  table has none and replaced by each logout, saves it there so that it outlasts
-  a restart, puts the value at login in each session, and refuses a session
-  carrying another value. The check for a logout compares no time, so a login
-  after a logout works whatever the host's clock does; a session's 30 days are
-  still checked against the clock. Losing the state directory ends every login
-  session, and no logout can make an ended one work again. Logging out is now
-  `POST /logout` from a form on the generator page, behind the CSRF check the
-  other forms have, so a link or image elsewhere on the same site cannot end the
-  sessions. A logout without a valid login session ends none and only removes
-  the cookie, as anyone can take a CSRF token from the login page. A logout that
-  cannot be saved answers 500. `session.NewManager`, which the tests use, keeps
-  the value in memory only; pixad uses `session.NewManagerWithDatabase`.
-- 2026-10-15 a cached file left empty or partial is no longer served, or fetched
-  again on every request (closes #252). Source and transformed image files, and
-  each transformed image's `.meta` file, are synced to disk before they are
-  renamed into place, and their directory after; the `.meta` file is written
-  before the image, and a failed directory sync removes the renamed file. The
-  reconciliation pass, at startup and once a day, removes a cached source or
-  transformed image whose file is empty, or not the size the database records
-  for it, with its rows and a warning naming the file, so the next request
-  fetches the source or converts the image again; requests compare no sizes.
-  Storing a source over a file of another size replaces that file. A source that
-  fails to decode is not removed for that alone: a damaged upstream image would
-  then be fetched on every request.
-- 2026-10-15 a transcode stops once its request's `downstream_timeout` has
-  ended, unless it has begun encoding (closes #243): the image processor looked
-  at its context only while it waited for a processing slot, so an image was
-  decoded and encoded, holding the slot, however long after its request's
-  deadline that took. `Process` now returns the context's error, and gives the
-  slot back, when the context has ended by the time it has the slot or before it
-  encodes. A request that waited for a shared transcode stopped at the first
-  request's deadline is answered 503 `server busy, try again later`, not 500,
-  though its own `downstream_timeout` may not have ended. govips gives pixa no
-  way to stop libvips once an encode has begun, so that encode keeps its slot
-  until it finishes; the limit of 8192 on each side of the source and of the
-  output bounds how long that takes. With one libvips thread, 8192x8192 random
-  pixels take up to about 1 second to encode as JPEG, 15 as PNG, 16 as GIF, 63
-  as WebP, 110 as AVIF and 96 as JPEG XL, the last three with an alpha channel,
-  plus up to about 11 seconds to decode the source; `README.md` gives these
-  times and no longer says the work stops when `downstream_timeout` ends. The
-  settings of the WebP, AVIF and JPEG XL encoders, which can hold a slot past
-  the default `downstream_timeout` of 60 seconds, are unchanged.
-- 2026-10-15 "Running under upaas" in `README.md` tells the operator to set
-  `PIXA_CACHE_MAX_BYTES` (closes #251): it listed the variable as optional, but
-  under upaas the volume is a directory on the host's filesystem. The default,
-  75% of the sum of that filesystem's free space and what the cache holds, and
-  at least 500 MiB, let one pixa take three quarters or more of the space the
-  host and every other app had left, and all of it when that was 500 MiB or
-  less. The section now says so. The default is unchanged.
-- 2026-10-15 the request metrics are labelled by route, not by path (closes
-  #254): their `handler` label was each request's path, so every new path, a 404
-  or an encrypted URL, added series to the Prometheus registry that were never
-  removed, and `/metrics` listed every encrypted URL's token. The label is now
-  the pattern of the chi route the request matches, such as `/v1/image/*`, or
-  `unmatched` for a request that matches none: one the router answers 404 or
-  405, or a CORS preflight. The metrics library takes the label before the
-  request is routed, so the metrics middleware asks the router which route
-  matches.
-- 2026-10-15 expired negative cache rows are removed on a schedule (closes
-  #244): the row of a failed fetch, which makes pixa answer 502 for that source
-  URL for 5 minutes, was deleted only when the same URL was requested again
-  after that, so a client asking for a new missing path each time added rows
-  that stayed for good. The eviction goroutine now calls `CleanExpired` at
-  startup and on each periodic tick, every 5 minutes in pixad, so a row stays at
-  most about 10 minutes. `CleanExpired` deletes at most 1000 rows per statement,
-  so a request waits behind 1000 deletions at most, however many rows have
-  expired. The goroutine runs with the disk cache disabled
-  (`cache_max_bytes: 0`) too, where it does nothing else, as the negative cache
-  is used either way.
-- 2026-10-15 a cached variant is served while its source URL is in the negative
-  cache (closes #245): `Get` checked the negative cache before the variant
-  cache, so once a fetch of the source failed, as one does when another variant
-  misses after the source was evicted and the upstream is down, every variant of
-  that source answered 502 for 5 minutes, also one on disk or one that could be
-  made from the cached source. The negative cache is now checked only just
-  before a fetch from the upstream, so a cached variant is served and a variant
-  not in the cache is made from the cached source when there is one. `README.md`
-  says so under `/v1/image/`. A refusal from the negative cache now counts as a
-  miss, as any other request that misses the variant cache does.
-- 2026-10-15 the host of a `/v1/image/` URL is lower-cased, and one with a port
-  needs a signature (closes #246): with `cdn.example.com` allowlisted,
-  `CDN.Example.com` and `cdn.example.com:8443` passed the allowlist too, and
-  each spelling was fetched and cached on its own and had connections of its own
-  under `upstream_connections_per_host`. The image URL parser now lower-cases
-  the host and refuses one holding `@`, which a URL reads as a user name and
-  password, with 400. A host with a port, or with a `:` and no port, needs a
-  signature even when it is allowlisted. That is checked in the image service,
-  not in `allowlist.IsAllowed`, which also matches `referer_blocklist`, where a
-  `Referer` is blocked whatever its port. A URL is signed over its host as it
-  writes it, in lower case, with any IPv6 brackets, `:` and port kept.
+- 2026-10-15 a source whose first bytes do not match its `Content-Type` answers
+  415 (closes #253): a body whose first bytes are not those of the format its
+  `Content-Type` names, such as a JPEG sent as `image/png`, text, or a body
+  under 12 bytes, answered 500 on both image routes and was logged as an error
+  of pixa's, and each request for it fetched the whole body again. Both routes
+  now answer 415, and the image service logs it at warning level as a refused
+  source. It also gets a negative cache entry, as an upstream error status does,
+  so the source URL is not fetched again for 5 minutes and answers 502
+  meanwhile.
 - 2026-10-15 a `:` in the source path or query no longer moves where the path
   ends and the query begins (closes #247): the signed data and the cache key
   joined host, path and query with `:` and no escaping, so a URL signed for
@@ -635,11 +510,11 @@ P2: security: per-origin rate limiting
   cancellation but keeps its deadline, and the others wait for its image or
   error holding no upstream connection or processing slot, and stop waiting when
   their own context ends; the request doing the processing waits for it even
-  then, until it ends, which can be after its deadline; a request whose context
-  has already ended starts nothing; each request counts one miss, and the
-  processing counts its fetch and transcode once; a panic while processing is
-  reported to Sentry when `sentry_dsn` is set and becomes an error for every
-  waiting request instead of stopping pixad; documented in `README.md`.
+  then, up to its deadline; a request whose context has already ended starts
+  nothing; each request counts one miss, and the processing counts its fetch and
+  transcode once; a panic while processing is reported to Sentry when
+  `sentry_dsn` is set and becomes an error for every waiting request instead of
+  stopping pixad; documented in `README.md`.
 - 2026-09-29 only the image routes send CORS headers (closes #98): the CORS
   middleware, with the `access_control_allow_origin` origin, moved from the
   router root onto a `/v1` subrouter holding `/v1/image/` and `/v1/e/`, where it

internal/config/config.go

@@ -823,7 +823,6 @@ func loadConfigFile(log *slog.Logger, appName string) (*smartconfig.Config, erro
 		// one whose path runs through a file, such as under a HOME of
 		// /dev/null. One that cannot be read or does not parse is a
 		// fatal startup error.
-		//nolint:gosec // G703: a fixed config location; HOME is the operator's
 		_, statErr := os.Stat(cleanPath)
 		if errors.Is(statErr, fs.ErrNotExist) || errors.Is(statErr, syscall.ENOTDIR) {
 			continue

internal/db/migrations/001_schema.sql

@@ -176,11 +176,3 @@ CREATE TABLE IF NOT EXISTS cache_stats (
     last_updated_at DATETIME DEFAULT CURRENT_TIMESTAMP
 );
 INSERT OR IGNORE INTO cache_stats (id) VALUES (1);
-
--- A random value that a login session carries from its login. pixa makes it
--- when the table has no row, and each logout replaces it; a login session
--- carrying another value is refused, so logging out ends every login session.
-CREATE TABLE IF NOT EXISTS logout (
-    id INTEGER PRIMARY KEY CHECK (id = 1),
-    logout_value TEXT NOT NULL
-);

internal/handlers/auth.go

@@ -89,18 +89,10 @@ func (s *Handlers) handleLoginPost(w http.ResponseWriter, r *http.Request) {
 	http.Redirect(w, r, "/", http.StatusSeeOther)
 }
 
-// HandleLogout logs out, which ends every login session (see
-// session.Manager.Logout), and redirects to login.
+// HandleLogout clears the session and redirects to login.
 func (s *Handlers) HandleLogout() http.HandlerFunc {
 	return func(w http.ResponseWriter, r *http.Request) {
-		err := s.sessMgr.Logout(w, r)
-		if err != nil {
-			s.log.Error("failed to log out", "error", err)
-			http.Error(w, "Failed to log out", http.StatusInternalServerError)
-
-			return
-		}
-
+		s.sessMgr.ClearSession(w)
 		http.Redirect(w, r, "/", http.StatusSeeOther)
 	}
 }

internal/handlers/auth_session_internal_test.go

@@ -185,98 +185,6 @@ func TestHandleLogout_ClearsSessionCookie(t *testing.T) {
 	}
 }
 
-// TestHandleLogout_EndsCopiedSession verifies that after POST /logout a copy
-// of the session cookie taken before it shows the login page, not the
-// generator page.
-func TestHandleLogout_EndsCopiedSession(t *testing.T) {
-	t.Parallel()
-
-	h, srv := newCSRFTestRouter(t)
-
-	copied := newSessionCookie(t, h)
-
-	req := httptest.NewRequestWithContext(
-		t.Context(), http.MethodPost, "/logout", nil)
-	req.AddCookie(copied)
-
-	rec := httptest.NewRecorder()
-	h.HandleLogout().ServeHTTP(rec, req)
-
-	if rec.Code != http.StatusSeeOther {
-		t.Fatalf("POST /logout status = %d, want %d",
-			rec.Code, http.StatusSeeOther)
-	}
-
-	req = httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", nil)
-	req.AddCookie(copied)
-
-	rec = httptest.NewRecorder()
-	srv.ServeHTTP(rec, req)
-
-	body := rec.Body.String()
-	t.Logf("GET / with the copied cookie: status %d, login page %v",
-		rec.Code, strings.Contains(body, loginForm))
-
-	if !strings.Contains(body, loginForm) || strings.Contains(body, generatorForm) {
-		t.Error("a cookie copied before /logout still opens the generator page")
-	}
-}
-
-// TestHandleLogout_SaveFails_Answers500 verifies that when the logout cannot
-// be saved in the database, POST /logout answers 500, leaves the session
-// cookie in place and ends no session.
-func TestHandleLogout_SaveFails_Answers500(t *testing.T) {
-	t.Parallel()
-
-	h, srv := newCSRFTestRouter(t)
-
-	db := setupTestDB(t)
-
-	sessMgr, err := session.NewManagerWithDatabase(
-		t.Context(), testSigningKey, db)
-	if err != nil {
-		t.Fatalf("session.NewManagerWithDatabase() error = %v", err)
-	}
-
-	h.sessMgr = sessMgr
-	cookie := newSessionCookie(t, h)
-
-	// Saving the logout fails on a closed database.
-	err = db.Close()
-	if err != nil {
-		t.Fatalf("closing the database: %v", err)
-	}
-
-	req := httptest.NewRequestWithContext(
-		t.Context(), http.MethodPost, "/logout", nil)
-	req.AddCookie(cookie)
-
-	rec := httptest.NewRecorder()
-	h.HandleLogout().ServeHTTP(rec, req)
-
-	t.Logf("POST /logout: status %d, Set-Cookie %q",
-		rec.Code, rec.Header().Get("Set-Cookie"))
-
-	if rec.Code != http.StatusInternalServerError {
-		t.Errorf("POST /logout status = %d, want %d",
-			rec.Code, http.StatusInternalServerError)
-	}
-
-	if c := findSessionCookie(rec); c != nil {
-		t.Errorf("a logout that failed set the session cookie: %s", c)
-	}
-
-	req = httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", nil)
-	req.AddCookie(cookie)
-
-	rec = httptest.NewRecorder()
-	srv.ServeHTTP(rec, req)
-
-	if !strings.Contains(rec.Body.String(), generatorForm) {
-		t.Error("a logout that failed ended the session")
-	}
-}
-
 // TestGeneratePost_NoSession_RedirectsToLogin verifies that POST /generate
 // with a valid CSRF token but no login session answers 303 to / and makes no
 // URL.

internal/handlers/first_bytes_internal_test.go

@@ -0,0 +1,180 @@
+package handlers
+
+import (
+	"bytes"
+	"encoding/json"
+	"image/color"
+	"log/slog"
+	"net/http"
+	"slices"
+	"testing"
+	"testing/fstest"
+	"time"
+
+	"github.com/go-chi/chi/v5"
+	"sneak.berlin/go/pixa/internal/encurl"
+	"sneak.berlin/go/pixa/internal/httpfetcher"
+	"sneak.berlin/go/pixa/internal/imgcache"
+)
+
+// mismatchedSourcePath is the path of the source that
+// TestFirstBytesNotMatchingContentType serves on allowlistedHost. The mock
+// fetcher names a file's Content-Type from its extension, so the source is
+// sent as image/png.
+const mismatchedSourcePath = "/images/source.png"
+
+// newRoutesFetchingThrough returns both image routes, and the Handlers behind
+// them, whose image service fetches through fetcher and has allowlistedHost on
+// its allowlist. The handlers and the image service log to log.
+func newRoutesFetchingThrough(
+	t *testing.T, fetcher httpfetcher.Fetcher, log *slog.Logger,
+) (*Handlers, http.Handler) {
+	t.Helper()
+
+	cache, err := imgcache.NewCache(setupTestDB(t), imgcache.CacheConfig{
+		StateDir:    t.TempDir(),
+		NegativeTTL: 5 * time.Minute,
+	})
+	if err != nil {
+		t.Fatalf("imgcache.NewCache() error = %v", err)
+	}
+
+	svc, err := imgcache.NewService(&imgcache.ServiceConfig{
+		Cache:      cache,
+		Fetcher:    fetcher,
+		SigningKey: testSigningKey,
+		Allowlist:  []string{allowlistedHost},
+		Logger:     log,
+	})
+	if err != nil {
+		t.Fatalf("imgcache.NewService() error = %v", err)
+	}
+
+	encGen, err := encurl.NewGenerator(testSigningKey)
+	if err != nil {
+		t.Fatalf("encurl.NewGenerator() error = %v", err)
+	}
+
+	h := &Handlers{log: log, imgSvc: svc, encGen: encGen}
+
+	r := chi.NewRouter()
+	r.Get("/v1/image/*", h.HandleImage())
+	r.Get("/v1/e/{token}/*", h.HandleImageEnc())
+
+	return h, r
+}
+
+// mismatchedSourcePlainURL returns the /v1/image/ URL of mismatchedSourcePath
+// on allowlistedHost at 50x50 as a JPEG.
+func mismatchedSourcePlainURL(*testing.T, *Handlers) string {
+	return "/v1/image/" + allowlistedHost + mismatchedSourcePath + "/50x50.jpeg"
+}
+
+// mismatchedSourceEncURL returns an encrypted /v1/e/ URL, made with h's
+// generator, of mismatchedSourcePath on allowlistedHost at 50x50 as a JPEG.
+func mismatchedSourceEncURL(t *testing.T, h *Handlers) string {
+	t.Helper()
+
+	token, err := h.encGen.Generate(&encurl.Payload{
+		SourceHost: allowlistedHost,
+		SourcePath: mismatchedSourcePath,
+		Width:      50,
+		Height:     50,
+		Format:     imgcache.FormatJPEG,
+	})
+	if err != nil {
+		t.Fatalf("Generate() error = %v", err)
+	}
+
+	return "/v1/e/" + token + "/img.jpg"
+}
+
+// loggedLevels reads the JSON log lines in logs and returns the messages
+// logged at each level.
+func loggedLevels(t *testing.T, logs *bytes.Buffer) map[string][]string {
+	t.Helper()
+
+	t.Logf("logged:\n%s", logs.String())
+
+	messages := make(map[string][]string)
+
+	dec := json.NewDecoder(logs)
+	for dec.More() {
+		var line struct {
+			Level string `json:"level"`
+			Msg   string `json:"msg"`
+		}
+
+		err := dec.Decode(&line)
+		if err != nil {
+			t.Fatalf("decoding log line: %v", err)
+		}
+
+		messages[line.Level] = append(messages[line.Level], line.Msg)
+	}
+
+	return messages
+}
+
+// TestFirstBytesNotMatchingContentType checks that both image routes answer
+// 415 for an upstream body whose first bytes are not those of the format its
+// Content-Type names, and log it as a refused source, not as an error of
+// pixa's. The next request for that source must answer 502 without fetching
+// it again, as after an upstream error status.
+func TestFirstBytesNotMatchingContentType(t *testing.T) {
+	t.Parallel()
+
+	bodies := map[string][]byte{
+		"JPEG sent as image/png":          generateTestJPEG(t, 10, 10, color.Black),
+		"text sent as image/png":          []byte("This is just text, not a PNG"),
+		"body too short to name a format": []byte("short"),
+	}
+
+	// The source's URL on each image route.
+	sourceURLs := map[string]func(*testing.T, *Handlers) string{
+		"plain URL":     mismatchedSourcePlainURL,
+		"encrypted URL": mismatchedSourceEncURL,
+	}
+
+	for bodyName, body := range bodies {
+		for urlName, sourceURL := range sourceURLs {
+			t.Run(urlName+", "+bodyName, func(t *testing.T) {
+				t.Parallel()
+
+				fetcher := &countingFetcher{
+					Fetcher: httpfetcher.NewMock(fstest.MapFS{
+						allowlistedHost + mismatchedSourcePath: &fstest.MapFile{
+							Data: body,
+						},
+					}),
+				}
+
+				var logs bytes.Buffer
+
+				h, routes := newRoutesFetchingThrough(t, fetcher,
+					slog.New(slog.NewJSONHandler(&logs, nil)))
+				target := sourceURL(t, h)
+
+				checkErrorBody(t, sendGet(t, routes, target),
+					http.StatusUnsupportedMediaType, "unsupported source format")
+
+				levels := loggedLevels(t, &logs)
+				if len(levels[slog.LevelError.String()]) != 0 {
+					t.Errorf("logged at error level: %q",
+						levels[slog.LevelError.String()])
+				}
+
+				if !slices.Contains(levels[slog.LevelWarn.String()], "source refused") {
+					t.Error(`no "source refused" line logged at warning level`)
+				}
+
+				checkErrorBody(t, sendGet(t, routes, target),
+					http.StatusBadGateway, "upstream error")
+
+				if n := fetcher.fetches.Load(); n != 1 {
+					t.Errorf("upstream fetched %d times, want 1", n)
+				}
+			})
+		}
+	}
+}

internal/handlers/handlers.go

@@ -124,8 +124,7 @@ func (s *Handlers) initImageService() error {
 	s.imgCache = cache
 
 	// Background eviction: startup reconciliation, then periodic and
-	// write-pressure passes, and the removal of expired negative cache
-	// rows, which is all it does when the disk cache is disabled.
+	// write-pressure passes. No-op when the disk cache is disabled.
 	cache.StartEviction(imgcache.DefaultEvictionInterval)
 
 	// Writes the counts requests could not write by their deadline
@@ -164,10 +163,7 @@ func (s *Handlers) initImageService() error {
 
 	// Initialize session manager (signing key is validated at config load
 	// time). Session cookies are always Secure/HttpOnly/SameSite=Strict.
-	// The random value that each logout replaces is saved in the database,
-	// so it outlasts a restart.
-	sessMgr, err := session.NewManagerWithDatabase(
-		context.Background(), s.config.SigningKey, s.db.DB())
+	sessMgr, err := session.NewManager(s.config.SigningKey)
 	if err != nil {
 		return err
 	}

internal/handlers/image.go

@@ -15,6 +15,7 @@ import (
 	"sneak.berlin/go/pixa/internal/httpfetcher"
 	"sneak.berlin/go/pixa/internal/imageprocessor"
 	"sneak.berlin/go/pixa/internal/imgcache"
+	"sneak.berlin/go/pixa/internal/magic"
 )
 
 // HandleImage handles the main image proxy route:
@@ -220,6 +221,18 @@ func (s *Handlers) HandleTooManyRequests() http.HandlerFunc {
 func (s *Handlers) respondImageError(
 	w http.ResponseWriter, req *imgcache.ImageRequest, err error,
 ) {
+	// An upstream body whose first bytes are not those of the format its
+	// Content-Type names is the upstream's error, not pixa's: the image
+	// service has logged it as a refused source.
+	if errors.Is(err, magic.ErrMagicByteMismatch) ||
+		errors.Is(err, magic.ErrUnknownFormat) ||
+		errors.Is(err, magic.ErrNotEnoughData) {
+		s.respondError(w, "unsupported source format",
+			http.StatusUnsupportedMediaType)
+
+		return
+	}
+
 	s.log.Error("failed to get image",
 		"host", req.SourceHost,
 		"path", req.SourcePath,
@@ -240,8 +253,7 @@ func (s *Handlers) respondImageError(
 	}
 
 	if errors.Is(err, httpfetcher.ErrTooManyConnections) ||
-		errors.Is(err, imageprocessor.ErrTooManyImages) ||
-		errors.Is(err, imgcache.ErrProcessingTimedOut) {
+		errors.Is(err, imageprocessor.ErrTooManyImages) {
 		s.respondError(w, "server busy, try again later",
 			http.StatusServiceUnavailable)
 

internal/handlers/imageenc.go

@@ -15,6 +15,7 @@ import (
 	"sneak.berlin/go/pixa/internal/httpfetcher"
 	"sneak.berlin/go/pixa/internal/imageprocessor"
 	"sneak.berlin/go/pixa/internal/imgcache"
+	"sneak.berlin/go/pixa/internal/magic"
 )
 
 // HandleImageEnc handles requests to /v1/e/{token}/* for encrypted
@@ -158,12 +159,14 @@ func (s *Handlers) handleImageError(w http.ResponseWriter, err error) {
 	case errors.Is(err, httpfetcher.ErrUpstreamTimeout):
 		s.respondError(w, "upstream timeout", http.StatusGatewayTimeout)
 	case errors.Is(err, httpfetcher.ErrTooManyConnections),
-		errors.Is(err, imageprocessor.ErrTooManyImages),
-		errors.Is(err, imgcache.ErrProcessingTimedOut):
+		errors.Is(err, imageprocessor.ErrTooManyImages):
 		s.respondError(w, "server busy, try again later",
 			http.StatusServiceUnavailable)
 	case errors.Is(err, httpfetcher.ErrInvalidContentType),
-		errors.Is(err, imageprocessor.ErrUnsupportedInputFormat):
+		errors.Is(err, imageprocessor.ErrUnsupportedInputFormat),
+		errors.Is(err, magic.ErrMagicByteMismatch),
+		errors.Is(err, magic.ErrUnknownFormat),
+		errors.Is(err, magic.ErrNotEnoughData):
 		s.respondError(w, "unsupported source format",
 			http.StatusUnsupportedMediaType)
 	case errors.Is(err, imageprocessor.ErrOutputTooLarge):

internal/handlers/server_busy_internal_test.go

@@ -1,7 +1,6 @@
 package handlers
 
 import (
-	"context"
 	"fmt"
 	"log/slog"
 	"net/http"
@@ -48,40 +47,3 @@ func TestServerBusyAnswers503(t *testing.T) {
 		}
 	}
 }
-
-// TestProcessingTimedOutAnswers503 checks that respondImageError and
-// handleImageError, which answer the errors of the two image routes, answer
-// 503 with the server busy error when the image service reports that
-// processing stopped at the deadline of the request doing it, wrapped as the
-// service wraps it. A request that waited for that processing gets this error
-// while its own deadline may have time left. The request's source only goes
-// into a log line, so it is left empty.
-func TestProcessingTimedOutAnswers503(t *testing.T) {
-	t.Parallel()
-
-	h := &Handlers{log: slog.New(slog.DiscardHandler)}
-	err := fmt.Errorf("%w: failed to encode: %w",
-		imgcache.ErrProcessingTimedOut, context.DeadlineExceeded)
-
-	plain := httptest.NewRecorder()
-	h.respondImageError(plain, &imgcache.ImageRequest{}, err)
-
-	encrypted := httptest.NewRecorder()
-	h.handleImageError(encrypted, err)
-
-	for name, rec := range map[string]*httptest.ResponseRecorder{
-		"respondImageError": plain, "handleImageError": encrypted,
-	} {
-		t.Logf("%s: status = %d, body = %q", name, rec.Code, rec.Body.String())
-
-		if rec.Code != http.StatusServiceUnavailable {
-			t.Errorf("%s: status = %d, want %d",
-				name, rec.Code, http.StatusServiceUnavailable)
-		}
-
-		if !strings.Contains(rec.Body.String(), "server busy, try again later") {
-			t.Errorf("%s: body = %q, want the server busy error",
-				name, rec.Body.String())
-		}
-	}
-}

internal/imageprocessor/deadline_internal_test.go

@@ -1,106 +0,0 @@
-package imageprocessor
-
-import (
-	"bytes"
-	"context"
-	"errors"
-	"io"
-	"testing"
-	"time"
-)
-
-// readerAfterDeadline is a Process input whose first Read waits until done
-// is closed, so the deadline passes after Process has taken its slot and
-// before it encodes.
-type readerAfterDeadline struct {
-	data   *bytes.Reader
-	done   <-chan struct{}
-	waited bool
-}
-
-func (r *readerAfterDeadline) Read(p []byte) (int, error) {
-	if !r.waited {
-		r.waited = true
-
-		<-r.done
-	}
-
-	return r.data.Read(p)
-}
-
-// inputNotToBeRead is a Process input that fails the test when it is read.
-type inputNotToBeRead struct {
-	t *testing.T
-}
-
-func (r inputNotToBeRead) Read([]byte) (int, error) {
-	r.t.Error("Process read its input although its deadline had passed")
-
-	return 0, io.EOF
-}
-
-// TestProcessStopsWhenDeadlinePassed checks that Process returns
-// context.DeadlineExceeded, and holds no slot afterwards, when its context's
-// deadline has passed before it starts, then without reading its input, or
-// while it reads its input. govips gives pixa no way to stop libvips once an
-// encode has begun, so Process must not begin one for a request that has
-// ended.
-func TestProcessStopsWhenDeadlinePassed(t *testing.T) {
-	t.Parallel()
-
-	input := createTestJPEG(t, 50, 50)
-
-	t.Run("before Process starts", func(t *testing.T) {
-		t.Parallel()
-
-		ctx, cancel := context.WithDeadline(t.Context(),
-			time.Now().Add(-time.Second))
-		defer cancel()
-
-		// With a slot free, the select in acquireSlot takes the slot or sees
-		// the ended context at random, so Process is called many times
-		for range 100 {
-			checkProcessStopsAtDeadline(ctx, t, inputNotToBeRead{t: t})
-		}
-	})
-
-	t.Run("while Process reads its input", func(t *testing.T) {
-		t.Parallel()
-
-		ctx, cancel := context.WithTimeout(t.Context(), 100*time.Millisecond)
-		defer cancel()
-
-		checkProcessStopsAtDeadline(ctx, t, &readerAfterDeadline{
-			data: bytes.NewReader(input), done: ctx.Done(),
-		})
-	})
-}
-
-// checkProcessStopsAtDeadline runs Process on input with ctx, whose deadline
-// has passed by the time Process could encode, and fails the test unless
-// Process returns context.DeadlineExceeded and gives its slot back.
-func checkProcessStopsAtDeadline(
-	ctx context.Context, t *testing.T, input io.Reader,
-) {
-	t.Helper()
-
-	proc := New(Params{MaxConcurrentProcessing: 1})
-
-	result, err := proc.Process(ctx, input, smallJPEGRequest())
-	if result != nil {
-		t.Logf("Process() made a %dx%d image after its deadline passed",
-			result.Width, result.Height)
-
-		_ = result.Content.Close()
-	}
-
-	t.Logf("Process() error = %v", err)
-
-	if !errors.Is(err, context.DeadlineExceeded) {
-		t.Errorf("Process() error = %v, want context.DeadlineExceeded", err)
-	}
-
-	if held := len(proc.processingSemaphore); held != 0 {
-		t.Errorf("%d slots still held after Process() returned", held)
-	}
-}

internal/imageprocessor/imageprocessor.go

@@ -208,10 +208,7 @@ func New(params Params) *ImageProcessor {
 // Process transforms an image according to the request. When
 // MaxConcurrentProcessing images are already being processed, it waits up
 // to ProcessingWaitTimeout for one to finish, then fails with
-// ErrTooManyImages. When ctx has ended once Process has a slot, or before it
-// encodes, it returns ctx's error and gives the slot back. govips gives pixa
-// no way to stop libvips once an encode has begun, so an encode keeps its
-// slot until it finishes.
+// ErrTooManyImages.
 func (p *ImageProcessor) Process(
 	ctx context.Context,
 	input io.Reader,
@@ -293,7 +290,7 @@ func (p *ImageProcessor) Process(
 	}
 
 	// Encode to target format
-	output, err := p.encode(ctx, img, outputFormat, req.Quality)
+	output, err := p.encode(img, outputFormat, req.Quality)
 	if err != nil {
 		return nil, fmt.Errorf("failed to encode: %w", err)
 	}
@@ -420,28 +417,25 @@ func (p *ImageProcessor) WaitForProcessing(ctx context.Context) int {
 
 // acquireSlot takes a slot in processingSemaphore, waiting at most
 // processingWaitTimeout for one to free up, and returns the func that gives
-// it back. When ctx has ended by the time it has a slot, it gives the slot
-// back and returns ctx's error.
+// it back. A free slot is taken even when ctx has ended; only the wait for
+// one stops when ctx ends, as the rest of Process does not check ctx.
 func (p *ImageProcessor) acquireSlot(ctx context.Context) (func(), error) {
+	release := func() { <-p.processingSemaphore }
+
 	select {
 	case p.processingSemaphore <- struct{}{}:
+		return release, nil
+	default:
+	}
+
+	select {
+	case p.processingSemaphore <- struct{}{}:
+		return release, nil
 	case <-time.After(p.processingWaitTimeout):
 		return nil, ErrTooManyImages
 	case <-ctx.Done():
 		return nil, ctx.Err()
 	}
-
-	release := func() { <-p.processingSemaphore }
-
-	// select takes the slot even when ctx has ended at the same moment
-	err := ctx.Err()
-	if err != nil {
-		release()
-
-		return nil, err
-	}
-
-	return release, nil
 }
 
 // detectFormat returns the format string for a govips image type.
@@ -533,17 +527,10 @@ func (p *ImageProcessor) resize(
 
 const defaultQuality = 85
 
-// encode encodes an image to the specified format. govips gives pixa no way
-// to stop libvips once an encode has begun, so when ctx has ended encode
-// returns ctx's error without beginning one.
+// encode encodes an image to the specified format.
 func (p *ImageProcessor) encode(
-	ctx context.Context, img *vips.ImageRef, format Format, quality int,
+	img *vips.ImageRef, format Format, quality int,
 ) ([]byte, error) {
-	err := ctx.Err()
-	if err != nil {
-		return nil, err
-	}
-
 	if quality <= 0 {
 		quality = defaultQuality
 	}
@@ -553,7 +540,7 @@ func (p *ImageProcessor) encode(
 	// libvips' built-in profile; govips' own sRGB path variable is set on
 	// first use but read without a lock, so concurrent requests race on it.
 	if img.HasICCProfile() {
-		err = img.TransformICCProfileWithFallback("srgb", "srgb")
+		err := img.TransformICCProfileWithFallback("srgb", "srgb")
 		if err != nil {
 			return nil, fmt.Errorf("failed to convert to sRGB: %w", err)
 		}
@@ -645,11 +632,10 @@ func exportWebP(img *vips.ImageRef, quality int) ([]byte, error) {
 
 // avifEffort is the AVIF effort, from 0 (fastest) to 9; 1 is the lowest
 // govips can set. With one thread, as pixad runs libvips, 1 takes about 51
-// seconds to save an 8192x8192 RGB image of random pixels, and about 110
-// with an alpha channel, the worst case, against the default
-// downstream_timeout of 60 seconds. On an image of milder noise, which 1
-// saves in about 12 seconds, 2 takes nearly a minute and libvips' default,
-// 4, takes minutes.
+// seconds to save an 8192x8192 image of random pixels, the worst case,
+// against the default downstream_timeout of 60 seconds. On an image of
+// milder noise, which 1 saves in about 12 seconds, 2 takes nearly a minute
+// and libvips' default, 4, takes minutes.
 const avifEffort = 1
 
 // avifBitdepth is the AVIF bit depth, 8 bits per sample for every image.

internal/imgcache/cache.go

@@ -32,12 +32,6 @@ const httpStatusOK = 200
 // variant not among them is served as before, reading its .meta file.
 const metaCacheSize = 10000
 
-// cleanExpiredBatchSize is the most expired negative cache rows one
-// statement of CleanExpired deletes. Each batch is a statement of its
-// own, so a request waits behind one batch at most, however many rows
-// have expired.
-const cleanExpiredBatchSize = 1000
-
 // CacheConfig holds cache configuration.
 type CacheConfig struct {
 	StateDir    string
@@ -56,9 +50,8 @@ type CacheConfig struct {
 
 	// DisableDiskCache turns the disk cache off entirely: no cache
 	// directories are created, lookups always miss, stores are
-	// no-ops, and the eviction goroutine only removes the expired
-	// negative cache rows. The config layer sets this when the
-	// operator configures cache_max_bytes: 0.
+	// no-ops, and no eviction machinery runs. The config layer sets
+	// this when the operator configures cache_max_bytes: 0.
 	DisableDiskCache bool
 
 	// Logger receives accounting and eviction log output. A nil
@@ -489,27 +482,17 @@ func (c *Cache) GetSourceContent(
 	return c.srcContent.LoadWithSize(contentHash)
 }
 
-// CleanExpired removes the negative cache rows past their expiry, one
-// batch per statement, until none is left. Once ctx ends it starts no
-// further batch and returns ctx's error. The eviction goroutine calls it
-// at startup and on every periodic tick, so a row stays about
-// NegativeTTL plus one eviction interval, whether or not its URL is
-// requested again.
+// CleanExpired removes expired entries from the cache.
 func (c *Cache) CleanExpired(ctx context.Context) error {
-	for {
-		if ctx.Err() != nil {
-			return ctx.Err()
-		}
-
-		deleted, err := c.cleanExpiredBatch(ctx)
-		if err != nil {
-			return err
-		}
-
-		if deleted < cleanExpiredBatchSize {
-			return nil
-		}
+	// Clean expired negative cache entries
+	_, err := c.db.ExecContext(ctx, `
+		DELETE FROM negative_cache WHERE expires_at < CURRENT_TIMESTAMP
+	`)
+	if err != nil {
+		return fmt.Errorf("failed to clean negative cache: %w", err)
 	}
+
+	return nil
 }
 
 // Stats returns cache statistics.
@@ -660,10 +643,10 @@ func (c *Cache) readHitAndMissCounts(ctx context.Context, stats *CacheStats) err
 // hold: it reads the content type from the variant's .meta file and
 // keeps it in metaCache, unless a StoreVariant has put one there
 // meanwhile, as the store's is newer. A read that finds no .meta file,
-// as one can when the store's .meta write failed or an eviction removed
-// the file after the read opened the variant, serves
-// application/octet-stream and keeps nothing, so metaCache only ever
-// holds a type read from a .meta file or passed to StoreVariant.
+// as one can between a store's writing of the variant file and of its
+// .meta file, serves application/octet-stream and keeps nothing, so
+// metaCache only ever holds a type read from a .meta file or passed to
+// StoreVariant.
 func (c *Cache) loadVariantWithMeta(
 	cacheKey VariantKey,
 ) (io.ReadCloser, int64, string, error) {
@@ -773,27 +756,3 @@ func (c *Cache) checkNegativeCache(
 
 	return true, nil
 }
-
-// cleanExpiredBatch deletes up to cleanExpiredBatchSize negative cache
-// rows past their expiry in one statement, and returns how many it
-// deleted. DELETE ... LIMIT needs a SQLite build option, so the subquery
-// picks the rows.
-func (c *Cache) cleanExpiredBatch(ctx context.Context) (int64, error) {
-	result, err := c.db.ExecContext(ctx, `
-		DELETE FROM negative_cache WHERE id IN (
-			SELECT id FROM negative_cache
-			WHERE expires_at < CURRENT_TIMESTAMP
-			LIMIT ?
-		)
-	`, cleanExpiredBatchSize)
-	if err != nil {
-		return 0, fmt.Errorf("failed to clean negative cache: %w", err)
-	}
-
-	deleted, err := result.RowsAffected()
-	if err != nil {
-		return 0, fmt.Errorf("failed to count cleaned negative cache rows: %w", err)
-	}
-
-	return deleted, nil
-}

internal/imgcache/cachesize.go

@@ -78,6 +78,10 @@ func (c *Cache) computeDefaultMaxBytes(
 	computed := spaceBytes / freeSpaceFractionDenominator * freeSpaceFractionNumerator
 	computed = min(computed, math.MaxInt64)
 
+	// gosec cannot see that min() above bounds computed, so it reads
+	// this conversion as potentially overflowing. It cannot: computed is
+	// at most math.MaxInt64 on every path here.
+	//nolint:gosec // G115: clamped to MaxInt64 by min above
 	limit := int64(computed)
 	limit = max(limit, DefaultCacheMaxBytesFloor)
 

internal/imgcache/concurrent_misses_internal_test.go

@@ -15,7 +15,6 @@ import (
 
 	"github.com/getsentry/sentry-go"
 	"sneak.berlin/go/pixa/internal/httpfetcher"
-	"sneak.berlin/go/pixa/internal/imageprocessor"
 	"sneak.berlin/go/pixa/internal/magic"
 )
 
@@ -436,47 +435,6 @@ func TestService_Get_ReturnsByItsDeadline(t *testing.T) {
 	}
 }
 
-// TestService_Get_WaiterGetsProcessingTimedOut has a request with no deadline
-// join the held fetch of a first request with a short one. With the only
-// processing slot held, the first request's transcode waits for it until the
-// first request's deadline ends, and stops then. The waiting request, whose
-// own context has not ended, must get ErrProcessingTimedOut, which the image
-// routes answer 503 as when pixa is busy.
-func TestService_Get_WaiterGetsProcessingTimedOut(t *testing.T) {
-	t.Parallel()
-
-	svc, fixtures, fetcher := setupHeldFetchService(t)
-	svc.processor = imageprocessor.New(
-		imageprocessor.Params{MaxConcurrentProcessing: 1},
-	)
-
-	holdProcessingSlot(t, svc.processor)
-
-	const firstTimeout = 500 * time.Millisecond
-
-	firstCtx, cancelFirst := context.WithTimeout(t.Context(), firstTimeout)
-	defer cancelFirst()
-
-	first := startGet(firstCtx, svc, photoVariant(fixtures, 85, FitCover))
-
-	<-fetcher.started
-
-	waiter := startGet(t.Context(), svc, photoVariant(fixtures, 85, FitCover))
-
-	time.Sleep(arrivalWait)
-	fetcher.releaseFetches()
-
-	t.Logf("first request: Get() error = %v", (<-first).err)
-
-	got := <-waiter
-	t.Logf("waiting request: Get() error = %v", got.err)
-
-	if !errors.Is(got.err, ErrProcessingTimedOut) {
-		t.Errorf("waiting request: Get() error = %v, want %v",
-			got.err, ErrProcessingTimedOut)
-	}
-}
-
 // panickingFetcher panics on every fetch.
 type panickingFetcher struct{}
 

internal/imgcache/eviction.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"database/sql"
 	"encoding/json"
-	"errors"
 	"fmt"
 	"io/fs"
 	"os"
@@ -18,11 +17,6 @@ import (
 // write-pressure wakeups triggered by stores.
 const DefaultEvictionInterval = 5 * time.Minute
 
-// reconciliationInterval is how long after one reconciliation pass ends
-// the next one starts. A pass reads a row for every cached file, so its
-// cost grows with the cache, and it runs this rarely for that reason.
-const reconciliationInterval = 24 * time.Hour
-
 // evictionBatchSize is how many LRU candidates of each class (variants
 // and source blobs) one eviction pass fetches from the database.
 const evictionBatchSize = 100
@@ -442,19 +436,15 @@ func (c *Cache) notifyWritePressure() {
 	}
 }
 
-// StartEviction launches the background eviction goroutine. At startup
-// it reconciles the database accounting with the cache directories,
-// removes the expired negative cache rows and evicts to the configured
-// limit. After that it reconciles again a day after each reconciliation
-// pass ends, removes the expired negative cache rows on every tick of
-// the given interval, and evicts to the limit on every tick, after each
-// reconciliation pass and on write-pressure notifications. On a disabled
-// cache it only removes the expired negative cache rows, as the negative
-// cache is used either way. It is a no-op when already started. The
-// goroutine outlives the caller, so it runs with its own context, which
-// StopEviction cancels.
+// StartEviction launches the background eviction goroutine, which
+// reconciles the database accounting with the cache directories at
+// startup and again on every periodic tick thereafter, and evicts to
+// the configured limit on the given periodic interval and on
+// write-pressure notifications. It is a no-op on a disabled cache or
+// when already started. The goroutine outlives the caller, so it runs
+// with its own context, which StopEviction cancels.
 func (c *Cache) StartEviction(interval time.Duration) {
-	if c.evictionCancel != nil {
+	if c.disabled || c.evictionCancel != nil {
 		return
 	}
 
@@ -490,19 +480,8 @@ func (c *Cache) evictionLoop(ctx context.Context, interval time.Duration) {
 	defer close(c.evictionDone)
 
 	c.runReconciliationPass(ctx)
-	c.runCleanExpired(ctx)
 	c.runEvictionPass(ctx)
 
-	// Reconciliation walks the cache directories and reads a row for
-	// every file, so it runs on a timer of its own rather than on the
-	// periodic tick or on write-pressure wakeups. The timer starts only
-	// once a pass has ended, so passes never run back to back, however
-	// long one takes. Running it daily, not just at startup, bounds how
-	// long a store's best-effort accounting insert can stay silently
-	// unaccounted for to about one day, plus the time the passes take.
-	reconciliationTimer := time.NewTimer(reconciliationInterval)
-	defer reconciliationTimer.Stop()
-
 	ticker := time.NewTicker(interval)
 	defer ticker.Stop()
 
@@ -510,11 +489,17 @@ func (c *Cache) evictionLoop(ctx context.Context, interval time.Duration) {
 		select {
 		case <-ctx.Done():
 			return
-		case <-reconciliationTimer.C:
-			c.runReconciliationPass(ctx)
-			reconciliationTimer.Reset(reconciliationInterval)
 		case <-ticker.C:
-			c.runCleanExpired(ctx)
+			// Reconciliation walks the cache directories, so it only
+			// runs on the periodic ticker rather than on every
+			// write-pressure wakeup, keeping it off the per-store hot
+			// path. Reusing the eviction interval itself (rather than a
+			// separate, longer one) is a deliberate choice: it is the
+			// simplest option that still bounds how long a store's
+			// best-effort accounting insert can stay silently
+			// unaccounted for to one interval, on a process that is
+			// already running this loop regardless.
+			c.runReconciliationPass(ctx)
 		case <-c.evictionPressure:
 		}
 
@@ -550,34 +535,20 @@ func (c *Cache) runReconciliationPass(ctx context.Context) {
 	}
 }
 
-// runCleanExpired runs CleanExpired, logging a failure instead of
-// propagating it (the loop must keep running). It does nothing once ctx
-// is cancelled.
-func (c *Cache) runCleanExpired(ctx context.Context) {
-	if ctx.Err() != nil {
-		return
-	}
-
-	err := c.CleanExpired(ctx)
-	if err != nil {
-		c.log.Warn("negative cache cleanup failed", "error", err)
-	}
-}
-
 // reconcileAccounting synchronizes the database size accounting with
 // the actual contents of the cache directories. It runs at startup and
-// then once a day, off the request hot path: it adopts variant files
-// that predate the accounting table (or whose accounting insert failed,
-// e.g. StoreVariant's best-effort insert under transient DB contention),
-// drops accounting rows whose files are missing, removes source blob
-// files the database does not know (and rows whose files are gone),
-// removes source and variant files that are empty or not the size their
-// rows record, sweeps stale temp files left behind by crashed writes,
-// and last checks the total cache usage against the tables. It reads the
-// tables a page at a time. Running it daily, not just once, bounds how
-// long such drift can accumulate unaccounted for on a long-running
-// process to about one day. Once ctx is cancelled, it stops at the next
-// file or row and returns ctx's error.
+// again on every periodic eviction tick thereafter, off the request
+// hot path: it adopts variant files that predate the accounting table
+// (or whose accounting insert failed, e.g. StoreVariant's best-effort
+// insert under transient DB contention), drops accounting rows whose
+// files are missing, removes source blob files the database does not
+// know (and rows whose files are gone), sweeps stale temp files left
+// behind by crashed writes, and last checks the total cache usage
+// against the tables. It reads the tables a page at a time. Running it
+// periodically, not just once, bounds how long such drift can
+// accumulate unaccounted for on a long-running process to one eviction
+// interval. Once ctx is cancelled, it stops at the next file or row and
+// returns ctx's error.
 func (c *Cache) reconcileAccounting(ctx context.Context) error {
 	if c.disabled {
 		return nil
@@ -606,9 +577,8 @@ func (c *Cache) reconcileAccounting(ctx context.Context) error {
 	return c.reconcileUsageTotal(ctx)
 }
 
-// reconcileVariantFiles walks the variant storage directory, checking
-// each variant file against its accounting row (reconcileVariantFile)
-// and sweeping stale temp files.
+// reconcileVariantFiles walks the variant storage directory, adopting
+// files without accounting rows and sweeping stale temp files.
 func (c *Cache) reconcileVariantFiles(ctx context.Context) error {
 	return filepath.WalkDir(
 		c.variants.baseDir,
@@ -633,55 +603,38 @@ func (c *Cache) reconcileVariantFiles(ctx context.Context) error {
 				return nil
 			}
 
-			return c.reconcileVariantFile(ctx, path, entry, VariantKey(name))
+			return c.adoptVariantFile(ctx, path, entry, VariantKey(name))
 		},
 	)
 }
 
-// reconcileVariantFile checks one variant file against its accounting
-// row. A file without a row is adopted into the accounting, unless it is
-// empty. A file that is empty, or not the size its row records, as a
-// crash or a restored snapshot can leave it, is removed with its row and
-// a warning naming it, so the variant is converted again.
-func (c *Cache) reconcileVariantFile(
-	ctx context.Context, path string, entry fs.DirEntry, cacheKey VariantKey,
-) error {
-	info, err := entry.Info()
-	if err != nil {
-		return fmt.Errorf("failed to stat variant file: %w", err)
-	}
-
-	var recordedSize int64
-
-	err = c.db.QueryRowContext(ctx,
-		`SELECT size_bytes FROM variant_content WHERE cache_key = ?`, string(cacheKey),
-	).Scan(&recordedSize)
-
-	switch {
-	case errors.Is(err, sql.ErrNoRows) && info.Size() > 0:
-		return c.adoptVariantFile(ctx, path, info, cacheKey)
-	case err != nil && !errors.Is(err, sql.ErrNoRows):
-		return fmt.Errorf("failed to check variant accounting row: %w", err)
-	case info.Size() > 0 && info.Size() == recordedSize:
-		return nil
-	}
-
-	// The file is empty, or not the size its row records
-	c.log.Warn("removing damaged cached variant file", "path", path,
-		"file_bytes", info.Size(), "recorded_bytes", recordedSize)
-
-	return c.evictVariant(ctx, cacheKey)
-}
-
 // adoptVariantFile inserts an accounting row for a variant file that
 // has none, using the file's size and modification time.
 func (c *Cache) adoptVariantFile(
-	ctx context.Context, path string, info fs.FileInfo, cacheKey VariantKey,
+	ctx context.Context, path string, entry fs.DirEntry, cacheKey VariantKey,
 ) error {
+	var rowExists int
+
+	err := c.db.QueryRowContext(ctx,
+		`SELECT COUNT(*) FROM variant_content WHERE cache_key = ?`, string(cacheKey),
+	).Scan(&rowExists)
+	if err != nil {
+		return fmt.Errorf("failed to check variant accounting row: %w", err)
+	}
+
+	if rowExists > 0 {
+		return nil
+	}
+
+	info, err := entry.Info()
+	if err != nil {
+		return fmt.Errorf("failed to stat variant file: %w", err)
+	}
+
 	modTime := info.ModTime().UTC().Format(sqliteTimestampLayout)
 	contentType := c.variantContentTypeFromSidecar(path)
 
-	_, err := c.db.ExecContext(ctx, `
+	_, err = c.db.ExecContext(ctx, `
 		INSERT INTO variant_content
 			(cache_key, size_bytes, content_type, created_at, last_accessed_at)
 		VALUES (?, ?, ?, ?, ?)
@@ -800,9 +753,10 @@ func queryStringColumn[T ~string](
 	return values, nil
 }
 
-// reconcileSourceFiles walks the source content directory, checking
-// each blob file against its source_content row (reconcileSourceFile)
-// and sweeping stale temp files.
+// reconcileSourceFiles walks the source content directory, removing
+// blob files the database does not track (they are unreachable: source
+// lookups always go through source_metadata) and sweeping stale temp
+// files.
 func (c *Cache) reconcileSourceFiles(ctx context.Context) error {
 	return filepath.WalkDir(
 		c.srcContent.baseDir,
@@ -823,55 +777,31 @@ func (c *Cache) reconcileSourceFiles(ctx context.Context) error {
 				return nil
 			}
 
-			return c.reconcileSourceFile(ctx, path, entry, ContentHash(name))
+			return c.removeUntrackedSourceFile(ctx, path, ContentHash(name))
 		},
 	)
 }
 
-// reconcileSourceFile checks one source blob file against its
-// source_content row. A file without a row is removed: it is
-// unreachable, as source lookups always go through source_metadata. A
-// file that is empty, or not the size its row records, as a crash or a
-// restored snapshot can leave it, is removed with its rows and a warning
-// naming it, so the source is fetched again.
-func (c *Cache) reconcileSourceFile(
-	ctx context.Context, path string, entry fs.DirEntry, contentHash ContentHash,
-) error {
-	info, err := entry.Info()
-	if err != nil {
-		return fmt.Errorf("failed to stat source content file: %w", err)
-	}
-
-	var recordedSize int64
-
-	err = c.db.QueryRowContext(ctx,
-		`SELECT size_bytes FROM source_content WHERE content_hash = ?`,
-		string(contentHash),
-	).Scan(&recordedSize)
-
-	switch {
-	case errors.Is(err, sql.ErrNoRows):
-		return c.removeUntrackedSourceFile(ctx, path, contentHash)
-	case err != nil:
-		return fmt.Errorf("failed to check source content row: %w", err)
-	case info.Size() > 0 && info.Size() == recordedSize:
-		return nil
-	}
-
-	// The file is empty, or not the size its row records
-	c.log.Warn("removing damaged cached source file", "path", path,
-		"file_bytes", info.Size(), "recorded_bytes", recordedSize)
-
-	return c.evictSourceBlob(ctx, contentHash)
-}
-
 // removeUntrackedSourceFile deletes a source blob file that has no
 // source_content row. Any source_metadata rows referencing the hash
 // are removed first so no row ever points at a deleted file.
 func (c *Cache) removeUntrackedSourceFile(
 	ctx context.Context, path string, contentHash ContentHash,
 ) error {
-	_, err := c.db.ExecContext(ctx,
+	var rowExists int
+
+	err := c.db.QueryRowContext(ctx,
+		`SELECT COUNT(*) FROM source_content WHERE content_hash = ?`, string(contentHash),
+	).Scan(&rowExists)
+	if err != nil {
+		return fmt.Errorf("failed to check source content row: %w", err)
+	}
+
+	if rowExists > 0 {
+		return nil
+	}
+
+	_, err = c.db.ExecContext(ctx,
 		`DELETE FROM source_metadata WHERE content_hash = ?`, string(contentHash))
 	if err != nil {
 		return fmt.Errorf("failed to delete metadata rows for untracked blob: %w", err)

internal/imgcache/eviction_internal_test.go

@@ -9,9 +9,7 @@ import (
 	"log/slog"
 	"os"
 	"path/filepath"
-	"strconv"
 	"strings"
-	"sync/atomic"
 	"testing"
 	"time"
 
@@ -920,84 +918,6 @@ func TestPeriodicReconciliationAdoptsFileThatAppearsAfterStartup(t *testing.T) {
 	}
 }
 
-// TestEvictionTicksDoNotReconcile starts the eviction loop with an
-// interval of 10 ms. The startup pass reconciles before it removes the
-// expired negative cache rows, so once the expired row stored before the
-// start is gone, the startup reconciliation has made all its reads. The
-// test then waits for three periodic ticks, each seen when it removes an
-// expired row stored after the previous one went. The reconciliation
-// pass runs at startup and then once a day, so none of those ticks may
-// read a content table for reconciliation.
-func TestEvictionTicksDoNotReconcile(t *testing.T) {
-	t.Parallel()
-
-	cache, _ := newEvictionTestCache(t, 1<<30)
-
-	var reconciliationReads atomic.Int64
-
-	cache.reconciliationReadTestHook = func(int) { reconciliationReads.Add(1) }
-
-	insertExpiredNegativeCacheRow(t, cache, "/startup.jpg")
-
-	cache.StartEviction(10 * time.Millisecond)
-	defer func() { _ = cache.StopEviction(t.Context()) }()
-
-	waitForNegativeCacheRowRemoved(t, cache, "/startup.jpg")
-
-	startupReads := reconciliationReads.Load()
-	t.Logf("the startup pass made %d reconciliation reads", startupReads)
-
-	if startupReads == 0 {
-		t.Fatal("the startup pass made no reconciliation read")
-	}
-
-	for tick := range 3 {
-		path := "/tick-" + strconv.Itoa(tick) + ".jpg"
-
-		insertExpiredNegativeCacheRow(t, cache, path)
-		waitForNegativeCacheRowRemoved(t, cache, path)
-	}
-
-	tickReads := reconciliationReads.Load() - startupReads
-	t.Logf("three eviction ticks made %d reconciliation reads", tickReads)
-
-	if tickReads != 0 {
-		t.Errorf("three eviction ticks made %d reconciliation reads, want 0: "+
-			"the reconciliation pass ran on an eviction tick", tickReads)
-	}
-}
-
-// insertExpiredNegativeCacheRow adds a negative_cache row for sourcePath
-// that expired an hour ago.
-func insertExpiredNegativeCacheRow(t *testing.T, cache *Cache, sourcePath string) {
-	t.Helper()
-
-	_, err := cache.db.ExecContext(t.Context(), `
-		INSERT INTO negative_cache
-			(source_host, source_path, status_code, expires_at)
-		VALUES (?, ?, 404, datetime('now', '-1 hour'))
-	`, testHostExample, sourcePath)
-	if err != nil {
-		t.Fatalf("failed to insert an expired negative cache row: %v", err)
-	}
-}
-
-// waitForNegativeCacheRowRemoved waits up to five seconds for the
-// eviction loop to remove the negative_cache row for sourcePath.
-func waitForNegativeCacheRowRemoved(t *testing.T, cache *Cache, sourcePath string) {
-	t.Helper()
-
-	deadline := time.Now().Add(5 * time.Second)
-
-	for negativeCacheRowCount(t, cache, sourcePath) > 0 {
-		if time.Now().After(deadline) {
-			t.Fatalf("the expired negative cache row for %s was not removed", sourcePath)
-		}
-
-		time.Sleep(10 * time.Millisecond)
-	}
-}
-
 // TestStopEvictionInterruptsPassInProgress holds the test database's
 // only connection, so the startup reconciliation pass waits for it, and
 // checks that StopEviction stops that pass instead of waiting for the

internal/imgcache/negative_cache_internal_test.go

@@ -3,7 +3,6 @@ package imgcache
 import (
 	"context"
 	"errors"
-	"strconv"
 	"testing"
 	"time"
 )
@@ -95,307 +94,6 @@ func TestNegativeCache_Expired(t *testing.T) {
 	}
 }
 
-// TestService_Get_ServesCachedVariantOfNegativeCachedURL caches a variant of
-// the test photo, then keeps a failed fetch of the photo's URL in the negative
-// cache, as a failed fetch for another variant does once the source has been
-// evicted. The cached variant is still served, and a variant not in the cache
-// is made from the cached source. Once the source is evicted, a variant not in
-// the cache needs a fetch and is refused.
-func TestService_Get_ServesCachedVariantOfNegativeCachedURL(t *testing.T) {
-	t.Parallel()
-
-	svc, fixtures := SetupTestService(t)
-
-	resp, err := svc.Get(t.Context(), photoVariant(fixtures, 85, FitCover))
-	if err != nil {
-		t.Fatalf("Get() first request error = %v", err)
-	}
-
-	_ = resp.Content.Close()
-
-	err = svc.cache.StoreNegative(t.Context(), photoVariant(fixtures, 85, FitCover),
-		httpStatusBadGateway, "upstream answered 503")
-	if err != nil {
-		t.Fatalf("StoreNegative() error = %v", err)
-	}
-
-	resp, err = svc.Get(t.Context(), photoVariant(fixtures, 85, FitCover))
-	if err != nil {
-		t.Fatalf("Get() of the cached variant error = %v", err)
-	}
-
-	_ = resp.Content.Close()
-
-	if resp.CacheStatus != CacheHit {
-		t.Errorf("cached variant CacheStatus = %q, want %q",
-			resp.CacheStatus, CacheHit)
-	}
-
-	resp, err = svc.Get(t.Context(), photoVariant(fixtures, 40, FitCover))
-	t.Logf("Get() of a variant made from the cached source: error = %v", err)
-
-	if err != nil {
-		t.Fatalf("Get() of a variant made from the cached source error = %v", err)
-	}
-
-	_ = resp.Content.Close()
-
-	contentHash, _, err := svc.cache.LookupSource(t.Context(),
-		photoVariant(fixtures, 85, FitCover))
-	if err != nil || contentHash == "" {
-		t.Fatalf("LookupSource() = %q, %v, want the cached source", contentHash, err)
-	}
-
-	err = svc.cache.evictSourceBlob(t.Context(), contentHash)
-	if err != nil {
-		t.Fatalf("evictSourceBlob() error = %v", err)
-	}
-
-	_, err = svc.Get(t.Context(), photoVariant(fixtures, 60, FitCover))
-	t.Logf("Get() of a variant whose source was evicted: error = %v", err)
-
-	if !errors.Is(err, ErrNegativeCached) {
-		t.Errorf("Get() of a variant whose source was evicted error = %v, want %v",
-			err, ErrNegativeCached)
-	}
-}
-
-// TestEvictionLoopRemovesExpiredNegativeCacheRows stores a failed fetch
-// that expires a second later and starts the eviction loop with a short
-// interval: a later tick must remove its row although its URL is never
-// requested again, and must keep a row that has not expired. It runs
-// with the disk cache enabled and disabled, as the negative cache is
-// used either way.
-func TestEvictionLoopRemovesExpiredNegativeCacheRows(t *testing.T) {
-	t.Parallel()
-
-	tests := []struct {
-		name             string
-		disableDiskCache bool
-	}{
-		{name: "disk cache enabled", disableDiskCache: false},
-		{name: "disk cache disabled", disableDiskCache: true},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			t.Parallel()
-
-			cache, err := NewCache(evictionTestDB(t), CacheConfig{
-				StateDir:         t.TempDir(),
-				NegativeTTL:      time.Second,
-				DisableDiskCache: tt.disableDiskCache,
-			})
-			if err != nil {
-				t.Fatalf("failed to create cache: %v", err)
-			}
-
-			expired := &ImageRequest{
-				SourceHost: testHostExample, SourcePath: "/gone.jpg",
-			}
-
-			err = cache.StoreNegative(t.Context(), expired, 404, "not found")
-			if err != nil {
-				t.Fatalf("StoreNegative failed: %v", err)
-			}
-
-			const notExpiredPath = "/later.jpg"
-
-			_, err = cache.db.ExecContext(t.Context(), `
-				INSERT INTO negative_cache
-					(source_host, source_path, status_code, expires_at)
-				VALUES (?, ?, 404, datetime('now', '+1 hour'))
-			`, testHostExample, notExpiredPath)
-			if err != nil {
-				t.Fatalf("failed to insert a row that has not expired: %v", err)
-			}
-
-			started := time.Now()
-
-			cache.StartEviction(100 * time.Millisecond)
-			defer func() { _ = cache.StopEviction(t.Context()) }()
-
-			for negativeCacheRowCount(t, cache, expired.SourcePath) > 0 {
-				if time.Since(started) > 10*time.Second {
-					t.Fatal("the expired row is still in negative_cache")
-				}
-
-				time.Sleep(50 * time.Millisecond)
-			}
-
-			t.Logf("the expired row was removed %s after the loop started",
-				time.Since(started))
-
-			if negativeCacheRowCount(t, cache, notExpiredPath) != 1 {
-				t.Error("the row that has not expired was removed")
-			}
-		})
-	}
-}
-
-// negativeCacheRowCount returns how many negative_cache rows hold
-// sourcePath.
-func negativeCacheRowCount(tb testing.TB, cache *Cache, sourcePath string) int {
-	tb.Helper()
-
-	var count int
-
-	err := cache.db.QueryRowContext(tb.Context(),
-		`SELECT COUNT(*) FROM negative_cache WHERE source_path = ?`,
-		sourcePath,
-	).Scan(&count)
-	if err != nil {
-		tb.Fatalf("failed to count negative_cache rows: %v", err)
-	}
-
-	return count
-}
-
-// TestCleanExpiredDeletesInBatches stores one expired negative cache row
-// more than one batch holds, and one row that has not expired. One
-// statement must delete one batch and no more, so a request waits behind
-// one batch at most; CleanExpired must then remove the expired row left
-// over and keep the row that has not expired.
-func TestCleanExpiredDeletesInBatches(t *testing.T) {
-	t.Parallel()
-
-	cache, err := NewCache(evictionTestDB(t), CacheConfig{StateDir: t.TempDir()})
-	if err != nil {
-		t.Fatalf("failed to create cache: %v", err)
-	}
-
-	ctx := t.Context()
-
-	insertExpiredNegativeCacheRows(t, cache, cleanExpiredBatchSize+1)
-
-	const notExpiredPath = "/later.jpg"
-
-	_, err = cache.db.ExecContext(ctx, `
-		INSERT INTO negative_cache
-			(source_host, source_path, status_code, expires_at)
-		VALUES (?, ?, 404, datetime('now', '+1 hour'))
-	`, testHostExample, notExpiredPath)
-	if err != nil {
-		t.Fatalf("failed to insert a row that has not expired: %v", err)
-	}
-
-	deleted, err := cache.cleanExpiredBatch(ctx)
-	if err != nil {
-		t.Fatalf("cleanExpiredBatch failed: %v", err)
-	}
-
-	t.Logf("one statement deleted %d of %d expired rows",
-		deleted, cleanExpiredBatchSize+1)
-
-	if deleted != cleanExpiredBatchSize {
-		t.Errorf("one statement deleted %d rows, want %d",
-			deleted, cleanExpiredBatchSize)
-	}
-
-	err = cache.CleanExpired(ctx)
-	if err != nil {
-		t.Fatalf("CleanExpired failed: %v", err)
-	}
-
-	if count := negativeCacheTableRowCount(t, cache); count != 1 {
-		t.Errorf("negative_cache holds %d rows after CleanExpired, want 1", count)
-	}
-
-	if negativeCacheRowCount(t, cache, notExpiredPath) != 1 {
-		t.Error("the row that has not expired was removed")
-	}
-}
-
-// TestCleanExpiredRemovesMoreThanOneBatch stores one expired negative
-// cache row more than one batch holds and calls CleanExpired once: it
-// must remove them all, so it fails if CleanExpired stops after its
-// first batch.
-func TestCleanExpiredRemovesMoreThanOneBatch(t *testing.T) {
-	t.Parallel()
-
-	cache, err := NewCache(evictionTestDB(t), CacheConfig{StateDir: t.TempDir()})
-	if err != nil {
-		t.Fatalf("failed to create cache: %v", err)
-	}
-
-	insertExpiredNegativeCacheRows(t, cache, cleanExpiredBatchSize+1)
-
-	before := negativeCacheTableRowCount(t, cache)
-
-	err = cache.CleanExpired(t.Context())
-	if err != nil {
-		t.Fatalf("CleanExpired failed: %v", err)
-	}
-
-	after := negativeCacheTableRowCount(t, cache)
-
-	t.Logf("CleanExpired removed %d of %d expired rows, %d per batch",
-		before-after, before, cleanExpiredBatchSize)
-
-	if after != 0 {
-		t.Errorf("negative_cache holds %d rows after CleanExpired, want 0", after)
-	}
-}
-
-// TestCleanExpiredStopsOnceContextEnds runs CleanExpired with a context
-// that has already ended: it must return the context's error and delete
-// no row.
-func TestCleanExpiredStopsOnceContextEnds(t *testing.T) {
-	t.Parallel()
-
-	cache, err := NewCache(evictionTestDB(t), CacheConfig{StateDir: t.TempDir()})
-	if err != nil {
-		t.Fatalf("failed to create cache: %v", err)
-	}
-
-	insertExpiredNegativeCacheRows(t, cache, 1)
-
-	ctx, cancel := context.WithCancel(t.Context())
-	cancel()
-
-	err = cache.CleanExpired(ctx)
-	if !errors.Is(err, context.Canceled) {
-		t.Errorf("CleanExpired() error = %v, want context.Canceled", err)
-	}
-
-	if count := negativeCacheTableRowCount(t, cache); count != 1 {
-		t.Errorf("negative_cache holds %d rows, want the expired row kept", count)
-	}
-}
-
-// insertExpiredNegativeCacheRows inserts count negative_cache rows that
-// expired an hour ago, each for a different source path.
-func insertExpiredNegativeCacheRows(t *testing.T, cache *Cache, count int) {
-	t.Helper()
-
-	for i := range count {
-		_, err := cache.db.ExecContext(t.Context(), `
-			INSERT INTO negative_cache
-				(source_host, source_path, status_code, expires_at)
-			VALUES (?, ?, 404, datetime('now', '-1 hour'))
-		`, testHostExample, "/expired/"+strconv.Itoa(i)+".jpg")
-		if err != nil {
-			t.Fatalf("failed to insert an expired row: %v", err)
-		}
-	}
-}
-
-// negativeCacheTableRowCount returns how many rows negative_cache holds.
-func negativeCacheTableRowCount(tb testing.TB, cache *Cache) int {
-	tb.Helper()
-
-	var count int
-
-	err := cache.db.QueryRowContext(tb.Context(),
-		`SELECT COUNT(*) FROM negative_cache`,
-	).Scan(&count)
-	if err != nil {
-		tb.Fatalf("failed to count negative_cache rows: %v", err)
-	}
-
-	return count
-}
-
 func TestService_Get_ReturnsErrorForNegativeCachedURL(t *testing.T) {
 	t.Parallel()
 

internal/imgcache/partial_cache_files_internal_test.go

@@ -1,359 +0,0 @@
-package imgcache
-
-import (
-	"bytes"
-	"crypto/sha256"
-	"encoding/hex"
-	"io"
-	"log/slog"
-	"os"
-	"path/filepath"
-	"strings"
-	"testing"
-)
-
-// getImage calls Get for req and returns the image it answered with and
-// its cache status.
-func getImage(t *testing.T, svc *Service, req *ImageRequest) ([]byte, CacheStatus) {
-	t.Helper()
-
-	resp, err := svc.Get(t.Context(), req)
-	if err != nil {
-		t.Fatalf("Get() of quality %d error = %v", req.Quality, err)
-	}
-
-	defer func() { _ = resp.Content.Close() }()
-
-	body, err := io.ReadAll(resp.Content)
-	if err != nil {
-		t.Fatalf("reading the image of quality %d: %v", req.Quality, err)
-	}
-
-	return body, resp.CacheStatus
-}
-
-// fileSize returns the size of the file at path.
-func fileSize(t *testing.T, path string) int64 {
-	t.Helper()
-
-	info, err := os.Stat(path)
-	if err != nil {
-		t.Fatalf("stat %s: %v", path, err)
-	}
-
-	return info.Size()
-}
-
-// truncateFile cuts the file at path down to size bytes.
-func truncateFile(t *testing.T, path string, size int64) {
-	t.Helper()
-
-	t.Logf("cutting %s from %d to %d bytes",
-		filepath.Base(path), fileSize(t, path), size)
-
-	err := os.Truncate(path, size)
-	if err != nil {
-		t.Fatalf("truncating %s: %v", path, err)
-	}
-}
-
-// cachedSourcePath returns the path of the cached source file req uses.
-func cachedSourcePath(t *testing.T, svc *Service, req *ImageRequest) string {
-	t.Helper()
-
-	contentHash, _, err := svc.cache.LookupSource(t.Context(), req)
-	if err != nil || contentHash == "" {
-		t.Fatalf("LookupSource() = %q, %v; want the cached source", contentHash, err)
-	}
-
-	return svc.cache.srcContent.hashToPath(contentHash)
-}
-
-// reconcileAsAtStartup runs the cache's reconciliation pass, as pixa does
-// when it starts, and returns what the pass logged.
-func reconcileAsAtStartup(t *testing.T, cache *Cache) string {
-	t.Helper()
-
-	var logBuf bytes.Buffer
-
-	cache.log = slog.New(slog.NewJSONHandler(&logBuf, nil))
-
-	err := cache.reconcileAccounting(t.Context())
-	if err != nil {
-		t.Fatalf("reconcileAccounting() error = %v", err)
-	}
-
-	t.Logf("reconciliation log: %s", logBuf.String())
-
-	return logBuf.String()
-}
-
-// assertWarningNames checks that logs hold a warning naming the file at
-// path.
-func assertWarningNames(t *testing.T, logs, path string) {
-	t.Helper()
-
-	for line := range strings.Lines(logs) {
-		if strings.Contains(line, `"level":"WARN"`) &&
-			strings.Contains(line, `"path":"`+path+`"`) {
-			return
-		}
-	}
-
-	t.Errorf("reconciliation logged no warning naming %s", path)
-}
-
-// TestService_Get_FetchesAgainForEmptyCachedSource empties the cached source
-// file, as a crash or a restored snapshot could, and runs the reconciliation
-// pass, which must remove it with a warning naming it. It then asks for two
-// new sizes of the source: the first must fetch it again and cache it whole,
-// so that the second is made from the cache without a third fetch.
-func TestService_Get_FetchesAgainForEmptyCachedSource(t *testing.T) {
-	t.Parallel()
-
-	svc, fixtures := SetupTestService(t)
-
-	getImage(t, svc, photoVariant(fixtures, 85, FitCover))
-
-	path := cachedSourcePath(t, svc, photoVariant(fixtures, 85, FitCover))
-	wholeSize := fileSize(t, path)
-
-	truncateFile(t, path, 0)
-	assertWarningNames(t, reconcileAsAtStartup(t, svc.cache), path)
-
-	getImage(t, svc, photoVariant(fixtures, 40, FitCover))
-	getImage(t, svc, photoVariant(fixtures, 30, FitCover))
-
-	fetches := readCacheStatsCounters(t, svc.cache).upstreamFetchCount
-	t.Logf("upstream fetches = %d", fetches)
-
-	if fetches != 2 {
-		t.Errorf("upstream fetches = %d, want 2: the empty cached source "+
-			"was kept and fetched again on every new size", fetches)
-	}
-
-	size := fileSize(t, path)
-	if size != wholeSize {
-		t.Errorf("cached source is %d bytes, want %d", size, wholeSize)
-	}
-}
-
-// TestService_Get_FetchesAgainForPartialCachedSource cuts the cached source
-// file to half its size, as a crash or a restored snapshot could, and runs
-// the reconciliation pass, which must remove it with a warning naming it.
-// It then asks for two new sizes of the source: the first must fetch it
-// again rather than fail to decode the partial file, and the second is
-// made from the cache.
-func TestService_Get_FetchesAgainForPartialCachedSource(t *testing.T) {
-	t.Parallel()
-
-	svc, fixtures := SetupTestService(t)
-
-	getImage(t, svc, photoVariant(fixtures, 85, FitCover))
-
-	path := cachedSourcePath(t, svc, photoVariant(fixtures, 85, FitCover))
-	wholeSize := fileSize(t, path)
-
-	truncateFile(t, path, wholeSize/2)
-	assertWarningNames(t, reconcileAsAtStartup(t, svc.cache), path)
-
-	getImage(t, svc, photoVariant(fixtures, 40, FitCover))
-	getImage(t, svc, photoVariant(fixtures, 30, FitCover))
-
-	fetches := readCacheStatsCounters(t, svc.cache).upstreamFetchCount
-	t.Logf("upstream fetches = %d", fetches)
-
-	if fetches != 2 {
-		t.Errorf("upstream fetches = %d, want 2", fetches)
-	}
-
-	size := fileSize(t, path)
-	if size != wholeSize {
-		t.Errorf("cached source is %d bytes, want %d", size, wholeSize)
-	}
-}
-
-// assertConvertedAgain caches a variant of the photo, cuts its file to size
-// bytes and runs the reconciliation pass, which must remove it with a
-// warning naming it. It then checks that Get converts the image again
-// instead of serving the file, and that the next Get serves the whole
-// variant from the cache.
-func assertConvertedAgain(t *testing.T, size int64) {
-	t.Helper()
-
-	svc, fixtures := SetupTestService(t)
-	req := photoVariant(fixtures, 85, FitCover)
-
-	want, _ := getImage(t, svc, req)
-
-	path := svc.cache.variants.keyToPath(CacheKey(req))
-
-	truncateFile(t, path, size)
-	assertWarningNames(t, reconcileAsAtStartup(t, svc.cache), path)
-
-	got, status := getImage(t, svc, req)
-	t.Logf("after the cut: %d bytes, %s", len(got), status)
-
-	if status != CacheMiss || !bytes.Equal(got, want) {
-		t.Errorf("after the cut: Get() = %d bytes, %s; want the %d bytes "+
-			"converted again, %s", len(got), status, len(want), CacheMiss)
-	}
-
-	got, status = getImage(t, svc, req)
-	t.Logf("next request: %d bytes, %s", len(got), status)
-
-	if status != CacheHit || !bytes.Equal(got, want) {
-		t.Errorf("next request: Get() = %d bytes, %s; want the %d bytes, %s",
-			len(got), status, len(want), CacheHit)
-	}
-}
-
-// TestService_Get_ConvertsAgainForEmptyCachedVariant empties a cached
-// variant file, as a crash or a restored snapshot could, and checks that it
-// is converted again rather than served empty.
-func TestService_Get_ConvertsAgainForEmptyCachedVariant(t *testing.T) {
-	t.Parallel()
-
-	assertConvertedAgain(t, 0)
-}
-
-// TestService_Get_ConvertsAgainForPartialCachedVariant cuts a cached
-// variant file to ten bytes, as a crash or a restored snapshot could, and
-// checks that it is converted again rather than served cut short.
-func TestService_Get_ConvertsAgainForPartialCachedVariant(t *testing.T) {
-	t.Parallel()
-
-	assertConvertedAgain(t, 10)
-}
-
-// TestReconciliationRemovesEmptyVariantFileWithoutRow checks that the
-// reconciliation pass removes an empty variant file that has no
-// accounting row, with a warning naming it, instead of adding a row for
-// it that would have it served empty.
-func TestReconciliationRemovesEmptyVariantFileWithoutRow(t *testing.T) {
-	t.Parallel()
-
-	cache, _ := newEvictionTestCache(t, 1<<30)
-
-	_, err := cache.variants.Store(testVariantKeyOne, bytes.NewReader(nil),
-		testContentTypeWebP)
-	if err != nil {
-		t.Fatalf("failed to store an empty variant file: %v", err)
-	}
-
-	logs := reconcileAsAtStartup(t, cache)
-
-	assertWarningNames(t, logs, cache.variants.keyToPath(testVariantKeyOne))
-
-	if cache.variants.Exists(testVariantKeyOne) {
-		t.Error("the empty variant file is still on disk")
-	}
-
-	rows := countRows(t, cache,
-		`SELECT COUNT(*) FROM variant_content WHERE cache_key = ?`,
-		string(testVariantKeyOne))
-	if rows != 0 {
-		t.Errorf("the empty variant file got %d accounting rows, want 0", rows)
-	}
-}
-
-// TestVariantStorage_StoreReplacesMetaFile stores a variant over one
-// already stored and checks that Store puts a new .meta file in place of
-// the old one, as it does the image, rather than writing into the old
-// file, which a crash could leave empty. The old .meta file is given a
-// second name before the store, so that it can be told apart from the new
-// one after it.
-func TestVariantStorage_StoreReplacesMetaFile(t *testing.T) {
-	t.Parallel()
-
-	storage, key := storeTestVariant(t)
-	metaPath := storage.keyToPath(key) + variantMetaSuffix
-	oldMetaPath := metaPath + ".old"
-
-	err := os.Link(metaPath, oldMetaPath)
-	if err != nil {
-		t.Fatalf("giving the .meta file a second name: %v", err)
-	}
-
-	_, err = storage.Store(key, bytes.NewReader([]byte("new variant data")),
-		testContentTypeJPEG)
-	if err != nil {
-		t.Fatalf("Store() error = %v", err)
-	}
-
-	oldMeta, err := os.Stat(oldMetaPath)
-	if err != nil {
-		t.Fatalf("stat of the old .meta file: %v", err)
-	}
-
-	newMeta, err := os.Stat(metaPath)
-	if err != nil {
-		t.Fatalf("stat of the new .meta file: %v", err)
-	}
-
-	if os.SameFile(oldMeta, newMeta) {
-		t.Error("Store() wrote into the old .meta file instead of putting " +
-			"a new one in its place")
-	}
-
-	reader, _, contentType, err := storage.LoadWithMeta(key)
-	if err != nil {
-		t.Fatalf("LoadWithMeta() error = %v", err)
-	}
-
-	_ = reader.Close()
-
-	t.Logf("content type after the store: %s", contentType)
-
-	if contentType != testContentTypeJPEG {
-		t.Errorf("content type = %q, want %q", contentType, testContentTypeJPEG)
-	}
-}
-
-// TestContentStorage_StoreHashedReplacesFileOfAnotherSize checks that a
-// store over a file of another size at the content's path, such as a
-// partial file a crash left, replaces it with the content.
-func TestContentStorage_StoreHashedReplacesFileOfAnotherSize(t *testing.T) {
-	t.Parallel()
-
-	storage, err := NewContentStorage(t.TempDir())
-	if err != nil {
-		t.Fatalf("NewContentStorage() error = %v", err)
-	}
-
-	content := []byte("the whole source image")
-	sum := sha256.Sum256(content)
-	hash := ContentHash(hex.EncodeToString(sum[:]))
-	path := storage.hashToPath(hash)
-
-	err = os.MkdirAll(filepath.Dir(path), StorageDirPerm)
-	if err != nil {
-		t.Fatalf("creating the content's directory: %v", err)
-	}
-
-	err = os.WriteFile(path, content[:5], StorageFilePerm)
-	if err != nil {
-		t.Fatalf("writing a partial file: %v", err)
-	}
-
-	_, err = storage.StoreHashed(hash, content)
-	if err != nil {
-		t.Fatalf("StoreHashed() error = %v", err)
-	}
-
-	stored, err := storage.Load(hash)
-	if err != nil {
-		t.Fatalf("Load() error = %v", err)
-	}
-
-	defer func() { _ = stored.Close() }()
-
-	got, err := io.ReadAll(stored)
-	if err != nil {
-		t.Fatalf("reading the stored content: %v", err)
-	}
-
-	if !bytes.Equal(got, content) {
-		t.Errorf("stored file = %q, want %q", got, content)
-	}
-}

internal/imgcache/service.go

@@ -9,7 +9,6 @@ import (
 	"log/slog"
 	"net/url"
 	"runtime/debug"
-	"strings"
 	"time"
 
 	"github.com/dustin/go-humanize"
@@ -129,20 +128,26 @@ func NewService(cfg *ServiceConfig) (*Service, error) {
 // ErrNegativeCached is returned when a URL is in the negative cache (recently failed).
 var ErrNegativeCached = errors.New("request is in negative cache (recently failed)")
 
-// ErrProcessingTimedOut is returned when processing an image stopped because
-// the downstream_timeout of the request doing it ended, to that request and to
-// every request that waited for its image.
-var ErrProcessingTimedOut = errors.New(
-	"image processing stopped at the deadline of the request doing it")
-
 // Get retrieves a processed image, fetching and processing if necessary.
-// While its source URL is in the negative cache, a cached variant is still
-// served and a variant not in the cache is still made from a cached source;
-// only a request that needs the source fetched is refused.
 func (s *Service) Get(ctx context.Context, req *ImageRequest) (*ImageResponse, error) {
 	// Propagate AllowHTTP setting to the request
 	req.AllowHTTP = s.allowHTTP
 
+	// Check negative cache first - skip fetching for recently-failed URLs
+	negHit, err := s.cache.checkNegativeCache(ctx, req)
+	if err != nil {
+		s.log.Warn("negative cache check failed", "error", err)
+	}
+
+	if negHit {
+		s.log.Debug("negative cache hit",
+			"host", req.SourceHost,
+			"path", req.SourcePath,
+		)
+
+		return nil, fmt.Errorf("%w: %w", httpfetcher.ErrUpstreamError, ErrNegativeCached)
+	}
+
 	// Check variant cache first (disk only, no DB)
 	result, err := s.cache.Lookup(ctx, req)
 	if err != nil {
@@ -213,16 +218,7 @@ func (s *Service) ValidateRequest(req *ImageRequest) error {
 		return fmt.Errorf("invalid source URL: %w", err)
 	}
 
-	// An allowlist entry names no port, so a host followed by ":", with or
-	// without a port, needs a signature. The port is read from the parsed
-	// URL, as the fetcher reads it: in an IPv6 address written without
-	// brackets, such as 2001:db8::1:8443, it is what follows the last ":".
-	// This is checked here and not in allowlist.IsAllowed, which also matches
-	// referer_blocklist, where a Referer must be blocked whatever its port.
-	hostHasPort := parsedURL.Port() != "" ||
-		strings.HasSuffix(parsedURL.Host, ":")
-
-	if !hostHasPort && s.allowlist.IsAllowed(parsedURL) {
+	if s.allowlist.IsAllowed(parsedURL) {
 		return nil
 	}
 
@@ -427,28 +423,12 @@ func (s *Service) processFromSourceOrFetch(
 
 // fetchAndProcess fetches from upstream, processes, and caches the result.
 // It counts the fetch with the bytes read from upstream, including when
-// reading the response or a later step fails. It fetches nothing while the
-// source URL is in the negative cache.
+// reading the response or a later step fails.
 func (s *Service) fetchAndProcess(
 	ctx context.Context,
 	req *ImageRequest,
 	cacheKey VariantKey,
 ) (*processedVariant, error) {
-	// Check negative cache - skip fetching for recently-failed URLs
-	negHit, err := s.cache.checkNegativeCache(ctx, req)
-	if err != nil {
-		s.log.Warn("negative cache check failed", "error", err)
-	}
-
-	if negHit {
-		s.log.Debug("negative cache hit",
-			"host", req.SourceHost,
-			"path", req.SourcePath,
-		)
-
-		return nil, fmt.Errorf("%w: %w", httpfetcher.ErrUpstreamError, ErrNegativeCached)
-	}
-
 	// Fetch from upstream
 	sourceURL := req.SourceURL()
 
@@ -458,13 +438,7 @@ func (s *Service) fetchAndProcess(
 	if err != nil {
 		// Store negative cache for certain errors
 		if isNegativeCacheable(err) {
-			statusCode := extractStatusCode(err)
-
-			storeErr := s.cache.StoreNegative(ctx, req, statusCode, err.Error())
-			if storeErr != nil {
-				s.log.Warn("failed to store negative cache entry",
-					"host", req.SourceHost, "path", req.SourcePath, "error", storeErr)
-			}
+			s.storeNegative(ctx, req, extractStatusCode(err), err)
 		}
 
 		return nil, fmt.Errorf("upstream fetch failed: %w", err)
@@ -510,9 +484,16 @@ func (s *Service) fetchAndProcess(
 		"cipher", fetchResult.TLSCipherSuite,
 	)
 
-	// Validate magic bytes match content type
+	// Validate magic bytes match content type. A body that does not is the
+	// upstream's error, so it gets a negative cache entry, as an upstream
+	// error status does.
 	err = magic.ValidateMagicBytes(sourceData, fetchResult.ContentType)
 	if err != nil {
+		s.log.Warn("source refused", "request_id", middleware.GetReqID(ctx),
+			"host", req.SourceHost, "path", req.SourcePath,
+			"content_type", fetchResult.ContentType, "error", err)
+		s.storeNegative(ctx, req, httpStatusUnsupportedMediaType, err)
+
 		return nil, fmt.Errorf("content validation failed: %w", err)
 	}
 
@@ -528,6 +509,19 @@ func (s *Service) fetchAndProcess(
 	)
 }
 
+// storeNegative stores a negative cache entry for req's source, whose fetch
+// failed with err, so the source is not fetched again until the entry
+// expires. A failure to store the entry is only logged.
+func (s *Service) storeNegative(
+	ctx context.Context, req *ImageRequest, statusCode int, err error,
+) {
+	storeErr := s.cache.StoreNegative(ctx, req, statusCode, err.Error())
+	if storeErr != nil {
+		s.log.Warn("failed to store negative cache entry",
+			"host", req.SourceHost, "path", req.SourcePath, "error", storeErr)
+	}
+}
+
 // processedVariant is a variant as processAndStore made it. Each request that
 // shared its processing serves it through a reader of its own.
 type processedVariant struct {
@@ -555,13 +549,7 @@ func (s *Service) processAndStore(
 		FitMode: imageprocessor.FitMode(req.FitMode),
 	}
 
-	// ctx has the deadline of the request doing the processing, which can
-	// end while a request waiting for its image has time left
 	processResult, err := s.processor.Process(ctx, source, processReq)
-	if errors.Is(err, context.DeadlineExceeded) {
-		return nil, fmt.Errorf("%w: %w", ErrProcessingTimedOut, err)
-	}
-
 	if err != nil {
 		return nil, fmt.Errorf("image processing failed: %w", err)
 	}
@@ -642,8 +630,9 @@ func signatureRequest(req *ImageRequest) *signature.Request {
 
 // HTTP status codes for error responses.
 const (
-	httpStatusBadGateway    = 502
-	httpStatusInternalError = 500
+	httpStatusBadGateway           = 502
+	httpStatusInternalError        = 500
+	httpStatusUnsupportedMediaType = 415
 )
 
 // isNegativeCacheable returns true if the error should be cached.

internal/imgcache/service_internal_test.go

@@ -250,97 +250,6 @@ func TestService_ValidateRequest_SignatureExactHostMatch(t *testing.T) {
 	}
 }
 
-// TestService_ValidateRequest_HostWithPortNeedsSignature verifies that a host
-// followed by a port, or by a colon with no port, matches no allowlist entry,
-// so that a request for it without a signature is refused, while the same
-// host with no port is allowlisted.
-func TestService_ValidateRequest_HostWithPortNeedsSignature(t *testing.T) {
-	t.Parallel()
-
-	svc, _ := SetupTestService(t,
-		WithSigningKey(testSigningKey),
-		WithAllowlist(testHostCDN, "203.0.113.5", "2001:db8::1"),
-	)
-
-	tests := []struct {
-		host        string
-		allowlisted bool
-	}{
-		{testHostCDN, true},
-		{"cdn.example.com:8443", false},
-		{"cdn.example.com:443", false},
-		{"cdn.example.com:", false},
-		{"203.0.113.5", true},
-		{"203.0.113.5:8080", false},
-		{"[2001:db8::1]", true},
-		{"[2001:db8::1]:8443", false},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.host, func(t *testing.T) {
-			t.Parallel()
-
-			req := &ImageRequest{
-				SourceHost: tt.host,
-				SourcePath: testPathCat,
-				Size:       Size{Width: 50, Height: 50},
-				Format:     FormatJPEG,
-				Quality:    85,
-				FitMode:    FitCover,
-			}
-
-			err := svc.ValidateRequest(req)
-			t.Logf("ValidateRequest() for host %q with no signature: %v",
-				tt.host, err)
-
-			if allowlisted := err == nil; allowlisted != tt.allowlisted {
-				t.Errorf("host %q served without a signature = %v, want %v",
-					tt.host, allowlisted, tt.allowlisted)
-			}
-		})
-	}
-}
-
-// TestService_ValidateRequest_UnbracketedIPv6PortNeedsSignature verifies that
-// an allowlisted IPv6 address written without brackets and followed by a
-// port, or by a colon with no port, needs a signature. Go's URL parser, which
-// the fetcher uses, reads 2001:db8::1:8443 as the host 2001:db8::1 with the
-// port 8443.
-func TestService_ValidateRequest_UnbracketedIPv6PortNeedsSignature(
-	t *testing.T,
-) {
-	t.Parallel()
-
-	svc, _ := SetupTestService(t,
-		WithSigningKey(testSigningKey),
-		WithAllowlist("2001:db8::1"),
-	)
-
-	for _, host := range []string{"2001:db8::1:8443", "2001:db8::1:"} {
-		t.Run(host, func(t *testing.T) {
-			t.Parallel()
-
-			req := &ImageRequest{
-				SourceHost: host,
-				SourcePath: testPathCat,
-				Size:       Size{Width: 50, Height: 50},
-				Format:     FormatJPEG,
-				Quality:    85,
-				FitMode:    FitCover,
-			}
-
-			err := svc.ValidateRequest(req)
-			t.Logf("ValidateRequest() for host %q with no signature: %v",
-				host, err)
-
-			if err == nil {
-				t.Errorf("host %q served without a signature, want refused",
-					host)
-			}
-		})
-	}
-}
-
 func TestService_Get_InvalidFile(t *testing.T) {
 	t.Parallel()
 

internal/imgcache/storage.go

@@ -80,8 +80,8 @@ func (s *ContentStorage) Store(r io.Reader) (ContentHash, int64, error) {
 // from hash, without recomputing it. Callers that already know the
 // hash before writing (e.g. because they must hold a hash-keyed lock
 // across the whole store operation) use this instead of Store. Like
-// Store, it is idempotent: a file of the content's size already at that
-// path is left untouched, and a file of another size is replaced.
+// Store, it is idempotent: content already on disk at that path is
+// left untouched.
 func (s *ContentStorage) StoreHashed(
 	hash ContentHash, data []byte,
 ) (int64, error) {
@@ -152,35 +152,28 @@ func (s *ContentStorage) Exists(hash ContentHash) bool {
 	return err == nil
 }
 
-// writeIfAbsent writes data with writeFileSynced to the path derived
-// from hash, unless a file of the data's size is already there.
+// writeIfAbsent writes data to the path derived from hash, unless
+// content already exists there, via a temp-file-plus-rename so
+// concurrent readers never observe a partial file.
 func (s *ContentStorage) writeIfAbsent(hash ContentHash, data []byte) error {
 	// Build path: <basedir>/<ab>/<cd>/<hash>
 	path := s.hashToPath(hash)
 
-	// A file of another size there is damaged, and is replaced
-	info, statErr := os.Stat(path)
-	if statErr == nil && info.Size() == int64(len(data)) {
+	// Check if already exists
+	_, statErr := os.Stat(path)
+	if statErr == nil {
 		return nil
 	}
 
-	err := os.MkdirAll(filepath.Dir(path), StorageDirPerm)
+	// Create directory structure
+	dir := filepath.Dir(path)
+
+	err := os.MkdirAll(dir, StorageDirPerm)
 	if err != nil {
 		return fmt.Errorf("failed to create directory: %w", err)
 	}
 
-	return writeFileSynced(path, data)
-}
-
-// writeFileSynced writes data to path through a temp file in the same
-// directory: the temp file is synced to disk, renamed to path, and the
-// directory synced after, so that neither a concurrent reader nor a
-// crash finds a partial file under path. When the directory sync fails,
-// the file is removed from path, so that a failed write leaves no file
-// there.
-func writeFileSynced(path string, data []byte) error {
-	dir := filepath.Dir(path)
-
+	// Write to temp file first, then rename for atomicity
 	tmpFile, err := os.CreateTemp(dir, ".tmp-*")
 	if err != nil {
 		return fmt.Errorf("failed to create temp file: %w", err)
@@ -188,20 +181,18 @@ func writeFileSynced(path string, data []byte) error {
 
 	tmpPath := tmpFile.Name()
 
+	// Each failure path below unlinks the temp file explicitly. This
+	// replaces a deferred cleanup that read a named result, which the
+	// canonical config does not permit; the set of paths that remove
+	// tmpPath, and the order relative to Close, is unchanged. This
+	// mirrors how MetadataStorage.Store and VariantStorage.Store below
+	// already express the same cleanup.
 	_, err = tmpFile.Write(data)
 	if err != nil {
 		_ = tmpFile.Close()
 		_ = os.Remove(tmpPath)
 
-		return fmt.Errorf("failed to write temp file: %w", err)
-	}
-
-	err = tmpFile.Sync()
-	if err != nil {
-		_ = tmpFile.Close()
-		_ = os.Remove(tmpPath)
-
-		return fmt.Errorf("failed to sync temp file: %w", err)
+		return fmt.Errorf("failed to write content: %w", err)
 	}
 
 	err = tmpFile.Close()
@@ -211,6 +202,7 @@ func writeFileSynced(path string, data []byte) error {
 		return fmt.Errorf("failed to close temp file: %w", err)
 	}
 
+	// Atomic rename
 	err = os.Rename(filepath.Clean(tmpPath), filepath.Clean(path))
 	if err != nil {
 		_ = os.Remove(tmpPath)
@@ -218,31 +210,6 @@ func writeFileSynced(path string, data []byte) error {
 		return fmt.Errorf("failed to rename temp file: %w", err)
 	}
 
-	err = syncDirectory(dir)
-	if err != nil {
-		_ = os.Remove(path)
-
-		return err
-	}
-
-	return nil
-}
-
-// syncDirectory syncs the directory dir to disk, so that a file just
-// renamed into it is still there after a crash.
-func syncDirectory(dir string) error {
-	d, err := os.Open(filepath.Clean(dir))
-	if err != nil {
-		return fmt.Errorf("failed to open directory: %w", err)
-	}
-
-	err = d.Sync()
-	_ = d.Close()
-
-	if err != nil {
-		return fmt.Errorf("failed to sync directory: %w", err)
-	}
-
 	return nil
 }
 
@@ -460,9 +427,7 @@ func NewVariantStorage(baseDir string, log *slog.Logger) (*VariantStorage, error
 	return &VariantStorage{baseDir: baseDir, log: log}, nil
 }
 
-// Store writes content and metadata to storage at the given key, each
-// with writeFileSynced. The .meta file is written first, so that a crash
-// cannot leave the content in place without it.
+// Store writes content and metadata to storage at the given key.
 func (s *VariantStorage) Store(
 	key VariantKey, r io.Reader, contentType string,
 ) (int64, error) {
@@ -475,11 +440,46 @@ func (s *VariantStorage) Store(
 	path := s.keyToPath(key)
 	metaPath := path + ".meta"
 
-	err = os.MkdirAll(filepath.Dir(path), StorageDirPerm)
+	// Create directory structure
+	dir := filepath.Dir(path)
+
+	err = os.MkdirAll(dir, StorageDirPerm)
 	if err != nil {
 		return 0, fmt.Errorf("failed to create directory: %w", err)
 	}
 
+	// Write content to temp file first, then rename for atomicity
+	tmpFile, err := os.CreateTemp(dir, ".tmp-*")
+	if err != nil {
+		return 0, fmt.Errorf("failed to create temp file: %w", err)
+	}
+
+	tmpPath := tmpFile.Name()
+
+	_, err = tmpFile.Write(data)
+	if err != nil {
+		_ = tmpFile.Close()
+		_ = os.Remove(tmpPath)
+
+		return 0, fmt.Errorf("failed to write content: %w", err)
+	}
+
+	err = tmpFile.Close()
+	if err != nil {
+		_ = os.Remove(tmpPath)
+
+		return 0, fmt.Errorf("failed to close temp file: %w", err)
+	}
+
+	// Atomic rename content
+	err = os.Rename(filepath.Clean(tmpPath), filepath.Clean(path))
+	if err != nil {
+		_ = os.Remove(tmpPath)
+
+		return 0, fmt.Errorf("failed to rename temp file: %w", err)
+	}
+
+	// Write metadata
 	meta := VariantMeta{
 		ContentType: contentType,
 		Size:        size,
@@ -491,19 +491,13 @@ func (s *VariantStorage) Store(
 		return 0, fmt.Errorf("failed to marshal metadata: %w", err)
 	}
 
-	// Metadata write failure is non-fatal: the content is then served as
-	// application/octet-stream.
-	err = writeFileSynced(metaPath, metaData)
+	// Metadata write failure is non-fatal; content is already stored.
+	err = os.WriteFile(metaPath, metaData, StorageFilePerm)
 	if err != nil {
 		s.log.Warn("failed to write variant metadata sidecar",
 			"path", metaPath, "error", err)
 	}
 
-	err = writeFileSynced(path, data)
-	if err != nil {
-		return 0, err
-	}
-
 	return size, nil
 }
 

internal/imgcache/urlparser.go

@@ -15,7 +15,6 @@ import (
 var (
 	ErrInvalidPath       = errors.New("invalid image path")
 	ErrMissingHost       = errors.New("missing source host")
-	ErrInvalidHost       = errors.New("invalid source host")
 	ErrMissingSize       = errors.New("missing size specification")
 	ErrInvalidSize       = errors.New("invalid size format")
 	ErrInvalidFormat     = errors.New("invalid or unsupported format")
@@ -144,17 +143,6 @@ func parseImageComponents(remainder string) (*ParsedURL, error) {
 		return nil, ErrMissingHost
 	}
 
-	// A URL reads what comes before "@" as a user name and password, and the
-	// rest as the host
-	if strings.Contains(host, "@") {
-		return nil, fmt.Errorf("%w: %q holds @", ErrInvalidHost, host)
-	}
-
-	// Host names do not depend on letter case, so the allowlist, the upstream
-	// connection limits, the cache and the signature see one spelling of each
-	// host
-	host = strings.ToLower(host)
-
 	// Extract query string if present in path
 	if qIndex := strings.Index(path, "?"); qIndex != -1 {
 		query = path[qIndex+1:]

internal/imgcache/urlparser_internal_test.go

@@ -234,49 +234,6 @@ func TestParseImagePath(t *testing.T) {
 	}
 }
 
-// TestParseImagePath_LowerCasesHost verifies that the host is lower-cased, so
-// that every spelling of its letter case is the same host to the allowlist,
-// the upstream connection limits and the cache, and that the path keeps its
-// case.
-func TestParseImagePath_LowerCasesHost(t *testing.T) {
-	t.Parallel()
-
-	got, err := ParseImagePath("CDN.Example.COM/Photos/Cat.jpg/800x600.webp")
-	if err != nil {
-		t.Fatalf("ParseImagePath() error = %v", err)
-	}
-
-	t.Logf("parsed host %q, path %q", got.Host, got.Path)
-
-	if got.Host != testHostCDN {
-		t.Errorf("Host = %q, want %q", got.Host, testHostCDN)
-	}
-
-	if got.Path != "/Photos/Cat.jpg" {
-		t.Errorf("Path = %q, want %q", got.Path, "/Photos/Cat.jpg")
-	}
-}
-
-// TestParseImagePath_RefusesUserinfo verifies that a host holding "@" is
-// refused: a URL reads what comes before the "@" as a user name and password
-// and the rest as the host.
-func TestParseImagePath_RefusesUserinfo(t *testing.T) {
-	t.Parallel()
-
-	for _, input := range []string{
-		"user@cdn.example.com/photos/cat.jpg/800x600.webp",
-		"user:password@cdn.example.com/photos/cat.jpg/800x600.webp",
-	} {
-		got, err := ParseImagePath(input)
-		t.Logf("ParseImagePath(%q) = %+v, error = %v", input, got, err)
-
-		if !errors.Is(err, ErrInvalidHost) {
-			t.Errorf("ParseImagePath(%q) error = %v, want %v",
-				input, err, ErrInvalidHost)
-		}
-	}
-}
-
 func TestParsedURL_ToImageRequest(t *testing.T) {
 	t.Parallel()
 

internal/middleware/middleware.go

@@ -8,15 +8,12 @@ import (
 	"net/http"
 	"net/netip"
 	"regexp"
-	"strings"
 	"time"
 
 	basicauth "github.com/99designs/basicauth-go"
-	"github.com/go-chi/chi/v5"
 	"github.com/go-chi/chi/v5/middleware"
 	"github.com/go-chi/cors"
 	"github.com/go-chi/httprate"
-	"github.com/prometheus/client_golang/prometheus"
 	metrics "github.com/slok/go-http-metrics/metrics/prometheus"
 	ghmm "github.com/slok/go-http-metrics/middleware"
 	"github.com/slok/go-http-metrics/middleware/std"
@@ -66,10 +63,6 @@ type Middleware struct {
 	log      *slog.Logger
 	config   *config.Config
 	clientIP *clientip.Resolver
-
-	// metricsRegistry is the registry Metrics registers its metrics with.
-	// nil is Prometheus' default registry, the one /metrics serves.
-	metricsRegistry prometheus.Registerer
 }
 
 // New creates a new Middleware instance.
@@ -216,91 +209,14 @@ func (s *Middleware) CORS() func(http.Handler) http.Handler {
 	})
 }
 
-// Metrics returns a Prometheus metrics middleware. Its handler label is the
-// pattern of the chi route a request matches, such as /v1/image/*, or
-// "unmatched" for a request that matches none, so the routes fix how many
-// values the label has. It is never the path, with which any client could
-// add series to the registry without limit. Its method label is the
-// request's method when that is a standard one, such as GET, and "other"
-// for any other: Go's HTTP server accepts any method name, so a label per
-// method would let a client add series the same way.
+// Metrics returns a Prometheus metrics middleware.
 func (s *Middleware) Metrics() func(http.Handler) http.Handler {
 	mdlw := ghmm.New(ghmm.Config{
-		Recorder: metrics.NewRecorder(metrics.Config{
-			Registry: s.metricsRegistry,
-		}),
+		Recorder: metrics.NewRecorder(metrics.Config{}),
 	})
 
 	return func(next http.Handler) http.Handler {
-		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			// The metrics library takes the method label from the request
-			// it is handed, so it is handed a shallow copy of r whose
-			// method is the label. next is served r itself, through the
-			// response writer the library wraps to see the status and size.
-			labelled := r.WithContext(r.Context())
-			labelled.Method = methodLabel(r.Method)
-
-			serveNext := http.HandlerFunc(
-				func(wrapped http.ResponseWriter, _ *http.Request) {
-					next.ServeHTTP(wrapped, r)
-				})
-
-			std.Handler(routePattern(r), mdlw, serveNext).ServeHTTP(w, labelled)
-		})
-	}
-}
-
-// routePattern returns the pattern of the chi route r matches, or
-// "unmatched" when it matches none or is not served by a chi router. The
-// metrics library takes the label before the request is routed, so the
-// router is asked which route matches, with the path it routes on:
-// r.URL.RawPath when set, else r.URL.Path, else /.
-func routePattern(r *http.Request) string {
-	pattern := ""
-
-	rctx := chi.RouteContext(r.Context())
-	if rctx != nil {
-		path := r.URL.RawPath
-		if path == "" {
-			path = r.URL.Path
-		}
-
-		if path == "" {
-			path = "/"
-		}
-
-		pattern = rctx.Routes.Find(chi.NewRouteContext(), r.Method, path)
-
-		// For the path a subrouter is mounted at, such as /v1 or /v1/, Find
-		// answers that path without asking the subrouter, which chi hands
-		// the request as /.
-		for _, route := range rctx.Routes.Routes() {
-			if route.SubRoutes != nil &&
-				route.Pattern == strings.TrimSuffix(pattern, "/")+"/*" &&
-				!route.SubRoutes.Match(chi.NewRouteContext(), r.Method, "/") {
-				pattern = ""
-			}
-		}
-	}
-
-	if pattern == "" {
-		return "unmatched"
-	}
-
-	return pattern
-}
-
-// methodLabel returns method when it is one of the standard methods net/http
-// names: GET, HEAD, POST, PUT, PATCH, DELETE, OPTIONS, CONNECT and TRACE.
-// For any other method it returns "other".
-func methodLabel(method string) string {
-	switch method {
-	case http.MethodGet, http.MethodHead, http.MethodPost, http.MethodPut,
-		http.MethodPatch, http.MethodDelete, http.MethodOptions,
-		http.MethodConnect, http.MethodTrace:
-		return method
-	default:
-		return "other"
+		return std.Handler("", mdlw, next)
 	}
 }
 

internal/middleware/middleware_internal_test.go

@@ -3,16 +3,12 @@ package middleware
 import (
 	"bytes"
 	"log/slog"
-	"maps"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
-	"slices"
 	"strings"
 	"testing"
 
-	"github.com/go-chi/chi/v5"
-	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/promhttp"
 
 	"sneak.berlin/go/pixa/internal/config"
@@ -202,7 +198,7 @@ func TestMetricsRecordsServedRequest(t *testing.T) {
 
 	// The line /metrics shows once one GET /test has been served.
 	const want = `http_request_duration_seconds_count{` +
-		`code="200",handler="unmatched",method="GET",service=""} 1`
+		`code="200",handler="/test",method="GET",service=""} 1`
 
 	mw := &Middleware{log: slog.Default(), config: &config.Config{}}
 
@@ -224,149 +220,6 @@ func TestMetricsRecordsServedRequest(t *testing.T) {
 	}
 }
 
-// TestMetricsLabelsRequestsByRoute checks that the metrics middleware labels
-// each request with the pattern of the route that answers it, never with its
-// path: two paths one route answers share that route's pattern, and requests
-// no route answers, a 404 or a 405, share "unmatched", among them requests for
-// the path the subrouter is mounted at, /v1 and /v1/. A request with an empty
-// path is answered by the / route, as chi routes it as /, and is labelled /.
-// With a label per path, any client could add series to the registry without
-// limit. The middleware registers with a registry of its own here, as another
-// test in this package registers with Prometheus' default registry, which can
-// be done only once.
-func TestMetricsLabelsRequestsByRoute(t *testing.T) {
-	t.Parallel()
-
-	registry := prometheus.NewRegistry()
-	mw := &Middleware{
-		log:             slog.Default(),
-		config:          &config.Config{},
-		metricsRegistry: registry,
-	}
-
-	// The image route is in a subrouter, as in pixa's router.
-	router := chi.NewRouter()
-	router.Use(mw.Metrics())
-	router.Get("/", func(w http.ResponseWriter, _ *http.Request) {
-		w.WriteHeader(http.StatusOK)
-	})
-	router.Route("/v1", func(r chi.Router) {
-		r.Get("/image/*", func(w http.ResponseWriter, _ *http.Request) {
-			w.WriteHeader(http.StatusOK)
-		})
-	})
-
-	for _, req := range []struct{ method, path string }{
-		{http.MethodGet, "/v1/image/a.example.com/one.jpg/1x1.png"},
-		{http.MethodGet, "/v1/image/b.example.com/two.jpg/1x1.png"},
-		{http.MethodGet, "/no-such-route"},
-		{http.MethodGet, "/v1/no-such-route"},
-		{http.MethodGet, "/v1"},
-		{http.MethodGet, "/v1/"},
-		{http.MethodPost, "/v1/image/a.example.com/one.jpg/1x1.png"},
-		// The request line GET http://pixa.example HTTP/1.1 leaves both
-		// r.URL.Path and r.URL.RawPath empty.
-		{http.MethodGet, "http://pixa.example"},
-	} {
-		r := httptest.NewRequestWithContext(
-			t.Context(), req.method, req.path, nil)
-		t.Logf("%s %s: r.URL.Path %q, r.URL.RawPath %q",
-			req.method, req.path, r.URL.Path, r.URL.RawPath)
-		router.ServeHTTP(httptest.NewRecorder(), r)
-	}
-
-	got := requestCountsByHandler(t, registry)
-	t.Logf("requests counted by handler label: %v", got)
-
-	want := map[string]uint64{"/": 1, "/v1/image/*": 2, "unmatched": 5}
-	if !maps.Equal(got, want) {
-		t.Errorf("requests counted by handler label = %v, want %v", got, want)
-	}
-}
-
-// requestCountsByHandler returns how many requests the request duration
-// histogram in registry counted under each value of its handler label.
-func requestCountsByHandler(
-	t *testing.T, registry *prometheus.Registry,
-) map[string]uint64 {
-	t.Helper()
-
-	families, err := registry.Gather()
-	if err != nil {
-		t.Fatalf("registry.Gather() error = %v", err)
-	}
-
-	counts := map[string]uint64{}
-
-	for _, family := range families {
-		if family.GetName() != "http_request_duration_seconds" {
-			continue
-		}
-
-		for _, metric := range family.GetMetric() {
-			for _, label := range metric.GetLabel() {
-				if label.GetName() == "handler" {
-					counts[label.GetValue()] += metric.GetHistogram().GetSampleCount()
-				}
-			}
-		}
-	}
-
-	return counts
-}
-
-// TestMetricsGivesMadeUpMethodsOneLabel checks that the metrics middleware
-// gives requests with methods that are not standard ones a single method
-// label, "other", and still serves each request with the method it was sent
-// with. With a label per method, any client could add series to the
-// registry without limit by making up a new method for each request.
-func TestMetricsGivesMadeUpMethodsOneLabel(t *testing.T) {
-	t.Parallel()
-
-	// The line /metrics shows once both made-up methods have been served.
-	const want = `http_request_duration_seconds_count{` +
-		`code="200",handler="unmatched",method="other",service=""} 2`
-
-	registry := prometheus.NewRegistry()
-	mw := &Middleware{
-		log:             slog.Default(),
-		config:          &config.Config{},
-		metricsRegistry: registry,
-	}
-
-	var served []string
-
-	handler := mw.Metrics()(http.HandlerFunc(
-		func(w http.ResponseWriter, r *http.Request) {
-			served = append(served, r.Method)
-
-			w.WriteHeader(http.StatusOK)
-		}))
-
-	methods := []string{"FROBNICATE", "WIBBLE"}
-	for _, method := range methods {
-		handler.ServeHTTP(httptest.NewRecorder(), httptest.NewRequestWithContext(
-			t.Context(), method, "/", nil))
-	}
-
-	t.Logf("methods the handler was served: %v", served)
-
-	if !slices.Equal(served, methods) {
-		t.Errorf("methods the handler was served = %v, want %v",
-			served, methods)
-	}
-
-	rec := httptest.NewRecorder()
-	promhttp.HandlerFor(registry, promhttp.HandlerOpts{}).ServeHTTP(rec,
-		httptest.NewRequestWithContext(
-			t.Context(), http.MethodGet, "/metrics", nil))
-
-	if !strings.Contains(rec.Body.String(), want) {
-		t.Errorf("/metrics does not count both made-up methods as one; "+
-			"want the line %q in:\n%s", want, rec.Body.String())
-	}
-}
-
 // TestLoggingLeavesOutSubmittedSigningKey checks that a login, a POST /
 // whose form carries the signing key, leaves no trace of the key in the
 // request's log line.

internal/server/logout_internal_test.go

@@ -1,269 +0,0 @@
-package server
-
-import (
-	"net/http"
-	"net/http/httptest"
-	"net/url"
-	"path/filepath"
-	"regexp"
-	"strings"
-	"testing"
-
-	"go.uber.org/fx"
-	"go.uber.org/fx/fxtest"
-
-	"sneak.berlin/go/pixa/internal/config"
-	"sneak.berlin/go/pixa/internal/database"
-	"sneak.berlin/go/pixa/internal/globals"
-	"sneak.berlin/go/pixa/internal/handlers"
-	"sneak.berlin/go/pixa/internal/healthcheck"
-	"sneak.berlin/go/pixa/internal/logger"
-	"sneak.berlin/go/pixa/internal/middleware"
-	"sneak.berlin/go/pixa/internal/session"
-)
-
-// logoutFormPattern extracts the CSRF token rendered into the generator
-// page's logout form.
-var logoutFormPattern = regexp.MustCompile(
-	`(?s)action="/logout".*?name="gorilla\.csrf\.Token" value="([^"]+)"`)
-
-// startPixa starts the components pixad's fx app builds, from cfg, and
-// returns the server with its routes, as newTestServer does.
-func startPixa(t *testing.T, cfg *config.Config) *Server {
-	t.Helper()
-
-	var (
-		h  *handlers.Handlers
-		mw *middleware.Middleware
-	)
-
-	app := fxtest.New(t,
-		fx.Supply(cfg),
-		fx.Provide(
-			globals.New,
-			logger.New,
-			database.New,
-			healthcheck.New,
-			handlers.New,
-			middleware.New,
-		),
-		fx.Populate(&h, &mw),
-	)
-	app.RequireStart()
-	t.Cleanup(app.RequireStop)
-
-	s := &Server{config: cfg, mw: mw, h: h}
-	s.SetupRoutes()
-
-	return s
-}
-
-// logIn logs in to s with the signing key, as postLogin does, and returns
-// the session cookie it sets.
-func logIn(t *testing.T, s *Server) *http.Cookie {
-	t.Helper()
-
-	rec := postLogin(t, s, firstClient, "", testSigningKey)
-
-	for _, c := range rec.Result().Cookies() {
-		if c.Name == session.CookieName {
-			return c
-		}
-	}
-
-	t.Fatal("logging in set no session cookie")
-
-	return nil
-}
-
-// opensGeneratorPage reports whether GET / with cookie shows s's generator
-// page, which only a valid login session opens.
-func opensGeneratorPage(t *testing.T, s *Server, cookie *http.Cookie) bool {
-	t.Helper()
-
-	req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", nil)
-	req.AddCookie(cookie)
-
-	rec := httptest.NewRecorder()
-	s.ServeHTTP(rec, req)
-
-	return rec.Code == http.StatusOK &&
-		strings.Contains(rec.Body.String(), `action="/generate"`)
-}
-
-// postLogout loads the generator page with GET / and sessionCookie and
-// submits its logout form with POST /logout, as a browser does.
-func postLogout(
-	t *testing.T, s *Server, sessionCookie *http.Cookie,
-) *httptest.ResponseRecorder {
-	t.Helper()
-
-	req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", nil)
-	req.AddCookie(sessionCookie)
-
-	page := httptest.NewRecorder()
-	s.ServeHTTP(page, req)
-
-	match := logoutFormPattern.FindStringSubmatch(page.Body.String())
-	if match == nil {
-		t.Fatal("the generator page has no logout form with a CSRF token")
-	}
-
-	form := url.Values{"gorilla.csrf.Token": {match[1]}}
-	req = httptest.NewRequestWithContext(t.Context(), http.MethodPost,
-		"/logout", strings.NewReader(form.Encode()))
-	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
-	req.AddCookie(sessionCookie)
-
-	for _, c := range page.Result().Cookies() {
-		req.AddCookie(c)
-	}
-
-	rec := httptest.NewRecorder()
-	s.ServeHTTP(rec, req)
-
-	return rec
-}
-
-// TestLogoutWithoutCSRFTokenEndsNoSession verifies that a request to /logout
-// carrying the session cookie but not the logout form's CSRF token ends no
-// session: GET /logout, as an image on another page of the same site makes a
-// browser send, answers 405, and POST /logout without the token answers 403.
-func TestLogoutWithoutCSRFTokenEndsNoSession(t *testing.T) {
-	t.Parallel()
-
-	tests := []struct {
-		method string
-		want   int
-	}{
-		{http.MethodGet, http.StatusMethodNotAllowed},
-		{http.MethodPost, http.StatusForbidden},
-	}
-
-	for _, tc := range tests {
-		t.Run(tc.method, func(t *testing.T) {
-			t.Parallel()
-
-			s := newTestServer(t)
-			cookie := logIn(t, s)
-
-			req := httptest.NewRequestWithContext(
-				t.Context(), tc.method, "/logout", nil)
-			req.AddCookie(cookie)
-
-			rec := httptest.NewRecorder()
-			s.ServeHTTP(rec, req)
-			t.Logf("%s /logout without the CSRF token: status %d",
-				tc.method, rec.Code)
-
-			if rec.Code != tc.want {
-				t.Errorf("%s /logout without the CSRF token status = %d, "+
-					"want %d", tc.method, rec.Code, tc.want)
-			}
-
-			if !opensGeneratorPage(t, s, cookie) {
-				t.Errorf("%s /logout without the CSRF token ended the "+
-					"login session", tc.method)
-			}
-		})
-	}
-}
-
-// TestLoginAfterLogoutOpensGeneratorPage verifies that after a logout from
-// the generator page a copy of the session cookie taken before it is
-// refused, and that a login after it opens the generator page.
-func TestLoginAfterLogoutOpensGeneratorPage(t *testing.T) {
-	t.Parallel()
-
-	s := newTestServer(t)
-
-	copied := logIn(t, s)
-	if !opensGeneratorPage(t, s, copied) {
-		t.Fatal("logging in did not open the generator page")
-	}
-
-	logout := postLogout(t, s, copied)
-	if logout.Code != http.StatusSeeOther {
-		t.Fatalf("POST /logout status = %d, want %d",
-			logout.Code, http.StatusSeeOther)
-	}
-
-	if opensGeneratorPage(t, s, copied) {
-		t.Error("a cookie copied before the logout still opens the " +
-			"generator page")
-	}
-
-	opens := opensGeneratorPage(t, s, logIn(t, s))
-	t.Logf("a login after the logout opens the generator page: %v", opens)
-
-	if !opens {
-		t.Error("a login after the logout does not open the generator page")
-	}
-}
-
-// TestLogoutOutlastsRestart verifies that a copy of the session cookie taken
-// before a logout is still refused by a pixa started again on the same state
-// directory, as pixad is after a restart.
-func TestLogoutOutlastsRestart(t *testing.T) {
-	t.Parallel()
-
-	s := newTestServer(t)
-
-	copied := logIn(t, s)
-	if !opensGeneratorPage(t, s, copied) {
-		t.Fatal("logging in did not open the generator page")
-	}
-
-	logout := postLogout(t, s, copied)
-	if logout.Code != http.StatusSeeOther {
-		t.Fatalf("POST /logout status = %d, want %d",
-			logout.Code, http.StatusSeeOther)
-	}
-
-	restarted := startPixa(t, s.config)
-
-	opens := opensGeneratorPage(t, restarted, copied)
-	t.Logf("after the restart, the copied cookie opens the generator page: %v",
-		opens)
-
-	if opens {
-		t.Error("after a restart, a cookie copied before the logout " +
-			"still opens the generator page")
-	}
-}
-
-// TestLostStateDirectoryEndsEveryLoginSession verifies that a pixa started on
-// an empty state directory, as pixad is after a restart that lost its state
-// directory, refuses a session cookie from before, and that a login on it
-// opens the generator page.
-func TestLostStateDirectoryEndsEveryLoginSession(t *testing.T) {
-	t.Parallel()
-
-	s := newTestServer(t)
-
-	before := logIn(t, s)
-	if !opensGeneratorPage(t, s, before) {
-		t.Fatal("logging in did not open the generator page")
-	}
-
-	emptyStateDir := *s.config
-	emptyStateDir.StateDir = t.TempDir()
-	emptyStateDir.DBURL = "file:" +
-		filepath.Join(emptyStateDir.StateDir, "state.sqlite3")
-
-	restarted := startPixa(t, &emptyStateDir)
-
-	opens := opensGeneratorPage(t, restarted, before)
-	t.Logf("on an empty state directory, the cookie from before opens the "+
-		"generator page: %v", opens)
-
-	if opens {
-		t.Error("on an empty state directory, a cookie from before still " +
-			"opens the generator page")
-	}
-
-	if !opensGeneratorPage(t, restarted, logIn(t, restarted)) {
-		t.Error("a login on an empty state directory does not open the " +
-			"generator page")
-	}
-}

internal/server/routes.go

@@ -69,9 +69,10 @@ func (s *Server) SetupRoutes() {
 		r.With(s.mw.RateLimit(LoginAttemptsPerMinute, time.Minute, nil)).
 			Post("/", s.h.HandleRoot())
 		r.Post("/generate", s.h.HandleGenerateURL())
-		r.Post("/logout", s.h.HandleLogout())
 	})
 
+	s.router.Get("/logout", s.h.HandleLogout())
+
 	// Image routes, the only ones that send CORS headers, as pages on other
 	// sites read them. They are a subrouter rather than a group: a group's
 	// middleware runs only for a request that matches one of its routes,

internal/session/session.go

@@ -1,16 +1,9 @@
-// Package session provides encrypted session cookie management, and keeps in
-// the database a random value that each logout replaces, so that a logout
-// ends every session made before it.
+// Package session provides encrypted session cookie management.
 package session
 
 import (
-	"context"
-	"crypto/rand"
-	"database/sql"
 	"errors"
-	"fmt"
 	"net/http"
-	"sync"
 	"time"
 
 	"github.com/gorilla/securecookie"
@@ -39,36 +32,14 @@ type Data struct {
 	Authenticated bool      `json:"auth"`
 	CreatedAt     time.Time `json:"created"`
 	ExpiresAt     time.Time `json:"expires"`
-
-	// LogoutValue is the Manager's random value, which each logout
-	// replaces, when the session was made.
-	LogoutValue string `json:"logoutValue"`
 }
 
 // Manager handles session creation and validation using encrypted cookies.
-//
-// Every login is made with the one signing key, so a logout ends every
-// session made before it, in every browser. The Manager keeps a random
-// value, which each logout replaces: a session carries the value current at
-// its login, and a session carrying another value is refused. The check for
-// a logout compares no time, so a login after a logout works whatever the
-// host's clock does; the session's expiry is still checked against the
-// clock. The Manager keeps the value in memory, so checking a session reads
-// no database; a Manager from NewManagerWithDatabase also saves it in the
-// database, so that it outlasts a restart.
 type Manager struct {
 	sc *securecookie.SecureCookie
-	db *sql.DB
-
-	// mu guards logoutValue, and is held while a logout is saved, so that
-	// the database and logoutValue agree.
-	mu          sync.Mutex
-	logoutValue string
 }
 
 // NewManager creates a session manager with keys derived from the signing key.
-// It makes a new random value and keeps it in memory only, so a restart ends
-// every session; NewManagerWithDatabase keeps the value in the database.
 //
 // Session cookies always carry the Secure, HttpOnly, and SameSite=Strict
 // attributes; this cannot be configured. Browsers treat http://localhost as a
@@ -92,44 +63,10 @@ func NewManager(signingKey string) (*Manager, error) {
 	sc.MaxAge(int(SessionTTL.Seconds()))
 
 	return &Manager{
-		sc:          sc,
-		logoutValue: rand.Text(),
+		sc: sc,
 	}, nil
 }
 
-// NewManagerWithDatabase creates a session manager like NewManager that keeps
-// its random value in db, which must have pixa's schema: it saves the value
-// NewManager made when db has none, and otherwise starts from the one saved
-// there.
-func NewManagerWithDatabase(
-	ctx context.Context, signingKey string, db *sql.DB,
-) (*Manager, error) {
-	m, err := NewManager(signingKey)
-	if err != nil {
-		return nil, err
-	}
-
-	_, err = db.ExecContext(ctx,
-		"INSERT OR IGNORE INTO logout (id, logout_value) VALUES (1, ?)",
-		m.logoutValue)
-	if err != nil {
-		return nil, fmt.Errorf(
-			"failed to save the random value in the logout table: %w", err)
-	}
-
-	err = db.QueryRowContext(ctx,
-		"SELECT logout_value FROM logout WHERE id = 1",
-	).Scan(&m.logoutValue)
-	if err != nil {
-		return nil, fmt.Errorf(
-			"failed to read the random value in the logout table: %w", err)
-	}
-
-	m.db = db
-
-	return m, nil
-}
-
 // CreateSession creates a new authenticated session and sets the cookie.
 func (m *Manager) CreateSession(w http.ResponseWriter) error {
 	now := time.Now()
@@ -137,7 +74,6 @@ func (m *Manager) CreateSession(w http.ResponseWriter) error {
 		Authenticated: true,
 		CreatedAt:     now,
 		ExpiresAt:     now.Add(SessionTTL),
-		LogoutValue:   m.currentLogoutValue(),
 	}
 
 	encoded, err := m.sc.Encode(CookieName, data)
@@ -186,30 +122,9 @@ func (m *Manager) ValidateSession(r *http.Request) (*Data, error) {
 		return nil, ErrInvalidSession
 	}
 
-	// A logout since the session was made has ended it.
-	if data.LogoutValue != m.currentLogoutValue() {
-		return nil, ErrInvalidSession
-	}
-
 	return &data, nil
 }
 
-// Logout removes the session cookie and, when r has a valid session, ends
-// every session made until now. A request without one ends no session, so
-// someone who has not logged in cannot end the sessions of those who have.
-func (m *Manager) Logout(w http.ResponseWriter, r *http.Request) error {
-	if m.IsAuthenticated(r) {
-		err := m.saveLogout(r.Context())
-		if err != nil {
-			return err
-		}
-	}
-
-	m.ClearSession(w)
-
-	return nil
-}
-
 // ClearSession removes the session cookie.
 func (m *Manager) ClearSession(w http.ResponseWriter) {
 	http.SetCookie(w, &http.Cookie{
@@ -230,32 +145,3 @@ func (m *Manager) IsAuthenticated(r *http.Request) bool {
 
 	return err == nil && data != nil && data.Authenticated
 }
-
-// saveLogout replaces the random value with a new one, saving it in the
-// database first when the Manager has one.
-func (m *Manager) saveLogout(ctx context.Context) error {
-	value := rand.Text()
-
-	m.mu.Lock()
-	defer m.mu.Unlock()
-
-	if m.db != nil {
-		_, err := m.db.ExecContext(ctx,
-			"UPDATE logout SET logout_value = ? WHERE id = 1", value)
-		if err != nil {
-			return fmt.Errorf("failed to save the logout: %w", err)
-		}
-	}
-
-	m.logoutValue = value
-
-	return nil
-}
-
-// currentLogoutValue returns the Manager's random value.
-func (m *Manager) currentLogoutValue() string {
-	m.mu.Lock()
-	defer m.mu.Unlock()
-
-	return m.logoutValue
-}

internal/session/session_logout_test.go

@@ -1,148 +0,0 @@
-package session_test
-
-import (
-	"database/sql"
-	"errors"
-	"net/http"
-	"net/http/httptest"
-	"testing"
-
-	"sneak.berlin/go/pixa/internal/database"
-	"sneak.berlin/go/pixa/internal/session"
-)
-
-const logoutTestSigningKey = "test-signing-key-12345"
-
-// newTestDatabase opens an empty in-memory database with pixa's schema. It
-// has one connection, as pixad's has: each connection to ":memory:" would
-// open a database of its own.
-func newTestDatabase(t *testing.T) *sql.DB {
-	t.Helper()
-
-	db, err := sql.Open("sqlite", ":memory:")
-	if err != nil {
-		t.Fatalf("sql.Open() error = %v", err)
-	}
-
-	db.SetMaxOpenConns(1)
-	t.Cleanup(func() { _ = db.Close() })
-
-	err = database.ApplyMigrations(t.Context(), db, nil)
-	if err != nil {
-		t.Fatalf("ApplyMigrations() error = %v", err)
-	}
-
-	return db
-}
-
-// createSessionCookie logs in with mgr and returns the session cookie it
-// sets.
-func createSessionCookie(t *testing.T, mgr *session.Manager) *http.Cookie {
-	t.Helper()
-
-	rec := httptest.NewRecorder()
-
-	err := mgr.CreateSession(rec)
-	if err != nil {
-		t.Fatalf("CreateSession() error = %v", err)
-	}
-
-	for _, c := range rec.Result().Cookies() {
-		if c.Name == session.CookieName {
-			return c
-		}
-	}
-
-	t.Fatalf("CreateSession() did not set a %q cookie", session.CookieName)
-
-	return nil
-}
-
-// requestWithCookie returns a GET request carrying cookie, or no cookie when
-// cookie is nil.
-func requestWithCookie(t *testing.T, cookie *http.Cookie) *http.Request {
-	t.Helper()
-
-	req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", nil)
-	if cookie != nil {
-		req.AddCookie(cookie)
-	}
-
-	return req
-}
-
-// TestLogout_EndsEverySessionMadeBeforeIt verifies that after a logout a
-// copy of the session cookie taken before it is refused and a login after it
-// works, and that both still hold for a new Manager on the same database, as
-// after a restart.
-func TestLogout_EndsEverySessionMadeBeforeIt(t *testing.T) {
-	t.Parallel()
-
-	db := newTestDatabase(t)
-
-	mgr, err := session.NewManagerWithDatabase(
-		t.Context(), logoutTestSigningKey, db)
-	if err != nil {
-		t.Fatalf("NewManagerWithDatabase() error = %v", err)
-	}
-
-	copied := createSessionCookie(t, mgr)
-
-	err = mgr.Logout(httptest.NewRecorder(), requestWithCookie(t, copied))
-	if err != nil {
-		t.Fatalf("Logout() error = %v", err)
-	}
-
-	_, err = mgr.ValidateSession(requestWithCookie(t, copied))
-	t.Logf("cookie copied before the logout: ValidateSession() error = %v", err)
-
-	if !errors.Is(err, session.ErrInvalidSession) {
-		t.Errorf("cookie copied before the logout: error = %v, want %v",
-			err, session.ErrInvalidSession)
-	}
-
-	loginAfter := createSessionCookie(t, mgr)
-
-	_, err = mgr.ValidateSession(requestWithCookie(t, loginAfter))
-	if err != nil {
-		t.Errorf("login after the logout: ValidateSession() error = %v", err)
-	}
-
-	restarted, err := session.NewManagerWithDatabase(
-		t.Context(), logoutTestSigningKey, db)
-	if err != nil {
-		t.Fatalf("NewManagerWithDatabase() after restart error = %v", err)
-	}
-
-	if restarted.IsAuthenticated(requestWithCookie(t, copied)) {
-		t.Error("after a restart, a cookie copied before the logout is accepted")
-	}
-
-	if !restarted.IsAuthenticated(requestWithCookie(t, loginAfter)) {
-		t.Error("after a restart, a login made after the logout is refused")
-	}
-}
-
-// TestLogout_WithoutSession_EndsNoSession verifies that a logout from a
-// request without a session cookie ends no session, so someone who has not
-// logged in cannot end the sessions of those who have.
-func TestLogout_WithoutSession_EndsNoSession(t *testing.T) {
-	t.Parallel()
-
-	mgr, err := session.NewManagerWithDatabase(
-		t.Context(), logoutTestSigningKey, newTestDatabase(t))
-	if err != nil {
-		t.Fatalf("NewManagerWithDatabase() error = %v", err)
-	}
-
-	cookie := createSessionCookie(t, mgr)
-
-	err = mgr.Logout(httptest.NewRecorder(), requestWithCookie(t, nil))
-	if err != nil {
-		t.Fatalf("Logout() error = %v", err)
-	}
-
-	if !mgr.IsAuthenticated(requestWithCookie(t, cookie)) {
-		t.Error("a logout without a session cookie ended a session")
-	}
-}

internal/static/style.css

@@ -122,20 +122,12 @@ header {
     margin-bottom: 2rem;
 }
 
-/* The logout button, drawn as a link. */
-
-header button {
-    width: auto;
-    margin: 0;
-    padding: 0;
-    background: none;
+header a {
     font-size: 0.875rem;
     color: #4b5563;
-    text-decoration: underline;
 }
 
-header button:hover {
-    background: none;
+header a:hover {
     color: #1f2937;
 }
 

internal/templates/generator.html

@@ -10,12 +10,9 @@
     <div class="page">
         <header>
             <h1>Pixa URL Generator</h1>
-            <form method="POST" action="/logout">
-                {{ .CSRFField }}
-                <button type="submit">
-                    Logout
-                </button>
-            </form>
+            <a href="/logout">
+                Logout
+            </a>
         </header>
 
         {{if .GeneratedURL}}

script/bootstrap

@@ -3,13 +3,11 @@
 # this repo. Idempotent: every install is guarded by a check so already
 # installed tools are skipped. Base tooling comes from nix, apt, brew,
 # or apk (detected in that order); assumes NOTHING is present (not git,
-# make, or go). The installed node is used only when its major version
-# is the pinned one; otherwise (node missing, or another major) the
-# pinned version is installed via nvm (installing nvm itself first,
-# from a hash-verified release archive, never curl | sh), and yarn and
-# the packages are installed under it. The linter is never installed on
-# the host: golangci-lint runs only in the lint phase of the Dockerfile
-# (see script/lint).
+# make, or go). Node is used directly if installed; otherwise it is
+# installed at a pinned version via nvm (installing nvm itself first,
+# from a hash-verified release archive, never curl | sh). The linter is
+# never installed on the host: golangci-lint runs only in the lint phase
+# of the Dockerfile (see script/lint).
 #
 #   script/bootstrap         git, make, Go, and Node, Yarn and the
 #                            prettier in yarn.lock for script/fmt and
@@ -112,50 +110,31 @@ ensure_nvm() {
     rm -rf "$tmp"
 }
 
-# True when the node on PATH has the pinned major version. Only the
-# major is compared, as REPO_POLICIES.md asks. Another major is not
-# used: the pinned yarn 1 prints a deprecation warning under node 24.
-# script/fmt and script/fmt-check carry this function unchanged, to run
-# the yarn installed here.
-node_is_pinned_major() {
-    if ! command -v node >/dev/null 2>&1; then return 1; fi
-    installed="$(node --version)"
-    installed="${installed#v}"
-    [ "${installed%%.*}" = "${NODE_VERSION%%.*}" ]
-}
-
 ensure_node() {
-    if node_is_pinned_major; then
-        echo "bootstrap: using node $(node --version)"
-        return 0
-    fi
+    if ! missing node; then return 0; fi
     ensure_nvm
     nvm_sh "nvm install $NODE_VERSION"
-    nvm_sh "nvm use $NODE_VERSION >/dev/null && \
-        echo \"bootstrap: using node \$(node --version)\""
 }
 
 ensure_yarn() {
-    if ! node_is_pinned_major; then
-        nvm_sh "nvm use $NODE_VERSION >/dev/null && corepack enable && \
-            corepack prepare yarn@$YARN_VERSION --activate"
-        return 0
-    fi
     if ! missing yarn; then return 0; fi
     if ! missing corepack; then
         corepack enable
         corepack prepare "yarn@$YARN_VERSION" --activate
+    elif [ -s "$HOME/.nvm/nvm.sh" ]; then
+        nvm_sh "nvm use $NODE_VERSION >/dev/null && corepack enable && \
+            corepack prepare yarn@$YARN_VERSION --activate"
     else
         npm install -g "yarn@$YARN_VERSION"
     fi
 }
 
 install_js_deps() {
-    if node_is_pinned_major; then
-        yarn install --frozen-lockfile
-    else
+    if missing yarn && [ -s "$HOME/.nvm/nvm.sh" ]; then
         nvm_sh "nvm use $NODE_VERSION >/dev/null && cd \"$ROOT\" && \
             yarn install --frozen-lockfile"
+    else
+        yarn install --frozen-lockfile
     fi
 }
 

script/cibuild

@@ -2,11 +2,9 @@
 # script/cibuild: run the CI build. It bootstraps first: a CI runner
 # checks out and runs this and nothing else, and script/fmt-check runs
 # the formatter on the host, which a pristine checkout cannot do.
-# A new CACHEBUST value reruns every RUN step, since a cached gate phase
-# is a check that did not run; unlike --no-cache, it keeps Go's build cache.
-# This relies on every stage of the Dockerfile declaring ARG CACHEBUST
-# first; a stage without that line is served from cache on an unchanged
-# tree, with no warning.
+# --no-cache for the same reason as script/docker: the gate phases the
+# final stage depends on are RUN steps, and a cached one is a check that
+# did not run.
 set -eu
 
 SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd -P)"
@@ -16,17 +14,16 @@ main() {
     cd "$ROOT"
     "$SCRIPT_DIR/bootstrap"
     "$SCRIPT_DIR/check"
-    # The version and the tag each get their own line: a failing
-    # command substitution inside an argument does not trip `set -e`,
-    # so the inline form degrades silently to an empty constant. The
-    # VERSION build argument takes precedence over the version a build
-    # stage derives from the .git in the context.
+    # Own line: a failing command substitution inside an argument does
+    # not trip `set -e`, so the inline form degrades silently to an
+    # empty constant. VERSION is computed here because .dockerignore
+    # excludes .git, so `git describe` in a build stage yields an empty
+    # version without failing.
     version="$(git describe --tags --always --dirty 2>/dev/null || true)"
     [ -n "$version" ] || version="unknown"
-    tag="$("$SCRIPT_DIR/projectname")"
-    docker build --build-arg CACHEBUST="$(date +%s)-$$" \
+    docker build --no-cache \
         --build-arg VERSION="$version" \
-        -t "$tag" .
+        -t "$("$SCRIPT_DIR/projectname")" .
 }
 
 main "$@"

script/docker

@@ -1,11 +1,8 @@
 #!/bin/sh
 # script/docker: build the Docker image tagged with the project name.
 # Identical in all repos; the tag comes from script/projectname.
-# A new CACHEBUST value reruns every RUN step, since a cached gate phase
-# is a check that did not run; unlike --no-cache, it keeps Go's build cache.
-# This relies on every stage of the Dockerfile declaring ARG CACHEBUST
-# first; a stage without that line is served from cache on an unchanged
-# tree, with no warning.
+# --no-cache because the gate phases the final stage depends on are RUN
+# steps, and a cached one is a check that did not run.
 set -eu
 
 SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd -P)"
@@ -13,17 +10,16 @@ ROOT="$(cd "$SCRIPT_DIR/.." && pwd -P)"
 
 main() {
     cd "$ROOT"
-    # The version and the tag each get their own line: a failing
-    # command substitution inside an argument does not trip `set -e`,
-    # so the inline form degrades silently to an empty constant. The
-    # VERSION build argument takes precedence over the version a build
-    # stage derives from the .git in the context.
+    # Own line: a failing command substitution inside an argument does
+    # not trip `set -e`, so the inline form degrades silently to an
+    # empty constant. VERSION is computed here because .dockerignore
+    # excludes .git, so `git describe` in a build stage yields an empty
+    # version without failing.
     version="$(git describe --tags --always --dirty 2>/dev/null || true)"
     [ -n "$version" ] || version="unknown"
-    tag="$("$SCRIPT_DIR/projectname")"
-    docker build --build-arg CACHEBUST="$(date +%s)-$$" \
+    docker build --no-cache \
         --build-arg VERSION="$version" \
-        -t "$tag" .
+        -t "$("$SCRIPT_DIR/projectname")" .
 }
 
 main "$@"

script/fmt

@@ -7,25 +7,15 @@ ROOT="$(cd "$(dirname "$0")/.." && pwd -P)"
 # Must match the pin in script/bootstrap.
 NODE_VERSION="22.17.0"
 
-# True when the node on PATH has the pinned major version: the same
-# function as in script/bootstrap, which uses it to decide where it
-# installs yarn.
-node_is_pinned_major() {
-    if ! command -v node >/dev/null 2>&1; then return 1; fi
-    installed="$(node --version)"
-    installed="${installed#v}"
-    [ "${installed%%.*}" = "${NODE_VERSION%%.*}" ]
-}
-
-# Run the yarn script/bootstrap installed: the yarn on PATH when the
-# node on PATH has the pinned major version, and otherwise yarn under
-# the pinned node in nvm, which bootstrap leaves off the PATH of the
-# shell that called it. nvm is a bash script, hence the subshell.
+# script/bootstrap installs node and yarn under nvm and leaves neither
+# on the PATH of the shell that called it, so resolve the pinned
+# toolchain here the way bootstrap's own install step does. nvm is a
+# bash script, hence the subshell.
 run_yarn() {
-    if node_is_pinned_major && command -v yarn >/dev/null 2>&1; then
+    if command -v yarn >/dev/null 2>&1; then
         exec yarn "$@"
     fi
-    if node_is_pinned_major || [ ! -s "$HOME/.nvm/nvm.sh" ]; then
+    if [ ! -s "$HOME/.nvm/nvm.sh" ]; then
         echo "fmt: no yarn; run script/bootstrap first" >&2
         exit 1
     fi

script/fmt-check

@@ -8,25 +8,15 @@ ROOT="$(cd "$(dirname "$0")/.." && pwd -P)"
 # Must match the pin in script/bootstrap.
 NODE_VERSION="22.17.0"
 
-# True when the node on PATH has the pinned major version: the same
-# function as in script/bootstrap, which uses it to decide where it
-# installs yarn.
-node_is_pinned_major() {
-    if ! command -v node >/dev/null 2>&1; then return 1; fi
-    installed="$(node --version)"
-    installed="${installed#v}"
-    [ "${installed%%.*}" = "${NODE_VERSION%%.*}" ]
-}
-
-# Run the yarn script/bootstrap installed: the yarn on PATH when the
-# node on PATH has the pinned major version, and otherwise yarn under
-# the pinned node in nvm, which bootstrap leaves off the PATH of the
-# shell that called it. nvm is a bash script, hence the subshell.
+# script/bootstrap installs node and yarn under nvm and leaves neither
+# on the PATH of the shell that called it, so resolve the pinned
+# toolchain here the way bootstrap's own install step does. nvm is a
+# bash script, hence the subshell.
 run_yarn() {
-    if node_is_pinned_major && command -v yarn >/dev/null 2>&1; then
+    if command -v yarn >/dev/null 2>&1; then
         exec yarn "$@"
     fi
-    if node_is_pinned_major || [ ! -s "$HOME/.nvm/nvm.sh" ]; then
+    if [ ! -s "$HOME/.nvm/nvm.sh" ]; then
         echo "fmt-check: no yarn; run script/bootstrap first" >&2
         exit 1
     fi

script/lint

@@ -5,12 +5,9 @@
 # make its answer untrustworthy.
 #
 # The phase is not the last stage in the file, so it is built only when
-# --target names it. A new CACHEBUST value reruns every RUN step, since
-# a cached lint layer is a lint that did not run; unlike --no-cache, it
-# keeps Go's build cache. This relies on every stage of the Dockerfile
-# declaring ARG CACHEBUST first; a stage without that line is served
-# from cache on an unchanged tree, with no warning.
-# --output type=cacheonly writes no image, since nothing uses one.
+# --target names it. --no-cache because a cached lint layer is a lint
+# that did not run. The tag makes each build replace the previous image
+# instead of leaving a dangling one behind.
 set -eu
 
 SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd -P)"
@@ -18,9 +15,9 @@ ROOT="$(cd "$SCRIPT_DIR/.." && pwd -P)"
 
 main() {
     cd "$ROOT"
-    docker build --build-arg CACHEBUST="$(date +%s)-$$" \
+    docker build --no-cache \
         --target lint \
-        --output type=cacheonly .
+        -t "$("$SCRIPT_DIR/projectname")-lint" .
 }
 
 main "$@"

script/test

@@ -2,12 +2,8 @@
 # script/test: run the test suite. Testing is a phase of the Dockerfile
 # and this builds that phase alone, on the same terms as script/lint:
 # --target because a phase that is not the last stage is built only when
-# named, and a new CACHEBUST value because a cached test layer is a test
-# that did not run; unlike --no-cache, it keeps Go's build cache. This
-# relies on every stage of the Dockerfile declaring ARG CACHEBUST first;
-# a stage without that line is served from cache on an unchanged tree,
-# with no warning. --output type=cacheonly writes no image, since
-# nothing uses one.
+# named, --no-cache because a cached test layer is a test that did not
+# run, and a tag so each build replaces the previous image.
 set -eu
 
 SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd -P)"
@@ -15,9 +11,9 @@ ROOT="$(cd "$SCRIPT_DIR/.." && pwd -P)"
 
 main() {
     cd "$ROOT"
-    docker build --build-arg CACHEBUST="$(date +%s)-$$" \
+    docker build --no-cache \
         --target test \
-        --output type=cacheonly .
+        -t "$("$SCRIPT_DIR/projectname")-test" .
 }
 
 main "$@"