fix(folders): fail safe when the reorder closure goes stale under lock

Greptile P1: the cycle-check fix's closure walk discovers ancestors with
plain reads before the FOR UPDATE lock is acquired. If a concurrent
transaction reparents one of those discovered members to point outside the
closure in that gap, the locked read reveals a parentId the cycle walk has
no data for. The walk's `?? null` fallback treated "not in closure" the
same as "reached root", so it could silently stop short of a real cycle
instead of detecting it -- e.g. A moves under B, the locked read shows a
concurrently-committed B -> C, and C already points back to A; since C was
never discovered by the pre-lock walk, the walk incorrectly terminates at B
instead of continuing to C and finding the cycle back to A.

A fully general fix would need to lock-and-expand the closure in rounds
until it's provably closed under FOR UPDATE, which reintroduces real
deadlock-ordering complexity. Took the simpler, still-correct path instead:
after acquiring the lock, verify every locked row's parentId is either null
or already a member of the locked closure. If not, the closure went stale
between discovery and locking -- fail the whole batch (errorCode: conflict,
409) rather than risk running the cycle walk on data it can't fully vouch
for. The client can retry, and a second attempt's closure walk will see the
now-settled state.

Added a regression test reproducing the exact scenario Greptile described.
Full local verification passed (typecheck, full vitest suite 11438 tests,
check:api-validation, lint all clean, 303 tests across the folders/locking
surface).

Author: waleedlatif1

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

@@ -426,6 +426,51 @@ describe('PUT /api/folders/reorder', () => {
     expect(mockTxUpdate).not.toHaveBeenCalled()
   })
 
+  it('rejects the write when a locked row points outside the discovered closure', async () => {
+    // Regression test: the ancestor-closure walk discovers members with PLAIN reads
+    // before the FOR UPDATE lock is acquired. If a concurrent request reparents one
+    // of those discovered members (target-1 here) to point at a folder outside the
+    // closure in that gap, the locked read reveals a parentId the cycle walk has no
+    // data for -- silently treating "not in closure" as root could let a real cycle
+    // through undetected. The fix fails the whole batch safely instead.
+    mockWhere
+      .mockReturnValueOnce([
+        { id: 'folder-1', workspaceId: 'workspace-123', resourceType: 'workflow' },
+      ])
+      .mockReturnValueOnce([{ id: 'folder-1', parentId: null }])
+      .mockReturnValueOnce([{ id: 'folder-1', workspaceId: 'workspace-123' }])
+      .mockReturnValueOnce({ limit: mockLimit })
+    mockLimit.mockReturnValueOnce([
+      { workspaceId: 'workspace-123', resourceType: 'workflow', deletedAt: null },
+    ])
+
+    queueTxSelectResults(
+      // closure walk, level 1: both starting ids resolve as roots (no further
+      // ancestors) at discovery time -- the walk correctly stops here.
+      [
+        { id: 'folder-1', parentId: null },
+        { id: 'target-1', parentId: null },
+      ],
+      [
+        { id: 'folder-1', locked: false, deletedAt: null },
+        // Concurrently reparented to a folder never discovered by the walk above.
+        { id: 'target-1', locked: false, deletedAt: null, parentId: 'outside-closure' },
+      ]
+    )
+
+    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(409)
+    const data = await response.json()
+    expect(data.error).toBe('Folder tree changed concurrently, please retry')
+    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

apps/sim/lib/folders/orchestration.ts

@@ -936,6 +936,8 @@ class FolderParentNotFoundError extends Error {}
 class FolderReorderNotFoundError extends Error {}
 /** Marks a cycle caught inside the reorder transaction as a 400, not a 500. */
 class FolderReorderCycleError extends Error {}
+/** Marks a closure that went stale between discovery and locking, caught inside the reorder transaction, as a 409, not a 500. */
+class FolderReorderStaleClosureError extends Error {}
 
 export async function performReorderFolders(params: PerformReorderFoldersParams): Promise<{
   success: boolean
@@ -1078,6 +1080,18 @@ export async function performReorderFolders(params: PerformReorderFoldersParams)
         throw new ResourceLockedError(resourceType, false, 'Folder is locked')
       }
 
+      // The ancestor-closure walk above discovers members with PLAIN reads before
+      // this FOR UPDATE lock is acquired -- a concurrent transaction could reparent
+      // one of those ancestors to point outside the discovered closure in that gap.
+      // If so, the locked read below reveals a parentId the cycle walk has no data
+      // for, and treating "not in closure" as "reached root" (as `?? null` would)
+      // could silently let a real cycle through undetected. Fail the whole batch
+      // safely -- the client can retry -- rather than risk that.
+      const closureIds = new Set(lockedRows.map((row) => row.id))
+      if (lockedRows.some((row) => row.parentId && !closureIds.has(row.parentId))) {
+        throw new FolderReorderStaleClosureError('Folder tree changed concurrently, please retry')
+      }
+
       // 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
@@ -1137,6 +1151,9 @@ export async function performReorderFolders(params: PerformReorderFoldersParams)
     if (error instanceof FolderReorderCycleError) {
       return { success: false, updated: 0, error: error.message, errorCode: 'validation' }
     }
+    if (error instanceof FolderReorderStaleClosureError) {
+      return { success: false, updated: 0, error: error.message, errorCode: 'conflict' }
+    }
     // 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 })