fix(workspaces): carry the governing organization instead of a derived boolean

Replaces `permissionGroupsGovernCreation: boolean` plus a separately
re-derived organization id with the organization id itself, so the
capability gate has one field carrying one fact and no branch TypeScript
cannot see through.

- `resolveGoverningPermissionGroupOrganization` replaces both
  `isWorkspaceCreationGovernedByPermissionGroups` and
  `governingOrganizationIdFor`; the id is derived once, before the
  transaction, and reused inside it.
- The creation policy now carries the resolved organization, so the create
  route does not re-issue the entitlement read that React's `cache()` memo
  does not span in an App Route.
- `lockWorkspaceCreationContext` takes the permission-group lock after the
  organization's own `FOR UPDATE` revalidation and owner lookup, so an
  org-wide key is no longer held across a blocking row-lock wait.
- `resolveDefaultGroup` and `getEntitledOrganizationPermissionConfig`
  require their executor, so a caller cannot silently reintroduce the
  second pooled-connection checkout.
- `acquirePermissionGroupOrgLock` skips the redundant `lock_timeout`
  `set_config` when the caller already bounded it at the same value.

Author: waleedlatif1

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

@@ -2,6 +2,7 @@ import { AuditAction, AuditResourceType, recordAudit } from '@sim/audit'
 import { db } from '@sim/db'
 import { type WorkspaceMode, workflow } from '@sim/db/schema'
 import { createLogger } from '@sim/logger'
+import { getPostgresErrorCode } from '@sim/utils/errors'
 import { and, eq, isNull } from 'drizzle-orm'
 import { type NextRequest, NextResponse } from 'next/server'
 import { listWorkspacesQuerySchema } from '@/lib/api/contracts'
@@ -157,6 +158,7 @@ export const POST = withRouteHandler(async (req: NextRequest) => {
       workspaceMode: creationPolicy.workspaceMode,
       billedAccountUserId: creationPolicy.billedAccountUserId,
       observedOrganizationId: creationPolicy.observedOrganizationId,
+      governingPermissionGroupOrganizationId: creationPolicy.governingPermissionGroupOrganizationId,
     })
 
     captureServerEvent(
@@ -207,6 +209,20 @@ export const POST = withRouteHandler(async (req: NextRequest) => {
         { status: 409 }
       )
     }
+    /**
+     * A lock timeout is contention, not a fault: creation serializes on the
+     * organization's mutation locks and now also on `permission_group:<org>`,
+     * so a concurrent create or a permission-group admin write can exhaust the
+     * `lock_timeout` and abort this transaction. Answer 503 like the
+     * permission-group routes do, rather than letting it reach the generic 500
+     * below — the caller should retry, and a 500 tells them the opposite.
+     */
+    if (getPostgresErrorCode(error) === '55P03') {
+      return NextResponse.json(
+        { error: 'This organization is being updated by another request. Please try again.' },
+        { status: 503 }
+      )
+    }
     logger.error('Error creating workspace:', error)
     return NextResponse.json({ error: 'Failed to create workspace' }, { status: 500 })
   }
@@ -220,6 +236,7 @@ async function createDefaultWorkspace(
     workspaceMode: WorkspaceMode
     billedAccountUserId: string
     observedOrganizationId: string | null
+    governingPermissionGroupOrganizationId: string | null
   }
 ) {
   const firstName = userName?.split(' ')[0] || null
@@ -231,6 +248,7 @@ async function createDefaultWorkspace(
     workspaceMode: creationPolicy.workspaceMode,
     billedAccountUserId: creationPolicy.billedAccountUserId,
     observedOrganizationId: creationPolicy.observedOrganizationId,
+    governingPermissionGroupOrganizationId: creationPolicy.governingPermissionGroupOrganizationId,
   })
 }
 

apps/sim/lib/permission-groups/capability-assertions.ts

