Skip to content

Commit 0c8f5d4

Browse files
committed
fix(workspaces): hand off a departing member's files without binding every file id
Member removal transferred file creators by selecting the departing member's file ids and updating with an id list, so a member owning more files than Postgres's 65,535 bind parameters failed the whole removal transaction. Every statement in the file creator handoff is now keyed by the caller's scope predicate: rows are locked in id order through a subquery, version heads load through a subquery, and the final update matches the same predicate. A versioned file committed after the history pass keeps its creator until its history is frozen.
1 parent 5536549 commit 0c8f5d4

3 files changed

Lines changed: 107 additions & 31 deletions

File tree

Lines changed: 31 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,31 +1,49 @@
11
import { workspaceFiles } from '@sim/db/schema'
2-
import { asc, inArray, type SQL } from 'drizzle-orm'
2+
import { and, asc, count, inArray, not, or, type SQL } from 'drizzle-orm'
33
import type { DbTransaction } from '@/lib/db/types'
44
import { snapshotWorkspaceFileSecretProvenanceInTx } from '@/lib/uploads/contexts/workspace/file-secret-provenance'
55
import {
66
isVersionHeadCurrent,
77
loadWorkspaceFileVersionHeads,
88
materializeWorkspaceFileVersionInTx,
9+
workspaceFileVersionHeadCurrentSql,
910
} from '@/lib/uploads/contexts/workspace/file-version-store'
1011

