fix(credentials): serialize every personal env map writer

Exporting the personal lock while two writers skipped it left the map
unserialized: `upsertPersonalEnvVars` merged against a read taken outside any
lock, and the settings PUT replaced the map wholesale. A wholesale replace
landing between another writer's read and its write-back is discarded whole,
so it takes the lock too.

The delete path now removes the key's mirrors directly instead of reconciling
against a key list. The reconcile prunes every mirror absent from that list,
so a secret added between the read and the prune lost its mirror while its
value survived. `setPersonalSecret` already takes the map lock and then the
user-identity fence inside it, so the targeted delete introduces no new lock
order.

Author: waleedlatif1

apps/sim/app/api/environment/route.ts

@@ -12,6 +12,7 @@ import { getSession } from '@/lib/auth'
 import { decryptSecret, encryptSecret } from '@/lib/core/security/encryption'
 import { generateRequestId } from '@/lib/core/utils/request'
 import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
+import { lockPersonalEnvMap } from '@/lib/credentials/env-locks'
 import { syncPersonalEnvCredentialsForUser } from '@/lib/credentials/environment'
 import type { EnvironmentVariable } from '@/lib/environment/api'
 import { captureServerEvent } from '@/lib/posthog/server'
@@ -53,21 +54,31 @@ export const POST = withRouteHandler(async (req: NextRequest) => {
       })
     ).then((entries) => Object.fromEntries(entries))
 
