fix(chat): stop losing sends aborted during mount-settling by j15z · Pull Request #6525 · simstudioai/sim

j15z · 2026-08-11T03:13:24Z

Summary

Sends started on a fresh chat surface were silently dropped when the hook's unmount cleanup ran mid-flight and aborted the in-flight POST. The AbortError read as a user stop, the optimistic pair rolled back, and — because MothershipHandoffStorage.consume() clears atomically — the replacement mount found nothing left to retry. This is what made cross-route auto-send handoffs (cmd+k Ask Sim, logs "Troubleshoot in Chat") lose messages.

Root cause (corrected). An earlier revision of this PR attributed the remount to "a Suspense hide/reveal cycling every effect." That is not what happens: React 19 disappears layout effects only when a mounted Suspense boundary re-suspends, and the cleanup in question (use-chat.ts, the abort('unmount:client_cleanup') teardown) is a passive effect, so a Suspense hide/reveal never runs it. Verified directly against react-dom@19.2.4:

mount → layout:setup, passive:setup
re-suspend → layout:cleanup ← passive:cleanup never fires
reveal → layout:setup

What does run it mid-flight is a genuine remount: StrictMode's dev double-mount (deterministic — it fires on every cross-route handoff in dev) and a real client-side navigation away while the send is still in flight. The cleanup's deps are all stable useCallbacks, so nothing else triggers it.

Changes

Idle-path sends go through the durable message queue instead of calling startSendMessage directly, so every send has a backing entry the dispatch loop can recover
Aborts are detected by signal state, not error identity. Every abort() here passes a string reason, and fetch rejects with the raw reason — so err.name === 'AbortError' was false for all of them, and aborts (including user "stop") were falling through to the generic handler and surfacing Failed to send message. Fixing this is an independent correctness win
A cleanup abort that strikes before the response headers arrive rolls back the optimistic pair and hands the message to recovery: the live replacement surface via the mothership-send-message event, or a one-shot stored handoff, or the restored queue entry for a chat-bound send. Attachments ride all three lanes
Recovery probes the orphaned stream before re-sending.sendReachedServer only rules out a send whose response landed; the request itself may well have been accepted. app/api/mothership/chat/route.ts → lib/copilot/chat/post.ts never reads request.signal, so an accepted request still runs resolveOrCreateChat, persistUserMessage, and the billed turn to completion even though the client socket is gone. Re-sending blind would leave the user with two chats and two billed runs for one message. Recovery therefore carries the withdrawn send's userMessageId as a stream id and polls it (2.5s ceiling): resolving to a chat means the server already has the message, so that chat is adopted instead of sending again. Only a stream the server has no record of (404 — genuinely never accepted) re-sends; a timeout also re-sends, which is the safe direction

Type of Change

Bug fix

Testing

use-chat.mount-send mounts the real hook and runs the real cleanup mid-flight. Each of these fails on the unfixed hook:

a cross-route handoff stays recoverable across a StrictMode double-mount (the production-shaped repro, driven by the thing that actually causes it rather than a hand-rolled unmount)
an aborted chatless send is delivered to a live replacement surface, with attachments
an aborted chatless send is re-persisted as a handoff when nothing claims the event
a send the server already received is not re-queued
recovery adopts the chat the server already created instead of sending twice (postCalls stays 1)
recovery does re-send when the probe 404s

Full home/hooks + stores/mothership-queue + lib/core/utils suites (364 tests), type-check, biome, and check:api-validation all pass.

Known residual

The probe narrows the duplicate window but does not close it: a server slower than the 2.5s ceiling still gets a second send. Closing it completely needs server-side idempotency on userMessageId in resolveOrCreateChat, which is a separate change with its own blast radius.

Checklist

Code follows project style guidelines
Self-reviewed my changes
Tests added/updated and passing
No new warnings introduced
I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

vercel · 2026-08-11T03:13:28Z

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment

