fix(backend): handle lost insert race and expired-key completion in idempotency layer

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 })]
+      );
+    });
+  });
+});

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)]
   );
 }