Skip to content

Commit 1551923

Browse files
j15zwaleedlatif1
j15z
and
waleedlatif1
authored
fix(chat): stop losing sends aborted during mount-settling (#6525)
* fix(chat): stop losing sends aborted during mount-settling * fix(chat): detect aborts by signal state, not error identity fetch rejects with the RAW abort reason when its signal carries one — abort('unmount:client_cleanup') surfaces as a plain string, so every err.name === 'AbortError' check missed it and the restore path never ran (verified live). The test stub now rejects with the raw reason like real fetch, which turns this gap red. * fix(chat): hand an aborted chatless send to the next mount The mount-settling cycle is a full remount — the pending chat key is regenerated per instance, so restoring the aborted send into the dead instance's queue orphaned it (verified live). A chatless send now re-persists as a one-shot MothershipHandoffStorage handoff the next mount's consumer re-sends; chat-bound sends keep the queue restore. * fix(chat): deliver an aborted chatless send to the live replacement surface The settling remount's consumer checks handoff storage before the restore microtask re-persists it, so the stored handoff sat unread until a navigation. The replacement surface's send listener IS registered by restore time — deliver the message directly through the claimable send event, keeping the stored handoff as the no-surface fallback. * refactor(chat): thread the recoverable-abort outcome through the send result Replaces the restorableCleanupAbortRef reset choreography with a widened startSendMessage return ('recoverable_cleanup_abort'), so the restore decision is ordinary data flow and the second caller cannot leave a stale flag behind. * fix(chat): carry attachments through the cross-mount send handoff The recoverable-abort delivery excluded attachment-bearing sends, so they restored under the dead instance's pending key and were silently lost. The claimable send event now carries fileAttachments end to end (dispatcher, home listener, restore path); only the storage fallback — whose shape cannot hold attachments — still queue-restores them. * fix(panel): forward event attachments to the copilot send * fix(chat): carry attachments through the stored handoff lane too The unclaimed-event fallback excluded attachment sends and restored them under the disposed mount's pending key. The persisted handoff now carries fileAttachments (they are plain references to already-uploaded files), the home consumer forwards them, and the recovery branch always hands off — no stranded lane remains. * 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. * 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. * test(chat): cover the departing surface's own recovery-event claim Greptile flagged that a surface being torn down could claim the recovery event its own cleanup emits — which would return `true`, suppress the storage fallback, and strand the message under a disposed pending key. It cannot: React removes the listener during the same synchronous unmount commit, while the recovery runs from the fetch rejection a microtask later, so by then nothing of the departing surface is listening. That ordering was previously only argued, never asserted — the suite unmounted a bare hook with no listener attached. This mounts a home.tsx-shaped surface that both drives useChat and registers the claiming listener, and asserts the departing listener claims zero times while the handoff still reaches storage. Confirmed meaningful: neutering the listener's removeEventListener cleanup so it survives teardown makes it claim, and the test fails. * fix(chat): hand off a chatless send when the probe is superseded The previous commit made a superseded probe leave the entry queued rather than re-send it. That is the right retry for a chat-bound key, which is the stable chat id, but wrong for a chatless one: a `pending::` key is regenerated every mount, so anything left under it is unreachable and the message is stranded — the same loss this PR exists to prevent, just reached by a different route. A superseded probe on a pending key now goes through the same recovery lanes as the cleanup-abort path (live replacement surface, else a one-shot stored handoff), still carrying the stream id so the next surface probes before it sends. Skipped when the entry is no longer under that key, since adoption migrating it to a live chat already leaves it recoverable there. The lane is extracted so both call sites share one implementation. The existing superseded test only asserted that nothing sent, which this bug satisfied trivially; it now also asserts the message survives. Confirmed red without the fix. --------- Co-authored-by: Waleed Latif <walif6@gmail.com>
1 parent a64ce49 commit 1551923

8 files changed

Lines changed: 866 additions & 22 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, undefined, 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, undefined, 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 ?? []

0 commit comments

Comments
 (0)