From 06be075b70147d419571d37a8a974e0f26958b1e Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Fri, 16 Oct 2026 00:51:26 +0200 Subject: [PATCH] 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 | 48 ++- cmd/webhooker/main.go | 3 - internal/delivery/engine.go | 120 +++--- internal/delivery/engine_integration_test.go | 6 +- internal/delivery/engine_lifecycle_test.go | 12 +- internal/delivery/engine_test.go | 4 +- internal/delivery/export_test.go | 5 + internal/delivery/inflight.go | 2 +- internal/delivery/inflight_test.go | 18 +- internal/delivery/store.go | 164 ++++++++ internal/delivery/store_test.go | 386 +++++++++++++++++++ internal/handlers/delivery_replay.go | 107 +---- internal/handlers/delivery_replay_test.go | 118 ++---- internal/handlers/event_resubmit.go | 42 +- internal/handlers/event_resubmit_test.go | 169 ++++---- internal/handlers/handlers.go | 6 +- internal/handlers/handlers_test.go | 38 +- internal/handlers/webhook.go | 284 +------------- internal/handlers/webhook_test.go | 100 ++--- internal/resetpw/resetpw_test.go | 11 +- internal/server/routes_test.go | 11 +- 21 files changed, 893 insertions(+), 761 deletions(-) create mode 100644 internal/delivery/store.go create mode 100644 internal/delivery/store_test.go diff --git a/README.md b/README.md index 45a474e..7eafaff 100644 --- a/README.md +++ b/README.md @@ -2153,12 +2153,8 @@ External Service 1. Look up Entrypoint by UUID — 404 if unknown, 410 if inactive 2. Read the body under the 1 MB cap - 3. Capture full request as Event - 4. Create Delivery records for each active Target - 5. Build self-contained delivery.Task structs - (target settings + event data inline for - bodies < 16 KiB) - 6. Notify Engine via channel (no DB read needed) + 3. Hand the full request, as an Event, to + Engine.StoreEvent │ ▼ ┌──────────────┐ @@ -2167,6 +2163,13 @@ External Service │ (worker │ │ pool) │ └──────┬───────┘ + 4. Store the Event, a pending Delivery for each + active Target, and the totals, in one + transaction + 5. Build a delivery.Task for each Delivery + (target settings + event data, the body + inline when it is under 16 KiB) + 6. Queue the tasks on the delivery channel │ ┌── bounded worker pool (N workers) ──┐ ▼ ▼ ▼ @@ -2180,6 +2183,12 @@ External Service └────────────┘ ``` +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 read and check the request, then make the one call. + ### Bounded Worker Pool The delivery engine uses a **fixed-size worker pool** (default: 10 workers) to @@ -2189,8 +2198,9 @@ preventing goroutine explosions regardless of queue depth. **Architecture:** - **Channels as queues:** Two buffered channels serve as bounded queues: a - delivery channel (new tasks from the webhook handler) 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. @@ -3054,6 +3064,7 @@ webhooker/ │ │ └── logfield.go # Encoded-byte budget for client-supplied log values │ ├── delivery/ │ │ ├── engine.go # Event-driven delivery engine (channel + timer based) +│ │ ├── store.go # Storing an event with its deliveries, and a replay, then queueing them │ │ ├── circuit_breaker.go # Per-target circuit breaker for http/slack targets with retries │ │ ├── target.go # Target interface, Task, Scheduler │ │ ├── target_http.go # HTTP target (retries, circuit breaker) @@ -3150,23 +3161,26 @@ Components are wired via Uber fx in this order: 10. `metrics.NewRegistry` — The registry `/metrics` serves 11. `metrics.New` — The delivery collectors, registered on that registry 12. `middleware.New` — HTTP middleware -13. `delivery.New` — Event-driven delivery engine +13. `delivery.New` — Event-driven delivery engine, which also stores each event + 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 -16. `delivery.Engine` → `delivery.Notifier` — interface bridge -17. `archive.Writers` → `archive.Archives` — interface bridge so deleting or +16. `archive.Writers` → `archive.Archives` — interface bridge so deleting or renaming a webhook or target reaches its archive files +17. `delivery.Engine` → `delivery.CircuitBreakers` — interface bridge so the + pages can show a target whose deliveries are paused 18. `server.New` — HTTP server and router The server starts via `fx.Invoke(func(*server.Server, *delivery.Engine, *database.RetentionReaper, *archive.Sweeper) {})`, -which triggers the fx lifecycle hooks in dependency order. The -`delivery.Notifier` interface allows the webhook handler to send self-contained -`delivery.Task` slices to the engine without a direct package dependency. Each -task carries all target settings and event data inline (for bodies under 16 KiB, -`delivery.MaxInlineBodySize`), so the engine can deliver without reading from -any database — it only writes to record results. +which triggers the fx lifecycle hooks in dependency order. The handlers take +`*delivery.Engine` itself: the receiver and resubmit handlers store an event +through `StoreEvent`, and the replay handler adds a delivery through +`CreateReplay`. Each task the engine queues carries the target's settings and +the event's data, and the body too when it is under 16 KiB +(`delivery.MaxInlineBodySize`); a larger body is read from the event row when +the attempt runs. ### Middleware Stack diff --git a/cmd/webhooker/main.go b/cmd/webhooker/main.go index a830af4..487176a 100644 --- a/cmd/webhooker/main.go +++ b/cmd/webhooker/main.go @@ -211,9 +211,6 @@ func newApp() *fx.App { delivery.New, archive.NewWriters, archive.NewSweeper, - // Wire *delivery.Engine as delivery.Notifier so the - // webhook handler can notify the engine of new deliveries. - func(e *delivery.Engine) delivery.Notifier { return e }, // Wire *archive.Writers as archive.Archives so deleting // or renaming a webhook or target reaches its archive // files. diff --git a/internal/delivery/engine.go b/internal/delivery/engine.go index 967f2b3..7cf5ecd 100644 --- a/internal/delivery/engine.go +++ b/internal/delivery/engine.go @@ -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,11 @@ import ( const ( // deliveryChannelSize is the buffer size for the delivery - // channel. New Tasks from the webhook handler are sent - // here. Workers drain this channel. Sized large enough - // that the webhook handler 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 @@ -127,17 +128,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) } @@ -157,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 @@ -232,38 +228,6 @@ func New( return e } -// Notify signals the delivery engine that new deliveries -// are ready. -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 @@ -324,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. @@ -472,9 +468,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 +895,7 @@ func (e *Engine) recoverSingleRetry( return } - e.ScheduleRetry(buildRecoveryTask( + e.ScheduleRetry(buildTask( d, webhookID, &event, &target, attemptNum+1, ), remaining) @@ -921,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 @@ -1822,7 +1818,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 +1998,7 @@ func (e *Engine) sendRecoveredDeliveries( continue } - task := buildRecoveryTask( + task := buildTask( &deliveries[i], webhookID, &event, &target, attempts[deliveries[i].ID]+1, ) diff --git a/internal/delivery/engine_integration_test.go b/internal/delivery/engine_integration_test.go index d226d99..f4820fd 100644 --- a/internal/delivery/engine_integration_test.go +++ b/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 { diff --git a/internal/delivery/engine_lifecycle_test.go b/internal/delivery/engine_lifecycle_test.go index 0bc2459..5da2c1a 100644 --- a/internal/delivery/engine_lifecycle_test.go +++ b/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) diff --git a/internal/delivery/engine_test.go b/internal/delivery/engine_test.go index 1caf3ca..a6e1dc0 100644 --- a/internal/delivery/engine_test.go +++ b/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", ) } } diff --git a/internal/delivery/export_test.go b/internal/delivery/export_test.go index 74dc555..318c40e 100644 --- a/internal/delivery/export_test.go +++ b/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, diff --git a/internal/delivery/inflight.go b/internal/delivery/inflight.go index c1ca6b7..ff978c9 100644 --- a/internal/delivery/inflight.go +++ b/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 { diff --git a/internal/delivery/inflight_test.go b/internal/delivery/inflight_test.go index 24ebc65..8e719a3 100644 --- a/internal/delivery/inflight_test.go +++ b/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, diff --git a/internal/delivery/store.go b/internal/delivery/store.go new file mode 100644 index 0000000..457dde3 --- /dev/null +++ b/internal/delivery/store.go @@ -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 +} diff --git a/internal/delivery/store_test.go b/internal/delivery/store_test.go new file mode 100644 index 0000000..977d6e6 --- /dev/null +++ b/internal/delivery/store_test.go @@ -0,0 +1,386 @@ +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 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() + + s := newISetup(t) + storeTestTarget(t, s, "only") + + require.NoError(t, + 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, 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()) +} + +// 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. 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 := newStoreTestEvent(s, `{"replay":"me"}`) + require.NoError(t, s.WebhookDB.Create(event).Error) + + 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 +} diff --git a/internal/handlers/delivery_replay.go b/internal/handlers/delivery_replay.go index 9a2c0a3..f23b628 100644 --- a/internal/handlers/delivery_replay.go +++ b/internal/handlers/delivery_replay.go @@ -5,9 +5,7 @@ import ( "github.com/go-chi/chi" "gorm.io/gorm" - "gorm.io/gorm/clause" "sneak.berlin/go/webhooker/internal/database" - "sneak.berlin/go/webhooker/internal/delivery" ) // The outcomes of a replay POST, as the notice codes its redirect @@ -46,12 +44,12 @@ const ( // HandleDeliveryReplay re-sends a finished delivery's event to its // target. // -// A replay never touches the delivery it repeats. It creates a NEW -// pending delivery row for the same event and target and hands it to -// the delivery engine through the same Notifier the receiver uses, so -// the original's status, attempts and timestamps stand as the record -// of what actually happened, and the replay is retried, SSRF-guarded -// and circuit-broken exactly as a first attempt is. +// A replay never touches the delivery it repeats. The delivery engine +// creates a NEW pending delivery row for the same event and target and +// queues it as it queues the receiver's, so the original's status, +// attempts and timestamps stand as the record of what actually +// happened, and the replay is retried, SSRF-guarded and circuit-broken +// exactly as a first attempt is. // // What is re-sent is the stored EVENT body, never the response the // original delivery received. @@ -145,7 +143,8 @@ func (h *Handlers) loadReplaySource( return &original, true } -// queueReplay writes the new delivery and hands it to the engine. +// queueReplay has the delivery engine create and queue the new +// delivery, unless an earlier replay is still in flight. func (h *Handlers) queueReplay( w http.ResponseWriter, r *http.Request, @@ -171,18 +170,8 @@ func (h *Handlers) queueReplay( return } - var event database.Event - - err = webhookDB. - First(&event, "id = ?", original.EventID).Error - if err != nil { - h.serverError(w, r, "failed to load event for replay", err) - - return - } - - task, err := createReplayDelivery( - webhookDB, webhook.ID, &event, target, + deliveryID, err := h.engine.CreateReplay( + webhook.ID, original.EventID, target, ) if err != nil { h.serverError( @@ -193,15 +182,14 @@ func (h *Handlers) queueReplay( } h.mtr.DeliveryReplayed(target.Type) - h.notifier.Notify([]delivery.Task{task}) h.log.Info( "delivery replay queued", "webhook_id", webhook.ID, - "event_id", event.ID, + "event_id", original.EventID, "target_id", target.ID, "replayed_delivery_id", original.ID, - "delivery_id", task.DeliveryID, + "delivery_id", deliveryID, ) redirectToEventLog(w, r, webhook, replayQueued) @@ -263,77 +251,6 @@ func countInFlightDeliveries( return count, err } -// createReplayDelivery writes the new pending delivery row, adds it to -// its target's totals in the same transaction, and returns the task -// that carries it to the delivery engine. -// -// The row is written with associations omitted, and neither Event nor -// Target is populated on it: GORM's SaveBeforeAssociations would -// otherwise upsert the whole target row — plaintext config, which for a -// Slack target is the credential — into the per-webhook event database. -// See https://git.eeqj.de/sneak/webhooker/issues/206. -func createReplayDelivery( - webhookDB *gorm.DB, - webhookID string, - event *database.Event, - target *database.Target, -) (delivery.Task, error) { - dlv := &database.Delivery{ - EventID: event.ID, - TargetID: target.ID, - Status: database.DeliveryStatusPending, - Replay: true, - } - - err := webhookDB.Transaction(func(tx *gorm.DB) error { - err := tx.Omit(clause.Associations).Create(dlv).Error - if err != nil { - return err - } - - return database.AddTargetTotals(tx, database.TargetTotals{ - TargetID: dlv.TargetID, Deliveries: 1, - }) - }) - if err != nil { - return delivery.Task{}, err - } - - return delivery.Task{ - DeliveryID: dlv.ID, - EventID: event.ID, - WebhookID: webhookID, - EntrypointID: event.EntrypointID, - TargetID: target.ID, - TargetName: target.Name, - TargetType: target.Type, - TargetURL: target.URL, - TargetHeaders: target.Headers, - TimeoutSeconds: target.TimeoutSeconds, - ForwardQuery: target.ForwardQuery, - ArchiveExpiry: target.ArchiveExpiry, - ArchiveRotation: target.ArchiveRotation, - MaxAttempts: target.MaxAttempts, - Method: event.Method, - RawQuery: event.RawQuery, - Headers: event.Headers, - ContentType: event.ContentType, - Body: replayBody(event.Body), - AttemptNum: 1, - }, nil -} - -// replayBody returns the stored event body for a replay task to carry -// inline, or nil when it is large enough that the engine should fetch -// it from the per-webhook database instead. -func replayBody(body string) *string { - if len(body) >= delivery.MaxInlineBodySize { - return nil - } - - return &body -} - // redirectToEventLog redirects a replay or resubmit back to the event // log it was triggered from, carrying the outcome as its notice. A // Replay form carries the list it was pressed in as show, so a replay diff --git a/internal/handlers/delivery_replay_test.go b/internal/handlers/delivery_replay_test.go index a4dd6d2..56780fb 100644 --- a/internal/handlers/delivery_replay_test.go +++ b/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, ¬if) + 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, ¬if) + 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, ¬if) + 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, ¬if) + 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. diff --git a/internal/handlers/event_resubmit.go b/internal/handlers/event_resubmit.go index 211ac00..8ca9c64 100644 --- a/internal/handlers/event_resubmit.go +++ b/internal/handlers/event_resubmit.go @@ -171,37 +171,29 @@ func loadResubmitSource( return src, true, nil } -// queueResubmit stores the copy and fans it out to the webhook's -// active targets. +// queueResubmit has the delivery engine store the copy, with a delivery +// 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, webhook database.Webhook, src resubmitSource, ) { - // The receiver's own query, run now: an active target created - // after the original event arrived is included, and an - // inactive one is skipped rather than refused. - targets, err := h.loadActiveTargets(webhook.ID) - if err != nil { - h.serverError(w, r, "failed to query targets", err) - - return + event := &database.Event{ + WebhookID: webhook.ID, + EntrypointID: src.EntrypointID, + Method: src.Method, + RawQuery: src.RawQuery, + Headers: src.Headers, + ContentType: src.ContentType, + Body: string(src.Body), + ResubmittedFromID: &src.ID, } - event, tasks, err := h.createAndFanOut( - eventSource{ - WebhookID: webhook.ID, - EntrypointID: src.EntrypointID, - Method: src.Method, - RawQuery: src.RawQuery, - HeadersJSON: src.Headers, - ContentType: src.ContentType, - Body: src.Body, - ResubmittedFromID: &src.ID, - }, - targets, - ) + deliveries, err := h.engine.StoreEvent(event) if err != nil { h.serverError(w, r, "failed to store resubmitted event", err) @@ -215,11 +207,11 @@ func (h *Handlers) queueResubmit( "webhook_id", webhook.ID, "event_id", event.ID, "resubmitted_from_id", src.ID, - "target_count", len(tasks), + "target_count", deliveries, ) code := resubmitQueued - if len(tasks) == 0 { + if deliveries == 0 { code = resubmitNoTargets } diff --git a/internal/handlers/event_resubmit_test.go b/internal/handlers/event_resubmit_test.go index da48546..9266a30 100644 --- a/internal/handlers/event_resubmit_test.go +++ b/internal/handlers/event_resubmit_test.go @@ -19,7 +19,7 @@ import ( // resubmitTargetURL 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 resubmitTargetURL = "http://93.184.216.34/hook" // resubmitEventHeaders and resubmitEventQuery are the stored header @@ -134,10 +134,9 @@ func TestHandleEventResubmit_DeliversToTargetCreatedAfterTheEvent( sess *session.Session db *database.Database dbMgr *database.WebhookDBManager - notif *recordingNotifier ) - app := newTestApp(t, &h, &sess, &db, &dbMgr, ¬if) + app := newTestApp(t, &h, &sess, &db, &dbMgr) app.RequireStart() t.Cleanup(app.RequireStop) @@ -189,11 +188,6 @@ func TestHandleEventResubmit_DeliversToTargetCreatedAfterTheEvent( t, database.DeliveryStatusPending, deliveries[0].Status, ) - tasks := notif.Tasks() - require.Len(t, tasks, 1) - assert.Equal(t, deliveries[0].ID, tasks[0].DeliveryID) - assertResubmitTask(t, tasks[0], wh.ID, &fresh, tgt) - assertNoLeakedTarget(t, webhookDB) } @@ -225,32 +219,6 @@ func assertEventCopy( ) } -// assertResubmitTask proves the task handed to the delivery engine is -// the one the receiver would build for the NEW event and this target. -func assertResubmitTask( - t *testing.T, - task delivery.Task, - webhookID string, - fresh *database.Event, - target *database.Target, -) { - t.Helper() - - assert.Equal(t, fresh.ID, task.EventID) - assert.Equal(t, webhookID, task.WebhookID) - assert.Equal(t, fresh.EntrypointID, task.EntrypointID) - assert.Equal(t, target.ID, task.TargetID) - assert.Equal(t, target.Type, task.TargetType) - assert.Equal(t, fresh.Method, task.Method) - assert.Equal(t, fresh.RawQuery, task.RawQuery) - assert.Equal(t, fresh.Headers, task.Headers) - assert.Equal(t, fresh.ContentType, task.ContentType) - assert.Equal(t, 1, task.AttemptNum) - - require.NotNil(t, task.Body) - assert.Equal(t, fresh.Body, *task.Body) -} - // TestHandleEventResubmit_IsRepeatable proves the requirement replay // deliberately does not meet: firing the same captured event at a // backend over and over must work, with no in-flight refusal, and each @@ -263,10 +231,9 @@ func TestHandleEventResubmit_IsRepeatable(t *testing.T) { sess *session.Session db *database.Database dbMgr *database.WebhookDBManager - notif *recordingNotifier ) - app := newTestApp(t, &h, &sess, &db, &dbMgr, ¬if) + app := newTestApp(t, &h, &sess, &db, &dbMgr) app.RequireStart() t.Cleanup(app.RequireStop) @@ -300,30 +267,18 @@ func TestHandleEventResubmit_IsRepeatable(t *testing.T) { require.NoError(t, err) events := listEvents(t, webhookDB) - require.Len(t, events, presses+1) + require.Len( + t, events, presses+1, "each resubmit creates its own event", + ) - tasks := notif.Tasks() - require.Len(t, tasks, presses) + assert.Empty( + t, listDeliveries(t, webhookDB, original.ID), + "each resubmit delivers its own new event", + ) - seen := make(map[string]struct{}, presses) - - for _, task := range tasks { - assert.Equal(t, tgt.ID, task.TargetID) - assert.NotEqual( - t, original.ID, task.EventID, - "each resubmit delivers its own new event", - ) - - _, dup := seen[task.EventID] - assert.False(t, dup, "each resubmit creates its own event") - - seen[task.EventID] = struct{}{} - - require.Len(t, listDeliveries(t, webhookDB, task.EventID), 1) - } - - // Every copy names the same source event, so twenty presses stay - // traceable to the one captured request. + // Every copy has its own delivery, and names the same source + // event, so twenty presses stay traceable to the one captured + // request. for _, e := range events { if e.ID == original.ID { continue @@ -331,6 +286,10 @@ func TestHandleEventResubmit_IsRepeatable(t *testing.T) { require.NotNil(t, e.ResubmittedFromID) assert.Equal(t, original.ID, *e.ResubmittedFromID) + + deliveries := listDeliveries(t, webhookDB, e.ID) + require.Len(t, deliveries, 1) + assert.Equal(t, tgt.ID, deliveries[0].TargetID) } } @@ -346,10 +305,9 @@ func TestHandleEventResubmit_OversizeBodySurvivesIntact(t *testing.T) { sess *session.Session db *database.Database dbMgr *database.WebhookDBManager - notif *recordingNotifier ) - app := newTestApp(t, &h, &sess, &db, &dbMgr, ¬if) + app := newTestApp(t, &h, &sess, &db, &dbMgr) app.RequireStart() t.Cleanup(app.RequireStop) @@ -385,22 +343,7 @@ func TestHandleEventResubmit_OversizeBodySurvivesIntact(t *testing.T) { t, stored, fresh.Body, "the stored body must be copied byte for byte", ) - - tasks := notif.Tasks() - require.Len(t, tasks, 1) - assert.Nil( - t, tasks[0].Body, - "a body over the inline limit is fetched from the new "+ - "event row rather than carried on the task", - ) - - // The engine's own read of the body, against the new event id: - // what it would send is what was stored. - var loaded database.Event - - require.NoError(t, webhookDB.Select("body"). - First(&loaded, "id = ?", tasks[0].EventID).Error) - assert.Equal(t, stored, loaded.Body) + assert.Len(t, listDeliveries(t, webhookDB, fresh.ID), 1) } // TestHandleEventResubmit_SkipsInactiveTarget proves a deactivated @@ -414,10 +357,9 @@ func TestHandleEventResubmit_SkipsInactiveTarget(t *testing.T) { sess *session.Session db *database.Database dbMgr *database.WebhookDBManager - notif *recordingNotifier ) - app := newTestApp(t, &h, &sess, &db, &dbMgr, ¬if) + app := newTestApp(t, &h, &sess, &db, &dbMgr) app.RequireStart() t.Cleanup(app.RequireStop) @@ -448,21 +390,19 @@ func TestHandleEventResubmit_SkipsInactiveTarget(t *testing.T) { "an inactive target is skipped, not an error", ) - tasks := notif.Tasks() - require.Len(t, tasks, 1) - assert.Equal(t, active.ID, tasks[0].TargetID) - webhookDB, err := dbMgr.GetDB(wh.ID) require.NoError(t, err) fresh := theOtherEvent(t, listEvents(t, webhookDB), original.ID) - require.Len(t, listDeliveries(t, webhookDB, fresh.ID), 1) + deliveries := listDeliveries(t, webhookDB, fresh.ID) + require.Len(t, deliveries, 1) + assert.Equal(t, active.ID, deliveries[0].TargetID) } // TestHandleEventResubmit_NoActiveTargetsStillStoresEvent proves a // source with nothing to deliver to behaves as the receiver does: the -// event is stored, nothing is queued, and the operator is told so -// rather than being shown an error. +// event is stored with no delivery, and the operator is told so rather +// than being shown an error. func TestHandleEventResubmit_NoActiveTargetsStillStoresEvent( t *testing.T, ) { @@ -473,10 +413,9 @@ func TestHandleEventResubmit_NoActiveTargetsStillStoresEvent( sess *session.Session db *database.Database dbMgr *database.WebhookDBManager - notif *recordingNotifier ) - app := newTestApp(t, &h, &sess, &db, &dbMgr, ¬if) + app := newTestApp(t, &h, &sess, &db, &dbMgr) app.RequireStart() t.Cleanup(app.RequireStop) @@ -493,12 +432,14 @@ func TestHandleEventResubmit_NoActiveTargetsStillStoresEvent( w.Header().Get("Location"), ) - assert.Empty(t, notif.Tasks()) - webhookDB, err := dbMgr.GetDB(wh.ID) require.NoError(t, err) - assert.Len(t, listEvents(t, webhookDB), 2) + events := listEvents(t, webhookDB) + assert.Len(t, events, 2) + + fresh := theOtherEvent(t, events, original.ID) + assert.Empty(t, listDeliveries(t, webhookDB, fresh.ID)) } // TestHandleEventResubmit_RefusesEventOfAnotherWebhook proves the @@ -514,10 +455,9 @@ func TestHandleEventResubmit_RefusesEventOfAnotherWebhook( sess *session.Session db *database.Database dbMgr *database.WebhookDBManager - notif *recordingNotifier ) - app := newTestApp(t, &h, &sess, &db, &dbMgr, ¬if) + app := newTestApp(t, &h, &sess, &db, &dbMgr) app.RequireStart() t.Cleanup(app.RequireStop) @@ -548,10 +488,49 @@ func TestHandleEventResubmit_RefusesEventOfAnotherWebhook( w = postResubmit(t, h, sess, mine.ID, "not-a-uuid") assert.Equal(t, http.StatusNotFound, w.Code) - assert.Empty( - t, notif.Tasks(), - "a refused resubmit must queue nothing", + for _, webhookID := range []string{theirs.ID, mine.ID} { + webhookDB, err := dbMgr.GetDB(webhookID) + require.NoError(t, err) + + assert.Len( + t, listEvents(t, webhookDB), 1, + "a refused resubmit must store nothing", + ) + } +} + +// TestHandleEventResubmit_FailedStoreIsAnError proves a copy the engine +// cannot store answers with the 500 error page and leaves no new event. +func TestHandleEventResubmit_FailedStoreIsAnError(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) + original := seedStoredEvent(t, dbMgr, wh.ID, `{"not":"stored"}`) + + // Without its event totals table the webhook's database refuses + // the totals write, which comes after the event row. + webhookDB, err := dbMgr.GetDB(wh.ID) + require.NoError(t, err) + require.NoError(t, + webhookDB.Migrator().DropTable(&database.EventTotals{})) + + w := postResubmit(t, h, sess, wh.ID, original.ID) + + assert.Equal(t, http.StatusInternalServerError, w.Code) + assert.Contains(t, w.Body.String(), "500 Internal Server Error") + assert.Len(t, listEvents(t, webhookDB), 1) } // TestHandleSourceLogs_ShowsResubmitProvenance proves the event log diff --git a/internal/handlers/handlers.go b/internal/handlers/handlers.go index 0770809..002033e 100644 --- a/internal/handlers/handlers.go +++ b/internal/handlers/handlers.go @@ -64,7 +64,7 @@ type HandlersParams struct { Healthcheck *healthcheck.Healthcheck Session *session.Session Middleware *middleware.Middleware - Notifier delivery.Notifier + Engine *delivery.Engine Archives archive.Archives CircuitBreakers delivery.CircuitBreakers SSRFGuard *delivery.Guard @@ -82,7 +82,7 @@ type Handlers struct { dbMgr *database.WebhookDBManager session *session.Session mw *middleware.Middleware - notifier delivery.Notifier + engine *delivery.Engine archives archive.Archives breakers delivery.CircuitBreakers mtr *metrics.Set @@ -138,7 +138,7 @@ func New( s.dbMgr = params.WebhookDBMgr s.session = params.Session s.mw = params.Middleware - s.notifier = params.Notifier + s.engine = params.Engine s.archives = params.Archives s.breakers = params.CircuitBreakers s.mtr = params.Metrics diff --git a/internal/handlers/handlers_test.go b/internal/handlers/handlers_test.go index 3d476de..fc7b9d1 100644 --- a/internal/handlers/handlers_test.go +++ b/internal/handlers/handlers_test.go @@ -30,33 +30,6 @@ import ( "sneak.berlin/go/webhooker/internal/session" ) -// recordingNotifier is a delivery.Notifier that records the tasks it -// was handed, so a test can prove a handler queued the delivery it -// claims to have queued — and, on the refusal paths, that it queued -// nothing. -type recordingNotifier struct { - mu sync.Mutex - tasks []delivery.Task -} - -func (n *recordingNotifier) Notify(tasks []delivery.Task) { - n.mu.Lock() - defer n.mu.Unlock() - - n.tasks = append(n.tasks, tasks...) -} - -// Tasks returns a copy of the recorded tasks. -func (n *recordingNotifier) Tasks() []delivery.Task { - n.mu.Lock() - defer n.mu.Unlock() - - out := make([]delivery.Task, len(n.tasks)) - copy(out, n.tasks) - - return out -} - // recordingArchives is an archive.Archives that records what it // was asked to do, so a test can prove that a deletion or rename // path reached the archive writers. After FailRenames, every @@ -264,11 +237,12 @@ func newTestAppWithConfig( database.NewWebhookDBManager, healthcheck.New, session.New, - func() *recordingNotifier { - return &recordingNotifier{} - }, - func(n *recordingNotifier) delivery.Notifier { - return n + archive.NewWriters, + // The engine is built on a lifecycle that is never started, + // 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) }, func() *recordingArchives { return &recordingArchives{} diff --git a/internal/handlers/webhook.go b/internal/handlers/webhook.go index de1cd6f..3af8976 100644 --- a/internal/handlers/webhook.go +++ b/internal/handlers/webhook.go @@ -2,14 +2,11 @@ package handlers import ( "encoding/json" - "fmt" "io" "net/http" "github.com/go-chi/chi" - "gorm.io/gorm" "sneak.berlin/go/webhooker/internal/database" - "sneak.berlin/go/webhooker/internal/delivery" "sneak.berlin/go/webhooker/internal/middleware" ) @@ -70,8 +67,9 @@ func (h *Handlers) HandleWebhook() http.HandlerFunc { } } -// processWebhookRequest reads the body, serializes headers, loads -// targets, and delivers the event. +// processWebhookRequest reads the request into an event and has the +// delivery engine store it, with a delivery for each of the webhook's +// active targets, and answers the sender. // // Nothing about the request itself is authenticated: the entrypoint // UUID in the path is the credential, and reaching here means it @@ -93,30 +91,24 @@ func (h *Handlers) processWebhookRequest( return } - targets, err := h.loadActiveTargets(entrypoint.WebhookID) + event := &database.Event{ + WebhookID: entrypoint.WebhookID, + EntrypointID: entrypoint.ID, + Method: r.Method, + RawQuery: r.URL.RawQuery, + Headers: string(headersJSON), + ContentType: r.Header.Get("Content-Type"), + Body: string(body), + } + + deliveries, err := h.engine.StoreEvent(event) if err != nil { - h.receiverError(w, "failed to query targets", err) + h.receiverError(w, "failed to store webhook event", err) return } - h.createAndDeliverEvent( - w, r, entrypoint, body, headersJSON, targets, - ) -} - -// loadActiveTargets returns all active targets for a webhook. -func (h *Handlers) loadActiveTargets( - webhookID string, -) ([]database.Target, error) { - var targets []database.Target - - err := h.db.DB().Where( - "webhook_id = ? AND active = ?", - webhookID, true, - ).Find(&targets).Error - - return targets, err + h.finishWebhookResponse(w, event, entrypoint, deliveries) } // lookupEntrypoint finds an entrypoint by UUID path. @@ -177,28 +169,6 @@ func (h *Handlers) readWebhookBody( return body, true } -// createAndDeliverEvent stores the received event, fans it out to the -// webhook's targets, and answers the sender. -func (h *Handlers) createAndDeliverEvent( - w http.ResponseWriter, - r *http.Request, - entrypoint database.Entrypoint, - body, headersJSON []byte, - targets []database.Target, -) { - event, tasks, err := h.createAndFanOut( - requestEventSource(r, entrypoint, headersJSON, body), - targets, - ) - if err != nil { - h.receiverError(w, "failed to store webhook event", err) - - return - } - - h.finishWebhookResponse(w, event, entrypoint, tasks) -} - // receiverError logs an error and answers the sender with a plain-text // 500. The receiver's answers are for programs, so it never sends the // error page the web UI uses. @@ -212,172 +182,19 @@ func (h *Handlers) receiverError( ) } -// eventSource carries the fields a new event is built from. The -// receiver fills it from the live request; the resubmit handler fills -// it from a stored event. Both then go through createAndFanOut, so an -// event is constructed and fanned out in one place however it entered -// the system. -type eventSource struct { - WebhookID string - EntrypointID string - Method string - RawQuery string - HeadersJSON string - ContentType string - Body []byte - - // ResubmittedFromID names the event this one copies. Only the - // resubmit path sets it. - ResubmittedFromID *string -} - -// event builds the row this source stores. -func (s eventSource) event() *database.Event { - return &database.Event{ - WebhookID: s.WebhookID, - EntrypointID: s.EntrypointID, - Method: s.Method, - RawQuery: s.RawQuery, - Headers: s.HeadersJSON, - Body: string(s.Body), - BodyBytes: int64(len(s.Body)), - ContentType: s.ContentType, - ResubmittedFromID: s.ResubmittedFromID, - } -} - -// requestEventSource describes the event a live receiver request -// stores. -func requestEventSource( - r *http.Request, - entrypoint database.Entrypoint, - headersJSON, body []byte, -) eventSource { - return eventSource{ - WebhookID: entrypoint.WebhookID, - EntrypointID: entrypoint.ID, - Method: r.Method, - RawQuery: r.URL.RawQuery, - HeadersJSON: string(headersJSON), - ContentType: r.Header.Get("Content-Type"), - Body: body, - } -} - -// createAndFanOut writes the event and one pending delivery per target, -// and adds them to the webhook's running totals, in a single -// transaction, then hands the tasks to the delivery engine. Every -// event is created here, received or resubmitted, so a resubmitted -// event is retried, SSRF-guarded and circuit-broken exactly as a -// received one is. Per-delivery replay is the one other path that -// creates a delivery: it adds one to an existing event without -// coming through here. -// -// The tasks are returned as well as queued, so a caller can report how -// many targets the event went to. -func (h *Handlers) createAndFanOut( - src eventSource, - targets []database.Target, -) (*database.Event, []delivery.Task, error) { - webhookDB, err := h.dbMgr.GetDB(src.WebhookID) - if err != nil { - return nil, nil, fmt.Errorf( - "getting webhook database: %w", err, - ) - } - - tx := webhookDB.Begin() - if tx.Error != nil { - return nil, nil, fmt.Errorf( - "beginning transaction: %w", tx.Error, - ) - } - - event := src.event() - - err = tx.Create(event).Error - if err != nil { - tx.Rollback() - - return nil, nil, fmt.Errorf("creating event: %w", err) - } - - tasks, err := buildDeliveryTasks( - tx, event, targets, inlineBody(src.Body), - ) - if err != nil { - tx.Rollback() - - return nil, nil, err - } - - err = database.AddEventTotals(tx, database.EventTotals{ - Events: 1, LastEventAt: &event.CreatedAt, - }) - if err != nil { - tx.Rollback() - - return nil, nil, err - } - - // A resubmitted copy did not arrive on its entrypoint's URL, so it - // leaves the entrypoint's last event as it is. - if src.ResubmittedFromID == nil { - err = database.AddEntrypointTotals(tx, database.EntrypointTotals{ - EntrypointID: event.EntrypointID, LastEventAt: event.CreatedAt, - }) - if err != nil { - tx.Rollback() - - return nil, nil, err - } - } - - err = tx.Commit().Error - if err != nil { - return nil, nil, fmt.Errorf( - "committing transaction: %w", 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. - h.mtr.EventReceived() - - if len(tasks) > 0 { - h.notifier.Notify(tasks) - } - - return event, tasks, nil -} - -// inlineBody returns a pointer to body as a string if it fits -// within the inline size limit, or nil otherwise. -func inlineBody(body []byte) *string { - if len(body) < delivery.MaxInlineBodySize { - s := string(body) - - return &s - } - - return nil -} - -// finishWebhookResponse logs the event and writes the HTTP response. +// finishWebhookResponse logs the stored event, with how many +// deliveries it has, and writes the HTTP response. func (h *Handlers) finishWebhookResponse( w http.ResponseWriter, event *database.Event, entrypoint database.Entrypoint, - tasks []delivery.Task, + deliveries int, ) { h.log.Info("webhook event created", "event_id", event.ID, "webhook_id", entrypoint.WebhookID, "entrypoint_id", entrypoint.ID, - "target_count", len(tasks), + "target_count", deliveries, ) w.WriteHeader(http.StatusOK) @@ -389,64 +206,3 @@ func (h *Handlers) finishWebhookResponse( ) } } - -// buildDeliveryTasks creates one pending delivery per target in the -// transaction, adds each to its target's totals, and returns the tasks -// for the delivery engine. The caller owns the transaction and rolls -// it back on error. -func buildDeliveryTasks( - tx *gorm.DB, - event *database.Event, - targets []database.Target, - bodyPtr *string, -) ([]delivery.Task, error) { - tasks := make([]delivery.Task, 0, len(targets)) - - for i := range targets { - dlv := &database.Delivery{ - EventID: event.ID, - TargetID: targets[i].ID, - Status: database.DeliveryStatusPending, - } - - err := tx.Create(dlv).Error - if err != nil { - return nil, fmt.Errorf( - "creating delivery for target %s: %w", - targets[i].ID, err, - ) - } - - err = database.AddTargetTotals(tx, database.TargetTotals{ - TargetID: targets[i].ID, Deliveries: 1, - }) - if err != nil { - return nil, err - } - - tasks = append(tasks, delivery.Task{ - DeliveryID: dlv.ID, - EventID: event.ID, - WebhookID: event.WebhookID, - EntrypointID: event.EntrypointID, - TargetID: targets[i].ID, - TargetName: targets[i].Name, - TargetType: targets[i].Type, - TargetURL: targets[i].URL, - TargetHeaders: targets[i].Headers, - TimeoutSeconds: targets[i].TimeoutSeconds, - ForwardQuery: targets[i].ForwardQuery, - ArchiveExpiry: targets[i].ArchiveExpiry, - ArchiveRotation: targets[i].ArchiveRotation, - MaxAttempts: targets[i].MaxAttempts, - Method: event.Method, - RawQuery: event.RawQuery, - Headers: event.Headers, - ContentType: event.ContentType, - Body: bodyPtr, - AttemptNum: 1, - }) - } - - return tasks, nil -} diff --git a/internal/handlers/webhook_test.go b/internal/handlers/webhook_test.go index f5028d9..4d6088e 100644 --- a/internal/handlers/webhook_test.go +++ b/internal/handlers/webhook_test.go @@ -10,7 +10,6 @@ import ( "net/netip" "strings" "testing" - "time" "github.com/go-chi/chi" "github.com/google/uuid" @@ -19,7 +18,6 @@ import ( "gorm.io/gorm/clause" "sneak.berlin/go/webhooker/internal/config" "sneak.berlin/go/webhooker/internal/database" - "sneak.berlin/go/webhooker/internal/delivery" "sneak.berlin/go/webhooker/internal/handlers" "sneak.berlin/go/webhooker/internal/middleware" "sneak.berlin/go/webhooker/internal/middleware/middlewaretest" @@ -209,50 +207,19 @@ func receivedLine(t *testing.T, buf *bytes.Buffer) map[string]any { return found[0] } -// targetWithEverySetting returns target settings with all six set, the -// archive ones included, so a task that drops one of them differs from -// the target. -func targetWithEverySetting() database.Target { - return database.Target{ - URL: replayTargetURL, - Headers: map[string]string{"X-Api-Key": "k"}, - TimeoutSeconds: 7, - ForwardQuery: true, - ArchiveExpiry: time.Hour, - ArchiveRotation: "daily", - } -} - -// assertTaskCarriesTargetSettings proves a delivery task carries every -// one of the target's settings. They are copied into the task one field -// at a time, and a field left out is sent empty without any error. -func assertTaskCarriesTargetSettings( - t *testing.T, task delivery.Task, target *database.Target, -) { - t.Helper() - - assert.Equal(t, target.ID, task.TargetID) - 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) -} - -// TestHandleWebhook_TaskCarriesEveryTargetSetting proves the task a -// received event hands the delivery engine carries every one of the -// target's settings. -func TestHandleWebhook_TaskCarriesEveryTargetSetting(t *testing.T) { +// TestHandleWebhook_StoresTheEvent proves an accepted request is +// answered with the receiver's 200 and body, and that the event is +// stored with a pending delivery to the webhook's active target. +func TestHandleWebhook_StoresTheEvent(t *testing.T) { t.Parallel() var ( h *handlers.Handlers db *database.Database - notif *recordingNotifier + dbMgr *database.WebhookDBManager ) - app := newTestApp(t, &h, &db, ¬if) + app := newTestApp(t, &h, &db, &dbMgr) app.RequireStart() t.Cleanup(app.RequireStop) @@ -261,7 +228,7 @@ func TestHandleWebhook_TaskCarriesEveryTargetSetting(t *testing.T) { entrypoint := seedEntrypoint(t, db, webhook.ID) target := seedConfiguredTarget( t, db, webhook.ID, database.TargetTypeHTTP, - targetWithEverySetting(), + database.Target{URL: replayTargetURL}, ) w := httptest.NewRecorder() @@ -271,8 +238,55 @@ func TestHandleWebhook_TaskCarriesEveryTargetSetting(t *testing.T) { )) require.Equal(t, http.StatusOK, w.Code) + assert.JSONEq(t, `{"status":"ok"}`, w.Body.String()) - tasks := notif.Tasks() - require.Len(t, tasks, 1) - assertTaskCarriesTargetSettings(t, tasks[0], target) + webhookDB, err := dbMgr.GetDB(webhook.ID) + require.NoError(t, err) + + events := listEvents(t, webhookDB) + require.Len(t, events, 1) + assert.Equal(t, entrypoint.ID, events[0].EntrypointID) + + deliveries := listDeliveries(t, webhookDB, events[0].ID) + require.Len(t, deliveries, 1) + assert.Equal(t, target.ID, deliveries[0].TargetID) + assert.Equal(t, database.DeliveryStatusPending, deliveries[0].Status) +} + +// TestHandleWebhook_FailedStoreAnswersPlainError proves that an event +// the engine cannot store is answered with the receiver's plain-text +// 500, and that nothing of it is left stored. +func TestHandleWebhook_FailedStoreAnswersPlainError(t *testing.T) { + t.Parallel() + + var ( + h *handlers.Handlers + db *database.Database + dbMgr *database.WebhookDBManager + ) + + app := newTestApp(t, &h, &db, &dbMgr) + app.RequireStart() + + t.Cleanup(app.RequireStop) + + webhook := seedWebhook(t, db) + entrypoint := seedEntrypoint(t, db, webhook.ID) + + // Without its event totals table the webhook's database refuses + // the totals write, which comes after the event row. + webhookDB, err := dbMgr.GetDB(webhook.ID) + require.NoError(t, err) + require.NoError(t, + webhookDB.Migrator().DropTable(&database.EventTotals{})) + + w := httptest.NewRecorder() + h.HandleWebhook().ServeHTTP(w, postRequest( + "/h/"+entrypoint.Path, nil, + map[string]string{"uuid": entrypoint.Path}, + )) + + assert.Equal(t, http.StatusInternalServerError, w.Code) + assert.Equal(t, "Internal server error\n", w.Body.String()) + assert.Empty(t, listEvents(t, webhookDB)) } diff --git a/internal/resetpw/resetpw_test.go b/internal/resetpw/resetpw_test.go index 490ce3a..7b67b52 100644 --- a/internal/resetpw/resetpw_test.go +++ b/internal/resetpw/resetpw_test.go @@ -131,10 +131,6 @@ func run( return code, stdout.String(), stderr.String() } -type noopNotifier struct{} - -func (n *noopNotifier) Notify([]delivery.Task) {} - type noopArchives struct { renameMu sync.Mutex } @@ -190,7 +186,12 @@ func newServerApp( database.NewWebhookDBManager, healthcheck.New, session.New, - func() delivery.Notifier { return &noopNotifier{} }, + archive.NewWriters, + // The engine is built on a lifecycle that is never started, + // so no worker runs. + func(p delivery.EngineParams) *delivery.Engine { + return delivery.New(fxtest.NewLifecycle(t), p) + }, func() archive.Archives { return &noopArchives{} }, func() delivery.CircuitBreakers { return &noopCircuitBreakers{} diff --git a/internal/server/routes_test.go b/internal/server/routes_test.go index b2ff6d3..e8f3965 100644 --- a/internal/server/routes_test.go +++ b/internal/server/routes_test.go @@ -48,10 +48,6 @@ const ( metricsAuthValue = "s3cret" ) -type noopNotifier struct{} - -func (n *noopNotifier) Notify([]delivery.Task) {} - // noopArchives satisfies handlers.New's archive.Archives // dependency. No test here checks what gets evicted or renamed, so // it records nothing. Its rename lock is a plain mutex. @@ -145,7 +141,12 @@ func newTestEnvWithConfig( database.NewWebhookDBManager, healthcheck.New, session.New, - func() delivery.Notifier { return &noopNotifier{} }, + archive.NewWriters, + // The engine is built on a lifecycle that is never started, + // so no worker runs and nothing is sent anywhere. + func(p delivery.EngineParams) *delivery.Engine { + return delivery.New(fxtest.NewLifecycle(t), p) + }, func() archive.Archives { return &noopArchives{} }, func() delivery.CircuitBreakers { return &noopCircuitBreakers{}