1 Commits

Author SHA1 Message Date
clawbot
0384de4a7b Terminally fail retrying deliveries with a non-retry target type (closes #82)
All checks were successful
check / check (push) Successful in 4m4s
Restart recovery and the 60s retry sweep both looked an orphaned
`retrying` delivery's target up in the registry and silently returned
when it did not implement `rescheduler`. If a target's type was edited
from a retry type (`http`/`slack`) to a fire-and-forget type
(`database`/`log`) or an unknown one while a delivery was still
retrying, that delivery stayed `retrying` forever.

Both sites now hand the delivery to one shared helper,
`failUnretryableRetry`, which records a `DeliveryResult` naming the
current target type as the reason and marks the delivery `failed`. It
logs at warn, not error: this is operator-caused state, not a system
fault.

Re-dispatching under the new type was rejected as it would perform a
delivery the operator never asked for; the event itself stays in the
per-webhook event database, so manual redelivery can recover it
deliberately.

Fire-and-forget targets never set status `retrying` under normal
operation, so this path stays unreachable for them in practice.
2026-08-09 05:51:51 +00:00
17 changed files with 328 additions and 936 deletions

View File

@@ -307,20 +307,13 @@ event routing.
| `user_id` | UUID | Foreign key → User | | `user_id` | UUID | Foreign key → User |
| `name` | string | Human-readable name | | `name` | string | Human-readable name |
| `description` | string | Optional description | | `description` | string | Optional description |
| `retention_days` | integer | Days to retain events (default: 30; 0 means retain forever) | | `retention_days` | integer | Days to retain events (default: 30) |
**Relations:** Belongs to User. Has many Entrypoints. Has many Targets. **Relations:** Belongs to User. Has many Entrypoints. Has many Targets.
The `retention_days` field controls how long event data is kept in the The `retention_days` field controls how long event data is kept in the
webhook's dedicated database before automatic cleanup. webhook's dedicated database before automatic cleanup.
Setting `retention_days` to `0` means "retain events forever". Because
the column carries a default of 30, a literal zero cannot survive an
insert, so a zero is rewritten on save to a sentinel of `365 * 1000`
days (`database.RetentionForeverDays`). The retention reaper recognises
that sentinel and skips the webhook entirely, and the web UI displays
such a webhook's retention as "forever" rather than as a day count.
#### Entrypoint #### Entrypoint
A receiver URL where external services POST webhook events. Each A receiver URL where external services POST webhook events. Each
@@ -516,7 +509,7 @@ This separation provides:
DB; the event database file is hard-deleted (permanently removed). DB; the event database file is hard-deleted (permanently removed).
- **Per-webhook retention** — the `retention_days` field on each webhook - **Per-webhook retention** — the `retention_days` field on each webhook
controls automatic cleanup of old events in that webhook's database controls automatic cleanup of old events in that webhook's database
only, or disables cleanup entirely when set to `0` (retain forever). only.
- **Performance** — each webhook's database has its own WAL, its own - **Performance** — each webhook's database has its own WAL, its own
page cache, and its own lock, so concurrent event ingestion across page cache, and its own lock, so concurrent event ingestion across
webhooks won't contend. webhooks won't contend.
@@ -643,6 +636,18 @@ This means:
durable fallback that ensures no retry is permanently lost, even under durable fallback that ensures no retry is permanently lost, even under
extreme backpressure. extreme backpressure.
**Changing a target's type does not migrate in-flight deliveries.** Only
`http` and `slack` targets own durable retries; `database` and `log`
targets are fire-and-forget and never produce a `retrying` delivery. If a
target's `type` is edited from a retrying type to a non-retrying (or
unknown) one while one of its deliveries is still `retrying`, both
recovery paths above terminally mark that delivery `failed` and record a
`DeliveryResult` naming the current target type as the reason, logging it
at warn level. The delivery is not re-dispatched under the new type — the
operator never asked for that delivery — and the event itself remains
stored in the per-webhook event database, so it can be redelivered
manually.
### Circuit Breaker (HTTP Targets with Retries) ### Circuit Breaker (HTTP Targets with Retries)
HTTP targets with `max_retries` > 0 are protected by a **per-target circuit breaker** that HTTP targets with `max_retries` > 0 are protected by a **per-target circuit breaker** that

10
TODO.md
View File

