Root Cause, Not Workaround
The first fix the agent proposes for the double offer is plausible: check for an existing offer just before sending one. The step-driven test from the start of this chapter fails on it at once, because the new check is one more read before the same write. Root cause means asking why until the answer is a mechanism you can point to in the code, and then fixing that mechanism rather than the symptom it produces.
An agent is very good at producing the first plausible fix and has no reason of its own to stop there. The test does not care how convincing the fix sounds. That is the arrangement this page relies on: the agent proposes, the deterministic test disposes, and you ask the questions that move the work from the symptom to the cause.
The First Plausible Fix
Asked to make the red test pass, the agent's first diff adds one line of defence. Just before writing the offer, it reads the class again and returns early if an offer already exists. The explanation is clean: "the second path will now see the first path's offer and stop". The diff is a line or two. It reads like the fix a careful person would write.
--- a/waitlist.py
+++ b/waitlist.py
@@ -14,6 +14,8 @@
state = store.read_state(class_id) # step 1: read
if state.free_places < 1 or state.active_offers > 0:
return None
+ if store.read_state(class_id).active_offers > 0: # check again just before sending
+ return None
member_id = store.offer_to_next(class_id) # step 2: write
if member_id is not None:
log.info("offer class=%s member=%s path=%s free_places=%d active_offers=%d",E AssertionError: assert 2 == 1 E + where 2 = len(['W1', 'W2']) test_double_offer.py:39: AssertionError ============================== 1 failed in 0.35s ===============================
--- a/waitlist.ts
+++ b/waitlist.ts
@@ -10,6 +10,7 @@
export async function offerFreePlace(store: Storage, classId: string, path: string) {
const state = await store.readState(classId); // step 1: read
if (state.freePlaces < 1 || state.activeOffers > 0) return undefined;
+ if ((await store.readState(classId)).activeOffers > 0) return undefined; // check again
const memberId = await store.offerToNext(classId); // step 2: write
if (memberId !== undefined) {
console.info(`offer class=${classId} member=${memberId} path=${path} ` +FAIL doubleOffer.test.ts > one offer per freed place AssertionError: expected [ 'W1', 'W2' ] to have a length of 1 but got 2
--- a/Waitlist.java
+++ b/Waitlist.java
@@ -17,6 +17,7 @@
public static Optional<String> offerFreePlace(Storage store, String classId, String path) {
ClassState state = store.readState(classId); // step 1: read
if (state.freePlaces() < 1 || state.activeOffers() > 0) return Optional.empty();
+ if (store.readState(classId).activeOffers() > 0) return Optional.empty(); // check again
Optional<String> member = store.offerToNext(classId); // step 2: write
member.ifPresent(m -> log.info(String.format(
"offer class=%s member=%s path=%s free_places=%d active_offers=%d",DoubleOfferTest > oneOfferPerFreedPlace() FAILED
org.opentest4j.AssertionFailedError: one offer per freed place ==> expected: <1> but was: <2>The extra check fails for the same reason the original code did. It is a read. The race is two reads before two writes, and adding a third and fourth read before the same two writes changes how often the collision happens, not whether it can. In production the check would have made the double offer rarer, perhaps much rarer, and that is exactly the problem: Kai's two-second guard also made it rarer, and it shipped because nothing could show it was not a fix.
Every symptom fix for a race has this property. An extra check, a retry, a catch on a duplicate-offer error, a short delay: each shrinks the window, each leaves two separate steps with nothing joining them, and each fails the step-driven test, because the test does not depend on the window's size. Without the test, the first of these would have shipped, as the guard did.
Asking Why Until It Is a Mechanism
The technique is old and simple: ask why, and do not accept an answer that is only another symptom. Two members got offers for one place. Why? Because two paths each sent one. Why? Because each saw a free place with no active offer. Why? Because each read the class before either wrote. Why could both read before either wrote? Because reading the state and writing the offer are two separate calls to storage, with nothing that makes them one step.
That last answer is where the questioning stops, and the stopping rule matters. It names a line range, the read and the write in the offer path, and a missing guarantee, that nothing makes those two lines indivisible. You can point at it. An answer like "a timing issue" or "the sweep is too aggressive" cannot be pointed at, so it is not yet a cause. Note what kind of cause it is: no single line is wrong. Each of the two lines does what it says. The defect is a relation between them, which is why reading either line alone never finds it.
Fixing the Mechanism
Once the cause is a missing guarantee, the fix is to supply it: make "check that the place is still unoffered, and record the offer" a single indivisible step. There are three standard ways. An update that succeeds only if the place is still unoffered, and reports whether it did. A lock on the class while the decision is made. Or a rule in the storage layer that refuses a second active offer for one place, so the second write fails loudly. Choosing among them is ordinary backend work, covered as the same race on a seat hold in Backend Deep Dive, Topic 34, "The Oversell — Concurrency at the Row". The agent can implement any of the three. Which one fits is your call, and the test judges the result.
Here the conditional update fits best. The offer path keeps its early read, which saves a write in the common case, but the guarantee moves into one storage call that checks and records together. In the database that call is a single conditional update; in the in-memory store the tests use, the same rule runs under the store's lock.
--- a/waitlist.py
+++ b/waitlist.py
@@ -14,7 +14,7 @@
state = store.read_state(class_id) # step 1: read
if state.free_places < 1 or state.active_offers > 0:
return None
- member_id = store.offer_to_next(class_id) # step 2: write
+ member_id = store.offer_if_unoffered(class_id) # check and write: one step
if member_id is not None:
log.info("offer class=%s member=%s path=%s free_places=%d active_offers=%d",
class_id, member_id, path, state.free_places, state.active_offers)--- a/memory_store.py
+++ b/memory_store.py
@@ -23,9 +23,12 @@
free = self.capacity[class_id] - len(self.booked[class_id])
return ClassState(free, len(self.offers[class_id]))
- def offer_to_next(self, class_id):
- """Offers to the first waiting member who has no offer yet."""
+ def offer_if_unoffered(self, class_id):
+ """Offers to the next waiting member only while a free place has no offer."""
with self.lock:
+ free = self.capacity[class_id] - len(self.booked[class_id])
+ if len(self.offers[class_id]) >= free:
+ return None
offered = set(self.offers[class_id])
member = next((m for m in self.waitlist[class_id] if m not in offered), None)
if member is not None:test_double_offer.py . [100%] ============================== 1 passed in 0.05s ===============================
--- a/waitlist.ts
+++ b/waitlist.ts
@@ -3,14 +3,14 @@
export interface Storage {
freePlace(classId: string, memberId: string): Promise<void>;
readState(classId: string): Promise<ClassState>;
- offerToNext(classId: string): Promise<string | undefined>;
+ offerIfUnoffered(classId: string): Promise<string | undefined>;
classesWithFreePlaces(): Promise<string[]>;
}
export async function offerFreePlace(store: Storage, classId: string, path: string) {
const state = await store.readState(classId); // step 1: read
if (state.freePlaces < 1 || state.activeOffers > 0) return undefined;
- const memberId = await store.offerToNext(classId); // step 2: write
+ const memberId = await store.offerIfUnoffered(classId); // check and write: one step
if (memberId !== undefined) {
console.info(`offer class=${classId} member=${memberId} path=${path} ` +
`free_places=${state.freePlaces} active_offers=${state.activeOffers}`);--- a/memoryStore.ts
+++ b/memoryStore.ts
@@ -22,9 +22,11 @@
return { freePlaces: free, activeOffers: this.offers.get(classId)!.length };
}
- /** Offers to the first waiting member who has no offer yet. */
- async offerToNext(classId: string) {
+ /** Offers to the next waiting member only while a free place has no offer. */
+ async offerIfUnoffered(classId: string) {
const offers = this.offers.get(classId)!;
+ const free = this.capacity.get(classId)! - this.booked.get(classId)!.length;
+ if (offers.length >= free) return undefined;
const member = this.waitlist.get(classId)!.find((m) => !offers.includes(m));
if (member !== undefined) offers.push(member);
return member; Test Files 1 passed (1)
Tests 1 passed (1)--- a/Waitlist.java
+++ b/Waitlist.java
@@ -10,14 +10,14 @@
public interface Storage {
void freePlace(String classId, String memberId);
ClassState readState(String classId);
- Optional<String> offerToNext(String classId);
+ Optional<String> offerIfUnoffered(String classId);
List<String> classesWithFreePlaces();
}
public static Optional<String> offerFreePlace(Storage store, String classId, String path) {
ClassState state = store.readState(classId); // step 1: read
if (state.freePlaces() < 1 || state.activeOffers() > 0) return Optional.empty();
- Optional<String> member = store.offerToNext(classId); // step 2: write
+ Optional<String> member = store.offerIfUnoffered(classId); // check and write: one step
member.ifPresent(m -> log.info(String.format(
"offer class=%s member=%s path=%s free_places=%d active_offers=%d",
classId, m, path, state.freePlaces(), state.activeOffers())));--- a/MemoryStore.java
+++ b/MemoryStore.java
@@ -22,8 +22,10 @@
return new Waitlist.ClassState(free, offers.get(classId).size());
}
- /** Offers to the first waiting member who has no offer yet. */
- @Override public synchronized Optional<String> offerToNext(String classId) {
+ /** Offers to the next waiting member only while a free place has no offer. */
+ @Override public synchronized Optional<String> offerIfUnoffered(String classId) {
+ int free = capacity.get(classId) - booked.get(classId).size();
+ if (offers.get(classId).size() >= free) return Optional.empty();
Optional<String> member = waitlist.get(classId).stream()
.filter(m -> !offers.get(classId).contains(m)).findFirst();
member.ifPresent(offers.get(classId)::add);BUILD SUCCESSFUL in 850ms
The test is unchanged and still forces both reads before both writes. Both paths still see one free place and no offer. The first write succeeds; the second finds the place offered and records nothing. That is what green means here: not that the collision became unlikely, but that the forced collision no longer produces a second offer.
Asking So You Get a Cause
The request that produces this kind of fix rules out the other kind in advance: do not add retries, delays or catches; first describe the interleaving that produces two offers, step by step, with the lines involved; then change the code so that interleaving cannot happen. The first half makes the agent commit to a mechanism you can check against the code before any diff exists. The second half names the shape of an acceptable fix.
Every proposed fix is still read for the shapes from Chapter 7. A new catch, a new retry, a new check that skips a case, a new delay: each needs a stated reason, and "the test passes now" is not one, because the test was written to fail only on the interleaving you forced, and a fix can sometimes satisfy the test while dodging the rule. The agent's explanation of its fix is a hypothesis like any other; the evidence is red before the change and green after it.
Removing the Old Guard
Kai's two-second pause treated the symptom. With the real fix in place it does nothing useful, and it does harm in two ways. It delays every offer the sweep makes after a cancellation, for no reason anyone can now state. And it tells the next reader that the problem is handled twice, which invites the next person to "simplify" the wrong half. Take it out on the same branch, in its own commit, and run the test again: still green, which is the proof the guard was never what kept the offers apart.
Looking for Siblings
A race of this shape rarely lives alone, because the person or agent who wrote it once wrote the surrounding code in the same style. At Cragmoor the booking path has the same read-then-write for the 16th place: it reads the number of bookings, sees 15, and inserts a booking. Two members booking the last place in the same moment can both succeed, and the class holds 17.
Ask the agent to find every place in the service that reads a count or a state and then writes based on it in a separate call, and to list them with file and line. Then write a step-driven test for each one before fixing any of them. Some will turn out to be safe for a reason the list missed; the tests are what tell those apart from the real ones, and the real ones get the same kind of fix.
- "If the symptom stops, the cause is gone." The guard made two offers rarer, not impossible, and rare is how this bug lived for two months between incidents.
- "Adding a check fixes a race." A check is one more read. Unless the check and the write are one indivisible step, the race moves inside the gap and stays open.
- "The agent's explanation of the cause is the cause." It is a hypothesis until the deterministic test fails before the fix and passes after it. An explanation is generated text, however well argued.
- "Root cause analysis is for post-mortems." It is the step that decides which fix to write. Done after the fix, it is a report about a decision that was already made.
- "A root cause is always one wrong line." Here it is a missing guarantee between two correct lines. Mechanisms are often relations between lines, which is why reading line by line misses them.
- Ask why until the answer names a mechanism in the code: a line range and the guarantee it lacks. An answer you cannot point at is still a symptom.
- Judge every proposed fix by the deterministic repro, never by its explanation. The test does not get more convinced by better wording.
- Rule out retries, delays and catches for a race in the request itself. They are the agent's shortest path to green, and each one hides the cause.
- Search for the same shape elsewhere once the cause is known, and write a failing test for each instance before fixing any of them.
Knowledge Check
The agent adds a second "is there already an offer?" check just before the offer is written. Why does the step-driven test stay red?
- The check is another read, and the test holds both paths until each has read, so both still pass it
- The test double caches the first read, so the new check always sees the stale state from before the cancellation
- The check runs after the offer has been recorded, so it can only ever find the same path's own offer
- The test asserts on the member's identity rather than the count, so any reordering makes it fail
You keep asking why the double offer happens. Which answer is the right place to stop?
- "The sweep runs far too often and keeps colliding with cancellations in the busiest full classes"
- "There is a timing problem somewhere between the cancellation path and the 30-second sweep"
- "Reading the state and writing the offer are two separate storage calls with nothing joining them"
- "Kai's guard waits only two seconds, which is not long enough for the slowest cancellations to finish"
Which proposed fix addresses the mechanism rather than the symptom?
- Retry the offer step up to three times whenever two offers are detected for one freed place
- Wait five seconds instead of two before the sweep offers any place freed by a recent cancellation
- Catch the duplicate-offer error and withdraw the second offer before its email goes out to the member
- Record the offer with one storage call that succeeds only if the place is still unoffered
The real fix is merged and green. Why should Kai's two-second guard be removed too?
- It makes the test fail now, because the delay pushes the sweep's write past the test's five-second timeout
- It now only delays offers and suggests the problem is handled twice, inviting the wrong cleanup later
- It conflicts with the conditional update, because both of them try to lock the same class at the same moment
- It hides the double offer in the logs, so any future incidents could not be traced back to their real cause
With the cause known, what is the right next step for the rest of the service?
- Ask the agent to apply the conditional-update pattern to every single write in the service in one change
- Assume the race was unique to the waitlist, since the booking module is older and much better tested by now
- Wait for a second incident in some other module before spending any time looking for races of the same shape
- Ask the agent to list every read-then-write on shared state, and write a failing test for each first
You got correct