Make SQLite durable under concurrent readers and stop re-delivering stranded webhooks (closes #256)

An operator running `sqlite3 <db> .dump` against their own per-webhook
database wedged it: inbound webhooks rejected with HTTP 500, delivered
webhooks stranded at `pending`, and every one of them POSTed a second
time on the next restart while the event log recorded a single attempt.

Durability. Every SQLite file — main, per-webhook, and archive — now
opens through one path, `internal/database/sqlite_open.go`, in WAL
journal mode with a 10-second busy timeout, `BEGIN IMMEDIATE`
transactions, and a bounded connection pool. WAL is what stops a reader
blocking writers at all. `_txlock=immediate` is what stops a `COMMIT`
failing while its transaction stays open on a pooled connection, which
is how four `database is locked` errors became 593 `cannot start a
transaction within a transaction`. `cache=shared` is gone, because
under it an in-process conflict is SQLITE_LOCKED, which the busy
handler does not retry. The busy timeout is applied before
journal_mode: the driver runs DSN pragmas in order on every new
connection, and `PRAGMA journal_mode` takes a lock, so the reverse
order leaves the one pragma that can block uncovered by the handler
meant to cover it.

Eligibility. `internal/delivery/inflight.go` holds the set of
deliveries the engine owns — taken when a task is queued, when a
target schedules a retry, and by every recovery path before it
re-dispatches; dropped when the worker that ran the task returns.
Recovery and both sweep arms re-dispatch only what the set does not
hold. Nothing decides that from a row's age: a delivery waiting in a
10000-deep channel is arbitrarily old and perfectly healthy, and
reasoning from age re-sends it. `takeForRedispatch` is the single gate
every re-dispatch goes through — ownership first, then a conditional
update confirming the row is still in the status the batch read.

Bookkeeping. `recordResult` and `updateDeliveryStatus` return their
errors instead of logging and dropping them, and a caller whose
bookkeeping write failed writes nothing at all: the delivery keeps
whichever non-terminal status it already held, and the sweeps recover
it. Every recovery path — pending and retrying alike — first settles
any delivery that already holds a successful `DeliveryResult` rather
than sending it again. Recovery continues each delivery's own attempt
numbering instead of restarting at 1. The sweep gains a
`pending`-with-age-bound arm, so a stranded delivery no longer waits
for a restart.

Docs. WAL produces `-wal`/`-shm` sidecars, so the backup and restore
procedures in README.md are corrected against measurement: both
documented procedures were re-run against a live instance, a `-wal`
left by a crash carries data the `.db` alone does not, and an archive
file normally holds its rows in a `-wal` rather than in the `.db`.

README.md

@@ -573,11 +573,18 @@ so while the service is running each `{name}.db` has a `{name}.db-wal`
 and a `{name}.db-shm` beside it. **`-wal` is part of the database, not a
 scratch file**: it holds committed transactions that are not yet in the
 `.db`, so a copy of the `.db` without its `-wal` is missing data and may
-have no readable schema at all. A clean shutdown checkpoints and removes
+have no readable schema at all. `-shm` is regenerable, but there is no
-both sidecars, so a stopped deployment has none; a killed or crashed one
+reason to separate the two — copy the directory and you have them.
-leaves them, and they must be carried with the `.db`. `-shm` is
+
-regenerable, but there is no reason to separate the two — copy the
+A clean shutdown closes `webhooker.db` and every `events-*.db`, which
-directory.
+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
@@ -633,17 +640,30 @@ run — it does not block ingestion — but back up with `.backup` or a
 stopped copy.
 
 Archive databases are the one exception the service is built for: the
-archive writer closes its handle after each write (debounced to at most
+archive writer closes and reopens its handle around writes (debounced
-one reopen per second), so an operator can move `archive-{uuid}.db`
+to at most one reopen per second), so an operator can move
-away for offline retention while the service runs, and it is recreated
+`archive-{uuid}.db` away for offline retention while the service runs,
-on the next write. Closing the handle checkpoints and removes that
+and it is recreated on the next write. See
-file's sidecars, so what is left to move is a single self-contained
-`.db` — but if a `-wal` is there, a write is in flight, and it has to
-move with it. See
 [Database Architecture](#database-architecture). That is a
 move-the-file-away workflow, not a substitute for the backup procedures
 above.
 
+**Move the sidecars with it.** Under WAL that workflow is no longer a
+single file, and the common case is the dangerous one. The reopen
+happens on the *next* write after the debounce window elapses, so after
+the last write of a burst nothing checkpoints: measured, 20 s after ten
+events the `archive-….db` was 4096 bytes — a header, no table — with
+all ten rows sitting in a 189 KB `-wal`. Copying the `.db` alone at that
+moment yields a file that opens with `no such table: archived_events`.
+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.
+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
 
 1. Stop the service.
@@ -659,10 +679,14 @@ above.
 
 3. Carry any `*.db-wal` and `*.db-shm` files that are in the backup.
    They are part of the database, and dropping a `-wal` silently
-   discards every transaction it still holds. Backups taken by either
+   discards every transaction it still holds. An `.backup` set will not
-   procedure above will not contain them — `.backup` writes a single
+   contain any: it writes a single consolidated file per database. A
-   consolidated file, and a clean stop checkpoints the sidecars away —
+   stop-and-copy set has none for `webhooker.db` or the `events-*.db`,
-   but a copy salvaged from a crashed instance will, and it needs them.
+   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
@@ -1704,6 +1728,40 @@ 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.
 
+#### Inbound HTTP metrics
+
+The middleware records three more on the same registry:
+
+| Metric | Type | Labels |
+| ------ | ---- | ------ |
+| `http_request_duration_seconds` | histogram | `service`, `handler`, `method`, `code` |
+| `http_response_size_bytes` | histogram | `service`, `handler`, `method`, `code` |
+| `http_requests_inflight` | gauge | `service`, `handler` |
+
+Two of those labels are written once per request from bytes the client
+chose, so both are bounded to something this service registers:
+
+- `handler` is the chi route pattern — `/webhook/{uuid}`, never the
+  concrete path. A request matching no route carries `(unmatched)`,
+  and no entrypoint UUID ever reaches a label.
+- `method` is the request method when the router can route it, and
+  `(unmatched)` otherwise. `net/http` accepts any RFC 9110 token as a
+  method, so the raw value bounds the label at nothing; the nine chi
+  matches routes for stay distinguishable, and a token that could only
+  ever have produced a 405 does not get a series of its own.
+
+The other two are not request-controlled: `code` is the status one of
+this service's own handlers wrote, and `service` is a fixed empty
+string.
+
+`http_requests_inflight` is deliberately aggregate — its `handler` is
+always `(all)`, one series counting the requests in flight across the
+whole service. The gauge is incremented before routing and decremented
+after the handler returns, and the route pattern exists only between
+those two moments, so labelling it by pattern would increment one
+series and decrement another, leaving every pattern permanently off by
+the number of requests it served.
+
 ### Rate Limiting
 
 Global blanket rate limiting middleware (e.g., a per-IP throttle shared

internal/database/sqlite_open.go

@@ -108,11 +108,20 @@ const (
 // synchronous is deliberately left at SQLite's default of FULL: this
 // is a webhook receiver whose one promise is that an event it answered
 // 200 for is durable.
+// The order of the _pragma parameters is load-bearing.
+// modernc.org/sqlite executes them in the order they appear, on every
+// new connection, before the connection is handed to the pool. Setting
+// journal_mode first means that pragma itself runs with no busy
+// handler installed: the pool opens connections lazily, so the moment
+// a new one is created is a moment the database is under load, and
+// PRAGMA journal_mode takes a lock. It would fail immediately with
+// SQLITE_BUSY and fail the query that caused the connection to be
+// opened. busy_timeout is therefore set first, so every pragma after
+// it — and the whole life of the connection — is covered.
 func SQLiteDSN(path, mode string) string {
 	q := url.Values{}
 	q.Set("mode", mode)
 	q.Set("_txlock", "immediate")
-	q.Add("_pragma", "journal_mode(WAL)")
 	q.Add(
 		"_pragma",
 		fmt.Sprintf(
@@ -120,6 +129,7 @@ func SQLiteDSN(path, mode string) string {
 			SQLiteBusyTimeout.Milliseconds(),
 		),
 	)
+	q.Add("_pragma", "journal_mode(WAL)")
 
 	return "file:" + path + "?" + q.Encode()
 }

internal/database/sqlite_open_test.go

@@ -40,6 +40,17 @@ func TestSQLiteDSNCarriesTheDurabilitySettings(t *testing.T) {
 	assert.Contains(t, dsn, "_txlock=immediate")
 	assert.Contains(t, dsn, "mode=rwc")
 
+	// busy_timeout must come first. The driver runs these in order on
+	// every new connection, and PRAGMA journal_mode takes a lock — a
+	// connection opened while the database is busy would fail on that
+	// pragma, with no busy handler yet installed to wait it out.
+	assert.Less(
+		t,
+		strings.Index(dsn, "busy_timeout"),
+		strings.Index(dsn, "journal_mode"),
+		"busy_timeout must be applied before journal_mode",
+	)
+
 	// cache=shared turns an in-process conflict into SQLITE_LOCKED,
 	// which the busy handler does not retry. It must never come back.
 	// See https://git.eeqj.de/sneak/webhooker/issues/256.

internal/delivery/engine.go

@@ -41,18 +41,23 @@ const (
 	// sweep runs.
 	retrySweepInterval = 60 * time.Second
 
-	// pendingSweepMinAge is how long a delivery must have sat at
+	// pendingSweepMinAge is how long a delivery must have sat
-	// pending before the sweep treats it as stranded rather than as
+	// untouched at pending before the sweep will look at it.
-	// in flight.
 	//
-	// A delivery is pending from the moment it is created until its
+	// It is not what keeps the sweep off live work — inflightSet is,
-	// outcome is written, which includes the whole time a worker
+	// and it is exact. This bound sets the re-dispatch cadence for a
-	// spends on it, so the bound has to clear the longest a live
+	// delivery that really is stranded: without it, a delivery the
-	// attempt can take: httpClientTimeout plus queueing behind the
+	// database will not let the engine settle would be re-sent on
-	// other deliveries in front of it. Five minutes is far above
+	// every 60-second tick.
-	// that, and still recovers a stranded delivery in minutes rather
+	//
-	// than at the next restart.
+	// It is nonetheless set clear of the longest legitimate attempt,
-	pendingSweepMinAge = 5 * time.Minute
+	// so that the two guards do not both have to be right. That
+	// length is MaxTargetTimeoutSeconds (300s), the per-target
+	// timeout the target form accepts — not httpClientTimeout, which
+	// is merely the default. Fifteen minutes leaves a margin of
+	// three times the ceiling rather than the zero margin the two
+	// equal values would have given.
+	pendingSweepMinAge = 15 * time.Minute
 
 	// pendingSweepBatch bounds how many stranded pending deliveries
 	// one sweep of one webhook re-dispatches. The sweep runs every
@@ -177,6 +182,12 @@ type Engine struct {
 	// dbTarget is retained so the engine can reach the archive
 	// writer registry for webhook eviction and the idle sweep.
 	dbTarget *databaseTarget
+
+	// inflight is the set of deliveries this engine currently owns.
+	// Recovery and the sweeps re-dispatch only what it does not
+	// hold. Held by value: its zero value works, so no constructor
+	// can leave it out. See inflight.go.
+	inflight inflightSet
 }
 
 // New creates and registers the delivery engine with the
@@ -209,12 +220,27 @@ func New(
 // are ready.
 func (e *Engine) Notify(tasks []Task) {
 	for i := range tasks {
+		// Owned before it is queued, and until the worker that runs
+		// it returns. A task can sit in a 10000-deep channel for a
+		// long time on a healthy system, and nothing may re-send it
+		// while it waits. See inflight.go.
+		if !e.inflight.retainIdle(tasks[i].DeliveryID) {
+			e.log.Warn(
+				"delivery already in flight, not queued again",
+				"delivery_id", tasks[i].DeliveryID,
+				"event_id", tasks[i].EventID,
+			)
+
+			continue
+		}
+
 		select {
 		case e.deliveryCh <- tasks[i]:
 		default:
+			e.inflight.release(tasks[i].DeliveryID)
 			e.log.Warn(
 				"delivery channel full, "+
-					"task will be recovered on restart",
+					"task will be recovered by the sweep",
 				"delivery_id", tasks[i].DeliveryID,
 				"event_id", tasks[i].EventID,
 			)
@@ -249,10 +275,20 @@ func (e *Engine) ScheduleRetry(
 		"next_attempt", task.AttemptNum,
 	)
 
+	// The reference is taken here rather than when the timer fires,
+	// so the delivery stays owned across the whole backoff window.
+	// Its caller is a target inside Deliver, so the engine already
+	// owns it; this second reference is what keeps that ownership
+	// alive after the worker returns and the row sits at retrying
+	// with nothing running. Without it the sweep finds the row
+	// orphaned and sends it again.
+	e.inflight.retain(task.DeliveryID)
+
 	time.AfterFunc(delay, func() {
 		select {
 		case e.retryCh <- task:
 		default:
+			e.inflight.release(task.DeliveryID)
 			e.log.Warn(
 				"retry channel full, delivery "+
 					"will be recovered by periodic sweep",
@@ -352,13 +388,35 @@ func (e *Engine) worker(ctx context.Context) {
 		case <-ctx.Done():
 			return
 		case task := <-e.deliveryCh:
-			e.processNewTask(ctx, &task)
+			e.runTask(ctx, &task, e.processNewTask)
 		case task := <-e.retryCh:
-			e.processRetryTask(ctx, &task)
+			e.runTask(ctx, &task, e.processRetryTask)
 		}
 	}
 }
 
+// runTask runs one task and then drops the reference the queueing
+// side took on its delivery.
+//
+// The release is deferred rather than written after the call because
+// every early return inside the processing paths must drop it too: a
+// delivery whose database could not be opened is one the engine has
+// stopped working on, and leaving it owned would hide it from the
+// sweep forever.
+//
+// Ownership does not necessarily end here. A target that scheduled a
+// retry took its own reference before this one is dropped, so the
+// delivery stays owned through the backoff window.
+func (e *Engine) runTask(
+	ctx context.Context,
+	task *Task,
+	run func(context.Context, *Task),
+) {
+	defer e.inflight.release(task.DeliveryID)
+
+	run(ctx, task)
+}
+
 func (e *Engine) recoverPending(ctx context.Context) {
 	defer e.wg.Done()
 
@@ -546,7 +604,16 @@ func (e *Engine) recoverRetryingDeliveries(
 		return
 	}
 
+	settled := e.reconcileDelivered(
+		webhookDB, webhookID, retrying,
+		e.loadTargetMap(retrying),
+	)
+
 	for i := range retrying {
+		if _, ok := settled[retrying[i].ID]; ok {
+			continue
+		}
+
 		e.recoverSingleRetry(
 			webhookDB, webhookID, &retrying[i],
 		)
@@ -608,6 +675,10 @@ func (e *Engine) recoverSingleRetry(
 		d, webhookID, &event, &target, attemptNum+1,
 	)
 
+	if !e.rescheduleRecovered(webhookDB, task, remaining) {
+		return
+	}
+
 	e.log.Info(
 		"recovering retrying delivery",
 		"webhook_id", webhookID,
@@ -615,8 +686,6 @@ func (e *Engine) recoverSingleRetry(
 		"attempt", attemptNum,
 		"remaining_backoff", remaining,
 	)
-
-	e.ScheduleRetry(task, remaining)
 }
 
 func (e *Engine) recoverPendingDeliveries(
@@ -626,12 +695,14 @@ func (e *Engine) recoverPendingDeliveries(
 ) {
 	var deliveries []database.Delivery
 
+	// No Preload: event bodies are read one at a time in
+	// sendRecoveredDeliveries, and only for the deliveries actually
+	// being sent.
 	result := webhookDB.
 		Where(
 			"status = ?",
 			database.DeliveryStatusPending,
 		).
-		Preload("Event").
 		Find(&deliveries)
 
 	if result.Error != nil {
@@ -681,19 +752,26 @@ func (e *Engine) recoverPendingBatch(
 	)
 }
 
-// reconcileDelivered finds the deliveries in a pending batch that
+// reconcileDelivered finds the deliveries in a recovered batch that
 // already have a successful DeliveryResult, marks them delivered, and
 // returns their ids so the caller does not send them a second time.
 //
 // This is the state the engine previously had no way to represent. A
-// delivery is left pending by a failed bookkeeping write, and that
+// delivery is left in a non-terminal state by a failed bookkeeping
-// covers two different histories: nothing was ever sent, or the send
+// write, and that covers two different histories: nothing was ever
-// reached the receiver and only the status write failed. Re-sending
+// sent, or the send reached the receiver and only the status write
-// was the sole option, so every stranded row produced a duplicate at
+// failed. Re-sending was the sole option, so every stranded row
-// the receiver and an event log that recorded one attempt for two
+// produced a duplicate at the receiver and an event log that recorded
-// POSTs. A successful result row distinguishes them: it is written
+// one attempt for two POSTs. A successful result row distinguishes
-// before the status, so its presence means the wire I/O happened and
+// them: it is written before the status, so its presence means the
-// was recorded, and all that is missing is the status.
+// wire I/O happened and was recorded, and all that is missing is the
+// status.
+//
+// Every recovery path runs this, not only the pending one. A delivery
+// abandoned at retrying can hold a successful result just as a pending
+// one can — a second attempt that reached the receiver and whose status
+// write then failed sits at retrying with success recorded — and
+// re-sending it is the same duplicate.
 //
 // Deliveries whose result row itself never landed are not in the
 // returned set and are re-sent, recorded as the further attempt they
@@ -749,7 +827,7 @@ func (e *Engine) reconcileDelivered(
 	}
 
 	e.log.Info(
-		"settling pending deliveries that already succeeded",
+		"settling recovered deliveries that already succeeded",
 		"webhook_id", webhookID,
 		"count", len(settled),
 	)
@@ -759,12 +837,22 @@ func (e *Engine) reconcileDelivered(
 			continue
 		}
 
+		// A delivery the engine is working on right now settles
+		// itself; writing over it from here would race that worker.
+		if !e.inflight.retainIdle(deliveries[i].ID) {
+			delete(settled, deliveries[i].ID)
+
+			continue
+		}
+
 		e.settleStatus(
 			webhookDB,
 			&deliveries[i],
 			targetMap[deliveries[i].TargetID].Type,
 			database.DeliveryStatusDelivered,
 		)
+
+		e.inflight.release(deliveries[i].ID)
 	}
 
 	return settled
@@ -851,6 +939,11 @@ func (e *Engine) sweepWebhookRetries(
 		return
 	}
 
+	settled := e.reconcileDelivered(
+		webhookDB, webhookID, retrying,
+		e.loadTargetMap(retrying),
+	)
+
 	for i := range retrying {
 		select {
 		case <-ctx.Done():
@@ -858,6 +951,10 @@ func (e *Engine) sweepWebhookRetries(
 		default:
 		}
 
+		if _, ok := settled[retrying[i].ID]; ok {
+			continue
+		}
+
 		e.sweepSingleRetry(
 			webhookDB, webhookID, &retrying[i],
 		)
@@ -869,20 +966,18 @@ func (e *Engine) sweepWebhookRetries(
 // sweepWebhookPending recovers deliveries stranded at pending.
 //
 // A delivery is created pending and leaves that state only when its
-// outcome is written, so a pending row older than the age bound is one
+// outcome is written, so a pending row the engine does not own is one
 // whose bookkeeping write failed — the state that used to sit there
 // until a restart, and then produce a duplicate at the receiver. The
 // sweep gives it the same reconcile-then-dispatch treatment restart
 // recovery gets, so it costs a minute rather than an operator
 // noticing.
 //
-// The age bound is what keeps the sweep off deliveries the workers
+// What keeps the sweep off live work is ownership, checked per
-// still hold: a delivery in flight is pending too, and re-dispatching
+// delivery in takeForRedispatch, not the age bound in this query.
-// one would race the worker that owns it. It is measured on updated_at
+// A delivery waiting in deliveryCh is pending and arbitrarily old —
-// rather than created_at because claimPending stamps that column when
+// the channel holds 10000 tasks and 10 workers drain it — so
-// a delivery is handed out, which is what stops the next sweep, a
+// reasoning from the row's age alone re-sends it. See inflight.go.
-// minute later, from sending the same delivery again while the first
-// attempt is still running.
 func (e *Engine) sweepWebhookPending(
 	ctx context.Context,
 	webhookDB *gorm.DB,
@@ -896,7 +991,6 @@ func (e *Engine) sweepWebhookPending(
 			database.DeliveryStatusPending,
 			time.Now().Add(-pendingSweepMinAge),
 		).
-		Preload("Event").
 		Limit(pendingSweepBatch).
 		Find(&pending).Error
 	if err != nil {
@@ -977,8 +1071,13 @@ func (e *Engine) sweepSingleRetry(
 		d, webhookID, &event, &target, attemptNum+1,
 	)
 
-	select {
+	if !e.redispatch(
-	case e.retryCh <- task:
+		e.retryCh, webhookDB, task,
+		database.DeliveryStatusRetrying,
+	) {
+		return
+	}
+
 	e.log.Info(
 		"retry sweep: "+
 			"recovered orphaned retrying delivery",
@@ -986,8 +1085,6 @@ func (e *Engine) sweepSingleRetry(
 		"webhook_id", webhookID,
 		"attempt", attemptNum+1,
 	)
-	default:
-	}
 }
 
 // failUnretryableRetry terminally fails an orphaned retrying
@@ -1010,6 +1107,16 @@ func (e *Engine) failUnretryableRetry(
 	d *database.Delivery,
 	target *database.Target,
 ) {
+	// Terminal, and reached from the recovery paths, so it takes
+	// ownership like every other write they make: a delivery the
+	// engine is still attempting must not be failed underneath the
+	// worker running it.
+	if !e.inflight.retainIdle(d.ID) {
+		return
+	}
+
+	defer e.inflight.release(d.ID)
+
 	e.log.Warn(
 		"failing orphaned retrying delivery: target "+
 			"type no longer supports retries",
@@ -1203,7 +1310,13 @@ func (e *Engine) updateDeliveryStatus(
 		)
 	}
 
+	// An empty type means the target row is gone — a delivery being
+	// settled long after its target was deleted. The row still has to
+	// be settled, but the counter is left alone rather than given a
+	// series labelled with the empty string.
+	if targetType != "" {
 		e.mtr.DeliveryStatusChanged(targetType, status)
+	}
 
 	return nil
 }
@@ -1317,44 +1430,133 @@ func (e *Engine) countAttempts(
 	return int(resultCount)
 }
 
-// claimPending takes ownership of a pending delivery before it is
+// takeForRedispatch decides whether a recovered delivery may be sent
-// re-dispatched, and reports whether the claim succeeded.
+// again, and takes it if so. It is the single gate every re-dispatch
+// path goes through, and it asks two separate questions in order.
 //
-// The claim is a compare-and-set on the status: it takes effect only
+// First, does the engine already own this delivery? Ownership is
-// while the delivery is still pending, so a worker that settled the
+// exact and mutually exclusive, so a delivery queued, being attempted,
-// delivery between the query and here wins and nothing is re-sent.
+// or waiting out a retry backoff is refused here, and two dispatchers
-// Stamping updated_at is the claim itself — the sweep selects on that
+// racing for the same delivery cannot both win. See inflight.go.
-// column, so a delivery handed out now is out of the sweep's reach for
-// a further pendingSweepMinAge, rather than being sent again on every
-// sweep for as long as the attempt takes.
 //
-// A claim that cannot be written means the database is refusing
+// Second, is the row still in the status that made it eligible? The
-// writes, which is the condition that stranded this delivery in the
+// batch was read some time ago and a worker may have settled a row
-// first place. Not sending is then the right answer: the attempt
+// since. The check is a conditional update rather than a read so the
-// could not be recorded either, and an unrecordable send is exactly
+// answer cannot go stale between asking and acting.
-// the duplicate this issue is about.
+//
-func (e *Engine) claimPending(
+// Stamping updated_at is the same statement, and it is a cadence
-	webhookDB *gorm.DB, d *database.Delivery,
+// control rather than a claim: the pending sweep selects on that
+// column, so a delivery handed out now is not selected again on the
+// next tick a minute later but after pendingSweepMinAge. A delivery
+// the database refuses to settle is therefore retried on that
+// interval instead of every tick.
+//
+// A failed write is a refusal. It means the database is not accepting
+// writes, which is the condition that stranded the delivery in the
+// first place; an attempt that cannot be recorded is exactly the
+// unlogged duplicate this is all here to prevent.
+//
+// The caller must release ownership if it then fails to queue the
+// task.
+func (e *Engine) takeForRedispatch(
+	webhookDB *gorm.DB,
+	deliveryID string,
+	eligible database.DeliveryStatus,
 ) bool {
+	if !e.inflight.retainIdle(deliveryID) {
+		return false
+	}
+
 	res := webhookDB.
 		Model(&database.Delivery{}).
 		Where(
-			"id = ? AND status = ?",
+			"id = ? AND status = ?", deliveryID, eligible,
-			d.ID, database.DeliveryStatusPending,
 		).
 		UpdateColumn("updated_at", time.Now())
+
 	if res.Error != nil {
 		e.log.Error(
-			"failed to claim pending delivery for recovery; "+
+			"failed to mark delivery for re-dispatch; "+
-				"leaving it for the next sweep",
+				"leaving it for a later sweep",
-			"delivery_id", d.ID,
+			"delivery_id", deliveryID,
 			"error", res.Error,
 		)
+		e.inflight.release(deliveryID)
 
 		return false
 	}
 
-	return res.RowsAffected == 1
+	if res.RowsAffected != 1 {
+		// Settled underneath us between the query and here.
+		e.inflight.release(deliveryID)
+
+		return false
+	}
+
+	return true
+}
+
+// queueRecovered puts an owned delivery's task on a worker channel,
+// dropping the ownership the gate took if it does not fit.
+func (e *Engine) queueRecovered(
+	ch chan<- Task, task Task,
+) bool {
+	select {
+	case ch <- task:
+		return true
+	default:
+		e.inflight.release(task.DeliveryID)
+		e.log.Warn(
+			"worker channel full during recovery; "+
+				"delivery will be recovered by a later sweep",
+			"delivery_id", task.DeliveryID,
+			"webhook_id", task.WebhookID,
+		)
+
+		return false
+	}
+}
+
+// redispatch hands a recovered delivery to a worker channel through
+// takeForRedispatch, and reports whether the task was queued.
+func (e *Engine) redispatch(
+	ch chan<- Task,
+	webhookDB *gorm.DB,
+	task Task,
+	eligible database.DeliveryStatus,
+) bool {
+	if !e.takeForRedispatch(
+		webhookDB, task.DeliveryID, eligible,
+	) {
+		return false
+	}
+
+	return e.queueRecovered(ch, task)
+}
+
+// rescheduleRecovered hands an orphaned retrying delivery back to the
+// retry timer, through the same gate. It reports whether the delivery
+// was rescheduled.
+//
+// The reference taken by the gate is dropped as soon as ScheduleRetry
+// has taken its own, which it does before returning: what keeps the
+// delivery owned through the backoff window is ScheduleRetry's
+// reference, not this one.
+func (e *Engine) rescheduleRecovered(
+	webhookDB *gorm.DB, task Task, delay time.Duration,
+) bool {
+	if !e.takeForRedispatch(
+		webhookDB, task.DeliveryID,
+		database.DeliveryStatusRetrying,
+	) {
+		return false
+	}
+
+	defer e.inflight.release(task.DeliveryID)
+
+	e.ScheduleRetry(task, delay)
+
+	return true
 }
 
 // countAttemptsBatch counts the recorded attempts of every delivery
@@ -1473,6 +1675,10 @@ func buildRecoveryTask(
 func (e *Engine) loadTargetMap(
 	deliveries []database.Delivery,
 ) map[string]database.Target {
+	if len(deliveries) == 0 {
+		return nil
+	}
+
 	seen := make(map[string]bool)
 
 	targetIDs := make([]string, 0, len(deliveries))
@@ -1512,6 +1718,15 @@ func (e *Engine) loadTargetMap(
 // sendRecoveredDeliveries re-dispatches pending deliveries, skipping
 // the ids in settled — those already reached their receiver and have
 // been marked delivered by reconcileDelivered.
+//
+// The skip and takeForRedispatch's status check answer different
+// questions and neither replaces the other. This one is "did this
+// delivery already succeed", which is what settles the row to
+// delivered instead of sending it, and which is the only thing that
+// keeps the retrying paths from terminally failing a delivery that
+// reconcile just settled. The status check is "is the row still what
+// the batch query said it was", which catches a worker settling it to
+// anything at all in between.
 func (e *Engine) sendRecoveredDeliveries(
 	ctx context.Context,
 	webhookDB *gorm.DB,
@@ -1549,27 +1764,40 @@ func (e *Engine) sendRecoveredDeliveries(
 			continue
 		}
 
-		if !e.claimPending(webhookDB, &deliveries[i]) {
+		if !e.takeForRedispatch(
+			webhookDB, deliveries[i].ID,
+			database.DeliveryStatusPending,
+		) {
+			continue
+		}
+
+		// The body is read here, one delivery at a time and only for
+		// deliveries that are actually being sent, rather than
+		// preloaded across the whole batch. A batch is up to
+		// pendingSweepBatch rows at up to the 1 MB ingest cap, and
+		// most of a sweep's batch is refused by the gate above — so
+		// preloading would hold hundreds of megabytes per webhook per
+		// tick to build tasks it then discards.
+		event, err := e.loadEvent(
+			webhookDB, deliveries[i].EventID,
+		)
+		if err != nil {
+			e.log.Error(
+				"failed to load event for recovered delivery",
+				"delivery_id", deliveries[i].ID,
+				"event_id", deliveries[i].EventID,
+				"error", err,
+			)
+			e.inflight.release(deliveries[i].ID)
+
 			continue
 		}
 
 		task := buildRecoveryTask(
-			&deliveries[i], webhookID,
+			&deliveries[i], webhookID, &event, &target,
-			&deliveries[i].Event, &target,
 			attempts[deliveries[i].ID]+1,
 		)
 
-		select {
+		e.queueRecovered(e.deliveryCh, task)
-		case e.deliveryCh <- task:
-		default:
-			e.log.Warn(
-				"delivery channel full during "+
-					"recovery, remaining deliveries "+
-					"will be recovered on next restart",
-				"delivery_id", deliveries[i].ID,
-			)
-
-			return
-		}
 	}
 }

internal/delivery/export_test.go

@@ -291,6 +291,26 @@ func (e *Engine) ExportWedgeWorker(release <-chan struct{}) {
 	})
 }
 
+// ExportInflightHeld reports how many deliveries the engine currently
+// owns, so a test can prove ownership is released rather than leaked.
+func (e *Engine) ExportInflightHeld() int {
+	return e.inflight.held()
+}
+
+// ExportRetainDelivery takes the first reference on a delivery, as the
+// queueing side does. It lets a test put a delivery into the state a
+// worker or a full channel would, without running the pool.
+func (e *Engine) ExportRetainDelivery(deliveryID string) bool {
+	return e.inflight.retainIdle(deliveryID)
+}
+
+// ExportRecoverRetryingDeliveries exposes recoverRetryingDeliveries.
+func (e *Engine) ExportRecoverRetryingDeliveries(
+	webhookDB *gorm.DB, webhookID string,
+) {
+	e.recoverRetryingDeliveries(webhookDB, webhookID)
+}
+
 // ExportDeliveryCh returns the delivery channel.
 func (e *Engine) ExportDeliveryCh() chan Task {
 	return e.deliveryCh

internal/delivery/inflight.go

@@ -0,0 +1,110 @@
+package delivery
+
+import "sync"
+
+// inflightSet records which deliveries the engine currently owns.
+//
+// A delivery is owned from the moment a task for it is handed to a
+// channel or to a retry timer until the engine has no further plan for
+// it in memory. Restart recovery and both arms of the periodic sweep
+// re-dispatch only deliveries the set does not hold, which is what
+// makes them exact rather than a guess about how long a row has sat at
+// pending.
+//
+// This replaces reasoning from timestamps. A delivery's row says
+// pending from creation until its outcome is written, which covers
+// four different situations — never dispatched, waiting in a channel,
+// being attempted right now, and genuinely stranded — and no column
+// distinguishes them. Only the engine knows which, and it knows
+// exactly. `deliveryChannelSize` is 10000 against 10 workers, so a
+// perfectly healthy delivery can wait far longer than any age bound
+// worth setting before its attempt even begins; an age bound alone
+// re-sends it. See
+// https://git.eeqj.de/sneak/webhooker/issues/256.
+//
+// In-memory state is sufficient because a data directory admits one
+// process: internal/datadir takes an flock on it at startup and a
+// second instance refuses to run. Deliveries owned by a process that
+// died are not in any successor's set, and restart recovery is what
+// picks those up.
+//
+// References are counted rather than held as a plain set because
+// ownership outlives the worker that took it. A target that schedules
+// a retry from inside Deliver adds a reference while the worker still
+// holds one, so the delivery stays owned across the gap between the
+// worker returning and the timer firing — the window in which a sweep
+// would otherwise find the row at retrying and send it again.
+//
+// The zero value is ready to use, and the Engine holds one by value.
+// That is deliberate: an engine built by a constructor that forgot to
+// initialise this would not refuse to re-dispatch anything, and the
+// symptom would be duplicate deliveries rather than a failure anybody
+// notices.
+type inflightSet struct {
+	mu  sync.Mutex
+	ids map[string]int
+}
+
+// retain adds a reference to a delivery the caller already knows the
+// engine owns, so that ownership survives the current holder letting
+// go. It cannot fail.
+func (s *inflightSet) retain(deliveryID string) {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+
+	if s.ids == nil {
+		s.ids = make(map[string]int)
+	}
+
+	s.ids[deliveryID]++
+}
+
+// retainIdle takes the first reference on a delivery, and reports
+// whether it got it. It fails when the engine already owns the
+// delivery, which is what makes two claimants — restart recovery and
+// the sweep run concurrently, or two sweep arms — mutually exclusive
+// rather than merely atomic.
+func (s *inflightSet) retainIdle(deliveryID string) bool {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+
+	if s.ids[deliveryID] > 0 {
+		return false
+	}
+
+	if s.ids == nil {
+		s.ids = make(map[string]int)
+	}
+
+	s.ids[deliveryID] = 1
+
+	return true
+}
+
+// release drops one reference. The delivery becomes eligible for
+// re-dispatch again once the last one goes.
+func (s *inflightSet) release(deliveryID string) {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+
+	n := s.ids[deliveryID] - 1
+	if n <= 0 {
+		delete(s.ids, deliveryID)
+
+		return
+	}
+
+	s.ids[deliveryID] = n
+}
+
+// held reports how many deliveries the engine currently owns. It
+// exists so a test can assert that ownership is released rather than
+// leaked: a reference that is never dropped hides its delivery from
+// every sweep for the life of the process, which is the one way this
+// mechanism can fail silently.
+func (s *inflightSet) held() int {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+
+	return len(s.ids)
+}

internal/delivery/inflight_test.go

@@ -0,0 +1,428 @@
+package delivery_test
+
+import (
+	"context"
+	"path/filepath"
+	"strings"
+	"sync"
+	"testing"
+	"time"
+
+	"github.com/google/uuid"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/webhooker/internal/database"
+	"sneak.berlin/go/webhooker/internal/delivery"
+)
+
+// These tests pin the rule that decides whether a delivery may be
+// handed back to a worker: the engine re-dispatches only what it does
+// not already own. Age alone is not that rule — a healthy delivery
+// waiting in a 10000-deep channel is old and must not be re-sent. See
+// https://git.eeqj.de/sneak/webhooker/issues/256.
+
+// fSweepSetup seeds the main database with the webhook row the sweep
+// enumerates, and returns the setup.
+func fSweepSetup(
+	t *testing.T, targetID, name string,
+) iSetup {
+	t.Helper()
+
+	s := newISetup(t)
+
+	iCreateTarget(t, s.MainDB, targetID,
+		s.WebhookID, name,
+		database.TargetTypeLog, "", 0,
+	)
+
+	require.NoError(t, s.MainDB.Create(&database.Webhook{
+		BaseModel: database.BaseModel{ID: s.WebhookID},
+		UserID:    uuid.New().String(),
+		Name:      name,
+	}).Error)
+
+	return s
+}
+
+// fDrain collects every task the engine has queued.
+//
+// Every caller drives the dispatch paths synchronously and has already
+// waited for them to return, so anything they queued is in the channel
+// by now. The short grace covers nothing but scheduler jitter, and is
+// kept small because one of these tests runs the drain forty times.
+func fDrain(e *delivery.Engine) []delivery.Task {
+	var out []delivery.Task
+
+	for {
+		select {
+		case task := <-e.ExportDeliveryCh():
+			out = append(out, task)
+		case task := <-e.ExportRetryCh():
+			out = append(out, task)
+		case <-time.After(25 * time.Millisecond):
+			return out
+		}
+	}
+}
+
+// TestArchiveHandleIsWAL closes the last gap in the durability
+// evidence: the main and per-webhook tiers each assert their journal
+// mode on a live handle, and the archive tier gets its settings from
+// the same code path but nothing checked the running file.
+func TestArchiveHandleIsWAL(t *testing.T) {
+	t.Parallel()
+
+	w := delivery.NewExportArchiveWriter(
+		filepath.Join(t.TempDir(), "archive-wal.db"),
+		archiveTestLogger(), 0,
+	)
+
+	require.NoError(t, w.Open(0))
+
+	var mode string
+
+	row := w.DB().Raw("pragma journal_mode").Row()
+	require.NoError(t, row.Scan(&mode))
+	assert.Equal(t, "wal", strings.ToLower(mode))
+
+	var busy string
+
+	row = w.DB().Raw("pragma busy_timeout").Row()
+	require.NoError(t, row.Scan(&busy))
+	assert.Equal(t, "10000", busy)
+}
+
+// TestSweepLeavesAQueuedDeliveryAlone is the case the age bound cannot
+// see. The delivery is queued and untouched, so its row is arbitrarily
+// old and still perfectly healthy; only ownership distinguishes it
+// from a stranded one.
+func TestSweepLeavesAQueuedDeliveryAlone(t *testing.T) {
+	t.Parallel()
+
+	targetID := uuid.New().String()
+	s := fSweepSetup(t, targetID, "queued")
+
+	event := iSeedEvent(
+		t, s.WebhookDB, s.WebhookID, `{"queued":true}`,
+	)
+
+	d := iSeedDelivery(
+		t, s.WebhookDB, event.ID, targetID,
+		database.DeliveryStatusPending,
+	)
+	rAgePending(t, s.WebhookDB, d.ID)
+
+	// Queued exactly as the receiver queues it, and never dequeued:
+	// no workers are running in this engine.
+	s.Engine.Notify([]delivery.Task{{
+		DeliveryID: d.ID,
+		EventID:    event.ID,
+		WebhookID:  s.WebhookID,
+		TargetID:   targetID,
+	}})
+
+	require.Equal(t, 1, s.Engine.ExportInflightHeld())
+
+	s.Engine.ExportSweepWebhookRetries(
+		context.Background(), s.WebhookID,
+	)
+
+	tasks := fDrain(s.Engine)
+	assert.Len(
+		t, tasks, 1,
+		"the sweep must not queue a delivery that is "+
+			"already waiting for a worker",
+	)
+}
+
+// TestRecoveryAndSweepDoNotDoubleDispatch drives the two entry points
+// the engine starts concurrently against one aged pending row. Before
+// ownership they both dispatched it.
+func TestRecoveryAndSweepDoNotDoubleDispatch(t *testing.T) {
+	t.Parallel()
+
+	targetID := uuid.New().String()
+	s := fSweepSetup(t, targetID, "racing")
+
+	event := iSeedEvent(
+		t, s.WebhookDB, s.WebhookID, `{"racing":true}`,
+	)
+
+	ctx := context.Background()
+
+	for range 40 {
+		d := iSeedDelivery(
+			t, s.WebhookDB, event.ID, targetID,
+			database.DeliveryStatusPending,
+		)
+		rAgePending(t, s.WebhookDB, d.ID)
+
+		var wg sync.WaitGroup
+
+		wg.Go(func() {
+			s.Engine.ExportRecoverPendingDeliveries(
+				ctx, s.WebhookDB, s.WebhookID,
+			)
+		})
+		wg.Go(func() {
+			s.Engine.ExportSweepWebhookRetries(
+				ctx, s.WebhookID,
+			)
+		})
+		wg.Wait()
+
+		tasks := fDrain(s.Engine)
+		require.Len(
+			t, tasks, 1,
+			"delivery %s dispatched %d times",
+			d.ID, len(tasks),
+		)
+
+		// No worker runs in this engine, so the reference the winner
+		// took is never released and earlier iterations' deliveries
+		// stay owned — which is itself the property under test, since
+		// both paths see them on every subsequent pass.
+	}
+}
+
+// TestConcurrentClaimsOfOneDeliveryYieldOneOwner exercises the
+// exclusion directly, rather than arguing it from a SQL predicate.
+func TestConcurrentClaimsOfOneDeliveryYieldOneOwner(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	eng := newISetup(t).Engine
+	deliveryID := uuid.New().String()
+
+	var (
+		wg  sync.WaitGroup
+		mu  sync.Mutex
+		won int
+	)
+
+	for range 64 {
+		wg.Go(func() {
+			if eng.ExportRetainDelivery(deliveryID) {
+				mu.Lock()
+				won++
+				mu.Unlock()
+			}
+		})
+	}
+
+	wg.Wait()
+
+	assert.Equal(t, 1, won)
+	assert.Equal(t, 1, eng.ExportInflightHeld())
+}
+
+// TestOwnershipIsReleasedAfterDelivery guards the other direction: a
+// leaked reference hides a delivery from every sweep for the life of
+// the process.
+func TestOwnershipIsReleasedAfterDelivery(t *testing.T) {
+	t.Parallel()
+
+	s := newISetup(t)
+	targetID := uuid.New().String()
+
+	iCreateTarget(t, s.MainDB, targetID,
+		s.WebhookID, "released",
+		database.TargetTypeLog, "", 0,
+	)
+
+	event := iSeedEvent(
+		t, s.WebhookDB, s.WebhookID, `{"released":true}`,
+	)
+
+	d := iSeedDelivery(
+		t, s.WebhookDB, event.ID, targetID,
+		database.DeliveryStatusPending,
+	)
+
+	s.Engine.ExportStart()
+
+	defer func() {
+		require.NoError(
+			t, s.Engine.ExportStop(context.Background()),
+		)
+	}()
+
+	body := `{"released":true}`
+
+	s.Engine.Notify([]delivery.Task{{
+		DeliveryID:   d.ID,
+		EventID:      event.ID,
+		WebhookID:    s.WebhookID,
+		TargetID:     targetID,
+		TargetName:   "released",
+		TargetType:   database.TargetTypeLog,
+		Body:         &body,
+		EntrypointID: event.EntrypointID,
+	}})
+
+	iWaitForDelivered(t, s.WebhookDB, d.ID)
+
+	assert.Eventually(
+		t,
+		func() bool {
+			return s.Engine.ExportInflightHeld() == 0
+		},
+		2*time.Second, 20*time.Millisecond,
+		"the delivery stayed owned after it was delivered",
+	)
+}
+
+// TestRetryingRecoverySkipsASuccessfulResult is the retrying-side twin
+// of the pending reconcile. A second attempt that reached the receiver
+// and whose status write then failed sits at retrying holding a
+// successful result, and re-sending it is the same duplicate.
+func TestRetryingRecoverySkipsASuccessfulResult(t *testing.T) {
+	t.Parallel()
+
+	targetID := uuid.New().String()
+	s := fSweepSetup(t, targetID, "retry-settled")
+
+	event := iSeedEvent(
+		t, s.WebhookDB, s.WebhookID, `{"retry":true}`,
+	)
+
+	d := iSeedDelivery(
+		t, s.WebhookDB, event.ID, targetID,
+		database.DeliveryStatusRetrying,
+	)
+	rSeedResult(t, s.WebhookDB, d.ID, 1, false)
+	rSeedResult(t, s.WebhookDB, d.ID, 2, true)
+
+	s.Engine.ExportRecoverRetryingDeliveries(
+		s.WebhookDB, s.WebhookID,
+	)
+
+	assert.Empty(
+		t, fDrain(s.Engine),
+		"a retrying delivery holding a successful result "+
+			"must not be sent again",
+	)
+
+	iAssertStatus(
+		t, s.WebhookDB, d.ID,
+		database.DeliveryStatusDelivered,
+	)
+}
+
+// TestRetryingSweepSkipsASuccessfulResult is the same rule on the
+// periodic sweep's retrying arm.
+func TestRetryingSweepSkipsASuccessfulResult(t *testing.T) {
+	t.Parallel()
+
+	targetID := uuid.New().String()
+	s := fSweepSetup(t, targetID, "retry-swept")
+
+	event := iSeedEvent(
+		t, s.WebhookDB, s.WebhookID, `{"swept":true}`,
+	)
+
+	d := iSeedDelivery(
+		t, s.WebhookDB, event.ID, targetID,
+		database.DeliveryStatusRetrying,
+	)
+	rSeedResult(t, s.WebhookDB, d.ID, 1, false)
+	rSeedResult(t, s.WebhookDB, d.ID, 2, true)
+
+	s.Engine.ExportSweepWebhookRetries(
+		context.Background(), s.WebhookID,
+	)
+
+	assert.Empty(t, fDrain(s.Engine))
+
+	iAssertStatus(
+		t, s.WebhookDB, d.ID,
+		database.DeliveryStatusDelivered,
+	)
+
+	var attempts int64
+
+	require.NoError(t, s.WebhookDB.
+		Model(&database.DeliveryResult{}).
+		Where("delivery_id = ?", d.ID).
+		Count(&attempts).Error)
+	assert.Equal(
+		t, int64(2), attempts,
+		"settling must not invent an attempt",
+	)
+}
+
+// TestScheduledRetryIsNotSweptDuringBackoff closes the window between
+// a target scheduling a retry and the timer firing. The row says
+// retrying and nothing is running, which is exactly what an orphaned
+// retry looks like from the database.
+func TestScheduledRetryIsNotSweptDuringBackoff(t *testing.T) {
+	t.Parallel()
+
+	targetID := uuid.New().String()
+	s := fSweepSetup(t, targetID, "backoff")
+
+	event := iSeedEvent(
+		t, s.WebhookDB, s.WebhookID, `{"backoff":true}`,
+	)
+
+	d := iSeedDelivery(
+		t, s.WebhookDB, event.ID, targetID,
+		database.DeliveryStatusRetrying,
+	)
+
+	s.Engine.ExportScheduleRetry(delivery.Task{
+		DeliveryID: d.ID,
+		EventID:    event.ID,
+		WebhookID:  s.WebhookID,
+		TargetID:   targetID,
+		AttemptNum: 2,
+	}, time.Hour)
+
+	require.Equal(t, 1, s.Engine.ExportInflightHeld())
+
+	s.Engine.ExportSweepWebhookRetries(
+		context.Background(), s.WebhookID,
+	)
+
+	assert.Empty(
+		t, fDrain(s.Engine),
+		"the sweep must not duplicate a retry that is "+
+			"already scheduled",
+	)
+}
+
+// TestRedispatchStampsTheRow pins the cadence control: a stranded
+// delivery that has just been handed out is not selected again by the
+// next tick a minute later.
+func TestRedispatchStampsTheRow(t *testing.T) {
+	t.Parallel()
+
+	targetID := uuid.New().String()
+	s := fSweepSetup(t, targetID, "stamped")
+
+	event := iSeedEvent(
+		t, s.WebhookDB, s.WebhookID, `{"stamped":true}`,
+	)
+
+	d := iSeedDelivery(
+		t, s.WebhookDB, event.ID, targetID,
+		database.DeliveryStatusPending,
+	)
+	rAgePending(t, s.WebhookDB, d.ID)
+
+	ctx := context.Background()
+
+	s.Engine.ExportSweepWebhookRetries(ctx, s.WebhookID)
+	require.Len(t, fDrain(s.Engine), 1)
+
+	var row database.Delivery
+
+	require.NoError(t, s.WebhookDB.
+		First(&row, "id = ?", d.ID).Error)
+	assert.WithinDuration(
+		t, time.Now(), row.UpdatedAt, time.Minute,
+		"a re-dispatched delivery must be stamped so the "+
+			"next tick does not select it again",
+	)
+}

internal/delivery/recovery_durability_test.go

@@ -163,19 +163,8 @@ func TestRecoveryContinuesTheAttemptNumbering(t *testing.T) {
 func TestSweepRecoversStrandedPending(t *testing.T) {
 	t.Parallel()
 
-	s := newISetup(t)
 	targetID := uuid.New().String()
-
+	s := fSweepSetup(t, targetID, "stranded")
-	iCreateTarget(t, s.MainDB, targetID,
-		s.WebhookID, "stranded",
-		database.TargetTypeLog, "", 0,
-	)
-
-	require.NoError(t, s.MainDB.Create(&database.Webhook{
-		BaseModel: database.BaseModel{ID: s.WebhookID},
-		UserID:    uuid.New().String(),
-		Name:      "stranded",
-	}).Error)
 
 	event := iSeedEvent(
 		t, s.WebhookDB, s.WebhookID, `{"stranded":true}`,
@@ -226,19 +215,8 @@ func TestSweepRecoversStrandedPending(t *testing.T) {
 func TestSweepClaimsAStrandedDeliveryOnlyOnce(t *testing.T) {
 	t.Parallel()
 
-	s := newISetup(t)
 	targetID := uuid.New().String()
-
+	s := fSweepSetup(t, targetID, "claimed")
-	iCreateTarget(t, s.MainDB, targetID,
-		s.WebhookID, "claimed",
-		database.TargetTypeLog, "", 0,
-	)
-
-	require.NoError(t, s.MainDB.Create(&database.Webhook{
-		BaseModel: database.BaseModel{ID: s.WebhookID},
-		UserID:    uuid.New().String(),
-		Name:      "claimed",
-	}).Error)
 
 	event := iSeedEvent(
 		t, s.WebhookDB, s.WebhookID, `{"claimed":true}`,
@@ -288,19 +266,8 @@ func TestSweepSettlesStrandedPendingWithoutResending(
 ) {
 	t.Parallel()
 
-	s := newISetup(t)
 	targetID := uuid.New().String()
-
+	s := fSweepSetup(t, targetID, "settled")
-	iCreateTarget(t, s.MainDB, targetID,
-		s.WebhookID, "settled",
-		database.TargetTypeLog, "", 0,
-	)
-
-	require.NoError(t, s.MainDB.Create(&database.Webhook{
-		BaseModel: database.BaseModel{ID: s.WebhookID},
-		UserID:    uuid.New().String(),
-		Name:      "settled",
-	}).Error)
 
 	event := iSeedEvent(
 		t, s.WebhookDB, s.WebhookID, `{"settled":true}`,

internal/delivery/target_config_view.go

@@ -23,6 +23,13 @@ type ConfigField struct {
 	Value string
 }
 
+// deletedNameSuffix marks the name of a target that no longer
+// exists. Deletes are soft and delivery history outlives the
+// target, so the event log shows names of targets that are gone;
+// an operator reading one needs to know it cannot be delivered
+// to, replayed to, or configured.
+const deletedNameSuffix = " (deleted)"
+
 // TargetView is the display-safe projection of a target for
 // the UI. It deliberately has no raw configuration field, so
 // no template — present or future — can render the stored
@@ -30,14 +37,37 @@ type ConfigField struct {
 type TargetView struct {
 	ID   string
 	Name string
+
+	// Deleted reports that this target's row is soft deleted.
+	// Only views built for historical display carry it set:
+	// every other projection is of a live row.
+	Deleted bool
+
 	Type   database.TargetType
 	Active bool
 	Config []ConfigField
 }
 
+// DisplayName is the name to render, marked when the target has
+// been deleted. Templates showing a name against historical data
+// must use it rather than Name, which stays the stored name.
+func (v TargetView) DisplayName() string {
+	if v.Deleted {
+		return v.Name + deletedNameSuffix
+	}
+
+	return v.Name
+}
+
 // NewTargetViews projects targets for rendering, replacing
 // each stored configuration blob with named, display-safe
 // fields.
+//
+// A soft-deleted row projects exactly as a live one does, minus
+// the deleted marker on its name: masking is a property of the
+// projection, not of the row's state, so a deleted target's
+// credential is as unreachable from a template as a live
+// target's.
 func NewTargetViews(
 	targets []database.Target,
 ) []TargetView {
@@ -49,6 +79,7 @@ func NewTargetViews(
 		views = append(views, TargetView{
 			ID:      t.ID,
 			Name:    t.Name,
+			Deleted: t.DeletedAt.Valid,
 			Type:    t.Type,
 			Active:  t.Active,
 			Config:  targetConfigFields(t),

internal/delivery/target_config_view_test.go

@@ -2,9 +2,11 @@ package delivery_test
 
 import (
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"gorm.io/gorm"
 	"sneak.berlin/go/webhooker/internal/database"
 	"sneak.berlin/go/webhooker/internal/delivery"
 )
@@ -17,6 +19,14 @@ const (
 	slackWebhookURL = "https://hooks.slack.com" +
 		slackSecretPath
 
+	// slackMaskedURL is what a Slack webhook URL renders as
+	// once masked: scheme and host, path elided.
+	slackMaskedURL = "https://hooks.slack.com/..."
+
+	// slackTargetName is the target name the Slack projection
+	// tests use.
+	slackTargetName = "slack-target"
+
 	viewExampleOrigin = "https://example.com"
 	viewExampleHook   = viewExampleOrigin + "/hook"
 	viewMaskedOrigin  = viewExampleOrigin + "/..."
@@ -33,7 +43,7 @@ func TestMaskedWebhookURL(t *testing.T) {
 	}{
 		"slack webhook": {
 			url:  slackWebhookURL,
-			want: "https://hooks.slack.com/...",
+			want: slackMaskedURL,
 		},
 		"query string dropped": {
 			url:  viewExampleOrigin + "/a?token=secret",
@@ -125,23 +135,61 @@ func viewFor(
 	return views[0]
 }
 
-func TestNewTargetViews_Slack(t *testing.T) {
+// TestNewTargetViews_DeletedTarget proves the projection marks
+// a soft-deleted target's name and masks its configuration by
+// the same rules a live target's is. Delivery history outlives
+// the target it names, so this projection is what an operator
+// reads about a target that no longer exists.
+func TestNewTargetViews_DeletedTarget(t *testing.T) {
 	t.Parallel()
 
-	view := viewFor(t, database.Target{
+	target := slackTarget()
-		Name:   "slack-target",
+	target.DeletedAt = gorm.DeletedAt{
+		Time:  time.Now(),
+		Valid: true,
+	}
+
+	view := viewFor(t, target)
+
+	assert.True(t, view.Deleted)
+	assert.Equal(t, slackTargetName, view.Name)
+	assert.Equal(
+		t, slackTargetName+" (deleted)", view.DisplayName(),
+	)
+	assert.Equal(
+		t,
+		map[string]string{"Webhook URL": slackMaskedURL},
+		fieldMap(view.Config),
+	)
+}
+
+// slackTarget is the live Slack target the projection tests
+// share.
+func slackTarget() database.Target {
+	return database.Target{
+		Name:   slackTargetName,
 		Type:   database.TargetTypeSlack,
 		Active: true,
 		Config: `{"webhookUrl":"` +
 			slackWebhookURL + `"}`,
-	})
+	}
+}
+
+func TestNewTargetViews_Slack(t *testing.T) {
+	t.Parallel()
+
+	view := viewFor(t, slackTarget())
+
+	assert.Equal(t, slackTargetName, view.Name)
+
+	// A live target is never marked, so the marker cannot
+	// reach a name that still exists.
+	assert.False(t, view.Deleted)
+	assert.Equal(t, slackTargetName, view.DisplayName())
 
-	assert.Equal(t, "slack-target", view.Name)
 	assert.Equal(
 		t,
-		map[string]string{
+		map[string]string{"Webhook URL": slackMaskedURL},
-			"Webhook URL": "https://hooks.slack.com/...",
-		},
 		fieldMap(view.Config),
 	)
 }
@@ -212,7 +260,7 @@ func TestNewTargetViews_HTTPMasksDestinationURL(t *testing.T) {
 
 	assert.Equal(
 		t,
-		"https://hooks.slack.com/...",
+		slackMaskedURL,
 		fields["Destination URL"],
 	)
 

internal/handlers/source_logs_deleted_target_test.go

@@ -0,0 +1,144 @@
+package handlers_test
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/webhooker/internal/database"
+	"sneak.berlin/go/webhooker/internal/handlers"
+	"sneak.berlin/go/webhooker/internal/session"
+)
+
+// deletedMarker is the suffix the event log appends to the name
+// of a target that no longer exists.
+const deletedMarker = " (deleted)"
+
+// deleteTargetThroughHandler removes a target through the real
+// deletion handler, so the test soft-deletes exactly the way the
+// UI does rather than by writing the timestamp itself.
+func deleteTargetThroughHandler(
+	t *testing.T,
+	h *handlers.Handlers,
+	sess *session.Session,
+	webhookID, targetID string,
+) {
+	t.Helper()
+
+	req := postRequest(
+		"/source/"+webhookID+"/targets/"+targetID+"/delete",
+		authenticatedCookies(
+			t, sess, deleteTestUserID, deleteTestUsername,
+		),
+		map[string]string{
+			paramSourceID: webhookID,
+			paramTargetID: targetID,
+		},
+	)
+	w := httptest.NewRecorder()
+
+	h.HandleTargetDelete().ServeHTTP(w, req)
+
+	require.Equal(t, http.StatusSeeOther, w.Code)
+}
+
+// TestHandleSourceLogs_NamesDeletedTarget proves a delivery
+// produced by a since-deleted target still names it on the event
+// log, marked as deleted.
+//
+// Deletes are soft and deliveries carry no foreign key to the
+// target row, so the history outlives the target. Against a
+// scoped lookup the delivery resolves to a zero view and the page
+// renders ": delivered" with nothing saying what it was delivered
+// to.
+func TestHandleSourceLogs_NamesDeletedTarget(t *testing.T) {
+	t.Parallel()
+
+	var (
+		h     *handlers.Handlers
+		sess  *session.Session
+		db    *database.Database
+		dbMgr *database.WebhookDBManager
+	)
+
+	app := newTestApp(t, &h, &sess, &db, &dbMgr)
+	app.RequireStart()
+
+	t.Cleanup(app.RequireStop)
+
+	wh := seedWebhook(t, db)
+	tgt := seedTarget(t, db, wh.ID, database.TargetTypeLog)
+
+	seedDeliveredEvent(t, dbMgr, wh.ID, tgt.ID)
+
+	// The control: the name is on the page while the target
+	// lives, and is not yet marked as deleted.
+	before := renderSourceLogsPage(t, h, sess, wh.ID)
+	assert.Contains(t, before, tgt.Name)
+	assert.NotContains(t, before, tgt.Name+deletedMarker)
+
+	deleteTargetThroughHandler(t, h, sess, wh.ID, tgt.ID)
+
+	after := renderSourceLogsPage(t, h, sess, wh.ID)
+
+	assert.Contains(
+		t, after, tgt.Name+deletedMarker,
+		"a delivery from a deleted target must keep its name, "+
+			"marked as no longer existing",
+	)
+	assert.Contains(
+		t, after, "delivered",
+		"the delivery history itself must survive the delete",
+	)
+}
+
+// TestHandleSourceLogs_MasksDeletedTargetConfig proves that
+// naming a deleted target does not widen what the page shows of
+// it: its stored configuration stays masked by exactly the rules
+// a live target's is.
+//
+// The lookup behind the name reads soft-deleted rows, so it
+// carries a full target row — credential blob included — into the
+// place a zero value used to sit. The projection to TargetView is
+// what keeps that blob away from the template, and it must hold
+// for a deleted row too.
+func TestHandleSourceLogs_MasksDeletedTargetConfig(t *testing.T) {
+	t.Parallel()
+
+	var (
+		h     *handlers.Handlers
+		sess  *session.Session
+		db    *database.Database
+		dbMgr *database.WebhookDBManager
+	)
+
+	app := newTestApp(t, &h, &sess, &db, &dbMgr)
+	app.RequireStart()
+
+	t.Cleanup(app.RequireStop)
+
+	wh := seedWebhook(t, db)
+	tgt := seedConfiguredTarget(
+		t, db, wh.ID,
+		database.TargetTypeSlack,
+		`{"webhookUrl":"`+slackWebhookURL+`"}`,
+	)
+
+	seedDeliveredEvent(t, dbMgr, wh.ID, tgt.ID)
+	deleteTargetThroughHandler(t, h, sess, wh.ID, tgt.ID)
+
+	body := renderSourceLogsPage(t, h, sess, wh.ID)
+
+	assert.NotContains(t, body, slackSecretPath)
+	assert.NotContains(t, body, "T00000000")
+	assert.NotContains(t, body, "B00000000")
+	assert.NotContains(
+		t, body, "XXXXXXXXXXXXXXXXXXXXXXXX",
+	)
+	assert.NotContains(t, body, "webhookUrl")
+
+	// The name is there; only the credential is not.
+	assert.Contains(t, body, tgt.Name+deletedMarker)
+}

internal/handlers/source_management.go

@@ -860,11 +860,16 @@ func (h *Handlers) HandleSourceLogs() http.HandlerFunc {
 //
 // The load is Unscoped because deleting a target only soft
 // deletes the row while its deliveries survive in the
-// per-webhook database: a scoped load leaves those deliveries
+// per-webhook database. Both halves of the map need those rows:
-// with a zero redactor, which renders their response bodies
+// a scoped load leaves an old delivery with a zero redactor,
-// unredacted. Only the redactor half of the map is built from
+// which renders its response bodies unredacted, and with a zero
-// deleted rows. The view half, which is what the page lists,
+// view, which renders its target as a blank name.
-// stays scoped.
+//
+// This map is historical display only. It is built for the event
+// log page and reaches nothing but DeliveryView.Target: the
+// target list on the source detail page, the edit form and the
+// replay path each resolve targets themselves, and a deleted row
+// is refused there as before.
 func (h *Handlers) loadTargetMap(
 	webhookID string,
 ) (map[string]eventLogTarget, error) {
@@ -880,21 +885,18 @@ func (h *Handlers) loadTargetMap(
 	targetMap := make(
 		map[string]eventLogTarget, len(targets),
 	)
-	live := make([]database.Target, 0, len(targets))
 
 	for i := range targets {
 		targetMap[targets[i].ID] = eventLogTarget{
 			Redactor: delivery.NewRedactor(&targets[i]),
 		}
-
-		if !targets[i].DeletedAt.Valid {
-			live = append(live, targets[i])
-		}
 	}
 
 	// The views come from NewTargetViews rather than being
-	// rebuilt here, so the masking rules stay in one place.
+	// rebuilt here, so the masking rules stay in one place and a
-	for _, v := range delivery.NewTargetViews(live) {
+	// deleted target's configuration is masked by the same code
+	// that masks a live one's.
+	for _, v := range delivery.NewTargetViews(targets) {
 		entry := targetMap[v.ID]
 		entry.View = v
 		targetMap[v.ID] = entry

internal/middleware/export_test.go

@@ -26,6 +26,10 @@ const UnmatchedRouteConst = unmatchedRoute
 // inflight gauge.
 const InflightHandlerConst = inflightHandler
 
+// UnmatchedMethodConst exposes the sentinel that stands in for a
+// method the router can never route.
+const UnmatchedMethodConst = unmatchedMethod
+
 // NewLoggingResponseWriterForTest wraps newLoggingResponseWriter
 // for use in external test packages.
 func NewLoggingResponseWriterForTest(

internal/middleware/metrics.go

@@ -26,6 +26,15 @@ import (
 // counting the requests in flight across the whole service.
 const inflightHandler = "(all)"
 
+// unmatchedMethod is the `method` label for a request whose method
+// the router can never route.
+//
+// It is deliberately the same sentinel as unmatchedRoute rather than
+// a spelling of its own: both stand for a client-chosen token that
+// matched nothing this service registers, and giving one idea two
+// spellings would read in a scrape as two different unmatched states.
+const unmatchedMethod = unmatchedRoute
+
 // routePatternID is the `handler` label for a request: the chi route
 // pattern, never the concrete path.
 //
@@ -50,9 +59,45 @@ func routePatternID(ctx context.Context) string {
 	return unmatchedRoute
 }
 
-// routePatternRecorder wraps a go-http-metrics recorder and replaces
+// methodID is the `method` label for a request: the request method
-// the handler id on every observation with the request's route
+// when the router can route it, and the unmatched sentinel otherwise.
-// pattern.
+//
+// net/http accepts any RFC 9110 token as a method and hands it
+// through verbatim, so the raw method is client-chosen bytes and
+// bounds the label at nothing — the same unauthenticated
+// series-minting the handler label carried, reached through a second
+// dimension. What bounds it is the set chi's router will match a
+// route for: its methodMap, which is unexported, so it is restated
+// here against the net/http constants it is built from. A token
+// outside that set can only ever produce chi's 405, so folding every
+// one of them onto a single series loses no information a scrape
+// could have used, while the nine methods that can reach a handler
+// stay distinguishable.
+//
+// chi.RegisterMethod would extend the router's set at runtime; this
+// service never calls it, and a caller that started to would have to
+// extend this switch with it.
+func methodID(method string) string {
+	switch method {
+	case http.MethodConnect,
+		http.MethodDelete,
+		http.MethodGet,
+		http.MethodHead,
+		http.MethodOptions,
+		http.MethodPatch,
+		http.MethodPost,
+		http.MethodPut,
+		http.MethodTrace:
+		return method
+	default:
+		return unmatchedMethod
+	}
+}
+
+// boundedLabelRecorder wraps a go-http-metrics recorder and replaces
+// the request-controlled labels on every observation with bounded
+// ones: the handler id becomes the request's route pattern, and the
+// method becomes one the router can route.
 //
 // This is the seam that makes the pattern usable at all. The metrics
 // middleware is global (see Server.setupGlobalMiddleware), so it is
@@ -71,29 +116,31 @@ func routePatternID(ctx context.Context) string {
 // Those never reach a handler, but chi has already matched the route
 // by the time the limiter runs, so their 429s land on the pattern
 // like any other response.
-type routePatternRecorder struct {
+type boundedLabelRecorder struct {
 	inner httpmetrics.Recorder
 }
 
-func (r routePatternRecorder) ObserveHTTPRequestDuration(
+func (r boundedLabelRecorder) ObserveHTTPRequestDuration(
 	ctx context.Context,
 	props httpmetrics.HTTPReqProperties,
 	duration time.Duration,
 ) {
 	props.ID = routePatternID(ctx)
+	props.Method = methodID(props.Method)
 	r.inner.ObserveHTTPRequestDuration(ctx, props, duration)
 }
 
-func (r routePatternRecorder) ObserveHTTPResponseSize(
+func (r boundedLabelRecorder) ObserveHTTPResponseSize(
 	ctx context.Context,
 	props httpmetrics.HTTPReqProperties,
 	sizeBytes int64,
 ) {
 	props.ID = routePatternID(ctx)
+	props.Method = methodID(props.Method)
 	r.inner.ObserveHTTPResponseSize(ctx, props, sizeBytes)
 }
 
-func (r routePatternRecorder) AddInflightRequests(
+func (r boundedLabelRecorder) AddInflightRequests(
 	ctx context.Context,
 	props httpmetrics.HTTPProperties,
 	quantity int,
@@ -102,7 +149,7 @@ func (r routePatternRecorder) AddInflightRequests(
 	r.inner.AddInflightRequests(ctx, props, quantity)
 }
 
-var _ httpmetrics.Recorder = routePatternRecorder{}
+var _ httpmetrics.Recorder = boundedLabelRecorder{}
 
 // Metrics returns middleware that records Prometheus HTTP metrics on
 // the default registry, which is the one the /metrics route gathers.
@@ -119,14 +166,14 @@ func metricsMiddleware(
 	rec httpmetrics.Recorder,
 ) func(http.Handler) http.Handler {
 	mdlw := ghmm.New(ghmm.Config{
-		Recorder: routePatternRecorder{inner: rec},
+		Recorder: boundedLabelRecorder{inner: rec},
 	})
 
 	return func(next http.Handler) http.Handler {
 		// The handler id is unmatchedRoute rather than "" so that
 		// the client-chosen URL path never enters the metrics
 		// pipeline at all: an empty id is the library's signal to
-		// substitute it. routePatternRecorder overwrites this value
+		// substitute it. boundedLabelRecorder overwrites this value
 		// on every observation, so it is reachable only if that
 		// decorator is removed — in which case the metrics collapse
 		// to one series instead of leaking again.

internal/middleware/metrics_method_test.go

@@ -0,0 +1,309 @@
+package middleware_test
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"github.com/google/uuid"
+	"github.com/prometheus/client_golang/prometheus"
+	"github.com/prometheus/client_golang/prometheus/promhttp"
+	dto "github.com/prometheus/client_model/go"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/webhooker/internal/middleware"
+)
+
+const (
+	// metricsProbeMethods is how many distinct invented method tokens
+	// each cardinality assertion drives. The measurement on the issue
+	// took 300 tokens from 106 exposition lines to 7,631 — about 25
+	// permanent lines per token, never reclaimed — so a probe of this
+	// size puts a regression thousands of lines over the bound rather
+	// than leaving it to a rounding argument.
+	metricsProbeMethods = 300
+
+	// probeMethodLen is how many characters each invented method
+	// token carries, matching the 12 the issue measured with.
+	probeMethodLen = 12
+
+	// methodLabel is the label these tests are about.
+	methodLabel = "method"
+)
+
+// realMethods is the positive control's domain: the methods chi's
+// router can match a route for, every one of which a client
+// legitimately sends and every one of which must keep a series of its
+// own. Bounding the label by collapsing these into one bucket would
+// destroy the metric it is meant to protect.
+func realMethods() []string {
+	return []string{
+		http.MethodConnect, http.MethodDelete, http.MethodGet,
+		http.MethodHead, http.MethodOptions, http.MethodPatch,
+		http.MethodPost, http.MethodPut, http.MethodTrace,
+	}
+}
+
+// methodProbePath returns the one receiver path a method probe
+// targets. Holding the path fixed leaves the method as the only
+// dimension varying, so any series growth a probe produces is the
+// method label's and nothing else's.
+func methodProbePath() string {
+	return "/webhook/" + uuid.NewString()
+}
+
+// inventedMethods returns n distinct RFC 9110 method tokens that no
+// router will ever match: uppercase hex from a fresh UUID, which is
+// both the shape and the length an unauthenticated flood would send.
+// net/http accepts any token as a method, so every one of these
+// reaches the metrics pipeline exactly as a real method does.
+func inventedMethods(n int) []string {
+	methods := make([]string, 0, n)
+
+	for range n {
+		token := strings.ToUpper(
+			strings.ReplaceAll(uuid.NewString(), "-", ""),
+		)
+		methods = append(methods, token[:probeMethodLen])
+	}
+
+	return methods
+}
+
+// driveMethods sends one request per supplied method to a single
+// fixed path.
+func driveMethods(
+	t *testing.T,
+	h http.Handler,
+	path string,
+	methods []string,
+) map[int]int {
+	t.Helper()
+
+	probes := make([]probe, 0, len(methods))
+
+	for _, m := range methods {
+		probes = append(probes, probe{method: m, path: path})
+	}
+
+	return drive(t, h, probes)
+}
+
+// methodLabels returns the set of distinct `method` values across
+// every gathered series that carries the label at all. The inflight
+// gauge does not carry it, and so contributes nothing rather than an
+// empty-string member.
+func methodLabels(families []*dto.MetricFamily) map[string]struct{} {
+	seen := make(map[string]struct{})
+
+	for _, fam := range families {
+		for _, m := range fam.GetMetric() {
+			for _, pair := range m.GetLabel() {
+				if pair.GetName() == methodLabel {
+					seen[pair.GetValue()] = struct{}{}
+				}
+			}
+		}
+	}
+
+	return seen
+}
+
+// scrapeLines renders the registry through the same promhttp handler
+// /metrics is mounted on and counts the sample lines it produced.
+//
+// This is the quantity the issue measured and the one a Prometheus
+// server pays for on every scrape: one histogram label set is a
+// single gathered series but around 25 lines of exposition, which is
+// why 300 method tokens cost thousands of lines rather than hundreds.
+func scrapeLines(t *testing.T, reg *prometheus.Registry) int {
+	t.Helper()
+
+	h := promhttp.HandlerFor(reg, promhttp.HandlerOpts{})
+	req := httptest.NewRequestWithContext(
+		t.Context(), http.MethodGet, "/metrics", nil,
+	)
+	w := httptest.NewRecorder()
+	h.ServeHTTP(w, req)
+	require.Equal(t, http.StatusOK, w.Code)
+
+	lines := 0
+
+	for line := range strings.SplitSeq(w.Body.String(), "\n") {
+		if line == "" || strings.HasPrefix(line, "#") {
+			continue
+		}
+
+		lines++
+	}
+
+	return lines
+}
+
+// TestMetrics_MethodSentinelIsTheRouteSentinel pins the convention
+// rather than the mechanism. An unroutable method and an unmatched
+// path are the same fact — a client-chosen token matching nothing
+// this service registers — so they carry one spelling. Two spellings
+// would read in a scrape as two different unmatched states.
+func TestMetrics_MethodSentinelIsTheRouteSentinel(t *testing.T) {
+	t.Parallel()
+
+	assert.Equal(
+		t,
+		middleware.UnmatchedRouteConst,
+		middleware.UnmatchedMethodConst,
+		"the unmatched sentinel must have exactly one spelling",
+	)
+}
+
+// TestMetrics_InventedMethodsMintOneLabelSet is the direct assertion
+// the issue asks for: N requests carrying N distinct invented method
+// tokens must produce exactly ONE method label. Before the fix this
+// produced N of them, on an unauthenticated route with no rate
+// limiter.
+func TestMetrics_InventedMethodsMintOneLabelSet(t *testing.T) {
+	t.Parallel()
+
+	h, reg := metricsTestRouter(t, generousReceiverLimit)
+
+	methods := inventedMethods(metricsProbeMethods)
+
+	codes := driveMethods(t, h, methodProbePath(), methods)
+	require.Equal(
+		t, metricsProbeMethods, codes[http.StatusMethodNotAllowed],
+		"every invented token should have been unroutable",
+	)
+
+	labels := methodLabels(gatherMetrics(t, reg))
+
+	// Asserted on the count rather than on the set, so that a
+	// regression reports one number instead of dumping every token it
+	// minted.
+	distinct := len(labels)
+
+	assert.Equal(
+		t, 1, distinct,
+		"invented methods must collapse onto one label",
+	)
+	assert.Contains(
+		t, keys(labels), middleware.UnmatchedMethodConst,
+		"that one label must be the unmatched sentinel",
+	)
+
+	// The scrape must not republish the tokens it was driven with
+	// either: a label that merely looks bounded while still echoing
+	// client bytes is the same defect wearing a different name.
+	echoed := 0
+
+	for _, m := range methods {
+		for label := range labels {
+			if strings.Contains(label, m) {
+				echoed++
+			}
+		}
+	}
+
+	assert.Equal(
+		t, 0, echoed,
+		"invented method tokens reached the metrics labels",
+	)
+}
+
+// TestMetrics_MethodSeriesCountIsFlatUnderAFlood reproduces the
+// measurement on the issue in miniature: scrape, drive several
+// hundred distinct method tokens, scrape again, and require the
+// second scrape to be no larger than the first. The first batch
+// establishes every label set the route can produce; a flood five
+// times its size must land on exactly those.
+func TestMetrics_MethodSeriesCountIsFlatUnderAFlood(t *testing.T) {
+	t.Parallel()
+
+	h, reg := metricsTestRouter(t, generousReceiverLimit)
+
+	path := methodProbePath()
+
+	driveMethods(t, h, path, inventedMethods(metricsProbeMethods))
+	seededSeries := seriesCount(gatherMetrics(t, reg))
+	seededLines := scrapeLines(t, reg)
+
+	driveMethods(t, h, path, inventedMethods(metricsProbeMethods*4))
+	floodedSeries := seriesCount(gatherMetrics(t, reg))
+	floodedLines := scrapeLines(t, reg)
+
+	t.Logf(
+		"after %d invented methods: %d series, %d lines; "+
+			"after %d more: %d series, %d lines",
+		metricsProbeMethods, seededSeries, seededLines,
+		metricsProbeMethods*4, floodedSeries, floodedLines,
+	)
+
+	assert.Equal(
+		t, seededSeries, floodedSeries,
+		"a flood of invented methods must not mint series",
+	)
+	assert.Equal(
+		t, seededLines, floodedLines,
+		"a flood of invented methods must not grow the scrape",
+	)
+}
+
+// TestMetrics_RealMethodsStayDistinct is the positive control. The
+// bound is worth nothing if it is bought by flattening the metric:
+// every method the router can route must still carry a series of its
+// own, one sample each, under the route pattern it was sent to.
+func TestMetrics_RealMethodsStayDistinct(t *testing.T) {
+	t.Parallel()
+
+	h, reg := metricsTestRouter(t, generousReceiverLimit)
+
+	methods := realMethods()
+
+	codes := driveMethods(t, h, methodProbePath(), methods)
+	require.Equal(
+		t, len(methods), codes[http.StatusNotFound],
+		"every real method should have reached the receiver",
+	)
+
+	families := gatherMetrics(t, reg)
+
+	want := make(map[string]struct{}, len(methods))
+	for _, m := range methods {
+		want[m] = struct{}{}
+	}
+
+	assert.Equal(
+		t, want, methodLabels(families),
+		"real methods must remain distinguishable",
+	)
+
+	// Appearing somewhere in the scrape is not enough: each method
+	// must own its duration series, holding the one sample it sent.
+	observed := 0
+
+	for _, fam := range families {
+		if !strings.HasSuffix(fam.GetName(), "request_duration_seconds") {
+			continue
+		}
+
+		for _, m := range fam.GetMetric() {
+			observed++
+
+			assert.Equal(
+				t, receiverRoutePattern,
+				labelValue(m, "handler"),
+			)
+			assert.Equal(
+				t, uint64(1),
+				m.GetHistogram().GetSampleCount(),
+				"method %q shares a series",
+				labelValue(m, methodLabel),
+			)
+		}
+	}
+
+	assert.Equal(
+		t, len(methods), observed,
+		"one duration series per routable method",
+	)
+}

internal/middleware/metrics_test.go

@@ -97,20 +97,24 @@ func metricsTestRouter(
 	return r, reg
 }
 
-// drivePaths sends one POST per supplied path and returns how many
+// probe is one request a cardinality assertion sends. Both label
-// responses carried each status code.
+// dimensions that have leaked are request-controlled — the path and
-func drivePaths(
+// the method — so both vary here and one driver sends them.
-	t *testing.T,
+type probe struct {
-	h http.Handler,
+	method string
-	paths []string,
+	path   string
-) map[int]int {
+}
+
+// drive sends every probe and returns how many responses carried each
+// status code.
+func drive(t *testing.T, h http.Handler, probes []probe) map[int]int {
 	t.Helper()
 
 	codes := make(map[int]int)
 
-	for _, p := range paths {
+	for _, p := range probes {
 		req := httptest.NewRequestWithContext(
-			t.Context(), http.MethodPost, p, nil,
+			t.Context(), p.method, p.path, nil,
 		)
 		w := httptest.NewRecorder()
 		h.ServeHTTP(w, req)
@@ -120,6 +124,25 @@ func drivePaths(
 	return codes
 }
 
+// drivePaths sends one POST per supplied path.
+func drivePaths(
+	t *testing.T,
+	h http.Handler,
+	paths []string,
+) map[int]int {
+	t.Helper()
+
+	probes := make([]probe, 0, len(paths))
+
+	for _, p := range paths {
+		probes = append(
+			probes, probe{method: http.MethodPost, path: p},
+		)
+	}
+
+	return drive(t, h, probes)
+}
+
 // receiverPaths returns n distinct /webhook/ paths, each naming a
 // fresh UUID exactly as an unauthenticated flood would.
 func receiverPaths(n int) []string {

templates/source_logs.html

@@ -39,7 +39,7 @@
                     <div class="flex items-center gap-4">
                         {{range .Deliveries}}
                         <span class="text-xs {{if eq .Status "delivered"}}text-green-600{{else if eq .Status "failed"}}text-red-600{{else if eq .Status "retrying"}}text-yellow-600{{else}}text-gray-400{{end}}">
-                            {{.Target.Name}}: {{.Status}}
+                            {{.Target.DisplayName}}: {{.Status}}
                         </span>
                         {{end}}
                         <span class="text-xs text-gray-400">{{.CreatedAt.Format "2006-01-02 15:04:05"}}</span>
@@ -74,7 +74,7 @@
                             <div class="py-2" x-data="{ attempts: false }">
                                 <div class="flex items-center justify-between cursor-pointer" @click="attempts = !attempts">
                                     <div class="flex items-center gap-3">
-                                        <span class="text-sm text-gray-700">{{.Target.Name}}</span>
+                                        <span class="text-sm text-gray-700">{{.Target.DisplayName}}</span>
                                         <span class="text-xs {{if eq .Status "delivered"}}text-green-600{{else if eq .Status "failed"}}text-red-600{{else if eq .Status "retrying"}}text-yellow-600{{else}}text-gray-400{{end}}">{{.Status}}</span>
                                     </div>
                                     <div class="flex items-center gap-3">