Skip to content

Commit ac7a732

Browse files
committed
fix(chat): deduplicate chat sends server-side instead of probing for them
A client cannot tell whether a request it aborted reached the server: the chat route never reads `request.signal`, so an accepted one still opens the chat, persists the user message, and bills the turn after the socket drops. #6525 answered that by polling the orphaned stream before retrying — a 2.5s guess that had to distinguish "no such stream" from "we stopped looking", and still left a window open. The codebase already owns the right tool. `IdempotencyService` backs webhook, polling, and billing dedup, and `billingIdempotency` exists for exactly this hazard: "a retry would double-record usage — real money". Chat sends now claim the same way, keyed on the client-generated `userMessageId` and scoped to the caller so nobody can probe another user's sends. A repeat gets 409 naming the chat the first attempt opened — deliberately the shape the pending-stream lock already returns, so the client's existing conflict handler reattaches instead of starting a turn, with only the chat-adoption line added. The claim fails open at every step. Deduplication saves a duplicate chat; the send IS the user's message, so an unreachable bookkeeping store degrades chat rather than taking it down. It is released when a send fails before recording a chat, and deliberately kept once recorded. Retrying now just reuses the id, which deletes the probe outright: the poll and its two constants, the three-state result, the epoch plumbing that kept a superseded poll from re-sending, and the chat-adoption branch it needed. The client hook nets 67 lines smaller. Idle sends go back to calling `startSendMessage` directly. #6525 routed them through the durable queue so recovery had a backing entry, which put every message in the product through the queue store, sessionStorage, and the dispatch loop for the sake of a rare path — and the recovery never needed it, since the message, attachments, contexts, and id are all in scope at the abort. Both callers now share one `handOffWithdrawnSend`. `startSendMessage` takes its optional tail as an options object; it was at six positional parameters and the retry id would have been a seventh. Tests cover both halves: the server dedups, scopes the key per user, records the chat, and still sends when the claim store is down; the client reuses the original id on retry and adopts the chat a deduplicated retry names. Each was confirmed red without its fix.
1 parent 9277e7d commit ac7a732

11 files changed

Lines changed: 577 additions & 492 deletions

File tree

apps/sim/app/workspace/[workspaceId]/home/home.tsx

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -342,7 +342,7 @@ export function Home({ chatId, userName, userId, tableViewsEnabled }: HomeProps)
342342
if (!detail?.message) return
343343
e.preventDefault()
344344
sendMessage(detail.message, detail.fileAttachments, detail.contexts, {
345-
...(detail.recoverStreamId ? { recoverStreamId: detail.recoverStreamId } : {}),
345+
...(detail.resumeUserMessageId ? { resumeUserMessageId: detail.resumeUserMessageId } : {}),
346346
})
347347
}
348348
window.addEventListener(MOTHERSHIP_SEND_MESSAGE_EVENT, handler)
@@ -373,7 +373,9 @@ export function Home({ chatId, userName, userId, tableViewsEnabled }: HomeProps)
373373
if (!handoff) return
374374
if (handoff.message) {
375375
sendMessage(handoff.message, handoff.fileAttachments, handoff.contexts, {
376-
...(handoff.recoverStreamId ? { recoverStreamId: handoff.recoverStreamId } : {}),
376+
...(handoff.resumeUserMessageId
377+
? { resumeUserMessageId: handoff.resumeUserMessageId }
378+
: {}),
377379
})
378380
return
379381
}

apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.mount-send.test.tsx

Lines changed: 140 additions & 248 deletions
Large diffs are not rendered by default.

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

Lines changed: 140 additions & 215 deletions
Large diffs are not rendered by default.

apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/panel.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -490,7 +490,7 @@ export const Panel = memo(function Panel() {
490490
e.preventDefault()
491491
setActiveTab('copilot')
492492
copilotSendMessage(detail.message, detail.fileAttachments, detail.contexts, {
493-
...(detail.recoverStreamId ? { recoverStreamId: detail.recoverStreamId } : {}),
493+
...(detail.resumeUserMessageId ? { resumeUserMessageId: detail.resumeUserMessageId } : {}),
494494
})
495495
}
496496
window.addEventListener(MOTHERSHIP_SEND_MESSAGE_EVENT, handler)

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

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,9 @@ const {
3636
appendCopilotChatMessages,
3737
persistChatResources,
3838
mockPublishStatusChanged,
39+
atomicallyClaimChatSend,
40+
storeChatSendResult,
41+
releaseChatSendClaim,
3942
} = vi.hoisted(() => ({
4043
generateWorkspaceSnapshot: vi.fn(),
4144
processContextsServer: vi.fn(),
@@ -51,6 +54,9 @@ const {
5154
appendCopilotChatMessages: vi.fn(),
5255
persistChatResources: vi.fn(),
5356
mockPublishStatusChanged: vi.fn(),
57+
atomicallyClaimChatSend: vi.fn(),
58+
storeChatSendResult: vi.fn(),
59+
releaseChatSendClaim: vi.fn(),
5460
}))
5561

5662
const getSession = authMockFns.mockGetSession
@@ -103,6 +109,14 @@ vi.mock('@/lib/copilot/chat/lifecycle', () => ({
103109
resolveOrCreateChat,
104110
}))
105111

112+
vi.mock('@/lib/core/idempotency', () => ({
113+
chatSendIdempotency: {
114+
atomicallyClaim: atomicallyClaimChatSend,
115+
storeResult: storeChatSendResult,
116+
release: releaseChatSendClaim,
117+
},
118+
}))
119+
106120
vi.mock('@/lib/copilot/chat/terminal-state', () => ({
107121
finalizeAssistantTurn,
108122
}))
@@ -132,6 +146,14 @@ describe('handleUnifiedChatPost', () => {
132146
beforeEach(() => {
133147
vi.clearAllMocks()
134148
resetDbChainMock()
149+
atomicallyClaimChatSend.mockResolvedValue({
150+
claimed: true,
151+
normalizedKey: 'chat-send:mothership:msg-1:userId=user-1',
152+
storageMethod: 'database',
153+
claimToken: 'claim-1',
154+
})
155+
storeChatSendResult.mockResolvedValue(true)
156+
releaseChatSendClaim.mockResolvedValue(undefined)
135157
getSession.mockResolvedValue({ user: { id: 'user-1' } })
136158
resolveWorkflowIdForUser.mockResolvedValue({
137159
status: 'resolved',
@@ -725,4 +747,105 @@ describe('handleUnifiedChatPost', () => {
725747
error: 'workspaceId is required when workflowId is not provided',
726748
})
727749
})
750+
751+
describe('deduplicating a repeated send', () => {
752+
/**
753+
* The client cannot tell whether a request it aborted reached the server —
754+
* the route never reads `request.signal`, so an accepted one runs to
755+
* completion regardless. Recovering such a send therefore retries it under
756+
* the original `userMessageId`, and this is what makes that safe.
757+
*/
758+
it('answers an already-claimed send with the chat the first attempt opened', async () => {
759+
atomicallyClaimChatSend.mockResolvedValue({
760+
claimed: false,
761+
normalizedKey: 'chat-send:mothership:msg-1:userId=user-1',
762+
storageMethod: 'database',
763+
existingResult: { success: true, status: 'completed', result: { chatId: 'chat-first' } },
764+
})
765+
766+
const response = await handleUnifiedChatPost(
767+
new NextRequest('http://localhost/api/mothership/chat', {
768+
method: 'POST',
769+
body: JSON.stringify({
770+
message: 'Hello',
771+
workspaceId: 'ws-1',
772+
userMessageId: 'msg-1',
773+
createNewChat: true,
774+
}),
775+
})
776+
)
777+
778+
expect(response.status).toBe(409)
779+
await expect(response.json()).resolves.toMatchObject({
780+
activeStreamId: 'msg-1',
781+
chatId: 'chat-first',
782+
})
783+
// The whole point: no second chat, no second billed turn.
784+
expect(resolveOrCreateChat).not.toHaveBeenCalled()
785+
expect(createSSEStream).not.toHaveBeenCalled()
786+
})
787+
788+
it('scopes the claim to the caller so one user cannot probe another', async () => {
789+
await handleUnifiedChatPost(
790+
new NextRequest('http://localhost/api/mothership/chat', {
791+
method: 'POST',
792+
body: JSON.stringify({
793+
message: 'Hello',
794+
workspaceId: 'ws-1',
795+
userMessageId: 'msg-1',
796+
createNewChat: true,
797+
}),
798+
})
799+
)
800+
801+
expect(atomicallyClaimChatSend).toHaveBeenCalledWith('mothership', 'msg-1', {
802+
userId: 'user-1',
803+
})
804+
})
805+
806+
it('records the chat against the send so a retry resolves to it', async () => {
807+
await handleUnifiedChatPost(
808+
new NextRequest('http://localhost/api/mothership/chat', {
809+
method: 'POST',
810+
body: JSON.stringify({
811+
message: 'Hello',
812+
workspaceId: 'ws-1',
813+
userMessageId: 'msg-1',
814+
createNewChat: true,
815+
}),
816+
})
817+
)
818+
819+
expect(storeChatSendResult).toHaveBeenCalledWith(
820+
'chat-send:mothership:msg-1:userId=user-1',
821+
expect.objectContaining({ result: { chatId: 'chat-1' } }),
822+
'database',
823+
'claim-1'
824+
)
825+
})
826+
827+
/**
828+
* Deduplication saves a duplicate chat; the send IS the user's message.
829+
* An unreachable bookkeeping store must degrade chat, never take it down.
830+
*/
831+
it('sends normally when the claim store is unavailable', async () => {
832+
atomicallyClaimChatSend.mockRejectedValue(new Error('idempotency store down'))
833+
834+
const response = await handleUnifiedChatPost(
835+
new NextRequest('http://localhost/api/mothership/chat', {
836+
method: 'POST',
837+
body: JSON.stringify({
838+
message: 'Hello',
839+
workspaceId: 'ws-1',
840+
userMessageId: 'msg-1',
841+
createNewChat: true,
842+
}),
843+
})
844+
)
845+
846+
expect(response.status).toBe(200)
847+
expect(createSSEStream).toHaveBeenCalled()
848+
expect(storeChatSendResult).not.toHaveBeenCalled()
849+
})
850+
})
728851
})

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

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,8 @@ import {
5858
sanitizeChatResources,
5959
} from '@/lib/copilot/resources/types'
6060
import { prepareExecutionContext } from '@/lib/copilot/tools/handlers/context'
61+
import type { AtomicClaimResult } from '@/lib/core/idempotency'
62+
import { chatSendIdempotency } from '@/lib/core/idempotency'
6163
import { captureServerEvent } from '@/lib/posthog/server'
6264
import { resolveWorkflowIdForUser } from '@/lib/workflows/utils'
6365
import {
@@ -943,10 +945,86 @@ async function resolveBranch(params: {
943945
}
944946
}
945947

948+
/** Namespace-local provider segment for {@link chatSendIdempotency} keys. */
949+
const CHAT_SEND_IDEMPOTENCY_PROVIDER = 'mothership'
950+
951+
/**
952+
* Claims this send so a retry of it can be recognised.
953+
*
954+
* Fails open. Deduplication is an optimization here — it saves a duplicate chat
955+
* and a duplicate billed turn — whereas the send itself is the user's message,
956+
* and refusing it because a bookkeeping store is unreachable would take chat
957+
* down for everyone rather than degrade it. An unavailable store therefore
958+
* returns `undefined`, which sends normally with no claim to finalize.
959+
*
960+
* The key is scoped to the caller: `userMessageId` is client-supplied, so an
961+
* unscoped one would let a user probe another's sends and read back their chat
962+
* id.
963+
*/
964+
async function claimChatSend(
965+
userMessageId: string,
966+
userId: string,
967+
requestId: string
968+
): Promise<AtomicClaimResult | undefined> {
969+
try {
970+
return await chatSendIdempotency.atomicallyClaim(
971+
CHAT_SEND_IDEMPOTENCY_PROVIDER,
972+
userMessageId,
973+
{ userId }
974+
)
975+
} catch (error) {
976+
logger.warn(`[${requestId}] Could not claim chat send; proceeding without deduplication`, {
977+
userMessageId,
978+
error: getErrorMessage(error, 'Unknown error'),
979+
})
980+
return undefined
981+
}
982+
}
983+
984+
/** Chat a previously-claimed send resolved to, when it got that far. */
985+
function claimedChatId(claim: AtomicClaimResult): string | undefined {
986+
const chatId = claim.existingResult?.result?.chatId
987+
return typeof chatId === 'string' && chatId ? chatId : undefined
988+
}
989+
990+
/**
991+
* Answers a send whose `userMessageId` was already claimed.
992+
*
993+
* Deliberately the same 409 shape the pending-stream lock returns, because the
994+
* client's conflict handler already knows how to reattach to `activeStreamId`
995+
* instead of starting a turn — a duplicate send and a send that collided with
996+
* an in-flight one want exactly the same thing. `chatId` rides along when the
997+
* first attempt got far enough to resolve one, letting a chatless client adopt
998+
* it without a stream-to-chat lookup.
999+
*/
1000+
function duplicateChatSendResponse(
1001+
claim: AtomicClaimResult,
1002+
userMessageId: string,
1003+
requestId: string
1004+
): NextResponse {
1005+
const chatId = claimedChatId(claim)
1006+
logger.info(`[${requestId}] Deduplicated a repeated chat send`, { userMessageId, chatId })
1007+
return NextResponse.json(
1008+
{
1009+
error: 'This message was already sent.',
1010+
activeStreamId: userMessageId,
1011+
...(chatId ? { chatId } : {}),
1012+
},
1013+
{ status: 409 }
1014+
)
1015+
}
1016+
9461017
export async function handleUnifiedChatPost(req: NextRequest) {
9471018
let actualChatId: string | undefined
9481019
let userMessageId = ''
9491020
let chatStreamLockAcquired = false
1021+
/* Held from the moment this send is claimed until the chat it belongs to is
1022+
recorded. Released on the error path so a failed send can be retried, and
1023+
deliberately NOT released once recorded — by then the chat exists and the
1024+
user message is persisted, so a retry must resolve to it rather than open
1025+
a second chat. */
1026+
let sendClaim: AtomicClaimResult | undefined
1027+
let sendClaimRecorded = false
9501028
// Started once we've parsed the body (need userMessageId to stamp as
9511029
// streamId). Every subsequent span (persistUserMessage,
9521030
// createRunSegment, the whole SSE stream, etc.) nests under this
@@ -981,6 +1059,11 @@ export async function handleUnifiedChatPost(req: NextRequest) {
9811059
const normalizedContexts = normalizeContexts(body.contexts) ?? []
9821060
userMessageId = body.userMessageId || generateId()
9831061

1062+
sendClaim = await claimChatSend(userMessageId, authenticatedUserId, requestId)
1063+
if (sendClaim?.claimed === false) {
1064+
return duplicateChatSendResponse(sendClaim, userMessageId, requestId)
1065+
}
1066+
9841067
otelRoot = startCopilotOtelRoot({
9851068
streamId: userMessageId,
9861069
executionId,
@@ -1073,6 +1156,28 @@ export async function handleUnifiedChatPost(req: NextRequest) {
10731156
}
10741157
}
10751158

1159+
/* Record the chat against this send as soon as it is known, the earliest
1160+
a retry can be answered with somewhere to go. From here the claim is
1161+
permanent: the user message lands moments later, so a retry must
1162+
resolve to this chat rather than open another. Failing to record only
1163+
costs deduplication, so it must not fail the send. */
1164+
if (sendClaim?.claimToken && actualChatId) {
1165+
sendClaimRecorded = await chatSendIdempotency
1166+
.storeResult(
1167+
sendClaim.normalizedKey,
1168+
{ success: true, status: 'completed', result: { chatId: actualChatId } },
1169+
sendClaim.storageMethod,
1170+
sendClaim.claimToken
1171+
)
1172+
.catch((error) => {
1173+
logger.warn(`[${requestId}] Could not record the chat for this send`, {
1174+
userMessageId,
1175+
error: getErrorMessage(error, 'Unknown error'),
1176+
})
1177+
return false
1178+
})
1179+
}
1180+
10761181
if (chatIsNew && actualChatId && body.resourceAttachments?.length) {
10771182
// Canonicalizes here, not just inside `persistChatResources`: several
10781183
// browser tabs collapse onto the one Browser panel before they are
@@ -1377,6 +1482,19 @@ export async function handleUnifiedChatPost(req: NextRequest) {
13771482
if (chatStreamLockAcquired && actualChatId && userMessageId) {
13781483
await releasePendingChatStream(actualChatId, userMessageId)
13791484
}
1485+
// Nothing was recorded against this send, so let a retry of it through
1486+
// rather than deduplicating against a chat that was never opened. A failed
1487+
// release just leaves the claim to expire on its own short in-progress TTL.
1488+
if (sendClaim?.claimToken && !sendClaimRecorded) {
1489+
await chatSendIdempotency
1490+
.release(sendClaim.normalizedKey, sendClaim.storageMethod, sendClaim.claimToken)
1491+
.catch((releaseError) => {
1492+
logger.warn(`[${requestId}] Could not release the claim for a failed send`, {
1493+
userMessageId,
1494+
error: getErrorMessage(releaseError, 'Unknown error'),
1495+
})
1496+
})
1497+
}
13801498
otelRoot?.finish('error', error)
13811499

13821500
if (isZodError(error)) {

apps/sim/lib/core/idempotency/service.ts

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -733,3 +733,29 @@ export const billingIdempotency = new IdempotencyService({
733733
ttlSeconds: 60 * 60, // 1 hour
734734
forceStorage: 'database',
735735
})
736+
737+
/**
738+
* Dedupes a chat send by its client-generated `userMessageId`, so re-sending
739+
* one is safe.
740+
*
741+
* The client cannot tell whether a request it aborted reached the server: the
742+
* chat route never reads `request.signal`, so an accepted request creates the
743+
* chat, persists the user message, and runs the (billed) turn even after the
744+
* browser drops the socket. Without this, a client that recovers an aborted
745+
* send has to choose between losing the message and duplicating the run.
746+
*
747+
* Storage is forced to Postgres for the same reason as {@link billingIdempotency}:
748+
* a missed dedup is a second LLM turn billed to the workspace, so the key must
749+
* not be evictable under Redis memory pressure. The added 1-5ms is invisible
750+
* next to the LLM call this request is about to make.
751+
*
752+
* `inProgressTtlSeconds` is short so a crashed pod cannot block a genuine retry
753+
* for the full hour, while completed sends stay deduplicated for it.
754+
*/
755+
export const chatSendIdempotency = new IdempotencyService({
756+
namespace: 'chat-send',
757+
ttlSeconds: 60 * 60, // 1 hour
758+
inProgressTtlSeconds: 60,
759+
retryFailures: true,
760+
forceStorage: 'database',
761+
})

0 commit comments

Comments
 (0)