Project	Deployment	Actions	Updated (UTC)
docs	Skipped		Aug 11, 2026 6:51am

cursor · 2026-08-11T03:13:36Z

PR Summary

Medium Risk
Touches the core mothership send/dispatch path and recovery timing; wrong probe or handoff logic could duplicate billed turns or still drop messages, though the new mount-send tests cover the main failure modes.

Overview
Fixes silent loss of chat sends when useChat unmount cleanup aborts an in-flight POST (e.g. StrictMode double-mount or navigation) before response headers arrive—especially cross-route handoffs that auto-send on home.

Recovery path: Idle sends now enqueue through the durable queue so dispatch can restore or hand off. Cleanup aborts before the server response are detected via the abort signal (not only AbortError, since fetch can reject with a string reason). The optimistic UI is rolled back and the message is recovered via the live mothership-send-message event, MothershipHandoffStorage, or queue restore for chat-bound keys.

Duplicate prevention: Recovery carries recoverStreamId (the aborted send’s userMessageId). Before re-posting, the client polls the orphaned stream (~2.5s); if the server already accepted the request, it adopts that chat and invalidates detail for hydration instead of sending again.

home.tsx, workflow panel copilot, handoff storage, and sendMothershipMessage now forward file attachments and recoverStreamId. Queue edits strip stale recoverStreamId so edited text is not wrongly deduped.

Adds regression tests in use-chat.mount-send.test.tsx for StrictMode handoff, handoff persistence, self-claim avoidance, probe adopt/re-send, and probe interrupted by unmount.

^{Reviewed by Cursor Bugbot for commit 45013a4. Configure here.}

greptile-apps · 2026-08-11T03:16:05Z

Greptile Summary

The PR makes fresh-chat sends recoverable across genuine remounts by routing them through the queue, carrying attachments through event and storage handoffs, and probing an orphaned stream before retrying.

Adds queue-backed send recovery and chat adoption for cleanup-aborted requests.
Preserves attachments and recovery stream IDs across live events and persisted handoffs.
Adds mount, StrictMode, attachment, adoption, and interrupted-probe regression coverage.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts	Routes idle sends through the durable queue and adds cleanup-abort recovery, stream probing, chat adoption, and safe restoration behavior.
apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.mount-send.test.tsx	Adds focused regression coverage for StrictMode remounts, attachment handoffs, stream adoption, and interrupted recovery probes.
apps/sim/lib/core/utils/browser-storage.ts	Extends persisted handoffs to retain JSON-safe attachment references and recovery stream IDs.
apps/sim/lib/mothership/events.ts	Extends claimable send events to carry attachments and recovery stream IDs.
apps/sim/app/workspace/[workspaceId]/home/home.tsx	Forwards recovered attachments and stream IDs from both live events and stored handoffs.
apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/panel.tsx	Forwards attachment and recovery metadata when the panel claims a send event.
apps/sim/stores/mothership-queue/store.ts	Clears stale recovery identity when a queued message is edited so the modified text is sent normally.
apps/sim/stores/mothership-queue/types.ts	Adds persisted recovery-stream metadata to queued messages.

Sequence Diagram

sequenceDiagram
participant Surface as Chat surface
participant Queue as Durable queue
participant API as Chat API
participant Probe as Stream probe
participant Next as Replacement surface
Surface->>Queue: Enqueue message
Queue->>API: POST chat request
Surface--xAPI: Cleanup aborts client request
Queue->>Next: Event or persisted handoff
Next->>Probe: Probe original userMessageId
alt Existing stream has a chat
Probe-->>Next: chatId
Next->>Next: Adopt chat and invalidate detail
else Stream not found or probe times out
Probe-->>Next: not_found
Next->>API: Retry queued send
else Replacement unmounts during probe
Probe-->>Next: superseded
Next->>Queue: Preserve recovery for later mount
end

_{Reviews (11): Last reviewed commit: "fix(chat): hand off a chatless send when..." | Re-trigger Greptile}

