From 03d9ebdd36f20fb2d36734e22361ce5a6328ed6f Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 5 Oct 2026 21:28:25 -0700 Subject: [PATCH 1/5] fix(mothership): keep local file tools running when the chat view changes Local file reads and imports now take the same Stop-only lifetime as browser actions: a history reconnect, a stream recovery or leaving the chat view leaves them running so they finish and report, and only the user's Stop cancels them. The desktop-tool classifier in client-executed-tools.ts is removed in favour of the single one in tools/desktop-tools.ts. When the desktop app holds a call, confirm now answers 409 to any other reporter (a stale replay, a lost race against the claim), and the client treats 409 as final instead of retrying a 404 five times. The not-started results now tell the model what it can act on: the action never started; don't retry it in this turn; ask the user to keep the chat open in the Sim desktop app. A local read the server cannot see picked up is no longer described as started. The obsolete admission probe script is deleted. Nothing ran it, it no longer type-checked against the repository API, and the integration suites cover the same admission behaviour against real Postgres. --- .../sim/app/api/copilot/confirm/route.test.ts | 10 +- apps/sim/app/api/copilot/confirm/route.ts | 22 +- .../home/hooks/stream/handle-tool-event.ts | 4 +- .../home/hooks/use-chat.dom.test.tsx | 50 ++- .../[workspaceId]/home/hooks/use-chat.ts | 13 +- apps/sim/lib/mothership/constants.ts | 2 +- .../mothership/request/tools/desktop-wait.ts | 2 +- .../mothership/request/tools/executor.test.ts | 20 +- .../lib/mothership/request/tools/executor.ts | 19 +- .../mothership/tools/client-executed-tools.ts | 22 +- .../tools/client/completion.test.ts | 13 + .../lib/mothership/tools/client/completion.ts | 7 + .../client/desktop-tool-pickup.integration.ts | 4 +- .../client/terminal-tool-execution.test.ts | 2 +- .../tools/client/terminal-tool-execution.ts | 2 +- bun.lock | 1 - package.json | 1 - scripts/check-explicit-any.baseline.json | 3 +- .../probes/mothership-request-admission.ts | 419 ------------------ 19 files changed, 133 insertions(+), 483 deletions(-) delete mode 100644 scripts/probes/mothership-request-admission.ts diff --git a/apps/sim/app/api/copilot/confirm/route.test.ts b/apps/sim/app/api/copilot/confirm/route.test.ts index 6a29cca19a4..6c0f3414f2e 100644 --- a/apps/sim/app/api/copilot/confirm/route.test.ts +++ b/apps/sim/app/api/copilot/confirm/route.test.ts @@ -165,7 +165,7 @@ describe('Copilot Confirm API Route', () => { }) ) - expect(response.status).toBe(404) + expect(response.status).toBe(409) expect(completeAsyncToolCall).not.toHaveBeenCalled() expect(detachAsyncToolCall).not.toHaveBeenCalled() expect(encryptSecret).not.toHaveBeenCalled() @@ -233,8 +233,10 @@ describe('Copilot Confirm API Route', () => { }) ) - expect(response.status).toBe(404) - expect(await response.json()).toEqual({ error: 'Pending client tool call not found' }) + expect(response.status).toBe(409) + expect(await response.json()).toEqual({ + error: 'The desktop app holds this tool call; only its own result settles it', + }) expect(completePendingAsyncToolCall).toHaveBeenCalledOnce() expect(completeClaimedAsyncToolCall).not.toHaveBeenCalled() expect(completeAsyncToolCall).not.toHaveBeenCalled() @@ -300,7 +302,7 @@ describe('Copilot Confirm API Route', () => { }) ) - expect(response.status).toBe(404) + expect(response.status).toBe(409) expect(completeClaimedAsyncToolCall).toHaveBeenCalledWith(expect.any(Object), 'desktop-browser') expect(publishToolConfirmation).not.toHaveBeenCalled() }) diff --git a/apps/sim/app/api/copilot/confirm/route.ts b/apps/sim/app/api/copilot/confirm/route.ts index f80fa0e7312..3305fa4d2e2 100644 --- a/apps/sim/app/api/copilot/confirm/route.ts +++ b/apps/sim/app/api/copilot/confirm/route.ts @@ -98,6 +98,18 @@ function acknowledgeSettledToolCall( return createConfirmationResponse(toolCallId, settledStatus, 'Tool call was already settled') } +/** + * A desktop call this report may not settle: the desktop app holds it under its claim (or a + * report raced that claim and lost), so only the claim's own result settles it. Final, not + * retryable: the reporter stops. + */ +function heldByAnotherReporterResponse(): NextResponse { + return NextResponse.json( + { error: 'The desktop app holds this tool call; only its own result settles it' }, + { status: 409 } + ) +} + /** Atomically finalize or detach a client tool before publishing its wakeup event. */ async function updateToolCallStatus( existing: NonNullable>>, @@ -321,7 +333,11 @@ export const POST = withRouteHandler((req: NextRequest) => { const isMutableClientToolCall = isWorkflowTool ? isWorkflowToolExecutionClaimable(existing.status, existing.permissionDecision) : existing.status === ASYNC_TOOL_STATUS.running || isPreclaimNativeTerminalOutcome - if ((isNativeClientTool || isWorkflowTool) && !isMutableClientToolCall) { + if (isNativeClientTool && !isMutableClientToolCall) { + span.setAttribute(TraceAttr.CopilotConfirmOutcome, CopilotConfirmOutcome.ToolCallNotFound) + return heldByAnotherReporterResponse() + } + if (isWorkflowTool && !isMutableClientToolCall) { span.setAttribute(TraceAttr.CopilotConfirmOutcome, CopilotConfirmOutcome.ToolCallNotFound) return createNotFoundResponse('Running client tool call not found') } @@ -334,7 +350,7 @@ export const POST = withRouteHandler((req: NextRequest) => { existing.status !== ASYNC_TOOL_STATUS.pending ) { span.setAttribute(TraceAttr.CopilotConfirmOutcome, CopilotConfirmOutcome.ToolCallNotFound) - return createNotFoundResponse('Pending client tool call not found') + return heldByAnotherReporterResponse() } let effectiveStatus = status @@ -478,7 +494,7 @@ export const POST = withRouteHandler((req: NextRequest) => { if (reconciledOutcome === 'conflict' && isPreclaimNativeTerminalOutcome) { span.setAttribute(TraceAttr.CopilotConfirmOutcome, CopilotConfirmOutcome.ToolCallNotFound) - return createNotFoundResponse('Pending client tool call not found') + return heldByAnotherReporterResponse() } if (reconciledOutcome !== 'updated') { diff --git a/apps/sim/app/workspace/[workspaceId]/home/hooks/stream/handle-tool-event.ts b/apps/sim/app/workspace/[workspaceId]/home/hooks/stream/handle-tool-event.ts index f110ff33b93..154aac0dce4 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/hooks/stream/handle-tool-event.ts +++ b/apps/sim/app/workspace/[workspaceId]/home/hooks/stream/handle-tool-event.ts @@ -17,9 +17,9 @@ import { } from '@/lib/mothership/resources/extraction' import { isClientExecutedToolCall, - isDesktopExecutedToolCall, isWorkflowToolName, } from '@/lib/mothership/tools/client-executed-tools' +import { isDesktopToolCall } from '@/lib/mothership/tools/desktop-tools' import { invalidateResourceQueries } from '@/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-registry' import type { StreamLoopContext } from '@/app/workspace/[workspaceId]/home/hooks/stream/stream-context' import { @@ -197,7 +197,7 @@ export function handleToolEvent(ctx: StreamLoopContext, parsed: ToolEvent): void // tools to it: its answer could only be an error, and that error would beat the real result. const shouldStartClientTool = isClientExecutedToolCall(name, args) && - (isDesktopApp() || !isDesktopExecutedToolCall(name, args)) && + (isDesktopApp() || !isDesktopToolCall(name, args)) && !isPartial && !deps.options.suppressedWorkflowToolStartIds?.has(rawId) && node?.kind === 'tool' && diff --git a/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.dom.test.tsx b/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.dom.test.tsx index d845f0aac4b..7213496c82f 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.dom.test.tsx +++ b/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.dom.test.tsx @@ -28,8 +28,14 @@ import { QueryClient, QueryClientProvider } from '@tanstack/react-query' import { createRoot, type Root } from 'react-dom/client' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -const { mockRequestJson, mockExecuteWorkflow, mockExecuteBrowserToolOnClient } = vi.hoisted(() => ({ +const { + mockRequestJson, + mockExecuteWorkflow, + mockExecuteBrowserToolOnClient, + mockExecuteLocalFilesystemTool, +} = vi.hoisted(() => ({ mockExecuteBrowserToolOnClient: vi.fn(), + mockExecuteLocalFilesystemTool: vi.fn(), mockRequestJson: vi.fn(), mockExecuteWorkflow: vi.fn< @@ -47,6 +53,9 @@ vi.mock('@/app/workspace/[workspaceId]/providers/feature-flags-provider', () => vi.mock('next/navigation', () => nextNavigationMock) vi.mock('@/lib/desktop', () => libDesktopMock) +vi.mock('@/lib/mothership/tools/client/local-filesystem', () => ({ + executeLocalFilesystemTool: mockExecuteLocalFilesystemTool, +})) vi.mock('@/lib/mothership/tools/client/browser-tool-execution', () => ({ executeBrowserToolOnClient: mockExecuteBrowserToolOnClient, })) @@ -2129,8 +2138,21 @@ describe('useChat remount send recovery', () => { ?.messages.map((message) => message.id) ).toEqual(['saved-user', 'saved-assistant']) }) - describe('a desktop browser action in flight', () => { - const chatId = 'chat-browser-action' + describe.each([ + { + kind: 'browser action', + toolName: 'browser_list_tabs', + arguments: {}, + lifetimeOf: () => mockExecuteBrowserToolOnClient.mock.calls[0]?.[5], + }, + { + kind: 'local file read', + toolName: 'read_local_file', + arguments: { path: '/Users/me/notes.txt' }, + lifetimeOf: () => mockExecuteLocalFilesystemTool.mock.calls[0]?.[3]?.signal, + }, + ])('a desktop $kind in flight', ({ toolName, arguments: toolArguments, lifetimeOf }) => { + const chatId = 'chat-desktop-action' const history: MothershipChatHistory = { id: chatId, mode: 'agent', @@ -2140,8 +2162,8 @@ describe('useChat remount send recovery', () => { resources: [], } - /** Opens a turn whose stream delivers one desktop browser call and stays open. */ - async function startBrowserAction() { + /** Opens a turn whose stream delivers one desktop tool call and stays open. */ + async function startDesktopAction() { let streamId: string | undefined const replays: string[] = [] mockRequestJson.mockImplementation((contract: AnyApiRouteContract) => @@ -2168,9 +2190,9 @@ describe('useChat remount send recovery', () => { phase: 'call', executor: 'client', mode: 'async', - toolName: 'browser_list_tabs', - toolCallId: 'browser-call', - arguments: {}, + toolName, + toolCallId: 'desktop-call', + arguments: toolArguments, }, } return new Response( @@ -2186,10 +2208,10 @@ describe('useChat remount send recovery', () => { await act(async () => { void chat.getResult().sendMessage('List my tabs') }) - await waitFor(() => mockExecuteBrowserToolOnClient.mock.calls.length === 1) - const toolSignal = mockExecuteBrowserToolOnClient.mock.calls[0]?.[5] + await waitFor(() => lifetimeOf() !== undefined) + const toolSignal = lifetimeOf() if (!(toolSignal instanceof AbortSignal)) - throw new Error('The browser action has no lifetime') + throw new Error('The desktop action has no lifetime') return { ...chat, toolSignal, replays } } @@ -2202,7 +2224,7 @@ describe('useChat remount send recovery', () => { }) it('keeps running when the window returns to view and the stream is recovered', async () => { - const { toolSignal, replays } = await startBrowserAction() + const { toolSignal, replays } = await startDesktopAction() Object.defineProperty(document, 'visibilityState', { configurable: true, @@ -2217,7 +2239,7 @@ describe('useChat remount send recovery', () => { }) it('keeps running when the chat view unmounts, so it finishes and reports its result', async () => { - const { toolSignal, unmount } = await startBrowserAction() + const { toolSignal, unmount } = await startDesktopAction() unmount() @@ -2225,7 +2247,7 @@ describe('useChat remount send recovery', () => { }) it('is cancelled when the user stops the chat', async () => { - const { toolSignal, getResult } = await startBrowserAction() + const { toolSignal, getResult } = await startDesktopAction() await act(async () => { await getResult().stopGeneration() diff --git a/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts b/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts index 64d4d813c0c..657c7c86961 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts +++ b/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts @@ -457,11 +457,12 @@ export async function waitForDetachedChatResolution( const USER_STOP_ABORT_REASON = 'user_stop:client_stopGeneration' /** - * The lifetime a browser action started from one stream observes: only the user's Stop cancels - * it. Replacing the stream reader (the window returning to view, a history reconnect) or leaving - * the chat view leaves it running, so it finishes and reports its own result. + * The lifetime a desktop tool (a browser action, a local file read or import) started from one + * stream observes: only the user's Stop cancels it. Replacing the stream reader (the window + * returning to view, a history reconnect) or leaving the chat view leaves it running, so it + * finishes and reports its own result. */ -function browserToolLifetime(streamSignal: AbortSignal | undefined): AbortSignal | undefined { +function desktopToolLifetime(streamSignal: AbortSignal | undefined): AbortSignal | undefined { if (!streamSignal) return undefined const lifetime = new AbortController() const followStop = () => { @@ -1595,7 +1596,7 @@ export function useChat( const options = { workspaceId, chatId: chatIdRef.current ?? selectedChatIdRef.current, - signal: abortControllerRef.current?.signal, + signal: desktopToolLifetime(abortControllerRef.current?.signal), } /** * Dynamic on purpose: the local-filesystem executor only runs for desktop-local @@ -2171,7 +2172,7 @@ export function useChat( shouldContinue?: () => boolean } ) => { - const browserToolSignal = browserToolLifetime(abortControllerRef.current?.signal) + const browserToolSignal = desktopToolLifetime(abortControllerRef.current?.signal) const activityTracker = getResourceActivityTracker( expectedGen ?? streamGenRef.current, options?.targetChatId diff --git a/apps/sim/lib/mothership/constants.ts b/apps/sim/lib/mothership/constants.ts index 90cdb8037a7..a8f0ba510a8 100644 --- a/apps/sim/lib/mothership/constants.ts +++ b/apps/sim/lib/mothership/constants.ts @@ -73,7 +73,7 @@ export const COPILOT_WORKFLOW_TOOL_CLIENT_GRACE_MS = 30_000 * Same cause as the workflow grace: only the chat view showing this chat starts the call, so a * call issued while the user is on another chat or page is claimed by nobody. A live view claims * within a second or two (stream frame -> IPC -> authorize), and there is no server fallback to - * run instead, so the call fails with a "not started, safe to retry" result rather than parking + * run instead, so the call fails with a "never started" result rather than parking * until a watchdog calls it hung. */ export const DESKTOP_TOOL_PICKUP_GRACE_MS = 15_000 diff --git a/apps/sim/lib/mothership/request/tools/desktop-wait.ts b/apps/sim/lib/mothership/request/tools/desktop-wait.ts index 39105cb2636..dc484735a01 100644 --- a/apps/sim/lib/mothership/request/tools/desktop-wait.ts +++ b/apps/sim/lib/mothership/request/tools/desktop-wait.ts @@ -11,7 +11,7 @@ import type { ResolvedSecretTraceRegistry } from '@/executor/utils/resolved-secr const logger = createLogger('CopilotDesktopToolWait') const DESKTOP_TOOL_NOT_STARTED_MESSAGE = - 'Not run: this chat is not open in the Sim desktop app, so nothing picked up this call and nothing happened on the user’s computer. It is safe to retry once the user opens this chat in the Sim desktop app.' + '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.' /** The model-facing result of a desktop call that was never picked up. */ export function desktopToolNotStarted(): { message: string; data: Record } { diff --git a/apps/sim/lib/mothership/request/tools/executor.test.ts b/apps/sim/lib/mothership/request/tools/executor.test.ts index aa2461e61fc..b86b155723a 100644 --- a/apps/sim/lib/mothership/request/tools/executor.test.ts +++ b/apps/sim/lib/mothership/request/tools/executor.test.ts @@ -919,7 +919,7 @@ describe('watchdog completion provenance', () => { expect(toolCall.result).toEqual({ success: false, - output: { error: expect.stringContaining('safe to retry'), notStarted: true }, + output: { error: expect.stringContaining('never started'), notStarted: true }, }) }) @@ -940,6 +940,24 @@ describe('watchdog completion provenance', () => { }) }) + it('does not claim a local read the server never saw picked up had started', async () => { + const { toolCall, context, execContext } = createHungClient() + toolCall.name = 'read_local_file' + toolCall.params = { path: '/Users/me/notes.txt' } + completePendingAsyncToolCall.mockResolvedValueOnce(null) + + await failPendingToolCall(toolCall.id, context, execContext) + + expect(toolCall.result).toEqual({ + success: false, + output: { + error: expect.stringContaining('may never have started'), + outcomeUnknown: true, + doNotRetry: true, + }, + }) + }) + it('preserves an actual completion that settles while encryption is pending', async () => { const { toolCall, context, execContext } = createHungClient() let finishEncryption: (value: { encrypted: string }) => void = () => {} diff --git a/apps/sim/lib/mothership/request/tools/executor.ts b/apps/sim/lib/mothership/request/tools/executor.ts index b1d02b39e5e..bed3f71d834 100644 --- a/apps/sim/lib/mothership/request/tools/executor.ts +++ b/apps/sim/lib/mothership/request/tools/executor.ts @@ -86,7 +86,7 @@ import { type ToolCallState, } from '@/lib/mothership/request/types' import { ensureHandlersRegistered, executeTool } from '@/lib/mothership/tool-executor' -import { isDesktopToolCall } from '@/lib/mothership/tools/desktop-tools' +import { isDesktopToolCall, isLocalReadToolCall } from '@/lib/mothership/tools/desktop-tools' import { withSandboxResourceScope } from '@/lib/mothership/tools/sandbox-resources' import { isMcpTool } from '@/executor/constants' @@ -414,6 +414,12 @@ const TOOL_RESULT_LOST_MESSAGE = 'This tool started, but its result never came back, so it was abandoned to let the conversation continue. Its outcome is unknown: it may already have taken effect, so inspect the current state before repeating it, and do not retry it automatically.' const DESKTOP_TOOL_RESULT_LOST_MESSAGE = 'The Sim desktop app started this action, but its result never came back (the chat view closed or the app stopped responding). Its outcome is unknown: it may already have taken effect, so inspect the current state before repeating it, and do not retry it automatically.' +/** + * A local read the server cannot see picked up (one an older desktop reads without claiming) may + * never have started; either way, reading changed nothing. + */ +const DESKTOP_LOCAL_READ_RESULT_MISSING_MESSAGE = + 'No result came back from the Sim desktop app for this read: it may never have started (this chat may not be open there), or its result was lost. Reading changes nothing 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.' const UNAVAILABLE_TOOL_SETTLEMENT_MESSAGE = 'The tool result could not be restored before the conversation resumed. Its outcome is unknown; do not retry it automatically.' @@ -423,8 +429,8 @@ const UNAVAILABLE_TOOL_SETTLEMENT_MESSAGE = * Execution ownership remains held while retained work cleans up. * * Without an explicit `failureMessage` the model learns which of two things happened: a desktop - * call nothing claimed never started (`notStarted`, safe to retry), and anything else started and - * lost its result (`outcomeUnknown`, `doNotRetry`). + * call nothing claimed never started (`notStarted`), and anything else started (or could not be + * seen starting) and lost its result (`outcomeUnknown`, `doNotRetry`). */ export async function failPendingToolCall( toolCallId: string, @@ -444,7 +450,12 @@ export async function failPendingToolCall( if (settled) return } const message = - failureMessage ?? (desktopCall ? DESKTOP_TOOL_RESULT_LOST_MESSAGE : TOOL_RESULT_LOST_MESSAGE) + failureMessage ?? + (!desktopCall + ? TOOL_RESULT_LOST_MESSAGE + : isLocalReadToolCall(toolCall.execName ?? toolCall.name, toolCall.params) + ? DESKTOP_LOCAL_READ_RESULT_MISSING_MESSAGE + : DESKTOP_TOOL_RESULT_LOST_MESSAGE) await settleAbandonedToolCall(toolCall, context, execContext, { message, data: { error: message, outcomeUnknown: true, doNotRetry: true }, diff --git a/apps/sim/lib/mothership/tools/client-executed-tools.ts b/apps/sim/lib/mothership/tools/client-executed-tools.ts index 167132015bf..a0cc8af4382 100644 --- a/apps/sim/lib/mothership/tools/client-executed-tools.ts +++ b/apps/sim/lib/mothership/tools/client-executed-tools.ts @@ -1,6 +1,4 @@ -import { isCurrentBrowserToolName } from '@sim/browser-protocol' -import { isTerminalToolName } from '@sim/terminal-protocol' -import { isNativeFileTool, isUserLocalVfsToolCall } from '@/lib/mothership/tools/local-filesystem' +import { isDesktopToolCall } from '@/lib/mothership/tools/desktop-tools' const WORKFLOW_TOOL_NAMES = new Set([ 'run_workflow', @@ -13,22 +11,6 @@ export function isWorkflowToolName(name: string): boolean { return WORKFLOW_TOOL_NAMES.has(name) } -/** - * Client-executed calls only the desktop app can run: local file access, the agent browser, and - * the terminal. A web tab watching the same chat must leave them to the desktop app. - */ -export function isDesktopExecutedToolCall( - name: string, - args: Record | undefined -): boolean { - return ( - isNativeFileTool(name) || - isUserLocalVfsToolCall(name, args) || - isCurrentBrowserToolName(name) || - isTerminalToolName(name) - ) -} - /** * Tool calls the browser starts from the call frame's own arguments: workflow * runs, local file access, browser actions, and terminal commands. The stream @@ -38,5 +20,5 @@ export function isClientExecutedToolCall( name: string, args: Record | undefined ): boolean { - return isWorkflowToolName(name) || isDesktopExecutedToolCall(name, args) + return isWorkflowToolName(name) || isDesktopToolCall(name, args) } diff --git a/apps/sim/lib/mothership/tools/client/completion.test.ts b/apps/sim/lib/mothership/tools/client/completion.test.ts index 2398243e4db..50237745666 100644 --- a/apps/sim/lib/mothership/tools/client/completion.test.ts +++ b/apps/sim/lib/mothership/tools/client/completion.test.ts @@ -40,6 +40,19 @@ describe('client tool completion reporting', () => { expect(signals.every((signal) => signal.aborted)).toBe(true) }) + it('stops at once when another client holds the call, instead of retrying a final answer', async () => { + vi.useFakeTimers() + fetchMock.mockResolvedValue(new Response(null, { status: 409 })) + let settled = false + + void reportClientToolCompletion('tool-1', 'error', 'Not run: delivered too late').then(() => { + settled = true + }) + await vi.advanceTimersByTimeAsync(0) + + expect(settled).toBe(true) + }) + it('uses a keepalive request with the exact terminal payload', async () => { await reportClientToolCompletionOnPageExit('tool-1', 'success', 'Browser action completed', { url: 'https://example.com', diff --git a/apps/sim/lib/mothership/tools/client/completion.ts b/apps/sim/lib/mothership/tools/client/completion.ts index 0cd9fae1e44..41405232f98 100644 --- a/apps/sim/lib/mothership/tools/client/completion.ts +++ b/apps/sim/lib/mothership/tools/client/completion.ts @@ -72,6 +72,13 @@ export async function reportClientToolCompletion( try { const response = await send(body) if (response.ok) return + if (response.status === 409) { + // The desktop app holds this call; its own result settles it, so retrying cannot help. + logger.info('Client tool completion was not needed: another reporter holds the call', { + toolCallId, + }) + return + } if (isRecordLike(data) && bodySize > largePayloadThreshold) { const { logs: _logs, ...dataWithoutLogs } = data diff --git a/apps/sim/lib/mothership/tools/client/desktop-tool-pickup.integration.ts b/apps/sim/lib/mothership/tools/client/desktop-tool-pickup.integration.ts index 45b67358c7e..98a8b3c983b 100644 --- a/apps/sim/lib/mothership/tools/client/desktop-tool-pickup.integration.ts +++ b/apps/sim/lib/mothership/tools/client/desktop-tool-pickup.integration.ts @@ -191,7 +191,7 @@ describe.runIf(Boolean(redisUrl))('a desktop tool call nobody picks up', () => { expect(Date.now() - startedAt).toBeLessThan(TURN_WAIT_MS - 5_000) expect(completion).toMatchObject({ status: 'error', - data: { notStarted: true, error: expect.stringContaining('safe to retry') }, + data: { notStarted: true, error: expect.stringContaining('never started') }, }) const [row] = await db .select({ status: copilotAsyncToolCalls.status }) @@ -245,7 +245,7 @@ describe.runIf(Boolean(redisUrl))('a desktop tool call nobody picks up', () => { data: { output: 'tests passed' }, }) - expect(staleReport.ok).toBe(false) + expect(staleReport.status).toBe(409) expect(realResult.status).toBe(200) expect(await answer).toMatchObject({ status: 'success' }) }, diff --git a/apps/sim/lib/mothership/tools/client/terminal-tool-execution.test.ts b/apps/sim/lib/mothership/tools/client/terminal-tool-execution.test.ts index 95cc1f2d703..7bf7968924b 100644 --- a/apps/sim/lib/mothership/tools/client/terminal-tool-execution.test.ts +++ b/apps/sim/lib/mothership/tools/client/terminal-tool-execution.test.ts @@ -67,7 +67,7 @@ describe('terminal client execution', () => { expect(reportClientToolCompletion).toHaveBeenCalledWith( 'terminal-stale', 'error', - expect.stringContaining('safe to retry'), + expect.stringContaining('never started'), expect.objectContaining({ notStarted: true }) ) }) diff --git a/apps/sim/lib/mothership/tools/client/terminal-tool-execution.ts b/apps/sim/lib/mothership/tools/client/terminal-tool-execution.ts index 8844c66acc1..71bfc1a6489 100644 --- a/apps/sim/lib/mothership/tools/client/terminal-tool-execution.ts +++ b/apps/sim/lib/mothership/tools/client/terminal-tool-execution.ts @@ -26,7 +26,7 @@ const logger = createLogger('CopilotTerminalToolExecution') /** Tool events older than this are replays, not live instructions. */ const MAX_EVENT_AGE_MS = 120_000 const STALE_EVENT_MESSAGE = - 'Not run: this terminal call reached the Sim desktop app too late to start safely, so nothing ran on the user’s computer. It is safe to retry.' + 'Not run: this terminal command never started, because it reached the Sim desktop app too late to start safely. Nothing ran 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.' const EXECUTED_STORAGE_PREFIX = 'sim:copilot:terminal-tool-executed:' /** diff --git a/bun.lock b/bun.lock index 26b986ecb4b..206323b095a 100644 --- a/bun.lock +++ b/bun.lock @@ -17,7 +17,6 @@ "ajv": "8.18.0", "commander": "^11.1.0", "concurrently": "10.0.5", - "drizzle-orm": "^0.45.2", "glob": "13.0.0", "gray-matter": "4.0.3", "husky": "9.1.7", diff --git a/package.json b/package.json index 99443cc674e..ac3f50aa1a7 100644 --- a/package.json +++ b/package.json @@ -178,7 +178,6 @@ "ajv": "8.18.0", "commander": "^11.1.0", "concurrently": "10.0.5", - "drizzle-orm": "^0.45.2", "glob": "13.0.0", "gray-matter": "4.0.3", "husky": "9.1.7", diff --git a/scripts/check-explicit-any.baseline.json b/scripts/check-explicit-any.baseline.json index a0b435321dd..af7872b706f 100644 --- a/scripts/check-explicit-any.baseline.json +++ b/scripts/check-explicit-any.baseline.json @@ -1972,7 +1972,6 @@ "packages/workflow-renderer/src/workflow-block/workflow-block-view.tsx": 12, "packages/workflow-types/src/workflow.ts": 1, "scripts/create-single-release.ts": 1, - "scripts/generate-docs.ts": 2, - "scripts/probes/mothership-request-admission.ts": 2 + "scripts/generate-docs.ts": 2 } } diff --git a/scripts/probes/mothership-request-admission.ts b/scripts/probes/mothership-request-admission.ts deleted file mode 100644 index bb77819e3d7..00000000000 --- a/scripts/probes/mothership-request-admission.ts +++ /dev/null @@ -1,419 +0,0 @@ -/** - * Exercises production run-admission transactions on disposable local PostgreSQL databases. - * Run with Bun and --tsconfig-override apps/sim/tsconfig.json. - * MOTHERSHIP_ADMISSION_PROBE_URL must point at localhost/mship_audit_*. - * The parent creates and drops its own database; a separate Stop process exits before admission. - * The fixture uses the committed pre-admission snapshot plus migration 0318, with only the - * user/workspace/chat columns needed for ownership. No provider or deployed service is called. - */ - -import assert from 'node:assert/strict' -import { resolve } from 'node:path' -import { createLogger } from '@sim/logger' -import { generateId } from '@sim/utils/id' -import { drizzle } from 'drizzle-orm/postgres-js' -import postgres from 'postgres' -import { mock } from 'bun:test' - -const root = resolve(import.meta.dir, '../..') -const logger = createLogger('RequestAdmissionProbe') -interface MigrationSnapshot { - enums: Record - tables: Record< - string, - { - columns: Record< - string, - { - name: string - type: string - notNull: boolean - primaryKey: boolean - default?: string | number | boolean - } - > - } - > -} - -const child = process.argv.includes('--stop-child') -const databaseUrl = process.env.MOTHERSHIP_ADMISSION_PROBE_URL -if (!databaseUrl) - throw new Error('MOTHERSHIP_ADMISSION_PROBE_URL must name a disposable local audit database') -const base = new URL(databaseUrl) -if ( - !base.pathname.startsWith('/mship_audit_') || - !['localhost', '127.0.0.1'].includes(base.hostname) -) - throw new Error('Expected local disposable database') -const adminUrl = new URL(base) -adminUrl.pathname = '/postgres' -const admin = child ? null : postgres(adminUrl.toString(), { max: 1, onnotice: () => {} }) -const name = child ? base.pathname.slice(1) : `mship_audit_admission_${Date.now()}` -if (admin) await admin`create database ${admin(name)}` -base.pathname = `/${name}` -const client = postgres(base.toString(), { max: 8, onnotice: () => {} }) -mock.module(`${root}/packages/db/index.ts`, () => ({ db: drizzle(client) })) -mock.module(`${root}/apps/sim/lib/mothership/request/otel.ts`, () => ({ markSpanForError() {} })) -const repo = await import('@/lib/mothership/async-runs/repository') -const quote = (value: string) => `"${value.replaceAll('"', '""')}"` -try { - if (child) { - assert.equal( - await repo.stopPendingRequest({ - userId: 'user-1', - workspaceId: 'ws-1', - streamId: 'process-exit', - }), - null - ) - } else { - const snapshot: MigrationSnapshot = await Bun.file( - `${root}/packages/db/migrations/meta/0317_snapshot.json` - ).json() - for (const enumName of [ - 'copilot_run_status', - 'copilot_async_tool_status', - 'copilot_tool_permission_decision', - ]) { - await client.unsafe( - `CREATE TYPE ${quote(enumName)} AS ENUM (${snapshot.enums[`public.${enumName}`].values.map((v: string) => `'${v}'`).join(',')})` - ) - } - for (const tableName of ['copilot_runs', 'copilot_async_tool_calls']) { - const table = snapshot.tables[`public.${tableName}`] - const columns = Object.values(table.columns).map( - (column) => - `${quote(column.name)} ${column.type}${column.notNull ? ' NOT NULL' : ''}${column.default !== undefined ? ` DEFAULT ${column.default}` : ''}${column.primaryKey ? ' PRIMARY KEY' : ''}` - ) - await client.unsafe(`CREATE TABLE ${quote(tableName)} (${columns.join(',')})`) - } - await client.unsafe('CREATE UNIQUE INDEX test_stream ON copilot_runs(stream_id)') - await client.unsafe('CREATE UNIQUE INDEX test_tool ON copilot_async_tool_calls(tool_call_id)') - await client.unsafe( - 'CREATE TABLE "user" (id text PRIMARY KEY); CREATE TABLE workspace (id text PRIMARY KEY); CREATE TABLE copilot_chats (id uuid PRIMARY KEY, user_id text NOT NULL, workspace_id text NOT NULL);' - ) - await client.unsafe( - await Bun.file( - `${root}/packages/db/migrations/0318_mothership_request_stop_admission.sql` - ).text() - ) - await client.unsafe( - "INSERT INTO \"user\" VALUES ('user-1'), ('user-2'); INSERT INTO workspace VALUES ('ws-1'), ('ws-2')" - ) - const chatId = generateId() - await client`insert into copilot_chats (id, user_id, workspace_id) values (${chatId}, 'user-1', 'ws-1')` - const input = (streamId: string, extra = {}) => ({ - streamId, - userId: 'user-1', - workspaceId: 'ws-1', - executionId: `exec-${streamId}`, - chatId, - ...extra, - }) - const scope = (streamId: string, extra = {}) => ({ - streamId, - userId: 'user-1', - workspaceId: 'ws-1', - ...extra, - }) - for (const status of ['complete', 'error', 'cancelled'] as const) { - const streamId = `terminal-${status}` - const run = await repo.createRunSegment(input(streamId)) - const toolCallId = `tool-${streamId}` - await repo.upsertAsyncToolCall({ runId: run.id, toolCallId, toolName: 'run_code' }) - await repo.updateRunStatus(run.id, status, { completedAt: new Date() }) - assert.equal( - (await repo.claimToolExecution({ runId: run.id, toolCallId, userId: 'user-1' })).outcome, - 'closed' - ) - await repo.updateRunStatus(run.id, 'paused_waiting_for_tool') - assert.equal((await repo.getLatestRunForStream(streamId, 'user-1'))?.status, status) - } - logger.info('PASS every terminal run refuses late tools and delayed pause events') - const terminalOwner = await repo.createRunSegment(input('terminal-command-owner')) - const ownerTool = { - runId: terminalOwner.id, - toolCallId: 'terminal-owner-tool', - userId: 'user-1', - } - await repo.upsertAsyncToolCall({ - runId: terminalOwner.id, - toolCallId: ownerTool.toolCallId, - toolName: 'run_code', - }) - assert.equal((await repo.claimToolExecution(ownerTool)).outcome, 'claimed') - const commandIdentity = { - id: generateId(), - sandboxId: 'recorded-sandbox', - sessionKey: 'chat:terminal-owner', - } - await repo.recordSimSandboxProcess({ ...ownerTool, process: commandIdentity }) - await repo.updateRunStatus(terminalOwner.id, 'complete', { completedAt: new Date() }) - await assert.rejects( - repo.recordSimSandboxProcess({ - ...ownerTool, - process: { ...commandIdentity, id: generateId() }, - }) - ) - assert.equal( - await repo.areStreamToolExecutionsSettled('terminal-command-owner', 'user-1'), - false - ) - await repo.settleSimToolExecution(ownerTool.toolCallId) - assert.equal( - await repo.areStreamToolExecutionsSettled('terminal-command-owner', 'user-1'), - false - ) - await repo.settleSimSandboxProcess(ownerTool.toolCallId, commandIdentity.id) - assert.equal( - await repo.areStreamToolExecutionsSettled('terminal-command-owner', 'user-1'), - true - ) - for (let i = 0; i < 30; i++) { - const streamId = `finish-race-${i}` - const run = await repo.createRunSegment(input(streamId)) - const tool = { runId: run.id, toolCallId: `tool-${streamId}`, userId: 'user-1' } - await repo.upsertAsyncToolCall({ - runId: run.id, - toolCallId: tool.toolCallId, - toolName: 'run_code', - }) - const [claim] = await Promise.all([ - repo.claimToolExecution(tool), - repo.updateRunStatus(run.id, 'complete'), - ]) - assert.equal( - await repo.areStreamToolExecutionsSettled(streamId, 'user-1'), - claim.outcome === 'closed' - ) - if (claim.outcome === 'claimed') await repo.settleSimToolExecution(tool.toolCallId) - assert.equal(await repo.areStreamToolExecutionsSettled(streamId, 'user-1'), true) - } - logger.info( - 'PASS terminal status fences commands without forging handler/process settlement; thirty completion/claim races' - ) - const createWorkbenchChat = async () => { - const id = generateId() - await client`insert into copilot_chats (id, user_id, workspace_id) values (${id}, 'user-1', 'ws-1')` - return id - } - const createWorkbenchTool = async (workbenchChatId: string, streamId: string) => { - const run = await repo.createRunSegment(input(streamId, { chatId: workbenchChatId })) - const tool = { runId: run.id, toolCallId: `tool-${streamId}`, userId: 'user-1' } - await repo.upsertAsyncToolCall({ ...tool, toolName: 'run_code' }) - return tool - } - const workbenchChatId = await createWorkbenchChat() - const sessionKey = `mothership-chat:${workbenchChatId}` - const prior = await createWorkbenchTool(workbenchChatId, 'workbench-prior') - assert.equal((await repo.claimToolExecution(prior)).outcome, 'claimed') - const priorCommand = { id: generateId(), sandboxId: 'workbench-vm', sessionKey } - await repo.recordSimSandboxProcess({ ...prior, process: priorCommand }) - await repo.updateRunStatus(prior.runId, 'complete') - const current = await createWorkbenchTool(workbenchChatId, 'workbench-current') - assert.equal((await repo.claimToolExecution(current)).outcome, 'claimed') - const access = { ...current, sessionKey } - assert.deepEqual(await repo.prepareWorkbenchAccess(access), { - handlersPending: true, - processes: [{ ...priorCommand, toolCallId: prior.toolCallId }], - }) - await repo.settleSimSandboxProcess(prior.toolCallId, priorCommand.id) - assert.deepEqual(await repo.prepareWorkbenchAccess(access), { - handlersPending: true, - processes: [], - }) - await repo.settleSimToolExecution(prior.toolCallId) - const ready = { handlersPending: false, processes: [] } - assert.deepEqual(await repo.prepareWorkbenchAccess(access), ready) - await assert.rejects(repo.prepareWorkbenchAccess({ ...prior, sessionKey })) - const sibling = { ...current, toolCallId: 'workbench-sibling' } - await repo.upsertAsyncToolCall({ ...sibling, toolName: 'run_code' }) - assert.equal((await repo.claimToolExecution(sibling)).outcome, 'claimed') - const siblingCommand = { ...priorCommand, id: generateId() } - await repo.recordSimSandboxProcess({ ...sibling, process: siblingCommand }) - assert.deepEqual(await repo.prepareWorkbenchAccess(access), ready) - assert.deepEqual(await repo.prepareWorkbenchAccess({ ...sibling, sessionKey }), ready) - const emptyNext = await repo.createRunSegment( - input('workbench-empty-next', { chatId: workbenchChatId }) - ) - assert.deepEqual(await repo.prepareWorkbenchAccess(access), ready) - assert.equal( - (await repo.getLatestRunForStream('workbench-empty-next', 'user-1'))?.toolAdmissionClosedAt, - null - ) - const neverClaimed = { ...current, toolCallId: 'workbench-never-claimed' } - await repo.upsertAsyncToolCall({ ...neverClaimed, toolName: 'run_code' }) - await assert.rejects(repo.prepareWorkbenchAccess({ ...neverClaimed, sessionKey })) - await assert.rejects(repo.prepareWorkbenchAccess({ ...access, userId: 'user-2' })) - await assert.rejects(repo.prepareWorkbenchAccess({ ...access, sessionKey: 'another-chat' })) - await repo.settleSimToolExecution(current.toolCallId) - await assert.rejects(repo.prepareWorkbenchAccess(access)) - await repo.settleSimToolExecution(sibling.toolCallId) - const nextTool = { runId: emptyNext.id, userId: 'user-1', toolCallId: 'workbench-next-tool' } - await repo.upsertAsyncToolCall({ ...nextTool, toolName: 'run_code' }) - assert.equal((await repo.claimToolExecution(nextTool)).outcome, 'claimed') - assert.deepEqual(await repo.prepareWorkbenchAccess({ ...nextTool, sessionKey }), { - handlersPending: false, - processes: [{ ...siblingCommand, toolCallId: sibling.toolCallId }], - }) - await repo.settleSimSandboxProcess(sibling.toolCallId, siblingCommand.id) - assert.deepEqual(await repo.prepareWorkbenchAccess({ ...nextTool, sessionKey }), ready) - logger.info( - 'PASS workbench recovery requires both receipts; parallel siblings survive; empty newer requests cannot interrupt admission; stale/unclaimed/foreign tools refused' - ) - for (let i = 0; i < 30; i++) { - const raceChatId = await createWorkbenchChat() - const older = await createWorkbenchTool(raceChatId, `workbench-race-old-${i}`) - const newer = await createWorkbenchTool(raceChatId, `workbench-race-new-${i}`) - assert.equal((await repo.claimToolExecution(newer)).outcome, 'claimed') - const [claim, state] = await Promise.all([ - repo.claimToolExecution(older), - repo.prepareWorkbenchAccess({ ...newer, sessionKey: `mothership-chat:${raceChatId}` }), - ]) - assert.deepEqual(state, { handlersPending: claim.outcome === 'claimed', processes: [] }) - assert.equal( - (await repo.claimToolExecution({ ...older, toolCallId: 'late' })).outcome, - 'closed' - ) - await assert.rejects( - repo.prepareWorkbenchAccess({ ...older, sessionKey: `mothership-chat:${raceChatId}` }) - ) - } - logger.info('PASS thirty predecessor-claim/workbench-takeover races without false readiness') - const corruptChatId = await createWorkbenchChat() - const corruptOwner = await createWorkbenchTool(corruptChatId, 'workbench-corrupt-owner') - assert.equal((await repo.claimToolExecution(corruptOwner)).outcome, 'claimed') - const corruptCommand = { id: generateId(), sandboxId: 'foreign', sessionKey: 'another-chat' } - await repo.recordSimSandboxProcess({ ...corruptOwner, process: corruptCommand }) - const successor = await createWorkbenchTool(corruptChatId, 'workbench-corrupt-successor') - assert.equal((await repo.claimToolExecution(successor)).outcome, 'claimed') - const successorAccess = { ...successor, sessionKey: `mothership-chat:${corruptChatId}` } - await assert.rejects(repo.prepareWorkbenchAccess(successorAccess), /does not match this chat/) - assert.equal( - (await repo.getLatestRunForStream('workbench-corrupt-owner', 'user-1')) - ?.toolAdmissionClosedAt, - null - ) - await repo.settleSimSandboxProcess(corruptOwner.toolCallId, corruptCommand.id) - await repo.settleSimToolExecution(corruptOwner.toolCallId) - await client`update copilot_runs set started_at = '2026-09-01T00:00:00.000001Z' where id = ${corruptOwner.runId}` - await client`update copilot_runs set started_at = '2026-09-01T00:00:00.000002Z' where id = ${successor.runId}` - assert.deepEqual(await repo.prepareWorkbenchAccess(successorAccess), ready) - assert.ok( - (await repo.getLatestRunForStream('workbench-corrupt-owner', 'user-1'))?.toolAdmissionClosedAt - ) - logger.info( - 'PASS inconsistent command scope rolls back takeover; admission respects PostgreSQL submillisecond run ordering' - ) - const proc = Bun.spawn( - [ - process.execPath, - '--tsconfig-override', - `${root}/apps/sim/tsconfig.json`, - import.meta.path, - '--stop-child', - ], - { - env: { ...process.env, MOTHERSHIP_ADMISSION_PROBE_URL: base.toString() }, - stdout: 'inherit', - stderr: 'inherit', - } - ) - assert.equal(await proc.exited, 0) - const delayed = await repo.createRunSegment(input('process-exit')) - assert.equal(delayed.status, 'cancelled') - assert.ok(delayed.toolAdmissionClosedAt) - assert.ok(delayed.completedAt!.getTime() >= delayed.startedAt.getTime()) - assert.equal(await repo.stopPendingRequest(scope('process-exit')), null) - await assert.rejects(repo.createRunSegment(input('process-exit'))) - await repo.upsertAsyncToolCall({ - runId: delayed.id, - toolCallId: 'late-tool', - toolName: 'run_code', - }) - assert.equal( - ( - await repo.claimToolExecution({ - runId: delayed.id, - toolCallId: 'late-tool', - userId: 'user-1', - }) - ).outcome, - 'closed' - ) - logger.info( - 'PASS native Stop process exits before admission; durable cancellation, repeat Stop, duplicate refusal and no tool claim' - ) - await repo.stopPendingRequest(scope('old')) - await client`update copilot_request_stops set stopped_at = now() - interval '2 days' where stream_id = 'old'` - const [old] = await client`select stopped_at from copilot_request_stops where stream_id = 'old'` - await repo.stopPendingRequest(scope('old')) - const [again] = - await client`select stopped_at from copilot_request_stops where stream_id = 'old'` - assert.equal(new Date(old.stopped_at).getTime(), new Date(again.stopped_at).getTime()) - const oldRun = await repo.createRunSegment(input('old', { workspaceId: undefined })) - assert.equal(oldRun.status, 'cancelled') - assert.equal(oldRun.workspaceId, 'ws-1') - assert.ok(oldRun.completedAt!.getTime() >= oldRun.startedAt.getTime()) - logger.info( - 'PASS delayed/repeated Stop retains intent; canonical chat workspace fallback and nonnegative run duration' - ) - await repo.stopPendingRequest(scope('other-actor', { userId: 'user-2' })) - assert.equal((await repo.createRunSegment(input('other-actor'))).status, 'active') - await repo.stopPendingRequest(scope('other-workspace', { workspaceId: 'ws-2' })) - assert.equal((await repo.createRunSegment(input('other-workspace'))).status, 'active') - const foreign = await repo.createRunSegment(input('foreign')) - assert.equal(await repo.stopPendingRequest(scope('foreign', { userId: 'user-2' })), null) - const existing = await repo.getLatestRunForStream('foreign', 'user-1') - assert.equal(existing?.status, 'active') - assert.equal(existing?.toolAdmissionClosedAt, null) - const wrongScope = await repo.stopPendingRequest(scope('foreign', { workspaceId: 'ws-2' })) - assert.equal(wrongScope?.id, foreign.id) - assert.equal(wrongScope?.workspaceId, 'ws-1') - logger.info( - 'PASS actor/workspace isolation including existing foreign run and asserted-scope race' - ) - let before = 0 - let after = 0 - for (let i = 0; i < 60; i++) { - const streamId = `race-${i}` - const pair = - i % 2 === 0 - ? Promise.all([ - repo.createRunSegment(input(streamId)), - repo.stopPendingRequest(scope(streamId)), - ]) - : Promise.all([ - repo.stopPendingRequest(scope(streamId)), - repo.createRunSegment(input(streamId)), - ]).then(([stop, run]) => [run, stop] as const) - const [run, stop] = await pair - if (stop === null) { - before++ - assert.equal(run.status, 'cancelled') - } else { - after++ - assert.equal(run.id, stop.id) - assert.equal(run.status, 'active') - } - } - assert.ok(before > 0 && after > 0) - logger.info( - `PASS 60 concurrent Stop/admission races: ${before} cancelled before admission, ${after} returned admitted owner` - ) - await assert.rejects( - repo.stopPendingRequest(scope('bad-foreign-key', { workspaceId: 'missing' })) - ) - const [notRecorded] = - await client`select count(*)::int as count from copilot_request_stops where stream_id = 'bad-foreign-key'` - assert.equal(notRecorded.count, 0) - logger.info('PASS failed durable write rolls back and cannot acknowledge Stop') - } -} finally { - await client.end() - if (admin) { - await admin`drop database ${admin(name)}` - await admin.end() - } -} From 44255714ba70a57e60d752be3715259407947c33 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 5 Oct 2026 22:16:47 -0700 Subject: [PATCH 2/5] 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. --- .../home/hooks/use-chat.dom.test.tsx | 18 +++++++++++ .../[workspaceId]/home/hooks/use-chat.ts | 30 +++++++------------ .../mothership/request/tools/desktop-wait.ts | 2 +- .../mothership/request/tools/executor.test.ts | 26 ++++++++++++++++ .../lib/mothership/request/tools/executor.ts | 21 +++++++++---- .../tools/client/completion.test.ts | 17 +++++++++++ .../lib/mothership/tools/client/completion.ts | 24 +++++++++------ 7 files changed, 104 insertions(+), 34 deletions(-) diff --git a/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.dom.test.tsx b/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.dom.test.tsx index 7213496c82f..69c7d17bca4 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.dom.test.tsx +++ b/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.dom.test.tsx @@ -2246,6 +2246,24 @@ describe('useChat remount send recovery', () => { expect(toolSignal.aborted).toBe(false) }) + it('is still cancelled by Stop after the stream was recovered', async () => { + const { toolSignal, replays, getResult } = await startDesktopAction() + Object.defineProperty(document, 'visibilityState', { + configurable: true, + get: () => 'visible', + }) + await act(async () => { + document.dispatchEvent(new Event('visibilitychange')) + }) + await waitFor(() => replays.length > 0) + + await act(async () => { + await getResult().stopGeneration() + }) + + expect(toolSignal.aborted).toBe(true) + }) + it('is cancelled when the user stops the chat', async () => { const { toolSignal, getResult } = await startDesktopAction() diff --git a/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts b/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts index 657c7c86961..e9fff207819 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts +++ b/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts @@ -456,23 +456,6 @@ export async function waitForDetachedChatResolution( /** The abort reason of the user's Stop. */ const USER_STOP_ABORT_REASON = 'user_stop:client_stopGeneration' -/** - * The lifetime a desktop tool (a browser action, a local file read or import) started from one - * stream observes: only the user's Stop cancels it. Replacing the stream reader (the window - * returning to view, a history reconnect) or leaving the chat view leaves it running, so it - * finishes and reports its own result. - */ -function desktopToolLifetime(streamSignal: AbortSignal | undefined): AbortSignal | undefined { - if (!streamSignal) return undefined - const lifetime = new AbortController() - const followStop = () => { - if (streamSignal.reason === USER_STOP_ABORT_REASON) lifetime.abort(USER_STOP_ABORT_REASON) - } - if (streamSignal.aborted) followStop() - else streamSignal.addEventListener('abort', followStop, { once: true }) - return lifetime.signal -} - /** * Runs a browser tool on the desktop client. The agent's tab reaches the * resource strip through the desktop tab list, so nothing is opened here. @@ -942,6 +925,13 @@ export function useChat( const reconnectExhaustedRecheckTimerRef = useRef | null>(null) const abortControllerRef = useRef(null) + /** + * The lifetime of the desktop tools (browser actions, local file reads and imports) this view + * starts: only the user's Stop ends it. It belongs to no stream reader, so replacing the reader + * (the window returning to view, a history reconnect) or leaving the chat view leaves a running + * tool alone to finish and report, and a Stop still reaches tools a replaced reader started. + */ + const desktopToolStopRef = useRef(null) const detachedChatResolutionControllersRef = useRef | null>(null) const detachedChatResolutionControllers = (detachedChatResolutionControllersRef.current ??= new Set()) @@ -1596,7 +1586,7 @@ export function useChat( const options = { workspaceId, chatId: chatIdRef.current ?? selectedChatIdRef.current, - signal: desktopToolLifetime(abortControllerRef.current?.signal), + signal: (desktopToolStopRef.current ??= new AbortController()).signal, } /** * Dynamic on purpose: the local-filesystem executor only runs for desktop-local @@ -2172,7 +2162,7 @@ export function useChat( shouldContinue?: () => boolean } ) => { - const browserToolSignal = desktopToolLifetime(abortControllerRef.current?.signal) + const browserToolSignal = (desktopToolStopRef.current ??= new AbortController()).signal const activityTracker = getResourceActivityTracker( expectedGen ?? streamGenRef.current, options?.targetChatId @@ -4444,6 +4434,8 @@ export function useChat( ) } clearResourceActivity(stopActivityTracker, true) + desktopToolStopRef.current?.abort(USER_STOP_ABORT_REASON) + desktopToolStopRef.current = null // Establish the stream boundary immediately after synchronous activity // settlement. Native cancellation above is deliberately fire-and-forget, diff --git a/apps/sim/lib/mothership/request/tools/desktop-wait.ts b/apps/sim/lib/mothership/request/tools/desktop-wait.ts index dc484735a01..952a5aa3d4c 100644 --- a/apps/sim/lib/mothership/request/tools/desktop-wait.ts +++ b/apps/sim/lib/mothership/request/tools/desktop-wait.ts @@ -11,7 +11,7 @@ import type { ResolvedSecretTraceRegistry } from '@/executor/utils/resolved-secr const logger = createLogger('CopilotDesktopToolWait') const DESKTOP_TOOL_NOT_STARTED_MESSAGE = - '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.' + '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.' /** The model-facing result of a desktop call that was never picked up. */ export function desktopToolNotStarted(): { message: string; data: Record } { diff --git a/apps/sim/lib/mothership/request/tools/executor.test.ts b/apps/sim/lib/mothership/request/tools/executor.test.ts index b86b155723a..13b4d6c2bc4 100644 --- a/apps/sim/lib/mothership/request/tools/executor.test.ts +++ b/apps/sim/lib/mothership/request/tools/executor.test.ts @@ -940,11 +940,37 @@ describe('watchdog completion provenance', () => { }) }) + it('says a local read the desktop claimed started before its result was lost', async () => { + const { toolCall, context, execContext } = createHungClient() + toolCall.name = 'read_local_file' + toolCall.params = { path: '/Users/me/notes.txt' } + completePendingAsyncToolCall.mockResolvedValueOnce(null) + mothershipAsyncRunsMockFns.mockGetAsyncToolCall.mockResolvedValueOnce({ + toolCallId: toolCall.id, + claimedBy: 'desktop-files', + }) + + await failPendingToolCall(toolCall.id, context, execContext) + + expect(toolCall.result).toEqual({ + success: false, + output: { + error: expect.stringContaining('desktop app started this action'), + outcomeUnknown: true, + doNotRetry: true, + }, + }) + }) + it('does not claim a local read the server never saw picked up had started', async () => { const { toolCall, context, execContext } = createHungClient() toolCall.name = 'read_local_file' toolCall.params = { path: '/Users/me/notes.txt' } completePendingAsyncToolCall.mockResolvedValueOnce(null) + mothershipAsyncRunsMockFns.mockGetAsyncToolCall.mockResolvedValueOnce({ + toolCallId: toolCall.id, + claimedBy: null, + }) await failPendingToolCall(toolCall.id, context, execContext) diff --git a/apps/sim/lib/mothership/request/tools/executor.ts b/apps/sim/lib/mothership/request/tools/executor.ts index bed3f71d834..386c288fd60 100644 --- a/apps/sim/lib/mothership/request/tools/executor.ts +++ b/apps/sim/lib/mothership/request/tools/executor.ts @@ -10,6 +10,7 @@ import type { } from '@/lib/mothership/async-runs/lifecycle' import { type CompleteAsyncToolCallInput, + getAsyncToolCall, markAsyncToolRunning, upsertAsyncToolCall, } from '@/lib/mothership/async-runs/repository' @@ -451,11 +452,7 @@ export async function failPendingToolCall( } const message = failureMessage ?? - (!desktopCall - ? TOOL_RESULT_LOST_MESSAGE - : isLocalReadToolCall(toolCall.execName ?? toolCall.name, toolCall.params) - ? DESKTOP_LOCAL_READ_RESULT_MISSING_MESSAGE - : DESKTOP_TOOL_RESULT_LOST_MESSAGE) + (desktopCall ? await desktopResultLostMessage(toolCall) : TOOL_RESULT_LOST_MESSAGE) await settleAbandonedToolCall(toolCall, context, execContext, { message, data: { error: message, outcomeUnknown: true, doNotRetry: true }, @@ -464,6 +461,20 @@ export async function failPendingToolCall( }) } +/** + * What a desktop call that lost its result tells the model. Only a local read can be in flight + * without a claim (an older desktop reads without claiming), and such a read may never have + * started; anything the desktop claimed did start. + */ +async function desktopResultLostMessage(toolCall: ToolCallState): Promise { + if (!isLocalReadToolCall(toolCall.execName ?? toolCall.name, toolCall.params)) + return DESKTOP_TOOL_RESULT_LOST_MESSAGE + const stored = await getAsyncToolCall(toolCall.id).catch(() => null) + return stored?.claimedBy + ? DESKTOP_TOOL_RESULT_LOST_MESSAGE + : DESKTOP_LOCAL_READ_RESULT_MISSING_MESSAGE +} + /** * Settles one abandoned tool locally once its durable failure is decided. With `unclaimedOnly` * it applies only while the call is still unclaimed and reports whether it did. diff --git a/apps/sim/lib/mothership/tools/client/completion.test.ts b/apps/sim/lib/mothership/tools/client/completion.test.ts index 50237745666..48f1be02cd6 100644 --- a/apps/sim/lib/mothership/tools/client/completion.test.ts +++ b/apps/sim/lib/mothership/tools/client/completion.test.ts @@ -53,6 +53,23 @@ describe('client tool completion reporting', () => { expect(settled).toBe(true) }) + it('stops at once when the trimmed retry of an oversized report learns the call is held', async () => { + vi.useFakeTimers() + fetchMock + .mockResolvedValueOnce(new Response(null, { status: 413 })) + .mockResolvedValueOnce(new Response(null, { status: 409 })) + let settled = false + + void reportClientToolCompletion('tool-1', 'error', 'Failed', { + logs: 'x'.repeat(11 * 1024 * 1024), + }).then(() => { + settled = true + }) + await vi.advanceTimersByTimeAsync(0) + + expect(settled).toBe(true) + }) + it('uses a keepalive request with the exact terminal payload', async () => { await reportClientToolCompletionOnPageExit('tool-1', 'success', 'Browser action completed', { url: 'https://example.com', diff --git a/apps/sim/lib/mothership/tools/client/completion.ts b/apps/sim/lib/mothership/tools/client/completion.ts index 41405232f98..5c97abcad23 100644 --- a/apps/sim/lib/mothership/tools/client/completion.ts +++ b/apps/sim/lib/mothership/tools/client/completion.ts @@ -32,6 +32,19 @@ async function fetchCompletion(input: RequestInfo | URL, init: RequestInit): Pro } } +/** + * Whether a delivery attempt needs no retry: the server took it, or answered 409 because the + * desktop app holds the call and only its own result settles it. + */ +function isSettledDelivery(response: Response, toolCallId: string): boolean { + if (response.ok) return true + if (response.status !== 409) return false + logger.info('Client tool completion was not needed: another reporter holds the call', { + toolCallId, + }) + return true +} + /** * Persist a client-executed tool result and wake the server-side async waiter. * Shared by workflow execution and desktop-native client tools. @@ -71,14 +84,7 @@ export async function reportClientToolCompletion( for (let attempt = 1; attempt <= maxAttempts; attempt++) { try { const response = await send(body) - if (response.ok) return - if (response.status === 409) { - // The desktop app holds this call; its own result settles it, so retrying cannot help. - logger.info('Client tool completion was not needed: another reporter holds the call', { - toolCallId, - }) - return - } + if (isSettledDelivery(response, toolCallId)) return if (isRecordLike(data) && bodySize > largePayloadThreshold) { const { logs: _logs, ...dataWithoutLogs } = data @@ -96,7 +102,7 @@ export async function reportClientToolCompletion( data: dataWithoutLogs, }) ) - if (retryResponse.ok) return + if (isSettledDelivery(retryResponse, toolCallId)) return lastError = new Error(`Completion retry failed with status ${retryResponse.status}`) } else { lastError = new Error(`Completion failed with status ${response.status}`) From 3a9bd010e5456824d6e367cfd2f4b6edf650f626 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 5 Oct 2026 22:39:06 -0700 Subject: [PATCH 3/5] fix(mothership): scope a desktop tool's Stop to its turn A desktop tool outlives the chat view that started it, so its Stop belongs to the turn, not the view: tools are keyed by the turn's stream id, which survives reader replacement and remounts and differs between chats. Stop in another chat leaves them running, and Stop from a view reopened on the turn still reaches them. --- .../home/hooks/desktop-tool-lifetimes.ts | 28 ++++++++++++++++ .../home/hooks/use-chat.dom.test.tsx | 32 ++++++++++++++++++- .../[workspaceId]/home/hooks/use-chat.ts | 20 ++++++------ 3 files changed, 68 insertions(+), 12 deletions(-) create mode 100644 apps/sim/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes.ts diff --git a/apps/sim/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes.ts b/apps/sim/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes.ts new file mode 100644 index 00000000000..4d64eac5281 --- /dev/null +++ b/apps/sim/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes.ts @@ -0,0 +1,28 @@ +import { LRUCache } from 'lru-cache' + +/** + * Turns whose desktop tools may still be running in this tab. A tool outlives the chat view that + * started it (and any stream reader), so the turn, not the view, owns its Stop. Bounded: a tool + * runs for minutes, far fewer turns than this. + */ +const turnStops = new LRUCache({ max: 64 }) + +/** + * The lifetime of a desktop tool (a browser action, a local file read or import) started for a + * turn: only the user's Stop of that turn ends it. Replacing the stream reader, leaving the chat + * view, or stopping another chat's turn leaves it running to finish and report its own result. + */ +export function desktopToolLifetime(streamId: string): AbortSignal { + let stop = turnStops.get(streamId) + if (!stop) { + stop = new AbortController() + turnStops.set(streamId, stop) + } + return stop.signal +} + +/** Cancels the desktop tools of a turn the user stopped, from whichever view started them. */ +export function stopDesktopTools(streamId: string, reason: string): void { + turnStops.get(streamId)?.abort(reason) + turnStops.delete(streamId) +} diff --git a/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.dom.test.tsx b/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.dom.test.tsx index 69c7d17bca4..beedb2284a5 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.dom.test.tsx +++ b/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.dom.test.tsx @@ -2212,7 +2212,7 @@ describe('useChat remount send recovery', () => { const toolSignal = lifetimeOf() if (!(toolSignal instanceof AbortSignal)) throw new Error('The desktop action has no lifetime') - return { ...chat, toolSignal, replays } + return { ...chat, toolSignal, replays, streamId: () => streamId } } beforeEach(() => { @@ -2264,6 +2264,36 @@ describe('useChat remount send recovery', () => { expect(toolSignal.aborted).toBe(true) }) + it('keeps running when the user stops a turn in another chat', async () => { + const { toolSignal, navigate, getResult } = await startDesktopAction() + navigate('chat-other', { ...history, id: 'chat-other' }) + await act(async () => { + void getResult().sendMessage('Something else') + }) + + await act(async () => { + await getResult().stopGeneration() + }) + + expect(toolSignal.aborted).toBe(false) + }) + + it('is still cancelled by Stop from the chat view reopened on its turn', async () => { + const { toolSignal, unmount, streamId } = await startDesktopAction() + unmount() + const reopened = renderUseChatInChat(chatId, { + ...history, + activeStreamId: streamId() ?? null, + }) + await waitFor(() => reopened.getResult().isSending) + + await act(async () => { + await reopened.getResult().stopGeneration() + }) + + expect(toolSignal.aborted).toBe(true) + }) + it('is cancelled when the user stops the chat', async () => { const { toolSignal, getResult } = await startDesktopAction() diff --git a/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts b/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts index e9fff207819..df09a098cbb 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts +++ b/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts @@ -91,6 +91,10 @@ import { isNativeFileTool, isUserLocalVfsToolCall } from '@/lib/mothership/tools import { initTerminalTransport } from '@/lib/terminal/transport' import { getQueryClient } from '@/app/_shell/providers/get-query-client' import { chatUrl } from '@/app/workspace/[workspaceId]/home/hooks/chat-url' +import { + desktopToolLifetime, + stopDesktopTools, +} from '@/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes' import { useFilePreviewController } from '@/app/workspace/[workspaceId]/home/hooks/preview' import { captureResourceActivityScope, @@ -925,13 +929,6 @@ export function useChat( const reconnectExhaustedRecheckTimerRef = useRef | null>(null) const abortControllerRef = useRef(null) - /** - * The lifetime of the desktop tools (browser actions, local file reads and imports) this view - * starts: only the user's Stop ends it. It belongs to no stream reader, so replacing the reader - * (the window returning to view, a history reconnect) or leaving the chat view leaves a running - * tool alone to finish and report, and a Stop still reaches tools a replaced reader started. - */ - const desktopToolStopRef = useRef(null) const detachedChatResolutionControllersRef = useRef | null>(null) const detachedChatResolutionControllers = (detachedChatResolutionControllersRef.current ??= new Set()) @@ -1586,7 +1583,7 @@ export function useChat( const options = { workspaceId, chatId: chatIdRef.current ?? selectedChatIdRef.current, - signal: (desktopToolStopRef.current ??= new AbortController()).signal, + signal: streamIdRef.current ? desktopToolLifetime(streamIdRef.current) : undefined, } /** * Dynamic on purpose: the local-filesystem executor only runs for desktop-local @@ -2162,7 +2159,9 @@ export function useChat( shouldContinue?: () => boolean } ) => { - const browserToolSignal = (desktopToolStopRef.current ??= new AbortController()).signal + const browserToolSignal = streamIdRef.current + ? desktopToolLifetime(streamIdRef.current) + : undefined const activityTracker = getResourceActivityTracker( expectedGen ?? streamGenRef.current, options?.targetChatId @@ -4434,8 +4433,7 @@ export function useChat( ) } clearResourceActivity(stopActivityTracker, true) - desktopToolStopRef.current?.abort(USER_STOP_ABORT_REASON) - desktopToolStopRef.current = null + if (sid) stopDesktopTools(sid, USER_STOP_ABORT_REASON) // Establish the stream boundary immediately after synchronous activity // settlement. Native cancellation above is deliberately fire-and-forget, From d8510e27a9f0f315dccfc080a3a45cfc68e42f18 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 5 Oct 2026 22:52:17 -0700 Subject: [PATCH 4/5] fix(mothership): hold a turn's Stop only while its desktop tools run Replace the bounded LRU of turn controllers with leases: each running browser action or local file tool holds its turn's Stop until it settles, so a live turn can never be evicted and settled turns leave nothing behind. --- .../home/hooks/desktop-tool-lifetimes.test.ts | 54 ++++++++++++ .../home/hooks/desktop-tool-lifetimes.ts | 57 +++++++++---- .../home/hooks/use-chat.dom.test.tsx | 3 + .../[workspaceId]/home/hooks/use-chat.ts | 84 +++++++++++-------- .../tools/client/browser-tool-execution.ts | 11 +-- .../tools/client/local-filesystem.ts | 8 +- 6 files changed, 155 insertions(+), 62 deletions(-) create mode 100644 apps/sim/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes.test.ts diff --git a/apps/sim/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes.test.ts b/apps/sim/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes.test.ts new file mode 100644 index 00000000000..7feb429078f --- /dev/null +++ b/apps/sim/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes.test.ts @@ -0,0 +1,54 @@ +import { describe, expect, it } from 'vitest' +import { leaseDesktopTool, stopDesktopTools } from './desktop-tool-lifetimes' + +describe('desktop tool leases', () => { + it('cancels every running tool of the stopped turn and no other turn', () => { + const first = leaseDesktopTool('turn-a') + const second = leaseDesktopTool('turn-a') + const other = leaseDesktopTool('turn-b') + + stopDesktopTools('turn-a', 'user_stop') + + expect(first.signal.aborted).toBe(true) + expect(second.signal.aborted).toBe(true) + expect(first.signal.reason).toBe('user_stop') + expect(other.signal.aborted).toBe(false) + other.release() + }) + + it('keeps a turn reachable by Stop while any of its tools still runs', () => { + const settled = leaseDesktopTool('turn-c') + const running = leaseDesktopTool('turn-c') + for (let turn = 0; turn < 500; turn++) leaseDesktopTool(`busy-${turn}`).release() + + settled.release() + settled.release() + stopDesktopTools('turn-c', 'user_stop') + + expect(running.signal.aborted).toBe(true) + }) + + it('gives a turn whose tools all settled a fresh lifetime for its next tool', () => { + const settled = leaseDesktopTool('turn-d') + settled.release() + stopDesktopTools('turn-d', 'user_stop') + + const next = leaseDesktopTool('turn-d') + + expect(settled.signal.aborted).toBe(false) + expect(next.signal).not.toBe(settled.signal) + expect(next.signal.aborted).toBe(false) + next.release() + }) + + it('does not let a tool that settles after Stop release a newer lease on the turn', () => { + const stopped = leaseDesktopTool('turn-e') + stopDesktopTools('turn-e', 'user_stop') + const next = leaseDesktopTool('turn-e') + + stopped.release() + stopDesktopTools('turn-e', 'user_stop') + + expect(next.signal.aborted).toBe(true) + }) +}) diff --git a/apps/sim/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes.ts b/apps/sim/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes.ts index 4d64eac5281..8d0d2559de9 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes.ts +++ b/apps/sim/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes.ts @@ -1,28 +1,51 @@ -import { LRUCache } from 'lru-cache' +/** The desktop tools of one turn that are still running in this tab, and the Stop that ends them. */ +interface RunningTurnTools { + stop: AbortController + running: number +} /** - * Turns whose desktop tools may still be running in this tab. A tool outlives the chat view that - * started it (and any stream reader), so the turn, not the view, owns its Stop. Bounded: a tool - * runs for minutes, far fewer turns than this. + * Turns with a desktop tool still running in this tab, keyed by the turn's stream id. A tool + * outlives the chat view that started it (and any stream reader), so the turn, not the view, owns + * its Stop. A turn is held only while one of its tools runs: each tool releases it as it settles. */ -const turnStops = new LRUCache({ max: 64 }) +const runningTurns = new Map() + +/** A running desktop tool's hold on its turn. */ +interface DesktopToolLease { + /** Aborted only by the user's Stop of the turn. */ + signal: AbortSignal + /** Called once when the tool settles. */ + release(): void +} /** - * The lifetime of a desktop tool (a browser action, a local file read or import) started for a - * turn: only the user's Stop of that turn ends it. Replacing the stream reader, leaving the chat - * view, or stopping another chat's turn leaves it running to finish and report its own result. + * Starts a desktop tool (a browser action, a local file read or import) for a turn. Only the + * user's Stop of that turn cancels it: replacing the stream reader, leaving the chat view, or + * stopping another chat's turn leaves it running to finish and report its own result. */ -export function desktopToolLifetime(streamId: string): AbortSignal { - let stop = turnStops.get(streamId) - if (!stop) { - stop = new AbortController() - turnStops.set(streamId, stop) +export function leaseDesktopTool(streamId: string): DesktopToolLease { + let turn = runningTurns.get(streamId) + if (!turn) { + turn = { stop: new AbortController(), running: 0 } + runningTurns.set(streamId, turn) + } + turn.running += 1 + const held = turn + let released = false + return { + signal: held.stop.signal, + release() { + if (released) return + released = true + held.running -= 1 + if (held.running === 0 && runningTurns.get(streamId) === held) runningTurns.delete(streamId) + }, } - return stop.signal } -/** Cancels the desktop tools of a turn the user stopped, from whichever view started them. */ +/** Cancels the running desktop tools of a turn the user stopped, from whichever view started them. */ export function stopDesktopTools(streamId: string, reason: string): void { - turnStops.get(streamId)?.abort(reason) - turnStops.delete(streamId) + runningTurns.get(streamId)?.stop.abort(reason) + runningTurns.delete(streamId) } diff --git a/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.dom.test.tsx b/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.dom.test.tsx index beedb2284a5..affe8a1c15c 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.dom.test.tsx +++ b/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.dom.test.tsx @@ -2217,6 +2217,9 @@ describe('useChat remount send recovery', () => { beforeEach(() => { libDesktopMockFns.mockIsDesktopApp.mockReturnValue(true) + const stillRunning = () => new Promise(() => {}) + mockExecuteBrowserToolOnClient.mockImplementation(stillRunning) + mockExecuteLocalFilesystemTool.mockImplementation(stillRunning) }) afterEach(() => { diff --git a/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts b/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts index df09a098cbb..feb4fde0e4a 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts +++ b/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts @@ -92,7 +92,7 @@ import { initTerminalTransport } from '@/lib/terminal/transport' import { getQueryClient } from '@/app/_shell/providers/get-query-client' import { chatUrl } from '@/app/workspace/[workspaceId]/home/hooks/chat-url' import { - desktopToolLifetime, + leaseDesktopTool, stopDesktopTools, } from '@/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes' import { useFilePreviewController } from '@/app/workspace/[workspaceId]/home/hooks/preview' @@ -472,10 +472,18 @@ function startClientBrowserTool( toolArgs: Record, scopeId: string, eventTs?: string, - signal?: AbortSignal + turnStreamId?: string ): void { if (!isCurrentBrowserToolName(toolName)) return - executeBrowserToolOnClient(toolCallId, toolName, toolArgs, scopeId, eventTs, signal) + const lease = turnStreamId ? leaseDesktopTool(turnStreamId) : undefined + void executeBrowserToolOnClient( + toolCallId, + toolName, + toolArgs, + scopeId, + eventTs, + lease?.signal + ).finally(() => lease?.release()) } /** @@ -1580,10 +1588,11 @@ export function useChat( return } handledClientLocalFilesystemToolIds.add(toolCallId) + const lease = streamIdRef.current ? leaseDesktopTool(streamIdRef.current) : undefined const options = { workspaceId, chatId: chatIdRef.current ?? selectedChatIdRef.current, - signal: streamIdRef.current ? desktopToolLifetime(streamIdRef.current) : undefined, + signal: lease?.signal, } /** * Dynamic on purpose: the local-filesystem executor only runs for desktop-local @@ -1594,35 +1603,40 @@ export function useChat( * report an error completion rather than leaving it hanging with the dedupe ref * already marked handled. */ - import('@/lib/mothership/tools/client/local-filesystem').then( - (m) => m.executeLocalFilesystemTool(toolCallId, toolName, toolArgs, options), - async (error) => { - logger.error('Failed to load local filesystem tool executor', { error }) - /** - * The recovery itself can reject (the helper chunks or the completion POST can - * fail for the same reason the executor chunk did). Contain it: an unhandled - * rejection here would settle nothing and surface as a console error, exactly - * like the executor's own report-failure path, which also degrades to a log. - */ - try { - const [{ reportClientToolCompletion }, { ASYNC_TOOL_CONFIRMATION_STATUS }] = - await Promise.all([ - import('@/lib/mothership/tools/client/completion'), - import('@/lib/mothership/async-runs/lifecycle'), - ]) - await reportClientToolCompletion( - toolCallId, - ASYNC_TOOL_CONFIRMATION_STATUS.error, - 'Local filesystem tool failed to load' - ) - } catch (reportError) { - logger.error('Failed to report local filesystem tool load failure', { - toolCallId, - error: reportError, - }) + import('@/lib/mothership/tools/client/local-filesystem') + .then( + (m) => m.executeLocalFilesystemTool(toolCallId, toolName, toolArgs, options), + async (error) => { + logger.error('Failed to load local filesystem tool executor', { error }) + /** + * The recovery itself can reject (the helper chunks or the completion POST can + * fail for the same reason the executor chunk did). Contain it: an unhandled + * rejection here would settle nothing and surface as a console error, exactly + * like the executor's own report-failure path, which also degrades to a log. + */ + try { + const [{ reportClientToolCompletion }, { ASYNC_TOOL_CONFIRMATION_STATUS }] = + await Promise.all([ + import('@/lib/mothership/tools/client/completion'), + import('@/lib/mothership/async-runs/lifecycle'), + ]) + await reportClientToolCompletion( + toolCallId, + ASYNC_TOOL_CONFIRMATION_STATUS.error, + 'Local filesystem tool failed to load' + ) + } catch (reportError) { + logger.error('Failed to report local filesystem tool load failure', { + toolCallId, + error: reportError, + }) + } } - } - ) + ) + .catch((error) => { + logger.error('Local filesystem tool execution failed unexpectedly', { toolCallId, error }) + }) + .finally(() => lease?.release()) }, [workspaceId, organizationId, scopeKey] ) @@ -2159,9 +2173,7 @@ export function useChat( shouldContinue?: () => boolean } ) => { - const browserToolSignal = streamIdRef.current - ? desktopToolLifetime(streamIdRef.current) - : undefined + const turnStreamId = streamIdRef.current const activityTracker = getResourceActivityTracker( expectedGen ?? streamGenRef.current, options?.targetChatId @@ -2182,7 +2194,7 @@ export function useChat( eventTs?: string ) => { const scopeId = activityScopeId() - startClientBrowserTool(toolCallId, toolName, toolArgs, scopeId, eventTs, browserToolSignal) + startClientBrowserTool(toolCallId, toolName, toolArgs, scopeId, eventTs, turnStreamId) } const startClientTerminalToolForStream = ( toolCallId: string, diff --git a/apps/sim/lib/mothership/tools/client/browser-tool-execution.ts b/apps/sim/lib/mothership/tools/client/browser-tool-execution.ts index eb65edd1e11..e4e1739a571 100644 --- a/apps/sim/lib/mothership/tools/client/browser-tool-execution.ts +++ b/apps/sim/lib/mothership/tools/client/browser-tool-execution.ts @@ -551,20 +551,21 @@ function timeoutForTool(toolName: BrowserToolName, params: Record, scopeId = useBrowserSessionStore.getState().activeScopeId, eventTs?: string, abortSignal?: AbortSignal -): void { +): Promise { if (retryRetainedTerminalCompletion(toolCallId)) { logger.info('Suppressing browser tool while recovering its terminal completion', { toolCallId, @@ -703,7 +704,7 @@ export function executeBrowserToolOnClient( } } runningBrowserToolCalls.add(toolCallId) - void doExecuteBrowserTool( + await doExecuteBrowserTool( toolCallId, toolName, params, diff --git a/apps/sim/lib/mothership/tools/client/local-filesystem.ts b/apps/sim/lib/mothership/tools/client/local-filesystem.ts index 65760975187..a78eb85d19e 100644 --- a/apps/sim/lib/mothership/tools/client/local-filesystem.ts +++ b/apps/sim/lib/mothership/tools/client/local-filesystem.ts @@ -337,17 +337,17 @@ async function execute( return executeUserLocalRead(toolCallId, args, context.signal) } -export function executeLocalFilesystemTool( +export async function executeLocalFilesystemTool( toolCallId: string, toolName: string, args: Record, context: LocalFilesystemExecutionContext -): void { +): Promise { if (isNativeFileTool(toolName)) { - void executeNativeFileTool(toolCallId, toolName, context.signal) + await executeNativeFileTool(toolCallId, toolName, context.signal) return } - void execute(toolCallId, toolName, args, context).then( + await execute(toolCallId, toolName, args, context).then( async (data) => { if (context.signal?.aborted) return try { From b609f821705ebf5c9dcca96076b27df7ae85a1d9 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 5 Oct 2026 23:35:02 -0700 Subject: [PATCH 5/5] test(mothership): import the lease registry by its absolute path --- .../[workspaceId]/home/hooks/desktop-tool-lifetimes.test.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/apps/sim/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes.test.ts b/apps/sim/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes.test.ts index 7feb429078f..07e4f2358c0 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes.test.ts +++ b/apps/sim/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes.test.ts @@ -1,5 +1,8 @@ import { describe, expect, it } from 'vitest' -import { leaseDesktopTool, stopDesktopTools } from './desktop-tool-lifetimes' +import { + leaseDesktopTool, + stopDesktopTools, +} from '@/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes' describe('desktop tool leases', () => { it('cancels every running tool of the stopped turn and no other turn', () => {