Skip to content

Commit ed03246

Browse files
committed
fix(mothership): guard a Send-now restored from its stored handoff against edits
A Send-now whose Stop settled has its POST out under the id its stored handoff carries. Restored after a reload it had no resumeUserMessageId, so the queue guard missed it, and an edit could run as a second turn. The guard now treats a settled handoff's id as an earlier attempt. A direct send can't reach the never-sent branch (a pending Stop queues it), so that restore now treats anything but a refusal as unknown. Docs: the 1-hour claim TTL, the claim failing closed, and the superseded admission conflict classed as busy.
1 parent 2e7082f commit ed03246

5 files changed

Lines changed: 121 additions & 17 deletions

File tree

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

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2329,6 +2329,70 @@ describe('useChat remount send recovery', () => {
23292329
expect(edited).toBeUndefined()
23302330
})
23312331

2332+
/**
2333+
* A Send-now whose Stop settled has its POST out under the id its stored
2334+
* handoff carries. Reloaded before the answer, the handoff comes back into the
2335+
* queue; the server may already have admitted that id, so the restored entry
2336+
* must not be editable into a second message.
2337+
*/
2338+
it.each([
2339+
{ stopRequired: false, editable: false },
2340+
{ stopRequired: true, editable: true },
2341+
])(
2342+
'guards a Send-now restored from its stored handoff (Stop still required: $stopRequired)',
2343+
async ({ stopRequired, editable }) => {
2344+
const history: MothershipChatHistory = {
2345+
id: 'chat-a',
2346+
mode: 'agent',
2347+
title: 'Restored handoff',
2348+
messages: [],
2349+
activeStreamId: null,
2350+
resources: [],
2351+
}
2352+
let loadHistory: (() => void) | undefined
2353+
mockRequestJson.mockImplementation(
2354+
() =>
2355+
new Promise((resolve) => {
2356+
loadHistory = () => resolve({ chat: history })
2357+
})
2358+
)
2359+
writeQueuedSendHandoffState({
2360+
id: 'restored-send-now',
2361+
chatId: 'chat-a',
2362+
workspaceId: 'ws-1',
2363+
supersededStreamId: 'previous-response',
2364+
userMessageId: 'send-now-request',
2365+
message: 'inspect the second invoice instead',
2366+
...(stopRequired ? { stopRequired: true } : {}),
2367+
requestedAt: Date.now(),
2368+
})
2369+
const { getResult } = renderUseChatInChat('chat-a')
2370+
/** The first moment a user could act on the restored entry, before dispatch. */
2371+
const editAtRestore: Array<ReturnType<ReturnType<typeof useChat>['editQueuedMessage']>> = []
2372+
let tried = false
2373+
const unsubscribe = useMothershipQueueStore.subscribe((queueState) => {
2374+
if (tried) return
2375+
if (!queueState.queues['chat-a']?.some((message) => message.id === 'restored-send-now'))
2376+
return
2377+
/** Set first: opening the entry for editing writes the store and re-enters here. */
2378+
tried = true
2379+
editAtRestore.push(getResult().editQueuedMessage('restored-send-now'))
2380+
})
2381+
try {
2382+
await waitFor(() => loadHistory !== undefined)
2383+
await act(async () => {
2384+
loadHistory?.()
2385+
await sleep(50)
2386+
})
2387+
await waitFor(() => editAtRestore.length === 1)
2388+
} finally {
2389+
unsubscribe()
2390+
}
2391+
2392+
expect(editAtRestore[0] !== undefined).toBe(editable)
2393+
}
2394+
)
2395+
23322396
/**
23332397
* A held message the server then refuses as busy is known not to be a turn
23342398
* there: the server answers a retry of an admitted id as a duplicate, never

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

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -236,8 +236,9 @@ interface WithdrawnSendResult {
236236
held?: boolean
237237
/**
238238
* The server refused this id outright (busy, or a predecessor still shutting
239-
* down). It answers a retry of an admitted id as a duplicate instead, so the
240-
* server is known not to have it, and its queue entry can be edited.
239+
* down). Within the 1-hour claim TTL it answers a retry of an admitted id as a
240+
* duplicate instead, so the server is known not to have it, and its queue
241+
* entry can be edited.
241242
*/
242243
notAdmitted?: boolean
243244
/**
@@ -4052,6 +4053,10 @@ export function useChat(
40524053
exact: true,
40534054
refetchType: 'none',
40544055
})
4056+
/* Admission's "superseded" conflict (another attempt re-took this id's
4057+
claim) also lands here with no stream named. That needs this attempt to
4058+
hold its claim past the 60s in-progress TTL before admitting, which a
4059+
lock wait of at most 5s does not reach. */
40554060
return { userMessageId, busy: true, notAdmitted: true }
40564061
}
40574062
/* "Already sent" with no stream for it means the earlier attempt is still
@@ -4426,11 +4431,9 @@ export function useChat(
44264431
: {}),
44274432
...(result.held ? { retryRequired: true } : {}),
44284433
...(result.busy ? busyRetry(1) : {}),
4429-
admissionUnknown: result.notAdmitted
4430-
? false
4431-
: result.neverSent
4432-
? options?.resumeUserMessageId !== undefined
4433-
: true,
4434+
/* Only a refusal of this id settles it. A direct send never waits on a Stop
4435+
(one pending queues it instead), so `neverSent` cannot occur here. */
4436+
admissionUnknown: !result.notAdmitted,
44344437
...((result.unreachable || result.busy) && activeChatKey.startsWith(PENDING_CHAT_KEY_PREFIX)
44354438
? { heldSurface: heldSendSurface }
44364439
: {}),

