deploy (#365)

Reviewed-on: #365

Dockerfile

@@ -88,7 +88,9 @@ RUN CGO_ENABLED=1 make build VERSION="$VERSION" GO_LDFLAGS='-extldflags "-static
 # alpine:3.21, 2026-03-17
 FROM alpine:3.21@sha256:c3f8e73fdb79deaebaa2037150150191b9dcbfba68b4a46d70103204c53f4709
 
-RUN apk --no-cache add ca-certificates
+# 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
 
 # Create non-root user
 RUN addgroup -g 1000 -S webhooker && \
@@ -99,13 +101,17 @@ 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
 
-USER webhooker
+# No USER: the entrypoint starts as root to make the data directory
+# webhooker's, then runs the app as webhooker.
 
 EXPOSE 8080
 
@@ -124,4 +130,5 @@ 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

@@ -7,6 +7,13 @@ services, durably stores them, and delivers them to configured targets
 with retry support, logging, and observability. Category: infrastructure
 / web service. License: MIT.
 
+Each entrypoint is a version 4 UUID served at `/webhook/{uuid}`, and
+that UUID is the entrypoint's only credential. webhooker does not use
+shared secrets, HMAC signatures or token headers on the receiver, and
+will not add them — read
+[The entrypoint URL is the authentication secret](#the-entrypoint-url-is-the-authentication-secret)
+before deploying one.
+
 ## Getting Started
 
 ### Prerequisites
@@ -37,9 +44,9 @@ make bootstrap
 # Run all checks (test, lint, format check)
 make check
 
-# Run in development mode. DATA_DIR defaults to /var/lib/webhooker in
+# Run the server from the clone. DATA_DIR defaults to
-# every environment, so set it (in .env or the shell) to a writable
+# /var/lib/webhooker in every environment, so set it (in .env or the
-# directory when running from a clone.
+# shell) to a writable directory.
 DATA_DIR=./data make dev
 
 # Build Docker image
@@ -71,11 +78,22 @@ make clean      # Remove bin/
 ### Configuration
 
 All configuration is via environment variables. For local development,
-you can place variables in a `.env` file in the project root (loaded
+you can place variables in a `.env` file in the process working
-automatically via `godotenv/autoload`).
+directory, read once at startup before anything else looks at the
+environment.
+
+The file is optional and having none is the normal case for a
+deployment. A file that is there but cannot be parsed aborts startup
+with a message naming it, because a single malformed line makes none
+of the file apply: every variable in it silently reverts to its
+default, which is exactly the failure [Invalid values abort
+startup](#invalid-values-abort-startup) exists to prevent, for all of
+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: `dev`). The setting controls exactly one behavior:
+or `prod` (default: `prod`; `dev` must be set explicitly). The setting
+controls exactly one behavior:
 
 | Behavior | `dev`                   | `prod`           |
 | -------- | ----------------------- | ---------------- |
@@ -117,7 +135,7 @@ TTY detection, and security headers are always applied.
 
 | Variable                | Description                         | Default  |
 | ----------------------- | ----------------------------------- | -------- |
-| `WEBHOOKER_ENVIRONMENT` | `dev` or `prod`                     | `dev`    |
+| `WEBHOOKER_ENVIRONMENT` | `dev` or `prod`                     | `prod`   |
 | `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` |
@@ -125,7 +143,7 @@ TTY detection, and security headers are always applied.
 | `MAINTENANCE_MODE`      | Report `maintenanceMode: true` in the healthcheck JSON. It does not change how any request is served — no maintenance page exists | `false`  |
 | `METRICS_USERNAME`      | Basic auth username for `/metrics`. Must be set together with `METRICS_PASSWORD`; one without the other fails startup | `""`     |
 | `METRICS_PASSWORD`      | Basic auth password for `/metrics`. Must be set together with `METRICS_USERNAME`; one without the other fails startup | `""`     |
-| `SENTRY_DSN`            | Sentry error reporting DSN          | `""`     |
+| `SENTRY_DSN`            | Sentry error reporting DSN. Unset leaves error reporting off; a value the Sentry SDK cannot parse fails startup rather than serving with reporting silently off | `""`     |
 | `RETENTION_SWEEP_INTERVAL` | How often the retention reaper and archive sweeper run (Go duration, must be positive) | `1h` |
 | `SESSION_IDLE_TIMEOUT`  | Idle session timeout (Go duration)  | `24h`    |
 | `RECEIVER_RATE_LIMIT`   | Receiver requests/minute per IP per entrypoint (10x that per IP across the route) | `120` |
@@ -139,6 +157,11 @@ 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
@@ -177,15 +200,16 @@ 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 that
+- **It cannot open link-local, or a cloud metadata endpoint at a
-  discloses credentials or user data.** An address is on the list below
+  non-public address that discloses credentials or user data.** An
-  when both of these hold: the provider fixes it, so it cannot collide
+  address is on the list below when it is not a public address and both
-  with anything you run; and reaching it hands out credentials, user
+  of these hold: the provider fixes it, so it cannot collide with
-  data or bootstrap material. Those stay blocked no matter what you
+  anything you run; and reaching it hands out credentials, user data or
-  list, including when you list them outright or list a supernet such
+  bootstrap material. Those stay blocked no matter what you list,
-  as `0.0.0.0/0`, `::/0`, `fd00::/8` or `100.64.0.0/10`. Treat this as
+  including when you list them outright or list a supernet such as
-  best effort rather than a guarantee — it is a hand-maintained list
+  `0.0.0.0/0`, `::/0`, `fd00::/8` or `100.64.0.0/10`. Treat this as best
-  and the caveat below the table applies:
+  effort rather than a guarantee — it is a hand-maintained list and the
+  caveat below the table applies:
 
   | Blocked unconditionally | What it is |
   | ----------------------- | ---------- |
@@ -224,7 +248,8 @@ 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.
+  escape hatch at all; Azure's `168.63.129.16` is refused by the default
+  blocklist instead, as described above.
 
   This list is not exhaustive of every cloud's metadata address — if
   yours is not here, do not allowlist the block that contains it.
@@ -382,10 +407,9 @@ 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 —
+at startup whenever `TRUSTED_PROXIES` is empty, in every environment,
-not only when `WEBHOOKER_ENVIRONMENT=prod`, because that variable
+because behind a proxy every client shares one bucket in `dev` and
-defaults to `dev` and an operator who never set it is precisely the
+`prod` alike. The warning is informational when nothing proxies to 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.
@@ -464,10 +488,20 @@ startup), every entry in `TRUSTED_PROXIES` and
 `ALLOWED_EGRESS_CIDRS` must be a CIDR block or a bare IP address, and
 `BIND_ADDRESS` must be an IP address literal — `localhost`,
 `127.0.0.1:8080` and `10.0.0.0/8` are each rejected rather than
-resolved, split, or narrowed to something they do not say.
+resolved, split, or narrowed to something they do not say — and
+`SENTRY_DSN` must parse as a Sentry DSN.
 `SESSION_IDLE_TIMEOUT` is the exception: a
 non-positive value there means idle expiry is disabled, not invalid.
 
+`SENTRY_DSN` is checked with the Sentry SDK's own parser, the same call
+the SDK makes on the DSN it is later handed, so what configuration
+accepts is exactly what will initialise. A typo in it is the one
+configuration mistake nothing downstream can ever notice — the variable
+is still set, so every later signal reports error reporting as on while
+no report is being sent — which is why it aborts rather than starting
+with reporting off. Leaving it unset is not a mistake and not affected:
+error reporting is simply off and startup is normal.
+
 Boolean variables (`DEBUG`, `MAINTENANCE_MODE`) accept exactly the
 spellings Go's `strconv.ParseBool` accepts — `1`, `t`, `T`, `TRUE`,
 `true`, `True`, `0`, `f`, `F`, `FALSE`, `false`, `False` — and nothing
@@ -504,6 +538,12 @@ 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
@@ -518,8 +558,9 @@ printf '%s' "$NEW_PASSWORD" | \
     DATA_DIR=/var/lib/webhooker webhooker resetpw admin
 ```
 
-In a container it is the same binary, which the image sets as `CMD`
+In a container it is the same binary. The image's `CMD` is
-rather than `ENTRYPOINT`, so the whole command has to be given:
+`/app/webhooker`, and a command given to `docker run` replaces all of
+it, so the whole command has to be given:
 
 ```bash
 docker run --rm -v webhooker-data:/var/lib/webhooker \
@@ -611,7 +652,6 @@ 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
 ```
@@ -657,38 +697,22 @@ 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 container runs as a non-root user (`webhooker`, UID 1000), exposes
+The app runs as a non-root user (`webhooker`, UID 1000), exposes port
-port 8080, and includes a health check against
+8080, and includes a health check against `/.well-known/healthcheck`.
-`/.well-known/healthcheck`. The `/var/lib/webhooker` volume holds all
+The `/var/lib/webhooker` volume holds all SQLite databases: the main
-SQLite databases: the main application database (`webhooker.db`), the
+application database (`webhooker.db`), the per-webhook event databases
-per-webhook event databases (`events-{uuid}.db`), and any archive
+(`events-{uuid}.db`), and any archive databases written by `database`
-databases written by `database` targets (`archive-{uuid}.db`). Mount
+targets (`archive-{uuid}.db`). Mount this as a persistent volume to
-this as a persistent volume to preserve data across container
+preserve data across container restarts.
-restarts.
 
-**The bind-mounted directory must be owned by UID 1000, or the
+**The container sets its data directory's owner and mode itself
-container does not start.** Docker creates a `-v` source path that
+before the app starts**, so a host directory can be mounted as it is,
-does not exist yet as `root:root`, and the process runs as UID 1000,
+whoever owns it. The image's `ENTRYPOINT`,
-so it cannot take its `DATA_DIR` lock:
+`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
-webhooker: locking data directory /var/lib/webhooker: open
+then runs the app as `webhooker`. Started with `--user`, it changes
-/var/lib/webhooker/webhooker.lock: permission denied
+nothing and runs the app as that user.
-```
-
-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 —
@@ -696,13 +720,59 @@ 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. A `DATA_DIR` webhooker creates itself is `0750`, but
+they are opened. The directory's `0750` is defence in depth — it stops
-a bind mount supplies its own directory and Docker's default for one
+other local users listing the directory and learning your webhook
-it creates is `0755`; the `0600` files hold there regardless. The
+UUIDs from the `events-{uuid}.db` filenames — not the barrier
-`chmod 750` above is defence in depth — it stops other local users
+protecting the credentials.
-listing the directory and learning your webhook UUIDs from the
+
-`events-{uuid}.db` filenames — not the barrier protecting the
+### Running under upaas
-credentials.
+
+[upaas](https://git.eeqj.de/sneak/upaas) builds the image from this
+repository's `Dockerfile` and runs it. The app needs:
+
+- **Network and port:** add no port mapping in upaas. upaas publishes
+  every mapped port on all interfaces of the host
+  ([upaas issue 113](https://git.eeqj.de/sneak/upaas/issues/113)),
+  which would put the plain-HTTP admin UI and receiver there. Instead,
+  set the app's Docker Network in upaas to your reverse proxy's Docker
+  network; the proxy then reaches the app at `upaas-` followed by the
+  app name, port `8080`. Leave `PORT` unset: the image's health check
+  probes `8080`.
+- **Volume:** one host directory mounted at `/var/lib/webhooker`.
+- **Environment variables:**
+    - `WEBHOOKER_ENVIRONMENT=prod`
+    - `TRUSTED_PROXIES`: your reverse proxy's address on that Docker
+      network. The `remoteIP` field of the `http request` log line for a
+      request that came through the proxy shows it; the health check's
+      own lines show `::1`. See [Trusted proxies](#trusted-proxies).
+    - Leave `BIND_ADDRESS` and `DATA_DIR` unset: the image sets
+      `BIND_ADDRESS` to `0.0.0.0`, and `DATA_DIR` defaults to
+      `/var/lib/webhooker`.
+    - Everything else is optional; see [Configuration](#configuration).
+- **Health check:** the image's own, which requests
+  `/.well-known/healthcheck`. upaas reads the container's health 60
+  seconds after a deploy and marks the deploy failed unless it is
+  `healthy`.
+- **First run:** the first start prints the `admin` password once, in
+  the banner described under [The admin account](#the-admin-account),
+  to the container's log. upaas names the container `upaas-` followed
+  by the app name, so for an app named `webhooker`:
+
+    ```bash
+    docker logs upaas-webhooker
+    ```
+
+  If the password is lost, stop the container, set a new password with
+  the app's own image and volume, and start it again (see
+  [Recovering a lost admin password](#recovering-a-lost-admin-password)):
+
+    ```bash
+    docker stop upaas-webhooker
+    docker run --rm --volumes-from upaas-webhooker \
+        "$(docker inspect -f '{{.Image}}' upaas-webhooker)" \
+        /app/webhooker resetpw -generate admin
+    docker start upaas-webhooker
+    ```
 
 ## Deployment behind a reverse proxy
 
@@ -725,17 +795,18 @@ 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. **Set `WEBHOOKER_ENVIRONMENT=prod`, and make sure the proxy sends
+2. **Make sure the environment is not `dev` (leave
-   `X-Forwarded-Proto`.** These are two requirements, not one. The
+   `WEBHOOKER_ENVIRONMENT` unset or set it to `prod`), and make sure
-   environment setting decides CORS and nothing else: the default
+   the proxy sends `X-Forwarded-Proto`.** These are two requirements,
+   not one. The environment setting decides CORS and nothing else:
    `dev` answers every origin with `Access-Control-Allow-Origin: *`
    (without credentials), which a server-rendered production
-   deployment has no use for. Cookie `Secure` and the strict
+   deployment has no use for, and `prod` — the default — disables it.
-   Origin/Referer mode are **not** tied to it — they are decided per
+   Cookie `Secure` and the strict Origin/Referer mode are **not** tied
-   request from the transport, which behind a proxy means the
+   to it — they are decided per request from the transport, which
-   `X-Forwarded-Proto` header. The block below sets it; without it
+   behind a proxy means the `X-Forwarded-Proto` header. The block below
-   every request is read as plaintext and cookies ship without
+   sets it; without it every request is read as plaintext and cookies
-   `Secure`. See [Configuration](#configuration).
+   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
@@ -827,7 +898,6 @@ 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
 ```
@@ -882,15 +952,10 @@ 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 `webhooker.db` and every `events-*.db`, which
+A clean shutdown closes every database, which checkpoints and removes
-checkpoints and removes their sidecars; a killed or crashed instance
+its sidecars; a killed or crashed instance leaves them, and they must be
-leaves them, and they must be carried with the `.db`. **Archive
+carried with the `.db`. An archive the service has not opened since a
-databases are different**: their handle is not closed at shutdown, so
+crash keeps that crash's sidecars, even across a later clean stop.
-`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
@@ -932,12 +997,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` added — the `sqlite3` CLI is **not** in it, so run
+`ca-certificates` and `su-exec` added — the `sqlite3` CLI is **not** in
-this on the host against the volume path, or from a throwaway container
+it, so run this on the host against the volume path, or from a
-that mounts the volume. Second, each file is captured at its own
+throwaway container that mounts the volume. Second, each file is
-instant, so a webhook created or an event delivered between two files
+captured at its own instant, so a webhook created or an event delivered
-being copied lands in one and not the other. If you need the whole set
+between two files being copied lands in one and not the other. If you
-coherent as of a single moment, stop the service.
+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
@@ -965,10 +1030,9 @@ 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.
+20 KB `.db` with no sidecars about a minute after its last write. A
-Shutdown is **not** on that list: the archive handle is not closed when
+clean stop closes it too. So either move `archive-{uuid}.db` together
-the service stops. So either move `archive-{uuid}.db` together with any
+with any `-wal`/`-shm` beside it, or wait until there are none.
-`-wal`/`-shm` beside it, or wait until there are none.
 
 ### Restore
 
@@ -987,28 +1051,16 @@ the service stops. So either move `archive-{uuid}.db` together with any
    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 has none for `webhooker.db` or the `events-*.db`,
+   stop-and-copy set normally has none, because a clean stop closes
-   because a clean stop closes those and checkpoints their sidecars
+   every database and checkpoints its sidecars away; the exception is an
-   away — but it will normally have them for `archive-*.db`, whose
+   archive not opened since a crash. A copy salvaged from a crashed
-   handle stays open across shutdown, and those carry the archive's
+   instance has them for everything, and needs all of them.
-   rows. A copy salvaged from a crashed instance has them for
-   everything, and needs all of them.
 
-4. **Fix ownership.** The container runs as the non-root `webhooker`
+4. Start the service. The container gives the directory and the
-   user, UID 1000 / GID 1000. Restored files must be owned by (or
+   restored files to the `webhooker` user before the app starts,
-   writable by) that UID, and so must the directory itself — SQLite
+   whoever restored them (see
-   creates the `-wal` and `-shm` sidecars beside the database, so a
+   [Running with Docker](#running-with-docker)). `AutoMigrate` runs
-   writable file inside a directory it cannot write is not enough:
+   against each restored database as it is opened.
-
-    ```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
 
@@ -1129,14 +1181,38 @@ backups at rest and restrict who can read them.
 
 ## The entrypoint URL is the authentication secret
 
-The receiver verifies nothing about an inbound request. The UUID in an
+**The entrypoint UUID is the credential, and it is the only one.**
-entrypoint's URL is its credential: anyone who holds that URL can
+webhooker mints a version 4 UUID per entrypoint and serves it at
-submit events to it, and the receiver checks nothing else about the
+`/webhook/{uuid}`. Possession of that URL is the authentication:
-sender. Treat an entrypoint URL the way you would treat an API token.
+anyone who holds it can submit events to the entrypoint, and the
+receiver verifies nothing else about the sender.
 
-There is no way to rotate the UUID in place. To retire one, delete the
+There is no shared secret, no HMAC signature, no bearer token and no
-entrypoint (or deactivate it, which answers `410`) and create a new
+second factor on the receiver, and none will be added. This was
-one, then point the sender at the new URL.
+considered and rejected; the implementation that existed was removed
+in [PR #279](https://git.eeqj.de/sneak/webhooker/pulls/279), closing
+[issue #67](https://git.eeqj.de/sneak/webhooker/issues/67) and
+[issue #241](https://git.eeqj.de/sneak/webhooker/issues/241). A
+proposal to reintroduce any of them — including as "defence in depth"
+alongside the UUID — is answered by this section. Inbound signature
+headers a sender sends anyway (`X-Hub-Signature` and its
+per-provider equivalents) are stored and forwarded as ordinary
+headers; nothing checks them.
+
+What that means for an operator:
+
+- **The URL is a capability, so treat it as a secret.** Keep it out of
+  logs, ticket bodies, chat messages and screenshots. Anyone who reads
+  it anywhere can post events as that sender.
+- **Rotating means minting a new entrypoint, not changing a key.**
+  There is no way to rotate the UUID in place. To retire one, delete
+  the entrypoint (or deactivate it, which answers `410`) and create a
+  new one, then point the sender at the new URL.
+- **A sender that cannot be given a secret URL is a constraint on that
+  integration, not a reason to change this.** If a service only
+  supports signed payloads to a well-known URL, raise it as its own
+  problem — pick a different integration path, or accept that it
+  cannot be used. It is not grounds to reintroduce shared secrets.
 
 ## Entrypoints
 
@@ -1146,8 +1222,15 @@ 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`. We
+`build` and `version` both take their value from `script/version`.
-provide:
+
+`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:
 
 - `script/bootstrap` — install all dependencies (idempotent)
 - `script/setup` — make a fresh clone ready for development
@@ -1214,6 +1297,16 @@ webhooker solves this by acting as a durable intermediary:
    backoff. Every delivery attempt is logged with status codes, response
    bodies, and timing.
 
+   **That guarantee is at-least-once, not exactly-once.** When a send
+   reaches its target but the write recording that outcome fails, the
+   delivery is deliberately left in a recoverable state rather than
+   marked done — losing a delivery is the worse failure — so the
+   pending sweep picks it up about fifteen minutes later, or the next
+   restart does, and the target receives a payload it already got.
+   webhooker adds no delivery identifier of its own to an outbound
+   request, so **make your receiver idempotent** against whatever the
+   payload itself carries.
+
 3. **Observability** — Full request/response logging for every webhook
    received and every delivery attempted. Prometheus metrics expose
    volume, latency, and error rates. The web UI provides real-time
@@ -1446,7 +1539,7 @@ events should be forwarded.
 | `type`           | TargetType | One of: `http`, `slack`, `database`, `log` |
 | `active`         | boolean    | Whether deliveries are enabled (default: true) |
 | `config`         | JSON text  | Type-specific configuration |
-| `max_retries`    | integer    | Maximum retry attempts for `http` and `slack` targets (0 = fire-and-forget, >0 = retries with backoff and a circuit breaker). Ignored by `database` and `log` targets |
+| `max_retries`    | integer    | Total delivery attempts for `http` and `slack` targets, not retries on top of the first: 0 is a single fire-and-forget attempt with no retries and no circuit breaker, and a value of N makes N attempts in all, with exponential backoff and a per-target circuit breaker. Ignored by `database` and `log` targets |
 | `max_queue_size` | integer    | Stored and shown on the target's detail view, but not enforced anywhere yet: nothing in the delivery engine consults it. Queue depth is set by the two fixed 10,000-entry channels |
 
 **Relations:** Belongs to Webhook. Has many Deliveries.
@@ -1454,12 +1547,12 @@ events should be forwarded.
 **Target types:**
 
 - **`http`** — Forward the event as an HTTP POST to a configured URL.
-  Behavior depends on `max_retries`: when `max_retries` is 0 (the
+  `max_retries` is the total number of delivery attempts, not retries on
-  default), the target operates in fire-and-forget mode — a single
+  top of the first: when `max_retries` is 0 (the default), the target
-  attempt with no retries and no circuit breaker. When `max_retries` is
+  operates in fire-and-forget mode, a single attempt with no retries and
-  greater than 0, failed deliveries are retried with exponential backoff
+  no circuit breaker; a value of N makes up to N attempts in all,
-  up to `max_retries` attempts, protected by a per-target circuit
+  retrying failed deliveries with exponential backoff and protecting them
-  breaker.
+  with a per-target circuit breaker.
 - **`slack`** — Post the event as a formatted message to a
   Slack-compatible incoming webhook URL (`webhookUrl` in `config`). It
   is built on the same HTTP core as `http` and honours `max_retries`
@@ -1639,6 +1732,28 @@ retries) is individually logged for full observability.
 
 **Relations:** Belongs to Delivery.
 
+#### 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:
+
+| 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 |
+
+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.
+
 #### Common Fields
 
 Every entity except `Setting` includes these fields from `BaseModel`.
@@ -1902,7 +2017,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. |
+| **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. |
 
 **Transitions:**
 
@@ -1940,7 +2055,9 @@ 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.
+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.
 
 ### Metrics
 
@@ -1954,7 +2071,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 an open circuit breaker refused is not one: it is counted as a retry instead |
+| `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_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` |
@@ -2571,7 +2688,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`) |
-| 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` |
+| `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` |
 | `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
@@ -2596,12 +2713,15 @@ abuse limit later; they are tracked as future work.
 | `POST` | `/source/{id}/edit`      | Edit webhook submission |
 | `POST` | `/source/{id}/delete`    | Delete webhook |
 | `GET`  | `/source/{id}/logs`      | Webhook event logs |
+| `GET`  | `/source/{id}/logs/{eventID}/body` | Download an event's full stored body. The log page renders each body only up to its cap, so this is the only route that serves a whole one; it is offered wherever a body is shown truncated |
 | `POST` | `/source/{id}/deliveries/{deliveryID}/replay` | Replay a finished delivery: creates a new delivery for the same event against the target's current configuration (30 per minute per bucket, then `429`) |
 | `POST` | `/source/{id}/events/{eventID}/resubmit` | Resubmit a stored event: creates a new event copying it and fans that out to every currently active target (30 per minute per bucket, then `429`) |
 | `POST` | `/source/{id}/entrypoints` | Add entrypoint to webhook |
 | `POST` | `/source/{id}/entrypoints/{entrypointID}/delete` | Delete an entrypoint |
 | `POST` | `/source/{id}/entrypoints/{entrypointID}/toggle` | Enable or disable an entrypoint |
 | `POST` | `/source/{id}/targets`   | Add target to webhook |
+| `GET`  | `/source/{id}/targets/{targetID}/edit` | Edit target form. The one page that renders a target's destination URL and header values in full, rather than masked |
+| `POST` | `/source/{id}/targets/{targetID}/edit` | Edit target submission |
 | `POST` | `/source/{id}/targets/{targetID}/delete` | Delete a target |
 | `POST` | `/source/{id}/targets/{targetID}/toggle` | Enable or disable a target |
 
@@ -2640,6 +2760,8 @@ webhooker/
 ├── internal/
 │   ├── banner/
 │   │   └── banner.go          # Ruled block for the one credential shown in the clear
+│   ├── ciscript/
+│   │   └── doc.go             # Tests for the CI shell scripts in script/; no runtime code
 │   ├── resetpw/
 │   │   └── resetpw.go         # `webhooker resetpw`: set an account's password, stopped deployments only
 │   ├── config/
@@ -2709,13 +2831,17 @@ webhooker/
 │   │   ├── ratelimit.go       # Per-IP rate limiting middleware (go-chi/httprate)
 │   │   ├── loginguard.go      # Login failure counters and the Argon2id verification semaphore
 │   │   └── testing.go         # NewForTest: Middleware without the fx lifecycle
+│   ├── reqtls/
+│   │   └── reqtls.go          # IsTLS: the one TLS predicate, r.TLS or X-Forwarded-Proto
 │   ├── server/
 │   │   ├── server.go          # Server struct, fx lifecycle, signal handling
 │   │   ├── http.go            # HTTP server setup with timeouts
 │   │   └── routes.go          # All route definitions
-│   └── session/
+│   ├── session/
-│       ├── session.go         # Cookie-based session management
+│   │   ├── session.go         # Cookie-based session management
-│       └── testing.go         # NewForTest: Session without the fx lifecycle
+│   │   └── testing.go         # NewForTest: Session without the fx lifecycle
+│   └── versionscript/
+│       └── doc.go             # Tests for script/version and the build files that use it
 ├── static/
 │   ├── static.go              # //go:embed directive
 │   ├── css/input.css          # Tailwind input, source for tailwind.css (make css)
@@ -2828,6 +2954,10 @@ check, see [The login endpoint](#the-login-endpoint).
 
 ### Authentication
 
+- **Webhook receiver:** the entrypoint UUID in the URL, and nothing
+  else. No shared secret, no HMAC signature, no token header, and none
+  will be added — see
+  [The entrypoint URL is the authentication secret](#the-entrypoint-url-is-the-authentication-secret).
 - **Web UI:** Cookie-based sessions using gorilla/sessions with
   encrypted cookies. Sessions are configured with HttpOnly, SameSite
   Lax, and Secure whenever the request is on TLS — the flag follows the
@@ -2867,7 +2997,8 @@ check, see [The login endpoint](#the-login-endpoint).
   mode
 - **The entrypoint URL is the receiver's only credential.** Nothing
   about an inbound request is verified; possession of the UUID
-  authorises submission (see
+  authorises submission, and no shared secret or signature check will
+  be added alongside it (see
   [The entrypoint URL is the authentication secret](#the-entrypoint-url-is-the-authentication-secret))
 - **SSRF prevention** for HTTP delivery targets: private/reserved IP
   ranges (RFC 1918, loopback, link-local, cloud metadata) are blocked
@@ -2907,7 +3038,11 @@ check, see [The login endpoint](#the-login-endpoint).
 - Prometheus metrics behind basic auth
 - Static assets embedded in binary (no filesystem access needed at
   runtime)
-- Container runs as non-root user (UID 1000)
+- 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`
 - GORM soft deletes on every entity that carries `BaseModel`, which is
   all of them but `Setting` (data preserved for audit)
 
@@ -2923,7 +3058,8 @@ 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`
+4. `delivery.Engine` — waits for its workers, then closes the archive
+   databases
 5. `healthcheck`
 6. `WebhookDBManager`
 7. the database close
@@ -3033,10 +3169,13 @@ 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,
+3. **Runtime stage** (`alpine:3.21`) — copies the static binary and
-   creates the `/var/lib/webhooker` directory for all SQLite databases,
+   `deploy/docker-entrypoint.sh`, creates the `/var/lib/webhooker`
-   runs as the non-root `webhooker` user (UID 1000), exposes port 8080,
+   directory for all SQLite databases, exposes port 8080, and includes
-   and includes a health check against `/.well-known/healthcheck`.
+   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`.
 
 The lint stage invokes `golangci-lint` directly rather than `make lint`:
 it is already the pinned linter image, and `make lint` builds
@@ -3115,3 +3254,5 @@ MIT
 ## Author
 
 [@sneak](https://sneak.berlin)
+
+

TODO.md

@@ -18,18 +18,27 @@ Issue branches do NOT touch this file — the manager maintains it on
 
 # Status
 
-1.0.0 is open, with work remaining. The milestone
+The milestone (https://git.eeqj.de/sneak/webhooker/milestone/9) is the
-(https://git.eeqj.de/sneak/webhooker/milestone/9) is the authoritative
+authoritative list, and the only place to read a count or a state of
-list, and the only place to read a count or a state of play from. This
+play from. This file records where the project is, not what is in
-file records where the project is, not what is in flight: a sentence
+flight: a sentence whose truth depends on a branch being unmerged is
-whose truth depends on a branch being unmerged is wrong the moment it
+wrong the moment it merges, and this file has been wrong that way
-merges, and this file has been wrong that way before.
+before.
 
-The tag is held on a durability defect
+The durability defect that held the tag has landed
-(https://git.eeqj.de/sneak/webhooker/issues/256): a concurrent reader
+(https://git.eeqj.de/sneak/webhooker/issues/256, commit `8d64259`).
-of a per-webhook event database strands delivered webhooks at
+Every SQLite handle opens with WAL journaling and a busy timeout, a
-`pending`, and the next restart re-delivers them. That issue gates
+bookkeeping write that fails leaves its delivery in a recoverable
-`v1.0.0`, and is where the fix's own state is tracked.
+state rather than a lying one, and recovery skips a delivery that
+already has a successful result row. Final pre-tag verification
+exercised it and confirmed it holds. Whatever the milestone still
+shows open is what remains before `v1.0.0`.
+
+Delivery is at-least-once by design, not by accident: a send whose
+result row does not land is attempted again, so a receiver can see a
+duplicate. That is deliberate — the alternative is a silent lost
+delivery — and the README says so under Rationale. It is not a defect
+to re-file.
 
 One caveat on reading a green check: a docs-only commit deliberately
 replays from the layer cache
@@ -39,11 +48,11 @@ commit invalidates the `COPY` layer and genuinely executes.
 
 # Next Step
 
-Land https://git.eeqj.de/sneak/webhooker/issues/256, then clear the
+Clear the rest of the open 1.0.0 milestone
-rest of the open 1.0.0 milestone and tag `v1.0.0`. Merging `next` into
+(https://git.eeqj.de/sneak/webhooker/milestone/9) and tag `v1.0.0`.
-`main` is a separate act from tagging and waits on neither of those:
+Merging `next` into `main` is a separate act from tagging and waits on
-`next` is kept mergeable at all times, which is the point of the
+neither of those: `next` is kept mergeable at all times, which is the
-branch.
+point of the branch.
 
 # Completed Steps
 

cmd/webhooker/dotenv_test.go

@@ -0,0 +1,107 @@
+package main
+
+import (
+	"bytes"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/webhooker/internal/config"
+)
+
+// dotEnvKey is a throwaway variable name these tests write and read,
+// so they cannot disturb real configuration.
+const dotEnvKey = "WEBHOOKER_TEST_DISPATCH_VALUE"
+
+// writeDotEnvInWorkingDir puts contents in a .env file in a fresh
+// temporary directory and moves the process there.
+//
+// The callers are deliberately not parallel and must stay that way:
+// t.Chdir moves the whole process. Go releases parallel tests only
+// after every sequential test in the package has finished, so nothing
+// else runs while these do.
+func writeDotEnvInWorkingDir(t *testing.T, contents string) {
+	t.Helper()
+
+	dir := t.TempDir()
+	require.NoError(t, os.WriteFile(
+		filepath.Join(dir, config.DotEnvPath),
+		[]byte(contents), 0o600,
+	))
+	t.Chdir(dir)
+}
+
+// TestDispatch_MalformedDotEnvRefuses pins the second half of the
+// defect. godotenv applies nothing at all when a file will not parse,
+// so one mistyped line used to revert every variable in it to its
+// default and start the server anyway, with no log line naming the
+// file. The refusal has to arrive before any subcommand runs, which
+// is why `help` — the one subcommand that touches nothing — is still
+// refused here.
+//
+//nolint:paralleltest // t.Chdir moves the whole process.
+func TestDispatch_MalformedDotEnvRefuses(t *testing.T) {
+	writeDotEnvInWorkingDir(t, "PORT 19615\n")
+
+	var stdout, stderr bytes.Buffer
+
+	code := dispatch(
+		[]string{helpCommand}, strings.NewReader(""), &stdout, &stderr,
+	)
+
+	require.Equal(t, 1, code, "a broken .env must exit non-zero")
+	assert.Contains(
+		t, stderr.String(), config.DotEnvPath,
+		"the refusal must name the file",
+	)
+	assert.Empty(
+		t, stdout.String(),
+		"the subcommand must not have run",
+	)
+}
+
+// TestDispatch_LoadsDotEnvBeforeSubcommands pins the ordering the
+// godotenv/autoload import used to provide for free. It ran in an
+// init(), so .env was in the environment before anything read it —
+// including config.DataDir, which both the DATA_DIR lock and resetpw
+// call outside the fx graph. Loading any later would let a .env that
+// sets DATA_DIR lock one directory while the config opened databases
+// in another.
+func TestDispatch_LoadsDotEnvBeforeSubcommands(t *testing.T) {
+	t.Setenv(dotEnvKey, "placeholder")
+	require.NoError(t, os.Unsetenv(dotEnvKey))
+
+	writeDotEnvInWorkingDir(t, dotEnvKey+"=from-dot-env\n")
+
+	var stdout, stderr bytes.Buffer
+
+	code := dispatch(
+		[]string{helpCommand}, strings.NewReader(""), &stdout, &stderr,
+	)
+
+	require.Equal(t, 0, code)
+	assert.Equal(
+		t, "from-dot-env", os.Getenv(dotEnvKey),
+		"the file must be applied before the subcommand runs",
+	)
+}
+
+// TestDispatch_MissingDotEnvIsFine pins the case most deployments are
+// in: no .env at all, which must stay a normal start.
+//
+//nolint:paralleltest // t.Chdir moves the whole process.
+func TestDispatch_MissingDotEnvIsFine(t *testing.T) {
+	t.Chdir(t.TempDir())
+
+	var stdout, stderr bytes.Buffer
+
+	code := dispatch(
+		[]string{helpCommand}, strings.NewReader(""), &stdout, &stderr,
+	)
+
+	require.Equal(t, 0, code)
+	assert.Empty(t, stderr.String())
+}

cmd/webhooker/main.go

@@ -54,6 +54,11 @@ const stopTimeout = 5 * time.Second
 // caller can tell "called wrong" from "declined".
 const exitUsage = 2
 
+// helpCommand is the subcommand that prints usage. The flag spellings
+// beside it in the switch are aliases; this is the name the usage text
+// documents and the one tests invoke.
+const helpCommand = "help"
+
 // Build-time variables set via -ldflags.
 //
 //nolint:gochecknoglobals // Build-time variables injected by the linker.
@@ -75,11 +80,27 @@ func main() {
 // every existing deployment invoke; that path is unchanged, including
 // where the DATA_DIR lock is taken relative to building the fx graph
 // and how fx propagates a non-zero exit itself.
+//
+// The optional .env file is read here, before any subcommand and so
+// before anything reads the environment — config.DataDir, which both
+// the DATA_DIR lock and resetpw call outside the fx graph, above all.
+// It used to be read from an init() in internal/config, which put it
+// earlier still but threw the error away: a single malformed line
+// applied none of the file and said nothing about it. A file that is
+// not there stays fine, since .env is optional and most deployments
+// do not have one.
 func dispatch(
 	args []string,
 	stdin io.Reader,
 	stdout, stderr io.Writer,
 ) int {
+	err := config.LoadDotEnv()
+	if err != nil {
+		_, _ = fmt.Fprintf(stderr, "%s: %v\n", appname, err)
+
+		return 1
+	}
+
 	if len(args) == 0 {
 		return run(stderr)
 	}
@@ -87,7 +108,7 @@ func dispatch(
 	switch args[0] {
 	case resetpw.Name:
 		return resetpw.Run(args[1:], stdin, stdout, stderr)
-	case "help", "-h", "-help", "--help":
+	case helpCommand, "-h", "-help", "--help":
 		usage(stdout)
 
 		return 0

cmd/webhooker/main_test.go

@@ -121,7 +121,7 @@ func TestDispatch_Help(t *testing.T) {
 	var stdout, stderr bytes.Buffer
 
 	code := dispatch(
-		[]string{"help"}, strings.NewReader(""), &stdout, &stderr,
+		[]string{helpCommand}, strings.NewReader(""), &stdout, &stderr,
 	)
 
 	require.Equal(t, 0, code)

deploy/docker-entrypoint.sh

@@ -0,0 +1,22 @@
+#!/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

@@ -4,6 +4,7 @@ package config
 import (
 	"errors"
 	"fmt"
+	"io/fs"
 	"log/slog"
 	"net/netip"
 	"os"
@@ -11,13 +12,11 @@ import (
 	"strings"
 	"time"
 
+	"github.com/getsentry/sentry-go"
+	"github.com/joho/godotenv"
 	"go.uber.org/fx"
 	"sneak.berlin/go/webhooker/internal/globals"
 	"sneak.berlin/go/webhooker/internal/logger"
-
-	// Populates the environment from a ./.env file automatically for
-	// development configuration. Kept in one place only (here).
-	_ "github.com/joho/godotenv/autoload"
 )
 
 const (
@@ -84,6 +83,12 @@ const (
 	// IPv6 prefix spends on the ::ffff:0:0/96 wrapper, so a /104
 	// covers the same addresses as an IPv4 /8.
 	mappedV4Offset = 96
+
+	// DotEnvPath is the optional file of KEY=value lines read into the
+	// environment at startup, relative to the process working
+	// directory. Exported so that documentation and tests name the
+	// same path the loader opens.
+	DotEnvPath = ".env"
 )
 
 // ErrInvalidEnvironment is returned when WEBHOOKER_ENVIRONMENT
@@ -107,6 +112,15 @@ var ErrInvalidCIDR = errors.New("invalid CIDR")
 // something that is not an IP address literal.
 var ErrInvalidBindAddress = errors.New("invalid bind address")
 
+// ErrInvalidSentryDSN is returned when SENTRY_DSN is set to something
+// the Sentry SDK cannot parse as a DSN.
+var ErrInvalidSentryDSN = errors.New("invalid Sentry DSN")
+
+// ErrDotEnvUnreadable is returned when the optional .env file exists
+// but cannot be read or parsed. A file that is not there is not an
+// error; a file that is there and broken is.
+var ErrDotEnvUnreadable = errors.New("unreadable .env file")
+
 // ErrIncompleteMetricsAuth is returned when exactly one of
 // METRICS_USERNAME and METRICS_PASSWORD carries a value. Neither
 // fallback is acceptable: serving /metrics on the username alone
@@ -178,9 +192,10 @@ 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 address; it is not exhaustive of every
+	// at a provider-fixed, non-public address; it is not
-	// cloud's metadata address. See alwaysBlockedNetworks for the
+	// exhaustive of every cloud's metadata address. See
-	// authoritative list and the criterion it is built from.
+	// alwaysBlockedNetworks for the authoritative list and the
+	// criterion it is built from.
 	AllowedEgressCIDRs []netip.Prefix
 
 	params *ConfigParams
@@ -212,12 +227,62 @@ func (c *Config) MetricsAuthEnabled() bool {
 	return c.MetricsUsername != "" && c.MetricsPassword != ""
 }
 
+// SentryEnabled reports whether error reporting is shipped to Sentry.
+// It is the only answer to that question in the codebase: the SDK
+// initialisation, the sentryhttp middleware registration and the
+// startup log's sentryEnabled field all read this one method, so the
+// log cannot report reporting as on while nothing is sending.
+//
+// A non-empty DSN is enough because loadFromEnv already parsed it with
+// the SDK's own parser and refused to build a Config around one the
+// SDK would reject, and because initialising the SDK with a DSN that
+// parsed and failed anyway aborts the process rather than leaving this
+// true and the client absent.
+func (c *Config) SentryEnabled() bool {
+	return c.SentryDSN != ""
+}
+
 // envString returns the value of the named environment variable,
 // or an empty string if not set.
 func envString(key string) string {
 	return os.Getenv(key)
 }
 
+// LoadDotEnv reads DotEnvPath into the environment when that file is
+// present, and reports a file that is present but broken.
+//
+// It has to run before anything reads the environment, so that every
+// reader agrees on what the environment holds — the DATA_DIR lock
+// taken before the fx graph exists as much as loadFromEnv itself. A
+// variable already set in the real environment wins: godotenv never
+// overwrites one.
+//
+// A missing file is not an error. It is a development convenience and
+// most deployments set the environment directly.
+//
+// Any other failure is. godotenv parses the whole file before setting
+// anything, so a single malformed line applies none of it: every
+// variable in the file silently reverts to its default, which defeats
+// the fail-loud guarantee for all of them at once.
+func LoadDotEnv() error {
+	return loadDotEnvFile(DotEnvPath)
+}
+
+// loadDotEnvFile is LoadDotEnv over a named file, so tests can point
+// at a temporary one instead of the process working directory.
+func loadDotEnvFile(path string) error {
+	err := godotenv.Load(path)
+	if err == nil || errors.Is(err, fs.ErrNotExist) {
+		return nil
+	}
+
+	return fmt.Errorf(
+		"%w: %s: %w; nothing in it was applied, so fix the file or "+
+			"remove it",
+		ErrDotEnvUnreadable, path, err,
+	)
+}
+
 // DataDir resolves DATA_DIR, applying DefaultDataDir when it is unset
 // or empty. It is exported so that entry points which must act on the
 // data directory before the fx graph exists — taking the exclusive
@@ -462,6 +527,41 @@ func envBindAddress(key, defaultValue string) (string, error) {
 	return addr.String(), nil
 }
 
+// envSentryDSN returns the value of the named environment variable
+// checked as a Sentry DSN. An unset (or empty, or whitespace-only)
+// value yields "", which means error reporting stays off — the common
+// case, and a normal start.
+//
+// A set value is parsed with sentry.NewDsn, which is the call
+// sentry.Init makes on the DSN it is handed, so what passes here is
+// exactly what the SDK will accept later and the two cannot disagree.
+// Reproducing the check by hand instead would cost this package its
+// dependency on the SDK — already a module dependency, already linked
+// into the binary — in exchange for a second definition of "valid DSN"
+// free to drift from the one that decides.
+//
+// A set value that does not parse is a hard error naming the key, so
+// startup fails loudly. Losing error reporting is the failure this
+// variable exists to prevent, and a typo in a DSN is silent forever:
+// nothing later in the process can notice that reports are going
+// nowhere. The bad value is quoted because it is a URL to a public
+// endpoint carrying a public key, not a secret.
+func envSentryDSN(key string) (string, error) {
+	v := strings.TrimSpace(os.Getenv(key))
+	if v == "" {
+		return "", nil
+	}
+
+	_, err := sentry.NewDsn(v)
+	if err != nil {
+		return "", fmt.Errorf(
+			"%w: %s: %q: %w", ErrInvalidSentryDSN, key, v, err,
+		)
+	}
+
+	return v, nil
+}
+
 // resolveMetricsAuth reads the /metrics basic-auth credentials and
 // rejects a half-set pair, naming both variables either way. The
 // error carries neither value: the password is a secret.
@@ -486,12 +586,14 @@ func resolveMetricsAuth() (string, string, error) {
 	)
 }
 
-// resolveEnvironment reads WEBHOOKER_ENVIRONMENT, defaulting to
+// resolveEnvironment reads WEBHOOKER_ENVIRONMENT, defaulting to prod
-// dev, and rejects unrecognised values.
+// when it is unset so a deployment that forgets the variable is not
+// silently permissive; dev must be set explicitly. It rejects
+// unrecognised values.
 func resolveEnvironment() (string, error) {
 	environment := os.Getenv("WEBHOOKER_ENVIRONMENT")
 	if environment == "" {
-		environment = EnvironmentDev
+		environment = EnvironmentProd
 	}
 
 	if environment != EnvironmentDev &&
@@ -594,6 +696,11 @@ func loadFromEnv() (*Config, error) {
 		return nil, err
 	}
 
+	sentryDSN, err := envSentryDSN("SENTRY_DSN")
+	if err != nil {
+		return nil, err
+	}
+
 	return &Config{
 		DataDir:                DataDir(),
 		Debug:                  debug,
@@ -603,7 +710,7 @@ func loadFromEnv() (*Config, error) {
 		MetricsPassword:        metricsPassword,
 		Port:                   port,
 		BindAddress:            bindAddress,
-		SentryDSN:              envString("SENTRY_DSN"),
+		SentryDSN:              sentryDSN,
 		RetentionSweepInterval: retentionSweepInterval,
 		SessionIdleTimeout:     sessionIdleTimeout,
 		ReceiverRateLimit:      receiverRateLimit,
@@ -640,12 +747,14 @@ func (c *Config) warnEgressAllowlist(log *slog.Logger) {
 
 	log.Warn(
 		"ALLOWED_EGRESS_CIDRS lets delivery targets reach these "+
-			"otherwise-blocked private/reserved networks. Anyone "+
+			"otherwise-blocked networks. Anyone who can create a "+
-			"who can create a delivery target can now make this "+
+			"delivery target can now make this process issue "+
-			"process issue requests into them, and read back the "+
+			"requests into them, and read back the response. Only "+
-			"response. Link-local and the known cloud instance "+
+			"the addresses the README lists as blocked "+
-			"metadata endpoints outside it stay blocked "+
+			"unconditionally stay blocked regardless of what is "+
-			"regardless of what is listed here.",
+			"listed here; a public cloud metadata address such as "+
+			"168.63.129.16 is reachable once it, or a block "+
+			"covering it, is listed.",
 		"allowedEgressCIDRs",
 		strings.Join(PrefixStrings(c.AllowedEgressCIDRs), ","),
 	)
@@ -668,10 +777,8 @@ 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. That
+// The warning is deliberately not gated on WEBHOOKER_ENVIRONMENT:
-// variable defaults to dev, so gating on it would silence the warning
+// behind a proxy every client shares one bucket in dev and prod alike.
-// 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 —
@@ -738,7 +845,7 @@ func New(lc fx.Lifecycle, params ConfigParams) (*Config, error) {
 		"receiverRateLimit", s.ReceiverRateLimit,
 		"trustedProxies", len(s.TrustedProxies),
 		"allowedEgressCIDRs", len(s.AllowedEgressCIDRs),
-		"hasSentryDSN", s.SentryDSN != "",
+		"sentryEnabled", s.SentryEnabled(),
 		"hasMetricsAuth", s.MetricsAuthEnabled(),
 	)
 

internal/config/config_test.go

@@ -44,9 +44,9 @@ func TestEnvironmentConfig(t *testing.T) {
 		isProd      bool
 	}{
 		{
-			name:   "default is dev",
+			name:   "default is prod",
-			isDev:  true,
+			isDev:  false,
-			isProd: false,
+			isProd: true,
 		},
 		{
 			name:     "explicit dev",
@@ -834,12 +834,13 @@ 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. Asserted on the clause naming the
+			// What stays shut is the whole unconditional set, not
-			// wider set rather than on "Link-local" alone, so the
+			// link-local alone; a public metadata address is not in
-			// string cannot narrow back to link-local only while
+			// it, so a listed block covering it opens it.
-			// the always-blocked set covers ULA, CGNAT and two
+			assert.Contains(t, logged, "blocked unconditionally")
-			// public metadata addresses as well.
+			assert.Contains(t, logged, "168.63.129.16 is reachable")
-			assert.Contains(t, logged, "metadata endpoints outside it")
+			// The listed blocks need not be private or reserved.
+			assert.NotContains(t, logged, "private/reserved")
 		})
 	}
 }
@@ -848,10 +849,9 @@ 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,
+// counting. It must fire whenever TRUSTED_PROXIES is empty, in any
-// in any environment: WEBHOOKER_ENVIRONMENT defaults to dev, so gating
+// environment, because behind a proxy every client shares one bucket
-// on it would silence the warning for exactly the operator who never
+// in dev and prod alike. It stays quiet once proxies are named.
-// configured the deployment. It stays quiet once proxies are named.
 func TestSharedRateLimitBucketWarning(t *testing.T) {
 	tests := []struct {
 		name           string
@@ -871,10 +871,6 @@ 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/config/dotenv_test.go

@@ -0,0 +1,158 @@
+package config_test
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/webhooker/internal/config"
+)
+
+// dotEnvKey is a throwaway variable name the .env tests write and
+// read, so they cannot disturb real configuration.
+const dotEnvKey = "WEBHOOKER_TEST_DOTENV_VALUE"
+
+// malformedDotEnv is a file godotenv cannot parse. The first line is
+// the realistic typo — a space where the `=` belongs — and the rest
+// make sure nothing downstream treats the file as salvageable line by
+// line.
+const malformedDotEnv = "PORT 19615\n" +
+	"this is not = valid ! syntax\n" +
+	"\"unclosed\n"
+
+// unsetDotEnvKey makes dotEnvKey genuinely absent for the duration of
+// the test and restores it afterwards. t.Setenv registers the restore;
+// the Unsetenv that follows is what the test actually needs, because a
+// variable set to the empty string is still present in os.Environ and
+// godotenv would refuse to overwrite it.
+func unsetDotEnvKey(t *testing.T) {
+	t.Helper()
+	t.Setenv(dotEnvKey, "placeholder")
+	require.NoError(t, os.Unsetenv(dotEnvKey))
+}
+
+// writeDotEnv writes contents to a .env file in a fresh temporary
+// directory and returns its path.
+func writeDotEnv(t *testing.T, contents string) string {
+	t.Helper()
+
+	path := filepath.Join(t.TempDir(), config.DotEnvPath)
+	require.NoError(t, os.WriteFile(path, []byte(contents), 0o600))
+
+	return path
+}
+
+// TestLoadDotEnv_MissingFileIsFine pins the case most deployments are
+// in. The file is optional: it is a development convenience, and a
+// deployment that configures the environment directly must start
+// normally rather than be refused for a file it was never meant to
+// have.
+//
+//nolint:paralleltest // unsetDotEnvKey uses t.Setenv.
+func TestLoadDotEnv_MissingFileIsFine(t *testing.T) {
+	unsetDotEnvKey(t)
+
+	absent := filepath.Join(t.TempDir(), config.DotEnvPath)
+	require.NoError(t, config.LoadDotEnvFileForTest(absent))
+
+	_, present := os.LookupEnv(dotEnvKey)
+	assert.False(t, present, "nothing may be set from an absent file")
+}
+
+// TestLoadDotEnv_AppliesValues pins that a well-formed file still
+// reaches the environment, which is the whole reason the file is read
+// at all.
+//
+//nolint:paralleltest // unsetDotEnvKey uses t.Setenv.
+func TestLoadDotEnv_AppliesValues(t *testing.T) {
+	unsetDotEnvKey(t)
+
+	path := writeDotEnv(t, "# a comment\n"+dotEnvKey+"=from-dot-env\n")
+
+	require.NoError(t, config.LoadDotEnvFileForTest(path))
+	assert.Equal(t, "from-dot-env", os.Getenv(dotEnvKey))
+}
+
+// TestLoadDotEnv_RealEnvironmentWins pins that the file cannot
+// override a variable the process was actually started with. A
+// deployment that sets DATA_DIR in its unit file must not have it
+// silently replaced by a stale .env left in the working directory.
+func TestLoadDotEnv_RealEnvironmentWins(t *testing.T) {
+	t.Setenv(dotEnvKey, "from-environment")
+
+	path := writeDotEnv(t, dotEnvKey+"=from-dot-env\n")
+
+	require.NoError(t, config.LoadDotEnvFileForTest(path))
+	assert.Equal(t, "from-environment", os.Getenv(dotEnvKey))
+}
+
+// TestLoadDotEnv_MalformedFileAborts is the defect this fixes. One bad
+// line makes godotenv apply none of the file, so every variable in it
+// reverts to its default; the process used to start that way with no
+// log line naming the file at all.
+//
+//nolint:paralleltest // unsetDotEnvKey uses t.Setenv.
+func TestLoadDotEnv_MalformedFileAborts(t *testing.T) {
+	unsetDotEnvKey(t)
+
+	path := writeDotEnv(
+		t, malformedDotEnv+dotEnvKey+"=from-dot-env\n",
+	)
+
+	err := config.LoadDotEnvFileForTest(path)
+
+	require.Error(t, err)
+	require.ErrorIs(t, err, config.ErrDotEnvUnreadable)
+	assert.Contains(
+		t, err.Error(), config.DotEnvPath,
+		"the failure must name the file it could not read",
+	)
+
+	_, present := os.LookupEnv(dotEnvKey)
+	assert.False(
+		t, present,
+		"a rejected file must apply nothing, not part of itself",
+	)
+}
+
+// TestLoadDotEnv_UnreadableFileAborts pins that only absence is
+// tolerated. A .env that exists but cannot be read is a file the
+// operator meant to be applied, so it fails like a malformed one
+// rather than being treated as though it were not there.
+func TestLoadDotEnv_UnreadableFileAborts(t *testing.T) {
+	t.Parallel()
+
+	// A directory in the file's place: open succeeds and the read
+	// fails, which no umask or root-ness can turn back into success
+	// the way a chmod could.
+	path := filepath.Join(t.TempDir(), config.DotEnvPath)
+	require.NoError(t, os.Mkdir(path, 0o750))
+
+	err := config.LoadDotEnvFileForTest(path)
+
+	require.Error(t, err)
+	require.ErrorIs(t, err, config.ErrDotEnvUnreadable)
+}
+
+// TestLoadDotEnv_ReadsTheWorkingDirectory pins the path LoadDotEnv
+// itself opens, which the tests above bypass. It is relative to the
+// process working directory, as it was under godotenv/autoload and as
+// the README documents.
+//
+//nolint:paralleltest // t.Chdir moves the whole process.
+func TestLoadDotEnv_ReadsTheWorkingDirectory(t *testing.T) {
+	unsetDotEnvKey(t)
+
+	dir := t.TempDir()
+	require.NoError(t, os.WriteFile(
+		filepath.Join(dir, config.DotEnvPath),
+		[]byte(dotEnvKey+"=from-working-directory\n"),
+		0o600,
+	))
+	t.Chdir(dir)
+
+	require.NoError(t, config.LoadDotEnv())
+	assert.Equal(t, "from-working-directory", os.Getenv(dotEnvKey))
+}

internal/config/env_test.go

@@ -518,8 +518,20 @@ type badEnvValueCase struct {
 }
 
 // badEnvValueCases is the config.New table, kept out of the test body
-// so the test itself stays readable.
+// so the test itself stays readable. It is assembled from per-variable
+// groups because one literal covering every variable outgrew the
+// function-length budget.
 func badEnvValueCases() []badEnvValueCase {
+	cases := listenerEnvValueCases()
+	cases = append(cases, flagEnvValueCases()...)
+	cases = append(cases, sentryEnvValueCases()...)
+
+	return cases
+}
+
+// listenerEnvValueCases covers the two variables that describe the
+// HTTP listener.
+func listenerEnvValueCases() []badEnvValueCase {
 	return []badEnvValueCase{
 		{
 			name:  "valid PORT is used",
@@ -542,27 +554,6 @@ func badEnvValueCases() []badEnvValueCase {
 			value:       "70000",
 			expectError: true,
 		},
-		{
-			name:  "valid DEBUG is used",
-			key:   envKeyDebug,
-			value: "true",
-			check: func(t *testing.T, cfg *config.Config) {
-				t.Helper()
-				assert.True(t, cfg.Debug)
-			},
-		},
-		{
-			name:        "unparseable DEBUG aborts startup",
-			key:         envKeyDebug,
-			value:       "ture",
-			expectError: true,
-		},
-		{
-			name:        "unparseable MAINTENANCE_MODE aborts startup",
-			key:         envKeyMaintenanceMode,
-			value:       "sometimes",
-			expectError: true,
-		},
 		{
 			name:  "valid BIND_ADDRESS is used",
 			key:   envKeyBindAddress,
@@ -595,6 +586,69 @@ func badEnvValueCases() []badEnvValueCase {
 	}
 }
 
+// flagEnvValueCases covers the boolean variables.
+func flagEnvValueCases() []badEnvValueCase {
+	return []badEnvValueCase{
+		{
+			name:  "valid DEBUG is used",
+			key:   envKeyDebug,
+			value: "true",
+			check: func(t *testing.T, cfg *config.Config) {
+				t.Helper()
+				assert.True(t, cfg.Debug)
+			},
+		},
+		{
+			name:        "unparseable DEBUG aborts startup",
+			key:         envKeyDebug,
+			value:       "ture",
+			expectError: true,
+		},
+		{
+			name:        "unparseable MAINTENANCE_MODE aborts startup",
+			key:         envKeyMaintenanceMode,
+			value:       "sometimes",
+			expectError: true,
+		},
+	}
+}
+
+// sentryEnvValueCases covers SENTRY_DSN. The three rejected values are
+// the ones measured on the defect: each initialised the SDK with an
+// error and left the process serving with error reporting off.
+func sentryEnvValueCases() []badEnvValueCase {
+	return []badEnvValueCase{
+		{
+			name:  "valid SENTRY_DSN is used",
+			key:   envKeySentryDSN,
+			value: validSentryDSN,
+			check: func(t *testing.T, cfg *config.Config) {
+				t.Helper()
+				assert.Equal(t, validSentryDSN, cfg.SentryDSN)
+				assert.True(t, cfg.SentryEnabled())
+			},
+		},
+		{
+			name:        "unparseable SENTRY_DSN aborts startup",
+			key:         envKeySentryDSN,
+			value:       "not-a-dsn",
+			expectError: true,
+		},
+		{
+			name:        "SENTRY_DSN that is not a URL aborts startup",
+			key:         envKeySentryDSN,
+			value:       "%%%",
+			expectError: true,
+		},
+		{
+			name:        "keyless SENTRY_DSN aborts startup",
+			key:         envKeySentryDSN,
+			value:       "https://example.invalid/1",
+			expectError: true,
+		},
+	}
+}
+
 // TestNewUsesDefaultsWhenUnset proves the fail-loud behaviour did not
 // break the legitimate unset case: absent variables still get their
 // documented defaults.
@@ -603,7 +657,7 @@ func TestNewUsesDefaultsWhenUnset(t *testing.T) {
 
 	for _, key := range []string{
 		envKeyPort, envKeyDebug, envKeyMaintenanceMode,
-		envKeyBindAddress,
+		envKeyBindAddress, envKeySentryDSN,
 	} {
 		require.NoError(t, os.Unsetenv(key))
 	}
@@ -626,4 +680,9 @@ func TestNewUsesDefaultsWhenUnset(t *testing.T) {
 		t, config.DefaultBindAddressForTest, cfg.BindAddress,
 	)
 	assert.Equal(t, bindAddressDefault, cfg.BindAddress)
+
+	// An absent SENTRY_DSN is the common case and must stay a normal
+	// start with error reporting off, not a refusal.
+	assert.Empty(t, cfg.SentryDSN)
+	assert.False(t, cfg.SentryEnabled())
 }

internal/config/export_test.go

@@ -51,6 +51,18 @@ func EnvPortForTest(key string, defaultValue int) (int, error) {
 	return envPort(key, defaultValue)
 }
 
+// EnvSentryDSNForTest exposes envSentryDSN.
+func EnvSentryDSNForTest(key string) (string, error) {
+	return envSentryDSN(key)
+}
+
+// LoadDotEnvFileForTest exposes the loader LoadDotEnv runs, over a
+// caller-named file rather than the process working directory, so
+// each .env state can be covered without moving the test process.
+func LoadDotEnvFileForTest(path string) error {
+	return loadDotEnvFile(path)
+}
+
 // EnvBindAddressForTest exposes envBindAddress.
 func EnvBindAddressForTest(key, defaultValue string) (string, error) {
 	return envBindAddress(key, defaultValue)

internal/config/sentry_test.go

@@ -0,0 +1,141 @@
+package config_test
+
+import (
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/webhooker/internal/config"
+)
+
+// envKeySentryDSN is the variable envSentryDSN reads in production.
+const envKeySentryDSN = "SENTRY_DSN"
+
+// validSentryDSN is a syntactically complete DSN. The host is under
+// .invalid (RFC 2606), so nothing a test builds around it can reach a
+// real Sentry installation.
+const validSentryDSN = "https://abc123@sentry.invalid/42"
+
+// envSentryDSNCase is one row of the envSentryDSN table.
+type envSentryDSNCase struct {
+	name        string
+	set         bool
+	value       string
+	expectError bool
+	expected    string
+}
+
+// envSentryDSNCases is the envSentryDSN table. The three invalid
+// values are the ones measured on the defect: each initialised the SDK
+// with an error and left the process serving with reporting off.
+func envSentryDSNCases() []envSentryDSNCase {
+	return []envSentryDSNCase{
+		{
+			name:     "unset means reporting off",
+			expected: "",
+		},
+		{
+			name:     "empty means reporting off",
+			set:      true,
+			value:    "",
+			expected: "",
+		},
+		{
+			name:     "whitespace means reporting off",
+			set:      true,
+			value:    "   ",
+			expected: "",
+		},
+		{
+			name:     "a valid DSN is kept",
+			set:      true,
+			value:    validSentryDSN,
+			expected: validSentryDSN,
+		},
+		{
+			name:     "surrounding whitespace is trimmed",
+			set:      true,
+			value:    "  " + validSentryDSN + "\t",
+			expected: validSentryDSN,
+		},
+		{
+			name:        "a value that is not a URL is rejected",
+			set:         true,
+			value:       "not-a-dsn",
+			expectError: true,
+		},
+		{
+			name:        "an unparseable URL is rejected",
+			set:         true,
+			value:       "%%%",
+			expectError: true,
+		},
+		{
+			name:        "a DSN without a public key is rejected",
+			set:         true,
+			value:       "https://example.invalid/1",
+			expectError: true,
+		},
+		{
+			name:        "a DSN without a project id is rejected",
+			set:         true,
+			value:       "https://abc123@sentry.invalid/",
+			expectError: true,
+		},
+		{
+			name:        "a non-HTTP scheme is rejected",
+			set:         true,
+			value:       "ftp://abc123@sentry.invalid/42",
+			expectError: true,
+		},
+	}
+}
+
+// TestEnvSentryDSN covers the helper directly. What it pins beyond the
+// value is the failure shape: a set-but-unparseable DSN names the
+// variable and the value, exactly as the other fail-loud helpers do,
+// so an operator reads the fix off the message.
+func TestEnvSentryDSN(t *testing.T) {
+	for _, tt := range envSentryDSNCases() {
+		t.Run(tt.name, func(t *testing.T) {
+			// Cannot use t.Parallel() here because t.Setenv
+			// is incompatible with parallel subtests.
+			if tt.set {
+				t.Setenv(envKeySentryDSN, tt.value)
+			} else {
+				require.NoError(t, os.Unsetenv(envKeySentryDSN))
+			}
+
+			got, err := config.EnvSentryDSNForTest(envKeySentryDSN)
+
+			if tt.expectError {
+				require.Error(t, err)
+				require.ErrorIs(t, err, config.ErrInvalidSentryDSN)
+				assert.Contains(t, err.Error(), envKeySentryDSN)
+				assert.Contains(t, err.Error(), tt.value)
+				assert.Empty(t, got)
+
+				return
+			}
+
+			require.NoError(t, err)
+			assert.Equal(t, tt.expected, got)
+		})
+	}
+}
+
+// TestSentryEnabled_TracksTheDSN pins that the one method answering
+// "is anything being reported" agrees with the DSN in every state. The
+// startup log, the SDK initialisation and the sentryhttp middleware
+// all read it, so a log field cannot report reporting as on while
+// nothing is sending.
+func TestSentryEnabled_TracksTheDSN(t *testing.T) {
+	t.Parallel()
+
+	assert.False(t, (&config.Config{}).SentryEnabled())
+	assert.True(
+		t,
+		(&config.Config{SentryDSN: validSentryDSN}).SentryEnabled(),
+	)
+}

internal/database/bootstrap_banner_test.go

@@ -3,6 +3,8 @@ package database_test
 import (
 	"bytes"
 	"context"
+	"log/slog"
+	"path/filepath"
 	"strings"
 	"testing"
 
@@ -83,3 +85,37 @@ 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,6 +8,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"io/fs"
 	"log/slog"
 	"os"
 	"path/filepath"
@@ -17,12 +18,12 @@ import (
 	"gorm.io/gorm"
 	"sneak.berlin/go/webhooker/internal/banner"
 	"sneak.berlin/go/webhooker/internal/config"
+	"sneak.berlin/go/webhooker/internal/datadir"
 	"sneak.berlin/go/webhooker/internal/gormlog"
 	"sneak.berlin/go/webhooker/internal/logger"
 )
 
 const (
-	dataDirPerm       = 0750
 	randomPasswordLen = 16
 	sessionKeyLen     = 32
 )
@@ -185,7 +186,9 @@ func (d *Database) connect() error {
 // caller's decision.
 func (d *Database) connectTo(dataDir string) error {
 	// Ensure the data directory exists before opening the database.
-	err := os.MkdirAll(dataDir, dataDirPerm)
+	// datadir.DirPerm is the single source of the directory mode; this
+	// package creates the directory too, since either may run first.
+	err := os.MkdirAll(dataDir, datadir.DirPerm)
 	if err != nil {
 		return fmt.Errorf(
 			"creating data directory %s: %w",
@@ -197,6 +200,12 @@ 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.
@@ -227,7 +236,12 @@ func (d *Database) connectTo(dataDir string) error {
 	}
 
 	d.db = db
-	d.log.Info("connected to database", "path", dbPath)
+
+	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

@@ -0,0 +1,166 @@
+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"
+)
+
+// TestWebhookDBManager_OpenAddsEventTierIndexes verifies that opening a
+// per-webhook database that predates these indexes creates them. 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"},
+	}
+
+	mgr, lc := setupTestWebhookDBManager(t)
+	ctx := context.Background()
+	require.NoError(t, lc.Start(ctx))
+
+	defer func() { require.NoError(t, lc.Stop(ctx)) }()
+
+	webhookID := uuid.New().String()
+
+	db, err := mgr.GetDB(webhookID)
+	require.NoError(t, err)
+
+	// A fresh database has them.
+	for _, ix := range indexes {
+		require.True(t, db.Migrator().HasIndex(ix.model, ix.name))
+	}
+
+	// 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))
+	}
+
+	// Drop the cached connection so the next open reopens the file and
+	// runs AutoMigrate against it, as a restart would.
+	require.NoError(t, mgr.CloseAll())
+
+	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())
+	}
+}

internal/database/model_delivery.go

@@ -1,5 +1,7 @@
 package database
 
+import "gorm.io/gorm"
+
 // DeliveryStatus represents the status of a delivery
 type DeliveryStatus string
 
@@ -29,12 +31,19 @@ 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"         json:"eventId"`
+	EventID  string         `gorm:"type:uuid;not null;index:idx_deliveries_event_id,priority:1"       json:"eventId"`
-	TargetID string         `gorm:"type:uuid;not null"         json:"targetId"`
+	TargetID string         `gorm:"type:uuid;not null"                                                json:"targetId"`
-	Status   DeliveryStatus `gorm:"not null;default:'pending'" json:"status"`
+	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"`
 
 	// Relations
 	Event           Event            `json:"event,omitzero"`

internal/database/model_delivery_result.go

@@ -1,10 +1,21 @@
 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   string `gorm:"type:uuid;not null"   json:"deliveryId"`
+	// 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"`
+
 	AttemptNum   int    `gorm:"not null"             json:"attemptNum"`
 	Success      bool   `json:"success"`
 	StatusCode   int    `json:"statusCode,omitempty"`

internal/database/model_event.go

@@ -1,9 +1,27 @@
 package database
 
+import (
+	"time"
+
+	"gorm.io/gorm"
+)
+
 // 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"`
+
 	WebhookID    string `gorm:"type:uuid;not null" json:"webhookId"`
 	EntrypointID string `gorm:"type:uuid;not null" json:"entrypointId"`
 

internal/database/sqlite_mode_test.go

@@ -93,11 +93,19 @@ func TestMainDatabaseFilesAreOwnerOnly(t *testing.T) {
 		t, filepath.Join(dataDir, database.MainDBFileName),
 	)
 
-	// The data directory stays group-readable. Deployments may rely on
+	// The data directory grants nothing to `other`. Asserted as a
-	// the group bit; the file mode is the barrier, not the directory.
+	// property rather than as an exact 0750, because MkdirAll applies
+	// the ambient umask: the exact mode is the developer's umask as
+	// much as the application's request, and pinning it would make
+	// `make check` pass or fail on where it is run. The group bits are
+	// deliberately left unasserted — deployments may rely on them.
 	info, err := os.Stat(dataDir)
 	require.NoError(t, err)
-	assert.Equal(t, fs.FileMode(0o750), info.Mode().Perm())
+	assert.Zero(
+		t,
+		info.Mode().Perm()&0o007,
+		"the data directory must not be world-accessible",
+	)
 }
 
 // TestPerWebhookEventDatabaseFilesAreOwnerOnly covers the events-*.db

internal/database/webhook_db_manager.go

@@ -13,6 +13,7 @@ import (
 	"gorm.io/driver/sqlite"
 	"gorm.io/gorm"
 	"sneak.berlin/go/webhooker/internal/config"
+	"sneak.berlin/go/webhooker/internal/datadir"
 	"sneak.berlin/go/webhooker/internal/gormlog"
 	"sneak.berlin/go/webhooker/internal/logger"
 )
@@ -40,6 +41,11 @@ 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
@@ -53,8 +59,9 @@ func NewWebhookDBManager(
 		log:     params.Logger.Get(),
 	}
 
-	// Create data directory if it doesn't exist
+	// Create data directory if it doesn't exist. datadir.DirPerm is the
-	err := os.MkdirAll(m.dataDir, dataDirPerm)
+	// single source of the directory mode; either package may run first.
+	err := os.MkdirAll(m.dataDir, datadir.DirPerm)
 	if err != nil {
 		return nil, fmt.Errorf(
 			"creating data directory %s: %w",
@@ -84,43 +91,39 @@ func (m *WebhookDBManager) GetDB(
 ) (*gorm.DB, error) {
 	// Fast path: already open
 	if val, ok := m.dbs.Load(webhookID); ok {
-		cachedDB, castOK := val.(*gorm.DB)
+		return asGormDB(val, webhookID)
-		if !castOK {
+	}
-			return nil, fmt.Errorf(
-				"%w for webhook %s",
-				errInvalidCachedDBType,
-				webhookID,
-			)
-		}
 
-		return cachedDB, nil
+	// 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)
 	}
 
-	// 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
+	m.dbs.Store(webhookID, db)
-	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)
+	return db, nil
-		if !castOK {
+}
-			return nil, fmt.Errorf(
-				"%w for webhook %s",
-				errInvalidCachedDBType,
-				webhookID,
-			)
-		}
 
-		return existingDB, 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 {
+		return nil, fmt.Errorf(
+			"%w for webhook %s",
+			errInvalidCachedDBType,
+			webhookID,
+		)
 	}
 
 	return db, nil
@@ -151,6 +154,11 @@ 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 {
@@ -184,6 +192,11 @@ 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,10 +1,14 @@
 package database_test
 
 import (
+	"bytes"
 	"context"
+	"log/slog"
 	"net/http"
 	"os"
 	"path/filepath"
+	"strings"
+	"sync"
 	"testing"
 
 	"github.com/google/uuid"
@@ -104,6 +108,54 @@ 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/datadir/lock.go

@@ -29,9 +29,11 @@ import (
 // process that was killed with SIGKILL blocks nothing.
 const LockFileName = "webhooker.lock"
 
-// dirPerm is the mode Acquire creates DATA_DIR with. It matches what
+// DirPerm is the mode DATA_DIR is created with. It is the single
-// internal/database uses, since whichever runs first creates it.
+// source of that mode: internal/database consumes it rather than
-const dirPerm = 0o750
+// keeping its own copy, so the two packages that both create the
+// directory cannot drift into disagreeing about its permissions.
+const DirPerm = 0o750
 
 // ErrLocked reports that another live process holds the data
 // directory. Callers that need to know whether a deployment is running
@@ -64,7 +66,7 @@ func Acquire(dir string) (*Lock, error) {
 		return nil, ErrNoDir
 	}
 
-	err := os.MkdirAll(dir, dirPerm)
+	err := os.MkdirAll(dir, DirPerm)
 	if err != nil {
 		return nil, fmt.Errorf(
 			"creating data directory %s: %w", dir, err,

internal/delivery/circuit_breaker.go

@@ -76,12 +76,20 @@ func (cb *CircuitBreaker) Allow() bool {
 	}
 }
 
-// CooldownRemaining returns how much time is left before
+// CooldownRemaining returns how long a delivery that Allow refused
-// an open circuit transitions to half-open.
+// 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.
 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_HalfOpenReturnsZero(
+func TestCircuitBreaker_CooldownRemaining_HalfOpenReturnsCooldown(
 	t *testing.T,
 ) {
 	t.Parallel()
@@ -282,9 +282,11 @@ func TestCircuitBreaker_CooldownRemaining_HalfOpenReturnsZero(
 
 	require.True(t, cb.Allow())
 
-	assert.Equal(t, time.Duration(0),
+	// The cooldown newShortCooldownCB gives the breaker.
+	assert.Equal(t, 50*time.Millisecond,
 		cb.CooldownRemaining(),
-		"half-open circuit should have zero cooldown remaining",
+		"a delivery refused while half-open should wait "+
+			"a whole cooldown",
 	)
 }
 

internal/delivery/engine.go

@@ -4,6 +4,7 @@ package delivery
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"log/slog"
 	"net/http"
@@ -361,6 +362,15 @@ 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")
 
@@ -375,6 +385,8 @@ func (e *Engine) stop(ctx context.Context) error {
 		return err
 	}
 
+	e.dbTarget.evictAll()
+
 	e.log.Info("delivery engine stopped")
 
 	return nil
@@ -437,9 +449,34 @@ 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.resolveEventBody(
+	event, err = e.hydrateEvent(
 		webhookDB, event, task,
 	)
 	if err != nil {
@@ -481,7 +518,7 @@ func (e *Engine) processRetryTask(
 		return
 	}
 
-	d, err := e.loadRetryDelivery(
+	d, err := e.loadDelivery(
 		webhookDB, task.DeliveryID,
 	)
 	if err != nil {
@@ -505,9 +542,13 @@ func (e *Engine) processRetryTask(
 		return
 	}
 
+	if e.abandonRetryForMissingTarget(webhookDB, d, task) {
+		return
+	}
+
 	event := buildEventFromTask(task)
 
-	event, err = e.resolveEventBody(
+	event, err = e.hydrateEvent(
 		webhookDB, event, task,
 	)
 	if err != nil {
@@ -529,6 +570,64 @@ func (e *Engine) processRetryTask(
 	e.processDelivery(ctx, webhookDB, d, task)
 }
 
+// abandonRetryForMissingTarget stops a retry chain whose target has
+// been deleted, and reports whether it did.
+//
+// A scheduled retry lives in memory as a time.AfterFunc holding the
+// target's configuration as it was when the chain began, and nothing
+// else on this path reads the target row. Without this check a
+// deletion stops nothing: the timer keeps firing and keeps sending to
+// the destination the operator removed, for the whole remaining
+// backoff chain. Terminalising in the recovery and sweep paths alone
+// is not enough, because those only see the delivery once nothing
+// holds it in memory — which is to say after a restart.
+//
+// The worker already owns this delivery, so the terminal write happens
+// here directly, exactly as a target's own Deliver fails one. Claiming
+// it again through the recovery gate would only fail against the
+// reference the worker itself is holding.
+//
+// A lookup that fails for any other reason is not a deletion — it is
+// the main database being unreadable — and the delivery goes ahead as
+// it did before. A guard that terminally failed deliveries on a
+// transient fault would be worse than the bug it fixes.
+func (e *Engine) abandonRetryForMissingTarget(
+	webhookDB *gorm.DB,
+	d *database.Delivery,
+	task *Task,
+) bool {
+	_, err := e.loadTarget(task.TargetID)
+	if err == nil {
+		return false
+	}
+
+	if !errors.Is(err, gorm.ErrRecordNotFound) {
+		e.log.Warn(
+			"could not confirm the target of a retrying "+
+				"delivery still exists; attempting anyway",
+			"delivery_id", task.DeliveryID,
+			"target_id", task.TargetID,
+			"error", err,
+		)
+
+		return false
+	}
+
+	targetType, reason := e.missingTargetReason(task.TargetID)
+
+	e.log.Warn(
+		"abandoning scheduled retry: target is gone",
+		"webhook_id", task.WebhookID,
+		"delivery_id", task.DeliveryID,
+		"target_id", task.TargetID,
+		"target_type", targetType,
+	)
+
+	e.failDelivery(webhookDB, d, targetType, reason)
+
+	return true
+}
+
 func (e *Engine) recoverInFlight(ctx context.Context) {
 	var webhookIDs []string
 
@@ -633,6 +732,18 @@ func (e *Engine) recoverSingleRetry(
 ) {
 	target, err := e.loadTarget(d.TargetID)
 	if err != nil {
+		// A target that is merely gone is an operator action with a
+		// terminal answer. Any other failure is the main database
+		// refusing to read, which is transient and must leave the
+		// delivery alone: failing every retrying delivery of every
+		// 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)
+
+			return
+		}
+
 		e.log.Error(
 			"failed to load target for retrying "+
 				"delivery recovery",
@@ -1028,6 +1139,14 @@ func (e *Engine) sweepSingleRetry(
 ) {
 	target, err := e.loadTarget(d.TargetID)
 	if err != nil {
+		// Deleted is terminal, unreadable is not; see
+		// recoverSingleRetry.
+		if errors.Is(err, gorm.ErrRecordNotFound) {
+			e.failMissingTarget(webhookDB, webhookID, d)
+
+			return
+		}
+
 		e.log.Error(
 			"retry sweep: failed to load target",
 			"delivery_id", d.ID,
@@ -1134,6 +1253,136 @@ func (e *Engine) failUnretryableRetry(
 		target.Type,
 	)
 
+	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.
+//
+// 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
+// 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(
+	webhookDB *gorm.DB,
+	webhookID string,
+	d *database.Delivery,
+) {
+	// Terminal, and reached from the recovery paths, so it takes
+	// ownership like every other write they make.
+	if !e.inflight.retainIdle(d.ID) {
+		return
+	}
+
+	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",
+		"webhook_id", webhookID,
+		"delivery_id", d.ID,
+		"status", d.Status,
+		"target_id", d.TargetID,
+		"target_type", targetType,
+	)
+
+	e.failDelivery(webhookDB, d, targetType, reason)
+}
+
+// missingTargetReason describes a target id that no longer resolves,
+// and returns the type of the deleted row where there still is one.
+//
+// The lookup is Unscoped because deletes are soft: the row survives
+// with deleted_at set, invisible to loadTarget's default scope.
+// Reading it is what separates "you deleted this target" from "this id
+// never named a row" — different things to whoever reads the event
+// log, and only the first is something an operator did. The widened
+// scope is deliberately confined to this terminal path: the engine's
+// normal target loading must go on refusing a deleted target, or
+// deleting one would stop nothing.
+//
+// The type comes back so the caller can label the delivery's status
+// transition with it. Where the row is gone entirely there is no type
+// to give, and updateDeliveryStatus leaves the counter alone rather
+// than opening a series named by the empty string.
+func (e *Engine) missingTargetReason(
+	targetID string,
+) (database.TargetType, string) {
+	var target database.Target
+
+	err := e.database.DB().Unscoped().
+		First(&target, "id = ?", targetID).Error
+	if err != nil {
+		return "", fmt.Sprintf(
+			"target %s no longer exists; the delivery "+
+				"has been failed terminally",
+			targetID,
+		)
+	}
+
+	return target.Type, fmt.Sprintf(
+		"target %q (type %s) was deleted; the delivery "+
+			"has been failed terminally",
+		target.Name, target.Type,
+	)
+}
+
+// failDelivery records why a delivery is over and then marks it
+// failed. The caller must already own the delivery: every call site is
+// either a worker holding the reference runTask took, or a recovery
+// path that took one through retainIdle.
+//
+// The result row is written first and a failure to write it stops the
+// transition, which is what keeps a delivery from ending failed with
+// an empty event log — the state that leaves an operator with nothing
+// but a server log line to work out what happened. A delivery whose
+// reason could not be recorded stays in the non-terminal state it
+// already holds, where the sweep will find it again; see
+// bookkeepingFailed.
+//
+// The target type is a parameter rather than read off d because the
+// orphaned-retry callers deliberately hold a delivery loaded without
+// its Target relation: populating d.Target would make GORM's
+// SaveBeforeAssociations upsert the whole target row — plaintext
+// config, which for a slack target is the credential — into the
+// per-webhook event database. See
+// https://git.eeqj.de/sneak/webhooker/issues/206.
+func (e *Engine) failDelivery(
+	webhookDB *gorm.DB,
+	d *database.Delivery,
+	targetType database.TargetType,
+	reason string,
+) {
 	err := e.recordResult(
 		webhookDB,
 		d,
@@ -1150,14 +1399,8 @@ func (e *Engine) failUnretryableRetry(
 		return
 	}
 
-	// The type is passed rather than assigned onto d: the delivery
-	// is loaded here without its target relation, and populating
-	// d.Target would make GORM's SaveBeforeAssociations upsert the
-	// whole target row — plaintext config, which for a slack target
-	// is the credential — into the per-webhook event database. See
-	// https://git.eeqj.de/sneak/webhooker/issues/206.
 	e.settleStatus(
-		webhookDB, d, target.Type,
+		webhookDB, d, targetType,
 		database.DeliveryStatusFailed,
 	)
 }
@@ -1178,9 +1421,19 @@ func (e *Engine) processDelivery(
 			"type", d.Target.Type,
 		)
 
-		e.settleStatus(
+		// The reason is recorded, not just logged. This branch used
+		// to fail the delivery with no DeliveryResult at all, which
+		// showed in the event log as "failed, no attempts recorded
+		// yet" and left one server log line as the only account of
+		// why anywhere.
+		e.failDelivery(
 			webhookDB, d, d.Target.Type,
-			database.DeliveryStatusFailed,
+			fmt.Sprintf(
+				"unknown target type %q: this build has no "+
+					"delivery implementation for it, so no "+
+					"attempt was made",
+				d.Target.Type,
+			),
 		)
 
 		return
@@ -1349,6 +1602,11 @@ func truncate(s string, maxLen int) string {
 
 // --- Helper functions ---
 
+// buildEventFromTask reconstructs the event a Task describes, as far
+// as the Task itself goes. The fields it cannot fill — the body when
+// it was too large to inline, and the receipt time, which no Task
+// carries — come from the stored row in hydrateEvent, which every
+// caller of this function runs next.
 func buildEventFromTask(task *Task) database.Event {
 	event := database.Event{
 		EntrypointID: task.EntrypointID,
@@ -1376,33 +1634,71 @@ func buildTargetFromTask(task *Task) database.Target {
 	return target
 }
 
-func (e *Engine) resolveEventBody(
+// hydrateEvent fills in the event fields a Task does not carry, by
+// reading the stored event row.
+//
+// CreatedAt is the event's receipt time and lives only in that row.
+// The Slack target renders it into every message it sends, so an
+// unhydrated event puts the zero time in front of a human on every
+// notification the product delivers. See
+// https://git.eeqj.de/sneak/webhooker/issues/257.
+//
+// The body comes from the same row when the Task did not inline it,
+// which is the case for a body at or above MaxInlineBodySize.
+//
+// A read failure is fatal to the delivery only when the body depended
+// on it. When the Task inlined the body, the delivery has everything
+// it needs to be sent and goes ahead with the timestamp unset: the row
+// can be gone under a retention reap while a queued delivery still
+// holds its body, and dropping a deliverable event to protect one
+// metadata field would be a worse failure than the one it prevents.
+func (e *Engine) hydrateEvent(
 	webhookDB *gorm.DB,
 	event database.Event,
 	task *Task,
 ) (database.Event, error) {
-	if task.Body != nil {
+	columns := []string{"created_at"}
+
+	if task.Body == nil {
+		columns = append(columns, "body")
+	}
+
+	var dbEvent database.Event
+
+	err := webhookDB.Select(columns).
+		First(&dbEvent, "id = ?", task.EventID).Error
+	if err != nil {
+		if task.Body == nil {
+			return event, fmt.Errorf(
+				"fetching event body: %w", err,
+			)
+		}
+
+		e.log.Warn(
+			"could not read the stored event; delivering "+
+				"the inlined body without its receipt time",
+			"event_id", task.EventID,
+			"delivery_id", task.DeliveryID,
+			"error", err,
+		)
+
 		event.Body = *task.Body
 
 		return event, nil
 	}
 
-	var dbEvent database.Event
+	event.CreatedAt = dbEvent.CreatedAt
 
-	err := webhookDB.Select("body").
+	if task.Body != nil {
-		First(&dbEvent, "id = ?", task.EventID).Error
+		event.Body = *task.Body
-	if err != nil {
+	} else {
-		return event, fmt.Errorf(
+		event.Body = dbEvent.Body
-			"fetching event body: %w", err,
-		)
 	}
 
-	event.Body = dbEvent.Body
-
 	return event, nil
 }
 
-func (e *Engine) loadRetryDelivery(
+func (e *Engine) loadDelivery(
 	webhookDB *gorm.DB, deliveryID string,
 ) (*database.Delivery, error) {
 	var d database.Delivery
@@ -1755,13 +2051,30 @@ func (e *Engine) sendRecoveredDeliveries(
 
 		target, ok := targetMap[deliveries[i].TargetID]
 		if !ok {
-			e.log.Error(
+			// A missing entry does not mean the target is gone: the
-				"target not found for delivery",
+			// map is also empty when its query failed. Only a lookup
-				"delivery_id", deliveries[i].ID,
+			// that finds no row ends the delivery; any other error
-				"target_id", deliveries[i].TargetID,
+			// leaves it pending for the next sweep. See
-			)
+			// recoverSingleRetry.
+			var err error
 
-			continue
+			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",
+					"delivery_id", deliveries[i].ID,
+					"target_id", deliveries[i].TargetID,
+					"error", err,
+				)
+
+				continue
+			}
 		}
 
 		if !e.takeForRedispatch(

internal/delivery/engine_integration_test.go

@@ -377,6 +377,17 @@ func TestProcessRetryTask_SuccessfulRetry(t *testing.T) {
 
 	bodyStr := event.Body
 	cfg := iHTTPConfig(ts.URL)
+
+	// The target row exists because the engine confirms a scheduled
+	// retry's target has not been deleted before it runs it. A retry
+	// task whose target id names no row at all is a state the service
+	// does not produce: the handler read that target to build the
+	// task. See https://git.eeqj.de/sneak/webhooker/issues/107.
+	iCreateTarget(
+		t, s.MainDB, targetID, s.WebhookID, "retry-target",
+		database.TargetTypeHTTP, cfg, 5,
+	)
+
 	task := iTask(
 		d, event, s.WebhookID, targetID,
 		"retry-target", cfg, 5, 2, &bodyStr,
@@ -456,6 +467,12 @@ func TestProcessRetryTask_LargeBody_FetchFromDB(
 	)
 
 	cfg := iHTTPConfig(ts.URL)
+
+	iCreateTarget(
+		t, s.MainDB, targetID, s.WebhookID, "retry-large",
+		database.TargetTypeHTTP, cfg, 5,
+	)
+
 	task := iTask(
 		d, event, s.WebhookID, targetID,
 		"retry-large", cfg, 5, 2, nil,
@@ -558,6 +575,12 @@ func TestWorkerLifecycle_ProcessesRetryChannel(
 
 	bodyStr := event.Body
 	cfg := iHTTPConfig(ts.URL)
+
+	iCreateTarget(
+		t, s.MainDB, targetID, s.WebhookID, "retry-chan-test",
+		database.TargetTypeHTTP, cfg, 5,
+	)
+
 	task := iTask(
 		d, event, s.WebhookID, targetID,
 		"retry-chan-test", cfg, 5, 2, &bodyStr,

internal/delivery/engine_lifecycle_test.go

@@ -2,6 +2,8 @@ package delivery_test
 
 import (
 	"context"
+	"fmt"
+	"path/filepath"
 	"testing"
 	"time"
 
@@ -269,3 +271,88 @@ 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,6 +17,7 @@ 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"
@@ -24,6 +25,7 @@ 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.
@@ -894,6 +896,100 @@ 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()
 
@@ -1070,6 +1166,10 @@ func TestIsForwardableHeader(t *testing.T) {
 	assert.False(t,
 		delivery.ExportIsForwardableHeader("Content-Length"),
 	)
+
+	assert.False(t,
+		delivery.ExportIsForwardableHeader("Content-Type"),
+	)
 }
 
 func TestTruncate(t *testing.T) {
@@ -1151,6 +1251,81 @@ 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/event_db_isolation_test.go

@@ -96,7 +96,15 @@ func TestEventDBHoldsNoTargetRows(t *testing.T) {
 	)
 	assertNoTargetRows(t, dbPath)
 
-	// A retry.
+	// A retry. Its target exists in the main database, because the
+	// engine confirms a scheduled retry's target has not been
+	// deleted before running it; see
+	// https://git.eeqj.de/sneak/webhooker/issues/107.
+	iCreateTarget(
+		t, s.MainDB, targetID, s.WebhookID, "leaky-target",
+		database.TargetTypeHTTP, cfg, 5,
+	)
+
 	rd := iSeedDelivery(
 		t, s.WebhookDB, event.ID, targetID,
 		database.DeliveryStatusRetrying,

internal/delivery/event_timestamp_test.go

@@ -0,0 +1,442 @@
+package delivery_test
+
+import (
+	"context"
+	"encoding/json"
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"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"
+	"sneak.berlin/go/webhooker/internal/delivery"
+)
+
+// tsEventCreatedAt is the receipt time seeded on the events these
+// tests deliver. It is far enough from both the zero time and from
+// now that neither can be mistaken for it.
+func tsEventCreatedAt() time.Time {
+	return time.Date(
+		2026, time.March, 4, 5, 6, 7, 0, time.UTC,
+	)
+}
+
+// tsZeroStamp is what a Slack message renders when the event handed
+// to FormatSlackMessage carries no CreatedAt.
+const tsZeroStamp = "*Timestamp:* `0001-01-01T00:00:00Z`"
+
+// tsEventBody is the body seeded on every event in this file. It is
+// small enough that a Task can inline it.
+const tsEventBody = `{"hello":"world"}`
+
+// tsUndeliverableHook stands in for a Slack incoming webhook on the
+// tests that never send: the config parser requires a URL, but no
+// request is made.
+const tsUndeliverableHook = "https://hooks.slack.com/services/T/B/x"
+
+// tsSink is a stand-in Slack incoming webhook that records the raw
+// body posted to it.
+type tsSink struct {
+	*httptest.Server
+
+	bodies chan []byte
+}
+
+func newTSSink(t *testing.T) *tsSink {
+	t.Helper()
+
+	s := &tsSink{bodies: make(chan []byte, 8)}
+
+	s.Server = httptest.NewServer(http.HandlerFunc(
+		func(w http.ResponseWriter, r *http.Request) {
+			body, _ := io.ReadAll(r.Body)
+
+			select {
+			case s.bodies <- body:
+			default:
+			}
+
+			w.WriteHeader(http.StatusOK)
+		},
+	))
+
+	t.Cleanup(s.Close)
+
+	return s
+}
+
+// text returns the Slack message text from the single payload the
+// sink received.
+func (s *tsSink) text(t *testing.T) string {
+	t.Helper()
+
+	select {
+	case raw := <-s.bodies:
+		t.Logf("raw slack payload: %s", raw)
+
+		var payload struct {
+			Text string `json:"text"`
+		}
+
+		require.NoError(t, json.Unmarshal(raw, &payload))
+
+		return payload.Text
+	case <-time.After(5 * time.Second):
+		t.Fatal("slack sink received no payload")
+
+		return ""
+	}
+}
+
+func tsSlackConfig(t *testing.T, url string) string {
+	t.Helper()
+
+	data, err := json.Marshal(
+		delivery.SlackTargetConfig{WebhookURL: url},
+	)
+	require.NoError(t, err)
+
+	return string(data)
+}
+
+// tsSeedEvent writes an event whose CreatedAt is tsEventCreatedAt
+// rather than the write time, so an assertion on the rendered
+// timestamp cannot pass by accident against "roughly now".
+func tsSeedEvent(
+	t *testing.T, db *gorm.DB, webhookID string,
+) database.Event {
+	t.Helper()
+
+	event := database.Event{
+		WebhookID:    webhookID,
+		EntrypointID: uuid.New().String(),
+		Method:       http.MethodPost,
+		Headers:      `{}`,
+		Body:         tsEventBody,
+		ContentType:  "application/json",
+	}
+	event.ID = uuid.New().String()
+	event.CreatedAt = tsEventCreatedAt()
+	event.UpdatedAt = tsEventCreatedAt()
+
+	require.NoError(t, db.Create(&event).Error)
+
+	var stored database.Event
+
+	require.NoError(t,
+		db.First(&stored, "id = ?", event.ID).Error,
+	)
+	require.Equal(t,
+		tsEventCreatedAt().UTC(), stored.CreatedAt.UTC(),
+		"seeded created_at did not round-trip",
+	)
+
+	return event
+}
+
+// tsSeedTarget writes the slack target row into the main database.
+// The retry path confirms the target still exists before sending.
+func tsSeedTarget(
+	t *testing.T, mainDB *gorm.DB, webhookID, config string,
+) database.Target {
+	t.Helper()
+
+	target := database.Target{
+		WebhookID: webhookID,
+		Name:      "slack-sink",
+		Type:      database.TargetTypeSlack,
+		Config:    config,
+		Active:    true,
+	}
+
+	require.NoError(t, mainDB.Create(&target).Error)
+
+	return target
+}
+
+func tsTask(
+	d database.Delivery,
+	event database.Event,
+	webhookID string,
+	target database.Target,
+	attemptNum int,
+	body *string,
+) delivery.Task {
+	return delivery.Task{
+		DeliveryID:   d.ID,
+		EventID:      event.ID,
+		WebhookID:    webhookID,
+		EntrypointID: event.EntrypointID,
+		TargetID:     target.ID,
+		TargetName:   target.Name,
+		TargetType:   database.TargetTypeSlack,
+		TargetConfig: target.Config,
+		MaxRetries:   0,
+		Method:       event.Method,
+		Headers:      event.Headers,
+		ContentType:  event.ContentType,
+		Body:         body,
+		AttemptNum:   attemptNum,
+	}
+}
+
+func tsAssertRealTimestamp(t *testing.T, text string) {
+	t.Helper()
+
+	assert.NotContains(t, text, tsZeroStamp,
+		"slack message carries the zero timestamp",
+	)
+	assert.Contains(t, text,
+		"*Timestamp:* `"+
+			tsEventCreatedAt().UTC().Format(time.RFC3339)+"`",
+		"slack message does not carry the event's receipt time",
+	)
+}
+
+// tsCase is one end-to-end delivery of a seeded event to a slack
+// sink, over whichever engine path `process` names.
+type tsCase struct {
+	// status is the delivery row's status before the engine runs.
+	// The retry path refuses a delivery that is not retrying.
+	status database.DeliveryStatus
+
+	// inlineBody mirrors a Task built for a body under
+	// MaxInlineBodySize. When false the engine reads the body back
+	// from the stored row.
+	inlineBody bool
+
+	attemptNum int
+
+	process func(
+		ctx context.Context, e *delivery.Engine, task *delivery.Task,
+	)
+}
+
+// run delivers one event through the named path and returns the
+// Slack message text the sink received.
+func (c tsCase) run(t *testing.T) (iSetup, database.Delivery, string) {
+	t.Helper()
+
+	s := newISetup(t)
+	sink := newTSSink(t)
+
+	cfg := tsSlackConfig(t, sink.URL)
+	target := tsSeedTarget(t, s.MainDB, s.WebhookID, cfg)
+	event := tsSeedEvent(t, s.WebhookDB, s.WebhookID)
+
+	d := iSeedDelivery(
+		t, s.WebhookDB, event.ID, target.ID, c.status,
+	)
+
+	var body *string
+
+	if c.inlineBody {
+		bodyStr := event.Body
+		body = &bodyStr
+	}
+
+	task := tsTask(
+		d, event, s.WebhookID, target, c.attemptNum, body,
+	)
+
+	c.process(context.TODO(), s.Engine, &task)
+
+	return s, d, sink.text(t)
+}
+
+// TestSlackFirstAttemptCarriesEventTimestamp covers the path an
+// event takes on its first delivery: the task comes from the
+// receiver and the engine reconstructs the event from it.
+func TestSlackFirstAttemptCarriesEventTimestamp(t *testing.T) {
+	t.Parallel()
+
+	s, d, text := tsCase{
+		status:     database.DeliveryStatusPending,
+		inlineBody: true,
+		attemptNum: 1,
+		process: func(
+			ctx context.Context,
+			e *delivery.Engine,
+			task *delivery.Task,
+		) {
+			e.ExportProcessNewTask(ctx, task)
+		},
+	}.run(t)
+
+	tsAssertRealTimestamp(t, text)
+
+	iAssertStatus(t, s.WebhookDB, d.ID,
+		database.DeliveryStatusDelivered,
+	)
+}
+
+// TestSlackFirstAttemptLargeBodyCarriesEventTimestamp covers the
+// first-attempt path for an event whose body exceeded
+// MaxInlineBodySize, so the task carries no body and the engine
+// reads it back from the stored row.
+func TestSlackFirstAttemptLargeBodyCarriesEventTimestamp(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	_, _, text := tsCase{
+		status:     database.DeliveryStatusPending,
+		inlineBody: false,
+		attemptNum: 1,
+		process: func(
+			ctx context.Context,
+			e *delivery.Engine,
+			task *delivery.Task,
+		) {
+			e.ExportProcessNewTask(ctx, task)
+		},
+	}.run(t)
+
+	tsAssertRealTimestamp(t, text)
+}
+
+// TestSlackRetryCarriesEventTimestamp covers the retry path, which
+// reconstructs the event from the same task the first attempt used.
+func TestSlackRetryCarriesEventTimestamp(t *testing.T) {
+	t.Parallel()
+
+	s, d, text := tsCase{
+		status:     database.DeliveryStatusRetrying,
+		inlineBody: true,
+		attemptNum: 2,
+		process: func(
+			ctx context.Context,
+			e *delivery.Engine,
+			task *delivery.Task,
+		) {
+			e.ExportProcessRetryTask(ctx, task)
+		},
+	}.run(t)
+
+	tsAssertRealTimestamp(t, text)
+
+	iAssertStatus(t, s.WebhookDB, d.ID,
+		database.DeliveryStatusDelivered,
+	)
+}
+
+// TestFormatSlackMessageOverTaskReconstructedEvent asserts on the
+// formatted message directly, over the event the delivery paths
+// reconstruct from a Task. It is the unit-level guard under the
+// end-to-end tests: revert the CreatedAt population in hydrateEvent
+// and this fails on the zero timestamp.
+func TestFormatSlackMessageOverTaskReconstructedEvent(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	s := newISetup(t)
+
+	cfg := tsSlackConfig(t, tsUndeliverableHook)
+	target := tsSeedTarget(t, s.MainDB, s.WebhookID, cfg)
+	event := tsSeedEvent(t, s.WebhookDB, s.WebhookID)
+
+	d := iSeedDelivery(
+		t, s.WebhookDB, event.ID, target.ID,
+		database.DeliveryStatusPending,
+	)
+
+	bodyStr := event.Body
+	task := tsTask(d, event, s.WebhookID, target, 1, &bodyStr)
+
+	rebuilt, err := s.Engine.ExportEventForTask(
+		s.WebhookDB, &task,
+	)
+	require.NoError(t, err)
+	assert.False(t, rebuilt.CreatedAt.IsZero(),
+		"reconstructed event carries the zero time",
+	)
+	assert.Equal(t,
+		tsEventCreatedAt().UTC(), rebuilt.CreatedAt.UTC(),
+	)
+
+	tsAssertRealTimestamp(
+		t, delivery.FormatSlackMessage(&rebuilt),
+	)
+}
+
+// TestFormatSlackMessageZeroTimestamp asserts the rendering choice
+// directly, without going through the engine: a zero CreatedAt (the
+// shape a reaped-row fallback produces) renders as "unknown" rather
+// than the year-1 zero time, while a real CreatedAt still renders as
+// RFC3339.
+func TestFormatSlackMessageZeroTimestamp(t *testing.T) {
+	t.Parallel()
+
+	zeroEvent := database.Event{
+		Method:      http.MethodPost,
+		ContentType: testContentType,
+		Body:        tsEventBody,
+	}
+
+	zeroText := delivery.FormatSlackMessage(&zeroEvent)
+
+	assert.NotContains(t, zeroText, "0001-01-01",
+		"slack message carries the zero-time year",
+	)
+	assert.Contains(t, zeroText, "*Timestamp:* `unknown`",
+		"slack message does not mark an unset receipt time as unknown",
+	)
+
+	nonZeroEvent := zeroEvent
+	nonZeroEvent.CreatedAt = tsEventCreatedAt()
+
+	nonZeroText := delivery.FormatSlackMessage(&nonZeroEvent)
+
+	assert.Contains(t, nonZeroText,
+		"*Timestamp:* `"+
+			tsEventCreatedAt().UTC().Format(time.RFC3339)+"`",
+		"slack message does not render a real receipt time as RFC3339",
+	)
+}
+
+// TestEventReconstructionSurvivesAReapedRow pins the fallback: an
+// event row reaped by retention while its delivery still holds the
+// body inline is still delivered, with the receipt time unset,
+// rather than dropped.
+func TestEventReconstructionSurvivesAReapedRow(t *testing.T) {
+	t.Parallel()
+
+	s := newISetup(t)
+
+	cfg := tsSlackConfig(t, tsUndeliverableHook)
+	target := tsSeedTarget(t, s.MainDB, s.WebhookID, cfg)
+	event := tsSeedEvent(t, s.WebhookDB, s.WebhookID)
+
+	d := iSeedDelivery(
+		t, s.WebhookDB, event.ID, target.ID,
+		database.DeliveryStatusPending,
+	)
+
+	bodyStr := event.Body
+	task := tsTask(d, event, s.WebhookID, target, 1, &bodyStr)
+
+	require.NoError(t, s.WebhookDB.Unscoped().Delete(
+		&database.Event{}, "id = ?", event.ID,
+	).Error)
+
+	rebuilt, err := s.Engine.ExportEventForTask(
+		s.WebhookDB, &task,
+	)
+	require.NoError(t, err)
+	assert.Equal(t, bodyStr, rebuilt.Body)
+	assert.True(t, rebuilt.CreatedAt.IsZero())
+
+	// A task with no inlined body has nothing left to deliver, so
+	// the same reaped row is an error there.
+	noBody := task
+	noBody.Body = nil
+
+	_, err = s.Engine.ExportEventForTask(s.WebhookDB, &noBody)
+	require.Error(t, err)
+}

internal/delivery/export_test.go

@@ -101,6 +101,19 @@ 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,
@@ -151,6 +164,16 @@ func (e *Engine) ExportProcessRetryTask(
 	e.processRetryTask(ctx, task)
 }
 
+// ExportEventForTask exposes the event reconstruction the delivery
+// paths run: buildEventFromTask followed by hydrateEvent.
+func (e *Engine) ExportEventForTask(
+	webhookDB *gorm.DB, task *Task,
+) (database.Event, error) {
+	return e.hydrateEvent(
+		webhookDB, buildEventFromTask(task), task,
+	)
+}
+
 // ExportProcessDelivery exposes processDelivery.
 func (e *Engine) ExportProcessDelivery(
 	ctx context.Context,
@@ -169,6 +192,14 @@ 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,
@@ -311,6 +342,31 @@ 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,6 +273,73 @@ 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

@@ -229,6 +229,13 @@ func mExhaustRetries(t *testing.T, s iSetup) {
 	body := event.Body
 	cfg := iHTTPConfig(ts.URL)
 
+	// The retry below is only run if its target still exists; see
+	// https://git.eeqj.de/sneak/webhooker/issues/107.
+	iCreateTarget(
+		t, s.MainDB, targetID, s.WebhookID, "metrics-fail",
+		database.TargetTypeHTTP, cfg, 2,
+	)
+
 	first := iTask(
 		d, event, s.WebhookID, targetID,
 		"metrics-fail", cfg, 2, 1, &body,
@@ -289,6 +296,13 @@ func TestDeliveryMetrics_CircuitBreakerGauge(t *testing.T) {
 	// rather than the budget is what stops the delivery.
 	maxRetries := delivery.ExportDefaultFailureThreshold + 5
 
+	// The retries below are only run if their target still exists;
+	// see https://git.eeqj.de/sneak/webhooker/issues/107.
+	iCreateTarget(
+		t, s.MainDB, targetID, s.WebhookID, "metrics-trip",
+		database.TargetTypeHTTP, cfg, maxRetries,
+	)
+
 	first := iTask(
 		d, event, s.WebhookID, targetID,
 		"metrics-trip", cfg, maxRetries, 1, &body,
@@ -353,6 +367,11 @@ func TestDeliveryMetrics_BreakerBlockedIsNotAnAttempt(
 	cfg := iHTTPConfig(ts.URL)
 	maxRetries := delivery.ExportDefaultFailureThreshold + 5
 
+	iCreateTarget(
+		t, s.MainDB, targetID, s.WebhookID, "metrics-blocked",
+		database.TargetTypeHTTP, cfg, maxRetries,
+	)
+
 	first := iTask(
 		d, event, s.WebhookID, targetID,
 		"metrics-blocked", cfg, maxRetries, 1, &body,
@@ -393,9 +412,10 @@ func TestDeliveryMetrics_BreakerBlockedIsNotAnAttempt(
 
 	s.Engine.ExportProcessRetryTask(context.TODO(), &blocked)
 
-	// The breaker refused it: rescheduled, so the retry counter
+	// The breaker refused it: rescheduled without rewriting the
-	// moved, but nothing was attempted or timed.
+	// retrying status it already had, so the retry counter did not
-	assert.InDelta(t, retriesBefore+1,
+	// move, and nothing was attempted or timed.
+	assert.InDelta(t, retriesBefore,
 		mCounter(t, reg, mRetries, mTypeHTTP), 0)
 	assert.InDelta(t, threshold,
 		mCounter(t, reg, mAttempts, mTypeHTTP), 0)

internal/delivery/redirect_test.go

@@ -170,7 +170,8 @@ func TestDelivery_CrossOriginRedirectDropsOriginScopedHeaders(
 // Stripping must not fire within the configured origin, or every
 // destination that redirects its own path would lose its
 // credential and start answering 401 — and would lose the inbound
-// signature the receiver verifies.
+// signature header the target endpoint verifies. webhooker's own
+// receiver verifies no signature; it only forwards the header.
 func TestDelivery_SameOriginRedirectKeepsOriginScopedHeaders(
 	t *testing.T,
 ) {
@@ -338,10 +339,11 @@ 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 the delivery path's own two are deliberately
+// is not in the set, and neither is the inbound Content-Type, because
-// excluded: Content-Type describes the body, which a 307 carries
+// it is not forwarded. Two more are deliberately excluded: a
-// across hosts, and the inbound User-Agent every real sender supplies
+// Content-Type configured on the target describes the body, which a
-// is overwritten before the request goes out.
+// 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()
 
@@ -370,6 +372,7 @@ func TestApplyRequestHeaders_ReportsOriginScopedNames(t *testing.T) {
 		&delivery.HTTPTargetConfig{
 			Headers: map[string]string{
 				probeHeaderName: probeHeaderValue,
+				"Content-Type":  testContentType,
 			},
 		},
 	)
@@ -377,7 +380,11 @@ func TestApplyRequestHeaders_ReportsOriginScopedNames(t *testing.T) {
 	assert.Equal(t,
 		[]string{probeHeaderName, inboundHeaderName}, names,
 		"both header classes are reported, and only those: "+
-			"Host is never forwarded, Content-Type and "+
+			"Host and the inbound Content-Type are never "+
-			"User-Agent are the delivery path's own",
+			"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",
 	)
 }

internal/delivery/ssrf.go

@@ -26,7 +26,7 @@ var (
 		"hostname resolved to no IP addresses",
 	)
 	errBlockedIP = errors.New(
-		"blocked private/reserved IP range",
+		"blocked private, reserved or cloud metadata address",
 	)
 	errBlockedMetadata = errors.New(
 		"blocked link-local or cloud instance metadata " +
@@ -37,9 +37,10 @@ var (
 	)
 )
 
-// blockedNetworks contains all private/reserved IP ranges
+// blockedNetworks is the default blocklist: the private and
-// that should be blocked to prevent SSRF attacks. An operator
+// reserved IP ranges, plus the public cloud metadata addresses,
-// can permit specific blocks out of this set with
+// that are 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
@@ -122,6 +123,8 @@ 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
@@ -216,8 +219,8 @@ func matchesAny(networks []*net.IPNet, ip net.IP) bool {
 }
 
 // isBlockedIP checks whether an IP address falls within
-// any blocked private/reserved network range, before any
+// the default blocklist, before any operator allowlist is
-// operator allowlist is considered.
+// considered.
 func isBlockedIP(ip net.IP) bool {
 	return matchesAny(blockedNetworks, ip)
 }
@@ -320,7 +323,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 instance metadata endpoint.
+//     cloud metadata endpoint at a non-public address.
 //  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,6 +390,41 @@ 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,6 +277,24 @@ 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,6 +1,7 @@
 package delivery_test
 
 import (
+	"context"
 	"errors"
 	"fmt"
 	"net/http"
@@ -361,3 +362,46 @@ 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,10 +11,11 @@ import (
 	"sneak.berlin/go/webhooker/internal/delivery"
 )
 
-// Literals these tests repeat, named so that the header name and the
+// Literals these tests repeat, named so that the header names 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,10 +197,14 @@ func (c *httpCore) circuitBreakerBlock(
 		"cooldown_remaining", remaining,
 	)
 
-	c.eng.settleStatus(
+	// A delivery already at retrying is left as it is, so a task
-		webhookDB, d, d.Target.Type,
+	// the breaker keeps turning away writes nothing each time.
-		database.DeliveryStatusRetrying,
+	if d.Status != database.DeliveryStatusRetrying {
-	)
+		c.eng.settleStatus(
+			webhookDB, d, d.Target.Type,
+			database.DeliveryStatusRetrying,
+		)
+	}
 
 	retryTask := *task
 	sched.ScheduleRetry(retryTask, remaining)
@@ -537,6 +541,11 @@ 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
 	}
@@ -549,6 +558,10 @@ 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,
@@ -569,10 +582,10 @@ func applyRequestHeaders(
 
 	req.Header.Set("User-Agent", "webhooker/1.0")
 
-	// Content-Type describes the body being sent rather than the
+	// A Content-Type configured on the target describes the body
-	// sender, and the delivery path sets it from the event itself.
+	// being sent rather than the sender. A 307/308 preserves the
-	// A 307/308 preserves the body across hosts, so stripping it
+	// body across hosts, so stripping it would send that body
-	// would send that body untyped.
+	// untyped.
 	delete(originScoped, "Content-Type")
 
 	// User-Agent is overwritten just above, so an inbound one never

internal/delivery/target_slack.go

@@ -231,10 +231,15 @@ func FormatSlackMessage(
 		event.ContentType,
 	)
 
+	timestamp := "unknown"
+	if !event.CreatedAt.IsZero() {
+		timestamp = event.CreatedAt.UTC().Format(time.RFC3339)
+	}
+
 	fmt.Fprintf(
 		&b,
 		"*Timestamp:* `%s`\n",
-		event.CreatedAt.UTC().Format(time.RFC3339),
+		timestamp,
 	)
 
 	fmt.Fprintf(

internal/delivery/terminal_state_test.go

@@ -0,0 +1,792 @@
+package delivery_test
+
+import (
+	"context"
+	"net/http"
+	"net/http/httptest"
+	"sync/atomic"
+	"testing"
+
+	"github.com/google/uuid"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/webhooker/internal/database"
+	"sneak.berlin/go/webhooker/internal/delivery"
+)
+
+// The two terminal-state gaps of
+// 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.
+
+// 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.
+//
+// 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
+// named a row: the surviving row is invisible to a scoped read.
+func tSeedDeletedTarget(
+	t *testing.T,
+	s iSetup,
+	name, url string,
+	status database.DeliveryStatus,
+) string {
+	t.Helper()
+
+	targetID := uuid.New().String()
+
+	iCreateTarget(
+		t, s.MainDB, targetID, s.WebhookID, name,
+		database.TargetTypeHTTP, iHTTPConfig(url), 5,
+	)
+
+	event := iSeedEvent(
+		t, s.WebhookDB, s.WebhookID, `{"target":"deleted"}`,
+	)
+
+	d := iSeedDelivery(
+		t, s.WebhookDB, event.ID, targetID, status,
+	)
+
+	iSeedFailedResult(t, s.WebhookDB, d.ID)
+
+	require.NoError(t, s.MainDB.Delete(
+		&database.Target{}, "id = ?", targetID,
+	).Error)
+
+	var scoped, unscoped int64
+
+	require.NoError(t, s.MainDB.
+		Model(&database.Target{}).
+		Where("id = ?", targetID).
+		Count(&scoped).Error)
+
+	require.NoError(t, s.MainDB.Unscoped().
+		Model(&database.Target{}).
+		Where("id = ?", targetID).
+		Count(&unscoped).Error)
+
+	require.Zero(t, scoped,
+		"the deleted target is still visible to a scoped read",
+	)
+	require.Equal(t, int64(1), unscoped,
+		"the delete was hard, so this test proves nothing about "+
+			"the soft-delete case it exists for",
+	)
+
+	return d.ID
+}
+
+// tLastResult returns a delivery's final recorded attempt, asserting
+// the expected number of them.
+func tLastResult(
+	t *testing.T,
+	s iSetup,
+	deliveryID string,
+	want int,
+) database.DeliveryResult {
+	t.Helper()
+
+	results := iResults(t, s.WebhookDB, deliveryID)
+	require.Len(t, results, want)
+
+	return results[want-1]
+}
+
+// --- 1. A failure with nothing recorded ---
+
+func TestProcessDelivery_UnknownTargetType_RecordsWhy(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	s := newISetup(t)
+
+	targetID := uuid.New().String()
+
+	event := iSeedEvent(
+		t, s.WebhookDB, s.WebhookID, `{"unknown":"type"}`,
+	)
+
+	seeded := iSeedDelivery(
+		t, s.WebhookDB, event.ID, targetID,
+		database.DeliveryStatusPending,
+	)
+
+	target := database.Target{
+		Name:   "mystery",
+		Type:   tUnknownType,
+		Config: iHTTPConfig("http://example.com/hook"),
+	}
+	target.ID = targetID
+
+	d := database.Delivery{
+		EventID:  event.ID,
+		TargetID: targetID,
+		Status:   database.DeliveryStatusPending,
+		Event:    event,
+		Target:   target,
+	}
+	d.ID = seeded.ID
+
+	body := event.Body
+	task := iTask(
+		seeded, event, s.WebhookID, targetID, "mystery",
+		target.Config, 0, 1, &body,
+	)
+	task.TargetType = tUnknownType
+
+	s.Engine.ExportProcessDelivery(
+		context.Background(), s.WebhookDB, &d, &task,
+	)
+
+	iAssertStatus(
+		t, s.WebhookDB, d.ID, database.DeliveryStatusFailed,
+	)
+
+	last := tLastResult(t, s, d.ID, 1)
+
+	assert.False(t, last.Success)
+	assert.Equal(t, 1, last.AttemptNum)
+	assert.Contains(t, last.Error, string(tUnknownType),
+		"the recorded reason does not name the offending type",
+	)
+}
+
+// --- 2. A retrying delivery whose target is gone ---
+
+func TestRecoverSingleRetry_TargetDeleted(t *testing.T) {
+	t.Parallel()
+
+	s := newISetup(t)
+
+	iCreateWebhook(
+		t, s.MainDB, s.WebhookID, "deleted-target-recovery",
+	)
+
+	deliveryID := tSeedDeletedTarget(
+		t, s, "gone-on-recovery", "http://example.com/hook",
+		database.DeliveryStatusRetrying,
+	)
+
+	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-on-recovery")
+	assert.Contains(t, last.Error, "was deleted")
+
+	assert.Empty(t, s.Engine.ExportRetryCh(),
+		"a delivery whose target is gone was rescheduled",
+	)
+	assert.Zero(t, s.Engine.ExportInflightHeld(),
+		"the terminal path leaked its ownership reference",
+	)
+}
+
+func TestSweepSingleRetry_TargetDeleted(t *testing.T) {
+	t.Parallel()
+
+	s := newISetup(t)
+
+	iCreateWebhook(
+		t, s.MainDB, s.WebhookID, "deleted-target-sweep",
+	)
+
+	deliveryID := tSeedDeletedTarget(
+		t, s, "gone-on-sweep", "http://example.com/hook",
+		database.DeliveryStatusRetrying,
+	)
+
+	// Twice, because the bug was an error the sweep repeated every
+	// minute for the life of the database: the second sweep must
+	// find nothing left to do.
+	s.Engine.ExportSweepWebhookRetries(
+		context.Background(), s.WebhookID,
+	)
+	s.Engine.ExportSweepWebhookRetries(
+		context.Background(), s.WebhookID,
+	)
+
+	iAssertStatus(
+		t, s.WebhookDB, deliveryID,
+		database.DeliveryStatusFailed,
+	)
+
+	last := tLastResult(t, s, deliveryID, 2)
+
+	assert.Contains(t, last.Error, "gone-on-sweep")
+	assert.Contains(t, last.Error, "was deleted")
+
+	assert.Empty(t, s.Engine.ExportRetryCh())
+	assert.Zero(t, s.Engine.ExportInflightHeld())
+}
+
+// TestSweepSingleRetry_TargetNeverExisted covers the other half of the
+// soft-delete distinction: an id with no row at all, deleted or
+// otherwise, must not be reported as something the operator deleted.
+func TestSweepSingleRetry_TargetNeverExisted(t *testing.T) {
+	t.Parallel()
+
+	s := newISetup(t)
+
+	iCreateWebhook(
+		t, s.MainDB, s.WebhookID, "target-never-existed",
+	)
+
+	targetID := uuid.New().String()
+
+	event := iSeedEvent(
+		t, s.WebhookDB, s.WebhookID, `{"target":"absent"}`,
+	)
+
+	d := iSeedDelivery(
+		t, s.WebhookDB, event.ID, targetID,
+		database.DeliveryStatusRetrying,
+	)
+
+	iSeedFailedResult(t, s.WebhookDB, d.ID)
+
+	s.Engine.ExportSweepWebhookRetries(
+		context.Background(), s.WebhookID,
+	)
+
+	iAssertStatus(
+		t, s.WebhookDB, d.ID, database.DeliveryStatusFailed,
+	)
+
+	last := tLastResult(t, s, d.ID, 2)
+
+	assert.Contains(t, last.Error, targetID)
+	assert.Contains(t, last.Error, "no longer exists")
+	assert.NotContains(t, last.Error, "was deleted",
+		"an id that never named a row was reported as a deletion",
+	)
+}
+
+// TestFailMissingTarget_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(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	s := newISetup(t)
+
+	iCreateWebhook(
+		t, s.MainDB, s.WebhookID, "no-target-row-deleted",
+	)
+
+	hookURL := "https://hooks.slack.com/services/T00/B00/x"
+
+	deliveryID := tSeedDeletedTarget(
+		t, s, "credential-bearing", hookURL,
+		database.DeliveryStatusRetrying,
+	)
+
+	s.Engine.ExportSweepWebhookRetries(
+		context.Background(), s.WebhookID,
+	)
+
+	iAssertStatus(
+		t, s.WebhookDB, deliveryID,
+		database.DeliveryStatusFailed,
+	)
+
+	var configs []string
+
+	require.NoError(t, s.WebhookDB.
+		Table("targets").
+		Pluck("config", &configs).Error)
+
+	assert.Empty(t, configs,
+		"the deleted-target terminal path wrote a target row "+
+			"into the per-webhook event database",
+	)
+}
+
+// --- 3. The scheduled retry chain ---
+
+// tRetryChainSetup wires a counting sink and a retrying delivery
+// against a live target pointing at it, and returns the task a
+// scheduled retry would carry — config and all, snapshotted as
+// ScheduleRetry snapshots it.
+func tRetryChainSetup(
+	t *testing.T,
+	s iSetup,
+	name string,
+	hits *atomic.Int64,
+) (delivery.Task, string) {
+	t.Helper()
+
+	ts := httptest.NewServer(http.HandlerFunc(
+		func(w http.ResponseWriter, _ *http.Request) {
+			hits.Add(1)
+			w.WriteHeader(http.StatusOK)
+		},
+	))
+	t.Cleanup(ts.Close)
+
+	iCreateWebhook(t, s.MainDB, s.WebhookID, name)
+
+	targetID := uuid.New().String()
+	cfg := iHTTPConfig(ts.URL)
+
+	iCreateTarget(
+		t, s.MainDB, targetID, s.WebhookID, name,
+		database.TargetTypeHTTP, cfg, 5,
+	)
+
+	event := iSeedEvent(
+		t, s.WebhookDB, s.WebhookID, `{"chain":"retry"}`,
+	)
+
+	d := iSeedDelivery(
+		t, s.WebhookDB, event.ID, targetID,
+		database.DeliveryStatusRetrying,
+	)
+
+	iSeedFailedResult(t, s.WebhookDB, d.ID)
+
+	body := event.Body
+
+	return iTask(
+		d, event, s.WebhookID, targetID, name, cfg, 5, 2, &body,
+	), targetID
+}
+
+// TestProcessRetryTask_TargetDeleted_MakesNoAttempt is the half the
+// deployability audit found worse than filed: terminalising on
+// recovery and sweep alone leaves the already-scheduled timer chain
+// running, and it holds the target's configuration from before the
+// deletion, so it goes on sending to a destination that was removed.
+func TestProcessRetryTask_TargetDeleted_MakesNoAttempt(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	s := newISetup(t)
+
+	var hits atomic.Int64
+
+	task, targetID := tRetryChainSetup(
+		t, s, "gone-mid-chain", &hits,
+	)
+
+	require.NoError(t, s.MainDB.Delete(
+		&database.Target{}, "id = ?", targetID,
+	).Error)
+
+	s.Engine.ExportProcessRetryTask(
+		context.Background(), &task,
+	)
+
+	assert.Zero(t, hits.Load(),
+		"a scheduled retry fired at a target the operator "+
+			"had already deleted",
+	)
+
+	iAssertStatus(
+		t, s.WebhookDB, task.DeliveryID,
+		database.DeliveryStatusFailed,
+	)
+
+	last := tLastResult(t, s, task.DeliveryID, 2)
+
+	assert.False(t, last.Success)
+	assert.Contains(t, last.Error, "was deleted")
+
+	assert.Zero(t, s.Engine.ExportInflightHeld())
+}
+
+// TestProcessRetryTask_TargetPresent_StillDelivers is the guard's
+// mutation check: a liveness check that refused every retry would pass
+// the test above and break every retry there is.
+func TestProcessRetryTask_TargetPresent_StillDelivers(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	s := newISetup(t)
+
+	var hits atomic.Int64
+
+	task, _ := tRetryChainSetup(t, s, "still-there", &hits)
+
+	s.Engine.ExportProcessRetryTask(
+		context.Background(), &task,
+	)
+
+	assert.Equal(t, int64(1), hits.Load())
+
+	iAssertStatus(
+		t, s.WebhookDB, task.DeliveryID,
+		database.DeliveryStatusDelivered,
+	)
+}
+
+// TestProcessRetryTask_TargetUnreadable_StillDelivers pins the other
+// half of the guard: only a target that is confirmed gone stops a
+// retry. A main database that cannot be read is a transient fault, and
+// a guard that abandoned deliveries on one would be a worse bug than
+// the one it fixes.
+func TestProcessRetryTask_TargetUnreadable_StillDelivers(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	s := newISetup(t)
+
+	var hits atomic.Int64
+
+	task, _ := tRetryChainSetup(t, s, "unreadable-main", &hits)
+
+	sqlDB, err := s.MainDB.DB()
+	require.NoError(t, err)
+	require.NoError(t, sqlDB.Close())
+
+	s.Engine.ExportProcessRetryTask(
+		context.Background(), &task,
+	)
+
+	assert.Equal(t, int64(1), hits.Load(),
+		"a retry was abandoned because the main database "+
+			"could not be read, not because its target was gone",
+	)
+
+	iAssertStatus(
+		t, s.WebhookDB, task.DeliveryID,
+		database.DeliveryStatusDelivered,
+	)
+}
+
+// TestRecoverSingleRetry_TargetUnreadable_LeavesDeliveryAlone is the
+// same rule on the recovery path. A read failure that is not
+// "record not found" must leave every retrying delivery of every
+// webhook exactly as it was.
+func TestRecoverSingleRetry_TargetUnreadable_LeavesDeliveryAlone(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	s := newISetup(t)
+
+	iCreateWebhook(
+		t, s.MainDB, s.WebhookID, "unreadable-on-recovery",
+	)
+
+	targetID := uuid.New().String()
+
+	iCreateTarget(
+		t, s.MainDB, targetID, s.WebhookID, "healthy",
+		database.TargetTypeHTTP,
+		iHTTPConfig("http://example.com/hook"), 5,
+	)
+
+	event := iSeedEvent(
+		t, s.WebhookDB, s.WebhookID, `{"still":"retrying"}`,
+	)
+
+	d := iSeedDelivery(
+		t, s.WebhookDB, event.ID, targetID,
+		database.DeliveryStatusRetrying,
+	)
+
+	iSeedFailedResult(t, s.WebhookDB, d.ID)
+
+	sqlDB, err := s.MainDB.DB()
+	require.NoError(t, err)
+	require.NoError(t, sqlDB.Close())
+
+	s.Engine.ExportRecoverRetryingDeliveries(
+		s.WebhookDB, s.WebhookID,
+	)
+
+	iAssertStatus(
+		t, s.WebhookDB, d.ID,
+		database.DeliveryStatusRetrying,
+	)
+
+	assert.Len(t, iResults(t, s.WebhookDB, d.ID), 1,
+		"an unreadable main database produced a terminal "+
+			"failure row",
+	)
+
+	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/handlers/ui_copy_test.go

@@ -300,3 +300,80 @@ func TestEntrypointCopyButtonIsProgressiveEnhancement(t *testing.T) {
 		"the page must render to completion, not abort partway",
 	)
 }
+
+// maxRetriesHelp is the wording both target forms must carry. The
+// delivery core makes max_retries attempts in total, not that many
+// retries on top of a first try (a fresh delivery starts at attempt 1
+// and target_http gives up once the attempt number reaches
+// max_retries), and 0 is special-cased to a single fire-and-forget
+// attempt with no circuit breaker.
+const maxRetriesHelp = "This is the total number of delivery attempts, " +
+	"not retries on top of the first: a value of 3 makes three attempts " +
+	"in all. 0 means a single attempt with no retries and no circuit " +
+	"breaker."
+
+// TestTargetFormMaxRetriesCopyMatchesBehaviour pins the max_retries
+// help text on both the create form (the add-target form on the webhook
+// detail page) and the edit form, so the copy cannot drift back to
+// calling the number a retry count.
+func TestTargetFormMaxRetriesCopyMatchesBehaviour(t *testing.T) {
+	t.Parallel()
+
+	var h *handlers.Handlers
+
+	var sess *session.Session
+
+	app := newTestApp(t, &h, &sess)
+	app.RequireStart()
+
+	t.Cleanup(app.RequireStop)
+
+	webhook := &database.Webhook{Name: "wh", RetentionDays: 14}
+	webhook.ID = testWebhookID
+
+	entrypoint := database.Entrypoint{Path: "abc123"}
+	entrypoint.ID = "ep-1"
+
+	createBody := renderPage(
+		t, h, sess, "source_detail.html", map[string]any{
+			dataKeyWebhook: webhook,
+			"Entrypoints": handlers.NewEntrypointViews(
+				[]database.Entrypoint{entrypoint},
+			),
+			"Targets": delivery.NewTargetViews(nil),
+			"Events":  []database.Event{},
+			"BaseURL": "https://hooks.example.com",
+		},
+	)
+
+	assert.Contains(
+		t, createBody, maxRetriesHelp,
+		"the add-target form must explain max_retries as total attempts",
+	)
+
+	// A slack target exercises the same max_retries field while needing
+	// only Config.URL from the edit template, so the test data stays
+	// minimal. The Target key mirrors the field names the template reads
+	// off the handler's view value.
+	editBody := renderPage(
+		t, h, sess, "target_edit.html", map[string]any{
+			dataKeyWebhook: webhook,
+			"Target": map[string]any{
+				"ID":         "tg-1",
+				"Name":       "t",
+				"Type":       "slack",
+				"Active":     true,
+				"MaxRetries": 3,
+				"Config": map[string]any{
+					"URL": "https://hooks.slack.com/services/x",
+				},
+			},
+			dataKeyError: "",
+		},
+	)
+
+	assert.Contains(
+		t, editBody, maxRetriesHelp,
+		"the target edit form must explain max_retries as total attempts",
+	)
+}

internal/middleware/csrf_test.go

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

internal/middleware/recoverer.go

@@ -133,6 +133,11 @@ 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(
@@ -164,6 +169,8 @@ 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,16 +304,44 @@ 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,
-// so a second WriteHeader would change nothing the client sees and
+// cookie included, so a second WriteHeader would change nothing the
-// would draw net/http's "superfluous response.WriteHeader" report.
+// 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"))
 
@@ -331,6 +359,7 @@ 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 default environment emitted one
+// deployment behind a TLS proxy in the dev environment emitted one
 // Secure cookie and one non-Secure cookie on the same response.
 // Everything kept working, which is exactly why nobody noticed.
 //

internal/server/http.go

@@ -70,7 +70,7 @@ func (s *Server) serveUntilShutdown() {
 	err := s.httpServer.ListenAndServe()
 	if err != nil && !errors.Is(err, http.ErrServerClosed) {
 		s.log.Error("listen error", "error", err)
-		s.shutdownOnListenFailure()
+		s.shutdownWithFailure()
 	}
 }
 

internal/server/listen_failure_test.go

@@ -93,7 +93,7 @@ func requireListenFailureExit(t *testing.T, env *testEnv) {
 	select {
 	case sig := <-app.Wait():
 		require.Equal(
-			t, server.ListenFailureExitCode, sig.ExitCode,
+			t, server.StartupFailureExitCode, sig.ExitCode,
 			"listen failure must exit non-zero",
 		)
 	case <-time.After(listenFailureDeadline):

internal/server/routes.go

@@ -92,11 +92,25 @@ func (s *Server) setupGlobalMiddleware() {
 func (s *Server) setupRoutes() {
 	s.router.Get("/", s.h.HandleIndex())
 
-	s.router.Mount(
+	// Static assets answer GET and HEAD only. chi's default 405
-		"/s",
+	// carries no Allow header, so this group supplies its own.
-		http.StripPrefix("/s", http.FileServer(http.FS(static.Static))),
+	staticFiles := 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,6 +7,7 @@ import (
 	"net/http/httptest"
 	"net/url"
 	"regexp"
+	"slices"
 	"strconv"
 	"strings"
 	"testing"
@@ -220,9 +221,21 @@ func (e *testEnv) csrfFrom(
 	// out of the markup has to be unescaped before it is submitted.
 	token := html.UnescapeString(match[1])
 
-	combined := make([]*http.Cookie, 0, len(cookies))
+	// A cookie the page sets replaces the one of the same name, as in
-	combined = append(combined, cookies...)
+	// a browser. Sent both, the server would read the first, older one.
-	combined = append(combined, w.Result().Cookies()...)
+	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...)
 
 	return token, combined
 }
@@ -396,13 +409,15 @@ func (e *testEnv) storedHash(t *testing.T, username string) string {
 
 // --- /s static group ---
 
-// TestStaticServesEveryMethod pins what the static mount actually
+// TestStaticServesOnlyGetAndHead pins the methods the static group
-// answers. chi's Mount registers the handler for all methods and
+// answers: GET and HEAD are served the asset, and the other methods
-// http.FileServer only special-cases HEAD (by suppressing the body),
+// chi routes (POST, PUT, DELETE and the rest) are refused with 405
-// so a POST or a DELETE to an asset is served the file rather than
+// and an Allow header naming those two. A method chi does not route,
-// refused. The README documents this; the test is what keeps the two
+// such as PROPFIND, is refused with 405 by the top-level router
-// from drifting.
+// before it reaches the static group, so it gets no Allow header.
-func TestStaticServesEveryMethod(t *testing.T) {
+// The README documents this; the test is what keeps the two from
+// drifting.
+func TestStaticServesOnlyGetAndHead(t *testing.T) {
 	t.Parallel()
 
 	env := newTestEnv(t)
@@ -417,6 +432,7 @@ func TestStaticServesEveryMethod(t *testing.T) {
 		http.MethodPost,
 		http.MethodPut,
 		http.MethodDelete,
+		"PROPFIND",
 	} {
 		t.Run(method, func(t *testing.T) {
 			t.Parallel()
@@ -428,18 +444,38 @@ func TestStaticServesEveryMethod(t *testing.T) {
 			w := httptest.NewRecorder()
 			env.router.ServeHTTP(w, req)
 
-			assert.Equal(t, http.StatusOK, w.Code,
+			switch method {
-				"static mount answers every method")
+			case http.MethodGet:
-
+				assert.Equal(t, http.StatusOK, w.Code)
-			if method == http.MethodHead {
+				assert.Equal(t, body, w.Body.Bytes(),
+					"the asset itself is returned")
+			case http.MethodHead:
+				assert.Equal(t, http.StatusOK, w.Code)
 				assert.Empty(t, w.Body.Bytes(),
 					"HEAD must not carry a body")
-
+			case "PROPFIND":
-				return
+				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",
+				)
 			}
-
-			assert.Equal(t, body, w.Body.Bytes(),
-				"the asset itself is returned")
 		})
 	}
 }
@@ -591,6 +627,59 @@ 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/server/sentry_failure_test.go

@@ -0,0 +1,99 @@
+package server_test
+
+import (
+	"context"
+	"net"
+	"strconv"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+	"go.uber.org/fx"
+	"sneak.berlin/go/webhooker/internal/config"
+	"sneak.berlin/go/webhooker/internal/globals"
+	"sneak.berlin/go/webhooker/internal/server"
+)
+
+// TestSentryInitFailure_ShutsDownTheApp pins that error reporting
+// which is configured and cannot be started ends the application
+// instead of serving without it.
+//
+// The measured defect logged `sentry init failure` and kept running,
+// so the deployment served traffic with reporting off while every
+// other signal — SENTRY_DSN still set, the startup summary's own
+// field — said it was on. Nothing later in the process can notice
+// that reports are going nowhere, which is why this exits rather than
+// degrades.
+//
+// The DSN is placed on a hand-built Config, which is the only way to
+// reach this branch at all: loadFromEnv now parses SENTRY_DSN with
+// sentry.NewDsn, the same call sentry.Init makes, so a DSN that
+// survives configuration cannot fail initialisation in the SDK
+// version this pins. The branch stays because that is a property of
+// the SDK's current implementation rather than of its contract.
+func TestSentryInitFailure_ShutsDownTheApp(t *testing.T) {
+	t.Parallel()
+
+	port := freePort(t)
+
+	env := newTestEnvWithConfig(t, &config.Config{
+		DataDir:     t.TempDir(),
+		Environment: config.EnvironmentDev,
+		BindAddress: loopbackV4,
+		Port:        port,
+		SentryDSN:   "not-a-dsn",
+	})
+
+	app := fx.New(
+		fx.NopLogger,
+		fx.Supply(env.log, env.cfg, env.mw, env.hnd),
+		fx.Provide(globals.New, server.New),
+		fx.Invoke(func(*server.Server) {}),
+	)
+
+	startCtx, cancelStart := context.WithTimeout(
+		context.Background(), lifecycleTimeout,
+	)
+	defer cancelStart()
+
+	require.NoError(t, app.Start(startCtx))
+
+	select {
+	case sig := <-app.Wait():
+		require.Equal(
+			t, server.StartupFailureExitCode, sig.ExitCode,
+			"a sentry failure must exit non-zero",
+		)
+	case <-time.After(listenFailureDeadline):
+		t.Fatal("a sentry failure left the app running")
+	}
+
+	// The stop sequence still has to complete: the failure must reach
+	// shutdown through fx rather than around it.
+	stopCtx, cancelStop := context.WithTimeout(
+		context.Background(), lifecycleTimeout,
+	)
+	defer cancelStop()
+
+	require.NoError(t, app.Stop(stopCtx))
+
+	// And it must give up before it listens. A process that bound the
+	// port and then exited would have accepted requests it could not
+	// report on, which is the state under test in miniature.
+	requireBindable(t, port)
+}
+
+// requireBindable asserts that the port is free, which it is only if
+// the server under test never claimed it.
+func requireBindable(t *testing.T, port int) {
+	t.Helper()
+
+	var listenCfg net.ListenConfig
+
+	listener, err := listenCfg.Listen(
+		t.Context(), "tcp",
+		net.JoinHostPort(loopbackV4, strconv.Itoa(port)),
+	)
+	require.NoError(t, err, "the server bound a port it then gave up")
+	require.NoError(t, listener.Close())
+}

internal/server/server.go

@@ -51,12 +51,13 @@ const (
 	minSentryFlush = 250 * time.Millisecond
 )
 
-// ListenFailureExitCode is the status the process exits with when the
+// StartupFailureExitCode is the status the process exits with when
-// HTTP listener cannot be established, or dies for a reason other
+// the serving goroutine gives up: the HTTP listener cannot be
-// than a requested shutdown. It must stay non-zero: systemd
+// established or dies for a reason other than a requested shutdown, or
-// `Restart=on-failure` and Docker's restart policies key off it, and a
+// error reporting is configured and cannot be started. It must stay
-// zero exit would read as a deliberate stop.
+// non-zero: systemd `Restart=on-failure` and Docker's restart policies
-const ListenFailureExitCode = 1
+// key off it, and a zero exit would read as a deliberate stop.
+const StartupFailureExitCode = 1
 
 // SentryFlushBudget reports how long the Sentry flush may run when
 // remaining is the time left on the fx stop context after the HTTP
@@ -135,11 +136,25 @@ func New(lc fx.Lifecycle, params ServerParams) (*Server, error) {
 }
 
 // Run configures Sentry and starts serving HTTP requests.
+//
+// A Sentry failure ends the application instead of listening. It runs
+// before the listener rather than after it so that the process never
+// binds a port it is about to give up.
 func (s *Server) Run() {
 	s.configure()
 
 	// logging before sentry, because sentry logs
-	s.enableSentry()
+	err := s.enableSentry()
+	if err != nil {
+		s.log.Error(
+			"SENTRY_DSN is set but error reporting could not be "+
+				"started; refusing to serve with it off",
+			"error", err,
+		)
+		s.shutdownWithFailure()
+
+		return
+	}
 
 	s.serve()
 }
@@ -150,11 +165,23 @@ func (s *Server) MaintenanceMode() bool {
 	return s.params.Config.MaintenanceMode
 }
 
-func (s *Server) enableSentry() {
+// enableSentry initialises the Sentry SDK when error reporting is
+// configured, and reports the failure when it is configured and cannot
+// be initialised. A DSN that is not set is not a failure: reporting
+// stays off and the server starts normally.
+//
+// There is no fallback to running with reporting off. An operator who
+// set SENTRY_DSN asked for failures to be visible, and serving traffic
+// with reporting quietly off is the one state nothing can ever tell
+// them about — the DSN is still set, so every later signal says it is
+// on. Config already refused a DSN the SDK cannot parse, which is what
+// a typo produces, so reaching this branch means the SDK refused
+// something that parsed: not a condition to guess at either.
+func (s *Server) enableSentry() error {
 	s.sentryEnabled.Store(false)
 
-	if s.params.Config.SentryDSN == "" {
+	if !s.params.Config.SentryEnabled() {
-		return
+		return nil
 	}
 
 	err := sentry.Init(sentryClientOptions(
@@ -166,19 +193,19 @@ func (s *Server) enableSentry() {
 		),
 	))
 	if err != nil {
-		s.log.Error("sentry init failure", "error", err)
+		return fmt.Errorf("initialising sentry: %w", err)
-		// Don't use fatal since we still want the service to run
-		return
 	}
 
 	s.log.Info("sentry error reporting activated")
 	s.sentryEnabled.Store(true)
+
+	return nil
 }
 
 // serve installs the signal watcher, starts the listener and blocks
 // until the server's context is cancelled. The process exit status is
 // fx's to decide — from a signal, or from the code
-// shutdownOnListenFailure hands the Shutdowner — so this reports
+// shutdownWithFailure hands the Shutdowner — so this reports
 // nothing back to its caller.
 func (s *Server) serve() {
 	ctx, cancelFunc := context.WithCancel(context.Background())
@@ -208,20 +235,24 @@ func (s *Server) serve() {
 	// Do not call cleanShutdown() here to avoid double invocation.
 }
 
-// shutdownOnListenFailure ends the application after the HTTP
+// shutdownWithFailure ends the application non-zero from the serving
-// listener failed. The fx OnStart hook returns as soon as the serving
+// goroutine. It is how anything on that goroutine fails fatally: the
-// goroutine is spawned, so nothing downstream of it ever learns that
+// fx OnStart hook returns as soon as the goroutine is spawned, so
-// the listen failed: fx reports RUNNING and the process sits alive
+// nothing downstream of it ever learns that the goroutine gave up. fx
-// with nothing bound, which is invisible to systemd and Docker
+// reports RUNNING and the process sits alive having done neither what
-// restart policies. Asking the Shutdowner to stop the app with a
+// it was asked nor anything visible instead, which systemd and
-// non-zero code is what turns that into a visible failure.
+// Docker restart policies cannot see. Asking the Shutdowner to stop
+// the app with a non-zero code is what turns that into a visible
+// failure, and it is the whole of "fatal" here — no panic, no
+// os.Exit, and every stop hook still runs.
 //
-// The context cancel that follows only unwinds serve()'s own wait.
+// The context cancel that follows only unwinds serve()'s own wait,
-// The shutdown itself runs through fx's normal stop sequence, so the
+// and is skipped before serve has installed one. The shutdown itself
-// clean-shutdown drain in cleanShutdown is reached unchanged.
+// runs through fx's normal stop sequence, so the clean-shutdown drain
-func (s *Server) shutdownOnListenFailure() {
+// in cleanShutdown is reached unchanged.
+func (s *Server) shutdownWithFailure() {
 	err := s.params.Shutdowner.Shutdown(
-		fx.ExitCode(ListenFailureExitCode),
+		fx.ExitCode(StartupFailureExitCode),
 	)
 	if err != nil {
 		s.log.Error("shutdown request failed", "error", err)

internal/session/codec_test.go

@@ -19,8 +19,8 @@ import (
 )
 
 // The tests below exercise the securecookie codecs underneath the
-// store and nothing else: Session.Get only decodes, so no server-side
+// store and nothing else: they decode through the store itself, so no
-// expiry check takes part in the result. They exist because
+// 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,10 +75,11 @@ func restamp(
 	return base64.URLEncoding.EncodeToString(payload)
 }
 
-// decodeCookie feeds value back through the store's decode path.
+// 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.
 func decodeCookie(
 	t *testing.T,
-	s *session.Session,
 	value string,
 ) (*sessions.Session, error) {
 	t.Helper()
@@ -94,7 +95,7 @@ func decodeCookie(
 		SameSite: http.SameSiteLaxMode,
 	})
 
-	sess, err := s.Get(req)
+	sess, err := session.NewStore(testKey()).Get(req, session.SessionName)
 	require.NotNil(t, sess)
 
 	return sess, err
@@ -105,7 +106,7 @@ func TestCodec_AcceptsCookieInsideAbsoluteCap(t *testing.T) {
 
 	s := testSession(t)
 
-	sess, err := decodeCookie(t, s, restamp(
+	sess, err := decodeCookie(t, restamp(
 		t,
 		issuedCookie(t, s),
 		time.Now().Add(-(testAbsoluteMaxAge-time.Hour)),
@@ -126,7 +127,7 @@ func TestCodec_RejectsCookiePastAbsoluteCap(t *testing.T) {
 
 	s := testSession(t)
 
-	sess, err := decodeCookie(t, s, restamp(
+	sess, err := decodeCookie(t, 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" is the environment when
+// it got the DEFAULT posture wrong: "dev" was then the environment when
-// WEBHOOKER_ENVIRONMENT is unset, so a deployment terminating TLS at a
+// WEBHOOKER_ENVIRONMENT was 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,10 +224,22 @@ 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) {
-	return s.store.Get(r, SessionName)
+	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
 }
 
 // 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" is the environment when
+// configured environment, and "dev" was then the environment when
-// WEBHOOKER_ENVIRONMENT is unset. A deployment behind a TLS proxy in
+// WEBHOOKER_ENVIRONMENT was unset. A deployment behind a TLS proxy in
 // that DEFAULT posture shipped the authentication cookie with no
 // Secure attribute and said nothing about it.
 //

templates/source_detail.html

@@ -120,9 +120,12 @@
                         <label class="text-sm text-gray-700">Timeout (seconds, blank = default):</label>
                         <input type="number" name="timeout" min="0" max="300" :disabled="targetType !== 'http'" class="input text-sm w-24">
                     </div>
-                    <div x-show="targetType === 'http'" class="flex gap-2 items-center">
+                    <div x-show="targetType === 'http'">
-                        <label class="text-sm text-gray-700">Max retries (0 = fire-and-forget):</label>
+                        <div class="flex gap-2 items-center">
-                        <input type="number" name="max_retries" value="0" min="0" max="20" class="input text-sm w-24">
+                            <label class="text-sm text-gray-700">Max retries:</label>
+                            <input type="number" name="max_retries" value="0" min="0" max="20" class="input text-sm w-24">
+                        </div>
+                        <p class="text-xs text-gray-500 mt-1">This is the total number of delivery attempts, not retries on top of the first: a value of 3 makes three attempts in all. 0 means a single attempt with no retries and no circuit breaker.</p>
                     </div>
                     <div x-show="targetType === 'slack'">
                         <input type="url" name="url" placeholder="https://hooks.slack.com/services/..." :disabled="targetType !== 'slack'" class="input text-sm">

templates/target_edit.html

@@ -69,7 +69,7 @@
             <div class="form-group">
                 <label for="max_retries" class="label">Max retries</label>
                 <input type="number" id="max_retries" name="max_retries" value="{{.Target.MaxRetries}}" min="0" max="20" class="input">
-                <p class="text-xs text-gray-500 mt-1">0 is fire-and-forget: one attempt, no circuit breaker.</p>
+                <p class="text-xs text-gray-500 mt-1">This is the total number of delivery attempts, not retries on top of the first: a value of 3 makes three attempts in all. 0 means a single attempt with no retries and no circuit breaker.</p>
             </div>
             {{end}}