Race your agent's C# before you review it: a 16-line harness for the bugs reading won't find
Agent-written caches look thread-safe and aren't. A start-gate harness and three tests make the failures repeatable. About 15 minutes to try.
Here’s a class that an agent will happily produce when asked for “a cache for exchange rates that doesn’t hammer the provider”. It compiles, it reads well, it uses a thread-safe collection, and a reviewer skimming the diff will nod at ConcurrentDictionary and move on.
public sealed class RateService(IRateProvider provider)
{
private readonly ConcurrentDictionary<string, Task<decimal>> _rates = new();
private readonly ConcurrentDictionary<string, int> _lookups = new();
public Task<decimal> GetRateAsync(string currency)
{
_lookups[currency] = _lookups.GetValueOrDefault(currency) + 1;
return _rates.GetOrAdd(currency, c => provider.FetchAsync(c));
}
public int Lookups(string currency) => _lookups.GetValueOrDefault(currency);
}
It has three defects. It can call the provider many times for one currency, which is the thing the cache exists to prevent. It loses counts. And the first time the provider returns a 503, the failed task is cached, so every later caller gets the same 503 until the process restarts.
None of this shows up in a review unless the reviewer is hunting for it, and the races never show up in a test that calls the method once. The fix is a small harness that makes concurrent callers arrive at the same instant, plus a test for each defect. Build it once and it works on any C# an agent hands you, from any agent. The whole thing runs in a minute and needs nothing beyond the .NET 10 SDK.
Why a collection that’s thread-safe doesn’t make the code thread-safe
Thread-safe collections guarantee that each single operation is safe. They say nothing about a sequence of operations, and agents write sequences. Two of the defects above come from that gap.
Microsoft’s documentation for ConcurrentDictionary.GetOrAdd is explicit. The valueFactory delegate is called outside the locks, GetOrAdd is not atomic with regard to other operations on the dictionary, and if it’s called simultaneously on different threads, the factory may run several times while only one key/value pair is added. For a factory that makes an HTTP call, “may run several times” means several HTTP calls, and all but one result is thrown away.
The counter is the classic read-modify-write. _lookups[currency] = _lookups.GetValueOrDefault(currency) + 1 is a read, an add and a write, and the dictionary only protects each half. Two threads read 5, both write 6, and one lookup vanishes.
The third defect isn’t a race at all. A Task that faulted is still a perfectly good value to store, and GetOrAdd stores it. It’s a lifetime bug that looks like a concurrency bug because it lives in the same line.
A likely reason agents write this (opinion, not measured) is that nothing in the prompt said “under load”, so nothing in the output accounts for it. Code that works when one thing happens at a time is the default.
Set up the project
The commands below create a class library and an xUnit project, both on net10.0.
dotnet new sln -n Rates
dotnet new classlib -n Rates -f net10.0 -o src/Rates
dotnet new xunit -n Rates.Tests -f net10.0 -o tests/Rates.Tests
dotnet sln add src/Rates tests/Rates.Tests
dotnet add tests/Rates.Tests reference src/Rates
rm src/Rates/Class1.cs tests/Rates.Tests/UnitTest1.cs
Put the provider interface in the library, next to the service above:
namespace Rates;
public interface IRateProvider
{
Task<decimal> FetchAsync(string currency, CancellationToken ct = default);
}
If you’re reviewing an existing service, skip the new project and add the harness and fake to its existing test project. The only requirement is that the class under test takes its slow dependency through an interface or a delegate, which is a good habit to ask the agent for anyway.
The fake that counts
The fake provider does two jobs. It counts how many times it was called, and it waits 20 milliseconds before answering so the window in which two callers overlap is wide enough to hit reliably. It can also be told to fail its first call.
using Rates;
namespace Rates.Tests;
public sealed class FakeProvider(int failFirst = 0) : IRateProvider
{
private int _calls;
public int Calls => Volatile.Read(ref _calls);
public async Task<decimal> FetchAsync(string currency, CancellationToken ct = default)
{
var n = Interlocked.Increment(ref _calls);
await Task.Delay(20, ct);
if (n <= failFirst) throw new HttpRequestException("upstream 503");
return 1.25m;
}
}
The delay is the cheapest way to turn “one caller in a million loses” into “one caller in two loses”. Real providers have latency, so it isn’t cheating. It’s a fair model of the thing the agent forgot.
The harness: one gate, many callers
Starting 32 tasks in a loop doesn’t make them concurrent. The first task is finished before the last one is scheduled. The harness holds every worker at a gate and opens it once they’re all waiting.
namespace Rates.Tests;
public static class Race
{
public static async Task RunAsync(int workers, Func<Task> work)
{
var gate = new TaskCompletionSource();
var tasks = Enumerable.Range(0, workers)
.Select(_ => Task.Run(async () => { await gate.Task; await work(); }))
.ToArray();
await Task.Delay(50);
gate.SetResult();
await Task.WhenAll(tasks);
}
}
The 50 millisecond delay lets the thread pool get every worker parked on gate.Task. Open the gate and they all resume together, which is the closest a test gets to a burst of requests landing on one instance.
Because a race is probabilistic, each test runs 25 rounds against a fresh service and fails with the round number. A test that passes 24 rounds out of 25 is a failing test.
Three tests, one per defect
using Rates;
namespace Rates.Tests;
public class RateServiceTests
{
private const int Rounds = 25;
private const int Workers = 32;
[Fact]
public async Task Concurrent_first_lookups_hit_the_provider_once()
{
for (var round = 0; round < Rounds; round++)
{
var provider = new FakeProvider();
var service = new RateService(provider);
await Race.RunAsync(Workers, () => service.GetRateAsync("EUR"));
Assert.True(provider.Calls == 1, $"round {round}: provider called {provider.Calls} times");
}
}
[Fact]
public async Task Lookup_counter_does_not_lose_updates()
{
for (var round = 0; round < Rounds; round++)
{
var service = new RateService(new FakeProvider());
await service.GetRateAsync("EUR");
await Race.RunAsync(Workers, () => service.GetRateAsync("EUR"));
Assert.True(service.Lookups("EUR") == Workers + 1,
$"round {round}: expected {Workers + 1}, got {service.Lookups("EUR")}");
}
}
[Fact]
public async Task A_failed_fetch_is_not_cached()
{
var service = new RateService(new FakeProvider(failFirst: 1));
await Assert.ThrowsAsync<HttpRequestException>(() => service.GetRateAsync("EUR"));
Assert.Equal(1.25m, await service.GetRateAsync("EUR"));
}
}
The counter test warms the entry first. That keeps it independent of the cache-stampede defect, so a failure there means the counter is wrong and nothing else.
The assertion messages matter more than they look. Assert.Equal(1, provider.Calls) fails with “Expected: 1, Actual: 2” and nothing about when. Putting the round and the observed value in the message is what lets you paste the failure straight back to an agent.
Run it against the agent’s code
dotnet test
Against the version above, all three fail. The relevant lines of the output:
Failed Rates.Tests.RateServiceTests.A_failed_fetch_is_not_cached [38 ms]
Error Message:
System.Net.Http.HttpRequestException : upstream 503
Failed Rates.Tests.RateServiceTests.Concurrent_first_lookups_hit_the_provider_once [76 ms]
Error Message:
round 0: provider called 2 times
Failed Rates.Tests.RateServiceTests.Lookup_counter_does_not_lose_updates [76 ms]
Error Message:
round 0: expected 33, got 32
Two things stand out. Every race test failed in round 0, so on a four-core machine these defects aren’t rare events needing thousands of iterations. And the provider was called twice, not 32 times. GetOrAdd only runs the factory again for callers that arrive before the first insert completes, so the damage is smaller than the worst case but the cache is still wrong, and a rate limiter on the provider’s side would still notice.
Now the prompt. Give the agent the failure output, not a description of it:
These three tests fail against RateService. Fix RateService only. Do not edit the
tests, FakeProvider or Race. Constraints: one provider call per currency no matter
how many concurrent callers, no lost lookup counts, and a failed fetch must not be
cached. A caller cancelling its own token must not cancel the shared fetch.
Run `dotnet test` and show me the result.
The last constraint is there on purpose. Without it, a tempting “fix” passes the caller’s token into the shared fetch, so one impatient caller cancels the lookup for everyone waiting on it. That’s a fourth defect the three tests don’t cover, which is a reason to say it in the prompt and, if it matters to you, add a test.
What a correct fix looks like
This version passes. It’s what the documentation points towards, not the only possible answer:
using System.Collections.Concurrent;
namespace Rates;
public sealed class RateService(IRateProvider provider)
{
private readonly ConcurrentDictionary<string, Lazy<Task<decimal>>> _rates = new();
private readonly ConcurrentDictionary<string, int> _lookups = new();
public async Task<decimal> GetRateAsync(string currency, CancellationToken ct = default)
{
_lookups.AddOrUpdate(currency, 1, (_, n) => n + 1);
var entry = _rates.GetOrAdd(currency,
c => new Lazy<Task<decimal>>(() => provider.FetchAsync(c)));
try
{
return await entry.Value.WaitAsync(ct);
}
catch when (entry.Value.IsFaulted)
{
_rates.TryRemove(KeyValuePair.Create(currency, entry));
throw;
}
}
public int Lookups(string currency) => _lookups.GetValueOrDefault(currency);
}
Each piece answers one defect. The dictionary now stores a Lazy<Task<decimal>>. GetOrAdd may still build several Lazy wrappers under contention, but a wrapper is cheap and does nothing until .Value is read, and Lazy<T> built with a factory is thread safe by default, so only one wrapper’s factory ever runs. Only the winner is stored, and everyone reads the winner’s .Value.
AddOrUpdate does the read and the write as one operation. The documentation example for GetOrAdd uses it to count 10,000 competing updates and prints the total it expects.
WaitAsync(ct) lets each caller stop waiting on its own token without touching the fetch itself. And the catch when (entry.Value.IsFaulted) filter removes a failed entry, but only if it’s the faulted fetch that threw. A caller that merely cancelled leaves the in-flight entry alone. TryRemove with a KeyValuePair removes the entry only if the dictionary still holds that same wrapper (a mismatched value returns false and removes nothing), so a fast retry that has already replaced it isn’t evicted by a slow loser.
Here’s the part that surprises people. The Lazy documentation says factory exceptions are cached, which sounds like the failed-fetch bug all over again. It doesn’t apply here: the factory returns a Task without throwing, and the failure lives inside the task. That’s why the explicit removal is needed.
Run it:
dotnet test
Passed! - Failed: 0, Passed: 3, Skipped: 0, Total: 3, Duration: 3 s - Rates.Tests.dll (net10.0)
Run it three more times without rebuilding. It passed each time. That’s the check that matters, because a race fix that passes once is still a guess.
Make it yours
For a hand-rolled lock rather than a concurrent collection, the same harness works unchanged: replace the service call and assert on whatever invariant the lock should protect, such as a balance that must equal the sum of its deposits.
For code that doesn’t await, Race.RunAsync still works, because Task.Run puts each worker on a pool thread. The gate does the rest.
For a service registered as a singleton in ASP.NET Core, test the class directly as above. A WebApplicationFactory test is slower and adds noise. The race lives in the class, and the singleton registration is only what makes it shared.
The review step that goes with the harness is a short search. Before reading the logic of any agent-written class that holds state, grep it for the patterns that deserve a test:
grep -rnE "GetOrAdd|ContainsKey|TryGetValue|\+\+|\+= |static .*(List|Dictionary)<" src --include=*.cs
Every hit is a question. Is a sequence of operations being treated as one? Is a value being read, changed and written back? Is a mutable static shared across requests? Each yes gets a test with the gate in it.
What goes wrong
A passing stress test proves the absence of the failures you provoked, not of every failure. The harness finds check-then-act and read-modify-write bugs in the code paths you exercise. It won’t find a lock held across an await, a deadlock that needs a particular ordering, or a memory-model problem that only appears on other hardware. Four cores in a CI container are not sixteen in production.
The delay in the fake is doing a lot of work. If your real dependency answers in 200 microseconds, the overlap window is narrower and a race may need more rounds to show. Raise Rounds rather than trusting a green run, and keep the fake’s delay if you want the test to fail fast.
Timing-based tests can flake in the other direction. The 50 millisecond settle in Race is generous on a laptop and not guaranteed on a starved CI agent. If it flakes, the gate is opening before workers are parked, which makes the test weaker rather than wrong. Raise the delay or count arrivals with a CountdownEvent.
Don’t bother if the object is created per request and never shared. A scoped service that holds no static state can’t race with itself, and a harness around it tests nothing.
The first 15 minutes
Pick the most recent agent-written class in your codebase that holds a field, a static or a cache. Copy Race.cs and a counting fake into its test project. Write one test: 32 callers behind the gate, one assertion on the invariant the class is supposed to keep. If it passes 25 rounds, you’ve learned something cheap. If it fails, paste the round number and the numbers into the agent, tell it which files it may not touch, and let it fix the code.