Skip to content

Commit b84001d

Browse files
committed
fix(chat): probe the orphaned stream before re-sending a withdrawn send
The cleanup-abort recovery treated "no response headers yet" as "the server never got it" and re-sent. It is not the same thing: the mothership chat route never reads `request.signal`, so a request it had already accepted still runs to completion — resolveOrCreateChat, persistUserMessage, and the billed turn all commit even though the client socket is gone. Re-sending blind therefore left the user with two chats and two billed runs for one message. Recovery now carries the withdrawn send's `userMessageId` as a stream id through both lanes (the live `mothership-send-message` event and the stored one-shot handoff) and through a restored queue entry. Before re-sending, the dispatcher polls that stream: when it resolves to a chat, the server already has the message, so the chat is adopted instead of sent again. Only a stream the server has no record of — a 404, i.e. genuinely never accepted — re-sends. Timing out re-sends too, which is the safe direction. Also corrects the root cause recorded in the comments. A Suspense hide/reveal cannot run this cleanup: React 19 disappears layout effects only, and this is a passive effect (verified against react-dom 19.2.4). What does run it is StrictMode's dev double-mount and a real client-side navigation away, both mid-flight — and because MothershipHandoffStorage consumes atomically, the replacement mount finds nothing left to retry.
1 parent 3e67769 commit b84001d

8 files changed

Lines changed: 362 additions & 45 deletions

File tree

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

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -341,7 +341,9 @@ export function Home({ chatId, userName, userId, tableViewsEnabled }: HomeProps)
341341
const detail = (e as CustomEvent<MothershipSendMessageDetail>).detail
342342
if (!detail?.message) return
343343
e.preventDefault()
344-
sendMessage(detail.message, detail.fileAttachments, detail.contexts)
344+
sendMessage(detail.message, detail.fileAttachments, detail.contexts, {
345+
...(detail.recoverStreamId ? { recoverStreamId: detail.recoverStreamId } : {}),
346+
})
345347
}
346348
window.addEventListener(MOTHERSHIP_SEND_MESSAGE_EVENT, handler)
347349
return () => window.removeEventListener(MOTHERSHIP_SEND_MESSAGE_EVENT, handler)
@@ -370,7 +372,9 @@ export function Home({ chatId, userName, userId, tableViewsEnabled }: HomeProps)
370372
const handoff = MothershipHandoffStorage.consume(workspaceId)
371373
if (!handoff) return
372374
if (handoff.message) {
373-
sendMessage(handoff.message, handoff.fileAttachments, handoff.contexts)
375+
sendMessage(handoff.message, handoff.fileAttachments, handoff.contexts, {
376+
...(handoff.recoverStreamId ? { recoverStreamId: handoff.recoverStreamId } : {}),
377+
})
374378
return
375379
}
376380
const contexts = handoff.contexts ?? []

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

Lines changed: 173 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,22 @@
11
/**
22
* @vitest-environment jsdom
33
*
4-
* Regression tests for the mount-settling send loss: a send started on a
5-
* fresh chat surface used to be silently dropped when React ran the unmount
6-
* cleanup mid-flight (a Suspense hide/reveal cycles every effect shortly
7-
* after Home mounts), aborting the fetch before it dispatched. The fix routes
8-
* idle sends through the durable message queue and restores the queued entry
9-
* when the cleanup abort strikes before the server received the request.
4+
* Regression tests for the remount send loss: a send started on a fresh chat
5+
* surface was silently dropped when the hook's unmount cleanup ran mid-flight
6+
* and aborted the POST. Two things run that cleanup while an auto-send from a
7+
* cross-route handoff is still in flight — StrictMode's dev double-mount, and a
8+
* real client-side navigation away — and because `MothershipHandoffStorage`
9+
* consumes atomically, the second mount finds nothing left to retry.
10+
*
11+
* (A Suspense hide/reveal does NOT cause this: React 19 disappears layout
12+
* effects only, so this passive cleanup never runs for it.)
13+
*
14+
* The fix routes idle sends through the durable queue so every send has a
15+
* recoverable entry, and recovers one the cleanup withdrew — probing the
16+
* orphaned stream first so a request the server had already accepted is
17+
* adopted rather than sent twice.
1018
*/
11-
import { act, type ReactNode } from 'react'
19+
import { act, type ReactNode, StrictMode, useEffect } from 'react'
1220
import { sleep } from '@sim/utils/helpers'
1321
import { QueryClient, QueryClientProvider } from '@tanstack/react-query'
1422
import { createRoot, type Root } from 'react-dom/client'
@@ -38,9 +46,21 @@ interface NetworkState {
3846
/** How the chat POST behaves for the next call. */
3947
postBehavior: 'hang' | 'accept'
4048
postCalls: number
49+
/**
50+
* Chat the orphaned-stream probe resolves to, standing in for a request the
51+
* server accepted before the client's cleanup abort tore the socket down.
52+
* `null` means the server has no such stream (it never accepted the request).
53+
*/
54+
orphanedStreamChatId: string | null
55+
streamProbes: number
4156
}
4257

