Skip to content

Commit 861c966

Browse files
j15zj15z
committed
fix(copilot): close resource persistence races
1 parent 29341bf commit 861c966

10 files changed

Lines changed: 288 additions & 119 deletions

File tree

‎apps/sim/app/api/copilot/chat/resources/route.ts‎

Lines changed: 89 additions & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ import {
1616
createNotFoundResponse,
1717
createUnauthorizedResponse,
1818
} from '@/lib/copilot/request/http'
19-
import type { ChatResource } from '@/lib/copilot/resources/persistence'
19+
import { type ChatResource, serializeChatResourceWrite } from '@/lib/copilot/resources/persistence'
2020
import type { MothershipResourceUpdate } from '@/lib/copilot/resources/types'
2121
import {
2222
canonicalizeDesktopSessionResource,
@@ -55,44 +55,45 @@ export const POST = withRouteHandler(async (req: NextRequest) => {
5555
return NextResponse.json({ success: true })
5656
}
5757

58-
const [chat] = await db
59-
.select({ resources: copilotChats.resources })
60-
.from(copilotChats)
61-
.where(
62-
and(
58+
const merged = await serializeChatResourceWrite(chatId, () =>
59+
db.transaction(async (tx) => {
60+
const scope = and(
6361
eq(copilotChats.id, chatId),
6462
eq(copilotChats.userId, userId),
6563
isNull(copilotChats.deletedAt)
6664
)
67-
)
68-
.limit(1)
65+
const [chat] = await tx
66+
.select({ resources: copilotChats.resources })
67+
.from(copilotChats)
68+
.where(scope)
69+
.for('update')
70+
.limit(1)
6971

70-
if (!chat) {
71-
return createNotFoundResponse('Chat not found or unauthorized')
72-
}
72+
if (!chat) return null
7373

74-
const existing = sanitizeChatResources(
75-
Array.isArray(chat.resources) ? (chat.resources as ChatResource[]) : []
76-
)
77-
const key = `${resource.type}:${resource.id}`
78-
const prev = existing.find((r) => `${r.type}:${r.id}` === key)
79-
80-
const merged: ChatResource[] = prev
81-
? existing.map((r) =>
82-
`${r.type}:${r.id}` === key ? mergeChatResource(r, resourceUpdate) : r
74+
const existing = sanitizeChatResources(
75+
Array.isArray(chat.resources) ? (chat.resources as ChatResource[]) : []
8376
)
84-
: [...existing, mergeChatResource(undefined, resourceUpdate)]
77+
const key = `${resource.type}:${resource.id}`
78+
const prev = existing.find((r) => `${r.type}:${r.id}` === key)
79+
const next: ChatResource[] = prev
80+
? existing.map((r) =>
81+
`${r.type}:${r.id}` === key ? mergeChatResource(r, resourceUpdate) : r
82+
)
83+
: [...existing, mergeChatResource(undefined, resourceUpdate)]
84+
85+
await tx
86+
.update(copilotChats)
87+
.set({ resources: sql`${JSON.stringify(next)}::jsonb`, updatedAt: new Date() })
88+
.where(scope)
89+
90+
return next
91+
})
92+
)
8593

86-
await db
87-
.update(copilotChats)
88-
.set({ resources: sql`${JSON.stringify(merged)}::jsonb`, updatedAt: new Date() })
89-
.where(
90-
and(
91-
eq(copilotChats.id, chatId),
92-
eq(copilotChats.userId, userId),
93-
isNull(copilotChats.deletedAt)
94-
)
95-
)
94+
if (!merged) {
95+
return createNotFoundResponse('Chat not found or unauthorized')
96+
}
9697

9798
logger.info('Added resource to chat', { chatId, resource })
9899

@@ -122,41 +123,44 @@ export const PATCH = withRouteHandler(async (req: NextRequest) => {
122123
if (!parsed.success) return parsed.response
123124
const { chatId, resources: newOrder } = parsed.data.body
124125

125-
const [chat] = await db
126-
.select({ resources: copilotChats.resources })
127-
.from(copilotChats)
128-
.where(
129-
and(
126+
const canonicalOrder = await serializeChatResourceWrite(chatId, () =>
127+
db.transaction(async (tx): Promise<ChatResource[] | null | undefined> => {
128+
const scope = and(
130129
eq(copilotChats.id, chatId),
131130
eq(copilotChats.userId, userId),
132131
isNull(copilotChats.deletedAt)
133132
)
134-
)
135-
.limit(1)
133+
const [chat] = await tx
134+
.select({ resources: copilotChats.resources })
135+
.from(copilotChats)
136+
.where(scope)
137+
.for('update')
138+
.limit(1)
136139

137-
if (!chat) {
138-
return createNotFoundResponse('Chat not found or unauthorized')
139-
}
140+
if (!chat) return undefined
140141

141-
const existing = sanitizeChatResources(
142-
Array.isArray(chat.resources) ? (chat.resources as ChatResource[]) : []
142+
const existing = sanitizeChatResources(
143+
Array.isArray(chat.resources) ? (chat.resources as ChatResource[]) : []
144+
)
145+
const next = reorderStoredChatResources(existing, newOrder)
146+
if (!next) return null
147+
148+
await tx
149+
.update(copilotChats)
150+
.set({ resources: sql`${JSON.stringify(next)}::jsonb`, updatedAt: new Date() })
151+
.where(scope)
152+
153+
return next
154+
})
143155
)
144-
const canonicalOrder = reorderStoredChatResources(existing, newOrder)
156+
157+
if (canonicalOrder === undefined) {
158+
return createNotFoundResponse('Chat not found or unauthorized')
159+
}
145160
if (!canonicalOrder) {
146161
return createBadRequestResponse('Reordered resources must match existing resources')
147162
}
148163

149-
await db
150-
.update(copilotChats)
151-
.set({ resources: sql`${JSON.stringify(canonicalOrder)}::jsonb`, updatedAt: new Date() })
152-
.where(
153-
and(
154-
eq(copilotChats.id, chatId),
155-
eq(copilotChats.userId, userId),
156-
isNull(copilotChats.deletedAt)
157-
)
158-
)
159-
160164
logger.info('Reordered resources for chat', { chatId, count: canonicalOrder.length })
161165

162166
return NextResponse.json({ success: true, resources: canonicalOrder })
@@ -185,39 +189,44 @@ export const DELETE = withRouteHandler(async (req: NextRequest) => {
185189
if (!parsed.success) return parsed.response
186190
const { chatId, resourceType, resourceId } = parsed.data.body
187191

188-
// Old builds could persist an inner browser/terminal tab id. Closing the
189-
// singleton panel removes every legacy row of that type so it cannot be
190-
// canonicalized back into view on the next hydration.
191-
const removePredicate =
192-
resourceType === 'browser' || resourceType === 'terminal'
193-
? sql`elem->>'type' = ${resourceType}`
194-
: sql`elem->>'type' = ${resourceType} AND elem->>'id' = ${resourceId}`
195-
196-
const [updated] = await db
197-
.update(copilotChats)
198-
.set({
199-
resources: sql`COALESCE((
200-
SELECT jsonb_agg(elem)
201-
FROM jsonb_array_elements(${copilotChats.resources}) elem
202-
WHERE NOT (${removePredicate})
203-
), '[]'::jsonb)`,
204-
updatedAt: new Date(),
205-
})
206-
.where(
207-
and(
192+
const merged = await serializeChatResourceWrite(chatId, () =>
193+
db.transaction(async (tx) => {
194+
const scope = and(
208195
eq(copilotChats.id, chatId),
209196
eq(copilotChats.userId, userId),
210197
isNull(copilotChats.deletedAt)
211198
)
212-
)
213-
.returning({ resources: copilotChats.resources })
199+
const [chat] = await tx
200+
.select({ resources: copilotChats.resources })
201+
.from(copilotChats)
202+
.where(scope)
203+
.for('update')
204+
.limit(1)
214205

215-
if (!updated) {
206+
if (!chat) return null
207+
208+
const existing = sanitizeChatResources(
209+
Array.isArray(chat.resources) ? (chat.resources as ChatResource[]) : []
210+
)
211+
const removeAllOfType = resourceType === 'browser' || resourceType === 'terminal'
212+
const next = existing.filter(
213+
(resource) =>
214+
resource.type !== resourceType || (!removeAllOfType && resource.id !== resourceId)
215+
)
216+
217+
await tx
218+
.update(copilotChats)
219+
.set({ resources: sql`${JSON.stringify(next)}::jsonb`, updatedAt: new Date() })
220+
.where(scope)
221+
222+
return next
223+
})
224+
)
225+
226+
if (!merged) {
216227
return createNotFoundResponse('Chat not found or unauthorized')
217228
}
218229

219-
const merged = Array.isArray(updated.resources) ? (updated.resources as ChatResource[]) : []
220-
221230
logger.info('Removed resource from chat', { chatId, resourceType, resourceId })
222231

223232
return NextResponse.json({ success: true, resources: merged })

‎apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts‎

Lines changed: 18 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1848,26 +1848,23 @@ export function useChat(
18481848
// Ephemeral panels were never persisted; nothing to delete server-side.
18491849
if (isEphemeralResource({ type: resourceType, id: resourceId, title: '' })) return
18501850

1851-
const { inFlight: inFlightAdd, wasPending } = resourcePersistenceQueue.remove(
1852-
resourceType,
1853-
resourceId
1854-
)
1855-
if (wasPending && !inFlightAdd) return
1851+
const {
1852+
inFlight: inFlightAdd,
1853+
scheduleDelete,
1854+
wasPending,
1855+
wasPersisted,
1856+
} = resourcePersistenceQueue.remove(resourceType, resourceId)
1857+
if (wasPending && !inFlightAdd && !wasPersisted) return
18561858

18571859
const persistChatId = chatIdRef.current ?? selectedChatIdRef.current
18581860
if (!persistChatId) return
1859-
const fireDelete = () => {
1861+
scheduleDelete(persistChatId, () =>
18601862
requestJson(removeMothershipChatResourceContract, {
18611863
body: { chatId: persistChatId, resourceType, resourceId },
18621864
}).catch((err) => {
18631865
logger.warn('Failed to persist resource removal', err)
18641866
})
1865-
}
1866-
if (inFlightAdd) {
1867-
inFlightAdd.finally(fireDelete)
1868-
} else {
1869-
fireDelete()
1870-
}
1867+
)
18711868
},
18721869
[resourcePersistenceQueue]
18731870
)
@@ -2279,11 +2276,16 @@ export function useChat(
22792276
useMothershipQueueStore.getState().migrate(pendingChatKey, resolvedChatId)
22802277
}
22812278
await Promise.allSettled(
2282-
pendingResources.map((resource) =>
2283-
requestJson(addMothershipChatResourceContract, {
2284-
body: { chatId: resolvedChatId, resource },
2279+
pendingResources.map((update) => {
2280+
const { clearViewId, ...resource } = update
2281+
return requestJson(addMothershipChatResourceContract, {
2282+
body: {
2283+
chatId: resolvedChatId,
2284+
resource,
2285+
...(clearViewId === true ? { clearViewId: true as const } : {}),
2286+
},
22852287
})
2286-
)
2288+
})
22872289
)
22882290
queryClient.invalidateQueries({
22892291
queryKey: mothershipChatKeys.detail(resolvedChatId),

‎apps/sim/app/workspace/[workspaceId]/tables/[tableId]/view-state.test.ts‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -87,10 +87,16 @@ describe('resolveTableViewPinTransition', () => {
8787
).toEqual({ nextViewId: 'view-pinned', pendingCreatedViewId: null })
8888
})
8989

90-
it('keeps the pending creation when the pin is already represented locally', () => {
90+
it('clears a different pending creation when the pin is already represented in the URL', () => {
9191
expect(
9292
resolveTableViewPinTransition('view-pinned', 'view-created', 'view-pinned', 'view-created')
93-
).toEqual({ nextViewId: null, pendingCreatedViewId: 'view-created' })
93+
).toEqual({ nextViewId: null, pendingCreatedViewId: null })
94+
})
95+
96+
it('keeps a pending creation when it created the pinned view', () => {
97+
expect(
98+
resolveTableViewPinTransition('view-pinned', 'view-pinned', 'view-pinned', 'view-pinned')
99+
).toEqual({ nextViewId: null, pendingCreatedViewId: 'view-pinned' })
94100
})
95101

96102
it('replaces a different active URL even if the pin was applied previously', () => {

‎apps/sim/app/workspace/[workspaceId]/tables/[tableId]/view-state.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,10 @@ export function resolveTableViewPinTransition(
6565
pendingCreatedViewId: string | null
6666
): TableViewPinTransition {
6767
if (activeViewId === pinnedViewId || (activeViewId === null && appliedViewId === pinnedViewId)) {
68-
return { nextViewId: null, pendingCreatedViewId }
68+
return {
69+
nextViewId: null,
70+
pendingCreatedViewId: pendingCreatedViewId === pinnedViewId ? pendingCreatedViewId : null,
71+
}
6972
}
7073
return { nextViewId: pinnedViewId, pendingCreatedViewId: null }
7174
}

‎apps/sim/lib/api/contracts/copilot.ts‎

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -90,15 +90,21 @@ export type CreateWorkflowCopilotChatBody = z.input<typeof createWorkflowCopilot
9090

9191
const copilotResourceTypeSchema = z.enum(PERSISTED_RESOURCE_TYPES)
9292

93-
const copilotChatResourceItemSchema = z.object({
94-
type: copilotResourceTypeSchema,
95-
// Matches the bound the chat-send path enforces.
96-
id: requiredFieldSchema('resource.id cannot be empty'),
97-
title: z.string(),
98-
// Saved view a table tab is pinned to (type "table" only). One schema for
99-
// add and reorder, so a reorder round-trip can never strip the pin.
100-
viewId: z.string().min(1).optional(),
101-
})
93+
const copilotChatResourceItemSchema = z
94+
.object({
95+
type: copilotResourceTypeSchema,
96+
id: requiredFieldSchema('resource.id cannot be empty'),
97+
title: z.string(),
98+
viewId: z.string().min(1).optional(),
99+
})
100+
.superRefine((resource, ctx) => {
101+
if (resource.viewId === undefined || resource.type === 'table') return
102+
ctx.addIssue({
103+
code: 'custom',
104+
path: ['viewId'],
105+
message: 'viewId is only valid for table resources',
106+
})
107+
})
102108

103109
export const addCopilotChatResourceBodySchema = z
104110
.object({

‎apps/sim/lib/api/contracts/mothership-chats.test.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,4 +49,13 @@ describe('addMothershipChatResourceBodySchema', () => {
4949
}).success
5050
).toBe(false)
5151
})
52+
53+
it('rejects a saved-view pin for a non-table resource', () => {
54+
expect(
55+
addMothershipChatResourceBodySchema.safeParse({
56+
chatId: 'chat-1',
57+
resource: { type: 'file', id: 'file-1', title: 'Accounts.csv', viewId: 'view-1' },
58+
}).success
59+
).toBe(false)
60+
})
5261
})

0 commit comments

Comments
 (0)