Sample review pack

Add retry with exponential backoff to the webhook dispatcher

Generated in 47s on an RTX 4080 with Qwen3 32B via LM Studio

model: Qwen3 32B (Q4_K_M)
host: LM Studio
hardware: RTX 4080, 64 GB RAM
time: 47s
depth: Standard
prompt tokens: 11,482
output tokens: 2,210
left machine: False

This pack was generated from a purpose-built open-source pull request and is shown unedited. Swap in your own model and hardware — the structure is what Diffsift gives you every time.

repo: hookrelay-oss/hookrelay · PR #214

feature/dispatch-retry → main

profile: general-csharp

mode: SinglePass · depth: Standard

Summary

This PR adds retry behaviour to the webhook dispatcher: failed deliveries are retried up to three times with exponential backoff, and a new `DeliveryAttempt` record captures the outcome of each attempt. The core change is well structured, but the retry counter is shared across concurrent deliveries, the `HttpClient` is created per attempt, and the backoff delay ignores the configured maximum.

Change intent

Make webhook delivery resilient to transient endpoint failures without dropping events.

Recommendation

Request changes. The shared mutable retry state (RDR-1) can cause deliveries to be abandoned early under load; the remaining findings are quality improvements that could follow in a later PR.

Risk level: Medium

Suggested review focus

· Concurrent delivery paths through `WebhookDispatcher.DispatchAsync`
· Retry timing behaviour when `MaxBackoff` is configured below the computed delay
· Socket usage under sustained retry load

Findings (3)

RDR-1

Concurrency

High High confidence

src/HookRelay/Dispatch/WebhookDispatcher.cs` line 91 · @@ -74,6 +82,24 @@

`_attemptCount` is an instance field on `WebhookDispatcher`, which is registered as a singleton. Two webhooks being delivered concurrently share the same counter, so a delivery can be abandoned after zero retries because a different delivery already consumed the attempts. Under load this silently drops events — the exact scenario the PR is meant to fix.

Suggested comment

`_attemptCount` is shared across all concurrent deliveries because the dispatcher is a singleton. A burst of failures on one endpoint will exhaust the counter for every in-flight delivery, so some webhooks get zero retries. Consider tracking the attempt count per delivery — e.g. a local variable in `DispatchAsync` or a field on `DeliveryAttempt` — instead of dispatcher state.

Context from PR ** Exact Match, High confidence

@@ -74,6 +82,24 @@

+    private int _attemptCount;
+
+    public async Task<DeliveryAttempt> DispatchAsync(WebhookEvent evt, CancellationToken ct)
+    {
+        while (_attemptCount < _options.MaxAttempts)
+        {
+            _attemptCount++;
+            var result = await SendAsync(evt, ct);
+            if (result.IsSuccess)
+            {
+                return DeliveryAttempt.Succeeded(evt, _attemptCount);
+            }
+
+            await Task.Delay(RetryPolicy.NextDelay(_attemptCount), ct);
+        }
+
+        return DeliveryAttempt.Abandoned(evt, _attemptCount);
+    }

Why this context matters: The field declaration and the loop that mutates it are both visible in the diff, confirming the counter lives on the singleton dispatcher rather than on the per-delivery call path.

RDR-2

Resource Management

Medium High confidence

src/HookRelay/Dispatch/WebhookDispatcher.cs` line 118 · @@ -104,3 +126,12 @@

`SendAsync` constructs a new `HttpClient` for every attempt. With retries this multiplies the problem: a single failing endpoint now creates up to four clients per event. Disposed `HttpClient` instances hold sockets in TIME_WAIT, so sustained retry traffic risks socket exhaustion on busy relays.

Suggested comment

`new HttpClient()` per attempt will exhaust sockets under sustained retry load — each disposed client parks a socket in TIME_WAIT. Inject `IHttpClientFactory` (already registered in `Program.cs`) and create the client from the factory, or hold a single static client with a `PooledConnectionLifetime`.

