Skip to content

Commit f57d28d

Browse files
waleedlatif1waleedlatif1
committed
fix(folders): proactive sweep for missing in-tx lock rechecks + parent-delete race
Proactively audited the whole codebase for the two bug classes Greptile has found repeatedly this session (missing in-transaction lock recheck; target parent lock-checked but not re-verified as still active) rather than waiting for another round to find each remaining instance. Found and fixed 9 sites: 1. Greptile P1 (new finding, on last round's own fix): the just-added workflow performUpdateFolder transaction checks the target parent's LOCK state but not whether it's still ACTIVE -- assertFolderMutable's lock walker treats a deleted parent as unlocked (it stops the walk when the row is missing rather than erroring), so a parent deleted between the earlier assertFolderParentValid precheck and this transaction could still have the moved folder's parentId written to point at it. Added a FOR UPDATE + isNull(deletedAt) recheck, race-free once the lock is held. Found and fixed the identical gap in the sibling generic kb/table performUpdateFolder in folders/orchestration.ts, which had the exact same shape from the same earlier round. 2. performDeleteResourceFolder (kb/table folder delete) -- wrapped in a transaction but never checked the folder's own lock state at all before cascading the delete. Added assertFolderMutable inside the tx. 3. deleteKnowledgeBase -- already row-locks via SELECT ... FOR UPDATE but never re-invoked assertResourceMutable after acquiring it. Added the recheck bound to tx. 4. deleteTable -- pre-check only, no transaction and no recheck at all. Wrapped the write in a transaction with an in-tx recheck. 5. archiveWorkspaceFileFolderRecursive (file-folder delete) -- transaction existed but never checked lock state. Added assertFolderMutable inside the tx, matching the sibling restoreWorkspaceFileFolder in the same file. 6. bulkArchiveWorkspaceFileItems (file/folder bulk delete) -- same gap. Since this function already acquires the workspace-scoped pg_advisory_xact_lock as its first step (which every other file-folder mutation in this file also acquires first), it's already fully serialized against concurrent mutations in the same workspace -- unlike the reorder batch fix, a simple per-id recheck loop is safe here without a closure-based ordered lock. 7. renameWorkspaceFile -- no transaction, no recheck. Wrapped in a transaction with the same "unless unlocking" in-tx recheck pattern used at the other lock-toggle-capable rename/update sites. 8. restoreWorkspaceFile -- no lock check anywhere. Added a direct `.locked` check on the already-fetched soft-deleted row (matches the pattern used by the other resource restore paths, since the generic lock engine only reads active rows and can't see a soft-deleted resource's own flag). 9. performDeleteFolder/deleteFolderRecursively (workflow folder delete) -- entirely non-transactional recursive cascade with zero lock checks. archiveWorkflowsByIdsInWorkspace (called mid-cascade) doesn't accept a `tx`, so wrapping the whole recursive cascade in one transaction isn't a clean fix without a larger, riskier change to that shared function. Applied the narrower, still-real improvement: a tightly-scoped transaction around just each folder's own lock recheck + its own delete write, closing the race for that specific row even though the cascade as a whole still isn't one atomic unit. Full local verification passed (typecheck, full vitest suite 11437 tests, check:api-validation, lint all clean, 302 tests across the folders/locking surface specifically).
1 parent 7685991 commit f57d28d

6 files changed

Lines changed: 201 additions & 62 deletions

File tree

‎apps/sim/lib/folders/orchestration.ts‎

Lines changed: 81 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -486,6 +486,13 @@ async function performDeleteResourceFolder<TCountKey extends 'knowledgeBases' |
486486
.limit(1)
487487
if (!existing) return null
488488

489+
// The route checks lock state before calling this function, but that's a
490+
// separate round-trip -- an admin could lock this folder in the window
491+
// between that check and this transaction. Re-check inside the transaction
492+
// (joining `tx` so the read is part of the same atomic unit as the writes
493+
// below) before applying anything.
494+
await assertFolderMutable(folderId, cascade.resourceType, tx)
495+
489496
const folderIds = await collectFolderSubtreeIds(workspaceId, cascade.resourceType, folderId, tx)
490497

491498
const archivedFolders = await tx
@@ -760,47 +767,80 @@ export async function performUpdateFolder(
760767
const isLockOnlyUpdate =
761768
params.name === undefined && params.parentId === undefined && params.sortOrder === undefined
762769

763-
const updated = await db.transaction(async (tx) => {
764-
// The route checks lock state before calling this function, but that's a
765-
// separate round-trip -- an admin could lock this folder (or its target
766-
// parent) in the window between that check and this transaction. Re-check
767-
// inside the transaction (joining `tx` so the read is part of the same
768-
// atomic unit as the write below) before applying anything.
769-
//
770-
// `assertFolderMutableUnlessUnlocking` only treats this folder's own
771-
// (about-to-be-cleared) lock as satisfied when this same request unlocks it --
772-
// a lock inherited from an ancestor still blocks. Skipped entirely for a
773-
// lock-only update, matching the file/kb/table lock-toggle precedent.
774-
if (!isLockOnlyUpdate) {
775-
await assertFolderMutableUnlessUnlocking(
776-
params.folderId,
777-
params.resourceType,
778-
params.locked === false,
779-
tx
780-
)
781-
}
782-
if (params.parentId) {
783-
// A folder in an unlocked location could otherwise be moved into a locked one.
784-
await assertFolderMutable(params.parentId, params.resourceType, tx)
785-
}
770+
try {
771+
const updated = await db.transaction(async (tx) => {
772+
// The route checks lock state before calling this function, but that's a
773+
// separate round-trip -- an admin could lock this folder (or its target
774+
// parent) in the window between that check and this transaction. Re-check
775+
// inside the transaction (joining `tx` so the read is part of the same
776+
// atomic unit as the write below) before applying anything.
777+
//
778+
// `assertFolderMutableUnlessUnlocking` only treats this folder's own
779+
// (about-to-be-cleared) lock as satisfied when this same request unlocks it --
780+
// a lock inherited from an ancestor still blocks. Skipped entirely for a
781+
// lock-only update, matching the file/kb/table lock-toggle precedent.
782+
if (!isLockOnlyUpdate) {
783+
await assertFolderMutableUnlessUnlocking(
784+
params.folderId,
785+
params.resourceType,
786+
params.locked === false,
787+
tx
788+
)
789+
}
790+
if (params.parentId) {
791+
// A folder in an unlocked location could otherwise be moved into a locked one.
792+
await assertFolderMutable(params.parentId, params.resourceType, tx)
793+
794+
// assertFolderMutable's lock walker treats a deleted parent as unlocked (it
795+
// stops the ancestor walk when the row is missing, rather than erroring), so
796+
// it alone doesn't catch a parent deleted between the earlier
797+
// assertFolderParentValid precheck and this transaction. FOR UPDATE here
798+
// makes the recheck race-free: once held, the parent's deletedAt can't
799+
// change until this transaction resolves.
800+
const [targetParent] = await tx
801+
.select({ id: folderTable.id })
802+
.from(folderTable)
803+
.where(
804+
and(
805+
eq(folderTable.id, params.parentId),
806+
eq(folderTable.resourceType, params.resourceType),
807+
isNull(folderTable.deletedAt)
808+
)
809+
)
810+
.for('update')
811+
.limit(1)
812+
if (!targetParent) {
813+
throw new FolderParentNotFoundError('Parent folder not found')
814+
}
815+
}
786816

787-
const [row] = await tx
788-
.update(folderTable)
789-
.set(updates)
790-
.where(
791-
and(
792-
eq(folderTable.id, params.folderId),
793-
eq(folderTable.workspaceId, params.workspaceId),
794-
eq(folderTable.resourceType, params.resourceType),
795-
isNull(folderTable.deletedAt)
817+
const [row] = await tx
818+
.update(folderTable)
819+
.set(updates)
820+
.where(
821+
and(
822+
eq(folderTable.id, params.folderId),
823+
eq(folderTable.workspaceId, params.workspaceId),
824+
eq(folderTable.resourceType, params.resourceType),
825+
isNull(folderTable.deletedAt)
826+
)
796827
)
797-
)
798-
.returning()
799-
return row
800-
})
828+
.returning()
829+
return row
830+
})
801831

802-
if (!updated) return { success: false, error: 'Folder not found', errorCode: 'not_found' }
803-
return { success: true, folder: updated }
832+
if (!updated) return { success: false, error: 'Folder not found', errorCode: 'not_found' }
833+
return { success: true, folder: updated }
834+
} catch (error) {
835+
// Propagate uncaught so the route's ResourceLockedError -> 423 handling applies.
836+
if (error instanceof ResourceLockedError) {
837+
throw error
838+
}
839+
if (error instanceof FolderParentNotFoundError) {
840+
return { success: false, error: error.message, errorCode: 'validation' }
841+
}
842+
throw error
843+
}
804844
}
805845

806846
export async function performDeleteFolder(
@@ -839,6 +879,9 @@ export async function performRestoreFolder(
839879
return performRestoreResourceFolder(params, TABLE_FOLDER_CASCADE)
840880
}
841881

882+
/** Marks a concurrent-deletion race on a target parent, caught inside a folder-update transaction, as a 400 (validation), not a 500. */
883+
class FolderParentNotFoundError extends Error {}
884+
842885
/** Marks a concurrent-deletion race caught inside the reorder transaction as a 404, not a 500. */
843886
class FolderReorderNotFoundError extends Error {}
844887
/** Marks a cycle caught inside the reorder transaction as a 400, not a 500. */

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -593,6 +593,12 @@ export async function deleteKnowledgeBase(
593593
await db.transaction(async (tx) => {
594594
await tx.execute(sql`SELECT 1 FROM knowledge_base WHERE id = ${knowledgeBaseId} FOR UPDATE`)
595595

596+
// The pre-check above is a separate round-trip -- an admin could lock this KB
597+
// in the window between that check and this transaction. Re-check inside the
598+
// transaction, joining `tx` so the read is part of the same atomic unit as
599+
// the FOR UPDATE lock and the writes below.
600+
await assertResourceMutable('knowledge_base', knowledgeBaseId, tx)
601+
596602
await tx
597603
.update(knowledgeBase)
598604
.set({

‎apps/sim/lib/table/service.ts‎

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -737,15 +737,23 @@ export async function deleteTable(
737737
await assertResourceMutable('table', tableId)
738738

739739
const now = new Date()
740-
const result = await db
741-
.update(userTableDefinitions)
742-
.set({ archivedAt: now, updatedAt: now })
743-
.where(and(eq(userTableDefinitions.id, tableId), isNull(userTableDefinitions.archivedAt)))
744-
.returning({
745-
createdBy: userTableDefinitions.createdBy,
746-
workspaceId: userTableDefinitions.workspaceId,
747-
name: userTableDefinitions.name,
748-
})
740+
const result = await db.transaction(async (tx) => {
741+
// The pre-check above is a separate round-trip -- an admin could lock this
742+
// table in the window between that check and this transaction. Re-check
743+
// inside the transaction (joining `tx` so the read is part of the same
744+
// atomic unit as the write below) before applying anything.
745+
await assertResourceMutable('table', tableId, tx)
746+
747+
return tx
748+
.update(userTableDefinitions)
749+
.set({ archivedAt: now, updatedAt: now })
750+
.where(and(eq(userTableDefinitions.id, tableId), isNull(userTableDefinitions.archivedAt)))
751+
.returning({
752+
createdBy: userTableDefinitions.createdBy,
753+
workspaceId: userTableDefinitions.workspaceId,
754+
name: userTableDefinitions.name,
755+
})
756+
})
749757

750758
const deleted = result[0]
751759
// Audit only genuine user deletes — rollback callers omit `actingUserId`. The

‎apps/sim/lib/uploads/contexts/workspace/workspace-file-folder-manager.ts‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -961,6 +961,13 @@ export async function archiveWorkspaceFileFolderRecursive(
961961

962962
if (!folder) throw new Error('Folder not found')
963963

964+
// The route checks lock state before calling this function, but that's a
965+
// separate round-trip -- an admin could lock this folder in the window
966+
// between that check and this transaction. Re-check inside the transaction
967+
// (joining `tx` so the read is part of the same atomic unit as the writes
968+
// below) before applying anything.
969+
await assertFolderMutable(folderId, 'file', tx)
970+
964971
const activeFolders = await tx
965972
.select({ id: workspaceFileFolder.id, parentId: workspaceFileFolder.parentId })
966973
.from(workspaceFileFolder)
@@ -1173,6 +1180,17 @@ export async function bulkArchiveWorkspaceFileItems(params: {
11731180
return db.transaction(async (tx) => {
11741181
await acquireWorkspaceFileFolderMutationLock(tx, params.workspaceId)
11751182

1183+
// The route checks lock state before calling this function, but that's a
1184+
// separate round-trip -- an admin could lock a file or folder in the window
1185+
// between that check and this transaction. Re-check inside the transaction
1186+
// (joining `tx`) before applying anything. Safe to check each id in turn
1187+
// without a closure-based ordered lock (unlike the reorder batch fix): the
1188+
// advisory lock above already fully serializes every file-folder mutation
1189+
// for this workspace, so no other transaction can be concurrently
1190+
// acquiring row locks here to race against.
1191+
await Promise.all(explicitFileIds.map((id) => assertResourceMutable('file', id, tx)))
1192+
await Promise.all(explicitFolderIds.map((id) => assertFolderMutable(id, 'file', tx)))
1193+
11761194
const activeFolders =
11771195
explicitFolderIds.length > 0
11781196
? await tx

‎apps/sim/lib/uploads/contexts/workspace/workspace-file-manager.ts‎

Lines changed: 44 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,10 @@ import { randomBytes } from 'crypto'
77
import { db } from '@sim/db'
88
import { workspaceFiles } from '@sim/db/schema'
99
import { createLogger } from '@sim/logger'
10+
import {
11+
assertResourceMutableUnlessUnlocking,
12+
ResourceLockedError,
13+
} from '@sim/platform-authz/resource-lock'
1014
import { getErrorMessage, getPostgresConstraintName, getPostgresErrorCode } from '@sim/utils/errors'
1115
import { generateShortId } from '@sim/utils/id'
1216
import { and, eq, isNull, sql } from 'drizzle-orm'
@@ -989,23 +993,41 @@ export async function renameWorkspaceFile(
989993
}
990994
}
991995

996+
const isLockOnlyUpdate = fileRecord.name === normalizedName
997+
992998
let updated: { id: string }[]
993999
try {
994-
updated = await db
995-
.update(workspaceFiles)
996-
.set({
997-
originalName: normalizedName,
998-
updatedAt: new Date(),
999-
...(locked !== undefined ? { locked } : {}),
1000-
})
1001-
.where(
1002-
and(
1003-
eq(workspaceFiles.id, fileId),
1004-
eq(workspaceFiles.workspaceId, workspaceId),
1005-
eq(workspaceFiles.context, 'workspace')
1000+
updated = await db.transaction(async (tx) => {
1001+
// The caller checks lock state before calling this function, but that's a
1002+
// separate round-trip -- an admin could lock this file in the window
1003+
// between that check and this transaction. Re-check inside the
1004+
// transaction (joining `tx`) before applying anything.
1005+
//
1006+
// `assertResourceMutableUnlessUnlocking` only treats this file's own
1007+
// (about-to-be-cleared) lock as satisfied when this same request unlocks
1008+
// it -- a lock inherited from an ancestor folder still blocks. Skipped
1009+
// entirely for a lock-only update, matching the file/kb/table
1010+
// lock-toggle precedent.
1011+
if (!isLockOnlyUpdate) {
1012+
await assertResourceMutableUnlessUnlocking('file', fileId, locked === false, tx)
1013+
}
1014+
1015+
return tx
1016+
.update(workspaceFiles)
1017+
.set({
1018+
originalName: normalizedName,
1019+
updatedAt: new Date(),
1020+
...(locked !== undefined ? { locked } : {}),
1021+
})
1022+
.where(
1023+
and(
1024+
eq(workspaceFiles.id, fileId),
1025+
eq(workspaceFiles.workspaceId, workspaceId),
1026+
eq(workspaceFiles.context, 'workspace')
1027+
)
10061028
)
1007-
)
1008-
.returning({ id: workspaceFiles.id })
1029+
.returning({ id: workspaceFiles.id })
1030+
})
10091031
} catch (error: unknown) {
10101032
if (getPostgresErrorCode(error) === '23505') {
10111033
throw new FileConflictError(normalizedName)
@@ -1077,6 +1099,14 @@ export async function restoreWorkspaceFile(workspaceId: string, fileId: string):
10771099
throw new Error('Cannot restore file into an archived workspace')
10781100
}
10791101

1102+
// The soft-deleted file's own `locked` flag still gates restoring it -- the
1103+
// generic lock engine only reads active rows, so this direct check (rather
1104+
// than assertResourceMutable, which would find nothing) matches the pattern
1105+
// used by the other resource restore paths.
1106+
if (fileRecord.locked) {
1107+
throw new ResourceLockedError('file', false, 'File is locked')
1108+
}
1109+
10801110
/**
10811111
* A concurrent upload/rename can claim the chosen name after `generateRestoreName`'s check (MVCC).
10821112
* Retries pick a new random suffix; 23505 maps to {@link FileConflictError} after exhaustion.

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

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,9 @@ const logger = createLogger('FolderLifecycle')
2222
/** All queries against `folder` in this module are scoped to workflow folders. */
2323
const isWorkflowFolder = eq(folder.resourceType, 'workflow')
2424

25+
/** Marks a concurrent-deletion race on a target parent, caught inside a folder-update transaction, as a 400 (validation), not a 500. */
26+
class WorkflowFolderParentNotFoundError extends Error {}
27+
2528
export interface PerformCreateFolderParams {
2629
userId: string
2730
workspaceId: string
@@ -215,6 +218,22 @@ export async function performUpdateFolder(
215218
if (params.parentId) {
216219
// A folder in an unlocked location could otherwise be moved into a locked one.
217220
await assertFolderMutable(params.parentId, 'workflow', tx)
221+
222+
// assertFolderMutable's lock walker treats a deleted parent as unlocked (it
223+
// stops the ancestor walk when the row is missing, rather than erroring), so
224+
// it alone doesn't catch a parent deleted between the earlier
225+
// assertFolderParentValid precheck and this transaction. FOR UPDATE here
226+
// makes the recheck race-free: once held, the parent's deletedAt can't
227+
// change until this transaction resolves.
228+
const [targetParent] = await tx
229+
.select({ id: folder.id })
230+
.from(folder)
231+
.where(and(eq(folder.id, params.parentId), isWorkflowFolder, isNull(folder.deletedAt)))
232+
.for('update')
233+
.limit(1)
234+
if (!targetParent) {
235+
throw new WorkflowFolderParentNotFoundError('Parent folder not found')
236+
}
218237
}
219238

220239
const [row] = await tx
@@ -244,6 +263,9 @@ export async function performUpdateFolder(
244263
if (error instanceof ResourceLockedError) {
245264
throw error
246265
}
266+
if (error instanceof WorkflowFolderParentNotFoundError) {
267+
return { success: false, error: error.message, errorCode: 'validation' }
268+
}
247269
if (getPostgresErrorCode(error) === '23505') {
248270
return {
249271
success: false,
@@ -307,7 +329,19 @@ async function deleteFolderRecursively(
307329
stats.workflows += workflowsInFolder.length
308330
}
309331

310-
await db.update(folder).set({ deletedAt: timestamp }).where(eq(folder.id, folderId))
332+
// The route checks lock state before calling this function, but that's a
333+
// separate round-trip, and this recursive cascade makes several of its own
334+
// round-trips per folder besides -- an admin could lock this specific folder
335+
// in the window before this write. A single, tightly-scoped transaction
336+
// around just the recheck and this folder's own write closes that race for
337+
// this row (the cascade as a whole still isn't one atomic unit, since
338+
// archiveWorkflowsByIdsInWorkspace above doesn't accept a `tx`, but each
339+
// folder's own delete now can't land after a lock was set for that folder
340+
// specifically).
341+
await db.transaction(async (tx) => {
342+
await assertFolderMutable(folderId, 'workflow', tx)
343+
await tx.update(folder).set({ deletedAt: timestamp }).where(eq(folder.id, folderId))
344+
})
311345
stats.folders += 1
312346

313347
return stats

0 commit comments

Comments
 (0)