Add per-delivery replay to the event log (closes #203)

A delivery that exhausted max_retries was failed forever. The event body is durably stored, so the only way to get it delivered was to download it and re-POST by hand. The event log now offers a Replay action on any finished delivery. Replay creates a NEW pending delivery for the same event and target and hands it to the delivery engine through the same Notifier the receiver uses, so it is retried, SSRF-guarded and circuit-broken exactly as a first attempt. The original delivery's status, timestamps and recorded attempts are never touched, and what is re-sent is the stored event body, not the response the original attempt received. The target is read as it stands now, including soft-deleted rows so that a deleted target refuses the replay with a message on the page instead of erroring or delivering from stale configuration. A deactivated target and a target id that names nothing refuse the same way, as does a replay of a delivery the engine has not finished. Two bounds on replay storms: the route carries a per-client POST rate limit of 30 per minute, and the handler refuses a replay while an earlier one for the same event and target is still pending or retrying. One new metric, webhooker_delivery_replays_total, on the existing target_type label. A replay is a real delivery and moves the attempt, outcome and duration series like any other; this counter is what separates it from ordinary traffic without adding a dimension to every existing series. The delivery row is written with associations omitted and with neither Event nor Target populated, so no target row reaches the per-webhook event database.
2026-08-20 05:45:04 +00:00
parent aba02bc509
commit c1fce4326c
12 changed files with 1240 additions and 14 deletions
--- a/internal/handlers/delivery_replay_test.go
+++ b/internal/handlers/delivery_replay_test.go
@@ -0,0 +1,526 @@
+package handlers_test
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"gorm.io/gorm"
+	"gorm.io/gorm/clause"
+	"sneak.berlin/go/webhooker/internal/database"
+	"sneak.berlin/go/webhooker/internal/delivery"
+	"sneak.berlin/go/webhooker/internal/handlers"
+	"sneak.berlin/go/webhooker/internal/session"
+)
+
+// paramDeliveryID is the chi URL parameter name the replay handler
+// reads.
+const paramDeliveryID = "deliveryID"
+
+// replayTargetURL is a public destination, so a target configured with
+// it is one the SSRF guard would accept. Nothing in these tests
+// dispatches to it: the notifier is recorded, not run.
+const replayTargetURL = "http://93.184.216.34/hook"
+
+// seedFailedDelivery records an event, a terminally failed delivery of
+// it to the given target, and the attempt that failed.
+func seedFailedDelivery(
+	t *testing.T,
+	dbMgr *database.WebhookDBManager,
+	webhookID, targetID string,
+) (*database.Event, *database.Delivery) {
+	t.Helper()
+
+	webhookDB, err := dbMgr.GetDB(webhookID)
+	require.NoError(t, err)
+
+	event := &database.Event{
+		WebhookID:    webhookID,
+		EntrypointID: "entrypoint-" + webhookID,
+		Method:       http.MethodPost,
+		Headers:      `{"X-Test":["yes"]}`,
+		Body:         `{"replay":"me"}`,
+		ContentType:  "application/json",
+	}
+
+	require.NoError(t, webhookDB.Omit(
+		clause.Associations,
+	).Create(event).Error)
+
+	dlv := &database.Delivery{
+		EventID:  event.ID,
+		TargetID: targetID,
+		Status:   database.DeliveryStatusFailed,
+	}
+
+	require.NoError(t, webhookDB.Omit(
+		clause.Associations,
+	).Create(dlv).Error)
+
+	result := &database.DeliveryResult{
+		DeliveryID: dlv.ID,
+		AttemptNum: 1,
+		Success:    false,
+		StatusCode: http.StatusBadGateway,
+		Error:      "connection refused",
+	}
+
+	require.NoError(t, webhookDB.Omit(
+		clause.Associations,
+	).Create(result).Error)
+
+	return event, dlv
+}
+
+// loadDelivery reads a delivery back out of a webhook's database.
+func loadDelivery(
+	t *testing.T, webhookDB *gorm.DB, deliveryID string,
+) database.Delivery {
+	t.Helper()
+
+	var dlv database.Delivery
+
+	require.NoError(
+		t,
+		webhookDB.First(&dlv, "id = ?", deliveryID).Error,
+	)
+
+	return dlv
+}
+
+// listDeliveries reads every delivery of an event.
+func listDeliveries(
+	t *testing.T, webhookDB *gorm.DB, eventID string,
+) []database.Delivery {
+	t.Helper()
+
+	var deliveries []database.Delivery
+
+	require.NoError(t, webhookDB.Where(
+		"event_id = ?", eventID,
+	).Find(&deliveries).Error)
+
+	return deliveries
+}
+
+// theOtherDelivery returns the one delivery in the slice that is not
+// excludeID. Identity is used rather than an ordering because the rows
+// are minted milliseconds apart and their ids are random.
+func theOtherDelivery(
+	t *testing.T,
+	deliveries []database.Delivery,
+	excludeID string,
+) database.Delivery {
+	t.Helper()
+
+	var found []database.Delivery
+
+	for _, d := range deliveries {
+		if d.ID != excludeID {
+			found = append(found, d)
+		}
+	}
+
+	require.Len(t, found, 1)
+
+	return found[0]
+}
+
+// postReplay runs the real replay handler for one delivery.
+func postReplay(
+	t *testing.T,
+	h *handlers.Handlers,
+	sess *session.Session,
+	webhookID, deliveryID string,
+) *httptest.ResponseRecorder {
+	t.Helper()
+
+	req := postRequest(
+		"/source/"+webhookID+"/deliveries/"+
+			deliveryID+"/replay",
+		authenticatedCookies(
+			t, sess, deleteTestUserID, deleteTestUsername,
+		),
+		map[string]string{
+			paramSourceID:   webhookID,
+			paramDeliveryID: deliveryID,
+		},
+	)
+
+	w := httptest.NewRecorder()
+	h.HandleDeliveryReplay().ServeHTTP(w, req)
+
+	return w
+}
+
+// TestHandleDeliveryReplay_AppendsDeliveryAndLeavesOriginal is the
+// core requirement: replaying a failed delivery succeeds, appends a
+// new delivery, and leaves the original row and its recorded attempt
+// exactly as they were.
+//
+// It also pins the two things a replay would be wrong to get from the
+// original: the task carries the target's CURRENT configuration, which
+// this test changes between the failure and the replay, and it carries
+// the stored EVENT body rather than anything the failed attempt
+// received back.
+func TestHandleDeliveryReplay_AppendsDeliveryAndLeavesOriginal(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	var (
+		h     *handlers.Handlers
+		sess  *session.Session
+		db    *database.Database
+		dbMgr *database.WebhookDBManager
+		notif *recordingNotifier
+	)
+
+	app := newTestApp(t, &h, &sess, &db, &dbMgr, &notif)
+	app.RequireStart()
+
+	t.Cleanup(app.RequireStop)
+
+	wh := seedWebhook(t, db)
+	tgt := seedConfiguredTarget(
+		t, db, wh.ID, database.TargetTypeHTTP,
+		`{"url":"`+replayTargetURL+`"}`,
+	)
+
+	event, original := seedFailedDelivery(
+		t, dbMgr, wh.ID, tgt.ID,
+	)
+
+	webhookDB, err := dbMgr.GetDB(wh.ID)
+	require.NoError(t, err)
+
+	before := loadDelivery(t, webhookDB, original.ID)
+
+	// The operator fixes the destination, which is the whole reason
+	// to replay. The replay must use this, not the config the
+	// original delivery ran against.
+	const fixedConfig = `{"url":"http://93.184.216.34/fixed"}`
+
+	require.NoError(t, db.DB().Model(&database.Target{}).
+		Where("id = ?", tgt.ID).
+		Update("config", fixedConfig).Error)
+
+	w := postReplay(t, h, sess, wh.ID, original.ID)
+
+	require.Equal(t, http.StatusSeeOther, w.Code)
+	assert.Equal(
+		t,
+		"/source/"+wh.ID+"/logs?replay=queued",
+		w.Header().Get("Location"),
+	)
+
+	deliveries := listDeliveries(t, webhookDB, event.ID)
+	require.Len(
+		t, deliveries, 2,
+		"replay must append a delivery, not reuse one",
+	)
+
+	replayed := theOtherDelivery(t, deliveries, original.ID)
+	assert.Equal(t, tgt.ID, replayed.TargetID)
+	assert.Equal(t, event.ID, replayed.EventID)
+	assert.Equal(
+		t, database.DeliveryStatusPending, replayed.Status,
+	)
+
+	assertDeliveryUntouched(t, webhookDB, before)
+
+	tasks := notif.Tasks()
+	require.Len(t, tasks, 1)
+	assertReplayTask(
+		t, tasks[0], wh.ID, event, tgt, replayed.ID, fixedConfig,
+	)
+	assertNoLeakedTarget(t, webhookDB)
+}
+
+// assertDeliveryUntouched proves a delivery row is exactly as it was
+// read before: same terminal status, same timestamps, and the same
+// recorded attempts.
+func assertDeliveryUntouched(
+	t *testing.T,
+	webhookDB *gorm.DB,
+	before database.Delivery,
+) {
+	t.Helper()
+
+	after := loadDelivery(t, webhookDB, before.ID)
+	assert.Equal(
+		t, before.Status, after.Status,
+		"replay must not resurrect the original delivery",
+	)
+	assert.Equal(t, before.UpdatedAt, after.UpdatedAt)
+	assert.Equal(t, before.CreatedAt, after.CreatedAt)
+
+	var attempts int64
+
+	require.NoError(t, webhookDB.
+		Model(&database.DeliveryResult{}).
+		Where("delivery_id = ?", before.ID).
+		Count(&attempts).Error)
+	assert.Equal(
+		t, int64(1), attempts,
+		"the original delivery's attempt history must stand",
+	)
+}
+
+// assertReplayTask proves the task handed to the delivery engine is
+// the one the receiver would build for this event and this target, and
+// that it carries wantConfig — the target's configuration as it stands
+// now rather than as the original delivery ran against it.
+func assertReplayTask(
+	t *testing.T,
+	task delivery.Task,
+	webhookID string,
+	event *database.Event,
+	target *database.Target,
+	wantDeliveryID, wantConfig string,
+) {
+	t.Helper()
+
+	assert.Equal(t, wantDeliveryID, task.DeliveryID)
+	assert.Equal(t, event.ID, task.EventID)
+	assert.Equal(t, webhookID, task.WebhookID)
+	assert.Equal(t, event.EntrypointID, task.EntrypointID)
+	assert.Equal(t, target.ID, task.TargetID)
+	assert.Equal(t, target.Type, task.TargetType)
+	assert.JSONEq(
+		t, wantConfig, task.TargetConfig,
+		"replay must use the target's current configuration",
+	)
+	assert.Equal(t, event.Method, task.Method)
+	assert.Equal(t, event.Headers, task.Headers)
+	assert.Equal(t, event.ContentType, task.ContentType)
+	assert.Equal(t, 1, task.AttemptNum)
+	require.NotNil(t, task.Body)
+	assert.Equal(
+		t, event.Body, *task.Body,
+		"replay re-sends the stored event body",
+	)
+}
+
+// assertNoLeakedTarget proves the per-webhook database holds no target
+// rows. AutoMigrate creates the table there because Delivery declares
+// the relation, so it is a ROW that signals a leak: an association
+// write would have upserted the whole target, plaintext config and
+// all, into the event database. See
+// https://git.eeqj.de/sneak/webhooker/issues/206.
+func assertNoLeakedTarget(t *testing.T, webhookDB *gorm.DB) {
+	t.Helper()
+
+	var leaked int64
+
+	require.NoError(t, webhookDB.Unscoped().
+		Model(&database.Target{}).Count(&leaked).Error)
+	assert.Zero(
+		t, leaked,
+		"replay must not write the target into the event database",
+	)
+}
+
+// TestHandleDeliveryReplay_RefusesDeletedTarget proves the required
+// refusal: a target deleted since the delivery ran is reported as
+// deleted rather than erroring, and nothing is created or queued.
+func TestHandleDeliveryReplay_RefusesDeletedTarget(t *testing.T) {
+	t.Parallel()
+
+	var (
+		h     *handlers.Handlers
+		sess  *session.Session
+		db    *database.Database
+		dbMgr *database.WebhookDBManager
+		notif *recordingNotifier
+	)
+
+	app := newTestApp(t, &h, &sess, &db, &dbMgr, &notif)
+	app.RequireStart()
+
+	t.Cleanup(app.RequireStop)
+
+	wh := seedWebhook(t, db)
+	tgt := seedConfiguredTarget(
+		t, db, wh.ID, database.TargetTypeHTTP,
+		`{"url":"`+replayTargetURL+`"}`,
+	)
+
+	event, original := seedFailedDelivery(
+		t, dbMgr, wh.ID, tgt.ID,
+	)
+
+	// Deletes are soft, so the delivery history outlives the target.
+	require.NoError(t, db.DB().Where(
+		"id = ?", tgt.ID,
+	).Delete(&database.Target{}).Error)
+
+	w := postReplay(t, h, sess, wh.ID, original.ID)
+
+	require.Equal(t, http.StatusSeeOther, w.Code)
+	assert.Equal(
+		t,
+		"/source/"+wh.ID+"/logs?replay=target-deleted",
+		w.Header().Get("Location"),
+	)
+
+	webhookDB, err := dbMgr.GetDB(wh.ID)
+	require.NoError(t, err)
+
+	assert.Len(
+		t, listDeliveries(t, webhookDB, event.ID), 1,
+		"a refused replay must create no delivery",
+	)
+	assert.Empty(
+		t, notif.Tasks(),
+		"a refused replay must queue nothing",
+	)
+
+	// The refusal is specific, which is why the target is looked up
+	// including soft-deleted rows: an id that never named a target
+	// is a different outcome, and a different message, from one the
+	// operator deleted.
+	_, orphan := seedFailedDelivery(
+		t, dbMgr, wh.ID, "target-that-never-existed",
+	)
+
+	missing := postReplay(t, h, sess, wh.ID, orphan.ID)
+	require.Equal(t, http.StatusSeeOther, missing.Code)
+	assert.Equal(
+		t,
+		"/source/"+wh.ID+"/logs?replay=target-missing",
+		missing.Header().Get("Location"),
+	)
+}
+
+// TestHandleDeliveryReplay_RefusesWhileEarlierReplayInFlight proves
+// the replay-storm guard: a second replay of the same event to the
+// same target is refused while the first is still queued, so repeated
+// submissions cannot stack copies of work the engine has not done.
+func TestHandleDeliveryReplay_RefusesWhileEarlierReplayInFlight(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	var (
+		h     *handlers.Handlers
+		sess  *session.Session
+		db    *database.Database
+		dbMgr *database.WebhookDBManager
+		notif *recordingNotifier
+	)
+
+	app := newTestApp(t, &h, &sess, &db, &dbMgr, &notif)
+	app.RequireStart()
+
+	t.Cleanup(app.RequireStop)
+
+	wh := seedWebhook(t, db)
+	tgt := seedConfiguredTarget(
+		t, db, wh.ID, database.TargetTypeHTTP,
+		`{"url":"`+replayTargetURL+`"}`,
+	)
+
+	event, original := seedFailedDelivery(
+		t, dbMgr, wh.ID, tgt.ID,
+	)
+
+	first := postReplay(t, h, sess, wh.ID, original.ID)
+	require.Equal(t, http.StatusSeeOther, first.Code)
+	require.Equal(
+		t,
+		"/source/"+wh.ID+"/logs?replay=queued",
+		first.Header().Get("Location"),
+	)
+
+	second := postReplay(t, h, sess, wh.ID, original.ID)
+	require.Equal(t, http.StatusSeeOther, second.Code)
+	assert.Equal(
+		t,
+		"/source/"+wh.ID+"/logs?replay=in-flight",
+		second.Header().Get("Location"),
+	)
+
+	webhookDB, err := dbMgr.GetDB(wh.ID)
+	require.NoError(t, err)
+
+	assert.Len(
+		t, listDeliveries(t, webhookDB, event.ID), 2,
+		"the refused second replay must add nothing",
+	)
+	assert.Len(
+		t, notif.Tasks(), 1,
+		"only the first replay reaches the delivery engine",
+	)
+
+	// A delivery the engine has not finished is not replayable
+	// either, which is the same rule seen from the other side.
+	queued := theOtherDelivery(
+		t, listDeliveries(t, webhookDB, event.ID), original.ID,
+	)
+
+	pending := postReplay(t, h, sess, wh.ID, queued.ID)
+	require.Equal(t, http.StatusSeeOther, pending.Code)
+	assert.Equal(
+		t,
+		"/source/"+wh.ID+"/logs?replay=not-terminal",
+		pending.Header().Get("Location"),
+	)
+}
+
+// TestHandleSourceLogs_RendersReplayControlAndBanner proves the action
+// reaches the page it belongs on: a finished delivery renders a POST
+// form carrying a CSRF token, and the outcome code a refusal redirects
+// with becomes a readable message.
+func TestHandleSourceLogs_RendersReplayControlAndBanner(t *testing.T) {
+	t.Parallel()
+
+	var (
+		h     *handlers.Handlers
+		sess  *session.Session
+		db    *database.Database
+		dbMgr *database.WebhookDBManager
+	)
+
+	app := newTestApp(t, &h, &sess, &db, &dbMgr)
+	app.RequireStart()
+
+	t.Cleanup(app.RequireStop)
+
+	wh := seedWebhook(t, db)
+	tgt := seedConfiguredTarget(
+		t, db, wh.ID, database.TargetTypeHTTP,
+		`{"url":"`+replayTargetURL+`"}`,
+	)
+
+	_, original := seedFailedDelivery(t, dbMgr, wh.ID, tgt.ID)
+
+	body := renderSourceLogsPage(t, h, sess, wh.ID)
+
+	assert.Contains(
+		t, body,
+		`action="/source/`+wh.ID+`/deliveries/`+
+			original.ID+`/replay"`,
+	)
+	assert.Contains(t, body, `method="POST"`)
+	assert.Contains(t, body, `name="csrf_token"`)
+	assert.Contains(t, body, ">Replay<")
+
+	refused := renderSourceLogsPageWithQuery(
+		t, h, sess, wh.ID, "?replay=target-deleted",
+	)
+
+	assert.Contains(t, refused, "alert-error")
+	assert.Contains(t, refused, "has been deleted")
+
+	// An outcome code nobody issued renders no banner at all.
+	unknown := renderSourceLogsPageWithQuery(
+		t, h, sess, wh.ID, "?replay=made-up",
+	)
+
+	assert.NotContains(t, unknown, "alert-error")
+	assert.NotContains(t, unknown, "alert-success")
+	assert.NotContains(t, unknown, "made-up")
+}