Refactor delivery targets to a Target interface (closes #77) #81
Reference in New Issue
Block a user
Delete Branch "issue-77-target-interface"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Refactors the delivery engine so each target TYPE is an implementation of a
Targetinterface, dispatched from a registry, with each target owning its full delivery including durable retries. Implements the authoritative design from issue #77 (the corrected "hand the DB + Scheduler to the target" design).The new interface
Deliverreceives everything a target needs to be autonomous and durable: the request context, the per-webhook*gorm.DB, the*database.Delivery, the attempt*Task, and aScheduler(the engine) for durable re-enqueue. The target makes one attempt, writes theDeliveryResult, updatesDeliveryStatus, and — for retry targets — decides whether to retry, computes its own backoff, gates with its own circuit breaker, and reschedules via the injectedScheduler.processDeliverycollapses to a registry lookup (map[database.TargetType]Target) and aDelivercall; an unknown target type still fails the delivery as before.Per-target ownership
httpTargetandslackTargetshare a retry core (httpCore) that owns retry, exponential backoff, and the per-target circuit breaker. The core is fire-and-forget whenMaxRetries == 0and adds breaker-gated backed-off retries whenMaxRetries > 0. The per-attempt request differs (HTTP forwards the body + filtered headers; Slack posts a formatted message) and is supplied as a closure, so each keeps its exact recording semantics (e.g. HTTP records no error string for a non-2xx, Slack recordsHTTP <code>).databaseTargetandlogTargetare fire-and-forget: they record a single successful attempt.Moved wholesale into the http/slack targets:
deliverHTTP*,handleHTTPRetry,circuitBreakerBlock,calcBackoff/calcRemainingBackoff/backoffElapsed, the circuit-breakersync.Map+getCircuitBreaker,clientForConfig,doHTTPRequest,applyRequestHeaders, and the config parsers. The engine keepsrecordResult,updateDeliveryStatus, andScheduleRetry.Slack MaxRetries gating
Slack is now on the same shared core as HTTP, with retry + breaker gated on
MaxRetries. AMaxRetriesof 0 stays single-attempt fire-and-forget, so every existing Slack target is unchanged; a Slack target configured with retries gets backoff + circuit breaker.Log-target full content
logTargetnow logs the ENTIRE inbound webhook — full request body and full request headers, plus method, content type, and the webhook id and entrypoint id — rather than a summary line. This supersedes the smaller log-summary work (#70).Task.EntrypointIDTo carry the entrypoint id to the log target,
Taskgains anEntrypointIDfield, populated in the webhook handler'sbuildDeliveryTasks, the engine's recovery-task builder, andbuildEventFromTask.Durability / recovery
The crash-durable async retry model is preserved unchanged: one attempt per worker turn; on failure the status is set
retrying, backoff is computed, and the task is re-enqueued viaScheduleRetry(atime.AfterFunconto the retry channel). On restart,recoverRetryingDeliveriesand the 60s sweep hand each orphanedretryingdelivery back to its target to recompute the remaining backoff and reschedule (targets that own retries implement an internalrescheduler; fire-and-forget targets, which never produceretryingdeliveries, are skipped).How behaviour is preserved
No external behaviour changes except the two called out above (log target full content; Slack gaining
MaxRetries-gated retries). All existing delivery tests pass with only theirexport_test.gowrappers re-pointed at the new structure —ExportDeliverHTTP/Slack/Database/Lognow call the targets,ExportGetCircuitBreaker/ExportClient/ExportClientForConfig/ExportDoHTTPRequestresolve against the HTTP target's shared client and breaker map, andExportParseHTTPConfig/ExportParseSlackConfigcall the relocated free functions. Added: alogTargettest asserting the log line contains the full body, headers, and ids, and a SlackMaxRetries-gated retry test.docker build .is green (fmt-check, lint, test, static build all pass).Closes #77
File-by-file change summary:
internal/delivery/target.go(new): theSchedulerandTargetinterfaces, the internalreschedulerinterface (used by recovery/sweep to let a target recompute its backoff), theattemptResultstruct, andinitTargetswhich builds themap[database.TargetType]Targetregistry and wires the shared SSRF-safe client into the http/slack targets.internal/delivery/target_http.go(new):HTTPTargetConfig, the sharedhttpCore(fire-and-forget vs breaker-gated retry orchestration,getCircuitBreaker+ the breakersync.Map,handleRetry,circuitBreakerBlock,remainingBackoff/backoffElapsed,calcBackoff), andhttpTarget(Deliver,attempt,doHTTPRequest,clientForConfig,applyRequestHeaders,isForwardableHeader,executeHTTPRequest,parseHTTPConfig).internal/delivery/target_slack.go(new):SlackTargetConfig,slackTarget(Deliver,attempt,readSlackResponse,failConfig),parseSlackConfig, and the message formatting (FormatSlackMessage,formatJSONBody,formatRawBody). Slack runs on the sharedhttpCore, gated onMaxRetries.internal/delivery/target_database.go(new):databaseTarget— fire-and-forget, records one success.internal/delivery/target_log.go(new):logTarget— logs the full body, full headers, method, content type, webhook id and entrypoint id, then records one success.internal/delivery/engine.go: removed the per-type delivery methods and the type switch;processDeliveryis now a registry lookup +Deliver. Removed the engine'sclientfield and circuit-breaker map (moved into the targets);Newand the test constructors callinitTargets.recoverSingleRetry/sweepSingleRetrynow delegate the backoff recompute to the delivery's target viarescheduler. RenamedscheduleRetryto the exportedScheduleRetry(theSchedulerimpl). AddedTask.EntrypointID, populated inbuildEventFromTaskandbuildRecoveryTask.internal/delivery/export_test.go: re-pointed the wrappers at the new structure (ExportDeliverHTTP/Slack/Database/Logcall the targets;ExportGetCircuitBreaker/ExportClient/ExportClientForConfig/ExportDoHTTPRequestresolve against the http target;ExportParseHTTPConfig/ExportParseSlackConfigcall the relocated free functions); the test engine constructors now callinitTargets.internal/delivery/engine_test.go: addedTestDeliverLog_LogsFullContent(asserts the log line carries the full body, headers, and ids) andTestDeliverSlack_WithRetries_SchedulesRetry(the gated Slack retry path), plus small test helpers.internal/handlers/webhook.go: populateTask.EntrypointIDfromentrypoint.IDinbuildDeliveryTasks.Validation:
(fmt-check, lint, full test suite, and the static build all pass inside the image.)
Critical review: PR #81 — Refactor delivery targets to a Target interface
Verdict: PASS (merge-ready).
Reviewed the full diff of all 9 changed files against
origin/main, compared every new target implementation line-by-line against the oldEnginemethods it replaces, built the container, and ran the delivery + handlers test suites.What was verified
Behaviour preservation (retry/backoff/breaker/status/attempt numbers):
httpTargetfire-and-forget path is byte-for-byte equivalent to the olddeliverHTTPFireAndForget: records attempt 1, success =reqErr == nil && 200 <= code < 300, empty error string on a non-2xx,Delivered/Failedtransitions unchanged.httpCore.withRetryreproducesdeliverHTTPWithRetryexactly: breaker gate →recordResult(attemptNum)→RecordSuccess+Delivered, orRecordFailure+handleRetry.handleRetrymatcheshandleHTTPRetry(attemptNum >= maxRetries→Failed; elseRetrying,calcBackoff(attemptNum),AttemptNum+1).circuitBreakerBlockis identical, including re-scheduling the same-attempt task with the cooldown remaining.MaxRetries == 0case the newfireAndForgetpath is equivalent to the olddeliverSlack/sendSlackRequest/handleSlackResponse/failSlackDelivery— attempt 1,HTTP <code>on non-2xx,sending request: …on transport error, same Content-Type/User-Agent headers, same shared SSRF-safe client. The newMaxRetries > 0retry+breaker path is the intended, explicitly-scoped new capability and does not affect existing (0-retry) Slack targets.database/logfire-and-forget: unchanged (record one success, markDelivered).lognow emits the full body, full headers, method, content type, and webhook + entrypoint ids (supersedes the oldbody_length-only line), as required.calcBackoff/remainingBackoff/backoffElapsedare identical to the old free/engine functions.Circuit breaker relocation:
Engine.ExportGetCircuitBreakernow resolves againste.httpTarget.getCircuitBreaker, i.e. the same map the HTTP target actually uses, so breaker-reaching tests address the correct breaker. HTTP and Slack owning separate breaker maps is correct: a target id belongs to exactly one target type, so the maps never key the same id from two owners — no collision.Recovery/sweep delegation: recovery and the 60s sweep now load the target, look it up in the registry, and skip anything not implementing the internal
rescheduler. This is safe: the only code paths that ever set statusretryingare the HTTP/Slack retry cores, and both implementrescheduler. Fire-and-forget targets provably never produceretryingdeliveries, so nothing the old code would have rescheduled is silently dropped.Task.EntrypointID: populated in all three builders — handler
buildDeliveryTasks(entrypoint.ID), enginebuildRecoveryTask(event.EntrypointID), andbuildEventFromTask(task.EntrypointID).Quality bar: single clean commit authored as
sneak; no AI/tooling references anywhere in the diff, commit message, or comments. Naming is consistent and stutter-free; error handling and logging preserved. New tests are meaningful: the log full-content test seeds real non-empty webhook/entrypoint UUIDs and asserts body marker, headers, content type, and both ids appear (non-vacuous), and assertsDelivered; the Slack gated-retry test drives a 503 server withMaxRetries: 5and assertsRetryingstatus plus a recorded failed 503 result. Existing tests pass with onlyexport_test.gowrappers re-pointed at the new structure, matching the stated design.Build/tests:
docker buildexits 0 (Dockerfile runsmake fmt-check,make lint,make test,make build; cache is content-keyed on the source copy, so the hit confirms this exact tree passed). Ran the tests directly to confirm:internal/deliveryok,internal/handlersok.Non-blocking observation (informational, low severity)
The recovery/sweep
reschedulerskip means an orphanedretryingdelivery whose target had its type mutated in the DB from a retry type (http/slack) to a fire-and-forget type (or to an unknown type) would now stay stuck inretrying, where the old code would eventually re-dispatch it and let the new type mark itDelivered/Failed. This requires mutating a target's type while aretryingdelivery for it is orphaned — an extremely narrow operational edge case that neither implementation handles "correctly," and it does not arise in normal operation. Noted only for completeness; not a merge blocker.Manager note (separate from the review above): this PR was already merged to
mainbysneakbefore the independent critical review completed, so #77 is closed via the merge. The review (posted above) landed a PASS verdict, so no rework is needed. The one non-blocking edge case it raised — an orphanedretryingdelivery whose targettypeis later mutated to a non-retry type would stay stuck inretrying— is captured for follow-up as #82.Independent critical review (the review gate) — no correctness blockers
For the record: the result of the independent reviewer I dispatched separately from the author agent. This PR was merged by @sneak before it finished, so the gate ran post-merge; it confirms the merged code is sound.
Verdict: no behavioural regressions. Verified line-by-line against the pre-refactor engine:
maxRetries, same exponential backoff (calcBackoff, shift capped), same next-attempt increment; fire-and-forget at aMaxRetriesof 0 records one attempt unchanged.Allow/RecordSuccess/RecordFailurepoints; breaker-open writes no result row, setsretrying, and reschedules the same attempt with the cooldown remaining.retryingrows).MaxRetriesof 0) unchanged: single attempt, no breaker, same result recording and headers. The new retry+breaker path applies only when retries are configured.Task.EntrypointIDpopulated on every path; the SSRF-safe transport is on every outbound path; exactly one result row per attempt (zero on breaker-open); no AI/tooling references; docker build green.One process caveat (not correctness): the branch was 2 commits behind
main(#78 retention reaper, #75 NoCache), so the refactor was never built together with those. A merge-tree check showed the merge is conflict-free and drops neither, and @sneak's merge is a real merge commit — so nothing was lost. The integratedmainbuild will be exercised by the next work units branching from it.Net: the independent gate agrees with the on-PR review — the refactor is sound. Follow-up #82 tracks the one narrow, non-blocking edge case.