fix(workspaces): protect non-banned co-members from stranding in the ban flow; trim redundant comments

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.

Author: waleedlatif1

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

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<void> {
   const requestId = generateRequestId()
@@ -360,7 +364,9 @@ export async function disableUserResources(userId: string): Promise<void> {
     .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)),
   ])
 

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

@@ -68,15 +68,46 @@ 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<void>) =>
+      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',
       name: 'My Workspace',
     })
   })
 
-  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()
   })
 

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

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

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