This is a submission for DEV's Summer Bug Smash: Smash Stories powered by Sentry.
## The bug that doesn't report itself
testsprite-cli stores API keys in a credentials file, one entry per profile. Writing a
profile looked like this:
mutateCredentialsFile(path, file => {
file[profile] = { ...file[profile], ...entry };
return file;
});
Read the whole file, mutate in memory, write the whole thing back via renameSync.
The rename is atomic, and that's what makes this interesting — it's not a corruption bug.
The file is never left half-written. Someone thought about atomicity and got it right.
What the rename doesn't decide is whose snapshot wins. Two writers, both read the same
starting state, both add their own profile, second renameSync lands last. The first
profile is gone.
Not corrupted. Not errored. Gone.
$ testsprite setup --profile staging &
$ testsprite setup --profile prod &
$ # both exit 0
$ # one of them didn't happen
Nothing raises. Nothing exits non-zero. You find out later, when a command that needs
staging reports AUTH_REQUIRED, and you assume you typo'd something and set it up again.
That's the part that stuck with me. The failure mode isn't "it broke," it's "it quietly
didn't happen, and the error surfaces somewhere else, later, attached to a different
command." A user hitting this has no path back to the cause.
And it isn't exotic usage. Two shells. A CI job overlapping a local setup. An agent running
commands in parallel — which, for a CLI whose whole job is being driven by an agent, is
approximately the normal case.
## The fix, which was the easy part
An advisory lock file held across the entire read-modify-write cycle, not just the write:
writeFileSync(lockPath, token, { flag: 'wx' });
wx is O_EXCL — it succeeds for exactly one process and throws EEXIST for everyone
else. Losers spin until a 5-second timeout.
Two details that aren't optional:
Stale lock reclaim. A process killed mid-write leaves the lock file behind and every
future write hangs. So a lock past a threshold gets reclaimed.
Re-checking ownership after the wait. Reclaim introduces its own race: while you were
waiting, someone may have decided your lock was stale and taken it. So immediately before
the write, assertHeld() re-reads the lock file and confirms the token is still yours.
That merged as #272. Tests passed.
Done.
## Except the tests couldn't have failed
Here's what I noticed afterward, and it's the actual story.
The suite had tests for the credentials module. They passed. They also would have passed
with the entire lock deleted — because every branch of that lock is cross-process by
construction, and the tests all ran in one process.
Think about what you'd need to reach:
- The
EEXISTpath needs a second process already holding the lock. - The wait loop needs contention that lasts longer than one process's critical section.
- Stale reclaim needs a lock file left by a process that died.
-
assertHeld()failing needs a third party to steal ownership mid-flight.
Not one of those is reachable from a single process. An in-process mutex wouldn't even have
fixed the original bug, and an in-process test can't reach the fix that did.
So the lock was real code protecting a real race, sitting behind a green suite that was
structurally incapable of noticing if it broke. Delete the lock, tests still pass, bug
comes back silently.
That's a worse position than having no test, because the green check reads as a claim.
## Making a race lose on purpose
#280 is tests only — no src/
changes, nothing ships to users.
It spawns eight real child processes — spawn(process.execPath, [CHILD_PATH]), not
worker threads, not a mocked fs — each writing a distinct profile to one shared
credentials file.
And spawning them is not enough, which is the trap.
Eight Node processes come up staggered by tens of milliseconds. The critical section is
shorter than that. Left alone they queue politely, one after another, never contend, and
the test passes without ever touching the lock — the same failure it was written to rule
out, one level up.
The fix is a starting gate:
- Each child writes a
<profile>.readymarker, then spin-waits for a sharedstartfile. - The parent blocks until all eight ready markers exist.
- The parent writes
start, releasing all eight into the critical section together.
The children sleep between polls with Atomics.wait on a throwaway SharedArrayBuffer
rather than setTimeout — a genuine blocking sleep, with no event-loop scheduling sitting
between the release and the write.
Now they collide on purpose. All eight profiles have to be present at the end, and no lock
file may remain.
## The other thing a concurrency test has to prove
A concurrency test that quietly stops testing anything is worse than no test — which is the
same hazard as the staggered-start one, wearing a different hat. Eight children exiting
early for an unrelated reason produces a perfectly green run that asserts nothing.
So the child's contract is pinned by exit code:
| Case | Exit | Asserted |
|---|---|---|
| Missing required env vars | 1 | names all four required vars |
| Start marker never appears | 2 | Timed out waiting for start marker: <path> |
| writeProfile rejects (bad] profile name) | 3 | stderr contains Invalid request. |
Without these, one typo'd env var name makes all eight children bail before they write
anything, and the test reports success.
Every case builds its own mkdtempSync root and removes it in finally; nothing touches
the real credentials path. Timeouts are bounded — 10s per child, 20s for the concurrent
case, 15s for the timeout case — because an unbounded concurrency test in CI is its own
kind of outage.
## What I took from it
The two PRs are the same length of work and only one of them was hard. Writing the lock was
mechanical once the race was clear. Building a situation where the lock visibly matters
took a starting gate, a blocking sleep primitive, and a set of exit codes whose only job is
to stop the test from lying.
The pattern I keep running into, in this repo and elsewhere:
A fix isn't verified until you've watched the test fail without it.
For a race, that's not a figure of speech — it's the entire engineering problem, because
the default outcome of a concurrency test is that it passes for the wrong reason.
Both PRs are merged: #272 (the
lock, from issue #77) and
#280 (the cross-process test).
I also wrote up a null-propagation crash class in Formbricks
for Clear the Lineup, which ended in the same place from the other direction: a fix that
looked complete, and a green suite that wasn't checking the thing I'd claimed.