‎apps/sim/lib/mothership/chat/post.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -939,9 +939,9 @@ const CHAT_SEND_IDEMPOTENCY_PROVIDER = 'user-message'
939939
/**
940940
* Claims this send so a retry of it can be recognised.
941941
*
942-
* Fails open: a missed deduplication costs a duplicate chat and turn, but
943-
* refusing the send loses the user's message. Returns `undefined` when the
944-
* store is unreachable, which sends normally with no claim to finalize.
942+
* Fails closed: the claim is stored in Postgres (`chatSendIdempotency` forces
943+
* database storage), so a store failure throws and the send is answered with a
944+
* 500 rather than run without deduplication.
945945
*
946946
* The key is scoped to the caller — `userMessageId` is client-supplied, so an
947947
* unscoped one would let a user probe another's sends for their chat id.

‎apps/sim/stores/mothership-queue/store.test.ts‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,39 @@ describe('useMothershipQueueStore', () => {
3030
})
3131

3232
describe('replaceAt', () => {
33+
it('treats a Send-now whose Stop settled as possibly sent under its handoff id', () => {
34+
useMothershipQueueStore.getState().enqueue('chat-A', {
35+
id: 'sent',
36+
content: 'original',
37+
queuedSendHandoff: {
38+
id: 'sent',
39+
chatId: 'chat-A',
40+
supersededStreamId: 'previous-response',
41+
userMessageId: 'send-now-request',
42+
},
43+
})
44+
useMothershipQueueStore.getState().enqueue('chat-A', {
45+
id: 'waiting',
46+
content: 'original',
47+
queuedSendHandoff: {
48+
id: 'waiting',
49+
chatId: 'chat-A',
50+
supersededStreamId: 'previous-response',
51+
userMessageId: 'not-sent-yet',
52+
stopRequired: true,
53+
},
54+
})
55+
useMothershipQueueStore.getState().replaceAt('chat-A', 'sent', { content: 'edited' })
56+
useMothershipQueueStore.getState().replaceAt('chat-A', 'waiting', { content: 'edited' })
57+
58+
const [sent, waiting] = useMothershipQueueStore.getState().queues['chat-A'] ?? []
59+
expect(sent).toMatchObject({
60+
content: 'original',
61+
queuedSendHandoff: { userMessageId: 'send-now-request' },
62+
})
63+
expect(waiting?.content).toBe('edited')
64+
})
65+
3366
it('treats any message resuming an earlier attempt as possibly sent, unless told otherwise', () => {
3467
useMothershipQueueStore
3568
.getState()

‎apps/sim/stores/mothership-queue/store.ts‎

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -52,15 +52,19 @@ const initialState = {
5252
}
5353

5454
/**
55-
* A message resuming an earlier attempt (`resumeUserMessageId`) may already be
56-
* a turn on the server, unless the writer knows it is not
57-
* (`admissionUnknown: false`). Every queue write goes through this, so no path
58-
* can queue such a message as editable by leaving the flag out.
55+
* A message carrying an earlier attempt's id may already be a turn on the
56+
* server, unless the writer knows it is not (`admissionUnknown: false`). The id
57+
* rides as `resumeUserMessageId` (a withdrawn send) or as its Stop handoff's
58+
* `userMessageId` (a Send-now restored from its stored handoff). A handoff
59+
* still waiting on its Stop (`stopRequired`) never sent that id: the handoff is
60+
* rewritten without it just before the POST. Every queue write goes through
61+
* this, so no path can queue such a message as editable by leaving the flag out.
5962
*/
6063
function withAdmissionGuard(message: QueuedMothershipMessage): QueuedMothershipMessage {
61-
if (message.resumeUserMessageId === undefined || message.admissionUnknown !== undefined) {
62-
return message
63-
}
64+
const handoff = message.queuedSendHandoff
65+
const earlierAttempt =
66+
message.resumeUserMessageId ?? (handoff?.stopRequired ? undefined : handoff?.userMessageId)
67+
if (earlierAttempt === undefined || message.admissionUnknown !== undefined) return message
6468
return { ...message, admissionUnknown: true }
6569
}
6670

0 commit comments

Comments
 (0)