fix(credentials): write an env value and its credential row together (#7160)

* fix(credentials): write an env value and its credential row together

Every writer of a workspace or personal environment map read-modify-writes a
single jsonb column, so they take an advisory lock on the map to serialize.
`deleteCredentialRecord` took none, and did the read, the edit and the
write-back outside a transaction: a secret written concurrently was read
before that write and silently dropped by the write-back.

The credential row was also written after its map transaction committed, in
four places. The delete direction left a row describing a value that was gone;
the create direction was worse than a stale row, because it cannot be repaired
by retrying — the key is in the map by then, so the next attempt reads it as
pre-existing, computes an empty `newKeys`, and never creates the row. Both
helpers already accept `executor`, and `setWorkspaceSecret` has been passing
the transaction since the parameter landed; these four were never migrated.

The personal reconcile stays outside its transaction: it opens its own and
takes the user-identity fence, so nesting it would have two transactions
taking two locks in opposite orders. It reconciles against the stored keys,
so a failure there is repaired by the next one rather than entrenched.

Also folds the four copies of the lock into one helper, since this would have
been the fifth.

* fix(workflows): say when a block is dropped before persistence

`workflow_blocks.name` is NOT NULL, so a block missing `type` or `name` has
to be dropped — but it was dropped silently. A block with no edges left no
trace anywhere: not in the returned warnings, not in a log line. The client
sanitizer warns on the identical condition; this is its server counterpart,
and the warnings array it feeds is already returned by the internal PUT, the
v2 write and the importer.

* improvement(chat): stop loading a transcript the v2 route never reads

Nothing caps a chat transcript — no per-chat message limit on write, no
pruning — and the v2 route keys continuity by `chatId`, so it read the whole
thing on every resumed turn and dropped it. Opt out there. The load stays the
default because the copilot send path does consume it.

* 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.

* 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

@@ -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,38 @@ 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 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)
+
+      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/app/api/v2/chat/route.ts

