fix(credentials): chunk the credential-ACL write the env save now depends on

`createWorkspaceEnvCredentials` wrote keys x members membership rows in one
statement, and neither side is bounded by the request contract. Past 65535
bind parameters that throws — previously a partial success, because the value
had already committed, but this now runs inside the value's transaction, so it
rolls the save back instead, deterministically, on every retry. A 50-member
workspace saving 150 keys reaches it. Chunked the same way the two personal
paths in this file already are.

Also from the audit:

- invalidate the decrypted-env cache after `deleteCredentialRecord` removes an
  env value, matching the dedicated delete paths; without it a deleted secret
  stayed resolvable for the cache TTL
- correct the comment claiming the personal reconcile "matches the replace" —
  it prunes against this request's key list, so a secret added after the commit
  still loses its mirror. Naming the gap instead of asserting it away
- name the one behavior change the in-transaction re-read introduces: a key
  whose submitted value already matched is not re-encrypted, so a concurrent
  write for that key now survives rather than being overwritten
- drop the lock-timeout constant and TSDoc left behind when the lock moved into
  the shared helper, and stop shadowing `finalEncrypted`

Author: waleedlatif1

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

@@ -58,7 +58,14 @@ export const POST = withRouteHandler(async (req: NextRequest) => {
      * 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.
+     * entirely.
+     *
+     * The reconcile below stays outside because it opens its own transaction.
+     * That leaves a known gap: it prunes mirrors against this request's key
+     * list, so a secret added after the commit loses its mirror while its
+     * value survives. Closing it means having the reconcile read the map
+     * itself rather than trust a caller's list, across all four of its
+     * callers.
      */
     await db.transaction(async (tx) => {
       await lockPersonalEnvMap(tx, session.user.id)

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

@@ -35,11 +35,6 @@ import {
 
 const logger = createLogger('WorkspaceEnvironmentAPI')
 
-/**
- * Bounds the workspace-environment advisory-lock wait so a stuck holder fails
- * fast (SQLSTATE 55P03) rather than hanging, even if the deployment lacks a
- * server-side `lock_timeout`. Transaction-scoped via `set_config(..., true)`.
- */
 /**
  * Restricts decrypted workspace env values to administrators. Members (including
  * read-only) receive the variable names with empty values so editor autocomplete

apps/sim/lib/credentials/environment.test.ts

@@ -15,6 +15,7 @@ vi.mock('@/lib/billing/organizations/billing-identity-lock', () => ({
 }))
 
 import {
+  createWorkspaceEnvCredentials,
   getPersonalEnvKeyRawAccess,
   getWorkspaceEnvKeyAdminAccess,
   syncPersonalEnvCredentialsForUser,
@@ -239,3 +240,41 @@ describe('syncPersonalEnvCredentialsForUser', () => {
     ])
   })
 })
+
+describe('createWorkspaceEnvCredentials', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+    resetDbChainMock()
+  })
+
+  /**
+   * The membership row count is keys × members, and neither is bounded by the
+   * request contract. A single statement past Postgres's 65535 bind parameters
+   * throws — and because this now runs inside the value's transaction, that
+   * would roll back the save on every retry rather than half-committing it.
+   */
+  it('splits a keys x members write too wide for one statement', async () => {
+    const keys = Array.from({ length: 40 }, (_, i) => `KEY_${i}`)
+    queueTableRows(workspace, [{ ownerId: 'owner-1' }])
+    queueTableRows(
+      permissions,
+      Array.from({ length: 60 }, (_, i) => ({ userId: `member-${i}` }))
+    )
+    // Every chunk of the credential insert reports its rows back as created.
+    dbChainMockFns.returning.mockImplementation(() =>
+      Promise.resolve(keys.map((_, i) => ({ id: `credential-${i}` })))
+    )
+
+    await createWorkspaceEnvCredentials({
+      workspaceId: 'ws-1',
+      newKeys: keys,
+      actingUserId: 'member-0',
+    })
+
+    const rowsPerCall = dbChainMockFns.values.mock.calls.map(([rows]) =>
+      Array.isArray(rows) ? rows.length : 1
+    )
+    expect(rowsPerCall.length).toBeGreaterThan(1)
+    expect(Math.max(...rowsPerCall)).toBeLessThanOrEqual(500)
+  })
+})

apps/sim/lib/credentials/environment.ts

@@ -17,7 +17,7 @@ import {
   hasWorkspaceAdminAccess,
 } from '@/lib/workspaces/permissions/utils'
 
-const PERSONAL_ENV_CREDENTIAL_WRITE_CHUNK_SIZE = 500
+const ENV_CREDENTIAL_WRITE_CHUNK_SIZE = 500
 
 export interface WorkspaceMembership {
   ownerId: string | null
@@ -437,27 +437,35 @@ export async function createWorkspaceEnvCredentials(params: {
 
   const now = params.updatedAt ?? new Date()
 
-  const inserted = await executor
-    .insert(credential)
-    .values(
-      keys.map((envKey) => ({
-        id: generateId(),
-        workspaceId,
-        type: 'env_workspace' as const,
-        displayName: envKey,
-        envKey,
-        createdBy: actingUserId,
-        createdAt: now,
-        updatedAt: now,
-      }))
-    )
-    .onConflictDoNothing()
-    .returning({ id: credential.id })
-  const createdIds = inserted.map((row) => row.id)
+  const credentialValues = keys.map((envKey) => ({
+    id: generateId(),
+    workspaceId,
+    type: 'env_workspace' as const,
+    displayName: envKey,
+    envKey,
+    createdBy: actingUserId,
+    createdAt: now,
+    updatedAt: now,
+  }))
+  const createdIds: string[] = []
+  for (const values of chunkArray(credentialValues, ENV_CREDENTIAL_WRITE_CHUNK_SIZE)) {
+    const inserted = await executor
+      .insert(credential)
+      .values(values)
+      .onConflictDoNothing()
+      .returning({ id: credential.id })
+    createdIds.push(...inserted.map((row) => row.id))
+  }
 
   if (createdIds.length === 0 || memberUserIds.length === 0) return
 
-  // Bulk-insert memberships for all new credentials × all workspace members in one query
+  /**
+   * Chunked because the row count is keys × members and neither side is
+   * bounded: a wide enough save exceeds Postgres's 65535 bind parameters and
+   * throws. Unchunked that was a partial success — the value was already
+   * committed — but this now runs inside the value's transaction, so it would
+   * roll the save back, deterministically, on every retry.
+   */
   const membershipValues = createdIds.flatMap((credentialId) =>
     memberUserIds.map((memberUserId) => ({
       id: generateId(),
@@ -472,7 +480,9 @@ export async function createWorkspaceEnvCredentials(params: {
     }))
   )
 
-  await executor.insert(credentialMember).values(membershipValues).onConflictDoNothing()
+  for (const values of chunkArray(membershipValues, ENV_CREDENTIAL_WRITE_CHUNK_SIZE)) {
+    await executor.insert(credentialMember).values(values).onConflictDoNothing()
+  }
 }
 
 /**
@@ -529,7 +539,7 @@ export async function upsertPersonalEnvCredentialForUser(params: {
       createdAt: updatedAt,
       updatedAt,
     }))
-    for (const values of chunkArray(credentialValues, PERSONAL_ENV_CREDENTIAL_WRITE_CHUNK_SIZE)) {
+    for (const values of chunkArray(credentialValues, ENV_CREDENTIAL_WRITE_CHUNK_SIZE)) {
       await tx.insert(credential).values(values).onConflictDoNothing()
     }
 
@@ -570,7 +580,7 @@ export async function upsertPersonalEnvCredentialForUser(params: {
       createdAt: updatedAt,
       updatedAt,
     }))
-    for (const values of chunkArray(membershipValues, PERSONAL_ENV_CREDENTIAL_WRITE_CHUNK_SIZE)) {
+    for (const values of chunkArray(membershipValues, ENV_CREDENTIAL_WRITE_CHUNK_SIZE)) {
       await tx
         .insert(credentialMember)
         .values(values)
@@ -696,7 +706,7 @@ export async function syncPersonalEnvCredentialsForUser(params: {
           updatedAt: now,
         }))
       )
-      for (const values of chunkArray(credentialValues, PERSONAL_ENV_CREDENTIAL_WRITE_CHUNK_SIZE)) {
+      for (const values of chunkArray(credentialValues, ENV_CREDENTIAL_WRITE_CHUNK_SIZE)) {
         await tx.insert(credential).values(values).onConflictDoNothing()
       }
 
@@ -724,10 +734,7 @@ export async function syncPersonalEnvCredentialsForUser(params: {
           createdAt: now,
           updatedAt: now,
         }))
-        for (const values of chunkArray(
-          membershipValues,
-          PERSONAL_ENV_CREDENTIAL_WRITE_CHUNK_SIZE
-        )) {
+        for (const values of chunkArray(membershipValues, ENV_CREDENTIAL_WRITE_CHUNK_SIZE)) {
           await tx
             .insert(credentialMember)
             .values(values)

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

@@ -601,6 +601,9 @@ export async function deleteCredentialRecord(
         })
       await deletePersonalEnvCredentialForUser({ userId: envOwnerUserId, envKey, executor: tx })
     })
+    // The value is gone; without this it stays resolvable from the cache for
+    // its TTL, as the dedicated delete paths already recognise.
+    invalidateEffectiveDecryptedEnvCache({ userId: envOwnerUserId })
     return true
   }
 
@@ -649,6 +652,7 @@ export async function deleteCredentialRecord(
         executor: tx,
       })
     })
+    invalidateEffectiveDecryptedEnvCache({ workspaceId })
     return true
   }
 

apps/sim/lib/environment/utils.ts

@@ -21,7 +21,6 @@ import {
 } from '@/lib/workspaces/permissions/utils'
 
 const logger = createLogger('EnvironmentUtils')
-const WORKSPACE_ENV_LOCK_TIMEOUT_MS = 5_000
 const EFFECTIVE_ENVIRONMENT_CACHE_TTL_MS = 2_000
 const EFFECTIVE_ENVIRONMENT_CACHE_MAX_ENTRIES = 1_000
 
@@ -439,6 +438,12 @@ export async function upsertPersonalEnvVars(
    * 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.
+   *
+   * One consequence worth naming: a key whose submitted value already matched
+   * the earlier read is not re-encrypted, so a value written concurrently for
+   * that key now survives instead of being overwritten with the identical
+   * plaintext. `added`/`updated` describe the earlier read and are reporting
+   * only — the keys actually written are exactly the re-encrypted ones.
    */
   const finalEncrypted = await db.transaction(async (tx) => {
     await lockPersonalEnvMap(tx, userId)
@@ -449,22 +454,22 @@ export async function upsertPersonalEnvVars(
       .where(eq(environment.userId, userId))
       .limit(1)
     const current = (currentRow?.variables as Record<string, string>) || {}
-    const finalEncrypted = { ...current, ...newlyEncrypted }
+    const merged = { ...current, ...newlyEncrypted }
 
     await tx
       .insert(environment)
       .values({
         id: generateId(),
         userId,
-        variables: finalEncrypted,
+        variables: merged,
         updatedAt: new Date(),
       })
       .onConflictDoUpdate({
         target: [environment.userId],
-        set: { variables: finalEncrypted, updatedAt: new Date() },
+        set: { variables: merged, updatedAt: new Date() },
       })
 
-    return finalEncrypted
+    return merged
   })
 
   invalidateEffectiveDecryptedEnvCache({ userId })