Align DeliveryResult tags and make the index table test-local

Re-align the json tag column in DeliveryResult after the wider
DeliveryID gorm tag, and move the test's index table out of package
scope into the test function.

Model: opus-5-5

Dockerfile

@@ -88,9 +88,7 @@ RUN CGO_ENABLED=1 make build VERSION="$VERSION" GO_LDFLAGS='-extldflags "-static
 # alpine:3.21, 2026-03-17
 FROM alpine:3.21@sha256:c3f8e73fdb79deaebaa2037150150191b9dcbfba68b4a46d70103204c53f4709
 
-# su-exec 0.2-r3 (Alpine 3.21), 2026-09-29: the entrypoint runs the app
-# as webhooker with it.
-RUN apk --no-cache add ca-certificates su-exec=0.2-r3
+RUN apk --no-cache add ca-certificates
 
 # Create non-root user
 RUN addgroup -g 1000 -S webhooker && \
@@ -101,17 +99,13 @@ WORKDIR /app
 # Copy binary from builder
 COPY --from=builder /build/bin/webhooker /app/webhooker
 
-# Not under /app, which belongs to webhooker: this script runs as root.
-COPY deploy/docker-entrypoint.sh /usr/local/bin/docker-entrypoint.sh
-
 # Create data directory for all SQLite databases (main app DB +
 # per-webhook event DBs). DATA_DIR defaults to /var/lib/webhooker.
 RUN mkdir -p /var/lib/webhooker
 
 RUN chown -R webhooker:webhooker /app /var/lib/webhooker
 
-# No USER: the entrypoint starts as root to make the data directory
-# webhooker's, then runs the app as webhooker.
+USER webhooker
 
 EXPOSE 8080
 
@@ -130,5 +124,4 @@ ENV BIND_ADDRESS=0.0.0.0
 HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \
     CMD wget --no-verbose --tries=1 --spider http://localhost:8080/.well-known/healthcheck || exit 1
 
-ENTRYPOINT ["/usr/local/bin/docker-entrypoint.sh"]
 CMD ["/app/webhooker"]

README.md

@@ -44,9 +44,9 @@ make bootstrap
 # Run all checks (test, lint, format check)
 make check
 
-# Run the server from the clone. DATA_DIR defaults to
-# /var/lib/webhooker in every environment, so set it (in .env or the
-# shell) to a writable directory.
+# Run in development mode. DATA_DIR defaults to /var/lib/webhooker in
+# every environment, so set it (in .env or the shell) to a writable
+# directory when running from a clone.
 DATA_DIR=./data make dev
 
 # Build Docker image
@@ -92,8 +92,7 @@ them at once. A variable already present in the real environment wins
 over the file's value for the same name.
 
 The environment is selected by setting `WEBHOOKER_ENVIRONMENT` to `dev`
-or `prod` (default: `prod`; `dev` must be set explicitly). The setting
-controls exactly one behavior:
+or `prod` (default: `dev`). The setting controls exactly one behavior:
 
 | Behavior | `dev`                   | `prod`           |
 | -------- | ----------------------- | ---------------- |
@@ -135,7 +134,7 @@ TTY detection, and security headers are always applied.
 
 | Variable                | Description                         | Default  |
 | ----------------------- | ----------------------------------- | -------- |
