fix(backend): handle lost insert race and expired-key completion in idempotency layer - #599
Open
iamwhitehat wants to merge 1 commit into
Open
Conversation
…dempotency layer (Protocol-Guild#500) Two gaps remain in the idempotency implementation after Protocol-Guild#584: 1. claimKey's INSERT ... WHERE NOT EXISTS is not atomic by itself. Two concurrent requests can both pass the NOT EXISTS check; the UNIQUE constraint then rejects one with Postgres 23505, which was unhandled and surfaced as a raw 500. Now translated into IdempotencyConflictError (409), matching the concurrent-duplicate contract. 2. completeKey/failKey had no expiry guard. A request finishing after its key expired could write its response into a row another request already re-claimed, cross-contaminating cached responses. Both updates now require expires_at > NOW(). Tests: new regressions in idempotencyIssue500.test.ts fail on the unpatched code (2/3 pass) and pass with the fix (3/3). Existing idempotency suites unchanged: 35/35.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-ups to #500. The main check-then-claim race was already fixed well in #584; while reviewing that work against the issue's acceptance criteria I found two gaps that remain on main.
1. Lost insert race surfaces as a raw 500
claimKeyclaims a key withINSERT ... WHERE NOT EXISTS. That WHERE clause is evaluated per-transaction, so two concurrent requests can both see no existing row and both attempt the INSERT. The UNIQUE constraint correctly rejects one of them, but nothing catches Postgres error 23505, so the loser gets an unhandled 500 instead of the documented 409 conflict.Fix: catch 23505 in Step 1 and throw IdempotencyConflictError (which the middleware already maps to 409). Other errors still propagate unchanged.
2. completeKey/failKey could write into an expired claim
Neither update checked expires_at. A request that finishes after its own key expired would overwrite a row that another request has already re-claimed via the expired-row path in Step 2, cross-contaminating the cached response. Both updates now require
expires_at > NOW().Testing
New regressions in
idempotencyIssue500.test.ts, following the existing query-mock style:Against unpatched code: 1/3 pass. With this fix: 3/3 pass.
Existing suites untouched and passing: idempotencyService.test.ts + idempotencyMiddleware.test.ts, 35/35 combined.
Full backend suite: this repo has pre-existing failures on clean main (20 suites / 122 tests fail there too, unrelated areas like csvPayrollImport, stellarService, scheduleService). Baseline verified by stashing this change and re-running: identical failures without the patch, and no failing suite overlaps the idempotency code.
Not addressed here
The issue's 100-concurrent-request load test needs a live Postgres in CI; worth doing but out of scope for this change. cleanupExpired is already wired hourly in index.ts.