The race condition I didn't know I'd fixed

There is a two-line guard in my payment webhook handler that stops a customer who has paid from being told they haven’t. I didn’t write it, and until this week I didn’t know it was there.

I want to be precise about what that does and doesn’t mean, because the two available takes on it, that it’s amazing or that it’s slop, are both too easy.

First, the bug it prevents

Quibble sells a compatibility test. You answer some questions, toilet paper over or under, pineapple on pizza, where the thermostat lives, then send a link to someone and they answer the same questions. A test costs $0.99, or $4.99 for the unlimited one. Small amounts, which matters for what follows.

The payment provider sends invoice status updates by webhook. processing, then success. Obvious enough.

Except they are independent HTTP requests over the public internet, and nothing guarantees the order they land in. A processing webhook can arrive after the success webhook for the same invoice. Retries make it likelier: the first delivery times out, gets queued, and shows up late.

Apply them in arrival order and here is the sequence.

1. Customer pays. success lands. Invoice marked paid. Everyone is happy.
2. The delayed processing webhook lands.
3. Invoice status is overwritten back to processing.
4. Customer refreshes and is looking at a payment wall for something they already bought.

The money is in your account. Your database says otherwise. You find out from a support email, if the customer bothers, which at a dollar they mostly won’t. They will just leave, and you will never learn why.

Here is the guard, at the top of the handler:

// Out-of-order delivery: never let a stale "processing" overwrite "success".
if ($invoice->isPaid() && ($status['status'] ?? null) !== Invoice::SUCCESS) {
    return response()->json(['ok' => true, 'handled' => 'stale']);
}

Paid is terminal. Once an invoice reaches success, nothing walks it back except an explicit refund flow. Two lines, and the class of bug is gone.

This is not a quirk of my provider. Stripe documents at-least-once delivery with no ordering guarantee too. If you have a webhook handler that writes state, you either have this guard or you have this bug, and there is a decent chance you have not checked which.

Where the code came from

One commit, 30 August, a048dbd7, replacing RevenueCat with monobank acquiring. Co-authored by Claude Fable 5. Three hundred lines of integration, a migration, a service, a controller, a rewrite of the payments layer, and 279 lines of tests. Written in an afternoon.

Everything I described above was in that commit. Not just the guard: the comment above it explaining why it is there, and a test that fires a late processing at a paid invoice and asserts the response comes back handled: stale, with its own comment saying “mono retries, and a late processing never undoes a success.”

I reviewed that commit. I read the diff, I ran the tests, I paid myself a dollar on staging and watched it work. I merged it because it worked.

I did not notice those two lines. If you had asked me last week what my webhook handler does about out-of-order delivery, I would not have had an answer, because I did not know the question was live.

The part that actually bothers me

Not that the AI wrote correct code. That happens constantly now and it is not interesting on its own.

What bothers me is the shape of my review. I checked that the happy path worked. I checked the tests were green. Both of those things are true of code with this bug in it, because the bug only appears under a race that a staging test with one buyer and a good network will never produce. My review would have passed the broken version too.

So the honest summary is not “the AI wrote a subtle race condition guard.” It is: I have no process that would have caught the absence of that guard. It happened to be right. I found out by accident, a week later, when something read my own code back to me.

That is a different problem than the one people usually argue about. The question is not whether the model is good enough. On this evidence it was better than me, on this specific thing, on that specific day. The question is that reviewing generated code by reading the diff and running the tests only catches the things you already knew to look for, and the whole value of the generated code is the things you didn’t.

I don’t have a clean answer. Two things I have changed:

Review by category, not by line. For anything that touches money or state transitions, I now write down the list of hard questions before I read the diff. Idempotency. Ordering. Retries. Partial failure. What is terminal. Then I check the code against the list instead of reading it forward and nodding. It is slower and it is not clever, and it is the only version of this that works, because a list I wrote in advance is not influenced by what the code happens to contain.

Assume the guards are load-bearing. The thing I nearly did, and the reason this post exists, is treat unfamiliar defensive code as noise to be cleaned up later. Every one of those early-return blocks is somebody’s story. If I can’t say what breaks when I delete it, I am not allowed to delete it.

