Store events and create deliveries in the delivery engine (closes #544)

The receiver, resubmit and replay handlers now parse the request and make one call. Engine.StoreEvent stores an event with a pending delivery for each active target and the totals in one transaction, then queues them; Engine.CreateReplay does the same for one replay delivery. The handlers take *delivery.Engine, so the Notifier interface and its bridge are gone. Archives and CircuitBreakers stay because the handler tests fake them. The tests that read a queued task moved to the engine's tests; the handler tests use an engine that is never started. Model: opus-5-5
2026-10-15 21:32:25 +00:00
parent 8406c77949
commit 60e787d7a5
15 changed files with 815 additions and 702 deletions
@@ -1,5 +1,5 @@
-// Package delivery manages asynchronous event delivery
-// to configured targets.
+// Package delivery stores each event with its deliveries and delivers
+// them, asynchronously, to configured targets.
 package delivery

 import (
@@ -23,10 +23,10 @@ import (

 const (
 	// deliveryChannelSize is the buffer size for the delivery
-	// channel. New Tasks from the webhook handler are sent
+	// channel. The Tasks of newly stored deliveries are sent
 	// here. Workers drain this channel. Sized large enough
-	// that the webhook handler should never block under
-	// normal load.
+	// that storing an event should never block under normal
+	// load.
 	deliveryChannelSize = 10000

 	// retryChannelSize is the buffer size for the retry
@@ -127,17 +127,11 @@ type Task struct {
 	AttemptNum int
 }

-// Notifier is the interface for notifying the delivery
-// engine about new deliveries.
-type Notifier interface {
-	Notify(tasks []Task)
-}
-
 // CircuitBreakers is how the handlers read a target's circuit
 // breaker, so the webhook page and the event log can say that
-// deliveries to the target are paused and until when. It keeps the
-// handlers free of the engine's internals and is trivially faked in
-// tests.
+// deliveries to the target are paused and until when. The handlers
+// hold the engine itself as well; they read breakers through this
+// interface so that a test can make one read as open.
 type CircuitBreakers interface {
 	StateAndCooldown(targetID string) (CircuitState, time.Duration)
 }
@@ -232,8 +226,8 @@ func New(
 	return e
 }

-// Notify signals the delivery engine that new deliveries
-// are ready.
+// Notify queues the tasks of deliveries already stored. StoreEvent and
+// CreateReplay call it once their transaction has committed.
 func (e *Engine) Notify(tasks []Task) {
 	for i := range tasks {
 		// Owned before it is queued, and until the worker that runs
@@ -472,9 +466,9 @@ func (e *Engine) processNewTask(
 	}

 	// The recovery routine's run at start can send and release this
-	// delivery before the receiver's Notify queues it. Ownership cannot
-	// refuse a delivery nobody holds, so the row decides whether it
-	// still needs sending.
+	// delivery before StoreEvent or CreateReplay queues it. Ownership
+	// cannot refuse a delivery nobody holds, so the row decides whether
+	// it still needs sending.
 	row, err := e.loadDelivery(webhookDB, task.DeliveryID)
 	if err != nil {
 		e.log.Error(
@@ -899,7 +893,7 @@ func (e *Engine) recoverSingleRetry(
 		return
 	}

-	e.ScheduleRetry(buildRecoveryTask(
+	e.ScheduleRetry(buildTask(
 		d, webhookID, &event, &target, attemptNum+1,
 	), remaining)

@@ -1822,7 +1816,11 @@ func (e *Engine) loadTarget(
 	return target, nil
 }

-func buildRecoveryTask(
+// buildTask builds the task that carries delivery d of event to target
+// through the engine's channels, as attempt attemptNum. The body is
+// carried only when it is under MaxInlineBodySize; a larger one is read
+// from the event row when the attempt runs (see hydrateEvent).
+func buildTask(
 	d *database.Delivery,
 	webhookID string,
 	event *database.Event,
@@ -1998,7 +1996,7 @@ func (e *Engine) sendRecoveredDeliveries(
 			continue
 		}

-		task := buildRecoveryTask(
+		task := buildTask(
 			&deliveries[i], webhookID, &event, &target,
 			attempts[deliveries[i].ID]+1,
 		)
@@ -0,0 +1,164 @@
+package delivery
+
+import (
+	"fmt"
+
+	"gorm.io/gorm"
+	"sneak.berlin/go/webhooker/internal/database"
+)
+
+// StoreEvent stores event with one pending delivery for each of its
+// webhook's active targets, and adds them to the totals, in one
+// transaction, then queues the deliveries. It returns how many
+// deliveries it created.
+//
+// The receiver stores every event it accepts through here, and a
+// resubmit stores its copy of a stored event, so every event is
+// stored, retried, SSRF-guarded and circuit-broken the same way however
+// it entered the system. CreateReplay is the only other place a
+// delivery is created.
+//
+// The targets are the webhook's active ones as they stand now: a
+// target created after a resubmitted event first arrived is included,
+// and an inactive one is skipped. An event with no active target is
+// stored with no deliveries.
+func (e *Engine) StoreEvent(event *database.Event) (int, error) {
+	var targets []database.Target
+
+	err := e.database.DB().Where(
+		"webhook_id = ? AND active = ?", event.WebhookID, true,
+	).Find(&targets).Error
+	if err != nil {
+		return 0, fmt.Errorf("loading active targets: %w", err)
+	}
+
+	webhookDB, err := e.dbManager.GetDB(event.WebhookID)
+	if err != nil {
+		return 0, fmt.Errorf("getting webhook database: %w", err)
+	}
+
+	event.BodyBytes = int64(len(event.Body))
+	tasks := make([]Task, 0, len(targets))
+
+	err = webhookDB.Transaction(func(tx *gorm.DB) error {
+		err := tx.Create(event).Error
+		if err != nil {
+			return fmt.Errorf("creating event: %w", err)
+		}
+
+		for i := range targets {
+			dlv, err := createDelivery(tx, event.ID, targets[i].ID, false)
+			if err != nil {
+				return err
+			}
+
+			tasks = append(tasks, buildTask(
+				dlv, event.WebhookID, event, &targets[i], 1,
+			))
+		}
+
+		err = database.AddEventTotals(tx, database.EventTotals{
+			Events: 1, LastEventAt: &event.CreatedAt,
+		})
+		if err != nil {
+			return err
+		}
+
+		// A resubmitted copy did not arrive on its entrypoint's URL, so
+		// it leaves the entrypoint's last event as it is.
+		if event.ResubmittedFromID != nil {
+			return nil
+		}
+
+		return database.AddEntrypointTotals(tx, database.EntrypointTotals{
+			EntrypointID: event.EntrypointID, LastEventAt: event.CreatedAt,
+		})
+	})
+	if err != nil {
+		return 0, err
+	}
+
+	// Counted here, after the commit: an event exists once it is
+	// durably stored, which is what the delivery counters are compared
+	// against on a dashboard. A resubmitted event counts too, because
+	// it produces deliveries that the delivery side counts; the
+	// resubmit counter is what separates the two.
+	e.mtr.EventReceived()
+
+	e.Notify(tasks)
+
+	return len(tasks), nil
+}
+
+// CreateReplay creates one more pending delivery of the stored event
+// eventID to target, marked as a replay, adds it to the target's totals
+// in the same transaction, and queues it. It returns the new
+// delivery's id.
+//
+// The caller passes the target as it stands now and has already
+// decided the replay may go ahead. The delivery being replayed is not
+// touched.
+func (e *Engine) CreateReplay(
+	webhookID, eventID string, target *database.Target,
+) (string, error) {
+	webhookDB, err := e.dbManager.GetDB(webhookID)
+	if err != nil {
+		return "", fmt.Errorf("getting webhook database: %w", err)
+	}
+
+	event, err := e.loadEvent(webhookDB, eventID)
+	if err != nil {
+		return "", err
+	}
+
+	var dlv *database.Delivery
+
+	err = webhookDB.Transaction(func(tx *gorm.DB) error {
+		var err error
+
+		dlv, err = createDelivery(tx, event.ID, target.ID, true)
+
+		return err
+	})
+	if err != nil {
+		return "", err
+	}
+
+	e.Notify([]Task{buildTask(dlv, webhookID, &event, target, 1)})
+
+	return dlv.ID, nil
+}
+
+// createDelivery writes a pending delivery of the event to the target
+// on the caller's transaction, and adds it to the target's totals.
+//
+// Neither Event nor Target is set on the row, and the per-webhook
+// database omits associations on every write in any case (see
+// internal/database/event_db_isolation.go), so the target's settings
+// are never copied into the event database.
+func createDelivery(
+	tx *gorm.DB, eventID, targetID string, replay bool,
+) (*database.Delivery, error) {
+	dlv := &database.Delivery{
+		EventID:  eventID,
+		TargetID: targetID,
+		Status:   database.DeliveryStatusPending,
+		Replay:   replay,
+	}
+
+	err := tx.Create(dlv).Error
+	if err != nil {
+		return nil, fmt.Errorf(
+			"creating delivery for target %s: %w", targetID, err,
+		)
+	}
+
+	err = database.AddTargetTotals(tx, database.TargetTotals{
+		TargetID: targetID, Deliveries: 1,
+	})
+	if err != nil {
+		return nil, err
+	}
+
+	return dlv, nil
+}
@@ -0,0 +1,377 @@
+package delivery_test
+
+import (
+	"net/http"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/google/uuid"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"gorm.io/gorm"
+	"sneak.berlin/go/webhooker/internal/database"
+	"sneak.berlin/go/webhooker/internal/delivery"
+)
+
+// storeTestTarget creates an active http target of the setup's webhook
+// with every one of a target's settings set, so a task that drops one
+// of them differs from the target.
+func storeTestTarget(
+	t *testing.T, s iSetup, name string,
+) database.Target {
+	t.Helper()
+
+	target := database.Target{
+		WebhookID:       s.WebhookID,
+		Name:            name,
+		Type:            database.TargetTypeHTTP,
+		Active:          true,
+		URL:             "http://93.184.216.34/" + name,
+		Headers:         map[string]string{"X-Api-Key": "k"},
+		TimeoutSeconds:  7,
+		ForwardQuery:    true,
+		ArchiveExpiry:   time.Hour,
+		ArchiveRotation: rotationDaily,
+		MaxAttempts:     3,
+	}
+
+	require.NoError(t, s.MainDB.Create(&target).Error)
+
+	return target
+}
+
+// newStoreTestEvent returns an event of the setup's webhook as the
+// receiver builds one, not yet stored.
+func newStoreTestEvent(s iSetup, body string) *database.Event {
+	return &database.Event{
+		WebhookID:    s.WebhookID,
+		EntrypointID: uuid.New().String(),
+		Method:       http.MethodPost,
+		RawQuery:     "a=1&b=2",
+		Headers:      `{"X-Test":["yes"]}`,
+		ContentType:  testContentType,
+		Body:         body,
+	}
+}
+
+// queuedTasks takes every task the engine has queued so far. The
+// engine in these tests is never started, so nothing else takes them.
+func queuedTasks(e *delivery.Engine) []delivery.Task {
+	var tasks []delivery.Task
+
+	for {
+		select {
+		case task := <-e.ExportDeliveryCh():
+			tasks = append(tasks, task)
+		default:
+			return tasks
+		}
+	}
+}
+
+// assertTaskFor proves task carries delivery deliveryID of event to
+// target: every one of the target's settings and every event field.
+// They are copied into the task one field at a time, and a field left
+// out is sent empty without any error.
+func assertTaskFor(
+	t *testing.T,
+	task delivery.Task,
+	deliveryID string,
+	event *database.Event,
+	target *database.Target,
+) {
+	t.Helper()
+
+	assert.Equal(t, deliveryID, task.DeliveryID)
+	assert.Equal(t, event.ID, task.EventID)
+	assert.Equal(t, event.WebhookID, task.WebhookID)
+	assert.Equal(t, event.EntrypointID, task.EntrypointID)
+
+	assert.Equal(t, target.ID, task.TargetID)
+	assert.Equal(t, target.Name, task.TargetName)
+	assert.Equal(t, target.Type, task.TargetType)
+	assert.Equal(t, target.URL, task.TargetURL)
+	assert.Equal(t, target.Headers, task.TargetHeaders)
+	assert.Equal(t, target.TimeoutSeconds, task.TimeoutSeconds)
+	assert.Equal(t, target.ForwardQuery, task.ForwardQuery)
+	assert.Equal(t, target.ArchiveExpiry, task.ArchiveExpiry)
+	assert.Equal(t, target.ArchiveRotation, task.ArchiveRotation)
+	assert.Equal(t, target.MaxAttempts, task.MaxAttempts)
+
+	assert.Equal(t, event.Method, task.Method)
+	assert.Equal(t, event.RawQuery, task.RawQuery)
+	assert.Equal(t, event.Headers, task.Headers)
+	assert.Equal(t, event.ContentType, task.ContentType)
+	require.NotNil(t, task.Body)
+	assert.Equal(t, event.Body, *task.Body)
+
+	assert.Equal(t, 1, task.AttemptNum)
+}
+
+// storedDeliveries reads every delivery of an event.
+func storedDeliveries(
+	t *testing.T, db *gorm.DB, eventID string,
+) []database.Delivery {
+	t.Helper()
+
+	var deliveries []database.Delivery
+
+	require.NoError(t, db.Where("event_id = ?", eventID).
+		Find(&deliveries).Error)
+
+	return deliveries
+}
+
+// storedEventTotals reads the webhook's event totals.
+func storedEventTotals(t *testing.T, db *gorm.DB) database.EventTotals {
+	t.Helper()
+
+	var totals database.EventTotals
+
+	require.NoError(t, db.First(&totals).Error)
+
+	return totals
+}
+
+// countRows counts the rows of model in db.
+func countRows(t *testing.T, db *gorm.DB, model any) int64 {
+	t.Helper()
+
+	var n int64
+
+	require.NoError(t, db.Model(model).Count(&n).Error)
+
+	return n
+}
+
+// TestStoreEvent_StoresDeliveriesToActiveTargets proves StoreEvent
+// stores the event with one pending delivery to each active target and
+// none to an inactive one, adds them to the totals, and queues one task
+// per delivery.
+func TestStoreEvent_StoresDeliveriesToActiveTargets(t *testing.T) {
+	t.Parallel()
+
+	s := newISetup(t)
+	targets := []database.Target{
+		storeTestTarget(t, s, "first"),
+		storeTestTarget(t, s, "second"),
+	}
+	off := storeTestTarget(t, s, "off")
+
+	require.NoError(t, s.MainDB.Model(&database.Target{}).
+		Where("id = ?", off.ID).Update("active", false).Error)
+
+	event := newStoreTestEvent(s, `{"hello":"world"}`)
+
+	created, err := s.Engine.StoreEvent(event)
+	require.NoError(t, err)
+	assert.Equal(t, 2, created)
+
+	var stored database.Event
+
+	require.NoError(t, s.WebhookDB.First(&stored, "id = ?", event.ID).Error)
+	assert.Equal(t, event.Body, stored.Body)
+	assert.Equal(t, int64(len(event.Body)), stored.BodyBytes)
+
+	rows := storedDeliveries(t, s.WebhookDB, event.ID)
+	queued := queuedTasks(s.Engine)
+
+	require.Len(t, rows, 2)
+	require.Len(t, queued, 2)
+
+	deliveries := map[string]database.Delivery{}
+	for _, dlv := range rows {
+		deliveries[dlv.TargetID] = dlv
+	}
+
+	tasks := map[string]delivery.Task{}
+	for _, task := range queued {
+		tasks[task.TargetID] = task
+	}
+
+	for i := range targets {
+		dlv, ok := deliveries[targets[i].ID]
+		require.True(t, ok, "no delivery to %s", targets[i].Name)
+		assert.Equal(t, database.DeliveryStatusPending, dlv.Status)
+		assert.False(t, dlv.Replay)
+		assert.Equal(t, int64(1),
+			targetTotals(t, s.WebhookDB, targets[i].ID).Deliveries)
+
+		assertTaskFor(t, tasks[targets[i].ID], dlv.ID, event, &targets[i])
+	}
+
+	totals := storedEventTotals(t, s.WebhookDB)
+	assert.Equal(t, int64(1), totals.Events)
+	require.NotNil(t, totals.LastEventAt)
+	assert.True(t, totals.LastEventAt.Equal(stored.CreatedAt))
+
+	var entrypoint database.EntrypointTotals
+
+	require.NoError(t, s.WebhookDB.First(
+		&entrypoint, "entrypoint_id = ?", event.EntrypointID,
+	).Error)
+	assert.True(t, entrypoint.LastEventAt.Equal(stored.CreatedAt))
+}
+
+// TestStoreEvent_ResubmittedCopyLeavesTheEntrypoint proves a
+// resubmitted copy is counted among the webhook's events but leaves its
+// entrypoint's last event as it is: it did not arrive on that URL.
+func TestStoreEvent_ResubmittedCopyLeavesTheEntrypoint(t *testing.T) {
+	t.Parallel()
+
+	s := newISetup(t)
+	storeTestTarget(t, s, "only")
+
+	sourceID := uuid.New().String()
+	event := newStoreTestEvent(s, `{"copy":true}`)
+	event.ResubmittedFromID = &sourceID
+
+	created, err := s.Engine.StoreEvent(event)
+	require.NoError(t, err)
+	assert.Equal(t, 1, created)
+	assert.Len(t, queuedTasks(s.Engine), 1)
+
+	assert.Equal(t, int64(1), storedEventTotals(t, s.WebhookDB).Events)
+	assert.Zero(t, countRows(t, s.WebhookDB, &database.EntrypointTotals{}))
+}
+
+// TestStoreEvent_NoActiveTarget proves an event of a webhook with no
+// active target is stored with no delivery and nothing queued.
+func TestStoreEvent_NoActiveTarget(t *testing.T) {
+	t.Parallel()
+
+	s := newISetup(t)
+	event := newStoreTestEvent(s, `{"to":"nobody"}`)
+
+	created, err := s.Engine.StoreEvent(event)
+	require.NoError(t, err)
+	assert.Zero(t, created)
+
+	assert.Equal(t, int64(1), countRows(t, s.WebhookDB, &database.Event{}))
+	assert.Empty(t, storedDeliveries(t, s.WebhookDB, event.ID))
+	assert.Empty(t, queuedTasks(s.Engine))
+}
+
+// TestStoreEvent_LargeBodyIsNotCarried proves a body at the inline
+// limit is stored whole but left off the task, for the attempt to read
+// from the event row.
+func TestStoreEvent_LargeBodyIsNotCarried(t *testing.T) {
+	t.Parallel()
+
+	s := newISetup(t)
+	storeTestTarget(t, s, "only")
+
+	event := newStoreTestEvent(
+		s, strings.Repeat("x", delivery.MaxInlineBodySize),
+	)
+
+	_, err := s.Engine.StoreEvent(event)
+	require.NoError(t, err)
+
+	tasks := queuedTasks(s.Engine)
+	require.Len(t, tasks, 1)
+	assert.Nil(t, tasks[0].Body)
+
+	var stored database.Event
+
+	require.NoError(t, s.WebhookDB.First(&stored, "id = ?", event.ID).Error)
+	assert.Equal(t, event.Body, stored.Body)
+}
+
+// TestStoreEvent_RollsBackWhenAWriteFails proves the event, its
+// deliveries and its totals are one transaction: when the target totals
+// write fails, after the event and its delivery were written, none of
+// them is left and nothing is queued.
+func TestStoreEvent_RollsBackWhenAWriteFails(t *testing.T) {
+	t.Parallel()
+
+	s := newISetup(t)
+	storeTestTarget(t, s, "only")
+
+	require.NoError(t,
+		s.WebhookDB.Migrator().DropTable(&database.TargetTotals{}))
+
+	_, err := s.Engine.StoreEvent(newStoreTestEvent(s, `{"lost":true}`))
+	require.Error(t, err)
+
+	assert.Zero(t, countRows(t, s.WebhookDB, &database.Event{}))
+	assert.Zero(t, countRows(t, s.WebhookDB, &database.Delivery{}))
+	assert.Zero(t, storedEventTotals(t, s.WebhookDB).Events)
+	assert.Zero(t, countRows(t, s.WebhookDB, &database.EntrypointTotals{}))
+	assert.Empty(t, queuedTasks(s.Engine))
+	assert.Zero(t, s.Engine.ExportInflightHeld())
+}
+
+// TestCreateReplay_CreatesAndQueuesAReplay proves CreateReplay adds a
+// new pending delivery marked as a replay, counts it in the target's
+// totals, queues its task built from the target it was given and the
+// stored event, and leaves the delivery it repeats as it was.
+func TestCreateReplay_CreatesAndQueuesAReplay(t *testing.T) {
+	t.Parallel()
+
+	s := newISetup(t)
+	target := storeTestTarget(t, s, "replayed")
+	event := iSeedEvent(t, s.WebhookDB, s.WebhookID, `{"replay":"me"}`)
+	original := iSeedDelivery(
+		t, s.WebhookDB, event.ID, target.ID,
+		database.DeliveryStatusFailed,
+	)
+
+	deliveryID, err := s.Engine.CreateReplay(s.WebhookID, event.ID, &target)
+	require.NoError(t, err)
+
+	replay := storedDelivery(t, s.WebhookDB, deliveryID)
+	assert.Equal(t, event.ID, replay.EventID)
+	assert.Equal(t, target.ID, replay.TargetID)
+	assert.Equal(t, database.DeliveryStatusPending, replay.Status)
+	assert.True(t, replay.Replay)
+
+	assert.Equal(t, database.DeliveryStatusFailed,
+		storedDelivery(t, s.WebhookDB, original.ID).Status)
+	assert.Equal(t, int64(1),
+		targetTotals(t, s.WebhookDB, target.ID).Deliveries)
+
+	tasks := queuedTasks(s.Engine)
+	require.Len(t, tasks, 1)
+	assertTaskFor(t, tasks[0], deliveryID, &event, &target)
+}
+
+// TestCreateReplay_RollsBackWhenAWriteFails proves the replay delivery
+// and its target totals are one transaction: when the totals write
+// fails, after the delivery was written, no new delivery is left and
+// nothing is queued.
+func TestCreateReplay_RollsBackWhenAWriteFails(t *testing.T) {
+	t.Parallel()
+
+	s := newISetup(t)
+	target := storeTestTarget(t, s, "replayed")
+	event := iSeedEvent(t, s.WebhookDB, s.WebhookID, `{"replay":"me"}`)
+	iSeedDelivery(
+		t, s.WebhookDB, event.ID, target.ID,
+		database.DeliveryStatusFailed,
+	)
+
+	require.NoError(t,
+		s.WebhookDB.Migrator().DropTable(&database.TargetTotals{}))
+
+	_, err := s.Engine.CreateReplay(s.WebhookID, event.ID, &target)
+	require.Error(t, err)
+
+	assert.Len(t, storedDeliveries(t, s.WebhookDB, event.ID), 1)
+	assert.Empty(t, queuedTasks(s.Engine))
+	assert.Zero(t, s.Engine.ExportInflightHeld())
+}
+
+// storedDelivery reads one delivery back.
+func storedDelivery(
+	t *testing.T, db *gorm.DB, deliveryID string,
+) database.Delivery {
+	t.Helper()
+
+	var dlv database.Delivery
+
+	require.NoError(t, db.First(&dlv, "id = ?", deliveryID).Error)
+
+	return dlv
+}