diff --git a/internal/delivery/engine.go b/internal/delivery/engine.go index d347a86..e3415c3 100644 --- a/internal/delivery/engine.go +++ b/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 diff --git a/internal/delivery/engine_integration_test.go b/internal/delivery/engine_integration_test.go index ba7fdc1..90d028e 100644 --- a/internal/delivery/engine_integration_test.go +++ b/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, diff --git a/internal/delivery/event_db_isolation_test.go b/internal/delivery/event_db_isolation_test.go index b865d5e..4bcd183 100644 --- a/internal/delivery/event_db_isolation_test.go +++ b/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, diff --git a/internal/delivery/metrics_test.go b/internal/delivery/metrics_test.go index 284114a..48492dd 100644 --- a/internal/delivery/metrics_test.go +++ b/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, diff --git a/internal/delivery/terminal_state_test.go b/internal/delivery/terminal_state_test.go new file mode 100644 index 0000000..382a6ff --- /dev/null +++ b/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()) +}