-    await db
-      .insert(environment)
-      .values({
-        id: generateId(),
-        userId: session.user.id,
-        variables: encryptedVariables,
-        updatedAt: new Date(),
-      })
-      .onConflictDoUpdate({
-        target: [environment.userId],
-        set: {
+    /**
+     * A wholesale replace still takes the map lock: without it this can land
+     * between another writer's read and its write-back, and that writer then
+     * persists a map derived from the pre-replace state, discarding this one
+     * entirely. The reconcile below matches the replace, so it stays outside.
+     */
+    await db.transaction(async (tx) => {
+      await lockPersonalEnvMap(tx, session.user.id)
+
+      await tx
+        .insert(environment)
+        .values({
+          id: generateId(),
+          userId: session.user.id,
           variables: encryptedVariables,
           updatedAt: new Date(),
-        },
-      })
+        })
+        .onConflictDoUpdate({
+          target: [environment.userId],
+          set: {
+            variables: encryptedVariables,
+            updatedAt: new Date(),
+          },
+        })
+    })
 
     await syncPersonalEnvCredentialsForUser({
       userId: session.user.id,

apps/sim/lib/credentials/orchestration/index.test.ts

@@ -21,7 +21,7 @@ const {
   mockDeleteConnectionCredential,
   mockDeleteOrphanedOAuthAccount,
   mockDeleteWorkspaceEnvCredentials,
-  mockSyncPersonalEnvCredentialsForUser,
+  mockDeletePersonalEnvCredentialForUser,
 } = vi.hoisted(() => ({
   mockRecordAudit: vi.fn(),
   mockGetCredentialActorContext: vi.fn(),
@@ -34,7 +34,7 @@ const {
   mockDeleteConnectionCredential: vi.fn(),
   mockDeleteOrphanedOAuthAccount: vi.fn(),
   mockDeleteWorkspaceEnvCredentials: vi.fn(),
-  mockSyncPersonalEnvCredentialsForUser: vi.fn(),
+  mockDeletePersonalEnvCredentialForUser: vi.fn(),
 }))
 
 vi.mock('@sim/audit', () => ({
@@ -62,7 +62,7 @@ vi.mock('@/lib/credentials/deletion', () => ({
 }))
 vi.mock('@/lib/credentials/environment', () => ({
   deleteWorkspaceEnvCredentials: mockDeleteWorkspaceEnvCredentials,
-  syncPersonalEnvCredentialsForUser: mockSyncPersonalEnvCredentialsForUser,
+  deletePersonalEnvCredentialForUser: mockDeletePersonalEnvCredentialForUser,
 }))
 vi.mock('@/lib/credentials/atlassian-service-account', () => ({
   AtlassianValidationError: class AtlassianValidationError extends Error {},
@@ -770,6 +770,15 @@ describe('deleteCredentialRecord', () => {
       return sql.includes('pg_advisory_xact_lock') && params.includes('user-1')
     })
     expect(locked).toBe(true)
+    /**
+     * Targeted, not a reconcile against a key list: a list read before the
+     * prune can miss a secret added since, and prune that secret's mirror
+     * while its value survives.
+     */
+    expect(mockDeletePersonalEnvCredentialForUser).toHaveBeenCalledWith(
+      expect.objectContaining({ userId: 'user-1', envKey: 'MY_KEY' })
+    )
+    expect(mockDeletePersonalEnvCredentialForUser.mock.calls[0][0].executor).toBeDefined()
   })
 
   it('revokes the backing OAuth grant of a deleted oauth credential', async () => {

apps/sim/lib/credentials/orchestration/index.ts

@@ -32,8 +32,8 @@ import {
 import { slackCustomBotDisplayName } from '@/lib/credentials/display-name'
 import { lockPersonalEnvMap, lockWorkspaceEnvMap } from '@/lib/credentials/env-locks'
 import {
+  deletePersonalEnvCredentialForUser,
   deleteWorkspaceEnvCredentials,
-  syncPersonalEnvCredentialsForUser,
 } from '@/lib/credentials/environment'
 import type { ServiceAccountFieldId } from '@/lib/credentials/service-account-fields'
 import {
@@ -569,13 +569,15 @@ export async function deleteCredentialRecord(
     const { envKey, envOwnerUserId } = credentialRow
     /**
      * Same read-modify-write on the personal map, under the same lock its
-     * other writers take. The credential reconcile stays outside: it opens its
-     * own transaction and takes the user-identity fence, so nesting it here
-     * would have two transactions taking two locks in opposite orders. It is a
-     * reconcile against the stored keys, so a failure is repaired by the next
-     * one rather than entrenched.
+     * other writers take, with the mirrors removed in the same transaction.
+     *
+     * Targeted rather than a reconcile: the reconcile prunes every mirror
+     * absent from a caller-supplied key list, so a secret added between the
+     * read and the prune lost its mirror while its value survived. Deleting
+     * this one key's mirrors cannot strand another secret, and the lock order
+     * — map, then user identity — is the one `setPersonalSecret` already takes.
      */
-    const current = await db.transaction(async (tx) => {
+    await db.transaction(async (tx) => {
       await lockPersonalEnvMap(tx, envOwnerUserId)
 
       const [personalRow] = await tx
@@ -597,11 +599,7 @@ export async function deleteCredentialRecord(
           target: [environment.userId],
           set: { variables, updatedAt: new Date() },
         })
-      return variables
-    })
-    await syncPersonalEnvCredentialsForUser({
-      userId: credentialRow.envOwnerUserId,
-      envKeys: Object.keys(current),
+      await deletePersonalEnvCredentialForUser({ userId: envOwnerUserId, envKey, executor: tx })
     })
     return true
   }

apps/sim/lib/environment/utils.ts

@@ -7,7 +7,7 @@ import { generateId } from '@sim/utils/id'
 import { eq, inArray } from 'drizzle-orm'
 import { LRUCache } from 'lru-cache'
 import { decryptSecret, encryptSecret } from '@/lib/core/security/encryption'
-import { lockWorkspaceEnvMap } from '@/lib/credentials/env-locks'
+import { lockPersonalEnvMap, lockWorkspaceEnvMap } from '@/lib/credentials/env-locks'
 import {
   createWorkspaceEnvCredentials,
   getAccessibleEnvCredentials,
@@ -435,20 +435,37 @@ export async function upsertPersonalEnvVars(
     newlyEncrypted[key] = encrypted
   }
 
-  const finalEncrypted = { ...existingEncrypted, ...newlyEncrypted }
+  /**
+   * The read above only decides which values changed; the merge has to be made
+   * against a read taken under the lock, or a key written concurrently is
+   * absent from this map and dropped by the write-back.
+   */
+  const finalEncrypted = await db.transaction(async (tx) => {
+    await lockPersonalEnvMap(tx, userId)
 
-  await db
-    .insert(environment)
-    .values({
-      id: generateId(),
-      userId,
-      variables: finalEncrypted,
-      updatedAt: new Date(),
-    })
-    .onConflictDoUpdate({
-      target: [environment.userId],
-      set: { variables: finalEncrypted, updatedAt: new Date() },
-    })
+    const [currentRow] = await tx
+      .select({ variables: environment.variables })
+      .from(environment)
+      .where(eq(environment.userId, userId))
+      .limit(1)
+    const current = (currentRow?.variables as Record<string, string>) || {}
+    const finalEncrypted = { ...current, ...newlyEncrypted }
+
+    await tx
+      .insert(environment)
+      .values({
+        id: generateId(),
+        userId,
+        variables: finalEncrypted,
+        updatedAt: new Date(),
+      })
+      .onConflictDoUpdate({
+        target: [environment.userId],
+        set: { variables: finalEncrypted, updatedAt: new Date() },
+      })
+
+    return finalEncrypted
+  })
 
   invalidateEffectiveDecryptedEnvCache({ userId })
   await syncPersonalEnvCredentialsForUser({