fix(folders): close parent-delete race in restore, fix file-folder errorCodes

Two more findings this round:

1. Greptile P1: performRestoreFolder (workflow) and performRestoreResourceFolder
   (kb/table) both resolve the target parent by reading its deletedAt inside the
   transaction, but that read was a plain SELECT, not SELECT ... FOR UPDATE. A
   parent could still be soft-deleted by a concurrent transaction between that
   read and the restore write below it, leaving the restored folder pointing at
   a now-deleted parent. Added FOR UPDATE to both reads.

   Also closed the same class of gap in performReorderFolders: the target-parent
   active check ran as a separate, unlocked pre-check before the closure-based
   lock query introduced last round. Folded it into the closure-lock query
   instead of adding a second incremental lock -- the closure query now also
   selects deletedAt and re-validates every target parent resolved to an active
   row only after FOR UPDATE is held on it, which is the only point where the
   check is actually race-free (a row's deletedAt can't change while the lock is
   held). A second, separately-ordered lock acquisition here would have
   reintroduced the exact deadlock class fixed last round.

2. Cursor: performUpdateFileFolder only recognized 'Folder not found' from
   updateWorkspaceFileFolder's thrown errors; the other three messages it can
   throw ('Folder cannot be its own parent', 'Target folder not found',
   'Cannot move a folder into one of its descendants') fell through to the
   generic errorCode: 'internal' bucket, so the folder PUT route (now using
   statusForOrchestrationError) returned 500 for what are actually validation
   failures. Mapped all three to errorCode: 'validation'. Found and fixed the
   same gap in performCreateFileFolder, which had an identical unhandled
   'Target folder not found' case from createWorkspaceFileFolder.

Added a regression test for the reorder parent-delete race. Full local
verification passed (typecheck, full vitest suite 11432 tests,
check:api-validation, lint all clean).

Author: waleedlatif1

apps/sim/app/api/folders/reorder/route.test.ts

@@ -383,6 +383,49 @@ describe('PUT /api/folders/reorder', () => {
     expect(mockTxUpdate).not.toHaveBeenCalled()
   })
 
