My idempotency library had one job. A dropped connection made it run the payment twice.
DEV Community

My idempotency library had one job. A dropped connection made it run the payment twice.

quayside is my idempotency library for Node.js. The whole API fits in one sentence: execute(key, fn) runs a function exactly once per key - if it already ran, you get the stored result back; if it is running right now, you don't run it again. Pluggable storage (memory, Redis, Postgres, MySQL), HTTP adapters for Express, Fastify, Hono and NestJS, fencing tokens enforced inside the store so a holder that lost its lock can never overwrite a newer execution. Zero runtime dependencies. It shipped at 1.0.0 last week. This post is about the bug that outlived everything I threw at the code before release - a 100% mutation score across 1,254 mutants, a storage contract suite running against real servers, an adversarial review whose 26 verified findings I fixed or triaged - and waited for the final security pass to be found. It lived in four lines I had written days earlier, to fix a different bug. And it broke the library on the one scenario idempotency libraries exist for. the one job Here is the canonical story every idempotency library tells in its README. A client POSTs a payment. The connection drops before the response arrives. The client has no idea whether the charge happened, so it retries with the same Idempotency-Key . Without protection, the customer gets charged twice. With protection, the retry either waits, gets told the first attempt is still running, or replays the stored response. quayside's mechanism for this is deliberately boring: the atomic create-if-absent write is the lock. execute(key, fn) writes an IN_PROGRESS record before your function runs - there is no separate locking step, so there is no gap between "checked" and "locked". Success transitions the record to COMPLETED and replays it for a result TTL. Failure deletes it so retries run fresh. A crashed process is handled by the lock TTL. And every transition out of IN_PROGRESS is guarded by a fencing token validated inside the storage - Lua on Redis, token-conditional UPDATEs on SQL - so a holder that stalled through a GC pause and lost its lease gets a FencingError from the store itself instead of silently overwriting the new holder's result. import { Idempotency } from 'quayside' import { RedisStorage } from 'quayside/redis' const idempotency = new Idempotency({ storage: new RedisStorage(redis), resultTtl: '24h', // how long a completed result stays replayable lockTtl: '30s' // how long a crashed execution blocks the key }) const result = await idempotency.execute('invoice:123', async () => { return createPayment() // runs once; later calls replay the result }) Dropped connection, then a retry. That is the use case. Hold that thought. by the numbers, I had permission to feel safe I have written before about what mutation testing did to a test suite with 100% coverage, so quayside got the same treatment from the start: every mutant dead, none suppressed - when a mutant was equivalent, the rule was to delete or restructure the code until it wasn't, never to annotate it away. The storage adapters all pass one shared contract suite against real servers via Testcontainers: 50-way concurrency races, SIGKILL crash recovery, split-brain fencing where a stale holder tries to overwrite a newer execution and must be rejected by the store itself. On top of that, an adversarial review pass: 38 candidate findings, 26 surviving verification, the ten worst fixed before the tag. I am listing all of this not to brag but to set up the fall. The numbers measure the code you wrote. They say nothing about the code you wrote last Tuesday to fix a review finding, which is where this story actually starts. the review that found a stuck lock One of those 26 findings was in the Fastify adapter. Fastify's pipeline is hooks, not a wrapping function, so the adapter bridges the engine's continuation across two hooks: preHandler acquires the lock and stashes a deferred; onSend resolves it with the captured response, which the engine then stores. The finding: onSend is not guaranteed to run. reply.hijack() - the documented escape hatch for SSE, streaming, proxying - skips it. A handler that never answers skips it. In those cases the deferred never settles, and the key stays locked until the lock TTL expires. Thirty seconds of 409s for every retry, with nothing actually running. A real bug, confirmed, worth fixing. The fix looked obvious. Node's raw ServerResponse emits close for every request, no matter what the framework's lifecycle does. So: settle the capture when the connection closes. // onSend never runs for a hijacked reply, an aborted connection or a // handler that never answers. Without this backstop the capture would // never settle and the key would stay locked until its TTL expired. reply.raw.on('close', () => { resolveCapture(null) }) Tests green. Mutation score still perfect. I even wrote a test asserting the new behavior - "a reply that never reaches onSend releases the key on close" - and it passed, proudly. That test was asserting the bug. the fix was the bug The security pass flagged those four lines as its single HIGH finding, and the exploit chain reads like a checklist of things I knew individually and failed to compose: - A client POSTs /payments withIdempotency-Key: K . The handler starts the charge. - Fifty milliseconds in, the client aborts the connection. Maybe a flaky mobile network. Maybe on purpose. - Node does not cancel the handler - the charge keeps running in the background. - But close fires on the raw response immediately. My backstop resolves the capture withnull . - To the engine, null means "the response was served but is not cacheable" - the code path built for oversized and binary bodies. It releases the record. Nothing is stored under K. The key is free. - The handler finishes the charge. onSend finally runs, tries to resolve the already-settled deferred - a no-op. No error, no warning. Perfect silence. - The client retries with the same key K. The slot is empty. The handler runs again. One idempotency key. Two charges. Zero log lines. And notice what the trigger is: a dropped connection followed by a retry. Not an exotic corner. Not an adversarial contortion. The exact scenario from the README, the one the library has as its single job. The deadlock I fixed was real but bounded - thirty seconds of 409s, annoying, visible. The fix I replaced it with was unbounded and invisible: it spent the library's entire reason to exist, silently, on its most common path. To be precise about what was at stake: this was caught before it ever reached a registry, in the last review gate before release. Nobody was double-charged. But it survived a mutation score of 100% and a full adversarial code review, because both of those examine the code that exists - and this was a composition failure between a lifecycle event and the semantics of null , each of which was individually correct. a close is a fact about the connection, not about the work The actual fix is one condition: reply.raw.on('close', () => { if (reply.raw.writableEnded) resolveCapture(null) }) writableEnded distinguishes the two worlds that close had been conflating. A hijacked reply finished its response before the connection closed - settle it, release the key, that is the stuck-lock fix still working. A connection that died before the response finished tells you nothing about the handler, because the handler is still running. Those keep the lock. What happens next is the part I find genuinely satisfying. The still-running handler eventually completes, and onSend - which does run in this case, late - stores the outcome. So the retry that arrives during the charge gets a 409 with Retry-After , and the retry that arrives after it gets the stored response replayed. The late completion is not lost work. It is exactly the result the retry came back for. If the process dies instead, the lock TTL reclaims the key - the same path that has always governed a crashed execution, tested by the same SIGKILL test. The abort case did not need new machinery. It needed to stop being special-cased into the wrong bucket. the asymmetry that decides it The lesson I wrote into the design doc afterwards, in bold, because I intend to re-read it: Releasing a lock early is an integrity failure. Holding one too long is an availability cost. When the two trade off, hold. The costs are not symmetric, and the pressure on you is inverted from the risk. A held lock is loud - users see 409s, dashboards light up, someone files an issue, and the damage is bounded by a TTL you chose. An early release is silent - everything returns 200, the work just quietly happens twice, and the bound is whatever a duplicated side effect costs your business. When I wrote the close backstop I was optimizing away the loud, bounded problem, and I paid for it with the silent, unbounded one. The same asymmetry shows up anywhere a lease, a lock or a saga step can be torn down by an event that merely correlates with the work being done: process managers reaping "idle" workers mid-transaction, orchestrators treating a lost heartbeat as a finished task, HTTP servers treating a gone client as a gone request. A connection event is a fact about the connection. The work has its own lifecycle, and only the work gets to say when it is over. what else the adversarial passes caught Three more, briefly, because they rhyme with the theme - every one of them was invisible to coverage and visible to an adversary: - The wait loop trusted the key's identity across time. With onConflict: 'wait' , a waiter polls until the holder finishes. But the holder's lock can expire mid-wait and a different payload can take the key over. The waiter was replaying whatever outcome showed up, including one that belonged to someone else's request body. It now re-checks the payload fingerprint on every poll and rejects with the same key-reuse error it would have thrown at the front door. - The HTTP kernel stored control flow as state. "This response must not be cached" used to

Read on DEV Community ↗ ← Back to News

Comments

No comments yet. Start the discussion.