fix(files): lock the workspace row before inserting a workspace file … · simstudioai/sim@2abff53 · GitHub
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

‎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

0 commit comments

Comments
 (0)