@@ -177,6 +177,7 @@ export const POST = withRouteHandler(
       // Chat block do, both of which post a single message with a chat id.
       const resolvedChat = await resolveOrCreateChat({
         ...(conversationId ? { chatId: conversationId } : {}),
+        includeTranscript: false,
         userId,
         workspaceId,
         model: MOTHERSHIP_CHAT_DEFAULT_MODEL,

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

@@ -4,7 +4,7 @@ import { workspaceEnvironment } from '@sim/db/schema'
 import { createLogger } from '@sim/logger'
 import { getErrorMessage } from '@sim/utils/errors'
 import { generateId } from '@sim/utils/id'
-import { eq, sql } from 'drizzle-orm'
+import { eq } from 'drizzle-orm'
 import { type NextRequest, NextResponse } from 'next/server'
 import {
   removeWorkspaceEnvironmentContract,
@@ -15,6 +15,7 @@ import { getSession } from '@/lib/auth'
 import { encryptSecret } from '@/lib/core/security/encryption'
 import { generateRequestId } from '@/lib/core/utils/request'
 import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
+import { lockWorkspaceEnvMap } from '@/lib/credentials/env-locks'
 import {
   createWorkspaceEnvCredentials,
   deleteWorkspaceEnvCredentials,
@@ -34,13 +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)`.
- */
-const WORKSPACE_ENV_LOCK_TIMEOUT_MS = 5_000
-
 /**
  * Restricts decrypted workspace env values to administrators. Members (including
  * read-only) receive the variable names with empty values so editor autocomplete
@@ -237,11 +231,8 @@ export const PUT = withRouteHandler(
         })
       ).then((entries) => Object.fromEntries(entries))
 
-      const { existingEncrypted, merged } = await db.transaction(async (tx) => {
-        await tx.execute(
-          sql`SELECT set_config('lock_timeout', ${`${WORKSPACE_ENV_LOCK_TIMEOUT_MS}ms`}, true)`
-        )
-        await tx.execute(sql`SELECT pg_advisory_xact_lock(hashtextextended(${workspaceId}, 0))`)
+      const { merged } = await db.transaction(async (tx) => {
+        await lockWorkspaceEnvMap(tx, workspaceId)
 
         const [existingRow] = await tx
           .select()
@@ -269,12 +260,24 @@ export const PUT = withRouteHandler(
             set: { variables: mergedVars, updatedAt: new Date() },
           })
 
-        return { existingEncrypted: existing, merged: mergedVars }
+        /**
+         * Inside the transaction because a value committed without its
+         * credential row cannot be repaired by retrying: the key is in the map
+         * by then, so the next attempt reads it as pre-existing, computes an
+         * empty `newKeys`, and never creates the row.
+         */
+        const newKeys = Object.keys(variables).filter((k) => !(k in existing))
+        await createWorkspaceEnvCredentials({
+          workspaceId,
+          newKeys,
+          actingUserId: userId,
+          executor: tx,
+        })
+
+        return { merged: mergedVars }
       })
 
       invalidateEffectiveDecryptedEnvCache({ workspaceId })
-      const newKeys = Object.keys(variables).filter((k) => !(k in existingEncrypted))
-      await createWorkspaceEnvCredentials({ workspaceId, newKeys, actingUserId: userId })
 
       recordAudit({
         workspaceId,
@@ -370,10 +373,7 @@ export const DELETE = withRouteHandler(
       }
 
       const result = await db.transaction(async (tx) => {
-        await tx.execute(
-          sql`SELECT set_config('lock_timeout', ${`${WORKSPACE_ENV_LOCK_TIMEOUT_MS}ms`}, true)`
-        )
-        await tx.execute(sql`SELECT pg_advisory_xact_lock(hashtextextended(${workspaceId}, 0))`)
+        await lockWorkspaceEnvMap(tx, workspaceId)
 
         const [existingRow] = await tx
           .select()
@@ -400,6 +400,12 @@ export const DELETE = withRouteHandler(
           .set({ variables: current, updatedAt: new Date() })
           .where(eq(workspaceEnvironment.workspaceId, workspaceId))
 
+        await deleteWorkspaceEnvCredentials({
+          workspaceId,
+          removedKeys: keys,
+          executor: tx,
+        })
+
         return { remainingKeysCount: Object.keys(current).length }
       })
 
@@ -408,7 +414,6 @@ export const DELETE = withRouteHandler(
       }
 
       invalidateEffectiveDecryptedEnvCache({ workspaceId })
-      await deleteWorkspaceEnvCredentials({ workspaceId, removedKeys: keys })
 
       recordAudit({
         workspaceId,

apps/sim/lib/copilot/chat/lifecycle.ts

@@ -212,7 +212,8 @@ export async function getAccessibleCopilotChat(
  */
 export async function getAccessibleCopilotChatWithMessages(
   chatId: string,
-  userId: string
+  userId: string,
+  options?: { includeTranscript?: boolean }
 ): Promise<CopilotChatDetailRow | null> {
   const [chat] = await db
     .select(copilotChatDetailColumns)
@@ -223,7 +224,14 @@ export async function getAccessibleCopilotChatWithMessages(
   const authorized = await authorizeCopilotChatRow(chat, chatId, userId)
   if (!authorized) return null
 
-  const messages = await loadCopilotChatMessages(chatId)
+  /**
+   * The transcript is unbounded — no per-chat message cap on write and no
+   * pruning — so a caller that only needs the chat's scope should not pay to
+   * materialize it. Every check `resolveOrCreateChat` runs reads detail
+   * columns only, so an empty list stays a truthful "not loaded" rather than
+   * "no messages" for the callers that opt out.
+   */
+  const messages = options?.includeTranscript === false ? [] : await loadCopilotChatMessages(chatId)
   return { ...authorized, messages }
 }
 
@@ -245,15 +253,22 @@ export async function resolveOrCreateChat(params: {
   model: string
   type?: 'mothership' | 'copilot'
   title?: string
+  /**
+   * Skips loading the transcript on the resume path. For a caller that keys
+   * continuity by `chatId` alone and never reads `conversationHistory`.
+   */
+  includeTranscript?: boolean
 }): Promise<ChatLoadResult> {
-  const { chatId, userId, workflowId, workspaceId, model, type, title } = params
+  const { chatId, userId, workflowId, workspaceId, model, type, title, includeTranscript } = params
 
   if (workspaceId) {
     await assertActiveWorkspaceAccess(workspaceId, userId)
   }
 
   if (chatId) {
-    const chat = await getAccessibleCopilotChatWithMessages(chatId, userId)
+    const chat = await getAccessibleCopilotChatWithMessages(chatId, userId, {
+      includeTranscript,
+    })
 
     if (chat) {
       if (workflowId && chat.workflowId !== workflowId) {

apps/sim/lib/credentials/env-locks.ts

@@ -0,0 +1,32 @@
+import { sql } from 'drizzle-orm'
+import type { DbOrTx } from '@/lib/db/types'
+
+const ENV_MAP_LOCK_TIMEOUT_MS = 5_000
+
+/**
+ * Serializes every writer of one environment variables map.
+ *
+ * Both maps are a single jsonb column that every writer read-modify-writes, so
+ * without this two concurrent writers each persist their own copy of the map
+ * and the later commit silently drops the earlier one's key. The lock is
+ * transaction-scoped, so it releases on commit or rollback with no unlock path
+ * to miss, and it must be taken before the read that the write is derived from.
+ *
+ * The keys are the bare workspace or user id, matching every writer that
+ * already takes this lock — a prefixed key would be a different lock and would
+ * serialize against nothing.
+ */
+async function lockEnvMap(tx: DbOrTx, lockKey: string): Promise<void> {
+  await tx.execute(sql`SELECT set_config('lock_timeout', ${`${ENV_MAP_LOCK_TIMEOUT_MS}ms`}, true)`)
+  await tx.execute(sql`SELECT pg_advisory_xact_lock(hashtextextended(${lockKey}, 0))`)
+}
+
+/** Serializes writers of one workspace's environment variables map. */
+export async function lockWorkspaceEnvMap(tx: DbOrTx, workspaceId: string): Promise<void> {
+  await lockEnvMap(tx, workspaceId)
+}
+
+/** Serializes writers of one user's personal environment variables map. */
+export async function lockPersonalEnvMap(tx: DbOrTx, userId: string): Promise<void> {
+  await lockEnvMap(tx, userId)
+}

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