Skip to content

Commit 8b93c4d

Browse files
author
Vikhyath Mondreti
committed
fix(computer): stop completion delivery at identity reset
1 parent e30ba7c commit 8b93c4d

4 files changed

Lines changed: 143 additions & 21 deletions

File tree

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

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { createLogger } from '@sim/logger'
22
import { toError } from '@sim/utils/errors'
3-
import { sleep } from '@sim/utils/helpers'
3+
import { interruptibleSleep } from '@sim/utils/helpers'
44
import { isRecordLike } from '@sim/utils/object'
55
import { backoffWithJitter } from '@sim/utils/retry'
66
import type {
@@ -21,12 +21,16 @@ export class CompletionReportError extends Error {
2121
}
2222

2323
async function fetchCompletion(input: RequestInfo | URL, init: RequestInit): Promise<Response> {
24+
init.signal?.throwIfAborted()
2425
const controller = new AbortController()
2526
const timeout = setTimeout(() => {
2627
controller.abort(new DOMException('Completion report timed out', 'TimeoutError'))
2728
}, COMPLETION_REPORT_ATTEMPT_TIMEOUT_MS)
2829
try {
29-
return await fetch(input, { ...init, signal: controller.signal })
30+
return await fetch(input, {
31+
...init,
32+
signal: init.signal ? AbortSignal.any([controller.signal, init.signal]) : controller.signal,
33+
})
3034
} finally {
3135
clearTimeout(timeout)
3236
}
@@ -54,7 +58,8 @@ export async function reportClientToolCompletion(
5458
status: AsyncConfirmationStatus,
5559
message?: string,
5660
data?: AsyncCompletionData,
57-
executionId?: string
61+
executionId?: string,
62+
signal?: AbortSignal
5863
): Promise<void> {
5964
const basePayload = {
6065
toolCallId,
@@ -68,6 +73,7 @@ export async function reportClientToolCompletion(
6873
method: 'POST',
6974
headers: { 'Content-Type': 'application/json', ...traceparentHeader() },
7075
body,
76+
signal,
7177
})
7278

7379
const body = JSON.stringify(basePayload)
@@ -82,6 +88,7 @@ export async function reportClientToolCompletion(
8288
// from retries are discarded server-side.
8389
const maxAttempts = 5
8490
for (let attempt = 1; attempt <= maxAttempts; attempt++) {
91+
signal?.throwIfAborted()
8592
try {
8693
const response = await send(body)
8794
if (isSettledDelivery(response, toolCallId)) return
@@ -108,11 +115,12 @@ export async function reportClientToolCompletion(
108115
lastError = new Error(`Completion failed with status ${response.status}`)
109116
}
110117
} catch (error) {
118+
if (signal?.aborted) throw error
111119
lastError = toError(error)
112120
}
113121

114122
if (attempt < maxAttempts) {
115-
await sleep(backoffWithJitter(attempt, null))
123+
await interruptibleSleep(backoffWithJitter(attempt, null), signal)
116124
}
117125
}
118126

@@ -131,7 +139,8 @@ export async function reportClientToolCompletionOnPageExit(
131139
toolCallId: string,
132140
status: AsyncConfirmationStatus,
133141
message: string,
134-
data?: AsyncCompletionData
142+
data?: AsyncCompletionData,
143+
signal?: AbortSignal
135144
): Promise<void> {
136145
// boundary-raw-fetch: keepalive is required so a terminal desktop result survives page unload
137146
const response = await fetchCompletion(COPILOT_CONFIRM_API_PATH, {
@@ -144,6 +153,7 @@ export async function reportClientToolCompletionOnPageExit(
144153
...(data !== undefined ? { data } : {}),
145154
}),
146155
keepalive: true,
156+
signal,
147157
})
148158
if (!isSettledDelivery(response, toolCallId)) {
149159
throw new CompletionReportError(`Page-exit completion failed with status ${response.status}`)

‎apps/sim/lib/mothership/tools/client/computer-tool-execution.test.ts‎

Lines changed: 26 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,8 @@ describe('computer action delivery', () => {
8282
id,
8383
completion.status,
8484
completion.message,
85-
completion.data
85+
completion.data,
86+
expect.objectContaining({ aborted: false })
8687
)
8788
} finally {
8889
report.resolve()
@@ -146,7 +147,9 @@ describe('computer action delivery', () => {
146147
id,
147148
'error',
148149
expect.stringContaining('may already have run'),
149-
expect.objectContaining({ doNotRetry: true })
150+
expect.objectContaining({ doNotRetry: true }),
151+
undefined,
152+
expect.objectContaining({ aborted: false })
150153
)
151154
})
152155

@@ -167,7 +170,9 @@ describe('computer action delivery', () => {
167170
id,
168171
'cancelled',
169172
expect.stringContaining('timed out'),
170-
{ doNotRetry: true, outcomeUnknown: true }
173+
{ doNotRetry: true, outcomeUnknown: true },
174+
undefined,
175+
expect.objectContaining({ aborted: false })
171176
)
172177
})
173178

@@ -193,7 +198,9 @@ describe('computer action delivery', () => {
193198
dispatchState: 'not_started',
194199
doNotRetry: false,
195200
outcomeUnknown: false,
196-
}
201+
},
202+
undefined,
203+
expect.objectContaining({ aborted: false })
197204
)
198205
await executeComputerToolOnClient(nextId(), { action: 'list_apps' }, now())
199206
expect(mocks.execute).toHaveBeenCalledTimes(2)
@@ -213,7 +220,9 @@ describe('computer action delivery', () => {
213220
id,
214221
'error',
215222
expect.any(String),
216-
expect.objectContaining({ doNotRetry: true, outcomeUnknown: true })
223+
expect.objectContaining({ doNotRetry: true, outcomeUnknown: true }),
224+
undefined,
225+
expect.objectContaining({ aborted: false })
217226
)
218227
})
219228

@@ -234,7 +243,9 @@ describe('computer action delivery', () => {
234243
id,
235244
'error',
236245
expect.stringContaining('may already have run'),
237-
expect.objectContaining({ doNotRetry: true })
246+
expect.objectContaining({ doNotRetry: true }),
247+
undefined,
248+
expect.objectContaining({ aborted: false })
238249
)
239250
})
240251
it.each([undefined, 'invalid', new Date(Date.now() - 121000).toISOString()])(
@@ -310,7 +321,9 @@ describe('computer action delivery', () => {
310321
expect.objectContaining({
311322
sequence: { completedSteps: 1, totalSteps: 2, error: 'Editor focus changed.' },
312323
observation: expect.objectContaining({ snapshotId: 'after' }),
313-
})
324+
}),
325+
undefined,
326+
expect.objectContaining({ aborted: false })
314327
)
315328
})
316329

