Skip to content

Commit a286f0a

Browse files
committed
fix(cleanup): delete an organization attachment once no chat references it
Organization Chat attachments (assistant/ keys) have no workspace_files row and a fork carries the same key, so the purge now deletes one under its own storage context only after confirming no remaining chat of that organization still references it.
1 parent 5298c1c commit a286f0a

2 files changed

Lines changed: 111 additions & 15 deletions

File tree

‎apps/sim/lib/cleanup/chat-cleanup.test.ts‎

Lines changed: 29 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,15 +15,25 @@ function attachment(key: string) {
1515
return { id: 'wf_x', key, filename: 'x.png', media_type: 'image/png', size: 1 }
1616
}
1717

18-
/** Purges one deleted chat whose message rows are `messages`; returns the deleted keys by context. */
19-
async function purge(messages: Record<string, unknown>[]) {
18+
/**
19+
* Purges one deleted chat whose message rows are `messages`, while `remaining` are the message
20+
* rows other chats of the organization still hold; returns the deleted keys by context.
21+
*/
22+
async function purge(
23+
messages: Record<string, unknown>[],
24+
remaining: Record<string, unknown>[] = []
25+
) {
2026
queueTableRows(workspaceFiles, [])
2127
queueTableRows(
2228
copilotMessages,
2329
messages.map((content) => ({ chatId, content }))
2430
)
2531
const cleanup = await prepareChatCleanup([chatId], 'test')
2632
queueTableRows(copilotChats, [])
33+
queueTableRows(
34+
copilotMessages,
35+
remaining.map((content) => ({ content }))
36+
)
2737
await cleanup.execute()
2838
return Object.fromEntries(
2939
storageServiceMockFns.mockDeleteFiles.mock.calls.map(([keys, context]) => [context, keys])
@@ -38,7 +48,7 @@ describe('chat purge storage', () => {
3848
storageServiceMockFns.mockDeleteFiles.mockResolvedValue({ deleted: 1, failed: [] })
3949
})
4050

41-
it('deletes only copilot-storage attachment keys as copilot storage', async () => {
51+
it('never deletes a workspace attachment as copilot storage', async () => {
4252
// A fork carries its source's key when the file's copy failed or the file was deleted, and
4353
// the copilot bucket can be the workspace bucket, so a workspace key here is another chat's file.
4454
const deleted = await purge([
@@ -47,14 +57,29 @@ describe('chat purge storage', () => {
4757
content: 'look',
4858
fileAttachments: [
4959
attachment('workspace/ws-1/1700-abc-shared.png'),
50-
attachment('assistant/org-1/user-1/u1/shared.png'),
5160
attachment('copilot/1234/legacy.png'),
5261
],
5362
},
5463
])
5564
expect(deleted).toEqual({ copilot: ['copilot/1234/legacy.png'] })
5665
})
5766

67+
it('deletes an organization attachment only once no remaining chat references it', async () => {
68+
const shared = 'assistant/org-1/user-1/u1/shared.png'
69+
const own = 'assistant/org-1/user-1/u2/own.png'
70+
const message = {
71+
role: 'user',
72+
content: 'look',
73+
fileAttachments: [attachment(shared), attachment(own)],
74+
}
75+
// A fork of this chat still holds the shared upload.
76+
const deleted = await purge(
77+
[message],
78+
[{ role: 'user', content: 'fork', fileAttachments: [attachment(shared)] }]
79+
)
80+
expect(deleted).toEqual({ mothership: [own] })
81+
})
82+
5883
it('deletes the inline images an assistant message published', async () => {
5984
const deleted = await purge([
6085
{

‎apps/sim/lib/cleanup/chat-cleanup.ts‎

Lines changed: 82 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { copilotChats, copilotMessages, workspaceFiles } from '@sim/db/schema'
33
import { createLogger } from '@sim/logger'
44
import { chunkArray } from '@sim/utils/helpers'
55
import { isRecordLike } from '@sim/utils/object'
6-
import { and, inArray, isNull } from 'drizzle-orm'
6+
import { and, eq, inArray, isNull, or, sql } from 'drizzle-orm'
77
import { env } from '@/lib/core/config/env'
88
import {
99
inlineChatImageKey,
@@ -36,6 +36,19 @@ interface FileRef {
3636
key: string
3737
context: ChatScopedContext
3838
chatId: string
39+
/**
40+
* An organization Chat attachment (`assistant/<orgId>/…`): no `workspace_files` row owns it,
41+
* and a fork carries the same key, so it is deleted only once no remaining chat of that
42+
* organization references it.
43+
*/
44+
organizationId?: string
45+
}
46+
47+
/** The organization an `assistant/<orgId>/…` attachment key was uploaded under. */
48+
function organizationAttachmentOwner(key: string): string | undefined {
49+
if (tryInferContextFromKey(key) !== 'mothership') return undefined
50+
const [, organizationId] = key.split('/')
51+
return organizationId || undefined
3952
}
4053

4154
/**
@@ -71,15 +84,15 @@ function inlineChatImageKeys(chatId: string, content: Record<string, unknown>):
7184
/**
7285
* Collect all file storage keys for the given chat IDs from three sources:
7386
* 1. workspaceFiles rows with chatId FK (chat-scoped contexts only)
74-
* 2. fileAttachments[].key inside each copilot_messages.content, for copilot-storage keys only
87+
* 2. fileAttachments[].key inside each copilot_messages.content: copilot keys, and
88+
* organization attachments under their own context (see {@link FileRef.organizationId})
7589
* 3. the chat-scoped inline images each assistant message published
7690
*
77-
* An attachment whose key belongs to another storage context is owned by its
78-
* `workspace_files` row (source 1, or the workspace file lifecycle), never by the message:
79-
* a fork carries the same attachment when its copy failed or its file was deleted, and the
80-
* copilot bucket falls back to the workspace bucket on GCS (and may be configured to it on
81-
* S3), so deleting such a key as copilot storage could delete a file another chat or the
82-
* workspace still uses.
91+
* A workspace attachment is owned by its `workspace_files` row (source 1, or the workspace
92+
* file lifecycle), never by the message: a fork carries the same attachment when its copy
93+
* failed or its file was deleted, and the copilot bucket falls back to the workspace bucket
94+
* on GCS (and may be configured to it on S3), so deleting such a key as copilot storage
95+
* could delete a file another chat or the workspace still uses.
8396
*/
8497
export async function collectChatFiles(chatIds: string[]): Promise<FileRef[]> {
8598
const files: FileRef[] = []
@@ -136,8 +149,12 @@ export async function collectChatFiles(chatIds: string[]): Promise<FileRef[]> {
136149
(attachment as Record<string, unknown>).key
137150
) {
138151
const key = (attachment as Record<string, unknown>).key as string
139-
if (tryInferContextFromKey(key) !== 'copilot') continue
140-
if (!seen.has(key)) {
152+
if (seen.has(key)) continue
153+
const organizationId = organizationAttachmentOwner(key)
154+
if (organizationId) {
155+
seen.add(key)
156+
files.push({ key, context: 'mothership', chatId: row.chatId, organizationId })
157+
} else if (tryInferContextFromKey(key) === 'copilot') {
141158
seen.add(key)
142159
files.push({ key, context: 'copilot', chatId: row.chatId })
143160
}
@@ -149,6 +166,55 @@ export async function collectChatFiles(chatIds: string[]): Promise<FileRef[]> {
149166
return files
150167
}
151168

169+
/**
170+
* Organization attachment keys that a remaining chat still references, so they outlive the
171+
* chats being purged. Runs after the caller deleted those chats' rows, so any match is
172+
* another chat: a fork, or a chat that is only soft-deleted and may be restored.
173+
*/
174+
async function organizationAttachmentsStillReferenced(files: FileRef[]): Promise<Set<string>> {
175+
const keysByOrganization = new Map<string, string[]>()
176+
for (const file of files) {
177+
if (!file.organizationId) continue
178+
const keys = keysByOrganization.get(file.organizationId)
179+
if (keys) keys.push(file.key)
180+
else keysByOrganization.set(file.organizationId, [file.key])
181+
}
182+
const referenced = new Set<string>()
183+
for (const [organizationId, keys] of keysByOrganization) {
184+
for (const chunk of chunkArray(keys, CHAT_FILE_COLLECT_CHUNK_SIZE)) {
185+
const wanted = new Set(chunk)
186+
const rows = await cleanupDb
187+
.select({ content: copilotMessages.content })
188+
.from(copilotMessages)
189+
.innerJoin(copilotChats, eq(copilotChats.id, copilotMessages.chatId))
190+
.where(
191+
and(
192+
eq(copilotChats.organizationId, organizationId),
193+
or(
194+
...chunk.map(
195+
(key) =>
196+
sql`${copilotMessages.content} @> ${JSON.stringify({ fileAttachments: [{ key }] })}::jsonb`
197+
)
198+
)
199+
)
200+
)
201+
for (const { content } of rows) {
202+
const attachments = isRecordLike(content) ? content.fileAttachments : undefined
203+
if (!Array.isArray(attachments)) continue
204+
for (const attachment of attachments) {
205+
if (
206+
isRecordLike(attachment) &&
207+
typeof attachment.key === 'string' &&
208+
wanted.has(attachment.key)
209+
)
210+
referenced.add(attachment.key)
211+
}
212+
}
213+
}
214+
}
215+
return referenced
216+
}
217+
152218
/** Groups files by storage context so each context can use one batch DELETE call. */
153219
export async function deleteStorageFiles(
154220
files: FileRef[],
@@ -271,7 +337,12 @@ export async function prepareChatCleanup(
271337
)
272338
}
273339
const confirmedChatIds = chatIds.filter((id) => !survivors.has(id))
274-
const confirmedFiles = files.filter((file) => !survivors.has(file.chatId))
340+
const sharedKeys = await organizationAttachmentsStillReferenced(
341+
files.filter((file) => !survivors.has(file.chatId))
342+
)
343+
const confirmedFiles = files.filter(
344+
(file) => !survivors.has(file.chatId) && !sharedKeys.has(file.key)
345+
)
275346

276347
// Call copilot backend
277348
if (confirmedChatIds.length > 0) {

0 commit comments

Comments
 (0)