fix(folders): close parent-lock write race, allow combined unlock+move

Two more findings this round, both real:

1. Greptile (3x, same root cause): assertFolderMutable's ancestor-chain
   walk (getFolderLockStatus in the shared platform-authz engine) and the
   resource's-own-row check (getResourceLockStatus) both read with plain
   SELECTs, not SELECT ... FOR UPDATE. A concurrent lock toggle on a parent
   folder (or a resource's own row) could still commit between that read
   and the caller's write, even though the caller had already re-checked
   inside its own transaction. Added FOR UPDATE to both shared queries --
   this is the single engine every lock check in the codebase goes through
   (reorder/restore/move/create/update/delete, all four resource types), so
   one fix closes it everywhere, including the two restore-path and one
   reorder-path call sites Greptile flagged individually.

   Reordering multiple folders in one batch can now row-lock several
   folders in a single transaction, so two concurrent reorder requests
   touching an overlapping folder set could deadlock if they lock in
   different orders. Sorted the id list before acquiring locks in
   performReorderFolders so concurrent batches always acquire in the same
   order.

2. Cursor: the table PATCH route's isLockOnlyUpdate gate was false whenever
   folderId also changed, so an admin combining locked: false with a move
   in one request still ran assertResourceMutable against the table's
   current (still-locked) state and was incorrectly rejected -- even though
   the request unlocks it as part of the same atomic write. Found and fixed
   the same bug class at three sibling call sites: renameTable (table),
   updateKnowledgeBase (knowledge base, folderId), performRenameWorkspaceFile
   (file, name), and the generic folders PUT route (any field). All four
   now skip the mutable pre-check specifically when this request also sets
   locked: false, not just when the update is lock-only.

Added regression tests for the combined-unlock-plus-other-field-change case
at all four sites. Full local verification passed (typecheck, full vitest
suite 11430 tests, check:api-validation, lint all clean).

Author: waleedlatif1

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

@@ -10,6 +10,7 @@ import {
   type MockUser,
   permissionsMock,
   permissionsMockFns,
+  resourceLockMockFns,
   workflowsOrchestrationMockFns,
 } from '@sim/testing'
 import { beforeEach, describe, expect, it, vi } from 'vitest'
@@ -237,6 +238,29 @@ describe('Individual Folder API Route', () => {
       expect(mockPerformUpdateFolder).not.toHaveBeenCalled()
     })
 
+    it('skips the mutable check when unlocking a locked folder combined with a move in the same request', async () => {
+      // Regression test: hasNonLockUpdate is true whenever parentId also changes, so
+      // a combined "unlock + move" request previously still ran
+      // policy.assertMutable(id) against the folder's current (still-locked) state
+      // and was incorrectly rejected, even though the request unlocks it as part of
+      // this same atomic write.
+      mockAuthenticatedUser()
+      resourceLockMockFns.mockAssertFolderMutable.mockReset()
+      resourceLockMockFns.mockAssertFolderMutable.mockResolvedValue(undefined)
+
+      const req = createMockRequest('PUT', { locked: false, parentId: 'folder-2' })
+      const params = Promise.resolve({ id: 'folder-1' })
+
+      const response = await PUT(req, { params })
+
+      expect(response.status).toBe(200)
+      expect(resourceLockMockFns.mockAssertFolderMutable).toHaveBeenCalledTimes(1)
+      expect(resourceLockMockFns.mockAssertFolderMutable).toHaveBeenCalledWith(
+        'folder-2',
+        'workflow'
+      )
+    })
+
     it('should update parent folder successfully', async () => {
       mockAuthenticatedUser()
 

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

@@ -79,8 +79,12 @@ export const PUT = withRouteHandler(
         )
       }
 
+      // An admin combining `locked: false` with other field changes (e.g. a move) in
+      // one request is unlocking the folder as part of this same atomic write -- the
+      // mutable-check must not block that. Skip only when this request isn't also
+      // unlocking.
       const hasNonLockUpdate = Object.keys(parsed.data.body).some((key) => key !== 'locked')
-      if (hasNonLockUpdate) {
+      if (hasNonLockUpdate && locked !== false) {
         await policy.assertMutable(id)
       }
       if (parentId !== undefined) {

apps/sim/lib/folders/orchestration.ts

@@ -874,11 +874,16 @@ export async function performReorderFolders(params: PerformReorderFoldersParams)
       // 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.
-      for (const update of validUpdates) {
-        await assertFolderMutable(update.id, resourceType, tx)
-      }
-      for (const parentId of targetParentIds) {
-        await assertFolderMutable(parentId, resourceType, tx)
+      //
+      // 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)
       }
 
       for (const update of validUpdates) {

apps/sim/lib/knowledge/service.test.ts

@@ -305,6 +305,29 @@ describe('updateKnowledgeBase — resource-lock enforcement', () => {
     expect(mockAssertResourceMutable).not.toHaveBeenCalled()
   })
 
+  it('skips the lock check when unlocking a locked knowledge base combined with a move in the same request', async () => {
+    // Regression test: hasNonLockUpdate is true whenever folderId also changes, so a
+    // combined "unlock + move" request previously still ran assertResourceMutable
+    // against the KB's current (still-locked) state and was incorrectly rejected,
+    // even though the request unlocks it as part of this same atomic write.
+    dbChainMockFns.limit
+      .mockResolvedValueOnce([{ workspaceId: 'ws-current', userId: 'u-1' }]) // currentKb (FOR UPDATE)
+      .mockResolvedValueOnce([
+        { workspaceId: 'ws-current', resourceType: 'knowledge_base', deletedAt: null },
+      ]) // assertFolderParentValid's parent lookup
+
+    await updateKnowledgeBase('kb-1', { folderId: 'folder-1', locked: false }, 'req-1').catch(
+      () => undefined
+    )
+
+    expect(mockAssertResourceMutable).not.toHaveBeenCalled()
+    expect(mockAssertFolderMutable).toHaveBeenCalledWith(
+      'folder-1',
+      'knowledge_base',
+      expect.anything()
+    )
+  })
+
   it('rejects moving the knowledge base into a locked destination folder with a 423', async () => {
     // Regression test: assertResourceMutable only checks the KB's *current* folder
     // chain -- without a separate assertFolderMutable(updates.folderId, ...) check,

apps/sim/lib/knowledge/service.ts

@@ -313,7 +313,11 @@ export async function updateKnowledgeBase(
     updates.workspaceId !== undefined ||
     updates.folderId !== undefined ||
     updates.chunkingConfig !== undefined
-  if (hasNonLockUpdate) {
+  // An admin combining `locked: false` with other field changes in one request is
+  // unlocking the knowledge base as part of this same atomic write -- the
+  // mutable-check must not block that. Skip only when this request isn't also
+  // unlocking.
+  if (hasNonLockUpdate && updates.locked !== false) {
     await assertResourceMutable('knowledge_base', knowledgeBaseId)
   }
 

apps/sim/lib/table/__tests__/service.test.ts

@@ -93,6 +93,31 @@ describe('table service — resource-lock enforcement', () => {
       expect(resourceLockMockFns.mockAssertResourceMutable).not.toHaveBeenCalled()
     })
 
+    it('skips the lock check when unlocking a locked table combined with a move in the same request', async () => {
+      // Regression test: isLockOnlyUpdate is false whenever folderId also changes, so a
+      // combined "unlock + move" request previously still ran assertResourceMutable
+      // against the table's current (still-locked) state and was incorrectly rejected,
+      // even though the request unlocks it as part of this same atomic write.
+      dbChainMockFns.limit
+        .mockResolvedValueOnce([{ workspaceId: 'ws-1' }]) // tableRow lookup
+        .mockResolvedValueOnce([{ workspaceId: 'ws-1', resourceType: 'table', deletedAt: null }]) // assertFolderParentValid's parent lookup
+      dbChainMockFns.returning.mockResolvedValueOnce([
+        {
+          id: 'tbl-1',
+          createdBy: 'user-1',
+          workspaceId: 'ws-1',
+          folderId: 'folder-1',
+          locked: false,
+        },
+      ])
+
+      await renameTable('tbl-1', 'new_name', 'req-1', undefined, 'folder-1', false, false)
+
+      expect(resourceLockMockFns.mockAssertResourceMutable).not.toHaveBeenCalled()
+      expect(resourceLockMockFns.mockAssertFolderMutable).toHaveBeenCalledWith('folder-1', 'table')
+      expect(dbChainMockFns.update).toHaveBeenCalledTimes(1)
+    })
+
     it('rejects moving the table into a locked destination folder with a 423', async () => {
       // Regression test: assertResourceMutable only checks the table's *current*
       // folder chain -- without a separate assertFolderMutable(folderId, ...) check,

apps/sim/lib/table/service.ts

@@ -575,7 +575,11 @@ export async function renameTable(
     throw new Error(nameValidation.errors.join(', '))
   }
 
-  if (!isLockOnlyUpdate) {
+  // `isLockOnlyUpdate` is false whenever name/folderId also change, but an admin
+  // combining `locked: false` with those changes in one request is unlocking the
+  // table as part of this same atomic write -- the mutable-check must not block
+  // that. Skip only when this request isn't also unlocking.
+  if (!isLockOnlyUpdate && locked !== false) {
     await assertResourceMutable('table', tableId)
   }
 

apps/sim/lib/workspace-files/orchestration/file-folder-lifecycle.test.ts

@@ -127,6 +127,26 @@ describe('workspace-files orchestration — resource-lock enforcement', () => {
 
       expect(resourceLockMockFns.mockAssertResourceMutable).not.toHaveBeenCalled()
     })
+
+    it('skips the lock check when unlocking a locked file combined with a rename in the same request', async () => {
+      // Regression test: isLockOnlyUpdate is false whenever the name also changes, so
+      // a combined "unlock + rename" request previously still ran
+      // assertResourceMutable against the file's current (still-locked) state and was
+      // incorrectly rejected, even though the request unlocks it as part of this same
+      // atomic write.
+      mockRenameWorkspaceFile.mockResolvedValueOnce({ id: 'file-1', name: 'renamed.txt' })
+
+      await performRenameWorkspaceFile({
+        workspaceId: 'ws-1',
+        fileId: 'file-1',
+        name: 'renamed.txt',
+        userId: 'user-1',
+        locked: false,
+        isLockOnlyUpdate: false,
+      })
+
+      expect(resourceLockMockFns.mockAssertResourceMutable).not.toHaveBeenCalled()
+    })
   })
 
   describe('performDeleteWorkspaceFileItems', () => {

apps/sim/lib/workspace-files/orchestration/file-folder-lifecycle.ts

@@ -313,7 +313,10 @@ export async function performRenameWorkspaceFile(
   const { workspaceId, fileId, name, userId, locked, isLockOnlyUpdate } = params
 
   try {
-    if (!isLockOnlyUpdate) {
+    // An admin combining `locked: false` with a rename in one request is unlocking
+    // the file as part of this same atomic write -- the mutable-check must not
+    // block that. Skip only when this request isn't also unlocking.
+    if (!isLockOnlyUpdate && locked !== false) {
       await assertResourceMutable('file', fileId)
     }
 

packages/platform-authz/src/resource-lock.ts

@@ -117,6 +117,12 @@ export async function getFolderLockStatus(
 
   while (currentFolderId && !visited.has(currentFolderId)) {
     visited.add(currentFolderId)
+    // `FOR UPDATE` row-locks each ancestor for the rest of the caller's transaction --
+    // without it, a lock toggled on this folder after this read but before the
+    // caller's write commits could still be silently bypassed. Callers that pass the
+    // default `db` client (a pre-check outside any transaction) get a single-statement
+    // implicit transaction, so the lock is released immediately and this is a no-op;
+    // callers that pass `tx` hold it until their transaction resolves.
     const [folderRow] = await dbClient
       .select({
         id: folder.id,
@@ -131,6 +137,7 @@ export async function getFolderLockStatus(
           isNull(folder.deletedAt)
         )
       )
+      .for('update')
       .limit(1)
 
     if (!folderRow) break
@@ -162,13 +169,16 @@ export async function getResourceLockStatus(
 ): Promise<LockStatus> {
   const config = RESOURCE_LOCK_LOOKUP[resourceType]
 
+  // See the `FOR UPDATE` comment in `getFolderLockStatus` above -- same reasoning
+  // applies to the resource's own row.
   const [row] = await dbClient
     .select({
       locked: config.lockedColumn,
       folderId: config.folderIdColumn,
     })
     .from(config.table)
     .where(eq(config.idColumn, resourceId))
+    .for('update')
     .limit(1)
 
   if (!row) return UNLOCKED_STATUS