A stranger fixed my bug. Then I found out he fixed the wrong half — and it still worked.

A developer discovered that a pull request to their Safari MCP server fixed a real bug, but half of the patch was unnecessary. The actual fix addressed a subtle concurrency bug where a write failure could desynchronize the request-response queue, causing cascading hangs. The developer built a model to understand the issue and found that the timeout path was safe, but the write-failure path left a callback in the queue that could consume a future reply.

Someone opened a pull request against my Safari MCP server last week. Three functions, same treatment each, clean write-up, all CI green. It fixed a real bug — his repro was solid, mine reproduced it too. Then I sat down to write it up, built a 40-line model of the thing to make sure I understood it, and discovered that half of his patch does nothing at all. Not "does something subtle." Nothing. The two hunks he described as the fix are behaviorally identical to the code they replace. The half he didn't emphasize is the one that closes the bug. And the reason neither of us could tell — the reason I couldn't tell about my own code — turns out to be the actual story here. My server talks to a small Swift helper for the things JavaScript can't do on a Mac: focus an app, hide a window, synthesize a real OS-level click. Newline-delimited JSON over stdin/stdout. Request and response are correlated positionally — there are no request IDs: js const helperQueue = ; // callbacks waiting for responses helperProc.stdout.on "data", chunk = { buf += chunk.toString ; const lines = buf.split "\n" ; buf = lines.pop ; for const line of lines { if line.trim continue; const cb = helperQueue.shift ; // first reply belongs to first waiter if cb cb line ; } } ; This works exactly as long as one invariant holds: Every callback in the queue corresponds to a request that was actually sent and whose reply is still coming. Violate that once and the queue is off by one. Reply N goes to waiter N+1. Forever — there is nothing in the protocol that can ever resynchronize it. And because every waiter is a setTimeout -guarded promise, the symptom is never an error. It's a hang. A helper call looks like this this is the real pre-fix code, lightly trimmed : js function helperGetFrontApp timeout = 2000 { return withHelperLock = new Promise resolve = { let resolved = false; const timer = setTimeout = { if resolved { resolved = true; resolve null ; } // path 1: gave up waiting }, timeout ; function cb line { if resolved return; resolved = true; clearTimeout timer ; resolve line.trim ; } helperQueue.push cb ; // ← pushed BEFORE the write try { helperProc.stdin.write '{"getFrontApp":true}\n' ; } catch { clearTimeout timer ; resolve null ; } // path 2: write blew up } ; } There are two ways to walk away from a request, and they are not the same, which is the whole point: Path 1 — the timeout. The helper is slow; we stop waiting. But a reply is still coming. The dead cb stays in the queue, gets shifted when the late line lands, sees resolved === true , and returns without touching anything. It ate exactly one line — which is precisely correct, because exactly one line was owed. The queue stays aligned. Path 2 — the write failure. stdin.write throws broken pipe: the helper died between our last call and this one . The request was never sent , so no reply is ever coming. But look at the catch : it calls resolve null and never sets resolved = true. So cb is still sitting in the queue, and it's still The next request comes in. Its reply arrives. shift hands it to the armed corpse of a request that was never sent. resolved is false, so it consumes the line, resolves a promise nobody is listening to, and the caller who actually sent a request gets nothing — it waits out its full timeout, and leaves its own callback behind when it does. That's not an off-by-one. That's a cascade. One failed write poisons the channel for the lifetime of the daemon. I've been wrong about my own concurrency code before, so I stopped arguing with myself and modeled it — a positional queue, the same resolved guard, the two abandonment paths, pre-fix and post-fix: PRE-FIX timeout → B got: reply-to-B ← already correct PRE-FIX writefail → B got: TIMEOUT B ← B's reply eaten by A's armed callback POST-FIX timeout → B got: reply-to-B POST-FIX writefail → B got: reply-to-B ← fixed There it is. The timeout path was never broken. The patch's headline change — replacing the abandoned callback with an explicit no-op consumer: js const idx = helperQueue.indexOf cb ; if idx = 0 helperQueue idx = = {}; // no-op consumer for the late reply ...replaces a callback that was already a no-op consumer because cb self-guards on resolved with a callback that is visibly a no-op consumer. Zero behavior change. It's a comment that happens to compile. The fix is the other hunk, the quiet one in the catch : js catch { const idx = helperQueue.indexOf cb ; if idx = 0 helperQueue.splice idx, 1 ; // never sent → nothing is coming → drop the slot clearTimeout timer ; resolve null ; } And note that the two hunks do opposite things — one keeps the slot, one removes it — for reasons that are entirely non-obvious unless you have the positional invariant in your head. Swap them and you've built a worse bug than the one you set out to fix: splice on timeout means the late reply gets shifted onto the next caller, who now gets someone else's answer and believes it. He got that distinction right in both directions, on his first try, in code he didn't write. That's the part I'm actually impressed by. Here's what I found when I grepped my own file for this pattern. Seven helper functions. Every one of them open-codes the same request/timeout/queue dance. Four carry some version of the no-op-consumer line: js 764: if idx = 0 helperQueue idx = = { helperConsecutiveTimeouts = 0; }; // late reply ⇒ alive 825: if idx = 0 helperQueue idx = = {}; // no-op consumer for a late reply 863: if idx = 0 helperQueue idx = = {}; // No-op consumer for late response 912: if idx = 0 helperQueue idx = = {}; Three don't. Those three are exactly the three he patched. So the honest reading isn't "I forgot the fix in three places." It's worse and more interesting: I wrote a defensive line four times without ever writing down what it defends against , and in the three functions where I skipped it, the code was accidentally fine anyway — while a genuinely broken path sat two lines below, in every single one of the seven. The invariant — an abandoned slot must consume exactly one reply, unless nothing was ever sent — is stated in exactly zero of those seven functions. Not in a comment, not in a name, not in a type. It lives only in whatever I happened to be holding in my head on the afternoon I wrote each one. A contributor reading this file has no way to check his patch against the rule, because the rule isn't there. He had to reconstruct it from the wreckage — and he reconstructed it correctly, which is why his catch hunk lands even though his description of why doesn't. That's the failure. Not the missing line. The missing place to put the line. I don't care much about DRY as an aesthetic. A little duplication beats a bad abstraction, and I'll take three copies of a five-line function over an inheritance hierarchy every day of the week. But that argument is about code you can see . This is about something else: A rule that exists in N copies has to be re-derived from scratchby whoever writes copy N+1 — including you, next year, at 11pm. Seven functions, seven independent recollections of an unwritten rule. I got the cosmetic half right four times out of seven and the load-bearing half wrong seven times out of seven. That is not a discipline problem you fix by being more careful. It's a coin flip you have to win every time, forever, and the odds get worse with every function added. The correct shape was always one function: // The helper protocol is POSITIONAL — replies are matched to requests by queue order, // not by id. So an abandoned request must leave the queue in a consistent state: // • timed out → a reply IS still coming → leave a no-op consumer to eat exactly one line // • write failed → nothing was ever sent → remove the slot entirely // Get this backwards and every subsequent reply goes to the wrong caller, silently, forever. function helperRequest payload, { timeout, onLine, onAbandon } { ... } Seven callers, one invariant, stated once, in the only place where it can be found by the person who needs it. Copy eight — written by someone who never read this article — gets it for free. That refactor is mine to do, and it's the actual fix. His patch fixed the three instances. The reason there were three instances to fix is still sitting in my file. 54 tests on this file. All green. Not one of them ever failed a write. Of course they didn't — you have to go out of your way to break a pipe in a test, and nobody writes that test until the bug that needs it has already shipped. The bug lived in the intersection of two things a test suite naturally avoids: the error path of the transport, and state that outlives a single call . My tests all assert what one request returns. The bug is only visible in what the next one returns. He didn't find it by reading my code looking for bugs. He found it because safari new tab hung on his machine, and he kept going past the symptom until he hit a queue that was one slot off. Do you have a positional protocol in production — FIFO correlation, no request IDs? I want to know whether "an abandoned slot must eat exactly one reply" is folklore that everyone rediscovers the hard way, or whether the real lesson is just don't build a protocol without request IDs . Because I'm now fairly sure the invariant I'm about to carefully centralize is one I shouldn't need to have at all. Source: achiya-automation/safari-mcp — a Safari MCP server that drives your real, logged-in browser. The PR is 53, by @jrepp.