j15z · 2026-08-11T03:21:23Z

@greptile

j15z · 2026-08-11T03:21:24Z

@cursor review

cursor

✅ Bugbot reviewed your changes and found no new issues!

2 issues from previous reviews remain unresolved.

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 04147f7. Configure here.}

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.

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.

…urface 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.

j15z · 2026-08-11T03:40:06Z

@greptile

j15z · 2026-08-11T03:40:07Z

@cursor review

cursor

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit ed4ea7c. Configure here.}

… 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.

j15z · 2026-08-11T04:03:04Z

@greptile

j15z · 2026-08-11T04:03:05Z

@cursor review

cursor

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit d078e82. Configure here.}

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.

j15z · 2026-08-11T05:36:04Z

@greptile

j15z · 2026-08-11T05:36:05Z

@cursor review

j15z · 2026-08-11T05:50:54Z

@greptile

j15z · 2026-08-11T05:50:55Z

@cursor review

cursor

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 3e67769. Configure here.}

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.

waleedlatif1 · 2026-08-11T06:22:27Z

@greptile

waleedlatif1 · 2026-08-11T06:22:29Z

@cursor review

…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.

waleedlatif1 · 2026-08-11T06:36:26Z

@greptile

waleedlatif1 · 2026-08-11T06:36:27Z

@cursor review

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.

waleedlatif1 · 2026-08-11T06:43:59Z

@greptile

waleedlatif1 · 2026-08-11T06:44:00Z

@cursor review

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.

waleedlatif1 · 2026-08-11T06:51:23Z

@greptile

waleedlatif1 · 2026-08-11T06:51:24Z

@cursor review

cursor

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 45013a4. Configure here.}

…them (#6536) * 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. * fix(chat): keep a withdrawn send in its own chat, and release stranded claims Audit follow-ups, two of them real defects in the previous commit. A withdrawn send routed unconditionally through the cross-surface lanes. Those deliver to whatever chat is mounted next, so sending in one chat and switching to another re-sent the message into the second one. The dispatcher already drew the distinction; the idle path now draws it too — a chat-bound key is the stable chat id, so re-queueing under it both retries durably and keeps the message where the user put it. Only a chatless key, which dies with its mount, goes to the lanes. The claim release sat in `catch`, so the two paths that return a response without throwing — a rejected branch, and a missing chat — stranded an in-progress claim for its full 60s TTL, and a retry inside that window got a spurious "already sent" instead of the real error. Moved to `finally`. Also: `userMessageId` is now length-bounded, since it becomes part of a Postgres key and an oversized one would throw inside the claim; `requestId` was still empty at claim time, so both dedup logs printed a blank prefix; the provider segment said `mothership` on a handler that also serves the workflow copilot, and now says what the key identifies; `retryFailures` was dead config, only read by `executeWithIdempotency`, which this caller never invokes; the doc pointed at `billingIdempotency`, which has no consumers, and now points at the live Stripe analogue. Trimmed: `sendClaimRecorded` folded into clearing `sendClaim`, the unread `kind` discriminant dropped from a one-arm union, the single-use `claimedChatId` inlined, and the prose on all three of those cut back to what the code does not already say. * 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. * refactor(chat): give the send claim a single point of permanence Recording the chat also dropped the claim when it failed, which left a second way for a claim to stop being tracked and a compound hole behind it: a failed record followed by a throw stranded the claim for its in-progress TTL, and a retry inside that window reattached to a turn that never started. Only one line now decides permanence — the claim is cleared immediately before the stream response — so `finally` releases it on every exit that did not start a turn, including a failed record. The `recorded` flag is gone with it. Covers the 400 early return with a release assertion: that path returns without throwing, so it is the one that proves the release has to live in `finally`.

fix(chat): stop losing sends aborted during mount-settling
04147f7

vercelBottemporarily deployed to Preview August 11, 2026 03:13 Inactive

greptile-appsBot reviewed Aug 11, 2026
View reviewed changes

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