Refactor delivery targets to a Target interface #77

New Issue

clawbot · 2026-08-07T15:05:53+02:00

clawbot commented

2026-08-07 15:05:53 +02:00

Part of the road to 1.0 (see #33).

Today the delivery engine dispatches by target type via methods on Engine (deliverHTTP, deliverSlack, deliverDatabase, deliverLog) selected from Target.Type, rather than a polymorphic target abstraction. Per @sneak, targets should be an interface, with each target type an implementation of it.

Design (per @sneak — see #77 (comment)):

Define a Target interface in internal/delivery. Each target type — http, slack, database, log — is an implementation.
The target owns its own delivery, including retries and backoff. We have both fire-and-forget targets (no retries) and retry targets (retry with backoff, as required). The retry/backoff policy AND its execution belong to the target implementation, not the engine: logTarget and databaseTarget are fire-and-forget (deliver once), while httpTarget and slackTarget retry with backoff (and the circuit breaker) as they do today.
The engine dispatches to the target via a registry (a map[database.TargetType]Target or a small factory) instead of a type switch, and remains responsible only for what is genuinely cross-target: pulling work off the queue, handing each delivery to the right target, and persisting the DeliveryResult / DeliveryStatus the target reports.
logTarget behaviour: log the entire inbound webhook — the full request body and request headers, plus method, content type, and the entrypoint/webhook ids — not just a summary line. This supersedes the smaller #70; that work folds into this implementation.

Definition of done:

a Target interface exists, and http, slack, database, log are implementations of it
each target owns its retry/backoff behaviour: fire-and-forget targets deliver once; retry targets retry with backoff (and circuit breaker) exactly as today
the engine dispatches via the interface/registry, not a type switch on TargetType
the log target logs the full inbound content and headers
delivery semantics are otherwise unchanged; existing delivery tests pass (adjusted only where behaviour genuinely moved into a target); docker build . green
no new external behaviour beyond the log-target content change; database archiving stays #43 and lands as the databaseTarget implementation

Sequencing: this reshapes internal/delivery/engine.go, so it should land after the in-flight engine work merges and before #43.

Part of the road to 1.0 (see #33). Today the delivery engine dispatches by target type via methods on `Engine` (`deliverHTTP`, `deliverSlack`, `deliverDatabase`, `deliverLog`) selected from `Target.Type`, rather than a polymorphic target abstraction. Per @sneak, targets should be an interface, with each target type an implementation of it. Design (per @sneak — see https://git.eeqj.de/sneak/webhooker/issues/77#issuecomment-43488): - Define a `Target` interface in `internal/delivery`. Each target type — `http`, `slack`, `database`, `log` — is an implementation. - The target owns its own delivery, including retries and backoff. We have both fire-and-forget targets (no retries) and retry targets (retry with backoff, as required). The retry/backoff policy AND its execution belong to the target implementation, not the engine: `logTarget` and `databaseTarget` are fire-and-forget (deliver once), while `httpTarget` and `slackTarget` retry with backoff (and the circuit breaker) as they do today. - The engine dispatches to the target via a registry (a `map[database.TargetType]Target` or a small factory) instead of a type switch, and remains responsible only for what is genuinely cross-target: pulling work off the queue, handing each delivery to the right target, and persisting the `DeliveryResult` / `DeliveryStatus` the target reports. - `logTarget` behaviour: log the entire inbound webhook — the full request body and request headers, plus method, content type, and the entrypoint/webhook ids — not just a summary line. This supersedes the smaller #70; that work folds into this implementation. Definition of done: - a `Target` interface exists, and `http`, `slack`, `database`, `log` are implementations of it - each target owns its retry/backoff behaviour: fire-and-forget targets deliver once; retry targets retry with backoff (and circuit breaker) exactly as today - the engine dispatches via the interface/registry, not a type switch on `TargetType` - the `log` target logs the full inbound content and headers - delivery semantics are otherwise unchanged; existing delivery tests pass (adjusted only where behaviour genuinely moved into a target); `docker build .` green - no new external behaviour beyond the log-target content change; database archiving stays #43 and lands as the `databaseTarget` implementation Sequencing: this reshapes `internal/delivery/engine.go`, so it should land after the in-flight engine work merges and before #43.

clawbot added this to the 1.0.0 milestone 2026-08-07 15:05:53 +02:00

sneak commented

2026-08-07 15:08:14 +02:00

No, we have fire-and-forget targets as well as retry targets. The target itself should be responsible for retries and backoff (as required). The log target should log the entire inbound webhook content and request headers etc.

clawbot commented

2026-08-07 15:11:23 +02:00

Updated the design above to match:

each target owns its own retries/backoff — fire-and-forget targets (log, database) deliver once; http/slack retry with backoff and the circuit breaker as today. The engine just dispatches via a registry and persists the result the target reports.
the log target now logs the full inbound body and headers, which supersedes #70 — that work folds in here.

Because this relocates the retry loop and circuit breaker into the target implementations, it is a non-trivial rewrite of engine.go. Before implementing, I'll post a concrete interface proposal here — the exact Target signature and how target-owned retries sit against the current queue/scheduler — for your sign-off. I'll sequence the rewrite after the in-flight engine.go work (#70 / PR #76) is resolved, since it touches the same file.

Updated the design above to match: - each target owns its own retries/backoff — fire-and-forget targets (`log`, `database`) deliver once; `http`/`slack` retry with backoff and the circuit breaker as today. The engine just dispatches via a registry and persists the result the target reports. - the `log` target now logs the full inbound body and headers, which supersedes #70 — that work folds in here. Because this relocates the retry loop and circuit breaker into the target implementations, it is a non-trivial rewrite of `engine.go`. Before implementing, I'll post a concrete interface proposal here — the exact `Target` signature and how target-owned retries sit against the current queue/scheduler — for your sign-off. I'll sequence the rewrite after the in-flight `engine.go` work (#70 / PR #76) is resolved, since it touches the same file.

clawbot commented

2026-08-07 15:16:54 +02:00

Design proposal (needs your sign-off before I implement)

I read the current internal/delivery engine. Findings, the proposed shape, and two decisions for you.

How it works today

Dispatch is a single type switch on d.Target.Type in processDelivery (engine.go:762).
Retries are asynchronous and crash-durable — NOT a synchronous sleep loop. One attempt runs per worker turn; on failure the engine writes status retrying, computes an exponential backoff, and re-enqueues the task via scheduleRetry (a timer that pushes onto the retry channel). Retry state lives in the DB, and on restart recoverRetryingDeliveries plus a 60s sweep re-enqueue orphaned retries. This durability is a real feature worth keeping.
The circuit breaker is HTTP-only today. Notably, Slack is currently single-attempt with no retry and no breaker; an HTTP target with MaxRetries of 0 is fire-and-forget.
DeliveryResult rows and status updates are written by helpers (recordResult, updateDeliveryStatus) called from the per-type methods.

Proposed shape

A Target interface, one implementation per type, dispatched from a registry:

type Target interface {
    Type() database.TargetType
    Deliver(a Attempt) Outcome
}

Deliver performs one attempt and returns an Outcome describing what to persist (an optional DeliveryResult, the new status) and, if the target wants a retry, a directive with the next attempt number and backoff. The engine's processDelivery collapses to: look up the target in the registry, call Deliver, persist the result/status, and (if asked) re-enqueue the retry. The circuit-breaker map and the SSRF-safe client move into the http/slack targets; http and slack share a small core so they behave identically. database and log are single-attempt. logTarget logs the full body, full headers, method, content type, and the webhook/entrypoint ids.

Decision 1 — where retry EXECUTION lives

Your instruction was that the target owns retries and backoff. Two ways to honour it:

A (recommended): the target owns the retry POLICY — whether to retry, the backoff schedule, max attempts, and circuit-breaker gating — and performs one attempt; the engine's existing async queue/timer executes that policy and persists everything. Keeps today's crash-durability and non-blocking workers; behaviour-preserving.
B (literal): the target runs its own synchronous attempt-and-sleep loop and reports only a final result. Simpler ownership, but it blocks a worker goroutine for the entire backoff (which can be very long) and loses all in-flight retries on restart — it throws away the durable recovery/sweep machinery. I do not recommend B.

Both give the target full ownership of the retry behaviour; they differ only in whether the durable async executor stays. My recommendation is A.

Decision 2 — Slack retry semantics

Slack today never retries and does not use the breaker; this issue lists it as a retry target. Recommendation: put Slack on the shared http core and gate retry + breaker on MaxRetries exactly like HTTP — a Slack target with MaxRetries of 0 stays fire-and-forget (preserving every existing Slack target), and one configured with retries gets backoff + breaker. Alternative: keep Slack strictly single-attempt. Recommended: the gated approach.

Notes

Carrying entrypoint_id into the log needs a small additive change: Task gains an EntrypointID field, populated where tasks are built. Event.Body is already fully resolved before dispatch, so full-body logging is available.
Migration is behaviour-preserving: existing delivery tests keep passing with only their export_test.go wrappers re-pointed (persistence moves into processDelivery), plus new pure-Deliver unit tests and a log-target full-content test.

Please pick A vs B (Decision 1) and the Slack semantics (Decision 2). On your sign-off I'll implement — sequenced after the in-flight engine.go work (PR #76) is resolved.

## Design proposal (needs your sign-off before I implement) I read the current `internal/delivery` engine. Findings, the proposed shape, and two decisions for you. ### How it works today - Dispatch is a single type switch on `d.Target.Type` in `processDelivery` (`engine.go:762`). - Retries are asynchronous and crash-durable — NOT a synchronous sleep loop. One attempt runs per worker turn; on failure the engine writes status `retrying`, computes an exponential backoff, and re-enqueues the task via `scheduleRetry` (a timer that pushes onto the retry channel). Retry state lives in the DB, and on restart `recoverRetryingDeliveries` plus a 60s sweep re-enqueue orphaned retries. This durability is a real feature worth keeping. - The circuit breaker is HTTP-only today. Notably, Slack is currently single-attempt with no retry and no breaker; an HTTP target with `MaxRetries` of 0 is fire-and-forget. - `DeliveryResult` rows and status updates are written by helpers (`recordResult`, `updateDeliveryStatus`) called from the per-type methods. ### Proposed shape A `Target` interface, one implementation per type, dispatched from a registry: ``` type Target interface { Type() database.TargetType Deliver(a Attempt) Outcome } ``` `Deliver` performs one attempt and returns an `Outcome` describing what to persist (an optional `DeliveryResult`, the new status) and, if the target wants a retry, a directive with the next attempt number and backoff. The engine's `processDelivery` collapses to: look up the target in the registry, call `Deliver`, persist the result/status, and (if asked) re-enqueue the retry. The circuit-breaker map and the SSRF-safe client move into the http/slack targets; http and slack share a small core so they behave identically. `database` and `log` are single-attempt. `logTarget` logs the full body, full headers, method, content type, and the webhook/entrypoint ids. ### Decision 1 — where retry EXECUTION lives Your instruction was that the target owns retries and backoff. Two ways to honour it: - **A (recommended):** the target owns the retry POLICY — whether to retry, the backoff schedule, max attempts, and circuit-breaker gating — and performs one attempt; the engine's existing async queue/timer executes that policy and persists everything. Keeps today's crash-durability and non-blocking workers; behaviour-preserving. - **B (literal):** the target runs its own synchronous attempt-and-sleep loop and reports only a final result. Simpler ownership, but it blocks a worker goroutine for the entire backoff (which can be very long) and loses all in-flight retries on restart — it throws away the durable recovery/sweep machinery. I do not recommend B. Both give the target full ownership of the retry behaviour; they differ only in whether the durable async executor stays. My recommendation is A. ### Decision 2 — Slack retry semantics Slack today never retries and does not use the breaker; this issue lists it as a retry target. Recommendation: put Slack on the shared http core and gate retry + breaker on `MaxRetries` exactly like HTTP — a Slack target with `MaxRetries` of 0 stays fire-and-forget (preserving every existing Slack target), and one configured with retries gets backoff + breaker. Alternative: keep Slack strictly single-attempt. Recommended: the gated approach. ### Notes - Carrying `entrypoint_id` into the log needs a small additive change: `Task` gains an `EntrypointID` field, populated where tasks are built. `Event.Body` is already fully resolved before dispatch, so full-body logging is available. - Migration is behaviour-preserving: existing delivery tests keep passing with only their `export_test.go` wrappers re-pointed (persistence moves into `processDelivery`), plus new pure-`Deliver` unit tests and a log-target full-content test. Please pick A vs B (Decision 1) and the Slack semantics (Decision 2). On your sign-off I'll implement — sequenced after the in-flight `engine.go` work (PR #76) is resolved.

sneak was assigned by clawbot

2026-08-07 15:16:54 +02:00

sneak commented

2026-08-07 15:21:57 +02:00

why do you think B can't be durable? just give the database to the targets so they can do the same thing the delivery engine could, and they can own their retries. you're being obtuse here.

clawbot commented

2026-08-07 15:24:35 +02:00

You're right — I over-constrained B. B does not mean a blocking sleep loop, and it does not lose durability: hand the target the per-webhook DB and the re-enqueue primitive and it does exactly what the engine does today — record the attempt, mark retrying, schedule the next attempt — while owning the retry entirely. Scrapping the A/B framing.

Corrected design:

Target interface; each type an implementation. Deliver receives everything it needs to be autonomous and durable: the request context, the event, the per-webhook *gorm.DB, the Delivery and attempt number, and a Scheduler the engine provides for durable re-enqueue (ScheduleRetry(task, delay) — the existing timer + queue).
Each target owns its whole delivery: make the attempt, write the DeliveryResult and status to the DB, decide whether to retry, compute its own backoff, gate with its own circuit breaker, and reschedule via the injected Scheduler. Fire-and-forget targets (log, database) just record success once. http and slack own their retry + backoff + breaker.
The engine keeps only the worker pool, the queue, and restart recovery — and it hands the DB + Scheduler to the target. On restart, recovery hands each orphaned retrying delivery back to its target to recompute remaining backoff and reschedule, so the schedule stays target-owned.
So deliverHTTPWithRetry, handleHTTPRetry, circuitBreakerBlock, calcBackoff, and the breaker map all move wholesale into httpTarget; the engine no longer knows anything target-specific.
logTarget logs the full body, full headers, method, content type, and the webhook/entrypoint ids.

For Slack: I'll put it on the same shared core as http and gate retry/breaker on MaxRetries (0 stays fire-and-forget, so existing Slack targets are unchanged). Say so if you'd rather it stay strictly single-attempt; otherwise that's what I'll build.

Implementing this shape now. It rewrites engine.go, so I'm treating the small log-target PR #76 as superseded — folding it into this and closing it — and branching this from current main.

You're right — I over-constrained B. B does not mean a blocking sleep loop, and it does not lose durability: hand the target the per-webhook DB and the re-enqueue primitive and it does exactly what the engine does today — record the attempt, mark `retrying`, schedule the next attempt — while owning the retry entirely. Scrapping the A/B framing. Corrected design: - `Target` interface; each type an implementation. `Deliver` receives everything it needs to be autonomous and durable: the request context, the event, the per-webhook `*gorm.DB`, the `Delivery` and attempt number, and a `Scheduler` the engine provides for durable re-enqueue (`ScheduleRetry(task, delay)` — the existing timer + queue). - Each target owns its whole delivery: make the attempt, write the `DeliveryResult` and status to the DB, decide whether to retry, compute its own backoff, gate with its own circuit breaker, and reschedule via the injected `Scheduler`. Fire-and-forget targets (`log`, `database`) just record success once. `http` and `slack` own their retry + backoff + breaker. - The engine keeps only the worker pool, the queue, and restart recovery — and it hands the DB + `Scheduler` to the target. On restart, recovery hands each orphaned `retrying` delivery back to its target to recompute remaining backoff and reschedule, so the schedule stays target-owned. - So `deliverHTTPWithRetry`, `handleHTTPRetry`, `circuitBreakerBlock`, `calcBackoff`, and the breaker map all move wholesale into `httpTarget`; the engine no longer knows anything target-specific. - `logTarget` logs the full body, full headers, method, content type, and the webhook/entrypoint ids. For Slack: I'll put it on the same shared core as http and gate retry/breaker on `MaxRetries` (0 stays fire-and-forget, so existing Slack targets are unchanged). Say so if you'd rather it stay strictly single-attempt; otherwise that's what I'll build. Implementing this shape now. It rewrites `engine.go`, so I'm treating the small log-target PR #76 as superseded — folding it into this and closing it — and branching this from current `main`.

clawbot referenced this issue

2026-08-07 15:26:42 +02:00

Implement the log delivery target (closes #70) #76

clawbot referenced this issue

2026-08-07 15:26:45 +02:00

Implement the log delivery target #70

sneak referenced this issue from a commit

2026-08-07 16:43:27 +02:00

Refactor delivery targets to a Target interface (closes #77)

clawbot referenced a pull request that will close this issue

2026-08-07 16:43:49 +02:00

Refactor delivery targets to a Target interface (closes #77) #81