43-
const state: NetworkState = { postBehavior: 'hang', postCalls: 0 }
58+
const state: NetworkState = {
59+
postBehavior: 'hang',
60+
postCalls: 0,
61+
orphanedStreamChatId: null,
62+
streamProbes: 0,
63+
}
4464

4565
/** An SSE response whose stream ends immediately without a terminal event. */
4666
function emptySseResponse(): Response {
@@ -55,6 +75,26 @@ function emptySseResponse(): Response {
5575
async function fetchStub(input: RequestInfo | URL, init?: RequestInit): Promise<Response> {
5676
const url = String(input instanceof Request ? input.url : input)
5777

78+
// The orphaned-stream probe: does the server hold a stream for the send the
79+
// cleanup abort withdrew?
80+
if (url.includes('/api/mothership/chat/stream')) {
81+
state.streamProbes++
82+
// 404 is what the server returns for a stream it never registered — i.e.
83+
// the request really was withdrawn before it was accepted.
84+
if (!state.orphanedStreamChatId) {
85+
return new Response(JSON.stringify({ error: 'stream gone' }), { status: 404 })
86+
}
87+
return new Response(
88+
JSON.stringify({
89+
success: true,
90+
events: [],
91+
status: 'streaming',
92+
chatId: state.orphanedStreamChatId,
93+
}),
94+
{ status: 200, headers: { 'Content-Type': 'application/json' } }
95+
)
96+
}
97+
5898
if (url.includes('/api/mothership/chat') && init?.method === 'POST') {
5999
state.postCalls++
60100
if (state.postBehavior === 'accept') return emptySseResponse()
@@ -108,6 +148,43 @@ function renderUseChat(): {
108148
}
109149
}
110150

151+
/**
152+
* Mounts the hook under StrictMode with a handoff already in storage, mirroring
153+
* `home.tsx`'s consume-and-auto-send effect. This is the production-shaped
154+
* failure: the dev double-mount runs the passive cleanup between the two
155+
* mounts, aborting the in-flight POST, and `consume` has already cleared the
156+
* entry so the second mount has nothing to replay.
157+
*/
158+
function renderStrictModeHandoffConsumer(): { unmount: () => void } {
159+
;(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true
160+
queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } })
161+
const container = document.createElement('div')
162+
const root = createRoot(container)
163+
mountedRoots.push(root)
164+
165+
function Probe() {
166+
const { sendMessage } = useChat('ws-1', undefined)
167+
useEffect(() => {
168+
const handoff = MothershipHandoffStorage.consume('ws-1')
169+
if (!handoff?.message) return
170+
sendMessage(handoff.message, handoff.fileAttachments, handoff.contexts, {
171+
...(handoff.recoverStreamId ? { recoverStreamId: handoff.recoverStreamId } : {}),
172+
})
173+
}, [sendMessage])
174+
return null
175+
}
176+
177+
act(() => {
178+
root.render(
179+
<StrictMode>
180+
<QueryClientProvider client={queryClient}>{(<Probe />) as ReactNode}</QueryClientProvider>
181+
</StrictMode>
182+
)
183+
})
184+
185+
return { unmount: () => act(() => root.unmount()) }
186+
}
187+
111188
/** Every queued message across all chat keys, flattened. */
112189
function allQueuedMessages() {
113190
return Object.values(useMothershipQueueStore.getState().queues).flat()
@@ -123,11 +200,13 @@ async function waitFor(predicate: () => boolean, budgetMs = 2000): Promise<void>
123200
}
124201
}
125202

