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
101 lines
2.8 KiB
Go
101 lines
2.8 KiB
Go
package backoff
|
|
|
|
import (
|
|
"fmt"
|
|
"testing"
|
|
"time"
|
|
|
|
"github.com/stretchr/testify/assert"
|
|
)
|
|
|
|
func TestExponential_Nominal(t *testing.T) {
|
|
policy := NewExponential(time.Minute, time.Hour, 24*time.Hour, 0)
|
|
wantDurations := []time.Duration{
|
|
time.Minute,
|
|
2 * time.Minute,
|
|
4 * time.Minute,
|
|
8 * time.Minute,
|
|
16 * time.Minute,
|
|
32 * time.Minute,
|
|
time.Hour,
|
|
time.Hour,
|
|
time.Hour,
|
|
}
|
|
for attemptNumber, wantDelay := range wantDurations {
|
|
t.Run(fmt.Sprint("attempt #", attemptNumber), func(t *testing.T) {
|
|
t.Parallel()
|
|
gotDelay, err := policy.GetRetryDelay(attemptNumber)
|
|
|
|
assert.NoError(t, err)
|
|
assert.Equal(t, wantDelay, gotDelay)
|
|
})
|
|
}
|
|
}
|
|
|
|
func TestExponential_Limit(t *testing.T) {
|
|
// Attempt: 0 1 2
|
|
// Delay: 1m 2m X
|
|
// sinceFirstAttempt: 1m 3m X
|
|
policy := NewExponential(time.Minute, 5*time.Minute, 3*time.Minute, 0)
|
|
|
|
delay, err := policy.GetRetryDelay(1)
|
|
assert.NoError(t, err)
|
|
assert.Equal(t, 2*time.Minute, delay)
|
|
|
|
_, err = policy.GetRetryDelay(2)
|
|
assert.ErrorIs(t, err, ErrMaxAttemptsReached)
|
|
}
|
|
|
|
func TestExponential_MaxAttemptsCap(t *testing.T) {
|
|
// abort-after is huge, so only the attempt cap (3) can stop retries.
|
|
policy := NewExponential(time.Minute, time.Hour, 100*24*time.Hour, 3)
|
|
|
|
for attempt := range 2 {
|
|
_, err := policy.GetRetryDelay(attempt)
|
|
assert.NoError(t, err, "attempt %d should be allowed to schedule another retry", attempt)
|
|
}
|
|
|
|
_, err := policy.GetRetryDelay(2)
|
|
assert.ErrorIs(t, err, ErrMaxAttemptsReached)
|
|
}
|
|
|
|
func TestExponential_CanRetryAttemptOnlyChecksAttemptCap(t *testing.T) {
|
|
policy := NewExponential(time.Minute, time.Hour, 2*time.Minute, 3)
|
|
limiter, ok := policy.(interface {
|
|
CanRetryAttempt(int) error
|
|
})
|
|
assert.True(t, ok)
|
|
|
|
assert.NoError(t, limiter.CanRetryAttempt(2))
|
|
assert.ErrorIs(t, limiter.CanRetryAttempt(3), ErrMaxAttemptsReached)
|
|
|
|
_, err := policy.GetRetryDelay(1)
|
|
assert.ErrorIs(t, err, ErrMaxAttemptsReached, "abort-after still applies when scheduling a future retry")
|
|
}
|
|
|
|
func TestExponential_LimitRetryDelay(t *testing.T) {
|
|
policy := NewExponential(time.Minute, time.Hour, 10*time.Minute, 0)
|
|
limiter, ok := policy.(interface {
|
|
LimitRetryDelay(int, time.Duration) (time.Duration, error)
|
|
})
|
|
assert.True(t, ok)
|
|
|
|
delay, err := limiter.LimitRetryDelay(1, 8*time.Minute)
|
|
assert.NoError(t, err)
|
|
assert.Equal(t, 8*time.Minute, delay)
|
|
|
|
_, err = limiter.LimitRetryDelay(1, 10*time.Minute)
|
|
assert.ErrorIs(t, err, ErrMaxAttemptsReached)
|
|
}
|
|
|
|
func TestExponential_LimitRetryWindow(t *testing.T) {
|
|
policy := NewExponential(time.Minute, time.Hour, 10*time.Minute, 0)
|
|
limiter, ok := policy.(interface {
|
|
LimitRetryWindow(time.Duration) error
|
|
})
|
|
assert.True(t, ok)
|
|
|
|
assert.NoError(t, limiter.LimitRetryWindow(10*time.Minute))
|
|
assert.ErrorIs(t, limiter.LimitRetryWindow(10*time.Minute+time.Nanosecond), ErrMaxAttemptsReached)
|
|
}
|