improvement(deletion): refactor to soft deletion of resources#3552
improvement(deletion): refactor to soft deletion of resources#3552icecrasher321 wants to merge 1 commit intofeat/mothership-copilotfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryMedium Risk Overview It also adds scope-based listing ( Written by Cursor Bugbot for commit 29e636b. Configure here. |
Greptile SummaryThis PR refactors resource deletion across the Sim application from hard deletes to soft deletes ("archiving"), covering workspaces, workflows, knowledge bases, tables, files, webhooks, schedules, chats, forms, MCP tools, and A2A agents. The approach adds Key findings:
Confidence Score: 2/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[DELETE /workspaces/:id] --> B[Fetch workspaceRecord - active only]
B --> C[Handle templates for workflows]
C --> D[archiveWorkspace]
D --> E[getWorkspaceWithOwner - incl archived]
E --> F{Already archived?}
F -->|Yes| G[Return archived: false]
F -->|No| H[archiveWorkflowsForWorkspace - OUTSIDE TX]
H --> I[archiveWorkflow per workflow - each in own TX]
I --> J[db.transaction - workspace-level resources]
J --> K[archive knowledgeBase]
J --> L[archive userTableDefinitions]
J --> M[archive workspaceFiles]
J --> N[cancel invitations]
J --> O[delete apiKeys]
J --> P[archive workflowSchedules - job type]
J --> Q[archive workspace record]
J -->|TX fails here| R[⚠️ Workflows already archived\nWorkspace still active]
Q --> S[Return archived: true]
S --> T[recordAudit + 200 OK]
subgraph archiveWorkflow
I --> W1[cleanupExternalWebhooks - OUTSIDE TX]
W1 --> W2[db.transaction]
W2 --> W3[archive schedules, webhooks, chat, form, mcpTools, a2aAgent, workflow]
end
Last reviewed commit: 29e636b |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return NextResponse.json({ error: 'Unauthorized' }, { status: 401 }) | ||
| } | ||
|
|
||
| const scope = (new URL(request.url).searchParams.get('scope') ?? 'active') as WorkspaceScope |
There was a problem hiding this comment.
WorkspaceScope type used but never imported
WorkspaceScope is referenced as a type assertion on this line, but it is not imported anywhere in the file. The type is exported from @/lib/workspaces/utils, so this will cause a TypeScript compilation error.
| const scope = (new URL(request.url).searchParams.get('scope') ?? 'active') as WorkspaceScope | |
| import { listUserWorkspaces, type WorkspaceScope } from '@/lib/workspaces/utils' |
And the cast can remain as-is once the type is imported:
const scope = (new URL(request.url).searchParams.get('scope') ?? 'active') as WorkspaceScope| await archiveWorkflowsForWorkspace(workspaceId, options) | ||
|
|
||
| await db.transaction(async (tx) => { |
There was a problem hiding this comment.
Workflow archiving outside transaction risks partial state
archiveWorkflowsForWorkspace is awaited on line 40 before the db.transaction block begins on line 42. Each workflow is archived in its own internal transaction inside archiveWorkflows. If the subsequent workspace-level db.transaction fails (e.g. a DB error while updating knowledgeBase, workspace, etc.), all workflows in the workspace will already be permanently soft-deleted while the workspace record itself remains active. Re-running the delete will then find zero non-archived workflows and still fail if the underlying DB issue persists, leaving the workspace permanently in a broken half-archived state.
Consider wrapping the workflow archiving step within the same transaction, or at a minimum applying a compensating step / idempotency guard to ensure these two phases always commit together.
| onSuccess: (_data, variables) => { | ||
| queryClient.invalidateQueries({ queryKey: workspaceKeys.all }) | ||
| queryClient.removeQueries({ queryKey: workspaceKeys.detail(variables.workspaceId) }) | ||
| queryClient.removeQueries({ queryKey: workspaceKeys.settings(variables.workspaceId) }) | ||
| queryClient.removeQueries({ queryKey: workspaceKeys.permissions(variables.workspaceId) }) | ||
| queryClient.removeQueries({ queryKey: workspaceKeys.members(variables.workspaceId) }) | ||
| }, |
There was a problem hiding this comment.
variables.workspaceId is always undefined
The onSuccess callback accesses variables.workspaceId, but variables has type CreateWorkspaceParams which only contains name. This means the four removeQueries calls are silently no-ops because they receive undefined as the workspace ID.
The correct workspace ID is available on the first parameter (data) returned by mutationFn. The callback signature should be changed to onSuccess: (data) => { ... } and data.id used in place of variables.workspaceId.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 4 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| logger.info(`Deleted templates for workflows in workspace ${workspaceId}`) | ||
| } else { | ||
| // Set workflowId to null for templates to create "orphaned" templates | ||
| // This allows templates to remain without source workflows |
There was a problem hiding this comment.
Workspace deletion lost atomicity for template handling
Medium Severity
Template deletion/orphaning (lines 256–264) now executes outside any transaction, separately from archiveWorkspace (which has its own internal transaction). If archiveWorkspace fails after templates have already been deleted or orphaned, the database is left in an inconsistent state — templates are gone or detached but the workspace and its workflows remain active. The old code performed both operations within a single transaction, guaranteeing atomicity.
| .from(workflow) | ||
| .where(eq(workflow.workspaceId, workspaceId)) | ||
| .orderBy(...orderByClause) | ||
| workflows = await listWorkflows(workspaceId, { scope }) |
There was a problem hiding this comment.
Workflow listing loses ordering tiebreaker via listWorkflows
Low Severity
When workspaceId is provided, the code now delegates to listWorkflows() which orders by (sortOrder, createdAt). The original inline query and the still-defined orderByClause on line 78 include asc(workflow.id) as a final tiebreaker for deterministic ordering. The else branch (no workspaceId) still uses orderByClause with the id tiebreaker, creating an inconsistency between the two code paths.
| ) | ||
| if (!permission) { | ||
| return NextResponse.json({ error: 'Access denied' }, { status: 403 }) | ||
| } |
There was a problem hiding this comment.
API endpoint now leaks workflow existence to unauthorized users
Low Severity
The refactored code separates the workflow lookup from the permission check. The old code used a single innerJoin with permissions, returning 404 for both "not found" and "no access." The new code returns 404 when the workflow doesn't exist and 403 when the user lacks permission, allowing an attacker to enumerate valid workflow IDs through this public API endpoint.
| }) | ||
| return false | ||
| } | ||
|
|
There was a problem hiding this comment.
Double database lookup in workspace file access verification
Low Severity
verifyWorkspaceFileAccess now calls getFileMetadataByKey with includeDeleted: true to check for archived files, and then lookupWorkspaceFileByKey internally calls getFileMetadataByKey again (with includeDeleted: false) for the same key. This results in two sequential database queries against the workspaceFiles table for every workspace file access check, when a single query with includeDeleted: true would suffice.
1 participant
Summary
Soft Deletion of Workspace and Resources: Workflows, KBs, Files, Tables
Type of Change
Testing
Added specific unit tests + tested manually
Checklist