From 820fdac20c330d3f52cd130ebfa8db085705c553 Mon Sep 17 00:00:00 2001 From: iamwhitehat Date: Sat, 22 Aug 2026 15:21:25 +0000 Subject: [PATCH] fix(backend): handle lost insert race and expired-key completion in idempotency layer (#500) Two gaps remain in the idempotency implementation after #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. --- .../__tests__/idempotencyIssue500.test.ts | 60 +++++++++++++++++++ backend/src/services/idempotencyService.ts | 46 +++++++++----- 2 files changed, 92 insertions(+), 14 deletions(-) create mode 100644 backend/src/services/__tests__/idempotencyIssue500.test.ts diff --git a/backend/src/services/__tests__/idempotencyIssue500.test.ts b/backend/src/services/__tests__/idempotencyIssue500.test.ts new file mode 100644 index 00000000..9c4fe920 --- /dev/null +++ b/backend/src/services/__tests__/idempotencyIssue500.test.ts @@ -0,0 +1,60 @@ +import { + claimKey, + completeKey, + IdempotencyConflictError, +} from '../idempotencyService.js'; +import { query } from '../../config/database.js'; + +jest.mock('../../config/database.js'); +jest.mock('../../utils/logger.js'); + +/** + * Regression tests for issue #500 follow-ups. + * + * These target two behaviors that the pre-fix code got wrong: + * + * 1. Lost insert race: when two concurrent requests pass the WHERE NOT EXISTS + * check, the database's UNIQUE constraint rejects the second INSERT with + * Postgres error 23505. claimKey must translate that into + * IdempotencyConflictError instead of letting a raw 500 escape. + * 2. Completion of an expired key: completeKey must not write a response into + * a row whose claim has already expired (another request may now own it). + * + * Both are simulated at the query-mock level, matching the existing test + * style in this directory (the unit suite runs without a live Postgres). + */ +describe('issue #500 regressions', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + describe('claimKey lost-race handling', () => { + it('translates unique violation (23505) into IdempotencyConflictError', async () => { + // Simulate: our INSERT lost the race against a concurrent claim. + const pgUniqueViolation = Object.assign(new Error('duplicate key value violates unique constraint "idempotency_keys_organization_id_idempotency_key_key"'), { code: '23505' }); + (query as jest.Mock).mockRejectedValueOnce(pgUniqueViolation); + + await expect(claimKey(1, 'race-key')).rejects.toThrow(IdempotencyConflictError); + }); + + it('does NOT swallow other insert errors', async () => { + const connectionFailure = Object.assign(new Error('ECONNREFUSED'), { code: 'ECONNREFUSED' }); + (query as jest.Mock).mockRejectedValueOnce(connectionFailure); + + await expect(claimKey(1, 'key-err')).rejects.toThrow(/ECONNREFUSED/); + }); + }); + + describe('completeKey expiry guard', () => { + it('only completes keys that have not expired', async () => { + (query as jest.Mock).mockResolvedValueOnce({ rowCount: 0, rows: [] }); + + await completeKey(1, 'expiring-key', 201, { ok: true }); + + expect(query).toHaveBeenCalledWith( + expect.stringContaining('expires_at > NOW()'), + [1, 'expiring-key', 201, JSON.stringify({ ok: true })] + ); + }); + }); +}); diff --git a/backend/src/services/idempotencyService.ts b/backend/src/services/idempotencyService.ts index db65a967..7c24408b 100644 --- a/backend/src/services/idempotencyService.ts +++ b/backend/src/services/idempotencyService.ts @@ -39,18 +39,36 @@ export async function claimKey( try { // Step 1: Try to insert a fresh in_progress row (skip expired rows // via the WHERE clause so they fall through to the conflict path). - const insertResult = await query( - `INSERT INTO idempotency_keys (organization_id, idempotency_key, status, expires_at) - SELECT $1, $2, 'in_progress', $3 - WHERE NOT EXISTS ( - SELECT 1 FROM idempotency_keys - WHERE organization_id = $1 AND idempotency_key = $2 AND expires_at > NOW() - )`, - [organizationId, idempotencyKey, expiresAt] - ); - - if ((insertResult.rowCount ?? 0) > 0) { - return null; + // + // The WHERE NOT EXISTS check is not by itself a claim: two concurrent + // transactions can both see no existing row and both attempt the INSERT. + // The UNIQUE (organization_id, idempotency_key) constraint is what makes + // this atomic: exactly one of them commits, the loser gets error 23505, + // which we translate into IdempotencyConflictError (409 at the middleware). + try { + const insertResult = await query( + `INSERT INTO idempotency_keys (organization_id, idempotency_key, status, expires_at) + SELECT $1, $2, 'in_progress', $3 + WHERE NOT EXISTS ( + SELECT 1 FROM idempotency_keys + WHERE organization_id = $1 AND idempotency_key = $2 AND expires_at > NOW() + )`, + [organizationId, idempotencyKey, expiresAt] + ); + + if ((insertResult.rowCount ?? 0) > 0) { + return null; + } + } catch (err: unknown) { + const code = (err as { code?: string })?.code; + if (code === '23505') { + // Lost the insert race: another request claimed the key between our + // NOT EXISTS evaluation and our INSERT. This is the concurrent-duplicate + // case: do NOT fall through to Steps 2/3 (the key is unexpired and + // in_progress), surface it as a conflict immediately. + throw new IdempotencyConflictError(organizationId, idempotencyKey); + } + throw err; } // Step 2: Key already exists (or was just expired). Try to claim an @@ -141,7 +159,7 @@ export async function completeKey( await query( `UPDATE idempotency_keys SET status = 'completed', response_status = $3, response_body = $4 - WHERE organization_id = $1 AND idempotency_key = $2`, + WHERE organization_id = $1 AND idempotency_key = $2 AND expires_at > NOW()`, [organizationId, idempotencyKey, responseStatus, JSON.stringify(responseBody)] ); } @@ -158,7 +176,7 @@ export async function failKey( await query( `UPDATE idempotency_keys SET status = 'failed', response_status = $3, response_body = $4 - WHERE organization_id = $1 AND idempotency_key = $2`, + WHERE organization_id = $1 AND idempotency_key = $2 AND expires_at > NOW()`, [organizationId, idempotencyKey, responseStatus, JSON.stringify(responseBody)] ); }