Send Content-Type once on a delivery (closes #246)

A delivery set Content-Type from the event's ContentType and then
added the inbound Content-Type from the event's stored headers, so
the target could receive two values. The inbound Content-Type is no
longer forwarded from the stored headers; the receiver already saved
it as the event's ContentType.

Which value wins is now stated at applyRequestHeaders: a Content-Type
configured on the target, otherwise the event's ContentType,
otherwise none.

Model: opus-5-5

README.md

@@ -157,11 +157,6 @@ private and reserved ranges — RFC 1918, loopback, CGNAT, link-local and
 the rest — are refused, which stops a target from being used to make
 webhooker probe the network it sits in.
 
-Besides the private and reserved ranges, the default blocklist refuses
-public cloud metadata addresses: currently only `168.63.129.16`, Azure's
-WireServer, which serves an Azure VM its credentials. Because it is a
-public address, listing it in `ALLOWED_EGRESS_CIDRS` reopens it.
-
 That default is also inconvenient for the thing webhooker is mostly
 for: taking a public webhook and forwarding it to something on your own
 network. A container on the same Docker network, a box on `10.x`, a
@@ -200,16 +195,15 @@ Two things this setting cannot do:
   the list is always an allowlist; an empty list (the default) means
   every private and reserved range stays refused. Note that
   `0.0.0.0/0` gets you most of the way there anyway, per above.
-- **It cannot open link-local, or a cloud metadata endpoint at a
-  non-public address that discloses credentials or user data.** An
-  address is on the list below when it is not a public address and both
-  of these hold: the provider fixes it, so it cannot collide with
-  anything you run; and reaching it hands out credentials, user data or
-  bootstrap material. Those stay blocked no matter what you list,
-  including when you list them outright or list a supernet such as
-  `0.0.0.0/0`, `::/0`, `fd00::/8` or `100.64.0.0/10`. Treat this as best
-  effort rather than a guarantee — it is a hand-maintained list and the
-  caveat below the table applies:
+- **It cannot open link-local, or a cloud metadata endpoint that
+  discloses credentials or user data.** An address is on the list below
+  when both of these hold: the provider fixes it, so it cannot collide
+  with anything you run; and reaching it hands out credentials, user
+  data or bootstrap material. Those stay blocked no matter what you
+  list, including when you list them outright or list a supernet such
+  as `0.0.0.0/0`, `::/0`, `fd00::/8` or `100.64.0.0/10`. Treat this as
+  best effort rather than a guarantee — it is a hand-maintained list
+  and the caveat below the table applies:
 
   | Blocked unconditionally | What it is |
   | ----------------------- | ---------- |
@@ -248,8 +242,7 @@ Two things this setting cannot do:
   encodings, which the default blocklist does not match. A publicly
   routable metadata address is not listed here, because nothing on this
   list can be reopened and blocking one that way would leave you no
-  escape hatch at all; Azure's `168.63.129.16` is refused by the default
-  blocklist instead, as described above.
+  escape hatch at all.
 
   This list is not exhaustive of every cloud's metadata address — if
   yours is not here, do not allowlist the block that contains it.
@@ -975,10 +968,15 @@ scratch file**: it holds committed transactions that are not yet in the
 have no readable schema at all. `-shm` is regenerable, but there is no
 reason to separate the two — copy the directory and you have them.
 
-A clean shutdown closes every database, which checkpoints and removes
-its sidecars; a killed or crashed instance leaves them, and they must be
-carried with the `.db`. An archive the service has not opened since a
-crash keeps that crash's sidecars, even across a later clean stop.
+A clean shutdown closes `webhooker.db` and every `events-*.db`, which
+checkpoints and removes their sidecars; a killed or crashed instance
+leaves them, and they must be carried with the `.db`. **Archive
+databases are different**: their handle is not closed at shutdown, so
+`archive-*.db-wal` and `-shm` normally survive a clean stop and the
+`-wal` can hold every row the archive has. Measured on a stopped
+instance: `archive-….db` 4096 bytes with no table, its `-wal` 157 KB
+holding all 8 archived events. Copying `DATA_DIR` in full is what makes
+this a non-issue; copying `.db` files out of it by name is not.
 
 Configuration is **not** in `DATA_DIR` — it comes from the environment
 and from a `.env` file read out of the process working directory. Back
@@ -1053,9 +1051,10 @@ The file becomes self-contained again when the handle closes, which
 happens on the next write past the debounce window, when the connection
 pool retires the idle connection (about a minute after the last write),
 or at the idle archive sweep — measured, the same file was a complete
-20 KB `.db` with no sidecars about a minute after its last write. A
-clean stop closes it too. So either move `archive-{uuid}.db` together
-with any `-wal`/`-shm` beside it, or wait until there are none.
+20 KB `.db` with no sidecars about a minute after its last write.
+Shutdown is **not** on that list: the archive handle is not closed when
+the service stops. So either move `archive-{uuid}.db` together with any
+`-wal`/`-shm` beside it, or wait until there are none.
 
 ### Restore
 
@@ -1074,10 +1073,12 @@ with any `-wal`/`-shm` beside it, or wait until there are none.
    They are part of the database, and dropping a `-wal` silently
    discards every transaction it still holds. An `.backup` set will not
    contain any: it writes a single consolidated file per database. A
-   stop-and-copy set normally has none, because a clean stop closes
-   every database and checkpoints its sidecars away; the exception is an
-   archive not opened since a crash. A copy salvaged from a crashed
-   instance has them for everything, and needs all of them.
+   stop-and-copy set has none for `webhooker.db` or the `events-*.db`,
+   because a clean stop closes those and checkpoints their sidecars
+   away — but it will normally have them for `archive-*.db`, whose
+   handle stays open across shutdown, and those carry the archive's
+   rows. A copy salvaged from a crashed instance has them for
+   everything, and needs all of them.
 
 4. **Fix ownership.** The container runs as the non-root `webhooker`
    user, UID 1000 / GID 1000. Restored files must be owned by (or
@@ -2721,7 +2722,7 @@ abuse limit later; they are tracked as future work.
 | ------ | --------------------------- | ----------- |
 | `GET`  | `/`                         | Root redirect, 303 (authenticated → `/sources`, unauthenticated → `/pages/login`) |
 | `GET`  | `/.well-known/healthcheck`  | Health check (JSON: `status`, `now`, `uptimeSeconds`, `uptimeHuman`, `version`, `appname`, `maintenanceMode`) |
-| `GET`, `HEAD` | `/s/*`              | Static file serving (embedded CSS, JS). `GET` and `HEAD` only — `POST`, `PUT`, `PATCH`, `DELETE`, `OPTIONS`, `TRACE` and `CONNECT` are answered `405 Method Not Allowed` with `Allow: GET, HEAD`. Any other method (such as `PROPFIND`) is refused by chi before it reaches this route, and gets `405` without an `Allow` header. Pinned by `TestStaticServesOnlyGetAndHead` |
+| any    | `/s/*`                      | Static file serving (embedded CSS, JS). Mounted for every method, not just `GET`/`HEAD`: chi's `Mount` registers all methods and `http.FileServer` special-cases only `HEAD` (by omitting the body), so a `POST` or `DELETE` to an asset is answered `200` with the file. Pinned by `TestStaticServesEveryMethod` |
 | `POST` | `/webhook/{uuid}`           | Webhook receiver endpoint. `POST` only — every other method is answered `405 Method Not Allowed` with `Allow: POST`. Rate limited (see [Rate Limiting](#rate-limiting)) |
 
 #### Authentication Endpoints
@@ -3087,8 +3088,7 @@ each hook. The order, read off the fx stop-hook log:
 3. `server` — the HTTP drain, bounded separately by
    `server.ShutdownTimeout` (**3 seconds**), then a Sentry flush if
    `SENTRY_DSN` is set
-4. `delivery.Engine` — waits for its workers, then closes the archive
-   databases
+4. `delivery.Engine`
 5. `healthcheck`
 6. `WebhookDBManager`
 7. the database close

cmd/webhooker/main.go

@@ -16,7 +16,6 @@ import (
 	"sneak.berlin/go/webhooker/internal/handlers"
 	"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/resetpw"
 	"sneak.berlin/go/webhooker/internal/server"
@@ -178,10 +177,6 @@ func newApp() *fx.App {
 			healthcheck.New,
 			session.New,
 			handlers.New,
-			// The registry /metrics serves, and the delivery
-			// collectors registered on it.
-			metrics.NewRegistry,
-			metrics.New,
 			middleware.New,
 			// The one SSRF guard both target-creation validation
 			// and the delivery dialer consult, so they cannot

internal/config/config.go

@@ -192,10 +192,9 @@ type Config struct {
 	// alwaysBlockedNetworks stays blocked no matter what is listed
 	// here. That set is link-local plus the cloud metadata
 	// endpoints outside it that disclose credentials or user data
-	// at a provider-fixed, non-public address; it is not
-	// exhaustive of every cloud's metadata address. See
-	// alwaysBlockedNetworks for the authoritative list and the
-	// criterion it is built from.
+	// at a provider-fixed address; it is not exhaustive of every
+	// cloud's metadata address. See alwaysBlockedNetworks for the
+	// authoritative list and the criterion it is built from.
 	AllowedEgressCIDRs []netip.Prefix
 
 	params *ConfigParams
@@ -747,14 +746,12 @@ func (c *Config) warnEgressAllowlist(log *slog.Logger) {
 
 	log.Warn(
 		"ALLOWED_EGRESS_CIDRS lets delivery targets reach these "+
-			"otherwise-blocked networks. Anyone who can create a "+
-			"delivery target can now make this process issue "+
-			"requests into them, and read back the response. Only "+
-			"the addresses the README lists as blocked "+
-			"unconditionally stay blocked regardless of what is "+
-			"listed here; a public cloud metadata address such as "+
-			"168.63.129.16 is reachable once it, or a block "+
-			"covering it, is listed.",
+			"otherwise-blocked private/reserved networks. Anyone "+
+			"who can create a delivery target can now make this "+
+			"process issue requests into them, and read back the "+
+			"response. Link-local and the known cloud instance "+
+			"metadata endpoints outside it stay blocked "+
+			"regardless of what is listed here.",
 		"allowedEgressCIDRs",
 		strings.Join(PrefixStrings(c.AllowedEgressCIDRs), ","),
 	)

internal/config/config_test.go

@@ -834,13 +834,12 @@ func TestEgressAllowlistWarning(t *testing.T) {
 			// to be able to read back which networks are open.
 			assert.Contains(t, logged, "10.0.0.0/8")
 			assert.Contains(t, logged, "127.0.0.0/8")
-			// What stays shut is the whole unconditional set, not
-			// link-local alone; a public metadata address is not in
-			// it, so a listed block covering it opens it.
-			assert.Contains(t, logged, "blocked unconditionally")
-			assert.Contains(t, logged, "168.63.129.16 is reachable")
-			// The listed blocks need not be private or reserved.
-			assert.NotContains(t, logged, "private/reserved")
+			// What stays shut. Asserted on the clause naming the
+			// wider set rather than on "Link-local" alone, so the
+			// string cannot narrow back to link-local only while
+			// the always-blocked set covers ULA, CGNAT and two
+			// public metadata addresses as well.
+			assert.Contains(t, logged, "metadata endpoints outside it")
 		})
 	}
 }

internal/delivery/engine.go

@@ -148,7 +148,6 @@ type EngineParams struct {
 	DBManager *database.WebhookDBManager
 	Logger    *logger.Logger
 	SSRFGuard *Guard
-	Metrics   *metrics.Set
 }
 
 // Engine processes queued deliveries in the background
@@ -168,10 +167,10 @@ type Engine struct {
 	retryCh    chan Task
 	workers    int
 
-	// mtr is the delivery metric set. Production wires the one
-	// registered on the registry /metrics serves; a test can
-	// substitute a set registered on a registry it holds, so it can
-	// gather what its own deliveries recorded.
+	// 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.
@@ -205,7 +204,7 @@ func New(
 		deliveryCh: make(chan Task, deliveryChannelSize),
 		retryCh:    make(chan Task, retryChannelSize),
 		workers:    defaultWorkers,
-		mtr:        params.Metrics,
+		mtr:        metrics.Default(),
 	}
 
 	e.initTargets(&http.Client{
@@ -363,15 +362,6 @@ func (e *Engine) start() {
 // stop cancels the worker pool's context and waits for the pool
 // to drain, bounded by the stop hook's context: a wedged worker
 // must not hang the process past fx's stop timeout.
-//
-// Once the pool has drained it closes the archive writers, so a
-// clean stop leaves no archive -wal behind. Nothing else holds a
-// writer for long by then: the archive sweeper stops before the
-// engine, and deleting a webhook only closes one. If the pool did
-// not drain in time, the writers are left open, as a kill would
-// leave them. Closing them would wait for any write in progress,
-// and a worker still running would then open new writers that
-// nothing closes, so it gains nothing over a kill.
 func (e *Engine) stop(ctx context.Context) error {
 	e.log.Info("delivery engine stopping")
 
@@ -386,8 +376,6 @@ func (e *Engine) stop(ctx context.Context) error {
 		return err
 	}
 
-	e.dbTarget.evictAll()
-
 	e.log.Info("delivery engine stopped")
 
 	return nil
@@ -740,7 +728,9 @@ func (e *Engine) recoverSingleRetry(
 		// webhook on one bad read would be a far larger fault than
 		// the strand it is meant to clear.
 		if errors.Is(err, gorm.ErrRecordNotFound) {
-			e.failMissingTarget(webhookDB, webhookID, d)
+			e.failMissingTargetRetry(
+				webhookDB, webhookID, d,
+			)
 
 			return
 		}
@@ -1143,7 +1133,9 @@ func (e *Engine) sweepSingleRetry(
 		// Deleted is terminal, unreadable is not; see
 		// recoverSingleRetry.
 		if errors.Is(err, gorm.ErrRecordNotFound) {
-			e.failMissingTarget(webhookDB, webhookID, d)
+			e.failMissingTargetRetry(
+				webhookDB, webhookID, d,
+			)
 
 			return
 		}
@@ -1257,19 +1249,19 @@ func (e *Engine) failUnretryableRetry(
 	e.failDelivery(webhookDB, d, target.Type, reason)
 }
 
-// failMissingTarget terminally fails a recovered delivery, pending or
-// retrying, whose target row is gone. Restart recovery and the periodic
-// sweep call it for both statuses, so the transition exists once.
+// failMissingTargetRetry terminally fails an orphaned retrying
+// delivery whose target row is gone. Both restart recovery and the
+// periodic sweep call it, so the transition exists once.
 //
-// Until it existed those paths logged the failed lookup and moved on,
-// which left the delivery where it was for the life of the database and
+// Until it existed both paths logged the failed lookup and returned,
+// which left the delivery retrying for the life of the database and
 // the sweep repeating the same error every minute forever. Failing it
 // with a recorded reason is the treatment the other orphaned-retry
 // cases already get, so all of them read alike in the event log.
 //
 // Logged at warn rather than error: a deleted target is an operator
 // action, not a system fault.
-func (e *Engine) failMissingTarget(
+func (e *Engine) failMissingTargetRetry(
 	webhookDB *gorm.DB,
 	webhookID string,
 	d *database.Delivery,
@@ -1282,37 +1274,13 @@ func (e *Engine) failMissingTarget(
 
 	defer e.inflight.release(d.ID)
 
-	// The batch was read before ownership was taken, and a worker may
-	// have settled the delivery and let it go in between. Only a row
-	// still in the status the batch read is failed.
-	row, err := e.loadDelivery(webhookDB, d.ID)
-	if err != nil {
-		e.log.Error(
-			"failed to load delivery",
-			"delivery_id", d.ID,
-			"error", err,
-		)
-
-		return
-	}
-
-	if row.Status != d.Status {
-		e.log.Debug(
-			"delivery already handled, not failed",
-			"delivery_id", d.ID,
-			"status", row.Status,
-		)
-
-		return
-	}
-
 	targetType, reason := e.missingTargetReason(d.TargetID)
 
 	e.log.Warn(
-		"failing recovered delivery: its target no longer exists",
+		"failing orphaned retrying delivery: "+
+			"its target no longer exists",
 		"webhook_id", webhookID,
 		"delivery_id", d.ID,
-		"status", d.Status,
 		"target_id", d.TargetID,
 		"target_type", targetType,
 	)
@@ -1346,14 +1314,15 @@ func (e *Engine) missingTargetReason(
 	if err != nil {
 		return "", fmt.Sprintf(
 			"target %s no longer exists; the delivery "+
-				"has been failed terminally",
+				"cannot be retried and has been failed "+
+				"terminally",
 			targetID,
 		)
 	}
 
 	return target.Type, fmt.Sprintf(
 		"target %q (type %s) was deleted; the delivery "+
-			"has been failed terminally",
+			"cannot be retried and has been failed terminally",
 		target.Name, target.Type,
 	)
 }
@@ -2052,31 +2021,14 @@ func (e *Engine) sendRecoveredDeliveries(
 
 		target, ok := targetMap[deliveries[i].TargetID]
 		if !ok {
-			// A missing entry does not mean the target is gone: the
-			// map is also empty when its query failed. Only a lookup
-			// that finds no row ends the delivery; any other error
-			// leaves it pending for the next sweep. See
-			// recoverSingleRetry.
-			var err error
-
-			target, err = e.loadTarget(deliveries[i].TargetID)
-			if errors.Is(err, gorm.ErrRecordNotFound) {
-				e.failMissingTarget(webhookDB, webhookID, &deliveries[i])
-
-				continue
-			}
-
-			if err != nil {
 			e.log.Error(
-					"failed to load target for recovered delivery",
+				"target not found for delivery",
 				"delivery_id", deliveries[i].ID,
 				"target_id", deliveries[i].TargetID,
-					"error", err,
 			)
 
 			continue
 		}
-		}
 
 		if !e.takeForRedispatch(
 			webhookDB, deliveries[i].ID,

internal/delivery/engine_lifecycle_test.go

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

internal/delivery/export_test.go

@@ -9,7 +9,6 @@ import (
 	"net/url"
 	"time"
 
-	"github.com/prometheus/client_golang/prometheus"
 	"go.uber.org/fx"
 	"gorm.io/gorm"
 	"sneak.berlin/go/webhooker/internal/database"
@@ -343,31 +342,6 @@ func (e *Engine) ExportRecoverRetryingDeliveries(
 	e.recoverRetryingDeliveries(webhookDB, webhookID)
 }
 
-// ExportFailMissingTarget exposes failMissingTarget, so a test can hand
-// it a delivery as a batch read it earlier.
-func (e *Engine) ExportFailMissingTarget(
-	webhookDB *gorm.DB,
-	webhookID string,
-	d *database.Delivery,
-) {
-	e.failMissingTarget(webhookDB, webhookID, d)
-}
-
-// ExportSendRecoveredDeliveries exposes sendRecoveredDeliveries, so a
-// test can hand it a target map that lacks a delivery's target.
-func (e *Engine) ExportSendRecoveredDeliveries(
-	ctx context.Context,
-	webhookDB *gorm.DB,
-	deliveries []database.Delivery,
-	webhookID string,
-	targetMap map[string]database.Target,
-	settled map[string]struct{},
-) {
-	e.sendRecoveredDeliveries(
-		ctx, webhookDB, deliveries, webhookID, targetMap, settled,
-	)
-}
-
 // ExportDeliveryCh returns the delivery channel.
 func (e *Engine) ExportDeliveryCh() chan Task {
 	return e.deliveryCh
@@ -390,7 +364,7 @@ func NewTestEngine(
 		deliveryCh: make(chan Task, deliveryChannelSize),
 		retryCh:    make(chan Task, retryChannelSize),
 		workers:    workers,
-		mtr:        metrics.New(prometheus.NewRegistry()),
+		mtr:        metrics.Default(),
 	}
 	e.initTargets(client)
 
@@ -405,7 +379,7 @@ func NewTestEngineSmallRetry(
 	e := &Engine{
 		log:     log,
 		retryCh: make(chan Task, 1),
-		mtr:     metrics.New(prometheus.NewRegistry()),
+		mtr:     metrics.Default(),
 	}
 	e.initTargets(nil)
 
@@ -428,7 +402,7 @@ func NewTestEngineWithDB(
 		deliveryCh: make(chan Task, deliveryChannelSize),
 		retryCh:    make(chan Task, retryChannelSize),
 		workers:    workers,
-		mtr:        metrics.New(prometheus.NewRegistry()),
+		mtr:        metrics.Default(),
 	}
 	e.initTargets(client)
 
@@ -436,7 +410,8 @@ func NewTestEngineWithDB(
 }
 
 // ExportSetMetrics substitutes the engine's metric set, so a test can
-// assert on collectors registered on a registry it holds.
+// 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
 }

internal/delivery/metrics_test.go

@@ -35,8 +35,9 @@ const (
 )
 
 // mIsolate gives the setup's engine a metric set registered on a
-// registry this test holds, so its exact assertions can gather from
-// it.
+// 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 {

internal/delivery/redirect_test.go

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

internal/delivery/ssrf.go

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

internal/delivery/ssrf_allowlist_test.go

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

internal/delivery/target_database.go

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

internal/delivery/target_database_evict_test.go

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

internal/delivery/terminal_state_test.go

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

internal/handlers/handlers.go

@@ -12,7 +12,6 @@ import (
 	"net/http"
 	"sync/atomic"
 
-	"github.com/prometheus/client_golang/prometheus"
 	"go.uber.org/fx"
 	"sneak.berlin/go/webhooker/internal/database"
 	"sneak.berlin/go/webhooker/internal/delivery"
@@ -62,8 +61,6 @@ type HandlersParams struct {
 	Notifier     delivery.Notifier
 	Evictor      delivery.WebhookEvictor
 	SSRFGuard    *delivery.Guard
-	Metrics      *metrics.Set
-	Registry     *prometheus.Registry
 }
 
 // Handlers provides HTTP handler methods for all application
@@ -125,7 +122,7 @@ func New(
 	s.mw = params.Middleware
 	s.notifier = params.Notifier
 	s.evictor = params.Evictor
-	s.mtr = params.Metrics
+	s.mtr = metrics.Default()
 	s.ssrf = params.SSRFGuard
 
 	// Parse all page templates once at startup

internal/handlers/handlers_test.go

@@ -20,7 +20,6 @@ import (
 	"sneak.berlin/go/webhooker/internal/handlers"
 	"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"
 )
@@ -110,8 +109,6 @@ func newTestApp(
 			func(r *recordingEvictor) delivery.WebhookEvictor {
 				return r
 			},
-			metrics.NewRegistry,
-			metrics.New,
 			middleware.New,
 			delivery.NewGuard,
 			handlers.New,

internal/handlers/metrics.go

@@ -1,20 +0,0 @@
-package handlers
-
-import (
-	"net/http"
-
-	"github.com/prometheus/client_golang/prometheus/promhttp"
-)
-
-// HandleMetrics returns the Prometheus scrape handler for the
-// registry every collector in this process registers on. It is what
-// promhttp.Handler builds for the global default registry, including
-// the promhttp_metric_handler_* series that count scrapes, pointed at
-// that registry instead.
-func (s *Handlers) HandleMetrics() http.HandlerFunc {
-	reg := s.params.Registry
-
-	return promhttp.InstrumentMetricHandler(
-		reg, promhttp.HandlerFor(reg, promhttp.HandlerOpts{}),
-	).ServeHTTP
-}

internal/metrics/metrics.go

@@ -3,17 +3,17 @@
 // deliveries are attempted, how they end, how long they take, how
 // deep the queues are, and how many circuit breakers are open.
 //
-// It also builds the registry the authenticated /metrics route
-// serves. These collectors, the inbound HTTP metrics recorded in
-// internal/middleware, and the Go runtime and process collectors all
-// register on that one registry, never on Prometheus's global default.
+// 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/collectors"
 	"github.com/prometheus/client_golang/prometheus/promauto"
 	"sneak.berlin/go/webhooker/internal/database"
 )
@@ -57,31 +57,25 @@ var knownTargetTypes = []database.TargetType{
 	database.TargetTypeSlack,
 }
 
-// NewRegistry returns the registry /metrics serves, carrying the Go
-// runtime and process collectors that Prometheus's global default
-// registry carries, so the go_* and process_* series stay in the
-// scrape.
+// 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.
 //
-// A registry of its own, rather than the global default, is what lets
-// two dependency graphs in one process — two tests, say — each
-// register their collectors without the second registration
-// panicking.
-func NewRegistry() *prometheus.Registry {
-	reg := prometheus.NewRegistry()
-	reg.MustRegister(
-		collectors.NewGoCollector(),
-		collectors.NewProcessCollector(
-			collectors.ProcessCollectorOpts{},
-		),
-	)
+//nolint:gochecknoglobals // one process-wide registration, by design
+var defaultSet = sync.OnceValue(func() *Set {
+	return New(prometheus.DefaultRegisterer)
+})
 
-	return reg
+// Default returns the process-wide metric set.
+func Default() *Set {
+	return defaultSet()
 }
 
 // Set is one registered group of webhooker's delivery collectors.
-// Production builds one on the registry /metrics serves; tests build
-// their own against a private registry so assertions are not
-// disturbed by deliveries other tests are making concurrently.
+// 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
@@ -99,7 +93,7 @@ type Set struct {
 // 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.Registry) *Set {
+func New(reg prometheus.Registerer) *Set {
 	factory := promauto.With(reg)
 
 	s := &Set{

internal/middleware/export_test.go

@@ -10,7 +10,8 @@ import (
 
 // MetricsMiddlewareForTest builds the metrics recording middleware
 // against a caller-supplied recorder, so a test can gather from its
-// own Prometheus registry without building a whole Middleware.
+// own Prometheus registry rather than the process-wide default one
+// that Middleware.Metrics uses.
 func MetricsMiddlewareForTest(
 	rec httpmetrics.Recorder,
 ) func(http.Handler) http.Handler {

internal/middleware/metrics.go

@@ -7,6 +7,7 @@ import (
 
 	"github.com/go-chi/chi"
 	httpmetrics "github.com/slok/go-http-metrics/metrics"
+	prommetrics "github.com/slok/go-http-metrics/metrics/prometheus"
 	ghmm "github.com/slok/go-http-metrics/middleware"
 	"github.com/slok/go-http-metrics/middleware/std"
 )
@@ -151,14 +152,16 @@ func (r boundedLabelRecorder) AddInflightRequests(
 var _ httpmetrics.Recorder = boundedLabelRecorder{}
 
 // Metrics returns middleware that records Prometheus HTTP metrics on
-// the registry the /metrics route serves. Every call shares the one
-// recorder New built, so any number of routers can install it.
+// the default registry, which is the one the /metrics route gathers.
 func (s *Middleware) Metrics() func(http.Handler) http.Handler {
-	return metricsMiddleware(s.metricsRecorder)
+	return metricsMiddleware(
+		prommetrics.NewRecorder(prommetrics.Config{}),
+	)
 }
 
 // metricsMiddleware builds the recording middleware against a given
-// recorder, so tests can gather from a registry of their own.
+// recorder, so tests can gather from a registry of their own instead
+// of the process-wide default.
 func metricsMiddleware(
 	rec httpmetrics.Recorder,
 ) func(http.Handler) http.Handler {

internal/middleware/metrics_test.go

@@ -57,8 +57,9 @@ const (
 // Server.setupWebhookRoutes inside it. That ordering is the whole
 // defect, so a test that flattens it would prove nothing.
 //
-// The recorder writes to a registry of the test's own, so each test
-// observes only its own traffic.
+// The recorder writes to a registry of the test's own rather than the
+// process-wide default one, so each test observes only its own
+// traffic.
 func metricsTestRouter(
 	t *testing.T,
 	receiverLimit int,

internal/middleware/middleware.go

@@ -13,9 +13,6 @@ import (
 	"github.com/go-chi/chi"
 	"github.com/go-chi/chi/middleware"
 	"github.com/go-chi/cors"
-	"github.com/prometheus/client_golang/prometheus"
-	httpmetrics "github.com/slok/go-http-metrics/metrics"
-	prommetrics "github.com/slok/go-http-metrics/metrics/prometheus"
 	"go.uber.org/fx"
 	"sneak.berlin/go/webhooker/internal/config"
 	"sneak.berlin/go/webhooker/internal/globals"
@@ -155,7 +152,6 @@ type MiddlewareParams struct {
 	Globals *globals.Globals
 	Config  *config.Config
 	Session *session.Session
-	Registry *prometheus.Registry
 }
 
 // Middleware provides HTTP middleware for logging, CORS, auth, and
@@ -165,12 +161,6 @@ type Middleware struct {
 	params  *MiddlewareParams
 	session *session.Session
 
-	// metricsRecorder records the inbound HTTP metrics on the
-	// registry /metrics serves. It is built once, in New, because
-	// building it registers its collectors, and a second
-	// registration on the same registry panics; see Metrics.
-	metricsRecorder httpmetrics.Recorder
-
 	// loginGuard counts failed credential verifications and bounds
 	// concurrent password hashing. It is built on first use so that
 	// every construction path gets one; see guard().
@@ -189,9 +179,6 @@ func New(
 	s.params = &params
 	s.log = params.Logger.Get()
 	s.session = params.Session
-	s.metricsRecorder = prommetrics.NewRecorder(
-		prommetrics.Config{Registry: params.Registry},
-	)
 
 	return s, nil
 }

internal/middleware/recoverer.go

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

internal/middleware/recoverer_test.go

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

internal/resetpw/resetpw_test.go

@@ -24,7 +24,6 @@ import (
 	"sneak.berlin/go/webhooker/internal/handlers"
 	"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/resetpw"
 	"sneak.berlin/go/webhooker/internal/session"
@@ -164,8 +163,6 @@ func newServerApp(
 			session.New,
 			func() delivery.Notifier { return &noopNotifier{} },
 			func() delivery.WebhookEvictor { return &noopEvictor{} },
-			metrics.NewRegistry,
-			metrics.New,
 			middleware.New,
 			delivery.NewGuard,
 			handlers.New,

internal/server/routes.go

@@ -7,6 +7,7 @@ import (
 	sentryhttp "github.com/getsentry/sentry-go/http"
 	"github.com/go-chi/chi"
 	"github.com/go-chi/chi/middleware"
+	"github.com/prometheus/client_golang/prometheus/promhttp"
 	"sneak.berlin/go/webhooker/static"
 )
 
@@ -91,25 +92,11 @@ func (s *Server) setupGlobalMiddleware() {
 func (s *Server) setupRoutes() {
 	s.router.Get("/", s.h.HandleIndex())
 
-	// Static assets answer GET and HEAD only. chi's default 405
-	// carries no Allow header, so this group supplies its own.
-	staticFiles := http.StripPrefix(
-		"/s", http.FileServer(http.FS(static.Static)),
+	s.router.Mount(
+		"/s",
+		http.StripPrefix("/s", http.FileServer(http.FS(static.Static))),
 	)
 
-	s.router.Route("/s", func(r chi.Router) {
-		r.MethodNotAllowed(func(w http.ResponseWriter, _ *http.Request) {
-			w.Header().Set("Allow", "GET, HEAD")
-			http.Error(
-				w,
-				"Method Not Allowed",
-				http.StatusMethodNotAllowed,
-			)
-		})
-		r.Method(http.MethodGet, "/*", staticFiles)
-		r.Method(http.MethodHead, "/*", staticFiles)
-	})
-
 	s.router.Route("/api/v1", func(_ chi.Router) {
 		// API routes will be added here.
 	})
@@ -129,7 +116,12 @@ func (s *Server) setupRoutes() {
 	if s.params.Config.MetricsAuthEnabled() {
 		s.router.Group(func(r chi.Router) {
 			r.Use(s.mw.MetricsAuth())
-			r.Get("/metrics", s.h.HandleMetrics())
+			r.Get(
+				"/metrics",
+				http.HandlerFunc(
+					promhttp.Handler().ServeHTTP,
+				),
+			)
 		})
 	}
 

internal/server/routes_test.go

@@ -23,7 +23,6 @@ import (
 	"sneak.berlin/go/webhooker/internal/handlers"
 	"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/server"
 	"sneak.berlin/go/webhooker/internal/session"
@@ -113,8 +112,6 @@ func newTestEnvWithConfig(
 			session.New,
 			func() delivery.Notifier { return &noopNotifier{} },
 			func() delivery.WebhookEvictor { return &noopEvictor{} },
-			metrics.NewRegistry,
-			metrics.New,
 			middleware.New,
 			delivery.NewGuard,
 			handlers.New,
@@ -399,15 +396,13 @@ func (e *testEnv) storedHash(t *testing.T, username string) string {
 
 // --- /s static group ---
 
-// TestStaticServesOnlyGetAndHead pins the methods the static group
-// answers: GET and HEAD are served the asset, and the other methods
-// chi routes (POST, PUT, DELETE and the rest) are refused with 405
-// and an Allow header naming those two. A method chi does not route,
-// such as PROPFIND, is refused with 405 by the top-level router
-// before it reaches the static group, so it gets no Allow header.
-// The README documents this; the test is what keeps the two from
-// drifting.
-func TestStaticServesOnlyGetAndHead(t *testing.T) {
+// TestStaticServesEveryMethod pins what the static mount actually
+// answers. chi's Mount registers the handler for all methods and
+// http.FileServer only special-cases HEAD (by suppressing the body),
+// so a POST or a DELETE to an asset is served the file rather than
+// refused. The README documents this; the test is what keeps the two
+// from drifting.
+func TestStaticServesEveryMethod(t *testing.T) {
 	t.Parallel()
 
 	env := newTestEnv(t)
@@ -422,7 +417,6 @@ func TestStaticServesOnlyGetAndHead(t *testing.T) {
 		http.MethodPost,
 		http.MethodPut,
 		http.MethodDelete,
-		"PROPFIND",
 	} {
 		t.Run(method, func(t *testing.T) {
 			t.Parallel()
@@ -434,38 +428,18 @@ func TestStaticServesOnlyGetAndHead(t *testing.T) {
 			w := httptest.NewRecorder()
 			env.router.ServeHTTP(w, req)
 
-			switch method {
-			case http.MethodGet:
-				assert.Equal(t, http.StatusOK, w.Code)
-				assert.Equal(t, body, w.Body.Bytes(),
-					"the asset itself is returned")
-			case http.MethodHead:
-				assert.Equal(t, http.StatusOK, w.Code)
+			assert.Equal(t, http.StatusOK, w.Code,
+				"static mount answers every method")
+
+			if method == http.MethodHead {
 				assert.Empty(t, w.Body.Bytes(),
 					"HEAD must not carry a body")
-			case "PROPFIND":
-				assert.Equal(
-					t, http.StatusMethodNotAllowed, w.Code,
-				)
-				assert.Empty(t, w.Header().Get("Allow"),
-					"chi refuses a method it does not route "+
-						"before the static group runs")
-				assert.NotContains(
-					t, w.Body.String(), string(body),
-					"a refused method must not get the asset",
-				)
-			default:
-				assert.Equal(
-					t, http.StatusMethodNotAllowed, w.Code,
-				)
-				assert.Equal(
-					t, "GET, HEAD", w.Header().Get("Allow"),
-				)
-				assert.NotContains(
-					t, w.Body.String(), string(body),
-					"a refused method must not get the asset",
-				)
+
+				return
 			}
+
+			assert.Equal(t, body, w.Body.Bytes(),
+				"the asset itself is returned")
 		})
 	}
 }
@@ -964,43 +938,3 @@ func TestMetricsRouteUnmountedOnHalfSetConfig(t *testing.T) {
 		})
 	}
 }
-
-// TestTwoMetricsRoutersInOneProcess pins
-// https://git.eeqj.de/sneak/webhooker/issues/227: a second
-// metrics-enabled router in one process used to panic, because the
-// HTTP metrics registered on Prometheus's global default registry.
-// Two routers are built over separate dependency graphs and a third
-// over the first graph again, and each must still serve the HTTP,
-// delivery and Go runtime series.
-func TestTwoMetricsRoutersInOneProcess(t *testing.T) {
-	t.Parallel()
-
-	first := newTestEnvWithConfig(
-		t, metricsConfig(t, metricsUser, metricsAuthValue),
-	)
-	second := newTestEnvWithConfig(
-		t, metricsConfig(t, metricsUser, metricsAuthValue),
-	)
-	third := &testEnv{
-		router: server.NewRouterForTest(
-			first.log.Get(), first.cfg, first.mw, first.hnd,
-		),
-	}
-
-	for _, env := range []*testEnv{first, second, third} {
-		env.get("/", nil)
-
-		scrape := env.metricsRequest(metricsUser, metricsAuthValue)
-		require.Equal(t, http.StatusOK, scrape.Code)
-
-		for _, series := range []string{
-			"http_request_duration_seconds",
-			"http_response_size_bytes",
-			"http_requests_inflight",
-			"webhooker_events_received_total",
-			"go_goroutines",
-		} {
-			assert.Contains(t, scrape.Body.String(), series)
-		}
-	}
-}