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: 60 of 60 inbound webhooks rejected with HTTP 500,
206 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`: a deferred
transaction that upgrades to a write lock mid-flight gets SQLITE_BUSY
without the busy handler being consulted. `cache=shared` is gone,
because under it an in-process conflict is SQLITE_LOCKED, which the
busy handler does not retry.

Delivery. `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 both sweeps recover
it. Recovery and the sweep now reconcile before re-sending: a pending
delivery that already holds a successful `DeliveryResult` is marked
delivered rather than sent again, which is the state that did not
previously exist. A delivery handed back out is claimed by
compare-and-set so successive sweeps cannot send it repeatedly, and it
continues its 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: both documented procedures were
re-run against a live instance, and a `-wal` left by a crash carries
data the `.db` alone does not.

Verified by reproducing the failure on unmodified `next` first — 6
targets, 60 events at 5/s, a concurrent `.dump` reader — which gave 38
HTTP 500s and 112 duplicate POSTs at the sinks across a restart. Both
arms of the matched pair now show 0 inbound 500s, 0 engine write
errors, and 0 new requests at the sinks after a restart, counted by
payload.

README.md

@@ -573,18 +573,11 @@ 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. `-shm` is regenerable, but there is no
+have no readable schema at all. A clean shutdown checkpoints and removes
-reason to separate the two — copy the directory and you have them.
+both sidecars, so a stopped deployment has none; a killed or crashed one
-
+leaves them, and they must be carried with the `.db`. `-shm` is
-A clean shutdown closes `webhooker.db` and every `events-*.db`, which
+regenerable, but there is no reason to separate the two — copy the
-checkpoints and removes their sidecars; a killed or crashed instance
+directory.
-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
@@ -640,30 +633,17 @@ 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 and reopens its handle around writes (debounced
+archive writer closes its handle after each write (debounced to at most
-to at most one reopen per second), so an operator can move
+one reopen per second), so an operator can move `archive-{uuid}.db`
-`archive-{uuid}.db` away for offline retention while the service runs,
+away for offline retention while the service runs, and it is recreated
-and it is recreated on the next write. See
+on the next write. Closing the handle checkpoints and removes that
+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.
@@ -679,14 +659,10 @@ the service stops. So either move `archive-{uuid}.db` together with any
 
 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. An `.backup` set will not
+   discards every transaction it still holds. Backups taken by either
-   contain any: it writes a single consolidated file per database. A
+   procedure above will not contain them — `.backup` writes a single
-   stop-and-copy set has none for `webhooker.db` or the `events-*.db`,
+   consolidated file, and a clean stop checkpoints the sidecars away —
-   because a clean stop closes those and checkpoints their sidecars
+   but a copy salvaged from a crashed instance will, and it needs them.
-   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
@@ -1728,40 +1704,6 @@ 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,20 +108,11 @@ 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(
@@ -129,7 +120,6 @@ 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,17 +40,6 @@ 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,23 +41,18 @@ const (
 	// sweep runs.
 	retrySweepInterval = 60 * time.Second
 
-	// pendingSweepMinAge is how long a delivery must have sat
+	// pendingSweepMinAge is how long a delivery must have sat at
-	// untouched at pending before the sweep will look at it.
+	// pending before the sweep treats it as stranded rather than as
+	// in flight.
 	//
-	// It is not what keeps the sweep off live work — inflightSet is,
+	// A delivery is pending from the moment it is created until its
-	// and it is exact. This bound sets the re-dispatch cadence for a
+	// outcome is written, which includes the whole time a worker
-	// delivery that really is stranded: without it, a delivery the
+	// spends on it, so the bound has to clear the longest a live
-	// database will not let the engine settle would be re-sent on
+	// attempt can take: httpClientTimeout plus queueing behind the
-	// every 60-second tick.
+	// other deliveries in front of it. Five minutes is far above
-	//
+	// that, and still recovers a stranded delivery in minutes rather
-	// It is nonetheless set clear of the longest legitimate attempt,
+	// than at the next restart.
-	// so that the two guards do not both have to be right. That
+	pendingSweepMinAge = 5 * time.Minute
-	// 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
@@ -182,12 +177,6 @@ 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
@@ -220,27 +209,12 @@ 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 by the sweep",
+					"task will be recovered on restart",
 				"delivery_id", tasks[i].DeliveryID,
 				"event_id", tasks[i].EventID,
 			)
@@ -275,20 +249,10 @@ 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",
@@ -388,35 +352,13 @@ func (e *Engine) worker(ctx context.Context) {
 		case <-ctx.Done():
 			return
 		case task := <-e.deliveryCh:
-			e.runTask(ctx, &task, e.processNewTask)
+			e.processNewTask(ctx, &task)
 		case task := <-e.retryCh:
-			e.runTask(ctx, &task, e.processRetryTask)
+			e.processRetryTask(ctx, &task)
 		}
 	}
 }
 
