Skip to content

Commit 390e1b3

Browse files
waleedlatif1claude
andcommitted
fix(copilot): bound resource-write locks and repair reorder persistence
Review follow-ups on the saved-view pinning work. Correctness: - Reorder persistence was parked by ANY pending write. A repeatedly failing update to an already-stored resource (a view pin) blocked tab ordering for the rest of the session; gate on unpersisted writes only, which are the ones the server's identity check can actually reject. - A parked reorder body that the server rejects was re-parked verbatim, so a tab closed after the order was captured poisoned it permanently. Discard on 400; keep retrying everything transient. - adoptScope merged the provisional and chat-scoped updates in the wrong order, letting an older pending write overwrite a newer one. - A view pin that arrived before the table finished its first adoption was dropped for good when the table data resolved after the views list. The stream path self-rescued through query invalidation; the restore path did not. Re-run the effect when adoption becomes possible. - Reordering a chat holding a legacy duplicate row 400'd forever. Compare identity sets so the duplicate collapses on write instead. - mergeChatResource aliased the caller's object into React state, the query cache and the pending-write queue at once. Copy it. Robustness: - The new copilot_chats FOR UPDATE transactions had no lock_timeout, and neither the pool nor the deployment sets one. finalizeAssistantTurn holds that same row across an assistant-message append, so a waiter could park a pool connection indefinitely. Bound all five writers. - mergeChatResource's field list is now one declaration that fails to compile when MothershipResource gains a field, rather than silently dropping it from both the merge and its no-op check. - Extraction can no longer emit viewId and clearViewId together, a pair the wire contract rejects and the merge would resolve to neither. - Restrict the eager view-id URL write to embedded tables, leaving standalone table behaviour identical to staging. - Drop the queue's unreachable unscoped bucket, its uncalled clear(), and its test-only getPendingUpdates(). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FwmaLmAXSK2hsPBGZmkPnT
1 parent 6c6da57 commit 390e1b3

9 files changed

Lines changed: 217 additions & 64 deletions

