Skip to content

Commit 54925ae

Browse files
committed
fix(workspaces): gate workspace creation before the transaction, not inside it
`lockWorkspaceCreationContext` called `isOrganizationCapabilityWithheld` with no executor, so it ran on the global `db` pool while the creation transaction already held a pooled connection and three advisory locks. That is the pool deadlock `packages/db/tx-tripwire.ts` exists to detect — it is firing in staging right now, four times per request, 144 lines across 36 requests in six hours, all on POST /api/workspaces. It also added up to four SEQUENTIAL reads to a lock hold that serializes every organization mutation, which is what pushed concurrent creates past the 5s lock_timeout and answered them as a generic 500. Nothing is given up by moving the gate out. The re-read was never serialized against permission-group writes: those take `permission_group:<org>` while creation takes `organization-mutation:<org>`, which are different advisory-lock ids and never contend. What the check actually provides is recency against the caller's earlier preflight, and that holds identically microseconds earlier. The governing organization is `organizationId ?? observedOrganizationId`, both known before the transaction, and `lockWorkspaceCreationContext` still refuses to commit unless live membership still equals `observedOrganizationId` — so a verdict computed outside can never be applied to a different organization. The comment justifying the old placement was also wrong: for POST /api/workspaces the preflight and the insert are the same request, not separate ones. Rejected alternatives: threading an executor through the resolver would touch seven functions across four files, requires exporting the uncached `resolveOrganizationEnterprisePlan`, and bypassing the React cache is a fail-OPEN semantic change. `runOutsideTransactionContext` only exits the ambient context — it would silence the tripwire while leaving the second pooled connection checkout in place. Adds create.test.ts pinning the ordering, because no existing test can catch a regression here: vitest.setup.ts mocks @sim/db globally, so the real pool instrumentation never runs and the tripwire cannot fire in any apps/sim test.
1 parent 8c2f206 commit 54925ae

4 files changed

Lines changed: 224 additions & 87 deletions

