fix(folders): recheck cycle-freedom inside the reorder transaction

Greptile P1: the route's circular-reference check reads an unlocked snapshot
of the folder tree before the transaction opens. Two concurrent reorder
requests can each move one end of what becomes an A<->B cycle -- each
request's own cycle check only sees its own proposed edge merged with a
stale snapshot of the other folder's parentId, so both can pass
independently and, once both commit, leave a genuine cycle in the tree that
the write path never re-validates.

A cycle can only newly appear through an edge this batch is writing, and
every folder whose parentId is being changed is already a `startId` that
the closure-lock query (from an earlier round's deadlock fix) walks and
locks. So the fix reuses that same locked, transaction-consistent data:
after acquiring FOR UPDATE on the closure, build a parentId map from the
locked rows (overridden by this batch's own proposed new parentIds) and
re-run the identical cycle-walk the route already does, but against data
that can no longer change out from under it. A cycle found here now rolls
back the whole transaction with the same 400/'Cannot create circular folder
reference' response the pre-check already returns for the same-request
case.

Added a regression test simulating the race: the route's pre-check snapshot
shows the target parent's parentId as still null, while the locked read
inside the transaction reflects a concurrent request that already committed
the other half of the cycle. Full local verification passed (typecheck,
full vitest suite 11437 tests, check:api-validation, lint all clean).

Author: waleedlatif1

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

@@ -426,6 +426,50 @@ describe('PUT /api/folders/reorder', () => {
     expect(mockTxUpdate).not.toHaveBeenCalled()
   })
 
+  it('rejects the write when a concurrent request created a cycle between the pre-check and the transaction', async () => {
+    // Regression test: the route's own cycle check reads an unlocked snapshot of
+    // the folder tree before this transaction opens. Two concurrent requests can
+    // each move one end of what becomes an A<->B cycle, both passing their own
+    // (mutually stale) check -- simulated here by having the pre-check snapshot
+    // show B's parentId as still null (stale), while the locked, tx-consistent
+    // read inside the transaction reflects a concurrent request that already
+    // committed B.parentId = A.
+    mockWhere
+      .mockReturnValueOnce([{ id: 'A', workspaceId: 'workspace-123', resourceType: 'workflow' }])
+      .mockReturnValueOnce([
+        { id: 'A', parentId: null },
+        { id: 'B', parentId: null },
+      ])
+      .mockReturnValueOnce([{ id: 'A', workspaceId: 'workspace-123' }])
+      .mockReturnValueOnce({ limit: mockLimit })
+    mockLimit.mockReturnValueOnce([
+      { workspaceId: 'workspace-123', resourceType: 'workflow', deletedAt: null },
+    ])
+
+    queueTxSelectResults(
+      [
+        { id: 'A', parentId: null },
+        { id: 'B', parentId: null },
+      ], // closure walk, level 1
+      [
+        { id: 'A', parentId: null, locked: false, deletedAt: null },
+        { id: 'B', parentId: 'A', locked: false, deletedAt: null }, // committed by a concurrent request
+      ]
+    )
+
+    const req = createMockRequest('PUT', {
+      workspaceId: 'workspace-123',
+      updates: [{ id: 'A', sortOrder: 0, parentId: 'B' }],
+    })
+
+    const response = await PUT(req)
+
+    expect(response.status).toBe(400)
+    const data = await response.json()
+    expect(data.error).toBe('Cannot create circular folder reference')
+    expect(mockTxUpdate).not.toHaveBeenCalled()
+  })
+
   it('rejects a batch that would form a cycle', async () => {
     mockWhere
       .mockReturnValueOnce([

apps/sim/lib/folders/orchestration.ts

@@ -841,6 +841,8 @@ export async function performRestoreFolder(
 
 /** Marks a concurrent-deletion race caught inside the reorder transaction as a 404, not a 500. */
 class FolderReorderNotFoundError extends Error {}
+/** Marks a cycle caught inside the reorder transaction as a 400, not a 500. */
+class FolderReorderCycleError extends Error {}
 
 export async function performReorderFolders(params: PerformReorderFoldersParams): Promise<{
   success: boolean
@@ -954,6 +956,7 @@ export async function performReorderFolders(params: PerformReorderFoldersParams)
       const lockedRows = await tx
         .select({
           id: folderTable.id,
+          parentId: folderTable.parentId,
           locked: folderTable.locked,
           deletedAt: folderTable.deletedAt,
         })
@@ -982,6 +985,32 @@ export async function performReorderFolders(params: PerformReorderFoldersParams)
         throw new ResourceLockedError(resourceType, false, 'Folder is locked')
       }
 
+      // The route's own cycle check reads an unlocked snapshot of the folder tree
+      // before this transaction opens -- two concurrent reorder requests can each
+      // move one end of what becomes an A<->B cycle, both passing their own
+      // (mutually stale) check. A cycle can only newly appear through an edge this
+      // batch is writing, and every folder whose parentId this batch changes is a
+      // `startId`, so re-walking from every `startId` using this transaction's own
+      // locked, tx-consistent `parentId` values (closure already contains every
+      // node such a walk could reach) reliably catches it before the write below.
+      const lockedParentById = new Map(lockedRows.map((row) => [row.id, row.parentId]))
+      for (const update of validUpdates) {
+        if (update.parentId !== undefined) {
+          lockedParentById.set(update.id, update.parentId || null)
+        }
+      }
+      for (const update of validUpdates) {
+        const visited = new Set<string>()
+        let cursor: string | null = update.id
+        while (cursor) {
+          if (visited.has(cursor)) {
+            throw new FolderReorderCycleError('Cannot create circular folder reference')
+          }
+          visited.add(cursor)
+          cursor = lockedParentById.get(cursor) ?? null
+        }
+      }
+
       for (const update of validUpdates) {
         const updateData: Record<string, unknown> = {
           sortOrder: update.sortOrder,
@@ -1012,6 +1041,9 @@ export async function performReorderFolders(params: PerformReorderFoldersParams)
     if (error instanceof FolderReorderNotFoundError) {
       return { success: false, updated: 0, error: error.message, errorCode: 'not_found' }
     }
+    if (error instanceof FolderReorderCycleError) {
+      return { success: false, updated: 0, error: error.message, errorCode: 'validation' }
+    }
     // An unexpected DB/transaction failure, not a client-caused validation error --
     // log the real cause but don't leak internal error details to the response.
     logger.error('Unexpected error reordering folders', { error })