Render delivery attempt detail in the event log (closes #202)

Expanding a delivery on the event log page now shows each recorded
attempt: attempt number, outcome, status code, duration, error and
response body. Previously a failure rendered as "target: failed" and
diagnosing it meant opening the per-webhook SQLite file by hand.

The response body is cut by SQLite via substr over a blob cast, the
same projection the event body uses, so an oversized stored response
never becomes a Go string. The page reports the cut with a marker.

Response bodies and errors are remote content, so both go through a
new delivery.Redactor that strips the target's own destination URL,
path, query and userinfo, plus the values of credential-shaped
request headers, before rendering. A cut body goes through RedactCut
as well, which drops any tail that is a proper prefix of a secret:
the remote chooses the padding in front of a credential it echoes,
so it chooses where the cut falls inside that credential. Target
configuration keeps reaching the template only as a TargetView.

Redactors are built from an unscoped target load. Deleting a target
only soft deletes the row while its deliveries survive, and a scoped
load would leave exactly those deliveries rendering unredacted. The
views the page lists stay scoped.

Attempt loading is chunked so the IN clause cannot exceed SQLite's
bound-parameter limit, its error is reported rather than discarded,
and the page renders at most 20 attempts per delivery, counting what
it leaves out.

static/css/tailwind.css is regenerated with the repo's pinned
tailwindcss for the utility classes the new markup uses.

README.md

@@ -127,40 +127,6 @@ empty string, and quietly withholding it would deny an endpoint that
 was asked for. The `hasMetricsAuth` field in the startup log and the
 existence of the route are the same value, so they cannot disagree.
 
-#### Single-instance lock
-
-Exactly one webhooker process may use a `DATA_DIR` at a time. Two
-processes sharing one open the same databases and each run delivery
-recovery over the same rows, so every pending delivery goes out twice —
-duplicate delivery to your endpoints, from nothing worse than an
-overlapping deploy or a double start.
-
-At startup, before anything opens a database, the process takes an
-exclusive advisory lock (`flock(2)`) on `{DATA_DIR}/webhooker.lock` and
-holds it for its lifetime. A second process pointed at the same
-directory prints a message naming it and exits non-zero:
-
-```
-webhooker: data directory is already in use by another instance: /var/lib/webhooker (/var/lib/webhooker/webhooker.lock). Only one webhooker may use a data directory: two both run delivery recovery over the same rows and both deliver
-```
-
-The lock is the kernel's, not the file's: it is released when the
-process exits, including `kill -9`, so a leftover `webhooker.lock`
-never blocks a restart and must not be deleted by hand. The file is
-also left in place on a clean shutdown, deliberately — unlinking it
-would let the next process lock a fresh inode while a third still held
-the old one.
-
-To run two webhookers on one host, give each its own `DATA_DIR`.
-
-`flock(2)` is host-local and per-inode: it arbitrates between processes
-and containers sharing a volume or bind mount on one machine, but not
-between hosts on a network filesystem, and a `DATA_DIR` inside a
-container's own writable layer is not shared with anything. On a
-filesystem that refuses `flock` outright, startup fails closed — the
-process reports the error and refuses to start rather than running
-unlocked.
-
 #### Trusted proxies
 
 `TRUSTED_PROXIES` is a comma-separated list of CIDR blocks (a bare
@@ -290,51 +256,6 @@ On first startup, webhooker creates an `admin` user
 with a randomly generated password and logs it to stdout. This password
 is only displayed once.
 
-#### What `DEBUG=true` exposes
-
-`DEBUG=true` lowers the log level to `DEBUG`, which turns on every
-statement GORM runs, the two by-design lookup misses on the
-unauthenticated routes, and the rate limiter's own rejections. It is
-meant to be safe to turn on while diagnosing a live service and safe to
-paste the output of into a bug report.
-
-What it does **not** put in the log:
-
-- **Values bound to a SQL statement.** Statements are logged with their
-  placeholders, never with the values substituted into them, at every
-  level. That is what keeps the session encryption key out of the first
-  boot's `INSERT INTO settings` and the `admin` account's Argon2id
-  password hash out of its `INSERT INTO users` — the two statements
-  that made a debug log worth stealing. It applies to every table and
-  every statement rather than to a list of tables known to hold a
-  secret, so a table added later is covered without anyone remembering
-  to add it. The cost is that a failing statement can no longer be
-  replayed from the log alone: the statement, the table, the driver
-  error and the row count are all still there, but its values have to
-  come from the database.
-  `internal/gormlog/firstboot_test.go` boots the real graph with
-  `DEBUG=true` against an empty `DATA_DIR` and asserts that neither
-  secret appears in what that boot wrote to stdout.
-  The one exception is `(*gorm.DB).Scan`, which GORM logs through its
-  own trace recorder rather than through this filter. No production
-  code path calls it, and `internal/gormlog/scan_guard_test.go` fails
-  if a non-test file adds one.
-- **Session cookies, API keys or target credentials.** None of these is
-  logged at any level.
-
-What is in the log regardless of `DEBUG`, and is not a debug-logging
-decision:
-
-- **The initial `admin` password**, in the clear, once, at `INFO`, on
-  the first boot that creates the account. That line is the only place
-  it is ever shown; the database stores the hash. A first boot's output
-  is not safe to paste anywhere until that account's password has been
-  changed.
-- **An authenticated operator's own configuration**, echoed back
-  untruncated — webhook names, target hostnames. See the logging
-  section under Security for the full list and for the per-line size
-  bound that covers unauthenticated traffic.
-
 ### Running with Docker
 
 ```bash
@@ -374,12 +295,10 @@ is both the simplest and the only complete rule:
 
 `{webhook_uuid}` is the webhook's UUID primary key in its canonical
 36-character hyphenated form, so a real filename looks like
-`events-3f2a1c9e-....db`. The only other file is `webhooker.lock`, the
+`events-3f2a1c9e-....db`. Nothing else is written to `DATA_DIR`, and no
-always-empty [single-instance lock](#single-instance-lock); it holds no
+`-wal` or `-shm` files are produced (see below); a transient
-state and is not part of the backup set — a copied one is stale and
+`{name}.db-journal` may exist beside a database while a write is in
-blocks nothing. No `-wal` or `-shm` files are produced (see below); a
+flight and is not part of the backup set.
-transient `{name}.db-journal` may exist beside a database while a write
-is in flight and is not part of the backup set either.
 
 Configuration is **not** in `DATA_DIR` — it comes from the environment
 and from a `.env` file read out of the process working directory. Back
@@ -1215,52 +1134,6 @@ 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.
 
-### Metrics
-
-`/metrics` serves one Prometheus registry behind basic auth (see
-[Infrastructure Endpoints](#infrastructure-endpoints)). Alongside the
-inbound HTTP metrics recorded by the middleware, it exposes the
-delivery pipeline — the part of the service that can be failing while
-the receive side looks perfectly healthy, because it is: events are
-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_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` |
-| `webhooker_delivery_duration_seconds` | histogram | Wall time of a single dispatched delivery attempt, the same duration the attempt's `DeliveryResult` records |
-| `webhooker_deliveries_pending` | gauge | Deliveries currently in `pending` |
-| `webhooker_deliveries_retrying` | gauge | Deliveries currently in `retrying` |
-| `webhooker_circuit_breakers_open` | gauge | Circuit breakers currently open |
-
-Every delivery metric carries exactly one label, `target_type`, and
-cardinality is the whole reason for that restriction. A target type is
-one of four compile-time constants, so the label domain is bounded by
-construction; a value outside that set collapses to `unknown` rather
-than minting a series of its own. Target ids, event ids and entrypoint
-ids are deliberately not labels: they are UUIDs minted per operator
-action or per inbound request, a series is never reclaimed once it
-exists, and labelling by any of them would make `/metrics` a memory
-leak that grows with traffic.
-
-The two queue-depth gauges are counted out of the databases by a
-sampler that runs every 30 seconds for as long as the delivery engine
-does, rather than tracked as deltas alongside the status transitions: a
-delta would have to be seeded at startup from rows a previous process
-wrote, and would drift permanently on any transition that failed to
-persist.
-
-Those two gauges also publish an `unknown` series, from startup rather
-than on first occurrence. Deliveries queued against a target that has
-since been deleted are counted there: that backlog is the one nobody is
-watching, so it is the one that must not silently vanish from the
-gauge. The outcome counters move only after the status change has been
-written, so a transition the database rejected is never reported as an
-outcome that happened.
-
 ### Rate Limiting
 
 Global blanket rate limiting middleware (e.g., a per-IP throttle shared
@@ -1545,24 +1418,6 @@ and the driver error — against a smaller fixed portion than the access
 log's, and `internal/gormlog/gormlog_test.go` asserts each line against
 `MaxAccessLogLineBytes` directly rather than leaving it as arithmetic.
 
-The adapter also logs no bound value at all: it implements
-`gorm.ParamsFilter` and discards the parameters, so GORM renders the
-statement with its placeholders intact instead of substituting the
-values into it. That is a separate property from the size bound and it
-is what a bound is no substitute for — the session encryption key is 44
-base64 characters and an Argon2id hash under 100, so both fit inside
-every budget above and a truncated secret is still a secret. It holds
-on all three arms of `Trace`, including the routine one an operator
-reaches at `DEBUG`, which is the only level at which a successful
-`INSERT` is written at all. One GORM path does not consult the filter —
-`(*gorm.DB).Scan`, which records the statement through GORM's own trace
-recorder. No production code path calls it; its one caller is
-`internal/database/database_test.go:91`, whose `SELECT 1` binds
-nothing, and `internal/gormlog/scan_guard_test.go` fails if a non-test
-file calls it. `Pluck`, `Row` and `Raw` all run through the normal
-callback processor and are filtered.
-See `#### What DEBUG=true exposes` under Configuration.
-
 What that ceiling does **not** cover, stated here so the figure is not
 read as more than it is:
 
@@ -1866,7 +1721,7 @@ imports. The entry point is `cmd/webhooker/main.go`.
 ```
 webhooker/
 ├── cmd/webhooker/
-│   └── main.go               # Entry point: sets globals, locks DATA_DIR, wires fx
+│   └── main.go               # Entry point: sets globals, wires fx
 ├── internal/
 │   ├── config/
 │   │   └── config.go          # Configuration loading from environment variables
@@ -1887,8 +1742,6 @@ webhooker/
 │   │   ├── retention.go       # Retention reaper (per-webhook event expiry)
 │   │   ├── testing.go         # NewTestDatabase: wrapper for tests, no fx lifecycle
 │   │   └── webhook_db_manager.go  # Per-webhook DB lifecycle manager
-│   ├── datadir/
-│   │   └── lock.go            # Exclusive advisory lock on DATA_DIR (one instance)
 │   ├── globals/
 │   │   └── globals.go         # Build-time variables (appname, version, arch)
 │   ├── gormlog/
@@ -1906,7 +1759,6 @@ webhooker/
 │   │   ├── target_log.go      # Log target (stdout)
 │   │   ├── target_config_view.go  # Masked target config for templates
 │   │   ├── archive_sweeper.go # Periodic pruning of idle archives
-│   │   ├── queue_depth.go     # Periodic sampler behind the queue-depth gauges
 │   │   ├── url_mask.go        # Strips credentials from *url.Error
 │   │   └── ssrf.go            # SSRF prevention (IP validation, safe HTTP transport)
 │   ├── handlers/
@@ -1924,8 +1776,6 @@ webhooker/
 │   │   └── lifecycle.go       # Shared stop-hook waiter, bounded by the stop context
 │   ├── logger/
 │   │   └── logger.go          # slog setup with TTY detection
-│   ├── metrics/
-│   │   └── metrics.go         # Delivery Prometheus collectors, labelled by target type
 │   ├── middleware/
 │   │   ├── middleware.go      # Logging, CORS, Auth, Metrics, MetricsAuth, SecurityHeaders, MaxBodySize
 │   │   ├── csrf.go            # CSRF protection middleware (gorilla/csrf)

cmd/webhooker/main.go

@@ -2,15 +2,11 @@
 package main
 
 import (
-	"fmt"
-	"io"
-	"os"
 	"time"
 
 	"go.uber.org/fx"
 	"sneak.berlin/go/webhooker/internal/config"
 	"sneak.berlin/go/webhooker/internal/database"
-	"sneak.berlin/go/webhooker/internal/datadir"
 	"sneak.berlin/go/webhooker/internal/delivery"
 	"sneak.berlin/go/webhooker/internal/globals"
 	"sneak.berlin/go/webhooker/internal/handlers"
@@ -60,32 +56,7 @@ func main() {
 	globals.Appname = appname
 	globals.Version = version
 
-	os.Exit(run(os.Stderr))
-}
-
-// run takes the exclusive DATA_DIR lock, then runs the application
-// under it, and returns the process exit status.
-//
-// The lock is taken here rather than inside the fx graph because it has
-// to be held before anything opens a database, and because a refusal
-// has to reach the operator as a plain line on standard error rather
-// than as one entry in an fx failure dump. It is released by the defer
-// on a clean shutdown, and by the kernel closing the descriptor on any
-// other exit — including the one fx performs itself when a start or
-// stop hook fails, which skips deferred calls.
-func run(stderr io.Writer) int {
-	lock, err := datadir.Acquire(config.DataDir())
-	if err != nil {
-		_, _ = fmt.Fprintf(stderr, "%s: %v\n", appname, err)
-
-		return 1
-	}
-
-	defer func() { _ = lock.Release() }()
-
 	newApp().Run()
-
-	return 0
 }
 
 // newApp builds the application graph. It is separate from main so

cmd/webhooker/main_test.go

@@ -1,13 +1,10 @@
 package main
 
 import (
-	"bytes"
 	"testing"
 	"time"
 
-	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/webhooker/internal/datadir"
 	"sneak.berlin/go/webhooker/internal/server"
 )
 
@@ -36,38 +33,6 @@ func TestNewApp_StopTimeout(t *testing.T) {
 	require.Less(t, got, dockerStopGrace)
 }
 
-// TestRunRefusesLockedDataDir pins what an operator's second start
-// does. The entry point must refuse before it builds the fx graph —
-// nothing may open a database in a DATA_DIR another process holds —
-// and must exit non-zero with a message naming the directory rather
-// than starting a second delivery engine over the same rows.
-//
-// flock(2) locks descriptors independently, so holding the lock here
-// is the same denial a separate process gets; internal/datadir pins
-// that property and covers the real two-process case.
-func TestRunRefusesLockedDataDir(t *testing.T) {
-	dir := t.TempDir()
-	t.Setenv("DATA_DIR", dir)
-
-	lock, err := datadir.Acquire(dir)
-	require.NoError(t, err)
-
-	defer func() { _ = lock.Release() }()
-
-	var stderr bytes.Buffer
-
-	code := run(&stderr)
-
-	require.Equal(
-		t, 1, code, "a second instance must exit non-zero",
-	)
-	assert.Contains(
-		t, stderr.String(), dir,
-		"the refusal must name the directory",
-	)
-	assert.Contains(t, stderr.String(), "another instance")
-}
-
 // tailHeadroom is the slack the fx stop budget must keep beyond the
 // server stop hook. The hooks that run after the server — the
 // delivery engine, the healthcheck, the webhook DB manager and the

go.mod

@@ -8,15 +8,13 @@ require (
 	github.com/go-chi/chi v1.5.5
 	github.com/go-chi/cors v1.2.1
 	github.com/go-chi/httprate v0.15.0
-	github.com/gofrs/flock v0.13.0
 	github.com/google/uuid v1.6.0
 	github.com/gorilla/csrf v1.7.3
 	github.com/gorilla/sessions v1.4.0
 	github.com/joho/godotenv v1.5.1
 	github.com/prometheus/client_golang v1.18.0
-	github.com/prometheus/client_model v0.5.0
 	github.com/slok/go-http-metrics v0.11.0
-	github.com/stretchr/testify v1.11.1
+	github.com/stretchr/testify v1.8.4
 	go.uber.org/fx v1.20.1
 	golang.org/x/crypto v0.38.0
 	gopkg.in/yaml.v3 v3.0.1
@@ -40,6 +38,7 @@ require (
 	github.com/mattn/go-sqlite3 v1.14.17 // indirect
 	github.com/matttproud/golang_protobuf_extensions/v2 v2.0.0 // indirect
 	github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect
+	github.com/prometheus/client_model v0.5.0 // indirect
 	github.com/prometheus/common v0.45.0 // indirect
 	github.com/prometheus/procfs v0.12.0 // indirect
 	github.com/remyoudompheng/bigfft v0.0.0-20230129092748-24d4a6f8daec // indirect
@@ -50,7 +49,7 @@ require (
 	go.uber.org/zap v1.23.0 // indirect
 	golang.org/x/mod v0.17.0 // indirect
 	golang.org/x/sync v0.14.0 // indirect
-	golang.org/x/sys v0.37.0 // indirect
+	golang.org/x/sys v0.33.0 // indirect
 	golang.org/x/text v0.25.0 // indirect
 	golang.org/x/tools v0.21.1-0.20240508182429-e35e4ccd0d2d // indirect
 	google.golang.org/protobuf v1.31.0 // indirect

go.sum

@@ -23,8 +23,6 @@ github.com/go-chi/httprate v0.15.0 h1:j54xcWV9KGmPf/X4H32/aTH+wBlrvxL7P+SdnRqxh5
 github.com/go-chi/httprate v0.15.0/go.mod h1:rzGHhVrsBn3IMLYDOZQsSU4fJNWcjui4fWKJcCId1R4=
 github.com/go-errors/errors v1.4.2 h1:J6MZopCL4uSllY1OfXM374weqZFFItUbrImctkmUxIA=
 github.com/go-errors/errors v1.4.2/go.mod h1:sIVyrIiJhuEF+Pj9Ebtd6P/rEYROXFi3BopGUQ5a5Og=
-github.com/gofrs/flock v0.13.0 h1:95JolYOvGMqeH31+FC7D2+uULf6mG61mEZ/A8dRYMzw=
-github.com/gofrs/flock v0.13.0/go.mod h1:jxeyy9R1auM5S6JYDBhDt+E2TCo7DkratH4Pgi8P+Z0=
 github.com/golang/protobuf v1.5.0/go.mod h1:FsONVRAS9T7sI+LIUmWTfcYkHO4aIWwzhcaSAoJOfIk=
 github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=
 github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=
@@ -83,11 +81,11 @@ github.com/rogpeppe/go-internal v1.10.0/go.mod h1:UQnix2H7Ngw/k4C5ijL5+65zddjncj
 github.com/slok/go-http-metrics v0.11.0 h1:ABJUpekCZSkQT1wQrFvS4kGbhea/w6ndFJaWJeh3zL0=
 github.com/slok/go-http-metrics v0.11.0/go.mod h1:ZGKeYG1ET6TEJpQx18BqAJAvxw9jBAZXCHU7bWQqqAc=
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
-github.com/stretchr/objx v0.5.2 h1:xuMeJ0Sdp5ZMRXx/aWO6RZxdr3beISkG5/G/aIRr3pY=
+github.com/stretchr/objx v0.5.1 h1:4VhoImhV/Bm0ToFkXFi8hXNXwpDRZ/ynw3amt82mzq0=
-github.com/stretchr/objx v0.5.2/go.mod h1:FRsXN1f5AsAjCGJKqEizvkpNtU+EGNCLh3NxZ/8L+MA=
+github.com/stretchr/objx v0.5.1/go.mod h1:/iHQpkQwBD6DLUmQ4pE+s1TXdob1mORJ4/UFdrifcy0=
 github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI=
-github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U=
+github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk=
-github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U=
+github.com/stretchr/testify v1.8.4/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo=
 github.com/zeebo/assert v1.3.0 h1:g7C04CbJuIDKNPFHmsk4hwZDO5O+kntRxzaUoNXj+IQ=
 github.com/zeebo/assert v1.3.0/go.mod h1:Pq9JiuJQpG8JLJdtkwrJESF0Foym2/D9XMU5ciN/wJ0=
 github.com/zeebo/xxh3 v1.0.2 h1:xZmwmqxHZA8AI603jOQ0tMqmBr9lPeFwGg6d+xy9DC0=
@@ -111,8 +109,8 @@ golang.org/x/mod v0.17.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c=
 golang.org/x/sync v0.14.0 h1:woo0S4Yywslg6hp4eUFjTVOyKt0RookbpAHG4c1HmhQ=
 golang.org/x/sync v0.14.0/go.mod h1:1dzgHSNfp02xaA81J2MS99Qcpr2w7fw1gpm99rleRqA=
 golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
-golang.org/x/sys v0.37.0 h1:fdNQudmxPjkdUTPnLn5mdQv7Zwvbvpaxqs831goi9kQ=
+golang.org/x/sys v0.33.0 h1:q3i8TbbEz+JRD9ywIRlyRAQbM0qF7hu24q3teo2hbuw=
-golang.org/x/sys v0.37.0/go.mod h1:OgkHotnGiDImocRcuBABYBEXf8A9a87e/uXjp9XT3ks=
+golang.org/x/sys v0.33.0/go.mod h1:BJP2sWEmIv4KK5OTEluFJCKSidICx8ciO85XgH3Ak8k=
 golang.org/x/text v0.25.0 h1:qVyWApTSYLk/drJRO5mDlNYskwQznZmkpV2c8q9zls4=
 golang.org/x/text v0.25.0/go.mod h1:WEdwpYrmk1qmdHvhkSTNPm3app7v4rsT8F2UD6+VHIA=
 golang.org/x/tools v0.21.1-0.20240508182429-e35e4ccd0d2d h1:vU5i/LfpvrRCpgM/VPfJLg5KjxD3E+hfT1SH+d9zLwg=

internal/config/config.go

@@ -26,10 +26,6 @@ const (
 	// EnvironmentProd represents production environment.
 	EnvironmentProd = "prod"
 
-	// DefaultDataDir is where all SQLite databases live when DATA_DIR
-	// is unset. The same default applies in every environment.
-	DefaultDataDir = "/var/lib/webhooker"
-
 	// defaultPort is the default HTTP listen port.
 	defaultPort = 8080
 
@@ -163,19 +159,6 @@ func envString(key string) string {
 	return os.Getenv(key)
 }
 
-// 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
-// directory lock, above all — resolve it exactly as Config does.
-func DataDir() string {
-	dir := envString("DATA_DIR")
-	if dir == "" {
-		return DefaultDataDir
-	}
-
-	return dir
-}
-
 // envBool returns the value of the named environment variable
 // parsed as a boolean. Returns defaultValue if not set. If the
 // variable is set but cannot be parsed, it returns a wrapped error
@@ -478,7 +461,7 @@ func loadFromEnv() (*Config, error) {
 	}
 
 	return &Config{
-		DataDir:                DataDir(),
+		DataDir:                envString("DATA_DIR"),
 		Debug:                  debug,
 		MaintenanceMode:        maintenanceMode,
 		Environment:            environment,
@@ -556,6 +539,14 @@ func New(lc fx.Lifecycle, params ConfigParams) (*Config, error) {
 	s.log = log
 	s.params = &params
 
+	// Set default DataDir. All SQLite databases (main application
+	// DB and per-webhook event DBs) live here. The same default is
+	// used regardless of environment; override with DATA_DIR if
+	// needed.
+	if s.DataDir == "" {
+		s.DataDir = "/var/lib/webhooker"
+	}
+
 	if s.Debug {
 		params.Logger.EnableDebugLogging()
 	}

internal/config/config_test.go

@@ -424,36 +424,6 @@ func TestDefaultDataDir(t *testing.T) {
 	}
 }
 
-// TestDataDirHelper pins the exported resolver against the value
-// Config carries. The entry point takes the DATA_DIR lock through the
-// helper before the fx graph exists, so the two disagreeing would mean
-// locking one directory and writing to another.
-func TestDataDirHelper(t *testing.T) {
-	for _, set := range []string{"", "/tmp/webhooker-datadir-helper"} {
-		name := "set"
-		if set == "" {
-			name = "unset"
-		}
-
-		t.Run(name, func(t *testing.T) {
-			// Cannot use t.Parallel() here because t.Setenv
-			// is incompatible with parallel subtests.
-			if set == "" {
-				require.NoError(t, os.Unsetenv("DATA_DIR"))
-			} else {
-				t.Setenv("DATA_DIR", set)
-			}
-
-			expected := set
-			if expected == "" {
-				expected = config.DefaultDataDir
-			}
-
-			assert.Equal(t, expected, config.DataDir())
-		})
-	}
-}
-
 func TestReceiverRateLimit(t *testing.T) {
 	tests := []struct {
 		name        string

internal/datadir/lock.go

@@ -1,126 +0,0 @@
-// Package datadir guards exclusive access to the directory holding
-// every SQLite database webhooker writes.
-//
-// Two processes sharing a DATA_DIR each open the same per-webhook
-// event databases and each run delivery recovery over the same rows,
-// so every pending delivery goes out twice. SQLite's own locking does
-// not prevent that: both writers are serialised correctly and both
-// deliver. The only thing that prevents it is refusing to be the
-// second process.
-//
-// The lock lives here rather than in the server's fx graph so that any
-// entry point which touches DATA_DIR — the server, or a CLI
-// subcommand that must not operate on a live deployment's data — takes
-// it the same way.
-package datadir
-
-import (
-	"errors"
-	"fmt"
-	"os"
-	"path/filepath"
-
-	"github.com/gofrs/flock"
-)
-
-// LockFileName is the advisory lock file created inside DATA_DIR. Its
-// contents are never read: the lock is the flock(2) held on the open
-// descriptor, not the file's existence, so a leftover file from a
-// process that was killed with SIGKILL blocks nothing.
-const LockFileName = "webhooker.lock"
-
-// dirPerm is the mode Acquire creates DATA_DIR with. It matches what
-// internal/database uses, since whichever runs first creates it.
-const dirPerm = 0o750
-
-// ErrLocked reports that another live process holds the data
-// directory. Callers that need to know whether a deployment is running
-// — rather than merely failing to start — test for this with
-// errors.Is.
-var ErrLocked = errors.New(
-	"data directory is already in use by another instance",
-)
-
-// ErrNoDir reports that Acquire was given an empty directory.
-var ErrNoDir = errors.New("no data directory given")
-
-// Lock is a held exclusive advisory lock on a data directory. It is
-// valid only while the process that took it lives: the kernel drops it
-// when the descriptor closes, whether that is Release, a normal exit,
-// or a SIGKILL.
-type Lock struct {
-	dir  string
-	file *flock.Flock
-}
-
-// Acquire takes the exclusive advisory lock on dir, creating dir if it
-// does not exist. It never waits: if another process holds the lock it
-// returns an error wrapping ErrLocked and naming dir.
-//
-// The returned Lock must be held for as long as the caller intends to
-// use dir.
-func Acquire(dir string) (*Lock, error) {
-	if dir == "" {
-		return nil, ErrNoDir
-	}
-
-	err := os.MkdirAll(dir, dirPerm)
-	if err != nil {
-		return nil, fmt.Errorf(
-			"creating data directory %s: %w", dir, err,
-		)
-	}
-
-	path := filepath.Join(dir, LockFileName)
-	fl := flock.New(path)
-
-	held, err := fl.TryLock()
-	if err != nil {
-		return nil, fmt.Errorf(
-			"locking data directory %s: %w", dir, err,
-		)
-	}
-
-	if !held {
-		// A no-op on flock v0.13.0, which closes its own descriptor on
-		// a failed TryLock; kept so no version can leak one.
-		_ = fl.Close()
-
-		return nil, fmt.Errorf(
-			"%w: %s (%s). Only one webhooker may use a data "+
-				"directory: two both run delivery recovery over the "+
-				"same rows and both deliver",
-			ErrLocked, dir, path,
-		)
-	}
-
-	return &Lock{dir: dir, file: fl}, nil
-}
-
-// Dir returns the locked directory.
-func (l *Lock) Dir() string {
-	return l.dir
-}
-
-// Path returns the lock file backing the lock.
-func (l *Lock) Path() string {
-	return l.file.Path()
-}
-
-// Release drops the lock and closes the descriptor. It is safe to call
-// more than once.
-//
-// The lock file is deliberately left on disk. Unlinking it would let
-// the next process create and lock a fresh inode while a third still
-// holds the old one, which is the one outcome this package exists to
-// prevent.
-func (l *Lock) Release() error {
-	err := l.file.Unlock()
-	if err != nil {
-		return fmt.Errorf(
-			"releasing lock on data directory %s: %w", l.dir, err,
-		)
-	}
-
-	return nil
-}

internal/datadir/lock_test.go

@@ -1,250 +0,0 @@
-package datadir_test
-
-import (
-	"bufio"
-	"fmt"
-	"io"
-	"os"
-	"os/exec"
-	"path/filepath"
-	"strings"
-	"testing"
-	"time"
-
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/webhooker/internal/datadir"
-)
-
-// holderEnv names the directory the re-executed test binary should
-// lock and hold. When it is unset the child test does nothing, so an
-// ordinary run is unaffected.
-const holderEnv = "WEBHOOKER_DATADIR_LOCK_HOLDER"
-
-// holderReadyPrefix labels the child's one-line report that it holds
-// the lock, so the parent can find it among the testing package's own
-// output on the same descriptor.
-const holderReadyPrefix = "DATADIR-LOCK-HELD "
-
-// holderReadyTimeout bounds the wait for the child to take the lock.
-// It only has to cover process start on a loaded shared host.
-const holderReadyTimeout = 60 * time.Second
-
-// holderHold is how long the child keeps the lock if nothing kills it.
-// A sleep rather than a bare block, so the runtime's deadlock detector
-// has a pending timer and the child cannot outlive a killed test run
-// by more than this.
-const holderHold = 10 * time.Minute
-
-// TestLockHolder is the child half of the two-process tests below. It
-// takes the lock on the directory named by holderEnv, reports the lock
-// file on standard output, and then holds it until it is killed.
-func TestLockHolder(t *testing.T) {
-	t.Parallel()
-
-	dir := os.Getenv(holderEnv)
-	if dir == "" {
-		return
-	}
-
-	lock, err := datadir.Acquire(dir)
-	require.NoError(t, err)
-
-	// Written to the descriptor directly: the parent reads fd 1, not
-	// the testing package's buffered report.
-	_, err = fmt.Fprintf(
-		os.Stdout, "%s%s\n", holderReadyPrefix, lock.Path(),
-	)
-	require.NoError(t, err)
-
-	time.Sleep(holderHold)
-}
-
-// startHolder re-executes this test binary as a separate process that
-// takes and holds the lock on dir, and returns once that process
-// actually holds it. The child is killed when the test ends.
-func startHolder(t *testing.T, dir string) *exec.Cmd {
-	t.Helper()
-
-	//nolint:gosec // Re-executing this test binary, with a fixed arg.
-	cmd := exec.CommandContext(
-		t.Context(), os.Args[0], "-test.run", "^TestLockHolder$",
-	)
-
-	cmd.Env = append(os.Environ(), holderEnv+"="+dir)
-	cmd.Stderr = os.Stderr
-
-	stdout, err := cmd.StdoutPipe()
-	require.NoError(t, err)
-	require.NoError(t, cmd.Start())
-
-	t.Cleanup(func() {
-		_ = cmd.Process.Kill()
-		_ = cmd.Wait()
-	})
-
-	ready := make(chan string, 1)
-
-	go func() {
-		scanner := bufio.NewScanner(stdout)
-		for scanner.Scan() {
-			after, found := strings.CutPrefix(
-				scanner.Text(), holderReadyPrefix,
-			)
-			if found {
-				ready <- after
-
-				break
-			}
-		}
-
-		close(ready)
-
-		// Keep draining so the child never blocks on a full pipe.
-		_, _ = io.Copy(io.Discard, stdout)
-	}()
-
-	select {
-	case path, ok := <-ready:
-		require.True(
-			t, ok, "holder exited without taking the lock",
-		)
-		require.Equal(t, filepath.Join(dir, datadir.LockFileName), path)
-	case <-time.After(holderReadyTimeout):
-		t.Fatal("timed out waiting for the holder to take the lock")
-	}
-
-	return cmd
-}
-
-// TestSecondInstanceRefused is the regression test for the duplicate
-// delivery this package exists to prevent: a real second process
-// pointed at a data directory a live process already holds must be
-// refused, with an error that names the directory.
-func TestSecondInstanceRefused(t *testing.T) {
-	t.Parallel()
-
-	dir := t.TempDir()
-
-	startHolder(t, dir)
-
-	lock, err := datadir.Acquire(dir)
-
-	require.Error(t, err, "the second instance took the lock too")
-	require.Nil(t, lock)
-	require.ErrorIs(
-		t, err, datadir.ErrLocked,
-		"the refusal must be distinguishable from any other failure",
-	)
-	assert.Contains(
-		t, err.Error(), dir,
-		"the refusal must name the directory it is about",
-	)
-}
-
-// TestRestartAfterHardKill is the other half of the regression: a
-// process killed with SIGKILL runs no cleanup and leaves its lock file
-// behind, and the next start must not be blocked by it. This is what a
-// pidfile would get wrong; the kernel drops a flock when the
-// descriptor closes, however the process died.
-func TestRestartAfterHardKill(t *testing.T) {
-	t.Parallel()
-
-	dir := t.TempDir()
-
-	holder := startHolder(t, dir)
-
-	require.NoError(t, holder.Process.Kill())
-
-	// Wait for the kill to have actually happened. Re-acquiring while
-	// the corpse still holds a descriptor would be a race, and would
-	// make this test pass or fail on scheduling.
-	_ = holder.Wait()
-
-	require.FileExists(
-		t, filepath.Join(dir, datadir.LockFileName),
-		"the stale lock file is what must not block the restart",
-	)
-
-	lock, err := datadir.Acquire(dir)
-	require.NoError(
-		t, err, "a hard-killed instance must not block the next start",
-	)
-
-	require.NoError(t, lock.Release())
-}
-
-// TestSecondFdInSameProcessRefused pins the flock(2) property the
-// tests in cmd/webhooker rely on: descriptors are locked
-// independently, so a second acquisition is denied even when it comes
-// from the process that already holds the lock.
-func TestSecondFdInSameProcessRefused(t *testing.T) {
-	t.Parallel()
-
-	dir := t.TempDir()
-
-	first, err := datadir.Acquire(dir)
-	require.NoError(t, err)
-
-	defer func() { _ = first.Release() }()
-
-	_, err = datadir.Acquire(dir)
-	require.ErrorIs(t, err, datadir.ErrLocked)
-}
-
-// TestReleaseAllowsReacquire covers the clean-shutdown path: the lock
-// is released on exit, so a restart is not blocked by the previous
-// run.
-func TestReleaseAllowsReacquire(t *testing.T) {
-	t.Parallel()
-
-	dir := t.TempDir()
-
-	first, err := datadir.Acquire(dir)
-	require.NoError(t, err)
-	require.NoError(t, first.Release())
-
-	second, err := datadir.Acquire(dir)
-	require.NoError(t, err)
-	require.NoError(t, second.Release())
-}
-
-// TestAcquireCreatesDataDir covers a first start against a DATA_DIR
-// that does not exist yet, which is the normal case for a fresh
-// deployment: the lock is taken before anything else creates it.
-func TestAcquireCreatesDataDir(t *testing.T) {
-	t.Parallel()
-
-	dir := filepath.Join(t.TempDir(), "nested", "data")
-
-	lock, err := datadir.Acquire(dir)
-	require.NoError(t, err)
-
-	defer func() { _ = lock.Release() }()
-
-	assert.Equal(t, dir, lock.Dir())
-	assert.FileExists(t, filepath.Join(dir, datadir.LockFileName))
-}
-
-// TestAcquireEmptyDir rejects an empty directory rather than locking
-// the process's working directory.
-func TestAcquireEmptyDir(t *testing.T) {
-	t.Parallel()
-
-	_, err := datadir.Acquire("")
-	require.ErrorIs(t, err, datadir.ErrNoDir)
-}
-
-// TestAcquireUnusableDir reports an unusable DATA_DIR clearly, naming
-// it, instead of failing later and deeper.
-func TestAcquireUnusableDir(t *testing.T) {
-	t.Parallel()
-
-	file := filepath.Join(t.TempDir(), "not-a-directory")
-	require.NoError(t, os.WriteFile(file, nil, 0o600))
-
-	_, err := datadir.Acquire(file)
-
-	require.Error(t, err)
-	assert.Contains(t, err.Error(), file)
-}

internal/delivery/engine.go

@@ -15,7 +15,6 @@ import (
 	"sneak.berlin/go/webhooker/internal/database"
 	"sneak.berlin/go/webhooker/internal/lifecycle"
 	"sneak.berlin/go/webhooker/internal/logger"
-	"sneak.berlin/go/webhooker/internal/metrics"
 )
 
 const (
@@ -140,12 +139,6 @@ type Engine struct {
 	retryCh    chan Task
 	workers    int
 
-	// mtr is the delivery metric set. Production wires the
-	// process-wide one; a test can substitute a set registered on
-	// a private registry so its assertions are not disturbed by
-	// deliveries other tests are making at the same time.
-	mtr *metrics.Set
-
 	// targets maps each target type to its implementation.
 	targets map[database.TargetType]Target
 
@@ -171,7 +164,6 @@ func New(
 		deliveryCh: make(chan Task, deliveryChannelSize),
 		retryCh:    make(chan Task, retryChannelSize),
 		workers:    defaultWorkers,
-		mtr:        metrics.Default(),
 	}
 
 	e.initTargets(&http.Client{
@@ -291,10 +283,6 @@ func (e *Engine) start() {
 
 	go e.retrySweep(ctx)
 
-	e.wg.Add(1)
-
-	go e.queueDepthSampler(ctx)
-
 	e.log.Info(
 		"delivery engine started",
 		"workers", e.workers,
@@ -849,15 +837,8 @@ func (e *Engine) failUnretryableRetry(
 		0,
 	)
 
-	// 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.updateDeliveryStatus(
-		webhookDB, d, target.Type,
+		webhookDB, d, database.DeliveryStatusFailed,
-		database.DeliveryStatusFailed,
 	)
 }
 
@@ -878,8 +859,7 @@ func (e *Engine) processDelivery(
 		)
 
 		e.updateDeliveryStatus(
-			webhookDB, d, d.Target.Type,
+			webhookDB, d, database.DeliveryStatusFailed,
-			database.DeliveryStatusFailed,
 		)
 
 		return
@@ -888,24 +868,6 @@ func (e *Engine) processDelivery(
 	target.Deliver(ctx, webhookDB, d, task, e)
 }
 
-// observeAttempt counts one delivery attempt that was actually
-// dispatched to a target, and records how long it took.
-//
-// It is called from the dispatch paths rather than from around
-// Target.Deliver, because Deliver is also entered for deliveries
-// that never reach the wire: a delivery an open circuit breaker
-// refuses sends nothing, records no DeliveryResult, and is
-// rescheduled. Counting those would climb the attempts counter with
-// no traffic behind it and fill the duration histogram with
-// microsecond samples, which would make the delivery-duration
-// quantiles improve during exactly the outage they exist to reveal.
-func (e *Engine) observeAttempt(
-	t database.TargetType, dur time.Duration,
-) {
-	e.mtr.DeliveryAttempted(t)
-	e.mtr.ObserveDeliveryDuration(t, dur)
-}
-
 // recordResult persists a DeliveryResult row describing a
 // single attempt. It is a cross-target helper the targets
 // call.
@@ -939,22 +901,10 @@ func (e *Engine) recordResult(
 }
 
 // updateDeliveryStatus persists a new status for a delivery.
-// It is a cross-target helper the targets call, and therefore the
+// It is a cross-target helper the targets call.
-// single point where a delivery's outcome — delivered, terminally
-// failed, or put back into retry — is counted.
-//
-// The target type is a parameter rather than read off d.Target
-// because one caller — failUnretryableRetry — deliberately holds a
-// delivery loaded without its target relation, and must keep it that
-// way: a populated d.Target makes GORM upsert the target row, config
-// and all, into the per-webhook database.
-//
-// The counter moves only after the row is written, so a transition
-// the database rejected is not claimed as an outcome that happened.
 func (e *Engine) updateDeliveryStatus(
 	webhookDB *gorm.DB,
 	d *database.Delivery,
-	targetType database.TargetType,
 	status database.DeliveryStatus,
 ) {
 	err := webhookDB.Model(d).
@@ -966,11 +916,7 @@ func (e *Engine) updateDeliveryStatus(
 			"status", status,
 			"error", err,
 		)
-
-		return
 	}
-
-	e.mtr.DeliveryStatusChanged(targetType, status)
 }
 
 func truncate(s string, maxLen int) string {

internal/delivery/engine_integration_test.go

@@ -886,82 +886,6 @@ func TestSweepSingleRetry_TypeNoLongerRetries(
 	)
 }
 
-// TestFailUnretryableRetry_WritesNoTargetRow proves the
-// orphaned-retry terminal path leaves no target row — and so no
-// plaintext target config — in the per-webhook event database.
-//
-// That path loads the delivery without its Target relation on
-// purpose. Populating d.Target makes GORM's SaveBeforeAssociations
-// upsert the whole target row on the status UPDATE, which for a slack
-// target writes the incoming-webhook credential into events-*.db.
-// See https://git.eeqj.de/sneak/webhooker/issues/206.
-func TestFailUnretryableRetry_WritesNoTargetRow(
-	t *testing.T,
-) {
-	t.Parallel()
-
-	s := newISetup(t)
-
-	iCreateWebhook(
-		t, s.MainDB, s.WebhookID, "no-target-row",
-	)
-
-	targetID := uuid.New().String()
-
-	// A Slack incoming-webhook URL: the target config IS the
-	// credential, which is what makes a leaked target row a
-	// disclosure rather than a curiosity.
-	hookURL := "https://hooks.slack.com/services/T00/B00/x"
-
-	iCreateTarget(t, s.MainDB, targetID,
-		s.WebhookID, "credential-bearing",
-		database.TargetTypeLog, iHTTPConfig(hookURL), 5,
-	)
-
-	event := iSeedEvent(
-		t, s.WebhookDB, s.WebhookID, `{"orphaned":"retry"}`,
-	)
-
-	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,
-	)
-
-	// The table exists in the per-webhook database because GORM
-	// migrates the Delivery relation's model alongside it. It must
-	// stay empty.
-	var targetRows int64
-
-	require.NoError(t, s.WebhookDB.
-		Table("targets").
-		Count(&targetRows).Error)
-
-	assert.Zero(t, targetRows,
-		"orphaned-retry terminal failure wrote a target row "+
-			"into the per-webhook event database",
-	)
-
-	var configs []string
-
-	require.NoError(t, s.WebhookDB.
-		Table("targets").
-		Pluck("config", &configs).Error)
-
-	assert.NotContains(
-		t, strings.Join(configs, " "), hookURL,
-	)
-}
-
 func TestRecoverSingleRetry_UnknownTargetType(
 	t *testing.T,
 ) {
@@ -1201,83 +1125,6 @@ func TestDeliverHTTP_TargetTimeout(t *testing.T) {
 	iAssertResultFailed(t, db, del.ID)
 }
 
-// TestDeliverHTTP_CutsStoredResponseAtMaxBodyLog pins the size
-// this engine stores for an oversized response, because the
-// event log's redaction is written against it: the row holds
-// exactly maxBodyLog bytes and records nothing about how much
-// more the remote sent, so a credential echoed across that
-// boundary reaches the database already severed and no reader
-// of the row can tell the cut happened.
-func TestDeliverHTTP_CutsStoredResponseAtMaxBodyLog(
-	t *testing.T,
-) {
-	t.Parallel()
-
-	// Padded so the cut falls five bytes before the end of the
-	// echoed webhook URL.
-	const (
-		severedTail = 5
-		overshoot   = 100000
-	)
-
-	sent := strings.Repeat(
-		"A",
-		delivery.ExportMaxBodyLog-len(slackWebhookURL)+
-			severedTail,
-	) + slackWebhookURL + strings.Repeat("Z", overshoot)
-
-	s := newISetup(t)
-
-	ts := httptest.NewServer(http.HandlerFunc(
-		func(w http.ResponseWriter, _ *http.Request) {
-			w.WriteHeader(http.StatusBadGateway)
-			_, _ = io.WriteString(w, sent)
-		},
-	))
-	defer ts.Close()
-
-	cfgJSON := iHTTPConfig(ts.URL)
-
-	event := iSeedEvent(
-		t, s.WebhookDB, s.WebhookID, `{"cut":"test"}`,
-	)
-	targetID := uuid.New().String()
-
-	del := iSeedDelivery(
-		t, s.WebhookDB, event.ID, targetID,
-		database.DeliveryStatusPending,
-	)
-
-	bodyStr := event.Body
-	task := iTask(
-		del, event, s.WebhookID, targetID,
-		"cut-target", cfgJSON, 0, 1, &bodyStr,
-	)
-
-	s.Engine.ExportProcessNewTask(context.TODO(), &task)
-
-	results := iResults(t, s.WebhookDB, del.ID)
-	require.Len(t, results, 1)
-
-	stored := results[0].ResponseBody
-
-	assert.Len(
-		t, stored, delivery.ExportMaxBodyLog,
-		"an oversized response is stored at exactly the cap",
-	)
-	assert.Equal(
-		t, sent[:delivery.ExportMaxBodyLog], stored,
-	)
-	assert.NotContains(
-		t, stored, slackWebhookURL,
-		"the echoed URL is severed by the cut",
-	)
-	assert.Contains(
-		t, stored, "T00000000",
-		"the severed prefix still carries the credential",
-	)
-}
-
 // iSeedEventAndDelivery creates event + delivery
 // for standalone tests.
 func iSeedEventAndDelivery(

internal/delivery/export_test.go

@@ -10,7 +10,6 @@ import (
 	"go.uber.org/fx"
 	"gorm.io/gorm"
 	"sneak.berlin/go/webhooker/internal/database"
-	"sneak.berlin/go/webhooker/internal/metrics"
 )
 
 // ErrExportArchiveWriterEvicted exposes the sentinel returned by
@@ -25,12 +24,6 @@ const (
 	ExportRetryChannelSize        = retryChannelSize
 	ExportDefaultFailureThreshold = defaultFailureThreshold
 	ExportDefaultCooldown         = defaultCooldown
-
-	// ExportMaxBodyLog is the cap the engine applies to a
-	// recorded response body. The event log's handling of a cut
-	// response is written against this number, so a test has to
-	// be able to name it.
-	ExportMaxBodyLog = maxBodyLog
 )
 
 // ExportIsBlockedIP exposes isBlockedIP for testing.
@@ -260,7 +253,6 @@ func NewTestEngine(
 		deliveryCh: make(chan Task, deliveryChannelSize),
 		retryCh:    make(chan Task, retryChannelSize),
 		workers:    workers,
-		mtr:        metrics.Default(),
 	}
 	e.initTargets(client)
 
@@ -275,7 +267,6 @@ func NewTestEngineSmallRetry(
 	e := &Engine{
 		log:     log,
 		retryCh: make(chan Task, 1),
-		mtr:     metrics.Default(),
 	}
 	e.initTargets(nil)
 
@@ -298,25 +289,12 @@ func NewTestEngineWithDB(
 		deliveryCh: make(chan Task, deliveryChannelSize),
 		retryCh:    make(chan Task, retryChannelSize),
 		workers:    workers,
-		mtr:        metrics.Default(),
 	}
 	e.initTargets(client)
 
 	return e
 }
 
-// ExportSetMetrics substitutes the engine's metric set, so a test can
-// assert on collectors registered on a private registry instead of
-// the process-wide ones every other test is also moving.
-func (e *Engine) ExportSetMetrics(mtr *metrics.Set) {
-	e.mtr = mtr
-}
-
-// ExportSampleQueueDepths runs one queue depth sample synchronously.
-func (e *Engine) ExportSampleQueueDepths(ctx context.Context) {
-	e.sampleQueueDepths(ctx)
-}
-
 // NewTestCircuitBreaker creates a CircuitBreaker with
 // custom settings for testing.
 func NewTestCircuitBreaker(

internal/delivery/metrics_test.go

@@ -1,545 +0,0 @@
-package delivery_test
-
-import (
-	"context"
-	"net/http"
-	"net/http/httptest"
-	"testing"
-
-	"github.com/google/uuid"
-	"github.com/prometheus/client_golang/prometheus"
-	dto "github.com/prometheus/client_model/go"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/webhooker/internal/database"
-	"sneak.berlin/go/webhooker/internal/delivery"
-	"sneak.berlin/go/webhooker/internal/metrics"
-)
-
-// Metric names as exposed on /metrics.
-const (
-	mAttempts  = "webhooker_delivery_attempts_total"
-	mSucceeded = "webhooker_deliveries_succeeded_total"
-	mFailed    = "webhooker_deliveries_failed_total"
-	mRetries   = "webhooker_delivery_retries_total"
-	mDuration  = "webhooker_delivery_duration_seconds"
-	mPending   = "webhooker_deliveries_pending"
-	mRetrying  = "webhooker_deliveries_retrying"
-	mBreakers  = "webhooker_circuit_breakers_open"
-)
-
-const (
-	mTypeHTTP    = "http"
-	mTypeLog     = "log"
-	mTypeUnknown = "unknown"
-)
-
-// mIsolate gives the setup's engine a metric set registered on a
-// private registry. The process-wide collectors are moved by every
-// other delivery test running in parallel, so exact assertions are
-// only possible against a registry this test owns.
-func mIsolate(
-	t *testing.T, s iSetup,
-) *prometheus.Registry {
-	t.Helper()
-
-	reg := prometheus.NewRegistry()
-	s.Engine.ExportSetMetrics(metrics.New(reg))
-
-	return reg
-}
-
-// mFind returns the series of the named metric carrying the given
-// target_type label.
-func mFind(
-	t *testing.T,
-	reg *prometheus.Registry,
-	name, targetType string,
-) *dto.Metric {
-	t.Helper()
-
-	families, err := reg.Gather()
-	require.NoError(t, err)
-
-	for _, fam := range families {
-		if fam.GetName() != name {
-			continue
-		}
-
-		for _, m := range fam.GetMetric() {
-			for _, label := range m.GetLabel() {
-				if label.GetName() == "target_type" &&
-					label.GetValue() == targetType {
-					return m
-				}
-			}
-		}
-	}
-
-	t.Fatalf(
-		"metric %s{target_type=%q} not found",
-		name, targetType,
-	)
-
-	return nil
-}
-
-func mCounter(
-	t *testing.T,
-	reg *prometheus.Registry,
-	name, targetType string,
-) float64 {
-	t.Helper()
-
-	return mFind(t, reg, name, targetType).
-		GetCounter().GetValue()
-}
-
-func mGauge(
-	t *testing.T,
-	reg *prometheus.Registry,
-	name, targetType string,
-) float64 {
-	t.Helper()
-
-	return mFind(t, reg, name, targetType).
-		GetGauge().GetValue()
-}
-
-// mHTTPDurations returns how many samples the delivery duration
-// histogram holds for the http target type, which is the type every
-// test here times.
-func mHTTPDurations(
-	t *testing.T, reg *prometheus.Registry,
-) uint64 {
-	t.Helper()
-
-	return mFind(t, reg, mDuration, mTypeHTTP).
-		GetHistogram().GetSampleCount()
-}
-
-// TestDeliveryMetrics_SuccessAndRetryExhaustion drives one delivery
-// that succeeds and one that fails every attempt until its retries
-// are exhausted, and asserts every delivery counter across both.
-func TestDeliveryMetrics_SuccessAndRetryExhaustion(
-	t *testing.T,
-) {
-	t.Parallel()
-
-	s := newISetup(t)
-	reg := mIsolate(t, s)
-
-	mDeliverOK(t, s)
-
-	assert.InDelta(t, 1.0,
-		mCounter(t, reg, mAttempts, mTypeHTTP), 0)
-	assert.InDelta(t, 1.0,
-		mCounter(t, reg, mSucceeded, mTypeHTTP), 0)
-	assert.InDelta(t, 0.0,
-		mCounter(t, reg, mFailed, mTypeHTTP), 0)
-	assert.InDelta(t, 0.0,
-		mCounter(t, reg, mRetries, mTypeHTTP), 0)
-	assert.Equal(t, uint64(1),
-		mHTTPDurations(t, reg))
-
-	mExhaustRetries(t, s)
-
-	// Two further attempts: the first is retried, the second is
-	// the last one allowed and fails the delivery terminally.
-	assert.InDelta(t, 3.0,
-		mCounter(t, reg, mAttempts, mTypeHTTP), 0)
-	assert.InDelta(t, 1.0,
-		mCounter(t, reg, mSucceeded, mTypeHTTP), 0)
-	assert.InDelta(t, 1.0,
-		mCounter(t, reg, mRetries, mTypeHTTP), 0)
-	assert.InDelta(t, 1.0,
-		mCounter(t, reg, mFailed, mTypeHTTP), 0)
-	assert.Equal(t, uint64(3),
-		mHTTPDurations(t, reg))
-
-	// Two consecutive failures are below the trip threshold.
-	assert.InDelta(t, 0.0,
-		mGauge(t, reg, mBreakers, mTypeHTTP), 0)
-
-	// The label is the target type and nothing finer: two http
-	// targets shared one series, and no other type's moved.
-	assert.InDelta(t, 0.0,
-		mCounter(t, reg, mAttempts, mTypeLog), 0)
-	assert.InDelta(t, 0.0,
-		mCounter(t, reg, mFailed, mTypeLog), 0)
-}
-
-// mDeliverOK delivers one event to a target that answers 200.
-func mDeliverOK(t *testing.T, s iSetup) {
-	t.Helper()
-
-	ts := httptest.NewServer(http.HandlerFunc(
-		func(w http.ResponseWriter, _ *http.Request) {
-			w.WriteHeader(http.StatusOK)
-		},
-	))
-	defer ts.Close()
-
-	event := iSeedEvent(
-		t, s.WebhookDB, s.WebhookID, `{"ok":true}`,
-	)
-	targetID := uuid.New().String()
-
-	d := iSeedDelivery(
-		t, s.WebhookDB, event.ID, targetID,
-		database.DeliveryStatusPending,
-	)
-
-	body := event.Body
-	task := iTask(
-		d, event, s.WebhookID, targetID,
-		"metrics-ok", iHTTPConfig(ts.URL), 3, 1, &body,
-	)
-
-	s.Engine.ExportProcessNewTask(context.TODO(), &task)
-
-	iAssertStatus(t, s.WebhookDB, d.ID,
-		database.DeliveryStatusDelivered,
-	)
-}
-
-// mExhaustRetries delivers to a target that answers 500 with a
-// two-attempt budget, driving both attempts so the delivery ends
-// terminally failed.
-func mExhaustRetries(t *testing.T, s iSetup) {
-	t.Helper()
-
-	ts := httptest.NewServer(http.HandlerFunc(
-		func(w http.ResponseWriter, _ *http.Request) {
-			w.WriteHeader(http.StatusInternalServerError)
-		},
-	))
-	defer ts.Close()
-
-	event := iSeedEvent(
-		t, s.WebhookDB, s.WebhookID, `{"ok":false}`,
-	)
-	targetID := uuid.New().String()
-
-	d := iSeedDelivery(
-		t, s.WebhookDB, event.ID, targetID,
-		database.DeliveryStatusPending,
-	)
-
-	body := event.Body
-	cfg := iHTTPConfig(ts.URL)
-
-	first := iTask(
-		d, event, s.WebhookID, targetID,
-		"metrics-fail", cfg, 2, 1, &body,
-	)
-
-	s.Engine.ExportProcessNewTask(context.TODO(), &first)
-
-	iAssertStatus(t, s.WebhookDB, d.ID,
-		database.DeliveryStatusRetrying,
-	)
-
-	// The engine's own scheduler would re-enqueue this after the
-	// backoff; driving the second attempt directly keeps the test
-	// deterministic and off the wall clock.
-	second := iTask(
-		d, event, s.WebhookID, targetID,
-		"metrics-fail", cfg, 2, 2, &body,
-	)
-
-	s.Engine.ExportProcessRetryTask(
-		context.TODO(), &second,
-	)
-
-	iAssertStatus(t, s.WebhookDB, d.ID,
-		database.DeliveryStatusFailed,
-	)
-}
-
-// TestDeliveryMetrics_CircuitBreakerGauge proves the open-breaker
-// gauge follows a breaker that trips.
-func TestDeliveryMetrics_CircuitBreakerGauge(t *testing.T) {
-	t.Parallel()
-
-	s := newISetup(t)
-	reg := mIsolate(t, s)
-
-	ts := httptest.NewServer(http.HandlerFunc(
-		func(w http.ResponseWriter, _ *http.Request) {
-			w.WriteHeader(http.StatusInternalServerError)
-		},
-	))
-	defer ts.Close()
-
-	event := iSeedEvent(
-		t, s.WebhookDB, s.WebhookID, `{"trip":true}`,
-	)
-	targetID := uuid.New().String()
-
-	d := iSeedDelivery(
-		t, s.WebhookDB, event.ID, targetID,
-		database.DeliveryStatusPending,
-	)
-
-	body := event.Body
-	cfg := iHTTPConfig(ts.URL)
-
-	// A retry budget above the failure threshold, so the breaker
-	// rather than the budget is what stops the delivery.
-	maxRetries := delivery.ExportDefaultFailureThreshold + 5
-
-	first := iTask(
-		d, event, s.WebhookID, targetID,
-		"metrics-trip", cfg, maxRetries, 1, &body,
-	)
-
-	s.Engine.ExportProcessNewTask(context.TODO(), &first)
-
-	assert.InDelta(t, 0.0,
-		mGauge(t, reg, mBreakers, mTypeHTTP), 0)
-
-	for attempt := 2; attempt <= delivery.
-		ExportDefaultFailureThreshold; attempt++ {
-		task := iTask(
-			d, event, s.WebhookID, targetID,
-			"metrics-trip", cfg, maxRetries, attempt, &body,
-		)
-
-		s.Engine.ExportProcessRetryTask(
-			context.TODO(), &task,
-		)
-	}
-
-	assert.InDelta(t, 1.0,
-		mGauge(t, reg, mBreakers, mTypeHTTP), 0)
-}
-
-// TestDeliveryMetrics_BreakerBlockedIsNotAnAttempt proves a delivery
-// an open circuit breaker refuses is neither counted as an attempt
-// nor observed in the duration histogram.
-//
-// It sends nothing and records no result row, so counting it would
-// climb the attempts counter with no traffic behind it and pull the
-// duration quantiles down with near-zero samples for as long as the
-// breaker stayed open — the metric moving the wrong way during the
-// outage it exists to reveal.
-func TestDeliveryMetrics_BreakerBlockedIsNotAnAttempt(
-	t *testing.T,
-) {
-	t.Parallel()
-
-	s := newISetup(t)
-	reg := mIsolate(t, s)
-
-	ts := httptest.NewServer(http.HandlerFunc(
-		func(w http.ResponseWriter, _ *http.Request) {
-			w.WriteHeader(http.StatusInternalServerError)
-		},
-	))
-	defer ts.Close()
-
-	event := iSeedEvent(
-		t, s.WebhookDB, s.WebhookID, `{"blocked":true}`,
-	)
-	targetID := uuid.New().String()
-
-	d := iSeedDelivery(
-		t, s.WebhookDB, event.ID, targetID,
-		database.DeliveryStatusPending,
-	)
-
-	body := event.Body
-	cfg := iHTTPConfig(ts.URL)
-	maxRetries := delivery.ExportDefaultFailureThreshold + 5
-
-	first := iTask(
-		d, event, s.WebhookID, targetID,
-		"metrics-blocked", cfg, maxRetries, 1, &body,
-	)
-
-	s.Engine.ExportProcessNewTask(context.TODO(), &first)
-
-	for attempt := 2; attempt <= delivery.
-		ExportDefaultFailureThreshold; attempt++ {
-		task := iTask(
-			d, event, s.WebhookID, targetID,
-			"metrics-blocked", cfg, maxRetries, attempt, &body,
-		)
-
-		s.Engine.ExportProcessRetryTask(context.TODO(), &task)
-	}
-
-	require.InDelta(t, 1.0,
-		mGauge(t, reg, mBreakers, mTypeHTTP), 0,
-		"breaker should be open before the blocked attempt")
-
-	threshold := float64(
-		delivery.ExportDefaultFailureThreshold,
-	)
-
-	assert.InDelta(t, threshold,
-		mCounter(t, reg, mAttempts, mTypeHTTP), 0)
-	assert.Equal(t, uint64(threshold),
-		mHTTPDurations(t, reg))
-
-	retriesBefore := mCounter(t, reg, mRetries, mTypeHTTP)
-
-	blocked := iTask(
-		d, event, s.WebhookID, targetID,
-		"metrics-blocked", cfg, maxRetries,
-		delivery.ExportDefaultFailureThreshold+1, &body,
-	)
-
-	s.Engine.ExportProcessRetryTask(context.TODO(), &blocked)
-
-	// The breaker refused it: rescheduled, so the retry counter
-	// moved, but nothing was attempted or timed.
-	assert.InDelta(t, retriesBefore+1,
-		mCounter(t, reg, mRetries, mTypeHTTP), 0)
-	assert.InDelta(t, threshold,
-		mCounter(t, reg, mAttempts, mTypeHTTP), 0)
-	assert.Equal(t, uint64(threshold),
-		mHTTPDurations(t, reg))
-}
-
-// TestDeliveryMetrics_OrphanedRetryFailureLabelled proves the
-// terminal failure of a delivery whose target no longer retries is
-// counted against the target's real type, not against unknown. The
-// type is threaded in as an argument because populating d.Target on
-// that path would write the target row into the per-webhook database
-// (https://git.eeqj.de/sneak/webhooker/issues/206).
-func TestDeliveryMetrics_OrphanedRetryFailureLabelled(
-	t *testing.T,
-) {
-	t.Parallel()
-
-	s := newISetup(t)
-	reg := mIsolate(t, s)
-
-	iCreateWebhook(
-		t, s.MainDB, s.WebhookID, "orphaned-label",
-	)
-
-	deliveryID := iSeedRetryingWithType(
-		t, s, database.TargetTypeLog,
-	)
-
-	s.Engine.ExportSweepWebhookRetries(
-		context.Background(), s.WebhookID,
-	)
-
-	iAssertStatus(t, s.WebhookDB, deliveryID,
-		database.DeliveryStatusFailed,
-	)
-
-	assert.InDelta(t, 1.0,
-		mCounter(t, reg, mFailed, mTypeLog), 0)
-}
-
-// TestDeliveryMetrics_QueueDepthGauges proves the sampler publishes
-// the queued deliveries it finds in the per-webhook databases, and
-// that a drained queue reads zero rather than keeping its last
-// value.
-func TestDeliveryMetrics_QueueDepthGauges(t *testing.T) {
-	t.Parallel()
-
-	s := newISetup(t)
-	reg := mIsolate(t, s)
-
-	iCreateWebhook(
-		t, s.MainDB, s.WebhookID, "queue-depth",
-	)
-
-	targetID := uuid.New().String()
-
-	iCreateTarget(t, s.MainDB, targetID, s.WebhookID,
-		"queue-depth-target", database.TargetTypeHTTP,
-		iHTTPConfig("https://example.com/hook"), 3,
-	)
-
-	event := iSeedEvent(
-		t, s.WebhookDB, s.WebhookID, `{"queued":true}`,
-	)
-
-	pending := iSeedDelivery(
-		t, s.WebhookDB, event.ID, targetID,
-		database.DeliveryStatusPending,
-	)
-
-	iSeedDelivery(
-		t, s.WebhookDB, event.ID, targetID,
-		database.DeliveryStatusPending,
-	)
-
-	retrying := iSeedDelivery(
-		t, s.WebhookDB, event.ID, targetID,
-		database.DeliveryStatusRetrying,
-	)
-
-	s.Engine.ExportSampleQueueDepths(context.Background())
-
-	assert.InDelta(t, 2.0,
-		mGauge(t, reg, mPending, mTypeHTTP), 0)
-	assert.InDelta(t, 1.0,
-		mGauge(t, reg, mRetrying, mTypeHTTP), 0)
-	assert.InDelta(t, 0.0,
-		mGauge(t, reg, mPending, mTypeLog), 0)
-
-	require.NoError(t, s.WebhookDB.
-		Model(&database.Delivery{}).
-		Where("id IN ?", []string{pending.ID, retrying.ID}).
-		Update(
-			"status", database.DeliveryStatusDelivered,
-		).Error)
-
-	s.Engine.ExportSampleQueueDepths(context.Background())
-
-	assert.InDelta(t, 1.0,
-		mGauge(t, reg, mPending, mTypeHTTP), 0)
-	assert.InDelta(t, 0.0,
-		mGauge(t, reg, mRetrying, mTypeHTTP), 0)
-}
-
-// TestDeliveryMetrics_QueueDepthDeletedTarget proves a backlog queued
-// against a target that has since been deleted stays visible, in the
-// unknown series, instead of being dropped. That backlog is the one
-// nobody is watching, so losing it would defeat the queue-depth
-// alerting this metric exists for.
-func TestDeliveryMetrics_QueueDepthDeletedTarget(t *testing.T) {
-	t.Parallel()
-
-	s := newISetup(t)
-	reg := mIsolate(t, s)
-
-	iCreateWebhook(
-		t, s.MainDB, s.WebhookID, "deleted-target",
-	)
-
-	// No target row is created: this is a delivery whose target was
-	// deleted out from under it.
-	targetID := uuid.New().String()
-
-	event := iSeedEvent(
-		t, s.WebhookDB, s.WebhookID, `{"orphan":true}`,
-	)
-
-	iSeedDelivery(
-		t, s.WebhookDB, event.ID, targetID,
-		database.DeliveryStatusPending,
-	)
-
-	iSeedDelivery(
-		t, s.WebhookDB, event.ID, targetID,
-		database.DeliveryStatusRetrying,
-	)
-
-	s.Engine.ExportSampleQueueDepths(context.Background())
-
-	assert.InDelta(t, 1.0,
-		mGauge(t, reg, mPending, mTypeUnknown), 0)
-	assert.InDelta(t, 1.0,
-		mGauge(t, reg, mRetrying, mTypeUnknown), 0)
-	assert.InDelta(t, 0.0,
-		mGauge(t, reg, mPending, mTypeHTTP), 0)
-}

internal/delivery/queue_depth.go

@@ -1,187 +0,0 @@
-package delivery
-
-import (
-	"context"
-	"fmt"
-	"time"
-
-	"sneak.berlin/go/webhooker/internal/database"
-)
-
-// queueDepthSampleInterval is how often the pending and retrying
-// queue depths are counted and published as gauges.
-const queueDepthSampleInterval = 30 * time.Second
-
-// queueDepthSampler publishes the pending and retrying queue depths
-// on a timer for as long as the engine runs.
-//
-// The depths are counted out of the databases rather than tracked as
-// deltas alongside the status transitions. A delta counter would have
-// to be seeded correctly at startup from rows written by a previous
-// process, and would drift permanently on any transition that failed
-// to persist. Counting is the measurement that cannot go wrong, and
-// it is the same whole-database walk the retry sweep already makes.
-func (e *Engine) queueDepthSampler(ctx context.Context) {
-	defer e.wg.Done()
-
-	ticker := time.NewTicker(queueDepthSampleInterval)
-	defer ticker.Stop()
-
-	e.sampleQueueDepths(ctx)
-
-	for {
-		select {
-		case <-ctx.Done():
-			return
-		case <-ticker.C:
-			e.sampleQueueDepths(ctx)
-		}
-	}
-}
-
-// sampleQueueDepths counts every queued delivery across all
-// per-webhook databases and publishes the result.
-func (e *Engine) sampleQueueDepths(ctx context.Context) {
-	if e.database == nil || e.dbManager == nil {
-		return
-	}
-
-	types, err := e.targetTypesByID()
-	if err != nil {
-		e.log.Error(
-			"queue depth sample: failed to load target types",
-			"error", err,
-		)
-
-		return
-	}
-
-	var webhookIDs []string
-
-	err = e.database.DB().
-		Model(&database.Webhook{}).
-		Pluck("id", &webhookIDs).Error
-	if err != nil {
-		e.log.Error(
-			"queue depth sample: failed to query webhook IDs",
-			"error", err,
-		)
-
-		return
-	}
-
-	pending := make(map[database.TargetType]int)
-	retrying := make(map[database.TargetType]int)
-
-	for _, webhookID := range webhookIDs {
-		select {
-		case <-ctx.Done():
-			return
-		default:
-		}
-
-		if !e.dbManager.DBExists(webhookID) {
-			continue
-		}
-
-		e.sampleWebhookQueueDepths(
-			webhookID, types, pending, retrying,
-		)
-	}
-
-	e.mtr.SetQueueDepths(pending, retrying)
-}
-
-// targetTypesByID maps every configured target id to its type. The
-// deliveries live in the per-webhook databases but carry only a
-// target id, so the type label has to come from the main database.
-func (e *Engine) targetTypesByID() (
-	map[string]database.TargetType, error,
-) {
-	var rows []struct {
-		ID   string
-		Type database.TargetType
-	}
-
-	err := e.database.DB().
-		Model(&database.Target{}).
-		Select("id", "type").
-		Scan(&rows).Error
-	if err != nil {
-		return nil, fmt.Errorf("loading targets: %w", err)
-	}
-
-	types := make(map[string]database.TargetType, len(rows))
-
-	for _, row := range rows {
-		types[row.ID] = row.Type
-	}
-
-	return types, nil
-}
-
-// sampleWebhookQueueDepths adds one webhook's queued deliveries into
-// the running totals.
-//
-// A delivery whose target has since been deleted is not in the type
-// map and so counts under the empty target type. Set.SetQueueDepths
-// folds that into the unknown series rather than dropping it: a
-// backlog stuck behind a deleted target is a backlog that still needs
-// to be alertable.
-func (e *Engine) sampleWebhookQueueDepths(
-	webhookID string,
-	types map[string]database.TargetType,
-	pending, retrying map[database.TargetType]int,
-) {
-	webhookDB, err := e.dbManager.GetDB(webhookID)
-	if err != nil {
-		e.log.Error(
-			"queue depth sample: failed to get webhook database",
-			"webhook_id", webhookID,
-			"error", err,
-		)
-
-		return
-	}
-
-	var rows []struct {
-		TargetID string
-		Status   database.DeliveryStatus
-		Depth    int
-	}
-
-	err = webhookDB.
-		Model(&database.Delivery{}).
-		Select("target_id", "status", "count(*) as depth").
-		Where("status IN ?", []database.DeliveryStatus{
-			database.DeliveryStatusPending,
-			database.DeliveryStatusRetrying,
-		}).
-		Group("target_id, status").
-		Scan(&rows).Error
-	if err != nil {
-		e.log.Error(
-			"queue depth sample: "+
-				"failed to count queued deliveries",
-			"webhook_id", webhookID,
-			"error", err,
-		)
-
-		return
-	}
-
-	for _, row := range rows {
-		targetType := types[row.TargetID]
-
-		switch row.Status {
-		case database.DeliveryStatusPending:
-			pending[targetType] += row.Depth
-		case database.DeliveryStatusRetrying:
-			retrying[targetType] += row.Depth
-		case database.DeliveryStatusDelivered,
-			database.DeliveryStatusFailed:
-			// Excluded by the query above: a delivery that has
-			// reached a terminal state is not queued.
-		}
-	}
-}

internal/delivery/target.go

@@ -27,12 +27,6 @@ type Scheduler interface {
 // own circuit breaker, and reschedules via the injected
 // Scheduler. Fire-and-forget targets simply record a single
 // attempt.
-//
-// An implementation reports each attempt it actually dispatches to
-// Engine.observeAttempt, alongside the DeliveryResult it records for
-// it. Deliver is also entered for attempts that never happen — an
-// open circuit breaker refuses one — so the count cannot be taken
-// from around this call.
 type Target interface {
 	Deliver(
 		ctx context.Context,
@@ -80,12 +74,6 @@ type attemptResult struct {
 	errMsg     string
 }
 
-// elapsed returns how long the attempt took. The field is stored in
-// milliseconds because that is what DeliveryResult persists.
-func (r attemptResult) elapsed() time.Duration {
-	return time.Duration(r.duration) * time.Millisecond
-}
-
 // initTargets builds the target registry, wiring each target
 // to the engine's persistence helpers and giving the HTTP and
 // Slack targets the shared SSRF-safe client. It is called by

internal/delivery/target_config_edit.go

@@ -1,119 +0,0 @@
-package delivery
-
-import (
-	"encoding/json"
-	"errors"
-	"fmt"
-
-	"sneak.berlin/go/webhooker/internal/database"
-)
-
-// errUnknownTargetTypeForEdit is returned when a stored target has a
-// type the edit form has no field set for.
-var errUnknownTargetTypeForEdit = errors.New(
-	"unknown target type",
-)
-
-// TargetConfigForm is the UNMASKED projection of a target's stored
-// configuration, for pre-filling the target edit form.
-//
-// It is the deliberate exception to the rule TargetView enforces
-// everywhere else: TargetView exists so that no template can render
-// a target's stored blob, because a destination URL's path segments
-// and a header value are both routinely the credential. An operator
-// cannot correct a value they cannot see, so the edit form — and
-// only the edit form — is shown the full value.
-//
-// Everything that keeps that exception narrow lives at the call
-// site: the route is behind RequireAuth and the webhook's ownership
-// check, and its group sets NoCache so the rendered secret is not
-// written to a shared cache. Do not reach for this type from any
-// other page.
-type TargetConfigForm struct {
-	// URL is the destination for an HTTP target and the webhook
-	// URL for a Slack target.
-	URL string
-	// Headers is the HTTP target's configured headers in the
-	// textarea representation, one "Name: value" per line.
-	Headers string
-	// Timeout is the HTTP target's per-request timeout in seconds,
-	// empty when unset.
-	Timeout string
-	// Expiry is the database (archive) target's row expiry.
-	Expiry string
-}
-
-// NewTargetConfigForm parses a target's stored configuration into
-// the edit form's fields.
-//
-// A configuration that does not parse is an error rather than a
-// zero-valued form that silently looks like a target with no
-// settings. The caller shows the operator that the stored value
-// could not be read, so that saving the form is understood as
-// replacing it rather than preserving it.
-func NewTargetConfigForm(
-	t *database.Target,
-) (TargetConfigForm, error) {
-	switch t.Type {
-	case database.TargetTypeHTTP:
-		cfg, err := parseHTTPConfig(t.Config)
-		if err != nil {
-			return TargetConfigForm{}, err
-		}
-
-		return TargetConfigForm{
-			URL:     cfg.URL,
-			Headers: FormatTargetHeaders(cfg.Headers),
-			Timeout: FormatTargetTimeout(cfg.Timeout),
-		}, nil
-	case database.TargetTypeSlack:
-		cfg, err := parseSlackConfig(t.Config)
-		if err != nil {
-			return TargetConfigForm{}, err
-		}
-
-		return TargetConfigForm{URL: cfg.WebhookURL}, nil
-	case database.TargetTypeDatabase:
-		return databaseConfigForm(t.Config)
-	case database.TargetTypeLog:
-		// The log target takes no configuration.
-		return TargetConfigForm{}, nil
-	default:
-		return TargetConfigForm{}, fmt.Errorf(
-			"%w: %q", errUnknownTargetTypeForEdit, t.Type,
-		)
-	}
-}
-
-// databaseConfigForm parses an archive target's optional expiry.
-// An absent or empty configuration is the keep-forever default and
-// yields an empty field, so re-saving the form unchanged stores the
-// same empty configuration it started with. An expiry that is set
-// but not a valid duration is an error, not a blank field.
-func databaseConfigForm(
-	configJSON string,
-) (TargetConfigForm, error) {
-	if configJSON == "" {
-		return TargetConfigForm{}, nil
-	}
-
-	var cfg databaseTargetConfig
-
-	err := json.Unmarshal([]byte(configJSON), &cfg)
-	if err != nil {
-		return TargetConfigForm{}, fmt.Errorf(
-			"parsing config JSON: %w", err,
-		)
-	}
-
-	if cfg.Expiry == "" || cfg.Expiry == archiveExpiryNever {
-		return TargetConfigForm{}, nil
-	}
-
-	err = ValidateArchiveExpiry(cfg.Expiry)
-	if err != nil {
-		return TargetConfigForm{}, err
-	}
-
-	return TargetConfigForm{Expiry: cfg.Expiry}, nil
-}

internal/delivery/target_database.go

@@ -42,14 +42,7 @@ func (t *databaseTarget) Deliver(
 	_ *Task,
 	_ Scheduler,
 ) {
-	start := time.Now()
-
 	err := t.archive(d)
-
-	elapsed := time.Since(start)
-
-	t.eng.observeAttempt(d.Target.Type, elapsed)
-
 	if err != nil {
 		t.eng.log.Error(
 			"failed to archive event to database target",
@@ -60,25 +53,22 @@ func (t *databaseTarget) Deliver(
 
 		t.eng.recordResult(
 			webhookDB, d, 1, false, 0, "",
-			err.Error(), elapsed.Milliseconds(),
+			err.Error(), 0,
 		)
 
 		t.eng.updateDeliveryStatus(
-			webhookDB, d, d.Target.Type,
+			webhookDB, d, database.DeliveryStatusFailed,
-			database.DeliveryStatusFailed,
 		)
 
 		return
 	}
 
 	t.eng.recordResult(
-		webhookDB, d, 1, true, 0, "", "",
+		webhookDB, d, 1, true, 0, "", "", 0,
-		elapsed.Milliseconds(),
 	)
 
 	t.eng.updateDeliveryStatus(
-		webhookDB, d, d.Target.Type,
+		webhookDB, d, database.DeliveryStatusDelivered,
-		database.DeliveryStatusDelivered,
 	)
 }
 

internal/delivery/target_headers.go

@@ -1,256 +0,0 @@
-package delivery
-
-import (
-	"errors"
-	"fmt"
-	"net/http"
-	"slices"
-	"strconv"
-	"strings"
-)
-
-// MaxTargetTimeoutSeconds bounds a per-target request timeout.
-// A delivery attempt holds a worker for its whole duration, so an
-// unbounded timeout lets one misconfigured target stall the queue
-// indefinitely. Five minutes is far beyond any healthy webhook
-// receiver and still finite.
-const MaxTargetTimeoutSeconds = 300
-
-// Errors returned when a target's header or timeout form input
-// cannot be turned into a configuration.
-//
-// None of these ever quotes a header VALUE. A target header value
-// is routinely an authorization token, and these messages are shown
-// to the user in an error page body.
-var (
-	errHeaderLineMalformed = errors.New(
-		`each header line must be "Name: value"`,
-	)
-	errHeaderNameInvalid = errors.New(
-		"header name must be a valid HTTP token",
-	)
-	errHeaderValueInvalid = errors.New(
-		"header value must not contain control characters",
-	)
-	errHeaderDuplicate = errors.New(
-		"header given more than once",
-	)
-	errHeaderReserved = errors.New(
-		"header is set by the delivery engine and cannot be " +
-			"overridden",
-	)
-	errTimeoutInvalid = errors.New(
-		"timeout must be a whole number of seconds",
-	)
-	errTimeoutOutOfRange = errors.New(
-		"timeout is out of range",
-	)
-)
-
-// isReservedTargetHeader reports whether name (canonicalised) is a
-// header a target configuration may not set, because the delivery
-// path or net/http itself writes it regardless.
-//
-// These are rejected rather than accepted-and-ignored. Storing a
-// header that provably never reaches the wire tells the operator
-// their configuration took effect when it did not, which is the
-// same failure mode as silently substituting a default for an
-// invalid value.
-func isReservedTargetHeader(name string) bool {
-	switch name {
-	case "Host", "Content-Length", "Transfer-Encoding", "Connection":
-		return true
-	case "User-Agent":
-		// applyRequestHeaders sets the User-Agent after it applies
-		// the configured headers, so a configured one would always
-		// be overwritten.
-		return true
-	default:
-		return false
-	}
-}
-
-// ParseTargetHeaders turns the target form's headers field — one
-// "Name: value" pair per line, blank lines ignored — into the map
-// stored in HTTPTargetConfig.Headers. Names are canonicalised, so a
-// name repeated in a different case is still a duplicate rather than
-// one pair silently overwriting the other.
-//
-// An input with no pairs yields an empty map, which omitempty drops
-// from the stored config: a target configured with no headers keeps
-// the same config JSON it had before this field existed.
-func ParseTargetHeaders(raw string) (map[string]string, error) {
-	headers := make(map[string]string)
-
-	for i, line := range strings.Split(raw, "\n") {
-		lineNum := i + 1
-
-		line = strings.TrimSpace(line)
-		if line == "" {
-			continue
-		}
-
-		name, value, err := parseHeaderLine(line)
-		if err != nil {
-			return nil, fmt.Errorf("line %d: %w", lineNum, err)
-		}
-
-		if _, dup := headers[name]; dup {
-			return nil, fmt.Errorf(
-				"line %d: %w: %q", lineNum,
-				errHeaderDuplicate, name,
-			)
-		}
-
-		headers[name] = value
-	}
-
-	return headers, nil
-}
-
-// parseHeaderLine splits and validates one "Name: value" line,
-// returning the canonicalised name and the trimmed value.
-func parseHeaderLine(line string) (string, string, error) {
-	rawName, value, found := strings.Cut(line, ":")
-	if !found {
-		return "", "", errHeaderLineMalformed
-	}
-
-	rawName = strings.TrimSpace(rawName)
-	if !validHeaderName(rawName) {
-		return "", "", fmt.Errorf(
-			"%w: %q", errHeaderNameInvalid, rawName,
-		)
-	}
-
-	name := http.CanonicalHeaderKey(rawName)
-	if isReservedTargetHeader(name) {
-		return "", "", fmt.Errorf(
-			"%w: %q", errHeaderReserved, name,
-		)
-	}
-
-	value = strings.TrimSpace(value)
-	if !validHeaderValue(value) {
-		return "", "", fmt.Errorf(
-			"%w: %q", errHeaderValueInvalid, name,
-		)
-	}
-
-	return name, value, nil
-}
-
-// validHeaderName reports whether name is a non-empty RFC 9110
-// field name. Rejecting anything else here is what keeps a value
-// containing CR or LF from being smuggled in as part of a name and
-// injecting a second header into the outbound request.
-func validHeaderName(name string) bool {
-	if name == "" {
-		return false
-	}
-
-	for i := range len(name) {
-		if !isTokenByte(name[i]) {
-			return false
-		}
-	}
-
-	return true
-}
-
-// isTokenByte reports whether c is a "tchar" per RFC 9110 5.6.2.
-func isTokenByte(c byte) bool {
-	switch {
-	case c >= 'a' && c <= 'z',
-		c >= 'A' && c <= 'Z',
-		c >= '0' && c <= '9':
-		return true
-	}
-
-	return strings.IndexByte("!#$%&'*+-.^_`|~", c) >= 0
-}
-
-// validHeaderValue reports whether value is a legal field value:
-// no control characters, which is the other half of the header
-// injection guard. An empty value is legal.
-func validHeaderValue(value string) bool {
-	for i := range len(value) {
-		c := value[i]
-		if c < 0x20 || c == 0x7f {
-			return false
-		}
-	}
-
-	return true
-}
-
-// FormatTargetHeaders renders a stored header map back into the
-// form's textarea representation, one "Name: value" per line.
-//
-// Names are sorted so that loading the edit form twice without
-// saving produces identical text; Go map iteration order would
-// otherwise reshuffle the field on every render.
-func FormatTargetHeaders(headers map[string]string) string {
-	if len(headers) == 0 {
-		return ""
-	}
-
-	names := make([]string, 0, len(headers))
-	for name := range headers {
-		names = append(names, name)
-	}
-
-	slices.Sort(names)
-
-	var b strings.Builder
-
-	for _, name := range names {
-		b.WriteString(name)
-		b.WriteString(": ")
-		b.WriteString(headers[name])
-		b.WriteString("\n")
-	}
-
-	return b.String()
-}
-
-// ParseTargetTimeout interprets the target form's timeout field as
-// a whole number of seconds. An empty field means "unset" and yields
-// 0, which omitempty drops from the stored config and which the
-// delivery path reads as "use the shared client's timeout".
-//
-// Anything else that is not a whole number in range is an error, not
-// a silently substituted default: a target whose timeout was typed
-// wrong must say so at the form rather than deliver on a timeout its
-// operator did not choose.
-func ParseTargetTimeout(raw string) (int, error) {
-	raw = strings.TrimSpace(raw)
-	if raw == "" {
-		return 0, nil
-	}
-
-	v, err := strconv.Atoi(raw)
-	if err != nil || v < 0 {
-		return 0, errTimeoutInvalid
-	}
-
-	if v > MaxTargetTimeoutSeconds {
-		return 0, fmt.Errorf(
-			"%w: at most %d seconds",
-			errTimeoutOutOfRange, MaxTargetTimeoutSeconds,
-		)
-	}
-
-	return v, nil
-}
-
-// FormatTargetTimeout renders a stored timeout for the form field.
-// An unset timeout renders as an empty field rather than "0", so the
-// placeholder can describe the default the target actually uses.
-func FormatTargetTimeout(timeout int) string {
-	if timeout <= 0 {
-		return ""
-	}
-
-	return strconv.Itoa(timeout)
-}

internal/delivery/target_headers_test.go

@@ -1,273 +0,0 @@
-package delivery_test
-
-import (
-	"encoding/json"
-	"strconv"
-	"testing"
-
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/webhooker/internal/database"
-	"sneak.berlin/go/webhooker/internal/delivery"
-)
-
-// Literals these tests repeat, named so that the header name and the
-// keep-forever archive config each have one definition.
-const (
-	headerAuthorization = "Authorization"
-	bearerValue         = "Bearer abc"
-	archiveConfigNever  = "{\"expiry\":\"never\"}"
-)
-
-func TestParseTargetHeaders_AcceptsPairs(t *testing.T) {
-	t.Parallel()
-
-	got, err := delivery.ParseTargetHeaders(
-		"  Authorization: Bearer abc \n\n" +
-			"x-tenant:acme\r\n" +
-			"X-Empty:\n",
-	)
-	require.NoError(t, err)
-
-	assert.Equal(
-		t,
-		map[string]string{
-			headerAuthorization: bearerValue,
-			"X-Tenant":          "acme",
-			"X-Empty":           "",
-		},
-		got,
-	)
-}
-
-// A configuration with no headers must stay indistinguishable from
-// one written before the field existed, so omitempty drops the key.
-func TestParseTargetHeaders_EmptyInputYieldsNoHeaders(t *testing.T) {
-	t.Parallel()
-
-	got, err := delivery.ParseTargetHeaders("\n  \n")
-	require.NoError(t, err)
-	assert.Empty(t, got)
-
-	encoded, err := json.Marshal(delivery.HTTPTargetConfig{
-		URL:     "https://example.com/h",
-		Headers: got,
-	})
-	require.NoError(t, err)
-	assert.JSONEq(
-		t, `{"url":"https://example.com/h"}`, string(encoded),
-	)
-}
-
-func TestParseTargetHeaders_Rejects(t *testing.T) {
-	t.Parallel()
-
-	cases := map[string]string{
-		"no colon":           "Authorization Bearer abc",
-		"empty name":         ": value",
-		"space in name":      "X Bad: value",
-		"reserved host":      "Host: evil.example",
-		"reserved ua":        "User-Agent: curl/8",
-		"reserved length":    "Content-Length: 0",
-		"duplicate any case": "X-A: 1\nx-a: 2",
-	}
-
-	for name, input := range cases {
-		t.Run(name, func(t *testing.T) {
-			t.Parallel()
-
-			_, err := delivery.ParseTargetHeaders(input)
-			require.Error(t, err)
-		})
-	}
-}
-
-// A header value is routinely a bearer token and these errors are
-// rendered into a 400 body, so no message may quote one.
-func TestParseTargetHeaders_ErrorsNeverQuoteAValue(t *testing.T) {
-	t.Parallel()
-
-	const secret = "QQNEVERINAMESSAGEQQ"
-
-	_, err := delivery.ParseTargetHeaders(
-		"X-A: " + secret + "\nx-a: " + secret,
-	)
-	require.Error(t, err)
-	assert.NotContains(t, err.Error(), secret)
-
-	_, err = delivery.ParseTargetHeaders(
-		"X Bad Name: " + secret,
-	)
-	require.Error(t, err)
-	assert.NotContains(t, err.Error(), secret)
-}
-
-// Loading the edit form twice without saving must not reshuffle
-// the textarea, which Go's map iteration order would otherwise do.
-func TestFormatTargetHeaders_IsSorted(t *testing.T) {
-	t.Parallel()
-
-	got := delivery.FormatTargetHeaders(map[string]string{
-		"X-Zed":             "z",
-		headerAuthorization: bearerValue,
-		"X-Alpha":           "a",
-	})
-
-	assert.Equal(
-		t,
-		"Authorization: Bearer abc\nX-Alpha: a\nX-Zed: z\n",
-		got,
-	)
-	assert.Empty(t, delivery.FormatTargetHeaders(nil))
-}
-
-func TestFormatTargetHeaders_RoundTripsThroughParse(t *testing.T) {
-	t.Parallel()
-
-	want := map[string]string{
-		headerAuthorization: bearerValue,
-		"X-Tenant":          "acme",
-	}
-
-	got, err := delivery.ParseTargetHeaders(
-		delivery.FormatTargetHeaders(want),
-	)
-	require.NoError(t, err)
-	assert.Equal(t, want, got)
-}
-
-func TestParseTargetTimeout(t *testing.T) {
-	t.Parallel()
-
-	got, err := delivery.ParseTargetTimeout("  30 ")
-	require.NoError(t, err)
-	assert.Equal(t, 30, got)
-
-	got, err = delivery.ParseTargetTimeout("")
-	require.NoError(t, err)
-	assert.Zero(t, got)
-
-	for _, bad := range []string{"soon", "-1", "1e3", "100000"} {
-		_, err = delivery.ParseTargetTimeout(bad)
-		require.Error(t, err, bad)
-	}
-}
-
-func TestFormatTargetTimeout(t *testing.T) {
-	t.Parallel()
-
-	assert.Equal(t, "30", delivery.FormatTargetTimeout(30))
-	assert.Empty(t, delivery.FormatTargetTimeout(0))
-	assert.Empty(t, delivery.FormatTargetTimeout(-1))
-}
-
-func TestNewTargetConfigForm(t *testing.T) {
-	t.Parallel()
-
-	form, err := delivery.NewTargetConfigForm(&database.Target{
-		Type: database.TargetTypeHTTP,
-		Config: `{"url":"https://example.com/h",` +
-			`"headers":{"Authorization":"Bearer abc"},` +
-			`"timeout":9}`,
-	})
-	require.NoError(t, err)
-	assert.Equal(t, "https://example.com/h", form.URL)
-	assert.Equal(t, "Authorization: Bearer abc\n", form.Headers)
-	assert.Equal(t, "9", form.Timeout)
-
-	form, err = delivery.NewTargetConfigForm(&database.Target{
-		Type:   database.TargetTypeSlack,
-		Config: `{"webhookUrl":"https://hooks.example/s"}`,
-	})
-	require.NoError(t, err)
-	assert.Equal(t, "https://hooks.example/s", form.URL)
-
-	form, err = delivery.NewTargetConfigForm(&database.Target{
-		Type:   database.TargetTypeDatabase,
-		Config: `{"expiry":"720h"}`,
-	})
-	require.NoError(t, err)
-	assert.Equal(t, "720h", form.Expiry)
-
-	form, err = delivery.NewTargetConfigForm(&database.Target{
-		Type: database.TargetTypeLog,
-	})
-	require.NoError(t, err)
-	assert.Empty(t, form.URL)
-}
-
-// A keep-forever archive target must pre-fill as an empty field, so
-// saving the form back unchanged stores the same empty config.
-func TestNewTargetConfigForm_DatabaseNeverIsBlank(t *testing.T) {
-	t.Parallel()
-
-	for _, cfg := range []string{"", `{}`, archiveConfigNever} {
-		form, err := delivery.NewTargetConfigForm(
-			&database.Target{
-				Type:   database.TargetTypeDatabase,
-				Config: cfg,
-			},
-		)
-		require.NoError(t, err, cfg)
-		assert.Empty(t, form.Expiry, cfg)
-	}
-}
-
-// An unreadable stored config is an error rather than a blank form
-// that looks like a target with no settings, so the caller can tell
-// the operator that saving replaces the stored value.
-func TestNewTargetConfigForm_UnreadableConfigErrors(t *testing.T) {
-	t.Parallel()
-
-	cases := []*database.Target{
-		{Type: database.TargetTypeHTTP, Config: "not json"},
-		{Type: database.TargetTypeHTTP, Config: `{}`},
-		{Type: database.TargetTypeSlack, Config: ""},
-		{
-			Type:   database.TargetTypeDatabase,
-			Config: `{"expiry":"soon"}`,
-		},
-		{Type: database.TargetType("nope")},
-	}
-
-	for _, target := range cases {
-		_, err := delivery.NewTargetConfigForm(target)
-		require.Error(t, err, target.Type)
-	}
-}
-
-// The ceiling exists so one misconfigured target cannot hold a
-// delivery worker indefinitely, and it is inclusive.
-func TestParseTargetTimeout_CeilingIsInclusive(t *testing.T) {
-	t.Parallel()
-
-	assert.Positive(t, delivery.MaxTargetTimeoutSeconds)
-
-	got, err := delivery.ParseTargetTimeout(
-		strconv.Itoa(delivery.MaxTargetTimeoutSeconds),
-	)
-	require.NoError(t, err)
-	assert.Equal(t, delivery.MaxTargetTimeoutSeconds, got)
-
-	_, err = delivery.ParseTargetTimeout(
-		strconv.Itoa(delivery.MaxTargetTimeoutSeconds + 1),
-	)
-	require.Error(t, err)
-}
-
-// Control characters in a value are how a second header would be
-// smuggled into the outbound request.
-func TestParseTargetHeaders_RejectsControlCharactersInValues(
-	t *testing.T,
-) {
-	t.Parallel()
-
-	for _, bad := range []string{
-		"X-A: one\x01two",
-		"X-A: one\ttwo",
-		"X-A: one\x7ftwo",
-	} {
-		_, err := delivery.ParseTargetHeaders(bad)
-		require.Error(t, err, bad)
-	}
-}

internal/delivery/target_http.go

@@ -74,8 +74,6 @@ func (c *httpCore) fireAndForget(
 	d *database.Delivery,
 	res attemptResult,
 ) {
-	c.eng.observeAttempt(d.Target.Type, res.elapsed())
-
 	c.eng.recordResult(
 		webhookDB, d, 1, res.success,
 		res.statusCode, res.respBody, res.errMsg,
@@ -84,7 +82,7 @@ func (c *httpCore) fireAndForget(
 
 	if res.success {
 		c.eng.updateDeliveryStatus(
-			webhookDB, d, d.Target.Type,
+			webhookDB, d,
 			database.DeliveryStatusDelivered,
 		)
 
@@ -92,8 +90,7 @@ func (c *httpCore) fireAndForget(
 	}
 
 	c.eng.updateDeliveryStatus(
-		webhookDB, d, d.Target.Type,
+		webhookDB, d, database.DeliveryStatusFailed,
-		database.DeliveryStatusFailed,
 	)
 }
 
@@ -110,17 +107,10 @@ func (c *httpCore) withRetry(
 		return
 	}
 
-	// Allow may have moved the breaker to half-open, and the
-	// attempt below may open or close it, so the gauge is
-	// republished on every exit from here.
-	defer c.publishCircuitState(d.Target.Type)
-
 	attemptNum := task.AttemptNum
 
 	res := attempt()
 
-	c.eng.observeAttempt(d.Target.Type, res.elapsed())
-
 	c.eng.recordResult(
 		webhookDB, d, attemptNum, res.success,
 		res.statusCode, res.respBody, res.errMsg,
@@ -131,7 +121,7 @@ func (c *httpCore) withRetry(
 		cb.RecordSuccess()
 
 		c.eng.updateDeliveryStatus(
-			webhookDB, d, d.Target.Type,
+			webhookDB, d,
 			database.DeliveryStatusDelivered,
 		)
 
@@ -156,8 +146,6 @@ func (c *httpCore) circuitBreakerBlock(
 		return false
 	}
 
-	defer c.publishCircuitState(d.Target.Type)
-
 	remaining := cb.CooldownRemaining()
 
 	c.eng.log.Info(
@@ -169,7 +157,7 @@ func (c *httpCore) circuitBreakerBlock(
 	)
 
 	c.eng.updateDeliveryStatus(
-		webhookDB, d, d.Target.Type,
+		webhookDB, d,
 		database.DeliveryStatusRetrying,
 	)
 
@@ -189,7 +177,7 @@ func (c *httpCore) handleRetry(
 ) {
 	if attemptNum >= maxRetries {
 		c.eng.updateDeliveryStatus(
-			webhookDB, d, d.Target.Type,
+			webhookDB, d,
 			database.DeliveryStatusFailed,
 		)
 
@@ -197,8 +185,7 @@ func (c *httpCore) handleRetry(
 	}
 
 	c.eng.updateDeliveryStatus(
-		webhookDB, d, d.Target.Type,
+		webhookDB, d, database.DeliveryStatusRetrying,
-		database.DeliveryStatusRetrying,
 	)
 
 	backoff := calcBackoff(attemptNum)
@@ -228,28 +215,6 @@ func (c *httpCore) getCircuitBreaker(
 	return cb
 }
 
-// publishCircuitState recounts this core's open breakers and
-// publishes the gauge. Each core holds the breakers of exactly one
-// target type, so the recount is over that type's targets alone.
-// Counting rather than adjusting a delta keeps the gauge honest
-// however a breaker changed state.
-func (c *httpCore) publishCircuitState(
-	targetType database.TargetType,
-) {
-	open := 0
-
-	c.circuitBreakers.Range(func(_, val any) bool {
-		cb, ok := val.(*CircuitBreaker)
-		if ok && cb.State() == CircuitOpen {
-			open++
-		}
-
-		return true
-	})
-
-	c.eng.mtr.SetCircuitBreakersOpen(targetType, open)
-}
-
 // remainingBackoff returns how long remains of the backoff
 // window for the last attempt of a recovered retrying
 // delivery. It implements rescheduler.
@@ -337,8 +302,7 @@ func (t *httpTarget) Deliver(
 		)
 
 		t.eng.updateDeliveryStatus(
-			webhookDB, d, d.Target.Type,
+			webhookDB, d, database.DeliveryStatusFailed,
-			database.DeliveryStatusFailed,
 		)
 
 		return

internal/delivery/target_log.go

@@ -2,7 +2,6 @@ package delivery
 
 import (
 	"context"
-	"time"
 
 	"gorm.io/gorm"
 	"sneak.berlin/go/webhooker/internal/database"
@@ -35,8 +34,6 @@ func (t *logTarget) Deliver(
 	_ *Task,
 	_ Scheduler,
 ) {
-	start := time.Now()
-
 	t.eng.log.Info(
 		"webhook event delivered to log target",
 		"delivery_id", d.ID,
@@ -51,17 +48,11 @@ func (t *logTarget) Deliver(
 		"body", d.Event.Body,
 	)
 
-	elapsed := time.Since(start)
-
-	t.eng.observeAttempt(d.Target.Type, elapsed)
-
 	t.eng.recordResult(
-		webhookDB, d, 1, true, 0, "", "",
+		webhookDB, d, 1, true, 0, "", "", 0,
-		elapsed.Milliseconds(),
 	)
 
 	t.eng.updateDeliveryStatus(
-		webhookDB, d, d.Target.Type,
+		webhookDB, d, database.DeliveryStatusDelivered,
-		database.DeliveryStatusDelivered,
 	)
 }

internal/delivery/target_redact.go

@@ -219,13 +219,6 @@ func headerSecrets(headers map[string]string) []string {
 // isCredentialHeaderName classifies a header by its name. The
 // value is never inspected, so the rule is the same
 // class-based one MaskURL applies to a destination URL.
-//
-// The fragments are short on purpose, and match anywhere in
-// the name, so abbreviations an operator might use are covered
-// too: X-Sig, X-Pass, X-HMAC. That over-matches — a header
-// named X-Design contains "sig" — and over-matching is the
-// safe direction here: the cost is a marker where an echoed
-// header value would have rendered.
 func isCredentialHeaderName(name string) bool {
 	name = strings.ToLower(strings.TrimSpace(name))
 
@@ -239,12 +232,10 @@ func isCredentialHeaderName(name string) bool {
 	// X-Api-Key, X-Hub-Signature, X-Auth-Token.
 	for _, fragment := range []string{
 		"auth",
-		"credential",
-		"hmac",
 		"key",
-		"pass",
+		"password",
 		"secret",
-		"sig",
+		"signature",
 		"token",
 	} {
 		if strings.Contains(name, fragment) {

internal/delivery/target_redact_test.go

@@ -89,10 +89,6 @@ func TestRedactor_RedactsCredentialShapedHeaderValues(
 			`"Cookie":"session=BBBBBBBBBBBB",` +
 			`"X-Api-Key":"CCCCCCCCCCCC",` +
 			`"X-Hub-Signature":"sha256=DDDDDDDDDDDD",` +
-			`"X-Sig":"EEEEEEEEEEEE",` +
-			`"X-Pass":"FFFFFFFFFFFF",` +
-			`"X-HMAC":"GGGGGGGGGGGG",` +
-			`"X-Credential":"HHHHHHHHHHHH",` +
 			`"Accept":"application/json",` +
 			`"User-Agent":"webhooker/1.0"}}`,
 	})
@@ -102,11 +98,6 @@ func TestRedactor_RedactsCredentialShapedHeaderValues(
 		"session=BBBBBBBBBBBB",
 		"CCCCCCCCCCCC",
 		"sha256=DDDDDDDDDDDD",
-		// Abbreviated names an operator might use.
-		"EEEEEEEEEEEE",
-		"FFFFFFFFFFFF",
-		"GGGGGGGGGGGG",
-		"HHHHHHHHHHHH",
 	} {
 		got := r.Redact("echo: " + secret)
 

internal/delivery/target_slack.go

@@ -101,8 +101,7 @@ func (t *slackTarget) failConfig(
 	)
 
 	t.eng.updateDeliveryStatus(
-		webhookDB, d, d.Target.Type,
+		webhookDB, d, database.DeliveryStatusFailed,
-		database.DeliveryStatusFailed,
 	)
 }
 

internal/gormlog/firstboot_test.go

@@ -1,229 +0,0 @@
-package gormlog_test
-
-import (
-	"context"
-	"database/sql"
-	"os"
-	"path/filepath"
-	"testing"
-
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"go.uber.org/fx"
-	"go.uber.org/fx/fxtest"
-	_ "modernc.org/sqlite" // Pure Go SQLite driver.
-	"sneak.berlin/go/webhooker/internal/config"
-	"sneak.berlin/go/webhooker/internal/database"
-	"sneak.berlin/go/webhooker/internal/globals"
-	"sneak.berlin/go/webhooker/internal/logger"
-	"sneak.berlin/go/webhooker/internal/session"
-)
-
-// argon2Prefix opens every encoded Argon2id hash this service
-// produces. It is asserted on separately from the hash itself, so that
-// a change to the password encoding cannot quietly turn the hash
-// assertion into a comparison against a string the log never held.
-const argon2Prefix = "$argon2id$"
-
-// settingsInsert and usersInsert are the two statements a first boot
-// runs that carry a secret. The sqlite dialector quotes identifiers
-// with backticks.
-const (
-	settingsInsert = "INSERT INTO `settings`"
-	usersInsert    = "INSERT INTO `users`"
-)
-
-// captureStdoutToFile redirects os.Stdout into a file for the rest of
-// the test and returns a function that reads back everything written
-// to it.
-//
-// A file rather than a pipe: internal/logger writes synchronously to
-// whatever os.Stdout is when it builds its handler, so once fx's start
-// returns, every byte the boot produced is already in the file and no
-// draining goroutine is needed to prove it. Redirecting the variable
-// before the application is built is what puts the service logger —
-// and therefore the GORM adapter, which writes through it — into the
-// capture.
-//
-// The redirect also decides the handler: a regular file is not a
-// character device, so internal/logger installs its JSON handler, the
-// one it installs in production under a log collector.
-func captureStdoutToFile(t *testing.T) func() string {
-	t.Helper()
-
-	path := filepath.Join(t.TempDir(), "stdout.log")
-
-	//nolint:gosec // The path is this test's own t.TempDir().
-	f, err := os.Create(path)
-	require.NoError(t, err)
-
-	orig := os.Stdout
-	os.Stdout = f
-
-	t.Cleanup(func() {
-		os.Stdout = orig
-		_ = f.Close()
-	})
-
-	return func() string {
-		require.NoError(t, f.Sync())
-
-		//nolint:gosec // As above.
-		b, readErr := os.ReadFile(path)
-		require.NoError(t, readErr)
-
-		return string(b)
-	}
-}
-
-// firstBootSecrets are the two values a first boot generates and
-// stores, read back out of the database.
-type firstBootSecrets struct {
-	sessionKey   string
-	passwordHash string
-}
-
-// readFirstBootSecrets reads those two secrets straight out of the
-// SQLite file with database/sql rather than through GORM, so that
-// reading them cannot itself add a line to the log under test.
-func readFirstBootSecrets(
-	t *testing.T, dataDir string,
-) firstBootSecrets {
-	t.Helper()
-
-	db, err := sql.Open("sqlite", filepath.Join(
-		dataDir, "webhooker.db",
-	))
-	require.NoError(t, err)
-
-	defer func() { require.NoError(t, db.Close()) }()
-
-	ctx := context.Background()
-
-	var got firstBootSecrets
-
-	require.NoError(t, db.QueryRowContext(
-		ctx, `SELECT value FROM settings WHERE key = 'session_key'`,
-	).Scan(&got.sessionKey))
-
-	require.NoError(t, db.QueryRowContext(
-		ctx, `SELECT password FROM users WHERE username = 'admin'`,
-	).Scan(&got.passwordHash))
-
-	require.NotEmpty(t, got.sessionKey)
-	require.Contains(t, got.passwordHash, argon2Prefix)
-
-	return got
-}
-
-// bootAtDebug starts and stops the real application graph against
-// dataDir with DEBUG=true, and returns everything it wrote to standard
-// output.
-//
-// config.New reads DEBUG from the environment exactly as the binary
-// does, internal/logger builds the handler it builds in production,
-// database.New runs the migrations and creates the admin user, and
-// session.New takes the session key. Those four are the whole of the
-// path that writes either secret.
-func bootAtDebug(t *testing.T, dataDir string) string {
-	t.Helper()
-
-	t.Setenv("DEBUG", "true")
-	t.Setenv("DATA_DIR", dataDir)
-
-	read := captureStdoutToFile(t)
-
-	var sess *session.Session
-
-	app := fxtest.New(
-		t,
-		fx.Provide(
-			globals.New,
-			logger.New,
-			config.New,
-			database.New,
-			session.New,
-		),
-		fx.Populate(&sess),
-	)
-
-	app.RequireStart()
-	app.RequireStop()
-
-	return read()
-}
-
-// requireFirstBootWasLogged is the non-vacuity half of the test below.
-// Without it a build that logged no SQL at all, or that never reached
-// DEBUG, would satisfy every absence assertion.
-func requireFirstBootWasLogged(t *testing.T, out string) {
-	t.Helper()
-
-	require.Contains(
-		t, out, `"level":"DEBUG"`,
-		"DEBUG=true did not reach the logger",
-	)
-	require.Contains(
-		t, out, settingsInsert,
-		"the session key INSERT was not logged, so its absence "+
-			"proves nothing",
-	)
-	require.Contains(
-		t, out, usersInsert,
-		"the admin user INSERT was not logged, so its absence "+
-			"proves nothing",
-	)
-}
-
-// TestFirstBootAtDebug_LogsNeitherSecret is the definition of done.
-//
-// A first boot is the only boot that writes either secret. The
-// settings INSERT carries the base64 session encryption key, which is
-// the whole of the session security model: anyone holding it can forge
-// an authenticated session cookie. The users INSERT carries the admin
-// account's Argon2id hash. Under interpolated statement logging both
-// landed in the log an operator diagnosing a startup problem pastes
-// into an issue.
-//
-// The secrets are read back out of the database file afterwards, so
-// the assertions are made against the values this boot actually
-// generated rather than against a pattern that might not match them.
-//
-// What this test does not cover: the initial admin password itself,
-// which internal/database logs once in the clear, on purpose, because
-// that line is the only place an operator ever sees it. That is a
-// separate decision from the SQL log, and it is documented in the
-// README rather than asserted here.
-//
-// Not parallel: it redirects os.Stdout and sets environment variables,
-// both process-global.
-//
-//nolint:paralleltest // Deliberately sequential; see above.
-func TestFirstBootAtDebug_LogsNeitherSecret(t *testing.T) {
-	dataDir := t.TempDir()
-
-	// An empty DATA_DIR is what makes this a first boot: with a
-	// database already in place neither INSERT runs.
-	entries, err := os.ReadDir(dataDir)
-	require.NoError(t, err)
-	require.Empty(t, entries, "DATA_DIR was not empty")
-
-	out := bootAtDebug(t, dataDir)
-
-	requireFirstBootWasLogged(t, out)
-
-	secrets := readFirstBootSecrets(t, dataDir)
-
-	assert.NotContains(
-		t, out, secrets.sessionKey,
-		"the session encryption key reached the debug log",
-	)
-	assert.NotContains(
-		t, out, secrets.passwordHash,
-		"the admin password hash reached the debug log",
-	)
-	assert.NotContains(
-		t, out, argon2Prefix,
-		"an encoded Argon2id hash reached the debug log",
-	)
-}

internal/gormlog/gormlog.go

@@ -17,10 +17,6 @@
 // level the operator controls, they are shaped by whichever handler
 // internal/logger selected, and every value a client can influence is
 // spent through logfield.Truncate.
-//
-// It also logs no bound value at all. See ParamsFilter: the statement
-// is written with its placeholders intact, at every level, so the
-// values a statement carries never reach the log in the first place.
 package gormlog
 
 import (
@@ -30,7 +26,6 @@ import (
 	"log/slog"
 	"time"
 
-	"gorm.io/gorm"
 	gormlogger "gorm.io/gorm/logger"
 	"sneak.berlin/go/webhooker/internal/logfield"
 )
@@ -52,14 +47,8 @@ type Logger struct {
 }
 
 // Interface compliance is asserted here rather than discovered at the
-// gorm.Open call sites. gorm.ParamsFilter is the optional half: GORM
+// gorm.Open call sites.
-// type-asserts for it and silently keeps interpolating if it is
+var _ gormlogger.Interface = (*Logger)(nil)
-// missing, so losing it would cost no build error and no test that
-// does not look at the emitted SQL.
-var (
-	_ gormlogger.Interface = (*Logger)(nil)
-	_ gorm.ParamsFilter    = (*Logger)(nil)
-)
 
 // New returns a GORM logger that writes through log.
 func New(log *slog.Logger) *Logger {
@@ -82,46 +71,6 @@ func (l *Logger) LogMode(gormlogger.LogLevel) gormlogger.Interface {
 	return l
 }
 
-// ParamsFilter drops every bound value before GORM renders a statement
-// for the log, so what is logged is the statement's shape — its
-// placeholders — and never the values in it.
-//
-// GORM builds the string it hands to Trace by calling
-// Dialector.Explain(sql, vars...), which substitutes each value into
-// the statement. Discarding vars here leaves the '?' placeholders in
-// place, because ExplainSQL only substitutes while it still has a
-// value for the next one. That happens before Trace is reached, so it
-// holds on all three of its arms: the failed statement, the slow one,
-// and the routine one an operator sees at DEBUG.
-//
-// This is the whole of the fix, and it is deliberately unconditional
-// rather than a list of tables to redact. At first boot the two
-// statements that carry a secret are the INSERT into settings holding
-// the base64 session key — which is the entire session security model,
-// since anyone with it can mint a valid cookie — and the INSERT into
-// users holding the Argon2id hash. A denylist would have had to be
-// extended by hand for every table added afterwards, and the cost of
-// missing one is a credential in a log that gets pasted into issues.
-//
-// What is given up is the ability to read a value out of the log. The
-// statement, the table, the error and the row count are all still
-// there, which is what identifies a failing statement; reproducing it
-// needs the values, and those an operator now gets from the database
-// rather than from the log.
-//
-// One GORM path does not consult this: (*gorm.DB).Scan records the
-// statement through gorm's own traceRecorder, which does not implement
-// this interface. No production code path calls it; its one caller is
-// internal/database/database_test.go:91, whose SELECT 1 binds nothing.
-// scan_guard_test.go fails if a non-test file calls it.
-// (*gorm.DB).Pluck, Row and Raw all run through the normal callback
-// processor and are filtered.
-func (l *Logger) ParamsFilter(
-	_ context.Context, sql string, _ ...any,
-) (string, []any) {
-	return sql, nil
-}
-
 // Info logs one of GORM's own informational messages.
 func (l *Logger) Info(
 	ctx context.Context, msg string, data ...any,
@@ -144,9 +93,9 @@ func (l *Logger) Error(
 }
 
 // Trace reports the outcome of a single statement. GORM calls it for
-// every statement it runs, so the cheap paths stay cheap: fc() renders
+// every statement it runs, so the cheap paths stay cheap: fc()
-// the statement — with placeholders, per ParamsFilter — and is called
+// renders the interpolated SQL and is called only on a branch that
-// only on a branch that will actually emit.
+// will actually emit.
 //
 // The arms are ordered exactly as GORM's own Trace orders them —
 // non-record-not-found error, then slow, then the routine case — so

internal/gormlog/gormlog_test.go

@@ -232,7 +232,7 @@ func TestSlowRecordNotFound_IsStillReportedSlow(t *testing.T) {
 				require.ErrorIs(t, err, gorm.ErrRecordNotFound)
 
 				assert.Contains(
-					t, buf.String(), slowLine,
+					t, buf.String(), "slow sql statement",
 					"a slow statement that missed was not "+
 						"reported as slow",
 				)
@@ -284,11 +284,9 @@ func TestRecordNotFoundFlood_DoesNotGrowWithInput(t *testing.T) {
 }
 
 // TestStatementError_LineIsBounded covers the branch that does log.
-// A driver error is not ErrRecordNotFound, so the statement is
+// A driver error is not ErrRecordNotFound, so the interpolated
-// written, and the driver's own error text can quote what the client
+// statement is written — and on an insert the interpolated value is
-// supplied. The statement's parameters are no longer part of that —
+// still whatever the client supplied.
-// see TestBoundValues_NeverReachTheLog — but the budget is what holds
-// the line when the statement itself, or the error, is the long part.
 func TestStatementError_LineIsBounded(t *testing.T) {
 	t.Parallel()
 
@@ -315,7 +313,7 @@ func TestStatementError_LineIsBounded(t *testing.T) {
 				require.Error(t, err)
 
 				assert.Contains(
-					t, buf.String(), errorLine,
+					t, buf.String(), "sql statement failed",
 				)
 				assertBounded(t, buf.String())
 			})
@@ -331,22 +329,22 @@ func TestStatementError_LineIsBounded(t *testing.T) {
 //     and would have cost this report, which is the one thing GORM's
 //     logger gave an operator that nothing else in this service does.
 //   - routine. The branch an operator reaches by turning the level
-//     down to DEBUG: every statement is reported, so every statement
+//     down to DEBUG: every statement is reported, so every
-//     has to be bounded too.
+//     statement's interpolated parameters have to be bounded too.
 func TestSucceedingStatement_LineIsBoundedOnEitherArm(t *testing.T) {
 	t.Parallel()
 
-	// routineLine is a substring of slowLine, so the routine arm
+	// "sql statement" is a substring of "slow sql statement", so the
-	// carries notWant as well: Contains alone cannot tell the two arms
+	// routine arm carries notWant as well: Contains alone cannot tell
-	// apart in that direction.
+	// the two arms apart in that direction.
 	arms := []struct {
 		name    string
 		slow    time.Duration
 		want    string
 		notWant string
 	}{
-		{"slow", alwaysSlow, slowLine, ""},
+		{"slow", alwaysSlow, "slow sql statement", ""},
-		{"routine", neverSlow, routineLine, slowLine},
+		{"routine", neverSlow, "sql statement", "slow sql statement"},
 	}
 
 	for _, a := range arms {

internal/gormlog/scan_guard_test.go

@@ -1,260 +0,0 @@
-package gormlog_test
-
-import (
-	"fmt"
-	"go/ast"
-	"go/parser"
-	"go/token"
-	"io/fs"
-	"os"
-	"path/filepath"
-	"strings"
-	"testing"
-
-	"github.com/stretchr/testify/require"
-)
-
-// minNonTestFiles guards the walk below against passing because it
-// found nothing to look at. The tree held 60 non-test .go files when
-// this was written.
-const minNonTestFiles = 40
-
-// isRowProducer reports whether name is a method that returns a
-// database/sql row handle. GORM's Row and Rows return *sql.Row and
-// *sql.Rows, so Scan on the result of one of them is database/sql's
-// Scan and never (*gorm.DB).Scan.
-func isRowProducer(name string) bool {
-	switch name {
-	case "Row", "Rows", "QueryRow", "QueryRowContext":
-		return true
-	default:
-		return false
-	}
-}
-
-// receiverIsRowHandle reports whether x is syntactically a call to a
-// row producer, which is the only receiver form this check accepts for
-// a Scan.
-func receiverIsRowHandle(x ast.Expr) bool {
-	call, ok := x.(*ast.CallExpr)
-	if !ok {
-		return false
-	}
-
-	sel, ok := call.Fun.(*ast.SelectorExpr)
-	if !ok {
-		return false
-	}
-
-	return isRowProducer(sel.Sel.Name)
-}
-
-// unguardedScans returns the position of every Scan call in file whose
-// receiver is not a row handle. It fails closed: a receiver it cannot
-// resolve syntactically — a local variable, a struct field — is
-// reported rather than assumed safe.
-func unguardedScans(
-	fset *token.FileSet, file *ast.File,
-) []token.Position {
-	var found []token.Position
-
-	ast.Inspect(file, func(n ast.Node) bool {
-		call, ok := n.(*ast.CallExpr)
-		if !ok {
-			return true
-		}
-
-		sel, ok := call.Fun.(*ast.SelectorExpr)
-		if !ok || sel.Sel.Name != "Scan" {
-			return true
-		}
-
-		if !receiverIsRowHandle(sel.X) {
-			found = append(found, fset.Position(sel.Sel.Pos()))
-		}
-
-		return true
-	})
-
-	return found
-}
-
-// moduleRoot walks up from the working directory to the directory
-// holding go.mod.
-func moduleRoot(t *testing.T) string {
-	t.Helper()
-
-	dir, err := os.Getwd()
-	require.NoError(t, err)
-
-	for {
-		_, statErr := os.Stat(filepath.Join(dir, "go.mod"))
-		if statErr == nil {
-			return dir
-		}
-
-		parent := filepath.Dir(dir)
-		require.NotEqual(t, parent, dir, "no go.mod above %s", dir)
-
-		dir = parent
-	}
-}
-
-// skipDir reports whether a directory holds no source this check
-// governs.
-func skipDir(name string) bool {
-	switch name {
-	case ".git", "bin", "node_modules", "testdata":
-		return true
-	default:
-		return false
-	}
-}
-
-// walkNonTestGo parses every non-test .go file under root and returns
-// how many it parsed along with every unguarded Scan it found.
-func walkNonTestGo(t *testing.T, root string) (int, []string) {
-	t.Helper()
-
-	var (
-		parsed int
-		hits   []string
-	)
-
-	fset := token.NewFileSet()
-
-	require.NoError(t, filepath.WalkDir(
-		root,
-		func(path string, d fs.DirEntry, err error) error {
-			if err != nil {
-				return err
-			}
-
-			if d.IsDir() {
-				if skipDir(d.Name()) {
-					return fs.SkipDir
-				}
-
-				return nil
-			}
-
-			if !isNonTestGo(d.Name()) {
-				return nil
-			}
-
-			file, err := parser.ParseFile(fset, path, nil, 0)
-			if err != nil {
-				return err
-			}
-
-			parsed++
-
-			for _, pos := range unguardedScans(fset, file) {
-				hits = append(hits, relPosition(root, pos))
-			}
-
-			return nil
-		},
-	))
-
-	return parsed, hits
-}
-
-// isNonTestGo reports whether a file name is Go source this check
-// governs.
-func isNonTestGo(name string) bool {
-	return strings.HasSuffix(name, ".go") &&
-		!strings.HasSuffix(name, "_test.go")
-}
-
-// relPosition renders pos with its path relative to root, so a failure
-// names the file the way the repository does.
-func relPosition(root string, pos token.Position) string {
-	name := pos.Filename
-
-	rel, err := filepath.Rel(root, name)
-	if err == nil {
-		name = rel
-	}
-
-	return fmt.Sprintf("%s:%d:%d", name, pos.Line, pos.Column)
-}
-
-// TestGormScanIsNeverCalledOutsideTests keeps (*gorm.DB).Scan out of
-// non-test code.
-//
-// It is the one statement path (*Logger).ParamsFilter does not reach:
-// Scan swaps GORM's own trace recorder in for the adapter, and that
-// recorder does not implement gorm.ParamsFilter, so the statement is
-// logged with its values interpolated. The package comment states the
-// limit; this fails when someone adds a call site anyway.
-//
-// The current tree has one caller, internal/database/database_test.go,
-// which this check does not govern: it is test-only and its SELECT 1
-// binds nothing.
-func TestGormScanIsNeverCalledOutsideTests(t *testing.T) {
-	t.Parallel()
-
-	parsed, offenders := walkNonTestGo(t, moduleRoot(t))
-
-	require.GreaterOrEqual(
-		t, parsed, minNonTestFiles,
-		"parsed %d non-test .go files, so this check found "+
-			"nothing to look at", parsed,
-	)
-	require.Empty(
-		t, offenders,
-		"Scan called on a receiver this check cannot show is a "+
-			"database/sql row handle. (*gorm.DB).Scan logs the "+
-			"statement with its bound values interpolated — use "+
-			"Find, Pluck, or Raw(...).Row().Scan instead. A "+
-			"database/sql Scan reached through a variable is "+
-			"reported too; write it as <producer>().Scan rather "+
-			"than widening this check.",
-	)
-}
-
-// scanGuardCase is one planted snippet and whether the check above
-// should report it.
-type scanGuardCase struct {
-	name string
-	body string
-	want int
-}
-
-func scanGuardCases() []scanGuardCase {
-	return []scanGuardCase{
-		{"gorm chain", `db.DB().Raw("SELECT 1").Scan(&v)`, 1},
-		{"gorm receiver", `gdb.Scan(&v)`, 1},
-		{"gorm via variable", "q := gdb.Raw(\"x\")\nq.Scan(&v)", 1},
-		{"gorm model chain", `gdb.Model(&x).Scan(&v)`, 1},
-		{"sql row", `gdb.Raw("SELECT 1").Row().Scan(&v)`, 0},
-		{"sql rows", `gdb.Raw("SELECT 1").Rows().Scan(&v)`, 0},
-		{"unrelated call", `gdb.Find(&v)`, 0},
-	}
-}
-
-// TestScanGuard_ReportsPlantedCalls proves the check fires. Without it
-// a detector that matched nothing would satisfy the walk above no
-// matter what the tree contained.
-func TestScanGuard_ReportsPlantedCalls(t *testing.T) {
-	t.Parallel()
-
-	for _, tc := range scanGuardCases() {
-		t.Run(tc.name, func(t *testing.T) {
-			t.Parallel()
-
-			fset := token.NewFileSet()
-			src := fmt.Sprintf(
-				"package p\n\nfunc f() {\n\t%s\n}\n", tc.body,
-			)
-
-			file, err := parser.ParseFile(
-				fset, tc.name+".go", src, 0,
-			)
-			require.NoError(t, err)
-
-			require.Len(t, unguardedScans(fset, file), tc.want)
-		})
-	}
-}

internal/gormlog/values_test.go

@@ -1,185 +0,0 @@
-package gormlog_test
-
-import (
-	"bytes"
-	"strings"
-	"testing"
-	"time"
-
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"gorm.io/gorm"
-)
-
-// secretValue is bound as a parameter by every case below. Nothing
-// else in this package writes it, so finding it in captured output
-// means a bound value was rendered into the log.
-const secretValue = "QQBOUNDVALUEMARKERQQ"
-
-// The three messages Trace emits under, one per arm.
-//
-// routineLine is a substring of slowLine, so a case that wants the
-// routine arm has to rule the slow one out as well rather than rely on
-// Contains alone.
-const (
-	routineLine = "sql statement"
-	slowLine    = "slow sql statement"
-	errorLine   = "sql statement failed"
-)
-
-// boundValueCase is one arm of Trace, driven by a statement that binds
-// secretValue.
-type boundValueCase struct {
-	name  string
-	slow  time.Duration
-	want  string
-	drive func(t *testing.T, gdb *gorm.DB)
-}
-
-// insertSecret returns a driver that inserts one row whose Name is the
-// secret.
-func insertSecret(id string) func(*testing.T, *gorm.DB) {
-	return func(t *testing.T, gdb *gorm.DB) {
-		t.Helper()
-
-		require.NoError(t, gdb.Create(&thing{
-			ID: id, Name: secretValue,
-		}).Error)
-	}
-}
-
-// insertSecretTwice drives the error arm: the same primary key a
-// second time is a UNIQUE constraint failure, which is an error GORM
-// logs with the statement.
-func insertSecretTwice(t *testing.T, gdb *gorm.DB) {
-	t.Helper()
-
-	require.NoError(t, gdb.Create(&thing{
-		ID: secretValue, Name: secretValue,
-	}).Error)
-	require.Error(t, gdb.Create(&thing{
-		ID: secretValue, Name: "other",
-	}).Error)
-}
-
-// selectSecret drives a query whose WHERE clause binds the secret,
-// covering the read side as well as the write side.
-func selectSecret(t *testing.T, gdb *gorm.DB) {
-	t.Helper()
-
-	var got []thing
-
-	require.NoError(
-		t, gdb.Where("name = ?", secretValue).Find(&got).Error,
-	)
-}
-
-func boundValueCases() []boundValueCase {
-	return []boundValueCase{
-		{
-			name: "routine", slow: neverSlow,
-			want: routineLine, drive: insertSecret("routine"),
-		},
-		{
-			name: "slow", slow: alwaysSlow,
-			want: slowLine, drive: insertSecret("slow"),
-		},
-		{
-			name: "error", slow: neverSlow,
-			want: errorLine, drive: insertSecretTwice,
-		},
-		{
-			name: "select", slow: neverSlow,
-			want: routineLine, drive: selectSecret,
-		},
-	}
-}
-
-// TestBoundValues_NeverReachTheLog states the values-off property
-// directly, on each arm of Trace that emits.
-//
-// Truncation is not what is being asserted. A bounded secret is still
-// a secret: the session key is 44 base64 characters and an Argon2id
-// hash under 100, so both fit inside every budget this package
-// applies. What keeps them out is that the adapter logs the
-// statement's shape and discards its parameters — see
-// (*Logger).ParamsFilter — and that has to hold at DEBUG as much as on
-// an error, because DEBUG is the level at which a successful INSERT is
-// written at all.
-//
-// Each case also requires a placeholder in the logged statement.
-// Without that, the absence of the value would be satisfied by a
-// logger that wrote nothing useful.
-func TestBoundValues_NeverReachTheLog(t *testing.T) {
-	t.Parallel()
-
-	for _, tc := range boundValueCases() {
-		for _, h := range handlers() {
-			t.Run(tc.name+"/"+h.name, func(t *testing.T) {
-				t.Parallel()
-
-				var buf bytes.Buffer
-
-				gdb := openDB(t, &buf, h.make(&buf), tc.slow)
-
-				tc.drive(t, gdb)
-
-				assertNoBoundValue(t, buf.String(), tc.want)
-			})
-		}
-	}
-}
-
-// assertNoBoundValue holds one captured arm to the property: it wrote
-// the line it was supposed to write, that line kept its placeholders,
-// and it carried no bound value.
-func assertNoBoundValue(t *testing.T, out, want string) {
-	t.Helper()
-
-	require.Contains(
-		t, out, want,
-		"the arm under test wrote nothing, so the assertions "+
-			"below are vacuous",
-	)
-	assert.NotContains(
-		t, out, secretValue,
-		"a bound parameter was rendered into the log",
-	)
-	assert.Contains(
-		t, out, "?",
-		"the statement was logged without its placeholders",
-	)
-}
-
-// TestInsert_KeepsOnePlaceholderPerBoundValue pins the shape of the
-// INSERT specifically, since that is the statement that carries both
-// first-boot secrets. A statement that dropped one value and kept the
-// other would satisfy the assertions above.
-func TestInsert_KeepsOnePlaceholderPerBoundValue(t *testing.T) {
-	t.Parallel()
-
-	var buf bytes.Buffer
-
-	gdb := openDB(t, &buf, handlers()[0].make(&buf), neverSlow)
-
-	require.NoError(t, gdb.Create(&thing{
-		ID: "m", Name: secretValue,
-	}).Error)
-
-	out := buf.String()
-
-	require.Contains(t, out, "INSERT INTO")
-	assert.NotContains(t, out, secretValue)
-
-	for line := range strings.SplitSeq(out, "\n") {
-		if !strings.Contains(line, "INSERT INTO") {
-			continue
-		}
-
-		assert.GreaterOrEqual(
-			t, strings.Count(line, "?"), 2,
-			"insert logged fewer placeholders than it bound "+
-				"values: %s", line,
-		)
-	}
-}

internal/handlers/delivery_result_view.go

@@ -7,15 +7,13 @@ import (
 // maxRenderedResponseBytes caps how many bytes of one stored
 // delivery response body reach the event log page.
 //
-// The bound is enforced here and in SQL, because this page's
+// It matches the cap the delivery engine applies when it
-// memory profile must not depend on a constant in another
+// records a result, so nothing written by the current engine
-// package staying where it is, and because rows predating the
+// is cut twice. The bound is enforced here anyway, and in
-// delivery engine's own cap or restored from an archive are
+// SQL: this page's memory profile must not depend on a
-// not covered by it at all.
+// constant in another package staying where it is, and rows
-//
+// predating that cap or restored from an archive are not
-// It happens to equal that engine cap, so a row written by the
+// covered by it at all.
-// current engine reaches this one exactly and is never cut
-// twice. Nothing here may assume the two differ: see view.
 const maxRenderedResponseBytes = 4096
 
 // deliveryResultColumns is the delivery attempt projection.
@@ -61,9 +59,8 @@ type DeliveryResultView struct {
 	// content and must only ever be rendered escaped.
 	ResponseBody string
 
-	// ResponseBytes is the size of the stored response body,
+	// ResponseBytes is the true size of the stored response
-	// before the cut and before redaction. It is what the
+	// body, before the cut and before redaction.
-	// remote sent only when ResponseSizeKnown is set.
 	ResponseBytes int64
 
 	// ResponseShownBytes is how much of that the page is
@@ -73,18 +70,10 @@ type DeliveryResultView struct {
 	// then changed the length.
 	ResponseShownBytes int
 
-	// ResponseTruncated reports that the body shown may be
+	// ResponseTruncated reports that the stored response was
-	// incomplete, so the page owes the reader a marker. Every
+	// larger than the cap, so the page owes the reader a
-	// body that reaches the cap counts, because one the
+	// marker.
-	// delivery engine cut at its own equal cap is
-	// indistinguishable from a complete one.
 	ResponseTruncated bool
-
-	// ResponseSizeKnown reports that ResponseBytes is the whole
-	// response the remote sent, so the marker may quote it. It
-	// is false for a body that only reaches the cap, where how
-	// much came after it was never recorded.
-	ResponseSizeKnown bool
 }
 
 // HasStatusCode reports whether the attempt got as far as an
@@ -115,31 +104,11 @@ func (r *deliveryResultRow) view(
 	redactor delivery.Redactor,
 ) DeliveryResultView {
 	body := r.ResponseBody
+	truncated := r.ResponseBytes > int64(len(body))
 
-	// Two different cuts can have shortened this body, and the
+	// Only a cut response can have been left mid-sequence by
-	// row records only one of them. SQLite cuts here, whenever
+	// this query, exactly as with an event body.
-	// the stored value is larger than the cap. The delivery
+	if truncated {
-	// engine cut earlier, whenever the remote sent more than
-	// its own maxBodyLog — which is this same number, so such a
-	// row stores the cut length as its whole length and nothing
-	// in it separates a response that ended at the cap from one
-	// severed there.
-	//
-	// So a body that reaches the cap is treated as cut either
-	// way. Gating on ResponseBytes alone would assume the two
-	// caps differ, and they do not: under the current engine
-	// that gate never opens.
-	cut := r.ResponseBytes > int64(len(body)) ||
-		len(body) >= maxRenderedResponseBytes
-
-	// The row holds more than the page shows only in the first
-	// of those cases. In the second the stored row is all there
-	// is, and its size is a floor rather than the true one.
-	sizeKnown := r.ResponseBytes > int64(len(body))
-
-	// Only a cut response can have been left mid-sequence,
-	// exactly as with an event body.
-	if cut {
 		body = trimPartialRune(body)
 	}
 
@@ -148,7 +117,7 @@ func (r *deliveryResultRow) view(
 	// controls where the cut falls inside that credential, and
 	// the severed prefix left behind matches no secret whole.
 	rendered := string(body)
-	if cut {
+	if truncated {
 		rendered = redactor.RedactCut(rendered)
 	} else {
 		rendered = redactor.Redact(rendered)
@@ -163,7 +132,6 @@ func (r *deliveryResultRow) view(
 		ResponseBody:       rendered,
 		ResponseBytes:      r.ResponseBytes,
 		ResponseShownBytes: len(body),
-		ResponseTruncated:  cut,
+		ResponseTruncated:  truncated,
-		ResponseSizeKnown:  sizeKnown,
 	}
 }

internal/handlers/delivery_result_view_test.go

@@ -249,78 +249,27 @@ func TestHandleSourceLogs_RedactsCredentialEchoedInError(
 	assert.Contains(t, body, "i/o timeout")
 }
 
-// severedPadding is the filler that puts the end of an echoed
+// TestHandleSourceLogs_RedactsCredentialSeveredByTheCut is the
-// webhook URL five bytes past a cut at the response cap, so
+// regression test for a redactor that ran after the cut. The
-// the cut leaves the workspace ID, the bot ID and all but the
+// remote chooses the padding in front of the credential it
-// last few token characters behind.
+// echoes, so it chooses where the 4096-byte cut lands inside
-func severedPadding() string {
+// that credential; the severed prefix matches no secret whole
+// and used to render verbatim.
+func TestHandleSourceLogs_RedactsCredentialSeveredByTheCut(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	// Padding sized so the cut falls five bytes before the end
+	// of the webhook URL, leaving the workspace ID, the bot ID
+	// and all but the last few token characters in the cut
+	// body.
 	const severedTail = 5
 
-	return strings.Repeat(
+	pad := strings.Repeat(
 		"A", responseCap-len(slackWebhookURL)+severedTail,
 	)
-}
+	stored := pad + slackWebhookURL + strings.Repeat("Z", 128)
-
-// TestHandleSourceLogs_RedactsCredentialSeveredByTheEngineCut
-// is the regression test for a redactor gated on the SQL cut
-// alone. The delivery engine stops reading a response at its
-// own cap, which is the same number of bytes this page
-// renders, so a row the engine cut is byte-for-byte
-// indistinguishable from a complete response and that gate
-// never opened on anything the engine writes.
-//
-// The seeded body is what the engine stores for any remote
-// that sends at least that much: exactly responseCap bytes,
-// ending in a severed webhook URL.
-// TestDeliverHTTP_CutsStoredResponseAtMaxBodyLog in
-// internal/delivery pins that this is the size it produces.
-func TestHandleSourceLogs_RedactsCredentialSeveredByTheEngineCut(
-	t *testing.T,
-) {
-	t.Parallel()
-
-	sent := severedPadding() + slackWebhookURL +
-		strings.Repeat("Z", 128)
-	stored := sent[:responseCap]
-
-	require.Len(
-		t, stored, responseCap,
-		"the engine stores exactly the cap, never more",
-	)
-	require.Contains(
-		t, stored, "T00000000",
-		"the severed credential must be in what is seeded",
-	)
-
-	body := seedFailureAndRender(
-		t,
-		database.TargetTypeSlack,
-		`{"webhookUrl":"`+slackWebhookURL+`"}`,
-		stored,
-	)
-
-	assert.NotContains(t, body, "T00000000")
-	assert.NotContains(t, body, "B00000000")
-	assert.Contains(t, body, delivery.RedactionMarker)
-	assert.Contains(
-		t, body, "reached the recording limit",
-		"a body the engine cut must not be shown as complete",
-	)
-}
-
-// TestHandleSourceLogs_RedactsCredentialSeveredBySQLCut covers
-// the same severing for a row larger than the cap, which is
-// SQLite's cut rather than the engine's. The current engine
-// writes no such row; rows predating its cap or restored from
-// an archive are not bounded by it, which is why the page cuts
-// again in SQL and has to redact that cut too.
-func TestHandleSourceLogs_RedactsCredentialSeveredBySQLCut(
-	t *testing.T,
-) {
-	t.Parallel()
-
-	stored := severedPadding() + slackWebhookURL +
-		strings.Repeat("Z", 128)
 
 	require.Greater(
 		t, len(stored), responseCap,
@@ -342,11 +291,11 @@ func TestHandleSourceLogs_RedactsCredentialSeveredBySQLCut(
 	assert.Contains(t, body, delivery.RedactionMarker)
 }
 
-// TestHandleSourceLogs_RedactsForSoftDeletedTarget covers a
+// TestHandleSourceLogs_RedactsForSoftDeletedTarget covers the
-// target an operator has deleted. The row is only soft deleted
+// target most likely to have a bad destination URL in its
-// and its deliveries survive in the per-webhook database, so
+// history: deleting and recreating is how an operator fixes a
-// its redactor has to survive with it or every response body
+// mistyped one. The row is only soft deleted and its
-// it ever recorded renders unredacted.
+// deliveries survive, so its redactor has to survive with it.
 func TestHandleSourceLogs_RedactsForSoftDeletedTarget(
 	t *testing.T,
 ) {

internal/handlers/export_test.go

@@ -51,9 +51,7 @@ func (s *Handlers) LoadEventLogViewsForTest(
 	webhook database.Webhook,
 	page int,
 ) []EventLogView {
-	views, _, _ := s.loadEventsWithDeliveries(
+	views, _ := s.loadEventsWithDeliveries(w, webhook, nil, page)
-		w, webhook, nil, page,
-	)
 
 	return views
 }
@@ -79,29 +77,18 @@ func (s *Handlers) RenderTemplateForTest(
 	s.renderTemplate(w, r, pageTemplate, data)
 }
 
-// BuildSlackTargetConfigForTest exposes
+// BuildSlackTargetConfigForTest exposes buildURLTargetConfig
-// buildSlackTargetConfig for use in the handlers_test package.
+// with the Slack target parameters for use in the
+// handlers_test package.
 func (s *Handlers) BuildSlackTargetConfigForTest(
 	w http.ResponseWriter,
 	r *http.Request,
 	targetURL string,
 ) (string, error) {
-	return s.buildSlackTargetConfig(w, r, targetURL)
+	return s.buildURLTargetConfig(
-}
+		w, r, targetURL, "webhookUrl",
-
+		"Webhook URL is required for Slack targets",
-// BuildHTTPTargetConfigForTest exposes buildHTTPTargetConfig
+	)
-// for use in the handlers_test package, taking the form fields
-// an HTTP target's configuration is built from.
-func (s *Handlers) BuildHTTPTargetConfigForTest(
-	w http.ResponseWriter,
-	r *http.Request,
-	targetURL, headers, timeout string,
-) (string, error) {
-	return s.buildHTTPTargetConfig(w, r, targetFormInput{
-		URL:     targetURL,
-		Headers: headers,
-		Timeout: timeout,
-	})
 }
 
 // BuildDatabaseTargetConfigForTest exposes

internal/handlers/handlers.go

@@ -18,7 +18,6 @@ import (
 	"sneak.berlin/go/webhooker/internal/globals"
 	"sneak.berlin/go/webhooker/internal/healthcheck"
 	"sneak.berlin/go/webhooker/internal/logger"
-	"sneak.berlin/go/webhooker/internal/metrics"
 	"sneak.berlin/go/webhooker/internal/middleware"
 	"sneak.berlin/go/webhooker/internal/session"
 	"sneak.berlin/go/webhooker/templates"
@@ -74,7 +73,6 @@ type Handlers struct {
 	mw        *middleware.Middleware
 	notifier  delivery.Notifier
 	evictor   delivery.WebhookEvictor
-	mtr       *metrics.Set
 	templates map[string]*template.Template
 
 	// dummyVerifications counts the equivalent-cost verifications
@@ -116,7 +114,6 @@ func New(
 	s.mw = params.Middleware
 	s.notifier = params.Notifier
 	s.evictor = params.Evictor
-	s.mtr = metrics.Default()
 
 	// Parse all page templates once at startup
 	s.templates = map[string]*template.Template{
@@ -127,7 +124,6 @@ func New(
 		"source_detail.html": parsePageTemplate("source_detail.html"),
 		"source_edit.html":   parsePageTemplate("source_edit.html"),
 		"source_logs.html":   parsePageTemplate("source_logs.html"),
-		"target_edit.html":   parsePageTemplate("target_edit.html"),
 	}
 
 	lc.Append(fx.Hook{

internal/handlers/source_management.go

@@ -805,12 +805,9 @@ func (h *Handlers) HandleSourceLogs() http.HandlerFunc {
 
 		page := h.parsePage(r)
 
-		evts, total, ok := h.loadEventsWithDeliveries(
+		evts, total := h.loadEventsWithDeliveries(
 			w, webhook, targets, page,
 		)
-		if !ok {
-			return
-		}
 
 		totalPages := int(total) / paginationPerPage
 		if int(total)%paginationPerPage != 0 {
@@ -902,22 +899,18 @@ func (h *Handlers) parsePage(r *http.Request) int {
 // deliveries from the per-webhook database. Events come back
 // as capped projections rather than database.Event rows: see
 // eventLogColumns for why the cut happens in SQL.
-//
-// The bool reports whether the load succeeded. It is false
-// once this has answered the request with an error, and the
-// caller must then render nothing further.
 func (h *Handlers) loadEventsWithDeliveries(
 	w http.ResponseWriter,
 	webhook database.Webhook,
 	targetMap map[string]eventLogTarget,
 	page int,
-) ([]EventLogView, int64, bool) {
+) ([]EventLogView, int64) {
 	var totalEvents int64
 
 	var result []EventLogView
 
 	if !h.dbMgr.DBExists(webhook.ID) {
-		return result, totalEvents, true
+		return result, totalEvents
 	}
 
 	webhookDB, err := h.dbMgr.GetDB(webhook.ID)
@@ -926,7 +919,7 @@ func (h *Handlers) loadEventsWithDeliveries(
 			w, "failed to get webhook database", err,
 		)
 
-		return nil, 0, false
+		return nil, 0
 	}
 
 	webhookDB.Model(&database.Event{}).Where(
@@ -964,16 +957,7 @@ func (h *Handlers) loadEventsWithDeliveries(
 		}
 	}
 
-	attempts, err := h.loadDeliveryResults(
+	attempts := h.loadDeliveryResults(webhookDB, deliveryIDs)
-		webhookDB, deliveryIDs,
-	)
-	if err != nil {
-		h.serverError(
-			w, "failed to load delivery attempts", err,
-		)
-
-		return nil, 0, false
-	}
 
 	for i := range rows {
 		result[i].Deliveries = newDeliveryViews(
@@ -981,7 +965,7 @@ func (h *Handlers) loadEventsWithDeliveries(
 		)
 	}
 
-	return result, totalEvents, true
+	return result, totalEvents
 }
 
 // deliveryIDChunkSize bounds how many delivery IDs go into one
@@ -1003,7 +987,7 @@ const deliveryIDChunkSize = 500
 func (h *Handlers) loadDeliveryResults(
 	webhookDB *gorm.DB,
 	deliveryIDs []string,
-) (map[string][]deliveryResultRow, error) {
+) map[string][]deliveryResultRow {
 	byDelivery := make(map[string][]deliveryResultRow)
 
 	for chunk := range slices.Chunk(
@@ -1019,11 +1003,15 @@ func (h *Handlers) loadDeliveryResults(
 			"delivery_id IN ?", chunk,
 		).Order("attempt_num ASC").Find(&rows).Error
 		if err != nil {
-			// Returning what was loaded so far renders the
+			// A discarded error here renders as a delivery that
-			// deliveries in the failed chunk as never having run,
+			// never ran, which is indistinguishable from one
-			// which is indistinguishable from ones that really
+			// that really never ran.
-			// never ran. The page fails instead.
+			h.log.Error(
-			return nil, err
+				"failed to load delivery attempts",
+				"error", err,
+			)
+
+			return byDelivery
 		}
 
 		for i := range rows {
@@ -1033,7 +1021,7 @@ func (h *Handlers) loadDeliveryResults(
 		}
 	}
 
-	return byDelivery, nil
+	return byDelivery
 }
 
 // newDeliveryViews projects deliveries for rendering,
@@ -1225,7 +1213,9 @@ func (h *Handlers) processTargetCreate(
 	// Referer headers and error trackers record.
 	name := r.PostFormValue("name")
 	targetType := database.TargetType(r.PostFormValue("type"))
+	targetURL := r.PostFormValue("url")
 	maxRetriesStr := r.PostFormValue("max_retries")
+	expiry := r.PostFormValue("expiry")
 
 	if name == "" {
 		http.Error(
@@ -1245,7 +1235,7 @@ func (h *Handlers) processTargetCreate(
 	}
 
 	configJSON, err := h.buildTargetConfig(
-		w, r, targetType, targetFormInputFrom(r),
+		w, r, targetType, targetURL, expiry,
 	)
 	if err != nil {
 		return
@@ -1302,60 +1292,28 @@ func parseNonNegativeInt(s string) int {
 	return 0
 }
 
-// targetFormInput carries the raw form values describing a target's
+// buildTargetConfig builds the JSON config string for a target.
-// configuration. Both the create and the edit path fill one and hand
+// The expiry form value is read by the caller (which bounds the
-// it to buildTargetConfig, so neither can come to validate a
+// request body) and applies to database targets only.
-// destination differently from the other.
-type targetFormInput struct {
-	// URL is the destination for an HTTP target and the webhook URL
-	// for a Slack target.
-	URL string
-	// Headers is an HTTP target's headers, one "Name: value" per
-	// line.
-	Headers string
-	// Timeout is an HTTP target's per-request timeout in seconds.
-	Timeout string
-	// Expiry is a database (archive) target's row expiry.
-	Expiry string
-}
-
-// targetFormInputFrom reads the configuration fields from a request
-// body. The body size cap is enforced by the MaxBodySize middleware,
-// which runs before CSRF parses the form.
-//
-// Every field is read with PostFormValue, not FormValue. FormValue
-// falls back to the query string, which would let
-// `POST /source/{id}/targets?url=https://hooks.slack.com/...`
-// configure a target from a value the request line carries — and the
-// request line, unlike the body, is what logs, proxies, Referer
-// headers and error trackers record. The headers field is under the
-// same rule and for the same reason: its values are authorization
-// tokens.
-func targetFormInputFrom(r *http.Request) targetFormInput {
-	return targetFormInput{
-		URL:     r.PostFormValue("url"),
-		Headers: r.PostFormValue("headers"),
-		Timeout: r.PostFormValue("timeout"),
-		Expiry:  r.PostFormValue("expiry"),
-	}
-}
-
-// buildTargetConfig builds the JSON config string for a target from
-// the submitted form values, writing its own 4xx response on
-// rejection. Which fields of in apply depends on the target type.
 func (h *Handlers) buildTargetConfig(
 	w http.ResponseWriter,
 	r *http.Request,
 	targetType database.TargetType,
-	in targetFormInput,
+	targetURL, expiry string,
 ) (string, error) {
 	switch targetType {
 	case database.TargetTypeHTTP:
-		return h.buildHTTPTargetConfig(w, r, in)
+		return h.buildURLTargetConfig(
+			w, r, targetURL, "url",
+			"URL is required for HTTP targets",
+		)
 	case database.TargetTypeSlack:
-		return h.buildSlackTargetConfig(w, r, in.URL)
+		return h.buildURLTargetConfig(
+			w, r, targetURL, "webhookUrl",
+			"Webhook URL is required for Slack targets",
+		)
 	case database.TargetTypeDatabase:
-		return h.buildDatabaseTargetConfig(w, in.Expiry)
+		return h.buildDatabaseTargetConfig(w, expiry)
 	case database.TargetTypeLog:
 		return "", nil
 	default:
@@ -1368,83 +1326,14 @@ func (h *Handlers) buildTargetConfig(
 	}
 }
 
-// buildHTTPTargetConfig builds config JSON for an HTTP target: an
+// buildURLTargetConfig builds config JSON for a target whose
-// SSRF-validated destination plus the optional headers and timeout
+// configuration is a single SSRF-validated URL stored under
-// the delivery path honours.
+// configKey. missingMsg is the error shown when no URL is given.
-func (h *Handlers) buildHTTPTargetConfig(
+func (h *Handlers) buildURLTargetConfig(
 	w http.ResponseWriter,
 	r *http.Request,
-	in targetFormInput,
+	targetURL, configKey, missingMsg string,
 ) (string, error) {
-	err := h.validateTargetURL(
-		w, r, in.URL, "URL is required for HTTP targets",
-	)
-	if err != nil {
-		return "", err
-	}
-
-	headers, err := delivery.ParseTargetHeaders(in.Headers)
-	if err != nil {
-		http.Error(
-			w,
-			"Invalid headers: "+err.Error(),
-			http.StatusBadRequest,
-		)
-
-		return "", err
-	}
-
-	timeout, err := delivery.ParseTargetTimeout(in.Timeout)
-	if err != nil {
-		http.Error(
-			w,
-			"Invalid timeout: "+err.Error(),
-			http.StatusBadRequest,
-		)
-
-		return "", err
-	}
-
-	return marshalTargetConfig(w, delivery.HTTPTargetConfig{
-		URL:     in.URL,
-		Headers: headers,
-		Timeout: timeout,
-	})
-}
-
-// buildSlackTargetConfig builds config JSON for a Slack target,
-// whose whole configuration is one SSRF-validated webhook URL.
-func (h *Handlers) buildSlackTargetConfig(
-	w http.ResponseWriter,
-	r *http.Request,
-	targetURL string,
-) (string, error) {
-	err := h.validateTargetURL(
-		w, r, targetURL,
-		"Webhook URL is required for Slack targets",
-	)
-	if err != nil {
-		return "", err
-	}
-
-	return marshalTargetConfig(w, delivery.SlackTargetConfig{
-		WebhookURL: targetURL,
-	})
-}
-
-// validateTargetURL rejects an empty or SSRF-blocked destination,
-// writing the 400 itself. missingMsg is the error shown when no URL
-// is given.
-//
-// It is the single point at which a user-supplied destination enters
-// the SSRF guard, on create and on edit alike. An edit path that
-// reached storage without passing through here would reopen the hole
-// the guard closes.
-func (h *Handlers) validateTargetURL(
-	w http.ResponseWriter,
-	r *http.Request,
-	targetURL, missingMsg string,
-) error {
 	if targetURL == "" {
 		http.Error(
 			w,
@@ -1452,7 +1341,7 @@ func (h *Handlers) validateTargetURL(
 			http.StatusBadRequest,
 		)
 
-		return errMissingURL
+		return "", errMissingURL
 	}
 
 	err := delivery.ValidateTargetURL(
@@ -1473,18 +1362,11 @@ func (h *Handlers) validateTargetURL(
 			http.StatusBadRequest,
 		)
 
-		return err
+		return "", err
 	}
 
-	return nil
+	cfg := map[string]any{configKey: targetURL}
-}
 
-// marshalTargetConfig serialises a target configuration for storage,
-// writing a 500 itself if it cannot.
-func marshalTargetConfig(
-	w http.ResponseWriter,
-	cfg any,
-) (string, error) {
 	configBytes, err := json.Marshal(cfg)
 	if err != nil {
 		http.Error(
@@ -1524,9 +1406,19 @@ func (h *Handlers) buildDatabaseTargetConfig(
 		return "", err
 	}
 
-	return marshalTargetConfig(
+	cfg := map[string]any{"expiry": expiry}
-		w, map[string]any{"expiry": expiry},
+
-	)
+	configBytes, err := json.Marshal(cfg)
+	if err != nil {
+		http.Error(
+			w, "Internal server error",
+			http.StatusInternalServerError,
+		)
+
+		return "", err
+	}
+
+	return string(configBytes), nil
 }
 
 // HandleEntrypointDelete handles deleting an entrypoint.

internal/handlers/target_edit.go

@@ -1,221 +0,0 @@
-package handlers
-
-import (
-	"net/http"
-
-	"github.com/go-chi/chi"
-	"sneak.berlin/go/webhooker/internal/database"
-	"sneak.berlin/go/webhooker/internal/delivery"
-)
-
-// targetEditTemplate is the page the target edit form renders.
-const targetEditTemplate = "target_edit.html"
-
-// tmplKeyTarget is the template data key for the target being
-// edited, and tmplKeyMaxTimeout for the timeout ceiling the form
-// tells the user about.
-const (
-	tmplKeyTarget     = "Target"
-	tmplKeyMaxTimeout = "MaxTimeout"
-)
-
-// configUnreadableMessage is shown when a target's stored
-// configuration does not parse. It says plainly that saving replaces
-// the stored value rather than preserving it, because the form
-// cannot pre-fill what it could not read.
-const configUnreadableMessage = "The stored configuration for this " +
-	"target could not be read. Enter the values below; saving " +
-	"replaces the stored configuration."
-
-// targetEditView is the display model for the target edit page.
-//
-// It carries the target's row fields alongside its UNMASKED
-// configuration, and deliberately omits database.Target's raw
-// Config blob: the form renders named fields, and giving the
-// template the blob as well would put an unreviewed second path to
-// the credential on the page.
-type targetEditView struct {
-	ID         string
-	Name       string
-	Type       database.TargetType
-	Active     bool
-	MaxRetries int
-	Config     delivery.TargetConfigForm
-}
-
-// HandleTargetEdit shows the form to edit a target.
-//
-// This page is the one place the full destination URL and header
-// values are shown. It is reachable only through the
-// /source/{sourceID} route group, which supplies RequireAuth and
-// NoCache, and only for a target of a webhook the session's user
-// owns; masking (delivery.TargetView) is unchanged everywhere else.
-func (h *Handlers) HandleTargetEdit() http.HandlerFunc {
-	return func(w http.ResponseWriter, r *http.Request) {
-		webhook, target, ok := h.ownedTarget(w, r)
-		if !ok {
-			return
-		}
-
-		cfg, err := delivery.NewTargetConfigForm(target)
-		msg := ""
-
-		if err != nil {
-			// The error carries the parse failure, never the
-			// blob, so it is safe to log against the target id.
-			h.log.Warn(
-				"stored target config could not be read for editing",
-				"target_id", target.ID,
-				"error", err,
-			)
-
-			msg = configUnreadableMessage
-		}
-
-		h.renderTargetEdit(w, r, webhook, target, cfg, msg)
-	}
-}
-
-// HandleTargetEditSubmit handles the target edit form submission.
-func (h *Handlers) HandleTargetEditSubmit() http.HandlerFunc {
-	return func(w http.ResponseWriter, r *http.Request) {
-		webhook, target, ok := h.ownedTarget(w, r)
-		if !ok {
-			return
-		}
-
-		// The body size cap is enforced by the MaxBodySize
-		// middleware, which runs before CSRF parses the form.
-		err := r.ParseForm()
-		if err != nil {
-			http.Error(
-				w, "Bad request", http.StatusBadRequest,
-			)
-
-			return
-		}
-
-		h.applyTargetEdit(w, r, webhook, target)
-	}
-}
-
-// applyTargetEdit validates and saves target edits.
-//
-// The submitted configuration goes through buildTargetConfig, the
-// same builder the create path uses, so an edited destination is
-// SSRF-validated exactly as a new one is.
-//
-// The target's type is not editable. Each type stores a different
-// configuration shape and its delivery history is recorded against
-// the target row, so changing the type of an existing target is
-// really the creation of a different one. The stored type decides
-// which fields the form offers and which builder runs.
-func (h *Handlers) applyTargetEdit(
-	w http.ResponseWriter,
-	r *http.Request,
-	webhook database.Webhook,
-	target *database.Target,
-) {
-	name := r.PostFormValue("name")
-	if name == "" {
-		http.Error(
-			w, "Name is required", http.StatusBadRequest,
-		)
-
-		return
-	}
-
-	configJSON, err := h.buildTargetConfig(
-		w, r, target.Type, targetFormInputFrom(r),
-	)
-	if err != nil {
-		// buildTargetConfig has already written the response.
-		return
-	}
-
-	target.Name = name
-	target.Config = configJSON
-
-	// Retries are offered only by the forms for target types that
-	// retry, so an absent field means "this form does not edit
-	// retries" rather than "set them to zero". Reading it
-	// unconditionally would silently disable retries on any target
-	// saved from a form that does not render the input.
-	if r.PostForm.Has("max_retries") {
-		target.MaxRetries = parseNonNegativeInt(
-			r.PostFormValue("max_retries"),
-		)
-	}
-
-	err = h.db.DB().Save(target).Error
-	if err != nil {
-		h.serverError(w, "failed to update target", err)
-
-		return
-	}
-
-	http.Redirect(
-		w, r, "/source/"+webhook.ID, http.StatusSeeOther,
-	)
-}
-
-// renderTargetEdit renders the target edit page with an optional
-// error message.
-func (h *Handlers) renderTargetEdit(
-	w http.ResponseWriter,
-	r *http.Request,
-	webhook database.Webhook,
-	target *database.Target,
-	cfg delivery.TargetConfigForm,
-	errMsg string,
-) {
-	// The template calls Webhook methods, which take pointer
-	// receivers; html/template cannot address a value stored in a
-	// map.
-	data := map[string]any{
-		tmplKeyWebhook: &webhook,
-		tmplKeyTarget: targetEditView{
-			ID:         target.ID,
-			Name:       target.Name,
-			Type:       target.Type,
-			Active:     target.Active,
-			MaxRetries: target.MaxRetries,
-			Config:     cfg,
-		},
-		tmplKeyMaxTimeout: delivery.MaxTargetTimeoutSeconds,
-		tmplKeyError:      errMsg,
-	}
-
-	h.renderTemplate(w, r, targetEditTemplate, data)
-}
-
-// ownedTarget resolves the request's sourceID and targetID
-// parameters to a target of a webhook the session's user owns.
-//
-// Ownership is decided by the webhook, and the target is then
-// scoped to that webhook, so a target id belonging to someone
-// else's webhook is a 404 rather than an edit of their target. It
-// reports false once it has written the response.
-func (h *Handlers) ownedTarget(
-	w http.ResponseWriter,
-	r *http.Request,
-) (database.Webhook, *database.Target, bool) {
-	webhook, ok := h.ownedWebhook(w, r)
-	if !ok {
-		return database.Webhook{}, nil, false
-	}
-
-	var target database.Target
-
-	err := h.db.DB().Where(
-		"id = ? AND webhook_id = ?",
-		chi.URLParam(r, "targetID"), webhook.ID,
-	).First(&target).Error
-	if err != nil {
-		http.NotFound(w, r)
-
-		return database.Webhook{}, nil, false
-	}
-
-	return webhook, &target, true
-}

internal/handlers/target_edit_test.go

@@ -1,637 +0,0 @@
-package handlers_test
-
-import (
-	"context"
-	"encoding/json"
-	"net/http"
-	"net/http/httptest"
-	"net/url"
-	"strings"
-	"testing"
-
-	"github.com/go-chi/chi"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"gorm.io/gorm/clause"
-	"sneak.berlin/go/webhooker/internal/database"
-	"sneak.berlin/go/webhooker/internal/delivery"
-)
-
-// The destinations the target edit tests configure. Both are literal
-// public addresses rather than hostnames so the SSRF check resolves
-// nothing: with a hostname, a sandbox without DNS would reject the
-// URL for the wrong reason and a test asserting rejection would pass
-// even with the guard removed.
-const (
-	editOriginalURL = "https://93.184.216.34/hooks/original"
-	editReplacedURL = "https://93.184.216.34/hooks/replaced"
-	// editBlockedURL resolves to loopback, which the SSRF guard
-	// refuses. It is what proves the guard runs on the edit path.
-	editBlockedURL = "http://127.0.0.1/hooks/internal"
-)
-
-// editAuthHeader carries a bearer credential, the case the headers
-// field exists for.
-const (
-	editBearerSecret = "QQEDITSECRETQQ"
-	editAuthHeader   = "Authorization: Bearer " + editBearerSecret
-)
-
-// targetRouter mounts the target create and edit routes on a chi
-// router so the handlers see the URL parameters they read.
-func targetRouter(env *sourceTestEnv) *chi.Mux {
-	router := chi.NewRouter()
-	router.Post(
-		"/source/{sourceID}/targets",
-		env.handlers.HandleTargetCreate(),
-	)
-	router.Get(
-		"/source/{sourceID}/targets/{targetID}/edit",
-		env.handlers.HandleTargetEdit(),
-	)
-	router.Post(
-		"/source/{sourceID}/targets/{targetID}/edit",
-		env.handlers.HandleTargetEditSubmit(),
-	)
-
-	return router
-}
-
-// serveTarget drives one request through the target routes as the
-// authenticated test user.
-func serveTarget(
-	env *sourceTestEnv,
-	method, path string,
-	form url.Values,
-) *httptest.ResponseRecorder {
-	body := ""
-	if form != nil {
-		body = form.Encode()
-	}
-
-	req := httptest.NewRequestWithContext(
-		context.Background(), method, path,
-		strings.NewReader(body),
-	)
-	if form != nil {
-		req.Header.Set(
-			"Content-Type",
-			"application/x-www-form-urlencoded",
-		)
-	}
-
-	for _, c := range env.cookies {
-		req.AddCookie(c)
-	}
-
-	w := httptest.NewRecorder()
-	targetRouter(env).ServeHTTP(w, req)
-
-	return w
-}
-
-// seedHTTPTarget creates a webhook and an HTTP target on it through
-// the real create handler, so every case starts from a target the
-// production path produced rather than a hand-written row.
-//
-// Standing the fx app up is what a handler test mostly costs, and
-// internal/handlers is already the slowest package in the suite, so
-// the tests below share one env per test function and give each case
-// its own webhook rather than its own app.
-func seedHTTPTarget(
-	t *testing.T,
-	env *sourceTestEnv,
-	headers, timeout string,
-) (database.Webhook, database.Target) {
-	t.Helper()
-
-	webhook := seedWebhookWithRetention(t, env.db, 30)
-
-	form := url.Values{}
-	form.Set("name", "original-name")
-	form.Set("type", string(database.TargetTypeHTTP))
-	form.Set("url", editOriginalURL)
-	form.Set("headers", headers)
-	form.Set("timeout", timeout)
-	form.Set("max_retries", "3")
-
-	w := serveTarget(
-		env, http.MethodPost,
-		"/source/"+webhook.ID+"/targets", form,
-	)
-	require.Equal(t, http.StatusSeeOther, w.Code, w.Body.String())
-
-	targets := targetsForWebhook(t, env.db, webhook.ID)
-	require.Len(t, targets, 1)
-
-	return webhook, targets[0]
-}
-
-// storedTarget reloads a target row.
-func storedTarget(
-	t *testing.T,
-	env *sourceTestEnv,
-	targetID string,
-) database.Target {
-	t.Helper()
-
-	var target database.Target
-
-	require.NoError(
-		t,
-		env.db.DB().Where("id = ?", targetID).
-			First(&target).Error,
-	)
-
-	return target
-}
-
-// storedHTTPConfig reloads a target and parses its stored HTTP
-// configuration.
-func storedHTTPConfig(
-	t *testing.T,
-	env *sourceTestEnv,
-	targetID string,
-) delivery.HTTPTargetConfig {
-	t.Helper()
-
-	var cfg delivery.HTTPTargetConfig
-
-	require.NoError(
-		t,
-		json.Unmarshal(
-			[]byte(storedTarget(t, env, targetID).Config), &cfg,
-		),
-	)
-
-	return cfg
-}
-
-// editForm is the fully populated edit submission for an HTTP
-// target.
-func editForm(targetURL, headers, timeout string) url.Values {
-	form := url.Values{}
-	form.Set("name", "edited-name")
-	form.Set("url", targetURL)
-	form.Set("headers", headers)
-	form.Set("timeout", timeout)
-	form.Set("max_retries", "5")
-
-	return form
-}
-
-// submitTargetEdit posts the edit form for a target.
-func submitTargetEdit(
-	env *sourceTestEnv,
-	webhookID, targetID string,
-	form url.Values,
-) *httptest.ResponseRecorder {
-	return serveTarget(
-		env, http.MethodPost,
-		"/source/"+webhookID+"/targets/"+targetID+"/edit",
-		form,
-	)
-}
-
-// TestHandleTargetCreate_Configuration covers the half of the gap
-// that is not about editing at all: HTTPTargetConfig has carried
-// Headers and Timeout, and the delivery path has honoured them, but
-// the create form wrote {"url":...} and nothing else, so a
-// destination needing an Authorization header could not be
-// configured through the UI at all.
-func TestHandleTargetCreate_Configuration(t *testing.T) {
-	t.Parallel()
-
-	env := setupSourceTest(t)
-
-	t.Run("stores headers and timeout", func(t *testing.T) {
-		t.Parallel()
-		assertCreateStoresHeadersAndTimeout(t, env)
-	})
-
-	t.Run("without them keeps a url-only config", func(t *testing.T) {
-		t.Parallel()
-		assertCreateKeepsURLOnlyConfig(t, env)
-	})
-}
-
-func assertCreateStoresHeadersAndTimeout(
-	t *testing.T, env *sourceTestEnv,
-) {
-	t.Helper()
-
-	_, target := seedHTTPTarget(
-		t, env, editAuthHeader+"\nX-Tenant: acme\n", "12",
-	)
-
-	cfg := storedHTTPConfig(t, env, target.ID)
-
-	assert.Equal(t, editOriginalURL, cfg.URL)
-	assert.Equal(t, 12, cfg.Timeout)
-	assert.Equal(
-		t,
-		map[string]string{
-			"Authorization": "Bearer " + editBearerSecret,
-			"X-Tenant":      "acme",
-		},
-		cfg.Headers,
-	)
-}
-
-// Without the new fields the stored shape must be the same
-// {"url":...} the create form wrote before they existed, so no
-// existing target's configuration is rewritten by this change.
-func assertCreateKeepsURLOnlyConfig(
-	t *testing.T, env *sourceTestEnv,
-) {
-	t.Helper()
-
-	_, target := seedHTTPTarget(t, env, "", "")
-
-	assert.JSONEq(
-		t, `{"url":"`+editOriginalURL+`"}`, target.Config,
-	)
-}
-
-// TestHandleTargetEditSubmit_Saves is the round trip the issue asks
-// for: create a target, edit it, and confirm the stored config
-// changed.
-func TestHandleTargetEditSubmit_Saves(t *testing.T) {
-	t.Parallel()
-
-	env := setupSourceTest(t)
-
-	t.Run("changes the destination URL", func(t *testing.T) {
-		t.Parallel()
-		assertEditChangesDestination(t, env)
-	})
-
-	t.Run("round trips headers and timeout", func(t *testing.T) {
-		t.Parallel()
-		assertEditRoundTripsHeadersAndTimeout(t, env)
-	})
-
-	t.Run("clearing them removes them", func(t *testing.T) {
-		t.Parallel()
-		assertEditClearingRemovesThem(t, env)
-	})
-
-	t.Run("absent max_retries is not zeroed", func(t *testing.T) {
-		t.Parallel()
-		assertEditKeepsAbsentMaxRetries(t, env)
-	})
-}
-
-func assertEditChangesDestination(
-	t *testing.T, env *sourceTestEnv,
-) {
-	t.Helper()
-
-	webhook, target := seedHTTPTarget(t, env, "", "")
-
-	w := submitTargetEdit(
-		env, webhook.ID, target.ID,
-		editForm(editReplacedURL, "", ""),
-	)
-	require.Equal(t, http.StatusSeeOther, w.Code, w.Body.String())
-
-	assert.Equal(
-		t,
-		editReplacedURL,
-		storedHTTPConfig(t, env, target.ID).URL,
-	)
-
-	reloaded := storedTarget(t, env, target.ID)
-	assert.Equal(t, "edited-name", reloaded.Name)
-	assert.Equal(t, 5, reloaded.MaxRetries)
-	assert.Equal(
-		t, database.TargetTypeHTTP, reloaded.Type,
-		"the edit form must not change a target's type",
-	)
-}
-
-// The two previously unreachable fields must survive create,
-// pre-fill and save.
-func assertEditRoundTripsHeadersAndTimeout(
-	t *testing.T, env *sourceTestEnv,
-) {
-	t.Helper()
-
-	webhook, target := seedHTTPTarget(t, env, editAuthHeader, "7")
-
-	w := submitTargetEdit(
-		env, webhook.ID, target.ID,
-		editForm(
-			editOriginalURL,
-			"Authorization: Bearer rotated\nX-Trace: on",
-			"21",
-		),
-	)
-	require.Equal(t, http.StatusSeeOther, w.Code, w.Body.String())
-
-	cfg := storedHTTPConfig(t, env, target.ID)
-
-	assert.Equal(t, 21, cfg.Timeout)
-	assert.Equal(
-		t,
-		map[string]string{
-			"Authorization": "Bearer rotated",
-			"X-Trace":       "on",
-		},
-		cfg.Headers,
-	)
-}
-
-// The direction a naive "only set what was submitted" implementation
-// gets wrong: an emptied field must remove the stored value, not
-// leave the previous one in place.
-func assertEditClearingRemovesThem(
-	t *testing.T, env *sourceTestEnv,
-) {
-	t.Helper()
-
-	webhook, target := seedHTTPTarget(t, env, editAuthHeader, "7")
-
-	w := submitTargetEdit(
-		env, webhook.ID, target.ID,
-		editForm(editOriginalURL, "", ""),
-	)
-	require.Equal(t, http.StatusSeeOther, w.Code, w.Body.String())
-
-	cfg := storedHTTPConfig(t, env, target.ID)
-
-	assert.Empty(t, cfg.Headers)
-	assert.Zero(t, cfg.Timeout)
-}
-
-// Retries are offered only by the forms for target types that retry.
-// An absent field means the form does not edit retries, not that
-// they should be turned off.
-func assertEditKeepsAbsentMaxRetries(
-	t *testing.T, env *sourceTestEnv,
-) {
-	t.Helper()
-
-	webhook, target := seedHTTPTarget(t, env, "", "")
-	require.Equal(t, 3, target.MaxRetries)
-
-	form := editForm(editOriginalURL, "", "")
-	form.Del("max_retries")
-
-	w := submitTargetEdit(env, webhook.ID, target.ID, form)
-	require.Equal(t, http.StatusSeeOther, w.Code, w.Body.String())
-
-	assert.Equal(
-		t, 3, storedTarget(t, env, target.ID).MaxRetries,
-	)
-}
-
-// TestHandleTargetEdit_PrefillsTheStoredValuesUnmasked covers the
-// deliberate exception to the masking rule. The operator cannot
-// correct a value they cannot see, so this page — and only this page
-// — renders the destination and the header values in full.
-func TestHandleTargetEdit_PrefillsTheStoredValuesUnmasked(
-	t *testing.T,
-) {
-	t.Parallel()
-
-	env := setupSourceTest(t)
-
-	webhook, target := seedHTTPTarget(t, env, editAuthHeader, "7")
-
-	w := serveTarget(
-		env, http.MethodGet,
-		"/source/"+webhook.ID+"/targets/"+target.ID+"/edit",
-		nil,
-	)
-	require.Equal(t, http.StatusOK, w.Code)
-
-	page := w.Body.String()
-
-	assert.Contains(t, page, editOriginalURL)
-	assert.Contains(t, page, "Bearer "+editBearerSecret)
-	assert.Contains(t, page, `value="7"`)
-	assert.Contains(t, page, "original-name")
-}
-
-// TestHandleTargetEditSubmit_Rejects covers every submission that
-// must not reach storage.
-//
-// The SSRF case is the most important assertion on this change: the
-// edited destination goes through the same guard the create path
-// uses. An edit that stored an unvalidated URL would reopen a closed
-// hole, since a target could then be created public and edited to
-// point at loopback.
-//
-// The header and timeout cases keep input that could not be
-// delivered as written out of storage: a stored value that provably
-// never reaches the wire reports a configuration that did not take
-// effect.
-func TestHandleTargetEditSubmit_Rejects(t *testing.T) {
-	t.Parallel()
-
-	env := setupSourceTest(t)
-
-	t.Run("an SSRF-blocked destination", func(t *testing.T) {
-		t.Parallel()
-		assertEditRejectsBlockedDestination(t, env)
-	})
-
-	t.Run("a query-string destination", func(t *testing.T) {
-		t.Parallel()
-		assertEditIgnoresQueryString(t, env)
-	})
-
-	headerCases := map[string]string{
-		"no colon":        "Authorization Bearer token",
-		"empty name":      ": value",
-		"invalid name":    "X Bad Name: value",
-		"reserved header": "User-Agent: curl/8",
-		"duplicate name":  "X-A: one\nx-a: two",
-	}
-
-	for name, headers := range headerCases {
-		t.Run("headers: "+name, func(t *testing.T) {
-			t.Parallel()
-			assertEditRejectsHeaders(t, env, headers)
-		})
-	}
-
-	timeoutCases := map[string]string{
-		"not a number": "soon",
-		"negative":     "-1",
-		"over ceiling": "100000",
-	}
-
-	for name, timeout := range timeoutCases {
-		t.Run("timeout: "+name, func(t *testing.T) {
-			t.Parallel()
-			assertEditRejectsTimeout(t, env, timeout)
-		})
-	}
-}
-
-func assertEditRejectsBlockedDestination(
-	t *testing.T, env *sourceTestEnv,
-) {
-	t.Helper()
-
-	webhook, target := seedHTTPTarget(t, env, "", "")
-
-	w := submitTargetEdit(
-		env, webhook.ID, target.ID,
-		editForm(editBlockedURL, "", ""),
-	)
-
-	assert.Equal(t, http.StatusBadRequest, w.Code)
-	assert.Contains(t, w.Body.String(), "Invalid target URL")
-	assert.Equal(
-		t, editOriginalURL,
-		storedHTTPConfig(t, env, target.ID).URL,
-		"a rejected edit must leave the stored config alone",
-	)
-}
-
-// The ingress rule the create path already follows applies to the
-// edit path too: reading a field with FormValue would let the request
-// line carry the credential, and the request line is what logs,
-// proxies and Referer headers record.
-func assertEditIgnoresQueryString(
-	t *testing.T, env *sourceTestEnv,
-) {
-	t.Helper()
-
-	webhook, target := seedHTTPTarget(t, env, "", "")
-
-	form := url.Values{}
-	form.Set("name", "edited-name")
-
-	w := serveTarget(
-		env, http.MethodPost,
-		"/source/"+webhook.ID+"/targets/"+target.ID+
-			"/edit?url="+url.QueryEscape(editReplacedURL)+
-			"&headers="+url.QueryEscape(editAuthHeader),
-		form,
-	)
-
-	assert.Equal(t, http.StatusBadRequest, w.Code)
-
-	cfg := storedHTTPConfig(t, env, target.ID)
-	assert.Equal(t, editOriginalURL, cfg.URL)
-	assert.Empty(t, cfg.Headers)
-}
-
-func assertEditRejectsHeaders(
-	t *testing.T, env *sourceTestEnv, headers string,
-) {
-	t.Helper()
-
-	webhook, target := seedHTTPTarget(t, env, "", "")
-
-	w := submitTargetEdit(
-		env, webhook.ID, target.ID,
-		editForm(editOriginalURL, headers, ""),
-	)
-
-	assert.Equal(t, http.StatusBadRequest, w.Code)
-	assert.Contains(t, w.Body.String(), "Invalid headers")
-	assert.Empty(
-		t, storedHTTPConfig(t, env, target.ID).Headers,
-		"a rejected header must not be stored",
-	)
-}
-
-func assertEditRejectsTimeout(
-	t *testing.T, env *sourceTestEnv, timeout string,
-) {
-	t.Helper()
-
-	webhook, target := seedHTTPTarget(t, env, "", "9")
-
-	w := submitTargetEdit(
-		env, webhook.ID, target.ID,
-		editForm(editOriginalURL, "", timeout),
-	)
-
-	assert.Equal(t, http.StatusBadRequest, w.Code)
-	assert.Contains(t, w.Body.String(), "Invalid timeout")
-	assert.Equal(
-		t, 9, storedHTTPConfig(t, env, target.ID).Timeout,
-		"a rejected timeout must leave the stored one alone",
-	)
-}
-
-// TestHandleTargetEdit_Scoping keeps the edit routes scoped the way
-// the delete and toggle routes are: ownership is decided by the
-// webhook, and the target is then scoped to it.
-func TestHandleTargetEdit_Scoping(t *testing.T) {
-	t.Parallel()
-
-	env := setupSourceTest(t)
-
-	t.Run("a target of another webhook", func(t *testing.T) {
-		t.Parallel()
-		assertTargetOfAnotherWebhook404s(t, env)
-	})
-
-	t.Run("a webhook of another user", func(t *testing.T) {
-		t.Parallel()
-		assertWebhookOfAnotherUser404s(t, env)
-	})
-}
-
-// A target id from elsewhere must not become editable by pairing it
-// with a webhook the user does own.
-func assertTargetOfAnotherWebhook404s(
-	t *testing.T, env *sourceTestEnv,
-) {
-	t.Helper()
-
-	mine := seedWebhookWithRetention(t, env.db, 30)
-	_, target := seedHTTPTarget(t, env, "", "")
-
-	get := serveTarget(
-		env, http.MethodGet,
-		"/source/"+mine.ID+"/targets/"+target.ID+"/edit", nil,
-	)
-	assert.Equal(t, http.StatusNotFound, get.Code)
-
-	post := submitTargetEdit(
-		env, mine.ID, target.ID,
-		editForm(editReplacedURL, "", ""),
-	)
-	assert.Equal(t, http.StatusNotFound, post.Code)
-
-	assert.Equal(
-		t, editOriginalURL,
-		storedHTTPConfig(t, env, target.ID).URL,
-	)
-}
-
-func assertWebhookOfAnotherUser404s(
-	t *testing.T, env *sourceTestEnv,
-) {
-	t.Helper()
-
-	other := &database.Webhook{
-		UserID:        "some-other-user",
-		Name:          "not mine",
-		RetentionDays: 30,
-	}
-	require.NoError(
-		t,
-		env.db.DB().Omit(clause.Associations).Create(other).Error,
-	)
-
-	target := seedConfiguredTarget(
-		t, env.db, other.ID, database.TargetTypeHTTP,
-		`{"url":"`+editOriginalURL+`"}`,
-	)
-
-	w := serveTarget(
-		env, http.MethodGet,
-		"/source/"+other.ID+"/targets/"+target.ID+"/edit", nil,
-	)
-
-	assert.Equal(t, http.StatusNotFound, w.Code)
-}

internal/handlers/webhook.go

@@ -217,11 +217,6 @@ func (h *Handlers) createAndDeliverEvent(
 		return
 	}
 
-	// Counted here, after the commit: an event is received once it
-	// is durably stored, which is what the delivery counters are
-	// compared against on a dashboard.
-	h.mtr.EventReceived()
-
 	h.finishWebhookResponse(w, event, entrypoint, tasks)
 }
 

internal/metrics/metrics.go

@@ -1,344 +0,0 @@
-// Package metrics defines the Prometheus collectors describing
-// webhooker's delivery pipeline: how many events arrive, how many
-// deliveries are attempted, how they end, how long they take, how
-// deep the queues are, and how many circuit breakers are open.
-//
-// The inbound HTTP metrics come from the go-http-metrics recorder in
-// internal/middleware and land on prometheus.DefaultRegisterer. These
-// collectors register there too, so both surfaces are gathered by the
-// one promhttp handler mounted on the authenticated /metrics route.
-package metrics
-
-import (
-	"sync"
-	"time"
-
-	"github.com/prometheus/client_golang/prometheus"
-	"github.com/prometheus/client_golang/prometheus/promauto"
-	"sneak.berlin/go/webhooker/internal/database"
-)
-
-// namespace prefixes every collector defined here.
-const namespace = "webhooker"
-
-// targetTypeLabel is the only label any delivery metric carries, and
-// cardinality is the whole reason for that.
-//
-// A target type is one of four compile-time constants, so the label
-// domain is bounded by construction. Target ids, event ids and
-// entrypoint ids are not: they are UUIDs minted per operator action
-// or per inbound request, a series is never reclaimed once it exists,
-// and labelling by any of them makes /metrics a memory leak that
-// grows with traffic. normalizeTargetType enforces the bound at every
-// call site — a type the registry does not know collapses into
-// unknownTargetType rather than minting a series of its own.
-const targetTypeLabel = "target_type"
-
-// unknownTargetType is the bucket for a target type outside the known
-// set, so an unrecognised value cannot mint a new series.
-const unknownTargetType = "unknown"
-
-// Delivery duration buckets, exponential from 5ms so the last bucket
-// (about 98s) sits above the 30s outbound HTTP client timeout.
-const (
-	durationBucketStart  = 0.005
-	durationBucketFactor = 3
-	durationBucketCount  = 10
-)
-
-// knownTargetTypes is the fixed label domain: the target types the
-// delivery engine implements.
-//
-//nolint:gochecknoglobals // the label domain, built once per process
-var knownTargetTypes = []database.TargetType{
-	database.TargetTypeHTTP,
-	database.TargetTypeDatabase,
-	database.TargetTypeLog,
-	database.TargetTypeSlack,
-}
-
-// defaultSet is the process-wide metric set, registered on the same
-// registry the HTTP middleware and the /metrics handler already use.
-// It is built on first use rather than in an init so that a test
-// binary that never touches metrics never registers them.
-//
-//nolint:gochecknoglobals // one process-wide registration, by design
-var defaultSet = sync.OnceValue(func() *Set {
-	return New(prometheus.DefaultRegisterer)
-})
-
-// Default returns the process-wide metric set.
-func Default() *Set {
-	return defaultSet()
-}
-
-// Set is one registered group of webhooker's delivery collectors.
-// Production uses the single Default set; tests build their own
-// against a private registry so assertions are not disturbed by
-// deliveries other tests are making concurrently.
-type Set struct {
-	eventsReceived      prometheus.Counter
-	deliveryAttempts    *prometheus.CounterVec
-	deliveriesSucceeded *prometheus.CounterVec
-	deliveriesFailed    *prometheus.CounterVec
-	deliveryRetries     *prometheus.CounterVec
-	deliveryDuration    *prometheus.HistogramVec
-	deliveriesPending   *prometheus.GaugeVec
-	deliveriesRetrying  *prometheus.GaugeVec
-	circuitBreakersOpen *prometheus.GaugeVec
-}
-
-// New registers a full set of delivery collectors on reg and returns
-// it. It panics if reg already holds them, which is the intended
-// behaviour for a duplicate registration.
-func New(reg prometheus.Registerer) *Set {
-	factory := promauto.With(reg)
-
-	s := &Set{
-		eventsReceived: factory.NewCounter(
-			prometheus.CounterOpts{
-				Namespace: namespace,
-				Name:      "events_received_total",
-				Help: "Webhook events received and " +
-					"stored, so the receive and deliver " +
-					"sides can be compared.",
-			},
-		),
-		deliveryDuration: factory.NewHistogramVec(
-			prometheus.HistogramOpts{
-				Namespace: namespace,
-				Name:      "delivery_duration_seconds",
-				Help: "Wall time of a single delivery " +
-					"attempt, by target type.",
-				Buckets: prometheus.ExponentialBuckets(
-					durationBucketStart,
-					durationBucketFactor,
-					durationBucketCount,
-				),
-			},
-			[]string{targetTypeLabel},
-		),
-	}
-
-	s.registerCounters(factory)
-	s.registerGauges(factory)
-	s.initSeries()
-
-	return s
-}
-
-// EventReceived counts one inbound webhook event stored.
-func (s *Set) EventReceived() {
-	s.eventsReceived.Inc()
-}
-
-// DeliveryAttempted counts one delivery attempt dispatched to a
-// target.
-func (s *Set) DeliveryAttempted(t database.TargetType) {
-	s.deliveryAttempts.
-		WithLabelValues(normalizeTargetType(t)).
-		Inc()
-}
-
-// ObserveDeliveryDuration records how long one delivery attempt took.
-func (s *Set) ObserveDeliveryDuration(
-	t database.TargetType, d time.Duration,
-) {
-	s.deliveryDuration.
-		WithLabelValues(normalizeTargetType(t)).
-		Observe(d.Seconds())
-}
-
-// DeliveryStatusChanged counts a delivery's transition into a new
-// status. The mapping from status to counter lives here, next to the
-// collectors, so the engine has a single call for every transition it
-// persists. A move back to pending is not an outcome and counts
-// nothing.
-func (s *Set) DeliveryStatusChanged(
-	t database.TargetType, status database.DeliveryStatus,
-) {
-	label := normalizeTargetType(t)
-
-	switch status {
-	case database.DeliveryStatusDelivered:
-		s.deliveriesSucceeded.WithLabelValues(label).Inc()
-	case database.DeliveryStatusFailed:
-		s.deliveriesFailed.WithLabelValues(label).Inc()
-	case database.DeliveryStatusRetrying:
-		s.deliveryRetries.WithLabelValues(label).Inc()
-	case database.DeliveryStatusPending:
-	}
-}
-
-// SetQueueDepths publishes the pending and retrying queue depths from
-// one sample. Every label in the queue domain is written on every
-// call, so a type whose queue has drained reads zero instead of
-// holding its last value forever.
-func (s *Set) SetQueueDepths(
-	pending, retrying map[database.TargetType]int,
-) {
-	pendingByLabel := foldToLabels(pending)
-	retryingByLabel := foldToLabels(retrying)
-
-	for _, label := range queueDepthLabels() {
-		s.deliveriesPending.WithLabelValues(label).
-			Set(float64(pendingByLabel[label]))
-		s.deliveriesRetrying.WithLabelValues(label).
-			Set(float64(retryingByLabel[label]))
-	}
-}
-
-// queueDepthLabels is the label domain of the two queue-depth gauges:
-// the known target types plus unknown.
-//
-// Unknown is a real bucket here, not a safety net. A delivery queued
-// against a target that has since been deleted carries a target id no
-// longer in the targets table, so the sample resolves it to the empty
-// type; folding it into unknown is what keeps that backlog visible.
-// Dropping it would hide the one queue nobody is watching.
-func queueDepthLabels() []string {
-	labels := make([]string, 0, len(knownTargetTypes)+1)
-
-	for _, t := range knownTargetTypes {
-		labels = append(labels, string(t))
-	}
-
-	return append(labels, unknownTargetType)
-}
-
-// foldToLabels collapses a per-target-type count onto the bounded
-// label domain, summing everything outside the known set into
-// unknown.
-func foldToLabels(
-	counts map[database.TargetType]int,
-) map[string]int {
-	byLabel := make(map[string]int, len(counts))
-
-	for t, n := range counts {
-		byLabel[normalizeTargetType(t)] += n
-	}
-
-	return byLabel
-}
-
-// SetCircuitBreakersOpen publishes how many of a target type's
-// circuit breakers are currently open.
-func (s *Set) SetCircuitBreakersOpen(
-	t database.TargetType, open int,
-) {
-	s.circuitBreakersOpen.
-		WithLabelValues(normalizeTargetType(t)).
-		Set(float64(open))
-}
-
-func (s *Set) registerCounters(factory promauto.Factory) {
-	s.deliveryAttempts = factory.NewCounterVec(
-		prometheus.CounterOpts{
-			Namespace: namespace,
-			Name:      "delivery_attempts_total",
-			Help: "Delivery attempts dispatched to a " +
-				"target, by target type.",
-		},
-		[]string{targetTypeLabel},
-	)
-
-	s.deliveriesSucceeded = factory.NewCounterVec(
-		prometheus.CounterOpts{
-			Namespace: namespace,
-			Name:      "deliveries_succeeded_total",
-			Help: "Deliveries that reached the delivered " +
-				"state, by target type.",
-		},
-		[]string{targetTypeLabel},
-	)
-
-	s.deliveriesFailed = factory.NewCounterVec(
-		prometheus.CounterOpts{
-			Namespace: namespace,
-			Name:      "deliveries_failed_total",
-			Help: "Deliveries that failed terminally and " +
-				"will not be retried, by target type.",
-		},
-		[]string{targetTypeLabel},
-	)
-
-	s.deliveryRetries = factory.NewCounterVec(
-		prometheus.CounterOpts{
-			Namespace: namespace,
-			Name:      "delivery_retries_total",
-			Help: "Deliveries put back into the retrying " +
-				"state, by target type.",
-		},
-		[]string{targetTypeLabel},
-	)
-}
-
-func (s *Set) registerGauges(factory promauto.Factory) {
-	s.deliveriesPending = factory.NewGaugeVec(
-		prometheus.GaugeOpts{
-			Namespace: namespace,
-			Name:      "deliveries_pending",
-			Help: "Deliveries currently in the pending " +
-				"state, by target type.",
-		},
-		[]string{targetTypeLabel},
-	)
-
-	s.deliveriesRetrying = factory.NewGaugeVec(
-		prometheus.GaugeOpts{
-			Namespace: namespace,
-			Name:      "deliveries_retrying",
-			Help: "Deliveries currently in the retrying " +
-				"state, by target type.",
-		},
-		[]string{targetTypeLabel},
-	)
-
-	s.circuitBreakersOpen = factory.NewGaugeVec(
-		prometheus.GaugeOpts{
-			Namespace: namespace,
-			Name:      "circuit_breakers_open",
-			Help: "Delivery circuit breakers currently " +
-				"open, by target type.",
-		},
-		[]string{targetTypeLabel},
-	)
-}
-
-// initSeries materialises every known-target-type series at zero, so
-// a dashboard and an alert rule see a target type that has not
-// delivered yet rather than a missing series.
-//
-// The queue-depth gauges additionally get their unknown series, which
-// holds deliveries queued against a deleted target. That backlog can
-// predate the process — it is read out of the databases, not counted
-// from transitions — so its series has to exist from the first scrape
-// rather than appearing only once a backlog has already built up.
-func (s *Set) initSeries() {
-	for _, t := range knownTargetTypes {
-		label := string(t)
-
-		s.deliveryAttempts.WithLabelValues(label)
-		s.deliveriesSucceeded.WithLabelValues(label)
-		s.deliveriesFailed.WithLabelValues(label)
-		s.deliveryRetries.WithLabelValues(label)
-		s.deliveriesPending.WithLabelValues(label)
-		s.deliveriesRetrying.WithLabelValues(label)
-		s.circuitBreakersOpen.WithLabelValues(label)
-	}
-
-	s.deliveriesPending.WithLabelValues(unknownTargetType)
-	s.deliveriesRetrying.WithLabelValues(unknownTargetType)
-}
-
-// normalizeTargetType maps a target type onto the bounded label
-// domain, collapsing anything outside it to unknownTargetType.
-func normalizeTargetType(t database.TargetType) string {
-	for _, known := range knownTargetTypes {
-		if t == known {
-			return string(known)
-		}
-	}
-
-	return unknownTargetType
-}

internal/metrics/metrics_test.go

@@ -1,285 +0,0 @@
-package metrics_test
-
-import (
-	"testing"
-
-	"github.com/prometheus/client_golang/prometheus"
-	dto "github.com/prometheus/client_model/go"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/webhooker/internal/database"
-	"sneak.berlin/go/webhooker/internal/metrics"
-)
-
-// knownLabels is the target_type label domain built from the target
-// types the delivery engine implements.
-func knownLabels() []string {
-	return []string{"http", "database", "log", "slack"}
-}
-
-// labelValues returns the target_type label values a metric family
-// currently carries.
-func labelValues(
-	t *testing.T, reg *prometheus.Registry, name string,
-) []string {
-	t.Helper()
-
-	families, err := reg.Gather()
-	require.NoError(t, err)
-
-	var values []string
-
-	for _, fam := range families {
-		if fam.GetName() != name {
-			continue
-		}
-
-		for _, m := range fam.GetMetric() {
-			for _, label := range m.GetLabel() {
-				if label.GetName() == "target_type" {
-					values = append(
-						values, label.GetValue(),
-					)
-				}
-			}
-		}
-	}
-
-	return values
-}
-
-func gaugeValue(
-	t *testing.T,
-	reg *prometheus.Registry,
-	name, targetType string,
-) float64 {
-	t.Helper()
-
-	families, err := reg.Gather()
-	require.NoError(t, err)
-
-	for _, fam := range families {
-		if fam.GetName() != name {
-			continue
-		}
-
-		for _, m := range fam.GetMetric() {
-			if hasTargetType(m, targetType) {
-				return m.GetGauge().GetValue()
-			}
-		}
-	}
-
-	t.Fatalf(
-		"gauge %s{target_type=%q} not found",
-		name, targetType,
-	)
-
-	return 0
-}
-
-func hasTargetType(m *dto.Metric, targetType string) bool {
-	for _, label := range m.GetLabel() {
-		if label.GetName() == "target_type" &&
-			label.GetValue() == targetType {
-			return true
-		}
-	}
-
-	return false
-}
-
-// TestUnknownTargetTypeCollapses is the cardinality guard: a target
-// type outside the known set must not mint a series of its own, or
-// /metrics grows without bound.
-func TestUnknownTargetTypeCollapses(t *testing.T) {
-	t.Parallel()
-
-	reg := prometheus.NewRegistry()
-	set := metrics.New(reg)
-
-	for _, bogus := range []string{
-		"a1b2c3d4-0000-0000-0000-000000000001",
-		"a1b2c3d4-0000-0000-0000-000000000002",
-		"webhook-forwarder",
-	} {
-		set.DeliveryAttempted(database.TargetType(bogus))
-	}
-
-	values := labelValues(
-		t, reg, "webhooker_delivery_attempts_total",
-	)
-
-	assert.ElementsMatch(t,
-		append(knownLabels(), "unknown"),
-		values,
-	)
-}
-
-// TestSetQueueDepthsZeroesDrainedTypes proves a queue that has
-// drained reads zero rather than holding its last sample forever.
-func TestSetQueueDepthsZeroesDrainedTypes(t *testing.T) {
-	t.Parallel()
-
-	reg := prometheus.NewRegistry()
-	set := metrics.New(reg)
-
-	set.SetQueueDepths(
-		map[database.TargetType]int{
-			database.TargetTypeHTTP: 7,
-		},
-		map[database.TargetType]int{
-			database.TargetTypeSlack: 2,
-		},
-	)
-
-	assert.InDelta(t, 7.0, gaugeValue(
-		t, reg, "webhooker_deliveries_pending", "http",
-	), 0)
-	assert.InDelta(t, 2.0, gaugeValue(
-		t, reg, "webhooker_deliveries_retrying", "slack",
-	), 0)
-
-	set.SetQueueDepths(
-		map[database.TargetType]int{},
-		map[database.TargetType]int{},
-	)
-
-	assert.InDelta(t, 0.0, gaugeValue(
-		t, reg, "webhooker_deliveries_pending", "http",
-	), 0)
-	assert.InDelta(t, 0.0, gaugeValue(
-		t, reg, "webhooker_deliveries_retrying", "slack",
-	), 0)
-}
-
-// TestKnownSeriesExistBeforeAnyDelivery proves every known target
-// type is published at zero from registration, so an alert rule does
-// not have to cope with a missing series.
-func TestKnownSeriesExistBeforeAnyDelivery(t *testing.T) {
-	t.Parallel()
-
-	reg := prometheus.NewRegistry()
-	metrics.New(reg)
-
-	for _, name := range []string{
-		"webhooker_delivery_attempts_total",
-		"webhooker_deliveries_succeeded_total",
-		"webhooker_deliveries_failed_total",
-		"webhooker_delivery_retries_total",
-		"webhooker_circuit_breakers_open",
-	} {
-		assert.ElementsMatch(t,
-			knownLabels(),
-			labelValues(t, reg, name),
-			"metric %s", name,
-		)
-	}
-
-	// The queue gauges additionally publish unknown from
-	// registration: a backlog queued against a deleted target lands
-	// there, and it can predate the process, so the series has to
-	// exist before the first sample rather than appearing only once
-	// something is already stuck.
-	for _, name := range []string{
-		"webhooker_deliveries_pending",
-		"webhooker_deliveries_retrying",
-	} {
-		assert.ElementsMatch(t,
-			append(knownLabels(), "unknown"),
-			labelValues(t, reg, name),
-			"metric %s", name,
-		)
-	}
-}
-
-// TestSetQueueDepthsFoldsUnknownTypes proves a queued delivery whose
-// target type is not a known one — a target deleted out from under it
-// resolves to the empty type — is summed into the unknown series
-// instead of being dropped, and that the fold is a sum rather than a
-// last-writer-wins.
-func TestSetQueueDepthsFoldsUnknownTypes(t *testing.T) {
-	t.Parallel()
-
-	reg := prometheus.NewRegistry()
-	set := metrics.New(reg)
-
-	set.SetQueueDepths(
-		map[database.TargetType]int{
-			database.TargetTypeHTTP:             1,
-			database.TargetType(""):             4,
-			database.TargetType("retired-type"): 3,
-		},
-		map[database.TargetType]int{
-			database.TargetType(""): 2,
-		},
-	)
-
-	assert.InDelta(t, 7.0, gaugeValue(
-		t, reg, "webhooker_deliveries_pending", "unknown",
-	), 0)
-	assert.InDelta(t, 2.0, gaugeValue(
-		t, reg, "webhooker_deliveries_retrying", "unknown",
-	), 0)
-	assert.InDelta(t, 1.0, gaugeValue(
-		t, reg, "webhooker_deliveries_pending", "http",
-	), 0)
-
-	set.SetQueueDepths(
-		map[database.TargetType]int{},
-		map[database.TargetType]int{},
-	)
-
-	assert.InDelta(t, 0.0, gaugeValue(
-		t, reg, "webhooker_deliveries_pending", "unknown",
-	), 0)
-}
-
-// TestDeliveryStatusChangedCounts maps each persisted status onto the
-// counter it moves.
-func TestDeliveryStatusChangedCounts(t *testing.T) {
-	t.Parallel()
-
-	reg := prometheus.NewRegistry()
-	set := metrics.New(reg)
-
-	set.DeliveryStatusChanged(
-		database.TargetTypeLog,
-		database.DeliveryStatusDelivered,
-	)
-	set.DeliveryStatusChanged(
-		database.TargetTypeLog,
-		database.DeliveryStatusFailed,
-	)
-	set.DeliveryStatusChanged(
-		database.TargetTypeLog,
-		database.DeliveryStatusRetrying,
-	)
-	set.DeliveryStatusChanged(
-		database.TargetTypeLog,
-		database.DeliveryStatusPending,
-	)
-
-	families, err := reg.Gather()
-	require.NoError(t, err)
-
-	counts := map[string]float64{}
-
-	for _, fam := range families {
-		for _, m := range fam.GetMetric() {
-			if hasTargetType(m, "log") {
-				counts[fam.GetName()] =
-					m.GetCounter().GetValue()
-			}
-		}
-	}
-
-	assert.InDelta(t, 1.0,
-		counts["webhooker_deliveries_succeeded_total"], 0)
-	assert.InDelta(t, 1.0,
-		counts["webhooker_deliveries_failed_total"], 0)
-	assert.InDelta(t, 1.0,
-		counts["webhooker_delivery_retries_total"], 0)
-	assert.InDelta(t, 0.0,
-		counts["webhooker_delivery_attempts_total"], 0)
-}

internal/server/routes.go

@@ -214,20 +214,6 @@ func (s *Server) setupSourceRoutes() {
 			s.h.HandleEntrypointToggle(),
 		)
 		r.Post("/targets", s.h.HandleTargetCreate())
-		// The edit form is the one page that renders a target's
-		// destination URL and header values in full; see
-		// delivery.TargetConfigForm. It belongs to this group for
-		// its RequireAuth and NoCache, which are what keep that
-		// exception from reaching an unauthenticated request or a
-		// shared cache.
-		r.Get(
-			"/targets/{targetID}/edit",
-			s.h.HandleTargetEdit(),
-		)
-		r.Post(
-			"/targets/{targetID}/edit",
-			s.h.HandleTargetEditSubmit(),
-		)
 		r.Post(
 			"/targets/{targetID}/delete",
 			s.h.HandleTargetDelete(),

templates/source_detail.html

@@ -110,14 +110,6 @@
                     <div x-show="targetType === 'http'">
                         <input type="url" name="url" placeholder="https://example.com/webhook" :disabled="targetType !== 'http'" class="input text-sm">
                     </div>
-                    <div x-show="targetType === 'http'">
-                        <textarea name="headers" rows="3" placeholder="Authorization: Bearer ..." :disabled="targetType !== 'http'" class="input text-sm"></textarea>
-                        <p class="text-xs text-gray-500 mt-1">Optional request headers, one <code>Name: value</code> per line, sent with every delivery.</p>
-                    </div>
-                    <div x-show="targetType === 'http'" class="flex gap-2 items-center">
-                        <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">
                         <label class="text-sm text-gray-700">Max retries (0 = fire-and-forget):</label>
                         <input type="number" name="max_retries" value="0" min="0" max="20" class="input text-sm w-24">
@@ -146,7 +138,6 @@
                             {{else}}
                             <span class="badge-error">Inactive</span>
                             {{end}}
-                            <a href="/source/{{$.Webhook.ID}}/targets/{{.ID}}/edit" class="text-xs text-gray-500 hover:text-primary-600" title="Edit">Edit</a>
                             <form method="POST" action="/source/{{$.Webhook.ID}}/targets/{{.ID}}/toggle" class="inline">
                                 <input type="hidden" name="csrf_token" value="{{$.CSRFToken}}">
                                 <button type="submit" class="text-xs text-gray-500 hover:text-primary-600" title="{{if .Active}}Deactivate{{else}}Activate{{end}}">

templates/source_logs.html

@@ -62,7 +62,7 @@
 
                                 <div x-show="attempts" x-cloak class="mt-2 space-y-2">
                                     {{if .AttemptsOmitted}}
-                                    <p class="text-xs text-gray-500">{{.AttemptsOmitted}} attempt{{if ne .AttemptsOmitted 1}}s{{end}} omitted between the first and last shown.</p>
+                                    <p class="text-xs text-gray-500">{{.AttemptsOmitted}} attempts omitted between the first and last shown.</p>
                                     {{end}}
                                     {{range .Results}}
                                     <div class="rounded-md bg-white border border-gray-200 p-2">
@@ -79,11 +79,7 @@
                                         <pre class="mt-2 text-xs text-gray-700 overflow-x-auto whitespace-pre-wrap break-all">{{.ResponseBody}}</pre>
                                         {{end}}
                                         {{if .ResponseTruncated}}
-                                        {{if .ResponseSizeKnown}}
                                         <p class="mt-1 text-xs text-gray-500">Response truncated for display: showing {{.ResponseShownBytes}} of {{.ResponseBytes}} bytes.</p>
-                                        {{else}}
-                                        <p class="mt-1 text-xs text-gray-500">Showing {{.ResponseShownBytes}} of the {{.ResponseBytes}} recorded bytes. The response reached the recording limit, so the remote may have sent more that was never stored.</p>
-                                        {{end}}
                                         {{end}}
                                     </div>
                                     {{else}}

templates/target_edit.html

@@ -1,83 +0,0 @@
-{{template "base" .}}
-
-{{define "title"}}Edit {{.Target.Name}} - Webhooker{{end}}
-
-{{define "content"}}
-<div class="max-w-2xl mx-auto px-6 py-8">
-    <div class="mb-6">
-        <a href="/source/{{.Webhook.ID}}" class="text-sm text-primary-600 hover:text-primary-700">&larr; Back to {{.Webhook.Name}}</a>
-        <h1 class="text-2xl font-medium text-gray-900 mt-2">Edit Target</h1>
-        <p class="text-sm text-gray-500 mt-1">Type: {{.Target.Type}}. A target's type cannot be changed; create a new target to deliver a different way.</p>
-    </div>
-
-    <div class="card p-6">
-        {{if .Error}}
-        <div class="alert-error">{{.Error}}</div>
-        {{end}}
-
-        {{if or (eq .Target.Type "http") (eq .Target.Type "slack")}}
-        <div class="mb-6 rounded-md bg-gray-50 p-4 text-sm text-gray-700">
-            This form shows the target's stored destination in full, including any credential carried in its URL or headers. It is the only page that does; everywhere else the value is masked.
-        </div>
-        {{end}}
-
-        <form method="POST" action="/source/{{.Webhook.ID}}/targets/{{.Target.ID}}/edit" class="space-y-6">
-            <input type="hidden" name="csrf_token" value="{{.CSRFToken}}">
-
-            <div class="form-group">
-                <label for="name" class="label">Name</label>
-                <input type="text" id="name" name="name" value="{{.Target.Name}}" required class="input">
-            </div>
-
-            {{if eq .Target.Type "http"}}
-            <div class="form-group">
-                <label for="url" class="label">Destination URL</label>
-                <input type="url" id="url" name="url" value="{{.Target.Config.URL}}" required class="input">
-                <p class="text-xs text-gray-500 mt-1">Revalidated on save; destinations that resolve to private or link-local addresses are rejected.</p>
-            </div>
-
-            <div class="form-group">
-                <label for="headers" class="label">Headers</label>
-                <textarea id="headers" name="headers" rows="4" class="input" placeholder="Authorization: Bearer ...">{{.Target.Config.Headers}}</textarea>
-                <p class="text-xs text-gray-500 mt-1">One <code>Name: value</code> per line, sent with every delivery. Leave blank for none. <code>Host</code>, <code>Content-Length</code>, <code>Transfer-Encoding</code>, <code>Connection</code> and <code>User-Agent</code> are set by the delivery engine and are rejected here rather than silently ignored.</p>
-            </div>
-
-            <div class="form-group">
-                <label for="timeout" class="label">Timeout (seconds)</label>
-                <input type="number" id="timeout" name="timeout" value="{{.Target.Config.Timeout}}" min="0" max="{{.MaxTimeout}}" class="input">
-                <p class="text-xs text-gray-500 mt-1">Per-request timeout, at most {{.MaxTimeout}} seconds. Leave blank to use the default.</p>
-            </div>
-            {{end}}
-
-            {{if eq .Target.Type "slack"}}
-            <div class="form-group">
-                <label for="url" class="label">Webhook URL</label>
-                <input type="url" id="url" name="url" value="{{.Target.Config.URL}}" required class="input">
-                <p class="text-xs text-gray-500 mt-1">Slack or Mattermost incoming webhook URL. Revalidated on save.</p>
-            </div>
-            {{end}}
-
-            {{if eq .Target.Type "database"}}
-            <div class="form-group">
-                <label for="expiry" class="label">Archive Expiry</label>
-                <input type="text" id="expiry" name="expiry" value="{{.Target.Config.Expiry}}" placeholder="never" class="input">
-                <p class="text-xs text-gray-500 mt-1">"never" (the default when blank) keeps archived rows forever, or a Go duration like "720h" prunes older rows.</p>
-            </div>
-            {{end}}
-
-            {{if or (eq .Target.Type "http") (eq .Target.Type "slack")}}
-            <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>
-            </div>
-            {{end}}
-
-            <div class="flex gap-3">
-                <button type="submit" class="btn-primary">Save Changes</button>
-                <a href="/source/{{.Webhook.ID}}" class="btn-secondary">Cancel</a>
-            </div>
-        </form>
-    </div>
-</div>
-{{end}}