Uh oh!
There was an error while loading. Please reload this page.
fix(chat): deduplicate chat sends server-side instead of probing for them - #6536
Conversation
…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.
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview Server: Adds Client: Recovery plumbing now carries Tests updated/added for remount recovery, dedup reuse of ids, 409 adoption, and server claim lifecycle. Reviewed by Cursor Bugbot for commit 9ab60fa. Configure here. |
Greptile SummaryThis follow-up replaces client-side probing of withdrawn chat sends with server-side idempotency keyed by the original user-message ID.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/copilot/chat/post.ts | Adds user-scoped chat-send claiming, duplicate responses, chat-result recording, and lifecycle-aware release behavior; the previously reported lifecycle defects are addressed or explicitly accepted. |
| apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts | Replaces orphan-stream probing with message-ID reuse and routes withdrawn sends through chat-appropriate handoff or queue recovery. |
| apps/sim/lib/core/idempotency/service.ts | Introduces a Postgres-backed chat-send idempotency service with one-hour completed retention and a short in-progress lease. |
| apps/sim/lib/copilot/chat/post.test.ts | Adds server coverage for duplicate claims, user scoping, result recording, lifecycle release, and fail-open behavior. |
| apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.mount-send.test.tsx | Updates remount recovery tests to verify message-ID reuse, duplicate-chat adoption, and stable chat-bound queueing. |
Sequence Diagram
sequenceDiagram
participant UI as Chat UI
participant API as Chat POST
participant Idem as Postgres Idempotency
participant Turn as Chat Turn
UI->>API: Send(userMessageId)
API->>Idem: Atomically claim user-scoped ID
alt First attempt
Idem-->>API: Claimed
API->>API: Resolve chat and record chatId
API->>Turn: Persist message and start stream
API-->>UI: SSE response
else Duplicate attempt
Idem-->>API: Existing claim/result
API-->>UI: 409 with activeStreamId and chatId
UI->>Turn: Reconnect to original stream
end
Reviews (5): Last reviewed commit: "refactor(chat): give the send claim a si..." | Re-trigger Greptile
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…d 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.
waleedlatif1
commented
Aug 11, 2026
waleedlatif1
commented
Aug 11, 2026
@cursor review |
Uh oh!
There was an error while loading. Please reload this page.
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.
waleedlatif1
commented
Aug 11, 2026
waleedlatif1
commented
Aug 11, 2026
@cursor review |
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`.
waleedlatif1
commented
Aug 11, 2026
waleedlatif1
commented
Aug 11, 2026
@cursor review |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
waleedlatif1
commented
Aug 11, 2026
waleedlatif1
commented
Aug 11, 2026
@cursor review |
There was a problem hiding this comment.
✅ 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 9ab60fa. Configure here.
There was a problem hiding this comment.
✅ 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 9ab60fa. Configure here.
Summary
Follow-up to #6525, which fixed a real bug — a send withdrawn by an unmount cleanup was silently lost — but paid for it with a client-side probe. This replaces that probe with the deduplication mechanism the codebase already owns, and deletes more than it adds.
The problem #6525 had to solve. A client cannot tell whether a request it aborted reached the server.
lib/copilot/chat/post.tsnever readsrequest.signal, so a request the server accepted still runsresolveOrCreateChat,persistUserMessage, and the billed turn to completion after the browser drops the socket (Next.js discussion). Retrying blind meant two chats and two billed runs; not retrying meant losing the message.How #6525 answered it. It polled the orphaned stream for up to 2.5s before deciding. That number was a guess with no production timing behind it, the poll had to distinguish "the server has no such stream" from "we stopped looking" (conflating them re-sent after teardown, opening an uncancellable POST), and it still left a window open for a server slower than the ceiling.
How this answers it.
IdempotencyServicealready backs webhook, polling, and billing deduplication here, andbillingIdempotencyexists for exactly this hazard — its own comment says a retry "would double-record usage — real money". Chat sends now claim through it, keyed on the client-generateduserMessageId. Retrying is just reusing the id.Server
chatSendIdempotencyclaims each send. The key is scoped to the authenticated user —userMessageIdis client-supplied, so an unscoped key would let one user probe another's sends and read back their chat idbillingIdempotency: a missed dedup is a second billed turn, so the key must survive Redis memory pressure. ~1-5ms is invisible next to the LLM call that followsIt fails open at every step. Deduplication saves a duplicate chat; the send is the user's message. An unreachable bookkeeping store must degrade chat, never take it down — an early draft failed closed and turned every send into a 409, which is exactly the outcome to avoid in production.
Client
Deleted with the probe: 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 required. The hook nets 67 lines smaller.
resumeUserMessageIdthrough the same lanes fix(chat): stop losing sends aborted during mount-settling #6525 built (live event, stored handoff, queue entry) — same plumbing, a payload that is consumed synchronously instead of awaitedstartSendMessagedirectly.fix(chat): stop losing sends aborted during mount-settling #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 a rare path. Recovery never needed it — message, attachments, contexts, and id are all in scope at the abort. Both callers now share onehandOffWithdrawnSenddispatchQueuedMessage's "re-read live" and the send, so an edit during that window sent pre-edit textstartSendMessagetakes its optional tail as an options object; it was at six positional parameters and the retry id would have been a seventhTesting
Server: deduplicates a repeat without opening a second chat or stream, scopes the key per user, records the chat against the send, and still sends when the claim store is down. Client: reuses the original id on retry, and adopts the chat a deduplicated retry names. Plus the coverage #6525 earned — StrictMode double-mount, live-surface delivery, stored-handoff fallback, and the departing surface not claiming its own recovery event.
Each new test was confirmed red without its fix. 2058 tests,
type-check, biome, andcheck:api-validationpass.Known residuals
All three are deliberate, and each is better than what
mainorstagingdoes in the same scenario.Losing the claim store loses deduplication for that window — the fail-open trade above. The dedup window is 1 hour;
userMessageIdis never reused, so a longer one would be harmless.A concurrent retry can be told to reattach to a turn that never starts. A retry arriving between
storeResultand the turn starting gets 409 and reattaches; if the original then also fails, there is nothing to attach to. The client keeps the optimistic pair (the generic 409 branch does not roll back) and finalizes with an error, so the user sees their message with a failed response rather than losing it — and the original'sfinallyreleases the claim, so sending again starts a fresh turn.mainloses the message with no error;stagingre-sends and can duplicate the turn.A failed
storeResultdegrades to no deduplication after 60s. The claim staysin-progress, whichatomicallyClaimDblets a retry reclaim once the lease expires, so a retry more than 60s later can start a second billed turn. Every alternative is worse: releasing on record failure duplicates immediately, and extending the lease to cover a turn (maxDurationis 3600s) lets a crashed pod poison the id for an hour. Leaving it in-progress still deduplicates for the first 60s, which is where recovery retries actually land — they fire from the unmount cleanup within milliseconds. Requires a Postgres write failure to reach, and logsCould not record the chat for this sendwhen it happens.Claim lifecycle
The load-bearing invariant, since it is easy to get wrong: the claim is permanent only once a turn is streaming. One line clears it, immediately before the stream response;
finallyreleases it on every other exit.Type of Change
Checklist