Skip to content

Commit 2abff53

Browse files
authored
fix(files): lock the workspace row before inserting a workspace file (#8972)
* fix(files): lock the workspace row before inserting a workspace file Upload, upload-session registration, owned-file creation, and fork file copy inserted the workspace_files row first and locked the workspace row only in the later storage increment. The insert's search-index trigger upserts the per-workspace workspace_file_search_dispatch_queue row, so these transactions held the queue row while waiting on the workspace row. updateWorkspaceFileContent takes the same two locks in the opposite order (workspace row, then the content UPDATE whose trigger reaches the queue row), and the two deadlocked. Take lockWorkspaceStorageForMutationInTx before the insert in each path so every storage-charging file write locks folder -> workspace row -> file rows/queue -> payer, and document that order on the lock helper. * improvement(files): take the workspace row lock inside the shared file-insert helper * fix(files): skip the lock-order test without the search trigger and scope the folder-lock wording
1 parent b4a172c commit 2abff53

4 files changed

Lines changed: 182 additions & 1 deletion

File tree

‎apps/sim/ee/workspace-forking/lib/copy/copy-files.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import { generateId } from '@sim/utils/id'
66
import { and, eq, inArray, isNull, or } from 'drizzle-orm'
77
import {
88
incrementStorageUsageForBillingContextInTx,
9+
lockWorkspaceStorageForMutationInTx,
910
resolveStorageBillingContext,
1011
} from '@/lib/billing/storage'
1112
import type { DbOrTx } from '@/lib/db/types'
@@ -360,6 +361,7 @@ export async function executeForkFileBlobCopies(
360361
assertForkCopyActive(control)
361362
await db.transaction(async (tx) => {
362363
assertForkCopyActive(control)
364+
await lockWorkspaceStorageForMutationInTx(tx, task.workspaceId)
363365
const [inserted] = await tx
364366
.insert(workspaceFiles)
365367
.values({

‎apps/sim/lib/billing/storage/tracking.ts‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -430,7 +430,17 @@ export async function applyStorageUsageDeltasInTx(
430430
return destinationUpdatedUsage
431431
}
432432

433-
/** Locks the canonical workspace before existing-file mutations that later change its storage ledger. */
433+
/**
434+
* Locks the canonical workspace row before a file mutation that changes its storage ledger.
435+
*
436+
* Every transaction that writes `workspace_files` and charges storage takes its locks in one order:
437+
* the folder advisory lock (only for operations that take one), then this workspace row, then file
438+
* rows, then the file writes (whose search-index trigger upserts the one-per-workspace
439+
* `workspace_file_search_dispatch_queue` row), then the payer row. Call this before the first file insert or update: an insert that reaches
440+
* the queue row first and only then waits on the workspace row deadlocks with a content write
441+
* that holds the workspace row and waits on the queue row. Re-locking later in the same
442+
* transaction, as the storage increment does, never waits.
443+
*/
434444
export async function lockWorkspaceStorageForMutationInTx(
435445
tx: DbOrTx,
436446
workspaceId: string
Lines changed: 167 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,167 @@
1+
/**
2+
* Real PostgreSQL lock ordering between new-file finalization and a concurrent content write in
3+
* the same workspace. Both reach the workspace row and the per-workspace search dispatch queue row
4+
* (through the `workspace_files` search-index trigger), so they must take them in the same order.
5+
*/
6+
import { mkdtempSync } from 'node:fs'
7+
import { rm } from 'node:fs/promises'
8+
import { tmpdir } from 'node:os'
9+
import path from 'node:path'
10+
import { db, dbFor } from '@sim/db'
11+
import {
12+
organization,
13+
user,
14+
workspace,
15+
workspaceFileSearchDispatchQueue,
16+
workspaceFiles,
17+
} from '@sim/db/schema'
18+
import { deleteWorkspaceFixture } from '@sim/db/testing/workspace-fixtures'
19+
import { createDeferred } from '@sim/testing/helpers/deferred'
20+
import { generateId } from '@sim/utils/id'
21+
import { eq, inArray, sql } from 'drizzle-orm'
22+
import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest'
23+
24+
const fixtureStorage = vi.hoisted(() => ({ root: '' }))
25+
vi.mock('@/lib/uploads/core/setup.server', () => ({
26+
get UPLOAD_DIR_SERVER() {
27+
return fixtureStorage.root
28+
},
29+
}))
30+
31+
import { lockWorkspaceStorageForMutationInTx } from '@/lib/billing/storage'
32+
import {
33+
createKnowledgeAclFixtureIds,
34+
seedKnowledgeAclFixture,
35+
} from '@/lib/knowledge/__integration__/seed-source-access-fixture'
36+
import {
37+
generateWorkspaceFileKey,
38+
registerUploadedWorkspaceFile,
39+
uploadWorkspaceFile,
40+
} from '@/lib/uploads/contexts/workspace/workspace-file-manager'
41+
import { uploadFile } from '@/lib/uploads/core/storage-service'
42+
43+
type FinalizeNewFile = (workspaceId: string, userId: string) => Promise<unknown>
44+
45+
const finalizeByUpload: FinalizeNewFile = (workspaceId, userId) => {
46+
const name = `upload-${generateId()}.txt`
47+
return uploadWorkspaceFile(workspaceId, userId, Buffer.from(name), name, 'text/plain', {
48+
notifyWorkspaceChange: false,
49+
})
50+
}
51+
52+
const finalizeByRegistration: FinalizeNewFile = async (workspaceId, userId) => {
53+
const name = `registered-${generateId()}.txt`
54+
const key = generateWorkspaceFileKey(workspaceId, name)
55+
await uploadFile({
56+
file: Buffer.from(name),
57+
fileName: key,
58+
contentType: 'text/plain',
59+
context: 'workspace',
60+
preserveKey: true,
61+
customKey: key,
62+
persistMetadata: false,
63+
})
64+
return registerUploadedWorkspaceFile({
65+
workspaceId,
66+
userId,
67+
key,
68+
originalName: name,
69+
contentType: 'text/plain',
70+
})
71+
}
72+
73+
/** `db:push` never installs the migration-only search-index trigger that reaches the queue row. */
74+
const [{ migrated }] = await db.execute<{ migrated: boolean }>(sql`SELECT EXISTS (
75+
SELECT 1 FROM pg_trigger
76+
WHERE tgrelid = 'workspace_files'::regclass AND tgname = 'workspace_files_search_index_pending'
77+
) AS migrated`)
78+
/** The suite's own `afterAll` never runs when it is skipped, so release the probe connection here. */
79+
if (!migrated) await db.$client.end()
80+
81+
describe.skipIf(!migrated)('workspace file storage lock order in PostgreSQL', () => {
82+
const fixtures: ReturnType<typeof createKnowledgeAclFixtureIds>[] = []
83+
84+
beforeAll(() => {
85+
fixtureStorage.root = mkdtempSync(path.join(tmpdir(), 'sim-file-lock-order-'))
86+
})
87+
88+
afterAll(async () => {
89+
for (const ids of fixtures) {
90+
await deleteWorkspaceFixture(db, eq(workspace.id, ids.workspaceId))
91+
await db.delete(organization).where(eq(organization.id, ids.organizationId))
92+
await db.delete(user).where(inArray(user.id, [ids.aliceId, ids.bobId]))
93+
}
94+
await rm(fixtureStorage.root, { recursive: true, force: true })
95+
await Promise.all([db.$client.end(), dbFor('cleanup').$client.end()])
96+
})
97+
98+
/** Seeds a workspace with one file, which also creates the workspace's dispatch queue row. */
99+
async function seedWorkspaceWithFile() {
100+
const ids = createKnowledgeAclFixtureIds()
101+
fixtures.push(ids)
102+
await seedKnowledgeAclFixture(ids)
103+
const name = `existing-${generateId()}.txt`
104+
const existing = await uploadWorkspaceFile(
105+
ids.workspaceId,
106+
ids.aliceId,
107+
Buffer.from(name),
108+
name,
109+
'text/plain',
110+
{ notifyWorkspaceChange: false }
111+
)
112+
const queued = await db
113+
.select({ workspaceId: workspaceFileSearchDispatchQueue.workspaceId })
114+
.from(workspaceFileSearchDispatchQueue)
115+
.where(eq(workspaceFileSearchDispatchQueue.workspaceId, ids.workspaceId))
116+
expect(queued).toHaveLength(1)
117+
return { ...ids, existingFileId: existing.id }
118+
}
119+
120+
it.each([
121+
['uploadWorkspaceFile', finalizeByUpload],
122+
['registerUploadedWorkspaceFile', finalizeByRegistration],
123+
])(
124+
'%s commits alongside a content write that already holds the workspace row',
125+
async (_name, finalize) => {
126+
const fixture = await seedWorkspaceWithFile()
127+
const ready = createDeferred<number>()
128+
const finalizeIsWaiting = createDeferred<void>()
129+
130+
// The lock sequence of `updateWorkspaceFileContent`: workspace row, then a content write.
131+
const contentWrite = db.transaction(async (tx) => {
132+
await lockWorkspaceStorageForMutationInTx(tx, fixture.workspaceId)
133+
const [connection] = await tx.execute<{ pid: number }>(sql`SELECT pg_backend_pid() AS pid`)
134+
ready.resolve(connection.pid)
135+
await finalizeIsWaiting.promise
136+
await tx
137+
.update(workspaceFiles)
138+
.set({ contentUpdatedAt: new Date(Date.now() + 1000) })
139+
.where(eq(workspaceFiles.id, fixture.existingFileId))
140+
})
141+
const contentWritePid = await ready.promise
142+
143+
const finalized = finalize(fixture.workspaceId, fixture.aliceId)
144+
const settled = Promise.allSettled([contentWrite, finalized])
145+
try {
146+
await expect
147+
.poll(
148+
async () =>
149+
(
150+
await db.execute(
151+
sql`SELECT pid FROM pg_stat_activity
152+
WHERE ${contentWritePid} = ANY(pg_blocking_pids(pid))`
153+
)
154+
).length,
155+
{ timeout: 4000 }
156+
)
157+
.toBe(1)
158+
} finally {
159+
finalizeIsWaiting.resolve()
160+
await settled
161+
}
162+
163+
const results = await settled
164+
expect(results.filter((result) => result.status === 'rejected')).toEqual([])
165+
}
166+
)
167+
})

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -310,11 +310,13 @@ interface WorkspaceFileMetadataInsert {
310310
* Attempts one active workspace-file insert and reports the row that this call
311311
* created. Conflict losers receive `undefined` and must inspect the active key
312312
* in the same transaction before deciding whether the operation is idempotent.
313+
* Locks the workspace row first so the insert follows the storage lock order.
313314
*/
314315
async function insertWorkspaceFileMetadataInTx(
315316
tx: DbOrTx,
316317
metadata: WorkspaceFileMetadataInsert
317318
): Promise<WorkspaceFileRow | undefined> {
319+
await lockWorkspaceStorageForMutationInTx(tx, metadata.workspaceId)
318320
const [inserted] = await tx
319321
.insert(workspaceFiles)
320322
.values({

0 commit comments

Comments
 (0)