@@ -338,7 +351,9 @@ describe('computer action delivery', () => {
338351
id,
339352
'cancelled',
340353
expect.any(String),
341-
expect.objectContaining({ doNotRetry: true })
354+
expect.objectContaining({ doNotRetry: true }),
355+
undefined,
356+
expect.objectContaining({ aborted: false })
342357
)
343358
})
344359
it('does not dispatch a tool when Stop already won', async () => {
@@ -402,7 +417,9 @@ describe('computer action delivery', () => {
402417
rejectedId,
403418
'error',
404419
expect.any(String),
405-
expect.anything()
420+
expect.anything(),
421+
undefined,
422+
expect.objectContaining({ aborted: false })
406423
)
407424
})
408425
})

‎apps/sim/lib/mothership/tools/client/computer-tool-execution.ts‎

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import {
1717
} from '@/lib/mothership/tools/client/completion'
1818
import { computerToolResultForModel } from '@/lib/mothership/tools/client/computer-tool-result'
1919
import { compactCompletionForPageExit } from '@/lib/mothership/tools/client/page-exit-completion'
20+
import { registerUserDataReset } from '@/stores/user-data-reset-registry'
2021

2122
const logger = createLogger('ComputerToolExecution')
2223
const MAX_EVENT_AGE_MS = 120_000
@@ -34,13 +35,21 @@ interface Completion {
3435
data?: unknown
3536
}
3637
interface Execution {
38+
identitySignal: AbortSignal
3739
completion?: Completion
3840
reporting?: Promise<void>
3941
retryTimer?: ReturnType<typeof setTimeout>
4042
deliveryAttempts?: number
4143
release?: () => void
4244
}
4345
const executions = new Map<string, Execution>()
46+
let identity = new AbortController()
47+
48+
registerUserDataReset('computer-tool-execution', () => {
49+
identity.abort()
50+
identity = new AbortController()
51+
for (const [toolCallId, execution] of executions) releaseExecution(toolCallId, execution)
52+
})
4453

