fix(folders): lock the full ancestor closure in one ordered statement

Two more findings this round:

1. Greptile (correctly pushing back on the previous round's fix): sorting
   the *starting* ids before calling assertFolderMutable on each doesn't
   actually prevent deadlocks in a reorder batch. Each call still walks and
   locks its own ancestor chain one row at a time, and two concurrent
   batches whose chains interleave differently (e.g. one locks folder A
   then their shared ancestor C, the other locks folder B then also wants
   C) can still each end up holding a row the other needs. Sorting the
   starting ids only orders the entry points, not the actual sequence of
   rows locked.

   The only way to make batch-vs-batch lock acquisition deadlock-safe is to
   compute the full set of rows that need locking up front and lock all of
   them in a single statement, ordered by id. Replaced the per-id
   assertFolderMutable loop in performReorderFolders with: a read-only
   breadth-first walk (no locks) to collect the full ancestor closure of
   every folder being reordered plus every target parent, then one
   `WHERE id IN (closure) ORDER BY id FOR UPDATE` statement that locks the
   entire closure atomically. Concurrent batches that do the same always
   converge on the same acquisition order and cannot form a cycle.

2. Cursor: the folder-duplicate route's assertTargetParentFolderMutable
   walks the target parent's ancestor chain with the same
   pattern the shared engine had before the previous round's fix -- a plain
   SELECT, not SELECT ... FOR UPDATE, so a parent could still be locked
   between this read and the new folder row being inserted. This one is a
   single linear ancestor walk (not a multi-root batch), so it doesn't have
   the deadlock risk above -- just needed the same FOR UPDATE fix already
   applied to the shared engine.

Updated the reorder route's tests for the new locking shape (a closure walk
plus one ordered lock read instead of N assertFolderMutable calls), added a
regression test for locking an ancestor beyond the batch's explicit ids,
and fixed the duplicate route's test mock to support the FOR UPDATE chain.
Full local verification passed (typecheck, full vitest suite 11431 tests,
check:api-validation, lint all clean).

Author: waleedlatif1

apps/sim/app/api/folders/[id]/duplicate/route.test.ts