11-
/** Freezes implicit history under the writer lock before replacing a shared file's live creator. */
12+
/** Contexts whose files keep version history, frozen before their creator changes. */
13+
const VERSIONED_FILE_CONTEXTS = ['workspace', 'project']
14+
15+
/**
16+
* Freezes implicit history under the writer lock before replacing a shared file's live creator.
17+
* Every statement is keyed by `condition`, never by a list of file ids: a departing member can
18+
* own more files than Postgres accepts bind parameters.
19+
*/
1220
export async function handoffFileCreatorsInTx(
1321
tx: DbTransaction,
1422
condition: SQL | undefined,
1523
successorId: string
1624
) {
1725
if (!condition) throw new Error('File creator handoff requires a scope')
18-
const files = await tx
19-
.select()
26+
// Takes every row lock in id order without loading the rows into memory.
27+
const locked = tx
28+
.select({ id: workspaceFiles.id })
2029
.from(workspaceFiles)
2130
.where(condition)
2231
.orderBy(asc(workspaceFiles.id))
2332
.for('update')
24-
const versioned = files.filter(
25-
(file) => file.context === 'workspace' || file.context === 'project'
26-
)
33+
.as('locked')
34+
await tx.select({ count: count() }).from(locked)
35+
36+
const isVersioned = inArray(workspaceFiles.context, VERSIONED_FILE_CONTEXTS)
37+
const versionedCondition = and(condition, isVersioned)
38+
// Also locks a versioned file committed after the lock pass before its history is frozen.
39+
const versioned = await tx
40+
.select()
41+
.from(workspaceFiles)
42+
.where(versionedCondition)
43+
.orderBy(asc(workspaceFiles.id))
44+
.for('update')
2745
const heads = await loadWorkspaceFileVersionHeads(
28-
versioned.map((file) => file.id),
46+
tx.select({ id: workspaceFiles.id }).from(workspaceFiles).where(versionedCondition),
2947
tx
3048
)
3149
for (const file of versioned) {
@@ -42,14 +60,15 @@ export async function handoffFileCreatorsInTx(
4260
)
4361
await materializeWorkspaceFileVersionInTx(tx, file, head, provenance, new Date())
4462
}
45-
if (!files.length) return
63+
4664
await tx
4765
.update(workspaceFiles)
4866
.set({ userId: successorId, updatedAt: new Date() })
4967
.where(
50-
inArray(
51-
workspaceFiles.id,
52-
files.map((file) => file.id)
68+
and(
69+
condition,
70+
// A versioned file committed after the history pass keeps its creator until frozen.
71+
or(not(isVersioned), workspaceFileVersionHeadCurrentSql())
5372
)
5473
)
5574
}

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

Lines changed: 49 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,17 @@ import {
77
workspaceFileVersion,
88
} from '@sim/db/schema'
99
import { generateId } from '@sim/utils/id'
10-
import { and, desc, eq, inArray, isNotNull, lt, notInArray, sql } from 'drizzle-orm'
10+
import {
11+
and,
12+
desc,
13+
eq,
14+
inArray,
15+
isNotNull,
16+
lt,
17+
notInArray,
18+
type SQLWrapper,
19+
sql,
20+
} from 'drizzle-orm'
1121
import {
1222
type CursorKey,
1323
type KeysetKey,
@@ -105,14 +115,14 @@ export async function loadWorkspaceFileVersionHead(
105115

106116
/**
107117
* Batched {@link loadWorkspaceFileVersionHead}: each file's highest version, keyed by file id.
108-
* The max is resolved from the `(file_id, version)` index first, so only head rows are fetched
109-
* rather than every retained version.
118+
* `fileIds` is a subquery, so the file set never becomes a bind-parameter list. The max is
119+
* resolved from the `(file_id, version)` index first, so only head rows are fetched rather than
120+
* every retained version.
110121
*/
111122
export async function loadWorkspaceFileVersionHeads(
112-
fileIds: string[],
123+
fileIds: SQLWrapper,
113124
executor: DbOrTx = db
114125
): Promise<Map<string, WorkspaceFileVersionSummaryRow>> {
115-
if (!fileIds.length) return new Map()
116126
const heads = await executor
117127
.select(versionSummaryColumns)
118128
.from(workspaceFileVersion)
@@ -713,27 +723,36 @@ export function getCurrentWorkspaceFileVersion(
713723
return withVersionSnapshot(file, currentVersionInSnapshot, executor)
714724
}
715725

726+
/**
727+
* Correlated columns for subqueries over the enclosing query's `workspace_files` row. Both sides
728+
* are table-qualified because Drizzle renders single-table columns bare, which would bind the outer
729+
* columns to the subquery's own table.
730+
*/
731+
function correlatedVersionColumns() {
732+
const qualified = (table: typeof workspaceFileVersion | typeof workspaceFiles, name: string) =>
733+
sql`${table}.${sql.identifier(name)}`
734+
return {
735+
head: {
736+
fileId: qualified(workspaceFileVersion, workspaceFileVersion.fileId.name),
737+
version: qualified(workspaceFileVersion, workspaceFileVersion.version.name),
738+
key: qualified(workspaceFileVersion, workspaceFileVersion.key.name),
739+
supersededAt: qualified(workspaceFileVersion, workspaceFileVersion.supersededAt.name),
740+
},
741+
file: {
742+
id: qualified(workspaceFiles, workspaceFiles.id.name),
743+
key: qualified(workspaceFiles, workspaceFiles.key.name),
744+
},
745+
}
746+
}
747+
716748
/**
717749
* The current version number of the enclosing query's `workspace_files` row, as a correlated
718750
* subquery so the row and its number come from one statement's snapshot. The newest row numbers
719751
* the file's bytes while it still holds them; otherwise the bytes are on the implicit version after
720752
* it, and a file with no rows is on version 1 — the numbering {@link implicitCurrentVersion} gives.
721-
* Both sides of the correlation are table-qualified because Drizzle renders single-table columns
722-
* bare, which would bind the outer columns to this subquery's own table.
723753
*/
724754
export function currentWorkspaceFileVersionNumberSql() {
725-
const qualified = (table: typeof workspaceFileVersion | typeof workspaceFiles, name: string) =>
726-
sql`${table}.${sql.identifier(name)}`
727-
const head = {
728-
fileId: qualified(workspaceFileVersion, workspaceFileVersion.fileId.name),
729-
version: qualified(workspaceFileVersion, workspaceFileVersion.version.name),
730-
key: qualified(workspaceFileVersion, workspaceFileVersion.key.name),
731-
supersededAt: qualified(workspaceFileVersion, workspaceFileVersion.supersededAt.name),
732-
}
733-
const file = {
734-
id: qualified(workspaceFiles, workspaceFiles.id.name),
735-
key: qualified(workspaceFiles, workspaceFiles.key.name),
736-
}
755+
const { head, file } = correlatedVersionColumns()
737756
const number = sql`case when ${head.supersededAt} is null and ${head.key} = ${file.key} then ${head.version} else ${head.version} + 1 end`
738757
return sql<number>`coalesce((
739758
select ${number} from ${workspaceFileVersion}
@@ -743,6 +762,17 @@ export function currentWorkspaceFileVersionNumberSql() {
743762
), ${INITIAL_WORKSPACE_FILE_VERSION})`.mapWith(Number)
744763
}
745764

765+
/** {@link isVersionHeadCurrent} for the enclosing query's `workspace_files` row. */
766+
export function workspaceFileVersionHeadCurrentSql() {
767+
const { head, file } = correlatedVersionColumns()
768+
return sql<boolean>`coalesce((
769+
select ${head.supersededAt} is null and ${head.key} = ${file.key} from ${workspaceFileVersion}
770+
where ${head.fileId} = ${file.id}
771+
order by ${head.version} desc
772+
limit 1
773+
), false)`
774+
}
775+
746776
/** One version of a file, or null when it never existed or retention removed it. */
747777
export function getWorkspaceFileVersion(
748778
file: WorkspaceFileVersionSubject,

‎apps/sim/lib/workspaces/access/shared-resource-handoff.integration.ts‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -492,6 +492,33 @@ describe('shared resource retention across workspace departure and account erasu
492492
).toMatchObject([{ version: 1, authorUserIds: [fixture.departingId] }])
493493
})
494494

495+
it('hands off more files than Postgres accepts bind parameters', async () => {
496+
const fixture = await seedResources(false)
497+
/** More ids than Postgres's 65,535 bind parameters per statement. */
498+
const bulkFileCount = 66_000
499+
await db.execute(sql`
500+
INSERT INTO ${workspaceFiles} (id, key, user_id, workspace_id, context, original_name, content_type)
501+
SELECT ${fixture.workspaceId} || '-kb-' || n, ${fixture.workspaceId} || '/kb/' || n,
502+
${fixture.departingId}, ${fixture.workspaceId}, 'knowledge-base', 'bulk.txt', 'text/plain'
503+
FROM generate_series(1, ${bulkFileCount}) AS n
504+
`)
505+
const result = await db.transaction((tx) =>
506+
revokeWorkspaceAccessTx(tx, {
507+
workspaceId: fixture.workspaceId,
508+
userId: fixture.departingId,
509+
})
510+
)
511+
expect(result.revoked).toBe(true)
512+
expect(
513+
await db
514+
.select({ userId: workspaceFiles.userId, files: sql<number>`count(*)::int` })
515+
.from(workspaceFiles)
516+
.where(eq(workspaceFiles.workspaceId, fixture.workspaceId))
517+
.groupBy(workspaceFiles.userId)
518+
).toEqual([{ userId: fixture.ownerId, files: bulkFileCount + 1 }])
519+
await assertTransferred(fixture)
520+
})
521+
495522
it('repairs references left by departures before the handoff fix', async () => {
496523
const fixture = await seedResources(true, false)
497524
await deleteUserAccount(fixture.departingId, { recordIdentity: false })

0 commit comments

Comments
 (0)