File tree

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { beforeEach, describe, expect, it, vi } from 'vitest'
5+
6+
const {
7+
mockTransaction,
8+
mockAssertWorkspaceCreationCapability,
9+
mockLockWorkspaceCreationContext,
10+
mockGetWorkspaceInvitePolicy,
11+
} = vi.hoisted(() => ({
12+
mockTransaction: vi.fn(),
13+
mockAssertWorkspaceCreationCapability: vi.fn(),
14+
mockLockWorkspaceCreationContext: vi.fn(),
15+
mockGetWorkspaceInvitePolicy: vi.fn(),
16+
}))
17+
18+
vi.mock('@sim/db', () => ({
19+
db: { transaction: mockTransaction },
20+
}))
21+
22+
vi.mock('@/lib/workspaces/policy', async (importOriginal) => {
23+
const actual = await importOriginal<typeof import('@/lib/workspaces/policy')>()
24+
return {
25+
...actual,
26+
assertWorkspaceCreationCapability: mockAssertWorkspaceCreationCapability,
27+
lockWorkspaceCreationContext: mockLockWorkspaceCreationContext,
28+
getWorkspaceInvitePolicy: mockGetWorkspaceInvitePolicy,
29+
}
30+
})
31+
32+
import { createWorkspace } from '@/lib/workspaces/create'
33+
import { WORKSPACE_MODE, WorkspaceCreationCapabilityWithheldError } from '@/lib/workspaces/policy'
34+
35+
const params = {
36+
userId: 'creator-1',
37+
observedOrganizationId: 'org-1',
38+
name: 'Test Workspace',
39+
organizationId: 'org-1',
40+
workspaceMode: WORKSPACE_MODE.ORGANIZATION,
41+
billedAccountUserId: 'creator-1',
42+
}
43+
44+
describe('createWorkspace capability gate placement', () => {
45+
beforeEach(() => {
46+
vi.clearAllMocks()
47+
mockGetWorkspaceInvitePolicy.mockResolvedValue({})
48+
})
49+
50+
/**
51+
* The gate resolves the organization's entitlement and default group — up to
52+
* four sequential reads. Run inside the transaction it checked out a SECOND
53+
* pooled connection while three advisory locks were held, which is what
54+
* `packages/db/tx-tripwire.ts` fires on and what pushed concurrent creates
55+
* past the 5s `lock_timeout` into a generic 500.
56+
*
57+
* Ordering is the whole fix, so it is asserted directly rather than inferred
58+
* from the absence of a tripwire warning: nothing else in the unit suite can
59+
* catch a regression here, because `vitest.setup.ts` mocks `@sim/db` globally
60+
* and the real pool instrumentation never runs.
61+
*/
62+
it('gates before opening the transaction, not inside it', async () => {
63+
/**
64+
* The callback is deliberately NOT invoked. This pins the ORDER of the gate
65+
* against `db.transaction`, so the transaction's own internals stay out of
66+
* the assertion and cannot make it fail for an unrelated reason.
67+
*/
68+
mockTransaction.mockResolvedValue({
69+
id: 'ws-1',
70+
name: params.name,
71+
organizationId: 'org-1',
72+
workspaceMode: WORKSPACE_MODE.ORGANIZATION,
73+
billedAccountUserId: 'creator-1',
74+
ownerId: 'creator-1',
75+
})
76+
77+
await createWorkspace(params)
78+
79+
expect(mockAssertWorkspaceCreationCapability).toHaveBeenCalledWith({
80+
organizationId: 'org-1',
81+
observedOrganizationId: 'org-1',
82+
})
83+
expect(mockAssertWorkspaceCreationCapability.mock.invocationCallOrder[0]).toBeLessThan(
84+
mockTransaction.mock.invocationCallOrder[0]
85+
)
86+
})
87+
88+
/** A withheld capability must refuse before any transaction is opened at all. */
89+
it('never opens a transaction when the capability is withheld', async () => {
90+
mockAssertWorkspaceCreationCapability.mockRejectedValue(
91+
new WorkspaceCreationCapabilityWithheldError()
92+
)
93+
94+
await expect(createWorkspace(params)).rejects.toBeInstanceOf(
95+
WorkspaceCreationCapabilityWithheldError
96+
)
97+
expect(mockTransaction).not.toHaveBeenCalled()
98+
})
99+
})

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

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import { buildDefaultWorkflowArtifacts } from '@/lib/workflows/defaults'
88
import { saveWorkflowToNormalizedTables } from '@/lib/workflows/persistence/utils'
99
import { getRandomWorkspaceColor } from '@/lib/workspaces/colors'
1010
import {
11+
assertWorkspaceCreationCapability,
1112
getWorkspaceInvitePolicy,
1213
lockWorkspaceCreationContext,
1314
resolveInviteFlags,
@@ -59,6 +60,15 @@ export function emitWorkspaceCreatedPlatformEvent(params: {
5960
* that snapshot under the shared organization/user locks before inserting the
6061
* workspace, owner permission, and optional starter workflow atomically.
6162
*/
63+
/**
64+
* The workspace.create capability gate is the CALLER's responsibility, because
65+
* it must not run inside this transaction — see
66+
* {@link assertWorkspaceCreationCapability}. `createWorkspace` calls it before
67+
* opening its transaction; the only other entry point,
68+
* {@link createDefaultPersonalWorkspaceInTransaction}, passes both
69+
* `organizationId` and `observedOrganizationId` as `null` by construction, so
70+
* the gate is a no-op on that path rather than a skipped check.
71+
*/
6272
export async function createWorkspaceInTransaction(
6373
tx: DbOrTx,
6474
{
@@ -167,6 +177,18 @@ export async function createWorkspaceInTransaction(
167177

168178
/** Creates a workspace through the canonical lock-and-insert transaction. */
169179
export async function createWorkspace(params: CreateWorkspaceParams) {
180+
/**
181+
* Gate before opening the transaction, never inside it — see
182+
* {@link assertWorkspaceCreationCapability} for why the lock never protected
183+
* this read. Its `WorkspaceCreationCapabilityWithheldError` is thrown from the
184+
* same call stack as before, so `app/api/workspaces/route.ts` still projects
185+
* the identical capability refusal.
186+
*/
187+
await assertWorkspaceCreationCapability({
188+
organizationId: params.organizationId,
189+
observedOrganizationId: params.observedOrganizationId,
190+
})
191+
170192
let created: CreatedWorkspace
171193
try {
172194
created = await db.transaction((tx) => createWorkspaceInTransaction(tx, params))

‎apps/sim/lib/workspaces/policy.test.ts‎

Lines changed: 62 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ vi.mock('@/lib/billing/core/plan', () => ({
4747
}))
4848

4949
import {
50+
assertWorkspaceCreationCapability,
5051
getOrganizationOwnerId,
5152
getWorkspaceCreationPolicy,
5253
getWorkspaceInvitePolicy,
@@ -77,6 +78,67 @@ describe('getOrganizationOwnerId', () => {
7778
})
7879
})
7980

81+
/**
82+
* The capability gate moved OUT of `lockWorkspaceCreationContext` so it no
83+
* longer runs on a second pooled connection inside the lock-holding
84+
* transaction. These three cases are the ones that lived there, retargeted
85+
* rather than rewritten — the gate's behavior is unchanged, only where it runs.
86+
*/
87+
describe('assertWorkspaceCreationCapability', () => {
88+
it('rejects when the group withheld workspace creation after the preflight', async () => {
89+
vi.clearAllMocks()
90+
resetDbChainMock()
91+
setEnvFlags({ isBillingEnabled: false })
92+
mockGetUserPermissionConfigForOrganization.mockResolvedValue({
93+
disableWorkspaceCreation: true,
94+
})
95+
96+
await expect(
97+
assertWorkspaceCreationCapability({
98+
organizationId: 'org-1',
99+
observedOrganizationId: 'org-1',
100+
})
101+
).rejects.toBeInstanceOf(WorkspaceCreationCapabilityWithheldError)
102+
expect(mockGetUserPermissionConfigForOrganization).toHaveBeenCalledWith('org-1')
103+
})
104+
105+
/**
106+
* A personal workspace is precisely the escape from a scoped group, so the
107+
* gate reads the caller's membership organization even when the workspace
108+
* being inserted carries none — the same organization the preflight used.
109+
* `observedOrganizationId` carries that membership, and
110+
* `lockWorkspaceCreationContext` still refuses to commit if live membership
111+
* has since diverged from it.
112+
*/
113+
it('rejects a personal workspace when the membership organization withheld creation', async () => {
114+
vi.clearAllMocks()
115+
resetDbChainMock()
116+
mockGetUserPermissionConfigForOrganization.mockResolvedValue({
117+
disableWorkspaceCreation: true,
118+
})
119+
120+
await expect(
121+
assertWorkspaceCreationCapability({
122+
organizationId: null,
123+
observedOrganizationId: 'org-1',
124+
})
125+
).rejects.toBeInstanceOf(WorkspaceCreationCapabilityWithheldError)
126+
})
127+
128+
it('leaves an unaffiliated creator alone, with no group to read', async () => {
129+
vi.clearAllMocks()
130+
resetDbChainMock()
131+
132+
await expect(
133+
assertWorkspaceCreationCapability({
134+
organizationId: null,
135+
observedOrganizationId: null,
136+
})
137+
).resolves.toBeUndefined()
138+
expect(mockGetUserPermissionConfigForOrganization).not.toHaveBeenCalled()
139+
})
140+
})
141+
80142
describe('lockWorkspaceCreationContext', () => {
81143
it('locks the destination organization and user before rejecting a stale org-mode policy', async () => {
82144
vi.clearAllMocks()
@@ -138,75 +200,6 @@ describe('lockWorkspaceCreationContext', () => {
138200
)
139201
})
140202

141-
/**
142-
* The preflight in `getWorkspaceCreationPolicy` and the insert are separate
143-
* requests. A group that withheld creation in between has to be caught under
144-
* the lock, or the in-flight create lands a workspace that carries no
145-
* `permissionGroupWorkspace` row to bring it back under the regime.
146-
*/
147-
it('rejects when the group withheld workspace creation after the preflight', async () => {
148-
vi.clearAllMocks()
149-
resetDbChainMock()
150-
setEnvFlags({ isBillingEnabled: false })
151-
mockAcquireOrganizationUserMutationLocks.mockResolvedValue(undefined)
152-
mockGetUserOrganization.mockResolvedValue({ organizationId: 'org-1', role: 'admin' })
153-
mockGetUserPermissionConfigForOrganization.mockResolvedValue({
154-
disableWorkspaceCreation: true,
155-
})
156-
queueTableRows(member, [{ userId: 'owner-1' }])
157-
const tx = dbChainMock.db as unknown as DbOrTx
158-
159-
await expect(
160-
lockWorkspaceCreationContext(tx, {
161-
userId: 'creator-1',
162-
organizationId: 'org-1',
163-
observedOrganizationId: 'org-1',
164-
})
165-
).rejects.toBeInstanceOf(WorkspaceCreationCapabilityWithheldError)
166-
expect(mockGetUserPermissionConfigForOrganization).toHaveBeenCalledWith('org-1')
167-
})
168-
169-
/**
170-
* A personal workspace is precisely the escape from a scoped group, so the
171-
* re-check reads the caller's membership organization even when the workspace
172-
* being inserted carries none — the same organization the preflight used.
173-
*/
174-
it('rejects a personal workspace when the membership organization withheld creation', async () => {
175-
vi.clearAllMocks()
176-
resetDbChainMock()
177-
mockAcquireOrganizationUserMutationLocks.mockResolvedValue(undefined)
178-
mockGetUserOrganization.mockResolvedValue({ organizationId: 'org-1', role: 'member' })
179-
mockGetUserPermissionConfigForOrganization.mockResolvedValue({
180-
disableWorkspaceCreation: true,
181-
})
182-
const tx = dbChainMock.db as unknown as DbOrTx
183-
184-
await expect(
185-
lockWorkspaceCreationContext(tx, {
186-
userId: 'creator-1',
187-
organizationId: null,
188-
observedOrganizationId: 'org-1',
189-
})
190-
).rejects.toBeInstanceOf(WorkspaceCreationCapabilityWithheldError)
191-
})
192-
193-
it('leaves an unaffiliated creator alone, with no group to read', async () => {
194-
vi.clearAllMocks()
195-
resetDbChainMock()
196-
mockAcquireOrganizationUserMutationLocks.mockResolvedValue(undefined)
197-
mockGetUserOrganization.mockResolvedValue(null)
198-
const tx = dbChainMock.db as unknown as DbOrTx
199-
200-
await expect(
201-
lockWorkspaceCreationContext(tx, {
202-
userId: 'creator-1',
203-
organizationId: null,
204-
observedOrganizationId: null,
205-
})
206-
).resolves.toEqual({ billedAccountUserId: 'creator-1' })
207-
expect(mockGetUserPermissionConfigForOrganization).not.toHaveBeenCalled()
208-
})
209-
210203
it('rejects when the paid org entitlement disappeared before insertion', async () => {
211204
vi.clearAllMocks()
212205
resetDbChainMock()

‎apps/sim/lib/workspaces/policy.ts‎

Lines changed: 41 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,47 @@ export class WorkspaceCreationCapabilityWithheldError extends WorkspaceCreationC
127127
* Returns the live billing owner. The caller must invoke this in the same
128128
* transaction as the workspace insert.
129129
*/
130+
/**
131+
* permission-group-enforced: workspace.create — the creation gate, deliberately
132+
* OUTSIDE the creation transaction.
133+
*
134+
* This resolves the organization's entitlement and default group, which is up to
135+
* four sequential reads. Running it inside {@link lockWorkspaceCreationContext}
136+
* checked out a SECOND pooled connection while that transaction already held one
137+
* plus three advisory locks — the pool deadlock `packages/db/tx-tripwire.ts`
138+
* exists to detect — and added those round trips to a lock hold that serializes
139+
* every organization mutation, which is what pushed concurrent creates past the
140+
* 5s `lock_timeout` and answered them as a generic 500.
141+
*
142+
* Nothing is given up by moving it out. The re-read was never serialized against
143+
* permission-group writes: those take `permission_group:<org>`
144+
* (`organizations/[id]/permission-groups/utils.ts`) while creation takes
145+
* `organization-mutation:<org>` (`billing/organizations/membership.ts`). Those
146+
* are different advisory-lock ids and never contend, so holding the lock while
147+
* reading bought no exclusion. What the check actually provides is RECENCY
148+
* against the caller's earlier preflight, and that holds identically here.
149+
*
150+
* The governing organization is `organizationId ?? observedOrganizationId`,
151+
* both known before the transaction. That is equivalent to the membership read
152+
* this previously used, because {@link lockWorkspaceCreationContext} throws
153+
* `WorkspaceCreationContextChangedError` unless live membership still equals
154+
* `observedOrganizationId` — so a verdict computed here can never be applied to
155+
* an organization other than the one that commits.
156+
*/
157+
export async function assertWorkspaceCreationCapability({
158+
organizationId,
159+
observedOrganizationId,
160+
}: {
161+
organizationId: string | null
162+
observedOrganizationId: string | null
163+
}): Promise<void> {
164+
const governingOrganizationId = organizationId ?? observedOrganizationId
165+
if (!governingOrganizationId) return
166+
if (await isOrganizationCapabilityWithheld(governingOrganizationId, 'workspace.create')) {
167+
throw new WorkspaceCreationCapabilityWithheldError()
168+
}
169+
}
170+
130171
export async function lockWorkspaceCreationContext(
131172
tx: DbOrTx,
132173
{
@@ -151,24 +192,6 @@ export async function lockWorkspaceCreationContext(
151192
throw new WorkspaceCreationContextChangedError()
152193
}
153194

154-
/**
155-
* permission-group-enforced: workspace.create — re-read under the lock because
156-
* the preflight in `getWorkspaceCreationPolicy` and the insert are separate
157-
* requests: a group that withheld creation in between would otherwise still
158-
* let the in-flight create land, and a new workspace carries no
159-
* `permissionGroupWorkspace` row to bring it back under the regime afterwards.
160-
* Governed by the same organization the preflight used — the explicit one, or
161-
* the caller's membership when the workspace would be personal — so a personal
162-
* workspace stays as governed here as it is there.
163-
*/
164-
const governingOrganizationId = organizationId ?? currentMembership?.organizationId ?? null
165-
if (
166-
governingOrganizationId &&
167-
(await isOrganizationCapabilityWithheld(governingOrganizationId, 'workspace.create'))
168-
) {
169-
throw new WorkspaceCreationCapabilityWithheldError()
170-
}
171-
172195
if (!organizationId) return { billedAccountUserId: userId }
173196

174197
if (isBillingEnabled) {

0 commit comments

Comments
 (0)