Skip to content
All posts

Hundreds of passing tests couldn't see this bug. Here's why.

Agent Loopr: Hundreds of passing tests couldn't see this bug — two process threads racing toward the same point and colliding

We shipped a lock.

It looked right. It was reviewed. All 1,018 tests in our suite passed against it, green the entire time the bug existed, including the ones written specifically to exercise the locking.

Then someone spawned twenty processes and raced them against each other. It failed completely. Every single time.

This is a writeup of that bug. The reason our tests missed it is a property of single-process test suites in general, and I hadn't internalized how completely one can hide a defect until I watched it happen.

The setup

This came out of open-sourcing an ops pipeline we run internally, the one that turns meeting transcripts and channel logs into task-board updates. Preparing it for other people to run is what surfaced the bug. A codebase somebody else might deploy differently than you do gets audited differently than one you only ever run yourself.

Between runs the pipeline persists several things to disk. Which deliveries it has already seen. Corrections a human has recorded. The role registry. Per-role memory.

Three of those (the idempotency store, corrections, and the registry mutator) shared the same underlying primitive for "read this file, modify it, write it back safely." A fourth, per-role memory, turned out to have no lock at all. Different defect, worse one, same audit.

That primitive had a lock. Roughly this:

function makeFifoLock() {
  let chain = Promise.resolve();
  return (fn) => {
    const run = chain.then(fn, fn);
    chain = run.then(() => undefined, () => undefined);
    return run;
  };
}

This is a real lock. It genuinely serializes async work: call it twice in the same process and the second call really does wait for the first to finish. Completely standard pattern. If you'd shown it to me in a code review I would have approved it without a second thought.

The problem is one word: process. This lock orders promises inside one JavaScript process. The files it was protecting exist specifically because more than one process touches them: a second CLI invocation approving a hold, a second worker processing a delivery. An in-process lock cannot see another process at all. It provides exactly zero protection against that case, while looking, reading and behaving in every local test exactly like a lock that works.

Why nothing caught it

Every test in the suite ran inside one Node process. Two "concurrent" calls in a test are two calls inside the same event loop, ordered by the same in-process primitive the test is supposed to be checking. The test and the implementation shared the exact same blind spot, because they were, definitionally, the same process.

A bug where the fix and the flaw live at the same layer of abstraction is invisible to any test written at that layer. Raise coverage. Add edge cases. Run the suite a thousand times. None of it touches the axis the bug lives on. The only way to see it is to step outside the process boundary the bug depends on and observe from there.

What it actually looked like broken

We built a probe. Spawn N real child processes, have each one open the same idempotency store, race them all against a single delivery key at the same instant. Two rounds:

state file sizeprocesses that accepted the delivery as new
empty2–4 out of 20
~75,000 records20 out of 20

The empty-file case looks almost fine. Mostly the OS scheduler happened to interleave things such that a real collision was rare. The grown-file case is total failure, every time, because the read-modify-write window is a JSON parse plus a stringify, and that window scales with file size.

Our persistence layer had also never called its own prune() function outside of its unit test. In a real deployment, that file only grows. The race window gets wider every day the process stays up.

The fix, and the fix's own bug

The actual fix is almost boring. A real cross-process lock, using openSync(path, 'wx') (an atomic "create this file or throw EEXIST" syscall) as a mutex, with a stale-lock recovery path for a process that dies mid-write. One lock, shared by every file the pipeline persists.

What's less boring is that the test for that fix had a bug of the same shape as the original bug, one layer up.

The first version of the process-racing test picked a fixed start time. Everyone begins at T+4 seconds. Each child imports its dependencies, then waits for the deadline. It ran green.

Then, still checking our own work, we deliberately skewed one child's startup by five full seconds and reran it against code with no lock at all.

It still passed.

The children were still compiling their imports when the fixed deadline arrived, so they finished at wildly different real times and never actually collided. The test had accidentally serialized its own "concurrent" workers, and a broken lock looked fine because nothing was really racing. We'd built a concurrency test whose passing result told us nothing about concurrency, in the same way the original in-process lock told us nothing about cross-process safety. The check and the thing it was checking shared a hidden assumption.

The real fix is a two-phase barrier. Every child process writes a "ready" file once its imports are done. The parent waits until every single child has signalled ready. Only then does it release them all at once, by writing a shared "go" file the children are spin-waiting on. No fixed deadline anywhere.

Re-run under that same five-second skew with the lock removed, sixteen workers this time: all sixteen claim the same key, every round. The mutant dies where it used to survive.

Five instances, one bug

That lock went through five separate rounds of this. Five instances of the same class of mistake in one lock's history, each surfaced only after the previous fix landed.

An ownership check that could leave a lock permanently stuck. A race inside the stale-lock recovery path itself. A catch block that swallowed errors and turned one real failure into thousands of silent retries.

Every one of them was an interaction between two rules that were each individually correct. "Fixing one instance of a bug is not fixing the bug" became close to a house motto by the end of the audit.

The general shape

Two lessons, in order of how expensive they were to learn.

  1. An in-process primitive protecting a cross-process resource looks exactly like a correct implementation, including in every test written at the same process boundary as the bug.
  2. A concurrency test has to prove it actually raced. Skew your workers' startup time on purpose and confirm the test still fails against known-broken code. If it doesn't, you don't have a concurrency test. You have a test that runs several things and gets a plausible-looking result.

The repo this came from is open source (Triage), and the full commit, with the before/after and the exact numbers above, is 5a3ae48 ("One lock, held across processes, for every file this repo writes") if you'd rather read the diff than take our word for the shape of it.

Ready to talk about
what you're building?

Book a 30-minute call and come as you are. No preparation needed on your end.

Book a Discovery Call