@@ -28,12 +28,10 @@ databases currently grow without bound.
# Completed Steps # Completed Steps
- 2026-08-09 Make retain-forever reachable from the normal create and - 2026-08-09 Restart recovery and the 60s retry sweep terminally fail an
edit flows (#79): a `RetentionForeverDays = 365 * 1000` sentinel, a orphaned `retrying` delivery whose target type no longer supports
`Webhook.BeforeSave` hook rewriting any non-positive `retention_days` retries, recording a `DeliveryResult` with the reason instead of
to it ahead of GORM's own column defaulting, a reaper that skips such leaving the delivery stuck forever (#82)
webhooks outright, form validation that honours `0` and rejects
garbage with a 400, and a retention UI that says "forever"
- 2026-08-07 Update golangci-lint to v2.12.2 (Docker image digest in - 2026-08-07 Update golangci-lint to v2.12.2 (Docker image digest in
`Dockerfile`, release-archive sha256 pins in `script/bootstrap`), `Dockerfile`, release-archive sha256 pins in `script/bootstrap`),
adopt the canonical `.golangci.yml` (v2 `linters.settings` layout so adopt the canonical `.golangci.yml` (v2 `linters.settings` layout so

View File

@@ -18,11 +18,6 @@ const (
testVersion = "test" testVersion = "test"
// testContentType is the event content type used in tests. // testContentType is the event content type used in tests.
testContentType = "application/json" testContentType = "application/json"
// testWebhookName is the Webhook.Name used in tests.
testWebhookName = "test-webhook"
// testForeverLabel is Webhook.RetentionLabel for a retain-forever
// webhook.
testForeverLabel = "forever"
) )
func setupTestDB( func setupTestDB(

View File

@@ -1,39 +1,6 @@
package database package database
import (
"strconv"
"gorm.io/gorm"
)
const (
// DefaultRetentionDays is the event retention period applied to a
// webhook created without an explicit retention value. It is the
// single source of truth for that policy and must stay in sync
// with the `gorm:"default:30"` column default on
// Webhook.RetentionDays below; a struct tag cannot reference a
// constant, so a test asserts the two agree.
DefaultRetentionDays = 30
// RetentionForeverDays is the sentinel RetentionDays value meaning
// "retain events forever". Users express that intent as 0, which
// Webhook.BeforeSave rewrites to this value: the column default
// substitutes DefaultRetentionDays for a zero value at insert
// time, so a zero can never survive a round trip to the database.
// Nothing outside this file may hardcode the number.
RetentionForeverDays = 365 * 1000
)
// Webhook represents a webhook processing unit that groups entrypoints and targets // Webhook represents a webhook processing unit that groups entrypoints and targets
//
// The receiver kinds below are deliberately mixed. BeforeSave has to
// take a pointer because it mutates the record, and GORM only invokes
// hooks declared that way. RetainsForever and RetentionLabel have to
// take values because html/template calls them on webhooks held in a
// template data map, which reflection cannot address; a pointer
// receiver there fails at render time rather than at compile time.
//
//nolint:recvcheck // GORM needs a pointer hook; templates need values.
type Webhook struct { type Webhook struct {
BaseModel BaseModel
@@ -41,9 +8,7 @@ type Webhook struct {
Name string `gorm:"not null" json:"name"` Name string `gorm:"not null" json:"name"`
Description string `json:"description"` Description string `json:"description"`
// RetentionDays is the number of days to retain events. A value of // RetentionDays is the number of days to retain events.
// RetentionForeverDays means retain forever. The column default
// must equal DefaultRetentionDays.
RetentionDays int `gorm:"default:30" json:"retentionDays"` RetentionDays int `gorm:"default:30" json:"retentionDays"`
// Relations // Relations
@@ -51,44 +16,3 @@ type Webhook struct {
Entrypoints []Entrypoint `json:"entrypoints,omitempty"` Entrypoints []Entrypoint `json:"entrypoints,omitempty"`
Targets []Target `json:"targets,omitempty"` Targets []Target `json:"targets,omitempty"`
} }
// BeforeSave normalises RetentionDays on every insert and update. A
// non-positive value is the user's way of asking for "retain forever",
// which is stored as the RetentionForeverDays sentinel.
//
// This has to happen in a hook rather than at the call sites. GORM
// substitutes the column default (DefaultRetentionDays) for a zero
// value while building the insert statement, which runs after
// BeforeSave; rewriting any later than this loses that race and the
// row lands at 30 days. Living on the model also means a future call
// site — a REST API, a fixture, a migration — cannot bypass it.
func (w *Webhook) BeforeSave(_ *gorm.DB) error {
if w.RetentionDays <= 0 {
w.RetentionDays = RetentionForeverDays
}
return nil
}
// RetainsForever reports whether this webhook's events are kept
// indefinitely. It accepts the RetentionForeverDays sentinel written by
// BeforeSave and, defensively, the non-positive values that rows
// written before the sentinel existed may still carry.
func (w Webhook) RetainsForever() bool {
return w.RetentionDays <= 0 ||
w.RetentionDays >= RetentionForeverDays
}
// RetentionLabel returns the webhook's retention policy as display
// text, so that no template has to know about the sentinel value.
func (w Webhook) RetentionLabel() string {
if w.RetainsForever() {
return "forever"
}
if w.RetentionDays == 1 {
return "1 day"
}
return strconv.Itoa(w.RetentionDays) + " days"
}

View File

@@ -1,184 +0,0 @@
package database_test
import (
"context"
"reflect"
"strconv"
"testing"
"github.com/google/uuid"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"gorm.io/gorm"
"gorm.io/gorm/clause"
"sneak.berlin/go/webhooker/internal/database"
)
// startedTestDB returns a started main database for model-level tests.
func startedTestDB(t *testing.T) *gorm.DB {
t.Helper()
db, lc := setupTestDB(t)
ctx := context.Background()
require.NoError(t, lc.Start(ctx))
t.Cleanup(func() { require.NoError(t, lc.Stop(ctx)) })
return db.DB()
}
// storedRetention reads the retention_days column straight out of the
// row, so the assertion is about what was persisted rather than about
// whatever the in-memory struct happens to hold.
func storedRetention(t *testing.T, db *gorm.DB, id string) int {
t.Helper()
var got int
require.NoError(
t,
db.Model(&database.Webhook{}).
Where("id = ?", id).
Pluck("retention_days", &got).Error,
)
return got
}
// newWebhookWithRetention creates a webhook through the ordinary Create
// path, so the BeforeSave hook and the GORM column default both apply
// exactly as they do in production.
func newWebhookWithRetention(
t *testing.T,
db *gorm.DB,
wh *database.Webhook,
) string {
t.Helper()
wh.UserID = uuid.New().String()
wh.Name = testWebhookName
require.NoError(
t,
db.Omit(clause.Associations).Create(wh).Error,
)
return wh.ID
}
func TestWebhookBeforeSave_ZeroBecomesForeverSentinel(t *testing.T) {
t.Parallel()
db := startedTestDB(t)
wh := &database.Webhook{RetentionDays: 0}
id := newWebhookWithRetention(t, db, wh)
assert.Equal(
t,
database.RetentionForeverDays,
storedRetention(t, db, id),
"a zero retention must be stored as the sentinel, "+
"not replaced by the column default",
)
}
func TestWebhookBeforeSave_NegativeBecomesForeverSentinel(t *testing.T) {
t.Parallel()
db := startedTestDB(t)
wh := &database.Webhook{RetentionDays: -5}
id := newWebhookWithRetention(t, db, wh)
assert.Equal(
t,
database.RetentionForeverDays,
storedRetention(t, db, id),
)
}
func TestWebhookBeforeSave_PositiveIsPreserved(t *testing.T) {
t.Parallel()
db := startedTestDB(t)
wh := &database.Webhook{RetentionDays: 7}
id := newWebhookWithRetention(t, db, wh)
assert.Equal(t, 7, storedRetention(t, db, id))
}
// TestWebhookBeforeSave_UpdateToZeroBecomesSentinel proves the hook
// fires on update as well as insert, via the same Save call the edit
// handler makes.
func TestWebhookBeforeSave_UpdateToZeroBecomesSentinel(t *testing.T) {
t.Parallel()
db := startedTestDB(t)
wh := &database.Webhook{RetentionDays: 30}
id := newWebhookWithRetention(t, db, wh)
require.Equal(t, 30, storedRetention(t, db, id))
wh.RetentionDays = 0
require.NoError(t, db.Omit(clause.Associations).Save(wh).Error)
assert.Equal(
t,
database.RetentionForeverDays,
storedRetention(t, db, id),
)
}
// TestWebhookRetentionColumnDefaultMatchesConstant guards the one place
// the default lives twice: a struct tag cannot reference a constant, so
// this asserts the tag and DefaultRetentionDays agree.
func TestWebhookRetentionColumnDefaultMatchesConstant(t *testing.T) {
t.Parallel()
field, ok := reflect.TypeFor[database.Webhook]().
FieldByName("RetentionDays")
require.True(t, ok, "Webhook.RetentionDays must exist")
assert.Equal(
t,
"default:"+strconv.Itoa(database.DefaultRetentionDays),
field.Tag.Get("gorm"),
)
}
func TestWebhookRetainsForeverAndLabel(t *testing.T) {
t.Parallel()
cases := []struct {
name string
days int
forever bool
label string
}{
{
"sentinel",
database.RetentionForeverDays, true, testForeverLabel,
},
{
"above sentinel",
database.RetentionForeverDays + 1, true, testForeverLabel,
},
{"legacy zero", 0, true, testForeverLabel},
{"legacy negative", -1, true, testForeverLabel},
{"default", database.DefaultRetentionDays, false, "30 days"},
{"one day", 1, false, "1 day"},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
wh := database.Webhook{RetentionDays: tc.days}
assert.Equal(t, tc.forever, wh.RetainsForever())
assert.Equal(t, tc.label, wh.RetentionLabel())
})
}
}

View File

@@ -114,8 +114,7 @@ func (r *RetentionReaper) run(ctx context.Context) {
} }
// sweep lists every webhook from the main database and reaps expired // sweep lists every webhook from the main database and reaps expired
// rows from each per-webhook database that has a finite retention // rows from each per-webhook database whose RetentionDays is positive.
// policy. Webhooks set to retain forever are skipped entirely.
func (r *RetentionReaper) sweep(ctx context.Context) { func (r *RetentionReaper) sweep(ctx context.Context) {
var webhooks []Webhook var webhooks []Webhook
@@ -140,13 +139,8 @@ func (r *RetentionReaper) sweep(ctx context.Context) {
wh := webhooks[i] wh := webhooks[i]
// Skip retain-forever webhooks before building any query. // RetentionDays of zero or less means retain forever.
// RetainsForever covers both the RetentionForeverDays if wh.RetentionDays <= 0 {
// sentinel and the non-positive values that predate it: the
// sentinel is a positive number, so without this the reaper
// would compute a cutoff a thousand years in the past and
// issue a DELETE matching nothing on every single sweep.
if wh.RetainsForever() {
continue continue
} }

View File

@@ -77,7 +77,7 @@ func createWebhook(
wh := &database.Webhook{ wh := &database.Webhook{
UserID: uuid.New().String(), UserID: uuid.New().String(),
Name: testWebhookName, Name: "test-webhook",
RetentionDays: retentionDays, RetentionDays: retentionDays,
} }
require.NoError( require.NoError(
@@ -85,11 +85,10 @@ func createWebhook(
db.Omit(clause.Associations).Create(wh).Error, db.Omit(clause.Associations).Create(wh).Error,
) )
// Webhook.BeforeSave rewrites a non-positive RetentionDays to the // The RetentionDays column carries a GORM default of 30, so a
// retain-forever sentinel, and the column's GORM default would // zero (or negative) value passed to Create is replaced by that
// otherwise substitute 30. Force the requested value with a // default. Force the requested value explicitly so the
// column-level update so tests can plant legacy rows that predate // retain-forever (<= 0) path can be exercised.
// the sentinel and still carry a literal 0 or negative value.
require.NoError( require.NoError(
t, t,
db.Model(wh). db.Model(wh).
@@ -99,30 +98,6 @@ func createWebhook(
return wh.ID return wh.ID
} }
// createWebhookNormally inserts a webhook through the ordinary Create
// path, with no column-level forcing, so Webhook.BeforeSave applies
// exactly as it does in production. Passing 0 therefore yields a row
// holding the RetentionForeverDays sentinel.
func createWebhookNormally(
t *testing.T,
db *gorm.DB,
retentionDays int,
) string {
t.Helper()
wh := &database.Webhook{
UserID: uuid.New().String(),
Name: testWebhookName,
RetentionDays: retentionDays,
}
require.NoError(
t,
db.Omit(clause.Associations).Create(wh).Error,
)
return wh.ID
}
// eventChain is the set of row IDs seeded for a single event. // eventChain is the set of row IDs seeded for a single event.
type eventChain struct { type eventChain struct {
eventID string eventID string
@@ -281,59 +256,12 @@ func TestRetentionReaper_ReapsExpiredKeepsRecent(t *testing.T) {
assertChainPresent(t, db, recent) assertChainPresent(t, db, recent)
} }
// TestRetentionReaper_SkipsSentinelReapsFiniteInSameSweep covers the
// end-to-end retain-forever path: a webhook created the normal way with
// a requested retention of 0 lands on the RetentionForeverDays
// sentinel, and the reaper leaves its ancient events alone while still
// reaping a finite-retention webhook in the very same sweep.
func TestRetentionReaper_SkipsSentinelReapsFiniteInSameSweep(
t *testing.T,
) {
t.Parallel()
env := setupRetentionTest(t)
foreverID := createWebhookNormally(t, env.mainDB.DB(), 0)
var stored database.Webhook
require.NoError(
t,
env.mainDB.DB().Where("id = ?", foreverID).
First(&stored).Error,
)
require.Equal(
t,
database.RetentionForeverDays,
stored.RetentionDays,
"a requested retention of 0 must persist as the sentinel",
)
finiteID := createWebhookNormally(t, env.mainDB.DB(), 30)
foreverDB, err := env.mgr.GetDB(foreverID)
require.NoError(t, err)
finiteDB, err := env.mgr.GetDB(finiteID)
require.NoError(t, err)
ancient := time.Now().Add(-365 * 24 * time.Hour)
kept := seedEventChain(t, foreverDB, foreverID, ancient)
doomed := seedEventChain(t, finiteDB, finiteID, ancient)
env.reaper.ExportSweep(context.Background())
assertChainPresent(t, foreverDB, kept)
assertChainGone(t, finiteDB, doomed)
}
func TestRetentionReaper_RetainsForeverWhenNonPositive(t *testing.T) { func TestRetentionReaper_RetainsForeverWhenNonPositive(t *testing.T) {
t.Parallel() t.Parallel()
env := setupRetentionTest(t) env := setupRetentionTest(t)
// A legacy row written before the sentinel existed still carries a // RetentionDays of zero means retain forever.
// literal 0; the <= 0 guard must keep honouring it.
webhookID := createWebhook(t, env.mainDB.DB(), 0) webhookID := createWebhook(t, env.mainDB.DB(), 0)
db, err := env.mgr.GetDB(webhookID) db, err := env.mgr.GetDB(webhookID)

View File

@@ -453,8 +453,9 @@ func (e *Engine) recoverRetryingDeliveries(
// recoverSingleRetry hands an orphaned retrying delivery back // recoverSingleRetry hands an orphaned retrying delivery back
// to its target to recompute the remaining backoff, then // to its target to recompute the remaining backoff, then
// reschedules it. Targets that do not own durable retries // reschedules it. Targets that do not own durable retries
// (fire-and-forget) never produce retrying deliveries, so // (fire-and-forget) never produce retrying deliveries, so a
// they are skipped. // delivery found in that state has had its target's type
// changed underneath it and is terminally failed.
func (e *Engine) recoverSingleRetry( func (e *Engine) recoverSingleRetry(
webhookDB *gorm.DB, webhookDB *gorm.DB,
webhookID string, webhookID string,
@@ -475,6 +476,10 @@ func (e *Engine) recoverSingleRetry(
rs, ok := e.targets[target.Type].(rescheduler) rs, ok := e.targets[target.Type].(rescheduler)
if !ok { if !ok {
e.failUnretryableRetry(
webhookDB, webhookID, d, &target,
)
return return
} }
@@ -649,8 +654,8 @@ func (e *Engine) sweepWebhookRetries(
// sweepSingleRetry re-enqueues an orphaned retrying delivery // sweepSingleRetry re-enqueues an orphaned retrying delivery
// whose backoff window has elapsed, delegating the backoff // whose backoff window has elapsed, delegating the backoff
// decision to the delivery's target. Targets that do not own // decision to the delivery's target. A delivery whose target
// durable retries are skipped. // no longer owns durable retries is terminally failed.
func (e *Engine) sweepSingleRetry( func (e *Engine) sweepSingleRetry(
webhookDB *gorm.DB, webhookDB *gorm.DB,
webhookID string, webhookID string,
@@ -670,6 +675,10 @@ func (e *Engine) sweepSingleRetry(
rs, ok := e.targets[target.Type].(rescheduler) rs, ok := e.targets[target.Type].(rescheduler)
if !ok { if !ok {
e.failUnretryableRetry(
webhookDB, webhookID, d, &target,
)
return return
} }
@@ -710,6 +719,59 @@ func (e *Engine) sweepSingleRetry(
} }
} }
// failUnretryableRetry terminally fails an orphaned retrying
// delivery whose target type no longer supports retries. Both
// restart recovery and the periodic sweep call it, so the
// terminal transition exists once.
//
// This is only reachable when a target's type has been changed
// out from under an in-flight retrying delivery (or the type is
// unknown to the registry): fire-and-forget targets never set
// status retrying themselves. Re-dispatching under the new type
// would be a delivery the operator never asked for, and leaving
// the row retrying strands it forever, so the delivery is
// failed with a recorded reason and can be redelivered
// manually. Logged at warn, not error: this is operator-caused
// state, not a system fault.
func (e *Engine) failUnretryableRetry(
webhookDB *gorm.DB,
webhookID string,
d *database.Delivery,
target *database.Target,
) {
e.log.Warn(
"failing orphaned retrying delivery: target "+
"type no longer supports retries",
"webhook_id", webhookID,
"delivery_id", d.ID,
"target_id", target.ID,
"target_name", target.Name,
"target_type", target.Type,
)
reason := fmt.Sprintf(
"target type %q does not support retries; "+
"delivery was left retrying by a previous "+
"target type and has been failed terminally",
target.Type,
)
e.recordResult(
webhookDB,
d,
e.countAttempts(webhookDB, d.ID)+1,
false,
0,
"",
reason,
0,
)
e.updateDeliveryStatus(
webhookDB, d, database.DeliveryStatusFailed,
)
}
// processDelivery dispatches a delivery to the target that // processDelivery dispatches a delivery to the target that
// owns its type. Unknown target types fail the delivery. // owns its type. Unknown target types fail the delivery.
func (e *Engine) processDelivery( func (e *Engine) processDelivery(

View File

@@ -748,6 +748,193 @@ func TestRecoverWebhookDeliveries_RetryingDeliveries(
case <-time.After(5 * time.Second): case <-time.After(5 * time.Second):
t.Fatal("expected retry task from recovery") t.Fatal("expected retry task from recovery")
} }
// Regression guard: a target that still supports retries
// must be rescheduled, never terminally failed, and must
// not gain a synthetic result row.
iAssertStatus(
t, s.WebhookDB, d.ID,
database.DeliveryStatusRetrying,
)
assert.Len(t, iResults(t, s.WebhookDB, d.ID), 1)
}
// --- Retrying deliveries whose target type changed ---
// iSeedRetryingWithType seeds a retrying delivery with one
// recorded failed attempt against a target of the given type,
// standing in for a target whose type was edited in the main
// database while the delivery was still retrying.
func iSeedRetryingWithType(
t *testing.T,
s iSetup,
targetType database.TargetType,
) string {
t.Helper()
targetID := uuid.New().String()
iCreateTarget(t, s.MainDB, targetID,
s.WebhookID, "mutated-target", targetType,
iHTTPConfig("http://example.com/hook"), 5,
)
event := iSeedEvent(
t, s.WebhookDB, s.WebhookID,
`{"orphaned":"retry"}`,
)
d := iSeedDelivery(
t, s.WebhookDB, event.ID, targetID,
database.DeliveryStatusRetrying,
)
iSeedFailedResult(t, s.WebhookDB, d.ID)
return d.ID
}
// iResults loads a delivery's results in attempt order.
func iResults(
t *testing.T, db *gorm.DB, deliveryID string,
) []database.DeliveryResult {
t.Helper()
var results []database.DeliveryResult
require.NoError(t, db.
Where("delivery_id = ?", deliveryID).
Order("attempt_num").
Find(&results).Error)
return results
}
// iAssertTerminallyFailed asserts the delivery ended failed
// with a result row recording why, and was not rescheduled.
func iAssertTerminallyFailed(
t *testing.T,
s iSetup,
deliveryID string,
targetType database.TargetType,
) {
t.Helper()
iAssertStatus(
t, s.WebhookDB, deliveryID,
database.DeliveryStatusFailed,
)
results := iResults(t, s.WebhookDB, deliveryID)
require.Len(t, results, 2)
last := results[1]
assert.False(t, last.Success)
assert.Equal(t, 2, last.AttemptNum)
assert.Contains(
t, last.Error, string(targetType),
)
assert.Contains(
t, last.Error, "does not support retries",
)
assert.Empty(t, s.Engine.ExportRetryCh())
}
func TestRecoverSingleRetry_TypeNoLongerRetries(
t *testing.T,
) {
t.Parallel()
s := newISetup(t)
iCreateWebhook(
t, s.MainDB, s.WebhookID, "mutated-type",
)
deliveryID := iSeedRetryingWithType(
t, s, database.TargetTypeLog,
)
s.Engine.ExportRecoverWebhookDeliveries(
context.Background(), s.WebhookID,
)
iAssertTerminallyFailed(
t, s, deliveryID, database.TargetTypeLog,
)
}
func TestSweepSingleRetry_TypeNoLongerRetries(
t *testing.T,
) {
t.Parallel()
s := newISetup(t)
iCreateWebhook(
t, s.MainDB, s.WebhookID, "mutated-type-sweep",
)
deliveryID := iSeedRetryingWithType(
t, s, database.TargetTypeDatabase,
)
s.Engine.ExportSweepWebhookRetries(
context.Background(), s.WebhookID,
)
iAssertTerminallyFailed(
t, s, deliveryID, database.TargetTypeDatabase,
)
}
func TestRecoverSingleRetry_UnknownTargetType(
t *testing.T,
) {
t.Parallel()
s := newISetup(t)
iCreateWebhook(
t, s.MainDB, s.WebhookID, "unknown-type",
)
unknown := database.TargetType("not-a-target-type")
deliveryID := iSeedRetryingWithType(t, s, unknown)
s.Engine.ExportRecoverWebhookDeliveries(
context.Background(), s.WebhookID,
)
iAssertTerminallyFailed(t, s, deliveryID, unknown)
}
func TestSweepSingleRetry_UnknownTargetType(
t *testing.T,
) {
t.Parallel()
s := newISetup(t)
iCreateWebhook(
t, s.MainDB, s.WebhookID, "unknown-type-sweep",
)
unknown := database.TargetType("not-a-target-type")
deliveryID := iSeedRetryingWithType(t, s, unknown)
s.Engine.ExportSweepWebhookRetries(
context.Background(), s.WebhookID,
)
iAssertTerminallyFailed(t, s, deliveryID, unknown)
} }
// iSeedFailedResult creates a failed delivery result. // iSeedFailedResult creates a failed delivery result.

View File

@@ -188,6 +188,13 @@ func (e *Engine) ExportRecoverInFlight(
e.recoverInFlight(ctx) e.recoverInFlight(ctx)
} }
// ExportSweepWebhookRetries exposes sweepWebhookRetries.
func (e *Engine) ExportSweepWebhookRetries(
ctx context.Context, webhookID string,
) {
e.sweepWebhookRetries(ctx, webhookID)
}
// ExportStart exposes start for testing. // ExportStart exposes start for testing.
func (e *Engine) ExportStart(ctx context.Context) { func (e *Engine) ExportStart(ctx context.Context) {
e.start(ctx) e.start(ctx)

View File

@@ -26,6 +26,8 @@ const (
maxBodyShift = 20 maxBodyShift = 20
// recentEventLimit is the number of recent events to show. // recentEventLimit is the number of recent events to show.
recentEventLimit = 20 recentEventLimit = 20
// defaultRetentionDays is the default event retention period.
defaultRetentionDays = 30
// paginationPerPage is the number of items per page. // paginationPerPage is the number of items per page.
paginationPerPage = 25 paginationPerPage = 25

View File

@@ -25,37 +25,6 @@ type WebhookListItem struct {
// errMissingURL signals that a required URL was not provided. // errMissingURL signals that a required URL was not provided.
var errMissingURL = errors.New("missing URL") var errMissingURL = errors.New("missing URL")
// errInvalidRetention signals a retention_days form value that is not
// a non-negative whole number.
var errInvalidRetention = errors.New("invalid retention days")
// retentionErrorMessage is what the create and edit forms show the user
// when parseRetentionDays returns errInvalidRetention.
const retentionErrorMessage = "Retention must be a whole number of " +
"days, or 0 to retain events forever."
// parseRetentionDays interprets a retention_days form value.
//
// An empty value yields fallback, which lets the create path apply the
// default and the edit path leave the stored value unchanged. A value
// of 0 is returned as 0 and is rewritten to the retain-forever
// sentinel by database.Webhook's BeforeSave hook, so no handler needs
// to know the sentinel. Anything unparseable or negative is an error
// rather than a silently substituted default.
func parseRetentionDays(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, errInvalidRetention
}
return v, nil
}
// EventWithDeliveries holds an event and its deliveries. // EventWithDeliveries holds an event and its deliveries.
type EventWithDeliveries struct { type EventWithDeliveries struct {
database.Event database.Event
@@ -137,20 +106,11 @@ func (h *Handlers) buildWebhookListItems(
// HandleSourceCreate shows the form to create a new webhook. // HandleSourceCreate shows the form to create a new webhook.
func (h *Handlers) HandleSourceCreate() http.HandlerFunc { func (h *Handlers) HandleSourceCreate() http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) { return func(w http.ResponseWriter, r *http.Request) {
h.renderTemplate( data := map[string]any{
w, r, "sources_new.html", newSourceFormData(""), tmplKeyError: "",
) }
}
}
// newSourceFormData builds the template data for the webhook creation h.renderTemplate(w, r, "sources_new.html", data)
// form, carrying the retention default so the pre-filled value comes
// from database.DefaultRetentionDays rather than being a third
// hardcoded copy of the same policy.
func newSourceFormData(errMsg string) map[string]any {
return map[string]any{
tmplKeyError: errMsg,
"DefaultRetentionDays": database.DefaultRetentionDays,
} }
} }
@@ -185,26 +145,23 @@ func (h *Handlers) HandleSourceCreateSubmit() http.HandlerFunc {
retentionStr := r.FormValue("retention_days") retentionStr := r.FormValue("retention_days")
if name == "" { if name == "" {
data := map[string]any{
tmplKeyError: "Name is required",
}
w.WriteHeader(http.StatusBadRequest) w.WriteHeader(http.StatusBadRequest)
h.renderTemplate( h.renderTemplate(w, r, "sources_new.html", data)
w, r, "sources_new.html",
newSourceFormData("Name is required"),
)
return return
} }
retentionDays, retErr := parseRetentionDays( retentionDays := defaultRetentionDays
retentionStr, database.DefaultRetentionDays,
)
if retErr != nil {
w.WriteHeader(http.StatusBadRequest)
h.renderTemplate(
w, r, "sources_new.html",
newSourceFormData(retentionErrorMessage),
)
return if retentionStr != "" {
v, convErr := strconv.Atoi(retentionStr)
if convErr == nil && v > 0 {
retentionDays = v
}
} }
h.createWebhookWithEntrypoint( h.createWebhookWithEntrypoint(
@@ -471,25 +428,7 @@ func (h *Handlers) applyWebhookEdit(
webhook.Name = name webhook.Name = name
webhook.Description = r.FormValue("description") webhook.Description = r.FormValue("description")
h.parseRetention(r, webhook)
// An empty field falls back to the stored value, so submitting the
// form without touching retention leaves the policy alone.
retentionDays, retErr := parseRetentionDays(
r.FormValue("retention_days"), webhook.RetentionDays,
)
if retErr != nil {
data := map[string]any{
tmplKeyWebhook: *webhook,
tmplKeyError: retentionErrorMessage,
}
w.WriteHeader(http.StatusBadRequest)
h.renderTemplate(w, r, "source_edit.html", data)
return
}
webhook.RetentionDays = retentionDays
err := h.db.DB().Save(webhook).Error err := h.db.DB().Save(webhook).Error
if err != nil { if err != nil {
@@ -503,6 +442,23 @@ func (h *Handlers) applyWebhookEdit(
) )
} }
// parseRetention parses and applies retention_days from the
// form.
func (h *Handlers) parseRetention(
r *http.Request,
webhook *database.Webhook,
) {
retStr := r.FormValue("retention_days")
if retStr == "" {
return
}
v, err := strconv.Atoi(retStr)
if err == nil && v > 0 {
webhook.RetentionDays = v
}
}
// HandleSourceDelete handles webhook deletion. // HandleSourceDelete handles webhook deletion.
func (h *Handlers) HandleSourceDelete() http.HandlerFunc { func (h *Handlers) HandleSourceDelete() http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) { return func(w http.ResponseWriter, r *http.Request) {

View File

@@ -1,481 +0,0 @@
package handlers_test
import (
"context"
"net/http"
"net/http/httptest"
"net/url"
"strconv"
"strings"
"testing"
"github.com/go-chi/chi"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"gorm.io/gorm/clause"
"sneak.berlin/go/webhooker/internal/database"
"sneak.berlin/go/webhooker/internal/handlers"
"sneak.berlin/go/webhooker/internal/session"
)
const (
// sourceTestUserID is the session user id used by the webhook
// management tests.
sourceTestUserID = "source-test-user"
// sourceIDParam is the chi URL parameter naming a webhook.
sourceIDParam = "sourceID"
)
// formRequest builds an urlencoded POST to path carrying the given
// cookies, plus any chi URL parameters the handler reads.
func formRequest(
path string,
cookies []*http.Cookie,
form url.Values,
urlParams map[string]string,
) *http.Request {
req := httptest.NewRequestWithContext(
context.Background(),
http.MethodPost,
path,
strings.NewReader(form.Encode()),
)
req.Header.Set(
"Content-Type", "application/x-www-form-urlencoded",
)
for _, c := range cookies {
req.AddCookie(c)
}
rctx := chi.NewRouteContext()
for k, v := range urlParams {
rctx.URLParams.Add(k, v)
}
return req.WithContext(
context.WithValue(req.Context(), chi.RouteCtxKey, rctx),
)
}
// getRequest builds a GET to path carrying the given cookies, plus any
// chi URL parameters the handler reads.
func getRequest(
t *testing.T,
path string,
cookies []*http.Cookie,
urlParams map[string]string,
) *http.Request {
t.Helper()
req := httptest.NewRequestWithContext(
context.Background(), http.MethodGet, path, nil,
)
for _, c := range cookies {
req.AddCookie(c)
}
rctx := chi.NewRouteContext()
for k, v := range urlParams {
rctx.URLParams.Add(k, v)
}
return req.WithContext(
context.WithValue(req.Context(), chi.RouteCtxKey, rctx),
)
}
// submitCreate posts the webhook creation form with the given
// retention_days value (omitted entirely when retention is nil) and
// returns the recorder.
func submitCreate(
t *testing.T,
h *handlers.Handlers,
cookies []*http.Cookie,
name string,
retention *string,
) *httptest.ResponseRecorder {
t.Helper()
form := url.Values{}
form.Set("name", name)
if retention != nil {
form.Set("retention_days", *retention)
}
req := formRequest("/sources/new", cookies, form, nil)
w := httptest.NewRecorder()
h.HandleSourceCreateSubmit().ServeHTTP(w, req)
return w
}
// onlyWebhook loads the single webhook belonging to the test user.
func onlyWebhook(
t *testing.T,
db *database.Database,
) database.Webhook {
t.Helper()
var webhooks []database.Webhook
require.NoError(
t,
db.DB().Where("user_id = ?", sourceTestUserID).
Find(&webhooks).Error,
)
require.Len(t, webhooks, 1)
return webhooks[0]
}
// seedWebhook inserts a webhook owned by the test user with an exact
// stored retention value, bypassing Webhook.BeforeSave via a
// column-level update so that legacy rows can be planted too.
func seedWebhook(
t *testing.T,
db *database.Database,
retentionDays int,
) database.Webhook {
t.Helper()
wh := &database.Webhook{
UserID: sourceTestUserID,
Name: "seeded",
RetentionDays: retentionDays,
}
require.NoError(
t,
db.DB().Omit(clause.Associations).Create(wh).Error,
)
require.NoError(
t,
db.DB().Model(wh).
Update("retention_days", retentionDays).Error,
)
wh.RetentionDays = retentionDays
return *wh
}
// storedRetentionDays reads the retention_days column for a webhook.
func storedRetentionDays(
t *testing.T,
db *database.Database,
id string,
) int {
t.Helper()
var got int
require.NoError(
t,
db.DB().Model(&database.Webhook{}).
Where("id = ?", id).
Pluck("retention_days", &got).Error,
)
return got
}
// sourceTestEnv bundles the handler, session, and database a webhook
// management test drives.
type sourceTestEnv struct {
handlers *handlers.Handlers
db *database.Database
cookies []*http.Cookie
}
func setupSourceTest(t *testing.T) *sourceTestEnv {
t.Helper()
var h *handlers.Handlers
var sess *session.Session
var db *database.Database
app := newTestApp(t, &h, &sess, &db)
app.RequireStart()
t.Cleanup(app.RequireStop)
return &sourceTestEnv{
handlers: h,
db: db,
cookies: authenticatedCookies(
t, sess, sourceTestUserID, "sourceuser",
),
}
}
// TestHandleSourceCreateSubmit_ZeroRetentionPersistsForever is the core
// regression test for the bug: the create form's 0 must reach the
// database as the retain-forever sentinel rather than being replaced by
// the column's default of 30.
func TestHandleSourceCreateSubmit_ZeroRetentionPersistsForever(
t *testing.T,
) {
t.Parallel()
env := setupSourceTest(t)
zero := "0"
w := submitCreate(t, env.handlers, env.cookies, "forever", &zero)
require.Equal(t, http.StatusSeeOther, w.Code)
wh := onlyWebhook(t, env.db)
assert.Equal(
t,
database.RetentionForeverDays,
storedRetentionDays(t, env.db, wh.ID),
)
assert.True(t, wh.RetainsForever())
}
func TestHandleSourceCreateSubmit_OmittedRetentionUsesDefault(
t *testing.T,
) {
t.Parallel()
env := setupSourceTest(t)
w := submitCreate(t, env.handlers, env.cookies, "defaulted", nil)
require.Equal(t, http.StatusSeeOther, w.Code)
wh := onlyWebhook(t, env.db)
assert.Equal(
t,
database.DefaultRetentionDays,
storedRetentionDays(t, env.db, wh.ID),
)
}
// TestHandleSourceCreate_PrefillsDefaultFromConstant keeps the create
// form's pre-filled retention from becoming a third hardcoded copy of
// the 30-day policy.
func TestHandleSourceCreate_PrefillsDefaultFromConstant(t *testing.T) {
t.Parallel()
env := setupSourceTest(t)
w := httptest.NewRecorder()
env.handlers.HandleSourceCreate().ServeHTTP(
w, getRequest(t, "/sources/new", env.cookies, nil),
)
require.Equal(t, http.StatusOK, w.Code)
body := w.Body.String()
assert.Contains(
t, body,
`value="`+strconv.Itoa(database.DefaultRetentionDays)+`"`,
)
assert.NotContains(
t, body, `max="365"`,
"a max below the sentinel would block retain-forever",
)
assert.Contains(t, body, `min="0"`)
}
func TestHandleSourceCreateSubmit_InvalidRetentionIsRejected(
t *testing.T,
) {
t.Parallel()
for _, raw := range []string{"abc", "-1", "3.5"} {
t.Run(raw, func(t *testing.T) {
t.Parallel()
env := setupSourceTest(t)
w := submitCreate(
t, env.handlers, env.cookies, "bad", &raw,
)
assert.Equal(t, http.StatusBadRequest, w.Code)
assert.Contains(
t, w.Body.String(), "Retention must be",
)
var count int64
require.NoError(
t,
env.db.DB().Model(&database.Webhook{}).
Where("user_id = ?", sourceTestUserID).
Count(&count).Error,
)
assert.Zero(
t, count,
"no webhook may be created from a rejected form",
)
})
}
}
// submitEdit posts the webhook edit form for the given webhook.
func submitEdit(
t *testing.T,
env *sourceTestEnv,
wh database.Webhook,
retention string,
) *httptest.ResponseRecorder {
t.Helper()
form := url.Values{}
form.Set("name", wh.Name)
form.Set("description", wh.Description)
form.Set("retention_days", retention)
req := formRequest(
"/source/"+wh.ID+"/edit",
env.cookies,
form,
map[string]string{sourceIDParam: wh.ID},
)
w := httptest.NewRecorder()
env.handlers.HandleSourceEditSubmit().ServeHTTP(w, req)
return w
}
func TestHandleSourceEditSubmit_ZeroRetentionPersistsForever(
t *testing.T,
) {
t.Parallel()
env := setupSourceTest(t)
wh := seedWebhook(t, env.db, database.DefaultRetentionDays)
w := submitEdit(t, env, wh, "0")
require.Equal(t, http.StatusSeeOther, w.Code)
assert.Equal(
t,
database.RetentionForeverDays,
storedRetentionDays(t, env.db, wh.ID),
)
}
func TestHandleSourceEditSubmit_InvalidRetentionIsRejected(
t *testing.T,
) {
t.Parallel()
env := setupSourceTest(t)
wh := seedWebhook(t, env.db, database.DefaultRetentionDays)
w := submitEdit(t, env, wh, "not-a-number")
assert.Equal(t, http.StatusBadRequest, w.Code)
assert.Contains(t, w.Body.String(), "Retention must be")
assert.Equal(
t,
database.DefaultRetentionDays,
storedRetentionDays(t, env.db, wh.ID),
"a rejected form must not change the stored retention",
)
}
func TestHandleSourceEditSubmit_EmptyRetentionLeavesValueUnchanged(
t *testing.T,
) {
t.Parallel()
env := setupSourceTest(t)
wh := seedWebhook(t, env.db, 7)
w := submitEdit(t, env, wh, "")
require.Equal(t, http.StatusSeeOther, w.Code)
assert.Equal(t, 7, storedRetentionDays(t, env.db, wh.ID))
}
// TestSourceEditForm_ForeverWebhookRoundTrips walks the exact path that
// the removed max="365" cap used to break: render the edit form for a
// retain-forever webhook, confirm the pre-filled sentinel is not capped
// by browser validation, then submit that pre-filled value straight
// back and confirm the retention policy survives untouched.
func TestSourceEditForm_ForeverWebhookRoundTrips(t *testing.T) {
t.Parallel()
env := setupSourceTest(t)
wh := seedWebhook(t, env.db, database.RetentionForeverDays)
req := getRequest(
t, "/source/"+wh.ID+"/edit", env.cookies,
map[string]string{sourceIDParam: wh.ID},
)
w := httptest.NewRecorder()
env.handlers.HandleSourceEdit().ServeHTTP(w, req)
require.Equal(t, http.StatusOK, w.Code)
sentinel := strconv.Itoa(database.RetentionForeverDays)
body := w.Body.String()
assert.Contains(
t, body, `value="`+sentinel+`"`,
"the edit form pre-fills the stored retention",
)
assert.NotContains(
t, body, `max="365"`,
"a max below the sentinel would block saving any edit",
)
assert.Contains(
t, body, "forever",
"the form explains what the sentinel means",
)
// Submit the pre-filled value back, exactly as a browser would.
post := submitEdit(t, env, wh, sentinel)
require.Equal(t, http.StatusSeeOther, post.Code)
assert.Equal(
t,
database.RetentionForeverDays,
storedRetentionDays(t, env.db, wh.ID),
)
}
// TestSourceListAndDetail_ShowForeverNotTheSentinelNumber checks that
// the retain-forever value is never rendered to the user as a raw day
// count on either read-only view.
func TestSourceListAndDetail_ShowForeverNotTheSentinelNumber(
t *testing.T,
) {
t.Parallel()
env := setupSourceTest(t)
wh := seedWebhook(t, env.db, database.RetentionForeverDays)
sentinel := strconv.Itoa(database.RetentionForeverDays)
listW := httptest.NewRecorder()
env.handlers.HandleSourceList().ServeHTTP(
listW, getRequest(t, "/sources", env.cookies, nil),
)
require.Equal(t, http.StatusOK, listW.Code)
assert.Contains(t, listW.Body.String(), "Retention: forever")
assert.NotContains(t, listW.Body.String(), sentinel)
detailW := httptest.NewRecorder()
env.handlers.HandleSourceDetail().ServeHTTP(
detailW,
getRequest(
t, "/source/"+wh.ID, env.cookies,
map[string]string{sourceIDParam: wh.ID},
),
)
require.Equal(t, http.StatusOK, detailW.Code)
assert.Contains(t, detailW.Body.String(), "Retention: forever")
assert.NotContains(t, detailW.Body.String(), sentinel)
}

View File

@@ -181,7 +181,7 @@
<!-- Info --> <!-- Info -->
<div class="mt-4 text-sm text-gray-400"> <div class="mt-4 text-sm text-gray-400">
<p>Retention: {{.Webhook.RetentionLabel}} &middot; Created: {{.Webhook.CreatedAt.Format "2006-01-02 15:04:05 UTC"}}</p> <p>Retention: {{.Webhook.RetentionDays}} days &middot; Created: {{.Webhook.CreatedAt.Format "2006-01-02 15:04:05 UTC"}}</p>
</div> </div>
</div> </div>
{{end}} {{end}}

View File

@@ -28,8 +28,7 @@
<div class="form-group"> <div class="form-group">
<label for="retention_days" class="label">Retention (days)</label> <label for="retention_days" class="label">Retention (days)</label>
<input type="number" id="retention_days" name="retention_days" value="{{.Webhook.RetentionDays}}" min="0" class="input"> <input type="number" id="retention_days" name="retention_days" value="{{.Webhook.RetentionDays}}" min="1" max="365" class="input">
<p class="text-xs text-gray-500 mt-1">Currently {{.Webhook.RetentionLabel}}. Enter 0 to retain events forever.</p>
</div> </div>
<div class="flex gap-3"> <div class="flex gap-3">

View File

@@ -25,7 +25,7 @@
<p class="text-sm text-gray-500 mt-1">{{.Description}}</p> <p class="text-sm text-gray-500 mt-1">{{.Description}}</p>
{{end}} {{end}}
</div> </div>
<span class="badge-info">Retention: {{.RetentionLabel}}</span> <span class="badge-info">{{.RetentionDays}}d retention</span>
</div> </div>
<div class="flex gap-6 mt-4 text-sm text-gray-500"> <div class="flex gap-6 mt-4 text-sm text-gray-500">
<span>{{.EntrypointCount}} entrypoint{{if ne .EntrypointCount 1}}s{{end}}</span> <span>{{.EntrypointCount}} entrypoint{{if ne .EntrypointCount 1}}s{{end}}</span>

View File

@@ -28,8 +28,8 @@
<div class="form-group"> <div class="form-group">
<label for="retention_days" class="label">Retention (days)</label> <label for="retention_days" class="label">Retention (days)</label>
<input type="number" id="retention_days" name="retention_days" value="{{.DefaultRetentionDays}}" min="0" class="input"> <input type="number" id="retention_days" name="retention_days" value="30" min="1" max="365" class="input">
<p class="text-xs text-gray-500 mt-1">How long to keep event data. Enter 0 to retain events forever.</p> <p class="text-xs text-gray-500 mt-1">How long to keep event data.</p>
</div> </div>
<div class="flex gap-3"> <div class="flex gap-3">