Skip to content

Commit a53aadc

Browse files
committed
fix(cleanup): keep chat files another chat of their scope still references
A fork names its source's storage key wherever it did not copy the file: an organization attachment, a message-only copilot attachment, or an upload whose blob copy failed (`publishedFileRefMaps` leaves those keys in the fork's messages). The fork comment said cleanup deletes a shared key only once no chat references it, but the reference check covered `assistant/<org>/` keys alone, so deleting the source chat deleted the upload a fork still showed. Every collected key now carries the scope whose chats may still reference it (`organizationId`, or `workspaceId` from its `workspace_files` row, else the chat's), and `deletableChatFiles` runs the existing batched `@>` containment over that scope's messages for every key before the handler or the retention path deletes it. A chat with no workspace or organization records `workspaceId: null`: no fork can be made from it. The payload field is optional, so events already queued still parse; for those, the scope is recovered from the key where it names one (`assistant/<org>/`, `workspace/<id>/`), a chat's own inline images need no check, and any other key is kept and logged rather than risk deleting a file a fork shows. The fork's `for('key share')` on the source row still orders the two: the check runs only after the source row is gone, and the fork lives in the same scope. Autopsy: the only shared-key test used an organization attachment, the one kind the check covered; no test forked a workspace chat whose upload copy failed, so the unchecked mothership and copilot keys went unnoticed. The new integration case deletes a source chat whose upload a fork still references, and fails on the old handler with the object gone; a second case pins how events queued before this change are handled.
1 parent 9e9fac3 commit a53aadc

7 files changed

Lines changed: 220 additions & 44 deletions

File tree

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

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,7 @@ import { describeError, toError } from '@sim/utils/errors'
44
import { inArray } from 'drizzle-orm'
55
import { chatBackendCleanupPayloadSchema } from '@/lib/cleanup/chat-backend-cleanup'
66
import { CHAT_BACKEND_CLEANUP_EVENT } from '@/lib/cleanup/chat-backend-cleanup-event'
7-
import {
8-
cleanupCopilotBackend,
9-
organizationAttachmentsStillReferenced,
10-
} from '@/lib/cleanup/chat-cleanup'
7+
import { cleanupCopilotBackend, deletableChatFiles } from '@/lib/cleanup/chat-cleanup'
118
import { env } from '@/lib/core/config/env'
129
import {
1310
continueOutboxHandler,
@@ -65,7 +62,8 @@ export const chatBackendCleanupOutboxHandlers = {
6562
* continues in a later run once its deadline nears. Any failure throws so the outbox retries it:
6663
* a failed backend call, a failed object delete, or chat ids queued for the backend reaching a
6764
* processor without `COPILOT_API_KEY`. An object already gone counts as deleted, so a retry
68-
* repeats only what is left.
65+
* repeats only what is left. A stored file another chat of its scope still references, as a
66+
* fork's uncopied attachment does, is kept ({@link deletableChatFiles}).
6967
*/
7068
[CHAT_BACKEND_CLEANUP_EVENT]: async (rawPayload, context) => {
7169
const { chatIds, files, copilotBackend, imagesSwept } =
@@ -100,9 +98,8 @@ export const chatBackendCleanupOutboxHandlers = {
10098
}
10199

102100
if (deletedFiles.length === 0) return
103-
const stillReferenced = await organizationAttachmentsStillReferenced(deletedFiles)
104101
const failures = await mapWithConcurrency(
105-
deletedFiles.filter((file) => !stillReferenced.has(file.key)),
102+
await deletableChatFiles(deletedFiles),
106103
FILE_DELETE_CONCURRENCY,
107104
async ({ key, context: storageContext }) => {
108105
context.signal.throwIfAborted()

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

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -313,6 +313,96 @@ describe('cleanup of a permanently deleted chat', () => {
313313
expect(backend.rejected).toBe(0)
314314
})
315315

316+
/**
317+
* A fork whose copy of an upload failed keeps naming the source's key in its messages, while
318+
* the upload's `workspace_files` row cascades away with the source chat.
319+
*/
320+
it('keeps a deleted chat’s upload that a fork in its workspace still references', async () => {
321+
const ownerId = await seedUser()
322+
const workspaceId = await seedWorkspace(ownerId)
323+
const uploadIn = async (chatId: string) => {
324+
const key = `workspace/${workspaceId}/1700000000000-${generateId().slice(0, 8)}-shot.png`
325+
await db.insert(workspaceFiles).values({
326+
id: generateId(),
327+
key,
328+
userId: ownerId,
329+
workspaceId,
330+
context: 'mothership',
331+
chatId,
332+
originalName: 'shot.png',
333+
contentType: 'image/png',
334+
sizeBytes: 1,
335+
})
336+
await putObject(key)
337+
return key
338+
}
339+
const forkedSource = await seedChat(ownerId, workspaceId)
340+
const sharedKey = await uploadIn(forkedSource)
341+
const fork = await seedChat(ownerId, workspaceId)
342+
await db.insert(copilotMessages).values({
343+
chatId: fork,
344+
messageId: generateId(),
345+
role: 'user',
346+
content: {
347+
role: 'user',
348+
content: 'see attached',
349+
fileAttachments: [{ key: sharedKey, filename: 'shot.png' }],
350+
},
351+
})
352+
const unforkedSource = await seedChat(ownerId, workspaceId)
353+
const ownKey = await uploadIn(unforkedSource)
354+
355+
await deleteCopilotChat({ chatId: forkedSource, userId: ownerId })
356+
await deleteCopilotChat({ chatId: unforkedSource, userId: ownerId })
357+
358+
expect(new Set((await deliverCleanups()).outcomes)).toEqual(new Set(['completed']))
359+
expect(await objectExists(sharedKey)).toBe(true)
360+
expect(await objectExists(ownKey)).toBe(false)
361+
})
362+
363+
it('keeps what an event queued before refs carried their scope cannot place', async () => {
364+
const ownerId = await seedUser()
365+
const workspaceId = await seedWorkspace(ownerId)
366+
const deletedChat = await seedChat(ownerId, workspaceId)
367+
const remainingChat = await seedChat(ownerId, workspaceId)
368+
const referenced = `workspace/${workspaceId}/1700000000000-abcd1234-kept.png`
369+
const unreferenced = `workspace/${workspaceId}/1700000000000-abcd1234-gone.png`
370+
const unplaceable = `copilot/${generateId()}/legacy.png`
371+
for (const key of [referenced, unreferenced, unplaceable]) await putObject(key)
372+
await db.insert(copilotMessages).values({
373+
chatId: remainingChat,
374+
messageId: generateId(),
375+
role: 'user',
376+
content: { role: 'user', content: 'x', fileAttachments: [{ key: referenced }] },
377+
})
378+
await db.delete(copilotChats).where(eq(copilotChats.id, deletedChat))
379+
await db.insert(outboxEvent).values({
380+
id: generateId(),
381+
eventType: CHAT_BACKEND_CLEANUP_EVENT,
382+
payload: {
383+
chatIds: [],
384+
files: [referenced, unreferenced].map((key) => ({
385+
key,
386+
context: 'mothership',
387+
chatId: deletedChat,
388+
})),
389+
},
390+
})
391+
await db.insert(outboxEvent).values({
392+
id: generateId(),
393+
eventType: CHAT_BACKEND_CLEANUP_EVENT,
394+
payload: {
395+
chatIds: [],
396+
files: [{ key: unplaceable, context: 'copilot', chatId: deletedChat }],
397+
},
398+
})
399+
400+
expect((await deliverCleanups()).outcomes).toEqual(['completed', 'completed'])
401+
expect(await objectExists(referenced)).toBe(true)
402+
expect(await objectExists(unreferenced)).toBe(false)
403+
expect(await objectExists(unplaceable)).toBe(true)
404+
})
405+
316406
it('records no cleanup when the delete rolls back', async () => {
317407
const ownerId = await seedUser()
318408
const workspaceId = await seedWorkspace(ownerId)

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ const chatFileSchema = z
2727
]),
2828
chatId: chatIdSchema,
2929
organizationId: chatIdSchema.optional(),
30+
workspaceId: chatIdSchema.nullable().optional(),
3031
})
3132
.strict()
3233

@@ -83,11 +84,12 @@ export async function enqueueChatBackendCleanup(
8384
})),
8485
...chunkArray([...filesByKey.values()], CHAT_BACKEND_CLEANUP_BATCH_SIZE).map((batch) => ({
8586
chatIds: [],
86-
files: batch.map(({ key, context, chatId, organizationId }) => ({
87+
files: batch.map(({ key, context, chatId, organizationId, workspaceId }) => ({
8788
key,
8889
context,
8990
chatId,
9091
...(organizationId ? { organizationId } : {}),
92+
...(workspaceId !== undefined ? { workspaceId } : {}),
9193
})),
9294
})),
9395
]

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -109,9 +109,9 @@ describe('chat cleanup file collection in PostgreSQL', () => {
109109
chatId,
110110
organizationId: 'org-1',
111111
},
112-
{ key: 'copilot/1234/legacy.png', context: 'copilot', chatId },
113-
{ key: 'copilot/5/other.png', context: 'copilot', chatId: otherChatId },
114-
{ key: 'copilot/9/removed.png', context: 'copilot', chatId },
112+
{ key: 'copilot/1234/legacy.png', context: 'copilot', chatId, workspaceId: null },
113+
{ key: 'copilot/5/other.png', context: 'copilot', chatId: otherChatId, workspaceId: null },
114+
{ key: 'copilot/9/removed.png', context: 'copilot', chatId, workspaceId: null },
115115
].sort((a, b) => compareStrings(a.key, b.key))
116116
)
117117
})

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

Lines changed: 35 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,9 @@ import { and, eq, inArray, or, sql } from 'drizzle-orm'
77
import { enqueueChatBackendCleanup } from '@/lib/cleanup/chat-backend-cleanup'
88
import {
99
type ChatFileRef,
10+
type ChatFileReferenceScope,
1011
type ChatScopedContext,
12+
chatFileReferenceScope,
1113
collectChatFiles,
1214
} from '@/lib/cleanup/chat-files'
1315
import { env } from '@/lib/core/config/env'
@@ -24,22 +26,38 @@ const COPILOT_CLEANUP_BATCH_SIZE = 1000
2426
const CHAT_FILE_COLLECT_CHUNK_SIZE = 500
2527

2628
/**
27-
* Organization attachment keys that a remaining chat still references, so they outlive the
28-
* chats being purged. Runs after the caller deleted those chats' rows, so any match is
29-
* another chat: a fork, or a chat that is only soft-deleted and may be restored.
29+
* The files among `files` that may be deleted: those no remaining chat of their scope still
30+
* references ({@link chatFileReferenceScope}). A fork keeps naming its source's key wherever it
31+
* did not copy the file, so a key still in another chat's `fileAttachments` outlives the chats
32+
* being purged, and a key whose scope is unknown is kept rather than risked. Runs after the caller
33+
* deleted those chats' rows, so any match is another chat: a fork, or a chat that is only
34+
* soft-deleted and may be restored.
3035
*/
31-
export async function organizationAttachmentsStillReferenced(
32-
files: readonly ChatFileRef[]
33-
): Promise<Set<string>> {
34-
const keysByOrganization = new Map<string, string[]>()
36+
export async function deletableChatFiles(files: readonly ChatFileRef[]): Promise<ChatFileRef[]> {
37+
const keysByScope = new Map<string, { scope: ChatFileReferenceScope; keys: string[] }>()
38+
const kept = new Set<string>()
3539
for (const file of files) {
36-
if (!file.organizationId) continue
37-
const keys = keysByOrganization.get(file.organizationId)
38-
if (keys) keys.push(file.key)
39-
else keysByOrganization.set(file.organizationId, [file.key])
40+
const scope = chatFileReferenceScope(file)
41+
if (scope === null) continue
42+
if (scope === undefined) {
43+
kept.add(file.key)
44+
continue
45+
}
46+
const scopeKey = `${scope.kind}:${scope.id}`
47+
const group = keysByScope.get(scopeKey)
48+
if (group) group.keys.push(file.key)
49+
else keysByScope.set(scopeKey, { scope, keys: [file.key] })
50+
}
51+
if (kept.size > 0) {
52+
logger.warn(`Keeping ${kept.size} chat files whose reference scope is unknown`, {
53+
keys: [...kept],
54+
})
4055
}
41-
const referenced = new Set<string>()
42-
for (const [organizationId, keys] of keysByOrganization) {
56+
for (const { scope, keys } of keysByScope.values()) {
57+
const inScope =
58+
scope.kind === 'organization'
59+
? eq(copilotChats.organizationId, scope.id)
60+
: eq(copilotChats.workspaceId, scope.id)
4361
for (const chunk of chunkArray(keys, CHAT_FILE_COLLECT_CHUNK_SIZE)) {
4462
const wanted = new Set(chunk)
4563
const rows = await cleanupDb
@@ -48,7 +66,7 @@ export async function organizationAttachmentsStillReferenced(
4866
.innerJoin(copilotChats, eq(copilotChats.id, copilotMessages.chatId))
4967
.where(
5068
and(
51-
eq(copilotChats.organizationId, organizationId),
69+
inScope,
5270
or(
5371
...chunk.map(
5472
(key) =>
@@ -65,12 +83,12 @@ export async function organizationAttachmentsStillReferenced(
6583
typeof attachment.key === 'string' &&
6684
wanted.has(attachment.key)
6785
)
68-
referenced.add(attachment.key)
86+
kept.add(attachment.key)
6987
}
7088
}
7189
}
7290
}
73-
return referenced
91+
return files.filter((file) => !kept.has(file.key))
7492
}
7593

7694
/** Groups files by storage context so each context can use one batch DELETE call. */
@@ -195,12 +213,9 @@ export async function prepareChatCleanup(
195213
)
196214
}
197215
const confirmedChatIds = chatIds.filter((id) => !survivors.has(id))
198-
const sharedKeys = await organizationAttachmentsStillReferenced(
216+
const confirmedFiles = await deletableChatFiles(
199217
files.filter((file) => !survivors.has(file.chatId))
200218
)
201-
const confirmedFiles = files.filter(
202-
(file) => !survivors.has(file.chatId) && !sharedKeys.has(file.key)
203-
)
204219

205220
// Call copilot backend
206221
if (confirmedChatIds.length > 0) {

0 commit comments

Comments
 (0)