@@ -19,15 +19,17 @@ describe('assertTargetParentFolderMutable', () => {
    */
   function buildTx(chain: Array<Record<string, unknown> | undefined>) {
     let call = 0
+    const limit = vi.fn().mockImplementation(() => {
+      const row = chain[call]
+      call += 1
+      return row ? [row] : []
+    })
     return {
       select: vi.fn().mockReturnValue({
         from: vi.fn().mockReturnValue({
           where: vi.fn().mockReturnValue({
-            limit: vi.fn().mockImplementation(() => {
-              const row = chain[call]
-              call += 1
-              return row ? [row] : []
-            }),
+            for: vi.fn().mockReturnValue({ limit }),
+            limit,
           }),
         }),
       }),

apps/sim/app/api/folders/[id]/duplicate/route.ts

@@ -269,6 +269,8 @@ export async function assertTargetParentFolderMutable(
 
   while (currentFolderId && !visited.has(currentFolderId)) {
     visited.add(currentFolderId)
+    // FOR UPDATE row-locks each ancestor for the rest of this transaction -- see the
+    // same comment on getFolderLockStatus in packages/platform-authz/src/resource-lock.ts.
     const [folder] = await tx
       .select({
         id: folderTable.id,
@@ -280,6 +282,7 @@ export async function assertTargetParentFolderMutable(
       })
       .from(folderTable)
       .where(eq(folderTable.id, currentFolderId))
+      .for('update')
       .limit(1)
 
     if (

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

@@ -4,7 +4,6 @@
  * @vitest-environment node
  */
 
-import { ResourceLockedError } from '@sim/platform-authz/resource-lock'
 import {
   authMockFns,
   createMockRequest,
@@ -49,6 +48,30 @@ describe('PUT /api/folders/reorder', () => {
   const mockTxUpdate = vi.fn()
   const mockTxSelect = vi.fn()
 
+  /**
+   * `performReorderFolders` issues several distinct `tx.select(...).from(...).where(...)`
+   * calls inside the transaction (the active-parents recheck, the ancestor-closure
+   * walk, and the final `ORDER BY id FOR UPDATE` lock read) -- this queue returns
+   * each call's result in order and makes the returned builder both directly
+   * awaitable and chainable via `.orderBy()`/`.for()` (both no-ops that return the
+   * same thenable), matching how drizzle's query builder supports either usage.
+   */
+  function queueTxSelectResults(...results: unknown[][]) {
+    let call = 0
+    mockTxSelect.mockImplementation(() => ({
+      from: vi.fn().mockReturnValue({
+        where: vi.fn().mockImplementation(() => {
+          const result = results[call] ?? []
+          call += 1
+          const thenable: any = Promise.resolve(result)
+          thenable.orderBy = vi.fn().mockReturnValue(thenable)
+          thenable.for = vi.fn().mockReturnValue(thenable)
+          return thenable
+        }),
+      }),
+    }))
+  }
+
   beforeEach(() => {
     vi.clearAllMocks()
     resourceLockMockFns.mockAssertFolderMutable.mockReset()
@@ -67,9 +90,9 @@ describe('PUT /api/folders/reorder', () => {
         }),
       }),
     })
-    mockTxSelect.mockReturnValue({
-      from: vi.fn().mockReturnValue({ where: vi.fn().mockResolvedValue([]) }),
-    })
+    // Default: closure walk finds no parent (chain ends immediately), lock read
+    // finds the folder unlocked.
+    queueTxSelectResults([{ id: 'folder-1', parentId: null }], [{ id: 'folder-1', locked: false }])
     mockDb.transaction.mockImplementation(async (cb: (tx: unknown) => Promise<unknown>) =>
       cb({ update: mockTxUpdate, select: mockTxSelect })
     )
@@ -302,19 +325,16 @@ describe('PUT /api/folders/reorder', () => {
     // update.parentId, since parentId is `null` not `undefined` -- succeed below,
     // simulating the folder being unlocked at that moment), but that's a separate
     // round-trip -- an admin could lock the folder in the window between that
-    // check and the transaction. The third assertFolderMutable call is the new
-    // in-transaction recheck this test targets.
+    // check and the transaction. The in-transaction ORDER BY id FOR UPDATE lock
+    // read is the recheck this test targets.
     mockWhere
       .mockReturnValueOnce([
         { id: 'folder-1', workspaceId: 'workspace-123', resourceType: 'workflow' },
       ])
       .mockReturnValueOnce([{ id: 'folder-1', parentId: null }])
       .mockReturnValueOnce([{ id: 'folder-1', workspaceId: 'workspace-123' }])
 
-    resourceLockMockFns.mockAssertFolderMutable
-      .mockResolvedValueOnce(undefined)
-      .mockResolvedValueOnce(undefined)
-      .mockRejectedValueOnce(new ResourceLockedError('workflow', false, 'Folder is locked'))
+    queueTxSelectResults([{ id: 'folder-1', parentId: null }], [{ id: 'folder-1', locked: true }])
 
     const req = createMockRequest('PUT', {
       workspaceId: 'workspace-123',
@@ -324,11 +344,45 @@ describe('PUT /api/folders/reorder', () => {
     const response = await PUT(req)
 
     expect(response.status).toBe(423)
-    expect(resourceLockMockFns.mockAssertFolderMutable).toHaveBeenCalledTimes(3)
     expect(mockDb.transaction).toHaveBeenCalledTimes(1)
     expect(mockTxUpdate).not.toHaveBeenCalled()
   })
 
+  it('rejects the write when an ancestor beyond the starting folder is locked', async () => {
+    // Regression test: the lock check must walk to ancestors not directly named in
+    // the batch (folder-1's own parent) and include them in the single ordered
+    // lock read -- not just the ids explicitly present in the batch. This also
+    // guards the closure-based fix for the deadlock finding: locking each
+    // starting id's ancestor chain separately can still deadlock a concurrent
+    // batch, so the whole closure must be computed first and locked in one
+    // ORDER BY id FOR UPDATE statement.
+    mockWhere
+      .mockReturnValueOnce([
+        { id: 'folder-1', workspaceId: 'workspace-123', resourceType: 'workflow' },
+      ])
+      .mockReturnValueOnce([{ id: 'folder-1', parentId: 'ancestor-1' }])
+      .mockReturnValueOnce([{ id: 'folder-1', workspaceId: 'workspace-123' }])
+
+    queueTxSelectResults(
+      [{ id: 'folder-1', parentId: 'ancestor-1' }], // closure walk, level 1
+      [{ id: 'ancestor-1', parentId: null }], // closure walk, level 2 (chain ends)
+      [
+        { id: 'ancestor-1', locked: true },
+        { id: 'folder-1', locked: false },
+      ] // single ordered lock read over the whole closure
+    )
+
+    const req = createMockRequest('PUT', {
+      workspaceId: 'workspace-123',
+      updates: [{ id: 'folder-1', sortOrder: 2, parentId: null }],
+    })
+
+    const response = await PUT(req)
+
+    expect(response.status).toBe(423)
+    expect(mockTxUpdate).not.toHaveBeenCalled()
+  })
+
   it('rejects a batch that would form a cycle', async () => {
     mockWhere
       .mockReturnValueOnce([

apps/sim/lib/folders/orchestration.ts

@@ -872,18 +872,57 @@ export async function performReorderFolders(params: PerformReorderFoldersParams)
       // 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
-      // both inside the transaction (joining `tx` so the read is part of the
-      // same atomic unit as the writes below) before applying anything.
+      // inside the transaction (joining `tx` so the read is part of the same
+      // atomic unit as the writes below) before applying anything.
       //
-      // assertFolderMutable now row-locks (FOR UPDATE) every folder it walks, so two
-      // concurrent reorder batches touching an overlapping set of folders must acquire
-      // those locks in the same order or they can deadlock. Sorting both id lists into
-      // a single deterministic order before locking guarantees that.
-      const lockOrder = Array.from(
-        new Set([...validUpdates.map((u) => u.id), ...targetParentIds])
-      ).sort()
-      for (const id of lockOrder) {
-        await assertFolderMutable(id, resourceType, tx)
+      // A batch can touch several independent folders (not one linear ancestor
+      // chain), so locking each one's ancestor chain separately -- even in a
+      // consistently sorted order of the *starting* ids -- can still deadlock
+      // against a concurrent batch: each chain walk acquires its own rows one at a
+      // time, and two batches whose chains interleave differently can each end up
+      // holding a row the other needs. The only way to make batch-vs-batch lock
+      // acquisition safe is to compute the full set of rows that need locking
+      // up front and lock all of them in a single statement, ordered by id --
+      // concurrent transactions that do the same always converge on the same
+      // acquisition order and cannot form a cycle.
+      const startIds = Array.from(new Set([...validUpdates.map((u) => u.id), ...targetParentIds]))
+      const closure = new Set(startIds)
+      let frontier = new Set(startIds)
+      while (frontier.size > 0) {
+        const rows = await tx
+          .select({ id: folderTable.id, parentId: folderTable.parentId })
+          .from(folderTable)
+          .where(
+            and(
+              inArray(folderTable.id, Array.from(frontier)),
+              eq(folderTable.resourceType, resourceType),
+              isNull(folderTable.deletedAt)
+            )
+          )
+        const nextFrontier = new Set<string>()
+        for (const row of rows) {
+          if (row.parentId && !closure.has(row.parentId)) {
+            closure.add(row.parentId)
+            nextFrontier.add(row.parentId)
+          }
+        }
+        frontier = nextFrontier
+      }
+
+      const lockedRows = await tx
+        .select({ id: folderTable.id, locked: folderTable.locked })
+        .from(folderTable)
+        .where(
+          and(
+            inArray(folderTable.id, Array.from(closure).sort()),
+            eq(folderTable.resourceType, resourceType)
+          )
+        )
+        .orderBy(folderTable.id)
+        .for('update')
+
+      if (lockedRows.some((row) => row.locked)) {
+        throw new ResourceLockedError(resourceType, false, 'Folder is locked')
       }
 
       for (const update of validUpdates) {