Why I am telling you this

I maintain bullshit-detector, a tool whose core rule is that no claim gets a verdict without a source. Its README says, in plain text, that it has no evaluation harness and that the only evidence of its accuracy is reports it wrote about content I picked. I put that there because a fact-checking tool that oversells itself is worse than none.

This is the same standard applied one level down. I shipped a payment integration that is more correct than my understanding of it. That is worth saying out loud, because a lot of people are currently in exactly this position and describing it as “I built a payment integration in an afternoon,” which is true and is not the whole sentence.

Two more traps in the same integration

Both also from that commit, both things I now actually understand.

The signing key rotates and there is no event for it.

Webhooks arrive with an X-Sign header: base64 of an ECDSA-SHA256 signature over the request body. The raw body. Parse the JSON and re-encode it before verifying and it fails every single time, in a way that looks exactly like an attacker probing you.

The subtler one: the provider rotates its public key. Rarely, but it does, and there is no rotation event to subscribe to. Cache the key, which you should, otherwise every webhook costs an extra round trip, and rotation silently turns every future payment into a 401. Your logs fill with signature failures and your first instinct is to look for an attacker.

The fix is boring, which is the point:

foreach ([false, true] as $fresh) {
    $pem = $this->publicKey($fresh);
    if ($pem && openssl_verify($body, $binary, $pem, OPENSSL_ALGO_SHA256) === 1) {
        return true;
    }
}

return false;

Verify with the cached key. On failure, drop the cache, fetch fresh, verify once more. Worst case one extra HTTP call. Rotation stops being an outage.

A 404 on an unknown invoice triples your own traffic.

mono retries a webhook up to three times until it sees a 200. Sensible design. But the instinct when a webhook references an invoice you don’t recognise is to 404 it, and now every stray delivery is three deliveries, and the pattern in your logs looks like someone hammering your endpoint.

if ($invoice === null) {
    Log::warning('mono webhook for unknown invoice', [...]);

    return response()->json(['ok' => true, 'handled' => 'unknown']);
}

200 means “received”, not “I agree with you”. Worth separating those early, because the retry behaviour makes the wrong choice self-amplifying.

On the provider itself. I used monobank internet acquiring, “plata by mono”. Two reasons, neither dramatic: Stripe does not operate in Ukraine, and my sole-proprietor account is already at mono, so settlement lands where my money already lives. Of what was available to me it was the friendliest by a distance.

The whole surface is three endpoints. POST /api/merchant/invoice/create takes an amount and returns an invoiceId and a pageUrl. GET /api/merchant/invoice/status reads an invoice. GET /api/merchant/pubkey returns the key that signs webhooks. Auth is one X-Token header, pageUrl is a hosted page so card data never touches my server, and it takes foreign-issued cards, so Quibble sells internationally. Ukrainian locale is charged in UAH, everyone else in USD.

The client is about seventy lines. No SDK, no dashboard-configured product catalogue, no webhook secret ceremony. What I give up is Billing, Tax, Radar, Connect, subscriptions I would have to build myself, and a much shorter tail of search results when something breaks at 23:00.

What I don’t know

Three things, and I would rather name them than let the post imply otherwise.

Whether the guard was reasoned or pattern-matched. I can’t see inside the model and the commit message doesn’t say. A correct line and a lucky line are the same line in a diff. That is the whole difficulty: the artefact does not carry its own justification, and I was treating “it is there” as if it were “it was decided”.

How many other guards in that commit I still don’t understand. I found this one because something read the file back to me, not because I went looking. There are 279 lines of tests in that commit and I have now read maybe forty of them with real attention. The honest number of things I have verified is smaller than the number of things that are correct.

Whether the out-of-order delivery has actually happened in production. The guard returns handled: stale and I am not counting those separately, so I have no figure to give you. The bug is documented by the provider and the guard is tested, which is not the same as saying I have watched it fire.

If you run a webhook handler that writes state and you have measured how often the stale path is hit, I would like to hear the number. That is the piece of evidence this post is missing.