126-
describe('useChat mount-settling send recovery', () => {
203+
describe('useChat remount send recovery', () => {
127204
beforeEach(() => {
128205
vi.stubGlobal('fetch', fetchStub)
129206
state.postBehavior = 'hang'
130207
state.postCalls = 0
208+
state.orphanedStreamChatId = null
209+
state.streamProbes = 0
131210
mockRequestJson.mockResolvedValue({ chats: [] })
132211
useMothershipQueueStore.setState({ queues: {}, editing: {} })
133212
window.sessionStorage.clear()
@@ -195,11 +274,11 @@ describe('useChat mount-settling send recovery', () => {
195274
// The dispatch claimed the queue head when the optimistic send applied.
196275
expect(allQueuedMessages()).toHaveLength(0)
197276

198-
// The cleanup abort (the same code path the mount-settling remount runs)
199-
// fires while the POST is still awaiting the server. A chatless surface
200-
// regenerates its queue key per mount, so recovery re-persists the send
201-
// as a one-shot handoff for the next mount's consumer instead of
202-
// restoring the dead instance's queue.
277+
// The cleanup abort (the same code path a StrictMode remount or a real
278+
// navigation away runs) fires while the POST is still awaiting the server.
279+
// A chatless surface regenerates its queue key per mount, so recovery
280+
// re-persists the send as a one-shot handoff for the next mount's consumer
281+
// instead of restoring the dead instance's queue.
203282
unmount()
204283
await waitFor(() => window.localStorage.getItem('sim_mothership_handoff') !== null)
205284

@@ -226,4 +305,84 @@ describe('useChat mount-settling send recovery', () => {
226305
expect(allQueuedMessages()).toHaveLength(0)
227306
expect(MothershipHandoffStorage.consume('ws-1')).toBeNull()
228307
})
308+
309+
/**
310+
* The end-to-end failure, driven by the thing that actually runs the cleanup
311+
* mid-flight rather than by a hand-rolled unmount. On the unfixed hook the
312+
* handoff is consumed, the POST is aborted, and nothing survives to retry.
313+
*/
314+
it('keeps a cross-route handoff recoverable across a StrictMode double-mount', async () => {
315+
MothershipHandoffStorage.store({ message: 'investigate this failed run' }, 'ws-1')
316+
317+
renderStrictModeHandoffConsumer()
318+
await waitFor(() => state.postCalls >= 1)
319+
320+
// Something must still be holding the message: either the live event was
321+
// claimed and it is queued/in flight again, or it is back in storage.
322+
await waitFor(() => {
323+
const stored = window.localStorage.getItem('sim_mothership_handoff')
324+
return stored !== null || allQueuedMessages().length > 0 || state.postCalls > 1
325+
})
326+
})
327+
328+
/**
329+
* The abort tears down the client socket but the route handler never reads
330+
* `request.signal` — a request the server had already accepted still creates
331+
* the chat, persists the user message, and runs (and bills) the turn. So the
332+
* recovered send has to ask whether that happened before sending again.
333+
*/
334+
describe('recovered send probes the orphaned stream before re-sending', () => {
335+
it('adopts the chat the server already created instead of sending twice', async () => {
336+
// The server accepted the withdrawn request and registered its stream.
337+
state.orphanedStreamChatId = 'chat-server-already-made'
338+
339+
const { getResult, unmount } = renderUseChat()
340+
await act(async () => {
341+
void getResult().sendMessage('only once please')
342+
})
343+
await waitFor(() => state.postCalls === 1)
344+
unmount()
345+
await waitFor(() => window.localStorage.getItem('sim_mothership_handoff') !== null)
346+
347+
// The next mount consumes the handoff, exactly as home.tsx does.
348+
const handoff = MothershipHandoffStorage.consume('ws-1')
349+
expect(handoff?.recoverStreamId).toBeTruthy()
350+
351+
const replacement = renderUseChat()
352+
await act(async () => {
353+
void replacement.getResult().sendMessage(handoff?.message as string, undefined, undefined, {
354+
recoverStreamId: handoff?.recoverStreamId as string,
355+
})
356+
})
357+
await waitFor(() => state.streamProbes > 0)
358+
await waitFor(() => replacement.getResult().resolvedChatId === 'chat-server-already-made')
359+
360+
expect(state.postCalls).toBe(1)
361+
expect(allQueuedMessages()).toHaveLength(0)
362+
})
363+
364+
it('re-sends when the server has no stream for it', async () => {
365+
// 404 from the probe: the request really was withdrawn before acceptance.
366+
state.orphanedStreamChatId = null
367+
368+
const { getResult, unmount } = renderUseChat()
369+
await act(async () => {
370+
void getResult().sendMessage('please actually send me')
371+
})
372+
await waitFor(() => state.postCalls === 1)
373+
unmount()
374+
await waitFor(() => window.localStorage.getItem('sim_mothership_handoff') !== null)
375+
376+
const handoff = MothershipHandoffStorage.consume('ws-1')
377+
const replacement = renderUseChat()
378+
await act(async () => {
379+
void replacement.getResult().sendMessage(handoff?.message as string, undefined, undefined, {
380+
recoverStreamId: handoff?.recoverStreamId as string,
381+
})
382+
})
383+
await waitFor(() => state.postCalls === 2)
384+
385+
expect(state.streamProbes).toBeGreaterThan(0)
386+
})
387+
})
229388
})

0 commit comments

Comments
 (0)