Context from PR ** Exact Match, High confidence

@@ -104,3 +126,12 @@

+    private async Task<SendResult> SendAsync(WebhookEvent evt, CancellationToken ct)
+    {
+        using var client = new HttpClient { Timeout = _options.RequestTimeout };
+        using var response = await client.PostAsJsonAsync(evt.TargetUrl, evt.Payload, ct);
+        return response.IsSuccessStatusCode
+            ? SendResult.Success(response.StatusCode)
+            : SendResult.Failure(response.StatusCode);
+    }

Why this context matters: The `using var client = new HttpClient()` inside the per-attempt send path is visible in the diff; combined with the retry loop in RDR-1 this runs once per attempt, not once per dispatcher.

RDR-3

Correctness

Low Medium confidence

src/HookRelay/Dispatch/RetryPolicy.cs` line 21 · @@ -0,0 +14,10 @@

`NextDelay` computes `BaseDelay * 2^attempt` but never applies `_options.MaxBackoff`, although the option is defined and documented in `DispatchOptions`. At the default 2s base, attempt 3 already waits 16s; if `MaxAttempts` is raised in configuration the delay grows unbounded and deliveries can hang far beyond the endpoint's expectations.

Suggested comment

`NextDelay` ignores `MaxBackoff` — the computed delay should be clamped: `TimeSpan.FromTicks(Math.Min(delay.Ticks, _options.MaxBackoff.Ticks))`. Worth a test at the boundary, since `MaxBackoff` is documented as a hard cap in `DispatchOptions`.

Context from PR ** Exact Match, High confidence

@@ -0,0 +14,10 @@

+    public static TimeSpan NextDelay(int attempt)
+    {
+        var multiplier = Math.Pow(2, attempt - 1);
+        return TimeSpan.FromMilliseconds(BaseDelay.TotalMilliseconds * multiplier);
+    }

Why this context matters: The full body of `NextDelay` is in the diff and contains no reference to `MaxBackoff`, while `DispatchOptions.MaxBackoff` appears in the same PR's options file.

Existing issues already in the codebase — not introduced by this PR

RDR-4

Test Coverage

Low Medium confidence

tests/HookRelay.Tests/Dispatch/WebhookDispatcherTests.cs

The new tests cover the success path and a single failure, but there is no test for cancellation during backoff. `Task.Delay` honours the token, so cancellation mid-backoff will throw `TaskCanceledException` out of `DispatchAsync` — probably intended, but currently unpinned by any test.

Reviewer note

Consider a test that cancels during the backoff delay and asserts the delivery surfaces `TaskCanceledException` (or a graceful abandon, if that is the intended contract).

Questions for the author

RDR-Q1 Should an abandoned delivery emit a dead-letter event, or is dropping after `MaxAttempts` intentional? (src/HookRelay/Dispatch/WebhookDispatcher.cs): The PR removes the previous `_logger.LogWarning` on failure but adds no terminal signal for abandoned deliveries.

File-by-file notes

src/HookRelay/Dispatch/WebhookDispatcher.cs

Adds the retry loop and per-attempt send path. Findings RDR-1 and RDR-2 apply here.

· Retry loop reads cleanly; the abandoned path returns a typed result rather than throwing.

src/HookRelay/Dispatch/RetryPolicy.cs

New static policy holder for backoff computation.

· Consider making the policy instance-based so `BaseDelay` can come from `DispatchOptions`.

src/HookRelay/Dispatch/DeliveryAttempt.cs

New immutable record for attempt outcomes. No issues found.

tests/HookRelay.Tests/Dispatch/WebhookDispatcherTests.cs

Covers success and single-failure paths with a fake handler.

· Missing cancellation-during-backoff coverage (RDR-4).

Want this for your pull requests?

Diffsift is launching soon. Join the list and get 25% off at launch.

Double opt-in — you confirm by email before anything is sent. Or use the hosted signup page.