Expose delivery metrics on /metrics (closes #209)

/metrics carried only the inbound HTTP surface, so a destination
failing for an hour, a growing retry backlog and a stuck-open circuit
breaker were all invisible: the receive side stays healthy in each
case because it is.

New internal/metrics registers, on the existing default registry that
the go-http-metrics recorder and the promhttp handler already share:

- webhooker_events_received_total
- webhooker_delivery_attempts_total
- webhooker_deliveries_succeeded_total
- webhooker_deliveries_failed_total
- webhooker_delivery_retries_total
- webhooker_delivery_duration_seconds
- webhooker_deliveries_pending / _retrying
- webhooker_circuit_breakers_open

The route mounting is untouched.

Every delivery metric carries one label, target_type, whose domain is
the four target-type constants; anything outside it collapses to
"unknown" so no series can be minted from a UUID. Target ids, event
ids and entrypoint ids are deliberately not labels.

Instrumentation sits at the points every target type already passes
through: processDelivery for the attempt counter and the duration
histogram, updateDeliveryStatus for the outcome counters. The
queue-depth gauges are counted out of the per-webhook databases by a
30s sampler rather than tracked as deltas, which would need seeding at
startup and would drift on any transition that failed to persist. The
open-breaker gauge is recounted from the target's breaker registry on
every state change.

README.md

@@ -107,26 +107,14 @@ TTY detection, and security headers are always applied.
 | `DATA_DIR`              | Directory for all SQLite databases  | `/var/lib/webhooker` |
 | `DEBUG`                 | Enable debug logging                | `false`  |
 | `MAINTENANCE_MODE`      | Report `maintenanceMode: true` in the healthcheck JSON. It does not change how any request is served — no maintenance page exists | `false`  |
-| `METRICS_USERNAME`      | Basic auth username for `/metrics`. Must be set together with `METRICS_PASSWORD`; one without the other fails startup | `""`     |
+| `METRICS_USERNAME`      | Basic auth username for `/metrics`  | `""`     |
-| `METRICS_PASSWORD`      | Basic auth password for `/metrics`. Must be set together with `METRICS_USERNAME`; one without the other fails startup | `""`     |
+| `METRICS_PASSWORD`      | Basic auth password for `/metrics`  | `""`     |
 | `SENTRY_DSN`            | Sentry error reporting DSN          | `""`     |
 | `RETENTION_SWEEP_INTERVAL` | How often the retention reaper and archive sweeper run (Go duration, must be positive) | `1h` |
 | `SESSION_IDLE_TIMEOUT`  | Idle session timeout (Go duration)  | `24h`    |
 | `RECEIVER_RATE_LIMIT`   | Receiver requests/minute per IP per entrypoint (10x that per IP across the route) | `120` |
 | `TRUSTED_PROXIES`       | CIDRs whose forwarded headers are trusted (unset: all clients behind a proxy share one rate-limit bucket; a correct login password is never throttled either way) | `""` (none) |
 
-#### Metrics credentials
-
-`METRICS_USERNAME` and `METRICS_PASSWORD` are set together or not at
-all. With both set, `/metrics` is served behind basic auth. With
-neither set, the route is not registered and returns 404. With one set
-and the other empty or unset, the process refuses to start and exits
-non-zero with an error naming both variables — mounting the endpoint
-on the username alone would publish it behind a password that is the
-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.
-
 #### Trusted proxies
 
 `TRUSTED_PROXIES` is a comma-separated list of CIDR blocks (a bare
@@ -1146,11 +1134,11 @@ arriving and being stored, they are just not getting anywhere.
 | Metric | Type | Meaning |
 | ------ | ---- | ------- |
 | `webhooker_events_received_total` | counter | Events received and durably stored. Compare against the delivery counters on one dashboard |
-| `webhooker_delivery_attempts_total` | counter | Delivery attempts actually dispatched to a target. A delivery an open circuit breaker refused is not one: it is counted as a retry instead |
+| `webhooker_delivery_attempts_total` | counter | Delivery attempts dispatched to a target |
 | `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_delivery_duration_seconds` | histogram | Wall time of a single delivery attempt |
 | `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 |
@@ -1172,14 +1160,6 @@ 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
@@ -1740,7 +1720,7 @@ abuse limit later; they are tracked as future work.
 
 | Method | Path       | Description |
 | ------ | ---------- | ----------- |
-| `GET`  | `/metrics` | Prometheus metrics, behind basic auth. The route is registered only when `METRICS_USERNAME` and `METRICS_PASSWORD` are both set; with neither set it does not exist and returns 404, and with only one set the process refuses to start |
+| `GET`  | `/metrics` | Prometheus metrics, behind basic auth. The route is registered only when `METRICS_USERNAME` is set; otherwise it does not exist and returns 404 |
 
 #### API (Planned)
 
@@ -1898,8 +1878,7 @@ Applied to all routes in this order:
    Permissions-Policy)
 3. **Logging** — Structured request logging (method, URL, status,
    latency, remote IP, user agent, request ID)
-4. **Metrics** — Prometheus HTTP metrics (if `METRICS_USERNAME` and
+4. **Metrics** — Prometheus HTTP metrics (if `METRICS_USERNAME` is set)
-   `METRICS_PASSWORD` are both set)
 5. **CORS** — Cross-origin resource sharing headers
 6. **Timeout** — 60-second request timeout
 7. **Recoverer** — Panic recovery: one `ERROR` record through
@@ -1931,9 +1910,8 @@ being read and without reaching CSRF, the route group's remaining
 middleware, or the handler. It is not rejected before *any* other
 middleware, though: the global entries listed above all run first, so
 such a request is still logged and given the security headers — and
-counted in the metrics, on a deployment where the `/metrics`
+counted in the metrics, on a deployment where `METRICS_USERNAME` is
-credentials are set and the Metrics middleware is therefore registered
+set and the Metrics middleware is therefore registered at all. The
-at all. The
 rejection itself is logged at `WARN` with the method, path and
 declared length. A chunked request, or
 one that lies about its length, is hard-capped by

internal/config/config.go

@@ -71,16 +71,6 @@ var ErrInvalidPort = errors.New("invalid port")
 // nor a bare IP address.
 var ErrInvalidCIDR = errors.New("invalid CIDR")
 
-// ErrIncompleteMetricsAuth is returned when exactly one of
-// METRICS_USERNAME and METRICS_PASSWORD carries a value. Neither
-// fallback is acceptable: serving /metrics on the username alone
-// publishes an endpoint whose password is the empty string, and
-// silently leaving it unmounted withholds an endpoint the operator
-// asked for. Half-set is a configuration error, so startup fails.
-var ErrIncompleteMetricsAuth = errors.New(
-	"incomplete metrics credentials",
-)
-
 //nolint:revive // ConfigParams is a standard fx naming convention.
 type ConfigParams struct {
 	fx.In
@@ -138,21 +128,6 @@ func (c *Config) IsProd() bool {
 	return c.Environment == EnvironmentProd
 }
 
-// MetricsAuthEnabled reports whether /metrics is served behind basic
-// auth. It is the only answer to that question in the codebase: the
-// route mount, the Prometheus recording middleware and the startup
-// log's hasMetricsAuth field all read this one method, so the log
-// cannot report auth as off while the route is mounted.
-//
-// It requires both credentials rather than the username alone.
-// loadFromEnv already rejects a half-set pair, but a Config built in
-// code bypasses that, and the failure mode this guards is an endpoint
-// mounted with a credential map whose only password is the empty
-// string.
-func (c *Config) MetricsAuthEnabled() bool {
-	return c.MetricsUsername != "" && c.MetricsPassword != ""
-}
-
 // envString returns the value of the named environment variable,
 // or an empty string if not set.
 func envString(key string) string {
@@ -354,30 +329,6 @@ func envPrefixList(key string) ([]netip.Prefix, error) {
 	return prefixes, nil
 }
 
-// resolveMetricsAuth reads the /metrics basic-auth credentials and
-// rejects a half-set pair, naming both variables either way. The
-// error carries neither value: the password is a secret.
-func resolveMetricsAuth() (string, string, error) {
-	username := envString("METRICS_USERNAME")
-	password := envString("METRICS_PASSWORD")
-
-	if (username == "") == (password == "") {
-		return username, password, nil
-	}
-
-	set, empty := "METRICS_USERNAME", "METRICS_PASSWORD"
-	if username == "" {
-		set, empty = empty, set
-	}
-
-	return "", "", fmt.Errorf(
-		"%w: %s is set but %s is empty; METRICS_USERNAME and "+
-			"METRICS_PASSWORD must both be set to serve /metrics, "+
-			"or both be empty to leave it unmounted",
-		ErrIncompleteMetricsAuth, set, empty,
-	)
-}
-
 // resolveEnvironment reads WEBHOOKER_ENVIRONMENT, defaulting to
 // dev, and rejects unrecognised values.
 func resolveEnvironment() (string, error) {
@@ -455,18 +406,13 @@ func loadFromEnv() (*Config, error) {
 		return nil, err
 	}
 
-	metricsUsername, metricsPassword, err := resolveMetricsAuth()
-	if err != nil {
-		return nil, err
-	}
-
 	return &Config{
 		DataDir:                envString("DATA_DIR"),
 		Debug:                  debug,
 		MaintenanceMode:        maintenanceMode,
 		Environment:            environment,
-		MetricsUsername:        metricsUsername,
+		MetricsUsername:        envString("METRICS_USERNAME"),
-		MetricsPassword:        metricsPassword,
+		MetricsPassword:        envString("METRICS_PASSWORD"),
 		Port:                   port,
 		SentryDSN:              envString("SENTRY_DSN"),
 		RetentionSweepInterval: retentionSweepInterval,
@@ -566,7 +512,8 @@ func New(lc fx.Lifecycle, params ConfigParams) (*Config, error) {
 		"receiverRateLimit", s.ReceiverRateLimit,
 		"trustedProxies", len(s.TrustedProxies),
 		"hasSentryDSN", s.SentryDSN != "",
-		"hasMetricsAuth", s.MetricsAuthEnabled(),
+		"hasMetricsAuth",
+		s.MetricsUsername != "" && s.MetricsPassword != "",
 	)
 
 	s.warnSharedRateLimitBucket(log)

internal/config/config_test.go

@@ -26,12 +26,6 @@ const (
 	// cidrPrivateV4 is the sample trusted-proxy block the
 	// TRUSTED_PROXIES cases are built from.
 	cidrPrivateV4 = "10.0.0.0/8"
-
-	// metricsAuthValue is the sample METRICS_PASSWORD the metrics
-	// credential cases are built from. It is asserted absent from
-	// the startup error, so it must not be a substring of either
-	// variable name that error prints.
-	metricsAuthValue = "s3cret"
 )
 
 func TestEnvironmentConfig(t *testing.T) {
@@ -732,168 +726,3 @@ func TestSharedRateLimitBucketWarning(t *testing.T) {
 		})
 	}
 }
-
-// metricsEnv describes what one subtest below puts in the
-// environment for a single METRICS_ variable. A variable that is
-// set to the empty string and one that is not set at all are
-// distinct inputs here, because the reported bug arrived through
-// the first of them.
-type metricsEnv struct {
-	set   bool
-	value string
-}
-
-// unset leaves the variable out of the environment entirely.
-func unset() metricsEnv {
-	return metricsEnv{set: false, value: ""}
-}
-
-// setTo sets the variable, including to the empty string.
-func setTo(value string) metricsEnv {
-	return metricsEnv{set: true, value: value}
-}
-
-// metricsAuthCase is one row of the table in TestMetricsAuthConfig,
-// named so the table can live in its own function and keep the test
-// itself short.
-type metricsAuthCase struct {
-	name        string
-	username    metricsEnv
-	password    metricsEnv
-	expectError bool
-	expectAuth  bool
-}
-
-// metricsAuthCases enumerates every combination of the two
-// credentials, counting "set to the empty string" and "not set at
-// all" as separate inputs on each side.
-func metricsAuthCases() []metricsAuthCase {
-	return []metricsAuthCase{
-		{
-			name:     "both unset leaves metrics unmounted",
-			username: unset(),
-			password: unset(),
-		},
-		{
-			name:     "both empty leaves metrics unmounted",
-			username: setTo(""),
-			password: setTo(""),
-		},
-		{
-			name:       "both set enables metrics auth",
-			username:   setTo("metrics"),
-			password:   setTo(metricsAuthValue),
-			expectAuth: true,
-		},
-		{
-			name:        "username with unset password fails",
-			username:    setTo("metrics"),
-			password:    unset(),
-			expectError: true,
-		},
-		{
-			name:        "username with empty password fails",
-			username:    setTo("metrics"),
-			password:    setTo(""),
-			expectError: true,
-		},
-		{
-			name:        "password with unset username fails",
-			username:    unset(),
-			password:    setTo(metricsAuthValue),
-			expectError: true,
-		},
-		{
-			name:        "password with empty username fails",
-			username:    setTo(""),
-			password:    setTo(metricsAuthValue),
-			expectError: true,
-		},
-	}
-}
-
-// TestMetricsAuthConfig covers every combination of METRICS_USERNAME
-// and METRICS_PASSWORD. Either both carry a value, in which case
-// /metrics is served behind basic auth, or neither does, in which
-// case the route is never mounted. One without the other is a
-// startup error rather than a fallback: mounting on the username
-// alone published /metrics behind a credential map that accepted an
-// empty password, which is the defect this test exists to pin. See
-// https://git.eeqj.de/sneak/webhooker/issues/205.
-func TestMetricsAuthConfig(t *testing.T) {
-	for _, tt := range metricsAuthCases() {
-		t.Run(tt.name, func(t *testing.T) {
-			// Cannot use t.Parallel() here because t.Setenv
-			// is incompatible with parallel subtests.
-			if tt.username.set {
-				t.Setenv("METRICS_USERNAME", tt.username.value)
-			} else {
-				require.NoError(
-					t, os.Unsetenv("METRICS_USERNAME"),
-				)
-			}
-
-			if tt.password.set {
-				t.Setenv("METRICS_PASSWORD", tt.password.value)
-			} else {
-				require.NoError(
-					t, os.Unsetenv("METRICS_PASSWORD"),
-				)
-			}
-
-			if tt.expectError {
-				assertMetricsAuthRejected(t)
-
-				return
-			}
-
-			assertMetricsAuthAccepted(t, tt.expectAuth)
-		})
-	}
-}
-
-// assertMetricsAuthRejected requires that fx refused to build the
-// graph, that the failure is ErrIncompleteMetricsAuth, and that the
-// operator is told both variable names — the point of failing here
-// rather than degrading is that the message says what to fix.
-func assertMetricsAuthRejected(t *testing.T) {
-	t.Helper()
-
-	var cfg *config.Config
-
-	app := fx.New(
-		fx.NopLogger,
-		fx.Provide(globals.New, logger.New, config.New),
-		fx.Populate(&cfg),
-	)
-
-	err := app.Err()
-	require.Error(t, err)
-	require.ErrorIs(t, err, config.ErrIncompleteMetricsAuth)
-	assert.Contains(t, err.Error(), "METRICS_USERNAME")
-	assert.Contains(t, err.Error(), "METRICS_PASSWORD")
-	// The password is a secret and must not reach a startup error.
-	assert.NotContains(t, err.Error(), metricsAuthValue)
-}
-
-// assertMetricsAuthAccepted requires that startup succeeded and that
-// MetricsAuthEnabled — the single value the /metrics mount and the
-// startup log both read — reports what the environment asked for.
-func assertMetricsAuthAccepted(t *testing.T, expectAuth bool) {
-	t.Helper()
-
-	var cfg *config.Config
-
-	app := fxtest.New(
-		t,
-		fx.Provide(globals.New, logger.New, config.New),
-		fx.Populate(&cfg),
-	)
-	require.NoError(t, app.Err())
-
-	app.RequireStart()
-
-	defer app.RequireStop()
-
-	assert.Equal(t, expectAuth, cfg.MetricsAuthEnabled())
-}

internal/delivery/engine.go

@@ -140,11 +140,11 @@ type Engine struct {
 	retryCh    chan Task
 	workers    int
 
-	// mtr is the delivery metric set. Production wires the
+	// mx 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
+	mx *metrics.Set
 
 	// targets maps each target type to its implementation.
 	targets map[database.TargetType]Target
@@ -171,7 +171,7 @@ func New(
 		deliveryCh: make(chan Task, deliveryChannelSize),
 		retryCh:    make(chan Task, retryChannelSize),
 		workers:    defaultWorkers,
-		mtr:        metrics.Default(),
+		mx:         metrics.Default(),
 	}
 
 	e.initTargets(&http.Client{
@@ -838,6 +838,11 @@ func (e *Engine) failUnretryableRetry(
 		target.Type,
 	)
 
+	// The delivery was loaded without its target relation, so
+	// attach it: updateDeliveryStatus reads the type to label the
+	// terminal failure it is about to count.
+	d.Target = *target
+
 	e.recordResult(
 		webhookDB,
 		d,
@@ -849,26 +854,28 @@ 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,
 	)
 }
 
 // processDelivery dispatches a delivery to the target that
 // owns its type. Unknown target types fail the delivery.
+//
+// It is also where the attempt counter and the duration histogram
+// are recorded, because it is the one point every target type
+// passes through on every attempt: a target added later is
+// instrumented without touching it, and the duration measured is
+// the whole cost of the attempt rather than whatever each target
+// happens to time for itself.
 func (e *Engine) processDelivery(
 	ctx context.Context,
 	webhookDB *gorm.DB,
 	d *database.Delivery,
 	task *Task,
 ) {
+	e.mx.DeliveryAttempted(d.Target.Type)
+
 	target, ok := e.targets[d.Target.Type]
 	if !ok {
 		e.log.Error(
@@ -878,32 +885,19 @@ func (e *Engine) processDelivery(
 		)
 
 		e.updateDeliveryStatus(
-			webhookDB, d, d.Target.Type,
+			webhookDB, d, database.DeliveryStatusFailed,
-			database.DeliveryStatusFailed,
 		)
 
 		return
 	}
 
-	target.Deliver(ctx, webhookDB, d, task, e)
+	start := time.Now()
-}
 
-// observeAttempt counts one delivery attempt that was actually
+	target.Deliver(ctx, webhookDB, d, task, e)
-// dispatched to a target, and records how long it took.
+
-//
+	e.mx.ObserveDeliveryDuration(
-// It is called from the dispatch paths rather than from around
+		d.Target.Type, time.Since(start),
-// 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
@@ -942,21 +936,13 @@ func (e *Engine) recordResult(
 // It is a cross-target helper the targets call, and therefore the
 // 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,
 ) {
+	e.mx.DeliveryStatusChanged(d.Target.Type, status)
+
 	err := webhookDB.Model(d).
 		Update("status", status).Error
 	if err != nil {
@@ -966,11 +952,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,
 ) {

internal/delivery/export_test.go

@@ -254,7 +254,7 @@ func NewTestEngine(
 		deliveryCh: make(chan Task, deliveryChannelSize),
 		retryCh:    make(chan Task, retryChannelSize),
 		workers:    workers,
-		mtr:        metrics.Default(),
+		mx:         metrics.Default(),
 	}
 	e.initTargets(client)
 
@@ -269,7 +269,7 @@ func NewTestEngineSmallRetry(
 	e := &Engine{
 		log:     log,
 		retryCh: make(chan Task, 1),
-		mtr:     metrics.Default(),
+		mx:      metrics.Default(),
 	}
 	e.initTargets(nil)
 
@@ -292,7 +292,7 @@ func NewTestEngineWithDB(
 		deliveryCh: make(chan Task, deliveryChannelSize),
 		retryCh:    make(chan Task, retryChannelSize),
 		workers:    workers,
-		mtr:        metrics.Default(),
+		mx:         metrics.Default(),
 	}
 	e.initTargets(client)
 
@@ -302,8 +302,8 @@ func NewTestEngineWithDB(
 // 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) {
+func (e *Engine) ExportSetMetrics(mx *metrics.Set) {
-	e.mtr = mtr
+	e.mx = mx
 }
 
 // ExportSampleQueueDepths runs one queue depth sample synchronously.

internal/delivery/metrics_test.go

@@ -31,7 +31,6 @@ const (
 const (
 	mTypeHTTP = "http"
 	mTypeLog  = "log"
-	mTypeUnknown = "unknown"
 )
 
 // mIsolate gives the setup's engine a metric set registered on a
@@ -106,15 +105,14 @@ func mGauge(
 		GetGauge().GetValue()
 }
 
-// mHTTPDurations returns how many samples the delivery duration
+func mObservations(
-// histogram holds for the http target type, which is the type every
+	t *testing.T,
-// test here times.
+	reg *prometheus.Registry,
-func mHTTPDurations(
+	name, targetType string,
-	t *testing.T, reg *prometheus.Registry,
 ) uint64 {
 	t.Helper()
 
-	return mFind(t, reg, mDuration, mTypeHTTP).
+	return mFind(t, reg, name, targetType).
 		GetHistogram().GetSampleCount()
 }
 
@@ -140,7 +138,7 @@ func TestDeliveryMetrics_SuccessAndRetryExhaustion(
 	assert.InDelta(t, 0.0,
 		mCounter(t, reg, mRetries, mTypeHTTP), 0)
 	assert.Equal(t, uint64(1),
-		mHTTPDurations(t, reg))
+		mObservations(t, reg, mDuration, mTypeHTTP))
 
 	mExhaustRetries(t, s)
 
@@ -155,7 +153,7 @@ func TestDeliveryMetrics_SuccessAndRetryExhaustion(
 	assert.InDelta(t, 1.0,
 		mCounter(t, reg, mFailed, mTypeHTTP), 0)
 	assert.Equal(t, uint64(3),
-		mHTTPDurations(t, reg))
+		mObservations(t, reg, mDuration, mTypeHTTP))
 
 	// Two consecutive failures are below the trip threshold.
 	assert.InDelta(t, 0.0,
@@ -315,128 +313,6 @@ func TestDeliveryMetrics_CircuitBreakerGauge(t *testing.T) {
 		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
@@ -500,46 +376,3 @@ func TestDeliveryMetrics_QueueDepthGauges(t *testing.T) {
 	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

@@ -89,7 +89,7 @@ func (e *Engine) sampleQueueDepths(ctx context.Context) {
 		)
 	}
 
-	e.mtr.SetQueueDepths(pending, retrying)
+	e.mx.SetQueueDepths(pending, retrying)
 }
 
 // targetTypesByID maps every configured target id to its type. The
@@ -121,13 +121,9 @@ func (e *Engine) targetTypesByID() (
 }
 
 // sampleWebhookQueueDepths adds one webhook's queued deliveries into
-// the running totals.
+// the running totals. A delivery whose target has since been deleted
-//
+// resolves to the empty type and lands in the unknown bucket rather
-// A delivery whose target has since been deleted is not in the type
+// than being dropped.
-// 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,

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_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_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,
 	)
 }
 
@@ -119,8 +116,6 @@ func (c *httpCore) withRetry(
 
 	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 +126,7 @@ func (c *httpCore) withRetry(
 		cb.RecordSuccess()
 
 		c.eng.updateDeliveryStatus(
-			webhookDB, d, d.Target.Type,
+			webhookDB, d,
 			database.DeliveryStatusDelivered,
 		)
 
@@ -169,7 +164,7 @@ func (c *httpCore) circuitBreakerBlock(
 	)
 
 	c.eng.updateDeliveryStatus(
-		webhookDB, d, d.Target.Type,
+		webhookDB, d,
 		database.DeliveryStatusRetrying,
 	)
 
@@ -189,7 +184,7 @@ func (c *httpCore) handleRetry(
 ) {
 	if attemptNum >= maxRetries {
 		c.eng.updateDeliveryStatus(
-			webhookDB, d, d.Target.Type,
+			webhookDB, d,
 			database.DeliveryStatusFailed,
 		)
 
@@ -197,8 +192,7 @@ func (c *httpCore) handleRetry(
 	}
 
 	c.eng.updateDeliveryStatus(
-		webhookDB, d, d.Target.Type,
+		webhookDB, d, database.DeliveryStatusRetrying,
-		database.DeliveryStatusRetrying,
 	)
 
 	backoff := calcBackoff(attemptNum)
@@ -247,7 +241,7 @@ func (c *httpCore) publishCircuitState(
 		return true
 	})
 
-	c.eng.mtr.SetCircuitBreakersOpen(targetType, open)
+	c.eng.mx.SetCircuitBreakersOpen(targetType, open)
 }
 
 // remainingBackoff returns how long remains of the backoff
@@ -337,8 +331,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_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/handlers/handlers.go

@@ -74,7 +74,7 @@ type Handlers struct {
 	mw        *middleware.Middleware
 	notifier  delivery.Notifier
 	evictor   delivery.WebhookEvictor
-	mtr       *metrics.Set
+	mx        *metrics.Set
 	templates map[string]*template.Template
 
 	// dummyVerifications counts the equivalent-cost verifications
@@ -116,7 +116,7 @@ func New(
 	s.mw = params.Middleware
 	s.notifier = params.Notifier
 	s.evictor = params.Evictor
-	s.mtr = metrics.Default()
+	s.mx = metrics.Default()
 
 	// Parse all page templates once at startup
 	s.templates = map[string]*template.Template{

internal/handlers/webhook.go

@@ -220,7 +220,7 @@ func (h *Handlers) createAndDeliverEvent(
 	// 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.mx.EventReceived()
 
 	h.finishWebhookResponse(w, event, entrypoint, tasks)
 }

internal/metrics/metrics.go

@@ -171,54 +171,20 @@ func (s *Set) DeliveryStatusChanged(
 }
 
 // SetQueueDepths publishes the pending and retrying queue depths from
-// one sample. Every label in the queue domain is written on every
+// one sample. Every known target type is written on every call, so a
-// call, so a type whose queue has drained reads zero instead of
+// type whose queue has drained reads zero instead of holding its last
-// holding its last value forever.
+// 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))
+		label := string(t)
+
+		s.deliveriesPending.WithLabelValues(label).
+			Set(float64(pending[t]))
+		s.deliveriesRetrying.WithLabelValues(label).
+			Set(float64(retrying[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
@@ -308,12 +274,6 @@ func (s *Set) registerGauges(factory promauto.Factory) {
 // 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)
@@ -326,9 +286,6 @@ func (s *Set) initSeries() {
 		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

internal/metrics/metrics_test.go

@@ -11,12 +11,6 @@ import (
 	"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(
@@ -111,7 +105,9 @@ func TestUnknownTargetTypeCollapses(t *testing.T) {
 	)
 
 	assert.ElementsMatch(t,
-		append(knownLabels(), "unknown"),
+		[]string{
+			"http", "database", "log", "slack", "unknown",
+		},
 		values,
 	)
 }
@@ -167,72 +163,16 @@ func TestKnownSeriesExistBeforeAnyDelivery(t *testing.T) {
 		"webhooker_deliveries_succeeded_total",
 		"webhooker_deliveries_failed_total",
 		"webhooker_delivery_retries_total",
+		"webhooker_deliveries_pending",
+		"webhooker_deliveries_retrying",
 		"webhooker_circuit_breakers_open",
 	} {
 		assert.ElementsMatch(t,
-			knownLabels(),
+			[]string{"http", "database", "log", "slack"},
 			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

internal/server/http.go

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

internal/server/listen_failure_test.go

@@ -1,95 +0,0 @@
-package server_test
-
-import (
-	"context"
-	"net"
-	"testing"
-	"time"
-
-	"github.com/stretchr/testify/require"
-	"go.uber.org/fx"
-	"sneak.berlin/go/webhooker/internal/globals"
-	"sneak.berlin/go/webhooker/internal/server"
-)
-
-// listenFailureDeadline is how long the app gets to give up after a
-// listen it cannot satisfy. The defect this pins left the process
-// reporting RUNNING for 183 seconds with nothing bound; a bind error
-// is known instantly, so anything past a moment here is that defect
-// back.
-const listenFailureDeadline = 2 * time.Second
-
-// lifecycleTimeout bounds the app's start and stop sequences so a
-// wedged hook fails the test instead of hanging it.
-const lifecycleTimeout = 15 * time.Second
-
-// TestListenFailure_ShutsDownTheApp pins that a listener the server
-// cannot bind terminates the application with a non-zero status.
-//
-// The fx OnStart hook returns as soon as the serving goroutine is
-// spawned, so a bind failure is discovered after fx has already
-// reported RUNNING. Nothing else in the graph observes it, and the
-// process used to stay alive with no listener: down, but indis-
-// tinguishable from healthy to systemd's Restart=on-failure and to
-// Docker's restart policies, which is the state this test exists to
-// keep from returning.
-//
-// The port is occupied by a listener this test holds open, on a
-// kernel-chosen port, so the failure is the real EADDRINUSE the
-// operator hits when a second instance starts. Loopback is enough to
-// collide with the server's wildcard bind: a listening socket on a
-// specific address blocks the wildcard from claiming the same port.
-func TestListenFailure_ShutsDownTheApp(t *testing.T) {
-	t.Parallel()
-
-	var listenCfg net.ListenConfig
-
-	occupied, err := listenCfg.Listen(
-		t.Context(), "tcp", "127.0.0.1:0",
-	)
-	require.NoError(t, err)
-
-	t.Cleanup(func() { _ = occupied.Close() })
-
-	addr, ok := occupied.Addr().(*net.TCPAddr)
-	require.True(t, ok, "listener is not TCP")
-
-	// The collaborators come from the wired graph rather than stubs,
-	// so the Server under test is the one that ships. Only the port
-	// is test-specific.
-	env := newTestEnv(t)
-	env.cfg.Port = addr.Port
-
-	app := fx.New(
-		fx.NopLogger,
-		fx.Supply(env.log, env.cfg, env.mw, env.hnd),
-		fx.Provide(globals.New, server.New),
-		fx.Invoke(func(*server.Server) {}),
-	)
-
-	startCtx, cancelStart := context.WithTimeout(
-		context.Background(), lifecycleTimeout,
-	)
-	defer cancelStart()
-
-	require.NoError(t, app.Start(startCtx))
-
-	select {
-	case sig := <-app.Wait():
-		require.Equal(
-			t, server.ListenFailureExitCode, sig.ExitCode,
-			"listen failure must exit non-zero",
-		)
-	case <-time.After(listenFailureDeadline):
-		t.Fatal("listen failure left the app running")
-	}
-
-	// The stop sequence still has to complete: the fix must reach
-	// shutdown through fx rather than around it.
-	stopCtx, cancelStop := context.WithTimeout(
-		context.Background(), lifecycleTimeout,
-	)
-	defer cancelStop()
-
-	require.NoError(t, app.Stop(stopCtx))
-}

internal/server/routes.go

@@ -55,11 +55,8 @@ func (s *Server) setupGlobalMiddleware() {
 	s.router.Use(s.mw.SecurityHeaders())
 	s.router.Use(s.mw.Logging())
 
-	// Metrics recording middleware, registered only when the
+	// Metrics middleware (only if credentials are configured)
-	// endpoint that exposes what it records is served. The
+	if s.params.Config.MetricsUsername != "" {
-	// condition is the same MetricsAuthEnabled the /metrics mount
-	// in setupRoutes reads.
-	if s.params.Config.MetricsAuthEnabled() {
 		s.router.Use(s.mw.Metrics())
 	}
 
@@ -106,14 +103,8 @@ func (s *Server) setupRoutes() {
 		s.h.HandleHealthCheck(),
 	)
 
-	// Authenticated /metrics route. The condition is
+	// set up authenticated /metrics route:
-	// Config.MetricsAuthEnabled and never the username alone: a
+	if s.params.Config.MetricsUsername != "" {
-	// username with an empty password would otherwise mount the
-	// endpoint behind a credential map that accepts an empty
-	// password. Config rejects that combination at startup, and
-	// this reads the same value the startup log reports, so the
-	// two cannot disagree about whether the route exists.
-	if s.params.Config.MetricsAuthEnabled() {
 		s.router.Group(func(r chi.Router) {
 			r.Use(s.mw.MetricsAuth())
 			r.Get(

internal/server/routes_test.go

@@ -34,13 +34,6 @@ import (
 // the CSRF middleware executed.
 const csrfCookieName = "_gorilla_csrf"
 
-const (
-	// metricsUser and metricsAuthValue are the /metrics basic-auth
-	// credentials the metrics routing tests below configure.
-	metricsUser      = "metrics"
-	metricsAuthValue = "s3cret"
-)
-
 type noopNotifier struct{}
 
 func (n *noopNotifier) Notify([]delivery.Task) {}
@@ -76,23 +69,9 @@ type testEnv struct {
 func newTestEnv(t *testing.T) *testEnv {
 	t.Helper()
 
-	return newTestEnvWithConfig(t, &config.Config{
-		DataDir:     t.TempDir(),
-		Environment: config.EnvironmentDev,
-	})
-}
-
-// newTestEnvWithConfig is newTestEnv over a caller-supplied Config,
-// for the routes whose existence the configuration decides. The same
-// pointer reaches the router and every middleware, so a test cannot
-// accidentally configure one and not the other.
-func newTestEnvWithConfig(
-	t *testing.T, cfg *config.Config,
-) *testEnv {
-	t.Helper()
-
 	var (
 		log   *logger.Logger
+		cfg   *config.Config
 		mw    *middleware.Middleware
 		hnd   *handlers.Handlers
 		sess  *session.Session
@@ -105,7 +84,12 @@ func newTestEnvWithConfig(
 		fx.Provide(
 			globals.New,
 			logger.New,
-			func() *config.Config { return cfg },
+			func() *config.Config {
+				return &config.Config{
+					DataDir:     t.TempDir(),
+					Environment: config.EnvironmentDev,
+				}
+			},
 			database.New,
 			database.NewWebhookDBManager,
 			healthcheck.New,
@@ -115,7 +99,7 @@ func newTestEnvWithConfig(
 			middleware.New,
 			handlers.New,
 		),
-		fx.Populate(&log, &mw, &hnd, &sess, &db, &dbMgr),
+		fx.Populate(&log, &cfg, &mw, &hnd, &sess, &db, &dbMgr),
 	)
 	app.RequireStart()
 	t.Cleanup(app.RequireStop)
@@ -673,119 +657,3 @@ func TestSourceLogsBody_OtherUser404s(t *testing.T) {
 	assert.Equal(t, http.StatusSeeOther, anon.Code)
 	assert.Equal(t, "/pages/login", anon.Header().Get("Location"))
 }
-
-// metricsConfig is a Config differing from the routing default only
-// in the two /metrics credentials.
-func metricsConfig(
-	t *testing.T, username, password string,
-) *config.Config {
-	t.Helper()
-
-	return &config.Config{
-		DataDir:         t.TempDir(),
-		Environment:     config.EnvironmentDev,
-		MetricsUsername: username,
-		MetricsPassword: password,
-	}
-}
-
-// metricsRequest asks the real router for /metrics with the given
-// basic-auth credentials, or with no Authorization header when
-// username is empty.
-func (e *testEnv) metricsRequest(
-	username, password string,
-) *httptest.ResponseRecorder {
-	req := httptest.NewRequestWithContext(
-		context.Background(), http.MethodGet, "/metrics", nil,
-	)
-
-	if username != "" {
-		req.SetBasicAuth(username, password)
-	}
-
-	w := httptest.NewRecorder()
-	e.router.ServeHTTP(w, req)
-
-	return w
-}
-
-// TestMetricsRouteUnmountedWithoutCredentials pins that with neither
-// credential configured the route does not exist, which is the
-// documented behaviour and the only valid way for /metrics to be
-// absent.
-func TestMetricsRouteUnmountedWithoutCredentials(t *testing.T) {
-	t.Parallel()
-
-	env := newTestEnvWithConfig(t, metricsConfig(t, "", ""))
-
-	assert.Equal(
-		t, http.StatusNotFound,
-		env.metricsRequest("", "").Code,
-	)
-}
-
-// TestMetricsRouteRequiresCredentials pins that with both credentials
-// configured the route exists and every request that does not carry
-// the configured pair is refused — including the empty password that
-// a half-set configuration used to make sufficient.
-func TestMetricsRouteRequiresCredentials(t *testing.T) {
-	t.Parallel()
-
-	env := newTestEnvWithConfig(
-		t, metricsConfig(t, metricsUser, metricsAuthValue),
-	)
-
-	assert.Equal(
-		t, http.StatusUnauthorized,
-		env.metricsRequest("", "").Code,
-		"no credentials must not reach the metrics handler",
-	)
-	assert.Equal(
-		t, http.StatusUnauthorized,
-		env.metricsRequest(metricsUser, "").Code,
-		"an empty password must not reach the metrics handler",
-	)
-	assert.Equal(
-		t, http.StatusUnauthorized,
-		env.metricsRequest(metricsUser, "wrong").Code,
-	)
-
-	ok := env.metricsRequest(metricsUser, metricsAuthValue)
-	assert.Equal(t, http.StatusOK, ok.Code)
-	assert.Contains(t, ok.Body.String(), "go_goroutines")
-}
-
-// TestMetricsRouteUnmountedOnHalfSetConfig pins the defect from
-// https://git.eeqj.de/sneak/webhooker/issues/205 at the routing
-// layer. Config rejects a half-set pair at startup, so this Config
-// cannot be reached from the environment; the assertion is that the
-// route tree does not publish an endpoint accepting an empty
-// password even when handed one anyway, because the mount and the
-// startup log's hasMetricsAuth read the same value.
-func TestMetricsRouteUnmountedOnHalfSetConfig(t *testing.T) {
-	t.Parallel()
-
-	for _, tc := range []struct {
-		name     string
-		username string
-		password string
-	}{
-		{name: "username only", username: metricsUser},
-		{name: "password only", password: metricsAuthValue},
-	} {
-		t.Run(tc.name, func(t *testing.T) {
-			t.Parallel()
-
-			cfg := metricsConfig(t, tc.username, tc.password)
-			env := newTestEnvWithConfig(t, cfg)
-
-			assert.False(t, cfg.MetricsAuthEnabled())
-			assert.Equal(
-				t, http.StatusNotFound,
-				env.metricsRequest(
-					tc.username, tc.password,
-				).Code,
-			)
-		})
-	}
-}

internal/server/server.go

@@ -50,13 +50,6 @@ const (
 	minSentryFlush = 250 * time.Millisecond
 )
 
-// ListenFailureExitCode is the status the process exits with when the
-// HTTP listener cannot be established, or dies for a reason other
-// than a requested shutdown. It must stay non-zero: systemd
-// `Restart=on-failure` and Docker's restart policies key off it, and a
-// zero exit would read as a deliberate stop.
-const ListenFailureExitCode = 1
-
 // SentryFlushBudget reports how long the Sentry flush may run when
 // remaining is the time left on the fx stop context after the HTTP
 // drain. sentry.Flush takes a bare duration and honours no context,
@@ -82,13 +75,13 @@ type ServerParams struct {
 	Config     *config.Config
 	Middleware *middleware.Middleware
 	Handlers   *handlers.Handlers
-	Shutdowner fx.Shutdowner
 }
 
 // Server is the main HTTP server that wires up routes and manages
 // graceful shutdown.
 type Server struct {
 	startupTime   time.Time
+	exitCode      int
 	sentryEnabled bool
 	log           *slog.Logger
 	cancelFunc    context.CancelFunc
@@ -166,12 +159,7 @@ func (s *Server) enableSentry() {
 	s.sentryEnabled = true
 }
 
-// serve installs the signal watcher, starts the listener and blocks
+func (s *Server) serve() int {
-// until the server's context is cancelled. The process exit status is
-// fx's to decide — from a signal, or from the code
-// shutdownOnListenFailure hands the Shutdowner — so this reports
-// nothing back to its caller.
-func (s *Server) serve() {
 	ctx, cancelFunc := context.WithCancel(context.Background())
 	s.cancelFunc = cancelFunc
 
@@ -197,30 +185,7 @@ func (s *Server) serve() {
 	<-ctx.Done()
 	// Shutdown is handled by the fx OnStop hook (cleanShutdown).
 	// Do not call cleanShutdown() here to avoid double invocation.
-}
+	return s.exitCode
-
-// shutdownOnListenFailure ends the application after the HTTP
-// listener failed. The fx OnStart hook returns as soon as the serving
-// goroutine is spawned, so nothing downstream of it ever learns that
-// the listen failed: fx reports RUNNING and the process sits alive
-// with nothing bound, which is invisible to systemd and Docker
-// restart policies. Asking the Shutdowner to stop the app with a
-// non-zero code is what turns that into a visible failure.
-//
-// The context cancel that follows only unwinds serve()'s own wait.
-// The shutdown itself runs through fx's normal stop sequence, so the
-// clean-shutdown drain in cleanShutdown is reached unchanged.
-func (s *Server) shutdownOnListenFailure() {
-	err := s.params.Shutdowner.Shutdown(
-		fx.ExitCode(ListenFailureExitCode),
-	)
-	if err != nil {
-		s.log.Error("shutdown request failed", "error", err)
-	}
-
-	if s.cancelFunc != nil {
-		s.cancelFunc()
-	}
 }
 
 func (s *Server) cleanupForExit() {
@@ -228,6 +193,9 @@ func (s *Server) cleanupForExit() {
 }
 
 func (s *Server) cleanShutdown(ctx context.Context) {
+	// initiate clean shutdown
+	s.exitCode = 0
+
 	ctxShutdown, shutdownCancel := context.WithTimeout(
 		ctx, ShutdownTimeout,
 	)