-// 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()
 
@@ -604,16 +546,7 @@ 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],
 		)
@@ -675,10 +608,6 @@ 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,
@@ -686,6 +615,8 @@ func (e *Engine) recoverSingleRetry(
 		"attempt", attemptNum,
 		"remaining_backoff", remaining,
 	)
+
+	e.ScheduleRetry(task, remaining)
 }
 
 func (e *Engine) recoverPendingDeliveries(
@@ -695,14 +626,12 @@ 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 {
@@ -752,26 +681,19 @@ func (e *Engine) recoverPendingBatch(
 	)
 }
 
-// reconcileDelivered finds the deliveries in a recovered batch that
+// reconcileDelivered finds the deliveries in a pending 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 in a non-terminal state by a failed bookkeeping
+// delivery is left pending by a failed bookkeeping write, and that
-// write, and that covers two different histories: nothing was ever
+// covers two different histories: nothing was ever sent, or the send
-// sent, or the send reached the receiver and only the status write
+// reached the receiver and only the status write failed. Re-sending
-// failed. Re-sending was the sole option, so every stranded row
+// was the sole option, so every stranded row produced a duplicate at
-// produced a duplicate at the receiver and an event log that recorded
+// the receiver and an event log that recorded one attempt for two
-// one attempt for two POSTs. A successful result row distinguishes
+// POSTs. A successful result row distinguishes them: it is written
-// them: it is written before the status, so its presence means the
+// before the status, so its presence means the wire I/O happened and
-// wire I/O happened and was recorded, and all that is missing is the
+// was recorded, and all that is missing is the status.
-// 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
@@ -827,7 +749,7 @@ func (e *Engine) reconcileDelivered(
 	}
 
 	e.log.Info(
-		"settling recovered deliveries that already succeeded",
+		"settling pending deliveries that already succeeded",
 		"webhook_id", webhookID,
 		"count", len(settled),
 	)
@@ -837,22 +759,12 @@ 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
@@ -939,11 +851,6 @@ func (e *Engine) sweepWebhookRetries(
 		return
 	}
 
-	settled := e.reconcileDelivered(
-		webhookDB, webhookID, retrying,
-		e.loadTargetMap(retrying),
-	)
-
 	for i := range retrying {
 		select {
 		case <-ctx.Done():
@@ -951,10 +858,6 @@ func (e *Engine) sweepWebhookRetries(
 		default:
 		}
 
-		if _, ok := settled[retrying[i].ID]; ok {
-			continue
-		}
-
 		e.sweepSingleRetry(
 			webhookDB, webhookID, &retrying[i],
 		)
@@ -966,18 +869,20 @@ 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 the engine does not own is one
+// outcome is written, so a pending row older than the age bound 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.
 //
-// What keeps the sweep off live work is ownership, checked per
+// The age bound is what keeps the sweep off deliveries the workers
-// delivery in takeForRedispatch, not the age bound in this query.
+// still hold: a delivery in flight is pending too, and re-dispatching
-// A delivery waiting in deliveryCh is pending and arbitrarily old —
+// one would race the worker that owns it. It is measured on updated_at
-// the channel holds 10000 tasks and 10 workers drain it — so
+// rather than created_at because claimPending stamps that column when
-// reasoning from the row's age alone re-sends it. See inflight.go.
+// a delivery is handed out, which is what stops the next sweep, a
+// 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,
@@ -991,6 +896,7 @@ func (e *Engine) sweepWebhookPending(
 			database.DeliveryStatusPending,
 			time.Now().Add(-pendingSweepMinAge),
 		).
+		Preload("Event").
 		Limit(pendingSweepBatch).
 		Find(&pending).Error
 	if err != nil {
@@ -1071,20 +977,17 @@ func (e *Engine) sweepSingleRetry(
 		d, webhookID, &event, &target, attemptNum+1,
 	)
 
-	if !e.redispatch(
+	select {
-		e.retryCh, webhookDB, task,
+	case e.retryCh <- task:
-		database.DeliveryStatusRetrying,
+		e.log.Info(
-	) {
+			"retry sweep: "+
-		return
+				"recovered orphaned retrying delivery",
+			"delivery_id", d.ID,
+			"webhook_id", webhookID,
+			"attempt", attemptNum+1,
+		)
+	default:
 	}
-
-	e.log.Info(
-		"retry sweep: "+
-			"recovered orphaned retrying delivery",
-		"delivery_id", d.ID,
-		"webhook_id", webhookID,
-		"attempt", attemptNum+1,
-	)
 }
 
 // failUnretryableRetry terminally fails an orphaned retrying
@@ -1107,16 +1010,6 @@ 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",
@@ -1310,13 +1203,7 @@ func (e *Engine) updateDeliveryStatus(
 		)
 	}
 
-	// An empty type means the target row is gone — a delivery being
+	e.mtr.DeliveryStatusChanged(targetType, status)
-	// 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
 }
@@ -1430,133 +1317,44 @@ func (e *Engine) countAttempts(
 	return int(resultCount)
 }
 
-// takeForRedispatch decides whether a recovered delivery may be sent
+// claimPending takes ownership of a pending delivery before it is
-// again, and takes it if so. It is the single gate every re-dispatch
+// re-dispatched, and reports whether the claim succeeded.
-// path goes through, and it asks two separate questions in order.
 //
-// First, does the engine already own this delivery? Ownership is
+// The claim is a compare-and-set on the status: it takes effect only
-// exact and mutually exclusive, so a delivery queued, being attempted,
+// while the delivery is still pending, so a worker that settled the
-// or waiting out a retry backoff is refused here, and two dispatchers
+// delivery between the query and here wins and nothing is re-sent.
-// racing for the same delivery cannot both win. See inflight.go.
+// Stamping updated_at is the claim itself — the sweep selects on that
+// 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.
 //
-// Second, is the row still in the status that made it eligible? The
+// A claim that cannot be written means the database is refusing
-// batch was read some time ago and a worker may have settled a row
+// writes, which is the condition that stranded this delivery in the
-// since. The check is a conditional update rather than a read so the
+// first place. Not sending is then the right answer: the attempt
-// answer cannot go stale between asking and acting.
+// could not be recorded either, and an unrecordable send is exactly
-//
+// the duplicate this issue is about.
-// Stamping updated_at is the same statement, and it is a cadence
+func (e *Engine) claimPending(
-// control rather than a claim: the pending sweep selects on that
+	webhookDB *gorm.DB, d *database.Delivery,
-// 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 = ?", deliveryID, eligible,
+			"id = ? AND status = ?",
+			d.ID, database.DeliveryStatusPending,
 		).
 		UpdateColumn("updated_at", time.Now())
-
 	if res.Error != nil {
 		e.log.Error(
-			"failed to mark delivery for re-dispatch; "+
+			"failed to claim pending delivery for recovery; "+
-				"leaving it for a later sweep",
+				"leaving it for the next sweep",
-			"delivery_id", deliveryID,
+			"delivery_id", d.ID,
 			"error", res.Error,
 		)
-		e.inflight.release(deliveryID)
 
 		return false
 	}
 
-	if res.RowsAffected != 1 {
+	return 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
@@ -1675,10 +1473,6 @@ 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))
@@ -1718,15 +1512,6 @@ 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,
@@ -1764,40 +1549,27 @@ func (e *Engine) sendRecoveredDeliveries(
 			continue
 		}
 
-		if !e.takeForRedispatch(
+		if !e.claimPending(webhookDB, &deliveries[i]) {
-			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, &event, &target,
+			&deliveries[i], webhookID,
+			&deliveries[i].Event, &target,
 			attempts[deliveries[i].ID]+1,
 		)
 
-		e.queueRecovered(e.deliveryCh, task)
+		select {
+		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,26 +291,6 @@ 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

@@ -1,110 +0,0 @@
-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

@@ -1,428 +0,0 @@
-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,8 +163,19 @@ 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}`,
@@ -215,8 +226,19 @@ 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}`,
@@ -266,8 +288,19 @@ 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,51 +23,21 @@ 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
 // blob.
 type TargetView struct {
-	ID   string
+	ID     string
-	Name 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 {
@@ -77,12 +47,11 @@ func NewTargetViews(
 		t := &targets[i]
 
 		views = append(views, TargetView{
-			ID:      t.ID,
+			ID:     t.ID,
-			Name:    t.Name,
+			Name:   t.Name,
-			Deleted: t.DeletedAt.Valid,
+			Type:   t.Type,
-			Type:    t.Type,
+			Active: t.Active,
-			Active:  t.Active,
+			Config: targetConfigFields(t),
-			Config:  targetConfigFields(t),
 		})
 	}
 

internal/delivery/target_config_view_test.go

@@ -2,11 +2,9 @@ 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"
 )
@@ -19,14 +17,6 @@ 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 + "/..."
@@ -43,7 +33,7 @@ func TestMaskedWebhookURL(t *testing.T) {
 	}{
 		"slack webhook": {
 			url:  slackWebhookURL,
-			want: slackMaskedURL,
+			want: "https://hooks.slack.com/...",
 		},
 		"query string dropped": {
 			url:  viewExampleOrigin + "/a?token=secret",
@@ -135,61 +125,23 @@ func viewFor(
 	return views[0]
 }
 
-// TestNewTargetViews_DeletedTarget proves the projection marks
+func TestNewTargetViews_Slack(t *testing.T) {
-// 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()
 
-	target := slackTarget()
+	view := viewFor(t, database.Target{
-	target.DeletedAt = gorm.DeletedAt{
+		Name:   "slack-target",
-		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{"Webhook URL": slackMaskedURL},
+		map[string]string{
+			"Webhook URL": "https://hooks.slack.com/...",
+		},
 		fieldMap(view.Config),
 	)
 }
@@ -260,7 +212,7 @@ func TestNewTargetViews_HTTPMasksDestinationURL(t *testing.T) {
 
 	assert.Equal(
 		t,
-		slackMaskedURL,
+		"https://hooks.slack.com/...",
 		fields["Destination URL"],
 	)
 

internal/handlers/source_logs_deleted_target_test.go

@@ -1,144 +0,0 @@
-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,16 +860,11 @@ 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. Both halves of the map need those rows:
+// per-webhook database: a scoped load leaves those deliveries
-// a scoped load leaves an old delivery with a zero redactor,
+// with a zero redactor, which renders their response bodies
-// which renders its response bodies unredacted, and with a zero
+// unredacted. Only the redactor half of the map is built from
-// view, which renders its target as a blank name.
+// deleted rows. The view half, which is what the page lists,
-//
+// 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) {
@@ -885,18 +880,21 @@ 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 and a
+	// rebuilt here, so the masking rules stay in one place.
-	// deleted target's configuration is masked by the same code
+	for _, v := range delivery.NewTargetViews(live) {
-	// 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,10 +26,6 @@ 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,15 +26,6 @@ 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.
 //
@@ -59,45 +50,9 @@ func routePatternID(ctx context.Context) string {
 	return unmatchedRoute
 }
 
-// methodID is the `method` label for a request: the request method
+// routePatternRecorder wraps a go-http-metrics recorder and replaces
-// when the router can route it, and the unmatched sentinel otherwise.
+// the handler id on every observation with the request's route
-//
+// 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
@@ -116,31 +71,29 @@ func methodID(method string) 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 boundedLabelRecorder struct {
+type routePatternRecorder struct {
 	inner httpmetrics.Recorder
 }
 
-func (r boundedLabelRecorder) ObserveHTTPRequestDuration(
+func (r routePatternRecorder) 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 boundedLabelRecorder) ObserveHTTPResponseSize(
+func (r routePatternRecorder) 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 boundedLabelRecorder) AddInflightRequests(
+func (r routePatternRecorder) AddInflightRequests(
 	ctx context.Context,
 	props httpmetrics.HTTPProperties,
 	quantity int,
@@ -149,7 +102,7 @@ func (r boundedLabelRecorder) AddInflightRequests(
 	r.inner.AddInflightRequests(ctx, props, quantity)
 }
 
-var _ httpmetrics.Recorder = boundedLabelRecorder{}
+var _ httpmetrics.Recorder = routePatternRecorder{}
 
 // Metrics returns middleware that records Prometheus HTTP metrics on
 // the default registry, which is the one the /metrics route gathers.
@@ -166,14 +119,14 @@ func metricsMiddleware(
 	rec httpmetrics.Recorder,
 ) func(http.Handler) http.Handler {
 	mdlw := ghmm.New(ghmm.Config{
-		Recorder: boundedLabelRecorder{inner: rec},
+		Recorder: routePatternRecorder{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. boundedLabelRecorder overwrites this value
+		// substitute it. routePatternRecorder 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

@@ -1,309 +0,0 @@
-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,34 +97,8 @@ func metricsTestRouter(
 	return r, reg
 }
 
-// probe is one request a cardinality assertion sends. Both label
+// drivePaths sends one POST per supplied path and returns how many
-// dimensions that have leaked are request-controlled — the path and
+// responses carried each status code.
-// the method — so both vary here and one driver sends them.
-type probe struct {
-	method string
-	path   string
-}
-
-// 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 probes {
-		req := httptest.NewRequestWithContext(
-			t.Context(), p.method, p.path, nil,
-		)
-		w := httptest.NewRecorder()
-		h.ServeHTTP(w, req)
-		codes[w.Code]++
-	}
-
-	return codes
-}
-
-// drivePaths sends one POST per supplied path.
 func drivePaths(
 	t *testing.T,
 	h http.Handler,
@@ -132,15 +106,18 @@ func drivePaths(
 ) map[int]int {
 	t.Helper()
 
-	probes := make([]probe, 0, len(paths))
+	codes := make(map[int]int)
 
 	for _, p := range paths {
-		probes = append(
+		req := httptest.NewRequestWithContext(
-			probes, probe{method: http.MethodPost, path: p},
+			t.Context(), http.MethodPost, p, nil,
 		)
+		w := httptest.NewRecorder()
+		h.ServeHTTP(w, req)
+		codes[w.Code]++
 	}
 
-	return drive(t, h, probes)
+	return codes
 }
 
 // receiverPaths returns n distinct /webhook/ paths, each naming a

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.DisplayName}}: {{.Status}}
+                            {{.Target.Name}}: {{.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.DisplayName}}</span>
+                                        <span class="text-sm text-gray-700">{{.Target.Name}}</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">