Skip to content

Commit b08ecbb

Browse files
committed
fix(workspaces): fix critical regression from the opt-in flag refactor
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.
1 parent c901420 commit b08ecbb

2 files changed

Lines changed: 75 additions & 31 deletions

File tree

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

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,37 @@ describe('workspace lifecycle', () => {
206206
expect(auditMockFns.mockRecordAudit).not.toHaveBeenCalled()
207207
})
208208

209+
it('does not record an audit entry for a fallback workspace whose transaction subsequently fails', async () => {
210+
mockGetWorkspaceWithOwner.mockResolvedValue({
211+
id: 'workspace-1',
212+
name: 'Workspace 1',
213+
ownerId: 'user-1',
214+
archivedAt: null,
215+
})
216+
mockListAccessibleWorkspaceRowsForUser.mockResolvedValue([
217+
accessibleWorkspaceRow('workspace-1'),
218+
])
219+
220+
const tx = createTx([{ userId: 'user-victim' }])
221+
mockTransaction.mockImplementation(async (callback: (trx: typeof tx) => Promise<void>) => {
222+
await callback(tx)
223+
throw new Error('serialization_failure')
224+
})
225+
226+
await expect(
227+
archiveWorkspace('workspace-1', {
228+
requestId: 'req-1',
229+
provisionFallbackForStrandedMembers: true,
230+
actorId: 'admin-1',
231+
})
232+
).rejects.toThrow('serialization_failure')
233+
234+
// recordAudit must only ever be called after the transaction has committed — otherwise a
235+
// failed transaction (e.g. a serialization abort) would leave a phantom audit entry pointing
236+
// at a fallback workspace that was rolled back.
237+
expect(auditMockFns.mockRecordAudit).not.toHaveBeenCalled()
238+
})
239+
209240
it('only provisions a fallback for the one member who would actually be stranded', async () => {
210241
mockGetWorkspaceWithOwner.mockResolvedValue({
211242
id: 'workspace-1',
@@ -327,6 +358,11 @@ describe('workspace lifecycle', () => {
327358
expect(mockListAccessibleWorkspaceRowsForUser).not.toHaveBeenCalled()
328359
expect(mockCreateWorkspaceRecord).not.toHaveBeenCalled()
329360
expect(mockTransaction).toHaveBeenCalledWith(expect.any(Function), undefined)
361+
// The archival writes must still run even when fallback provisioning is skipped entirely —
362+
// this is the exact regression a prior version of this fix introduced (an early return that
363+
// skipped all archival writes whenever the flag was off).
364+
expect(tx.update).toHaveBeenCalledTimes(8)
365+
expect(tx.delete).toHaveBeenCalledTimes(1)
330366
})
331367

332368
it('is idempotent for already archived workspaces', async () => {

apps/sim/lib/workspaces/lifecycle.ts

Lines changed: 39 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -120,37 +120,23 @@ export async function archiveWorkspace(
120120
? ({ isolationLevel: 'serializable' } as const)
121121
: undefined
122122

123-
const provisionedWorkspaceUserIds = await db.transaction(async (tx) => {
124-
if (!options.provisionFallbackForStrandedMembers) {
125-
return []
126-
}
127-
128-
const strandedUserIds = await findMembersStrandedByArchival(tx, workspaceId)
129-
for (const userId of strandedUserIds) {
130-
// Intentionally bypasses getWorkspaceCreationPolicy: this is a system-provisioned safety
131-
// net (never blocked by "who can create a workspace" rules), not user self-service.
132-
const fallbackWorkspace = await createWorkspaceRecord({
133-
userId,
134-
name: FALLBACK_WORKSPACE_NAME,
135-
organizationId: null,
136-
workspaceMode: WORKSPACE_MODE.PERSONAL,
137-
billedAccountUserId: userId,
138-
executor: tx,
139-
})
140-
141-
if (options.actorId) {
142-
recordAudit({
143-
workspaceId: fallbackWorkspace.id,
144-
actorId: options.actorId,
145-
actorName: options.actorName,
146-
actorEmail: options.actorEmail,
147-
action: AuditAction.WORKSPACE_CREATED,
148-
resourceType: AuditResourceType.WORKSPACE,
149-
resourceId: fallbackWorkspace.id,
150-
resourceName: fallbackWorkspace.name,
151-
description: `Auto-created replacement workspace "${fallbackWorkspace.name}" for a member left with no workspace after deleting "${workspaceRecord.name}"`,
152-
metadata: { deletedWorkspaceId: workspaceId, recipientUserId: userId },
123+
const provisionedFallbacks = await db.transaction(async (tx) => {
124+
const fallbacks: Array<{ userId: string; workspaceId: string; name: string }> = []
125+
126+
if (options.provisionFallbackForStrandedMembers) {
127+
const strandedUserIds = await findMembersStrandedByArchival(tx, workspaceId)
128+
for (const userId of strandedUserIds) {
129+
// Intentionally bypasses getWorkspaceCreationPolicy: this is a system-provisioned safety
130+
// net (never blocked by "who can create a workspace" rules), not user self-service.
131+
const fallbackWorkspace = await createWorkspaceRecord({
132+
userId,
133+
name: FALLBACK_WORKSPACE_NAME,
134+
organizationId: null,
135+
workspaceMode: WORKSPACE_MODE.PERSONAL,
136+
billedAccountUserId: userId,
137+
executor: tx,
153138
})
139+
fallbacks.push({ userId, workspaceId: fallbackWorkspace.id, name: fallbackWorkspace.name })
154140
}
155141
}
156142

@@ -276,9 +262,31 @@ export async function archiveWorkspace(
276262
})
277263
.where(and(eq(workspace.id, workspaceId), isNull(workspace.archivedAt)))
278264

279-
return strandedUserIds
265+
return fallbacks
280266
}, transactionConfig)
281267

268+
// Recorded only after the transaction commits — recordAudit is fire-and-forget and doesn't
269+
// participate in the transaction, so recording it earlier could leave a phantom audit entry
270+
// pointing at a fallback workspace that got rolled back (e.g. on a serialization failure).
271+
if (options.actorId) {
272+
for (const fallback of provisionedFallbacks) {
273+
recordAudit({
274+
workspaceId: fallback.workspaceId,
275+
actorId: options.actorId,
276+
actorName: options.actorName,
277+
actorEmail: options.actorEmail,
278+
action: AuditAction.WORKSPACE_CREATED,
279+
resourceType: AuditResourceType.WORKSPACE,
280+
resourceId: fallback.workspaceId,
281+
resourceName: fallback.name,
282+
description: `Auto-created replacement workspace "${fallback.name}" for a member left with no workspace after deleting "${workspaceRecord.name}"`,
283+
metadata: { deletedWorkspaceId: workspaceId, recipientUserId: fallback.userId },
284+
})
285+
}
286+
}
287+
288+
const provisionedWorkspaceUserIds = provisionedFallbacks.map((fallback) => fallback.userId)
289+
282290
await archiveWorkflowsForWorkspace(workspaceId, options)
283291

284292
logger.info(`[${options.requestId}] Archived workspace ${workspaceId}`)

0 commit comments

Comments
 (0)