Skip to content

Commit d7bfc58

Browse files
committed
fix(desktop): cap a renewed import's wait at the client tool limit, renew at once, and report the extended wait
- A chat-view import's lease extends its wait only up to the cap every client tool has (CLIENT_TOOL_RESULT_TIMEOUT_MS), so an import that hangs with its page alive still settles - The page renews the lease as soon as the import starts, then every heartbeat - The force-fail log names an extended wait and how long it lasted; the wait span's budget includes the extension - Tests for the cap, the bound on failed lease lookups, and the client heartbeat
1 parent 313fe15 commit d7bfc58

6 files changed

Lines changed: 159 additions & 27 deletions

File tree

‎apps/sim/app/api/desktop/tool/authorize/route.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ import {
2121
chatViewDesktopLeaseOwnerToken,
2222
getDesktopToolClaimOwner,
2323
isDesktopToolCall,
24-
isLeasedChatViewDesktopTool,
2524
isLocalReadToolCall,
2625
} from '@/lib/mothership/tools/desktop-tools'
2726

@@ -116,13 +115,14 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
116115
{ status: 409 }
117116
)
118117
if (toolCall.status !== 'pending') return alreadyStarted()
119-
// The import's lease is held by this session; the chat view renews it while the import runs.
118+
// An import runs as long as its files take, so its claim takes a lease this session holds:
119+
// the chat view renews it while the import runs. Reads finish in seconds and take none.
120120
const { outcome } = await claimDesktopToolCall({
121121
toolCallId: toolCall.toolCallId,
122122
runId: toolCall.runId,
123123
userId,
124124
claimedBy: DESKTOP_TOOL_CLAIM_OWNER.files,
125-
...(principal && isLeasedChatViewDesktopTool(toolCall.toolName)
125+
...(principal
126126
? { chatView: { ownerToken: chatViewDesktopLeaseOwnerToken(principal.sessionId) } }
127127
: {}),
128128
})

‎apps/sim/lib/mothership/request/lifecycle/run.test.ts‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -221,6 +221,7 @@ vi.mock('@/lib/mothership/request/enterprise-byok', () => ({
221221

222222
import { resetUsageGateCache } from '@/lib/billing/core/usage-gate-cache'
223223
import { buildPersistedAssistantMessage } from '@/lib/mothership/chat/persisted-message'
224+
import { CLIENT_TOOL_RESULT_TIMEOUT_MS } from '@/lib/mothership/constants'
224225
import {
225226
MothershipStreamV1CompletionStatus,
226227
MothershipStreamV1ToolOutcome,
@@ -3619,6 +3620,7 @@ describe('runCopilotLifecycle', () => {
36193620
expect((await turn.lifecycle).success).toBe(true)
36203621
expect(turn.resumedWithLostImport()).toBe(true)
36213622
} finally {
3623+
mothershipAsyncRunsMockFns.mockGetChatViewDesktopLeaseRemainingMs.mockReset()
36223624
vi.useRealTimers()
36233625
}
36243626
})
@@ -3639,6 +3641,43 @@ describe('runCopilotLifecycle', () => {
36393641
expect((await turn.lifecycle).success).toBe(true)
36403642
expect(turn.resumedWithLostImport()).toBe(true)
36413643
} finally {
3644+
mothershipAsyncRunsMockFns.mockGetChatViewDesktopLeaseRemainingMs.mockReset()
3645+
vi.useRealTimers()
3646+
}
3647+
})
3648+
3649+
it('gives up a renewed import at the client result cap, however long the renewals go on', async () => {
3650+
vi.useFakeTimers()
3651+
try {
3652+
// The page stays alive and renews, but the import itself never finishes.
3653+
mothershipAsyncRunsMockFns.mockGetChatViewDesktopLeaseRemainingMs.mockResolvedValue(50_000)
3654+
const turn = runImportTurn()
3655+
await vi.advanceTimersByTimeAsync(CLIENT_TOOL_RESULT_TIMEOUT_MS - 1_000)
3656+
expect(turn.bodies).toHaveLength(1)
3657+
await vi.advanceTimersByTimeAsync(2_000)
3658+
expect((await turn.lifecycle).success).toBe(true)
3659+
expect(turn.resumedWithLostImport()).toBe(true)
3660+
} finally {
3661+
mothershipAsyncRunsMockFns.mockGetChatViewDesktopLeaseRemainingMs.mockReset()
3662+
vi.useRealTimers()
3663+
}
3664+
})
3665+
3666+
it('gives up an import whose lease cannot be read for a whole lease', async () => {
3667+
vi.useFakeTimers()
3668+
try {
3669+
mothershipAsyncRunsMockFns.mockGetChatViewDesktopLeaseRemainingMs.mockRejectedValue(
3670+
new Error('database unavailable')
3671+
)
3672+
const turn = runImportTurn()
3673+
// The first failed lookup comes at the default budget (90 s); retries go on for one lease.
3674+
await vi.advanceTimersByTimeAsync(145_000)
3675+
expect(turn.bodies).toHaveLength(1)
3676+
await vi.advanceTimersByTimeAsync(10_000)
3677+
expect((await turn.lifecycle).success).toBe(true)
3678+
expect(turn.resumedWithLostImport()).toBe(true)
3679+
} finally {
3680+
mothershipAsyncRunsMockFns.mockGetChatViewDesktopLeaseRemainingMs.mockReset()
36423681
vi.useRealTimers()
36433682
}
36443683
})

‎apps/sim/lib/mothership/request/lifecycle/run.ts‎

Lines changed: 30 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,10 @@ import {
2222
getChatViewDesktopLeaseRemainingMs,
2323
updateRunStatus,
2424
} from '@/lib/mothership/async-runs/repository'
25-
import { TOOL_WATCHDOG_RESUME_GRACE_MS } from '@/lib/mothership/constants'
25+
import {
26+
CLIENT_TOOL_RESULT_TIMEOUT_MS,
27+
TOOL_WATCHDOG_RESUME_GRACE_MS,
28+
} from '@/lib/mothership/constants'
2629
import {
2730
type CopilotEnvironmentContext,
2831
prepareCopilotEnvironmentContext,
@@ -1371,6 +1374,12 @@ async function runCheckpointLoop(
13711374
}>
13721375
deadlineAt: number
13731376
waitBudgetMs: number
1377+
/** When this wait began. */
1378+
startedAt: number
1379+
/** Past this, the call is given up however its lease stands: any client tool's cap. */
1380+
ceilingAt: number
1381+
/** Why the deadline was moved past the plain budget, if it was. */
1382+
extendedBy?: 'lease' | 'lease_lookup'
13741383
/** Until when a failing lease lookup is retried before the call is given up. */
13751384
leaseLookupRetryUntil?: number
13761385
}
@@ -1403,6 +1412,8 @@ async function runCheckpointLoop(
14031412
),
14041413
deadlineAt: now + waitBudgetMs,
14051414
waitBudgetMs,
1415+
startedAt: now,
1416+
ceilingAt: now + Math.max(waitBudgetMs, CLIENT_TOOL_RESULT_TIMEOUT_MS),
14061417
})
14071418
maximumWaitBudgetMs = Math.max(maximumWaitBudgetMs, waitBudgetMs)
14081419
}
@@ -1426,30 +1437,43 @@ async function runCheckpointLoop(
14261437
const expiredTools = overdueTools.filter(([toolCallId, watchdog], index) => {
14271438
const lease = leases[index]
14281439
const checkedAt = Date.now()
1440+
if (checkedAt >= watchdog.ceilingAt) return true
1441+
const extendTo = (deadlineAt: number, reason: 'lease' | 'lease_lookup') => {
1442+
watchdog.deadlineAt = Math.min(deadlineAt, watchdog.ceilingAt)
1443+
watchdog.extendedBy = reason
1444+
maximumWaitBudgetMs = Math.max(
1445+
maximumWaitBudgetMs,
1446+
watchdog.deadlineAt - watchdog.startedAt
1447+
)
1448+
return false
1449+
}
14291450
if ('error' in lease) {
14301451
watchdog.leaseLookupRetryUntil ??= checkedAt + SIM_TOOL_EXECUTION_LEASE_SECONDS * 1000
14311452
if (checkedAt >= watchdog.leaseLookupRetryUntil) return true
14321453
logger.warn('Could not read a pending tool call lease; checking again', {
14331454
toolCallId,
14341455
error: getErrorMessage(lease.error),
14351456
})
1436-
watchdog.deadlineAt = checkedAt + LEASE_LOOKUP_RETRY_MS
1437-
return false
1457+
return extendTo(checkedAt + LEASE_LOOKUP_RETRY_MS, 'lease_lookup')
14381458
}
14391459
watchdog.leaseLookupRetryUntil = undefined
14401460
if (typeof lease.remainingMs !== 'number' || lease.remainingMs <= 0) return true
1441-
watchdog.deadlineAt = checkedAt + lease.remainingMs + LEASE_RECHECK_SLACK_MS
1442-
return false
1461+
return extendTo(checkedAt + lease.remainingMs + LEASE_RECHECK_SLACK_MS, 'lease')
14431462
})
14441463
if (expiredTools.length > 0) {
14451464
await Promise.all(
14461465
expiredTools.map(async ([toolCallId, watchdog]) => {
1466+
const waitedMs = Date.now() - watchdog.startedAt
14471467
logger.error(
1448-
'Pending tool execution exceeded its resume wait budget; force-failing',
1468+
watchdog.extendedBy
1469+
? 'Pending tool execution outlived its renewed lease or its cap; force-failing'
1470+
: 'Pending tool execution exceeded its resume wait budget; force-failing',
14491471
{
14501472
checkpointId: continuation.checkpointId,
14511473
toolCallId,
14521474
waitBudgetMs: watchdog.waitBudgetMs,
1475+
waitedMs,
1476+
...(watchdog.extendedBy ? { extendedBy: watchdog.extendedBy } : {}),
14531477
}
14541478
)
14551479
await failPendingToolCall(toolCallId, context, execContext)

‎apps/sim/lib/mothership/tools/client/native-files.test.ts‎

Lines changed: 70 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import {
44
apiClientRequestMockFns,
55
} from '@sim/testing/mocks/api-client-request.mock'
66
import { libDesktopMock, libDesktopMockFns } from '@sim/testing/mocks/lib-desktop.mock'
7-
import { beforeEach, expect, it, vi } from 'vitest'
7+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
88

99
const hoisted = vi.hoisted(() => ({
1010
invoke: vi.fn(),
@@ -23,6 +23,8 @@ vi.mock('@/lib/mothership/tools/client/completion', () => ({
2323
}))
2424

2525
import type { DesktopLocalFileManifest } from '@sim/desktop-bridge'
26+
import { ApiClientError } from '@/lib/api/client/errors'
27+
import { renewDesktopToolLeaseContract } from '@/lib/api/contracts/desktop-executor'
2628
import {
2729
executeNativeFileTool,
2830
importNativeFiles,
@@ -150,3 +152,70 @@ it('a read the user stopped while the desktop was reading reports nothing', asyn
150152
await executeNativeFileTool('tool', 'read_local_file', stop.signal)
151153
expect(mocks.complete).not.toHaveBeenCalled()
152154
})
155+
156+
describe('an import keeps its lease while it runs', () => {
157+
/** The import's upload, which the test finishes when it chooses. */
158+
let finishUpload: () => void
159+
/** What the server answers each lease renewal, in turn. */
160+
let renewals: Array<() => Promise<unknown>>
161+
const renewalsSent = () =>
162+
mocks.json.mock.calls.filter(([contract]) => contract === renewDesktopToolLeaseContract).length
163+
164+
beforeEach(() => {
165+
vi.useFakeTimers()
166+
renewals = []
167+
mocks.invoke.mockImplementation(async (request: { operation: string }) =>
168+
request.operation === 'manifest'
169+
? { ok: true, data: manifest }
170+
: { ok: true, data: { kind: 'chunk', bytes: new Uint8Array([65, 66, 67]), eof: true } }
171+
)
172+
mocks.upload.mockImplementation(
173+
() =>
174+
new Promise((resolve) => {
175+
finishUpload = () => resolve({ id: 'saved-file', name: 'report.txt' })
176+
})
177+
)
178+
mocks.json.mockImplementation(async (contract: unknown) => {
179+
if (contract !== renewDesktopToolLeaseContract) return { folder: { id: 'created-folder' } }
180+
const answer = renewals.shift()
181+
return answer ? answer() : { renewed: true }
182+
})
183+
})
184+
185+
afterEach(() => {
186+
vi.useRealTimers()
187+
})
188+
189+
const refused = (status: number) => () =>
190+
Promise.reject(new ApiClientError({ message: `HTTP ${status}`, status, body: {} }))
191+
192+
it('renews at once, then every heartbeat, and stops when the import ends', async () => {
193+
const run = executeNativeFileTool('tool', 'import_local_files')
194+
await vi.advanceTimersByTimeAsync(0)
195+
expect(renewalsSent()).toBe(1)
196+
await vi.advanceTimersByTimeAsync(40_000)
197+
expect(renewalsSent()).toBe(3)
198+
finishUpload()
199+
await run
200+
await vi.advanceTimersByTimeAsync(60_000)
201+
expect(renewalsSent()).toBe(3)
202+
})
203+
204+
it('stops renewing once the server refuses the call', async () => {
205+
renewals.push(refused(410))
206+
const run = executeNativeFileTool('tool', 'import_local_files')
207+
await vi.advanceTimersByTimeAsync(60_000)
208+
expect(renewalsSent()).toBe(1)
209+
finishUpload()
210+
await run
211+
})
212+
213+
it('keeps renewing through failures that may pass', async () => {
214+
renewals.push(refused(401), refused(429), refused(503))
215+
const run = executeNativeFileTool('tool', 'import_local_files')
216+
await vi.advanceTimersByTimeAsync(60_000)
217+
expect(renewalsSent()).toBe(4)
218+
finishUpload()
219+
await run
220+
})
221+
})

‎apps/sim/lib/mothership/tools/client/native-files.ts‎

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -112,17 +112,20 @@ export async function importNativeFiles(
112112
}
113113

114114
/**
115-
* Renews an import's lease every heartbeat until stopped, or until the server refuses it (the call
116-
* was stopped, settled, or its lease already lapsed), the way a desktop renews a bound call.
115+
* Renews an import's lease at once and then every heartbeat, until stopped or until the server
116+
* refuses it (410: the call was stopped, settled, or its lease already lapsed), the way a desktop
117+
* renews a bound call. The first renewal comes right after the claim, so a slow start cannot
118+
* outlast the lease the claim took.
117119
*/
118120
function keepImportLeased(toolCallId: string): { stop(): void } {
119-
const timer = setInterval(() => {
121+
let stopped = false
122+
const renew = () => {
123+
if (stopped) return
120124
requestJson(renewDesktopToolLeaseContract, { body: { toolCallId, chatView: true } }).catch(
121125
(error) => {
122-
// 410: the server refuses the call (stopped, settled, or its lease lapsed), so stop. Any
123-
// other failure may pass: keep renewing, as the lease outlasts a couple of missed beats.
126+
// Any other failure may pass: keep renewing, as the lease outlasts a couple of missed beats.
124127
if (error instanceof ApiClientError && error.status === 410) {
125-
clearInterval(timer)
128+
stop()
126129
return
127130
}
128131
logger.warn('Could not renew the import lease; trying again next beat', {
@@ -131,8 +134,14 @@ function keepImportLeased(toolCallId: string): { stop(): void } {
131134
})
132135
}
133136
)
134-
}, SIM_TOOL_EXECUTION_HEARTBEAT_MS)
135-
return { stop: () => clearInterval(timer) }
137+
}
138+
const timer = setInterval(renew, SIM_TOOL_EXECUTION_HEARTBEAT_MS)
139+
const stop = () => {
140+
stopped = true
141+
clearInterval(timer)
142+
}
143+
renew()
144+
return { stop }
136145
}
137146

138147
/** The server claims imports before reading their manifest, preventing replayed uploads. */

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

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -95,12 +95,3 @@ export const STOPPED_WHILE_RUNNING_MESSAGE =
9595
export function chatViewDesktopLeaseOwnerToken(sessionId: string): string {
9696
return `chat-view:${sessionId}`
9797
}
98-
99-
/**
100-
* Desktop calls the chat view keeps alive with a renewed lease while they run: imports, which
101-
* take as long as their files do (up to 1,000 entries of up to 64 MB each). Reads finish in
102-
* seconds and keep the default budget.
103-
*/
104-
export function isLeasedChatViewDesktopTool(toolName: string): boolean {
105-
return toolName === 'import_local_files'
106-
}

0 commit comments

Comments
 (0)