BaryoDev / BaryoDev/barakoCMS

JobBackoff has no jitter, so a provider outage retries every job in lockstep

Open
#702 0 comments 0 reactions 0 assignees View on GitHub
bug core help wanted up-for-grabs
Dominant language
C#
Stars
6
Forks
7
Avg merge
4h 42m
Merged PRs (30d)
307

Description

`JobBackoff.DelayFor` is exponential with a cap and nothing else:

```csharp
var exponent = Math.Min(attempt - 1, 30);
var seconds = Math.Min((long)baseSeconds << exponent, maxSeconds);
return TimeSpan.FromSeconds(seconds);
```

Every job that fails at the same moment therefore retries at the same moment. With `Jobs:BackoffBaseSeconds` at its default of 30, five hundred jobs failing against one provider all wake at 30 seconds, then 60, then 120, in lockstep, against a service that is by definition already having a bad time. That is the thundering herd, and the first retry is the worst of it because that is when the pile is largest.

The workflow runner already knows this and does the opposite:

```csharp
/// Exponential with jitter, so a provider that failed for everyone is not retried by everyone at once.
public static TimeSpan Backoff(int attempts, Random random)
{
var seconds = Math.Min(Math.Pow(2, Math.Max(attempts, 1)) * 5, 600);
var jitter = random.NextDouble() * seconds * 0.25;
return TimeSpan.FromSeconds(seconds + jitter);
}
```

So the two retry mechanisms in this repo disagree about the case that matters most, and the one without jitter is the one D15 designated as the shared queue that webhook delivery, email and AI indexing all move onto. The more consumers land on it, the more correlated the herd gets.

### What to change

Add jitter to `JobBackoff.DelayFor`. Decorrelated jitter is the better default than the runner's proportional jitter, since proportional jitter still leaves the peak in the same place, but matching `WorkflowRetryPolicy` would already be a large improvement over none.

Whichever is chosen, both should end up using the same one. Two backoff curves with different herd behaviour is the kind of thing that is only ever discovered during an incident.

### How it should be tested

`JobBackoffTests` exists and passes today, because a deterministic curve is easy to assert. A jittered curve needs a different assertion shape: that N calls at the same attempt number do not all return the same delay, and that every result stays within the intended bounds. Assert the spread, not one value.

Note that adding jitter makes the current tests fail if they assert exact equality, which is the correct signal rather than a problem to work around.

### Where I checked

`barakoCMS/Infrastructure/Jobs/JobBackoff.cs`, `barakoCMS/Infrastructure/Jobs/JobOptions.cs`, `barakoCMS/Features/Workflows/WorkflowRunner.cs`, `BarakoCMS.Tests/JobBackoffTests.cs`. Searched open issues for backoff and jitter; #695 is the runner's candidate selection, which is a different defect in the same area.

Contributor guide

Open the contributing guide

Research direction

Start with barakoCMS/Infrastructure/Jobs/JobBackoff.cs and BarakoCMS.Tests/JobBackoffTests.cs, then compare the existing retry behavior in barakoCMS/Features/Workflows/WorkflowRunner.cs. Choose a jitter approach, ensure the job backoff stays within its intended bounds, and update tests to assert spread across repeated calls rather than exact values. Done means the shared job retry path no longer returns identical delays for the same attempt and its tests pass.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
backend, distributed-systems
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
64/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.