4554
function releaseExecution(toolCallId: string, execution: Execution): void {
4655
if (executions.get(toolCallId) === execution) executions.delete(toolCallId)
@@ -60,10 +69,13 @@ async function deliver(toolCallId: string, execution: Execution): Promise<void>
6069
toolCallId,
6170
completion.status,
6271
completion.message,
63-
completion.data
72+
completion.data,
73+
undefined,
74+
execution.identitySignal
6475
)
6576
.then(() => releaseExecution(toolCallId, execution))
6677
.catch((error) => {
78+
if (execution.identitySignal.aborted) return
6779
logger.warn('Computer action result delivery failed; retained for redelivery', {
6880
toolCallId,
6981
error: getErrorMessage(error),
@@ -93,7 +105,7 @@ export async function executeComputerToolOnClient(
93105
): Promise<void> {
94106
const existing = executions.get(toolCallId)
95107
if (existing) return deliver(toolCallId, existing)
96-
const execution: Execution = {}
108+
const execution: Execution = { identitySignal: identity.signal }
97109
const claim = replayLedger.claim(toolCallId)
98110
const reject = async (message: string, data?: Record<string, unknown>) => {
99111
const dataWithError = { error: message, ...data }
@@ -111,7 +123,9 @@ export async function executeComputerToolOnClient(
111123
toolCallId,
112124
ASYNC_TOOL_CONFIRMATION_STATUS.error,
113125
message,
114-
dataWithError
126+
dataWithError,
127+
undefined,
128+
execution.identitySignal
115129
).catch((error) =>
116130
logger.warn('Could not report computer action rejection', {
117131
toolCallId,
@@ -145,8 +159,11 @@ export async function executeComputerToolOnClient(
145159
)
146160
executions.set(toolCallId, execution)
147161
const actionController = new AbortController()
162+
const actionSignal = signal
163+
? AbortSignal.any([signal, execution.identitySignal])
164+
: execution.identitySignal
148165
let dispatched = false
149-
let cancelled = signal?.aborted === true
166+
let cancelled = actionSignal.aborted
150167
const cancel = () => {
151168
cancelled = true
152169
actionController.abort()
@@ -173,7 +190,8 @@ export async function executeComputerToolOnClient(
173190
toolCallId,
174191
completion.status,
175192
completion.message,
176-
completion.data
193+
completion.data,
194+
execution.identitySignal
177195
)
178196
.then(() => releaseExecution(toolCallId, execution))
179197
.catch((error) =>
@@ -196,7 +214,7 @@ export async function executeComputerToolOnClient(
196214
}
197215
}
198216
execution.release = () => window.removeEventListener('pagehide', onPageHide)
199-
signal?.addEventListener('abort', cancel, { once: true })
217+
actionSignal.addEventListener('abort', cancel, { once: true })
200218
window.addEventListener('pagehide', onPageHide)
201219
let timer: ReturnType<typeof setTimeout> | undefined
202220
try {
@@ -252,7 +270,7 @@ export async function executeComputerToolOnClient(
252270
}
253271
} finally {
254272
if (timer) clearTimeout(timer)
255-
signal?.removeEventListener('abort', cancel)
273+
actionSignal.removeEventListener('abort', cancel)
256274
}
257275
await deliver(toolCallId, execution)
258276
}

‎apps/sim/lib/mothership/tools/client/computer-tool-unload.test.ts‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import { createDeferred } from '@sim/testing/helpers/deferred'
77
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
88
import { copilotConfirmBodySchema } from '@/lib/api/contracts/copilot'
99
import { executeComputerToolOnClient } from '@/lib/mothership/tools/client/computer-tool-execution'
10+
import { clearUserData } from '@/stores'
1011

1112
const native = vi.hoisted(() => ({ execute: vi.fn(), cancel: vi.fn() }))
1213
vi.mock('@/lib/computer-use/transport', () => ({
@@ -345,4 +346,80 @@ describe('native computer completion during page unload', () => {
345346
await execution
346347
}
347348
})
349+
350+
it.each(['retained', 'backoff', 'request', 'native'] as const)(
351+
'stops old-identity delivery during %s and admits the next identity',
352+
async (phase) => {
353+
const nativeResult = createDeferred<ComputerUseResult>()
354+
const pendingResponse = createDeferred<Response>()
355+
const pendingSignals: AbortSignal[] = []
356+
let signedOut = false
357+
let connected = false
358+
const fetcher = vi.fn<typeof fetch>((_url, init) => {
359+
if (connected) return Promise.resolve(new Response(null, { status: 200 }))
360+
if (phase === 'request') {
361+
const signal = init?.signal
362+
if (!signal) throw new Error('Missing completion abort signal')
363+
pendingSignals.push(signal)
364+
return new Promise<Response>((resolve, reject) => {
365+
signal.addEventListener('abort', () => reject(signal.reason), { once: true })
366+
pendingResponse.promise.then(resolve, reject)
367+
})
368+
}
369+
return Promise.resolve(new Response(null, { status: signedOut ? 401 : 503 }))
370+
})
371+
vi.stubGlobal('fetch', fetcher)
372+
const beacon = vi.fn(() => false)
373+
Object.defineProperty(navigator, 'sendBeacon', { configurable: true, value: beacon })
374+
if (phase === 'native') native.execute.mockReturnValueOnce(nativeResult.promise)
375+
else native.execute.mockResolvedValueOnce({ kind: 'apps', apps: [] })
376+
const id = nextId()
377+
const timestamp = new Date().toISOString()
378+
const execution = executeComputerToolOnClient(id, { action: 'list_apps' }, timestamp)
379+
await flushMicrotasks()
380+
if (phase === 'retained') {
381+
await vi.advanceTimersByTimeAsync(15_000)
382+
await execution
383+
expect(fetcher.mock.calls.length).toBeGreaterThanOrEqual(5)
384+
}
385+
if (phase === 'request') {
386+
window.dispatchEvent(new Event('pagehide'))
387+
await flushMicrotasks()
388+
expect(pendingSignals).toHaveLength(2)
389+
}
390+
const callsBeforeSignOut = fetcher.mock.calls.length
391+
try {
392+
signedOut = true
393+
expect(await clearUserData()).toBe(true)
394+
nativeResult.resolve({ kind: 'apps', apps: [] })
395+
await flushMicrotasks()
396+
expect(vi.getTimerCount()).toBe(0)
397+
if (phase === 'request') expect(pendingSignals.every((signal) => signal.aborted)).toBe(true)
398+
if (phase === 'native') expect(native.cancel).toHaveBeenCalledExactlyOnceWith(id)
399+
beacon.mockClear()
400+
window.dispatchEvent(new Event('pagehide'))
401+
await vi.advanceTimersByTimeAsync(120_000)
402+
expect(fetcher).toHaveBeenCalledTimes(callsBeforeSignOut)
403+
expect(beacon).not.toHaveBeenCalled()
404+
expect(native.execute).toHaveBeenCalledOnce()
405+
connected = true
406+
native.execute.mockResolvedValueOnce({ kind: 'apps', apps: [] })
407+
const next = nextId()
408+
await executeComputerToolOnClient(next, { action: 'list_apps' }, new Date().toISOString())
409+
expect(
410+
copilotConfirmBodySchema.parse(JSON.parse(String(fetcher.mock.lastCall?.[1]?.body)))
411+
).toMatchObject({ toolCallId: next, status: 'success' })
412+
expect(native.execute).toHaveBeenCalledTimes(2)
413+
} finally {
414+
connected = true
415+
nativeResult.resolve({ kind: 'apps', apps: [] })
416+
pendingResponse.resolve(new Response(null, { status: 200 }))
417+
await vi.advanceTimersByTimeAsync(120_000)
418+
await execution
419+
const cleanup = executeComputerToolOnClient(id, { action: 'list_apps' }, timestamp)
420+
await vi.advanceTimersByTimeAsync(15_000)
421+
await cleanup
422+
}
423+
}
424+
)
348425
})

0 commit comments

Comments
 (0)