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.

internal/delivery/engine.go

@@ -41,6 +41,26 @@ const (
 	// sweep runs.
 	retrySweepInterval = 60 * time.Second
 
+	// pendingSweepMinAge is how long a delivery must have sat at
+	// pending before the sweep treats it as stranded rather than as
+	// in flight.
+	//
+	// A delivery is pending from the moment it is created until its
+	// outcome is written, which includes the whole time a worker
+	// spends on it, so the bound has to clear the longest a live
+	// attempt can take: httpClientTimeout plus queueing behind the
+	// other deliveries in front of it. Five minutes is far above
+	// that, and still recovers a stranded delivery in minutes rather
+	// than at the next restart.
+	pendingSweepMinAge = 5 * time.Minute
+
+	// pendingSweepBatch bounds how many stranded pending deliveries
+	// one sweep of one webhook re-dispatches. The sweep runs every
+	// retrySweepInterval, so a larger backlog drains across
+	// successive sweeps instead of arriving as one burst against a
+	// database that was already struggling to accept writes.
+	pendingSweepBatch = 500
+
 	// MaxInlineBodySize is the maximum event body size that
 	// will be carried inline in a Task through the channel.
 	// Bodies at or above this size are left nil and fetched
@@ -634,11 +654,120 @@ func (e *Engine) recoverPendingDeliveries(
 		"count", len(deliveries),
 	)
 
+	e.recoverPendingBatch(
+		ctx, webhookDB, webhookID, deliveries,
+	)
+}
+
+// recoverPendingBatch settles every delivery in the batch that was
+// already delivered, and re-dispatches only the rest. Both the
+// restart-time recovery and the periodic sweep go through it, so a
+// pending delivery is treated the same however it was found.
+func (e *Engine) recoverPendingBatch(
+	ctx context.Context,
+	webhookDB *gorm.DB,
+	webhookID string,
+	deliveries []database.Delivery,
+) {
 	targetMap := e.loadTargetMap(deliveries)
 
-	e.sendRecoveredDeliveries(
-		ctx, deliveries, webhookID, targetMap,
+	settled := e.reconcileDelivered(
+		webhookDB, webhookID, deliveries, targetMap,
 	)
+
+	e.sendRecoveredDeliveries(
+		ctx, webhookDB, deliveries, webhookID,
+		targetMap, settled,
+	)
+}
+
+// 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 pending by a failed bookkeeping write, and that
+// covers two different histories: nothing was ever sent, or the send
+// reached the receiver and only the status write failed. Re-sending
+// was the sole option, so every stranded row produced a duplicate at
+// the receiver and an event log that recorded one attempt for two
+// POSTs. A successful result row distinguishes them: it is written
+// before the status, so its presence means the wire I/O happened and
+// was recorded, and all that is missing is the status.
+//
+// Deliveries whose result row itself never landed are not in the
+// returned set and are re-sent, recorded as the further attempt they
+// are. That is honest at-least-once delivery rather than a silent
+// duplicate.
+func (e *Engine) reconcileDelivered(
+	webhookDB *gorm.DB,
+	webhookID string,
+	deliveries []database.Delivery,
+	targetMap map[string]database.Target,
+) map[string]struct{} {
+	settled := make(map[string]struct{})
+
+	if len(deliveries) == 0 {
+		return settled
+	}
+
+	ids := make([]string, 0, len(deliveries))
+	for i := range deliveries {
+		ids = append(ids, deliveries[i].ID)
+	}
+
+	var deliveredIDs []string
+
+	err := webhookDB.
+		Model(&database.DeliveryResult{}).
+		Where(
+			"delivery_id IN ? AND success = ?", ids, true,
+		).
+		Distinct().
+		Pluck("delivery_id", &deliveredIDs).Error
+	if err != nil {
+		// Every delivery stays out of the settled set, so the batch
+		// is re-sent exactly as it was before this check existed.
+		// That is the safe direction: a duplicate delivery beats
+		// declaring a delivery successful on a query that failed.
+		e.log.Error(
+			"failed to query successful delivery results; "+
+				"pending deliveries will be re-sent",
+			"webhook_id", webhookID,
+			"error", err,
+		)
+
+		return settled
+	}
+
+	for _, id := range deliveredIDs {
+		settled[id] = struct{}{}
+	}
+
+	if len(settled) == 0 {
+		return settled
+	}
+
+	e.log.Info(
+		"settling pending deliveries that already succeeded",
+		"webhook_id", webhookID,
+		"count", len(settled),
+	)
+
+	for i := range deliveries {
+		if _, ok := settled[deliveries[i].ID]; !ok {
+			continue
+		}
+
+		e.settleStatus(
+			webhookDB,
+			&deliveries[i],
+			targetMap[deliveries[i].TargetID].Type,
+			database.DeliveryStatusDelivered,
+		)
+	}
+
+	return settled
 }
 
 func (e *Engine) retrySweep(ctx context.Context) {
@@ -733,6 +862,65 @@ func (e *Engine) sweepWebhookRetries(
 			webhookDB, webhookID, &retrying[i],
 		)
 	}
+
+	e.sweepWebhookPending(ctx, webhookDB, webhookID)
+}
+
+// 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
+// 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
+// still hold: a delivery in flight is pending too, and re-dispatching
+// one would race the worker that owns it. It is measured on updated_at
+// rather than created_at because claimPending stamps that column when
+// 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,
+	webhookID string,
+) {
+	var pending []database.Delivery
+
+	err := webhookDB.
+		Where(
+			"status = ? AND updated_at < ?",
+			database.DeliveryStatusPending,
+			time.Now().Add(-pendingSweepMinAge),
+		).
+		Preload("Event").
+		Limit(pendingSweepBatch).
+		Find(&pending).Error
+	if err != nil {
+		e.log.Error(
+			"retry sweep: "+
+				"failed to query pending deliveries",
+			"webhook_id", webhookID,
+			"error", err,
+		)
+
+		return
+	}
+
+	if len(pending) == 0 {
+		return
+	}
+
+	e.log.Info(
+		"retry sweep: recovering stranded pending deliveries",
+		"webhook_id", webhookID,
+		"count", len(pending),
+	)
+
+	e.recoverPendingBatch(ctx, webhookDB, webhookID, pending)
 }
 
 // sweepSingleRetry re-enqueues an orphaned retrying delivery
