mirror of
https://github.com/simstudioai/sim.git
synced 2026-09-24 15:45:35 +08:00
Waleed
f8644cc679
* 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`.