-| `WEBHOOKER_ENVIRONMENT` | `dev` or `prod`                     | `prod`   |
+| `WEBHOOKER_ENVIRONMENT` | `dev` or `prod`                     | `dev`    |
 | `PORT`                  | HTTP listen port                    | `8080`   |
 | `BIND_ADDRESS`          | IP address the HTTP listener binds. Loopback by default, so the cleartext listener is not published on every interface. The Docker image ships `0.0.0.0` instead. See [Bind address](#bind-address) | `127.0.0.1` (image: `0.0.0.0`) |
 | `DATA_DIR`              | Directory for all SQLite databases  | `/var/lib/webhooker` |
@@ -157,11 +156,6 @@ private and reserved ranges — RFC 1918, loopback, CGNAT, link-local and
 the rest — are refused, which stops a target from being used to make
 webhooker probe the network it sits in.
 
-Besides the private and reserved ranges, the default blocklist refuses
-public cloud metadata addresses: currently only `168.63.129.16`, Azure's
-WireServer, which serves an Azure VM its credentials. Because it is a
-public address, listing it in `ALLOWED_EGRESS_CIDRS` reopens it.
-
 That default is also inconvenient for the thing webhooker is mostly
 for: taking a public webhook and forwarding it to something on your own
 network. A container on the same Docker network, a box on `10.x`, a
@@ -200,16 +194,15 @@ Two things this setting cannot do:
   the list is always an allowlist; an empty list (the default) means
   every private and reserved range stays refused. Note that
   `0.0.0.0/0` gets you most of the way there anyway, per above.
-- **It cannot open link-local, or a cloud metadata endpoint at a
-  non-public address that discloses credentials or user data.** An
-  address is on the list below when it is not a public address and both
-  of these hold: the provider fixes it, so it cannot collide with
-  anything you run; and reaching it hands out credentials, user data or
-  bootstrap material. Those stay blocked no matter what you list,
-  including when you list them outright or list a supernet such as
-  `0.0.0.0/0`, `::/0`, `fd00::/8` or `100.64.0.0/10`. Treat this as best
-  effort rather than a guarantee — it is a hand-maintained list and the
-  caveat below the table applies:
+- **It cannot open link-local, or a cloud metadata endpoint that
+  discloses credentials or user data.** An address is on the list below
+  when both of these hold: the provider fixes it, so it cannot collide
+  with anything you run; and reaching it hands out credentials, user
+  data or bootstrap material. Those stay blocked no matter what you
+  list, including when you list them outright or list a supernet such
+  as `0.0.0.0/0`, `::/0`, `fd00::/8` or `100.64.0.0/10`. Treat this as
+  best effort rather than a guarantee — it is a hand-maintained list
+  and the caveat below the table applies:
 
   | Blocked unconditionally | What it is |
   | ----------------------- | ---------- |
@@ -248,8 +241,7 @@ Two things this setting cannot do:
   encodings, which the default blocklist does not match. A publicly
   routable metadata address is not listed here, because nothing on this
   list can be reopened and blocking one that way would leave you no
-  escape hatch at all; Azure's `168.63.129.16` is refused by the default
-  blocklist instead, as described above.
+  escape hatch at all.
 
   This list is not exhaustive of every cloud's metadata address — if
   yours is not here, do not allowlist the block that contains it.
@@ -407,9 +399,10 @@ the bucket is. See [Rate Limiting](#rate-limiting).
 
 The remedy is to set `TRUSTED_PROXIES` to your reverse proxy's
 address, which restores per-client buckets. webhooker logs a warning
-at startup whenever `TRUSTED_PROXIES` is empty, in every environment,
-because behind a proxy every client shares one bucket in `dev` and
-`prod` alike. The warning is informational when nothing proxies to the
+at startup whenever `TRUSTED_PROXIES` is empty, in every environment —
+not only when `WEBHOOKER_ENVIRONMENT=prod`, because that variable
+defaults to `dev` and an operator who never set it is precisely the
+one at risk. The warning is informational when nothing proxies to the
 process: with no proxy in front, the peer address is the client's own
 and the buckets are already per-client. See
 [Rate Limiting](#rate-limiting) for what each limit shares.
@@ -538,12 +531,6 @@ its Argon2id hash. There is no second account and no forgot-password
 flow, so the banner and the reset command below are the only two ways
 in.
 
-A start that finds no `webhooker.db` in `DATA_DIR` also logs
-`created a new, empty database` at `WARN`, with the file's path,
-shortly before the banner. On a deployment that has run before, that
-line means `DATA_DIR` was empty, most often because its volume is not
-mounted.
-
 #### Recovering a lost admin password
 
 `webhooker resetpw` sets an existing account's password from the
@@ -558,9 +545,8 @@ printf '%s' "$NEW_PASSWORD" | \
     DATA_DIR=/var/lib/webhooker webhooker resetpw admin
 ```
 
-In a container it is the same binary. The image's `CMD` is
-`/app/webhooker`, and a command given to `docker run` replaces all of
-it, so the whole command has to be given:
+In a container it is the same binary, which the image sets as `CMD`
+rather than `ENTRYPOINT`, so the whole command has to be given:
 
 ```bash
 docker run --rm -v webhooker-data:/var/lib/webhooker \
@@ -652,6 +638,7 @@ decision:
 docker run -d \
   -p 127.0.0.1:8080:8080 \
   -v /path/to/data:/var/lib/webhooker \
+  -e WEBHOOKER_ENVIRONMENT=prod \
   -e BIND_ADDRESS=0.0.0.0 \
   webhooker:latest
 ```
@@ -697,22 +684,38 @@ those three values rather than trusting the figure. Measured at 65s on
 Docker 29.7.2.) A container `unhealthy` with `connection refused` in
 its health log, or a published port that resets connections, is this.
 
-The app runs as a non-root user (`webhooker`, UID 1000), exposes port
-8080, and includes a health check against `/.well-known/healthcheck`.
-The `/var/lib/webhooker` volume holds all SQLite databases: the main
-application database (`webhooker.db`), the per-webhook event databases
-(`events-{uuid}.db`), and any archive databases written by `database`
-targets (`archive-{uuid}.db`). Mount this as a persistent volume to
-preserve data across container restarts.
+The container runs as a non-root user (`webhooker`, UID 1000), exposes
+port 8080, and includes a health check against
+`/.well-known/healthcheck`. The `/var/lib/webhooker` volume holds all
+SQLite databases: the main application database (`webhooker.db`), the
+per-webhook event databases (`events-{uuid}.db`), and any archive
+databases written by `database` targets (`archive-{uuid}.db`). Mount
+this as a persistent volume to preserve data across container
+restarts.
 
-**The container sets its data directory's owner and mode itself
-before the app starts**, so a host directory can be mounted as it is,
-whoever owns it. The image's `ENTRYPOINT`,
-`deploy/docker-entrypoint.sh`, starts as root, creates `DATA_DIR` if
-it is missing, gives the directory and anything in it that belongs to
-another user to `webhooker`, sets the directory to `0750`, and only
-then runs the app as `webhooker`. Started with `--user`, it changes
-nothing and runs the app as that user.
+**The bind-mounted directory must be owned by UID 1000, or the
+container does not start.** Docker creates a `-v` source path that
+does not exist yet as `root:root`, and the process runs as UID 1000,
+so it cannot take its `DATA_DIR` lock:
+
+```
+webhooker: locking data directory /var/lib/webhooker: open
+/var/lib/webhooker/webhooker.lock: permission denied
+```
+
+It exits non-zero at that point, before opening any database. Create
+the directory ahead of the first `docker run`:
+
+```bash
+mkdir -p /path/to/data
+chown 1000:1000 /path/to/data
+chmod 750 /path/to/data
+```
+
+The same `chown` is what a restore needs — see step 4 of
+[Restore](#restore). A **named volume** does not have this problem:
+Docker copies the image's ownership onto a volume it initializes, and
+the image creates `/var/lib/webhooker` owned by `webhooker`.
 
 **The file modes are not yours to set, and do not depend on the
 directory.** `webhooker.db` holds target configuration in plaintext —
@@ -720,10 +723,13 @@ bearer tokens, API keys, Slack webhook URLs — along with the session
 encryption key, so webhooker creates every SQLite file it owns `0600`:
 each database and both of its `-wal` and `-shm` sidecars, across all
 three tiers. Files an earlier build left `0644` are tightened when
-they are opened. The directory's `0750` is defence in depth — it stops
-other local users listing the directory and learning your webhook
-UUIDs from the `events-{uuid}.db` filenames — not the barrier
-protecting the credentials.
+they are opened. A `DATA_DIR` webhooker creates itself is `0750`, but
+a bind mount supplies its own directory and Docker's default for one
+it creates is `0755`; the `0600` files hold there regardless. The
+`chmod 750` above is defence in depth — it stops other local users
+listing the directory and learning your webhook UUIDs from the
+`events-{uuid}.db` filenames — not the barrier protecting the
+credentials.
 
 ### Running under upaas
 
@@ -739,6 +745,17 @@ repository's `Dockerfile` and runs it. The app needs:
   app name, port `8080`. Leave `PORT` unset: the image's health check
   probes `8080`.
 - **Volume:** one host directory mounted at `/var/lib/webhooker`.
+  upaas bind-mounts the host path it is given and does not create it,
+  and the container does not start unless UID 1000 owns it (see
+  [Running with Docker](#running-with-docker)). Create it before the
+  first deploy:
+
+    ```bash
+    mkdir -p /path/to/data
+    chown 1000:1000 /path/to/data
+    chmod 750 /path/to/data
+    ```
+
 - **Environment variables:**
     - `WEBHOOKER_ENVIRONMENT=prod`
     - `TRUSTED_PROXIES`: your reverse proxy's address on that Docker
@@ -795,18 +812,17 @@ reports.
    serves the admin login form and the unauthenticated receiver with
    no TLS at all, and the proxy in front of it changes nothing about
    that.
-2. **Make sure the environment is not `dev` (leave
-   `WEBHOOKER_ENVIRONMENT` unset or set it to `prod`), and make sure
-   the proxy sends `X-Forwarded-Proto`.** These are two requirements,
-   not one. The environment setting decides CORS and nothing else:
+2. **Set `WEBHOOKER_ENVIRONMENT=prod`, and make sure the proxy sends
+   `X-Forwarded-Proto`.** These are two requirements, not one. The
+   environment setting decides CORS and nothing else: the default
    `dev` answers every origin with `Access-Control-Allow-Origin: *`
    (without credentials), which a server-rendered production
-   deployment has no use for, and `prod` — the default — disables it.
-   Cookie `Secure` and the strict Origin/Referer mode are **not** tied
-   to it — they are decided per request from the transport, which
-   behind a proxy means the `X-Forwarded-Proto` header. The block below
-   sets it; without it every request is read as plaintext and cookies
-   ship without `Secure`. See [Configuration](#configuration).
+   deployment has no use for. Cookie `Secure` and the strict
+   Origin/Referer mode are **not** tied to it — they are decided per
+   request from the transport, which behind a proxy means the
+   `X-Forwarded-Proto` header. The block below sets it; without it
+   every request is read as plaintext and cookies ship without
+   `Secure`. See [Configuration](#configuration).
 3. **Set `TRUSTED_PROXIES` to the proxy's address.** Unset, every rate
    limiter keys on the connecting peer, which behind a proxy is the
    proxy on every request: all clients collapse into one global bucket
@@ -898,6 +914,7 @@ sent — `$scheme` above does.
 With that block, webhooker's environment is:
 
 ```sh
+WEBHOOKER_ENVIRONMENT=prod
 BIND_ADDRESS=127.0.0.1   # the default; stated here to be explicit
 TRUSTED_PROXIES=127.0.0.1
 ```
@@ -952,10 +969,15 @@ scratch file**: it holds committed transactions that are not yet in the
 have no readable schema at all. `-shm` is regenerable, but there is no
 reason to separate the two — copy the directory and you have them.
 
-A clean shutdown closes every database, which checkpoints and removes
-its sidecars; a killed or crashed instance leaves them, and they must be
-carried with the `.db`. An archive the service has not opened since a
-crash keeps that crash's sidecars, even across a later clean stop.
+A clean shutdown closes `webhooker.db` and every `events-*.db`, which
+checkpoints and removes their sidecars; a killed or crashed instance
+leaves them, and they must be carried with the `.db`. **Archive
+databases are different**: their handle is not closed at shutdown, so
+`archive-*.db-wal` and `-shm` normally survive a clean stop and the
+`-wal` can hold every row the archive has. Measured on a stopped
+instance: `archive-….db` 4096 bytes with no table, its `-wal` 157 KB
+holding all 8 archived events. Copying `DATA_DIR` in full is what makes
+this a non-issue; copying `.db` files out of it by name is not.
 
 Configuration is **not** in `DATA_DIR` — it comes from the environment
 and from a `.env` file read out of the process working directory. Back
@@ -997,12 +1019,12 @@ done
 `.backup` reads through the WAL and writes a single consistent file with
 no sidecars of its own, so the destination is complete as it stands.
 Two caveats. First, the runtime image is `alpine:3.21` with only
-`ca-certificates` and `su-exec` added — the `sqlite3` CLI is **not** in
-it, so run this on the host against the volume path, or from a
-throwaway container that mounts the volume. Second, each file is
-captured at its own instant, so a webhook created or an event delivered
-between two files being copied lands in one and not the other. If you
-need the whole set coherent as of a single moment, stop the service.
+`ca-certificates` added — the `sqlite3` CLI is **not** in it, so run
+this on the host against the volume path, or from a throwaway container
+that mounts the volume. Second, each file is captured at its own
+instant, so a webhook created or an event delivered between two files
+being copied lands in one and not the other. If you need the whole set
+coherent as of a single moment, stop the service.
 
 Note that `sqlite3 <db> .dump` is **not** one of these procedures: it is
 an export, it holds a read transaction open for as long as it runs, and
@@ -1030,9 +1052,10 @@ The file becomes self-contained again when the handle closes, which
 happens on the next write past the debounce window, when the connection
 pool retires the idle connection (about a minute after the last write),
 or at the idle archive sweep — measured, the same file was a complete
-20 KB `.db` with no sidecars about a minute after its last write. A
-clean stop closes it too. So either move `archive-{uuid}.db` together
-with any `-wal`/`-shm` beside it, or wait until there are none.
+20 KB `.db` with no sidecars about a minute after its last write.
+Shutdown is **not** on that list: the archive handle is not closed when
+the service stops. So either move `archive-{uuid}.db` together with any
+`-wal`/`-shm` beside it, or wait until there are none.
 
 ### Restore
 
@@ -1051,16 +1074,28 @@ with any `-wal`/`-shm` beside it, or wait until there are none.
    They are part of the database, and dropping a `-wal` silently
    discards every transaction it still holds. An `.backup` set will not
    contain any: it writes a single consolidated file per database. A
-   stop-and-copy set normally has none, because a clean stop closes
-   every database and checkpoints its sidecars away; the exception is an
-   archive not opened since a crash. A copy salvaged from a crashed
-   instance has them for everything, and needs all of them.
+   stop-and-copy set has none for `webhooker.db` or the `events-*.db`,
+   because a clean stop closes those and checkpoints their sidecars
+   away — but it will normally have them for `archive-*.db`, whose
+   handle stays open across shutdown, and those carry the archive's
+   rows. A copy salvaged from a crashed instance has them for
+   everything, and needs all of them.
 
-4. Start the service. The container gives the directory and the
-   restored files to the `webhooker` user before the app starts,
-   whoever restored them (see
-   [Running with Docker](#running-with-docker)). `AutoMigrate` runs
-   against each restored database as it is opened.
+4. **Fix ownership.** The container runs as the non-root `webhooker`
+   user, UID 1000 / GID 1000. Restored files must be owned by (or
+   writable by) that UID, and so must the directory itself — SQLite
+   creates the `-wal` and `-shm` sidecars beside the database, so a
+   writable file inside a directory it cannot write is not enough:
+
+    ```bash
+    chown -R 1000:1000 /path/to/data
+    ```
+
+    Restoring as `root` on the host and forgetting this step is the
+    usual way a restore fails.
+
+5. Start the service. `AutoMigrate` runs against each restored database
+   as it is opened.
 
 ### Upgrades
 
@@ -1222,15 +1257,8 @@ standard: normalized scripts in `script/` are the entrypoints for the
 development workflow. Ten of the Makefile's seventeen targets are thin
 shims that call them; `build`, `run`, `dev`, `deps`, `clean`, `css` and
 `version` are inline commands with no script behind them, though
-`build` and `version` both take their value from `script/version`.
-
-`make check` needs the third-party browser assets in `static/`, which
-are not committed, so run `make bootstrap` (or just `make assets`) once
-after cloning. Without them the tests fail with a message naming that
-remedy. `make check` does not fetch them itself because it must not
-change any files in the repo.
-
-We provide:
+`build` and `version` both take their value from `script/version`. We
+provide:
 
 - `script/bootstrap` — install all dependencies (idempotent)
 - `script/setup` — make a fresh clone ready for development
@@ -1734,25 +1762,19 @@ retries) is individually logged for full observability.
 
 #### Event-tier indexes
 
-These indexes on the per-webhook event databases are declared in the model
-tags, so `AutoMigrate` creates them on a fresh and on an existing database:
+Beyond the primary keys, the per-webhook event databases carry secondary
+indexes on the columns the background work reads by, each created by
+`AutoMigrate` on a fresh and on an existing database:
 
-| Table              | Columns                     | Serves |
-| ------------------ | --------------------------- | ------ |
-| `deliveries`       | `status`, `deleted_at`      | Startup recovery, the retry and pending sweeps every 60 seconds and the queue-depth sampler every 30 seconds, which select deliveries by status |
-| `deliveries`       | `event_id`, `deleted_at`    | The event log, which loads each event's deliveries, and retention, which selects and deletes the deliveries of expired events |
-| `delivery_results` | `delivery_id`, `deleted_at` | The event log, which loads the attempts of a page's deliveries, and retention, which deletes the attempts of expired events |
-| `events`           | `deleted_at`, `created_at`  | Retention, which selects expired events by age |
-| `events`           | `created_at`                | Retention's delete of the expired events themselves |
+| Column                         | Serves |
+| ------------------------------ | ------ |
+| `deliveries.status`            | The recovery and sweep queries that select deliveries by status once a minute |
+| `deliveries.event_id`          | Loading a page of the event log, which reads deliveries by event |
+| `delivery_results.delivery_id` | Loading a page of the event log, which reads results by delivery |
+| `events.created_at`            | Retention, which deletes events by age |
 
-GORM's soft delete adds `deleted_at IS NULL` to these queries; retention's
-deletes leave it out, but their lookups of expired rows keep it. SQLite keeps
-no statistics on these tables, and without them it rates the `deleted_at`
-index, which every live row matches, above an index on a column matched
-against several values or compared with `<`. So every index but the last also
-covers `deleted_at`. It comes second, so that retention's deletes can use the
-index without it, except in `events`, where `created_at` is compared with `<`
-and SQLite narrows by a `<` only on the last column it uses.
+The `events.resubmitted_from_id` column is also indexed, to resolve the
+resubmit relationship both ways in the event log.
 
 #### Common Fields
 
@@ -2017,7 +2039,7 @@ rescans the database anyway).
 | ----------- | -------- |
 | **Closed**  | Normal operation. Deliveries flow through. Consecutive failures are counted. |
 | **Open**    | Target appears down. Deliveries are skipped and rescheduled for after the cooldown. |
-| **Half-Open** | Cooldown expired. One probe delivery is allowed to test if the target has recovered. Other deliveries are rescheduled for one whole cooldown later. |
+| **Half-Open** | Cooldown expired. One probe delivery is allowed to test if the target has recovered. |
 
 **Transitions:**
 
@@ -2055,9 +2077,7 @@ operations), and log targets (stdout) do not use circuit breakers.
 When a circuit is open and a new delivery arrives, the engine marks the
 delivery as `retrying` and schedules a retry timer for after the
 remaining cooldown period. This ensures no deliveries are lost — they're
-just delayed until the target is healthy again. A delivery already in
-`retrying` keeps that status without another database write each time
-the breaker turns it away.
+just delayed until the target is healthy again.
 
 ### Metrics
 
@@ -2071,7 +2091,7 @@ arriving and being stored, they are just not getting anywhere.
 | Metric | Type | Meaning |
 | ------ | ---- | ------- |
 | `webhooker_events_received_total` | counter | Events received and durably stored. Compare against the delivery counters on one dashboard |
-| `webhooker_delivery_attempts_total` | counter | Delivery attempts actually dispatched to a target. A delivery a circuit breaker refused is not one: it is counted as a retry instead, but only when the refusal moves it into `retrying` |
+| `webhooker_delivery_attempts_total` | counter | Delivery attempts actually dispatched to a target. A delivery an open circuit breaker refused is not one: it is counted as a retry instead |
 | `webhooker_deliveries_succeeded_total` | counter | Deliveries that reached `delivered` |
 | `webhooker_deliveries_failed_total` | counter | Deliveries that failed terminally and will not be retried |
 | `webhooker_delivery_retries_total` | counter | Deliveries put back into `retrying` |
@@ -2688,7 +2708,7 @@ abuse limit later; they are tracked as future work.
 | ------ | --------------------------- | ----------- |
 | `GET`  | `/`                         | Root redirect, 303 (authenticated → `/sources`, unauthenticated → `/pages/login`) |
 | `GET`  | `/.well-known/healthcheck`  | Health check (JSON: `status`, `now`, `uptimeSeconds`, `uptimeHuman`, `version`, `appname`, `maintenanceMode`) |
-| `GET`, `HEAD` | `/s/*`              | Static file serving (embedded CSS, JS). `GET` and `HEAD` only — `POST`, `PUT`, `PATCH`, `DELETE`, `OPTIONS`, `TRACE` and `CONNECT` are answered `405 Method Not Allowed` with `Allow: GET, HEAD`. Any other method (such as `PROPFIND`) is refused by chi before it reaches this route, and gets `405` without an `Allow` header. Pinned by `TestStaticServesOnlyGetAndHead` |
+| any    | `/s/*`                      | Static file serving (embedded CSS, JS). Mounted for every method, not just `GET`/`HEAD`: chi's `Mount` registers all methods and `http.FileServer` special-cases only `HEAD` (by omitting the body), so a `POST` or `DELETE` to an asset is answered `200` with the file. Pinned by `TestStaticServesEveryMethod` |
 | `POST` | `/webhook/{uuid}`           | Webhook receiver endpoint. `POST` only — every other method is answered `405 Method Not Allowed` with `Allow: POST`. Rate limited (see [Rate Limiting](#rate-limiting)) |
 
 #### Authentication Endpoints
@@ -3038,11 +3058,7 @@ check, see [The login endpoint](#the-login-endpoint).
 - Prometheus metrics behind basic auth
 - Static assets embedded in binary (no filesystem access needed at
   runtime)
-- The app runs as the non-root `webhooker` user (UID 1000) in the
-  container. The image sets no `USER`, so these run as root: the
-  `ENTRYPOINT` script, which sets the data directory's owner and mode
-  before the app starts; the image's health check; and `docker exec`,
-  unless given `--user`
+- Container runs as non-root user (UID 1000)
 - GORM soft deletes on every entity that carries `BaseModel`, which is
   all of them but `Setting` (data preserved for audit)
 
@@ -3058,8 +3074,7 @@ each hook. The order, read off the fx stop-hook log:
 3. `server` — the HTTP drain, bounded separately by
    `server.ShutdownTimeout` (**3 seconds**), then a Sentry flush if
    `SENTRY_DSN` is set
-4. `delivery.Engine` — waits for its workers, then closes the archive
-   databases
+4. `delivery.Engine`
 5. `healthcheck`
 6. `WebhookDBManager`
 7. the database close
@@ -3169,13 +3184,10 @@ version is fixed independently of the compiler's:
    `GO_LDFLAGS`, so neither can drop the `-X` that stamps the version.
    The version arrives as the `VERSION` build arg, since the context
    has no `.git` (see [Version stamping](#version-stamping)).
-3. **Runtime stage** (`alpine:3.21`) — copies the static binary and
-   `deploy/docker-entrypoint.sh`, creates the `/var/lib/webhooker`
-   directory for all SQLite databases, exposes port 8080, and includes
-   a health check against `/.well-known/healthcheck`. It sets no
-   `USER`: the `ENTRYPOINT` script starts as root, sets the data
-   directory's owner and mode, and runs the app as the non-root
-   `webhooker` user (UID 1000) through `su-exec`.
+3. **Runtime stage** (`alpine:3.21`) — copies the static binary,
+   creates the `/var/lib/webhooker` directory for all SQLite databases,
+   runs as the non-root `webhooker` user (UID 1000), exposes port 8080,
+   and includes a health check against `/.well-known/healthcheck`.
 
 The lint stage invokes `golangci-lint` directly rather than `make lint`:
 it is already the pinned linter image, and `make lint` builds
@@ -3254,5 +3266,3 @@ MIT
 ## Author
 
 [@sneak](https://sneak.berlin)
-
-

deploy/docker-entrypoint.sh

@@ -1,22 +0,0 @@
-#!/bin/sh
-# deploy/docker-entrypoint.sh: the image's ENTRYPOINT. A bind-mounted
-# data directory keeps its owner from the host, often root, and the app
-# could not write to it. Started as root, this creates DATA_DIR if
-# needed, gives it and everything in it to webhooker, sets its mode, and
-# runs the command as webhooker, so the app never runs as root. Started
-# as another user, it only runs the command.
-set -eu
-
-main() {
-    if [ "$(id -u)" != 0 ]; then
-        exec "$@"
-    fi
-
-    dir="${DATA_DIR:-/var/lib/webhooker}"
-    mkdir -p "$dir"
-    find "$dir" ! -user webhooker -exec chown -h webhooker:webhooker {} +
-    chmod 750 "$dir"
-    exec su-exec webhooker "$@"
-}
-
-main "$@"

internal/config/config.go

@@ -192,10 +192,9 @@ type Config struct {
 	// alwaysBlockedNetworks stays blocked no matter what is listed
 	// here. That set is link-local plus the cloud metadata
 	// endpoints outside it that disclose credentials or user data
-	// at a provider-fixed, non-public address; it is not
-	// exhaustive of every cloud's metadata address. See
-	// alwaysBlockedNetworks for the authoritative list and the
-	// criterion it is built from.
+	// at a provider-fixed address; it is not exhaustive of every
+	// cloud's metadata address. See alwaysBlockedNetworks for the
+	// authoritative list and the criterion it is built from.
 	AllowedEgressCIDRs []netip.Prefix
 
 	params *ConfigParams
@@ -586,14 +585,12 @@ func resolveMetricsAuth() (string, string, error) {
 	)
 }
 
-// resolveEnvironment reads WEBHOOKER_ENVIRONMENT, defaulting to prod
-// when it is unset so a deployment that forgets the variable is not
-// silently permissive; dev must be set explicitly. It rejects
-// unrecognised values.
+// resolveEnvironment reads WEBHOOKER_ENVIRONMENT, defaulting to
+// dev, and rejects unrecognised values.
 func resolveEnvironment() (string, error) {
 	environment := os.Getenv("WEBHOOKER_ENVIRONMENT")
 	if environment == "" {
-		environment = EnvironmentProd
+		environment = EnvironmentDev
 	}
 
 	if environment != EnvironmentDev &&
@@ -747,14 +744,12 @@ func (c *Config) warnEgressAllowlist(log *slog.Logger) {
 
 	log.Warn(
 		"ALLOWED_EGRESS_CIDRS lets delivery targets reach these "+
-			"otherwise-blocked networks. Anyone who can create a "+
-			"delivery target can now make this process issue "+
-			"requests into them, and read back the response. Only "+
-			"the addresses the README lists as blocked "+
-			"unconditionally stay blocked regardless of what is "+
-			"listed here; a public cloud metadata address such as "+
-			"168.63.129.16 is reachable once it, or a block "+
-			"covering it, is listed.",
+			"otherwise-blocked private/reserved networks. Anyone "+
+			"who can create a delivery target can now make this "+
+			"process issue requests into them, and read back the "+
+			"response. Link-local and the known cloud instance "+
+			"metadata endpoints outside it stay blocked "+
+			"regardless of what is listed here.",
 		"allowedEgressCIDRs",
 		strings.Join(PrefixStrings(c.AllowedEgressCIDRs), ","),
 	)
@@ -777,8 +772,10 @@ func (c *Config) warnEgressAllowlist(log *slog.Logger) {
 // everyone else's wrong passwords, and the receiver's limits become
 // service-wide ceilings.
 //
-// The warning is deliberately not gated on WEBHOOKER_ENVIRONMENT:
-// behind a proxy every client shares one bucket in dev and prod alike.
+// The warning is deliberately not gated on WEBHOOKER_ENVIRONMENT. That
+// variable defaults to dev, so gating on it would silence the warning
+// for exactly the operator who forgot to configure the deployment —
+// the case it exists to catch.
 //
 // The default of trusting nobody is deliberate — trusting forwarded
 // headers from arbitrary peers lets any client choose its own bucket —

internal/config/config_test.go

@@ -44,9 +44,9 @@ func TestEnvironmentConfig(t *testing.T) {
 		isProd      bool
 	}{
 		{
-			name:   "default is prod",
-			isDev:  false,
-			isProd: true,
+			name:   "default is dev",
+			isDev:  true,
+			isProd: false,
 		},
 		{
 			name:     "explicit dev",
@@ -834,13 +834,12 @@ func TestEgressAllowlistWarning(t *testing.T) {
 			// to be able to read back which networks are open.
 			assert.Contains(t, logged, "10.0.0.0/8")
 			assert.Contains(t, logged, "127.0.0.0/8")
-			// What stays shut is the whole unconditional set, not
-			// link-local alone; a public metadata address is not in
-			// it, so a listed block covering it opens it.
-			assert.Contains(t, logged, "blocked unconditionally")
-			assert.Contains(t, logged, "168.63.129.16 is reachable")
-			// The listed blocks need not be private or reserved.
-			assert.NotContains(t, logged, "private/reserved")
+			// What stays shut. Asserted on the clause naming the
+			// wider set rather than on "Link-local" alone, so the
+			// string cannot narrow back to link-local only while
+			// the always-blocked set covers ULA, CGNAT and two
+			// public metadata addresses as well.
+			assert.Contains(t, logged, "metadata endpoints outside it")
 		})
 	}
 }
@@ -849,9 +848,10 @@ func TestEgressAllowlistWarning(t *testing.T) {
 // tells an operator a deployment behind a reverse proxy shares one
 // rate-limit bucket between every client, which turns the receiver
 // limits into service-wide ceilings and collapses login failure
-// counting. It must fire whenever TRUSTED_PROXIES is empty, in any
-// environment, because behind a proxy every client shares one bucket
-// in dev and prod alike. It stays quiet once proxies are named.
+// counting. It must fire whenever TRUSTED_PROXIES is empty,
+// in any environment: WEBHOOKER_ENVIRONMENT defaults to dev, so gating
+// on it would silence the warning for exactly the operator who never
+// configured the deployment. It stays quiet once proxies are named.
 func TestSharedRateLimitBucketWarning(t *testing.T) {
 	tests := []struct {
 		name           string
@@ -871,6 +871,10 @@ func TestSharedRateLimitBucketWarning(t *testing.T) {
 			expectWarning:  false,
 		},
 		{
+			// The default environment. An internet-exposed
+			// deployment whose operator never set
+			// WEBHOOKER_ENVIRONMENT lands here and has exactly
+			// the exposure the warning announces.
 			name:          "dev without trusted proxies warns",
 			environment:   config.EnvironmentDev,
 			expectWarning: true,

internal/database/bootstrap_banner_test.go

@@ -3,8 +3,6 @@ package database_test
 import (
 	"bytes"
 	"context"
-	"log/slog"
-	"path/filepath"
 	"strings"
 	"testing"
 
@@ -85,37 +83,3 @@ func TestFirstBoot_PrintsTheAdminPasswordAsABanner(t *testing.T) {
 		t, ok, "the printed password must open the seeded account",
 	)
 }
-
-// TestNewDatabase_IsLoggedWithItsPath is the log half of
-// https://git.eeqj.de/sneak/webhooker/issues/359. A DATA_DIR that is
-// unexpectedly empty boots exactly like a first start, so the start
-// that creates the database must say so, and where. Opening that
-// database again must not.
-func TestNewDatabase_IsLoggedWithItsPath(t *testing.T) {
-	t.Parallel()
-
-	dir := t.TempDir()
-
-	open := func() string {
-		var out bytes.Buffer
-
-		db, err := database.Open(dir, slog.New(slog.NewTextHandler(&out, nil)))
-		require.NoError(t, err)
-		require.NoError(t, db.Close())
-
-		return out.String()
-	}
-
-	const created = `level=WARN msg="created a new, empty database"`
-
-	first := open()
-	second := open()
-
-	assert.Contains(
-		t, first,
-		created+" path="+filepath.Join(dir, database.MainDBFileName),
-	)
-	assert.NotContains(
-		t, second, created, "an existing database is not new",
-	)
-}

internal/database/database.go

@@ -8,7 +8,6 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"io/fs"
 	"log/slog"
 	"os"
 	"path/filepath"
@@ -200,12 +199,6 @@ func (d *Database) connectTo(dataDir string) error {
 	// Construct the main application database path inside DATA_DIR.
 	dbPath := filepath.Join(dataDir, MainDBFileName)
 
-	// Checked before opening, which creates the file. A DATA_DIR that
-	// is unexpectedly empty -- its volume not mounted, say -- looks
-	// exactly like a first start, so a new database is a warning.
-	_, statErr := os.Stat(dbPath)
-	created := errors.Is(statErr, fs.ErrNotExist)
-
 	// Opened through OpenSQLite so this handle carries the same WAL
 	// journaling, busy timeout, immediate-transaction locking, and pool
 	// bounds as every other database file. See sqlite_open.go.
@@ -236,12 +229,7 @@ func (d *Database) connectTo(dataDir string) error {
 	}
 
 	d.db = db
-
-	if created {
-		d.log.Warn("created a new, empty database", "path", dbPath)
-	} else {
 	d.log.Info("connected to database", "path", dbPath)
-	}
 
 	// Run migrations
 	return d.migrate()

internal/database/event_tier_indexes_test.go

@@ -2,33 +2,37 @@ package database_test
 
 import (
 	"context"
-	"fmt"
 	"testing"
-	"time"
 
 	"github.com/google/uuid"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-	"gorm.io/gorm"
 	"sneak.berlin/go/webhooker/internal/database"
 )
 
+// indexedColumn names a secondary index by the model and struct field
+// GORM derives the index name from.
+type indexedColumn struct {
+	model any
+	field string
+}
+
 // TestWebhookDBManager_OpenAddsEventTierIndexes verifies that opening a
-// per-webhook database that predates these indexes creates them. It
+// per-webhook database that predates these indexes creates them, so the
+// queries that read by those columns stop scanning whole tables. It
 // stands in for an older database file by dropping the indexes
 // AutoMigrate just created, then reopening the same file.
 func TestWebhookDBManager_OpenAddsEventTierIndexes(t *testing.T) {
 	t.Parallel()
 
-	indexes := []struct {
-		model any
-		name  string
-	}{
-		{&database.Delivery{}, "idx_deliveries_status"},
-		{&database.Delivery{}, "idx_deliveries_event_id"},
-		{&database.DeliveryResult{}, "idx_delivery_results_delivery_id"},
-		{&database.Event{}, "idx_events_deleted_at_created_at"},
-		{&database.Event{}, "idx_events_created_at"},
+	// The columns the background work reads by: the recovery and sweep
+	// queries (status), the event log (event_id and delivery_id) and
+	// retention (created_at).
+	eventTierIndexes := []indexedColumn{
+		{&database.Delivery{}, "Status"},
+		{&database.Delivery{}, "EventID"},
+		{&database.DeliveryResult{}, "DeliveryID"},
+		{&database.Event{}, "CreatedAt"},
 	}
 
 	mgr, lc := setupTestWebhookDBManager(t)
@@ -43,14 +47,14 @@ func TestWebhookDBManager_OpenAddsEventTierIndexes(t *testing.T) {
 	require.NoError(t, err)
 
 	// A fresh database has them.
-	for _, ix := range indexes {
-		require.True(t, db.Migrator().HasIndex(ix.model, ix.name))
+	for _, ix := range eventTierIndexes {
+		require.True(t, db.Migrator().HasIndex(ix.model, ix.field))
 	}
 
 	// Stand in for a database file created before the indexes existed.
-	for _, ix := range indexes {
-		require.NoError(t, db.Migrator().DropIndex(ix.model, ix.name))
-		require.False(t, db.Migrator().HasIndex(ix.model, ix.name))
+	for _, ix := range eventTierIndexes {
+		require.NoError(t, db.Migrator().DropIndex(ix.model, ix.field))
+		require.False(t, db.Migrator().HasIndex(ix.model, ix.field))
 	}
 
 	// Drop the cached connection so the next open reopens the file and
@@ -60,107 +64,9 @@ func TestWebhookDBManager_OpenAddsEventTierIndexes(t *testing.T) {
 	db, err = mgr.GetDB(webhookID)
 	require.NoError(t, err)
 
-	for _, ix := range indexes {
-		assert.True(t, db.Migrator().HasIndex(ix.model, ix.name),
-			"opening the existing database should create %s", ix.name)
-	}
-}
-
-// TestEventTierQueriesUseTheirIndexes verifies that the statements the
-// indexes are for use them. GORM builds each statement in a dry run as
-// the code named above it does, soft-delete condition included, and
-// SQLite, which keeps no statistics on these tables, must plan to seek
-// on each index listed by the columns in parentheses.
-func TestEventTierQueriesUseTheirIndexes(t *testing.T) {
-	t.Parallel()
-
-	mgr, lc := setupTestWebhookDBManager(t)
-	ctx := context.Background()
-	require.NoError(t, lc.Start(ctx))
-
-	defer func() { require.NoError(t, lc.Stop(ctx)) }()
-
-	db, err := mgr.GetDB(uuid.New().String())
-	require.NoError(t, err)
-
-	dry := db.Session(&gorm.Session{DryRun: true})
-	ids := []string{
-		uuid.New().String(), uuid.New().String(), uuid.New().String(),
-	}
-	cutoff := time.Now()
-
-	var (
-		deliveries []database.Delivery
-		results    []database.DeliveryResult
-		depths     []struct{ Depth int }
-	)
-
-	byStatus := "idx_deliveries_status (status=? AND deleted_at=?)"
-	byEvent := "idx_deliveries_event_id (event_id=? AND deleted_at=?)"
-	byAge := "idx_events_deleted_at_created_at (deleted_at=? AND created_at<?)"
-
-	// The delivery engine: recovery and the retry sweep, the sweep for
-	// stranded pending deliveries, and the queue depth count.
-	assertPlanUses(t, db, dry.Where(
-		"status = ?", database.DeliveryStatusRetrying,
-	).Find(&deliveries), byStatus)
-	assertPlanUses(t, db, dry.Where(
-		"status = ? AND updated_at < ?",
-		database.DeliveryStatusPending, cutoff,
-	).Limit(500).Find(&deliveries), byStatus)
-	assertPlanUses(t, db, dry.Model(&database.Delivery{}).
-		Select("target_id", "status", "count(*) as depth").
-		Where("status IN ?", []database.DeliveryStatus{
-			database.DeliveryStatusPending,
-			database.DeliveryStatusRetrying,
-		}).Group("target_id, status").Find(&depths), byStatus)
-
-	// The event log: each event's deliveries, then their attempts
-	// (loadEventsWithDeliveries, loadDeliveryResults).
-	assertPlanUses(t, db, dry.Where("event_id = ?", ids[0]).
-		Find(&deliveries), byEvent)
-	assertPlanUses(t, db, dry.Where("delivery_id IN ?", ids).
-		Order("attempt_num ASC").Find(&results),
-		"idx_delivery_results_delivery_id (delivery_id=? AND deleted_at=?)")
-
-	// Retention's three deletes (reapExpired), whose subqueries are built
-	// afresh for each statement as it builds them.
-	expiredEventIDs := func() *gorm.DB {
-		return dry.Model(&database.Event{}).Select("id").
-			Where("created_at < ?", cutoff)
-	}
-
-	assertPlanUses(t, db, dry.Unscoped().Where(
-		"delivery_id IN (?)", dry.Model(&database.Delivery{}).
-			Select("id").Where("event_id IN (?)", expiredEventIDs()),
-	).Delete(&database.DeliveryResult{}),
-		"idx_delivery_results_delivery_id (delivery_id=?)", byEvent, byAge)
-	assertPlanUses(t, db, dry.Unscoped().Where(
-		"event_id IN (?)", expiredEventIDs(),
-	).Delete(&database.Delivery{}),
-		"idx_deliveries_event_id (event_id=?)", byAge)
-	assertPlanUses(t, db, dry.Unscoped().Where(
-		"created_at < ?", cutoff,
-	).Delete(&database.Event{}), "idx_events_created_at (created_at<?)")
-}
-
-// assertPlanUses asserts that SQLite's plan for a statement GORM built
-// in a dry run, run with the same SQL and arguments GORM would send,
-// names each of the given indexes.
-func assertPlanUses(
-	t *testing.T, db, built *gorm.DB, indexes ...string,
-) {
-	t.Helper()
-
-	var plan []struct{ Detail string }
-
-	require.NoError(t, db.Raw(
-		"EXPLAIN QUERY PLAN "+built.Statement.SQL.String(),
-		built.Statement.Vars...,
-	).Scan(&plan).Error)
-
-	for _, index := range indexes {
-		assert.Contains(t, fmt.Sprint(plan), index,
-			built.Statement.SQL.String())
+	for _, ix := range eventTierIndexes {
+		assert.True(t, db.Migrator().HasIndex(ix.model, ix.field),
+			"opening the existing database should create the index on %s",
+			ix.field)
 	}
 }

internal/database/export_test.go

@@ -5,7 +5,6 @@ import (
 	"io"
 	"log/slog"
 	"os"
-	"testing"
 	"time"
 
 	"go.uber.org/fx"
@@ -80,14 +79,3 @@ func (d *Database) ExportSetBannerOut(w io.Writer) {
 func DummyPasswordHashForTest() string {
 	return dummyPasswordHash()
 }
-
-// HashAtShippedCostForTest makes HashPassword hash at the shipped
-// memory cost until t ends. t must not run in parallel with other
-// tests, which would hash at that cost alongside it.
-func HashAtShippedCostForTest(t *testing.T) {
-	t.Helper()
-
-	hashAtShippedCostInTest = true
-
-	t.Cleanup(func() { hashAtShippedCostInTest = false })
-}

internal/database/model_delivery.go

@@ -1,7 +1,5 @@
 package database
 
-import "gorm.io/gorm"
-
 // DeliveryStatus represents the status of a delivery
 type DeliveryStatus string
 
@@ -31,19 +29,12 @@ func (s DeliveryStatus) Terminal() bool {
 }
 
 // Delivery represents a delivery attempt for an event to a target
-//
-//nolint:lll // a struct tag cannot wrap
 type Delivery struct {
 	BaseModel
 
-	EventID  string         `gorm:"type:uuid;not null;index:idx_deliveries_event_id,priority:1"       json:"eventId"`
+	EventID  string         `gorm:"type:uuid;not null;index"         json:"eventId"`
 	TargetID string         `gorm:"type:uuid;not null"               json:"targetId"`
-	Status   DeliveryStatus `gorm:"not null;default:'pending';index:idx_deliveries_status,priority:1" json:"status"`
-
-	// DeletedAt repeats the BaseModel field only to be the second column
-	// of the event_id and status indexes, for the reason DeliveryResult
-	// gives.
-	DeletedAt gorm.DeletedAt `gorm:"index:idx_deliveries_event_id,priority:2;index:idx_deliveries_status,priority:2" json:"deletedAt,omitzero"`
+	Status   DeliveryStatus `gorm:"not null;default:'pending';index" json:"status"`
 
 	// Relations
 	Event           Event            `json:"event,omitzero"`

internal/database/model_delivery_result.go

@@ -1,21 +1,10 @@
 package database
 
-import "gorm.io/gorm"
-
 // DeliveryResult represents the result of a delivery attempt
-//
-//nolint:lll // a struct tag cannot wrap
 type DeliveryResult struct {
 	BaseModel
 
-	// DeliveryID and DeletedAt make up one index, in that order.
-	// DeletedAt repeats the BaseModel field only to join it: GORM adds
-	// "deleted_at IS NULL" to almost every query, and where a column is
-	// matched against several values SQLite otherwise reads through the
-	// deleted_at index, which every live row matches.
-	DeliveryID string         `gorm:"type:uuid;not null;index:idx_delivery_results_delivery_id,priority:1" json:"deliveryId"`
-	DeletedAt  gorm.DeletedAt `gorm:"index:idx_delivery_results_delivery_id,priority:2"                    json:"deletedAt,omitzero"`
-
+	DeliveryID   string `gorm:"type:uuid;not null;index" json:"deliveryId"`
 	AttemptNum   int    `gorm:"not null"                 json:"attemptNum"`
 	Success      bool   `json:"success"`
 	StatusCode   int    `json:"statusCode,omitempty"`

internal/database/model_event.go

@@ -1,26 +1,16 @@
 package database
 
-import (
-	"time"
-
-	"gorm.io/gorm"
-)
+import "time"
 
 // Event represents a captured webhook event
-//
-//nolint:lll // a struct tag cannot wrap
 type Event struct {
 	BaseModel
 
-	// CreatedAt and DeletedAt repeat the BaseModel fields only to index
-	// them for retention, which finds events by age. Its lookups carry
-	// GORM's "deleted_at IS NULL" (see DeliveryResult) and compare
-	// created_at with <, so their index has deleted_at first: SQLite
-	// narrows by a < only on the last column it uses. Its final delete
-	// has no deleted_at condition and uses the index on created_at
-	// alone. The other tables keep the unindexed BaseModel created_at.
-	CreatedAt time.Time      `gorm:"index;index:idx_events_deleted_at_created_at,priority:2" json:"createdAt"`
-	DeletedAt gorm.DeletedAt `gorm:"index:idx_events_deleted_at_created_at,priority:1"       json:"deletedAt,omitzero"`
+	// CreatedAt overrides BaseModel.CreatedAt only to add an index:
+	// retention deletes events by age, so events.created_at is queried
+	// on every sweep. The other tables keep the unindexed BaseModel
+	// field.
+	CreatedAt time.Time `gorm:"index" json:"createdAt"`
 
 	WebhookID    string `gorm:"type:uuid;not null" json:"webhookId"`
 	EntrypointID string `gorm:"type:uuid;not null" json:"entrypointId"`

internal/database/password.go

@@ -9,7 +9,6 @@ import (
 	"math/big"
 	"strings"
 	"sync"
-	"testing"
 
 	"golang.org/x/crypto/argon2"
 )
@@ -64,30 +63,10 @@ func DefaultPasswordConfig() *PasswordConfig {
 	}
 }
 
-// testArgon2Memory is the Argon2id memory cost, in KiB, that a test
-// binary hashes with: 1 MB instead of the shipped 64 MB. Every test
-// that starts a database hashes the bootstrap admin password, dozens
-// of them run in parallel, and under the race detector each 64 MB hash
-// holds about 150 MB. VerifyPassword reads the cost from the hash it
-// checks, so verification follows.
-const testArgon2Memory = 1024
-
-// hashAtShippedCostInTest makes a test binary hash at the shipped
-// memory cost. Only TestHashPassword_ShippedParameters sets it.
-//
-//nolint:gochecknoglobals // set by one test, see above
-var hashAtShippedCostInTest bool
-
-// HashPassword generates an Argon2id hash of the password. A binary
-// built by go test hashes at testArgon2Memory; one built by go build
-// always hashes at the defaults.
+// HashPassword generates an Argon2id hash of the password
 func HashPassword(password string) (string, error) {
 	config := DefaultPasswordConfig()
 
-	if testing.Testing() && !hashAtShippedCostInTest {
-		config.Memory = testArgon2Memory
-	}
-
 	// Generate a salt
 	salt := make([]byte, config.SaltLen)
 

internal/database/password_test.go

@@ -192,39 +192,6 @@ func TestHashPasswordUniqueness(t *testing.T) {
 	}
 }
 
-// TestHashPassword_ShippedParameters hashes and verifies through
-// HashPassword at the shipped Argon2id parameters. Every other test
-// hashes at the lower memory cost a test binary uses, so this is the
-// one that keeps production hashing covered. One hash and one
-// verification: each costs 64 MB.
-//
-//nolint:paralleltest // changes the hashing cost for the whole binary
-func TestHashPassword_ShippedParameters(t *testing.T) {
-	database.HashAtShippedCostForTest(t)
-
-	password := "correct horse battery staple"
-
-	hash, err := database.HashPassword(password)
-	if err != nil {
-		t.Fatalf("hashing with the shipped parameters: %v", err)
-	}
-
-	const shipped = "$argon2id$v=19$m=65536,t=1,p=4$"
-
-	if !strings.HasPrefix(hash, shipped) {
-		t.Errorf("hash = %q, want prefix %q", hash, shipped)
-	}
-
-	valid, err := database.VerifyPassword(password, hash)
-	if err != nil {
-		t.Fatalf("VerifyPassword() error = %v", err)
-	}
-
-	if !valid {
-		t.Error("VerifyPassword() returned false for correct password")
-	}
-}
-
 // TestVerifyDummyPassword_DoesRealWork covers the anti-enumeration
 // path. Login charges an unknown username a verification against a
 // dummy hash so that a nonexistent account is not answered in

internal/database/webhook_db_manager.go

@@ -41,11 +41,6 @@ type WebhookDBManager struct {
 	dataDir string
 	dbs     sync.Map // map[webhookID]*gorm.DB
 	log     *slog.Logger
-
-	// mu is held while a database is opened, deleted, or closed, so
-	// each file has at most one open handle. Reading an already cached
-	// handle does not take it.
-	mu sync.Mutex
 }
 
 // NewWebhookDBManager creates a new WebhookDBManager and
@@ -91,34 +86,8 @@ func (m *WebhookDBManager) GetDB(
 ) (*gorm.DB, error) {
 	// Fast path: already open
 	if val, ok := m.dbs.Load(webhookID); ok {
-		return asGormDB(val, webhookID)
-	}
-
-	// Slow path: open the database under the lock, looking in the
-	// cache again first. A caller that raced another one here then
-	// waits for its handle instead of opening a second one.
-	m.mu.Lock()
-	defer m.mu.Unlock()
-
-	if val, ok := m.dbs.Load(webhookID); ok {
-		return asGormDB(val, webhookID)
-	}
-
-	db, err := m.openDB(webhookID)
-	if err != nil {
-		return nil, err
-	}
-
-	m.dbs.Store(webhookID, db)
-
-	return db, nil
-}
-
-// asGormDB returns a value read from the cache as the database
-// handle it is.
-func asGormDB(val any, webhookID string) (*gorm.DB, error) {
-	db, ok := val.(*gorm.DB)
-	if !ok {
+		cachedDB, castOK := val.(*gorm.DB)
+		if !castOK {
 			return nil, fmt.Errorf(
 				"%w for webhook %s",
 				errInvalidCachedDBType,
@@ -126,6 +95,36 @@ func asGormDB(val any, webhookID string) (*gorm.DB, error) {
 			)
 		}
 
+		return cachedDB, nil
+	}
+
+	// Slow path: open/create the database
+	db, err := m.openDB(webhookID)
+	if err != nil {
+		return nil, err
+	}
+
+	// Store it; if another goroutine beat us, close ours
+	actual, loaded := m.dbs.LoadOrStore(webhookID, db)
+	if loaded {
+		// Another goroutine created it first; close our duplicate
+		sqlDB, closeErr := db.DB()
+		if closeErr == nil {
+			_ = sqlDB.Close()
+		}
+
+		existingDB, castOK := actual.(*gorm.DB)
+		if !castOK {
+			return nil, fmt.Errorf(
+				"%w for webhook %s",
+				errInvalidCachedDBType,
+				webhookID,
+			)
+		}
+
+		return existingDB, nil
+	}
+
 	return db, nil
 }
 
@@ -154,11 +153,6 @@ func (m *WebhookDBManager) DBExists(
 func (m *WebhookDBManager) DeleteDB(
 	webhookID string,
 ) error {
-	// Held until the files are gone, so GetDB cannot open the file
-	// again between the close and the removal.
-	m.mu.Lock()
-	defer m.mu.Unlock()
-
 	// Close and remove from cache
 	if val, ok := m.dbs.LoadAndDelete(webhookID); ok {
 		if gormDB, castOK := val.(*gorm.DB); castOK {
@@ -192,11 +186,6 @@ func (m *WebhookDBManager) DeleteDB(
 // CloseAll closes all open per-webhook database connections.
 // Called during application shutdown.
 func (m *WebhookDBManager) CloseAll() error {
-	// An open already under way finishes and is cached first, so it
-	// is closed here rather than cached after this loop has passed.
-	m.mu.Lock()
-	defer m.mu.Unlock()
-
 	var lastErr error
 
 	m.dbs.Range(func(key, value any) bool {

internal/database/webhook_db_manager_test.go

@@ -1,14 +1,10 @@
 package database_test
 
 import (
-	"bytes"
 	"context"
-	"log/slog"
 	"net/http"
 	"os"
 	"path/filepath"
-	"strings"
-	"sync"
 	"testing"
 
 	"github.com/google/uuid"
@@ -108,54 +104,6 @@ func TestWebhookDBManager_CreateAndGetDB(t *testing.T) {
 	assert.Equal(t, `{"test": true}`, readEvent.Body)
 }
 
-// Many callers ask for one webhook's database at the same moment,
-// before it is cached. Only one of them may open the file; the others
-// must wait for its handle. openDB logs one "opened per-webhook
-// database" line per open, and those lines are what is counted.
-func TestWebhookDBManager_ConcurrentFirstTouchOpensOnce(t *testing.T) {
-	t.Parallel()
-
-	var logs bytes.Buffer
-
-	mgr := database.NewTestWebhookDBManagerWithLogger(
-		t.TempDir(),
-		slog.New(slog.NewTextHandler(&logs, nil)),
-	)
-
-	t.Cleanup(func() { assert.NoError(t, mgr.CloseAll()) })
-
-	webhookID := uuid.New().String()
-
-	const callers = 16
-
-	start := make(chan struct{})
-	handles := make([]*gorm.DB, callers)
-	errs := make([]error, callers)
-
-	var wg sync.WaitGroup
-
-	for i := range callers {
-		wg.Go(func() {
-			<-start
-
-			handles[i], errs[i] = mgr.GetDB(webhookID)
-		})
-	}
-
-	close(start)
-	wg.Wait()
-
-	for i := range callers {
-		require.NoError(t, errs[i])
-		assert.Same(t, handles[0], handles[i])
-	}
-
-	assert.Equal(
-		t, 1,
-		strings.Count(logs.String(), "opened per-webhook database"),
-	)
-}
-
 func TestWebhookDBManager_DeleteDB(t *testing.T) {
 	t.Parallel()
 

internal/delivery/circuit_breaker.go

@@ -76,20 +76,12 @@ func (cb *CircuitBreaker) Allow() bool {
 	}
 }
 
-// CooldownRemaining returns how long a delivery that Allow refused
-// should wait before it is tried again. Closed, it returns zero.
-// Open, it returns what is left of the cooldown, or zero once that
-// has passed. Half-open, it returns the whole cooldown: the one
-// probe delivery is still in flight, and if it fails the circuit
-// reopens for that long.
+// CooldownRemaining returns how much time is left before
+// an open circuit transitions to half-open.
 func (cb *CircuitBreaker) CooldownRemaining() time.Duration {
 	cb.mu.Lock()
 	defer cb.mu.Unlock()
 
-	if cb.state == CircuitHalfOpen {
-		return cb.cooldown
-	}
-
 	if cb.state != CircuitOpen {
 		return 0
 	}

internal/delivery/circuit_breaker_test.go

@@ -267,7 +267,7 @@ func TestCircuitBreaker_CooldownRemaining_ClosedReturnsZero(
 	)
 }
 
-func TestCircuitBreaker_CooldownRemaining_HalfOpenReturnsCooldown(
+func TestCircuitBreaker_CooldownRemaining_HalfOpenReturnsZero(
 	t *testing.T,
 ) {
 	t.Parallel()
@@ -282,11 +282,9 @@ func TestCircuitBreaker_CooldownRemaining_HalfOpenReturnsCooldown(
 
 	require.True(t, cb.Allow())
 
-	// The cooldown newShortCooldownCB gives the breaker.
-	assert.Equal(t, 50*time.Millisecond,
+	assert.Equal(t, time.Duration(0),
 		cb.CooldownRemaining(),
-		"a delivery refused while half-open should wait "+
-			"a whole cooldown",
+		"half-open circuit should have zero cooldown remaining",
 	)
 }
 

internal/delivery/engine.go

@@ -362,15 +362,6 @@ func (e *Engine) start() {
 // stop cancels the worker pool's context and waits for the pool
 // to drain, bounded by the stop hook's context: a wedged worker
 // must not hang the process past fx's stop timeout.
-//
-// Once the pool has drained it closes the archive writers, so a
-// clean stop leaves no archive -wal behind. Nothing else holds a
-// writer for long by then: the archive sweeper stops before the
-// engine, and deleting a webhook only closes one. If the pool did
-// not drain in time, the writers are left open, as a kill would
-// leave them. Closing them would wait for any write in progress,
-// and a worker still running would then open new writers that
-// nothing closes, so it gains nothing over a kill.
 func (e *Engine) stop(ctx context.Context) error {
 	e.log.Info("delivery engine stopping")
 
@@ -385,8 +376,6 @@ func (e *Engine) stop(ctx context.Context) error {
 		return err
 	}
 
-	e.dbTarget.evictAll()
-
 	e.log.Info("delivery engine stopped")
 
 	return nil
@@ -449,31 +438,6 @@ func (e *Engine) processNewTask(
 		return
 	}
 
-	// Restart recovery can send and release this delivery before the
-	// receiver's Notify queues it. Ownership cannot refuse a delivery
-	// nobody holds, so the row decides whether it still needs sending.
-	row, err := e.loadDelivery(webhookDB, task.DeliveryID)
-	if err != nil {
-		e.log.Error(
-			"failed to load delivery",
-			"delivery_id", task.DeliveryID,
-			"error", err,
-		)
-
-		return
-	}
-
-	if row.Status != database.DeliveryStatusPending {
-		e.log.Info(
-			"delivery already handled, not sent again",
-			"delivery_id", task.DeliveryID,
-			"event_id", task.EventID,
-			"status", row.Status,
-		)
-
-		return
-	}
-
 	event := buildEventFromTask(task)
 
 	event, err = e.hydrateEvent(
@@ -518,7 +482,7 @@ func (e *Engine) processRetryTask(
 		return
 	}
 
-	d, err := e.loadDelivery(
+	d, err := e.loadRetryDelivery(
 		webhookDB, task.DeliveryID,
 	)
 	if err != nil {
@@ -739,7 +703,9 @@ func (e *Engine) recoverSingleRetry(
 		// webhook on one bad read would be a far larger fault than
 		// the strand it is meant to clear.
 		if errors.Is(err, gorm.ErrRecordNotFound) {
-			e.failMissingTarget(webhookDB, webhookID, d)
+			e.failMissingTargetRetry(
+				webhookDB, webhookID, d,
+			)
 
 			return
 		}
@@ -1142,7 +1108,9 @@ func (e *Engine) sweepSingleRetry(
 		// Deleted is terminal, unreadable is not; see
 		// recoverSingleRetry.
 		if errors.Is(err, gorm.ErrRecordNotFound) {
-			e.failMissingTarget(webhookDB, webhookID, d)
+			e.failMissingTargetRetry(
+				webhookDB, webhookID, d,
+			)
 
 			return
 		}
@@ -1256,19 +1224,19 @@ func (e *Engine) failUnretryableRetry(
 	e.failDelivery(webhookDB, d, target.Type, reason)
 }
 
-// failMissingTarget terminally fails a recovered delivery, pending or
-// retrying, whose target row is gone. Restart recovery and the periodic
-// sweep call it for both statuses, so the transition exists once.
+// failMissingTargetRetry terminally fails an orphaned retrying
+// delivery whose target row is gone. Both restart recovery and the
+// periodic sweep call it, so the transition exists once.
 //
-// Until it existed those paths logged the failed lookup and moved on,
-// which left the delivery where it was for the life of the database and
+// Until it existed both paths logged the failed lookup and returned,
+// which left the delivery retrying for the life of the database and
 // the sweep repeating the same error every minute forever. Failing it
 // with a recorded reason is the treatment the other orphaned-retry
 // cases already get, so all of them read alike in the event log.
 //
 // Logged at warn rather than error: a deleted target is an operator
 // action, not a system fault.
-func (e *Engine) failMissingTarget(
+func (e *Engine) failMissingTargetRetry(
 	webhookDB *gorm.DB,
 	webhookID string,
 	d *database.Delivery,
@@ -1281,37 +1249,13 @@ func (e *Engine) failMissingTarget(
 
 	defer e.inflight.release(d.ID)
 
-	// The batch was read before ownership was taken, and a worker may
-	// have settled the delivery and let it go in between. Only a row
-	// still in the status the batch read is failed.
-	row, err := e.loadDelivery(webhookDB, d.ID)
-	if err != nil {
-		e.log.Error(
-			"failed to load delivery",
-			"delivery_id", d.ID,
-			"error", err,
-		)
-
-		return
-	}
-
-	if row.Status != d.Status {
-		e.log.Debug(
-			"delivery already handled, not failed",
-			"delivery_id", d.ID,
-			"status", row.Status,
-		)
-
-		return
-	}
-
 	targetType, reason := e.missingTargetReason(d.TargetID)
 
 	e.log.Warn(
-		"failing recovered delivery: its target no longer exists",
+		"failing orphaned retrying delivery: "+
+			"its target no longer exists",
 		"webhook_id", webhookID,
 		"delivery_id", d.ID,
-		"status", d.Status,
 		"target_id", d.TargetID,
 		"target_type", targetType,
 	)
@@ -1345,14 +1289,15 @@ func (e *Engine) missingTargetReason(
 	if err != nil {
 		return "", fmt.Sprintf(
 			"target %s no longer exists; the delivery "+
-				"has been failed terminally",
+				"cannot be retried and has been failed "+
+				"terminally",
 			targetID,
 		)
 	}
 
 	return target.Type, fmt.Sprintf(
 		"target %q (type %s) was deleted; the delivery "+
-			"has been failed terminally",
+			"cannot be retried and has been failed terminally",
 		target.Name, target.Type,
 	)
 }
@@ -1698,7 +1643,7 @@ func (e *Engine) hydrateEvent(
 	return event, nil
 }
 
-func (e *Engine) loadDelivery(
+func (e *Engine) loadRetryDelivery(
 	webhookDB *gorm.DB, deliveryID string,
 ) (*database.Delivery, error) {
 	var d database.Delivery
@@ -2051,31 +1996,14 @@ func (e *Engine) sendRecoveredDeliveries(
 
 		target, ok := targetMap[deliveries[i].TargetID]
 		if !ok {
-			// A missing entry does not mean the target is gone: the
-			// map is also empty when its query failed. Only a lookup
-			// that finds no row ends the delivery; any other error
-			// leaves it pending for the next sweep. See
-			// recoverSingleRetry.
-			var err error
-
-			target, err = e.loadTarget(deliveries[i].TargetID)
-			if errors.Is(err, gorm.ErrRecordNotFound) {
-				e.failMissingTarget(webhookDB, webhookID, &deliveries[i])
-
-				continue
-			}
-
-			if err != nil {
 			e.log.Error(
-					"failed to load target for recovered delivery",
+				"target not found for delivery",
 				"delivery_id", deliveries[i].ID,
 				"target_id", deliveries[i].TargetID,
-					"error", err,
 			)
 
 			continue
 		}
-		}
 
 		if !e.takeForRedispatch(
 			webhookDB, deliveries[i].ID,

internal/delivery/engine_lifecycle_test.go

@@ -2,8 +2,6 @@ package delivery_test
 
 import (
 	"context"
-	"fmt"
-	"path/filepath"
 	"testing"
 	"time"
 
@@ -271,88 +269,3 @@ func TestEngine_StopHookHonoursStopTimeout(t *testing.T) {
 
 	requireStopHookExpires(t, lc.hooks[0], "delivery engine")
 }
-
-// deliverToArchive runs one delivery to a database target through
-// the running engine and returns the webhook's archive file path.
-// The archive writer holds the file open afterwards.
-func deliverToArchive(t *testing.T, s iSetup) string {
-	t.Helper()
-
-	deliveryID, task := seedLogTask(t, s)
-	task.TargetType = database.TargetTypeDatabase
-
-	s.Engine.Notify([]delivery.Task{task})
-
-	iWaitForDelivered(t, s.WebhookDB, deliveryID)
-
-	return filepath.Join(
-		filepath.Dir(s.DBMgr.DBPath(s.WebhookID)),
-		fmt.Sprintf("archive-%s.db", s.WebhookID),
-	)
-}
-
-// TestEngine_StopHookClosesArchives is the regression test for an
-// archive split across two files by a clean stop. The engine never
-// closed its archive writers, so after a stop the archived rows
-// could sit in archive-{id}.db-wal while archive-{id}.db held no
-// table at all, and copying the .db on its own gave an empty
-// database.
-func TestEngine_StopHookClosesArchives(t *testing.T) {
-	t.Parallel()
-
-	s := newISetup(t)
-
-	lc := startEngineViaHook(t, s.Engine)
-
-	path := deliverToArchive(t, s)
-	require.FileExists(
-		t, path+"-wal",
-		"an open archive should have a -wal for the stop to remove",
-	)
-
-	require.NoError(t, lc.hooks[0].OnStop(context.Background()))
-
-	wals, err := filepath.Glob(
-		filepath.Join(filepath.Dir(path), "archive-*.db-wal"),
-	)
-	require.NoError(t, err)
-	require.Empty(
-		t, wals, "a clean stop must leave no archive -wal behind",
-	)
-
-	// With no -wal beside it, the row can only be in the .db.
-	count, err := countArchivedRows(path)
-	require.NoError(t, err)
-	require.Equal(t, int64(1), count)
-}
-
-// TestEngine_StopHookTimeoutLeavesArchivesOpen covers a stop whose
-// budget runs out while a worker is still running. The archive
-// writers are left open, as a kill would leave them: closing them
-// would wait for any write in progress, and that worker would then
-// open new writers that nothing closes.
-func TestEngine_StopHookTimeoutLeavesArchivesOpen(t *testing.T) {
-	t.Parallel()
-
-	s := newISetup(t)
-
-	lc := startEngineViaHook(t, s.Engine)
-
-	deliverToArchive(t, s)
-
-	release := make(chan struct{})
-
-	t.Cleanup(func() {
-		close(release)
-		s.Engine.EvictWebhook(s.WebhookID)
-	})
-
-	s.Engine.ExportWedgeWorker(release)
-
-	requireStopHookExpires(t, lc.hooks[0], "delivery engine")
-
-	require.True(
-		t, s.Engine.ExportArchiveHandleOpen(s.WebhookID),
-		"a stop that timed out must not close archive writers",
-	)
-}

internal/delivery/engine_test.go

@@ -17,7 +17,6 @@ import (
 	"time"
 
 	"github.com/google/uuid"
-	"github.com/prometheus/client_golang/prometheus"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gorm.io/driver/sqlite"
@@ -25,7 +24,6 @@ import (
 	_ "modernc.org/sqlite"
 	"sneak.berlin/go/webhooker/internal/database"
 	"sneak.berlin/go/webhooker/internal/delivery"
-	"sneak.berlin/go/webhooker/internal/metrics"
 )
 
 // testContentType is the event content type used in tests.
@@ -896,100 +894,6 @@ func TestDeliverHTTP_CircuitBreakerBlocks(t *testing.T) {
 	)
 }
 
-// recordingScheduler keeps the delay of every retry it is asked to
-// schedule, and schedules nothing.
-type recordingScheduler struct {
-	delays []time.Duration
-}
-
-func (s *recordingScheduler) ScheduleRetry(
-	_ delivery.Task, delay time.Duration,
-) {
-	s.delays = append(s.delays, delay)
-}
-
-// TestDeliverHTTP_HalfOpenBreakerDelaysQueuedTasks proves that while a
-// half-open breaker's one probe delivery is in flight, every other task
-// for the target is put back with a whole cooldown as its delay rather
-// than none, and that its status is written the first time the breaker
-// turns it away and not on each pass after that.
-func TestDeliverHTTP_HalfOpenBreakerDelaysQueuedTasks(t *testing.T) {
-	t.Parallel()
-
-	db := testWebhookDB(t)
-	e := testEngine(t, 1)
-
-	// Every write of retrying moves the retry counter, so on a registry
-	// this test owns the counter is the number of those writes.
-	reg := prometheus.NewRegistry()
-	e.ExportSetMetrics(metrics.New(reg))
-
-	targetID := uuid.New().String()
-	cb := newShortCooldownCB(t)
-	e.ExportSetCircuitBreaker(targetID, cb)
-
-	for range delivery.ExportDefaultFailureThreshold {
-		cb.RecordFailure()
-	}
-
-	time.Sleep(60 * time.Millisecond)
-
-	require.True(t, cb.Allow(), "the probe delivery should go through")
-	require.Equal(t, delivery.CircuitHalfOpen, cb.State())
-
-	cfg := newHTTPTargetConfig(
-		"http://will-not-be-called.invalid",
-	)
-	sched := &recordingScheduler{}
-
-	const queued, passes = 3, 4
-
-	for range queued {
-		event := seedEvent(t, db, `{"cb":"half-open"}`)
-		dlv := seedDelivery(
-			t, db, event.ID, targetID,
-			database.DeliveryStatusPending,
-		)
-
-		for range passes {
-			// Each pass starts from the stored row, as a retry does.
-			var row database.Delivery
-
-			require.NoError(t, db.First(
-				&row, "id = ?", dlv.ID,
-			).Error)
-
-			fix := buildHTTPFixture(
-				row, event, targetID,
-				"test-cb-half-open", cfg, 5, 1,
-			)
-
-			e.ExportDeliverHTTPWithScheduler(
-				context.TODO(), db, fix.Delivery, fix.Task, sched,
-			)
-		}
-
-		assertDeliveryStatus(t, db, dlv.ID,
-			database.DeliveryStatusRetrying,
-		)
-	}
-
-	require.Len(t, sched.delays, queued*passes)
-
-	for _, delay := range sched.delays {
-		// The cooldown newShortCooldownCB gives the breaker.
-		assert.Equal(t, 50*time.Millisecond, delay,
-			"a task turned away while half-open should wait "+
-				"a whole cooldown",
-		)
-	}
-
-	assert.InDelta(t, float64(queued),
-		mCounter(t, reg, mRetries, mTypeHTTP), 0,
-		"status should be written once per task, not once per pass",
-	)
-}
-
 func TestGetCircuitBreaker_CreatesOnDemand(t *testing.T) {
 	t.Parallel()
 
@@ -1166,10 +1070,6 @@ func TestIsForwardableHeader(t *testing.T) {
 	assert.False(t,
 		delivery.ExportIsForwardableHeader("Content-Length"),
 	)
-
-	assert.False(t,
-		delivery.ExportIsForwardableHeader("Content-Type"),
-	)
 }
 
 func TestTruncate(t *testing.T) {
@@ -1251,81 +1151,6 @@ func TestDoHTTPRequest_ForwardsHeaders(t *testing.T) {
 	)
 }
 
-// The event's stored inbound headers carry the same Content-Type the
-// receiver saved as the event's ContentType, so a delivery could send
-// it twice. It must go out exactly once, with a Content-Type configured
-// on the target winning, then the event's ContentType.
-func TestApplyRequestHeaders_SendsOneContentType(t *testing.T) {
-	t.Parallel()
-
-	cases := map[string]struct {
-		inbound    string
-		event      string
-		configured string
-		want       []string
-	}{
-		"inbound and event agree": {
-			inbound: testContentType,
-			event:   testContentType,
-			want:    []string{testContentType},
-		},
-		"inbound and event disagree": {
-			inbound: "text/plain",
-			event:   testContentType,
-			want:    []string{testContentType},
-		},
-		"event has none": {
-			inbound: testContentType,
-			want:    nil,
-		},
-		"target configures its own": {
-			inbound:    testContentType,
-			event:      testContentType,
-			configured: "application/xml",
-			want:       []string{"application/xml"},
-		},
-	}
-
-	for name, tc := range cases {
-		t.Run(name, func(t *testing.T) {
-			t.Parallel()
-
-			inbound, err := json.Marshal(map[string][]string{
-				headerContentType: {tc.inbound},
-			})
-			require.NoError(t, err)
-
-			cfg := &delivery.HTTPTargetConfig{}
-			if tc.configured != "" {
-				cfg.Headers = map[string]string{
-					headerContentType: tc.configured,
-				}
-			}
-
-			req, err := http.NewRequestWithContext(
-				context.Background(),
-				http.MethodPost,
-				"https://target.example.com/hook",
-				http.NoBody,
-			)
-			require.NoError(t, err)
-
-			delivery.ExportApplyRequestHeaders(
-				req,
-				&database.Event{
-					Headers:     string(inbound),
-					ContentType: tc.event,
-				},
-				cfg,
-			)
-
-			assert.Equal(t,
-				tc.want, req.Header.Values(headerContentType),
-			)
-		})
-	}
-}
-
 func TestProcessDelivery_RoutesToCorrectHandler(
 	t *testing.T,
 ) {

internal/delivery/export_test.go

@@ -101,19 +101,6 @@ func (e *Engine) ExportDeliverHTTP(
 	e.httpTarget.Deliver(ctx, webhookDB, d, task, e)
 }
 
-// ExportDeliverHTTPWithScheduler delivers via the http target, handing
-// any retry to sched instead of the engine, so a test can see the
-// delay each retry is given.
-func (e *Engine) ExportDeliverHTTPWithScheduler(
-	ctx context.Context,
-	webhookDB *gorm.DB,
-	d *database.Delivery,
-	task *Task,
-	sched Scheduler,
-) {
-	e.httpTarget.Deliver(ctx, webhookDB, d, task, sched)
-}
-
 // ExportDeliverDatabase delivers via the database target.
 func (e *Engine) ExportDeliverDatabase(
 	webhookDB *gorm.DB, d *database.Delivery,
@@ -192,14 +179,6 @@ func (e *Engine) ExportGetCircuitBreaker(
 	return e.httpTarget.getCircuitBreaker(targetID)
 }
 
-// ExportSetCircuitBreaker makes cb the http target's circuit breaker
-// for targetID, so a test can use one with a short cooldown.
-func (e *Engine) ExportSetCircuitBreaker(
-	targetID string, cb *CircuitBreaker,
-) {
-	e.httpTarget.circuitBreakers.Store(targetID, cb)
-}
-
 // ExportParseHTTPConfig exposes parseHTTPConfig.
 func (e *Engine) ExportParseHTTPConfig(
 	configJSON string,
@@ -342,31 +321,6 @@ func (e *Engine) ExportRecoverRetryingDeliveries(
 	e.recoverRetryingDeliveries(webhookDB, webhookID)
 }
 
-// ExportFailMissingTarget exposes failMissingTarget, so a test can hand
-// it a delivery as a batch read it earlier.
-func (e *Engine) ExportFailMissingTarget(
-	webhookDB *gorm.DB,
-	webhookID string,
-	d *database.Delivery,
-) {
-	e.failMissingTarget(webhookDB, webhookID, d)
-}
-
-// ExportSendRecoveredDeliveries exposes sendRecoveredDeliveries, so a
-// test can hand it a target map that lacks a delivery's target.
-func (e *Engine) ExportSendRecoveredDeliveries(
-	ctx context.Context,
-	webhookDB *gorm.DB,
-	deliveries []database.Delivery,
-	webhookID string,
-	targetMap map[string]database.Target,
-	settled map[string]struct{},
-) {
-	e.sendRecoveredDeliveries(
-		ctx, webhookDB, deliveries, webhookID, targetMap, settled,
-	)
-}
-
 // ExportDeliveryCh returns the delivery channel.
 func (e *Engine) ExportDeliveryCh() chan Task {
 	return e.deliveryCh

internal/delivery/inflight_test.go

@@ -273,73 +273,6 @@ func TestOwnershipIsReleasedAfterDelivery(t *testing.T) {
 	)
 }
 
-// TestNotifyAfterRecoveryDoesNotSendAgain is the startup race of
-// https://git.eeqj.de/sneak/webhooker/issues/299. The receiver has
-// written a delivery, restart recovery finds it pending, sends it and
-// releases it, and only then does the receiver's Notify for it arrive.
-// Nothing owns the delivery by then, so Notify takes it.
-func TestNotifyAfterRecoveryDoesNotSendAgain(t *testing.T) {
-	t.Parallel()
-
-	targetID := uuid.New().String()
-	s := fSweepSetup(t, targetID, "recovered")
-
-	event := iSeedEvent(
-		t, s.WebhookDB, s.WebhookID, `{"recovered":true}`,
-	)
-
-	d := iSeedDelivery(
-		t, s.WebhookDB, event.ID, targetID,
-		database.DeliveryStatusPending,
-	)
-
-	s.Engine.ExportStart()
-
-	defer func() {
-		require.NoError(
-			t, s.Engine.ExportStop(context.Background()),
-		)
-	}()
-
-	// Restart recovery sends the delivery and lets it go.
-	iWaitForDelivered(t, s.WebhookDB, d.ID)
-	require.Eventually(
-		t,
-		func() bool {
-			return s.Engine.ExportInflightHeld() == 0
-		},
-		5*time.Second, 20*time.Millisecond,
-	)
-
-	body := event.Body
-
-	s.Engine.Notify([]delivery.Task{{
-		DeliveryID:   d.ID,
-		EventID:      event.ID,
-		WebhookID:    s.WebhookID,
-		TargetID:     targetID,
-		TargetName:   "recovered",
-		TargetType:   database.TargetTypeLog,
-		Body:         &body,
-		EntrypointID: event.EntrypointID,
-	}})
-
-	// Notify took the delivery, and a worker releases it once it has
-	// run the task.
-	require.Eventually(
-		t,
-		func() bool {
-			return s.Engine.ExportInflightHeld() == 0
-		},
-		5*time.Second, 20*time.Millisecond,
-	)
-
-	assert.Len(
-		t, iResults(t, s.WebhookDB, d.ID), 1,
-		"the delivery was sent a second time",
-	)
-}
-
 // TestRetryingRecoverySkipsASuccessfulResult is the retrying-side twin
 // of the pending reconcile. A second attempt that reached the receiver
 // and whose status write then failed sits at retrying holding a

internal/delivery/metrics_test.go

@@ -412,10 +412,9 @@ func TestDeliveryMetrics_BreakerBlockedIsNotAnAttempt(
 
 	s.Engine.ExportProcessRetryTask(context.TODO(), &blocked)
 
-	// The breaker refused it: rescheduled without rewriting the
-	// retrying status it already had, so the retry counter did not
-	// move, and nothing was attempted or timed.
-	assert.InDelta(t, retriesBefore,
+	// The breaker refused it: rescheduled, so the retry counter
+	// moved, but nothing was attempted or timed.
+	assert.InDelta(t, retriesBefore+1,
 		mCounter(t, reg, mRetries, mTypeHTTP), 0)
 	assert.InDelta(t, threshold,
 		mCounter(t, reg, mAttempts, mTypeHTTP), 0)

internal/delivery/redirect_test.go

@@ -339,11 +339,10 @@ func TestRedirectPolicy_StopsAtHopCap(t *testing.T) {
 // The set the redirect policy strips is whatever the delivery path
 // actually put on the wire, so a header added to the forward set is
 // covered without a second edit. A header the event never carried
-// is not in the set, and neither is the inbound Content-Type, because
-// it is not forwarded. Two more are deliberately excluded: a
-// Content-Type configured on the target describes the body, which a
-// 307 carries across hosts, and the inbound User-Agent every real
-// sender supplies is overwritten before the request goes out.
+// is not in the set, and the delivery path's own two are deliberately
+// excluded: Content-Type describes the body, which a 307 carries
+// across hosts, and the inbound User-Agent every real sender supplies
+// is overwritten before the request goes out.
 func TestApplyRequestHeaders_ReportsOriginScopedNames(t *testing.T) {
 	t.Parallel()
 
@@ -372,7 +371,6 @@ func TestApplyRequestHeaders_ReportsOriginScopedNames(t *testing.T) {
 		&delivery.HTTPTargetConfig{
 			Headers: map[string]string{
 				probeHeaderName: probeHeaderValue,
-				"Content-Type":  testContentType,
 			},
 		},
 	)
@@ -380,11 +378,7 @@ func TestApplyRequestHeaders_ReportsOriginScopedNames(t *testing.T) {
 	assert.Equal(t,
 		[]string{probeHeaderName, inboundHeaderName}, names,
 		"both header classes are reported, and only those: "+
-			"Host and the inbound Content-Type are never "+
-			"forwarded, User-Agent is the delivery path's own",
-	)
-	assert.NotContains(t, names, "Content-Type",
-		"a Content-Type configured on the target must survive "+
-			"a cross-origin 307/308 with the body it describes",
+			"Host is never forwarded, Content-Type and "+
+			"User-Agent are the delivery path's own",
 	)
 }

internal/delivery/ssrf.go

@@ -26,7 +26,7 @@ var (
 		"hostname resolved to no IP addresses",
 	)
 	errBlockedIP = errors.New(
-		"blocked private, reserved or cloud metadata address",
+		"blocked private/reserved IP range",
 	)
 	errBlockedMetadata = errors.New(
 		"blocked link-local or cloud instance metadata " +
@@ -37,10 +37,9 @@ var (
 	)
 )
 
-// blockedNetworks is the default blocklist: the private and
-// reserved IP ranges, plus the public cloud metadata addresses,
-// that are blocked to prevent SSRF attacks. An operator can
-// permit specific blocks out of this set with
+// blockedNetworks contains all private/reserved IP ranges
+// that should be blocked to prevent SSRF attacks. An operator
+// can permit specific blocks out of this set with
 // ALLOWED_EGRESS_CIDRS; see Guard.
 //
 //nolint:gochecknoglobals // package-level network list is appropriate here
@@ -123,8 +122,6 @@ func init() {
 		"::1/128",
 		"fc00::/7",
 		"fe80::/10",
-		// Azure WireServer, a public address that serves VM credentials.
-		"168.63.129.16/32",
 	})
 
 	// Every entry is named. The set must not grow or shrink
@@ -219,8 +216,8 @@ func matchesAny(networks []*net.IPNet, ip net.IP) bool {
 }
 
 // isBlockedIP checks whether an IP address falls within
-// the default blocklist, before any operator allowlist is
-// considered.
+// any blocked private/reserved network range, before any
+// operator allowlist is considered.
 func isBlockedIP(ip net.IP) bool {
 	return matchesAny(blockedNetworks, ip)
 }
@@ -323,7 +320,7 @@ func (g *Guard) allows(ip net.IP) bool {
 //
 //  1. alwaysBlockedNetworks is refused before the allowlist is
 //     consulted, so no configured CIDR reaches link-local or a
-//     cloud metadata endpoint at a non-public address.
+//     cloud instance metadata endpoint.
 //  2. The allowlist is consulted next, so a listed private
 //     network becomes reachable.
 //  3. Everything else keeps the default blocklist's answer.

internal/delivery/ssrf_allowlist_test.go

@@ -390,41 +390,6 @@ func TestGuardAllowlist_PublicUnaffected(t *testing.T) {
 	}
 }
 
-// TestGuardAllowlist_AzureWireServerReopenable covers Azure's
-// WireServer, a public address that serves VM credentials. The
-// default guard refuses it, but because it is public it sits in
-// the default blocklist rather than the unconditional set, so an
-// operator who lists it can reach it.
-func TestGuardAllowlist_AzureWireServerReopenable(t *testing.T) {
-	t.Parallel()
-
-	const wireServerIP = "168.63.129.16"
-
-	target := "http://" + wireServerIP + "/?comp=versions"
-
-	defaultGuard := delivery.NewTestGuard()
-
-	err := defaultGuard.ValidateTargetURL(context.Background(), target)
-	require.Error(t, err,
-		"WireServer must be refused with no allowlist set",
-	)
-	assert.NotContains(t, err.Error(), metadataRefusalClause,
-		"WireServer must be refused by the default blocklist, "+
-			"which an allowlist can override",
-	)
-
-	assertDialRefused(t, defaultGuard, target)
-
-	listed := delivery.NewTestGuard(
-		netip.MustParsePrefix(wireServerIP + "/32"),
-	)
-
-	assert.NoError(t,
-		listed.ValidateTargetURL(context.Background(), target),
-		"an operator who lists WireServer must be able to reach it",
-	)
-}
-
 // TestGuardCheckIP_BothPathsShareOneDecision asserts that the
 // validator and the dialer are not two policies that happen to
 // agree: both are defined in terms of checkIP, so the exported

internal/delivery/target_database.go

@@ -277,24 +277,6 @@ func (t *databaseTarget) evict(webhookID string) {
 	)
 }
 
-// evictAll evicts every cached archive writer, exactly as evict
-// does for one webhook. The engine calls it at shutdown, once its
-// workers have returned. Closing the last handle on an archive
-// moves the contents of its -wal into the .db and removes the
-// -wal, so a clean stop leaves each archive as a single file.
-func (t *databaseTarget) evictAll() {
-	t.mu.Lock()
-
-	writers := t.writers
-	t.writers = nil
-
-	t.mu.Unlock()
-
-	for _, w := range writers {
-		w.evict()
-	}
-}
-
 // sweepWebhook prunes one webhook's archive of rows older than
 // expiry, without requiring a write. It returns nil (nothing to
 // do) when the archive file does not exist, so a sweep never

internal/delivery/target_database_evict_test.go

@@ -1,7 +1,6 @@
 package delivery_test
 
 import (
-	"context"
 	"errors"
 	"fmt"
 	"net/http"
@@ -362,46 +361,3 @@ func TestEvictWebhook_LaterDeliveryRecreatesWriter(t *testing.T) {
 		"a later delivery should recreate the writer",
 	)
 }
-
-// TestEngineStop_WriteAfterStopIsRefused proves the engine's stop
-// closes each archive writer the way deleting its webhook does: a
-// write that reaches a writer after the stop is refused, reopens
-// nothing and adds no row.
-func TestEngineStop_WriteAfterStopIsRefused(t *testing.T) {
-	t.Parallel()
-
-	eng, _ := evictTestEngine(t)
-
-	webhookDB := testWebhookDB(t)
-	event := seedEvent(t, webhookDB, `{"archived":true}`)
-	d := seedDatabaseTargetDelivery(t, webhookDB, event, "")
-
-	eng.ExportDeliverDatabase(webhookDB, d)
-
-	w := eng.ExportArchiveWriterFor(event.WebhookID)
-	require.NotNil(t, w)
-	require.True(t, w.HandleOpen())
-
-	require.NoError(t, eng.ExportStop(context.Background()))
-
-	err := w.Write(evictTestRow("ev-after-stop"), 0)
-
-	require.ErrorIs(
-		t, err, delivery.ErrExportArchiveWriterEvicted,
-		"a write after the stop must be refused",
-	)
-	assert.False(
-		t, w.HandleOpen(),
-		"a refused write must not reopen the archive",
-	)
-	assert.False(
-		t, eng.ExportHasArchiveWriter(event.WebhookID),
-		"the stop should empty the registry",
-	)
-
-	count, err := countArchivedRows(w.Path())
-	require.NoError(t, err)
-	assert.Equal(
-		t, int64(1), count, "the refused row must not be written",
-	)
-}

internal/delivery/target_headers_test.go

@@ -11,11 +11,10 @@ import (
 	"sneak.berlin/go/webhooker/internal/delivery"
 )
 
-// Literals these tests repeat, named so that the header names and the
+// Literals these tests repeat, named so that the header name and the
 // keep-forever archive config each have one definition.
 const (
 	headerAuthorization = "Authorization"
-	headerContentType   = "Content-Type"
 	bearerValue         = "Bearer abc"
 	archiveConfigNever  = "{\"expiry\":\"never\"}"
 )

internal/delivery/target_http.go

@@ -197,14 +197,10 @@ func (c *httpCore) circuitBreakerBlock(
 		"cooldown_remaining", remaining,
 	)
 
-	// A delivery already at retrying is left as it is, so a task
-	// the breaker keeps turning away writes nothing each time.
-	if d.Status != database.DeliveryStatusRetrying {
 	c.eng.settleStatus(
 		webhookDB, d, d.Target.Type,
 		database.DeliveryStatusRetrying,
 	)
-	}
 
 	retryTask := *task
 	sched.ScheduleRetry(retryTask, remaining)
@@ -541,11 +537,6 @@ func isForwardableHeader(name string) bool {
 		"Upgrade", "Proxy-Authorization",
 		"Proxy-Connection", "Content-Length":
 		return false
-	case "Content-Type":
-		// applyRequestHeaders sets Content-Type itself. The receiver
-		// already stored this inbound value as the event's
-		// ContentType, so forwarding it too would send it twice.
-		return false
 	default:
 		return true
 	}
@@ -558,10 +549,6 @@ func isForwardableHeader(name string) bool {
 // policy strips exactly that set on a hop that leaves the origin,
 // so the forward set is decided here and only here — a header added
 // to it is covered off-origin without a second edit elsewhere.
-//
-// Content-Type goes out once: a Content-Type configured on the target
-// wins, otherwise the event's ContentType, otherwise none. The inbound
-// Content-Type in the event's headers is never forwarded.
 func applyRequestHeaders(
 	req *http.Request,
 	event *database.Event,
@@ -582,10 +569,10 @@ func applyRequestHeaders(
 
 	req.Header.Set("User-Agent", "webhooker/1.0")
 
-	// A Content-Type configured on the target describes the body
-	// being sent rather than the sender. A 307/308 preserves the
-	// body across hosts, so stripping it would send that body
-	// untyped.
+	// Content-Type describes the body being sent rather than the
+	// sender, and the delivery path sets it from the event itself.
+	// A 307/308 preserves the body across hosts, so stripping it
+	// would send that body untyped.
 	delete(originScoped, "Content-Type")
 
 	// User-Agent is overwritten just above, so an inbound one never

internal/delivery/terminal_state_test.go

@@ -18,17 +18,16 @@ import (
 // https://git.eeqj.de/sneak/webhooker/issues/107: a delivery failed
 // with nothing in its event log to say why, and a retrying delivery
 // whose target was deleted, which used to keep sending and then never
-// terminalise. Section 4 is the same deleted-target gap for a pending
-// delivery: https://git.eeqj.de/sneak/webhooker/issues/293.
+// terminalise.
 
 // tUnknownType is a target type no build implements. It stands in for
 // a target whose type was written by a build that knew a type this one
 // does not.
 const tUnknownType = database.TargetType("pubsub")
 
-// tSeedDeletedTarget creates a target, a delivery against it at the
-// given status with one recorded failed attempt, and then deletes the
-// target the way the source page does.
+// tSeedDeletedTarget creates a target, a retrying delivery against it
+// with one recorded failed attempt, and then deletes the target the
+// way the source page does.
 //
 // It asserts the delete is soft, because that is the whole reason the
 // engine could not tell a deleted target from a target id that never
@@ -37,7 +36,6 @@ func tSeedDeletedTarget(
 	t *testing.T,
 	s iSetup,
 	name, url string,
-	status database.DeliveryStatus,
 ) string {
 	t.Helper()
 
@@ -53,7 +51,8 @@ func tSeedDeletedTarget(
 	)
 
 	d := iSeedDelivery(
-		t, s.WebhookDB, event.ID, targetID, status,
+		t, s.WebhookDB, event.ID, targetID,
+		database.DeliveryStatusRetrying,
 	)
 
 	iSeedFailedResult(t, s.WebhookDB, d.ID)
@@ -174,7 +173,6 @@ func TestRecoverSingleRetry_TargetDeleted(t *testing.T) {
 
 	deliveryID := tSeedDeletedTarget(
 		t, s, "gone-on-recovery", "http://example.com/hook",
-		database.DeliveryStatusRetrying,
 	)
 
 	s.Engine.ExportRecoverWebhookDeliveries(
@@ -212,7 +210,6 @@ func TestSweepSingleRetry_TargetDeleted(t *testing.T) {
 
 	deliveryID := tSeedDeletedTarget(
 		t, s, "gone-on-sweep", "http://example.com/hook",
-		database.DeliveryStatusRetrying,
 	)
 
 	// Twice, because the bug was an error the sweep repeated every
@@ -281,11 +278,11 @@ func TestSweepSingleRetry_TargetNeverExisted(t *testing.T) {
 	)
 }
 
-// TestFailMissingTarget_WritesNoTargetRow holds the new terminal path
-// to the same rule as the existing one: no target row, and so no
+// TestFailMissingTargetRetry_WritesNoTargetRow holds the new terminal
+// path to the same rule as the existing one: no target row, and so no
 // plaintext target config, may be written into the per-webhook event
 // database. See https://git.eeqj.de/sneak/webhooker/issues/206.
-func TestFailMissingTarget_WritesNoTargetRow(
+func TestFailMissingTargetRetry_WritesNoTargetRow(
 	t *testing.T,
 ) {
 	t.Parallel()
@@ -300,7 +297,6 @@ func TestFailMissingTarget_WritesNoTargetRow(
 
 	deliveryID := tSeedDeletedTarget(
 		t, s, "credential-bearing", hookURL,
-		database.DeliveryStatusRetrying,
 	)
 
 	s.Engine.ExportSweepWebhookRetries(
@@ -533,260 +529,3 @@ func TestRecoverSingleRetry_TargetUnreadable_LeavesDeliveryAlone(
 
 	assert.Zero(t, s.Engine.ExportInflightHeld())
 }
-
-// --- 4. A pending delivery whose target is gone ---
-
-func TestRecoverPending_TargetDeleted(t *testing.T) {
-	t.Parallel()
-
-	s := newISetup(t)
-
-	deliveryID := tSeedDeletedTarget(
-		t, s, "gone-while-pending", "http://example.com/hook",
-		database.DeliveryStatusPending,
-	)
-
-	s.Engine.ExportRecoverWebhookDeliveries(
-		context.Background(), s.WebhookID,
-	)
-
-	iAssertStatus(
-		t, s.WebhookDB, deliveryID,
-		database.DeliveryStatusFailed,
-	)
-
-	last := tLastResult(t, s, deliveryID, 2)
-
-	assert.False(t, last.Success)
-	assert.Equal(t, 2, last.AttemptNum)
-	assert.Contains(t, last.Error, "gone-while-pending")
-	assert.Contains(t, last.Error, "was deleted")
-
-	assert.Empty(t, fDrain(s.Engine),
-		"a delivery whose target is gone was sent",
-	)
-	assert.Zero(t, s.Engine.ExportInflightHeld(),
-		"the terminal path leaked its ownership reference",
-	)
-}
-
-// TestRecoverPending_TargetDeleted_LeavesAnOwnedDeliveryAlone: the
-// terminal write takes ownership like every other recovery write, so a
-// delivery the engine still holds is not failed underneath its worker.
-func TestRecoverPending_TargetDeleted_LeavesAnOwnedDeliveryAlone(
-	t *testing.T,
-) {
-	t.Parallel()
-
-	s := newISetup(t)
-
-	deliveryID := tSeedDeletedTarget(
-		t, s, "gone-but-owned", "http://example.com/hook",
-		database.DeliveryStatusPending,
-	)
-
-	require.True(t, s.Engine.ExportRetainDelivery(deliveryID))
-
-	s.Engine.ExportRecoverWebhookDeliveries(
-		context.Background(), s.WebhookID,
-	)
-
-	iAssertStatus(
-		t, s.WebhookDB, deliveryID,
-		database.DeliveryStatusPending,
-	)
-
-	assert.Len(t, iResults(t, s.WebhookDB, deliveryID), 1,
-		"a delivery the engine owns was failed underneath it",
-	)
-}
-
-// TestFailMissingTarget_LeavesASettledDeliveryAlone: the recovery paths
-// read their batch before taking ownership, and a worker may send a
-// delivery and let it go in between. The terminal write goes by the row
-// as it is now, not as the batch read it.
-func TestFailMissingTarget_LeavesASettledDeliveryAlone(
-	t *testing.T,
-) {
-	t.Parallel()
-
-	s := newISetup(t)
-
-	deliveryID := tSeedDeletedTarget(
-		t, s, "gone-after-sending", "http://example.com/hook",
-		database.DeliveryStatusPending,
-	)
-
-	var batch database.Delivery
-
-	require.NoError(t, s.WebhookDB.First(
-		&batch, "id = ?", deliveryID,
-	).Error)
-
-	// A worker settles the delivery after the batch was read.
-	require.NoError(t, s.WebhookDB.Model(&database.Delivery{}).
-		Where("id = ?", deliveryID).
-		Update("status", database.DeliveryStatusDelivered).Error)
-
-	s.Engine.ExportFailMissingTarget(
-		s.WebhookDB, s.WebhookID, &batch,
-	)
-
-	iAssertStatus(
-		t, s.WebhookDB, deliveryID,
-		database.DeliveryStatusDelivered,
-	)
-
-	assert.Len(t, iResults(t, s.WebhookDB, deliveryID), 1,
-		"a delivery settled after the batch read was then failed",
-	)
-	assert.Zero(t, s.Engine.ExportInflightHeld())
-}
-
-// TestSweepPending_TargetDeleted sweeps twice over a batch that also
-// holds a healthy stranded delivery. The one whose target is gone is
-// failed once and then left alone; the healthy one is queued by the
-// first sweep and not again by the second.
-func TestSweepPending_TargetDeleted(t *testing.T) {
-	t.Parallel()
-
-	liveTargetID := uuid.New().String()
-	s := fSweepSetup(t, liveTargetID, "still-there")
-
-	deliveryID := tSeedDeletedTarget(
-		t, s, "gone-on-pending-sweep", "http://example.com/hook",
-		database.DeliveryStatusPending,
-	)
-	rAgePending(t, s.WebhookDB, deliveryID)
-
-	event := iSeedEvent(
-		t, s.WebhookDB, s.WebhookID, `{"target":"live"}`,
-	)
-
-	healthy := iSeedDelivery(
-		t, s.WebhookDB, event.ID, liveTargetID,
-		database.DeliveryStatusPending,
-	)
-	rAgePending(t, s.WebhookDB, healthy.ID)
-
-	ctx := context.Background()
-
-	s.Engine.ExportSweepWebhookRetries(ctx, s.WebhookID)
-
-	tasks := fDrain(s.Engine)
-	require.Len(t, tasks, 1,
-		"the first sweep did not queue the healthy delivery",
-	)
-	assert.Equal(t, healthy.ID, tasks[0].DeliveryID)
-
-	s.Engine.ExportSweepWebhookRetries(ctx, s.WebhookID)
-
-	assert.Empty(t, fDrain(s.Engine),
-		"the second sweep queued a delivery again",
-	)
-
-	iAssertStatus(
-		t, s.WebhookDB, deliveryID,
-		database.DeliveryStatusFailed,
-	)
-
-	last := tLastResult(t, s, deliveryID, 2)
-
-	assert.Contains(t, last.Error, "gone-on-pending-sweep")
-	assert.Contains(t, last.Error, "was deleted")
-}
-
-// TestSendRecoveredDeliveries_TargetMissingFromMap: the batch's target
-// map is empty when its query failed, so every delivery in the batch is
-// looked up on its own. A healthy one is sent to the target that lookup
-// finds.
-func TestSendRecoveredDeliveries_TargetMissingFromMap(
-	t *testing.T,
-) {
-	t.Parallel()
-
-	s := newISetup(t)
-
-	targetID := uuid.New().String()
-
-	iCreateTarget(
-		t, s.MainDB, targetID, s.WebhookID, "found-on-lookup",
-		database.TargetTypeLog, "", 0,
-	)
-
-	event := iSeedEvent(
-		t, s.WebhookDB, s.WebhookID, `{"map":"empty"}`,
-	)
-
-	d := iSeedDelivery(
-		t, s.WebhookDB, event.ID, targetID,
-		database.DeliveryStatusPending,
-	)
-
-	s.Engine.ExportSendRecoveredDeliveries(
-		context.Background(), s.WebhookDB,
-		[]database.Delivery{d}, s.WebhookID,
-		map[string]database.Target{}, nil,
-	)
-
-	tasks := fDrain(s.Engine)
-	require.Len(t, tasks, 1,
-		"the healthy delivery was not queued exactly once",
-	)
-	assert.Equal(t, d.ID, tasks[0].DeliveryID)
-	assert.Equal(t, targetID, tasks[0].TargetID)
-	assert.Equal(t, database.TargetTypeLog, tasks[0].TargetType)
-
-	iAssertStatus(
-		t, s.WebhookDB, d.ID,
-		database.DeliveryStatusPending,
-	)
-}
-
-// TestRecoverPending_TargetUnreadable_LeavesDeliveryAlone: a failed
-// read of the main database is not a deleted target. Restart recovery
-// holds every pending delivery of the webhook in one batch, so failing
-// on this would fail all of them.
-func TestRecoverPending_TargetUnreadable_LeavesDeliveryAlone(
-	t *testing.T,
-) {
-	t.Parallel()
-
-	s := newISetup(t)
-
-	targetID := uuid.New().String()
-
-	iCreateTarget(
-		t, s.MainDB, targetID, s.WebhookID, "healthy",
-		database.TargetTypeLog, "", 0,
-	)
-
-	event := iSeedEvent(
-		t, s.WebhookDB, s.WebhookID, `{"still":"pending"}`,
-	)
-
-	d := iSeedDelivery(
-		t, s.WebhookDB, event.ID, targetID,
-		database.DeliveryStatusPending,
-	)
-
-	sqlDB, err := s.MainDB.DB()
-	require.NoError(t, err)
-	require.NoError(t, sqlDB.Close())
-
-	s.Engine.ExportRecoverPendingDeliveries(
-		context.Background(), s.WebhookDB, s.WebhookID,
-	)
-
-	iAssertStatus(
-		t, s.WebhookDB, d.ID,
-		database.DeliveryStatusPending,
-	)
-
-	assert.Empty(t, iResults(t, s.WebhookDB, d.ID),
-		"an unreadable main database produced a terminal "+
-			"failure row",
-	)
-	assert.Empty(t, fDrain(s.Engine))
-	assert.Zero(t, s.Engine.ExportInflightHeld())
-}

internal/middleware/csrf_test.go

@@ -380,8 +380,9 @@ func csrfTookStrictPath(
 
 // TestCSRF_ForwardedProtoSpellingsTakeStrictPath runs the header
 // spellings a real proxy emits through the middleware. The environment
-// is set to dev -- the permissive setting -- to pin that the routing is
-// a per-request transport decision and owes nothing to configuration.
+// is dev -- the DEFAULT when WEBHOOKER_ENVIRONMENT is unset -- to pin
+// that the routing is a per-request transport decision and owes
+// nothing to configuration.
 func TestCSRF_ForwardedProtoSpellingsTakeStrictPath(t *testing.T) {
 	t.Parallel()
 

internal/middleware/recoverer.go

@@ -133,11 +133,6 @@ func (w *recoverResponseWriter) Unwrap() http.ResponseWriter {
 // what the access log records and the metrics count, and outside the
 // sentryhttp handler, whose Repanic option depends on something
 // further out recovering what it re-raises.
-//
-// Unlike http.Error on its own, it deletes any Set-Cookie the handler
-// set before panicking, because a request that failed must not hand
-// the client a credential; every other header is left to http.Error.
-// See https://git.eeqj.de/sneak/webhooker/issues/193.
 func (s *Middleware) Recoverer() func(http.Handler) http.Handler {
 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(
@@ -169,8 +164,6 @@ func (s *Middleware) Recoverer() func(http.Handler) http.Handler {
 					return
 				}
 
-				rw.Header().Del("Set-Cookie")
-
 				http.Error(
 					rw,
 					http.StatusText(

internal/middleware/recoverer_test.go

@@ -304,44 +304,16 @@ func TestRecovererRepanicsErrAbortHandler(t *testing.T) {
 	)
 }
 
-// TestRecovererDropsSetCookieFromTheRecovered500 covers a handler that
-// sets a cookie and a redirect target and then panics before sending
-// anything. A request that failed must not hand the client a
-// credential, so the 500 carries no cookie; Location is left alone.
-func TestRecovererDropsSetCookieFromTheRecovered500(t *testing.T) {
-	t.Parallel()
-
-	probe := newRecovererProbe(
-		t, false,
-		func(w http.ResponseWriter, _ *http.Request) {
-			w.Header().Set("Set-Cookie", "session=x")
-			w.Header().Set("Location", "/after")
-
-			panic(panicMarker)
-		},
-	)
-
-	resp, err := probe.get(t)
-	require.NoError(t, err)
-	require.NoError(t, resp.Body.Close())
-
-	assert.Equal(t, http.StatusInternalServerError, resp.StatusCode)
-	assert.Empty(t, resp.Cookies())
-	assert.Equal(t, "/after", resp.Header.Get("Location"))
-}
-
 // TestRecovererKeepsAnAlreadyCommittedResponse covers a handler that
 // panics after sending its status. The bytes are already on the wire,
-// cookie included, so a second WriteHeader would change nothing the
-// client sees and would draw net/http's "superfluous
-// response.WriteHeader" report.
+// so a second WriteHeader would change nothing the client sees and
+// would draw net/http's "superfluous response.WriteHeader" report.
 func TestRecovererKeepsAnAlreadyCommittedResponse(t *testing.T) {
 	t.Parallel()
 
 	probe := newRecovererProbe(
 		t, false,
 		func(w http.ResponseWriter, _ *http.Request) {
-			w.Header().Set("Set-Cookie", "session=x")
 			w.WriteHeader(committedStatus)
 			_, _ = w.Write([]byte("partial"))
 
@@ -359,7 +331,6 @@ func TestRecovererKeepsAnAlreadyCommittedResponse(t *testing.T) {
 
 	assert.Equal(t, committedStatus, resp.StatusCode)
 	assert.Equal(t, "partial", string(body))
-	assert.Len(t, resp.Cookies(), 1)
 
 	record := probe.panicRecord(t)
 	assert.Equal(t, panicMarker, record["panic"])

internal/reqtls/reqtls.go

@@ -5,7 +5,7 @@
 // several packages, by hand, and the answers disagreed. The session
 // cookie's Secure attribute was decided at startup from the configured
 // environment while the CSRF cookie's was decided per-request, so a
-// deployment behind a TLS proxy in the dev environment emitted one
+// deployment behind a TLS proxy in the default environment emitted one
 // Secure cookie and one non-Secure cookie on the same response.
 // Everything kept working, which is exactly why nobody noticed.
 //

internal/resetpw/resetpw_test.go

@@ -140,7 +140,7 @@ func (n *noopEvictor) EvictWebhook(string) {}
 // and the database, exactly as internal/handlers builds them.
 //
 // One application per test function, not per case: every start that
-// finds no account seeds one with an Argon2id hash, and this package's
+// finds no account seeds one at 64 MB of Argon2id, and this package's
 // budget is not the place to spend that repeatedly.
 func newServerApp(
 	t *testing.T, dir string,

internal/server/routes.go

@@ -92,25 +92,11 @@ func (s *Server) setupGlobalMiddleware() {
 func (s *Server) setupRoutes() {
 	s.router.Get("/", s.h.HandleIndex())
 
-	// Static assets answer GET and HEAD only. chi's default 405
-	// carries no Allow header, so this group supplies its own.
-	staticFiles := http.StripPrefix(
-		"/s", http.FileServer(http.FS(static.Static)),
+	s.router.Mount(
+		"/s",
+		http.StripPrefix("/s", http.FileServer(http.FS(static.Static))),
 	)
 
-	s.router.Route("/s", func(r chi.Router) {
-		r.MethodNotAllowed(func(w http.ResponseWriter, _ *http.Request) {
-			w.Header().Set("Allow", "GET, HEAD")
-			http.Error(
-				w,
-				"Method Not Allowed",
-				http.StatusMethodNotAllowed,
-			)
-		})
-		r.Method(http.MethodGet, "/*", staticFiles)
-		r.Method(http.MethodHead, "/*", staticFiles)
-	})
-
 	s.router.Route("/api/v1", func(_ chi.Router) {
 		// API routes will be added here.
 	})

internal/server/routes_test.go

@@ -7,7 +7,6 @@ import (
 	"net/http/httptest"
 	"net/url"
 	"regexp"
-	"slices"
 	"strconv"
 	"strings"
 	"testing"
@@ -221,21 +220,9 @@ func (e *testEnv) csrfFrom(
 	// out of the markup has to be unescaped before it is submitted.
 	token := html.UnescapeString(match[1])
 
-	// A cookie the page sets replaces the one of the same name, as in
-	// a browser. Sent both, the server would read the first, older one.
-	set := w.Result().Cookies()
-	combined := make([]*http.Cookie, 0, len(cookies)+len(set))
-
-	for _, c := range cookies {
-		replaced := slices.ContainsFunc(set, func(n *http.Cookie) bool {
-			return n.Name == c.Name
-		})
-		if !replaced {
-			combined = append(combined, c)
-		}
-	}
-
-	combined = append(combined, set...)
+	combined := make([]*http.Cookie, 0, len(cookies))
+	combined = append(combined, cookies...)
+	combined = append(combined, w.Result().Cookies()...)
 
 	return token, combined
 }
@@ -409,15 +396,13 @@ func (e *testEnv) storedHash(t *testing.T, username string) string {
 
 // --- /s static group ---
 
-// TestStaticServesOnlyGetAndHead pins the methods the static group
-// answers: GET and HEAD are served the asset, and the other methods
-// chi routes (POST, PUT, DELETE and the rest) are refused with 405
-// and an Allow header naming those two. A method chi does not route,
-// such as PROPFIND, is refused with 405 by the top-level router
-// before it reaches the static group, so it gets no Allow header.
-// The README documents this; the test is what keeps the two from
-// drifting.
-func TestStaticServesOnlyGetAndHead(t *testing.T) {
+// TestStaticServesEveryMethod pins what the static mount actually
+// answers. chi's Mount registers the handler for all methods and
+// http.FileServer only special-cases HEAD (by suppressing the body),
+// so a POST or a DELETE to an asset is served the file rather than
+// refused. The README documents this; the test is what keeps the two
+// from drifting.
+func TestStaticServesEveryMethod(t *testing.T) {
 	t.Parallel()
 
 	env := newTestEnv(t)
@@ -432,7 +417,6 @@ func TestStaticServesOnlyGetAndHead(t *testing.T) {
 		http.MethodPost,
 		http.MethodPut,
 		http.MethodDelete,
-		"PROPFIND",
 	} {
 		t.Run(method, func(t *testing.T) {
 			t.Parallel()
@@ -444,38 +428,18 @@ func TestStaticServesOnlyGetAndHead(t *testing.T) {
 			w := httptest.NewRecorder()
 			env.router.ServeHTTP(w, req)
 
-			switch method {
-			case http.MethodGet:
-				assert.Equal(t, http.StatusOK, w.Code)
-				assert.Equal(t, body, w.Body.Bytes(),
-					"the asset itself is returned")
-			case http.MethodHead:
-				assert.Equal(t, http.StatusOK, w.Code)
+			assert.Equal(t, http.StatusOK, w.Code,
+				"static mount answers every method")
+
+			if method == http.MethodHead {
 				assert.Empty(t, w.Body.Bytes(),
 					"HEAD must not carry a body")
-			case "PROPFIND":
-				assert.Equal(
-					t, http.StatusMethodNotAllowed, w.Code,
-				)
-				assert.Empty(t, w.Header().Get("Allow"),
-					"chi refuses a method it does not route "+
-						"before the static group runs")
-				assert.NotContains(
-					t, w.Body.String(), string(body),
-					"a refused method must not get the asset",
-				)
-			default:
-				assert.Equal(
-					t, http.StatusMethodNotAllowed, w.Code,
-				)
-				assert.Equal(
-					t, "GET, HEAD", w.Header().Get("Allow"),
-				)
-				assert.NotContains(
-					t, w.Body.String(), string(body),
-					"a refused method must not get the asset",
-				)
+
+				return
 			}
+
+			assert.Equal(t, body, w.Body.Bytes(),
+				"the asset itself is returned")
 		})
 	}
 }
@@ -627,59 +591,6 @@ func TestPagesLogin_CorrectPasswordSurvivesASpentBudget(
 	)
 }
 
-// TestPagesLogin_CookiesFromAnEarlierDatabase is
-// https://git.eeqj.de/sneak/webhooker/issues/359. A new database
-// brings a new session key, and the operator's browser still holds
-// the session and CSRF cookies signed with the old one. Logging in
-// must work as from a fresh browser and leave cookies the new key
-// accepts.
-func TestPagesLogin_CookiesFromAnEarlierDatabase(t *testing.T) {
-	t.Parallel()
-
-	const (
-		username = "operator"
-		password = "correct-horse-battery-staple"
-	)
-
-	earlier := newTestEnv(t)
-	earlierID, _ := earlier.seedUser(t, username, password)
-	_, stale := earlier.csrfFrom(t, "/pages/login", nil)
-	stale = append(stale, earlier.authCookies(t, earlierID, username)...)
-
-	env := newTestEnv(t)
-	env.seedUser(t, username, password)
-
-	token, cookies := env.csrfFrom(t, "/pages/login", stale)
-
-	form := url.Values{}
-	form.Set("csrf_token", token)
-	form.Set("username", username)
-	form.Set("password", password)
-
-	w := env.post("/pages/login", form, cookies)
-	require.Equal(
-		t, http.StatusSeeOther, w.Code,
-		"a session cookie from another key must not fail the login",
-	)
-
-	// The response deletes the old session cookie and then sets the
-	// new one; a browser keeps the last.
-	var fresh *http.Cookie
-
-	for _, c := range w.Result().Cookies() {
-		if c.Name == session.SessionName {
-			fresh = c
-		}
-	}
-
-	require.NotNil(t, fresh, "login must set a session cookie")
-	assert.Equal(
-		t, "/sources",
-		env.get("/", []*http.Cookie{fresh}).Header().Get("Location"),
-		"the new session cookie must authenticate",
-	)
-}
-
 // --- /user/{username} group ---
 
 // TestPasswordChange_OversizeBody_RejectedAndPasswordUnchanged

internal/session/codec_test.go

@@ -19,8 +19,8 @@ import (
 )
 
 // The tests below exercise the securecookie codecs underneath the
-// store and nothing else: they decode through the store itself, so no
-// server-side expiry check takes part in the result. They exist because
+// store and nothing else: Session.Get only decodes, so no server-side
+// expiry check takes part in the result. They exist because
 // NewCookieStore gives its codecs a 30-day max age that assigning
 // store.Options does not override, which would let the codec accept a
 // cookie weeks past the cap the cookie attribute advertises.
@@ -75,11 +75,10 @@ func restamp(
 	return base64.URLEncoding.EncodeToString(payload)
 }
 
-// decodeCookie feeds value back through the store's decode path. It
-// asks the store rather than Session.Get, which treats a cookie that
-// does not decode as absent and so hides the codec's reason.
+// decodeCookie feeds value back through the store's decode path.
 func decodeCookie(
 	t *testing.T,
+	s *session.Session,
 	value string,
 ) (*sessions.Session, error) {
 	t.Helper()
@@ -95,7 +94,7 @@ func decodeCookie(
 		SameSite: http.SameSiteLaxMode,
 	})
 
-	sess, err := session.NewStore(testKey()).Get(req, session.SessionName)
+	sess, err := s.Get(req)
 	require.NotNil(t, sess)
 
 	return sess, err
@@ -106,7 +105,7 @@ func TestCodec_AcceptsCookieInsideAbsoluteCap(t *testing.T) {
 
 	s := testSession(t)
 
-	sess, err := decodeCookie(t, restamp(
+	sess, err := decodeCookie(t, s, restamp(
 		t,
 		issuedCookie(t, s),
 		time.Now().Add(-(testAbsoluteMaxAge-time.Hour)),
@@ -127,7 +126,7 @@ func TestCodec_RejectsCookiePastAbsoluteCap(t *testing.T) {
 
 	s := testSession(t)
 
-	sess, err := decodeCookie(t, restamp(
+	sess, err := decodeCookie(t, s, restamp(
 		t,
 		issuedCookie(t, s),
 		time.Now().Add(-(testAbsoluteMaxAge+time.Hour)),

internal/session/session.go

@@ -146,8 +146,8 @@ func newStore(key []byte) *sessions.CookieStore {
 //
 // This is decided per-request, not once at startup. Deciding it at
 // startup from the configured environment is what this replaces, and
-// it got the DEFAULT posture wrong: "dev" was then the environment when
-// WEBHOOKER_ENVIRONMENT was unset, so a deployment terminating TLS at a
+// it got the DEFAULT posture wrong: "dev" is the environment when
+// WEBHOOKER_ENVIRONMENT is unset, so a deployment terminating TLS at a
 // proxy without also setting the environment emitted the
 // authentication cookie with no Secure attribute -- silently, and on
 // the same response as a CSRF cookie that did have one.
@@ -224,22 +224,10 @@ func New(
 }
 
 // Get retrieves a session for the request.
-//
-// A session cookie that does not decode -- one signed with an earlier
-// session key, say, because the database was made anew -- is treated
-// as absent: the caller gets a new, empty session and no error, and
-// the next save replaces the cookie.
 func (s *Session) Get(
 	r *http.Request,
 ) (*sessions.Session, error) {
-	sess, err := s.store.Get(r, SessionName)
-	if sess == nil {
-		return nil, err
-	}
-
-	// For a cookie that does not decode, gorilla/sessions returns a
-	// new, empty session alongside the error that is dropped here.
-	return sess, nil
+	return s.store.Get(r, SessionName)
 }
 
 // GetKey returns the raw 32-byte authentication key used for

internal/session/session_test.go

@@ -990,8 +990,8 @@ func sessionCookieFrom(
 
 // TestSave_SecureFollowsRequestTransport is the regression test for
 // the defect this replaces: Secure was fixed at startup from the
-// configured environment, and "dev" was then the environment when
-// WEBHOOKER_ENVIRONMENT was unset. A deployment behind a TLS proxy in
+// configured environment, and "dev" is the environment when
+// WEBHOOKER_ENVIRONMENT is unset. A deployment behind a TLS proxy in
 // that DEFAULT posture shipped the authentication cookie with no
 // Secure attribute and said nothing about it.
 //

script/test

@@ -22,23 +22,13 @@
 # The one figure above 90s is GOMAXPROCS 1, a synthetic core floor rather than
 # a condition CI runs under. If a CPU-limited runner ever puts a real run near
 # 67s, that is the datum to revisit the org figure with.
-#
-# -p 4 -parallel 8 keep the run under 2 GB of memory: at most four test
-# binaries build or run at once, each with at most eight parallel tests. Under
-# -race every test binary and every link costs a few hundred MB, so the
-# defaults (one per core) add up to several GB on a many-core host.
-#
-# No -v: the Docker build cuts each step's log off at 2 MiB, and verbose output
-# from the whole suite passes that before a failure is printed. Without it, go
-# test prints one result line per package and, for a package that fails,
-# everything its tests wrote, application log lines included.
 set -eu
 
 ROOT="$(cd "$(dirname "$0")/.." && pwd -P)"
 
 main() {
     cd "$ROOT"
-    go test -race -p 4 -parallel 8 -timeout 90s ./...
+    go test -v -race -timeout 90s ./...
 }
 
 main "$@"