Skip to content

Commit d6a5238

Browse files
committed
fix(chat): never re-send on an unresolved probe, and reconnect after adopting
Two defects in the orphaned-stream probe, both found by Bugbot. A probe cut short by an epoch change (unmount, chat switch) returned the same `undefined` as "the server has no such stream", so the dispatcher fell through to `startSendMessage`. After unmount the teardown has already dropped the abort controller, so that send opened a POST nothing could cancel — duplicating the very message this recovery exists to protect. The probe now reports `superseded` distinctly and the dispatcher leaves the entry queued, keeping its `recoverStreamId` so a later mount probes again. Adopting the recovered chat also invalidated only the chat list. Hydration reconnects to a live turn solely on `chatHistory.activeStreamId`, and that query is cached for MOTHERSHIP_CHAT_HISTORY_STALE_TIME — on a chat-bound recover the client normally holds a copy predating this stream, so the adopted chat rendered with the running response invisible. Adoption now invalidates the chat detail too. Both regression tests were confirmed to fail without their fix: the first re-sends (2 POSTs instead of 1), the second never invalidates. The probe stub gained a `pending` mode because a `gone` probe answers on the first attempt and leaves nothing in flight to interrupt — the earlier draft of the first test passed with the guard removed and proved nothing.
1 parent b84001d commit d6a5238

2 files changed

Lines changed: 138 additions & 27 deletions

File tree

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

Lines changed: 91 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -47,18 +47,22 @@ interface NetworkState {
4747
postBehavior: 'hang' | 'accept'
4848
postCalls: number
4949
/**
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).
50+
* How the orphaned-stream probe answers:
51+
* - `found` — the server accepted the withdrawn request and owns a chat
52+
* - `gone` — 404, it has no such stream (never accepted)
53+
* - `pending` — registered but no owner yet, so the probe keeps polling;
54+
* this is the only mode that leaves a probe in flight to interrupt
5355
*/
54-
orphanedStreamChatId: string | null
56+
probeBehavior: 'found' | 'gone' | 'pending'
57+
orphanedStreamChatId: string
5558
streamProbes: number
5659
}
5760

5861
const state: NetworkState = {
5962
postBehavior: 'hang',
6063
postCalls: 0,
61-
orphanedStreamChatId: null,
64+
probeBehavior: 'gone',
65+
orphanedStreamChatId: 'chat-server-already-made',
6266
streamProbes: 0,
6367
}
6468

