Skip to content

Commit 4905ae9

Browse files
committed
fix(projects): address review on Project locking, archive notifications and list policy cost
1 parent d48264a commit 4905ae9

6 files changed

Lines changed: 90 additions & 31 deletions

File tree

‎apps/sim/ee/workspace-forking/lib/promote/promote.test.ts‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -247,6 +247,8 @@ function emptyCopyResult() {
247247

248248
beforeEach(() => {
249249
resetDbChainMock()
250+
queueTableRows(workspace, [{ name: 'Target' }])
251+
queueTableRows(workspace, [{ archivedAt: null, forkSyncNewWorkflowsExcluded: false }])
250252
mockGetUsersWithPermissions.mockResolvedValue([])
251253
mockLoadSourceDeployedStates.mockResolvedValue({
252254
deployedWorkflows: [],
@@ -367,11 +369,6 @@ describe('promoteFork gates', () => {
367369
})
368370

369371
describe('promoteFork dependent values', () => {
370-
beforeEach(() => {
371-
queueTableRows(workspace, [{ name: 'Target' }])
372-
queueTableRows(workspace, [{ archivedAt: null, forkSyncNewWorkflowsExcluded: false }])
373-
})
374-
375372
it('unions the dependent-value picks into the copy discovery set (a re-picked document must be copied)', async () => {
376373
mockComputePlan.mockResolvedValue(
377374
makePlan({
@@ -482,8 +479,6 @@ describe('promoteFork dependent values', () => {
482479

483480
describe('promoteFork trigger URLs', () => {
484481
beforeEach(() => {
485-
queueTableRows(workspace, [{ name: 'Target' }])
486-
queueTableRows(workspace, [{ archivedAt: null, forkSyncNewWorkflowsExcluded: false }])
487482
// A block only holds a public URL when its config declares a `useWebhookUrl` field, so the
488483
// fixture has to look like a webhook trigger to the shared predicate.
489484
vi.mocked(getBlock).mockReturnValue({

‎apps/sim/ee/workspace-forking/lib/promote/promote.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -630,6 +630,8 @@ export async function promoteFork(params: PromoteForkParams): Promise<PromoteFor
630630
await acquireForkTargetLock(tx, targetWorkspaceId)
631631
await acquireForkEdgeLock(tx, edge.childWorkspaceId)
632632
await lockForkRevision(tx, revisionScope)
633+
/** Before any target write, so a concurrent archive and this promote lock in one order. */
634+
await lockActiveWorkspace(tx, targetWorkspaceId)
633635
if (admission) {
634636
await assertForkPreviewFresh(tx, revisionScope, admission)
635637
}
@@ -997,7 +999,6 @@ export async function promoteFork(params: PromoteForkParams): Promise<PromoteFor
997999
const writtenItems: typeof plan.items = []
9981000
const needsConfiguration: PromoteTxApplied['needsConfiguration'] = []
9991001
const clearedOptional: PromoteTxApplied['clearedOptional'] = []
1000-
if (plan.items.length) await lockActiveWorkspace(tx, targetWorkspaceId)
10011002
for (const item of plan.items) {
10021003
// Use the pre-read source state (loaded above, before the tx). An item only
10031004
// exists when its state was present at read time, so this lookup hits; the

‎apps/sim/lib/projects/__integration__/foundation.integration.ts‎

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -174,15 +174,21 @@ async function fixture(org = true, count = 2) {
174174
return { ownerId, teammateId, outsiderId, organizationId, projectId, ids, owner, teammate }
175175
}
176176

177-
/** Waits until a session is blocked behind `blockerPid`'s locks. */
178-
async function waitUntilBlockedBy(blockerPid: number) {
177+
/**
178+
* Waits until the operation under test is blocked behind `blockerPid`: a session whose
179+
* current statement contains `waitingIn` (an advisory lock tag or row-lock clause), so an
180+
* unrelated waiter cannot release the barrier early.
181+
*/
182+
async function waitUntilBlockedBy(blockerPid: number, waitingIn: string) {
179183
await expect
180184
.poll(
181185
async () =>
182186
(
183-
await db.execute(
184-
sql`SELECT 1 FROM pg_stat_activity WHERE ${blockerPid} = ANY(pg_blocking_pids(pid))`
185-
)
187+
await db.execute(sql`
188+
SELECT 1 FROM pg_stat_activity
189+
WHERE ${blockerPid} = ANY(pg_blocking_pids(pid))
190+
AND position(${waitingIn.toLowerCase()} in lower(query)) > 0
191+
`)
186192
).length,
187193
{ timeout: 2000, interval: 10 }
188194
)
@@ -457,7 +463,7 @@ describe('Project foundation at the database and application boundary', () => {
457463
name: 'Concurrent child',
458464
})
459465
try {
460-
await waitUntilBlockedBy(await locked.promise)
466+
await waitUntilBlockedBy(await locked.promise, "lock='project_backfill'")
461467
} finally {
462468
release.resolve()
463469
}
@@ -841,7 +847,7 @@ describe('Project foundation at the database and application boundary', () => {
841847
{}
842848
)
843849
try {
844-
await waitUntilBlockedBy(blocker)
850+
await waitUntilBlockedBy(blocker, 'for share')
845851
} finally {
846852
release.resolve()
847853
await archive
@@ -870,7 +876,7 @@ describe('Project foundation at the database and application boundary', () => {
870876
(error: unknown) => error
871877
)
872878
try {
873-
await waitUntilBlockedBy(blocker)
879+
await waitUntilBlockedBy(blocker, 'for share')
874880
} finally {
875881
release.resolve()
876882
await archive
@@ -1196,7 +1202,7 @@ describe('Project foundation at the database and application boundary', () => {
11961202
prepareProjectsForAccountDeletion(tx, f.ownerId, [f.ids[1]])
11971203
)
11981204
try {
1199-
await waitUntilBlockedBy(blocker)
1205+
await waitUntilBlockedBy(blocker, "lock='project'")
12001206
} finally {
12011207
release.resolve()
12021208
await unlink
@@ -1226,7 +1232,7 @@ describe('Project foundation at the database and application boundary', () => {
12261232
const blocker = await held.promise
12271233
const ban = disableUserResources(f.ownerId)
12281234
try {
1229-
await waitUntilBlockedBy(blocker)
1235+
await waitUntilBlockedBy(blocker, "lock='project'")
12301236
} finally {
12311237
release.resolve()
12321238
await transfer

‎apps/sim/lib/projects/account-deletion.ts‎

Lines changed: 31 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -39,15 +39,20 @@ type ProjectDeletionDecision =
3939
| { remove: true }
4040
| { archive: boolean; ownerId?: string }
4141

42-
/** An org admin, else a teammate who administers every surviving environment. */
42+
/**
43+
* An org admin, else a teammate who administers every surviving environment. With `hold`,
44+
* the successor's membership or grants stay share-locked until commit, so the handoff
45+
* cannot land on someone demoted concurrently.
46+
*/
4347
async function findProjectSuccessor(
4448
executor: DbOrTx,
4549
record: typeof project.$inferSelect,
4650
userId: string,
47-
survivorIds: string[]
51+
survivorIds: string[],
52+
hold: boolean
4853
): Promise<string | null> {
4954
if (record.organizationId) {
50-
const [admin] = await executor
55+
const adminQuery = executor
5156
.select({ userId: member.userId })
5257
.from(member)
5358
.where(
@@ -59,6 +64,7 @@ async function findProjectSuccessor(
5964
)
6065
.orderBy(asc(member.userId))
6166
.limit(1)
67+
const [admin] = await (hold ? adminQuery.for('share') : adminQuery)
6268
if (admin) return admin.userId
6369
}
6470
const [teammate] = await executor
@@ -76,7 +82,20 @@ async function findProjectSuccessor(
7682
.having(sql`count(*) = ${survivorIds.length}`)
7783
.orderBy(asc(permissions.userId))
7884
.limit(1)
79-
return teammate?.userId ?? null
85+
if (!teammate || !hold) return teammate?.userId ?? null
86+
const held = await executor
87+
.select({ id: permissions.id })
88+
.from(permissions)
89+
.where(
90+
and(
91+
eq(permissions.entityType, 'workspace'),
92+
eq(permissions.permissionType, 'admin'),
93+
eq(permissions.userId, teammate.userId),
94+
inArray(permissions.entityId, survivorIds)
95+
)
96+
)
97+
.for('share')
98+
return held.length === survivorIds.length ? teammate.userId : null
8099
}
81100

82101
interface ProjectEnvironment {
@@ -111,7 +130,8 @@ async function planProjectDeletion(
111130
record: typeof project.$inferSelect,
112131
members: ProjectEnvironment[],
113132
userId: string,
114-
doomed: Set<string>
133+
doomed: Set<string>,
134+
hold: boolean
115135
): Promise<ProjectDeletionDecision> {
116136
const survivors = members.filter((row) => !doomed.has(row.id))
117137
if (!survivors.length) {
@@ -125,7 +145,8 @@ async function planProjectDeletion(
125145
executor,
126146
record,
127147
userId,
128-
survivors.map((row) => row.id)
148+
survivors.map((row) => row.id),
149+
hold
129150
)
130151
return ownerId
131152
? { archive, ownerId }
@@ -153,7 +174,8 @@ export async function getProjectAccountDeletionBlockers(
153174
record,
154175
environments.get(record.id) ?? [],
155176
userId,
156-
doomed
177+
doomed,
178+
false
157179
)
158180
if ('blocker' in decision) blockers.push(decision.blocker)
159181
}
@@ -198,7 +220,8 @@ export async function prepareProjectsForAccountDeletion(
198220
record,
199221
environments.get(record.id) ?? [],
200222
userId,
201-
doomed
223+
doomed,
224+
true
202225
)
203226
if ('blocker' in decision) throw new ProjectConflictError(decision.blocker)
204227
if ('remove' in decision) {

‎apps/sim/lib/projects/application/authorization.ts‎

Lines changed: 38 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,18 @@
11
import type { Principal, SessionPrincipal } from '@sim/auth/principal'
2-
import { member, permissions, project, projectWorkspace, workspace } from '@sim/db/schema'
2+
import {
3+
member,
4+
permissionGroup,
5+
permissions,
6+
project,
7+
projectWorkspace,
8+
workspace,
9+
} from '@sim/db/schema'
310
import { createLogger } from '@sim/logger'
411
import { isOrgAdminRole } from '@sim/platform-authz/workspace'
5-
import { and, asc, eq, inArray } from 'drizzle-orm'
12+
import { and, asc, eq, inArray, sql } from 'drizzle-orm'
613
import { PrincipalKindAuthorizationError } from '@/lib/core/application/workspace-authorization'
714
import { OrchestrationError } from '@/lib/core/orchestration/types'
15+
import { textArrayLiteral } from '@/lib/db/arrays'
816
import type { DbTransaction } from '@/lib/db/types'
917
import { CAPABILITY_RULES, refuseCapability } from '@/lib/permission-groups/capabilities'
1018
import { acquirePermissionGroupOrgLock } from '@/lib/permission-groups/locks'
@@ -48,7 +56,8 @@ type ProjectAccess = Awaited<ReturnType<typeof loadProjectAccess>>
4856

4957
/**
5058
* Loads the caller's org role and every environment with its grant for `records` in three
51-
* queries, whatever their count.
59+
* queries, whatever their count. A snapshot read also learns, in one more query, which
60+
* records any permission group restricts, so unrestricted ones skip per-environment policy.
5261
*/
5362
async function loadProjectAccess(
5463
tx: DbTransaction,
@@ -106,7 +115,27 @@ async function loadProjectAccess(
106115
if (rows) rows.push(access)
107116
else environmentsByProject.set(projectId, [access])
108117
}
118+
const organizationIds = [
119+
...new Set(records.flatMap((record) => (record.organizationId ? [record.organizationId] : []))),
120+
]
121+
const restricted =
122+
mode === 'snapshot' && organizationIds.length
123+
? new Set(
124+
(
125+
await tx.execute<{ id: string }>(sql`
126+
SELECT DISTINCT denied.id FROM ${permissionGroup},
127+
jsonb_array_elements_text(
128+
COALESCE(${permissionGroup.config}->'deniedPartialAccessProjectIssues', '[]'::jsonb)
129+
) AS denied(id)
130+
WHERE ${inArray(permissionGroup.organizationId, organizationIds)}
131+
AND denied.id = ANY(${textArrayLiteral(records.map((record) => record.id))})
132+
`)
133+
).map((row) => row.id)
134+
)
135+
: null
109136
return {
137+
/** Whether a permission group might restrict Issues for the Project; held reads check all. */
138+
mayRestrictIssues: (projectId: string) => restricted === null || restricted.has(projectId),
110139
isOrgAdmin: (organizationId: string | null) =>
111140
organizationId !== null &&
112141
membership?.organizationId === organizationId &&
@@ -145,7 +174,12 @@ async function evaluateProjectAccess(
145174
'Organization admin or admin access to every environment is required'
146175
)
147176
let canUseIssues = !record.archivedAt && visible.length > 0
148-
if (canUseIssues && visible.length < active.length && record.organizationId) {
177+
if (
178+
canUseIssues &&
179+
visible.length < active.length &&
180+
record.organizationId &&
181+
access.mayRestrictIssues(record.id)
182+
) {
149183
for (const environment of visible) {
150184
const { config } = await resolveVerifiedUserAccessControlContext(
151185
principal.userId,

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -241,7 +241,7 @@ async function archiveWorkspaceRecordsInTransaction(
241241
isPublic: false,
242242
updatedAt: now,
243243
})
244-
.where(and(eq(workflowMcpServer.workspaceId, workspaceId), isNull(workflowMcpServer.deletedAt)))
244+
.where(eq(workflowMcpServer.workspaceId, workspaceId))
245245
.returning({ id: workflowMcpServer.id })
246246

247247
await tx

0 commit comments

Comments
 (0)