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

The receiver, resubmit and replay handlers now read and check 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, and the engine's notify is
no longer exported. 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

README.md

@@ -2187,7 +2187,7 @@ A **Resubmit** stores its copy of an event through the same `Engine.StoreEvent`,
 and a **Replay** adds one delivery of a stored event through
 `Engine.CreateReplay`, which writes the delivery and its target's totals in one
 transaction and queues it. Those two methods are the only code that creates a
-delivery; the handlers parse the request and make the one call.
+delivery; the handlers read and check the request, then make the one call.
 
 ### Bounded Worker Pool
 
@@ -2198,8 +2198,9 @@ preventing goroutine explosions regardless of queue depth.
 **Architecture:**
 
 - **Channels as queues:** Two buffered channels serve as bounded queues: a
-  delivery channel (the tasks of deliveries the engine has just stored) and a
-  retry channel (tasks from backoff timers). Both are buffered to 10,000.
+  delivery channel (the tasks of newly stored deliveries and of the `pending`
+  deliveries the recovery routine hands back) and a retry channel (tasks from
+  backoff timers). Both are buffered to 10,000.
 - **Fan-out via channel, not goroutines:** When an event arrives with multiple
   targets, each `delivery.Task` is sent to the delivery channel. Workers pick
   them up and process them — no goroutine-per-target.
@@ -3161,7 +3162,7 @@ Components are wired via Uber fx in this order:
 11. `metrics.New` — The delivery collectors, registered on that registry
 12. `middleware.New` — HTTP middleware
 13. `delivery.New` — Event-driven delivery engine, which also stores each event
-    with its deliveries
+    with its deliveries and creates replay deliveries
 14. `archive.NewWriters` — The database targets' archive writers, and the lock
     every rename of their files runs under
 15. `archive.NewSweeper` — Periodic pruning of idle archives

internal/delivery/engine.go

@@ -23,10 +23,11 @@ import (
 
 const (
 	// deliveryChannelSize is the buffer size for the delivery
-	// channel. The Tasks of newly stored deliveries are sent
-	// here. Workers drain this channel. Sized large enough
-	// that storing an event should never block under normal
-	// load.
+	// channel. The Tasks of newly stored deliveries, and of the
+	// pending deliveries the recovery routine hands back, are
+	// sent here. Workers drain this channel. Sized large enough
+	// that no task should be dropped under normal load: one that
+	// does not fit is left for the sweep.
 	deliveryChannelSize = 10000
 
 	// retryChannelSize is the buffer size for the retry
@@ -151,12 +152,13 @@ type EngineParams struct {
 }
 
 // Engine processes queued deliveries in the background
-// using a bounded worker pool architecture. It owns only
-// the cross-target machinery: the worker pool, the queue
-// and retry channels, the recovery routine, and the persistence
-// helpers and Scheduler that individual targets rely on.
-// Each target type owns its own delivery, including retries,
-// backoff, and circuit breaking.
+// using a bounded worker pool architecture. It also stores each
+// event with its deliveries and creates replay deliveries (see
+// store.go). Beyond that it owns only the cross-target machinery:
+// the worker pool, the delivery and retry channels, the recovery
+// routine, and the persistence helpers and Scheduler that
+// individual targets rely on. Each target type owns its own
+// delivery, including retries, backoff, and circuit breaking.
 type Engine struct {
 	database   *database.Database
 	dbManager  *database.WebhookDBManager
@@ -226,38 +228,6 @@ func New(
 	return e
 }
 
-// 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
-		// it returns. A task can sit in a 10000-deep channel for a
-		// long time on a healthy system, and nothing may re-send it
-		// while it waits. See inflight.go.
-		if !e.inflight.retainIdle(tasks[i].DeliveryID) {
-			e.log.Warn(
-				"delivery already in flight, not queued again",
-				"delivery_id", tasks[i].DeliveryID,
-				"event_id", tasks[i].EventID,
-			)
-
-			continue
-		}
-
-		select {
-		case e.deliveryCh <- tasks[i]:
-		default:
-			e.inflight.release(tasks[i].DeliveryID)
-			e.log.Warn(
-				"delivery channel full, "+
-					"task will be recovered by the sweep",
-				"delivery_id", tasks[i].DeliveryID,
-				"event_id", tasks[i].EventID,
-			)
-		}
-	}
-}
-
 // StateAndCooldown implements CircuitBreakers. It returns the state of
 // the target's circuit breaker and, while the breaker is open, what is
 // left of its cooldown; the cooldown is zero once that has passed and
@@ -318,6 +288,38 @@ func (e *Engine) ScheduleRetry(
 	})
 }
 
