diff --git a/README.md b/README.md index 277d3b3..ca836d8 100644 --- a/README.md +++ b/README.md @@ -1591,29 +1591,27 @@ did not arrive on the URL and counts in neither. A delivery destination for events. Each target defines where and how events should be forwarded. -| Field | Type | Description | -| ------------- | ---------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `id` | UUID | Primary key | -| `webhook_id` | UUID | Foreign key → Webhook | -| `name` | string | Human-readable name | -| `type` | TargetType | One of: `http`, `slack`, `database`, `log` | -| `active` | boolean | Whether deliveries are enabled (default: true) | -| `config` | JSON text | Type-specific configuration | -| `max_retries` | integer | Total delivery attempts for `http` and `slack` targets, not retries on top of the first: 0 is a single fire-and-forget attempt with no retries and no circuit breaker, and a value of N makes N attempts in all, with exponential backoff and a per-target circuit breaker. Ignored by `database` and `log` targets. The web UI labels it Delivery attempts | +| Field | Type | Description | +| -------------- | ---------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `id` | UUID | Primary key | +| `webhook_id` | UUID | Foreign key → Webhook | +| `name` | string | Human-readable name | +| `type` | TargetType | One of: `http`, `slack`, `database`, `log` | +| `active` | boolean | Whether deliveries are enabled (default: true) | +| `config` | JSON text | Type-specific configuration | +| `max_attempts` | integer | How many times an `http` or `slack` target attempts each delivery in all: at least 1, and 1 by default. Ignored by `database` and `log` targets. The web UI labels it Delivery attempts | **Relations:** Belongs to Webhook. Has many Deliveries. **Target types:** -- **`http`** — Forward the event as an HTTP POST to a configured URL. - `max_retries` is the total number of delivery attempts, not retries on top of - the first: when `max_retries` is 0 (the default), the target operates in - fire-and-forget mode, a single attempt with no retries and no circuit breaker; - a value of N makes up to N attempts in all, retrying failed deliveries with - exponential backoff and protecting them with a per-target circuit breaker. +- **`http`** — Forward the event as an HTTP POST to a configured URL. With + `max_attempts` at 1 the target makes a single attempt; above 1 it retries a + failed delivery with exponential backoff, up to that many attempts in all, + behind a per-target circuit breaker. - **`slack`** — Post the event as a formatted message to a Slack-compatible incoming webhook URL (`webhookUrl` in `config`). It is built on the same HTTP - core as `http` and honours `max_retries` identically, circuit breaker + core as `http` and honours `max_attempts` identically, circuit breaker included. See the Slack target section under "Per-Webhook Event Databases" for the message format. - **`database`** — Archive the full event as a row into the target's own archive @@ -2186,11 +2184,11 @@ External Service ▼ ▼ ▼ ┌────────────┐ ┌────────────┐ ┌────────────┐ │ HTTP Target│ │ HTTP Target│ │ Log Target │ - │(max_retries│ │(max_retries│ │ (stdout) │ - │ == 0) │ │ > 0, │ └────────────┘ - │ fire+forget│ │ backoff + │ + │max_attempts│ │max_attempts│ │ (stdout) │ + │ == 1, │ │ > 1, │ └────────────┘ + │ one attempt│ │ backoff + │ └────────────┘ │ circuit │ - │ breaker) │ + │ breaker │ └────────────┘ ``` @@ -2267,7 +2265,7 @@ re-inject it to every currently active target. ### Circuit Breaker (HTTP and Slack Targets with Retries) -`http` and `slack` targets with `max_retries` > 0 are protected by a +`http` and `slack` targets with `max_attempts` > 1 are protected by a **per-target circuit breaker** that prevents hammering a down target with repeated failed delivery attempts. The circuit breaker is in-memory only and resets on restart (which is fine — startup recovery rescans the database @@ -2308,10 +2306,10 @@ anyway). - **Cooldown:** 30 seconds in open state before probing **Scope:** Circuit breakers apply to **`http` and `slack` targets with -`max_retries` > 0**. The Slack target is built on the same HTTP core and hands -its own `max_retries` to the same retry path, so it gets a breaker with the same -5-failure / 30-second defaults. Fire-and-forget targets of either type -(`max_retries` == 0), database targets (local operations), and log targets +`max_attempts` > 1**. The Slack target is built on the same HTTP core and hands +its own `max_attempts` to the same retry path, so it gets a breaker with the +same 5-failure / 30-second defaults. Single-attempt targets of either type +(`max_attempts` == 1), database targets (local operations), and log targets (stdout) do not use circuit breakers. When a circuit is open and a new delivery arrives, the engine marks the delivery diff --git a/internal/database/event_db_isolation_test.go b/internal/database/event_db_isolation_test.go index 6f38dbb..762a415 100644 --- a/internal/database/event_db_isolation_test.go +++ b/internal/database/event_db_isolation_test.go @@ -25,10 +25,11 @@ func TestEventDBCreateOmitsAssociations(t *testing.T) { require.NoError(t, err) target := database.Target{ - WebhookID: webhookID, - Name: "leaky-target", - Type: database.TargetTypeSlack, - Config: `{"webhookUrl":"https://hooks.example/secret"}`, + WebhookID: webhookID, + Name: "leaky-target", + Type: database.TargetTypeSlack, + Config: `{"webhookUrl":"https://hooks.example/secret"}`, + MaxAttempts: 1, } target.ID = uuid.New().String() diff --git a/internal/database/model_parent_test.go b/internal/database/model_parent_test.go index 2c9ee9e..28b8421 100644 --- a/internal/database/model_parent_test.go +++ b/internal/database/model_parent_test.go @@ -23,8 +23,9 @@ func TestPreloadedModelsMarshalWithoutTheirParent(t *testing.T) { Name: testWebhookName, Entrypoints: []database.Entrypoint{{Path: uuid.New().String()}}, Targets: []database.Target{{ - Name: "log", - Type: database.TargetTypeLog, + Name: "log", + Type: database.TargetTypeLog, + MaxAttempts: 1, }}, } require.NoError(t, db.Create(&stored).Error) diff --git a/internal/database/model_secrets_test.go b/internal/database/model_secrets_test.go index 84bd98f..37fb4d0 100644 --- a/internal/database/model_secrets_test.go +++ b/internal/database/model_secrets_test.go @@ -46,9 +46,10 @@ func TestModelsDoNotMarshalTheirSecrets(t *testing.T) { { name: "target config", model: database.Target{ - Name: keptField, - Type: database.TargetTypeSlack, - Config: `{"webhookUrl":"https://h/s/` + marker + `"}`, + Name: keptField, + Type: database.TargetTypeSlack, + Config: `{"webhookUrl":"https://h/s/` + marker + `"}`, + MaxAttempts: 1, }, }, { @@ -97,8 +98,9 @@ func TestWebhookMarshalsNoTargetConfig(t *testing.T) { encoded := marshalModel(t, database.Webhook{ Name: keptField, Targets: []database.Target{{ - Name: "slack", - Config: `{"webhookUrl":"https://h/s/` + marker + `"}`, + Name: "slack", + Config: `{"webhookUrl":"https://h/s/` + marker + `"}`, + MaxAttempts: 1, }}, }) diff --git a/internal/database/model_target.go b/internal/database/model_target.go index c10127c..c38dad5 100644 --- a/internal/database/model_target.go +++ b/internal/database/model_target.go @@ -29,9 +29,12 @@ type Target struct { // handler that marshals the model itself. Config string `gorm:"type:text" json:"-"` // JSON configuration - // For HTTP targets (max_retries=0 means fire-and-forget, - // >0 enables retries with backoff) - MaxRetries int `json:"maxRetries,omitempty"` + // MaxAttempts is how many times an HTTP or Slack target attempts + // each delivery in all, at least 1. 1 is a single attempt with no + // circuit breaker; above 1, a failed attempt is retried with + // backoff behind the breaker. The table's check constraint refuses + // a target saved with less than 1, whatever its type. + MaxAttempts int `gorm:"not null;check:max_attempts >= 1" json:"maxAttempts,omitempty"` // Relations. No model marshals the record it belongs to: // Webhook.Targets leads back here, and the JSON could loop. diff --git a/internal/database/model_target_test.go b/internal/database/model_target_test.go new file mode 100644 index 0000000..76fcd7c --- /dev/null +++ b/internal/database/model_target_test.go @@ -0,0 +1,44 @@ +package database_test + +import ( + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/webhooker/internal/database" +) + +// newHTTPTarget returns an http target that makes maxAttempts attempts +// at each delivery. +func newHTTPTarget(maxAttempts int) *database.Target { + return &database.Target{ + WebhookID: uuid.New().String(), + Name: "target", + Type: database.TargetTypeHTTP, + Config: `{"url":"https://example.com/"}`, + MaxAttempts: maxAttempts, + } +} + +// TestTargetCreate_RefusesZeroAttempts relies on the targets table +// alone: nothing in the model checks the number, so the check +// constraint is what keeps a target with 0 attempts out of the +// database. +func TestTargetCreate_RefusesZeroAttempts(t *testing.T) { + t.Parallel() + + db := startedTestDB(t) + + err := db.Create(newHTTPTarget(0)).Error + require.Error(t, err) + assert.Contains(t, err.Error(), "CHECK constraint failed") +} + +func TestTargetCreate_AcceptsOneAttempt(t *testing.T) { + t.Parallel() + + db := startedTestDB(t) + + require.NoError(t, db.Create(newHTTPTarget(1)).Error) +} diff --git a/internal/database/schema_test.go b/internal/database/schema_test.go index cc378ff..10e2d75 100644 --- a/internal/database/schema_test.go +++ b/internal/database/schema_test.go @@ -87,13 +87,13 @@ func TestMainSchemaHoldsEveryModel(t *testing.T) { Active: true, }) assertRoundTrip(t, db, &database.Target{ - BaseModel: newBaseModel(), - WebhookID: uuid.New().String(), - Name: "target", - Type: database.TargetTypeHTTP, - Active: true, - Config: `{"url":"https://example.com/"}`, - MaxRetries: 3, + BaseModel: newBaseModel(), + WebhookID: uuid.New().String(), + Name: "target", + Type: database.TargetTypeHTTP, + Active: true, + Config: `{"url":"https://example.com/"}`, + MaxAttempts: 3, }) } diff --git a/internal/db/migrations/main/001_schema.sql b/internal/db/migrations/main/001_schema.sql index 4911a0d..8f4cb38 100644 --- a/internal/db/migrations/main/001_schema.sql +++ b/internal/db/migrations/main/001_schema.sql @@ -80,10 +80,11 @@ CREATE TABLE targets ( type text NOT NULL, active numeric DEFAULT true, config text, - max_retries integer, + max_attempts integer NOT NULL, PRIMARY KEY (id), CONSTRAINT fk_webhooks_targets - FOREIGN KEY (webhook_id) REFERENCES webhooks (id) + FOREIGN KEY (webhook_id) REFERENCES webhooks (id), + CONSTRAINT chk_targets_max_attempts CHECK (max_attempts >= 1) ); CREATE INDEX idx_targets_deleted_at ON targets (deleted_at); diff --git a/internal/delivery/archive_sweeper_test.go b/internal/delivery/archive_sweeper_test.go index bbd0418..46a1f3a 100644 --- a/internal/delivery/archive_sweeper_test.go +++ b/internal/delivery/archive_sweeper_test.go @@ -124,11 +124,12 @@ func (env *archiveEnv) addDatabaseTarget( t.Helper() tgt := &database.Target{ - WebhookID: webhookID, - Name: "Archive", - Type: database.TargetTypeDatabase, - Active: true, - Config: configJSON, + WebhookID: webhookID, + Name: "Archive", + Type: database.TargetTypeDatabase, + Active: true, + Config: configJSON, + MaxAttempts: 1, } require.NoError( t, diff --git a/internal/delivery/engine.go b/internal/delivery/engine.go index be78e66..cde4128 100644 --- a/internal/delivery/engine.go +++ b/internal/delivery/engine.go @@ -107,7 +107,7 @@ type Task struct { TargetName string TargetType database.TargetType TargetConfig string - MaxRetries int + MaxAttempts int Method string RawQuery string @@ -1769,10 +1769,10 @@ func buildEventFromTask(task *Task) database.Event { func buildTargetFromTask(task *Task) database.Target { target := database.Target{ - Name: task.TargetName, - Type: task.TargetType, - Config: task.TargetConfig, - MaxRetries: task.MaxRetries, + Name: task.TargetName, + Type: task.TargetType, + Config: task.TargetConfig, + MaxAttempts: task.MaxAttempts, } target.ID = task.TargetID @@ -2028,7 +2028,7 @@ func (e *Engine) countAttemptsBatch( // 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. + // delivery is bounded by the target's MaxAttempts. var attemptIDs []string err := webhookDB. @@ -2106,7 +2106,7 @@ func buildRecoveryTask( TargetName: target.Name, TargetType: target.Type, TargetConfig: target.Config, - MaxRetries: target.MaxRetries, + MaxAttempts: target.MaxAttempts, Method: event.Method, RawQuery: event.RawQuery, Headers: event.Headers, diff --git a/internal/delivery/engine_integration_test.go b/internal/delivery/engine_integration_test.go index 2812281..14580c6 100644 --- a/internal/delivery/engine_integration_test.go +++ b/internal/delivery/engine_integration_test.go @@ -170,7 +170,7 @@ func iTask( d database.Delivery, event database.Event, webhookID, targetID, name, config string, - maxRetries, attemptNum int, + maxAttempts, attemptNum int, body *string, ) delivery.Task { return delivery.Task{ @@ -181,7 +181,7 @@ func iTask( TargetName: name, TargetType: database.TargetTypeHTTP, TargetConfig: config, - MaxRetries: maxRetries, + MaxAttempts: maxAttempts, Method: event.Method, Headers: event.Headers, ContentType: event.ContentType, @@ -242,7 +242,7 @@ func TestProcessNewTask_InlineBody(t *testing.T) { cfg := iHTTPConfig(ts.URL) task := iTask( d, event, s.WebhookID, targetID, - "test-target", cfg, 0, 1, &bodyStr, + "test-target", cfg, 1, 1, &bodyStr, ) s.Engine.ExportProcessNewTask( @@ -293,7 +293,7 @@ func TestProcessNewTask_LargeBody_FetchFromDB( cfg := iHTTPConfig(ts.URL) task := iTask( d, event, s.WebhookID, targetID, - "test-large", cfg, 0, 1, nil, + "test-large", cfg, 1, 1, nil, ) s.Engine.ExportProcessNewTask( @@ -320,7 +320,7 @@ func TestProcessNewTask_InvalidWebhookID(t *testing.T) { TargetName: "test", TargetType: database.TargetTypeHTTP, TargetConfig: iHTTPConfig("http://localhost:9999"), - MaxRetries: 0, + MaxAttempts: 1, Body: nil, AttemptNum: 1, } @@ -506,7 +506,7 @@ func TestWorkerLifecycle_StartStop(t *testing.T) { task := iTask( d, event, s.WebhookID, targetID, "lifecycle-test", "", - 0, 1, &bodyStr, + 1, 1, &bodyStr, ) task.TargetType = database.TargetTypeLog @@ -645,7 +645,7 @@ func TestRecoverPendingDeliveries(t *testing.T) { iCreateTarget(t, s.MainDB, targetID, s.WebhookID, "recovery-target", - database.TargetTypeLog, "", 0, + database.TargetTypeLog, "", 1, ) iSeedPendingDeliveries( @@ -686,17 +686,17 @@ func iCreateTarget( targetID, webhookID, name string, targetType database.TargetType, config string, - maxRetries int, + maxAttempts int, ) { t.Helper() target := database.Target{ - WebhookID: webhookID, - Name: name, - Type: targetType, - Active: true, - Config: config, - MaxRetries: maxRetries, + WebhookID: webhookID, + Name: name, + Type: targetType, + Active: true, + Config: config, + MaxAttempts: maxAttempts, } target.ID = targetID @@ -1077,7 +1077,7 @@ func TestRecoverInFlight_WithPendingDeliveries( iCreateTarget(t, s.MainDB, targetID, s.WebhookID, "recover-target", - database.TargetTypeLog, "", 0, + database.TargetTypeLog, "", 1, ) event := iSeedEvent( @@ -1232,7 +1232,7 @@ func TestDeliverHTTP_CustomTargetHeaders(t *testing.T) { task := iTask( d, event, s.WebhookID, targetID, "auth-target", string(cfgJSON), - 0, 1, &bodyStr, + 1, 1, &bodyStr, ) s.Engine.ExportProcessNewTask( @@ -1277,7 +1277,7 @@ func TestDeliverHTTP_TargetTimeout(t *testing.T) { task, d := iHTTPTaskAndDelivery( event, del, "timeout-target", - string(cfgJSON), 0, 1, + string(cfgJSON), 1, 1, ) e.ExportDeliverHTTP(context.TODO(), db, d, task) @@ -1339,7 +1339,7 @@ func TestDeliverHTTP_CutsStoredResponseAtMaxBodyLog( bodyStr := event.Body task := iTask( del, event, s.WebhookID, targetID, - "cut-target", cfgJSON, 0, 1, &bodyStr, + "cut-target", cfgJSON, 1, 1, &bodyStr, ) s.Engine.ExportProcessNewTask(context.TODO(), &task) @@ -1403,7 +1403,7 @@ func iHTTPTaskAndDelivery( event database.Event, del database.Delivery, name, config string, - maxRetries, attemptNum int, + maxAttempts, attemptNum int, ) (*delivery.Task, *database.Delivery) { task := &delivery.Task{ DeliveryID: del.ID, @@ -1413,7 +1413,7 @@ func iHTTPTaskAndDelivery( TargetName: name, TargetType: database.TargetTypeHTTP, TargetConfig: config, - MaxRetries: maxRetries, + MaxAttempts: maxAttempts, AttemptNum: attemptNum, } @@ -1423,9 +1423,10 @@ func iHTTPTaskAndDelivery( Status: database.DeliveryStatusPending, Event: event, Target: database.Target{ - Name: name, - Type: database.TargetTypeHTTP, - Config: config, + Name: name, + Type: database.TargetTypeHTTP, + Config: config, + MaxAttempts: maxAttempts, }, } d.ID = del.ID @@ -1466,7 +1467,7 @@ func TestDeliverHTTP_InvalidConfig(t *testing.T) { ) task, d := iHTTPTaskAndDelivery( - event, del, "bad-config", `not-json`, 0, 1, + event, del, "bad-config", `not-json`, 1, 1, ) e.ExportDeliverHTTP(context.TODO(), db, d, task) @@ -1490,7 +1491,7 @@ func TestDeliverHTTP_InvalidConfigUnrecordedStaysPending(t *testing.T) { ) task, d := iHTTPTaskAndDelivery( - event, del, "bad-config", `not-json`, 0, 1, + event, del, "bad-config", `not-json`, 1, 1, ) require.NoError(t, db.Exec("drop table delivery_results").Error) diff --git a/internal/delivery/engine_lifecycle_test.go b/internal/delivery/engine_lifecycle_test.go index 7db4258..1775fc8 100644 --- a/internal/delivery/engine_lifecycle_test.go +++ b/internal/delivery/engine_lifecycle_test.go @@ -149,7 +149,7 @@ func seedLogTask( bodyStr := event.Body task := iTask( d, event, s.WebhookID, targetID, - "hook-context-test", "", 0, 1, &bodyStr, + "hook-context-test", "", 1, 1, &bodyStr, ) task.TargetType = database.TargetTypeLog @@ -278,7 +278,7 @@ func TestEngine_StopHookHonoursStopTimeout(t *testing.T) { } // seedHeldDelivery gives the setup's webhook an http target with -// maxRetries, pointed at a receiver that keeps every request open until +// maxAttempts, pointed at a receiver that keeps every request open until // the client gives up on it, and seeds one delivery to that target at // status. It returns the task that sends attempt attemptNum of the // delivery, and a channel that is closed when a request reaches the @@ -286,7 +286,7 @@ func TestEngine_StopHookHonoursStopTimeout(t *testing.T) { func seedHeldDelivery( t *testing.T, s iSetup, - maxRetries int, + maxAttempts int, status database.DeliveryStatus, attemptNum int, ) (delivery.Task, <-chan struct{}) { @@ -315,7 +315,7 @@ func seedHeldDelivery( iCreateWebhook(t, s.MainDB, s.WebhookID, "hook") iCreateTarget( t, s.MainDB, targetID, s.WebhookID, "held", - database.TargetTypeHTTP, cfg, maxRetries, + database.TargetTypeHTTP, cfg, maxAttempts, ) event := iSeedEvent(t, s.WebhookDB, s.WebhookID, `{"held":true}`) @@ -325,7 +325,7 @@ func seedHeldDelivery( return iTask( d, event, s.WebhookID, targetID, "held", cfg, - maxRetries, attemptNum, &bodyStr, + maxAttempts, attemptNum, &bodyStr, ), arrived } @@ -380,7 +380,7 @@ func TestEngine_StopLeavesFireAndForgetAttemptPending(t *testing.T) { lc := startEngineViaHook(t, s.Engine) task, arrived := seedHeldDelivery( - t, s, 0, database.DeliveryStatusPending, 1, + t, s, 1, database.DeliveryStatusPending, 1, ) s.Engine.Notify([]delivery.Task{task}) @@ -404,13 +404,13 @@ func TestEngine_StopLeavesFireAndForgetAttemptPending(t *testing.T) { func TestEngine_StopLeavesLastRetryAttemptRetrying(t *testing.T) { t.Parallel() - const maxRetries = 2 + const maxAttempts = 2 s := newISetup(t) lc := startEngineViaHook(t, s.Engine) task, arrived := seedHeldDelivery( - t, s, maxRetries, database.DeliveryStatusRetrying, maxRetries, + t, s, maxAttempts, database.DeliveryStatusRetrying, maxAttempts, ) iSeedFailedResult(t, s.WebhookDB, task.DeliveryID) @@ -435,9 +435,10 @@ func deliverToArchive(t *testing.T, s iSetup) (string, string) { iCreateWebhook(t, s.MainDB, s.WebhookID, "hook") tgt := &database.Target{ - WebhookID: s.WebhookID, - Name: "archive", - Type: database.TargetTypeDatabase, + WebhookID: s.WebhookID, + Name: "archive", + Type: database.TargetTypeDatabase, + MaxAttempts: 1, } require.NoError( t, s.MainDB.Omit(clause.Associations).Create(tgt).Error, diff --git a/internal/delivery/engine_test.go b/internal/delivery/engine_test.go index 7ebb016..52a3d44 100644 --- a/internal/delivery/engine_test.go +++ b/internal/delivery/engine_test.go @@ -142,7 +142,7 @@ func buildHTTPFixture( dlv database.Delivery, event database.Event, targetID, name, config string, - maxRetries, attemptNum int, + maxAttempts, attemptNum int, ) httpDeliveryFixture { task := &delivery.Task{ DeliveryID: dlv.ID, @@ -152,7 +152,7 @@ func buildHTTPFixture( TargetName: name, TargetType: database.TargetTypeHTTP, TargetConfig: config, - MaxRetries: maxRetries, + MaxAttempts: maxAttempts, AttemptNum: attemptNum, } @@ -162,10 +162,10 @@ func buildHTTPFixture( Status: dlv.Status, Event: event, Target: database.Target{ - Name: name, - Type: database.TargetTypeHTTP, - Config: config, - MaxRetries: maxRetries, + Name: name, + Type: database.TargetTypeHTTP, + Config: config, + MaxAttempts: maxAttempts, }, } d.ID = dlv.ID @@ -277,7 +277,7 @@ func TestDeliverHTTP_Success(t *testing.T) { cfg := newHTTPTargetConfig(ts.URL) fix := buildHTTPFixture( - dlv, event, targetID, "test-http", cfg, 0, 1, + dlv, event, targetID, "test-http", cfg, 1, 1, ) e.ExportDeliverHTTP( @@ -324,7 +324,7 @@ func TestDeliverHTTP_Failure(t *testing.T) { cfg := newHTTPTargetConfig(ts.URL) fix := buildHTTPFixture( dlv, event, targetID, - "test-http-fail", cfg, 0, 1, + "test-http-fail", cfg, 1, 1, ) e.ExportDeliverHTTP( @@ -473,14 +473,23 @@ func TestDeliverHTTP_WithRetries_Success(t *testing.T) { ) } -func TestDeliverHTTP_MaxRetriesExhausted(t *testing.T) { +// TestDeliverHTTP_MaxAttemptsExhausted proves a target with three +// delivery attempts makes at most three: the first two failures are +// retried, the third is final, and each attempt goes through the +// target's circuit breaker. +func TestDeliverHTTP_MaxAttemptsExhausted(t *testing.T) { t.Parallel() + const maxAttempts = 3 + db := testWebhookDB(t) + var requests atomic.Int32 + ts := httptest.NewServer( http.HandlerFunc( func(w http.ResponseWriter, _ *http.Request) { + requests.Add(1) w.WriteHeader(http.StatusBadGateway) }, ), @@ -489,26 +498,103 @@ func TestDeliverHTTP_MaxRetriesExhausted(t *testing.T) { e := testEngine(t, 1) targetID := uuid.New().String() + + // It opens on the last failure only if every attempt reached it. + cb := delivery.NewTestCircuitBreaker(maxAttempts, time.Minute) + e.ExportSetCircuitBreaker(targetID, cb) + event := seedEvent(t, db, `{"retry":"exhaust"}`) dlv := seedDelivery( t, db, event.ID, targetID, - database.DeliveryStatusRetrying, + database.DeliveryStatusPending, + ) + + cfg := newHTTPTargetConfig(ts.URL) + sched := &recordingScheduler{} + + for attempt := 1; attempt <= maxAttempts; attempt++ { + fix := buildHTTPFixture( + dlv, event, targetID, + "test-http-exhaust", cfg, maxAttempts, attempt, + ) + + e.ExportDeliverHTTPWithScheduler( + context.TODO(), db, fix.Delivery, fix.Task, sched, + ) + } + + assert.Equal(t, int32(maxAttempts), requests.Load()) + assert.Len(t, sched.delays, maxAttempts-1, + "every failure but the last should be retried", + ) + assertDeliveryStatus(t, db, dlv.ID, + database.DeliveryStatusFailed, + ) + assert.Equal(t, delivery.CircuitOpen, cb.State(), + "every attempt should be counted by the circuit breaker", + ) +} + +// TestDeliverHTTP_SingleAttemptSkipsCircuitBreaker proves a target +// with one delivery attempt makes exactly that attempt, even while +// its circuit breaker is open, and that a failure is final. +func TestDeliverHTTP_SingleAttemptSkipsCircuitBreaker(t *testing.T) { + t.Parallel() + + db := testWebhookDB(t) + + var requests atomic.Int32 + + ts := httptest.NewServer( + http.HandlerFunc( + func(w http.ResponseWriter, _ *http.Request) { + requests.Add(1) + w.WriteHeader(http.StatusBadGateway) + }, + ), + ) + defer ts.Close() + + e := testEngine(t, 1) + targetID := uuid.New().String() + + cb := e.ExportGetCircuitBreaker(targetID) + + for range delivery.ExportDefaultFailureThreshold { + cb.RecordFailure() + } + + require.Equal(t, delivery.CircuitOpen, cb.State()) + + event := seedEvent(t, db, `{"retry":"none"}`) + + dlv := seedDelivery( + t, db, event.ID, targetID, + database.DeliveryStatusPending, ) cfg := newHTTPTargetConfig(ts.URL) fix := buildHTTPFixture( dlv, event, targetID, - "test-http-exhaust", cfg, 3, 3, + "test-http-single", cfg, 1, 1, + ) + sched := &recordingScheduler{} + + e.ExportDeliverHTTPWithScheduler( + context.TODO(), db, fix.Delivery, fix.Task, sched, ) - e.ExportDeliverHTTP( - context.TODO(), db, fix.Delivery, fix.Task, + assert.Equal(t, int32(1), requests.Load(), + "the open circuit breaker should not hold the attempt back", ) - + assert.Empty(t, sched.delays, "no retry should be scheduled") assertDeliveryStatus(t, db, dlv.ID, database.DeliveryStatusFailed, ) + assertDeliveryResult( + t, db, dlv.ID, false, http.StatusBadGateway, + ) } func TestDeliverHTTP_SchedulesRetryOnFailure( @@ -727,9 +813,10 @@ func seedConcurrencyTasks( Status: database.DeliveryStatusPending, Event: event, Target: database.Target{ - Name: fmt.Sprintf("task-%d", i), - Type: database.TargetTypeHTTP, - Config: targetCfg, + Name: fmt.Sprintf("task-%d", i), + Type: database.TargetTypeHTTP, + Config: targetCfg, + MaxAttempts: 1, }, } tasks[i].ID = dlv.ID @@ -741,7 +828,7 @@ func seedConcurrencyTasks( TargetName: tasks[i].Target.Name, TargetType: tasks[i].Target.Type, TargetConfig: tasks[i].Target.Config, - MaxRetries: 0, + MaxAttempts: 1, AttemptNum: 1, } } @@ -1688,9 +1775,10 @@ func buildSlackDelivery( Status: dlv.Status, Event: event, Target: database.Target{ - Name: name, - Type: database.TargetTypeSlack, - Config: slackCfg, + Name: name, + Type: database.TargetTypeSlack, + Config: slackCfg, + MaxAttempts: 1, }, } d.ID = dlv.ID @@ -1874,9 +1962,10 @@ func TestDeliverSlack_InvalidConfig(t *testing.T) { Status: database.DeliveryStatusPending, Event: event, Target: database.Target{ - Name: "test-slack-bad", - Type: database.TargetTypeSlack, - Config: `{"not_webhook_url":"missing"}`, + Name: "test-slack-bad", + Type: database.TargetTypeSlack, + Config: `{"not_webhook_url":"missing"}`, + MaxAttempts: 1, }, } d.ID = dlv.ID @@ -1933,8 +2022,9 @@ func TestProcessDelivery_RoutesToSlack(t *testing.T) { ) task := &delivery.Task{ - DeliveryID: dlv.ID, - TargetType: database.TargetTypeSlack, + DeliveryID: dlv.ID, + TargetType: database.TargetTypeSlack, + MaxAttempts: 1, } e.ExportProcessDelivery( @@ -2051,10 +2141,10 @@ func buildSlackRetryDelivery( Status: database.DeliveryStatusPending, Event: event, Target: database.Target{ - Name: "test-slack-retry", - Type: database.TargetTypeSlack, - Config: cfg, - MaxRetries: 5, + Name: "test-slack-retry", + Type: database.TargetTypeSlack, + Config: cfg, + MaxAttempts: 5, }, } d.ID = dlv.ID @@ -2089,11 +2179,11 @@ func TestDeliverSlack_WithRetries_SchedulesRetry( ) task := &delivery.Task{ - DeliveryID: dlv.ID, - TargetID: targetID, - TargetType: database.TargetTypeSlack, - MaxRetries: 5, - AttemptNum: 1, + DeliveryID: dlv.ID, + TargetID: targetID, + TargetType: database.TargetTypeSlack, + MaxAttempts: 5, + AttemptNum: 1, } e.ExportProcessDelivery(context.TODO(), db, d, task) diff --git a/internal/delivery/event_db_isolation_test.go b/internal/delivery/event_db_isolation_test.go index 4bcd183..2428b65 100644 --- a/internal/delivery/event_db_isolation_test.go +++ b/internal/delivery/event_db_isolation_test.go @@ -152,7 +152,7 @@ func TestEventDBHoldsNoTargetRowsOnFailedDelivery(t *testing.T) { ) task := iTask( d, event, s.WebhookID, targetID, - "leaky-target", cfg, 0, 1, &body, + "leaky-target", cfg, 1, 1, &body, ) s.Engine.ExportProcessNewTask(context.TODO(), &task) diff --git a/internal/delivery/event_timestamp_test.go b/internal/delivery/event_timestamp_test.go index 09e70cf..14b39a0 100644 --- a/internal/delivery/event_timestamp_test.go +++ b/internal/delivery/event_timestamp_test.go @@ -147,11 +147,12 @@ func tsSeedTarget( t.Helper() target := database.Target{ - WebhookID: webhookID, - Name: "slack-sink", - Type: database.TargetTypeSlack, - Config: config, - Active: true, + WebhookID: webhookID, + Name: "slack-sink", + Type: database.TargetTypeSlack, + Config: config, + Active: true, + MaxAttempts: 1, } require.NoError(t, mainDB.Create(&target).Error) @@ -176,7 +177,7 @@ func tsTask( TargetName: target.Name, TargetType: database.TargetTypeSlack, TargetConfig: target.Config, - MaxRetries: 0, + MaxAttempts: 1, Method: event.Method, Headers: event.Headers, ContentType: event.ContentType, diff --git a/internal/delivery/export_test.go b/internal/delivery/export_test.go index 8c9aeb4..51a3e61 100644 --- a/internal/delivery/export_test.go +++ b/internal/delivery/export_test.go @@ -142,9 +142,10 @@ func (e *Engine) ExportDeliverSlack( d *database.Delivery, ) { task := &Task{ - DeliveryID: d.ID, - TargetID: d.TargetID, - AttemptNum: 1, + DeliveryID: d.ID, + TargetID: d.TargetID, + MaxAttempts: d.Target.MaxAttempts, + AttemptNum: 1, } e.targets[database.TargetTypeSlack].Deliver( diff --git a/internal/delivery/inflight_test.go b/internal/delivery/inflight_test.go index 059aa5a..c50e3c0 100644 --- a/internal/delivery/inflight_test.go +++ b/internal/delivery/inflight_test.go @@ -32,7 +32,7 @@ func fSweepSetup( iCreateTarget(t, s.MainDB, targetID, s.WebhookID, name, - database.TargetTypeLog, "", 0, + database.TargetTypeLog, "", 1, ) require.NoError(t, s.MainDB.Create(&database.Webhook{ @@ -229,7 +229,7 @@ func TestOwnershipIsReleasedAfterDelivery(t *testing.T) { iCreateTarget(t, s.MainDB, targetID, s.WebhookID, "released", - database.TargetTypeLog, "", 0, + database.TargetTypeLog, "", 1, ) event := iSeedEvent( diff --git a/internal/delivery/metrics_test.go b/internal/delivery/metrics_test.go index e7dccba..6bd124c 100644 --- a/internal/delivery/metrics_test.go +++ b/internal/delivery/metrics_test.go @@ -291,20 +291,20 @@ func TestDeliveryMetrics_CircuitBreakerGauge(t *testing.T) { body := event.Body cfg := iHTTPConfig(ts.URL) - // A retry budget above the failure threshold, so the breaker + // An attempt budget above the failure threshold, so the breaker // rather than the budget is what stops the delivery. - maxRetries := delivery.ExportDefaultFailureThreshold + 5 + maxAttempts := 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, + database.TargetTypeHTTP, cfg, maxAttempts, ) first := iTask( d, event, s.WebhookID, targetID, - "metrics-trip", cfg, maxRetries, 1, &body, + "metrics-trip", cfg, maxAttempts, 1, &body, ) s.Engine.ExportProcessNewTask(context.TODO(), &first) @@ -316,7 +316,7 @@ func TestDeliveryMetrics_CircuitBreakerGauge(t *testing.T) { ExportDefaultFailureThreshold; attempt++ { task := iTask( d, event, s.WebhookID, targetID, - "metrics-trip", cfg, maxRetries, attempt, &body, + "metrics-trip", cfg, maxAttempts, attempt, &body, ) s.Engine.ExportProcessRetryTask( @@ -364,16 +364,16 @@ func TestDeliveryMetrics_BreakerBlockedIsNotAnAttempt( body := event.Body cfg := iHTTPConfig(ts.URL) - maxRetries := delivery.ExportDefaultFailureThreshold + 5 + maxAttempts := delivery.ExportDefaultFailureThreshold + 5 iCreateTarget( t, s.MainDB, targetID, s.WebhookID, "metrics-blocked", - database.TargetTypeHTTP, cfg, maxRetries, + database.TargetTypeHTTP, cfg, maxAttempts, ) first := iTask( d, event, s.WebhookID, targetID, - "metrics-blocked", cfg, maxRetries, 1, &body, + "metrics-blocked", cfg, maxAttempts, 1, &body, ) s.Engine.ExportProcessNewTask(context.TODO(), &first) @@ -382,7 +382,7 @@ func TestDeliveryMetrics_BreakerBlockedIsNotAnAttempt( ExportDefaultFailureThreshold; attempt++ { task := iTask( d, event, s.WebhookID, targetID, - "metrics-blocked", cfg, maxRetries, attempt, &body, + "metrics-blocked", cfg, maxAttempts, attempt, &body, ) s.Engine.ExportProcessRetryTask(context.TODO(), &task) @@ -405,7 +405,7 @@ func TestDeliveryMetrics_BreakerBlockedIsNotAnAttempt( blocked := iTask( d, event, s.WebhookID, targetID, - "metrics-blocked", cfg, maxRetries, + "metrics-blocked", cfg, maxAttempts, delivery.ExportDefaultFailureThreshold+1, &body, ) diff --git a/internal/delivery/recovery_durability_test.go b/internal/delivery/recovery_durability_test.go index 2dc23c8..1eabec4 100644 --- a/internal/delivery/recovery_durability_test.go +++ b/internal/delivery/recovery_durability_test.go @@ -66,7 +66,7 @@ func TestRecoverySkipsPendingWithSuccessfulResult( iCreateTarget(t, s.MainDB, targetID, s.WebhookID, "already-delivered", - database.TargetTypeLog, "", 0, + database.TargetTypeLog, "", 1, ) event := iSeedEvent( @@ -129,7 +129,7 @@ func TestRecoveryContinuesTheAttemptNumbering(t *testing.T) { iCreateTarget(t, s.MainDB, targetID, s.WebhookID, "numbering", - database.TargetTypeLog, "", 0, + database.TargetTypeLog, "", 1, ) event := iSeedEvent( @@ -354,16 +354,17 @@ func TestFailedResultWriteLeavesDeliveryRecoverable( Status: database.DeliveryStatusPending, Event: event, Target: database.Target{ - Name: "unwritable", - Type: database.TargetTypeHTTP, - Config: iHTTPConfig(ts.URL), + Name: "unwritable", + Type: database.TargetTypeHTTP, + Config: iHTTPConfig(ts.URL), + MaxAttempts: 1, }, } full.ID = d.ID s.Engine.ExportDeliverHTTP( context.Background(), s.WebhookDB, full, - &delivery.Task{DeliveryID: d.ID, AttemptNum: 1}, + &delivery.Task{DeliveryID: d.ID, MaxAttempts: 1, AttemptNum: 1}, ) assert.Equal( @@ -444,10 +445,10 @@ func TestFailedResultWriteWithRetriesLeavesDeliveryRecoverable( Status: database.DeliveryStatusPending, Event: event, Target: database.Target{ - Name: "unwritable", - Type: database.TargetTypeHTTP, - Config: iHTTPConfig(ts.URL), - MaxRetries: 3, + Name: "unwritable", + Type: database.TargetTypeHTTP, + Config: iHTTPConfig(ts.URL), + MaxAttempts: 3, }, } full.ID = d.ID @@ -457,9 +458,8 @@ func TestFailedResultWriteWithRetriesLeavesDeliveryRecoverable( s.Engine.ExportDeliverHTTPWithScheduler( context.Background(), s.WebhookDB, full, &delivery.Task{ - DeliveryID: d.ID, - TargetID: targetID, - AttemptNum: 1, + DeliveryID: d.ID, TargetID: targetID, + MaxAttempts: 3, AttemptNum: 1, }, sched, ) diff --git a/internal/delivery/redirect_test.go b/internal/delivery/redirect_test.go index 507327f..76027e7 100644 --- a/internal/delivery/redirect_test.go +++ b/internal/delivery/redirect_test.go @@ -96,7 +96,7 @@ func deliverWithProbeHeaders( body := event.Body task := iTask( d, event, s.WebhookID, targetID, - "redirect-target", string(cfg), 0, 1, &body, + "redirect-target", string(cfg), 1, 1, &body, ) s.Engine.ExportProcessNewTask(context.TODO(), &task) diff --git a/internal/delivery/target_config_view.go b/internal/delivery/target_config_view.go index 2f02285..e8b4771 100644 --- a/internal/delivery/target_config_view.go +++ b/internal/delivery/target_config_view.go @@ -120,8 +120,8 @@ func unavailableConfigFields() []ConfigField { } // slackConfigFields describes a Slack target: its masked -// webhook URL and its retry count. Only the masked URL is -// shown; the full URL is the credential. +// webhook URL and its number of delivery attempts. Only the +// masked URL is shown; the full URL is the credential. func slackConfigFields(t *database.Target) []ConfigField { cfg, err := parseSlackConfig(t.Config) if err != nil { @@ -131,7 +131,7 @@ func slackConfigFields(t *database.Target) []ConfigField { return []ConfigField{{ Label: "Webhook URL", Value: cfg.MaskedWebhookURL(), - }, maxRetriesField(t)} + }, maxAttemptsField(t)} } // httpConfigFields describes an HTTP target: its destination @@ -178,23 +178,18 @@ func httpConfigFields(t *database.Target) []ConfigField { }) } - fields = append(fields, maxRetriesField(t)) + fields = append(fields, maxAttemptsField(t)) return fields } -// maxRetriesField describes a target's retry count, which lives -// on the target row rather than in its configuration blob. A -// stored 0 makes a single attempt, so it is shown as 1. -func maxRetriesField(t *database.Target) ConfigField { - attempts := strconv.Itoa(t.MaxRetries) - if t.MaxRetries == 0 { - attempts = "1 (fire-and-forget: no retries, no circuit breaker)" - } - +// maxAttemptsField describes a target's number of delivery +// attempts, which lives on the target row rather than in its +// configuration blob. +func maxAttemptsField(t *database.Target) ConfigField { return ConfigField{ Label: "Delivery attempts", - Value: attempts, + Value: strconv.Itoa(t.MaxAttempts), } } diff --git a/internal/delivery/target_config_view_test.go b/internal/delivery/target_config_view_test.go index 85ffde0..8a60b24 100644 --- a/internal/delivery/target_config_view_test.go +++ b/internal/delivery/target_config_view_test.go @@ -32,7 +32,7 @@ const ( viewMaskedOrigin = viewExampleOrigin + "/..." viewUnavailable = "(unavailable)" viewExpiryNever = "never" - viewMaxRetries = "Delivery attempts" + viewMaxAttempts = "Delivery attempts" ) func TestMaskedWebhookURL(t *testing.T) { @@ -171,6 +171,7 @@ func slackTarget() database.Target { Active: true, Config: `{"webhookUrl":"` + slackWebhookURL + `"}`, + MaxAttempts: 1, } } @@ -189,28 +190,29 @@ func TestNewTargetViews_Slack(t *testing.T) { assert.Equal( t, map[string]string{ - "Webhook URL": slackMaskedURL, - viewMaxRetries: "1 (fire-and-forget: no retries, no circuit breaker)", + "Webhook URL": slackMaskedURL, + viewMaxAttempts: "1", }, fieldMap(view.Config), ) } -// TestNewTargetViews_SlackRetries proves a Slack target shows -// its retry count the same way an HTTP target does. -func TestNewTargetViews_SlackRetries(t *testing.T) { +// TestNewTargetViews_SlackAttempts proves a Slack target shows +// its number of delivery attempts the same way an HTTP target +// does. +func TestNewTargetViews_SlackAttempts(t *testing.T) { t.Parallel() target := slackTarget() - target.MaxRetries = 2 + target.MaxAttempts = 2 view := viewFor(t, target) assert.Equal( t, map[string]string{ - "Webhook URL": slackMaskedURL, - viewMaxRetries: "2", + "Webhook URL": slackMaskedURL, + viewMaxAttempts: "2", }, fieldMap(view.Config), ) @@ -225,7 +227,7 @@ func TestNewTargetViews_HTTP(t *testing.T) { `"timeout":30,` + `"headers":{"Authorization":"Bearer sekrit"},` + `"forwardQuery":true}`, - MaxRetries: 5, + MaxAttempts: 5, }) fields := fieldMap(view.Config) @@ -237,7 +239,7 @@ func TestNewTargetViews_HTTP(t *testing.T) { "Timeout": "30s", "Headers": "1 configured", "Query string": "passed on to this target", - viewMaxRetries: "5", + viewMaxAttempts: "5", }, fields, ) @@ -248,19 +250,22 @@ func TestNewTargetViews_HTTP(t *testing.T) { } } -func TestNewTargetViews_HTTPFireAndForget(t *testing.T) { +// TestNewTargetViews_HTTPSingleAttempt proves a single attempt is +// shown as the plain number, like any other. +func TestNewTargetViews_HTTPSingleAttempt(t *testing.T) { t.Parallel() view := viewFor(t, database.Target{ - Type: database.TargetTypeHTTP, - Config: `{"url":"` + viewExampleHook + `"}`, + Type: database.TargetTypeHTTP, + Config: `{"url":"` + viewExampleHook + `"}`, + MaxAttempts: 1, }) assert.Equal( t, map[string]string{ "Destination URL": viewMaskedOrigin, - viewMaxRetries: "1 (fire-and-forget: no retries, no circuit breaker)", + viewMaxAttempts: "1", }, fieldMap(view.Config), ) @@ -274,8 +279,9 @@ func TestNewTargetViews_HTTPMasksDestinationURL(t *testing.T) { t.Parallel() view := viewFor(t, database.Target{ - Type: database.TargetTypeHTTP, - Config: `{"url":"` + slackWebhookURL + `"}`, + Type: database.TargetTypeHTTP, + Config: `{"url":"` + slackWebhookURL + `"}`, + MaxAttempts: 1, }) fields := fieldMap(view.Config) @@ -398,19 +404,23 @@ func TestNewTargetViews_Unpresentable(t *testing.T) { Config: blob, }, "unparseable json": { - Type: database.TargetTypeSlack, - Config: blob, + Type: database.TargetTypeSlack, + Config: blob, + MaxAttempts: 1, }, "empty slack config": { - Type: database.TargetTypeSlack, + Type: database.TargetTypeSlack, + MaxAttempts: 1, }, "slack config without url": { - Type: database.TargetTypeSlack, - Config: `{}`, + Type: database.TargetTypeSlack, + Config: `{}`, + MaxAttempts: 1, }, "unparseable http json": { - Type: database.TargetTypeHTTP, - Config: `{"url":`, + Type: database.TargetTypeHTTP, + Config: `{"url":`, + MaxAttempts: 1, }, "unparseable archive json": { Type: database.TargetTypeDatabase, diff --git a/internal/delivery/target_headers_test.go b/internal/delivery/target_headers_test.go index 4250dee..ef765bc 100644 --- a/internal/delivery/target_headers_test.go +++ b/internal/delivery/target_headers_test.go @@ -189,6 +189,7 @@ func TestNewTargetConfigForm(t *testing.T) { Config: `{"url":"https://example.com/h",` + `"headers":{"Authorization":"Bearer abc"},` + `"timeout":9}`, + MaxAttempts: 1, }) require.NoError(t, err) assert.Equal(t, "https://example.com/h", form.URL) @@ -196,8 +197,9 @@ func TestNewTargetConfigForm(t *testing.T) { assert.Equal(t, "9", form.Timeout) form, err = delivery.NewTargetConfigForm(&database.Target{ - Type: database.TargetTypeSlack, - Config: `{"webhookUrl":"https://hooks.example/s"}`, + Type: database.TargetTypeSlack, + Config: `{"webhookUrl":"https://hooks.example/s"}`, + MaxAttempts: 1, }) require.NoError(t, err) assert.Equal(t, "https://hooks.example/s", form.URL) @@ -242,9 +244,9 @@ func TestNewTargetConfigForm_UnreadableConfigErrors(t *testing.T) { t.Parallel() cases := []*database.Target{ - {Type: database.TargetTypeHTTP, Config: "not json"}, - {Type: database.TargetTypeHTTP, Config: `{}`}, - {Type: database.TargetTypeSlack, Config: ""}, + {Type: database.TargetTypeHTTP, Config: "not json", MaxAttempts: 1}, + {Type: database.TargetTypeHTTP, Config: `{}`, MaxAttempts: 1}, + {Type: database.TargetTypeSlack, Config: "", MaxAttempts: 1}, { Type: database.TargetTypeDatabase, Config: `{"expiry":"soon"}`, diff --git a/internal/delivery/target_http.go b/internal/delivery/target_http.go index 009c070..ce18d22 100644 --- a/internal/delivery/target_http.go +++ b/internal/delivery/target_http.go @@ -53,10 +53,10 @@ type httpCore struct { } // deliver runs one delivery attempt through the retry core. -// A maxRetries of 0 is fire-and-forget: a single attempt is -// recorded and no circuit breaker is consulted. A positive -// maxRetries gates the attempt on the circuit breaker and -// schedules a backed-off retry on failure. +// A maxAttempts of 1 is a single attempt, recorded without +// consulting a circuit breaker. A higher maxAttempts gates the +// attempt on the circuit breaker and schedules a backed-off retry +// on failure, until maxAttempts attempts have been made. // // ctx is the context attempt sends its request with. Stopping the // engine cancels it; see interruptedByStop. @@ -66,17 +66,17 @@ func (c *httpCore) deliver( d *database.Delivery, task *Task, sched Scheduler, - maxRetries int, + maxAttempts int, attempt func() attemptResult, ) { - if maxRetries == 0 { + if maxAttempts == 1 { c.fireAndForget(ctx, webhookDB, d, attempt()) return } c.withRetry( - ctx, webhookDB, d, task, sched, maxRetries, attempt, + ctx, webhookDB, d, task, sched, maxAttempts, attempt, ) } @@ -153,7 +153,7 @@ func (c *httpCore) withRetry( d *database.Delivery, task *Task, sched Scheduler, - maxRetries int, + maxAttempts int, attempt func() attemptResult, ) { cb := c.getCircuitBreaker(task.TargetID) @@ -205,7 +205,7 @@ func (c *httpCore) withRetry( cb.RecordFailure() c.handleRetry( - webhookDB, d, task, sched, maxRetries, attemptNum, + webhookDB, d, task, sched, maxAttempts, attemptNum, ) } @@ -266,10 +266,10 @@ func (c *httpCore) handleRetry( d *database.Delivery, task *Task, sched Scheduler, - maxRetries int, + maxAttempts int, attemptNum int, ) { - if attemptNum >= maxRetries { + if attemptNum >= maxAttempts { c.eng.settleStatus( webhookDB, d, d.Target.Type, database.DeliveryStatusFailed, @@ -439,7 +439,7 @@ func (t *httpTarget) Deliver( t.deliver( ctx, webhookDB, d, task, sched, - d.Target.MaxRetries, attempt, + d.Target.MaxAttempts, attempt, ) } diff --git a/internal/delivery/target_http_query_test.go b/internal/delivery/target_http_query_test.go index fb3a5de..b06a398 100644 --- a/internal/delivery/target_http_query_test.go +++ b/internal/delivery/target_http_query_test.go @@ -47,7 +47,7 @@ func deliverWithQuery(t *testing.T, cfg string) database.DeliveryResult { database.DeliveryStatusPending, ) task := iTask( - d, event, s.WebhookID, d.TargetID, "query", cfg, 0, 1, &event.Body, + d, event, s.WebhookID, d.TargetID, "query", cfg, 1, 1, &event.Body, ) task.RawQuery = eventQuery @@ -144,8 +144,9 @@ func TestDeliverHTTP_ForwardedQueryKeepsTheTargetURLMasked(t *testing.T) { t.Cleanup(ts.Close) target := &database.Target{ - Type: database.TargetTypeHTTP, - Config: httpTargetConfig(t, ts.URL+"/in?token="+secret, true), + Type: database.TargetTypeHTTP, + Config: httpTargetConfig(t, ts.URL+"/in?token="+secret, true), + MaxAttempts: 1, } deliverWithQuery(t, target.Config) diff --git a/internal/delivery/target_redact_test.go b/internal/delivery/target_redact_test.go index 52613de..a4c010f 100644 --- a/internal/delivery/target_redact_test.go +++ b/internal/delivery/target_redact_test.go @@ -22,8 +22,9 @@ func TestRedactor_RemovesSlackWebhookURL(t *testing.T) { t.Parallel() r := delivery.NewRedactor(&database.Target{ - Type: database.TargetTypeSlack, - Config: `{"webhookUrl":"` + redactWebhookURL + `"}`, + Type: database.TargetTypeSlack, + Config: `{"webhookUrl":"` + redactWebhookURL + `"}`, + MaxAttempts: 1, }) got := r.Redact("no_service for " + redactWebhookURL) @@ -52,8 +53,9 @@ func TestRedactor_RemovesSecretSeveredByACut(t *testing.T) { t.Parallel() r := delivery.NewRedactor(&database.Target{ - Type: database.TargetTypeSlack, - Config: `{"webhookUrl":"` + redactWebhookURL + `"}`, + Type: database.TargetTypeSlack, + Config: `{"webhookUrl":"` + redactWebhookURL + `"}`, + MaxAttempts: 1, }) // Every cut position inside the credential, not just a @@ -96,6 +98,7 @@ func TestRedactor_RedactsCredentialShapedHeaderValues( `"X-Credential":"HHHHHHHHHHHH",` + `"Accept":"application/json",` + `"User-Agent":"webhooker/1.0"}}`, + MaxAttempts: 1, }) for _, secret := range []string{ @@ -136,6 +139,7 @@ func TestRedactor_IgnoresVeryShortHeaderValues(t *testing.T) { Type: database.TargetTypeHTTP, Config: `{"url":"https://example.com/in",` + `"headers":{"X-Api-Key":"ab"}}`, + MaxAttempts: 1, }) const response = "rabbit" @@ -150,8 +154,9 @@ func TestRedactor_RemovesBarePath(t *testing.T) { t.Parallel() r := delivery.NewRedactor(&database.Target{ - Type: database.TargetTypeSlack, - Config: `{"webhookUrl":"` + redactWebhookURL + `"}`, + Type: database.TargetTypeSlack, + Config: `{"webhookUrl":"` + redactWebhookURL + `"}`, + MaxAttempts: 1, }) got := r.Redact("POST " + redactSecretPath + " 404") @@ -183,8 +188,9 @@ func TestRedactor_RemovesHTTPURLQueryAndUserinfo(t *testing.T) { raw := dest.String() r := delivery.NewRedactor(&database.Target{ - Type: database.TargetTypeHTTP, - Config: `{"url":"` + raw + `"}`, + Type: database.TargetTypeHTTP, + Config: `{"url":"` + raw + `"}`, + MaxAttempts: 1, }) for _, echoed := range []string{ @@ -232,8 +238,9 @@ func TestRedactor_RemovesEchoedQueryOfURLWithoutPath(t *testing.T) { "https://example.com?token=" + secret, } { r := delivery.NewRedactor(&database.Target{ - Type: database.TargetTypeHTTP, - Config: `{"url":"` + dest + `"}`, + Type: database.TargetTypeHTTP, + Config: `{"url":"` + dest + `"}`, + MaxAttempts: 1, }) for echoed, want := range echoes { @@ -253,8 +260,9 @@ func TestRedactor_LeavesUnrelatedTextAlone(t *testing.T) { const response = "ok=false error=channel_not_found" r := delivery.NewRedactor(&database.Target{ - Type: database.TargetTypeSlack, - Config: `{"webhookUrl":"` + redactWebhookURL + `"}`, + Type: database.TargetTypeSlack, + Config: `{"webhookUrl":"` + redactWebhookURL + `"}`, + MaxAttempts: 1, }) assert.Equal(t, response, r.Redact(response)) @@ -279,8 +287,9 @@ func TestRedactor_EmptyUserinfoDoesNotShredTheBody(t *testing.T) { require.Empty(t, parsed.User.String()) r := delivery.NewRedactor(&database.Target{ - Type: database.TargetTypeHTTP, - Config: `{"url":"` + dest + `"}`, + Type: database.TargetTypeHTTP, + Config: `{"url":"` + dest + `"}`, + MaxAttempts: 1, }) const body = "ok=false error=channel_not_found" @@ -314,8 +323,8 @@ func TestRedactor_ZeroValueAndConfiglessTargets(t *testing.T) { for _, tgt := range []database.Target{ {Type: database.TargetTypeLog}, {Type: database.TargetTypeDatabase}, - {Type: database.TargetTypeSlack, Config: "not json"}, - {Type: database.TargetTypeHTTP, Config: ""}, + {Type: database.TargetTypeSlack, Config: "not json", MaxAttempts: 1}, + {Type: database.TargetTypeHTTP, Config: "", MaxAttempts: 1}, } { assert.Equal( t, text, diff --git a/internal/delivery/target_slack.go b/internal/delivery/target_slack.go index 553b98e..2b9129d 100644 --- a/internal/delivery/target_slack.go +++ b/internal/delivery/target_slack.go @@ -30,9 +30,8 @@ type SlackTargetConfig struct { // slackTarget delivers events to Slack incoming webhooks. It // formats the event into a Slack message and posts it as // JSON. It shares the retry core with the HTTP target: a -// MaxRetries of 0 stays single-attempt fire-and-forget -// (preserving existing Slack targets), while a positive -// MaxRetries adds backoff and circuit breaking. +// MaxAttempts of 1 is a single attempt, while a higher one +// adds backoff and circuit breaking. type slackTarget struct { *httpCore @@ -83,7 +82,7 @@ func (t *slackTarget) Deliver( t.deliver( ctx, webhookDB, d, task, sched, - d.Target.MaxRetries, attempt, + d.Target.MaxAttempts, attempt, ) } diff --git a/internal/delivery/terminal_state_test.go b/internal/delivery/terminal_state_test.go index 20ac2f3..ef2b0cf 100644 --- a/internal/delivery/terminal_state_test.go +++ b/internal/delivery/terminal_state_test.go @@ -743,7 +743,7 @@ func TestSendRecoveredDeliveries_TargetMissingFromMap( iCreateTarget( t, s.MainDB, targetID, s.WebhookID, "found-on-lookup", - database.TargetTypeLog, "", 0, + database.TargetTypeLog, "", 1, ) event := iSeedEvent( @@ -790,7 +790,7 @@ func TestRecoverPending_TargetUnreadable_LeavesDeliveryAlone( iCreateTarget( t, s.MainDB, targetID, s.WebhookID, "healthy", - database.TargetTypeLog, "", 0, + database.TargetTypeLog, "", 1, ) event := iSeedEvent( diff --git a/internal/handlers/delivery_replay.go b/internal/handlers/delivery_replay.go index 96a8cfc..091ba4f 100644 --- a/internal/handlers/delivery_replay.go +++ b/internal/handlers/delivery_replay.go @@ -308,7 +308,7 @@ func createReplayDelivery( TargetName: target.Name, TargetType: target.Type, TargetConfig: target.Config, - MaxRetries: target.MaxRetries, + MaxAttempts: target.MaxAttempts, Method: event.Method, RawQuery: event.RawQuery, Headers: event.Headers, diff --git a/internal/handlers/event_log.go b/internal/handlers/event_log.go index 63a5d7c..52f17bf 100644 --- a/internal/handlers/event_log.go +++ b/internal/handlers/event_log.go @@ -496,7 +496,7 @@ const deliveryIDChunkSize = 500 // // Each response body is cut by SQLite rather than in Go, for // the reason deliveryResultColumns gives. How many attempts a -// delivery has is the target's MaxRetries, which the +// delivery has is bounded by the target's MaxAttempts, which the // authenticated operator sets; how many of them reach the page // is bounded again by maxRenderedAttempts. func (h *Handlers) loadDeliveryResults( diff --git a/internal/handlers/export_test.go b/internal/handlers/export_test.go index 852f79a..6357e46 100644 --- a/internal/handlers/export_test.go +++ b/internal/handlers/export_test.go @@ -37,10 +37,10 @@ const MaxRenderedResponseBytesForTest = maxRenderedResponseBytes // per-delivery attempt ceiling to the handlers_test package. const MaxRenderedAttemptsForTest = maxRenderedAttempts -// MaxTargetRetriesForTest exposes the target max_retries ceiling to +// MaxTargetAttemptsForTest exposes the target max_attempts ceiling to // the handlers_test package, so the tests assert against the constant // the handlers enforce rather than a number copied beside it. -const MaxTargetRetriesForTest = maxTargetRetries +const MaxTargetAttemptsForTest = maxTargetAttempts // EventDBLeftMsgForTest and SidecarLeftMsgForTest expose the two // messages the webhook delete handler logs when a file of the event diff --git a/internal/handlers/source_delete_test.go b/internal/handlers/source_delete_test.go index 211e891..4ef4143 100644 --- a/internal/handlers/source_delete_test.go +++ b/internal/handlers/source_delete_test.go @@ -64,10 +64,11 @@ func seedTarget( t.Helper() tgt := &database.Target{ - WebhookID: webhookID, - Name: "t-" + string(targetType), - Type: targetType, - Active: true, + WebhookID: webhookID, + Name: "t-" + string(targetType), + Type: targetType, + Active: true, + MaxAttempts: 1, } require.NoError( diff --git a/internal/handlers/source_detail_test.go b/internal/handlers/source_detail_test.go index 84ddefb..6ccfc13 100644 --- a/internal/handlers/source_detail_test.go +++ b/internal/handlers/source_detail_test.go @@ -37,11 +37,12 @@ func seedConfiguredTarget( t.Helper() tgt := &database.Target{ - WebhookID: webhookID, - Name: "t-" + string(targetType), - Type: targetType, - Active: true, - Config: config, + WebhookID: webhookID, + Name: "t-" + string(targetType), + Type: targetType, + Active: true, + Config: config, + MaxAttempts: 1, } require.NoError( diff --git a/internal/handlers/target_attempts.go b/internal/handlers/target_attempts.go new file mode 100644 index 0000000..c98712e --- /dev/null +++ b/internal/handlers/target_attempts.go @@ -0,0 +1,81 @@ +package handlers + +import ( + "errors" + "strconv" + "strings" +) + +// maxTargetAttempts bounds a target's max_attempts. +// +// Both target forms already declare max="20" on the input, so this +// enforces server-side what the UI has always advertised rather than +// introducing a new limit. +// +// The number is not cosmetic. Every attempt writes a delivery_results +// row that the event log then loads and renders, and the engine backs +// off by 2^(n-1) seconds, so attempt 20 is already about six days +// after the first. A value beyond this buys no additional durability +// and only costs rows. +const maxTargetAttempts = 20 + +// Errors returned when a max_attempts form value cannot be turned into +// a number of attempts. +var ( + // errAttemptsInvalid signals a max_attempts form value that is not + // a whole number of at least 1. + errAttemptsInvalid = errors.New( + "must be a whole number, at least 1", + ) + + // errAttemptsTooLarge signals a max_attempts form value that is a + // whole number but above maxTargetAttempts. It is distinguished + // from errAttemptsInvalid so the message can name the ceiling + // instead of implying the input was not a number. + errAttemptsTooLarge = errors.New("out of range") +) + +// parseMaxAttempts interprets a max_attempts form value. +// +// An ABSENT value — the field empty or not submitted — yields +// fallback, which lets the create path apply its default and the edit +// path leave the stored value alone. A value that is SET BUT INVALID +// is an error: unparseable, below 1, or above maxTargetAttempts. +// +// The distinction is the whole point of this function. Returning a +// default for input the operator typed but that did not parse would +// silently turn retries off on a store-and-forward proxy — and on the +// edit path destroy a working configuration over a typo. A default +// answers a question that was not asked; it never answers one that +// was asked badly. +func parseMaxAttempts(raw string, fallback int) (int, error) { + raw = strings.TrimSpace(raw) + if raw == "" { + return fallback, nil + } + + v, err := strconv.Atoi(raw) + if err != nil || v < 1 { + return 0, errAttemptsInvalid + } + + if v > maxTargetAttempts { + return 0, errAttemptsTooLarge + } + + return v, nil +} + +// attemptsErrorMessage returns the message the create and edit forms +// show for a rejected max_attempts value. Any error other than +// errAttemptsTooLarge falls back to the generic wording, so an +// unrecognised parse failure still produces a sensible 400. +func attemptsErrorMessage(err error) string { + if errors.Is(err, errAttemptsTooLarge) { + return errAttemptsTooLarge.Error() + + ": at most " + strconv.Itoa(maxTargetAttempts) + + " attempts" + } + + return errAttemptsInvalid.Error() +} diff --git a/internal/handlers/target_attempts_test.go b/internal/handlers/target_attempts_test.go new file mode 100644 index 0000000..8dfa859 --- /dev/null +++ b/internal/handlers/target_attempts_test.go @@ -0,0 +1,429 @@ +package handlers_test + +import ( + "net/http" + "net/url" + "strconv" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/webhooker/internal/database" + "sneak.berlin/go/webhooker/internal/handlers" +) + +// attemptsTargetURL is the destination the attempts-validation targets +// point at. It is a literal public address rather than a hostname so +// the SSRF check resolves nothing and a sandbox without DNS cannot +// make these cases pass or fail for the wrong reason. +const attemptsTargetURL = "https://93.184.216.34/hooks/attempts" + +const ( + // wayAboveCeiling is the typo'd-extra-zero case from the report. + wayAboveCeiling = "999999999" + + // notANumber is the plainest garbage an operator can type, and + // the value the report submitted on the edit form. + notANumber = "abc" + + // workingAttempts is the number of attempts a seeded target is + // already delivering with, which a rejected submission must not + // disturb. + workingAttempts = 2 +) + +// aboveCeiling is the smallest rejected whole number. +func aboveCeiling() string { + return strconv.Itoa(handlers.MaxTargetAttemptsForTest + 1) +} + +// overCeilingAttempts is whole-number input past the limit, which is +// rejected with the limit named. +func overCeilingAttempts() []string { + return []string{aboveCeiling(), wayAboveCeiling} +} + +// belowOneAttempts is whole-number input under the floor of one +// attempt, which is rejected with the floor named. +func belowOneAttempts() []string { + return []string{"0", "-5"} +} + +// unparseableAttempts is input an operator can type into the field +// that is not a number of attempts. Each must be REJECTED: silently +// reading any of them as a default turns retries off on a +// store-and-forward proxy without saying so. +// +// The twenty-digit case is here because it parses as digits but +// overflows int, which is the one failure the field's own min/max +// attributes cannot describe. +func unparseableAttempts() []string { + return []string{ + notANumber, + "2.7", + "12345678901234567890", + "1e3", + } +} + +// createAttemptsForm is a complete, otherwise-valid HTTP target +// creation, so the only thing any case below varies is max_attempts. +func createAttemptsForm(attempts string) url.Values { + form := url.Values{} + form.Set("name", "attempts-target") + form.Set("type", string(database.TargetTypeHTTP)) + form.Set("url", attemptsTargetURL) + + if attempts != absentField { + form.Set("max_attempts", attempts) + } + + return form +} + +// absentField marks a field the form does not submit at all, which is +// the case that legitimately takes a default and must stay distinct +// from a field submitted with garbage in it. +const absentField = "\x00absent" + +// absentAttempts is every way of saying "the operator did not set +// this", each of which takes the default rather than a 400. Blank and +// whitespace-only count as absent here because they do in the timeout +// and retention controls on the same forms; a rule the fields do not +// share would be its own surprise. +func absentAttempts() []string { + return []string{absentField, "", " "} +} + +// createWithAttempts posts the target create form for a fresh webhook +// and returns the webhook and the response. +func createWithAttempts( + t *testing.T, + env *sourceTestEnv, + attempts string, +) (database.Webhook, int, string) { + t.Helper() + + webhook := seedWebhookWithRetention(t, env.db, 30) + + w := serveTarget( + env, http.MethodPost, + "/hook/"+webhook.ID+"/targets", + createAttemptsForm(attempts), + ) + + return webhook, w.Code, w.Body.String() +} + +// TestTargetCreate_AttemptsAboveCeilingRejected proves the create form +// enforces a ceiling at all, and that the 400 names it — a rejection +// that does not say what the limit is leaves the operator guessing. +func TestTargetCreate_AttemptsAboveCeilingRejected(t *testing.T) { + t.Parallel() + + env := setupSourceTest(t) + ceiling := strconv.Itoa(handlers.MaxTargetAttemptsForTest) + + for _, attempts := range overCeilingAttempts() { + webhook, code, body := createWithAttempts(t, env, attempts) + + assert.Equal(t, http.StatusBadRequest, code, + "max_attempts=%s should be rejected", attempts) + assert.Contains(t, body, ceiling, + "the rejection for %s should name the ceiling", + attempts) + assert.Empty(t, + targetsForWebhook(t, env.db, webhook.ID), + "no target should be created for %s", attempts) + } +} + +// TestTargetCreate_AttemptsBelowOneRejected proves the create form +// refuses 0 and less, naming the floor: every delivery is attempted +// at least once. +func TestTargetCreate_AttemptsBelowOneRejected(t *testing.T) { + t.Parallel() + + env := setupSourceTest(t) + + for _, attempts := range belowOneAttempts() { + webhook, code, body := createWithAttempts(t, env, attempts) + + assert.Equal(t, http.StatusBadRequest, code, + "max_attempts=%s should be rejected", attempts) + assert.Contains(t, body, "at least 1", + "the rejection for %s should name the floor", + attempts) + assert.Empty(t, + targetsForWebhook(t, env.db, webhook.ID), + "no target should be created for %s", attempts) + } +} + +// TestTargetCreate_UnparseableAttemptsRejected is the core of the +// defect: each of these was accepted with HTTP 200 and stored as a +// default. +func TestTargetCreate_UnparseableAttemptsRejected(t *testing.T) { + t.Parallel() + + env := setupSourceTest(t) + + for _, attempts := range unparseableAttempts() { + webhook, code, body := createWithAttempts(t, env, attempts) + + assert.Equal(t, http.StatusBadRequest, code, + "max_attempts=%q should be rejected, not coerced", + attempts) + assert.Contains(t, body, "whole number", + "the rejection for %q should say why", attempts) + assert.Empty(t, + targetsForWebhook(t, env.db, webhook.ID), + "no target should be created for %q", attempts) + } +} + +// TestTargetCreate_ValidAttemptsStored covers the accepting half, +// including both bounds: a bound that rejects its own limit would +// make the advertised range unreachable. +func TestTargetCreate_ValidAttemptsStored(t *testing.T) { + t.Parallel() + + env := setupSourceTest(t) + + for _, want := range []int{1, 3, handlers.MaxTargetAttemptsForTest} { + webhook, code, body := createWithAttempts( + t, env, strconv.Itoa(want), + ) + + require.Equal(t, http.StatusSeeOther, code, body) + + targets := targetsForWebhook(t, env.db, webhook.ID) + require.Len(t, targets, 1) + assert.Equal(t, want, targets[0].MaxAttempts) + } +} + +// TestTargetCreate_AbsentAttemptsTakesDefault keeps the two cases +// distinct. An omitted field is not an operator asking for something +// invalid, so it still gets the default of one attempt rather than a +// 400 — otherwise the fix above would make the form unusable. +func TestTargetCreate_AbsentAttemptsTakesDefault(t *testing.T) { + t.Parallel() + + env := setupSourceTest(t) + + for _, attempts := range absentAttempts() { + webhook, code, body := createWithAttempts(t, env, attempts) + + require.Equal(t, http.StatusSeeOther, code, body) + + targets := targetsForWebhook(t, env.db, webhook.ID) + require.Len(t, targets, 1) + assert.Equal(t, 1, targets[0].MaxAttempts, + "an absent max_attempts should take the default") + } +} + +// seedAttemptsTarget creates an HTTP target already delivering with +// workingAttempts attempts, through the real create handler. +func seedAttemptsTarget( + t *testing.T, + env *sourceTestEnv, +) (database.Webhook, database.Target) { + t.Helper() + + webhook, code, body := createWithAttempts( + t, env, strconv.Itoa(workingAttempts), + ) + require.Equal(t, http.StatusSeeOther, code, body) + + targets := targetsForWebhook(t, env.db, webhook.ID) + require.Len(t, targets, 1) + require.Equal(t, workingAttempts, targets[0].MaxAttempts) + + return webhook, targets[0] +} + +// editAttemptsForm is a complete edit submission that changes the +// target's name as well, so a rejected submission can be shown to +// have written nothing at all rather than merely to have left +// max_attempts alone. +func editAttemptsForm(attempts string) url.Values { + form := url.Values{} + form.Set("name", "renamed-by-edit") + form.Set("url", attemptsTargetURL) + + if attempts != absentField { + form.Set("max_attempts", attempts) + } + + return form +} + +// assertEditRejectedAndUnchanged submits an edit expected to fail and +// checks both halves of the requirement: the 400 explains itself, and +// the target it was submitted against is untouched. +func assertEditRejectedAndUnchanged( + t *testing.T, + env *sourceTestEnv, + attempts, wantReason string, +) { + t.Helper() + + webhook, target := seedAttemptsTarget(t, env) + + w := submitTargetEdit( + env, webhook.ID, target.ID, editAttemptsForm(attempts), + ) + + assert.Equal(t, http.StatusBadRequest, w.Code, + "max_attempts=%q should be rejected on edit", attempts) + assert.Contains(t, w.Body.String(), wantReason, + "the rejection for %q should say why", attempts) + + stored := storedTarget(t, env, target.ID) + assert.Equal(t, workingAttempts, stored.MaxAttempts, + "a rejected edit must not destroy the working number "+ + "of attempts with %q", attempts) + assert.Equal(t, "attempts-target", stored.Name, + "a rejected edit must write nothing at all") +} + +// TestTargetEdit_UnparseableAttemptsRejected is the damaging half of +// the defect. A target delivering with retries, re-saved with a typo +// in the field, returned 200 and was left with retries disabled and +// nothing said. +func TestTargetEdit_UnparseableAttemptsRejected(t *testing.T) { + t.Parallel() + + env := setupSourceTest(t) + + for _, attempts := range unparseableAttempts() { + assertEditRejectedAndUnchanged( + t, env, attempts, "whole number", + ) + } +} + +// TestTargetEdit_AttemptsBelowOneRejected proves the floor applies +// to the edit path too. +func TestTargetEdit_AttemptsBelowOneRejected(t *testing.T) { + t.Parallel() + + env := setupSourceTest(t) + + for _, attempts := range belowOneAttempts() { + assertEditRejectedAndUnchanged(t, env, attempts, "at least 1") + } +} + +// TestTargetEdit_AttemptsAboveCeilingRejected proves the ceiling +// applies to the edit path too, naming itself, so the two paths +// cannot disagree about what is storable. +func TestTargetEdit_AttemptsAboveCeilingRejected(t *testing.T) { + t.Parallel() + + env := setupSourceTest(t) + ceiling := strconv.Itoa(handlers.MaxTargetAttemptsForTest) + + for _, attempts := range overCeilingAttempts() { + assertEditRejectedAndUnchanged(t, env, attempts, ceiling) + } +} + +// TestTargetEdit_ValidAttemptsStored covers the accepting half of the +// edit path, so the bounds cannot be enforced by simply refusing +// every submission. +func TestTargetEdit_ValidAttemptsStored(t *testing.T) { + t.Parallel() + + env := setupSourceTest(t) + + for _, want := range []int{1, 9, handlers.MaxTargetAttemptsForTest} { + webhook, target := seedAttemptsTarget(t, env) + + w := submitTargetEdit( + env, webhook.ID, target.ID, + editAttemptsForm(strconv.Itoa(want)), + ) + require.Equal(t, + http.StatusSeeOther, w.Code, w.Body.String(), + ) + + assert.Equal(t, want, + storedTarget(t, env, target.ID).MaxAttempts) + } +} + +// TestTargetEdit_AbsentAttemptsLeavesStoredValue is the edit path's +// absent-versus-invalid case. The field is only offered by the forms +// for types that retry, so a submission without it must leave the +// stored count alone rather than be rejected or reset. +func TestTargetEdit_AbsentAttemptsLeavesStoredValue(t *testing.T) { + t.Parallel() + + env := setupSourceTest(t) + + for _, attempts := range absentAttempts() { + webhook, target := seedAttemptsTarget(t, env) + + w := submitTargetEdit( + env, webhook.ID, target.ID, + editAttemptsForm(attempts), + ) + require.Equal(t, + http.StatusSeeOther, w.Code, w.Body.String(), + ) + + assert.Equal(t, workingAttempts, + storedTarget(t, env, target.ID).MaxAttempts, + "an absent max_attempts must leave the stored "+ + "count alone (%q)", attempts) + } +} + +// TestTargetAttempts_CreateAndEditAgreeOnEveryCase proves the two +// paths cannot disagree, which is what let the create form and the +// edit form drift apart in the first place. Every input is submitted +// to both and the accept/reject verdicts are compared. +func TestTargetAttempts_CreateAndEditAgreeOnEveryCase(t *testing.T) { + t.Parallel() + + env := setupSourceTest(t) + + accepted := []string{ + "1", "2", + strconv.Itoa(handlers.MaxTargetAttemptsForTest), + } + overCeiling := overCeilingAttempts() + belowOne := belowOneAttempts() + unparseable := unparseableAttempts() + + cases := make( + []string, 0, + len(accepted)+len(overCeiling)+len(belowOne)+ + len(unparseable), + ) + cases = append(cases, accepted...) + cases = append(cases, overCeiling...) + cases = append(cases, belowOne...) + cases = append(cases, unparseable...) + + for _, attempts := range cases { + _, createCode, _ := createWithAttempts(t, env, attempts) + + webhook, target := seedAttemptsTarget(t, env) + editCode := submitTargetEdit( + env, webhook.ID, target.ID, + editAttemptsForm(attempts), + ).Code + + assert.Equal(t, + createCode == http.StatusBadRequest, + editCode == http.StatusBadRequest, + "create and edit must agree on max_attempts=%q "+ + "(create %d, edit %d)", + attempts, createCode, editCode, + ) + } +} diff --git a/internal/handlers/target_create.go b/internal/handlers/target_create.go index 224cb3f..5410f47 100644 --- a/internal/handlers/target_create.go +++ b/internal/handlers/target_create.go @@ -102,9 +102,10 @@ func (h *Handlers) newTarget( in targetFormInput, ) (*database.Target, string, error) { target := &database.Target{ - WebhookID: webhookID, - Type: in.Type, - Active: true, + WebhookID: webhookID, + Type: in.Type, + Active: true, + MaxAttempts: 1, } errMsg, err := h.setTargetFromForm(ctx, target, in) @@ -117,9 +118,10 @@ func (h *Handlers) newTarget( // setTargetFromForm validates a target form against the target's type // and, when it accepts it, sets the target's name, configuration and -// retry count from it. It returns the message the form shows for -// anything it refuses, an unknown type among them, and then leaves the -// target unchanged; an error is the server's fault, as for newTarget. +// number of delivery attempts from it. It returns the message the form +// shows for anything it refuses, an unknown type among them, and then +// leaves the target unchanged; an error is the server's fault, as for +// newTarget. // The add target form and the target edit form both go through here, // so the two cannot come to disagree about what a target may be. func (h *Handlers) setTargetFromForm( @@ -136,20 +138,20 @@ func (h *Handlers) setTargetFromForm( return errMsg, err } - // An empty max_retries keeps the target's count: the - // fire-and-forget default of 0 for a new target, and the stored - // count for an edited one, since the forms for target types that - // do not retry have no such field. A value that is filled in but - // invalid is refused rather than becoming that count, so a typo - // cannot destroy the count a target is delivering with. - maxRetries, err := parseMaxRetries(in.MaxRetries, target.MaxRetries) + // An empty max_attempts keeps the target's count: the default of + // 1 for a new target, and the stored count for an edited one, + // since the forms for target types that do not retry have no such + // field. A value that is filled in but invalid is refused rather + // than becoming that count, so a typo cannot destroy the count a + // target is delivering with. + maxAttempts, err := parseMaxAttempts(in.MaxAttempts, target.MaxAttempts) if err != nil { - return "Invalid delivery attempts: " + retriesErrorMessage(err), nil + return "Invalid delivery attempts: " + attemptsErrorMessage(err), nil } target.Name = in.Name target.Config = configJSON - target.MaxRetries = maxRetries + target.MaxAttempts = maxAttempts return "", nil } @@ -176,8 +178,8 @@ type targetFormInput struct { // ForwardQuery is an HTTP target's checkbox that passes each // event's query string on to it. ForwardQuery bool - // MaxRetries is an HTTP or Slack target's max_retries. - MaxRetries string + // MaxAttempts is an HTTP or Slack target's max_attempts. + MaxAttempts string // Expiry is a database (archive) target's row expiry. Expiry string // Rotation is a database (archive) target's rotation. @@ -204,7 +206,7 @@ func targetFormInputFrom(r *http.Request) targetFormInput { Headers: r.PostFormValue("headers"), Timeout: r.PostFormValue("timeout"), ForwardQuery: r.PostFormValue("forward_query") != "", - MaxRetries: r.PostFormValue("max_retries"), + MaxAttempts: r.PostFormValue("max_attempts"), Expiry: r.PostFormValue("expiry"), Rotation: r.PostFormValue("rotation"), } diff --git a/internal/handlers/target_create_query_test.go b/internal/handlers/target_create_query_test.go index 3eb3914..1d0603b 100644 --- a/internal/handlers/target_create_query_test.go +++ b/internal/handlers/target_create_query_test.go @@ -196,7 +196,7 @@ func TestHandleTargetCreate_QueryStringCannotSupplyNameOrType( w, _ := postTargetCreate( t, env, webhook.ID, - "name=leaky&type=slack&max_retries=9&expiry=30d&url="+ + "name=leaky&type=slack&max_attempts=9&expiry=30d&url="+ url.QueryEscape(targetSecretURL), url.Values{}, ) diff --git a/internal/handlers/target_create_test.go b/internal/handlers/target_create_test.go index 253f63e..e993d6a 100644 --- a/internal/handlers/target_create_test.go +++ b/internal/handlers/target_create_test.go @@ -21,24 +21,24 @@ func TestHandleTargetCreate_EveryType(t *testing.T) { // fields is the rest of each submission, as a query string. cases := []struct { - targetType database.TargetType - fields string - wantConfig string - wantRetries int + targetType database.TargetType + fields string + wantConfig string + wantAttempts int }{ { - database.TargetTypeHTTP, "timeout=12&max_retries=3", + database.TargetTypeHTTP, "timeout=12&max_attempts=3", `{"url":"` + editOriginalURL + `","timeout":12}`, 3, }, { - database.TargetTypeSlack, "max_retries=4", + database.TargetTypeSlack, "max_attempts=4", `{"webhookUrl":"` + editOriginalURL + `"}`, 4, }, { database.TargetTypeDatabase, "expiry=720h", - `{"expiry":"720h"}`, 0, + `{"expiry":"720h"}`, 1, }, - {database.TargetTypeLog, "", "", 0}, + {database.TargetTypeLog, "", "", 1}, } for _, tc := range cases { @@ -62,7 +62,7 @@ func TestHandleTargetCreate_EveryType(t *testing.T) { targets := targetsForWebhook(t, env.db, webhook.ID) require.Len(t, targets, 1) assert.Equal(t, tc.targetType, targets[0].Type) - assert.Equal(t, tc.wantRetries, targets[0].MaxRetries) + assert.Equal(t, tc.wantAttempts, targets[0].MaxAttempts) if tc.wantConfig == "" { assert.Empty(t, targets[0].Config) @@ -90,11 +90,11 @@ func TestHandleTargetCreate_RefusedFormComesBack(t *testing.T) { { database.TargetTypeHTTP, "name=private&url=" + editBlockedURL + - "&timeout=12&max_retries=3", + "&timeout=12&max_attempts=3", "Invalid target URL", }, { - database.TargetTypeSlack, "name=no-url&max_retries=4", + database.TargetTypeSlack, "name=no-url&max_attempts=4", "Webhook URL is required for Slack targets", }, { @@ -133,8 +133,8 @@ func TestHandleTargetCreate_RefusedFormComesBack(t *testing.T) { assert.Contains(t, page, html.EscapeString(tc.reason)) // Each value comes back in a data attribute of the targets - // section named after its field (max_retries as - // data-max-retries), except url, which comes back in + // section named after its field (max_attempts as + // data-max-attempts), except url, which comes back in // data-destination; templates/source_detail.html says why. for field := range typed { attr := "data-" + strings.ReplaceAll(field, "_", "-") diff --git a/internal/handlers/target_edit.go b/internal/handlers/target_edit.go index 9a29fb4..ce57455 100644 --- a/internal/handlers/target_edit.go +++ b/internal/handlers/target_edit.go @@ -82,7 +82,7 @@ func (h *Handlers) HandleTargetEdit() http.HandlerFunc { Headers: cfg.Headers, Timeout: cfg.Timeout, ForwardQuery: cfg.ForwardQuery, - MaxRetries: strconv.Itoa(target.MaxRetries), + MaxAttempts: strconv.Itoa(target.MaxAttempts), Expiry: cfg.Expiry, Rotation: cfg.Rotation, } diff --git a/internal/handlers/target_edit_test.go b/internal/handlers/target_edit_test.go index f73aada..57c114f 100644 --- a/internal/handlers/target_edit_test.go +++ b/internal/handlers/target_edit_test.go @@ -117,7 +117,7 @@ func seedHTTPTarget( form.Set("url", editOriginalURL) form.Set("headers", headers) form.Set("timeout", timeout) - form.Set("max_retries", "3") + form.Set("max_attempts", "3") w := serveTarget( env, http.MethodPost, @@ -179,7 +179,7 @@ func editForm(targetURL, headers, timeout string) url.Values { form.Set("url", targetURL) form.Set("headers", headers) form.Set("timeout", timeout) - form.Set("max_retries", "5") + form.Set("max_attempts", "5") return form } @@ -280,9 +280,9 @@ func TestHandleTargetEditSubmit_Saves(t *testing.T) { assertEditClearingRemovesThem(t, env) }) - t.Run("absent max_retries is not zeroed", func(t *testing.T) { + t.Run("absent max_attempts is kept", func(t *testing.T) { t.Parallel() - assertEditKeepsAbsentMaxRetries(t, env) + assertEditKeepsAbsentMaxAttempts(t, env) }) } @@ -307,7 +307,7 @@ func assertEditChangesDestination( reloaded := storedTarget(t, env, target.ID) assert.Equal(t, "edited-name", reloaded.Name) - assert.Equal(t, 5, reloaded.MaxRetries) + assert.Equal(t, 5, reloaded.MaxAttempts) assert.Equal( t, database.TargetTypeHTTP, reloaded.Type, "the edit form must not change a target's type", @@ -368,25 +368,25 @@ func assertEditClearingRemovesThem( assert.Zero(t, cfg.Timeout) } -// Retries are offered only by the forms for target types that retry. -// An absent field means the form does not edit retries, not that -// they should be turned off. -func assertEditKeepsAbsentMaxRetries( +// Delivery attempts are offered only by the forms for target types +// that retry. An absent field means the form does not edit them, not +// that retries should be turned off. +func assertEditKeepsAbsentMaxAttempts( t *testing.T, env *sourceTestEnv, ) { t.Helper() webhook, target := seedHTTPTarget(t, env, "", "") - require.Equal(t, 3, target.MaxRetries) + require.Equal(t, 3, target.MaxAttempts) form := editForm(editOriginalURL, "", "") - form.Del("max_retries") + form.Del("max_attempts") w := submitTargetEdit(env, webhook.ID, target.ID, form) require.Equal(t, http.StatusSeeOther, w.Code, w.Body.String()) assert.Equal( - t, 3, storedTarget(t, env, target.ID).MaxRetries, + t, 3, storedTarget(t, env, target.ID).MaxAttempts, ) } @@ -659,12 +659,12 @@ func TestHandleTargetEditSubmit_RefusedFormComesBack(t *testing.T) { { database.TargetTypeHTTP, "name=edited&url=" + editBlockedURL + - "&headers=X-Edited:+kept&timeout=12&max_retries=3", + "&headers=X-Edited:+kept&timeout=12&max_attempts=3", "Invalid target URL", }, { database.TargetTypeSlack, - "name=edited&url=" + editOriginalURL + "&max_retries=25", + "name=edited&url=" + editOriginalURL + "&max_attempts=25", "Invalid delivery attempts", }, { diff --git a/internal/handlers/target_retries.go b/internal/handlers/target_retries.go deleted file mode 100644 index 6e68916..0000000 --- a/internal/handlers/target_retries.go +++ /dev/null @@ -1,83 +0,0 @@ -package handlers - -import ( - "errors" - "strconv" - "strings" -) - -// maxTargetRetries bounds a target's max_retries. -// -// Both target forms already declare max="20" on the input, so this -// enforces server-side what the UI has always advertised rather than -// introducing a new limit. -// -// The number is not cosmetic. Every attempt writes a delivery_results -// row that the event log then loads and renders, and the engine backs -// off by 2^(n-1) seconds, so attempt 20 is already about six days -// after the first. A value beyond this buys no additional durability -// and only costs rows. -const maxTargetRetries = 20 - -// Errors returned when a max_retries form value cannot be turned into -// a retry count. -var ( - // errRetriesInvalid signals a max_retries form value that is not - // a non-negative whole number. - errRetriesInvalid = errors.New( - "must be a whole number", - ) - - // errRetriesTooLarge signals a max_retries form value that is a - // whole number but above maxTargetRetries. It is distinguished - // from errRetriesInvalid so the message can name the ceiling - // instead of implying the input was not a number. - errRetriesTooLarge = errors.New("out of range") -) - -// parseMaxRetries interprets a max_retries form value. -// -// An ABSENT value — the field empty or not submitted — yields -// fallback, which lets the create path apply its default and the edit -// path leave the stored value alone. A value that is SET BUT INVALID -// is an error: unparseable, negative, or above maxTargetRetries. -// -// The distinction is the whole point of this function. max_retries=0 -// means fire-and-forget, so returning 0 for input the operator typed -// but that did not parse silently disables retries on a -// store-and-forward proxy — and on the edit path it destroys a -// working retry configuration over a typo. A default answers a -// question that was not asked; it never answers one that was asked -// badly. -func parseMaxRetries(raw string, fallback int) (int, error) { - raw = strings.TrimSpace(raw) - if raw == "" { - return fallback, nil - } - - v, err := strconv.Atoi(raw) - if err != nil || v < 0 { - return 0, errRetriesInvalid - } - - if v > maxTargetRetries { - return 0, errRetriesTooLarge - } - - return v, nil -} - -// retriesErrorMessage returns the message the create and edit forms -// show for a rejected max_retries value. Any error other than -// errRetriesTooLarge falls back to the generic wording, so an -// unrecognised parse failure still produces a sensible 400. -func retriesErrorMessage(err error) string { - if errors.Is(err, errRetriesTooLarge) { - return errRetriesTooLarge.Error() + - ": at most " + strconv.Itoa(maxTargetRetries) + - " attempts" - } - - return errRetriesInvalid.Error() + - ", or 0 for fire-and-forget" -} diff --git a/internal/handlers/target_retries_test.go b/internal/handlers/target_retries_test.go deleted file mode 100644 index 686af04..0000000 --- a/internal/handlers/target_retries_test.go +++ /dev/null @@ -1,385 +0,0 @@ -package handlers_test - -import ( - "net/http" - "net/url" - "strconv" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "sneak.berlin/go/webhooker/internal/database" - "sneak.berlin/go/webhooker/internal/handlers" -) - -// retriesTargetURL is the destination the retry-validation targets -// point at. It is a literal public address rather than a hostname so -// the SSRF check resolves nothing and a sandbox without DNS cannot -// make these cases pass or fail for the wrong reason. -const retriesTargetURL = "https://93.184.216.34/hooks/retries" - -const ( - // wayAboveCeiling is the typo'd-extra-zero case from the report. - wayAboveCeiling = "999999999" - - // notANumber is the plainest garbage an operator can type, and - // the value the report submitted on the edit form. - notANumber = "abc" - - // workingRetries is the retry count a seeded target is already - // delivering with, which a rejected submission must not disturb. - workingRetries = 2 -) - -// aboveCeiling is the smallest rejected whole number. -func aboveCeiling() string { - return strconv.Itoa(handlers.MaxTargetRetriesForTest + 1) -} - -// overCeilingRetries is whole-number input past the limit, which is -// rejected with the limit named. -func overCeilingRetries() []string { - return []string{aboveCeiling(), wayAboveCeiling} -} - -// unparseableRetries is input an operator can type into the field -// that is not a retry count. Each must be REJECTED: silently reading -// any of them as 0 turns a store-and-forward proxy into -// fire-and-forget without saying so. -// -// The twenty-digit case is here because it parses as digits but -// overflows int, which is the one failure the field's own min/max -// attributes cannot describe. -func unparseableRetries() []string { - return []string{ - notANumber, - "2.7", - "-5", - "12345678901234567890", - "1e3", - } -} - -// createRetriesForm is a complete, otherwise-valid HTTP target -// creation, so the only thing any case below varies is max_retries. -func createRetriesForm(retries string) url.Values { - form := url.Values{} - form.Set("name", "retries-target") - form.Set("type", string(database.TargetTypeHTTP)) - form.Set("url", retriesTargetURL) - - if retries != absentField { - form.Set("max_retries", retries) - } - - return form -} - -// absentField marks a field the form does not submit at all, which is -// the case that legitimately takes a default and must stay distinct -// from a field submitted with garbage in it. -const absentField = "\x00absent" - -// absentRetries is every way of saying "the operator did not set -// this", each of which takes the default rather than a 400. Blank and -// whitespace-only count as absent here because they do in the timeout -// and retention controls on the same forms; a rule the fields do not -// share would be its own surprise. -func absentRetries() []string { - return []string{absentField, "", " "} -} - -// createWithRetries posts the target create form for a fresh webhook -// and returns the webhook and the response. -func createWithRetries( - t *testing.T, - env *sourceTestEnv, - retries string, -) (database.Webhook, int, string) { - t.Helper() - - webhook := seedWebhookWithRetention(t, env.db, 30) - - w := serveTarget( - env, http.MethodPost, - "/hook/"+webhook.ID+"/targets", - createRetriesForm(retries), - ) - - return webhook, w.Code, w.Body.String() -} - -// TestTargetCreate_RetriesAboveCeilingRejected proves the create form -// enforces a ceiling at all, and that the 400 names it — a rejection -// that does not say what the limit is leaves the operator guessing. -func TestTargetCreate_RetriesAboveCeilingRejected(t *testing.T) { - t.Parallel() - - env := setupSourceTest(t) - ceiling := strconv.Itoa(handlers.MaxTargetRetriesForTest) - - for _, retries := range overCeilingRetries() { - webhook, code, body := createWithRetries(t, env, retries) - - assert.Equal(t, http.StatusBadRequest, code, - "max_retries=%s should be rejected", retries) - assert.Contains(t, body, ceiling, - "the rejection for %s should name the ceiling", - retries) - assert.Empty(t, - targetsForWebhook(t, env.db, webhook.ID), - "no target should be created for %s", retries) - } -} - -// TestTargetCreate_UnparseableRetriesRejected is the core of the -// defect: each of these was accepted with HTTP 200 and stored as 0. -func TestTargetCreate_UnparseableRetriesRejected(t *testing.T) { - t.Parallel() - - env := setupSourceTest(t) - - for _, retries := range unparseableRetries() { - webhook, code, body := createWithRetries(t, env, retries) - - assert.Equal(t, http.StatusBadRequest, code, - "max_retries=%q should be rejected, not coerced", - retries) - assert.Contains(t, body, "whole number", - "the rejection for %q should say why", retries) - assert.Empty(t, - targetsForWebhook(t, env.db, webhook.ID), - "no target should be created for %q", retries) - } -} - -// TestTargetCreate_ValidRetriesStored covers the accepting half, -// including the ceiling itself: a bound that rejects its own limit -// would make the advertised maximum unreachable. -func TestTargetCreate_ValidRetriesStored(t *testing.T) { - t.Parallel() - - env := setupSourceTest(t) - - for _, want := range []int{0, 3, handlers.MaxTargetRetriesForTest} { - webhook, code, body := createWithRetries( - t, env, strconv.Itoa(want), - ) - - require.Equal(t, http.StatusSeeOther, code, body) - - targets := targetsForWebhook(t, env.db, webhook.ID) - require.Len(t, targets, 1) - assert.Equal(t, want, targets[0].MaxRetries) - } -} - -// TestTargetCreate_AbsentRetriesTakesDefault keeps the two cases -// distinct. An omitted field is not an operator asking for something -// invalid, so it still gets the fire-and-forget default rather than a -// 400 — otherwise the fix above would make the form unusable. -func TestTargetCreate_AbsentRetriesTakesDefault(t *testing.T) { - t.Parallel() - - env := setupSourceTest(t) - - for _, retries := range absentRetries() { - webhook, code, body := createWithRetries(t, env, retries) - - require.Equal(t, http.StatusSeeOther, code, body) - - targets := targetsForWebhook(t, env.db, webhook.ID) - require.Len(t, targets, 1) - assert.Equal(t, 0, targets[0].MaxRetries, - "an absent max_retries should take the default") - } -} - -// seedRetriesTarget creates an HTTP target already delivering with -// workingRetries retries, through the real create handler. -func seedRetriesTarget( - t *testing.T, - env *sourceTestEnv, -) (database.Webhook, database.Target) { - t.Helper() - - webhook, code, body := createWithRetries( - t, env, strconv.Itoa(workingRetries), - ) - require.Equal(t, http.StatusSeeOther, code, body) - - targets := targetsForWebhook(t, env.db, webhook.ID) - require.Len(t, targets, 1) - require.Equal(t, workingRetries, targets[0].MaxRetries) - - return webhook, targets[0] -} - -// editRetriesForm is a complete edit submission that changes the -// target's name as well, so a rejected submission can be shown to -// have written nothing at all rather than merely to have left -// max_retries alone. -func editRetriesForm(retries string) url.Values { - form := url.Values{} - form.Set("name", "renamed-by-edit") - form.Set("url", retriesTargetURL) - - if retries != absentField { - form.Set("max_retries", retries) - } - - return form -} - -// assertEditRejectedAndUnchanged submits an edit expected to fail and -// checks both halves of the requirement: the 400 explains itself, and -// the target it was submitted against is untouched. -func assertEditRejectedAndUnchanged( - t *testing.T, - env *sourceTestEnv, - retries, wantReason string, -) { - t.Helper() - - webhook, target := seedRetriesTarget(t, env) - - w := submitTargetEdit( - env, webhook.ID, target.ID, editRetriesForm(retries), - ) - - assert.Equal(t, http.StatusBadRequest, w.Code, - "max_retries=%q should be rejected on edit", retries) - assert.Contains(t, w.Body.String(), wantReason, - "the rejection for %q should say why", retries) - - stored := storedTarget(t, env, target.ID) - assert.Equal(t, workingRetries, stored.MaxRetries, - "a rejected edit must not destroy the working retry "+ - "count with %q", retries) - assert.Equal(t, "retries-target", stored.Name, - "a rejected edit must write nothing at all") -} - -// TestTargetEdit_UnparseableRetriesRejected is the damaging half of -// the defect. A target delivering with two retries, re-saved with a -// typo in the field, returned 200 and was left with retries disabled -// and nothing said. -func TestTargetEdit_UnparseableRetriesRejected(t *testing.T) { - t.Parallel() - - env := setupSourceTest(t) - - for _, retries := range unparseableRetries() { - assertEditRejectedAndUnchanged( - t, env, retries, "whole number", - ) - } -} - -// TestTargetEdit_RetriesAboveCeilingRejected proves the ceiling -// applies to the edit path too, naming itself, so the two paths -// cannot disagree about what is storable. -func TestTargetEdit_RetriesAboveCeilingRejected(t *testing.T) { - t.Parallel() - - env := setupSourceTest(t) - ceiling := strconv.Itoa(handlers.MaxTargetRetriesForTest) - - for _, retries := range overCeilingRetries() { - assertEditRejectedAndUnchanged(t, env, retries, ceiling) - } -} - -// TestTargetEdit_ValidRetriesStored covers the accepting half of the -// edit path, so the ceiling cannot be enforced by simply refusing -// every submission. -func TestTargetEdit_ValidRetriesStored(t *testing.T) { - t.Parallel() - - env := setupSourceTest(t) - - for _, want := range []int{0, 9, handlers.MaxTargetRetriesForTest} { - webhook, target := seedRetriesTarget(t, env) - - w := submitTargetEdit( - env, webhook.ID, target.ID, - editRetriesForm(strconv.Itoa(want)), - ) - require.Equal(t, - http.StatusSeeOther, w.Code, w.Body.String(), - ) - - assert.Equal(t, want, - storedTarget(t, env, target.ID).MaxRetries) - } -} - -// TestTargetEdit_AbsentRetriesLeavesStoredValue is the edit path's -// absent-versus-invalid case. Retries are only offered by the forms -// for types that retry, so a submission without the field must leave -// the stored count alone rather than be rejected or zeroed. -func TestTargetEdit_AbsentRetriesLeavesStoredValue(t *testing.T) { - t.Parallel() - - env := setupSourceTest(t) - - for _, retries := range absentRetries() { - webhook, target := seedRetriesTarget(t, env) - - w := submitTargetEdit( - env, webhook.ID, target.ID, - editRetriesForm(retries), - ) - require.Equal(t, - http.StatusSeeOther, w.Code, w.Body.String(), - ) - - assert.Equal(t, workingRetries, - storedTarget(t, env, target.ID).MaxRetries, - "an absent max_retries must leave the stored "+ - "count alone (%q)", retries) - } -} - -// TestTargetRetries_CreateAndEditAgreeOnEveryCase proves the two -// paths cannot disagree, which is what let the create form and the -// edit form drift apart in the first place. Every input is submitted -// to both and the accept/reject verdicts are compared. -func TestTargetRetries_CreateAndEditAgreeOnEveryCase(t *testing.T) { - t.Parallel() - - env := setupSourceTest(t) - - accepted := []string{ - "0", "1", - strconv.Itoa(handlers.MaxTargetRetriesForTest), - } - overCeiling := overCeilingRetries() - unparseable := unparseableRetries() - - cases := make( - []string, 0, - len(accepted)+len(overCeiling)+len(unparseable), - ) - cases = append(cases, accepted...) - cases = append(cases, overCeiling...) - cases = append(cases, unparseable...) - - for _, retries := range cases { - _, createCode, _ := createWithRetries(t, env, retries) - - webhook, target := seedRetriesTarget(t, env) - editCode := submitTargetEdit( - env, webhook.ID, target.ID, - editRetriesForm(retries), - ).Code - - assert.Equal(t, - createCode == http.StatusBadRequest, - editCode == http.StatusBadRequest, - "create and edit must agree on max_retries=%q "+ - "(create %d, edit %d)", - retries, createCode, editCode, - ) - } -} diff --git a/internal/handlers/target_toggle_test.go b/internal/handlers/target_toggle_test.go index 05a9695..196e54d 100644 --- a/internal/handlers/target_toggle_test.go +++ b/internal/handlers/target_toggle_test.go @@ -59,7 +59,7 @@ func TestHandleTargetToggle_DoesNotUndoAnEdit(t *testing.T) { stored := storedTarget(t, env, tgt.ID) assert.False(t, stored.Active) assert.Equal(t, "edited-name", stored.Name) - assert.Equal(t, 5, stored.MaxRetries) + assert.Equal(t, 5, stored.MaxAttempts) assert.Equal( t, editReplacedURL, storedHTTPConfig(t, env, tgt.ID).URL, ) diff --git a/internal/handlers/ui_copy_test.go b/internal/handlers/ui_copy_test.go index 8b9e655..8b5cf69 100644 --- a/internal/handlers/ui_copy_test.go +++ b/internal/handlers/ui_copy_test.go @@ -348,21 +348,19 @@ func TestEntrypointCopyButtonIsProgressiveEnhancement(t *testing.T) { ) } -// maxRetriesHelp is the wording both target forms must carry. The -// delivery core makes max_retries attempts in total, not that many +// maxAttemptsHelp is the wording both target forms must carry. The +// delivery core makes max_attempts attempts in total, not that many // retries on top of a first try (a fresh delivery starts at attempt 1 // and target_http gives up once the attempt number reaches -// max_retries), and 0 is special-cased to a single fire-and-forget -// attempt with no circuit breaker. -const maxRetriesHelp = "How many times each delivery is attempted in " + - "all, the first attempt included. 0 means a single attempt with no " + - "retries and no circuit breaker." +// max_attempts). +const maxAttemptsHelp = "How many times each delivery is attempted in " + + "all, the first attempt included." -// TestTargetFormMaxRetriesCopyMatchesBehaviour pins the max_retries +// TestTargetFormMaxAttemptsCopyMatchesBehaviour pins the max_attempts // label, "Delivery attempts", and help text on both the create form // (the add-target form on the webhook detail page) and the edit form, // so the copy cannot drift back to calling the number a retry count. -func TestTargetFormMaxRetriesCopyMatchesBehaviour(t *testing.T) { +func TestTargetFormMaxAttemptsCopyMatchesBehaviour(t *testing.T) { t.Parallel() var h *handlers.Handlers @@ -394,11 +392,11 @@ func TestTargetFormMaxRetriesCopyMatchesBehaviour(t *testing.T) { assert.Contains(t, createBody, "Delivery attempts:") assert.Contains( - t, createBody, maxRetriesHelp, - "the add-target form must explain max_retries as total attempts", + t, createBody, maxAttemptsHelp, + "the add-target form must explain max_attempts as total attempts", ) - // A slack target exercises the same max_retries field while needing + // A slack target exercises the same max_attempts field while needing // only a URL from the edit template, so the test data stays // minimal. The Target and TargetForm keys mirror the field names // the template reads off the handler's values. @@ -412,8 +410,8 @@ func TestTargetFormMaxRetriesCopyMatchesBehaviour(t *testing.T) { "Active": true, }, "TargetForm": map[string]any{ - "URL": "https://hooks.slack.com/services/x", - "MaxRetries": "3", + "URL": "https://hooks.slack.com/services/x", + "MaxAttempts": "3", }, dataKeyError: "", }, @@ -421,8 +419,8 @@ func TestTargetFormMaxRetriesCopyMatchesBehaviour(t *testing.T) { assert.Contains(t, editBody, `class="label">Delivery attempts`) assert.Contains( - t, editBody, maxRetriesHelp, - "the target edit form must explain max_retries as total attempts", + t, editBody, maxAttemptsHelp, + "the target edit form must explain max_attempts as total attempts", ) } diff --git a/internal/handlers/webhook.go b/internal/handlers/webhook.go index 4417de4..6358134 100644 --- a/internal/handlers/webhook.go +++ b/internal/handlers/webhook.go @@ -433,7 +433,7 @@ func buildDeliveryTasks( TargetName: targets[i].Name, TargetType: targets[i].Type, TargetConfig: targets[i].Config, - MaxRetries: targets[i].MaxRetries, + MaxAttempts: targets[i].MaxAttempts, Method: event.Method, RawQuery: event.RawQuery, Headers: event.Headers, diff --git a/internal/server/alpine_browser_test.go b/internal/server/alpine_browser_test.go index d7b3a71..6587a59 100644 --- a/internal/server/alpine_browser_test.go +++ b/internal/server/alpine_browser_test.go @@ -363,11 +363,11 @@ func checkAddEachTargetType(ctx context.Context, t *testing.T, url string) { }{ { "http", "http", - "csrf_token name type url headers timeout max_retries", + "csrf_token name type url headers timeout max_attempts", map[string]string{"url": publicTargetURL}, }, { - "slack", "slack", "csrf_token name type url max_retries", + "slack", "slack", "csrf_token name type url max_attempts", map[string]string{"url": publicTargetURL}, }, { @@ -639,11 +639,11 @@ func checkRefusedEdits( values map[string]string }{ {page + "/targets/" + targetID + "/edit", map[string]string{ - "#name": "edited-target", - "#url": "http://127.0.0.1/hook", - "#headers": "X-Edited: kept", - "#timeout": "12", - "#max_retries": "3", + "#name": "edited-target", + "#url": "http://127.0.0.1/hook", + "#headers": "X-Edited: kept", + "#timeout": "12", + "#max_attempts": "3", }}, {page + "/edit", map[string]string{ "#name": "edited-webhook", diff --git a/internal/server/archive_download_test.go b/internal/server/archive_download_test.go index 6e2239a..71da33d 100644 --- a/internal/server/archive_download_test.go +++ b/internal/server/archive_download_test.go @@ -27,10 +27,11 @@ func TestHook_DownloadArchive(t *testing.T) { env.seedTarget(t, wh.ID) archive := &database.Target{ - WebhookID: wh.ID, - Name: "kept", - Type: database.TargetTypeDatabase, - Active: true, + WebhookID: wh.ID, + Name: "kept", + Type: database.TargetTypeDatabase, + Active: true, + MaxAttempts: 1, } require.NoError(t, env.db.DB().Omit(clause.Associations).Create(archive).Error, diff --git a/internal/server/routes_test.go b/internal/server/routes_test.go index a74be16..f7d9b57 100644 --- a/internal/server/routes_test.go +++ b/internal/server/routes_test.go @@ -402,11 +402,12 @@ func (e *testEnv) seedTarget( t.Helper() tgt := &database.Target{ - WebhookID: webhookID, - Name: "routed-target", - Type: database.TargetTypeHTTP, - Active: true, - Config: `{"url":"http://93.184.216.34/hook"}`, + WebhookID: webhookID, + Name: "routed-target", + Type: database.TargetTypeHTTP, + Active: true, + Config: `{"url":"http://93.184.216.34/hook"}`, + MaxAttempts: 1, } require.NoError( diff --git a/static/js/app.js b/static/js/app.js index 3d91446..7ae079e 100644 --- a/static/js/app.js +++ b/static/js/app.js @@ -151,7 +151,7 @@ document.addEventListener("alpine:init", function () { headers: "", timeout: "", forwardQuery: false, - maxRetries: "", + maxAttempts: "", expiry: "", rotation: "", init() { @@ -165,7 +165,7 @@ document.addEventListener("alpine:init", function () { this.timeout = refused.timeout; this.forwardQuery = this.$root.hasAttribute("data-forward-query"); - this.maxRetries = refused.maxRetries; + this.maxAttempts = refused.maxAttempts; this.expiry = refused.expiry; this.rotation = refused.rotation; }, @@ -185,7 +185,7 @@ document.addEventListener("alpine:init", function () { this.headers = ""; this.timeout = ""; this.forwardQuery = false; - this.maxRetries = ""; + this.maxAttempts = ""; this.expiry = ""; this.rotation = ""; this.$refs.form.reset(); diff --git a/templates/source_detail.html b/templates/source_detail.html index ecf3fc5..a47c5df 100644 --- a/templates/source_detail.html +++ b/templates/source_detail.html @@ -135,7 +135,7 @@ data-headers="{{.TargetForm.Headers}}" data-timeout="{{.TargetForm.Timeout}}" {{if .TargetForm.ForwardQuery}}data-forward-query{{end}} - data-max-retries="{{.TargetForm.MaxRetries}}" + data-max-attempts="{{.TargetForm.MaxAttempts}}" data-expiry="{{.TargetForm.Expiry}}" data-rotation="{{.TargetForm.Rotation}}">
How many times each delivery is attempted in all, the first attempt included. 0 means a single attempt with no retries and no circuit breaker.
+How many times each delivery is attempted in all, the first attempt included.
How many times each delivery is attempted in all, the first attempt included. 0 means a single attempt with no retries and no circuit breaker.
+How many times each delivery is attempted in all, the first attempt included.
How many times each delivery is attempted in all, the first attempt included. 0 means a single attempt with no retries and no circuit breaker.
+ + +How many times each delivery is attempted in all, the first attempt included.