Close the two remaining delivery terminal-state gaps (closes #107)

internal/delivery/engine.go

@@ -4,6 +4,7 @@ package delivery
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"log/slog"
 	"net/http"
@@ -505,6 +506,10 @@ func (e *Engine) processRetryTask(
 		return
 	}
 
+	if e.abandonRetryForMissingTarget(webhookDB, d, task) {
+		return
+	}
+
 	event := buildEventFromTask(task)
 
 	event, err = e.resolveEventBody(
@@ -529,6 +534,64 @@ func (e *Engine) processRetryTask(
 	e.processDelivery(ctx, webhookDB, d, task)
 }
 
+// abandonRetryForMissingTarget stops a retry chain whose target has
+// been deleted, and reports whether it did.
+//
+// A scheduled retry lives in memory as a time.AfterFunc holding the
+// target's configuration as it was when the chain began, and nothing
+// else on this path reads the target row. Without this check a
+// deletion stops nothing: the timer keeps firing and keeps sending to
+// the destination the operator removed, for the whole remaining
+// backoff chain. Terminalising in the recovery and sweep paths alone
+// is not enough, because those only see the delivery once nothing
+// holds it in memory — which is to say after a restart.
+//
+// The worker already owns this delivery, so the terminal write happens
+// here directly, exactly as a target's own Deliver fails one. Claiming
+// it again through the recovery gate would only fail against the
+// reference the worker itself is holding.
+//
+// A lookup that fails for any other reason is not a deletion — it is
+// the main database being unreadable — and the delivery goes ahead as
+// it did before. A guard that terminally failed deliveries on a
+// transient fault would be worse than the bug it fixes.
+func (e *Engine) abandonRetryForMissingTarget(
+	webhookDB *gorm.DB,
+	d *database.Delivery,
+	task *Task,
+) bool {
+	_, err := e.loadTarget(task.TargetID)
+	if err == nil {
+		return false
+	}
+
+	if !errors.Is(err, gorm.ErrRecordNotFound) {
+		e.log.Warn(
+			"could not confirm the target of a retrying "+
+				"delivery still exists; attempting anyway",
+			"delivery_id", task.DeliveryID,
+			"target_id", task.TargetID,
+			"error", err,
+		)
+
+		return false
+	}
+
+	targetType, reason := e.missingTargetReason(task.TargetID)
+
+	e.log.Warn(
+		"abandoning scheduled retry: target is gone",
+		"webhook_id", task.WebhookID,
+		"delivery_id", task.DeliveryID,
+		"target_id", task.TargetID,
+		"target_type", targetType,
+	)
+
+	e.failDelivery(webhookDB, d, targetType, reason)
+
+	return true
+}
+
 func (e *Engine) recoverInFlight(ctx context.Context) {
 	var webhookIDs []string
 
@@ -633,6 +696,20 @@ func (e *Engine) recoverSingleRetry(
 ) {
 	target, err := e.loadTarget(d.TargetID)
 	if err != nil {
+		// A target that is merely gone is an operator action with a
+		// terminal answer. Any other failure is the main database
+		// refusing to read, which is transient and must leave the
+		// delivery alone: failing every retrying delivery of every
+		// webhook on one bad read would be a far larger fault than
+		// the strand it is meant to clear.
+		if errors.Is(err, gorm.ErrRecordNotFound) {
+			e.failMissingTargetRetry(
+				webhookDB, webhookID, d,
+			)
+
+			return
+		}
+
 		e.log.Error(
 			"failed to load target for retrying "+
 				"delivery recovery",
@@ -1028,6 +1105,16 @@ func (e *Engine) sweepSingleRetry(
 ) {
 	target, err := e.loadTarget(d.TargetID)
 	if err != nil {
+		// Deleted is terminal, unreadable is not; see
+		// recoverSingleRetry.
+		if errors.Is(err, gorm.ErrRecordNotFound) {
+			e.failMissingTargetRetry(
+				webhookDB, webhookID, d,
+			)
+
+			return
+		}
+
 		e.log.Error(
 			"retry sweep: failed to load target",
 			"delivery_id", d.ID,
@@ -1134,6 +1221,113 @@ func (e *Engine) failUnretryableRetry(
 		target.Type,
 	)
 
+	e.failDelivery(webhookDB, d, target.Type, reason)
+}
+
+// failMissingTargetRetry terminally fails an orphaned retrying
+// delivery whose target row is gone. Both restart recovery and the
+// periodic sweep call it, so the transition exists once.
+//
+// Until it existed both paths logged the failed lookup and returned,
+// which left the delivery retrying for the life of the database and
+// the sweep repeating the same error every minute forever. Failing it
+// with a recorded reason is the treatment the other orphaned-retry
+// cases already get, so all of them read alike in the event log.
+//
+// Logged at warn rather than error: a deleted target is an operator
+// action, not a system fault.
+func (e *Engine) failMissingTargetRetry(
+	webhookDB *gorm.DB,
+	webhookID string,
+	d *database.Delivery,
+) {
+	// Terminal, and reached from the recovery paths, so it takes
+	// ownership like every other write they make.
+	if !e.inflight.retainIdle(d.ID) {
+		return
+	}
+
+	defer e.inflight.release(d.ID)
+
+	targetType, reason := e.missingTargetReason(d.TargetID)
+
+	e.log.Warn(
+		"failing orphaned retrying delivery: "+
+			"its target no longer exists",
+		"webhook_id", webhookID,
+		"delivery_id", d.ID,
+		"target_id", d.TargetID,
+		"target_type", targetType,
+	)
+
+	e.failDelivery(webhookDB, d, targetType, reason)
+}
+
+// missingTargetReason describes a target id that no longer resolves,
+// and returns the type of the deleted row where there still is one.
+//
+// The lookup is Unscoped because deletes are soft: the row survives
+// with deleted_at set, invisible to loadTarget's default scope.
+// Reading it is what separates "you deleted this target" from "this id
+// never named a row" — different things to whoever reads the event
+// log, and only the first is something an operator did. The widened
+// scope is deliberately confined to this terminal path: the engine's
+// normal target loading must go on refusing a deleted target, or
+// deleting one would stop nothing.
+//
+// The type comes back so the caller can label the delivery's status
+// transition with it. Where the row is gone entirely there is no type
+// to give, and updateDeliveryStatus leaves the counter alone rather
+// than opening a series named by the empty string.
+func (e *Engine) missingTargetReason(
+	targetID string,
+) (database.TargetType, string) {
+	var target database.Target
+
+	err := e.database.DB().Unscoped().
+		First(&target, "id = ?", targetID).Error
+	if err != nil {
+		return "", fmt.Sprintf(
+			"target %s no longer exists; the delivery "+
+				"cannot be retried and has been failed "+
+				"terminally",
+			targetID,
+		)
+	}
+
+	return target.Type, fmt.Sprintf(
+		"target %q (type %s) was deleted; the delivery "+
+			"cannot be retried and has been failed terminally",
+		target.Name, target.Type,
+	)
+}
+
+// failDelivery records why a delivery is over and then marks it
+// failed. The caller must already own the delivery: every call site is
+// either a worker holding the reference runTask took, or a recovery
+// path that took one through retainIdle.
+//
+// The result row is written first and a failure to write it stops the
+// transition, which is what keeps a delivery from ending failed with
+// an empty event log — the state that leaves an operator with nothing
+// but a server log line to work out what happened. A delivery whose
+// reason could not be recorded stays in the non-terminal state it
+// already holds, where the sweep will find it again; see
+// bookkeepingFailed.
+//
+// The target type is a parameter rather than read off d because the
+// orphaned-retry callers deliberately hold a delivery loaded without
+// its Target relation: populating d.Target would make GORM's
+// SaveBeforeAssociations upsert the whole target row — plaintext
+// config, which for a slack target is the credential — into the
+// per-webhook event database. See
+// https://git.eeqj.de/sneak/webhooker/issues/206.
+func (e *Engine) failDelivery(
+	webhookDB *gorm.DB,
+	d *database.Delivery,
+	targetType database.TargetType,
+	reason string,
+) {
 	err := e.recordResult(
 		webhookDB,
 		d,
@@ -1150,14 +1344,8 @@ func (e *Engine) failUnretryableRetry(
 		return
 	}
 
-	// The type is passed rather than assigned onto d: the delivery
-	// is loaded here without its target relation, and populating
-	// d.Target would make GORM's SaveBeforeAssociations upsert the
-	// whole target row — plaintext config, which for a slack target
-	// is the credential — into the per-webhook event database. See
-	// https://git.eeqj.de/sneak/webhooker/issues/206.
 	e.settleStatus(
-		webhookDB, d, target.Type,
+		webhookDB, d, targetType,
 		database.DeliveryStatusFailed,
 	)
 }
@@ -1178,9 +1366,19 @@ func (e *Engine) processDelivery(
 			"type", d.Target.Type,
 		)
 
-		e.settleStatus(
+		// The reason is recorded, not just logged. This branch used
+		// to fail the delivery with no DeliveryResult at all, which
+		// showed in the event log as "failed, no attempts recorded
+		// yet" and left one server log line as the only account of
+		// why anywhere.
+		e.failDelivery(
 			webhookDB, d, d.Target.Type,
-			database.DeliveryStatusFailed,
+			fmt.Sprintf(
+				"unknown target type %q: this build has no "+
+					"delivery implementation for it, so no "+
+					"attempt was made",
+				d.Target.Type,
+			),
 		)
 
 		return

internal/delivery/engine_integration_test.go

@@ -377,6 +377,17 @@ func TestProcessRetryTask_SuccessfulRetry(t *testing.T) {
 
 	bodyStr := event.Body
 	cfg := iHTTPConfig(ts.URL)
+
+	// The target row exists because the engine confirms a scheduled
+	// retry's target has not been deleted before it runs it. A retry
+	// task whose target id names no row at all is a state the service
+	// does not produce: the handler read that target to build the
+	// task. See https://git.eeqj.de/sneak/webhooker/issues/107.
+	iCreateTarget(
+		t, s.MainDB, targetID, s.WebhookID, "retry-target",
+		database.TargetTypeHTTP, cfg, 5,
+	)
+
 	task := iTask(
 		d, event, s.WebhookID, targetID,
 		"retry-target", cfg, 5, 2, &bodyStr,
@@ -456,6 +467,12 @@ func TestProcessRetryTask_LargeBody_FetchFromDB(
 	)
 
 	cfg := iHTTPConfig(ts.URL)
+
+	iCreateTarget(
+		t, s.MainDB, targetID, s.WebhookID, "retry-large",
+		database.TargetTypeHTTP, cfg, 5,
+	)
+
 	task := iTask(
 		d, event, s.WebhookID, targetID,
 		"retry-large", cfg, 5, 2, nil,
@@ -558,6 +575,12 @@ func TestWorkerLifecycle_ProcessesRetryChannel(
 
 	bodyStr := event.Body
 	cfg := iHTTPConfig(ts.URL)
+
+	iCreateTarget(
+		t, s.MainDB, targetID, s.WebhookID, "retry-chan-test",
+		database.TargetTypeHTTP, cfg, 5,
+	)
+
 	task := iTask(
 		d, event, s.WebhookID, targetID,
 		"retry-chan-test", cfg, 5, 2, &bodyStr,

internal/delivery/event_db_isolation_test.go

@@ -96,7 +96,15 @@ func TestEventDBHoldsNoTargetRows(t *testing.T) {
 	)
 	assertNoTargetRows(t, dbPath)
 
-	// A retry.
+	// A retry. Its target exists in the main database, because the
+	// engine confirms a scheduled retry's target has not been
+	// deleted before running it; see
+	// https://git.eeqj.de/sneak/webhooker/issues/107.
+	iCreateTarget(
+		t, s.MainDB, targetID, s.WebhookID, "leaky-target",
+		database.TargetTypeHTTP, cfg, 5,
+	)
+
 	rd := iSeedDelivery(
 		t, s.WebhookDB, event.ID, targetID,
 		database.DeliveryStatusRetrying,

internal/delivery/metrics_test.go

@@ -229,6 +229,13 @@ func mExhaustRetries(t *testing.T, s iSetup) {
 	body := event.Body
 	cfg := iHTTPConfig(ts.URL)
 
+	// The retry below is only run if its target still exists; see
+	// https://git.eeqj.de/sneak/webhooker/issues/107.
+	iCreateTarget(
+		t, s.MainDB, targetID, s.WebhookID, "metrics-fail",
+		database.TargetTypeHTTP, cfg, 2,
+	)
+
 	first := iTask(
 		d, event, s.WebhookID, targetID,
 		"metrics-fail", cfg, 2, 1, &body,
@@ -289,6 +296,13 @@ func TestDeliveryMetrics_CircuitBreakerGauge(t *testing.T) {
 	// rather than the budget is what stops the delivery.
 	maxRetries := delivery.ExportDefaultFailureThreshold + 5
 
+	// The retries below are only run if their target still exists;
+	// see https://git.eeqj.de/sneak/webhooker/issues/107.
+	iCreateTarget(
+		t, s.MainDB, targetID, s.WebhookID, "metrics-trip",
+		database.TargetTypeHTTP, cfg, maxRetries,
+	)
+
 	first := iTask(
 		d, event, s.WebhookID, targetID,
 		"metrics-trip", cfg, maxRetries, 1, &body,
@@ -353,6 +367,11 @@ func TestDeliveryMetrics_BreakerBlockedIsNotAnAttempt(
 	cfg := iHTTPConfig(ts.URL)
 	maxRetries := delivery.ExportDefaultFailureThreshold + 5
 
+	iCreateTarget(
+		t, s.MainDB, targetID, s.WebhookID, "metrics-blocked",
+		database.TargetTypeHTTP, cfg, maxRetries,
+	)
+
 	first := iTask(
 		d, event, s.WebhookID, targetID,
 		"metrics-blocked", cfg, maxRetries, 1, &body,

internal/delivery/terminal_state_test.go

@@ -0,0 +1,531 @@
+package delivery_test
+
+import (
+	"context"
+	"net/http"
+	"net/http/httptest"
+	"sync/atomic"
+	"testing"
+
+	"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"
+)
+
+// The two terminal-state gaps of
+// https://git.eeqj.de/sneak/webhooker/issues/107: a delivery failed
+// with nothing in its event log to say why, and a retrying delivery
+// whose target was deleted, which used to keep sending and then never
+// terminalise.
+
+// tUnknownType is a target type no build implements. It stands in for
+// a target whose type was written by a build that knew a type this one
+// does not.
+const tUnknownType = database.TargetType("pubsub")
+
+// tSeedDeletedTarget creates a target, a retrying delivery against it
+// with one recorded failed attempt, and then deletes the target the
+// way the source page does.
+//
+// It asserts the delete is soft, because that is the whole reason the
+// engine could not tell a deleted target from a target id that never
+// named a row: the surviving row is invisible to a scoped read.
+func tSeedDeletedTarget(
+	t *testing.T,
+	s iSetup,
+	name, url string,
+) string {
+	t.Helper()
+
+	targetID := uuid.New().String()
+
+	iCreateTarget(
+		t, s.MainDB, targetID, s.WebhookID, name,
+		database.TargetTypeHTTP, iHTTPConfig(url), 5,
+	)
+
+	event := iSeedEvent(
+		t, s.WebhookDB, s.WebhookID, `{"target":"deleted"}`,
+	)
+
+	d := iSeedDelivery(
+		t, s.WebhookDB, event.ID, targetID,
+		database.DeliveryStatusRetrying,
+	)
+
+	iSeedFailedResult(t, s.WebhookDB, d.ID)
+
+	require.NoError(t, s.MainDB.Delete(
+		&database.Target{}, "id = ?", targetID,
+	).Error)
+
+	var scoped, unscoped int64
+
+	require.NoError(t, s.MainDB.
+		Model(&database.Target{}).
+		Where("id = ?", targetID).
+		Count(&scoped).Error)
+
+	require.NoError(t, s.MainDB.Unscoped().
+		Model(&database.Target{}).
+		Where("id = ?", targetID).
+		Count(&unscoped).Error)
+
+	require.Zero(t, scoped,
+		"the deleted target is still visible to a scoped read",
+	)
+	require.Equal(t, int64(1), unscoped,
+		"the delete was hard, so this test proves nothing about "+
+			"the soft-delete case it exists for",
+	)
+
+	return d.ID
+}
+
+// tLastResult returns a delivery's final recorded attempt, asserting
+// the expected number of them.
+func tLastResult(
+	t *testing.T,
+	s iSetup,
+	deliveryID string,
+	want int,
+) database.DeliveryResult {
+	t.Helper()
+
+	results := iResults(t, s.WebhookDB, deliveryID)
+	require.Len(t, results, want)
+
+	return results[want-1]
+}
+
+// --- 1. A failure with nothing recorded ---
+
+func TestProcessDelivery_UnknownTargetType_RecordsWhy(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	s := newISetup(t)
+
+	targetID := uuid.New().String()
+
+	event := iSeedEvent(
+		t, s.WebhookDB, s.WebhookID, `{"unknown":"type"}`,
+	)
+
+	seeded := iSeedDelivery(
+		t, s.WebhookDB, event.ID, targetID,
+		database.DeliveryStatusPending,
+	)
+
+	target := database.Target{
+		Name:   "mystery",
+		Type:   tUnknownType,
+		Config: iHTTPConfig("http://example.com/hook"),
+	}
+	target.ID = targetID
+
+	d := database.Delivery{
+		EventID:  event.ID,
+		TargetID: targetID,
+		Status:   database.DeliveryStatusPending,
+		Event:    event,
+		Target:   target,
+	}
+	d.ID = seeded.ID
+
+	body := event.Body
+	task := iTask(
+		seeded, event, s.WebhookID, targetID, "mystery",
+		target.Config, 0, 1, &body,
+	)
+	task.TargetType = tUnknownType
+
+	s.Engine.ExportProcessDelivery(
+		context.Background(), s.WebhookDB, &d, &task,
+	)
+
+	iAssertStatus(
+		t, s.WebhookDB, d.ID, database.DeliveryStatusFailed,
+	)
+
+	last := tLastResult(t, s, d.ID, 1)
+
+	assert.False(t, last.Success)
+	assert.Equal(t, 1, last.AttemptNum)
+	assert.Contains(t, last.Error, string(tUnknownType),
+		"the recorded reason does not name the offending type",
+	)
+}
+
+// --- 2. A retrying delivery whose target is gone ---
+
+func TestRecoverSingleRetry_TargetDeleted(t *testing.T) {
+	t.Parallel()
+
+	s := newISetup(t)
+
+	iCreateWebhook(
+		t, s.MainDB, s.WebhookID, "deleted-target-recovery",
+	)
+
+	deliveryID := tSeedDeletedTarget(
+		t, s, "gone-on-recovery", "http://example.com/hook",
+	)
+
+	s.Engine.ExportRecoverWebhookDeliveries(
+		context.Background(), s.WebhookID,
+	)
+
+	iAssertStatus(
+		t, s.WebhookDB, deliveryID,
+		database.DeliveryStatusFailed,
+	)
+
+	last := tLastResult(t, s, deliveryID, 2)
+
+	assert.False(t, last.Success)
+	assert.Equal(t, 2, last.AttemptNum)
+	assert.Contains(t, last.Error, "gone-on-recovery")
+	assert.Contains(t, last.Error, "was deleted")
+
+	assert.Empty(t, s.Engine.ExportRetryCh(),
+		"a delivery whose target is gone was rescheduled",
+	)
+	assert.Zero(t, s.Engine.ExportInflightHeld(),
+		"the terminal path leaked its ownership reference",
+	)
+}
+
+func TestSweepSingleRetry_TargetDeleted(t *testing.T) {
+	t.Parallel()
+
+	s := newISetup(t)
+
+	iCreateWebhook(
+		t, s.MainDB, s.WebhookID, "deleted-target-sweep",
+	)
+
+	deliveryID := tSeedDeletedTarget(
+		t, s, "gone-on-sweep", "http://example.com/hook",
+	)
+
+	// Twice, because the bug was an error the sweep repeated every
+	// minute for the life of the database: the second sweep must
+	// find nothing left to do.
+	s.Engine.ExportSweepWebhookRetries(
+		context.Background(), s.WebhookID,
+	)
+	s.Engine.ExportSweepWebhookRetries(
+		context.Background(), s.WebhookID,
+	)
+
+	iAssertStatus(
+		t, s.WebhookDB, deliveryID,
+		database.DeliveryStatusFailed,
+	)
+
+	last := tLastResult(t, s, deliveryID, 2)
+
+	assert.Contains(t, last.Error, "gone-on-sweep")
+	assert.Contains(t, last.Error, "was deleted")
+
+	assert.Empty(t, s.Engine.ExportRetryCh())
+	assert.Zero(t, s.Engine.ExportInflightHeld())
+}
+
+// TestSweepSingleRetry_TargetNeverExisted covers the other half of the
+// soft-delete distinction: an id with no row at all, deleted or
+// otherwise, must not be reported as something the operator deleted.
+func TestSweepSingleRetry_TargetNeverExisted(t *testing.T) {
+	t.Parallel()
+
+	s := newISetup(t)
+
+	iCreateWebhook(
+		t, s.MainDB, s.WebhookID, "target-never-existed",
+	)
+
+	targetID := uuid.New().String()
+
+	event := iSeedEvent(
+		t, s.WebhookDB, s.WebhookID, `{"target":"absent"}`,
+	)
+
+	d := iSeedDelivery(
+		t, s.WebhookDB, event.ID, targetID,
+		database.DeliveryStatusRetrying,
+	)
+
+	iSeedFailedResult(t, s.WebhookDB, d.ID)
+
+	s.Engine.ExportSweepWebhookRetries(
+		context.Background(), s.WebhookID,
+	)
+
+	iAssertStatus(
+		t, s.WebhookDB, d.ID, database.DeliveryStatusFailed,
+	)
+
+	last := tLastResult(t, s, d.ID, 2)
+
+	assert.Contains(t, last.Error, targetID)
+	assert.Contains(t, last.Error, "no longer exists")
+	assert.NotContains(t, last.Error, "was deleted",
+		"an id that never named a row was reported as a deletion",
+	)
+}
+
+// TestFailMissingTargetRetry_WritesNoTargetRow holds the new terminal
+// path to the same rule as the existing one: no target row, and so no
+// plaintext target config, may be written into the per-webhook event
+// database. See https://git.eeqj.de/sneak/webhooker/issues/206.
+func TestFailMissingTargetRetry_WritesNoTargetRow(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	s := newISetup(t)
+
+	iCreateWebhook(
+		t, s.MainDB, s.WebhookID, "no-target-row-deleted",
+	)
+
+	hookURL := "https://hooks.slack.com/services/T00/B00/x"
+
+	deliveryID := tSeedDeletedTarget(
+		t, s, "credential-bearing", hookURL,
+	)
+
+	s.Engine.ExportSweepWebhookRetries(
+		context.Background(), s.WebhookID,
+	)
+
+	iAssertStatus(
+		t, s.WebhookDB, deliveryID,
+		database.DeliveryStatusFailed,
+	)
+
+	var configs []string
+
+	require.NoError(t, s.WebhookDB.
+		Table("targets").
+		Pluck("config", &configs).Error)
+
+	assert.Empty(t, configs,
+		"the deleted-target terminal path wrote a target row "+
+			"into the per-webhook event database",
+	)
+}
+
+// --- 3. The scheduled retry chain ---
+
+// tRetryChainSetup wires a counting sink and a retrying delivery
+// against a live target pointing at it, and returns the task a
+// scheduled retry would carry — config and all, snapshotted as
+// ScheduleRetry snapshots it.
+func tRetryChainSetup(
+	t *testing.T,
+	s iSetup,
+	name string,
+	hits *atomic.Int64,
+) (delivery.Task, string) {
+	t.Helper()
+
+	ts := httptest.NewServer(http.HandlerFunc(
+		func(w http.ResponseWriter, _ *http.Request) {
+			hits.Add(1)
+			w.WriteHeader(http.StatusOK)
+		},
+	))
+	t.Cleanup(ts.Close)
+
+	iCreateWebhook(t, s.MainDB, s.WebhookID, name)
+
+	targetID := uuid.New().String()
+	cfg := iHTTPConfig(ts.URL)
+
+	iCreateTarget(
+		t, s.MainDB, targetID, s.WebhookID, name,
+		database.TargetTypeHTTP, cfg, 5,
+	)
+
+	event := iSeedEvent(
+		t, s.WebhookDB, s.WebhookID, `{"chain":"retry"}`,
+	)
+
+	d := iSeedDelivery(
+		t, s.WebhookDB, event.ID, targetID,
+		database.DeliveryStatusRetrying,
+	)
+
+	iSeedFailedResult(t, s.WebhookDB, d.ID)
+
+	body := event.Body
+
+	return iTask(
+		d, event, s.WebhookID, targetID, name, cfg, 5, 2, &body,
+	), targetID
+}
+
+// TestProcessRetryTask_TargetDeleted_MakesNoAttempt is the half the
+// deployability audit found worse than filed: terminalising on
+// recovery and sweep alone leaves the already-scheduled timer chain
+// running, and it holds the target's configuration from before the
+// deletion, so it goes on sending to a destination that was removed.
+func TestProcessRetryTask_TargetDeleted_MakesNoAttempt(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	s := newISetup(t)
+
+	var hits atomic.Int64
+
+	task, targetID := tRetryChainSetup(
+		t, s, "gone-mid-chain", &hits,
+	)
+
+	require.NoError(t, s.MainDB.Delete(
+		&database.Target{}, "id = ?", targetID,
+	).Error)
+
+	s.Engine.ExportProcessRetryTask(
+		context.Background(), &task,
+	)
+
+	assert.Zero(t, hits.Load(),
+		"a scheduled retry fired at a target the operator "+
+			"had already deleted",
+	)
+
+	iAssertStatus(
+		t, s.WebhookDB, task.DeliveryID,
+		database.DeliveryStatusFailed,
+	)
+
+	last := tLastResult(t, s, task.DeliveryID, 2)
+
+	assert.False(t, last.Success)
+	assert.Contains(t, last.Error, "was deleted")
+
+	assert.Zero(t, s.Engine.ExportInflightHeld())
+}
+
+// TestProcessRetryTask_TargetPresent_StillDelivers is the guard's
+// mutation check: a liveness check that refused every retry would pass
+// the test above and break every retry there is.
+func TestProcessRetryTask_TargetPresent_StillDelivers(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	s := newISetup(t)
+
+	var hits atomic.Int64
+
+	task, _ := tRetryChainSetup(t, s, "still-there", &hits)
+
+	s.Engine.ExportProcessRetryTask(
+		context.Background(), &task,
+	)
+
+	assert.Equal(t, int64(1), hits.Load())
+
+	iAssertStatus(
+		t, s.WebhookDB, task.DeliveryID,
+		database.DeliveryStatusDelivered,
+	)
+}
+
+// TestProcessRetryTask_TargetUnreadable_StillDelivers pins the other
+// half of the guard: only a target that is confirmed gone stops a
+// retry. A main database that cannot be read is a transient fault, and
+// a guard that abandoned deliveries on one would be a worse bug than
+// the one it fixes.
+func TestProcessRetryTask_TargetUnreadable_StillDelivers(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	s := newISetup(t)
+
+	var hits atomic.Int64
+
+	task, _ := tRetryChainSetup(t, s, "unreadable-main", &hits)
+
+	sqlDB, err := s.MainDB.DB()
+	require.NoError(t, err)
+	require.NoError(t, sqlDB.Close())
+
+	s.Engine.ExportProcessRetryTask(
+		context.Background(), &task,
+	)
+
+	assert.Equal(t, int64(1), hits.Load(),
+		"a retry was abandoned because the main database "+
+			"could not be read, not because its target was gone",
+	)
+
+	iAssertStatus(
+		t, s.WebhookDB, task.DeliveryID,
+		database.DeliveryStatusDelivered,
+	)
+}
+
+// TestRecoverSingleRetry_TargetUnreadable_LeavesDeliveryAlone is the
+// same rule on the recovery path. A read failure that is not
+// "record not found" must leave every retrying delivery of every
+// webhook exactly as it was.
+func TestRecoverSingleRetry_TargetUnreadable_LeavesDeliveryAlone(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	s := newISetup(t)
+
+	iCreateWebhook(
+		t, s.MainDB, s.WebhookID, "unreadable-on-recovery",
+	)
+
+	targetID := uuid.New().String()
+
+	iCreateTarget(
+		t, s.MainDB, targetID, s.WebhookID, "healthy",
+		database.TargetTypeHTTP,
+		iHTTPConfig("http://example.com/hook"), 5,
+	)
+
+	event := iSeedEvent(
+		t, s.WebhookDB, s.WebhookID, `{"still":"retrying"}`,
+	)
+
+	d := iSeedDelivery(
+		t, s.WebhookDB, event.ID, targetID,
+		database.DeliveryStatusRetrying,
+	)
+
+	iSeedFailedResult(t, s.WebhookDB, d.ID)
+
+	sqlDB, err := s.MainDB.DB()
+	require.NoError(t, err)
+	require.NoError(t, sqlDB.Close())
+
+	s.Engine.ExportRecoverRetryingDeliveries(
+		s.WebhookDB, s.WebhookID,
+	)
+
+	iAssertStatus(
+		t, s.WebhookDB, d.ID,
+		database.DeliveryStatusRetrying,
+	)
+
+	assert.Len(t, iResults(t, s.WebhookDB, d.ID), 1,
+		"an unreadable main database produced a terminal "+
+			"failure row",
+	)
+
+	assert.Zero(t, s.Engine.ExportInflightHeld())
+}