Skip to content

Commit 4425571

Browse files
committed
fix(mothership): Stop reaches every desktop tool of the view, and results say what is known
Desktop tools now take one lifetime the view owns and only the user's Stop ends, so Stop still reaches a tool that a replaced stream reader started. A held-call 409 is final on the trimmed retry of an oversized report too. The not-started result no longer asserts why nothing picked the call up, and a local read the desktop claimed is described as started when its result is lost.
1 parent 03d9ebd commit 4425571

7 files changed

Lines changed: 104 additions & 34 deletions

File tree

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

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2246,6 +2246,24 @@ describe('useChat remount send recovery', () => {
22462246
expect(toolSignal.aborted).toBe(false)
22472247
})
22482248

2249+
it('is still cancelled by Stop after the stream was recovered', async () => {
2250+
const { toolSignal, replays, getResult } = await startDesktopAction()
2251+
Object.defineProperty(document, 'visibilityState', {
2252+
configurable: true,
2253+
get: () => 'visible',
2254+
})
2255+
await act(async () => {
2256+
document.dispatchEvent(new Event('visibilitychange'))
2257+
})
2258+
await waitFor(() => replays.length > 0)
2259+
2260+
await act(async () => {
2261+
await getResult().stopGeneration()
2262+
})
2263+
2264+
expect(toolSignal.aborted).toBe(true)
2265+
})
2266+
22492267
it('is cancelled when the user stops the chat', async () => {
22502268
const { toolSignal, getResult } = await startDesktopAction()
22512269

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

Lines changed: 11 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -456,23 +456,6 @@ export async function waitForDetachedChatResolution(
456456
/** The abort reason of the user's Stop. */
457457
const USER_STOP_ABORT_REASON = 'user_stop:client_stopGeneration'
458458

459-
/**
460-
* The lifetime a desktop tool (a browser action, a local file read or import) started from one
461-
* stream observes: only the user's Stop cancels it. Replacing the stream reader (the window
462-
* returning to view, a history reconnect) or leaving the chat view leaves it running, so it
463-
* finishes and reports its own result.
464-
*/
465-
function desktopToolLifetime(streamSignal: AbortSignal | undefined): AbortSignal | undefined {
466-
if (!streamSignal) return undefined
467-
const lifetime = new AbortController()
468-
const followStop = () => {
469-
if (streamSignal.reason === USER_STOP_ABORT_REASON) lifetime.abort(USER_STOP_ABORT_REASON)
470-
}
471-
if (streamSignal.aborted) followStop()
472-
else streamSignal.addEventListener('abort', followStop, { once: true })
473-
return lifetime.signal
474-
}
475-
476459
/**
477460
* Runs a browser tool on the desktop client. The agent's tab reaches the
478461
* resource strip through the desktop tab list, so nothing is opened here.
@@ -942,6 +925,13 @@ export function useChat(
942925
const reconnectExhaustedRecheckTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null)
943926

944927
const abortControllerRef = useRef<AbortController | null>(null)
928+
/**
929+
* The lifetime of the desktop tools (browser actions, local file reads and imports) this view
930+
* starts: only the user's Stop ends it. It belongs to no stream reader, so replacing the reader
931+
* (the window returning to view, a history reconnect) or leaving the chat view leaves a running
932+
* tool alone to finish and report, and a Stop still reaches tools a replaced reader started.
933+
*/
934+
const desktopToolStopRef = useRef<AbortController | null>(null)
945935
const detachedChatResolutionControllersRef = useRef<Set<AbortController> | null>(null)
946936
const detachedChatResolutionControllers = (detachedChatResolutionControllersRef.current ??=
947937
new Set())
@@ -1596,7 +1586,7 @@ export function useChat(
15961586
const options = {
15971587
workspaceId,
15981588
chatId: chatIdRef.current ?? selectedChatIdRef.current,
1599-
signal: desktopToolLifetime(abortControllerRef.current?.signal),
1589+
signal: (desktopToolStopRef.current ??= new AbortController()).signal,
16001590
}
16011591
/**
16021592
* Dynamic on purpose: the local-filesystem executor only runs for desktop-local
@@ -2172,7 +2162,7 @@ export function useChat(
21722162
shouldContinue?: () => boolean
21732163
}
21742164
) => {
2175-
const browserToolSignal = desktopToolLifetime(abortControllerRef.current?.signal)
2165+
const browserToolSignal = (desktopToolStopRef.current ??= new AbortController()).signal
21762166
const activityTracker = getResourceActivityTracker(
21772167
expectedGen ?? streamGenRef.current,
21782168
options?.targetChatId
@@ -4444,6 +4434,8 @@ export function useChat(
44444434
)
44454435
}
44464436
clearResourceActivity(stopActivityTracker, true)
4437+
desktopToolStopRef.current?.abort(USER_STOP_ABORT_REASON)
4438+
desktopToolStopRef.current = null
44474439

44484440
// Establish the stream boundary immediately after synchronous activity
44494441
// settlement. Native cancellation above is deliberately fire-and-forget,

‎apps/sim/lib/mothership/request/tools/desktop-wait.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import type { ResolvedSecretTraceRegistry } from '@/executor/utils/resolved-secr
1111
const logger = createLogger('CopilotDesktopToolWait')
1212

1313
const DESKTOP_TOOL_NOT_STARTED_MESSAGE =
14-
'Not run: this action never started, because nothing in the Sim desktop app picked it up (this chat is not open there). Nothing happened on the user’s computer. Do not retry it in this turn; tell the user to keep this chat open in the Sim desktop app, or to ask again later.'
14+
'Not run: this action never started, because nothing in the Sim desktop app picked it up. Nothing happened on the user’s computer. Do not retry it in this turn; tell the user to keep this chat open in the Sim desktop app, or to ask again later.'
1515

1616
/** The model-facing result of a desktop call that was never picked up. */
1717
export function desktopToolNotStarted(): { message: string; data: Record<string, unknown> } {

‎apps/sim/lib/mothership/request/tools/executor.test.ts‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -940,11 +940,37 @@ describe('watchdog completion provenance', () => {
940940
})
941941
})
942942

943+
it('says a local read the desktop claimed started before its result was lost', async () => {
944+
const { toolCall, context, execContext } = createHungClient()
945+
toolCall.name = 'read_local_file'
946+
toolCall.params = { path: '/Users/me/notes.txt' }
947+
completePendingAsyncToolCall.mockResolvedValueOnce(null)
948+
mothershipAsyncRunsMockFns.mockGetAsyncToolCall.mockResolvedValueOnce({
949+
toolCallId: toolCall.id,
950+
claimedBy: 'desktop-files',
951+
})
952+
953+
await failPendingToolCall(toolCall.id, context, execContext)
954+
955+
expect(toolCall.result).toEqual({
956+
success: false,
957+
output: {
958+
error: expect.stringContaining('desktop app started this action'),
959+
outcomeUnknown: true,
960+
doNotRetry: true,
961+
},
962+
})
963+
})
964+
943965
it('does not claim a local read the server never saw picked up had started', async () => {
944966
const { toolCall, context, execContext } = createHungClient()
945967
toolCall.name = 'read_local_file'
946968
toolCall.params = { path: '/Users/me/notes.txt' }
947969
completePendingAsyncToolCall.mockResolvedValueOnce(null)
970+
mothershipAsyncRunsMockFns.mockGetAsyncToolCall.mockResolvedValueOnce({
971+
toolCallId: toolCall.id,
972+
claimedBy: null,
973+
})
948974

949975
await failPendingToolCall(toolCall.id, context, execContext)
950976

‎apps/sim/lib/mothership/request/tools/executor.ts‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import type {
1010
} from '@/lib/mothership/async-runs/lifecycle'
1111
import {
1212
type CompleteAsyncToolCallInput,
13+
getAsyncToolCall,
1314
markAsyncToolRunning,
1415
upsertAsyncToolCall,
1516
} from '@/lib/mothership/async-runs/repository'
@@ -451,11 +452,7 @@ export async function failPendingToolCall(
451452
}
452453
const message =
453454
failureMessage ??
454-
(!desktopCall
455-
? TOOL_RESULT_LOST_MESSAGE
456-
: isLocalReadToolCall(toolCall.execName ?? toolCall.name, toolCall.params)
457-
? DESKTOP_LOCAL_READ_RESULT_MISSING_MESSAGE
458-
: DESKTOP_TOOL_RESULT_LOST_MESSAGE)
455+
(desktopCall ? await desktopResultLostMessage(toolCall) : TOOL_RESULT_LOST_MESSAGE)
459456
await settleAbandonedToolCall(toolCall, context, execContext, {
460457
message,
461458
data: { error: message, outcomeUnknown: true, doNotRetry: true },
@@ -464,6 +461,20 @@ export async function failPendingToolCall(
464461
})
465462
}
466463

464+
/**
465+
* What a desktop call that lost its result tells the model. Only a local read can be in flight
466+
* without a claim (an older desktop reads without claiming), and such a read may never have
467+
* started; anything the desktop claimed did start.
468+
*/
469+
async function desktopResultLostMessage(toolCall: ToolCallState): Promise<string> {
470+
if (!isLocalReadToolCall(toolCall.execName ?? toolCall.name, toolCall.params))
471+
return DESKTOP_TOOL_RESULT_LOST_MESSAGE
472+
const stored = await getAsyncToolCall(toolCall.id).catch(() => null)
473+
return stored?.claimedBy
474+
? DESKTOP_TOOL_RESULT_LOST_MESSAGE
475+
: DESKTOP_LOCAL_READ_RESULT_MISSING_MESSAGE
476+
}
477+
467478
/**
468479
* Settles one abandoned tool locally once its durable failure is decided. With `unclaimedOnly`
469480
* it applies only while the call is still unclaimed and reports whether it did.

‎apps/sim/lib/mothership/tools/client/completion.test.ts‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,23 @@ describe('client tool completion reporting', () => {
5353
expect(settled).toBe(true)
5454
})
5555

56+
it('stops at once when the trimmed retry of an oversized report learns the call is held', async () => {
57+
vi.useFakeTimers()
58+
fetchMock
59+
.mockResolvedValueOnce(new Response(null, { status: 413 }))
60+
.mockResolvedValueOnce(new Response(null, { status: 409 }))
61+
let settled = false
62+
63+
void reportClientToolCompletion('tool-1', 'error', 'Failed', {
64+
logs: 'x'.repeat(11 * 1024 * 1024),
65+
}).then(() => {
66+
settled = true
67+
})
68+
await vi.advanceTimersByTimeAsync(0)
69+
70+
expect(settled).toBe(true)
71+
})
72+
5673
it('uses a keepalive request with the exact terminal payload', async () => {
5774
await reportClientToolCompletionOnPageExit('tool-1', 'success', 'Browser action completed', {
5875
url: 'https://example.com',

‎apps/sim/lib/mothership/tools/client/completion.ts‎

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,19 @@ async function fetchCompletion(input: RequestInfo | URL, init: RequestInit): Pro
3232
}
3333
}
3434

35+
/**
36+
* Whether a delivery attempt needs no retry: the server took it, or answered 409 because the
37+
* desktop app holds the call and only its own result settles it.
38+
*/
39+
function isSettledDelivery(response: Response, toolCallId: string): boolean {
40+
if (response.ok) return true
41+
if (response.status !== 409) return false
42+
logger.info('Client tool completion was not needed: another reporter holds the call', {
43+
toolCallId,
44+
})
45+
return true
46+
}
47+
3548
/**
3649
* Persist a client-executed tool result and wake the server-side async waiter.
3750
* Shared by workflow execution and desktop-native client tools.
@@ -71,14 +84,7 @@ export async function reportClientToolCompletion(
7184
for (let attempt = 1; attempt <= maxAttempts; attempt++) {
7285
try {
7386
const response = await send(body)
74-
if (response.ok) return
75-
if (response.status === 409) {
76-
// The desktop app holds this call; its own result settles it, so retrying cannot help.
77-
logger.info('Client tool completion was not needed: another reporter holds the call', {
78-
toolCallId,
79-
})
80-
return
81-
}
87+
if (isSettledDelivery(response, toolCallId)) return
8288

8389
if (isRecordLike(data) && bodySize > largePayloadThreshold) {
8490
const { logs: _logs, ...dataWithoutLogs } = data
@@ -96,7 +102,7 @@ export async function reportClientToolCompletion(
96102
data: dataWithoutLogs,
97103
})
98104
)
99-
if (retryResponse.ok) return
105+
if (isSettledDelivery(retryResponse, toolCallId)) return
100106
lastError = new Error(`Completion retry failed with status ${retryResponse.status}`)
101107
} else {
102108
lastError = new Error(`Completion failed with status ${response.status}`)

0 commit comments

Comments
 (0)