Author SHA1 Message Date
sneak 433fdceee2 Say max_retries is the total attempt count, not a retry count (closes #316)
check / check (push) Failing after 1s
The delivery core makes max_retries attempts in total: a fresh delivery
starts at attempt one and gives up once the attempt number reaches
max_retries, so 3 is three attempts, not four, and 0 is special-cased to
a single fire-and-forget attempt with no retries and no circuit breaker.
The create and edit target forms called it "Max retries" with no total,
and the README data-model row called it "maximum retry attempts", so an
operator wanting "try, then retry twice" would enter the wrong number.

Both forms and the README rows now state the number is the total number
of delivery attempts, with the 0 case spelled out. The delivery
arithmetic is unchanged. A UI copy test renders both forms and pins the
shared wording so it cannot drift back to a retry count.

Model: opus-4-8
2026-09-21 07:55:07 +00:00
8 changed files with 95 additions and 111 deletions
+7 -23
View File
@@ -1507,7 +1507,7 @@ events should be forwarded.
| `type` | TargetType | One of: `http`, `slack`, `database`, `log` | | `type` | TargetType | One of: `http`, `slack`, `database`, `log` |
| `active` | boolean | Whether deliveries are enabled (default: true) | | `active` | boolean | Whether deliveries are enabled (default: true) |
| `config` | JSON text | Type-specific configuration | | `config` | JSON text | Type-specific configuration |
| `max_retries` | integer | Maximum retry attempts for `http` and `slack` targets (0 = fire-and-forget, >0 = retries with backoff and a circuit breaker). Ignored by `database` and `log` targets | | `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 |
| `max_queue_size` | integer | Stored and shown on the target's detail view, but not enforced anywhere yet: nothing in the delivery engine consults it. Queue depth is set by the two fixed 10,000-entry channels | | `max_queue_size` | integer | Stored and shown on the target's detail view, but not enforced anywhere yet: nothing in the delivery engine consults it. Queue depth is set by the two fixed 10,000-entry channels |
**Relations:** Belongs to Webhook. Has many Deliveries. **Relations:** Belongs to Webhook. Has many Deliveries.
@@ -1515,12 +1515,12 @@ events should be forwarded.
**Target types:** **Target types:**
- **`http`** — Forward the event as an HTTP POST to a configured URL. - **`http`** — Forward the event as an HTTP POST to a configured URL.
Behavior depends on `max_retries`: when `max_retries` is 0 (the `max_retries` is the total number of delivery attempts, not retries on
default), the target operates in fire-and-forget mode — a single top of the first: when `max_retries` is 0 (the default), the target
attempt with no retries and no circuit breaker. When `max_retries` is operates in fire-and-forget mode, a single attempt with no retries and
greater than 0, failed deliveries are retried with exponential backoff no circuit breaker; a value of N makes up to N attempts in all,
up to `max_retries` attempts, protected by a per-target circuit retrying failed deliveries with exponential backoff and protecting them
breaker. with a per-target circuit breaker.
- **`slack`** — Post the event as a formatted message to a - **`slack`** — Post the event as a formatted message to a
Slack-compatible incoming webhook URL (`webhookUrl` in `config`). It Slack-compatible incoming webhook URL (`webhookUrl` in `config`). It
is built on the same HTTP core as `http` and honours `max_retries` is built on the same HTTP core as `http` and honours `max_retries`
@@ -1700,22 +1700,6 @@ retries) is individually logged for full observability.
**Relations:** Belongs to Delivery. **Relations:** Belongs to Delivery.
#### Event-tier indexes
Beyond the primary keys, the per-webhook event databases carry secondary
indexes on the columns the background work reads by, each created by
`AutoMigrate` on a fresh and on an existing database:
| Column | Serves |
| ------------------------------ | ------ |
| `deliveries.status` | The recovery and sweep queries that select deliveries by status once a minute |
| `deliveries.event_id` | Loading a page of the event log, which reads deliveries by event |
| `delivery_results.delivery_id` | Loading a page of the event log, which reads results by delivery |
| `events.created_at` | Retention, which deletes events by age |
The `events.resubmitted_from_id` column is also indexed, to resolve the
resubmit relationship both ways in the event log.
#### Common Fields #### Common Fields
Every entity except `Setting` includes these fields from `BaseModel`. Every entity except `Setting` includes these fields from `BaseModel`.
@@ -1,72 +0,0 @@
package database_test
import (
"context"
"testing"
"github.com/google/uuid"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"sneak.berlin/go/webhooker/internal/database"
)
// indexedColumn names a secondary index by the model and struct field
// GORM derives the index name from.
type indexedColumn struct {
model any
field string
}
// eventTierIndexes are the columns the background work reads by: the
// recovery and sweep queries (status), the event log (event_id and
// delivery_id) and retention (created_at).
var eventTierIndexes = []indexedColumn{
{&database.Delivery{}, "Status"},
{&database.Delivery{}, "EventID"},
{&database.DeliveryResult{}, "DeliveryID"},
{&database.Event{}, "CreatedAt"},
}
// TestWebhookDBManager_OpenAddsEventTierIndexes verifies that opening a
// per-webhook database that predates these indexes creates them, so the
// queries above stop scanning whole tables. It stands in for an older
// database file by dropping the indexes AutoMigrate just created, then
// reopening the same file.
func TestWebhookDBManager_OpenAddsEventTierIndexes(t *testing.T) {
t.Parallel()
mgr, lc := setupTestWebhookDBManager(t)
ctx := context.Background()
require.NoError(t, lc.Start(ctx))
defer func() { require.NoError(t, lc.Stop(ctx)) }()
webhookID := uuid.New().String()
db, err := mgr.GetDB(webhookID)
require.NoError(t, err)
// A fresh database has them.
for _, ix := range eventTierIndexes {
require.True(t, db.Migrator().HasIndex(ix.model, ix.field))
}
// Stand in for a database file created before the indexes existed.
for _, ix := range eventTierIndexes {
require.NoError(t, db.Migrator().DropIndex(ix.model, ix.field))
require.False(t, db.Migrator().HasIndex(ix.model, ix.field))
}
// Drop the cached connection so the next open reopens the file and
// runs AutoMigrate against it, as a restart would.
require.NoError(t, mgr.CloseAll())
db, err = mgr.GetDB(webhookID)
require.NoError(t, err)
for _, ix := range eventTierIndexes {
assert.True(t, db.Migrator().HasIndex(ix.model, ix.field),
"opening the existing database should create the index on %s",
ix.field)
}
}
+3 -3
View File
@@ -32,9 +32,9 @@ func (s DeliveryStatus) Terminal() bool {
type Delivery struct { type Delivery struct {
BaseModel BaseModel
EventID string `gorm:"type:uuid;not null;index" json:"eventId"` EventID string `gorm:"type:uuid;not null" json:"eventId"`
TargetID string `gorm:"type:uuid;not null" json:"targetId"` TargetID string `gorm:"type:uuid;not null" json:"targetId"`
Status DeliveryStatus `gorm:"not null;default:'pending';index" json:"status"` Status DeliveryStatus `gorm:"not null;default:'pending'" json:"status"`
// Relations // Relations
Event Event `json:"event,omitzero"` Event Event `json:"event,omitzero"`
+1 -1
View File
@@ -4,7 +4,7 @@ package database
type DeliveryResult struct { type DeliveryResult struct {
BaseModel BaseModel
DeliveryID string `gorm:"type:uuid;not null;index" json:"deliveryId"` DeliveryID string `gorm:"type:uuid;not null" json:"deliveryId"`
AttemptNum int `gorm:"not null" json:"attemptNum"` AttemptNum int `gorm:"not null" json:"attemptNum"`
Success bool `json:"success"` Success bool `json:"success"`
StatusCode int `json:"statusCode,omitempty"` StatusCode int `json:"statusCode,omitempty"`
-8
View File
@@ -1,17 +1,9 @@
package database package database
import "time"
// Event represents a captured webhook event // Event represents a captured webhook event
type Event struct { type Event struct {
BaseModel BaseModel
// CreatedAt overrides BaseModel.CreatedAt only to add an index:
// retention deletes events by age, so events.created_at is queried
// on every sweep. The other tables keep the unindexed BaseModel
// field.
CreatedAt time.Time `gorm:"index" json:"createdAt"`
WebhookID string `gorm:"type:uuid;not null" json:"webhookId"` WebhookID string `gorm:"type:uuid;not null" json:"webhookId"`
EntrypointID string `gorm:"type:uuid;not null" json:"entrypointId"` EntrypointID string `gorm:"type:uuid;not null" json:"entrypointId"`
+77
View File
@@ -300,3 +300,80 @@ func TestEntrypointCopyButtonIsProgressiveEnhancement(t *testing.T) {
"the page must render to completion, not abort partway", "the page must render to completion, not abort partway",
) )
} }
// maxRetriesHelp is the wording both target forms must carry. The
// delivery core makes max_retries 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 = "This is the total number of delivery attempts, " +
"not retries on top of the first: a value of 3 makes three attempts " +
"in all. 0 means a single attempt with no retries and no circuit " +
"breaker."
// TestTargetFormMaxRetriesCopyMatchesBehaviour pins the max_retries
// 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) {
t.Parallel()
var h *handlers.Handlers
var sess *session.Session
app := newTestApp(t, &h, &sess)
app.RequireStart()
t.Cleanup(app.RequireStop)
webhook := &database.Webhook{Name: "wh", RetentionDays: 14}
webhook.ID = testWebhookID
entrypoint := database.Entrypoint{Path: "abc123"}
entrypoint.ID = "ep-1"
createBody := renderPage(
t, h, sess, "source_detail.html", map[string]any{
dataKeyWebhook: webhook,
"Entrypoints": handlers.NewEntrypointViews(
[]database.Entrypoint{entrypoint},
),
"Targets": delivery.NewTargetViews(nil),
"Events": []database.Event{},
"BaseURL": "https://hooks.example.com",
},
)
assert.Contains(
t, createBody, maxRetriesHelp,
"the add-target form must explain max_retries as total attempts",
)
// A slack target exercises the same max_retries field while needing
// only Config.URL from the edit template, so the test data stays
// minimal. The Target key mirrors the field names the template reads
// off the handler's view value.
editBody := renderPage(
t, h, sess, "target_edit.html", map[string]any{
dataKeyWebhook: webhook,
"Target": map[string]any{
"ID": "tg-1",
"Name": "t",
"Type": "slack",
"Active": true,
"MaxRetries": 3,
"Config": map[string]any{
"URL": "https://hooks.slack.com/services/x",
},
},
dataKeyError: "",
},
)
assert.Contains(
t, editBody, maxRetriesHelp,
"the target edit form must explain max_retries as total attempts",
)
}
+6 -3
View File
@@ -120,9 +120,12 @@
<label class="text-sm text-gray-700">Timeout (seconds, blank = default):</label> <label class="text-sm text-gray-700">Timeout (seconds, blank = default):</label>
<input type="number" name="timeout" min="0" max="300" :disabled="targetType !== 'http'" class="input text-sm w-24"> <input type="number" name="timeout" min="0" max="300" :disabled="targetType !== 'http'" class="input text-sm w-24">
</div> </div>
<div x-show="targetType === 'http'" class="flex gap-2 items-center"> <div x-show="targetType === 'http'">
<label class="text-sm text-gray-700">Max retries (0 = fire-and-forget):</label> <div class="flex gap-2 items-center">
<input type="number" name="max_retries" value="0" min="0" max="20" class="input text-sm w-24"> <label class="text-sm text-gray-700">Max retries:</label>
<input type="number" name="max_retries" value="0" min="0" max="20" class="input text-sm w-24">
</div>
<p class="text-xs text-gray-500 mt-1">This is the total number of delivery attempts, not retries on top of the first: a value of 3 makes three attempts in all. 0 means a single attempt with no retries and no circuit breaker.</p>
</div> </div>
<div x-show="targetType === 'slack'"> <div x-show="targetType === 'slack'">
<input type="url" name="url" placeholder="https://hooks.slack.com/services/..." :disabled="targetType !== 'slack'" class="input text-sm"> <input type="url" name="url" placeholder="https://hooks.slack.com/services/..." :disabled="targetType !== 'slack'" class="input text-sm">
+1 -1
View File
@@ -69,7 +69,7 @@
<div class="form-group"> <div class="form-group">
<label for="max_retries" class="label">Max retries</label> <label for="max_retries" class="label">Max retries</label>
<input type="number" id="max_retries" name="max_retries" value="{{.Target.MaxRetries}}" min="0" max="20" class="input"> <input type="number" id="max_retries" name="max_retries" value="{{.Target.MaxRetries}}" min="0" max="20" class="input">
<p class="text-xs text-gray-500 mt-1">0 is fire-and-forget: one attempt, no circuit breaker.</p> <p class="text-xs text-gray-500 mt-1">This is the total number of delivery attempts, not retries on top of the first: a value of 3 makes three attempts in all. 0 means a single attempt with no retries and no circuit breaker.</p>
</div> </div>
{{end}} {{end}}