+// 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
+		// it returns. A task can sit in a 10000-deep channel for a
+		// long time on a healthy system, and nothing may re-send it
+		// while it waits. See inflight.go.
+		if !e.inflight.retainIdle(tasks[i].DeliveryID) {
+			e.log.Warn(
+				"delivery already in flight, not queued again",
+				"delivery_id", tasks[i].DeliveryID,
+				"event_id", tasks[i].EventID,
+			)
+
+			continue
+		}
+
+		select {
+		case e.deliveryCh <- tasks[i]:
+		default:
+			e.inflight.release(tasks[i].DeliveryID)
+			e.log.Warn(
+				"delivery channel full, "+
+					"task will be recovered by the sweep",
+				"delivery_id", tasks[i].DeliveryID,
+				"event_id", tasks[i].EventID,
+			)
+		}
+	}
+}
+
 // userAgent is the User-Agent header of every http and slack
 // delivery request: the program name and the running build's
 // version.
@@ -915,7 +917,7 @@ func (e *Engine) recoverSingleRetry(
 // it stopped working on before then: for example one whose bookkeeping
 // write failed, whose attempt a stop cut off, that was still queued
 // when the engine last stopped, whose task was dropped because the
-// delivery channel was full (by Notify or by queueRecovered), or whose
+// delivery channel was full (by notify or by queueRecovered), or whose
 // worker could not open the webhook's database.
 //
 // At start every pending delivery is taken, however recent, so one a

internal/delivery/engine_integration_test.go

@@ -492,7 +492,7 @@ func TestWorkerLifecycle_StartStop(t *testing.T) {
 	)
 	task.TargetType = database.TargetTypeLog
 
-	s.Engine.Notify([]delivery.Task{task})
+	s.Engine.ExportNotify([]delivery.Task{task})
 
 	iWaitForDelivered(t, s.WebhookDB, d.ID)
 
@@ -1452,7 +1452,7 @@ func TestDeliverHTTP_NoURL(t *testing.T) {
 	iAssertResultFailed(t, db, del.ID)
 }
 
-// --- Notify batching ---
+// --- notify batching ---
 
 func TestNotify_MultipleTasks(t *testing.T) {
 	t.Parallel()
@@ -1467,7 +1467,7 @@ func TestNotify_MultipleTasks(t *testing.T) {
 		}
 	}
 
-	e.Notify(tasks)
+	e.ExportNotify(tasks)
 
 	for i := range 5 {
 		select {

internal/delivery/engine_lifecycle_test.go

@@ -29,7 +29,7 @@ const (
 	// OnStart before the caller may enqueue work. A worker pool
 	// wrongly rooted in the already-done hook context has nothing
 	// but ctx.Done() ready in its select, so it is deterministically
-	// gone by the end of this window. Without the wait, Notify would
+	// gone by the end of this window. Without the wait, notify would
 	// race the pool's very first select, in which a ready ctx.Done()
 	// and a ready deliveryCh are chosen between at random and a
 	// doomed pool still delivers.
@@ -182,7 +182,7 @@ func TestEngine_WorkersOutliveStartHookContext(t *testing.T) {
 	// routine's run at start cannot enqueue it.
 	deliveryID, task := seedLogTask(t, s)
 
-	s.Engine.Notify([]delivery.Task{task})
+	s.Engine.ExportNotify([]delivery.Task{task})
 
 	iWaitForDelivered(t, s.WebhookDB, deliveryID)
 }
@@ -202,7 +202,7 @@ func TestEngine_StopHookStopsWorkers(t *testing.T) {
 	// Let the pool prove it is running before stopping it, so a
 	// fast OnStop cannot pass by stopping something already dead.
 	firstID, firstTask := seedLogTask(t, s)
-	s.Engine.Notify([]delivery.Task{firstTask})
+	s.Engine.ExportNotify([]delivery.Task{firstTask})
 	iWaitForDelivered(t, s.WebhookDB, firstID)
 
 	var stopErr error
@@ -232,7 +232,7 @@ func TestEngine_StopHookStopsWorkers(t *testing.T) {
 	// With every worker gone, a freshly notified task must sit
 	// untouched in the queue rather than being delivered.
 	secondID, secondTask := seedLogTask(t, s)
-	s.Engine.Notify([]delivery.Task{secondTask})
+	s.Engine.ExportNotify([]delivery.Task{secondTask})
 
 	time.Sleep(200 * time.Millisecond)
 
@@ -383,7 +383,7 @@ func TestEngine_StopLeavesFireAndForgetAttemptPending(t *testing.T) {
 		t, s, 1, database.DeliveryStatusPending, 1,
 	)
 
-	s.Engine.Notify([]delivery.Task{task})
+	s.Engine.ExportNotify([]delivery.Task{task})
 	stopDuringAttempt(t, lc, arrived)
 
 	iAssertStatus(
@@ -448,7 +448,7 @@ func deliverToArchive(t *testing.T, s iSetup) string {
 	task.TargetID = tgt.ID
 	task.TargetType = database.TargetTypeDatabase
 
-	s.Engine.Notify([]delivery.Task{task})
+	s.Engine.ExportNotify([]delivery.Task{task})
 
 	iWaitForDelivered(t, s.WebhookDB, deliveryID)
 

internal/delivery/engine_test.go

@@ -218,7 +218,7 @@ func TestNotify_NonBlocking(t *testing.T) {
 	done := make(chan struct{})
 
 	go func() {
-		e.Notify([]delivery.Task{
+		e.ExportNotify([]delivery.Task{
 			{DeliveryID: "overflow-1"},
 			{DeliveryID: "overflow-2"},
 		})
@@ -229,7 +229,7 @@ func TestNotify_NonBlocking(t *testing.T) {
 	case <-done:
 	case <-time.After(2 * time.Second):
 		t.Fatal(
-			"Notify blocked when delivery channel was full",
+			"notify blocked when delivery channel was full",
 		)
 	}
 }

internal/delivery/export_test.go

@@ -158,6 +158,11 @@ func (e *Engine) ExportUpdateDeliveryStatus(
 	return e.updateDeliveryStatus(webhookDB, d, "", status)
 }
 
+// ExportNotify exposes notify.
+func (e *Engine) ExportNotify(tasks []Task) {
+	e.notify(tasks)
+}
+
 // ExportProcessNewTask exposes processNewTask.
 func (e *Engine) ExportProcessNewTask(
 	ctx context.Context, task *Task,

internal/delivery/inflight.go

@@ -61,7 +61,7 @@ func (s *inflightSet) retain(deliveryID string) {
 
 // retainIdle takes the first reference on a delivery, and reports
 // whether it got it. It fails when the engine already owns the
-// delivery, which is what makes two claimants — Notify and the
+// delivery, which is what makes two claimants — notify and the
 // recovery routine, say — mutually exclusive rather than merely
 // atomic.
 func (s *inflightSet) retainIdle(deliveryID string) bool {

internal/delivery/inflight_test.go

@@ -84,9 +84,9 @@ func TestSweepLeavesAQueuedDeliveryAlone(t *testing.T) {
 	)
 	rAgePending(t, s.WebhookDB, d.ID)
 
-	// Queued exactly as the receiver queues it, and never dequeued:
+	// Queued exactly as StoreEvent queues it, and never dequeued:
 	// no workers are running in this engine.
-	s.Engine.Notify([]delivery.Task{{
+	s.Engine.ExportNotify([]delivery.Task{{
 		DeliveryID: d.ID,
 		EventID:    event.ID,
 		WebhookID:  s.WebhookID,
@@ -109,7 +109,7 @@ func TestSweepLeavesAQueuedDeliveryAlone(t *testing.T) {
 
 // TestRecoveryAndSweepDoNotDoubleDispatch drives the recovery routine's
 // run at start and a tick of the sweep at once against one aged pending
-// row. The engine runs them one after the other, but Notify can race
+// row. The engine runs them one after the other, but notify can race
 // either, and ownership is what keeps any two claimants from both
 // sending. Before ownership the two runs both dispatched the row.
 func TestRecoveryAndSweepDoNotDoubleDispatch(t *testing.T) {
@@ -224,7 +224,7 @@ func TestOwnershipIsReleasedAfterDelivery(t *testing.T) {
 
 	body := `{"released":true}`
 
-	s.Engine.Notify([]delivery.Task{{
+	s.Engine.ExportNotify([]delivery.Task{{
 		DeliveryID:   d.ID,
 		EventID:      event.ID,
 		WebhookID:    s.WebhookID,
@@ -248,10 +248,10 @@ func TestOwnershipIsReleasedAfterDelivery(t *testing.T) {
 }
 
 // TestNotifyAfterRecoveryDoesNotSendAgain is the startup race of
-// https://git.eeqj.de/sneak/webhooker/issues/299. The receiver has
+// https://git.eeqj.de/sneak/webhooker/issues/299. StoreEvent has
 // written a delivery, the recovery routine's run at start finds it
-// pending, sends it and releases it, and only then does the receiver's
-// Notify for it arrive. Nothing owns the delivery by then, so Notify
+// pending, sends it and releases it, and only then does StoreEvent's
+// notify for it arrive. Nothing owns the delivery by then, so notify
 // takes it.
 func TestNotifyAfterRecoveryDoesNotSendAgain(t *testing.T) {
 	t.Parallel()
@@ -289,7 +289,7 @@ func TestNotifyAfterRecoveryDoesNotSendAgain(t *testing.T) {
 
 	body := event.Body
 
-	s.Engine.Notify([]delivery.Task{{
+	s.Engine.ExportNotify([]delivery.Task{{
 		DeliveryID:   d.ID,
 		EventID:      event.ID,
 		WebhookID:    s.WebhookID,
@@ -300,7 +300,7 @@ func TestNotifyAfterRecoveryDoesNotSendAgain(t *testing.T) {
 		EntrypointID: event.EntrypointID,
 	}})
 
-	// Notify took the delivery, and a worker releases it once it has
+	// notify took the delivery, and a worker releases it once it has
 	// run the task.
 	require.Eventually(
 		t,

internal/delivery/store.go

@@ -85,7 +85,7 @@ func (e *Engine) StoreEvent(event *database.Event) (int, error) {
 	// resubmit counter is what separates the two.
 	e.mtr.EventReceived()
 
-	e.Notify(tasks)
+	e.notify(tasks)
 
 	return len(tasks), nil
 }
@@ -124,7 +124,7 @@ func (e *Engine) CreateReplay(
 		return "", err
 	}
 
-	e.Notify([]Task{buildTask(dlv, webhookID, &event, target, 1)})
+	e.notify([]Task{buildTask(dlv, webhookID, &event, target, 1)})
 
 	return dlv.ID, nil
 }

internal/delivery/store_test.go

@@ -280,9 +280,10 @@ func TestStoreEvent_LargeBodyIsNotCarried(t *testing.T) {
 }
 
 // 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.
+// deliveries and its totals are one transaction: when the entrypoint
+// totals write fails, the last one, after the event, its delivery, the
+// target totals and the event totals were written, none of them is left
+// and nothing is queued.
 func TestStoreEvent_RollsBackWhenAWriteFails(t *testing.T) {
 	t.Parallel()
 
@@ -290,15 +291,19 @@ func TestStoreEvent_RollsBackWhenAWriteFails(t *testing.T) {
 	storeTestTarget(t, s, "only")
 
 	require.NoError(t,
-		s.WebhookDB.Migrator().DropTable(&database.TargetTotals{}))
+		s.WebhookDB.Migrator().DropTable(&database.EntrypointTotals{}))
 
 	_, 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.Zero(t, countRows(t, s.WebhookDB, &database.TargetTotals{}))
+
+	totals := storedEventTotals(t, s.WebhookDB)
+	assert.Zero(t, totals.Events)
+	assert.Nil(t, totals.LastEventAt)
+
 	assert.Empty(t, queuedTasks(s.Engine))
 	assert.Zero(t, s.Engine.ExportInflightHeld())
 }
@@ -306,13 +311,17 @@ func TestStoreEvent_RollsBackWhenAWriteFails(t *testing.T) {
 // 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.
+// stored event, and leaves the delivery it repeats as it was. The
+// stored event has every event field set, so a task that drops one of
+// them differs from the event.
 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"}`)
+	event := newStoreTestEvent(s, `{"replay":"me"}`)
+	require.NoError(t, s.WebhookDB.Create(event).Error)
+
 	original := iSeedDelivery(
 		t, s.WebhookDB, event.ID, target.ID,
 		database.DeliveryStatusFailed,
@@ -334,7 +343,7 @@ func TestCreateReplay_CreatesAndQueuesAReplay(t *testing.T) {
 
 	tasks := queuedTasks(s.Engine)
 	require.Len(t, tasks, 1)
-	assertTaskFor(t, tasks[0], deliveryID, &event, &target)
+	assertTaskFor(t, tasks[0], deliveryID, event, &target)
 }
 
 // TestCreateReplay_RollsBackWhenAWriteFails proves the replay delivery

internal/handlers/event_resubmit.go

@@ -172,8 +172,10 @@ func loadResubmitSource(
 }
 
 // queueResubmit has the delivery engine store the copy, with a delivery
-// for each of the webhook's active targets as they stand now, exactly
-// as it stores a received event.
+// for each of the webhook's active targets as they stand now, through
+// the same StoreEvent the receiver uses. The one difference is that the
+// copy leaves its entrypoint's last event as it is, since it did not
+// arrive on that URL.
 func (h *Handlers) queueResubmit(
 	w http.ResponseWriter,
 	r *http.Request,

internal/handlers/handlers_test.go

@@ -239,8 +239,8 @@ func newTestAppWithConfig(
 			session.New,
 			archive.NewWriters,
 			// The engine is built on a lifecycle that is never started,
-			// so no worker runs: every delivery a handler creates stays
-			// pending, where the test can read it.
+			// so no worker runs: every delivery the engine creates for a
+			// handler stays pending, where the test can read it.
 			func(p delivery.EngineParams) *delivery.Engine {
 				return delivery.New(fxtest.NewLifecycle(t), p)
 			},