+  it('rejects the write when the target parent is concurrently soft-deleted', async () => {
+    // Regression test: assertFolderParentValid only validates the target parent at
+    // request-validation time, using the default (non-tx) db client -- a parent
+    // soft-deleted after that read but before (or during) this transaction must
+    // still be caught. The closure-lock query is the only point where this is
+    // race-free: once FOR UPDATE is held on the parent's row, its deletedAt state
+    // can't change until this transaction resolves, so that's where the final
+    // active check must happen -- not an earlier, unlocked pre-check.
+    mockWhere
+      .mockReturnValueOnce([
+        { id: 'folder-1', workspaceId: 'workspace-123', resourceType: 'workflow' },
+      ])
+      .mockReturnValueOnce([])
+      .mockReturnValueOnce([{ id: 'folder-1', workspaceId: 'workspace-123' }])
+      .mockReturnValueOnce({ limit: mockLimit })
+    mockLimit.mockReturnValueOnce([
+      { workspaceId: 'workspace-123', resourceType: 'workflow', deletedAt: null },
+    ])
+
+    queueTxSelectResults(
+      [
+        { id: 'folder-1', parentId: null },
+        { id: 'target-1', parentId: null },
+      ], // closure walk, level 1
+      [
+        { id: 'folder-1', locked: false, deletedAt: null },
+        { id: 'target-1', locked: false, deletedAt: new Date() }, // deleted after validation
+      ]
+    )
+
+    const req = createMockRequest('PUT', {
+      workspaceId: 'workspace-123',
+      updates: [{ id: 'folder-1', sortOrder: 0, parentId: 'target-1' }],
+    })
+
+    const response = await PUT(req)
+
+    expect(response.status).toBe(404)
+    const data = await response.json()
+    expect(data.error).toBe('Parent folder not found')
+    expect(mockTxUpdate).not.toHaveBeenCalled()
+  })
+
   it('rejects a batch that would form a cycle', async () => {
     mockWhere
       .mockReturnValueOnce([

apps/sim/lib/folders/orchestration.ts

@@ -183,6 +183,9 @@ async function performCreateFileFolder(
     ) {
       return { success: false, error: toError(error).message, errorCode: 'conflict' }
     }
+    if (toError(error).message === 'Target folder not found') {
+      return { success: false, error: 'Target folder not found', errorCode: 'validation' }
+    }
     logger.error('Failed to create file folder', { error })
     return { success: false, error: 'Internal server error', errorCode: 'internal' }
   }
@@ -230,8 +233,16 @@ async function performUpdateFileFolder(
     ) {
       return { success: false, error: toError(error).message, errorCode: 'conflict' }
     }
-    if (toError(error).message === 'Folder not found') {
-      return { success: false, error: 'Folder not found', errorCode: 'not_found' }
+    const message = toError(error).message
+    if (message === 'Folder not found') {
+      return { success: false, error: message, errorCode: 'not_found' }
+    }
+    if (
+      message === 'Folder cannot be its own parent' ||
+      message === 'Target folder not found' ||
+      message === 'Cannot move a folder into one of its descendants'
+    ) {
+      return { success: false, error: message, errorCode: 'validation' }
     }
     logger.error('Failed to update file folder', { error })
     return { success: false, error: 'Internal server error', errorCode: 'internal' }
@@ -603,10 +614,16 @@ async function performRestoreResourceFolder<TCountKey extends 'knowledgeBases' |
     // leaving the folder orphaned under an archived parent.
     let resolvedParentId = raw.parentId
     if (resolvedParentId) {
+      // FOR UPDATE closes the window where a concurrent transaction soft-deletes
+      // this parent between this read and the write below -- without it, this
+      // plain read isn't locked, so the parent could be deleted out from under us
+      // after we've already decided it's active, restoring this folder under a
+      // parent that no longer exists.
       const [parent] = await tx
         .select({ deletedAt: folderTable.deletedAt })
         .from(folderTable)
         .where(eq(folderTable.id, resolvedParentId))
+        .for('update')
         .limit(1)
       if (!parent || parent.deletedAt) resolvedParentId = null
     }
@@ -855,20 +872,6 @@ export async function performReorderFolders(params: PerformReorderFoldersParams)
 
   try {
     await db.transaction(async (tx) => {
-      // Re-check each target parent is still active inside the transaction --
-      // assertFolderParentValid above only reads at validation time, so a parent
-      // concurrently soft-deleted before this transaction opens could otherwise
-      // leave an active folder pointing at a deleted one.
-      if (targetParentIds.length > 0) {
-        const activeParents = await tx
-          .select({ id: folderTable.id })
-          .from(folderTable)
-          .where(and(inArray(folderTable.id, targetParentIds), isNull(folderTable.deletedAt)))
-        if (activeParents.length !== targetParentIds.length) {
-          throw new FolderReorderNotFoundError('Parent folder not found')
-        }
-      }
-
       // The route checks lock state before calling this function, but that's a
       // separate round-trip -- an admin could lock a source folder or a target
       // parent in the window between that check and this transaction. Re-check
@@ -909,8 +912,18 @@ export async function performReorderFolders(params: PerformReorderFoldersParams)
         frontier = nextFrontier
       }
 
+      // Selecting without an isNull(deletedAt) filter and re-checking it here (rather
+      // than trusting the closure walk above, which reads before this lock is held)
+      // is what makes this race-free: a row can still be soft-deleted by another
+      // transaction right up until this statement acquires FOR UPDATE, but not after
+      // -- so `deletedAt` on a just-locked row is guaranteed accurate for the rest of
+      // this transaction.
       const lockedRows = await tx
-        .select({ id: folderTable.id, locked: folderTable.locked })
+        .select({
+          id: folderTable.id,
+          locked: folderTable.locked,
+          deletedAt: folderTable.deletedAt,
+        })
         .from(folderTable)
         .where(
           and(
@@ -921,7 +934,18 @@ export async function performReorderFolders(params: PerformReorderFoldersParams)
         .orderBy(folderTable.id)
         .for('update')
 
-      if (lockedRows.some((row) => row.locked)) {
+      const activeLockedIds = new Set(
+        lockedRows.filter((row) => !row.deletedAt).map((row) => row.id)
+      )
+      // Re-check every target parent resolved to an active row under this lock --
+      // assertFolderParentValid only reads at validation time, so a parent
+      // concurrently soft-deleted before (or racing) this transaction could
+      // otherwise leave an active folder pointing at a deleted one.
+      if (targetParentIds.some((id) => !activeLockedIds.has(id))) {
+        throw new FolderReorderNotFoundError('Parent folder not found')
+      }
+
+      if (lockedRows.some((row) => !row.deletedAt && row.locked)) {
         throw new ResourceLockedError(resourceType, false, 'Folder is locked')
       }
 

apps/sim/lib/workflows/orchestration/folder-lifecycle.ts

@@ -509,10 +509,16 @@ export async function performRestoreFolder(
 
     let resolvedParentId = existingFolder.parentId
     if (resolvedParentId) {
+      // FOR UPDATE closes the window where a concurrent transaction soft-deletes
+      // this parent between this read and the write below -- without it, this
+      // plain read isn't locked, so the parent could be deleted out from under us
+      // after we've already decided it's active, restoring this folder under a
+      // parent that no longer exists.
       const [parentFolder] = await tx
         .select({ deletedAt: folder.deletedAt })
         .from(folder)
         .where(eq(folder.id, resolvedParentId))
+        .for('update')
 
       if (!parentFolder || parentFolder.deletedAt) {
         resolvedParentId = null