mirror of
https://github.com/hanzo-fi/webhooks.git
synced 2026-10-30 20:32:55 +00:00
Maxence MaireauxandGitHub
c04477b005
* docs: deep-dive webhooks architecture, prod retry-storm analysis and improvement plan
* fix(worker): treat permanent 4xx as terminal and bound response reads
Non-2xx responses were all retried until the abort-after window elapsed,
so a 403 (bad credentials), 404 (gone) or 405 (wrong method) was retried
for days — the root cause of the production retry storms (Paynovate 403,
ngrok 404, example.com 405).
MakeAttempt now marks 4xx (except 408/429) as 'failed' immediately, honors
an explicit Retry-After on transient responses, and bounds the response
body read to 64 KiB to protect worker memory.
* feat(worker): cap delivery attempts and lower default abort-after
abort-after (default 30d) was the only bound on retries. Combined with the
1m->1h backoff, a dead endpoint was retried ~724 times over 30 days (matching
the observed Tenora retry count) before being marked failed.
Add a --max-attempts hard cap (default 15) enforced by the exponential policy,
and lower the default --abort-after from 30d to 72h. Together they bound a dead
endpoint to a handful of attempts instead of hundreds. The cap is disabled with
--max-attempts=0 for callers that want abort-after-only semantics.
* feat(worker): add attempts retention and orphan reclamation
The attempts table had no retention: success rows were kept forever and
attempts of deleted configs looped in the retry queue indefinitely. In
production this grew the table past 100 GB and dominated cross-AZ RDS cost
(a one-off cleanup cut replication traffic 145GB->0.3GB/15min, ~$39k/yr).
Add a background Retention runner (worker only) that periodically:
- marks pending attempts of deleted configs as 'failed' (FailOrphanedAttempts)
- purges 'success'/'failed' attempts past their retention, in bounded batches
Configurable via --retention-period (1h), --retention-success-delay (30d) and
--retention-failed-delay (90d); a 0 delay disables that purge.
* feat: add OpenTelemetry metrics for webhook delivery
The service emitted only traces and logs, so retry storms were detected from
the AWS bill rather than a dashboard. Add OTLP metrics, wired into both the
serve and worker commands:
- webhooks_delivery_attempts_total{status,status_class} counter
- webhooks_delivery_duration_seconds histogram
- webhooks_retry_queue_depth observable gauge (the leading storm indicator)
Delivery attributes are kept low-cardinality; per-endpoint breakdown stays in
the otelhttp delivery spans to avoid a label explosion across customer endpoints.
* fix: harden retention and Retry-After handling (self-review)
Self-review findings before opening the PR:
- NewRetention: guard against a zero/negative period (time.NewTicker panics
on <= 0 if --retention-period=0 is passed explicitly)
- FailOrphanedAttempts: update in bounded batches — the production orphan
backlog reached millions of rows and a single UPDATE would hold locks and
produce one giant transaction
- Purge/orphan batch subqueries: FOR UPDATE SKIP LOCKED so concurrent
retention runners (multiple worker replicas) do not contend on the same rows
- parseRetryAfter: clamp endpoint-controlled Retry-After to 6h so an absurd
value cannot park a delivery years in the future
* fix(ci): adapt e2e retry spec to terminal 4xx and harden metrics lifecycle
The e2e 'Retries' spec drove its retry scenario with a 404, which the
terminal-4xx change now fails immediately — the spec timed out waiting for
'to retry' attempts. Switch the retry scenario to a transient 500 and add a
dedicated spec asserting that a permanent 4xx fails immediately without ever
entering the retry queue.
Also addresses review findings on the queue-depth gauge:
- only register the DB-backed gauge when a metrics exporter (or in-memory
metrics) is configured — the otlpmetrics module installs a periodic reader
even with the no-op exporter, which would run the COUNT every collect
interval for a value nobody reads
- stop closing the store from the worker lifecycle hook (bunconnect owns the
DB) and register otlpmetrics after postgres in serve, so the final metrics
flush no longer hits a closed database
- add direct Postgres tests for PurgeFinishedAttempts (cutoffs, disabled
retention, batch draining) and FailOrphanedAttempts (orphan reclamation,
live-config preservation, terminal rows untouched)
* fix(webhooks): harden retry retention safeguards
* test(webhooks): cover retry delivery after endpoint recovery
* fix(webhooks): bound retry-after by retry window
* fix(webhooks): address retry review feedback
* fix(webhooks): close retry window loopholes
* fix(webhooks): address retry review followups
* test(webhooks): cover retry store failure paths
* fix(webhooks): keep retry history during retention
26 lines
813 B
Go
26 lines
813 B
Go
package webhooks
|
|
|
|
import "time"
|
|
|
|
type BackoffPolicy interface {
|
|
GetRetryDelay(attemptNumber int) (time.Duration, error)
|
|
}
|
|
|
|
// RetryAttemptLimiter is implemented by retry policies that can reject a retry
|
|
// attempt before the outbound HTTP call is made.
|
|
type RetryAttemptLimiter interface {
|
|
CanRetryAttempt(attemptNumber int) error
|
|
}
|
|
|
|
// RetryDelayLimiter is implemented by retry policies that can reject a custom
|
|
// retry delay before it is persisted.
|
|
type RetryDelayLimiter interface {
|
|
LimitRetryDelay(attemptNumber int, delay time.Duration) (time.Duration, error)
|
|
}
|
|
|
|
// RetryWindowLimiter is implemented by retry policies that can reject a retry
|
|
// schedule based on the real elapsed time since the first delivery attempt.
|
|
type RetryWindowLimiter interface {
|
|
LimitRetryWindow(elapsed time.Duration) error
|
|
}
|