From 118fa73d1d7689b07a81c6dcbf6f12e8a6771331 Mon Sep 17 00:00:00 2001 From: waleed Date: Tue, 7 Jul 2026 15:39:13 -0700 Subject: [PATCH 01/12] fix(workspaces): prevent deleting a workspace that strands other members The workspace DELETE route only checked the deleter's own workspace count before allowing deletion, not the workspace being deleted. An org admin with implicit admin access to every org workspace could delete another member's only workspace, leaving them with zero workspaces. archiveWorkspace now checks, inside the same transaction as the archive, whether any member of the workspace being deleted would be left with zero active workspaces, and blocks if so. The ban flow (disableUserResources) opts out via force: true since it must fully disable a banned user's workspaces regardless of other members. --- .../sim/app/api/workspaces/[id]/route.test.ts | 125 ++++++++ apps/sim/app/api/workspaces/[id]/route.ts | 42 +-- apps/sim/lib/workflows/lifecycle.test.ts | 55 +++- apps/sim/lib/workflows/lifecycle.ts | 4 +- apps/sim/lib/workspaces/lifecycle.test.ts | 194 ++++++++++++ apps/sim/lib/workspaces/lifecycle.ts | 286 +++++++++++------- 6 files changed, 564 insertions(+), 142 deletions(-) create mode 100644 apps/sim/app/api/workspaces/[id]/route.test.ts diff --git a/apps/sim/app/api/workspaces/[id]/route.test.ts b/apps/sim/app/api/workspaces/[id]/route.test.ts new file mode 100644 index 00000000000..b916ba3c19b --- /dev/null +++ b/apps/sim/app/api/workspaces/[id]/route.test.ts @@ -0,0 +1,125 @@ +/** + * @vitest-environment node + */ +import { + auditMock, + auditMockFns, + authMockFns, + createMockRequest, + permissionsMock, + permissionsMockFns, + posthogServerMock, + posthogServerMockFns, +} from '@sim/testing' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { mockArchiveWorkspace } = vi.hoisted(() => ({ + mockArchiveWorkspace: vi.fn(), +})) + +vi.mock('@/lib/workspaces/permissions/utils', () => permissionsMock) + +vi.mock('@/lib/workspaces/lifecycle', () => ({ + archiveWorkspace: mockArchiveWorkspace, +})) + +vi.mock('@sim/audit', () => auditMock) + +vi.mock('@/lib/posthog/server', () => posthogServerMock) + +import { DELETE } from '@/app/api/workspaces/[id]/route' + +function callDelete(workspaceId = 'workspace-1') { + const request = createMockRequest('DELETE', {}) + return DELETE(request, { params: Promise.resolve({ id: workspaceId }) }) +} + +describe('DELETE /api/workspaces/[id]', () => { + beforeEach(() => { + vi.clearAllMocks() + authMockFns.mockGetSession.mockResolvedValue({ + user: { id: 'user-admin', name: 'Admin', email: 'admin@example.com' }, + }) + permissionsMockFns.mockGetUserEntityPermissions.mockResolvedValue('admin') + }) + + it('returns 401 when the caller is unauthenticated', async () => { + authMockFns.mockGetSession.mockResolvedValue(null) + + const response = await callDelete() + + expect(response.status).toBe(401) + expect(mockArchiveWorkspace).not.toHaveBeenCalled() + }) + + it('returns 403 when the caller lacks admin permission', async () => { + permissionsMockFns.mockGetUserEntityPermissions.mockResolvedValue('write') + + const response = await callDelete() + + expect(response.status).toBe(403) + expect(mockArchiveWorkspace).not.toHaveBeenCalled() + }) + + it('returns 400 and does not record an audit entry when archival would strand a member', async () => { + mockArchiveWorkspace.mockResolvedValue({ + archived: false, + workspaceName: 'Victim Workspace', + strandedUserIds: ['user-victim'], + }) + + const response = await callDelete() + const body = await response.json() + + expect(response.status).toBe(400) + expect(body.error).toMatch(/no other workspace/i) + expect(auditMockFns.mockRecordAudit).not.toHaveBeenCalled() + expect(posthogServerMockFns.mockCaptureServerEvent).not.toHaveBeenCalled() + }) + + it('returns 404 when the workspace does not exist', async () => { + mockArchiveWorkspace.mockResolvedValue({ archived: false }) + + const response = await callDelete() + + expect(response.status).toBe(404) + expect(auditMockFns.mockRecordAudit).not.toHaveBeenCalled() + }) + + it('archives the workspace, records an audit entry, and captures the event on success', async () => { + mockArchiveWorkspace.mockResolvedValue({ + archived: true, + workspaceName: 'Test Workspace', + }) + + const response = await callDelete('workspace-1') + const body = await response.json() + + expect(response.status).toBe(200) + expect(body).toEqual({ success: true }) + expect(mockArchiveWorkspace).toHaveBeenCalledWith('workspace-1', { + requestId: 'workspace-workspace-1', + }) + expect(auditMockFns.mockRecordAudit).toHaveBeenCalledWith( + expect.objectContaining({ + workspaceId: 'workspace-1', + actorId: 'user-admin', + resourceName: 'Test Workspace', + }) + ) + expect(posthogServerMockFns.mockCaptureServerEvent).toHaveBeenCalledWith( + 'user-admin', + 'workspace_deleted', + expect.objectContaining({ workspace_id: 'workspace-1' }), + expect.objectContaining({ groups: { workspace: 'workspace-1' } }) + ) + }) + + it('returns 500 when archival throws unexpectedly', async () => { + mockArchiveWorkspace.mockRejectedValue(new Error('db exploded')) + + const response = await callDelete() + + expect(response.status).toBe(500) + }) +}) diff --git a/apps/sim/app/api/workspaces/[id]/route.ts b/apps/sim/app/api/workspaces/[id]/route.ts index 1b215792d94..1dc4ee0bf25 100644 --- a/apps/sim/app/api/workspaces/[id]/route.ts +++ b/apps/sim/app/api/workspaces/[id]/route.ts @@ -12,7 +12,7 @@ import { archiveWorkspace } from '@/lib/workspaces/lifecycle' const logger = createLogger('WorkspaceByIdAPI') import { db } from '@sim/db' -import { permissions, workspace } from '@sim/db/schema' +import { workspace } from '@sim/db/schema' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { getEffectiveWorkspacePermission, @@ -239,30 +239,6 @@ export const DELETE = withRouteHandler( } try { - const [[workspaceRecord], totalWorkspaces] = await Promise.all([ - db - .select({ name: workspace.name }) - .from(workspace) - .where(and(eq(workspace.id, workspaceId), isNull(workspace.archivedAt))) - .limit(1), - db - .select({ id: permissions.entityId }) - .from(permissions) - .innerJoin(workspace, eq(permissions.entityId, workspace.id)) - .where( - and( - eq(permissions.userId, session.user.id), - eq(permissions.entityType, 'workspace'), - isNull(workspace.archivedAt) - ) - ), - ]) - - /** Counts all workspace memberships (any role), not just admin — prevents the user from reaching a zero-workspace state. */ - if (totalWorkspaces.length <= 1) { - return NextResponse.json({ error: 'Cannot delete the only workspace' }, { status: 400 }) - } - logger.info(`Deleting workspace ${workspaceId} for user ${session.user.id}`) const workspaceWorkflows = await db @@ -276,7 +252,17 @@ export const DELETE = withRouteHandler( requestId: `workspace-${workspaceId}`, }) - if (!archiveResult.archived && !workspaceRecord) { + if (archiveResult.strandedUserIds?.length) { + return NextResponse.json( + { + error: + 'Cannot delete this workspace: one or more members have no other workspace to fall back to', + }, + { status: 400 } + ) + } + + if (!archiveResult.archived) { return NextResponse.json({ error: 'Workspace not found' }, { status: 404 }) } @@ -288,8 +274,8 @@ export const DELETE = withRouteHandler( action: AuditAction.WORKSPACE_DELETED, resourceType: AuditResourceType.WORKSPACE, resourceId: workspaceId, - resourceName: workspaceRecord?.name, - description: `Archived workspace "${workspaceRecord?.name || workspaceId}"`, + resourceName: archiveResult.workspaceName, + description: `Archived workspace "${archiveResult.workspaceName || workspaceId}"`, metadata: { affected: { workflows: workflowIds.length, diff --git a/apps/sim/lib/workflows/lifecycle.test.ts b/apps/sim/lib/workflows/lifecycle.test.ts index db4af82822e..dd5133bc182 100644 --- a/apps/sim/lib/workflows/lifecycle.test.ts +++ b/apps/sim/lib/workflows/lifecycle.test.ts @@ -10,14 +10,21 @@ import { } from '@sim/testing' import { beforeEach, describe, expect, it, vi } from 'vitest' -const { mockSelect, mockTransaction, mockCleanupExternalWebhook, mockWorkflowDeleted } = vi.hoisted( - () => ({ - mockSelect: vi.fn(), - mockTransaction: vi.fn(), - mockCleanupExternalWebhook: vi.fn(), - mockWorkflowDeleted: vi.fn(), - }) -) +const { + mockSelect, + mockTransaction, + mockDelete, + mockCleanupExternalWebhook, + mockWorkflowDeleted, + mockArchiveWorkspace, +} = vi.hoisted(() => ({ + mockSelect: vi.fn(), + mockTransaction: vi.fn(), + mockDelete: vi.fn(), + mockCleanupExternalWebhook: vi.fn(), + mockWorkflowDeleted: vi.fn(), + mockArchiveWorkspace: vi.fn(), +})) const mockGetWorkflowById = workflowsUtilsMockFns.mockGetWorkflowById @@ -25,9 +32,14 @@ vi.mock('@sim/db', () => ({ db: { select: mockSelect, transaction: mockTransaction, + delete: mockDelete, }, })) +vi.mock('@/lib/workspaces/lifecycle', () => ({ + archiveWorkspace: mockArchiveWorkspace, +})) + vi.mock('@/lib/workflows/utils', () => workflowsUtilsMock) vi.mock('@/lib/webhooks/provider-subscriptions', () => ({ @@ -46,7 +58,7 @@ vi.mock('@/lib/core/telemetry', () => ({ }, })) -import { archiveWorkflow } from '@/lib/workflows/lifecycle' +import { archiveWorkflow, disableUserResources } from '@/lib/workflows/lifecycle' function createSelectChain(result: T) { const chain = { @@ -129,3 +141,28 @@ describe('workflow lifecycle', () => { expect(fetch).not.toHaveBeenCalled() }) }) + +describe('disableUserResources', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('force-archives every owned workspace so banning is never blocked by other members', async () => { + mockSelect.mockReturnValue(createSelectChain([{ id: 'workspace-1' }, { id: 'workspace-2' }])) + mockDelete.mockReturnValue({ where: vi.fn().mockResolvedValue([]) }) + mockArchiveWorkspace.mockResolvedValue({ archived: true, workspaceName: 'Workspace' }) + + await disableUserResources('user-banned') + + expect(mockArchiveWorkspace).toHaveBeenCalledTimes(2) + expect(mockArchiveWorkspace).toHaveBeenCalledWith( + 'workspace-1', + expect.objectContaining({ force: true }) + ) + expect(mockArchiveWorkspace).toHaveBeenCalledWith( + 'workspace-2', + expect.objectContaining({ force: true }) + ) + expect(mockDelete).toHaveBeenCalled() + }) +}) diff --git a/apps/sim/lib/workflows/lifecycle.ts b/apps/sim/lib/workflows/lifecycle.ts index 82040e2750e..2fbc7457a0c 100644 --- a/apps/sim/lib/workflows/lifecycle.ts +++ b/apps/sim/lib/workflows/lifecycle.ts @@ -360,7 +360,9 @@ export async function disableUserResources(userId: string): Promise { .where(and(eq(workspace.ownerId, userId), isNull(workspace.archivedAt))) await Promise.all([ - ...ownedWorkspaces.map((w) => archiveWorkspace(w.id, { requestId })), + // force: true — a banned user's owned workspaces must be fully disabled regardless of + // whether other members would be left with zero workspaces. + ...ownedWorkspaces.map((w) => archiveWorkspace(w.id, { requestId, force: true })), db.delete(apiKey).where(eq(apiKey.userId, userId)), ]) diff --git a/apps/sim/lib/workspaces/lifecycle.test.ts b/apps/sim/lib/workspaces/lifecycle.test.ts index 1013b2280f4..9a24e8a96a3 100644 --- a/apps/sim/lib/workspaces/lifecycle.test.ts +++ b/apps/sim/lib/workspaces/lifecycle.test.ts @@ -35,6 +35,26 @@ function createUpdateChain() { } } +function createMembersChain(members: Array<{ userId: string }>) { + return { + from: vi.fn().mockReturnValue({ + where: vi.fn().mockResolvedValue(members), + }), + } +} + +function createWorkspaceCountsChain(counts: Array<{ userId: string; workspaceCount: number }>) { + return { + from: vi.fn().mockReturnValue({ + innerJoin: vi.fn().mockReturnValue({ + where: vi.fn().mockReturnValue({ + groupBy: vi.fn().mockResolvedValue(counts), + }), + }), + }), + } +} + describe('workspace lifecycle', () => { beforeEach(() => { vi.clearAllMocks() @@ -55,6 +75,7 @@ describe('workspace lifecycle', () => { }) const tx = { + selectDistinct: vi.fn().mockReturnValue(createMembersChain([])), select: vi.fn().mockReturnValue({ from: vi.fn().mockReturnValue({ where: vi.fn().mockResolvedValue([{ id: 'kb-1' }]), @@ -82,6 +103,179 @@ describe('workspace lifecycle', () => { expect(tx.delete).toHaveBeenCalledTimes(1) }) + it('blocks archival when a member would be left with zero workspaces', async () => { + mockGetWorkspaceWithOwner.mockResolvedValue({ + id: 'workspace-1', + name: 'Workspace 1', + ownerId: 'user-1', + archivedAt: null, + }) + mockSelect.mockReturnValue({ + from: vi.fn().mockReturnValue({ + where: vi.fn().mockResolvedValue([{ id: 'server-1' }]), + }), + }) + + const tx = { + selectDistinct: vi.fn().mockReturnValue(createMembersChain([{ userId: 'user-victim' }])), + select: vi + .fn() + .mockReturnValue( + createWorkspaceCountsChain([{ userId: 'user-victim', workspaceCount: 1 }]) + ), + update: vi.fn().mockImplementation(() => createUpdateChain()), + delete: vi.fn().mockImplementation(() => ({ + where: vi.fn().mockResolvedValue([]), + })), + } + mockTransaction.mockImplementation(async (callback: (trx: typeof tx) => Promise) => + callback(tx) + ) + + const result = await archiveWorkspace('workspace-1', { requestId: 'req-1' }) + + expect(result).toEqual({ + archived: false, + workspaceName: 'Workspace 1', + strandedUserIds: ['user-victim'], + }) + expect(tx.update).not.toHaveBeenCalled() + expect(tx.delete).not.toHaveBeenCalled() + }) + + it('blocks archival when only one of several members would be stranded', async () => { + mockGetWorkspaceWithOwner.mockResolvedValue({ + id: 'workspace-1', + name: 'Workspace 1', + ownerId: 'user-1', + archivedAt: null, + }) + mockSelect.mockReturnValue({ + from: vi.fn().mockReturnValue({ + where: vi.fn().mockResolvedValue([]), + }), + }) + + const tx = { + selectDistinct: vi + .fn() + .mockReturnValue(createMembersChain([{ userId: 'user-victim' }, { userId: 'user-safe' }])), + select: vi.fn().mockReturnValue( + createWorkspaceCountsChain([ + { userId: 'user-victim', workspaceCount: 1 }, + { userId: 'user-safe', workspaceCount: 3 }, + ]) + ), + update: vi.fn().mockImplementation(() => createUpdateChain()), + delete: vi.fn().mockImplementation(() => ({ + where: vi.fn().mockResolvedValue([]), + })), + } + mockTransaction.mockImplementation(async (callback: (trx: typeof tx) => Promise) => + callback(tx) + ) + + const result = await archiveWorkspace('workspace-1', { requestId: 'req-1' }) + + expect(result).toEqual({ + archived: false, + workspaceName: 'Workspace 1', + strandedUserIds: ['user-victim'], + }) + expect(tx.update).not.toHaveBeenCalled() + }) + + it('proceeds when every member has another active workspace', async () => { + mockGetWorkspaceWithOwner.mockResolvedValue({ + id: 'workspace-1', + name: 'Workspace 1', + ownerId: 'user-1', + archivedAt: null, + }) + mockArchiveWorkflowsForWorkspace.mockResolvedValue(0) + mockSelect.mockReturnValue({ + from: vi.fn().mockReturnValue({ + where: vi.fn().mockResolvedValue([]), + }), + }) + + const tx = { + selectDistinct: vi + .fn() + .mockReturnValue( + createMembersChain([{ userId: 'user-safe-1' }, { userId: 'user-safe-2' }]) + ), + select: vi + .fn() + .mockReturnValueOnce( + createWorkspaceCountsChain([ + { userId: 'user-safe-1', workspaceCount: 2 }, + { userId: 'user-safe-2', workspaceCount: 4 }, + ]) + ) + .mockReturnValue({ + from: vi.fn().mockReturnValue({ + where: vi.fn().mockResolvedValue([]), + }), + }), + update: vi.fn().mockImplementation(() => createUpdateChain()), + delete: vi.fn().mockImplementation(() => ({ + where: vi.fn().mockResolvedValue([]), + })), + } + mockTransaction.mockImplementation(async (callback: (trx: typeof tx) => Promise) => + callback(tx) + ) + + const result = await archiveWorkspace('workspace-1', { requestId: 'req-1' }) + + expect(result).toEqual({ + archived: true, + workspaceName: 'Workspace 1', + }) + // No knowledge bases found, so the two KB-dependent updates (document, knowledgeConnector) are skipped. + expect(tx.update).toHaveBeenCalledTimes(8) + }) + + it('skips the stranded-member check when force is set', async () => { + mockGetWorkspaceWithOwner.mockResolvedValue({ + id: 'workspace-1', + name: 'Workspace 1', + ownerId: 'user-1', + archivedAt: null, + }) + mockArchiveWorkflowsForWorkspace.mockResolvedValue(0) + mockSelect.mockReturnValue({ + from: vi.fn().mockReturnValue({ + where: vi.fn().mockResolvedValue([]), + }), + }) + + const tx = { + selectDistinct: vi.fn().mockReturnValue(createMembersChain([{ userId: 'user-victim' }])), + select: vi.fn().mockReturnValue({ + from: vi.fn().mockReturnValue({ + where: vi.fn().mockResolvedValue([]), + }), + }), + update: vi.fn().mockImplementation(() => createUpdateChain()), + delete: vi.fn().mockImplementation(() => ({ + where: vi.fn().mockResolvedValue([]), + })), + } + mockTransaction.mockImplementation(async (callback: (trx: typeof tx) => Promise) => + callback(tx) + ) + + const result = await archiveWorkspace('workspace-1', { requestId: 'req-1', force: true }) + + expect(result).toEqual({ + archived: true, + workspaceName: 'Workspace 1', + }) + expect(tx.selectDistinct).not.toHaveBeenCalled() + }) + it('is idempotent for already archived workspaces', async () => { mockGetWorkspaceWithOwner.mockResolvedValue({ id: 'workspace-1', diff --git a/apps/sim/lib/workspaces/lifecycle.ts b/apps/sim/lib/workspaces/lifecycle.ts index 59162b42682..284b5133849 100644 --- a/apps/sim/lib/workspaces/lifecycle.ts +++ b/apps/sim/lib/workspaces/lifecycle.ts @@ -7,6 +7,7 @@ import { knowledgeBase, knowledgeConnector, mcpServers, + permissions, userTableDefinitions, workflowMcpServer, workflowSchedule, @@ -14,6 +15,7 @@ import { workspaceFiles, } from '@sim/db/schema' import { createLogger } from '@sim/logger' +import type { DbOrTx } from '@sim/workflow-persistence/types' import { and, eq, inArray, isNull, sql } from 'drizzle-orm' import { mcpPubSub } from '@/lib/mcp/pubsub' import { mcpService } from '@/lib/mcp/service' @@ -24,12 +26,70 @@ const logger = createLogger('WorkspaceLifecycle') interface ArchiveWorkspaceOptions { requestId: string + /** + * Skips the "would strand a member" safety check. Only for account-disable flows where every + * workspace owned by the disabled user must be archived regardless of member workspace counts. + */ + force?: boolean +} + +interface ArchiveWorkspaceResult { + archived: boolean + workspaceName?: string + /** Present only when archival was blocked because it would leave these members with zero workspaces. */ + strandedUserIds?: string[] +} + +class WorkspaceArchiveBlockedError extends Error { + constructor(readonly strandedUserIds: string[]) { + super('Archiving this workspace would leave one or more members with no workspace') + this.name = 'WorkspaceArchiveBlockedError' + } +} + +/** + * Returns the userIds of workspace members for whom `workspaceId` is their only active + * (non-archived) workspace. Must be called against the same executor used to perform the + * archival so the check and the write are atomic. + */ +async function findMembersStrandedByArchival( + executor: DbOrTx, + workspaceId: string +): Promise { + const members = await executor + .selectDistinct({ userId: permissions.userId }) + .from(permissions) + .where(and(eq(permissions.entityId, workspaceId), eq(permissions.entityType, 'workspace'))) + + if (members.length === 0) { + return [] + } + + const memberIds = members.map((member) => member.userId) + + const workspaceCounts = await executor + .select({ + userId: permissions.userId, + workspaceCount: sql`count(distinct ${workspace.id})`, + }) + .from(permissions) + .innerJoin(workspace, eq(permissions.entityId, workspace.id)) + .where( + and( + inArray(permissions.userId, memberIds), + eq(permissions.entityType, 'workspace'), + isNull(workspace.archivedAt) + ) + ) + .groupBy(permissions.userId) + + return workspaceCounts.filter((row) => Number(row.workspaceCount) <= 1).map((row) => row.userId) } export async function archiveWorkspace( workspaceId: string, options: ArchiveWorkspaceOptions -): Promise<{ archived: boolean; workspaceName?: string }> { +): Promise { const workspaceRecord = await getWorkspaceWithOwner(workspaceId, { includeArchived: true }) if (!workspaceRecord) { @@ -47,129 +107,147 @@ export async function archiveWorkspace( .from(workflowMcpServer) .where(eq(workflowMcpServer.workspaceId, workspaceId)) - await db.transaction(async (tx) => { - await tx - .update(knowledgeBase) - .set({ - deletedAt: now, - updatedAt: now, - }) - .where(and(eq(knowledgeBase.workspaceId, workspaceId), isNull(knowledgeBase.deletedAt))) - - const workspaceKbIds = await tx - .select({ id: knowledgeBase.id }) - .from(knowledgeBase) - .where(eq(knowledgeBase.workspaceId, workspaceId)) + try { + await db.transaction(async (tx) => { + if (!options.force) { + const strandedUserIds = await findMembersStrandedByArchival(tx, workspaceId) + if (strandedUserIds.length > 0) { + throw new WorkspaceArchiveBlockedError(strandedUserIds) + } + } - const knowledgeBaseIds = workspaceKbIds.map((entry) => entry.id) - if (knowledgeBaseIds.length > 0) { await tx - .update(document) - .set({ archivedAt: now }) - .where( - and( - inArray(document.knowledgeBaseId, knowledgeBaseIds), - isNull(document.archivedAt), - isNull(document.deletedAt) + .update(knowledgeBase) + .set({ + deletedAt: now, + updatedAt: now, + }) + .where(and(eq(knowledgeBase.workspaceId, workspaceId), isNull(knowledgeBase.deletedAt))) + + const workspaceKbIds = await tx + .select({ id: knowledgeBase.id }) + .from(knowledgeBase) + .where(eq(knowledgeBase.workspaceId, workspaceId)) + + const knowledgeBaseIds = workspaceKbIds.map((entry) => entry.id) + if (knowledgeBaseIds.length > 0) { + await tx + .update(document) + .set({ archivedAt: now }) + .where( + and( + inArray(document.knowledgeBaseId, knowledgeBaseIds), + isNull(document.archivedAt), + isNull(document.deletedAt) + ) ) - ) + + await tx + .update(knowledgeConnector) + .set({ archivedAt: now, status: 'paused', updatedAt: now }) + .where( + and( + inArray(knowledgeConnector.knowledgeBaseId, knowledgeBaseIds), + isNull(knowledgeConnector.archivedAt), + isNull(knowledgeConnector.deletedAt) + ) + ) + } await tx - .update(knowledgeConnector) - .set({ archivedAt: now, status: 'paused', updatedAt: now }) + .update(userTableDefinitions) + .set({ + archivedAt: now, + updatedAt: now, + }) .where( and( - inArray(knowledgeConnector.knowledgeBaseId, knowledgeBaseIds), - isNull(knowledgeConnector.archivedAt), - isNull(knowledgeConnector.deletedAt) + eq(userTableDefinitions.workspaceId, workspaceId), + isNull(userTableDefinitions.archivedAt) ) ) - } - - await tx - .update(userTableDefinitions) - .set({ - archivedAt: now, - updatedAt: now, - }) - .where( - and( - eq(userTableDefinitions.workspaceId, workspaceId), - isNull(userTableDefinitions.archivedAt) - ) - ) - await tx - .update(workspaceFiles) - .set({ - deletedAt: now, - }) - .where(and(eq(workspaceFiles.workspaceId, workspaceId), isNull(workspaceFiles.deletedAt))) + await tx + .update(workspaceFiles) + .set({ + deletedAt: now, + }) + .where(and(eq(workspaceFiles.workspaceId, workspaceId), isNull(workspaceFiles.deletedAt))) - await tx - .update(invitation) - .set({ - status: 'cancelled', - updatedAt: now, - }) - .where( - and( - eq(invitation.status, 'pending'), - sql`${invitation.id} IN ( + await tx + .update(invitation) + .set({ + status: 'cancelled', + updatedAt: now, + }) + .where( + and( + eq(invitation.status, 'pending'), + sql`${invitation.id} IN ( SELECT ${invitationWorkspaceGrant.invitationId} FROM ${invitationWorkspaceGrant} WHERE ${invitationWorkspaceGrant.workspaceId} = ${workspaceId} )` + ) ) - ) - await tx - .delete(apiKey) - .where(and(eq(apiKey.workspaceId, workspaceId), eq(apiKey.type, 'workspace'))) + await tx + .delete(apiKey) + .where(and(eq(apiKey.workspaceId, workspaceId), eq(apiKey.type, 'workspace'))) - await tx - .update(workflowMcpServer) - .set({ - deletedAt: now, - isPublic: false, - updatedAt: now, - }) - .where(eq(workflowMcpServer.workspaceId, workspaceId)) - - await tx - .update(mcpServers) - .set({ - deletedAt: now, - enabled: false, - updatedAt: now, - }) - .where(and(eq(mcpServers.workspaceId, workspaceId), isNull(mcpServers.deletedAt))) - - await tx - .update(workflowSchedule) - .set({ - archivedAt: now, - updatedAt: now, - status: 'disabled', - nextRunAt: null, - lastQueuedAt: null, - }) - .where( - and( - eq(workflowSchedule.sourceWorkspaceId, workspaceId), - eq(workflowSchedule.sourceType, 'job'), - isNull(workflowSchedule.archivedAt) + await tx + .update(workflowMcpServer) + .set({ + deletedAt: now, + isPublic: false, + updatedAt: now, + }) + .where(eq(workflowMcpServer.workspaceId, workspaceId)) + + await tx + .update(mcpServers) + .set({ + deletedAt: now, + enabled: false, + updatedAt: now, + }) + .where(and(eq(mcpServers.workspaceId, workspaceId), isNull(mcpServers.deletedAt))) + + await tx + .update(workflowSchedule) + .set({ + archivedAt: now, + updatedAt: now, + status: 'disabled', + nextRunAt: null, + lastQueuedAt: null, + }) + .where( + and( + eq(workflowSchedule.sourceWorkspaceId, workspaceId), + eq(workflowSchedule.sourceType, 'job'), + isNull(workflowSchedule.archivedAt) + ) ) - ) - await tx - .update(workspace) - .set({ - archivedAt: now, - updatedAt: now, - }) - .where(and(eq(workspace.id, workspaceId), isNull(workspace.archivedAt))) - }) + await tx + .update(workspace) + .set({ + archivedAt: now, + updatedAt: now, + }) + .where(and(eq(workspace.id, workspaceId), isNull(workspace.archivedAt))) + }) + } catch (error) { + if (error instanceof WorkspaceArchiveBlockedError) { + return { + archived: false, + workspaceName: workspaceRecord.name, + strandedUserIds: error.strandedUserIds, + } + } + throw error + } await archiveWorkflowsForWorkspace(workspaceId, options) From a56f2fe8cef99aa1c80adeab0ad7cbdc0beb41f3 Mon Sep 17 00:00:00 2001 From: waleed Date: Tue, 7 Jul 2026 16:08:12 -0700 Subject: [PATCH 02/12] fix(workspaces): fix false-positive stranding and TOCTOU race in delete guard Address two review findings on the stranded-member check: - The check only counted a member's explicit permission rows, so an org admin whose only explicit row was on the workspace being deleted could be wrongly reported as stranded even though they still have access to every other workspace in their org. Now reuses listAccessibleWorkspaceRowsForUser (explicit rows + org-admin-derived access) instead of a raw permission count, parameterized with an executor so it can run against the same tx. - Two concurrent deletions of different workspaces shared by the same sole member could each read a pre-deletion count, both conclude the member isn't stranded, and together leave them with zero workspaces. The archive transaction now runs under serializable isolation (skipped when force is set, since that path never runs the check) so Postgres detects the write skew and aborts one transaction instead. --- apps/sim/lib/workspaces/lifecycle.test.ts | 195 ++++++++++------------ apps/sim/lib/workspaces/lifecycle.ts | 53 +++--- apps/sim/lib/workspaces/utils.ts | 20 ++- 3 files changed, 129 insertions(+), 139 deletions(-) diff --git a/apps/sim/lib/workspaces/lifecycle.test.ts b/apps/sim/lib/workspaces/lifecycle.test.ts index 9a24e8a96a3..780c20e9665 100644 --- a/apps/sim/lib/workspaces/lifecycle.test.ts +++ b/apps/sim/lib/workspaces/lifecycle.test.ts @@ -4,10 +4,16 @@ import { permissionsMock, permissionsMockFns } from '@sim/testing' import { beforeEach, describe, expect, it, vi } from 'vitest' -const { mockSelect, mockTransaction, mockArchiveWorkflowsForWorkspace } = vi.hoisted(() => ({ +const { + mockSelect, + mockTransaction, + mockArchiveWorkflowsForWorkspace, + mockListAccessibleWorkspaceRowsForUser, +} = vi.hoisted(() => ({ mockSelect: vi.fn(), mockTransaction: vi.fn(), mockArchiveWorkflowsForWorkspace: vi.fn(), + mockListAccessibleWorkspaceRowsForUser: vi.fn(), })) const mockGetWorkspaceWithOwner = permissionsMockFns.mockGetWorkspaceWithOwner @@ -25,6 +31,10 @@ vi.mock('@/lib/workflows/lifecycle', () => ({ vi.mock('@/lib/workspaces/permissions/utils', () => permissionsMock) +vi.mock('@/lib/workspaces/utils', () => ({ + listAccessibleWorkspaceRowsForUser: mockListAccessibleWorkspaceRowsForUser, +})) + import { archiveWorkspace } from './lifecycle' function createUpdateChain() { @@ -43,24 +53,36 @@ function createMembersChain(members: Array<{ userId: string }>) { } } -function createWorkspaceCountsChain(counts: Array<{ userId: string; workspaceCount: number }>) { +function accessibleWorkspaceRow(workspaceId: string) { + return { workspace: { id: workspaceId }, permissionType: 'admin' as const } +} + +function createTx(members: Array<{ userId: string }>) { return { - from: vi.fn().mockReturnValue({ - innerJoin: vi.fn().mockReturnValue({ - where: vi.fn().mockReturnValue({ - groupBy: vi.fn().mockResolvedValue(counts), - }), + selectDistinct: vi.fn().mockReturnValue(createMembersChain(members)), + select: vi.fn().mockReturnValue({ + from: vi.fn().mockReturnValue({ + where: vi.fn().mockResolvedValue([]), }), }), + update: vi.fn().mockImplementation(() => createUpdateChain()), + delete: vi.fn().mockImplementation(() => ({ + where: vi.fn().mockResolvedValue([]), + })), } } describe('workspace lifecycle', () => { beforeEach(() => { vi.clearAllMocks() + mockSelect.mockReturnValue({ + from: vi.fn().mockReturnValue({ + where: vi.fn().mockResolvedValue([]), + }), + }) }) - it('archives workspace and dependent resources', async () => { + it('archives workspace and dependent resources under serializable isolation', async () => { mockGetWorkspaceWithOwner.mockResolvedValue({ id: 'workspace-1', name: 'Workspace 1', @@ -68,24 +90,8 @@ describe('workspace lifecycle', () => { archivedAt: null, }) mockArchiveWorkflowsForWorkspace.mockResolvedValue(2) - mockSelect.mockReturnValue({ - from: vi.fn().mockReturnValue({ - where: vi.fn().mockResolvedValue([{ id: 'server-1' }]), - }), - }) - const tx = { - selectDistinct: vi.fn().mockReturnValue(createMembersChain([])), - select: vi.fn().mockReturnValue({ - from: vi.fn().mockReturnValue({ - where: vi.fn().mockResolvedValue([{ id: 'kb-1' }]), - }), - }), - update: vi.fn().mockImplementation(() => createUpdateChain()), - delete: vi.fn().mockImplementation(() => ({ - where: vi.fn().mockResolvedValue([]), - })), - } + const tx = createTx([]) mockTransaction.mockImplementation(async (callback: (trx: typeof tx) => Promise) => callback(tx) ) @@ -99,8 +105,12 @@ describe('workspace lifecycle', () => { expect(mockArchiveWorkflowsForWorkspace).toHaveBeenCalledWith('workspace-1', { requestId: 'req-1', }) - expect(tx.update).toHaveBeenCalledTimes(10) + expect(tx.update).toHaveBeenCalledTimes(8) expect(tx.delete).toHaveBeenCalledTimes(1) + expect(mockListAccessibleWorkspaceRowsForUser).not.toHaveBeenCalled() + expect(mockTransaction).toHaveBeenCalledWith(expect.any(Function), { + isolationLevel: 'serializable', + }) }) it('blocks archival when a member would be left with zero workspaces', async () => { @@ -110,24 +120,11 @@ describe('workspace lifecycle', () => { ownerId: 'user-1', archivedAt: null, }) - mockSelect.mockReturnValue({ - from: vi.fn().mockReturnValue({ - where: vi.fn().mockResolvedValue([{ id: 'server-1' }]), - }), - }) + mockListAccessibleWorkspaceRowsForUser.mockResolvedValue([ + accessibleWorkspaceRow('workspace-1'), + ]) - const tx = { - selectDistinct: vi.fn().mockReturnValue(createMembersChain([{ userId: 'user-victim' }])), - select: vi - .fn() - .mockReturnValue( - createWorkspaceCountsChain([{ userId: 'user-victim', workspaceCount: 1 }]) - ), - update: vi.fn().mockImplementation(() => createUpdateChain()), - delete: vi.fn().mockImplementation(() => ({ - where: vi.fn().mockResolvedValue([]), - })), - } + const tx = createTx([{ userId: 'user-victim' }]) mockTransaction.mockImplementation(async (callback: (trx: typeof tx) => Promise) => callback(tx) ) @@ -139,6 +136,7 @@ describe('workspace lifecycle', () => { workspaceName: 'Workspace 1', strandedUserIds: ['user-victim'], }) + expect(mockListAccessibleWorkspaceRowsForUser).toHaveBeenCalledWith('user-victim', 'active', tx) expect(tx.update).not.toHaveBeenCalled() expect(tx.delete).not.toHaveBeenCalled() }) @@ -150,27 +148,13 @@ describe('workspace lifecycle', () => { ownerId: 'user-1', archivedAt: null, }) - mockSelect.mockReturnValue({ - from: vi.fn().mockReturnValue({ - where: vi.fn().mockResolvedValue([]), - }), - }) + mockListAccessibleWorkspaceRowsForUser.mockImplementation(async (userId: string) => + userId === 'user-victim' + ? [accessibleWorkspaceRow('workspace-1')] + : [accessibleWorkspaceRow('workspace-1'), accessibleWorkspaceRow('workspace-2')] + ) - const tx = { - selectDistinct: vi - .fn() - .mockReturnValue(createMembersChain([{ userId: 'user-victim' }, { userId: 'user-safe' }])), - select: vi.fn().mockReturnValue( - createWorkspaceCountsChain([ - { userId: 'user-victim', workspaceCount: 1 }, - { userId: 'user-safe', workspaceCount: 3 }, - ]) - ), - update: vi.fn().mockImplementation(() => createUpdateChain()), - delete: vi.fn().mockImplementation(() => ({ - where: vi.fn().mockResolvedValue([]), - })), - } + const tx = createTx([{ userId: 'user-victim' }, { userId: 'user-safe' }]) mockTransaction.mockImplementation(async (callback: (trx: typeof tx) => Promise) => callback(tx) ) @@ -185,7 +169,7 @@ describe('workspace lifecycle', () => { expect(tx.update).not.toHaveBeenCalled() }) - it('proceeds when every member has another active workspace', async () => { + it('does not strand an org admin who has no explicit permission row on another workspace', async () => { mockGetWorkspaceWithOwner.mockResolvedValue({ id: 'workspace-1', name: 'Workspace 1', @@ -193,36 +177,41 @@ describe('workspace lifecycle', () => { archivedAt: null, }) mockArchiveWorkflowsForWorkspace.mockResolvedValue(0) - mockSelect.mockReturnValue({ - from: vi.fn().mockReturnValue({ - where: vi.fn().mockResolvedValue([]), - }), + // The org admin's only *explicit* permission row is on workspace-1, but they still have + // access to workspace-2 purely through their organization admin role. + mockListAccessibleWorkspaceRowsForUser.mockResolvedValue([ + accessibleWorkspaceRow('workspace-1'), + accessibleWorkspaceRow('workspace-2'), + ]) + + const tx = createTx([{ userId: 'user-org-admin' }]) + mockTransaction.mockImplementation(async (callback: (trx: typeof tx) => Promise) => + callback(tx) + ) + + const result = await archiveWorkspace('workspace-1', { requestId: 'req-1' }) + + expect(result).toEqual({ + archived: true, + workspaceName: 'Workspace 1', }) + expect(tx.update).toHaveBeenCalledTimes(8) + }) - const tx = { - selectDistinct: vi - .fn() - .mockReturnValue( - createMembersChain([{ userId: 'user-safe-1' }, { userId: 'user-safe-2' }]) - ), - select: vi - .fn() - .mockReturnValueOnce( - createWorkspaceCountsChain([ - { userId: 'user-safe-1', workspaceCount: 2 }, - { userId: 'user-safe-2', workspaceCount: 4 }, - ]) - ) - .mockReturnValue({ - from: vi.fn().mockReturnValue({ - where: vi.fn().mockResolvedValue([]), - }), - }), - update: vi.fn().mockImplementation(() => createUpdateChain()), - delete: vi.fn().mockImplementation(() => ({ - where: vi.fn().mockResolvedValue([]), - })), - } + it('proceeds when every member has another active workspace', async () => { + mockGetWorkspaceWithOwner.mockResolvedValue({ + id: 'workspace-1', + name: 'Workspace 1', + ownerId: 'user-1', + archivedAt: null, + }) + mockArchiveWorkflowsForWorkspace.mockResolvedValue(0) + mockListAccessibleWorkspaceRowsForUser.mockResolvedValue([ + accessibleWorkspaceRow('workspace-1'), + accessibleWorkspaceRow('workspace-2'), + ]) + + const tx = createTx([{ userId: 'user-safe-1' }, { userId: 'user-safe-2' }]) mockTransaction.mockImplementation(async (callback: (trx: typeof tx) => Promise) => callback(tx) ) @@ -237,7 +226,7 @@ describe('workspace lifecycle', () => { expect(tx.update).toHaveBeenCalledTimes(8) }) - it('skips the stranded-member check when force is set', async () => { + it('skips the stranded-member check and serializable isolation when force is set', async () => { mockGetWorkspaceWithOwner.mockResolvedValue({ id: 'workspace-1', name: 'Workspace 1', @@ -245,24 +234,8 @@ describe('workspace lifecycle', () => { archivedAt: null, }) mockArchiveWorkflowsForWorkspace.mockResolvedValue(0) - mockSelect.mockReturnValue({ - from: vi.fn().mockReturnValue({ - where: vi.fn().mockResolvedValue([]), - }), - }) - const tx = { - selectDistinct: vi.fn().mockReturnValue(createMembersChain([{ userId: 'user-victim' }])), - select: vi.fn().mockReturnValue({ - from: vi.fn().mockReturnValue({ - where: vi.fn().mockResolvedValue([]), - }), - }), - update: vi.fn().mockImplementation(() => createUpdateChain()), - delete: vi.fn().mockImplementation(() => ({ - where: vi.fn().mockResolvedValue([]), - })), - } + const tx = createTx([{ userId: 'user-victim' }]) mockTransaction.mockImplementation(async (callback: (trx: typeof tx) => Promise) => callback(tx) ) @@ -274,6 +247,8 @@ describe('workspace lifecycle', () => { workspaceName: 'Workspace 1', }) expect(tx.selectDistinct).not.toHaveBeenCalled() + expect(mockListAccessibleWorkspaceRowsForUser).not.toHaveBeenCalled() + expect(mockTransaction).toHaveBeenCalledWith(expect.any(Function), undefined) }) it('is idempotent for already archived workspaces', async () => { diff --git a/apps/sim/lib/workspaces/lifecycle.ts b/apps/sim/lib/workspaces/lifecycle.ts index 284b5133849..8270a7921d9 100644 --- a/apps/sim/lib/workspaces/lifecycle.ts +++ b/apps/sim/lib/workspaces/lifecycle.ts @@ -15,12 +15,13 @@ import { workspaceFiles, } from '@sim/db/schema' import { createLogger } from '@sim/logger' -import type { DbOrTx } from '@sim/workflow-persistence/types' import { and, eq, inArray, isNull, sql } from 'drizzle-orm' +import type { DbOrTx } from '@/lib/db/types' import { mcpPubSub } from '@/lib/mcp/pubsub' import { mcpService } from '@/lib/mcp/service' import { archiveWorkflowsForWorkspace } from '@/lib/workflows/lifecycle' import { getWorkspaceWithOwner } from '@/lib/workspaces/permissions/utils' +import { listAccessibleWorkspaceRowsForUser } from '@/lib/workspaces/utils' const logger = createLogger('WorkspaceLifecycle') @@ -48,9 +49,15 @@ class WorkspaceArchiveBlockedError extends Error { } /** - * Returns the userIds of workspace members for whom `workspaceId` is their only active - * (non-archived) workspace. Must be called against the same executor used to perform the - * archival so the check and the write are atomic. + * Returns the userIds of explicit workspace members for whom `workspaceId` is their only + * accessible active (non-archived) workspace. "Accessible" includes workspaces granted through + * an explicit permission row AND workspaces derived from organization owner/admin role — an org + * admin can always fall back to the rest of the organization's workspaces even without an + * explicit permission row on any of them, so they are never stranded by this deletion. + * + * Must be called against the same executor used to perform the archival, under + * `serializable` isolation, so the check and the write are atomic with respect to a concurrent + * deletion of another workspace shared by the same member. */ async function findMembersStrandedByArchival( executor: DbOrTx, @@ -65,25 +72,16 @@ async function findMembersStrandedByArchival( return [] } - const memberIds = members.map((member) => member.userId) - - const workspaceCounts = await executor - .select({ - userId: permissions.userId, - workspaceCount: sql`count(distinct ${workspace.id})`, - }) - .from(permissions) - .innerJoin(workspace, eq(permissions.entityId, workspace.id)) - .where( - and( - inArray(permissions.userId, memberIds), - eq(permissions.entityType, 'workspace'), - isNull(workspace.archivedAt) - ) - ) - .groupBy(permissions.userId) + const strandedUserIds: string[] = [] + for (const { userId } of members) { + const accessible = await listAccessibleWorkspaceRowsForUser(userId, 'active', executor) + const hasOtherWorkspace = accessible.some((row) => row.workspace.id !== workspaceId) + if (!hasOtherWorkspace) { + strandedUserIds.push(userId) + } + } - return workspaceCounts.filter((row) => Number(row.workspaceCount) <= 1).map((row) => row.userId) + return strandedUserIds } export async function archiveWorkspace( @@ -107,6 +105,15 @@ export async function archiveWorkspace( .from(workflowMcpServer) .where(eq(workflowMcpServer.workspaceId, workspaceId)) + // serializable: without it, two concurrent deletions of different workspaces shared by the + // same sole member could each read a pre-deletion workspace count, both conclude the member + // isn't stranded, and together leave them with zero workspaces. Postgres detects this write + // skew under serializable isolation and aborts one transaction. Skipped when force is set, + // since that path never runs the stranded check in the first place. + const transactionConfig = options.force + ? undefined + : ({ isolationLevel: 'serializable' } as const) + try { await db.transaction(async (tx) => { if (!options.force) { @@ -237,7 +244,7 @@ export async function archiveWorkspace( updatedAt: now, }) .where(and(eq(workspace.id, workspaceId), isNull(workspace.archivedAt))) - }) + }, transactionConfig) } catch (error) { if (error instanceof WorkspaceArchiveBlockedError) { return { diff --git a/apps/sim/lib/workspaces/utils.ts b/apps/sim/lib/workspaces/utils.ts index 4c65d5e5f35..e448f54d69e 100644 --- a/apps/sim/lib/workspaces/utils.ts +++ b/apps/sim/lib/workspaces/utils.ts @@ -51,12 +51,16 @@ export async function getWorkspaceBilledAccountUserId(workspaceId: string): Prom * Workspaces the user administers purely through organization owner/admin role, * with no explicit permission row required. Empty when the user is not an org * owner/admin. Implements the workspace-permission inheritance model. + * + * Accepts an optional executor so callers already inside a transaction (e.g. a + * workspace-archival safety check) can run this against `tx` instead of `db`. */ export async function getOrgAdminWorkspaceRows( userId: string, - scope: WorkspaceScope = 'active' + scope: WorkspaceScope = 'active', + executor: DbOrTx = db ): Promise> { - const [membership] = await db + const [membership] = await executor .select({ organizationId: member.organizationId, role: member.role }) .from(member) .where(eq(member.userId, userId)) @@ -74,20 +78,24 @@ export async function getOrgAdminWorkspaceRows( ? and(orgFilter, sql`${workspaceTable.archivedAt} IS NOT NULL`) : and(orgFilter, isNull(workspaceTable.archivedAt)) - return db.select().from(workspaceTable).where(where).orderBy(desc(workspaceTable.createdAt)) + return executor.select().from(workspaceTable).where(where).orderBy(desc(workspaceTable.createdAt)) } /** * Every workspace a user can access: explicit permission grants plus workspaces * derived from organization owner/admin role. Deduped with explicit rows first. + * + * Accepts an optional executor so callers already inside a transaction can run + * this against `tx` instead of `db`. */ export async function listAccessibleWorkspaceRowsForUser( userId: string, - scope: WorkspaceScope = 'active' + scope: WorkspaceScope = 'active', + executor: DbOrTx = db ): Promise< Array<{ workspace: typeof workspaceTable.$inferSelect; permissionType: PermissionType }> > { - const explicit = await db + const explicit = await executor .select({ workspace: workspaceTable, permissionType: permissions.permissionType }) .from(permissions) .innerJoin(workspaceTable, eq(permissions.entityId, workspaceTable.id)) @@ -108,7 +116,7 @@ export async function listAccessibleWorkspaceRowsForUser( ) .orderBy(desc(workspaceTable.createdAt)) - const orgRows = await getOrgAdminWorkspaceRows(userId, scope) + const orgRows = await getOrgAdminWorkspaceRows(userId, scope, executor) if (orgRows.length === 0) { return explicit } From fcc7a65e11a5838537de8bebf662234ef98629a4 Mon Sep 17 00:00:00 2001 From: waleed Date: Tue, 7 Jul 2026 17:22:53 -0700 Subject: [PATCH 03/12] fix(workspaces): auto-provision a replacement workspace instead of blocking deletion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Blocking deletion when it would strand a member was the wrong UX: an org admin doing legitimate cleanup would be stopped because some unrelated, possibly-inactive member happened to have this as their only workspace. archiveWorkspace now always allows the deletion and instead auto-provisions a personal fallback workspace, atomically in the same transaction, for any member who would otherwise be left with zero active workspaces. This guarantees the invariant holds regardless of which route/link/client the affected member's next request comes through, unlike the pre-existing client-side "no workspaces" recovery which only covers the generic /workspace redirect page. Extracted the workspace-creation write (insert workspace + owner permission row + default workflow) out of the POST /api/workspaces route into a shared lib/workspaces/create.ts so both the route and the archival safety net use the same logic, parameterized by an optional executor so it can run inside an existing transaction. The ban flow (disableUserResources) keeps force: true, now meaning "skip stranded-member handling entirely" — a banned user's owned workspaces must still be fully disabled, and the banned user specifically shouldn't be handed a fresh workspace as a side effect. --- .../sim/app/api/workspaces/[id]/route.test.ts | 37 +-- apps/sim/app/api/workspaces/[id]/route.ts | 13 +- apps/sim/app/api/workspaces/route.ts | 109 +------ apps/sim/lib/workspaces/create.test.ts | 122 ++++++++ apps/sim/lib/workspaces/create.ts | 157 ++++++++++ apps/sim/lib/workspaces/lifecycle.test.ts | 50 +++- apps/sim/lib/workspaces/lifecycle.ts | 269 +++++++++--------- 7 files changed, 492 insertions(+), 265 deletions(-) create mode 100644 apps/sim/lib/workspaces/create.test.ts create mode 100644 apps/sim/lib/workspaces/create.ts diff --git a/apps/sim/app/api/workspaces/[id]/route.test.ts b/apps/sim/app/api/workspaces/[id]/route.test.ts index b916ba3c19b..cfd489bdf1a 100644 --- a/apps/sim/app/api/workspaces/[id]/route.test.ts +++ b/apps/sim/app/api/workspaces/[id]/route.test.ts @@ -61,22 +61,6 @@ describe('DELETE /api/workspaces/[id]', () => { expect(mockArchiveWorkspace).not.toHaveBeenCalled() }) - it('returns 400 and does not record an audit entry when archival would strand a member', async () => { - mockArchiveWorkspace.mockResolvedValue({ - archived: false, - workspaceName: 'Victim Workspace', - strandedUserIds: ['user-victim'], - }) - - const response = await callDelete() - const body = await response.json() - - expect(response.status).toBe(400) - expect(body.error).toMatch(/no other workspace/i) - expect(auditMockFns.mockRecordAudit).not.toHaveBeenCalled() - expect(posthogServerMockFns.mockCaptureServerEvent).not.toHaveBeenCalled() - }) - it('returns 404 when the workspace does not exist', async () => { mockArchiveWorkspace.mockResolvedValue({ archived: false }) @@ -115,6 +99,27 @@ describe('DELETE /api/workspaces/[id]', () => { ) }) + it('succeeds and records which members were auto-provisioned a replacement workspace', async () => { + mockArchiveWorkspace.mockResolvedValue({ + archived: true, + workspaceName: 'Test Workspace', + provisionedWorkspaceUserIds: ['user-victim'], + }) + + const response = await callDelete('workspace-1') + const body = await response.json() + + expect(response.status).toBe(200) + expect(body).toEqual({ success: true }) + expect(auditMockFns.mockRecordAudit).toHaveBeenCalledWith( + expect.objectContaining({ + metadata: expect.objectContaining({ + provisionedWorkspaceUserIds: ['user-victim'], + }), + }) + ) + }) + it('returns 500 when archival throws unexpectedly', async () => { mockArchiveWorkspace.mockRejectedValue(new Error('db exploded')) diff --git a/apps/sim/app/api/workspaces/[id]/route.ts b/apps/sim/app/api/workspaces/[id]/route.ts index 1dc4ee0bf25..23a52a7d590 100644 --- a/apps/sim/app/api/workspaces/[id]/route.ts +++ b/apps/sim/app/api/workspaces/[id]/route.ts @@ -252,16 +252,6 @@ export const DELETE = withRouteHandler( requestId: `workspace-${workspaceId}`, }) - if (archiveResult.strandedUserIds?.length) { - return NextResponse.json( - { - error: - 'Cannot delete this workspace: one or more members have no other workspace to fall back to', - }, - { status: 400 } - ) - } - if (!archiveResult.archived) { return NextResponse.json({ error: 'Workspace not found' }, { status: 404 }) } @@ -281,6 +271,9 @@ export const DELETE = withRouteHandler( workflows: workflowIds.length, }, archived: archiveResult.archived, + ...(archiveResult.provisionedWorkspaceUserIds?.length && { + provisionedWorkspaceUserIds: archiveResult.provisionedWorkspaceUserIds, + }), }, request, }) diff --git a/apps/sim/app/api/workspaces/route.ts b/apps/sim/app/api/workspaces/route.ts index 8d35dc7429b..bed366bf2ae 100644 --- a/apps/sim/app/api/workspaces/route.ts +++ b/apps/sim/app/api/workspaces/route.ts @@ -1,8 +1,7 @@ import { AuditAction, AuditResourceType, recordAudit } from '@sim/audit' import { db } from '@sim/db' -import { permissions, settings, type WorkspaceMode, workflow, workspace } from '@sim/db/schema' +import { settings, type WorkspaceMode, workflow } from '@sim/db/schema' import { createLogger } from '@sim/logger' -import { generateId } from '@sim/utils/id' import { and, eq, isNull } from 'drizzle-orm' import { type NextRequest, NextResponse } from 'next/server' import { listWorkspacesQuerySchema } from '@/lib/api/contracts' @@ -13,9 +12,7 @@ import type { PlanCategory } from '@/lib/billing/plan-helpers' import { PlatformEvents } from '@/lib/core/telemetry' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { captureServerEvent } from '@/lib/posthog/server' -import { buildDefaultWorkflowArtifacts } from '@/lib/workflows/defaults' -import { saveWorkflowToNormalizedTables } from '@/lib/workflows/persistence/utils' -import { getRandomWorkspaceColor } from '@/lib/workspaces/colors' +import { createWorkspaceRecord } from '@/lib/workspaces/create' import { CONTACT_OWNER_TO_UPGRADE_REASON, evaluateWorkspaceInvitePolicy, @@ -283,90 +280,19 @@ async function createWorkspace({ workspaceMode, billedAccountUserId, }: CreateWorkspaceParams) { - const workspaceId = generateId() - const workflowId = generateId() - const now = new Date() - const color = explicitColor || getRandomWorkspaceColor() - - try { - await db.transaction(async (tx) => { - await tx.insert(workspace).values({ - id: workspaceId, - name, - color, - ownerId: userId, - organizationId, - workspaceMode, - billedAccountUserId, - allowPersonalApiKeys: true, - createdAt: now, - updatedAt: now, - }) - - const permissionRows = [ - { - id: generateId(), - entityType: 'workspace' as const, - entityId: workspaceId, - userId, - permissionType: 'admin' as const, - createdAt: now, - updatedAt: now, - }, - ] - - if ( - workspaceMode === WORKSPACE_MODE.ORGANIZATION && - billedAccountUserId && - billedAccountUserId !== userId - ) { - permissionRows.push({ - id: generateId(), - entityType: 'workspace' as const, - entityId: workspaceId, - userId: billedAccountUserId, - permissionType: 'admin' as const, - createdAt: now, - updatedAt: now, - }) - } - - await tx.insert(permissions).values(permissionRows) - - if (!skipDefaultWorkflow) { - await tx.insert(workflow).values({ - id: workflowId, - userId, - workspaceId, - folderId: null, - name: 'default-agent', - description: 'Your first workflow - start building here!', - lastSynced: now, - createdAt: now, - updatedAt: now, - isDeployed: false, - runCount: 0, - variables: {}, - }) - - const { workflowState } = buildDefaultWorkflowArtifacts() - await saveWorkflowToNormalizedTables(workflowId, workflowState, tx) - } - - logger.info( - skipDefaultWorkflow - ? `Created ${workspaceMode} workspace ${workspaceId} for user ${userId}` - : `Created ${workspaceMode} workspace ${workspaceId} with initial workflow ${workflowId} for user ${userId}` - ) - }) - } catch (error) { - logger.error(`Failed to create workspace ${workspaceId}:`, error) - throw error - } + const record = await createWorkspaceRecord({ + userId, + name, + skipDefaultWorkflow, + explicitColor, + organizationId, + workspaceMode, + billedAccountUserId, + }) try { PlatformEvents.workspaceCreated({ - workspaceId, + workspaceId: record.id, userId, name, }) @@ -389,16 +315,7 @@ async function createWorkspace({ : CONTACT_OWNER_TO_UPGRADE_REASON return { - id: workspaceId, - name, - color, - ownerId: userId, - organizationId, - workspaceMode, - billedAccountUserId, - allowPersonalApiKeys: true, - createdAt: now, - updatedAt: now, + ...record, role: 'owner', permissions: 'admin', inviteMembersEnabled: invitePolicy.allowed, diff --git a/apps/sim/lib/workspaces/create.test.ts b/apps/sim/lib/workspaces/create.test.ts new file mode 100644 index 00000000000..cf0bcf9e1f2 --- /dev/null +++ b/apps/sim/lib/workspaces/create.test.ts @@ -0,0 +1,122 @@ +/** + * @vitest-environment node + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { mockTransaction, mockSaveWorkflowToNormalizedTables } = vi.hoisted(() => ({ + mockTransaction: vi.fn(), + mockSaveWorkflowToNormalizedTables: vi.fn(), +})) + +vi.mock('@sim/db', () => ({ + db: { + transaction: mockTransaction, + }, +})) + +vi.mock('@/lib/workflows/defaults', () => ({ + buildDefaultWorkflowArtifacts: () => ({ workflowState: { blocks: {}, edges: [] } }), +})) + +vi.mock('@/lib/workflows/persistence/utils', () => ({ + saveWorkflowToNormalizedTables: (...args: unknown[]) => + mockSaveWorkflowToNormalizedTables(...args), +})) + +vi.mock('@/lib/workspaces/colors', () => ({ + getRandomWorkspaceColor: () => '#123456', +})) + +import { createWorkspaceRecord } from './create' + +function createTx() { + return { + insert: vi.fn().mockImplementation(() => ({ + values: vi.fn().mockResolvedValue([]), + })), + } +} + +describe('createWorkspaceRecord', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('opens its own transaction when no executor is provided', async () => { + const tx = createTx() + mockTransaction.mockImplementation(async (callback: (trx: typeof tx) => Promise) => + callback(tx) + ) + + const record = await createWorkspaceRecord({ + userId: 'user-1', + name: 'My Workspace', + organizationId: null, + workspaceMode: 'personal', + billedAccountUserId: 'user-1', + }) + + expect(mockTransaction).toHaveBeenCalledTimes(1) + expect(record.name).toBe('My Workspace') + expect(record.ownerId).toBe('user-1') + expect(record.workspaceMode).toBe('personal') + // workspace insert, permission insert, workflow insert (default workflow not skipped) + expect(tx.insert).toHaveBeenCalledTimes(3) + expect(mockSaveWorkflowToNormalizedTables).toHaveBeenCalledTimes(1) + }) + + it('runs directly against a provided executor instead of opening a nested transaction', async () => { + const tx = createTx() + + await createWorkspaceRecord({ + userId: 'user-1', + name: 'My Workspace', + organizationId: null, + workspaceMode: 'personal', + billedAccountUserId: 'user-1', + executor: tx as never, + }) + + expect(mockTransaction).not.toHaveBeenCalled() + expect(tx.insert).toHaveBeenCalledTimes(3) + }) + + it('skips the default workflow insert when skipDefaultWorkflow is set', async () => { + const tx = createTx() + + await createWorkspaceRecord({ + userId: 'user-1', + name: 'My Workspace', + organizationId: null, + workspaceMode: 'personal', + billedAccountUserId: 'user-1', + skipDefaultWorkflow: true, + executor: tx as never, + }) + + // workspace insert, permission insert — no workflow insert + expect(tx.insert).toHaveBeenCalledTimes(2) + expect(mockSaveWorkflowToNormalizedTables).not.toHaveBeenCalled() + }) + + it('adds a second admin permission row for the billed account when it differs from the owner in org mode', async () => { + const tx = createTx() + + await createWorkspaceRecord({ + userId: 'user-1', + name: 'Org Workspace', + organizationId: 'org-1', + workspaceMode: 'organization', + billedAccountUserId: 'user-billing', + executor: tx as never, + }) + + const permissionValuesCall = tx.insert.mock.results[1].value.values as ReturnType + const insertedPermissionRows = permissionValuesCall.mock.calls[0][0] + expect(insertedPermissionRows).toHaveLength(2) + expect(insertedPermissionRows.map((row: { userId: string }) => row.userId)).toEqual([ + 'user-1', + 'user-billing', + ]) + }) +}) diff --git a/apps/sim/lib/workspaces/create.ts b/apps/sim/lib/workspaces/create.ts new file mode 100644 index 00000000000..490d69a5194 --- /dev/null +++ b/apps/sim/lib/workspaces/create.ts @@ -0,0 +1,157 @@ +import { db } from '@sim/db' +import { permissions, type WorkspaceMode, workflow, workspace } from '@sim/db/schema' +import { createLogger } from '@sim/logger' +import { generateId } from '@sim/utils/id' +import type { DbOrTx } from '@/lib/db/types' +import { buildDefaultWorkflowArtifacts } from '@/lib/workflows/defaults' +import { saveWorkflowToNormalizedTables } from '@/lib/workflows/persistence/utils' +import { getRandomWorkspaceColor } from '@/lib/workspaces/colors' +import { WORKSPACE_MODE } from '@/lib/workspaces/policy' + +const logger = createLogger('WorkspaceCreate') + +export interface CreateWorkspaceRecordParams { + userId: string + name: string + skipDefaultWorkflow?: boolean + explicitColor?: string + organizationId: string | null + workspaceMode: WorkspaceMode + billedAccountUserId: string + /** + * Runs the insert against an existing transaction instead of opening a new one — for callers + * that need workspace creation to be atomic with other writes (e.g. archiving a workspace that + * would otherwise strand a member). Defaults to opening its own transaction against `db`. + */ + executor?: DbOrTx +} + +export interface CreatedWorkspaceRecord { + id: string + name: string + color: string + ownerId: string + organizationId: string | null + workspaceMode: WorkspaceMode + billedAccountUserId: string + allowPersonalApiKeys: boolean + createdAt: Date + updatedAt: Date +} + +/** + * Core workspace-creation write: inserts the workspace, its owner admin permission row, and + * (unless skipped) a default starter workflow. Shared by the `POST /api/workspaces` route and + * the workspace-archival safety net that auto-provisions a replacement workspace for a member + * who would otherwise be left with zero workspaces. + */ +export async function createWorkspaceRecord({ + userId, + name, + skipDefaultWorkflow = false, + explicitColor, + organizationId, + workspaceMode, + billedAccountUserId, + executor, +}: CreateWorkspaceRecordParams): Promise { + const workspaceId = generateId() + const workflowId = generateId() + const now = new Date() + const color = explicitColor || getRandomWorkspaceColor() + + const run = async (tx: DbOrTx) => { + await tx.insert(workspace).values({ + id: workspaceId, + name, + color, + ownerId: userId, + organizationId, + workspaceMode, + billedAccountUserId, + allowPersonalApiKeys: true, + createdAt: now, + updatedAt: now, + }) + + const permissionRows = [ + { + id: generateId(), + entityType: 'workspace' as const, + entityId: workspaceId, + userId, + permissionType: 'admin' as const, + createdAt: now, + updatedAt: now, + }, + ] + + if ( + workspaceMode === WORKSPACE_MODE.ORGANIZATION && + billedAccountUserId && + billedAccountUserId !== userId + ) { + permissionRows.push({ + id: generateId(), + entityType: 'workspace' as const, + entityId: workspaceId, + userId: billedAccountUserId, + permissionType: 'admin' as const, + createdAt: now, + updatedAt: now, + }) + } + + await tx.insert(permissions).values(permissionRows) + + if (!skipDefaultWorkflow) { + await tx.insert(workflow).values({ + id: workflowId, + userId, + workspaceId, + folderId: null, + name: 'default-agent', + description: 'Your first workflow - start building here!', + lastSynced: now, + createdAt: now, + updatedAt: now, + isDeployed: false, + runCount: 0, + variables: {}, + }) + + const { workflowState } = buildDefaultWorkflowArtifacts() + await saveWorkflowToNormalizedTables(workflowId, workflowState, tx) + } + + logger.info( + skipDefaultWorkflow + ? `Created ${workspaceMode} workspace ${workspaceId} for user ${userId}` + : `Created ${workspaceMode} workspace ${workspaceId} with initial workflow ${workflowId} for user ${userId}` + ) + } + + try { + if (executor) { + await run(executor) + } else { + await db.transaction(run) + } + } catch (error) { + logger.error(`Failed to create workspace ${workspaceId}:`, error) + throw error + } + + return { + id: workspaceId, + name, + color, + ownerId: userId, + organizationId, + workspaceMode, + billedAccountUserId, + allowPersonalApiKeys: true, + createdAt: now, + updatedAt: now, + } +} diff --git a/apps/sim/lib/workspaces/lifecycle.test.ts b/apps/sim/lib/workspaces/lifecycle.test.ts index 780c20e9665..6c5549b2469 100644 --- a/apps/sim/lib/workspaces/lifecycle.test.ts +++ b/apps/sim/lib/workspaces/lifecycle.test.ts @@ -9,11 +9,13 @@ const { mockTransaction, mockArchiveWorkflowsForWorkspace, mockListAccessibleWorkspaceRowsForUser, + mockCreateWorkspaceRecord, } = vi.hoisted(() => ({ mockSelect: vi.fn(), mockTransaction: vi.fn(), mockArchiveWorkflowsForWorkspace: vi.fn(), mockListAccessibleWorkspaceRowsForUser: vi.fn(), + mockCreateWorkspaceRecord: vi.fn(), })) const mockGetWorkspaceWithOwner = permissionsMockFns.mockGetWorkspaceWithOwner @@ -35,6 +37,10 @@ vi.mock('@/lib/workspaces/utils', () => ({ listAccessibleWorkspaceRowsForUser: mockListAccessibleWorkspaceRowsForUser, })) +vi.mock('@/lib/workspaces/create', () => ({ + createWorkspaceRecord: mockCreateWorkspaceRecord, +})) + import { archiveWorkspace } from './lifecycle' function createUpdateChain() { @@ -80,6 +86,7 @@ describe('workspace lifecycle', () => { where: vi.fn().mockResolvedValue([]), }), }) + mockCreateWorkspaceRecord.mockResolvedValue({ id: 'fallback-workspace' }) }) it('archives workspace and dependent resources under serializable isolation', async () => { @@ -108,18 +115,20 @@ describe('workspace lifecycle', () => { expect(tx.update).toHaveBeenCalledTimes(8) expect(tx.delete).toHaveBeenCalledTimes(1) expect(mockListAccessibleWorkspaceRowsForUser).not.toHaveBeenCalled() + expect(mockCreateWorkspaceRecord).not.toHaveBeenCalled() expect(mockTransaction).toHaveBeenCalledWith(expect.any(Function), { isolationLevel: 'serializable', }) }) - it('blocks archival when a member would be left with zero workspaces', async () => { + it('auto-provisions a replacement workspace for a member who would be stranded, and still archives', async () => { mockGetWorkspaceWithOwner.mockResolvedValue({ id: 'workspace-1', name: 'Workspace 1', ownerId: 'user-1', archivedAt: null, }) + mockArchiveWorkflowsForWorkspace.mockResolvedValue(0) mockListAccessibleWorkspaceRowsForUser.mockResolvedValue([ accessibleWorkspaceRow('workspace-1'), ]) @@ -132,22 +141,33 @@ describe('workspace lifecycle', () => { const result = await archiveWorkspace('workspace-1', { requestId: 'req-1' }) expect(result).toEqual({ - archived: false, + archived: true, workspaceName: 'Workspace 1', - strandedUserIds: ['user-victim'], + provisionedWorkspaceUserIds: ['user-victim'], }) expect(mockListAccessibleWorkspaceRowsForUser).toHaveBeenCalledWith('user-victim', 'active', tx) - expect(tx.update).not.toHaveBeenCalled() - expect(tx.delete).not.toHaveBeenCalled() + expect(mockCreateWorkspaceRecord).toHaveBeenCalledWith( + expect.objectContaining({ + userId: 'user-victim', + workspaceMode: 'personal', + organizationId: null, + billedAccountUserId: 'user-victim', + executor: tx, + }) + ) + // Deletion is never blocked — the workspace is still archived alongside the fallback creation. + expect(tx.update).toHaveBeenCalledTimes(8) + expect(tx.delete).toHaveBeenCalledTimes(1) }) - it('blocks archival when only one of several members would be stranded', async () => { + it('only provisions a fallback for the one member who would actually be stranded', async () => { mockGetWorkspaceWithOwner.mockResolvedValue({ id: 'workspace-1', name: 'Workspace 1', ownerId: 'user-1', archivedAt: null, }) + mockArchiveWorkflowsForWorkspace.mockResolvedValue(0) mockListAccessibleWorkspaceRowsForUser.mockImplementation(async (userId: string) => userId === 'user-victim' ? [accessibleWorkspaceRow('workspace-1')] @@ -162,11 +182,14 @@ describe('workspace lifecycle', () => { const result = await archiveWorkspace('workspace-1', { requestId: 'req-1' }) expect(result).toEqual({ - archived: false, + archived: true, workspaceName: 'Workspace 1', - strandedUserIds: ['user-victim'], + provisionedWorkspaceUserIds: ['user-victim'], }) - expect(tx.update).not.toHaveBeenCalled() + expect(mockCreateWorkspaceRecord).toHaveBeenCalledTimes(1) + expect(mockCreateWorkspaceRecord).toHaveBeenCalledWith( + expect.objectContaining({ userId: 'user-victim' }) + ) }) it('does not strand an org admin who has no explicit permission row on another workspace', async () => { @@ -195,10 +218,11 @@ describe('workspace lifecycle', () => { archived: true, workspaceName: 'Workspace 1', }) + expect(mockCreateWorkspaceRecord).not.toHaveBeenCalled() expect(tx.update).toHaveBeenCalledTimes(8) }) - it('proceeds when every member has another active workspace', async () => { + it('proceeds without provisioning when every member has another active workspace', async () => { mockGetWorkspaceWithOwner.mockResolvedValue({ id: 'workspace-1', name: 'Workspace 1', @@ -222,11 +246,12 @@ describe('workspace lifecycle', () => { archived: true, workspaceName: 'Workspace 1', }) + expect(mockCreateWorkspaceRecord).not.toHaveBeenCalled() // No knowledge bases found, so the two KB-dependent updates (document, knowledgeConnector) are skipped. expect(tx.update).toHaveBeenCalledTimes(8) }) - it('skips the stranded-member check and serializable isolation when force is set', async () => { + it('skips the stranded-member check and provisioning entirely when force is set (ban flow)', async () => { mockGetWorkspaceWithOwner.mockResolvedValue({ id: 'workspace-1', name: 'Workspace 1', @@ -235,7 +260,7 @@ describe('workspace lifecycle', () => { }) mockArchiveWorkflowsForWorkspace.mockResolvedValue(0) - const tx = createTx([{ userId: 'user-victim' }]) + const tx = createTx([{ userId: 'user-banned' }]) mockTransaction.mockImplementation(async (callback: (trx: typeof tx) => Promise) => callback(tx) ) @@ -248,6 +273,7 @@ describe('workspace lifecycle', () => { }) expect(tx.selectDistinct).not.toHaveBeenCalled() expect(mockListAccessibleWorkspaceRowsForUser).not.toHaveBeenCalled() + expect(mockCreateWorkspaceRecord).not.toHaveBeenCalled() expect(mockTransaction).toHaveBeenCalledWith(expect.any(Function), undefined) }) diff --git a/apps/sim/lib/workspaces/lifecycle.ts b/apps/sim/lib/workspaces/lifecycle.ts index 8270a7921d9..32c3d913bae 100644 --- a/apps/sim/lib/workspaces/lifecycle.ts +++ b/apps/sim/lib/workspaces/lifecycle.ts @@ -20,16 +20,23 @@ import type { DbOrTx } from '@/lib/db/types' import { mcpPubSub } from '@/lib/mcp/pubsub' import { mcpService } from '@/lib/mcp/service' import { archiveWorkflowsForWorkspace } from '@/lib/workflows/lifecycle' +import { createWorkspaceRecord } from '@/lib/workspaces/create' import { getWorkspaceWithOwner } from '@/lib/workspaces/permissions/utils' +import { WORKSPACE_MODE } from '@/lib/workspaces/policy' import { listAccessibleWorkspaceRowsForUser } from '@/lib/workspaces/utils' const logger = createLogger('WorkspaceLifecycle') +/** Fallback workspace name for a member auto-provisioned a replacement — matches the + * pre-existing "you have no workspaces" client-side recovery naming for consistency. */ +const FALLBACK_WORKSPACE_NAME = 'My Workspace' + interface ArchiveWorkspaceOptions { requestId: string /** - * Skips the "would strand a member" safety check. Only for account-disable flows where every - * workspace owned by the disabled user must be archived regardless of member workspace counts. + * Skips auto-provisioning replacement workspaces for members who'd otherwise be stranded. + * Only for account-disable flows: a banned user's owned workspaces must be fully disabled, and + * the banned user specifically should not be handed a fresh workspace as a side effect. */ force?: boolean } @@ -37,15 +44,9 @@ interface ArchiveWorkspaceOptions { interface ArchiveWorkspaceResult { archived: boolean workspaceName?: string - /** Present only when archival was blocked because it would leave these members with zero workspaces. */ - strandedUserIds?: string[] -} - -class WorkspaceArchiveBlockedError extends Error { - constructor(readonly strandedUserIds: string[]) { - super('Archiving this workspace would leave one or more members with no workspace') - this.name = 'WorkspaceArchiveBlockedError' - } + /** userIds who were auto-provisioned a replacement workspace because this deletion would + * otherwise have left them with zero active workspaces. */ + provisionedWorkspaceUserIds?: string[] } /** @@ -107,158 +108,163 @@ export async function archiveWorkspace( // serializable: without it, two concurrent deletions of different workspaces shared by the // same sole member could each read a pre-deletion workspace count, both conclude the member - // isn't stranded, and together leave them with zero workspaces. Postgres detects this write - // skew under serializable isolation and aborts one transaction. Skipped when force is set, - // since that path never runs the stranded check in the first place. + // isn't stranded, and together leave them with zero workspaces without either provisioning a + // replacement. Postgres detects this write skew under serializable isolation and aborts one + // transaction. Skipped when force is set, since that path never runs this check at all. const transactionConfig = options.force ? undefined : ({ isolationLevel: 'serializable' } as const) - try { - await db.transaction(async (tx) => { - if (!options.force) { - const strandedUserIds = await findMembersStrandedByArchival(tx, workspaceId) - if (strandedUserIds.length > 0) { - throw new WorkspaceArchiveBlockedError(strandedUserIds) - } - } + let provisionedWorkspaceUserIds: string[] = [] - await tx - .update(knowledgeBase) - .set({ - deletedAt: now, - updatedAt: now, + await db.transaction(async (tx) => { + if (!options.force) { + const strandedUserIds = await findMembersStrandedByArchival(tx, workspaceId) + for (const userId of strandedUserIds) { + await createWorkspaceRecord({ + userId, + name: FALLBACK_WORKSPACE_NAME, + organizationId: null, + workspaceMode: WORKSPACE_MODE.PERSONAL, + billedAccountUserId: userId, + executor: tx, }) - .where(and(eq(knowledgeBase.workspaceId, workspaceId), isNull(knowledgeBase.deletedAt))) - - const workspaceKbIds = await tx - .select({ id: knowledgeBase.id }) - .from(knowledgeBase) - .where(eq(knowledgeBase.workspaceId, workspaceId)) - - const knowledgeBaseIds = workspaceKbIds.map((entry) => entry.id) - if (knowledgeBaseIds.length > 0) { - await tx - .update(document) - .set({ archivedAt: now }) - .where( - and( - inArray(document.knowledgeBaseId, knowledgeBaseIds), - isNull(document.archivedAt), - isNull(document.deletedAt) - ) - ) - - await tx - .update(knowledgeConnector) - .set({ archivedAt: now, status: 'paused', updatedAt: now }) - .where( - and( - inArray(knowledgeConnector.knowledgeBaseId, knowledgeBaseIds), - isNull(knowledgeConnector.archivedAt), - isNull(knowledgeConnector.deletedAt) - ) - ) } + provisionedWorkspaceUserIds = strandedUserIds + } + + await tx + .update(knowledgeBase) + .set({ + deletedAt: now, + updatedAt: now, + }) + .where(and(eq(knowledgeBase.workspaceId, workspaceId), isNull(knowledgeBase.deletedAt))) + + const workspaceKbIds = await tx + .select({ id: knowledgeBase.id }) + .from(knowledgeBase) + .where(eq(knowledgeBase.workspaceId, workspaceId)) + const knowledgeBaseIds = workspaceKbIds.map((entry) => entry.id) + if (knowledgeBaseIds.length > 0) { await tx - .update(userTableDefinitions) - .set({ - archivedAt: now, - updatedAt: now, - }) + .update(document) + .set({ archivedAt: now }) .where( and( - eq(userTableDefinitions.workspaceId, workspaceId), - isNull(userTableDefinitions.archivedAt) + inArray(document.knowledgeBaseId, knowledgeBaseIds), + isNull(document.archivedAt), + isNull(document.deletedAt) ) ) await tx - .update(workspaceFiles) - .set({ - deletedAt: now, - }) - .where(and(eq(workspaceFiles.workspaceId, workspaceId), isNull(workspaceFiles.deletedAt))) - - await tx - .update(invitation) - .set({ - status: 'cancelled', - updatedAt: now, - }) + .update(knowledgeConnector) + .set({ archivedAt: now, status: 'paused', updatedAt: now }) .where( and( - eq(invitation.status, 'pending'), - sql`${invitation.id} IN ( + inArray(knowledgeConnector.knowledgeBaseId, knowledgeBaseIds), + isNull(knowledgeConnector.archivedAt), + isNull(knowledgeConnector.deletedAt) + ) + ) + } + + await tx + .update(userTableDefinitions) + .set({ + archivedAt: now, + updatedAt: now, + }) + .where( + and( + eq(userTableDefinitions.workspaceId, workspaceId), + isNull(userTableDefinitions.archivedAt) + ) + ) + + await tx + .update(workspaceFiles) + .set({ + deletedAt: now, + }) + .where(and(eq(workspaceFiles.workspaceId, workspaceId), isNull(workspaceFiles.deletedAt))) + + await tx + .update(invitation) + .set({ + status: 'cancelled', + updatedAt: now, + }) + .where( + and( + eq(invitation.status, 'pending'), + sql`${invitation.id} IN ( SELECT ${invitationWorkspaceGrant.invitationId} FROM ${invitationWorkspaceGrant} WHERE ${invitationWorkspaceGrant.workspaceId} = ${workspaceId} )` - ) ) + ) - await tx - .delete(apiKey) - .where(and(eq(apiKey.workspaceId, workspaceId), eq(apiKey.type, 'workspace'))) + await tx + .delete(apiKey) + .where(and(eq(apiKey.workspaceId, workspaceId), eq(apiKey.type, 'workspace'))) - await tx - .update(workflowMcpServer) - .set({ - deletedAt: now, - isPublic: false, - updatedAt: now, - }) - .where(eq(workflowMcpServer.workspaceId, workspaceId)) + await tx + .update(workflowMcpServer) + .set({ + deletedAt: now, + isPublic: false, + updatedAt: now, + }) + .where(eq(workflowMcpServer.workspaceId, workspaceId)) - await tx - .update(mcpServers) - .set({ - deletedAt: now, - enabled: false, - updatedAt: now, - }) - .where(and(eq(mcpServers.workspaceId, workspaceId), isNull(mcpServers.deletedAt))) + await tx + .update(mcpServers) + .set({ + deletedAt: now, + enabled: false, + updatedAt: now, + }) + .where(and(eq(mcpServers.workspaceId, workspaceId), isNull(mcpServers.deletedAt))) - await tx - .update(workflowSchedule) - .set({ - archivedAt: now, - updatedAt: now, - status: 'disabled', - nextRunAt: null, - lastQueuedAt: null, - }) - .where( - and( - eq(workflowSchedule.sourceWorkspaceId, workspaceId), - eq(workflowSchedule.sourceType, 'job'), - isNull(workflowSchedule.archivedAt) - ) + await tx + .update(workflowSchedule) + .set({ + archivedAt: now, + updatedAt: now, + status: 'disabled', + nextRunAt: null, + lastQueuedAt: null, + }) + .where( + and( + eq(workflowSchedule.sourceWorkspaceId, workspaceId), + eq(workflowSchedule.sourceType, 'job'), + isNull(workflowSchedule.archivedAt) ) + ) - await tx - .update(workspace) - .set({ - archivedAt: now, - updatedAt: now, - }) - .where(and(eq(workspace.id, workspaceId), isNull(workspace.archivedAt))) - }, transactionConfig) - } catch (error) { - if (error instanceof WorkspaceArchiveBlockedError) { - return { - archived: false, - workspaceName: workspaceRecord.name, - strandedUserIds: error.strandedUserIds, - } - } - throw error - } + await tx + .update(workspace) + .set({ + archivedAt: now, + updatedAt: now, + }) + .where(and(eq(workspace.id, workspaceId), isNull(workspace.archivedAt))) + }, transactionConfig) await archiveWorkflowsForWorkspace(workspaceId, options) logger.info(`[${options.requestId}] Archived workspace ${workspaceId}`) + if (provisionedWorkspaceUserIds.length > 0) { + logger.info( + `[${options.requestId}] Provisioned replacement workspaces for members stranded by archiving ${workspaceId}`, + { userIds: provisionedWorkspaceUserIds } + ) + } await mcpService.clearCache(workspaceId).catch(() => undefined) @@ -274,5 +280,6 @@ export async function archiveWorkspace( return { archived: true, workspaceName: workspaceRecord.name, + ...(provisionedWorkspaceUserIds.length > 0 && { provisionedWorkspaceUserIds }), } } From 24ac78059bcc379147f92d3d4a3e70b73eeeffcd Mon Sep 17 00:00:00 2001 From: waleed Date: Tue, 7 Jul 2026 17:25:24 -0700 Subject: [PATCH 04/12] chore(workspaces): tighten inline comments in archiveWorkspace --- apps/sim/lib/workspaces/lifecycle.ts | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/apps/sim/lib/workspaces/lifecycle.ts b/apps/sim/lib/workspaces/lifecycle.ts index 32c3d913bae..892e59715e3 100644 --- a/apps/sim/lib/workspaces/lifecycle.ts +++ b/apps/sim/lib/workspaces/lifecycle.ts @@ -27,8 +27,7 @@ import { listAccessibleWorkspaceRowsForUser } from '@/lib/workspaces/utils' const logger = createLogger('WorkspaceLifecycle') -/** Fallback workspace name for a member auto-provisioned a replacement — matches the - * pre-existing "you have no workspaces" client-side recovery naming for consistency. */ +/** Matches the pre-existing "you have no workspaces" client-side recovery naming. */ const FALLBACK_WORKSPACE_NAME = 'My Workspace' interface ArchiveWorkspaceOptions { @@ -106,11 +105,10 @@ export async function archiveWorkspace( .from(workflowMcpServer) .where(eq(workflowMcpServer.workspaceId, workspaceId)) - // serializable: without it, two concurrent deletions of different workspaces shared by the - // same sole member could each read a pre-deletion workspace count, both conclude the member - // isn't stranded, and together leave them with zero workspaces without either provisioning a - // replacement. Postgres detects this write skew under serializable isolation and aborts one - // transaction. Skipped when force is set, since that path never runs this check at all. + // serializable: without it, two concurrent deletions sharing a sole member could each read a + // pre-deletion workspace count and both skip provisioning a replacement. Postgres detects this + // write skew under serializable isolation and aborts one transaction. Skipped when force is + // set, since that path never runs the stranded-member check at all. const transactionConfig = options.force ? undefined : ({ isolationLevel: 'serializable' } as const) From 8a6d4faa9f73de4f59fd2012d00da6e358c7335f Mon Sep 17 00:00:00 2001 From: waleed Date: Tue, 7 Jul 2026 17:38:07 -0700 Subject: [PATCH 05/12] fix(workspaces): record an audit entry for auto-provisioned fallback workspaces Normal workspace creation gets its own WORKSPACE_CREATED audit row; the auto-provisioned fallback for a stranded member didn't, making it only discoverable indirectly via the deletion's audit metadata. archiveWorkspace now accepts an optional actor (from the deleting admin's session) and records a WORKSPACE_CREATED audit entry per fallback workspace it provisions, attributing the action to the admin who triggered the deletion. --- .../sim/app/api/workspaces/[id]/route.test.ts | 3 ++ apps/sim/app/api/workspaces/[id]/route.ts | 3 ++ apps/sim/lib/workspaces/lifecycle.test.ts | 46 +++++++++++++++++-- apps/sim/lib/workspaces/lifecycle.ts | 22 ++++++++- 4 files changed, 70 insertions(+), 4 deletions(-) diff --git a/apps/sim/app/api/workspaces/[id]/route.test.ts b/apps/sim/app/api/workspaces/[id]/route.test.ts index cfd489bdf1a..a8aee5c45d6 100644 --- a/apps/sim/app/api/workspaces/[id]/route.test.ts +++ b/apps/sim/app/api/workspaces/[id]/route.test.ts @@ -83,6 +83,9 @@ describe('DELETE /api/workspaces/[id]', () => { expect(body).toEqual({ success: true }) expect(mockArchiveWorkspace).toHaveBeenCalledWith('workspace-1', { requestId: 'workspace-workspace-1', + actorId: 'user-admin', + actorName: 'Admin', + actorEmail: 'admin@example.com', }) expect(auditMockFns.mockRecordAudit).toHaveBeenCalledWith( expect.objectContaining({ diff --git a/apps/sim/app/api/workspaces/[id]/route.ts b/apps/sim/app/api/workspaces/[id]/route.ts index 23a52a7d590..56fdbb358a7 100644 --- a/apps/sim/app/api/workspaces/[id]/route.ts +++ b/apps/sim/app/api/workspaces/[id]/route.ts @@ -250,6 +250,9 @@ export const DELETE = withRouteHandler( const archiveResult = await archiveWorkspace(workspaceId, { requestId: `workspace-${workspaceId}`, + actorId: session.user.id, + actorName: session.user.name, + actorEmail: session.user.email, }) if (!archiveResult.archived) { diff --git a/apps/sim/lib/workspaces/lifecycle.test.ts b/apps/sim/lib/workspaces/lifecycle.test.ts index 6c5549b2469..18e547f6d40 100644 --- a/apps/sim/lib/workspaces/lifecycle.test.ts +++ b/apps/sim/lib/workspaces/lifecycle.test.ts @@ -1,7 +1,7 @@ /** * @vitest-environment node */ -import { permissionsMock, permissionsMockFns } from '@sim/testing' +import { auditMock, auditMockFns, permissionsMock, permissionsMockFns } from '@sim/testing' import { beforeEach, describe, expect, it, vi } from 'vitest' const { @@ -41,6 +41,8 @@ vi.mock('@/lib/workspaces/create', () => ({ createWorkspaceRecord: mockCreateWorkspaceRecord, })) +vi.mock('@sim/audit', () => auditMock) + import { archiveWorkspace } from './lifecycle' function createUpdateChain() { @@ -86,7 +88,7 @@ describe('workspace lifecycle', () => { where: vi.fn().mockResolvedValue([]), }), }) - mockCreateWorkspaceRecord.mockResolvedValue({ id: 'fallback-workspace' }) + mockCreateWorkspaceRecord.mockResolvedValue({ id: 'fallback-workspace', name: 'My Workspace' }) }) it('archives workspace and dependent resources under serializable isolation', async () => { @@ -138,7 +140,12 @@ describe('workspace lifecycle', () => { callback(tx) ) - const result = await archiveWorkspace('workspace-1', { requestId: 'req-1' }) + const result = await archiveWorkspace('workspace-1', { + requestId: 'req-1', + actorId: 'admin-1', + actorName: 'Admin', + actorEmail: 'admin@example.com', + }) expect(result).toEqual({ archived: true, @@ -155,11 +162,44 @@ describe('workspace lifecycle', () => { executor: tx, }) ) + expect(auditMockFns.mockRecordAudit).toHaveBeenCalledWith( + expect.objectContaining({ + actorId: 'admin-1', + resourceId: 'fallback-workspace', + metadata: expect.objectContaining({ + deletedWorkspaceId: 'workspace-1', + recipientUserId: 'user-victim', + }), + }) + ) // Deletion is never blocked — the workspace is still archived alongside the fallback creation. expect(tx.update).toHaveBeenCalledTimes(8) expect(tx.delete).toHaveBeenCalledTimes(1) }) + it('does not record an audit entry for the fallback workspace when no actor is provided', async () => { + mockGetWorkspaceWithOwner.mockResolvedValue({ + id: 'workspace-1', + name: 'Workspace 1', + ownerId: 'user-1', + archivedAt: null, + }) + mockArchiveWorkflowsForWorkspace.mockResolvedValue(0) + mockListAccessibleWorkspaceRowsForUser.mockResolvedValue([ + accessibleWorkspaceRow('workspace-1'), + ]) + + const tx = createTx([{ userId: 'user-victim' }]) + mockTransaction.mockImplementation(async (callback: (trx: typeof tx) => Promise) => + callback(tx) + ) + + await archiveWorkspace('workspace-1', { requestId: 'req-1' }) + + expect(mockCreateWorkspaceRecord).toHaveBeenCalled() + expect(auditMockFns.mockRecordAudit).not.toHaveBeenCalled() + }) + it('only provisions a fallback for the one member who would actually be stranded', async () => { mockGetWorkspaceWithOwner.mockResolvedValue({ id: 'workspace-1', diff --git a/apps/sim/lib/workspaces/lifecycle.ts b/apps/sim/lib/workspaces/lifecycle.ts index 892e59715e3..5f8bdfc6797 100644 --- a/apps/sim/lib/workspaces/lifecycle.ts +++ b/apps/sim/lib/workspaces/lifecycle.ts @@ -1,3 +1,4 @@ +import { AuditAction, AuditResourceType, recordAudit } from '@sim/audit' import { db } from '@sim/db' import { apiKey, @@ -38,6 +39,10 @@ interface ArchiveWorkspaceOptions { * the banned user specifically should not be handed a fresh workspace as a side effect. */ force?: boolean + /** Attributed as the actor on the audit log entry for any auto-provisioned replacement workspace. */ + actorId?: string + actorName?: string | null + actorEmail?: string | null } interface ArchiveWorkspaceResult { @@ -119,7 +124,7 @@ export async function archiveWorkspace( if (!options.force) { const strandedUserIds = await findMembersStrandedByArchival(tx, workspaceId) for (const userId of strandedUserIds) { - await createWorkspaceRecord({ + const fallbackWorkspace = await createWorkspaceRecord({ userId, name: FALLBACK_WORKSPACE_NAME, organizationId: null, @@ -127,6 +132,21 @@ export async function archiveWorkspace( billedAccountUserId: userId, executor: tx, }) + + if (options.actorId) { + recordAudit({ + workspaceId: fallbackWorkspace.id, + actorId: options.actorId, + actorName: options.actorName, + actorEmail: options.actorEmail, + action: AuditAction.WORKSPACE_CREATED, + resourceType: AuditResourceType.WORKSPACE, + resourceId: fallbackWorkspace.id, + resourceName: fallbackWorkspace.name, + description: `Auto-created replacement workspace "${fallbackWorkspace.name}" for a member left with no workspace after deleting "${workspaceRecord.name}"`, + metadata: { deletedWorkspaceId: workspaceId, recipientUserId: userId }, + }) + } } provisionedWorkspaceUserIds = strandedUserIds } From e013149c2e8af13d383ab363cd59dd6942f0d7bc Mon Sep 17 00:00:00 2001 From: waleed Date: Tue, 7 Jul 2026 17:48:13 -0700 Subject: [PATCH 06/12] =?UTF-8?q?refactor(workspaces):=20apply=20/simplify?= =?UTF-8?q?=20review=20=E2=80=94=20invert=20stranded-provisioning=20to=20o?= =?UTF-8?q?pt-in?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per multi-angle simplify/altitude review of the auto-provisioning fix: - archiveWorkspace's stranded-member handling is now opt-in (provisionFallbackForStrandedMembers, default off) instead of opt-out (force). A future caller of archiveWorkspace that doesn't know about this feature now gets the safe default (no side-effect workspace creation) rather than having to remember to pass force: true. Kept the logic inside archiveWorkspace's single transaction rather than splitting into two composable functions, since that would either reintroduce the TOCTOU race (separate transactions) or require threading an external tx through archiveWorkspace (bigger, riskier change for uncertain benefit). - provisionedWorkspaceUserIds is now returned from the transaction callback instead of mutated via an outer `let`. - Deduplicated CreateWorkspaceParams in app/api/workspaces/route.ts — now Omit instead of a hand-copied interface that could drift from the shared type. - Added a comment at the createWorkspaceRecord call site in archiveWorkspace noting it intentionally bypasses getWorkspaceCreationPolicy (system-provisioned safety net, not user self-service). - Renamed a colliding local test helper (createTx used for two different mock shapes in adjacent lib/workspaces test files). disableUserResources (ban flow) no longer needs a flag at all — the default now matches its existing behavior. --- .../sim/app/api/workspaces/[id]/route.test.ts | 1 + apps/sim/app/api/workspaces/[id]/route.ts | 1 + apps/sim/app/api/workspaces/route.ts | 12 +-- apps/sim/lib/workflows/lifecycle.test.ts | 6 +- apps/sim/lib/workflows/lifecycle.ts | 4 +- apps/sim/lib/workspaces/create.test.ts | 10 +-- apps/sim/lib/workspaces/lifecycle.test.ts | 36 +++++--- apps/sim/lib/workspaces/lifecycle.ts | 83 ++++++++++--------- 8 files changed, 81 insertions(+), 72 deletions(-) diff --git a/apps/sim/app/api/workspaces/[id]/route.test.ts b/apps/sim/app/api/workspaces/[id]/route.test.ts index a8aee5c45d6..2774b0c8638 100644 --- a/apps/sim/app/api/workspaces/[id]/route.test.ts +++ b/apps/sim/app/api/workspaces/[id]/route.test.ts @@ -83,6 +83,7 @@ describe('DELETE /api/workspaces/[id]', () => { expect(body).toEqual({ success: true }) expect(mockArchiveWorkspace).toHaveBeenCalledWith('workspace-1', { requestId: 'workspace-workspace-1', + provisionFallbackForStrandedMembers: true, actorId: 'user-admin', actorName: 'Admin', actorEmail: 'admin@example.com', diff --git a/apps/sim/app/api/workspaces/[id]/route.ts b/apps/sim/app/api/workspaces/[id]/route.ts index 56fdbb358a7..e84c3d3f855 100644 --- a/apps/sim/app/api/workspaces/[id]/route.ts +++ b/apps/sim/app/api/workspaces/[id]/route.ts @@ -250,6 +250,7 @@ export const DELETE = withRouteHandler( const archiveResult = await archiveWorkspace(workspaceId, { requestId: `workspace-${workspaceId}`, + provisionFallbackForStrandedMembers: true, actorId: session.user.id, actorName: session.user.name, actorEmail: session.user.email, diff --git a/apps/sim/app/api/workspaces/route.ts b/apps/sim/app/api/workspaces/route.ts index bed366bf2ae..da438d4ac85 100644 --- a/apps/sim/app/api/workspaces/route.ts +++ b/apps/sim/app/api/workspaces/route.ts @@ -12,7 +12,7 @@ import type { PlanCategory } from '@/lib/billing/plan-helpers' import { PlatformEvents } from '@/lib/core/telemetry' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { captureServerEvent } from '@/lib/posthog/server' -import { createWorkspaceRecord } from '@/lib/workspaces/create' +import { type CreateWorkspaceRecordParams, createWorkspaceRecord } from '@/lib/workspaces/create' import { CONTACT_OWNER_TO_UPGRADE_REASON, evaluateWorkspaceInvitePolicy, @@ -261,15 +261,7 @@ async function createDefaultWorkspace( }) } -interface CreateWorkspaceParams { - userId: string - name: string - skipDefaultWorkflow?: boolean - explicitColor?: string - organizationId: string | null - workspaceMode: WorkspaceMode - billedAccountUserId: string -} +type CreateWorkspaceParams = Omit async function createWorkspace({ userId, diff --git a/apps/sim/lib/workflows/lifecycle.test.ts b/apps/sim/lib/workflows/lifecycle.test.ts index dd5133bc182..7ed2f5455db 100644 --- a/apps/sim/lib/workflows/lifecycle.test.ts +++ b/apps/sim/lib/workflows/lifecycle.test.ts @@ -147,7 +147,7 @@ describe('disableUserResources', () => { vi.clearAllMocks() }) - it('force-archives every owned workspace so banning is never blocked by other members', async () => { + it('archives every owned workspace without opting into fallback provisioning, so banning is never blocked by other members', async () => { mockSelect.mockReturnValue(createSelectChain([{ id: 'workspace-1' }, { id: 'workspace-2' }])) mockDelete.mockReturnValue({ where: vi.fn().mockResolvedValue([]) }) mockArchiveWorkspace.mockResolvedValue({ archived: true, workspaceName: 'Workspace' }) @@ -157,11 +157,11 @@ describe('disableUserResources', () => { expect(mockArchiveWorkspace).toHaveBeenCalledTimes(2) expect(mockArchiveWorkspace).toHaveBeenCalledWith( 'workspace-1', - expect.objectContaining({ force: true }) + expect.not.objectContaining({ provisionFallbackForStrandedMembers: true }) ) expect(mockArchiveWorkspace).toHaveBeenCalledWith( 'workspace-2', - expect.objectContaining({ force: true }) + expect.not.objectContaining({ provisionFallbackForStrandedMembers: true }) ) expect(mockDelete).toHaveBeenCalled() }) diff --git a/apps/sim/lib/workflows/lifecycle.ts b/apps/sim/lib/workflows/lifecycle.ts index 2fbc7457a0c..82040e2750e 100644 --- a/apps/sim/lib/workflows/lifecycle.ts +++ b/apps/sim/lib/workflows/lifecycle.ts @@ -360,9 +360,7 @@ export async function disableUserResources(userId: string): Promise { .where(and(eq(workspace.ownerId, userId), isNull(workspace.archivedAt))) await Promise.all([ - // force: true — a banned user's owned workspaces must be fully disabled regardless of - // whether other members would be left with zero workspaces. - ...ownedWorkspaces.map((w) => archiveWorkspace(w.id, { requestId, force: true })), + ...ownedWorkspaces.map((w) => archiveWorkspace(w.id, { requestId })), db.delete(apiKey).where(eq(apiKey.userId, userId)), ]) diff --git a/apps/sim/lib/workspaces/create.test.ts b/apps/sim/lib/workspaces/create.test.ts index cf0bcf9e1f2..394daca5a83 100644 --- a/apps/sim/lib/workspaces/create.test.ts +++ b/apps/sim/lib/workspaces/create.test.ts @@ -29,7 +29,7 @@ vi.mock('@/lib/workspaces/colors', () => ({ import { createWorkspaceRecord } from './create' -function createTx() { +function createInsertOnlyTx() { return { insert: vi.fn().mockImplementation(() => ({ values: vi.fn().mockResolvedValue([]), @@ -43,7 +43,7 @@ describe('createWorkspaceRecord', () => { }) it('opens its own transaction when no executor is provided', async () => { - const tx = createTx() + const tx = createInsertOnlyTx() mockTransaction.mockImplementation(async (callback: (trx: typeof tx) => Promise) => callback(tx) ) @@ -66,7 +66,7 @@ describe('createWorkspaceRecord', () => { }) it('runs directly against a provided executor instead of opening a nested transaction', async () => { - const tx = createTx() + const tx = createInsertOnlyTx() await createWorkspaceRecord({ userId: 'user-1', @@ -82,7 +82,7 @@ describe('createWorkspaceRecord', () => { }) it('skips the default workflow insert when skipDefaultWorkflow is set', async () => { - const tx = createTx() + const tx = createInsertOnlyTx() await createWorkspaceRecord({ userId: 'user-1', @@ -100,7 +100,7 @@ describe('createWorkspaceRecord', () => { }) it('adds a second admin permission row for the billed account when it differs from the owner in org mode', async () => { - const tx = createTx() + const tx = createInsertOnlyTx() await createWorkspaceRecord({ userId: 'user-1', diff --git a/apps/sim/lib/workspaces/lifecycle.test.ts b/apps/sim/lib/workspaces/lifecycle.test.ts index 18e547f6d40..51f24b600a8 100644 --- a/apps/sim/lib/workspaces/lifecycle.test.ts +++ b/apps/sim/lib/workspaces/lifecycle.test.ts @@ -105,7 +105,10 @@ describe('workspace lifecycle', () => { callback(tx) ) - const result = await archiveWorkspace('workspace-1', { requestId: 'req-1' }) + const result = await archiveWorkspace('workspace-1', { + requestId: 'req-1', + provisionFallbackForStrandedMembers: true, + }) expect(result).toEqual({ archived: true, @@ -113,10 +116,10 @@ describe('workspace lifecycle', () => { }) expect(mockArchiveWorkflowsForWorkspace).toHaveBeenCalledWith('workspace-1', { requestId: 'req-1', + provisionFallbackForStrandedMembers: true, }) expect(tx.update).toHaveBeenCalledTimes(8) expect(tx.delete).toHaveBeenCalledTimes(1) - expect(mockListAccessibleWorkspaceRowsForUser).not.toHaveBeenCalled() expect(mockCreateWorkspaceRecord).not.toHaveBeenCalled() expect(mockTransaction).toHaveBeenCalledWith(expect.any(Function), { isolationLevel: 'serializable', @@ -142,6 +145,7 @@ describe('workspace lifecycle', () => { const result = await archiveWorkspace('workspace-1', { requestId: 'req-1', + provisionFallbackForStrandedMembers: true, actorId: 'admin-1', actorName: 'Admin', actorEmail: 'admin@example.com', @@ -172,7 +176,6 @@ describe('workspace lifecycle', () => { }), }) ) - // Deletion is never blocked — the workspace is still archived alongside the fallback creation. expect(tx.update).toHaveBeenCalledTimes(8) expect(tx.delete).toHaveBeenCalledTimes(1) }) @@ -194,7 +197,10 @@ describe('workspace lifecycle', () => { callback(tx) ) - await archiveWorkspace('workspace-1', { requestId: 'req-1' }) + await archiveWorkspace('workspace-1', { + requestId: 'req-1', + provisionFallbackForStrandedMembers: true, + }) expect(mockCreateWorkspaceRecord).toHaveBeenCalled() expect(auditMockFns.mockRecordAudit).not.toHaveBeenCalled() @@ -219,7 +225,10 @@ describe('workspace lifecycle', () => { callback(tx) ) - const result = await archiveWorkspace('workspace-1', { requestId: 'req-1' }) + const result = await archiveWorkspace('workspace-1', { + requestId: 'req-1', + provisionFallbackForStrandedMembers: true, + }) expect(result).toEqual({ archived: true, @@ -240,8 +249,6 @@ describe('workspace lifecycle', () => { archivedAt: null, }) mockArchiveWorkflowsForWorkspace.mockResolvedValue(0) - // The org admin's only *explicit* permission row is on workspace-1, but they still have - // access to workspace-2 purely through their organization admin role. mockListAccessibleWorkspaceRowsForUser.mockResolvedValue([ accessibleWorkspaceRow('workspace-1'), accessibleWorkspaceRow('workspace-2'), @@ -252,7 +259,10 @@ describe('workspace lifecycle', () => { callback(tx) ) - const result = await archiveWorkspace('workspace-1', { requestId: 'req-1' }) + const result = await archiveWorkspace('workspace-1', { + requestId: 'req-1', + provisionFallbackForStrandedMembers: true, + }) expect(result).toEqual({ archived: true, @@ -280,18 +290,20 @@ describe('workspace lifecycle', () => { callback(tx) ) - const result = await archiveWorkspace('workspace-1', { requestId: 'req-1' }) + const result = await archiveWorkspace('workspace-1', { + requestId: 'req-1', + provisionFallbackForStrandedMembers: true, + }) expect(result).toEqual({ archived: true, workspaceName: 'Workspace 1', }) expect(mockCreateWorkspaceRecord).not.toHaveBeenCalled() - // No knowledge bases found, so the two KB-dependent updates (document, knowledgeConnector) are skipped. expect(tx.update).toHaveBeenCalledTimes(8) }) - it('skips the stranded-member check and provisioning entirely when force is set (ban flow)', async () => { + it('never checks or provisions when provisionFallbackForStrandedMembers is not set (ban flow default)', async () => { mockGetWorkspaceWithOwner.mockResolvedValue({ id: 'workspace-1', name: 'Workspace 1', @@ -305,7 +317,7 @@ describe('workspace lifecycle', () => { callback(tx) ) - const result = await archiveWorkspace('workspace-1', { requestId: 'req-1', force: true }) + const result = await archiveWorkspace('workspace-1', { requestId: 'req-1' }) expect(result).toEqual({ archived: true, diff --git a/apps/sim/lib/workspaces/lifecycle.ts b/apps/sim/lib/workspaces/lifecycle.ts index 5f8bdfc6797..498b02d9946 100644 --- a/apps/sim/lib/workspaces/lifecycle.ts +++ b/apps/sim/lib/workspaces/lifecycle.ts @@ -34,11 +34,13 @@ const FALLBACK_WORKSPACE_NAME = 'My Workspace' interface ArchiveWorkspaceOptions { requestId: string /** - * Skips auto-provisioning replacement workspaces for members who'd otherwise be stranded. - * Only for account-disable flows: a banned user's owned workspaces must be fully disabled, and - * the banned user specifically should not be handed a fresh workspace as a side effect. + * Opts into auto-provisioning a replacement workspace for any member who'd otherwise be left + * with zero active workspaces. Off by default so archival stays a pure "delete this workspace" + * primitive for callers that don't ask for it (e.g. the account-disable flow, where a banned + * user's workspaces must be fully archived without handing the banned user a fresh one). + * Only the interactive DELETE route should set this. */ - force?: boolean + provisionFallbackForStrandedMembers?: boolean /** Attributed as the actor on the audit log entry for any auto-provisioned replacement workspace. */ actorId?: string actorName?: string | null @@ -112,43 +114,44 @@ export async function archiveWorkspace( // serializable: without it, two concurrent deletions sharing a sole member could each read a // pre-deletion workspace count and both skip provisioning a replacement. Postgres detects this - // write skew under serializable isolation and aborts one transaction. Skipped when force is - // set, since that path never runs the stranded-member check at all. - const transactionConfig = options.force - ? undefined - : ({ isolationLevel: 'serializable' } as const) - - let provisionedWorkspaceUserIds: string[] = [] - - await db.transaction(async (tx) => { - if (!options.force) { - const strandedUserIds = await findMembersStrandedByArchival(tx, workspaceId) - for (const userId of strandedUserIds) { - const fallbackWorkspace = await createWorkspaceRecord({ - userId, - name: FALLBACK_WORKSPACE_NAME, - organizationId: null, - workspaceMode: WORKSPACE_MODE.PERSONAL, - billedAccountUserId: userId, - executor: tx, - }) + // write skew under serializable isolation and aborts one transaction. Only needed when the + // stranded-member check actually runs. + const transactionConfig = options.provisionFallbackForStrandedMembers + ? ({ isolationLevel: 'serializable' } as const) + : undefined + + const provisionedWorkspaceUserIds = await db.transaction(async (tx) => { + if (!options.provisionFallbackForStrandedMembers) { + return [] + } + + const strandedUserIds = await findMembersStrandedByArchival(tx, workspaceId) + for (const userId of strandedUserIds) { + // Intentionally bypasses getWorkspaceCreationPolicy: this is a system-provisioned safety + // net (never blocked by "who can create a workspace" rules), not user self-service. + const fallbackWorkspace = await createWorkspaceRecord({ + userId, + name: FALLBACK_WORKSPACE_NAME, + organizationId: null, + workspaceMode: WORKSPACE_MODE.PERSONAL, + billedAccountUserId: userId, + executor: tx, + }) - if (options.actorId) { - recordAudit({ - workspaceId: fallbackWorkspace.id, - actorId: options.actorId, - actorName: options.actorName, - actorEmail: options.actorEmail, - action: AuditAction.WORKSPACE_CREATED, - resourceType: AuditResourceType.WORKSPACE, - resourceId: fallbackWorkspace.id, - resourceName: fallbackWorkspace.name, - description: `Auto-created replacement workspace "${fallbackWorkspace.name}" for a member left with no workspace after deleting "${workspaceRecord.name}"`, - metadata: { deletedWorkspaceId: workspaceId, recipientUserId: userId }, - }) - } + if (options.actorId) { + recordAudit({ + workspaceId: fallbackWorkspace.id, + actorId: options.actorId, + actorName: options.actorName, + actorEmail: options.actorEmail, + action: AuditAction.WORKSPACE_CREATED, + resourceType: AuditResourceType.WORKSPACE, + resourceId: fallbackWorkspace.id, + resourceName: fallbackWorkspace.name, + description: `Auto-created replacement workspace "${fallbackWorkspace.name}" for a member left with no workspace after deleting "${workspaceRecord.name}"`, + metadata: { deletedWorkspaceId: workspaceId, recipientUserId: userId }, + }) } - provisionedWorkspaceUserIds = strandedUserIds } await tx @@ -272,6 +275,8 @@ export async function archiveWorkspace( updatedAt: now, }) .where(and(eq(workspace.id, workspaceId), isNull(workspace.archivedAt))) + + return strandedUserIds }, transactionConfig) await archiveWorkflowsForWorkspace(workspaceId, options) From 0ebf656e9b6c313fc31648976e76b0094296705a Mon Sep 17 00:00:00 2001 From: waleed Date: Tue, 7 Jul 2026 17:51:14 -0700 Subject: [PATCH 07/12] chore(workspaces): drop redundant test comments already implied by adjacent assertions --- apps/sim/lib/workspaces/create.test.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/apps/sim/lib/workspaces/create.test.ts b/apps/sim/lib/workspaces/create.test.ts index 394daca5a83..f24d84ff9a1 100644 --- a/apps/sim/lib/workspaces/create.test.ts +++ b/apps/sim/lib/workspaces/create.test.ts @@ -60,7 +60,6 @@ describe('createWorkspaceRecord', () => { expect(record.name).toBe('My Workspace') expect(record.ownerId).toBe('user-1') expect(record.workspaceMode).toBe('personal') - // workspace insert, permission insert, workflow insert (default workflow not skipped) expect(tx.insert).toHaveBeenCalledTimes(3) expect(mockSaveWorkflowToNormalizedTables).toHaveBeenCalledTimes(1) }) @@ -94,7 +93,6 @@ describe('createWorkspaceRecord', () => { executor: tx as never, }) - // workspace insert, permission insert — no workflow insert expect(tx.insert).toHaveBeenCalledTimes(2) expect(mockSaveWorkflowToNormalizedTables).not.toHaveBeenCalled() }) From 0f8c1f46e97da168ac4a534e1888323a25de7de1 Mon Sep 17 00:00:00 2001 From: waleed Date: Tue, 7 Jul 2026 18:09:13 -0700 Subject: [PATCH 08/12] fix(workspaces): fix critical regression from the opt-in flag refactor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit's `if (!options.provisionFallbackForStrandedMembers) { return [] }` returned from the ENTIRE transaction callback, not just the stranded-member check — silently skipping every archival write (workspace.archivedAt, apiKey deletion, mcpServers, workflowSchedule, etc.) whenever the flag wasn't set. That's every caller except the interactive DELETE route, including the ban flow: banning a user would report `{ archived: true }` while leaving the workspace and all its resources fully live. Also moves the audit-log call for auto-provisioned fallback workspaces outside the transaction. recordAudit is fire-and-forget and doesn't participate in the transaction, so recording it before the transaction commits could leave a phantom audit entry pointing at a fallback workspace that got rolled back (e.g. on a serialization failure). Added a test asserting archival writes still run when the flag is off (the exact gap that let the regression through undetected), and a test proving no audit entry is recorded when the transaction subsequently fails. Both caught by Greptile review before merge. --- apps/sim/lib/workspaces/lifecycle.test.ts | 36 ++++++++++++ apps/sim/lib/workspaces/lifecycle.ts | 70 +++++++++++++---------- 2 files changed, 75 insertions(+), 31 deletions(-) diff --git a/apps/sim/lib/workspaces/lifecycle.test.ts b/apps/sim/lib/workspaces/lifecycle.test.ts index 51f24b600a8..ac428a98c0b 100644 --- a/apps/sim/lib/workspaces/lifecycle.test.ts +++ b/apps/sim/lib/workspaces/lifecycle.test.ts @@ -206,6 +206,37 @@ describe('workspace lifecycle', () => { expect(auditMockFns.mockRecordAudit).not.toHaveBeenCalled() }) + it('does not record an audit entry for a fallback workspace whose transaction subsequently fails', async () => { + mockGetWorkspaceWithOwner.mockResolvedValue({ + id: 'workspace-1', + name: 'Workspace 1', + ownerId: 'user-1', + archivedAt: null, + }) + mockListAccessibleWorkspaceRowsForUser.mockResolvedValue([ + accessibleWorkspaceRow('workspace-1'), + ]) + + const tx = createTx([{ userId: 'user-victim' }]) + mockTransaction.mockImplementation(async (callback: (trx: typeof tx) => Promise) => { + await callback(tx) + throw new Error('serialization_failure') + }) + + await expect( + archiveWorkspace('workspace-1', { + requestId: 'req-1', + provisionFallbackForStrandedMembers: true, + actorId: 'admin-1', + }) + ).rejects.toThrow('serialization_failure') + + // recordAudit must only ever be called after the transaction has committed — otherwise a + // failed transaction (e.g. a serialization abort) would leave a phantom audit entry pointing + // at a fallback workspace that was rolled back. + expect(auditMockFns.mockRecordAudit).not.toHaveBeenCalled() + }) + it('only provisions a fallback for the one member who would actually be stranded', async () => { mockGetWorkspaceWithOwner.mockResolvedValue({ id: 'workspace-1', @@ -327,6 +358,11 @@ describe('workspace lifecycle', () => { expect(mockListAccessibleWorkspaceRowsForUser).not.toHaveBeenCalled() expect(mockCreateWorkspaceRecord).not.toHaveBeenCalled() expect(mockTransaction).toHaveBeenCalledWith(expect.any(Function), undefined) + // The archival writes must still run even when fallback provisioning is skipped entirely — + // this is the exact regression a prior version of this fix introduced (an early return that + // skipped all archival writes whenever the flag was off). + expect(tx.update).toHaveBeenCalledTimes(8) + expect(tx.delete).toHaveBeenCalledTimes(1) }) it('is idempotent for already archived workspaces', async () => { diff --git a/apps/sim/lib/workspaces/lifecycle.ts b/apps/sim/lib/workspaces/lifecycle.ts index 498b02d9946..c19bc4c1d5f 100644 --- a/apps/sim/lib/workspaces/lifecycle.ts +++ b/apps/sim/lib/workspaces/lifecycle.ts @@ -120,37 +120,23 @@ export async function archiveWorkspace( ? ({ isolationLevel: 'serializable' } as const) : undefined - const provisionedWorkspaceUserIds = await db.transaction(async (tx) => { - if (!options.provisionFallbackForStrandedMembers) { - return [] - } - - const strandedUserIds = await findMembersStrandedByArchival(tx, workspaceId) - for (const userId of strandedUserIds) { - // Intentionally bypasses getWorkspaceCreationPolicy: this is a system-provisioned safety - // net (never blocked by "who can create a workspace" rules), not user self-service. - const fallbackWorkspace = await createWorkspaceRecord({ - userId, - name: FALLBACK_WORKSPACE_NAME, - organizationId: null, - workspaceMode: WORKSPACE_MODE.PERSONAL, - billedAccountUserId: userId, - executor: tx, - }) - - if (options.actorId) { - recordAudit({ - workspaceId: fallbackWorkspace.id, - actorId: options.actorId, - actorName: options.actorName, - actorEmail: options.actorEmail, - action: AuditAction.WORKSPACE_CREATED, - resourceType: AuditResourceType.WORKSPACE, - resourceId: fallbackWorkspace.id, - resourceName: fallbackWorkspace.name, - description: `Auto-created replacement workspace "${fallbackWorkspace.name}" for a member left with no workspace after deleting "${workspaceRecord.name}"`, - metadata: { deletedWorkspaceId: workspaceId, recipientUserId: userId }, + const provisionedFallbacks = await db.transaction(async (tx) => { + const fallbacks: Array<{ userId: string; workspaceId: string; name: string }> = [] + + if (options.provisionFallbackForStrandedMembers) { + const strandedUserIds = await findMembersStrandedByArchival(tx, workspaceId) + for (const userId of strandedUserIds) { + // Intentionally bypasses getWorkspaceCreationPolicy: this is a system-provisioned safety + // net (never blocked by "who can create a workspace" rules), not user self-service. + const fallbackWorkspace = await createWorkspaceRecord({ + userId, + name: FALLBACK_WORKSPACE_NAME, + organizationId: null, + workspaceMode: WORKSPACE_MODE.PERSONAL, + billedAccountUserId: userId, + executor: tx, }) + fallbacks.push({ userId, workspaceId: fallbackWorkspace.id, name: fallbackWorkspace.name }) } } @@ -276,9 +262,31 @@ export async function archiveWorkspace( }) .where(and(eq(workspace.id, workspaceId), isNull(workspace.archivedAt))) - return strandedUserIds + return fallbacks }, transactionConfig) + // Recorded only after the transaction commits — recordAudit is fire-and-forget and doesn't + // participate in the transaction, so recording it earlier could leave a phantom audit entry + // pointing at a fallback workspace that got rolled back (e.g. on a serialization failure). + if (options.actorId) { + for (const fallback of provisionedFallbacks) { + recordAudit({ + workspaceId: fallback.workspaceId, + actorId: options.actorId, + actorName: options.actorName, + actorEmail: options.actorEmail, + action: AuditAction.WORKSPACE_CREATED, + resourceType: AuditResourceType.WORKSPACE, + resourceId: fallback.workspaceId, + resourceName: fallback.name, + description: `Auto-created replacement workspace "${fallback.name}" for a member left with no workspace after deleting "${workspaceRecord.name}"`, + metadata: { deletedWorkspaceId: workspaceId, recipientUserId: fallback.userId }, + }) + } + } + + const provisionedWorkspaceUserIds = provisionedFallbacks.map((fallback) => fallback.userId) + await archiveWorkflowsForWorkspace(workspaceId, options) logger.info(`[${options.requestId}] Archived workspace ${workspaceId}`) From 398695d1cfa7fdde2c74daaf57e08a6c8dac54bc Mon Sep 17 00:00:00 2001 From: waleed Date: Tue, 7 Jul 2026 18:15:36 -0700 Subject: [PATCH 09/12] chore(workspaces): remove redundant actorId field doc, already covered by surrounding context --- apps/sim/lib/workspaces/lifecycle.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/apps/sim/lib/workspaces/lifecycle.ts b/apps/sim/lib/workspaces/lifecycle.ts index c19bc4c1d5f..a0f1fcc6525 100644 --- a/apps/sim/lib/workspaces/lifecycle.ts +++ b/apps/sim/lib/workspaces/lifecycle.ts @@ -41,7 +41,6 @@ interface ArchiveWorkspaceOptions { * Only the interactive DELETE route should set this. */ provisionFallbackForStrandedMembers?: boolean - /** Attributed as the actor on the audit log entry for any auto-provisioned replacement workspace. */ actorId?: string actorName?: string | null actorEmail?: string | null From 6f5e3a39093d2ff13d60372c6a7b1e8c71628cb8 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 7 Jul 2026 18:46:29 -0700 Subject: [PATCH 10/12] fix(workspaces): exclude banned users and include org-admin candidates in stranding check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two gaps found by Cursor review on the latest commit: - findMembersStrandedByArchival only considered users with an explicit permissions row on the workspace being deleted. An org admin who accesses that workspace purely through their organization role (no permission row at all) was never evaluated as a stranding candidate, so they could be left with zero workspaces without getting a fallback. Now unions explicit permission holders with the organization's admins/owners (via the member table + ORG_ADMIN_ROLES) before running the same accessibility check on every candidate. - A banned user who still holds a permissions row on someone else's workspace (they don't have to own it) would get a fresh fallback workspace if stranded by that workspace's deletion — even though banned users should never receive new resources. Now filters actively-banned userIds (via the existing getActivelyBannedUserIds helper, which also covers blocked emails/domains) out of the stranded list before provisioning. Added tests for both: an org admin with no explicit permission row gets provisioned when stranded, and an actively banned stranded member does not get provisioned (and gets no audit entry). --- apps/sim/lib/workspaces/lifecycle.test.ts | 88 ++++++++++++++++++++++- apps/sim/lib/workspaces/lifecycle.ts | 52 +++++++++++--- 2 files changed, 127 insertions(+), 13 deletions(-) diff --git a/apps/sim/lib/workspaces/lifecycle.test.ts b/apps/sim/lib/workspaces/lifecycle.test.ts index ac428a98c0b..47349542883 100644 --- a/apps/sim/lib/workspaces/lifecycle.test.ts +++ b/apps/sim/lib/workspaces/lifecycle.test.ts @@ -10,12 +10,14 @@ const { mockArchiveWorkflowsForWorkspace, mockListAccessibleWorkspaceRowsForUser, mockCreateWorkspaceRecord, + mockGetActivelyBannedUserIds, } = vi.hoisted(() => ({ mockSelect: vi.fn(), mockTransaction: vi.fn(), mockArchiveWorkflowsForWorkspace: vi.fn(), mockListAccessibleWorkspaceRowsForUser: vi.fn(), mockCreateWorkspaceRecord: vi.fn(), + mockGetActivelyBannedUserIds: vi.fn(), })) const mockGetWorkspaceWithOwner = permissionsMockFns.mockGetWorkspaceWithOwner @@ -41,6 +43,10 @@ vi.mock('@/lib/workspaces/create', () => ({ createWorkspaceRecord: mockCreateWorkspaceRecord, })) +vi.mock('@/lib/auth/ban', () => ({ + getActivelyBannedUserIds: mockGetActivelyBannedUserIds, +})) + vi.mock('@sim/audit', () => auditMock) import { archiveWorkspace } from './lifecycle' @@ -65,9 +71,18 @@ function accessibleWorkspaceRow(workspaceId: string) { return { workspace: { id: workspaceId }, permissionType: 'admin' as const } } -function createTx(members: Array<{ userId: string }>) { +function createTx( + members: Array<{ userId: string }>, + orgAdminMembers: Array<{ userId: string }> = [] +) { + const selectDistinct = vi.fn() + // First call is always the explicit-permissions query; the second (only reached when the + // workspace has an organizationId) is the org-admin query. + selectDistinct.mockReturnValueOnce(createMembersChain(members)) + selectDistinct.mockReturnValueOnce(createMembersChain(orgAdminMembers)) + return { - selectDistinct: vi.fn().mockReturnValue(createMembersChain(members)), + selectDistinct, select: vi.fn().mockReturnValue({ from: vi.fn().mockReturnValue({ where: vi.fn().mockResolvedValue([]), @@ -89,6 +104,7 @@ describe('workspace lifecycle', () => { }), }) mockCreateWorkspaceRecord.mockResolvedValue({ id: 'fallback-workspace', name: 'My Workspace' }) + mockGetActivelyBannedUserIds.mockResolvedValue([]) }) it('archives workspace and dependent resources under serializable isolation', async () => { @@ -303,6 +319,74 @@ describe('workspace lifecycle', () => { expect(tx.update).toHaveBeenCalledTimes(8) }) + it('provisions a fallback for an org admin who is stranded but has no explicit permission row', async () => { + mockGetWorkspaceWithOwner.mockResolvedValue({ + id: 'workspace-1', + name: 'Workspace 1', + ownerId: 'user-1', + organizationId: 'org-1', + archivedAt: null, + }) + mockArchiveWorkflowsForWorkspace.mockResolvedValue(0) + // The org admin has no row in `permissions` for this workspace at all — they only appear as + // an org-admin candidate. Their only accessible workspace is this one, so they're stranded. + mockListAccessibleWorkspaceRowsForUser.mockResolvedValue([ + accessibleWorkspaceRow('workspace-1'), + ]) + + const tx = createTx([], [{ userId: 'user-org-admin-no-row' }]) + mockTransaction.mockImplementation(async (callback: (trx: typeof tx) => Promise) => + callback(tx) + ) + + const result = await archiveWorkspace('workspace-1', { + requestId: 'req-1', + provisionFallbackForStrandedMembers: true, + }) + + expect(result).toEqual({ + archived: true, + workspaceName: 'Workspace 1', + provisionedWorkspaceUserIds: ['user-org-admin-no-row'], + }) + expect(tx.selectDistinct).toHaveBeenCalledTimes(2) + expect(mockCreateWorkspaceRecord).toHaveBeenCalledWith( + expect.objectContaining({ userId: 'user-org-admin-no-row' }) + ) + }) + + it('does not provision a fallback for an actively banned stranded member', async () => { + mockGetWorkspaceWithOwner.mockResolvedValue({ + id: 'workspace-1', + name: 'Workspace 1', + ownerId: 'user-1', + archivedAt: null, + }) + mockArchiveWorkflowsForWorkspace.mockResolvedValue(0) + mockListAccessibleWorkspaceRowsForUser.mockResolvedValue([ + accessibleWorkspaceRow('workspace-1'), + ]) + mockGetActivelyBannedUserIds.mockResolvedValue(['user-banned']) + + const tx = createTx([{ userId: 'user-banned' }]) + mockTransaction.mockImplementation(async (callback: (trx: typeof tx) => Promise) => + callback(tx) + ) + + const result = await archiveWorkspace('workspace-1', { + requestId: 'req-1', + provisionFallbackForStrandedMembers: true, + actorId: 'admin-1', + }) + + expect(result).toEqual({ + archived: true, + workspaceName: 'Workspace 1', + }) + expect(mockCreateWorkspaceRecord).not.toHaveBeenCalled() + expect(auditMockFns.mockRecordAudit).not.toHaveBeenCalled() + }) + it('proceeds without provisioning when every member has another active workspace', async () => { mockGetWorkspaceWithOwner.mockResolvedValue({ id: 'workspace-1', diff --git a/apps/sim/lib/workspaces/lifecycle.ts b/apps/sim/lib/workspaces/lifecycle.ts index a0f1fcc6525..66d9f8ad395 100644 --- a/apps/sim/lib/workspaces/lifecycle.ts +++ b/apps/sim/lib/workspaces/lifecycle.ts @@ -8,6 +8,7 @@ import { knowledgeBase, knowledgeConnector, mcpServers, + member, permissions, userTableDefinitions, workflowMcpServer, @@ -16,7 +17,9 @@ import { workspaceFiles, } from '@sim/db/schema' import { createLogger } from '@sim/logger' +import { ORG_ADMIN_ROLES } from '@sim/platform-authz/workspace' import { and, eq, inArray, isNull, sql } from 'drizzle-orm' +import { getActivelyBannedUserIds } from '@/lib/auth/ban' import type { DbOrTx } from '@/lib/db/types' import { mcpPubSub } from '@/lib/mcp/pubsub' import { mcpService } from '@/lib/mcp/service' @@ -55,11 +58,14 @@ interface ArchiveWorkspaceResult { } /** - * Returns the userIds of explicit workspace members for whom `workspaceId` is their only - * accessible active (non-archived) workspace. "Accessible" includes workspaces granted through - * an explicit permission row AND workspaces derived from organization owner/admin role — an org - * admin can always fall back to the rest of the organization's workspaces even without an - * explicit permission row on any of them, so they are never stranded by this deletion. + * Returns the userIds who would be left with zero accessible active (non-archived) workspaces if + * `workspaceId` were archived. Candidates are the union of explicit workspace members AND the + * organization's admins/owners — an org admin can access a workspace purely through their org + * role with no permission row at all, so they must be checked even though they never show up as + * an explicit member. "Accessible" (via `listAccessibleWorkspaceRowsForUser`) already accounts for + * that same org-admin-derived access when deciding whether a candidate has another workspace to + * fall back to. Actively banned users are excluded from the result — they should never receive a + * new resource as a side effect of someone else's action. * * Must be called against the same executor used to perform the archival, under * `serializable` isolation, so the check and the write are atomic with respect to a concurrent @@ -67,19 +73,34 @@ interface ArchiveWorkspaceResult { */ async function findMembersStrandedByArchival( executor: DbOrTx, - workspaceId: string + workspaceId: string, + organizationId: string | null ): Promise { - const members = await executor + const explicitMembers = await executor .selectDistinct({ userId: permissions.userId }) .from(permissions) .where(and(eq(permissions.entityId, workspaceId), eq(permissions.entityType, 'workspace'))) - if (members.length === 0) { + const candidateUserIds = new Set(explicitMembers.map((row) => row.userId)) + + if (organizationId) { + const orgAdmins = await executor + .selectDistinct({ userId: member.userId }) + .from(member) + .where( + and(eq(member.organizationId, organizationId), inArray(member.role, [...ORG_ADMIN_ROLES])) + ) + for (const { userId } of orgAdmins) { + candidateUserIds.add(userId) + } + } + + if (candidateUserIds.size === 0) { return [] } const strandedUserIds: string[] = [] - for (const { userId } of members) { + for (const userId of candidateUserIds) { const accessible = await listAccessibleWorkspaceRowsForUser(userId, 'active', executor) const hasOtherWorkspace = accessible.some((row) => row.workspace.id !== workspaceId) if (!hasOtherWorkspace) { @@ -87,7 +108,12 @@ async function findMembersStrandedByArchival( } } - return strandedUserIds + if (strandedUserIds.length === 0) { + return [] + } + + const bannedUserIds = new Set(await getActivelyBannedUserIds(strandedUserIds)) + return strandedUserIds.filter((userId) => !bannedUserIds.has(userId)) } export async function archiveWorkspace( @@ -123,7 +149,11 @@ export async function archiveWorkspace( const fallbacks: Array<{ userId: string; workspaceId: string; name: string }> = [] if (options.provisionFallbackForStrandedMembers) { - const strandedUserIds = await findMembersStrandedByArchival(tx, workspaceId) + const strandedUserIds = await findMembersStrandedByArchival( + tx, + workspaceId, + workspaceRecord.organizationId + ) for (const userId of strandedUserIds) { // Intentionally bypasses getWorkspaceCreationPolicy: this is a system-provisioned safety // net (never blocked by "who can create a workspace" rules), not user self-service. From 277e794af1dc573dfaea2cdc9ddaed1adc773c89 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 8 Jul 2026 20:04:19 -0700 Subject: [PATCH 11/12] fix(workspaces): defer telemetry until commit, avoid second pooled connection mid-transaction createWorkspaceRecord now only fires workspaceCreated telemetry when it commits its own transaction; callers passing a nested executor (the fallback-provisioning path) fire it themselves after their outer transaction commits, avoiding a phantom event for a workspace that gets rolled back. getActivelyBannedUserIds now accepts an executor so the stranded-member check runs the banned-user lookup against the open transaction instead of borrowing a second pooled connection while the first sits idle-in-transaction. --- apps/sim/app/api/workspaces/route.ts | 11 ---------- apps/sim/lib/auth/ban.ts | 13 ++++++++++-- apps/sim/lib/workspaces/create.test.ts | 24 ++++++++++++++++++---- apps/sim/lib/workspaces/create.ts | 17 +++++++++++++++ apps/sim/lib/workspaces/lifecycle.test.ts | 19 ++++++++++++++--- apps/sim/lib/workspaces/lifecycle.ts | 25 +++++++++++++++++------ 6 files changed, 83 insertions(+), 26 deletions(-) diff --git a/apps/sim/app/api/workspaces/route.ts b/apps/sim/app/api/workspaces/route.ts index da438d4ac85..e8cc520609c 100644 --- a/apps/sim/app/api/workspaces/route.ts +++ b/apps/sim/app/api/workspaces/route.ts @@ -9,7 +9,6 @@ import { createWorkspaceContract } from '@/lib/api/contracts/workspaces' import { parseRequest } from '@/lib/api/server' import { getSession } from '@/lib/auth' import type { PlanCategory } from '@/lib/billing/plan-helpers' -import { PlatformEvents } from '@/lib/core/telemetry' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { captureServerEvent } from '@/lib/posthog/server' import { type CreateWorkspaceRecordParams, createWorkspaceRecord } from '@/lib/workspaces/create' @@ -282,16 +281,6 @@ async function createWorkspace({ billedAccountUserId, }) - try { - PlatformEvents.workspaceCreated({ - workspaceId: record.id, - userId, - name, - }) - } catch { - // Telemetry should not fail the operation - } - const invitePolicy = await getWorkspaceInvitePolicy({ organizationId, workspaceMode, diff --git a/apps/sim/lib/auth/ban.ts b/apps/sim/lib/auth/ban.ts index 8ca733d2994..621c675e422 100644 --- a/apps/sim/lib/auth/ban.ts +++ b/apps/sim/lib/auth/ban.ts @@ -1,6 +1,7 @@ import { db, user } from '@sim/db' import { inArray, sql } from 'drizzle-orm' import { getAccessControlConfig, isEmailBlockedByAccessControl } from '@/lib/auth/access-control' +import type { DbOrTx } from '@/lib/db/types' /** * True when a ban is currently in effect. Mirrors better-auth admin-plugin @@ -34,14 +35,22 @@ export async function isEmailBlocked(email: string | null | undefined): Promise< * active account ban, or an email/domain in the appconfig blocked lists. * One user query plus the cached access-control fetch. Throws on db * failure — callers must fail closed. + * + * Accepts an optional executor so callers already inside a transaction (e.g. a + * workspace-archival safety check under `serializable` isolation) can run this + * against `tx` instead of borrowing a second pooled connection while the first + * sits idle-in-transaction. */ -export async function getActivelyBannedUserIds(userIds: string[]): Promise { +export async function getActivelyBannedUserIds( + userIds: string[], + executor: DbOrTx = db +): Promise { const ids = [...new Set(userIds.filter(Boolean))] if (ids.length === 0) return [] const [accessControl, rows] = await Promise.all([ getAccessControlConfig(), - db + executor .select({ id: user.id, email: user.email, banned: user.banned, banExpires: user.banExpires }) .from(user) .where(inArray(user.id, ids)), diff --git a/apps/sim/lib/workspaces/create.test.ts b/apps/sim/lib/workspaces/create.test.ts index f24d84ff9a1..fa1922b170a 100644 --- a/apps/sim/lib/workspaces/create.test.ts +++ b/apps/sim/lib/workspaces/create.test.ts @@ -3,10 +3,12 @@ */ import { beforeEach, describe, expect, it, vi } from 'vitest' -const { mockTransaction, mockSaveWorkflowToNormalizedTables } = vi.hoisted(() => ({ - mockTransaction: vi.fn(), - mockSaveWorkflowToNormalizedTables: vi.fn(), -})) +const { mockTransaction, mockSaveWorkflowToNormalizedTables, mockWorkspaceCreatedEvent } = + vi.hoisted(() => ({ + mockTransaction: vi.fn(), + mockSaveWorkflowToNormalizedTables: vi.fn(), + mockWorkspaceCreatedEvent: vi.fn(), + })) vi.mock('@sim/db', () => ({ db: { @@ -14,6 +16,10 @@ vi.mock('@sim/db', () => ({ }, })) +vi.mock('@/lib/core/telemetry', () => ({ + PlatformEvents: { workspaceCreated: mockWorkspaceCreatedEvent }, +})) + vi.mock('@/lib/workflows/defaults', () => ({ buildDefaultWorkflowArtifacts: () => ({ workflowState: { blocks: {}, edges: [] } }), })) @@ -62,6 +68,12 @@ describe('createWorkspaceRecord', () => { expect(record.workspaceMode).toBe('personal') expect(tx.insert).toHaveBeenCalledTimes(3) expect(mockSaveWorkflowToNormalizedTables).toHaveBeenCalledTimes(1) + // Safe to fire immediately: this call committed its own transaction before returning. + expect(mockWorkspaceCreatedEvent).toHaveBeenCalledWith({ + workspaceId: record.id, + userId: 'user-1', + name: 'My Workspace', + }) }) it('runs directly against a provided executor instead of opening a nested transaction', async () => { @@ -78,6 +90,10 @@ describe('createWorkspaceRecord', () => { expect(mockTransaction).not.toHaveBeenCalled() expect(tx.insert).toHaveBeenCalledTimes(3) + // Must NOT fire here: the caller's outer transaction hasn't committed yet and could still + // roll back, which would make this a phantom event for a workspace that never existed. The + // caller owns firing this once its own transaction commits. + expect(mockWorkspaceCreatedEvent).not.toHaveBeenCalled() }) it('skips the default workflow insert when skipDefaultWorkflow is set', async () => { diff --git a/apps/sim/lib/workspaces/create.ts b/apps/sim/lib/workspaces/create.ts index 490d69a5194..93b77eecede 100644 --- a/apps/sim/lib/workspaces/create.ts +++ b/apps/sim/lib/workspaces/create.ts @@ -2,6 +2,7 @@ import { db } from '@sim/db' import { permissions, type WorkspaceMode, workflow, workspace } from '@sim/db/schema' import { createLogger } from '@sim/logger' import { generateId } from '@sim/utils/id' +import { PlatformEvents } from '@/lib/core/telemetry' import type { DbOrTx } from '@/lib/db/types' import { buildDefaultWorkflowArtifacts } from '@/lib/workflows/defaults' import { saveWorkflowToNormalizedTables } from '@/lib/workflows/persistence/utils' @@ -44,6 +45,10 @@ export interface CreatedWorkspaceRecord { * (unless skipped) a default starter workflow. Shared by the `POST /api/workspaces` route and * the workspace-archival safety net that auto-provisions a replacement workspace for a member * who would otherwise be left with zero workspaces. + * + * Fires the `workspaceCreated` telemetry event itself only when it manages its own transaction + * (no `executor` passed). Callers that pass `executor` are joining an outer transaction that can + * still roll back after this returns, so they own firing that event once their transaction commits. */ export async function createWorkspaceRecord({ userId, @@ -142,6 +147,18 @@ export async function createWorkspaceRecord({ throw error } + // Only fire when this call committed its own transaction. When `executor` is a caller-supplied + // `tx`, the insert isn't durable yet — the caller's transaction can still roll back after this + // function returns, so firing here would risk a phantom event for a workspace that never + // existed. Callers that pass `executor` must fire this themselves once their transaction commits. + if (!executor) { + try { + PlatformEvents.workspaceCreated({ workspaceId, userId, name }) + } catch { + // Telemetry should not fail the operation + } + } + return { id: workspaceId, name, diff --git a/apps/sim/lib/workspaces/lifecycle.test.ts b/apps/sim/lib/workspaces/lifecycle.test.ts index 47349542883..3fc6a1fe8f9 100644 --- a/apps/sim/lib/workspaces/lifecycle.test.ts +++ b/apps/sim/lib/workspaces/lifecycle.test.ts @@ -11,6 +11,7 @@ const { mockListAccessibleWorkspaceRowsForUser, mockCreateWorkspaceRecord, mockGetActivelyBannedUserIds, + mockWorkspaceCreatedEvent, } = vi.hoisted(() => ({ mockSelect: vi.fn(), mockTransaction: vi.fn(), @@ -18,6 +19,7 @@ const { mockListAccessibleWorkspaceRowsForUser: vi.fn(), mockCreateWorkspaceRecord: vi.fn(), mockGetActivelyBannedUserIds: vi.fn(), + mockWorkspaceCreatedEvent: vi.fn(), })) const mockGetWorkspaceWithOwner = permissionsMockFns.mockGetWorkspaceWithOwner @@ -47,6 +49,10 @@ vi.mock('@/lib/auth/ban', () => ({ getActivelyBannedUserIds: mockGetActivelyBannedUserIds, })) +vi.mock('@/lib/core/telemetry', () => ({ + PlatformEvents: { workspaceCreated: mockWorkspaceCreatedEvent }, +})) + vi.mock('@sim/audit', () => auditMock) import { archiveWorkspace } from './lifecycle' @@ -192,6 +198,11 @@ describe('workspace lifecycle', () => { }), }) ) + expect(mockWorkspaceCreatedEvent).toHaveBeenCalledWith({ + workspaceId: 'fallback-workspace', + userId: 'user-victim', + name: 'My Workspace', + }) expect(tx.update).toHaveBeenCalledTimes(8) expect(tx.delete).toHaveBeenCalledTimes(1) }) @@ -247,10 +258,12 @@ describe('workspace lifecycle', () => { }) ).rejects.toThrow('serialization_failure') - // recordAudit must only ever be called after the transaction has committed — otherwise a - // failed transaction (e.g. a serialization abort) would leave a phantom audit entry pointing - // at a fallback workspace that was rolled back. + // recordAudit and the workspaceCreated telemetry event must only ever fire after the + // transaction has committed — otherwise a failed transaction (e.g. a serialization abort) + // would leave a phantom audit entry / event pointing at a fallback workspace that was + // rolled back. expect(auditMockFns.mockRecordAudit).not.toHaveBeenCalled() + expect(mockWorkspaceCreatedEvent).not.toHaveBeenCalled() }) it('only provisions a fallback for the one member who would actually be stranded', async () => { diff --git a/apps/sim/lib/workspaces/lifecycle.ts b/apps/sim/lib/workspaces/lifecycle.ts index 66d9f8ad395..7f387711c73 100644 --- a/apps/sim/lib/workspaces/lifecycle.ts +++ b/apps/sim/lib/workspaces/lifecycle.ts @@ -20,6 +20,7 @@ import { createLogger } from '@sim/logger' import { ORG_ADMIN_ROLES } from '@sim/platform-authz/workspace' import { and, eq, inArray, isNull, sql } from 'drizzle-orm' import { getActivelyBannedUserIds } from '@/lib/auth/ban' +import { PlatformEvents } from '@/lib/core/telemetry' import type { DbOrTx } from '@/lib/db/types' import { mcpPubSub } from '@/lib/mcp/pubsub' import { mcpService } from '@/lib/mcp/service' @@ -112,7 +113,7 @@ async function findMembersStrandedByArchival( return [] } - const bannedUserIds = new Set(await getActivelyBannedUserIds(strandedUserIds)) + const bannedUserIds = new Set(await getActivelyBannedUserIds(strandedUserIds, executor)) return strandedUserIds.filter((userId) => !bannedUserIds.has(userId)) } @@ -294,11 +295,23 @@ export async function archiveWorkspace( return fallbacks }, transactionConfig) - // Recorded only after the transaction commits — recordAudit is fire-and-forget and doesn't - // participate in the transaction, so recording it earlier could leave a phantom audit entry - // pointing at a fallback workspace that got rolled back (e.g. on a serialization failure). - if (options.actorId) { - for (const fallback of provisionedFallbacks) { + // Recorded/fired only after the transaction commits — recordAudit and the telemetry event are + // fire-and-forget and don't participate in the transaction, so triggering them earlier could + // leave a phantom audit entry / event pointing at a fallback workspace that got rolled back + // (e.g. on a serialization failure). `createWorkspaceRecord` defers its own `workspaceCreated` + // event for exactly this reason when given an `executor` — this is where it gets fired instead. + for (const fallback of provisionedFallbacks) { + try { + PlatformEvents.workspaceCreated({ + workspaceId: fallback.workspaceId, + userId: fallback.userId, + name: fallback.name, + }) + } catch { + // Telemetry should not fail the operation + } + + if (options.actorId) { recordAudit({ workspaceId: fallback.workspaceId, actorId: options.actorId, From c6be4cff9f6a0a052eef2e11766cd719d46161a5 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 8 Jul 2026 20:12:57 -0700 Subject: [PATCH 12/12] fix(workspaces): protect non-banned co-members from stranding in the ban flow; trim redundant comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit disableUserResources now opts into provisionFallbackForStrandedMembers. Actively banned users are already excluded from receiving a fallback (findMembersStrandedByArchival filters them out), so this only protects a non-banned co-member of a banned owner's workspace from the same stranding bug this PR fixes for interactive deletion — previously they had no safety net at all. Also trims several inline comments that only restated what the adjacent assertion or test title already made clear, splitting a couple of telemetry assertions into their own descriptively-named tests instead of narrating them inline. --- apps/sim/lib/workflows/lifecycle.test.ts | 6 +-- apps/sim/lib/workflows/lifecycle.ts | 10 ++++- apps/sim/lib/workspaces/create.test.ts | 40 ++++++++++++++---- apps/sim/lib/workspaces/create.ts | 4 -- apps/sim/lib/workspaces/lifecycle.test.ts | 50 +++++++++++++++++------ apps/sim/lib/workspaces/lifecycle.ts | 7 ++-- 6 files changed, 85 insertions(+), 32 deletions(-) diff --git a/apps/sim/lib/workflows/lifecycle.test.ts b/apps/sim/lib/workflows/lifecycle.test.ts index 7ed2f5455db..c63a08f590f 100644 --- a/apps/sim/lib/workflows/lifecycle.test.ts +++ b/apps/sim/lib/workflows/lifecycle.test.ts @@ -147,7 +147,7 @@ describe('disableUserResources', () => { vi.clearAllMocks() }) - it('archives every owned workspace without opting into fallback provisioning, so banning is never blocked by other members', async () => { + it('archives every owned workspace with fallback provisioning opted in, so a non-banned co-member is not stranded', async () => { mockSelect.mockReturnValue(createSelectChain([{ id: 'workspace-1' }, { id: 'workspace-2' }])) mockDelete.mockReturnValue({ where: vi.fn().mockResolvedValue([]) }) mockArchiveWorkspace.mockResolvedValue({ archived: true, workspaceName: 'Workspace' }) @@ -157,11 +157,11 @@ describe('disableUserResources', () => { expect(mockArchiveWorkspace).toHaveBeenCalledTimes(2) expect(mockArchiveWorkspace).toHaveBeenCalledWith( 'workspace-1', - expect.not.objectContaining({ provisionFallbackForStrandedMembers: true }) + expect.objectContaining({ provisionFallbackForStrandedMembers: true }) ) expect(mockArchiveWorkspace).toHaveBeenCalledWith( 'workspace-2', - expect.not.objectContaining({ provisionFallbackForStrandedMembers: true }) + expect.objectContaining({ provisionFallbackForStrandedMembers: true }) ) expect(mockDelete).toHaveBeenCalled() }) diff --git a/apps/sim/lib/workflows/lifecycle.ts b/apps/sim/lib/workflows/lifecycle.ts index 82040e2750e..0ccd8c942b8 100644 --- a/apps/sim/lib/workflows/lifecycle.ts +++ b/apps/sim/lib/workflows/lifecycle.ts @@ -346,7 +346,11 @@ export async function archiveWorkflowsByIdsInWorkspace( /** * Disables all resources owned by a banned user by archiving every workspace * they own (cascading to workflows, chats, KBs, tables, files, etc.) - * and deleting their personal API keys. + * and deleting their personal API keys. Still opts into stranded-member + * fallback provisioning: the banned owner is excluded from receiving a + * fallback (`findMembersStrandedByArchival` filters actively banned users), + * but a non-banned co-member of an owned workspace can still be stranded by + * the ban and must get the same safety net as an interactive deletion. */ export async function disableUserResources(userId: string): Promise { const requestId = generateRequestId() @@ -360,7 +364,9 @@ export async function disableUserResources(userId: string): Promise { .where(and(eq(workspace.ownerId, userId), isNull(workspace.archivedAt))) await Promise.all([ - ...ownedWorkspaces.map((w) => archiveWorkspace(w.id, { requestId })), + ...ownedWorkspaces.map((w) => + archiveWorkspace(w.id, { requestId, provisionFallbackForStrandedMembers: true }) + ), db.delete(apiKey).where(eq(apiKey.userId, userId)), ]) diff --git a/apps/sim/lib/workspaces/create.test.ts b/apps/sim/lib/workspaces/create.test.ts index fa1922b170a..e7cd6ce9352 100644 --- a/apps/sim/lib/workspaces/create.test.ts +++ b/apps/sim/lib/workspaces/create.test.ts @@ -68,7 +68,38 @@ describe('createWorkspaceRecord', () => { expect(record.workspaceMode).toBe('personal') expect(tx.insert).toHaveBeenCalledTimes(3) expect(mockSaveWorkflowToNormalizedTables).toHaveBeenCalledTimes(1) - // Safe to fire immediately: this call committed its own transaction before returning. + }) + + it('runs directly against a provided executor instead of opening a nested transaction', async () => { + const tx = createInsertOnlyTx() + + await createWorkspaceRecord({ + userId: 'user-1', + name: 'My Workspace', + organizationId: null, + workspaceMode: 'personal', + billedAccountUserId: 'user-1', + executor: tx as never, + }) + + expect(mockTransaction).not.toHaveBeenCalled() + expect(tx.insert).toHaveBeenCalledTimes(3) + }) + + it('fires the workspaceCreated event once its own transaction commits', async () => { + const tx = createInsertOnlyTx() + mockTransaction.mockImplementation(async (callback: (trx: typeof tx) => Promise) => + callback(tx) + ) + + const record = await createWorkspaceRecord({ + userId: 'user-1', + name: 'My Workspace', + organizationId: null, + workspaceMode: 'personal', + billedAccountUserId: 'user-1', + }) + expect(mockWorkspaceCreatedEvent).toHaveBeenCalledWith({ workspaceId: record.id, userId: 'user-1', @@ -76,7 +107,7 @@ describe('createWorkspaceRecord', () => { }) }) - it('runs directly against a provided executor instead of opening a nested transaction', async () => { + it('does not fire the workspaceCreated event when given a caller-owned executor', async () => { const tx = createInsertOnlyTx() await createWorkspaceRecord({ @@ -88,11 +119,6 @@ describe('createWorkspaceRecord', () => { executor: tx as never, }) - expect(mockTransaction).not.toHaveBeenCalled() - expect(tx.insert).toHaveBeenCalledTimes(3) - // Must NOT fire here: the caller's outer transaction hasn't committed yet and could still - // roll back, which would make this a phantom event for a workspace that never existed. The - // caller owns firing this once its own transaction commits. expect(mockWorkspaceCreatedEvent).not.toHaveBeenCalled() }) diff --git a/apps/sim/lib/workspaces/create.ts b/apps/sim/lib/workspaces/create.ts index 93b77eecede..893c961b82e 100644 --- a/apps/sim/lib/workspaces/create.ts +++ b/apps/sim/lib/workspaces/create.ts @@ -147,10 +147,6 @@ export async function createWorkspaceRecord({ throw error } - // Only fire when this call committed its own transaction. When `executor` is a caller-supplied - // `tx`, the insert isn't durable yet — the caller's transaction can still roll back after this - // function returns, so firing here would risk a phantom event for a workspace that never - // existed. Callers that pass `executor` must fire this themselves once their transaction commits. if (!executor) { try { PlatformEvents.workspaceCreated({ workspaceId, userId, name }) diff --git a/apps/sim/lib/workspaces/lifecycle.test.ts b/apps/sim/lib/workspaces/lifecycle.test.ts index 3fc6a1fe8f9..dee447fa0d4 100644 --- a/apps/sim/lib/workspaces/lifecycle.test.ts +++ b/apps/sim/lib/workspaces/lifecycle.test.ts @@ -82,8 +82,7 @@ function createTx( orgAdminMembers: Array<{ userId: string }> = [] ) { const selectDistinct = vi.fn() - // First call is always the explicit-permissions query; the second (only reached when the - // workspace has an organizationId) is the org-admin query. + // Mocked in call order: explicit-permissions query, then org-admin query. selectDistinct.mockReturnValueOnce(createMembersChain(members)) selectDistinct.mockReturnValueOnce(createMembersChain(orgAdminMembers)) @@ -233,7 +232,7 @@ describe('workspace lifecycle', () => { expect(auditMockFns.mockRecordAudit).not.toHaveBeenCalled() }) - it('does not record an audit entry for a fallback workspace whose transaction subsequently fails', async () => { + it('does not record an audit entry or fire the workspaceCreated event for a fallback workspace whose transaction subsequently fails', async () => { mockGetWorkspaceWithOwner.mockResolvedValue({ id: 'workspace-1', name: 'Workspace 1', @@ -258,10 +257,6 @@ describe('workspace lifecycle', () => { }) ).rejects.toThrow('serialization_failure') - // recordAudit and the workspaceCreated telemetry event must only ever fire after the - // transaction has committed — otherwise a failed transaction (e.g. a serialization abort) - // would leave a phantom audit entry / event pointing at a fallback workspace that was - // rolled back. expect(auditMockFns.mockRecordAudit).not.toHaveBeenCalled() expect(mockWorkspaceCreatedEvent).not.toHaveBeenCalled() }) @@ -341,8 +336,6 @@ describe('workspace lifecycle', () => { archivedAt: null, }) mockArchiveWorkflowsForWorkspace.mockResolvedValue(0) - // The org admin has no row in `permissions` for this workspace at all — they only appear as - // an org-admin candidate. Their only accessible workspace is this one, so they're stranded. mockListAccessibleWorkspaceRowsForUser.mockResolvedValue([ accessibleWorkspaceRow('workspace-1'), ]) @@ -400,6 +393,40 @@ describe('workspace lifecycle', () => { expect(auditMockFns.mockRecordAudit).not.toHaveBeenCalled() }) + it('provisions a fallback for a non-banned stranded co-member while excluding the banned owner in the same run', async () => { + mockGetWorkspaceWithOwner.mockResolvedValue({ + id: 'workspace-1', + name: 'Workspace 1', + ownerId: 'user-banned', + archivedAt: null, + }) + mockArchiveWorkflowsForWorkspace.mockResolvedValue(0) + mockListAccessibleWorkspaceRowsForUser.mockResolvedValue([ + accessibleWorkspaceRow('workspace-1'), + ]) + mockGetActivelyBannedUserIds.mockResolvedValue(['user-banned']) + + const tx = createTx([{ userId: 'user-banned' }, { userId: 'user-cohort' }]) + mockTransaction.mockImplementation(async (callback: (trx: typeof tx) => Promise) => + callback(tx) + ) + + const result = await archiveWorkspace('workspace-1', { + requestId: 'req-1', + provisionFallbackForStrandedMembers: true, + }) + + expect(result).toEqual({ + archived: true, + workspaceName: 'Workspace 1', + provisionedWorkspaceUserIds: ['user-cohort'], + }) + expect(mockCreateWorkspaceRecord).toHaveBeenCalledTimes(1) + expect(mockCreateWorkspaceRecord).toHaveBeenCalledWith( + expect.objectContaining({ userId: 'user-cohort' }) + ) + }) + it('proceeds without provisioning when every member has another active workspace', async () => { mockGetWorkspaceWithOwner.mockResolvedValue({ id: 'workspace-1', @@ -431,7 +458,7 @@ describe('workspace lifecycle', () => { expect(tx.update).toHaveBeenCalledTimes(8) }) - it('never checks or provisions when provisionFallbackForStrandedMembers is not set (ban flow default)', async () => { + it('never checks or provisions when provisionFallbackForStrandedMembers is not set, but still performs the archival writes (ban flow default)', async () => { mockGetWorkspaceWithOwner.mockResolvedValue({ id: 'workspace-1', name: 'Workspace 1', @@ -455,9 +482,6 @@ describe('workspace lifecycle', () => { expect(mockListAccessibleWorkspaceRowsForUser).not.toHaveBeenCalled() expect(mockCreateWorkspaceRecord).not.toHaveBeenCalled() expect(mockTransaction).toHaveBeenCalledWith(expect.any(Function), undefined) - // The archival writes must still run even when fallback provisioning is skipped entirely — - // this is the exact regression a prior version of this fix introduced (an early return that - // skipped all archival writes whenever the flag was off). expect(tx.update).toHaveBeenCalledTimes(8) expect(tx.delete).toHaveBeenCalledTimes(1) }) diff --git a/apps/sim/lib/workspaces/lifecycle.ts b/apps/sim/lib/workspaces/lifecycle.ts index 7f387711c73..1d3fd7a643b 100644 --- a/apps/sim/lib/workspaces/lifecycle.ts +++ b/apps/sim/lib/workspaces/lifecycle.ts @@ -40,9 +40,10 @@ interface ArchiveWorkspaceOptions { /** * Opts into auto-provisioning a replacement workspace for any member who'd otherwise be left * with zero active workspaces. Off by default so archival stays a pure "delete this workspace" - * primitive for callers that don't ask for it (e.g. the account-disable flow, where a banned - * user's workspaces must be fully archived without handing the banned user a fresh one). - * Only the interactive DELETE route should set this. + * primitive for callers that don't need it. Safe to combine with a banned owner: actively banned + * users are excluded from receiving a fallback regardless of this flag (see + * `findMembersStrandedByArchival`), so the account-disable flow also sets this to protect any + * non-banned co-member of the banned owner's workspace. */ provisionFallbackForStrandedMembers?: boolean actorId?: string