Skip to content

Commit 796939f

Browse files
committed
Merge foundation review fixes into Project files
2 parents 1f72d11 + e3e0f52 commit 796939f

20 files changed

Lines changed: 251 additions & 46 deletions

‎.github/scripts/resolve-audit-base.sh‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,5 +37,6 @@ elif [ -n "${GITHUB_BEFORE:-}" ] &&
3737
git fetch --depth=1 origin "$GITHUB_BEFORE"
3838
echo "ref=$GITHUB_BEFORE" >> "$GITHUB_OUTPUT"
3939
else
40+
git fetch --depth=2 origin "$GITHUB_SHA"
4041
echo 'ref=HEAD~1' >> "$GITHUB_OUTPUT"
4142
fi

‎apps/realtime/src/handlers/file-doc-editor.integration.ts‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,30 @@ describe('Project document editor attribution over real Redis', () => {
124124
}
125125
)
126126

127+
check('retirement fences a seed that has not reached Redis yet', async () => {
128+
const name = `project-file-doc:${generateId()}/${generateId()}`
129+
const retiredId = generateId()
130+
const replacementId = generateId()
131+
const retired = new Y.Doc()
132+
const replacement = new Y.Doc()
133+
rooms.push({ name, doc: retired })
134+
retired.getMap(FILE_DOC_SEED.configMap).set(FILE_DOC_SEED.flag, true)
135+
retired.getMap(FILE_DOC_SEED.configMap).set(FILE_DOC_SEED.docIdKey, retiredId)
136+
replacement.getMap(FILE_DOC_SEED.configMap).set(FILE_DOC_SEED.flag, true)
137+
replacement.getMap(FILE_DOC_SEED.configMap).set(FILE_DOC_SEED.docIdKey, replacementId)
138+
try {
139+
expect(await store.retireDocumentGeneration(name, retiredId, replacementId)).toEqual({
140+
status: 'applied',
141+
docId: retiredId,
142+
})
143+
expect(await store.seedIfEmpty(name, Y.encodeStateAsUpdate(retired), 1, true)).toBe(false)
144+
expect(await store.seedIfEmpty(name, Y.encodeStateAsUpdate(replacement), 1, true)).toBe(true)
145+
expect(await store.getDocumentGeneration(name)).toBe(replacementId)
146+
} finally {
147+
replacement.destroy()
148+
}
149+
})
150+
127151
check(
128152
'retiring a generation fences old seeds and delayed retirement preserves the replacement',
129153
async () => {

‎apps/realtime/src/handlers/file-doc-store.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ const SEED_IF_EMPTY_SCRIPT =
6767

6868
/** Preserve a replacement identity even before its seed exists, fencing late seeds for the retired one. */
6969
const RETIRE_DOCUMENT_SCRIPT =
70-
"local generation = redis.call('get', KEYS[2]); if generation ~= ARGV[1] then return false end; redis.call('set', KEYS[2], ARGV[2], 'EX', ARGV[3]); redis.call('del', KEYS[1], KEYS[4], KEYS[5]); redis.call('expire', KEYS[3], ARGV[3]); redis.call('expire', KEYS[6], ARGV[3]); return generation"
70+
"local generation = redis.call('get', KEYS[2]); if generation and generation ~= ARGV[1] then return false end; redis.call('set', KEYS[2], ARGV[2], 'EX', ARGV[3]); redis.call('del', KEYS[1], KEYS[4], KEYS[5]); redis.call('expire', KEYS[3], ARGV[3]); redis.call('expire', KEYS[6], ARGV[3]); return generation or ARGV[1]"
7171

7272
/** Orders a durable replacement with seeds and merges, and fences publishers in the same transaction. */
7373
const INVALIDATE_DOCUMENT_SCRIPT =
@@ -818,7 +818,8 @@ export class FileDocStore {
818818
if (!retiredDocId || !replacementDocId || retiredDocId === replacementDocId)
819819
throw new Error('Document retirement requires distinct identities')
820820
if (!this.enabled) {
821-
if ((await this.getDocumentGeneration(name)) !== retiredDocId) return { status: 'stale' }
821+
const generation = await this.getDocumentGeneration(name)
822+
if (generation && generation !== retiredDocId) return { status: 'stale' }
822823
const previous = this.localInvalidations.get(name)
823824
this.localInvalidations.set(name, {
824825
version: previous?.version ?? 0,

‎apps/sim/lib/projects/account-deletion.ts‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import { ProjectConflictError } from '@/lib/projects/errors'
1111
import { purgeProjectFilesInTx } from '@/lib/projects/files/purge'
1212
import { lockProjectBackfillWrites, lockProjects } from '@/lib/projects/membership'
1313
import {
14+
assertProjectCreatorSuccessor,
1415
findProjectSuccessor,
1516
handoffProjectCreatorReferencesTx,
1617
listSharedResourceProjectIdsForUser,
@@ -115,6 +116,7 @@ export async function getProjectAccountDeletionBlockers(
115116
)
116117
const doomed = new Set(doomedWorkspaceIds)
117118
const blockers: string[] = []
119+
const creatorProjects = new Set(await listSharedResourceProjectIdsForUser(db, userId))
118120
for (const record of records) {
119121
const decision = await planProjectDeletion(
120122
db,
@@ -124,7 +126,25 @@ export async function getProjectAccountDeletionBlockers(
124126
doomed,
125127
false
126128
)
127-
if ('blocker' in decision) blockers.push(decision.blocker)
129+
if ('blocker' in decision) {
130+
blockers.push(decision.blocker)
131+
continue
132+
}
133+
if ('remove' in decision || !creatorProjects.has(record.id)) continue
134+
try {
135+
await assertProjectCreatorSuccessor(
136+
db,
137+
{ ...record, ownerId: decision.ownerId ?? record.ownerId },
138+
userId,
139+
(environments.get(record.id) ?? [])
140+
.filter((row) => !doomed.has(row.id))
141+
.map((row) => row.id),
142+
false
143+
)
144+
} catch (error) {
145+
if (!(error instanceof ProjectConflictError)) throw error
146+
blockers.push(error.message)
147+
}
128148
}
129149
return blockers
130150
}

‎apps/sim/lib/projects/files/__integration__/private-purge.integration.ts‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { mkdtempSync } from 'node:fs'
2-
import { chmod, mkdir, readdir, readFile, rm, writeFile } from 'node:fs/promises'
2+
import { mkdir, readdir, readFile, rm, writeFile } from 'node:fs/promises'
33
import { tmpdir } from 'node:os'
44
import { dirname, join } from 'node:path'
55
import { db } from '@sim/db'
@@ -39,6 +39,7 @@ import { processOutboxEventById } from '@/lib/core/outbox/service'
3939
import { prepareProjectsForAccountDeletion } from '@/lib/projects/account-deletion'
4040
import { createProjectFileUploadSession } from '@/lib/projects/files/application/uploads'
4141
import { workspaceFileStorageCleanupOutboxHandlers } from '@/lib/uploads/contexts/workspace/workspace-file-storage-cleanup-outbox'
42+
import * as storageService from '@/lib/uploads/core/storage-service'
4243
import { UPLOAD_URL_TTL_MS } from '@/lib/uploads/upload-session/provider'
4344
import { PROJECT_FILE_UPLOAD_BINDING_KEY } from '@/lib/uploads/upload-session/types'
4445
import { deleteUserAccount } from '@/lib/users/account-deletion'
@@ -673,7 +674,15 @@ describe('Private Project teardown and durable object cleanup', () => {
673674
await mkdir(prefix, { recursive: true })
674675
await writeFile(join(prefix, 'late.bin'), 'late')
675676
}
676-
await chmod(obstructed, 0o000)
677+
const deleteStoredFile = storageService.deleteFile
678+
const deletion = vi.spyOn(storageService, 'deleteFile').mockImplementation((params) => {
679+
if (params.key.startsWith(`project/${orphanIds[0]}/`)) {
680+
return Promise.reject(
681+
Object.assign(new Error('Fixture prefix deletion failed'), { code: 'EACCES' })
682+
)
683+
}
684+
return deleteStoredFile(params)
685+
})
677686
const { projectFilePrefixCleanupOutboxHandlers } = await import(
678687
'@/lib/projects/files/prefix-cleanup'
679688
)
@@ -718,7 +727,7 @@ describe('Private Project teardown and durable object cleanup', () => {
718727
})
719728
expect(await readFile(join(storageRoot, live.keys[0]), 'utf8')).toBe('new')
720729
} finally {
721-
await chmod(obstructed, 0o755)
730+
deletion.mockRestore()
722731
}
723732
expect(await readFile(join(obstructed, 'late.bin'), 'utf8')).toBe('late')
724733
await db

‎apps/sim/lib/projects/resource-handoff.ts‎

Lines changed: 25 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -86,25 +86,23 @@ export async function listSharedResourceProjectIdsForUser(executor: DbOrTx, user
8686
}
8787

8888
/** Validates the canonical Project owner rather than borrowing an environment's payer. */
89-
export async function handoffProjectCreatorReferencesTx(
90-
tx: DbTransaction,
89+
export async function assertProjectCreatorSuccessor(
90+
executor: DbOrTx,
9191
record: typeof project.$inferSelect,
9292
departingUserId: string,
93-
environmentIds: string[]
93+
environmentIds: string[],
94+
hold: boolean
9495
) {
9596
const successorId = record.ownerId
9697
if (successorId === departingUserId)
9798
throw new ProjectConflictError(
9899
'Project resources need a successor before this account can leave'
99100
)
100-
const [successor] = await tx
101-
.select({ id: user.id })
102-
.from(user)
103-
.where(eq(user.id, successorId))
104-
.for('key share')
101+
const successorQuery = executor.select({ id: user.id }).from(user).where(eq(user.id, successorId))
102+
const [successor] = await (hold ? successorQuery.for('key share') : successorQuery)
105103
if (!successor) throw new ProjectConflictError('Project successor changed; retry the operation')
106-
const [admin] = record.organizationId
107-
? await tx
104+
const adminQuery = record.organizationId
105+
? executor
108106
.select({ id: member.id })
109107
.from(member)
110108
.where(
@@ -114,11 +112,11 @@ export async function handoffProjectCreatorReferencesTx(
114112
inArray(member.role, ORG_ADMIN_ROLES)
115113
)
116114
)
117-
.for('share')
118-
: []
115+
: null
116+
const [admin] = adminQuery ? await (hold ? adminQuery.for('share') : adminQuery) : []
119117
if (!admin) {
120-
const grants = environmentIds.length
121-
? await tx
118+
const grantQuery = environmentIds.length
119+
? executor
122120
.select({ id: permissions.entityId })
123121
.from(permissions)
124122
.where(
@@ -129,13 +127,24 @@ export async function handoffProjectCreatorReferencesTx(
129127
inArray(permissions.entityId, environmentIds)
130128
)
131129
)
132-
.for('share')
133-
: []
130+
: null
131+
const grants = grantQuery ? await (hold ? grantQuery.for('share') : grantQuery) : []
134132
if (!environmentIds.length || grants.length !== environmentIds.length)
135133
throw new ProjectConflictError(
136134
'Project successor must administer every surviving environment'
137135
)
138136
}
137+
}
138+
139+
/** Transfers creator references only after the canonical successor is held and validated. */
140+
export async function handoffProjectCreatorReferencesTx(
141+
tx: DbTransaction,
142+
record: typeof project.$inferSelect,
143+
departingUserId: string,
144+
environmentIds: string[]
145+
) {
146+
await assertProjectCreatorSuccessor(tx, record, departingUserId, environmentIds, true)
147+
const successorId = record.ownerId
139148
await handoffFileCreatorsInTx(
140149
tx,
141150
and(eq(workspaceFiles.projectId, record.id), eq(workspaceFiles.userId, departingUserId)),

‎apps/sim/lib/uploads/contexts/workspace/__integration__/creator-lifetime.integration.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,9 @@ it('partial departure retains Project attribution; successor handoff and prior-d
165165
context: 'project',
166166
folderId: projectFolderId,
167167
})
168+
await expect(db.delete(user).where(eq(user.id, creatorId))).rejects.toMatchObject({
169+
cause: { code: '23514', constraint_name: 'project_resource_creator_handoff' },
170+
})
168171
await db.transaction((tx) =>
169172
changeProjectStoragePayersInTx(tx, [
170173
{

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

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -891,7 +891,6 @@ export async function createOwnedWorkspaceFile<T>(params: {
891891
const name = params.fileName(fileId)
892892
const buffer = Buffer.from(params.content, 'utf-8')
893893
const storageKey = generateWorkspaceFileKey(params.workspaceId, name)
894-
const storageBillingContext = await resolveStorageBillingContext(params.workspaceId)
895894
const uploadResult = await uploadFile({
896895
file: buffer,
897896
fileName: storageKey,
@@ -909,7 +908,11 @@ export async function createOwnedWorkspaceFile<T>(params: {
909908
persistMetadata: false,
910909
})
911910
try {
912-
const { owner, updatedUsage } = await db.transaction(async (tx) => {
911+
const { owner, updatedUsage, billing } = await db.transaction(async (tx) => {
912+
const accounting = await prepareFileAccountingInTx(tx, {
913+
entityType: 'workspace',
914+
entityId: params.workspaceId,
915+
})
913916
const inserted = await insertWorkspaceFileMetadataInTx(tx, {
914917
id: fileId,
915918
key: uploadResult.key,
@@ -922,14 +925,14 @@ export async function createOwnedWorkspaceFile<T>(params: {
922925
context: params.context,
923926
})
924927
if (!inserted) throw new Error(`Owned ${params.context} file ${fileId} was not inserted`)
925-
const updatedUsage = await incrementStorageUsageForBillingContextInTx(
926-
tx,
927-
storageBillingContext,
928-
buffer.length
929-
)
930-
return { owner: await params.insertOwner(tx, fileId), updatedUsage }
928+
const updatedUsage = await accounting.mutation.applyDelta(buffer.length)
929+
return {
930+
owner: await params.insertOwner(tx, fileId),
931+
updatedUsage,
932+
billing: accounting.billing,
933+
}
931934
})
932-
void maybeNotifyStorageLimitForBillingContext(storageBillingContext, updatedUsage)
935+
void maybeNotifyStorageLimitForBillingContext(billing, updatedUsage)
933936
return { fileId, owner }
934937
} catch (error) {
935938
await cleanupWorkspaceStorageObject(
Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
import { describe, expect, it } from 'vitest'
2+
import { fileDocumentInputIdentity } from '@/lib/uploads/documents/input-identity'
3+
4+
describe('compiled document input identity', () => {
5+
it('is independent of dependency query order and still changes with a revision', () => {
6+
const owner = { entityType: 'project', entityId: 'project-1' } as const
7+
const files = ['b', 'a'].map((id) => ({
8+
id,
9+
key: `project/project-1/${id}`,
10+
contentUpdatedAt: new Date(1),
11+
sizeBytes: 1,
12+
}))
13+
const identity = fileDocumentInputIdentity(owner, files)
14+
expect(fileDocumentInputIdentity(owner, files.toReversed())).toBe(identity)
15+
expect(
16+
fileDocumentInputIdentity(owner, [{ ...files[0], contentUpdatedAt: new Date(2) }, files[1]])
17+
).not.toBe(identity)
18+
expect(files.map((file) => file.id)).toEqual(['b', 'a'])
19+
})
20+
})
Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,21 +1,24 @@
11
import type { WorkspaceFileRow } from '@sim/db/schema'
2+
import { compareStrings } from '@sim/utils/string'
23
import type { EditableFileOwner } from '@/lib/workspace-files/ownership'
34

45
/** Binds a rendered cache entry to every authorized input revision, in canonical file order. */
56
export function fileDocumentInputIdentity(
67
owner: EditableFileOwner,
7-
files: readonly WorkspaceFileRow[]
8+
files: readonly Pick<WorkspaceFileRow, 'id' | 'key' | 'contentUpdatedAt' | 'sizeBytes'>[]
89
) {
910
return files.length > 0
1011
? JSON.stringify({
1112
version: 1,
12-
inputs: files.map((file) => ({
13-
fileId: file.id,
14-
key: file.key,
15-
context: owner.entityType,
16-
contentVersion: file.contentUpdatedAt.toISOString(),
17-
size: file.sizeBytes,
18-
})),
13+
inputs: files
14+
.toSorted((left, right) => compareStrings(left.id, right.id))
15+
.map((file) => ({
16+
fileId: file.id,
17+
key: file.key,
18+
context: owner.entityType,
19+
contentVersion: file.contentUpdatedAt.toISOString(),
20+
size: file.sizeBytes,
21+
})),
1922
})
2023
: undefined
2124
}

0 commit comments

Comments
 (0)