fix(workspaces): auto-provision a replacement workspace instead of blocking deletion#5494
fix(workspaces): auto-provision a replacement workspace instead of blocking deletion#5494waleedlatif1 wants to merge 12 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview
Workspace creation is centralized in Broad unit/route test coverage was added for these flows. Reviewed by Cursor Bugbot for commit c6be4cf. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes a bug in the workspace DELETE route where the pre-deletion check only inspected the deleter's own workspace count rather than accounting for all members of the workspace being deleted, allowing an org admin to strand other members with zero workspaces. The fix shifts from blocking deletion to auto-provisioning a personal fallback workspace for any member who would otherwise be left with zero active workspaces, atomically inside a
Confidence Score: 4/5Safe to merge for the interactive deletion path; the ban flow has a latent failure mode that can leave workspace resources partially unarchived when a banned user owns multiple workspaces. The core bug fix (stranded-member auto-provisioning under serializable isolation) is well-designed and the previously flagged issues are resolved. The one remaining defect is in apps/sim/lib/workflows/lifecycle.ts — the disableUserResources function runs concurrent serializable archiveWorkspace calls that can abort each other; switching to sequential execution would eliminate the conflict. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Admin
participant DELETE_Route as DELETE /api/workspaces/[id]
participant archiveWorkspace
participant findStranded as findMembersStrandedByArchival
participant createWorkspaceRecord
participant DB as PostgreSQL (SERIALIZABLE tx)
participant Audit
Admin->>DELETE_Route: "DELETE /api/workspaces/{id}"
DELETE_Route->>archiveWorkspace: "archiveWorkspace(id, {provisionFallbackForStrandedMembers: true, actorId})"
archiveWorkspace->>DB: BEGIN SERIALIZABLE
DB->>findStranded: SELECT explicit members + org admins
findStranded->>DB: listAccessibleWorkspaceRowsForUser (per candidate)
findStranded-->>archiveWorkspace: strandedUserIds[]
loop for each stranded user
archiveWorkspace->>createWorkspaceRecord: "createWorkspaceRecord({userId, executor: tx})"
createWorkspaceRecord->>DB: INSERT workspace + permissions + workflow
end
archiveWorkspace->>DB: UPDATE/DELETE workspace resources (KB, files, keys, schedules...)
archiveWorkspace->>DB: UPDATE workspace SET archivedAt
DB-->>archiveWorkspace: COMMIT → provisionedFallbacks[]
archiveWorkspace->>Audit: recordAudit WORKSPACE_CREATED (per fallback)
archiveWorkspace-->>DELETE_Route: "{archived: true, provisionedWorkspaceUserIds}"
DELETE_Route->>Audit: recordAudit WORKSPACE_DELETED (+provisionedWorkspaceUserIds metadata)
DELETE_Route-->>Admin: 200 OK
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Admin
participant DELETE_Route as DELETE /api/workspaces/[id]
participant archiveWorkspace
participant findStranded as findMembersStrandedByArchival
participant createWorkspaceRecord
participant DB as PostgreSQL (SERIALIZABLE tx)
participant Audit
Admin->>DELETE_Route: "DELETE /api/workspaces/{id}"
DELETE_Route->>archiveWorkspace: "archiveWorkspace(id, {provisionFallbackForStrandedMembers: true, actorId})"
archiveWorkspace->>DB: BEGIN SERIALIZABLE
DB->>findStranded: SELECT explicit members + org admins
findStranded->>DB: listAccessibleWorkspaceRowsForUser (per candidate)
findStranded-->>archiveWorkspace: strandedUserIds[]
loop for each stranded user
archiveWorkspace->>createWorkspaceRecord: "createWorkspaceRecord({userId, executor: tx})"
createWorkspaceRecord->>DB: INSERT workspace + permissions + workflow
end
archiveWorkspace->>DB: UPDATE/DELETE workspace resources (KB, files, keys, schedules...)
archiveWorkspace->>DB: UPDATE workspace SET archivedAt
DB-->>archiveWorkspace: COMMIT → provisionedFallbacks[]
archiveWorkspace->>Audit: recordAudit WORKSPACE_CREATED (per fallback)
archiveWorkspace-->>DELETE_Route: "{archived: true, provisionedWorkspaceUserIds}"
DELETE_Route->>Audit: recordAudit WORKSPACE_DELETED (+provisionedWorkspaceUserIds metadata)
DELETE_Route-->>Admin: 200 OK
Reviews (8): Last reviewed commit: "fix(workspaces): protect non-banned co-m..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit b075b9f. Configure here.
waleedlatif1
changed the title
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 96b1e68. Configure here.
|
@cursor review |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit b08ecbb. Configure here.
|
@cursor review |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit ca07155. Configure here.
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.
…te 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.
…ocking deletion 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.
…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.
…ioning to opt-in 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<CreateWorkspaceRecordParams, 'executor'> 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.
…jacent assertions
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.
…d by surrounding context
…s in stranding check 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).
…nnection 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.
…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.
waleedlatif1
force-pushed
the
fix-workspace-delete-strand-members
branch
from
ca07155 to
c6be4cf
Compare
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit c6be4cf. Configure here.
1 participant
Summary
/workspaceredirect page).archiveWorkspacenever blocks deletion. Instead, when opted in viaprovisionFallbackForStrandedMembers(set by the interactive DELETE route only), it auto-provisions a personal fallback workspace, atomically in the sameserializabletransaction, for any member who would otherwise be left with zero active workspaces. This is off by default — a future caller ofarchiveWorkspacethat doesn't know about this feature gets the safe, side-effect-free default rather than having to remember to opt out.listAccessibleWorkspaceRowsForUser), so an org admin is never wrongly flagged just because their only explicit permission row was on the workspace being deleted.WORKSPACE_CREATEDaudit entry (attributed to the deleting admin) for each auto-provisioned fallback workspace, alongsideprovisionedWorkspaceUserIdsmetadata on the deletion's own audit entry.lib/workspaces/create.ts(createWorkspaceRecord, parameterized by an optional executor) soPOST /api/workspacesand the archival safety net share one implementation;app/api/workspaces/route.ts'sCreateWorkspaceParamsis nowOmit<CreateWorkspaceRecordParams, 'executor'>instead of a hand-copied duplicate.disableUserResources(ban flow) needs no changes at all — the default (no opt-in) already matches its required behavior: a banned user's owned workspaces are fully archived regardless of other members' workspace counts./simplifyreview (reuse / simplification / efficiency / altitude) against the diff and applied every actionable finding: returned the provisioned-users list from the transaction instead of mutating an outerlet, deduplicated theCreateWorkspaceParamsinterface, inverted the stranded-handling flag to opt-in (resolving the "surprising side effect for future callers" altitude concern without reintroducing the TOCTOU race a literal function-split would cause), and renamed a colliding test helper. The efficiency pass confirmed the per-member loop and single-transaction sequential writes are correct as written (parallelizing statements on one DB connection wouldn't help and risks write-ordering bugs).Type of Change
Testing
archiveWorkspace: opt-in provisioning for a stranded member (still archives), only the actually-stranded member in a mixed group, org-admin not falsely stranded, no provisioning when everyone's safe, audit entry recorded/skipped correctly, default-off behavior for the ban flow, serializable isolation assertedcreateWorkspaceRecord(own transaction vs. provided executor, skip-default-workflow, billed-account permission row)DELETE /api/workspaces/[id]covering 401/403/404/200 (including provisioned-user audit metadata)/500tsc --noEmit,biome check, andbun run check:api-validationall passapp/api/workspaces,lib/workspaces,lib/workflows, and every other caller of the touched shared helpersChecklist