improvement(permissions): retry lock timeouts and audit admin upserts truthfully

Bounding the row-lock wait with `lock_timeout` made a competing writer raise
55P03, which the retry set excluded — so the bounded wait failed the request
where the unbounded one would have waited. 55P03 is now retried like the other
contention aborts, the timeout is shortened since each attempt releases its
pooled connection, and contention that outlives every attempt answers with a
busy conflict instead of a generic server error.

The admin member upsert recorded MEMBER_ADDED even when the conflict branch
amended an existing membership, putting a join that never happened in the
workspace audit trail. It now records a role change, matching the action the
response already reported.

Adds the missing test coverage for withTransactionRetry.

Co-Authored-By: Claude <noreply@anthropic.com>

Author: icecrasher321

apps/sim/app/api/v1/admin/workspaces/[id]/members/route.ts

@@ -290,18 +290,27 @@ export const POST = withRouteHandler(
       /** A returned id we did not mint means the conflict branch ran. */
       const wasCreated = written?.id === permissionId
 
-      logger.info(`Admin API: Added user ${userId} to workspace ${workspaceId}`, {
-        permissions: permissionLevel,
-        permissionId,
-      })
+      logger.info(
+        wasCreated
+          ? `Admin API: Added user ${userId} to workspace ${workspaceId}`
+          : `Admin API: Updated user ${userId} permissions in workspace ${workspaceId}`,
+        { permissions: permissionLevel, permissionId }
+      )
 
+      /**
+       * The conflict branch amended a membership that already existed, so it is
+       * a role change rather than an addition — recording it as `MEMBER_ADDED`
+       * would put a join that never happened in the workspace's audit trail.
+       */
       recordAudit({
         workspaceId,
         actorId: 'admin-api',
-        action: AuditAction.MEMBER_ADDED,
+        action: wasCreated ? AuditAction.MEMBER_ADDED : AuditAction.MEMBER_ROLE_CHANGED,
         resourceType: AuditResourceType.WORKSPACE,
         resourceId: workspaceId,
-        description: `Admin API added member to workspace with ${permissionLevel} permissions`,
+        description: wasCreated
+          ? `Admin API added member to workspace with ${permissionLevel} permissions`
+          : `Admin API changed workspace member permissions to ${permissionLevel}`,
         metadata: { targetUserId: userId, permissions: permissionLevel },
         request,
       })

apps/sim/app/api/workspaces/[id]/permissions/route.test.ts

@@ -314,6 +314,29 @@ describe('workspace permissions route', () => {
       expect(dbChainMockFns.set).not.toHaveBeenCalled()
     })
 
+    /**
+     * The `lock_timeout` this route sets makes Postgres abort the transaction
+     * under contention. Retries cover the transient case; when they run out the
+     * caller must still get something actionable, not the driver error rendered
+     * as "Internal server error".
+     */
+    it('answers contention that outlives the retries with a busy conflict', async () => {
+      queuePersonalWorkspace([permissionRow(ADMIN_ID, 'admin'), permissionRow(MEMBER_ID, 'read')])
+      dbChainMockFns.transaction.mockRejectedValue(
+        Object.assign(new Error('canceling statement due to lock timeout'), { code: '55P03' })
+      )
+
+      const response = await PATCH(
+        createMockRequest('PATCH', { updates: [{ userId: MEMBER_ID, permissions: 'write' }] }),
+        routeContext
+      )
+
+      expect(response.status).toBe(409)
+      await expect(response.json()).resolves.toMatchObject({
+        error: 'This workspace is busy right now. Try again in a moment.',
+      })
+    })
+
     it('aborts when the workspace leaves its organization mid-request', async () => {
       queuePersonalWorkspace([permissionRow(ADMIN_ID, 'admin'), permissionRow(MEMBER_ID, 'read')])
       permissionsMockFns.mockGetWorkspaceWithOwner.mockResolvedValue({

apps/sim/app/api/workspaces/[id]/permissions/route.ts

@@ -3,6 +3,7 @@ import { db } from '@sim/db'
 import { member, permissions, user, workspace, workspaceEnvironment } from '@sim/db/schema'
 import { createLogger } from '@sim/logger'
 import { isOrgAdminRole, ORG_ADMIN_ROLES } from '@sim/platform-authz/workspace'
+import { getPostgresErrorCode } from '@sim/utils/errors'
 import { and, eq, inArray, sql } from 'drizzle-orm'
 import { type NextRequest, NextResponse } from 'next/server'
 import {
@@ -14,7 +15,7 @@ import { getSession } from '@/lib/auth'
 import { HttpError } from '@/lib/core/utils/http-error'
 import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
 import { syncWorkspaceEnvCredentials } from '@/lib/credentials/environment'
-import { withTransactionRetry } from '@/lib/db/transaction'
+import { isRetryableTransactionError, withTransactionRetry } from '@/lib/db/transaction'
 import type { DbOrTx } from '@/lib/db/types'
 import { captureServerEvent } from '@/lib/posthog/server'
 import {
@@ -75,11 +76,31 @@ class WorkspaceContextChangedError extends HttpError {
   }
 }
 
+/**
+ * Another writer held the rows for longer than every attempt allowed. Distinct
+ * from the conflicts above: nothing about the request is wrong and the state is
+ * unchanged, so the answer is to retry rather than to reload — and it must not
+ * reach the client as a generic server error, which is what bounding the lock
+ * wait would otherwise have produced.
+ */
+class WorkspaceBusyError extends HttpError {
+  readonly statusCode = 409
+
+  constructor() {
+    super('This workspace is busy right now. Try again in a moment.')
+    this.name = 'WorkspaceBusyError'
+  }
+}
+
 /**
  * Bounds the wait on the row locks below so a stuck holder fails fast
  * (SQLSTATE 55P03) instead of parking a pooled connection indefinitely.
+ *
+ * Kept short because `withTransactionRetry` retries that timeout: the connection
+ * is released between attempts, so three bounded waits contend better than one
+ * long one and never pin a pool slot for more than this.
  */
-const PERMISSIONS_LOCK_TIMEOUT_MS = 5000
+const PERMISSIONS_LOCK_TIMEOUT_MS = 3000
 
 /** Organization owners/admins among `userIds`, empty for a personal workspace. */
 async function loadOrgAdminTargets(
@@ -315,10 +336,11 @@ export const PATCH = withRouteHandler(
     }
 
     /**
-     * Retried on a deadlock: the ordered locks below close this route against
+     * Retried on contention: the ordered locks below close this route against
      * itself, but member removal takes the billed account before the departing
-     * user, so a cross-path cycle remains. The victim of one is asked by the
-     * database to try again, and answering it with a 500 would surface as
+     * user, so a cross-path cycle remains — and the `lock_timeout` above turns a
+     * slow competing writer into an abort of its own. Both are the database
+     * asking for a retry, and answering either with a 500 would surface as
      * "Internal server error" for a click that would have worked.
      */
     const { previousRoles, changedUserIds } = await withTransactionRetry(async (tx) => {
@@ -491,6 +513,20 @@ export const PATCH = withRouteHandler(
         previousRoles: lockedByUserId,
         changedUserIds: new Set(changedUpdates.map((update) => update.userId)),
       }
+    }).catch((error) => {
+      /**
+       * Contention that outlived every attempt. Answering with the driver error
+       * would render as "Internal server error" for a request that is simply
+       * queued behind another writer.
+       */
+      if (isRetryableTransactionError(error)) {
+        logger.warn('Permission update exhausted retries under contention', {
+          workspaceId,
+          code: getPostgresErrorCode(error),
+        })
+        throw new WorkspaceBusyError()
+      }
+      throw error
     })
 
     /**

apps/sim/lib/db/transaction.test.ts

@@ -0,0 +1,71 @@
+/**
+ * @vitest-environment node
+ */
+import { dbChainMockFns, resetDbChainMock } from '@sim/testing'
+import { beforeEach, describe, expect, it, vi } from 'vitest'
+import { isRetryableTransactionError, withTransactionRetry } from '@/lib/db/transaction'
+
+/** Shaped like the `postgres` driver's error, which carries `code` on the error. */
+function pgError(code: string): Error {
+  return Object.assign(new Error(`pg error ${code}`), { code })
+}
+
+describe('withTransactionRetry', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+    resetDbChainMock()
+  })
+
+  describe('isRetryableTransactionError', () => {
+    /**
+     * `55P03` is what a caller's own `lock_timeout` raises. Excluding it would
+     * make bounding the lock wait worse than not bounding it.
+     */
+    it.each(['40001', '40P01', '55P03'])('treats %s as retryable', (code) => {
+      expect(isRetryableTransactionError(pgError(code))).toBe(true)
+    })
+
+    it.each(['23505', '23503', '42P01'])('treats %s as terminal', (code) => {
+      expect(isRetryableTransactionError(pgError(code))).toBe(false)
+    })
+
+    it('treats a non-postgres error as terminal', () => {
+      expect(isRetryableTransactionError(new Error('boom'))).toBe(false)
+    })
+  })
+
+  it('returns the callback result without retrying when it succeeds', async () => {
+    const fn = vi.fn().mockResolvedValue('ok')
+
+    await expect(withTransactionRetry(fn)).resolves.toBe('ok')
+    expect(dbChainMockFns.transaction).toHaveBeenCalledTimes(1)
+  })
+
+  it.each(['40001', '40P01', '55P03'])(
+    'retries a %s abort and returns the later result',
+    async (code) => {
+      const fn = vi.fn().mockRejectedValueOnce(pgError(code)).mockResolvedValue('recovered')
+
+      await expect(withTransactionRetry(fn)).resolves.toBe('recovered')
+      expect(fn).toHaveBeenCalledTimes(2)
+    }
+  )
+
+  it('gives up after the attempt cap and rethrows the driver error', async () => {
+    const fn = vi.fn().mockRejectedValue(pgError('40P01'))
+
+    await expect(withTransactionRetry(fn, { attempts: 3 })).rejects.toMatchObject({ code: '40P01' })
+    expect(fn).toHaveBeenCalledTimes(3)
+  })
+
+  /**
+   * The typed domain errors callers throw to force a rollback must not be
+   * replayed — a second attempt would reach the same decision.
+   */
+  it('does not retry an error the database did not raise', async () => {
+    const fn = vi.fn().mockRejectedValue(new Error('membership changed'))
+
+    await expect(withTransactionRetry(fn)).rejects.toThrow('membership changed')
+    expect(fn).toHaveBeenCalledTimes(1)
+  })
+})

apps/sim/lib/db/transaction.ts

@@ -8,11 +8,16 @@ import type { DbOrTx } from '@/lib/db/types'
 const logger = createLogger('DbTransaction')
 
 /**
- * `serialization_failure` and `deadlock_detected`. Postgres raises both only
- * after aborting and fully rolling back the transaction, which is what makes a
- * fresh attempt safe rather than a duplicate.
+ * `serialization_failure`, `deadlock_detected`, and `lock_not_available`.
+ * Postgres raises all three only after aborting the transaction, so the work is
+ * fully rolled back and a fresh attempt is a retry rather than a duplicate.
+ *
+ * `55P03` is what a caller's own `lock_timeout` produces: it means a competing
+ * transaction still held the row, which is the case most likely to succeed on a
+ * second attempt. Excluding it would make bounding the wait strictly worse than
+ * not bounding it — the request would fail instead of waiting.
  */
-const RETRYABLE_TRANSACTION_CODES = new Set(['40001', '40P01'])
+const RETRYABLE_TRANSACTION_CODES = new Set(['40001', '40P01', '55P03'])
 
 const DEFAULT_ATTEMPTS = 3
 
@@ -22,15 +27,23 @@ export function isRetryableTransactionError(error: unknown): boolean {
 }
 
 /**
- * Runs `fn` in a transaction, retrying when Postgres aborts it as a deadlock
- * victim or on a serialization failure.
+ * Runs `fn` in a transaction, retrying when Postgres aborts it for contention —
+ * a deadlock, a serialization failure, or a lock timeout.
  *
- * Both are the database asking the loser to try again, and neither leaves
- * partial work behind — so without a retry they surface as a generic 500 for a
+ * All three are the database asking the loser to try again, and none leaves
+ * partial work behind, so without a retry they surface as a generic 500 for a
  * condition that would have succeeded on a second attempt. Any other error,
  * including the typed domain errors a caller throws to force a rollback,
  * propagates on the first occurrence.
  *
+ * Each attempt is its own transaction, so the pooled connection is released
+ * between them — pairing a short `lock_timeout` with a retry holds a connection
+ * for far less time than a single unbounded wait would.
+ *
+ * When the attempts are exhausted the original Postgres error propagates;
+ * callers that answer HTTP should map it with {@link isRetryableTransactionError}
+ * rather than let it become a generic 500.
+ *
  * `fn` must be free of side effects outside the transaction: it can run more
  * than once, and only the committing attempt is durable.
  */