@@ -81,15 +85,16 @@ async function fetchStub(input: RequestInfo | URL, init?: RequestInit): Promise<
8185
state.streamProbes++
8286
// 404 is what the server returns for a stream it never registered — i.e.
8387
// the request really was withdrawn before it was accepted.
84-
if (!state.orphanedStreamChatId) {
88+
if (state.probeBehavior === 'gone') {
8589
return new Response(JSON.stringify({ error: 'stream gone' }), { status: 404 })
8690
}
8791
return new Response(
8892
JSON.stringify({
8993
success: true,
9094
events: [],
9195
status: 'streaming',
92-
chatId: state.orphanedStreamChatId,
96+
// `pending` omits the owner, so the probe keeps polling.
97+
...(state.probeBehavior === 'found' ? { chatId: state.orphanedStreamChatId } : {}),
9398
}),
9499
{ status: 200, headers: { 'Content-Type': 'application/json' } }
95100
)
@@ -205,7 +210,7 @@ describe('useChat remount send recovery', () => {
205210
vi.stubGlobal('fetch', fetchStub)
206211
state.postBehavior = 'hang'
207212
state.postCalls = 0
208-
state.orphanedStreamChatId = null
213+
state.probeBehavior = 'gone'
209214
state.streamProbes = 0
210215
mockRequestJson.mockResolvedValue({ chats: [] })
211216
useMothershipQueueStore.setState({ queues: {}, editing: {} })
@@ -334,7 +339,7 @@ describe('useChat remount send recovery', () => {
334339
describe('recovered send probes the orphaned stream before re-sending', () => {
335340
it('adopts the chat the server already created instead of sending twice', async () => {
336341
// The server accepted the withdrawn request and registered its stream.
337-
state.orphanedStreamChatId = 'chat-server-already-made'
342+
state.probeBehavior = 'found'
338343

339344
const { getResult, unmount } = renderUseChat()
340345
await act(async () => {
@@ -361,9 +366,85 @@ describe('useChat remount send recovery', () => {
361366
expect(allQueuedMessages()).toHaveLength(0)
362367
})
363368

369+
/**
370+
* Adoption alone does not surface the running turn: hydration reconnects
371+
* only when `chatHistory.activeStreamId` is set, and that query is cached
372+
* for 30s. Without an explicit detail invalidation the adopted chat renders
373+
* with the live response invisible.
374+
*/
375+
it('invalidates the adopted chat detail so hydration can reconnect', async () => {
376+
state.probeBehavior = 'found'
377+
378+
const { getResult, unmount } = renderUseChat()
379+
await act(async () => {
380+
void getResult().sendMessage('surface the running turn')
381+
})
382+
await waitFor(() => state.postCalls === 1)
383+
unmount()
384+
await waitFor(() => window.localStorage.getItem('sim_mothership_handoff') !== null)
385+
386+
const handoff = MothershipHandoffStorage.consume('ws-1')
387+
const replacement = renderUseChat()
388+
const invalidateSpy = vi.spyOn(queryClient, 'invalidateQueries')
389+
390+
await act(async () => {
391+
void replacement.getResult().sendMessage(handoff?.message as string, undefined, undefined, {
392+
recoverStreamId: handoff?.recoverStreamId as string,
393+
})
394+
})
395+
await waitFor(() =>
396+
invalidateSpy.mock.calls.some(([arg]) => {
397+
const key = (arg as { queryKey?: unknown[] } | undefined)?.queryKey
398+
return Array.isArray(key) && key.includes('chat-server-already-made')
399+
})
400+
)
401+
402+
expect(state.postCalls).toBe(1)
403+
})
404+
405+
/**
406+
* A probe cut short by unmount answers "unknown", not "safe to send".
407+
* Falling through to `startSendMessage` there would open a POST whose
408+
* abort controller the teardown already dropped — an uncancellable request
409+
* that duplicates the send. The entry must stay queued instead.
410+
*/
411+
it('does not send when the probe is cut short by unmount', async () => {
412+
state.probeBehavior = 'gone'
413+
const { getResult, unmount } = renderUseChat()
414+
await act(async () => {
415+
void getResult().sendMessage('do not zombie me')
416+
})
417+
await waitFor(() => state.postCalls === 1)
418+
unmount()
419+
await waitFor(() => window.localStorage.getItem('sim_mothership_handoff') !== null)
420+
421+
const handoff = MothershipHandoffStorage.consume('ws-1')
422+
423+
/* `pending` keeps the probe polling instead of answering on the first
424+
attempt, which is what leaves one in flight to interrupt. A `gone`
425+
probe answers immediately and the re-send would already have happened
426+
before the unmount — that version of this test cannot fail. */
427+
state.probeBehavior = 'pending'
428+
const replacement = renderUseChat()
429+
await act(async () => {
430+
void replacement.getResult().sendMessage(handoff?.message as string, undefined, undefined, {
431+
recoverStreamId: handoff?.recoverStreamId as string,
432+
})
433+
})
434+
await waitFor(() => state.streamProbes > 0)
435+
const postsBeforeUnmount = state.postCalls
436+
replacement.unmount()
437+
438+
// Well past the probe's poll budget: nothing may send after teardown.
439+
await act(async () => {
440+
await sleep(3000)
441+
})
442+
expect(state.postCalls).toBe(postsBeforeUnmount)
443+
})
444+
364445
it('re-sends when the server has no stream for it', async () => {
365446
// 404 from the probe: the request really was withdrawn before acceptance.
366-
state.orphanedStreamChatId = null
447+
state.probeBehavior = 'gone'
367448

368449
const { getResult, unmount } = renderUseChat()
369450
await act(async () => {

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

Lines changed: 47 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,17 @@ export interface SendMessageOptions {
153153
*/
154154
type StartSendMessageResult = boolean | { kind: 'recoverable_cleanup_abort'; streamId: string }
155155

156+
/**
157+
* Outcome of asking the server whether it accepted the request a cleanup abort
158+
* withdrew. `superseded` is deliberately distinct from `not_found`: the former
159+
* means the answer is unknown because this dispatch is no longer current, and
160+
* must not be treated as licence to re-send.
161+
*/
162+
type RecoveredSendProbe =
163+
| { status: 'adopted'; chatId: string }
164+
| { status: 'not_found' }
165+
| { status: 'superseded' }
166+
156167
export interface UseChatReturn {
157168
messages: ChatMessage[]
158169
isSending: boolean
@@ -4513,22 +4524,28 @@ export function useChat(
45134524
* the caller re-sends.
45144525
*/
45154526
const resolveRecoveredSendChatId = useCallback(
4516-
async (streamId: string, epoch: number): Promise<string | undefined> => {
4527+
async (streamId: string, epoch: number): Promise<RecoveredSendProbe> => {
45174528
const deadline = Date.now() + RECOVERED_SEND_PROBE_TIMEOUT_MS
45184529
while (true) {
45194530
const resolve = resolveDetachedChatForStreamRef.current
4520-
if (!resolve) return undefined
4521-
// Stop as soon as this dispatch is superseded (chat switch, unmount).
4522-
// The caller adopts what this returns, which rewrites the URL, and
4523-
// doing that after the user moved on would hijack their navigation.
4524-
if (epoch !== queueDispatchEpochRef.current) return undefined
4531+
if (!resolve) return { status: 'not_found' }
4532+
/* "Superseded" is NOT the same answer as "the server has no such
4533+
stream", and the caller must not conflate them: adopting rewrites
4534+
the URL, and re-sending after an unmount would open a POST whose
4535+
abort controller the teardown has already dropped — a zombie request
4536+
nothing can cancel, duplicating the very send this recovery exists
4537+
to protect. Reported distinctly so the caller leaves the entry
4538+
queued for a later mount instead. */
4539+
if (epoch !== queueDispatchEpochRef.current) return { status: 'superseded' }
45254540
const resolution = await resolve(streamId)
4526-
if (epoch !== queueDispatchEpochRef.current) return undefined
4527-
if (resolution.chatId) return resolution.chatId
4541+
if (epoch !== queueDispatchEpochRef.current) return { status: 'superseded' }
4542+
if (resolution.chatId) return { status: 'adopted', chatId: resolution.chatId }
45284543
// A terminal status means the stream existed and finished without a
45294544
// durable owner; polling cannot improve on that.
4530-
if (resolution.terminal) return undefined
4531-
if (Date.now() + RECOVERED_SEND_PROBE_INTERVAL_MS >= deadline) return undefined
4545+
if (resolution.terminal) return { status: 'not_found' }
4546+
if (Date.now() + RECOVERED_SEND_PROBE_INTERVAL_MS >= deadline) {
4547+
return { status: 'not_found' }
4548+
}
45324549
await sleep(RECOVERED_SEND_PROBE_INTERVAL_MS)
45334550
}
45344551
},
@@ -4640,16 +4657,23 @@ export function useChat(
46404657
the chat and the billed run. Probe the orphaned stream first and
46414658
adopt its chat instead when it exists. */
46424659
if (liveMsg.recoverStreamId) {
4643-
const adoptedChatId = await resolveRecoveredSendChatId(
4644-
liveMsg.recoverStreamId,
4645-
options.epoch
4646-
)
4647-
if (adoptedChatId) {
4660+
const probe = await resolveRecoveredSendChatId(liveMsg.recoverStreamId, options.epoch)
4661+
/* Unknown, not "safe to send" — leave the entry queued (it keeps its
4662+
`recoverStreamId`) so the next mount's drain probes again. */
4663+
if (probe.status === 'superseded') return
4664+
if (probe.status === 'adopted') {
46484665
removeQueuedMessage()
4649-
adoptResolvedChatId(adoptedChatId, {
4666+
adoptResolvedChatId(probe.chatId, {
46504667
replaceHomeHistory: true,
46514668
invalidateList: true,
46524669
})
4670+
/* Adoption alone does not surface the running turn. Hydration only
4671+
reconnects when `chatHistory.activeStreamId` is set, and that
4672+
query is cached for `MOTHERSHIP_CHAT_HISTORY_STALE_TIME` — on a
4673+
chat-bound recover the client usually holds a copy predating this
4674+
stream, so without an explicit detail invalidation the adopted
4675+
chat renders with the live response invisible. */
4676+
invalidateChatQueries({ includeDetail: true, targetChatId: probe.chatId })
46534677
return
46544678
}
46554679
}
@@ -4677,7 +4701,13 @@ export function useChat(
46774701
userRemovedDuringDispatchRef.current.delete(msg.id)
46784702
}
46794703
},
4680-
[startSendMessage, workspaceId, resolveRecoveredSendChatId, adoptResolvedChatId]
4704+
[
4705+
startSendMessage,
4706+
workspaceId,
4707+
resolveRecoveredSendChatId,
4708+
adoptResolvedChatId,
4709+
invalidateChatQueries,
4710+
]
46814711
)
46824712

46834713
const runQueueDispatchLoop = useCallback(async () => {

0 commit comments

Comments
 (0)