BaryoDev / BaryoDev/barakoCMS

Due workflow runs can starve behind twenty older runs that are backing off

Open
#695 0 comments 0 reactions 0 assignees View on GitHub
bug core
Dominant language
C#
Stars
6
Forks
7
Avg merge
4h 42m
Merged PRs (30d)
307

Description

`WorkflowRunner.RunOnceAsync` selects candidates by age and status only, then decides due-ness afterwards in `NextDue`:

```csharp
var due = await query.Query()
.Where(r => r.Status == RunStatus.Pending || r.Status == RunStatus.Running)
.OrderBy(r => r.CreatedAt)
.Take(20)
.ToListAsync(ct);
```

`NextDue` then returns null for an attempt whose `NextAttemptAt` is in the future, and null for one another node holds under a live lease. If all twenty of the oldest candidates are in that state, the pass does nothing for the tenant and moves on.

A run that became due one second ago is never looked at, because it is twenty-first by `CreatedAt`. It is reachable whenever a tenant accumulates twenty runs in backoff, and `WorkflowRetryPolicy.Backoff` reaches ten minutes plus jitter, so a provider outage produces exactly that. On a multi-node deployment the twenty slots also fill with attempts other nodes hold, which has the same effect with no failures involved at all.

### What to change

Push due-ness into the query instead of filtering after it. That needs a run-level next-due timestamp on `WorkflowRun`, maintained by `Recompute`, so it can be indexed alongside the existing `Status` and `CreatedAt` indexes in `ServiceCollectionExtensions`.

### Related, same loop: tenants are not treated fairly

`PartitionsWithWorkAsync` returns tenants in whatever order Postgres yields, the loop visits them in that order, and `RunOnceAsync` returns on the first successful claim. A tenant that always has work and sorts early can therefore crowd out every tenant after it indefinitely. A rotating start offset across passes is enough, and it is worth doing in the same change since it is the same fifteen lines.

### How it should be tested

Construct the case where broken and fixed differ: queue more than twenty runs in one tenant with future `NextAttemptAt`, queue one that is due now, run a pass, assert the due one ran. The existing tests drain to completion and never build that state, so they pass either way.

### Where I checked

`barakoCMS/Features/Workflows/WorkflowRunner.cs` (`RunOnceAsync`, `NextDue`, `PartitionsWithWorkAsync`), the `WorkflowRun` schema registration in `barakoCMS/Extensions/ServiceCollectionExtensions.cs`. Searched open issues for runner starvation, fairness and noisy neighbour. Nothing open covers either half.

Contributor guide

Open the contributing guide

Research direction

Start in barakoCMS/Features/Workflows/WorkflowRunner.cs at RunOnceAsync, NextDue, and PartitionsWithWorkAsync, then inspect WorkflowRun registration in barakoCMS/Extensions/ServiceCollectionExtensions.cs and the existing workflow tests. Reproduce the more-than-twenty backoff case and add coverage showing the due run executes; verify tenant work is rotated fairly and the new next-due field and indexes are maintained by Recompute.

Written by the indexing model from the issue text.

Assessment

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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.