File tree

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

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,11 @@ import {
1616
createNotFoundResponse,
1717
createUnauthorizedResponse,
1818
} from '@/lib/copilot/request/http'
19-
import { type ChatResource, serializeChatResourceWrite } from '@/lib/copilot/resources/persistence'
19+
import {
20+
type ChatResource,
21+
serializeChatResourceWrite,
22+
setChatResourceTxTimeouts,
23+
} from '@/lib/copilot/resources/persistence'
2024
import type { MothershipResourceUpdate } from '@/lib/copilot/resources/types'
2125
import {
2226
canonicalizeDesktopSessionResource,
@@ -57,6 +61,7 @@ export const POST = withRouteHandler(async (req: NextRequest) => {
5761

5862
const merged = await serializeChatResourceWrite(chatId, () =>
5963
db.transaction(async (tx) => {
64+
await setChatResourceTxTimeouts(tx)
6065
const scope = and(
6166
eq(copilotChats.id, chatId),
6267
eq(copilotChats.userId, userId),
@@ -125,6 +130,7 @@ export const PATCH = withRouteHandler(async (req: NextRequest) => {
125130

126131
const canonicalOrder = await serializeChatResourceWrite(chatId, () =>
127132
db.transaction(async (tx): Promise<ChatResource[] | null | undefined> => {
133+
await setChatResourceTxTimeouts(tx)
128134
const scope = and(
129135
eq(copilotChats.id, chatId),
130136
eq(copilotChats.userId, userId),
@@ -191,6 +197,7 @@ export const DELETE = withRouteHandler(async (req: NextRequest) => {
191197

192198
const merged = await serializeChatResourceWrite(chatId, () =>
193199
db.transaction(async (tx) => {
200+
await setChatResourceTxTimeouts(tx)
194201
const scope = and(
195202
eq(copilotChats.id, chatId),
196203
eq(copilotChats.userId, userId),

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

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import { isRecordLike } from '@sim/utils/object'
1818
import { backoffWithJitter } from '@sim/utils/retry'
1919
import { useQueryClient } from '@tanstack/react-query'
2020
import { usePathname, useRouter } from 'next/navigation'
21+
import { isApiClientError } from '@/lib/api/client/errors'
2122
import { requestJson } from '@/lib/api/client/request'
2223
import {
2324
addMothershipChatResourceContract,
@@ -1706,7 +1707,7 @@ export function useChat(
17061707
while (true) {
17071708
const pendingOrder = pendingResourceReordersRef.current.get(chatId)
17081709
if (!pendingOrder) return
1709-
if (resourcePersistenceQueue.getPendingResourceKeys(chatId).size > 0) return
1710+
if (resourcePersistenceQueue.hasUnpersistedWrites(chatId)) return
17101711

17111712
const inFlightWrites = resourcePersistenceQueue.getInFlightWrites(chatId)
17121713
if (inFlightWrites.length > 0) {
@@ -1721,10 +1722,21 @@ export function useChat(
17211722
body: { chatId, resources: pendingOrder },
17221723
})
17231724
} catch (error) {
1724-
if (!pendingResourceReordersRef.current.has(chatId)) {
1725+
// 400 is the server rejecting the body's identity set — a tab was
1726+
// closed after this order was captured. Replaying it verbatim can
1727+
// only fail again, so drop it and let the next reorder or hydration
1728+
// re-establish the order. Everything else (offline, 401, 5xx) is
1729+
// transient and keeps the body for the next retry.
1730+
const unsatisfiable = isApiClientError(error) && error.status === 400
1731+
if (!unsatisfiable && !pendingResourceReordersRef.current.has(chatId)) {
17251732
pendingResourceReordersRef.current.set(chatId, pendingOrder)
17261733
}
1727-
logger.warn('Failed to persist resource reorder; will retry on next hydration', error)
1734+
logger.warn(
1735+
unsatisfiable
1736+
? 'Discarded a resource reorder the server rejected'
1737+
: 'Failed to persist resource reorder; will retry on next hydration',
1738+
error
1739+
)
17281740
return
17291741
}
17301742
}
@@ -1891,7 +1903,7 @@ export function useChat(
18911903
}
18921904

18931905
const persistenceScopeId = persistChatId ?? pendingChatKeyRef.current
1894-
resourcePersistenceQueue.enqueue(resourceUpdate, persistChatId, existing, persistenceScopeId)
1906+
resourcePersistenceQueue.enqueue(resourceUpdate, persistChatId, persistenceScopeId, existing)
18951907
return existing === undefined
18961908
},
18971909
[queryClient, resourcePersistenceQueue]

apps/sim/app/workspace/[workspaceId]/tables/[tableId]/table.tsx

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -666,7 +666,11 @@ export function Table({
666666
return
667667
}
668668

669-
if (activeView && activeViewId === null) {
669+
// Embedded tables record the adopted id BEFORE the revision guard can bail:
670+
// `resolveTableViewSelection` resolves a null param to the restored view, so
671+
// leaving the param unwritten lets a later render drift back to the default.
672+
// Standalone tables have no restored view and keep writing it below.
673+
if (embedded && activeView && activeViewId === null) {
670674
setTableParams({ view: activeView.id })
671675
}
672676
const nextViewRevision = getTableViewRevision(activeView)
@@ -685,7 +689,7 @@ export function Table({
685689
if (preserved && preserved.viewId !== nextViewId) {
686690
preservedViewStateRef.current = null
687691
}
688-
if (activeView && activeViewId === ALL_VIEW_PARAM) {
692+
if (activeView && (activeViewId === null || activeViewId === ALL_VIEW_PARAM)) {
689693
setTableParams({ view: activeView.id })
690694
}
691695
const keep = preserved?.viewId === nextViewId ? preserved.keep : undefined
@@ -737,7 +741,21 @@ export function Table({
737741
if (!transition.nextViewId) return
738742
preservedViewStateRef.current = null
739743
setTableParams({ view: transition.nextViewId })
740-
}, [embedded, viewPin, views, activeViewId, tableId, consumeViewPin, setTableParams])
744+
// `viewsAvailable`/`tableAvailable` are what gate first adoption, and
745+
// adoption records itself in a ref, which re-renders nothing. Without them
746+
// a pin that arrives before the table is ready is never reconsidered — the
747+
// restore path has no query invalidation to nudge `views` and rescue it.
748+
}, [
749+
embedded,
750+
viewPin,
751+
views,
752+
activeViewId,
753+
tableId,
754+
viewsAvailable,
755+
tableAvailable,
756+
consumeViewPin,
757+
setTableParams,
758+
])
741759

742760
/**
743761
* Live state pruned the same way `pruneViewConfig` prunes the stored config on

apps/sim/lib/copilot/resources/client-persistence-queue.test.ts

Lines changed: 63 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,8 @@ describe('ResourcePersistenceQueue', () => {
3636
.mockResolvedValueOnce({ success: true })
3737
const queue = new ResourcePersistenceQueue({ persist, onError })
3838

39-
queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-a' }, 'chat-1')
40-
queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-b' }, 'chat-1')
39+
queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-a' }, 'chat-1', 'chat-1')
40+
queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-b' }, 'chat-1', 'chat-1')
4141

4242
await Promise.resolve()
4343
expect(persist).toHaveBeenCalledTimes(1)
@@ -59,12 +59,11 @@ describe('ResourcePersistenceQueue', () => {
5959
.mockResolvedValueOnce({ success: true })
6060
const queue = new ResourcePersistenceQueue({ persist, onError })
6161

62-
queue.enqueue({ ...TABLE_RESOURCE, clearViewId: true }, 'chat-1')
63-
queue.enqueue(TABLE_RESOURCE, 'chat-1')
62+
queue.enqueue({ ...TABLE_RESOURCE, clearViewId: true }, 'chat-1', 'chat-1')
63+
queue.enqueue(TABLE_RESOURCE, 'chat-1', 'chat-1')
6464
first.reject(new Error('offline'))
6565
await Promise.allSettled(Array.from(queue.inFlight.values()))
6666

67-
expect(queue.getPendingUpdates('chat-1')).toEqual([{ ...TABLE_RESOURCE, clearViewId: true }])
6867
await queue.flush('chat-1')
6968

7069
expect(persist.mock.calls[1]).toEqual(['chat-1', { ...TABLE_RESOURCE, clearViewId: true }])
@@ -81,10 +80,10 @@ describe('ResourcePersistenceQueue', () => {
8180
const remove = vi.fn().mockResolvedValue({ success: true })
8281
const queue = new ResourcePersistenceQueue({ persist, onError })
8382

84-
queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-a' }, 'chat-1')
83+
queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-a' }, 'chat-1', 'chat-1')
8584
const removal = queue.remove(TABLE_RESOURCE.type, TABLE_RESOURCE.id, 'chat-1')
8685
removal.scheduleDelete('chat-1', remove)
87-
queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-b' }, 'chat-1')
86+
queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-b' }, 'chat-1', 'chat-1')
8887
await Promise.resolve()
8988

9089
expect(persist).toHaveBeenCalledTimes(1)
@@ -106,7 +105,7 @@ describe('ResourcePersistenceQueue', () => {
106105
.mockReturnValueOnce(failed.promise)
107106
const queue = new ResourcePersistenceQueue({ persist, onError })
108107

109-
queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-a' }, 'chat-1', TABLE_RESOURCE)
108+
queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-a' }, 'chat-1', 'chat-1', TABLE_RESOURCE)
110109
failed.reject(new Error('offline'))
111110
await Promise.allSettled(Array.from(queue.inFlight.values()))
112111

@@ -125,7 +124,7 @@ describe('ResourcePersistenceQueue', () => {
125124
.mockReturnValueOnce(failed.promise)
126125
const queue = new ResourcePersistenceQueue({ persist, onError })
127126

128-
queue.enqueue(TABLE_RESOURCE, 'chat-1')
127+
queue.enqueue(TABLE_RESOURCE, 'chat-1', 'chat-1')
129128
failed.reject(new Error('offline'))
130129
await Promise.allSettled(Array.from(queue.inFlight.values()))
131130

@@ -142,13 +141,13 @@ describe('ResourcePersistenceQueue', () => {
142141
const remove = vi.fn().mockReturnValue(deletion.promise)
143142
const queue = new ResourcePersistenceQueue({ persist, onError })
144143

145-
queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-a' }, 'chat-1', TABLE_RESOURCE)
144+
queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-a' }, 'chat-1', 'chat-1', TABLE_RESOURCE)
146145
await Promise.allSettled(Array.from(queue.inFlight.values()))
147146
const removal = queue.remove(TABLE_RESOURCE.type, TABLE_RESOURCE.id, 'chat-1')
148147
removal.scheduleDelete('chat-1', remove)
149148
await vi.waitFor(() => expect(remove).toHaveBeenCalledOnce())
150149

151-
queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-b' }, 'chat-1')
150+
queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-b' }, 'chat-1', 'chat-1')
152151
await Promise.resolve()
153152
expect(persist).toHaveBeenCalledOnce()
154153

@@ -188,10 +187,10 @@ describe('ResourcePersistenceQueue', () => {
188187
.mockResolvedValue({ success: true })
189188
const queue = new ResourcePersistenceQueue({ persist, onError })
190189

191-
queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-a' }, 'chat-1')
190+
queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-a' }, 'chat-1', 'chat-1')
192191
await vi.waitFor(() => expect(onError).toHaveBeenCalledOnce())
193192

194-
queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-b' }, 'chat-2')
193+
queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-b' }, 'chat-2', 'chat-2')
195194
await vi.waitFor(() => expect(persist).toHaveBeenCalledTimes(2))
196195

197196
expect(persist.mock.calls[1]).toEqual(['chat-2', { ...TABLE_RESOURCE, viewId: 'view-b' }])
@@ -210,7 +209,7 @@ describe('ResourcePersistenceQueue', () => {
210209
.mockResolvedValue({ success: true })
211210
const queue = new ResourcePersistenceQueue({ persist, onError })
212211

213-
queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-a' }, undefined, undefined, 'pending-chat-1')
212+
queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-a' }, undefined, 'pending-chat-1')
214213

215214
expect(queue.getPendingResourceKeys('pending-chat-1')).toEqual(new Set(['table:table-1']))
216215
await queue.flush('chat-1', 'pending-chat-1')
@@ -220,6 +219,54 @@ describe('ResourcePersistenceQueue', () => {
220219
expect(queue.getPendingResourceKeys('chat-1')).toEqual(new Set())
221220
})
222221

222+
it('does not report an unpersisted write once the resource reaches the server', async () => {
223+
const persist = vi
224+
.fn<(chatId: string, update: MothershipResourceUpdate) => Promise<unknown>>()
225+
.mockResolvedValueOnce({ success: true })
226+
.mockRejectedValue(new Error('offline'))
227+
const queue = new ResourcePersistenceQueue({ persist, onError })
228+
229+
queue.enqueue(TABLE_RESOURCE, 'chat-1', 'chat-1')
230+
await Promise.allSettled(queue.getInFlightWrites('chat-1'))
231+
expect(queue.hasUnpersistedWrites('chat-1')).toBe(false)
232+
233+
// A pin update for the same, already-stored resource keeps failing. The
234+
// resource is on the server, so a reorder naming it stays valid.
235+
queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-a' }, 'chat-1', 'chat-1')
236+
await vi.waitFor(() => expect(onError).toHaveBeenCalledOnce())
237+
expect(queue.getPendingResourceKeys('chat-1')).toEqual(new Set(['table:table-1']))
238+
expect(queue.hasUnpersistedWrites('chat-1')).toBe(false)
239+
240+
await queue.flush('chat-1')
241+
expect(queue.hasUnpersistedWrites('chat-1')).toBe(false)
242+
})
243+
244+
it('reports an unpersisted write while a first add has never succeeded', async () => {
245+
const persist = vi
246+
.fn<(chatId: string, update: MothershipResourceUpdate) => Promise<unknown>>()
247+
.mockRejectedValue(new Error('offline'))
248+
const queue = new ResourcePersistenceQueue({ persist, onError })
249+
250+
queue.enqueue(TABLE_RESOURCE, 'chat-1', 'chat-1')
251+
await vi.waitFor(() => expect(onError).toHaveBeenCalledOnce())
252+
253+
expect(queue.hasUnpersistedWrites('chat-1')).toBe(true)
254+
})
255+
256+
it('keeps the newer chat-scoped update when a provisional scope is adopted', async () => {
257+
const persist = vi
258+
.fn<(chatId: string, update: MothershipResourceUpdate) => Promise<unknown>>()
259+
.mockResolvedValue({ success: true })
260+
const queue = new ResourcePersistenceQueue({ persist, onError })
261+
262+
queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-a' }, undefined, 'pending-chat-1')
263+
queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-b' }, undefined, 'chat-1')
264+
await queue.flush('chat-1', 'pending-chat-1')
265+
266+
expect(persist).toHaveBeenCalledTimes(1)
267+
expect(persist).toHaveBeenCalledWith('chat-1', { ...TABLE_RESOURCE, viewId: 'view-b' })
268+
})
269+
223270
it('remembers a stored resource until a failed deletion eventually succeeds', async () => {
224271
const persist = vi
225272
.fn<(chatId: string, update: MothershipResourceUpdate) => Promise<unknown>>()
@@ -228,14 +275,14 @@ describe('ResourcePersistenceQueue', () => {
228275
const remove = vi.fn().mockRejectedValueOnce(new Error('delete failed'))
229276
const queue = new ResourcePersistenceQueue({ persist, onError })
230277

231-
queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-a' }, 'chat-1', TABLE_RESOURCE)
278+
queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-a' }, 'chat-1', 'chat-1', TABLE_RESOURCE)
232279
await Promise.allSettled(queue.getInFlightWrites('chat-1'))
233280

234281
const firstRemoval = queue.remove(TABLE_RESOURCE.type, TABLE_RESOURCE.id, 'chat-1')
235282
firstRemoval.scheduleDelete('chat-1', remove)
236283
await Promise.allSettled(queue.getInFlightWrites('chat-1'))
237284

238-
queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-b' }, 'chat-1')
285+
queue.enqueue({ ...TABLE_RESOURCE, viewId: 'view-b' }, 'chat-1', 'chat-1')
239286
await Promise.allSettled(queue.getInFlightWrites('chat-1'))
240287

241288
const secondRemoval = queue.remove(TABLE_RESOURCE.type, TABLE_RESOURCE.id, 'chat-1')

0 commit comments

Comments
 (0)