@@ -839,7 +1027,7 @@ func (e *Engine) failUnretryableRetry(
 		target.Type,
 	)
 
-	e.recordResult(
+	err := e.recordResult(
 		webhookDB,
 		d,
 		e.countAttempts(webhookDB, d.ID)+1,
@@ -849,6 +1037,11 @@ func (e *Engine) failUnretryableRetry(
 		reason,
 		0,
 	)
+	if err != nil {
+		e.bookkeepingFailed(d, err)
+
+		return
+	}
 
 	// The type is passed rather than assigned onto d: the delivery
 	// is loaded here without its target relation, and populating
@@ -856,7 +1049,7 @@ func (e *Engine) failUnretryableRetry(
 	// whole target row — plaintext config, which for a slack target
 	// is the credential — into the per-webhook event database. See
 	// https://git.eeqj.de/sneak/webhooker/issues/206.
-	e.updateDeliveryStatus(
+	e.settleStatus(
 		webhookDB, d, target.Type,
 		database.DeliveryStatusFailed,
 	)
@@ -878,7 +1071,7 @@ func (e *Engine) processDelivery(
 			"type", d.Target.Type,
 		)
 
-		e.updateDeliveryStatus(
+		e.settleStatus(
 			webhookDB, d, d.Target.Type,
 			database.DeliveryStatusFailed,
 		)
@@ -910,6 +1103,14 @@ func (e *Engine) observeAttempt(
 // recordResult persists a DeliveryResult row describing a
 // single attempt. It is a cross-target helper the targets
 // call.
+//
+// It returns its error rather than swallowing it. A DeliveryResult
+// row is the only record that an attempt happened at all, so a
+// caller that ignored a failed write would go on to mark the
+// delivery delivered — leaving the event log claiming one attempt
+// for a receiver that got two. Every caller must instead stop
+// advancing the delivery's status and let it stay in the
+// non-terminal state it already holds; see bookkeepingFailed.
 func (e *Engine) recordResult(
 	webhookDB *gorm.DB,
 	d *database.Delivery,
@@ -918,7 +1119,7 @@ func (e *Engine) recordResult(
 	statusCode int,
 	respBody, errMsg string,
 	durationMs int64,
-) {
+) error {
 	result := &database.DeliveryResult{
 		DeliveryID:   d.ID,
 		AttemptNum:   attemptNum,
@@ -931,12 +1132,44 @@ func (e *Engine) recordResult(
 
 	err := webhookDB.Create(result).Error
 	if err != nil {
-		e.log.Error(
-			"failed to record delivery result",
-			"delivery_id", d.ID,
-			"error", err,
+		return fmt.Errorf(
+			"recording delivery result for %s: %w", d.ID, err,
 		)
 	}
+
+	return nil
+}
+
+// bookkeepingFailed reports that a delivery's own record of what
+// happened could not be written, and deliberately writes nothing in
+// response.
+//
+// Leaving the row alone is the whole point. A delivery is created
+// pending and only ever leaves that state through
+// updateDeliveryStatus, so a delivery whose bookkeeping write failed
+// is still pending or retrying — the two non-terminal states, per
+// DeliveryStatus.Terminal — and both are swept and recovered. Writing
+// anything here would need the very database that just refused a
+// write, and would be one more thing to fail; not writing cannot.
+//
+// The cost is honest at-least-once behaviour: a send that reached the
+// receiver but whose result row did not land is attempted again, and
+// recorded as the further attempt it is. What no longer happens is the
+// silent duplicate — a second POST the event log denies ever
+// occurred. Where the result row *did* land and only the status write
+// failed, reconcileDelivered settles the row without re-sending.
+func (e *Engine) bookkeepingFailed(
+	d *database.Delivery, err error,
+) {
+	e.log.Error(
+		"delivery bookkeeping write failed; leaving delivery "+
+			"in a recoverable state",
+		"delivery_id", d.ID,
+		"event_id", d.EventID,
+		"target_id", d.TargetID,
+		"status", d.Status,
+		"error", err,
+	)
 }
 
 // updateDeliveryStatus persists a new status for a delivery.
@@ -952,26 +1185,45 @@ func (e *Engine) recordResult(
 //
 // The counter moves only after the row is written, so a transition
 // the database rejected is not claimed as an outcome that happened.
+// For the same reason the error is returned rather than logged and
+// dropped: a delivery whose status write failed has not reached that
+// status, and its caller must not act as though it had.
 func (e *Engine) updateDeliveryStatus(
 	webhookDB *gorm.DB,
 	d *database.Delivery,
 	targetType database.TargetType,
 	status database.DeliveryStatus,
-) {
+) error {
 	err := webhookDB.Model(d).
 		Update("status", status).Error
 	if err != nil {
-		e.log.Error(
-			"failed to update delivery status",
-			"delivery_id", d.ID,
-			"status", status,
-			"error", err,
+		return fmt.Errorf(
+			"updating delivery %s to status %s: %w",
+			d.ID, status, err,
 		)
-
-		return
 	}
 
 	e.mtr.DeliveryStatusChanged(targetType, status)
+
+	return nil
+}
+
+// settleStatus moves a delivery to its outcome status and reports a
+// failed write through bookkeepingFailed, which leaves the row
+// recoverable. It exists so the target call sites read as one
+// statement rather than four lines of identical error handling.
+func (e *Engine) settleStatus(
+	webhookDB *gorm.DB,
+	d *database.Delivery,
+	targetType database.TargetType,
+	status database.DeliveryStatus,
+) {
+	err := e.updateDeliveryStatus(
+		webhookDB, d, targetType, status,
+	)
+	if err != nil {
+		e.bookkeepingFailed(d, err)
+	}
 }
 
 func truncate(s string, maxLen int) string {
@@ -1065,6 +1317,95 @@ func (e *Engine) countAttempts(
 	return int(resultCount)
 }
 
+// claimPending takes ownership of a pending delivery before it is
+// re-dispatched, and reports whether the claim succeeded.
+//
+// The claim is a compare-and-set on the status: it takes effect only
+// while the delivery is still pending, so a worker that settled the
+// delivery between the query and here wins and nothing is re-sent.
+// 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.
+//
+// A claim that cannot be written means the database is refusing
+// writes, which is the condition that stranded this delivery in the
+// first place. Not sending is then the right answer: the attempt
+// could not be recorded either, and an unrecordable send is exactly
+// the duplicate this issue is about.
+func (e *Engine) claimPending(
+	webhookDB *gorm.DB, d *database.Delivery,
+) bool {
+	res := webhookDB.
+		Model(&database.Delivery{}).
+		Where(
+			"id = ? AND status = ?",
+			d.ID, database.DeliveryStatusPending,
+		).
+		UpdateColumn("updated_at", time.Now())
+	if res.Error != nil {
+		e.log.Error(
+			"failed to claim pending delivery for recovery; "+
+				"leaving it for the next sweep",
+			"delivery_id", d.ID,
+			"error", res.Error,
+		)
+
+		return false
+	}
+
+	return res.RowsAffected == 1
+}
+
+// countAttemptsBatch counts the recorded attempts of every delivery
+// in a batch with one grouped query, keyed by delivery id. Deliveries
+// with no attempts are simply absent from the result, which reads back
+// as the zero this caller wants.
+//
+// One query rather than one per delivery: this runs on the recovery
+// path, which is a burst of writes against a database that has just
+// been under enough contention to strand these rows in the first
+// place. See https://git.eeqj.de/sneak/webhooker/issues/256.
+func (e *Engine) countAttemptsBatch(
+	webhookDB *gorm.DB, deliveries []database.Delivery,
+) map[string]int {
+	counts := make(map[string]int, len(deliveries))
+
+	if len(deliveries) == 0 {
+		return counts
+	}
+
+	ids := make([]string, 0, len(deliveries))
+	for i := range deliveries {
+		ids = append(ids, deliveries[i].ID)
+	}
+
+	// One delivery id per recorded attempt, tallied here rather than
+	// grouped in SQL: internal/gormlog forbids (*gorm.DB).Scan, which
+	// a GROUP BY into a struct would need, and an attempt row per
+	// delivery is bounded by the target's MaxRetries.
+	var attemptIDs []string
+
+	err := webhookDB.
+		Model(&database.DeliveryResult{}).
+		Where("delivery_id IN ?", ids).
+		Pluck("delivery_id", &attemptIDs).Error
+	if err != nil {
+		e.log.Error(
+			"failed to count delivery attempts for recovery",
+			"error", err,
+		)
+
+		return counts
+	}
+
+	for _, id := range attemptIDs {
+		counts[id]++
+	}
+
+	return counts
+}
+
 func (e *Engine) loadEvent(
 	webhookDB *gorm.DB, eventID string,
 ) (database.Event, error) {
@@ -1168,12 +1509,24 @@ func (e *Engine) loadTargetMap(
 	return targetMap
 }
 
+// sendRecoveredDeliveries re-dispatches pending deliveries, skipping
+// the ids in settled — those already reached their receiver and have
+// been marked delivered by reconcileDelivered.
 func (e *Engine) sendRecoveredDeliveries(
 	ctx context.Context,
+	webhookDB *gorm.DB,
 	deliveries []database.Delivery,
 	webhookID string,
 	targetMap map[string]database.Target,
+	settled map[string]struct{},
 ) {
+	// The attempt number continues each delivery's own history
+	// rather than restarting at 1. A recovered delivery may already
+	// have recorded attempts, and numbering the next one 1 again
+	// both collides in the event log and hands the retry path a
+	// backoff computed from the wrong attempt.
+	attempts := e.countAttemptsBatch(webhookDB, deliveries)
+
 	for i := range deliveries {
 		select {
 		case <-ctx.Done():
@@ -1181,6 +1534,10 @@ func (e *Engine) sendRecoveredDeliveries(
 		default:
 		}
 
+		if _, ok := settled[deliveries[i].ID]; ok {
+			continue
+		}
+
 		target, ok := targetMap[deliveries[i].TargetID]
 		if !ok {
 			e.log.Error(
@@ -1192,9 +1549,14 @@ func (e *Engine) sendRecoveredDeliveries(
 			continue
 		}
 
+		if !e.claimPending(webhookDB, &deliveries[i]) {
+			continue
+		}
+
 		task := buildRecoveryTask(
 			&deliveries[i], webhookID,
-			&deliveries[i].Event, &target, 1,
+			&deliveries[i].Event, &target,
+			attempts[deliveries[i].ID]+1,
 		)
 
 		select {