@@ -97,18 +97,9 @@ export async function isOrganizationCapabilityWithheld(
 
 /**
  * {@link isOrganizationCapabilityWithheld} for an organization whose regime the
- * caller has ALREADY established with `isOrganizationPermissionRegimeActive`,
- * reading the group on the given executor.
- *
- * For the one caller that must answer this question inside the transaction that
- * commits the governed act, under `acquirePermissionGroupOrgLock`, so an admin
- * cannot revoke the capability in the check-to-write window. Splitting the
- * entitlement half off is what lets the remaining read run on the transaction's
- * own connection instead of checking out a second pooled one.
- *
- * Passing an executor without having checked entitlement would read a stale
- * default group as authoritative for an organization the regime no longer
- * governs, so that check is the caller's obligation, not an optional one.
+ * caller has ALREADY established, reading the group on the given executor. That
+ * establishment is the caller's obligation, not an optional one — see
+ * {@link getEntitledOrganizationPermissionConfig}.
  */
 export async function isEntitledOrganizationCapabilityWithheld(
   organizationId: string,

apps/sim/lib/permission-groups/locks.ts

@@ -23,7 +23,17 @@ const PERMISSION_GROUP_LOCK_TIMEOUT_MS = 5_000
  *
  * `pg_advisory_xact_lock` auto-releases at transaction end (safe on pooled
  * connections), and `lock_timeout` bounds the wait (raising SQLSTATE 55P03)
- * instead of hanging if a holder is stuck.
+ * instead of hanging if a holder is stuck. The key string is the contention
+ * identity: any change to its format silently stops contending with in-flight
+ * holders.
+ *
+ * `lockTimeoutAlreadyBounded` skips the `set_config` round trip for a caller
+ * that has already bounded `lock_timeout` transaction-locally at the same
+ * 5000ms — every advisory lock in `lib/billing/organizations/membership.ts`
+ * does, and workspace creation takes those first. It stays a separate statement
+ * rather than being folded into the `pg_advisory_xact_lock` select: target-list
+ * evaluation order is unspecified, so the bound might not be in force when the
+ * lock is requested.
  *
  * LOCK ORDER: this is a LEAF lock. Every transaction that holds it — the five
  * `organizations/[id]/permission-groups` route transactions, and the workspace
@@ -38,11 +48,14 @@ const PERMISSION_GROUP_LOCK_TIMEOUT_MS = 5_000
  */
 export async function acquirePermissionGroupOrgLock(
   tx: DbOrTx,
-  organizationId: string
+  organizationId: string,
+  options?: { lockTimeoutAlreadyBounded?: boolean }
 ): Promise<void> {
-  await tx.execute(
-    sql`select set_config('lock_timeout', ${`${PERMISSION_GROUP_LOCK_TIMEOUT_MS}ms`}, true)`
-  )
+  if (!options?.lockTimeoutAlreadyBounded) {
+    await tx.execute(
+      sql`select set_config('lock_timeout', ${`${PERMISSION_GROUP_LOCK_TIMEOUT_MS}ms`}, true)`
+    )
+  }
   await tx.execute(
     sql`select pg_advisory_xact_lock(hashtextextended(${`permission_group:${organizationId}`}, 0))`
   )

apps/sim/lib/permission-groups/resolve.server.ts

@@ -94,14 +94,14 @@ function inactiveUserAccessControlContext(organizationId: string | null): UserAc
 /**
  * The organization's single default group (`isDefault`), or `null`.
  *
- * Takes an executor so a caller that must read the group under
- * `acquirePermissionGroupOrgLock` — inside the transaction that holds it — can
- * do so on the transaction's own connection instead of checking out a second
- * one from the pool.
+ * The executor is required, not defaulted: a caller that must read the group
+ * under `acquirePermissionGroupOrgLock` has to read it on the transaction's own
+ * connection, and a default would let that caller silently check out a second
+ * pooled connection while advisory locks are held.
  */
 async function resolveDefaultGroup(
   organizationId: string,
-  executor: DbOrTx = db
+  executor: DbOrTx
 ): Promise<ResolvedPermissionGroup | null> {
   const [defaultGroup] = await executor
     .select({
@@ -190,7 +190,7 @@ export async function resolveWorkspaceGroup(
     }
   }
 
-  return resolveDefaultGroup(organizationId)
+  return resolveDefaultGroup(organizationId, db)
 }
 
 /**
@@ -293,7 +293,7 @@ export async function getUserPermissionConfigForOrganization(
   if (!(await isOrganizationPermissionRegimeActive(organizationId))) {
     return mergeEnvAllowlist(null)
   }
-  return getEntitledOrganizationPermissionConfig(organizationId)
+  return getEntitledOrganizationPermissionConfig(organizationId, db)
 }
 
 /**
@@ -332,7 +332,7 @@ export async function isOrganizationPermissionRegimeActive(
  */
 export async function getEntitledOrganizationPermissionConfig(
   organizationId: string,
-  executor: DbOrTx = db
+  executor: DbOrTx
 ): Promise<PermissionGroupConfig | null> {
   const resolved = await resolveDefaultGroup(organizationId, executor)
   return mergeEnvAllowlist(resolved?.config ?? null)

apps/sim/lib/workspaces/create.test.ts

@@ -1,30 +1,26 @@
 /**
  * @vitest-environment node
  */
+import {
+  dbChainMockFns,
+  resetDbChainMock,
+  workflowsPersistenceUtilsMock,
+  workflowsPersistenceUtilsMockFns,
+} from '@sim/testing'
 import { beforeEach, describe, expect, it, vi } from 'vitest'
 
 const {
-  mockTransaction,
-  mockIsWorkspaceCreationGovernedByPermissionGroups,
+  mockResolveGoverningPermissionGroupOrganization,
   mockLockWorkspaceCreationContext,
   mockGetWorkspaceInvitePolicy,
-  mockSaveWorkflowToNormalizedTables,
 } = vi.hoisted(() => ({
-  mockTransaction: vi.fn(),
-  mockIsWorkspaceCreationGovernedByPermissionGroups: vi.fn(),
+  mockResolveGoverningPermissionGroupOrganization: vi.fn(),
   mockLockWorkspaceCreationContext: vi.fn(),
   mockGetWorkspaceInvitePolicy: vi.fn(),
-  mockSaveWorkflowToNormalizedTables: vi.fn(),
-}))
-
-vi.mock('@sim/db', () => ({
-  db: { transaction: mockTransaction },
 }))
 
 /** The starter workflow is not what these cases are about, and it reaches the block registry. */
-vi.mock('@/lib/workflows/persistence/utils', () => ({
-  saveWorkflowToNormalizedTables: mockSaveWorkflowToNormalizedTables,
-}))
+vi.mock('@/lib/workflows/persistence/utils', () => workflowsPersistenceUtilsMock)
 
 vi.mock('@/lib/workflows/defaults', () => ({
   buildDefaultWorkflowArtifacts: () => ({ workflowState: {} }),
@@ -34,8 +30,7 @@ vi.mock('@/lib/workspaces/policy', async (importOriginal) => {
   const actual = await importOriginal<typeof import('@/lib/workspaces/policy')>()
   return {
     ...actual,
-    isWorkspaceCreationGovernedByPermissionGroups:
-      mockIsWorkspaceCreationGovernedByPermissionGroups,
+    resolveGoverningPermissionGroupOrganization: mockResolveGoverningPermissionGroupOrganization,
     lockWorkspaceCreationContext: mockLockWorkspaceCreationContext,
     getWorkspaceInvitePolicy: mockGetWorkspaceInvitePolicy,
   }
@@ -60,29 +55,27 @@ const params = {
 describe('createWorkspace capability-gate placement', () => {
   beforeEach(() => {
     vi.clearAllMocks()
+    resetDbChainMock()
     mockGetWorkspaceInvitePolicy.mockResolvedValue({})
   })
 
   /**
-   * The ENTITLEMENT half must be settled before the transaction opens: it
-   * bottoms out in the `cache()`d `isOrganizationOnEnterprisePlan`, which admits
-   * no executor, so running it inside checked out a SECOND pooled connection
-   * while three advisory locks were held — what `packages/db/tx-tripwire.ts`
-   * fires on, and what pushed concurrent creates past the 5s `lock_timeout`.
+   * The ENTITLEMENT half must be settled before the transaction opens — see
+   * {@link resolveGoverningPermissionGroupOrganization}.
    *
    * Asserted as an explicit ordering rather than inferred from the absence of a
    * tripwire warning: nothing else in the unit suite can catch a regression
    * here, because `vitest.setup.ts` mocks `@sim/db` globally and the real pool
    * instrumentation never runs.
    */
   it('resolves the permission regime before opening the transaction', async () => {
-    mockIsWorkspaceCreationGovernedByPermissionGroups.mockResolvedValue(true)
+    mockResolveGoverningPermissionGroupOrganization.mockResolvedValue('org-1')
     /**
      * The callback is deliberately NOT invoked, so the transaction's own
      * internals stay out of the assertion and cannot fail it for an unrelated
      * reason.
      */
-    mockTransaction.mockResolvedValue({
+    dbChainMockFns.transaction.mockResolvedValue({
       id: 'ws-1',
       name: params.name,
       organizationId: 'org-1',
@@ -93,27 +86,27 @@ describe('createWorkspace capability-gate placement', () => {
 
     await createWorkspace(params)
 
-    expect(mockIsWorkspaceCreationGovernedByPermissionGroups).toHaveBeenCalledWith({
+    expect(mockResolveGoverningPermissionGroupOrganization).toHaveBeenCalledWith({
       organizationId: 'org-1',
       observedOrganizationId: 'org-1',
     })
     expect(
-      mockIsWorkspaceCreationGovernedByPermissionGroups.mock.invocationCallOrder[0]
-    ).toBeLessThan(mockTransaction.mock.invocationCallOrder[0])
+      mockResolveGoverningPermissionGroupOrganization.mock.invocationCallOrder[0]
+    ).toBeLessThan(dbChainMockFns.transaction.mock.invocationCallOrder[0])
   })
 
   /**
    * The capability itself is enforced INSIDE the transaction, under the
-   * permission-group lock — so the regime answer has to reach
+   * permission-group lock — so the governing organization has to reach
    * `lockWorkspaceCreationContext`. Dropping it there would silently skip the
    * gate for every governed organization.
    */
-  it('carries the regime answer into the locked creation context', async () => {
-    mockIsWorkspaceCreationGovernedByPermissionGroups.mockResolvedValue(true)
+  it('carries the governing organization into the locked creation context', async () => {
+    mockResolveGoverningPermissionGroupOrganization.mockResolvedValue('org-1')
     mockLockWorkspaceCreationContext.mockResolvedValue({ billedAccountUserId: 'creator-1' })
     const tx = { insert: vi.fn(() => ({ values: vi.fn() })) } as unknown as DbOrTx
-    mockTransaction.mockImplementation((callback: (executor: DbOrTx) => Promise<unknown>) =>
-      callback(tx)
+    dbChainMockFns.transaction.mockImplementation(
+      (callback: (executor: DbOrTx) => Promise<unknown>) => callback(tx)
     )
 
     await createWorkspace({ ...params, skipDefaultWorkflow: true })
@@ -122,14 +115,40 @@ describe('createWorkspace capability-gate placement', () => {
       userId: 'creator-1',
       organizationId: 'org-1',
       observedOrganizationId: 'org-1',
-      permissionGroupsGovernCreation: true,
+      governingPermissionGroupOrganizationId: 'org-1',
+    })
+  })
+
+  /**
+   * The preflight policy resolved this value microseconds earlier in the same
+   * request, and React's `cache()` memo does not span the two calls, so a
+   * forwarded answer must be used as-is rather than re-read.
+   */
+  it('reuses the governing organization the caller already resolved', async () => {
+    mockLockWorkspaceCreationContext.mockResolvedValue({ billedAccountUserId: 'creator-1' })
+    const tx = { insert: vi.fn(() => ({ values: vi.fn() })) } as unknown as DbOrTx
+    dbChainMockFns.transaction.mockImplementation(
+      (callback: (executor: DbOrTx) => Promise<unknown>) => callback(tx)
+    )
+
+    await createWorkspace({
+      ...params,
+      skipDefaultWorkflow: true,
+      governingPermissionGroupOrganizationId: 'org-1',
     })
+
+    expect(mockResolveGoverningPermissionGroupOrganization).not.toHaveBeenCalled()
+    expect(mockLockWorkspaceCreationContext).toHaveBeenCalledWith(
+      tx,
+      expect.objectContaining({ governingPermissionGroupOrganizationId: 'org-1' })
+    )
   })
 })
 
 describe('createDefaultPersonalWorkspaceInTransaction', () => {
   beforeEach(() => {
     vi.clearAllMocks()
+    resetDbChainMock()
   })
 
   /**
@@ -147,12 +166,27 @@ describe('createDefaultPersonalWorkspaceInTransaction', () => {
       userName: 'Ada Lovelace',
     })
 
-    expect(mockIsWorkspaceCreationGovernedByPermissionGroups).not.toHaveBeenCalled()
+    expect(mockResolveGoverningPermissionGroupOrganization).not.toHaveBeenCalled()
     expect(mockLockWorkspaceCreationContext).toHaveBeenCalledWith(tx, {
       userId: 'user-1',
       organizationId: null,
       observedOrganizationId: null,
-      permissionGroupsGovernCreation: false,
+      governingPermissionGroupOrganizationId: null,
     })
   })
+
+  /** The starter workflow is built before the locks, so it must still be written. */
+  it('seeds the starter workflow it built before taking the locks', async () => {
+    mockLockWorkspaceCreationContext.mockResolvedValue({ billedAccountUserId: 'user-1' })
+    const tx = { insert: vi.fn(() => ({ values: vi.fn() })) } as unknown as DbOrTx
+
+    await createDefaultPersonalWorkspaceInTransaction(tx, {
+      userId: 'user-1',
+      userName: 'Ada Lovelace',
+    })
+
+    expect(
+      workflowsPersistenceUtilsMockFns.mockSaveWorkflowToNormalizedTables
+    ).toHaveBeenCalledWith(expect.any(String), {}, { workspaceId: null, subjectUserId: null }, tx)
+  })
 })