Skip to content

Commit d7a16ab

Browse files
committed
fix(chat): make a send's claim permanent only once its turn starts
The claim became permanent as soon as the chat resolved, but three exits still return without starting a turn — a rejected branch, a missing chat, and a pending-stream collision. The last one matters: the queued-send-handoff path deliberately retries under the original `userMessageId` after a collision, and against a permanent claim that retry deduplicated to a chat whose turn never ran, reattaching to a stream that does not exist. A send that had merely collided became unsendable for the claim's full hour. The claim is now dropped immediately before the stream response is returned, so `finally` releases it on every other exit. Recording the chat still happens as early as possible — a concurrent duplicate needs somewhere to go — it just no longer implies the turn happened.
1 parent 98099a6 commit d7a16ab

2 files changed

Lines changed: 62 additions & 8 deletions

File tree

apps/sim/lib/copilot/chat/post.test.ts

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -847,5 +847,52 @@ describe('handleUnifiedChatPost', () => {
847847
expect(createSSEStream).toHaveBeenCalled()
848848
expect(storeChatSendResult).not.toHaveBeenCalled()
849849
})
850+
851+
/**
852+
* The queued-send-handoff path deliberately retries under the original
853+
* `userMessageId` after a stream collision. If the collided attempt left a
854+
* permanent claim, that retry would deduplicate against a chat whose turn
855+
* never started and reattach to a stream that does not exist.
856+
*/
857+
it('releases the claim when a stream collision stops the turn from starting', async () => {
858+
acquirePendingChatStream.mockResolvedValue(false)
859+
getPendingChatStreamId.mockResolvedValue('other-stream')
860+
861+
const response = await handleUnifiedChatPost(
862+
new NextRequest('http://localhost/api/mothership/chat', {
863+
method: 'POST',
864+
body: JSON.stringify({
865+
message: 'Hello',
866+
workspaceId: 'ws-1',
867+
userMessageId: 'msg-1',
868+
createNewChat: true,
869+
}),
870+
})
871+
)
872+
873+
expect(response.status).toBe(409)
874+
expect(releaseChatSendClaim).toHaveBeenCalledWith(
875+
'chat-send:user-message:msg-1:userId=user-1',
876+
'database',
877+
'claim-1'
878+
)
879+
})
880+
881+
it('keeps the claim once a turn is actually streaming', async () => {
882+
const response = await handleUnifiedChatPost(
883+
new NextRequest('http://localhost/api/mothership/chat', {
884+
method: 'POST',
885+
body: JSON.stringify({
886+
message: 'Hello',
887+
workspaceId: 'ws-1',
888+
userMessageId: 'msg-1',
889+
createNewChat: true,
890+
}),
891+
})
892+
)
893+
894+
expect(response.status).toBe(200)
895+
expect(releaseChatSendClaim).not.toHaveBeenCalled()
896+
})
850897
})
851898
})

apps/sim/lib/copilot/chat/post.ts

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1142,9 +1142,10 @@ export async function handleUnifiedChatPost(req: NextRequest) {
11421142
}
11431143

11441144
/* Record the chat as soon as it is known — the earliest a retry can be
1145-
answered with somewhere to go. The claim becomes permanent here: the
1146-
user message lands moments later, so a retry must resolve to this chat
1147-
rather than open another. */
1145+
answered with somewhere to go. The claim is not permanent yet: several
1146+
exits below still return without starting a turn, and a retry of those
1147+
must be allowed to start one. Recording only fails open, so drop the
1148+
claim when it does. */
11481149
if (sendClaim?.claimToken && actualChatId) {
11491150
const recorded = await chatSendIdempotency
11501151
.storeResult(
@@ -1160,7 +1161,7 @@ export async function handleUnifiedChatPost(req: NextRequest) {
11601161
})
11611162
return false
11621163
})
1163-
if (recorded) sendClaim = undefined
1164+
if (!recorded) sendClaim = undefined
11641165
}
11651166

11661167
if (chatIsNew && actualChatId && body.resourceAttachments?.length) {
@@ -1456,6 +1457,11 @@ export async function handleUnifiedChatPost(req: NextRequest) {
14561457
const rootTraceparent = `00-${rootCtx.traceId}-${rootCtx.spanId}-${
14571458
(rootCtx.traceFlags & 0x1) === 0x1 ? '01' : '00'
14581459
}`
1460+
/* A turn is running. Only now is the claim permanent, so the `finally`
1461+
below leaves it in place and a retry of this send resolves to this
1462+
chat instead of opening another. Every earlier exit returns without a
1463+
turn, and releases. */
1464+
sendClaim = undefined
14591465
return new Response(stream, {
14601466
headers: {
14611467
...SSE_RESPONSE_HEADERS,
@@ -1497,10 +1503,11 @@ export async function handleUnifiedChatPost(req: NextRequest) {
14971503
{ status: 500 }
14981504
)
14991505
} finally {
1500-
/* A claim still held here never recorded a chat — the send threw, or
1501-
returned early on a rejected branch or a missing chat. Release it so a
1502-
retry is treated as new rather than deduplicated against a chat that was
1503-
never opened. Must be `finally`: those early returns skip `catch`. */
1506+
/* A claim still held here never started a turn — the send threw, or
1507+
returned early on a rejected branch, a missing chat, or a chat that
1508+
already has a stream running. Release it so a retry may start one rather
1509+
than deduplicating against a turn that never happened. Must be
1510+
`finally`: those early returns skip `catch`. */
15041511
if (sendClaim?.claimToken) {
15051512
await chatSendIdempotency
15061513
.release(sendClaim.normalizedKey, sendClaim.storageMethod, sendClaim.claimToken)

0 commit comments

Comments
 (0)