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

internal/handlers/delivery_replay_test.go

@@ -12,7 +12,6 @@ import (
 	"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"
 )
@@ -23,7 +22,7 @@ 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.
+// dispatches to it: the delivery engine is never started.
 const replayTargetURL = "http://93.184.216.34/hook"
 
 // replayEventQuery is the query string a seeded event arrived with.
@@ -163,14 +162,8 @@ func postReplay(
 
 // 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.
+// new delivery marked as a replay, and leaves the original row and its
+// recorded attempt exactly as they were.
 func TestHandleDeliveryReplay_AppendsDeliveryAndLeavesOriginal(
 	t *testing.T,
 ) {
@@ -181,10 +174,9 @@ func TestHandleDeliveryReplay_AppendsDeliveryAndLeavesOriginal(
 		sess  *session.Session
 		db    *database.Database
 		dbMgr *database.WebhookDBManager
-		notif *recordingNotifier
 	)
 
-	app := newTestApp(t, &h, &sess, &db, &dbMgr, &notif)
+	app := newTestApp(t, &h, &sess, &db, &dbMgr)
 	app.RequireStart()
 
 	t.Cleanup(app.RequireStop)
@@ -204,15 +196,6 @@ func TestHandleDeliveryReplay_AppendsDeliveryAndLeavesOriginal(
 
 	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 URL the original
-	// delivery ran against.
-	const fixedURL = "http://93.184.216.34/fixed"
-
-	require.NoError(t, db.DB().Model(&database.Target{}).
-		Where("id = ?", tgt.ID).
-		Update("url", fixedURL).Error)
-
 	w := postReplay(t, h, sess, wh.ID, original.ID)
 
 	require.Equal(t, http.StatusSeeOther, w.Code)
@@ -234,14 +217,9 @@ func TestHandleDeliveryReplay_AppendsDeliveryAndLeavesOriginal(
 	assert.Equal(
 		t, database.DeliveryStatusPending, replayed.Status,
 	)
+	assert.True(t, replayed.Replay)
 
 	assertDeliveryUntouched(t, webhookDB, before)
-
-	tasks := notif.Tasks()
-	require.Len(t, tasks, 1)
-	assertReplayTask(
-		t, tasks[0], wh.ID, event, tgt, replayed.ID, fixedURL,
-	)
 	assertNoLeakedTarget(t, webhookDB)
 }
 
@@ -275,45 +253,6 @@ func assertDeliveryUntouched(
 	)
 }
 
-// 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 wantURL — the target's URL 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, wantURL 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.Equal(
-		t, wantURL, task.TargetURL,
-		"replay must use the target's current URL",
-	)
-	assert.Equal(t, event.Method, task.Method)
-	assert.Equal(
-		t, replayEventQuery, task.RawQuery,
-		"replay re-sends the stored query string",
-	)
-	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: an association write would have upserted the whole target,
 // plaintext config and all, into the event database. See
@@ -327,12 +266,10 @@ func assertNoLeakedTarget(t *testing.T, webhookDB *gorm.DB) {
 	)
 }
 
-// TestHandleDeliveryReplay_TaskCarriesEveryTargetSetting proves the
-// task a replay hands the delivery engine carries every one of the
-// target's settings.
-func TestHandleDeliveryReplay_TaskCarriesEveryTargetSetting(
-	t *testing.T,
-) {
+// TestHandleDeliveryReplay_FailedStoreIsAnError proves a replay the
+// engine cannot store answers with the 500 error page and adds no
+// delivery.
+func TestHandleDeliveryReplay_FailedStoreIsAnError(t *testing.T) {
 	t.Parallel()
 
 	var (
@@ -340,10 +277,9 @@ func TestHandleDeliveryReplay_TaskCarriesEveryTargetSetting(
 		sess  *session.Session
 		db    *database.Database
 		dbMgr *database.WebhookDBManager
-		notif *recordingNotifier
 	)
 
-	app := newTestApp(t, &h, &sess, &db, &dbMgr, &notif)
+	app := newTestApp(t, &h, &sess, &db, &dbMgr)
 	app.RequireStart()
 
 	t.Cleanup(app.RequireStop)
@@ -351,22 +287,28 @@ func TestHandleDeliveryReplay_TaskCarriesEveryTargetSetting(
 	wh := seedWebhook(t, db)
 	tgt := seedConfiguredTarget(
 		t, db, wh.ID, database.TargetTypeHTTP,
-		targetWithEverySetting(),
+		database.Target{URL: replayTargetURL},
 	)
 
-	_, original := seedFailedDelivery(t, dbMgr, wh.ID, tgt.ID)
+	event, original := seedFailedDelivery(t, dbMgr, wh.ID, tgt.ID)
+
+	// Without its target totals table the webhook's database refuses
+	// the totals write, which comes after the delivery row.
+	webhookDB, err := dbMgr.GetDB(wh.ID)
+	require.NoError(t, err)
+	require.NoError(t,
+		webhookDB.Migrator().DropTable(&database.TargetTotals{}))
 
 	w := postReplay(t, h, sess, wh.ID, original.ID)
-	require.Equal(t, http.StatusSeeOther, w.Code)
 
-	tasks := notif.Tasks()
-	require.Len(t, tasks, 1)
-	assertTaskCarriesTargetSettings(t, tasks[0], tgt)
+	assert.Equal(t, http.StatusInternalServerError, w.Code)
+	assert.Contains(t, w.Body.String(), "500 Internal Server Error")
+	assert.Len(t, listDeliveries(t, webhookDB, event.ID), 1)
 }
 
 // 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.
+// deleted rather than erroring, and nothing is created.
 func TestHandleDeliveryReplay_RefusesDeletedTarget(t *testing.T) {
 	t.Parallel()
 
@@ -375,10 +317,9 @@ func TestHandleDeliveryReplay_RefusesDeletedTarget(t *testing.T) {
 		sess  *session.Session
 		db    *database.Database
 		dbMgr *database.WebhookDBManager
-		notif *recordingNotifier
 	)
 
-	app := newTestApp(t, &h, &sess, &db, &dbMgr, &notif)
+	app := newTestApp(t, &h, &sess, &db, &dbMgr)
 	app.RequireStart()
 
 	t.Cleanup(app.RequireStop)
@@ -414,10 +355,6 @@ func TestHandleDeliveryReplay_RefusesDeletedTarget(t *testing.T) {
 		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
@@ -450,10 +387,9 @@ func TestHandleDeliveryReplay_RefusesWhileEarlierReplayInFlight(
 		sess  *session.Session
 		db    *database.Database
 		dbMgr *database.WebhookDBManager
-		notif *recordingNotifier
 	)
 
-	app := newTestApp(t, &h, &sess, &db, &dbMgr, &notif)
+	app := newTestApp(t, &h, &sess, &db, &dbMgr)
 	app.RequireStart()
 
 	t.Cleanup(app.RequireStop)
@@ -491,10 +427,6 @@ func TestHandleDeliveryReplay_RefusesWhileEarlierReplayInFlight(
 		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.