Skip to content

Commit 0dd29c7

Browse files
committed
fix(desktop): imports arrive whole, survive the rate limit, and record their folders
- The proxy no longer runs for the desktop's raw upload routes (import, and the browser file transfer that has the same shape). Running it made Next buffer the body and cut it off at its 10 MB proxy limit. The import route also requires a declared length and refuses a body that does not match it, so a cut-off file is never stored as complete. - A file entry with no body is refused instead of stored as an empty file. - A name Sim cannot store (one with a backslash) is refused for the whole import before anything lands, and at the contract. - A chunk that runs past the size the manifest listed is refused, like one that ends early. - A rate-limited entry is sent again after the limit clears. A 429 means nothing was stored, so a large tree no longer fails part way. - Folders an import creates are audited as folder creations and announced to the workspace's file views, as folders made by hand are. - Tests record what the fake Sim stored instead of asserting mock calls. The integration suite removes its audit rows.
1 parent 6bd22b0 commit 0dd29c7

11 files changed

Lines changed: 227 additions & 39 deletions

File tree

‎apps/desktop/src/main/desktop-executor/runner.test.ts‎

Lines changed: 44 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { tmpdir } from 'node:os'
33
import { join } from 'node:path'
44
import type { TerminalToolResponse } from '@sim/terminal-protocol'
55
import { afterEach, describe, expect, it, vi } from 'vitest'
6+
import { DeviceRequestError } from '@/main/desktop-executor/client'
67
import type {
78
ClaimedDesktopCall,
89
DesktopImportEntryRequest,
@@ -142,19 +143,27 @@ describe('background imports', () => {
142143
}
143144
}
144145

145-
/** Records what reached Sim, with each file's bytes as text. */
146-
function recordingSim(fail?: (request: DesktopImportEntryRequest) => boolean) {
146+
/**
147+
* A fake Sim import route: records every entry it stores, with each file's bytes as text, and
148+
* the token each request presented. `answer` can refuse a request instead.
149+
*/
150+
function recordingSim(answer?: (request: DesktopImportEntryRequest) => Error | null) {
147151
const stored: Array<{ kind: string; relativePath: string; text?: string }> = []
148-
const importEntry = vi.fn(async (request: DesktopImportEntryRequest) => {
149-
if (fail?.(request)) throw new Error('Sim refused the entry')
152+
const tokens: string[] = []
153+
let requests = 0
154+
const importEntry = async (request: DesktopImportEntryRequest) => {
155+
requests += 1
156+
tokens.push(request.call.executionToken)
157+
const refusal = answer?.(request)
158+
if (refusal) throw refusal
150159
stored.push({
151160
kind: request.kind,
152161
relativePath: request.relativePath,
153162
...(request.content ? { text: await request.content.text() } : {}),
154163
})
155164
return { id: `id-${stored.length}`, name: request.relativePath || request.sourceName }
156-
})
157-
return { stored, importEntry }
165+
}
166+
return { stored, tokens, importEntry, requests: () => requests }
158167
}
159168

160169
it("stores a folder's tree in Sim, each file with the bytes on disk", async () => {
@@ -171,10 +180,7 @@ describe('background imports', () => {
171180
{ kind: 'directory', relativePath: 'q3' },
172181
{ kind: 'file', relativePath: 'q3/summary.txt', text: 'quarterly numbers' },
173182
])
174-
expect(sim.importEntry.mock.calls[0]?.[0]).toMatchObject({
175-
sourceName: 'Reports',
176-
call: { executionToken: 'token-import-1' },
177-
})
183+
expect(new Set(sim.tokens)).toEqual(new Set(['token-import-1']))
178184
expect(completion.data).toMatchObject({
179185
success: true,
180186
workspaceId: 'ws-1',
@@ -190,7 +196,9 @@ describe('background imports', () => {
190196
})
191197

192198
it('reports what landed when an import stops part way, and not to retry it', async () => {
193-
const sim = recordingSim((request) => request.relativePath === 'q3/summary.txt')
199+
const sim = recordingSim((request) =>
200+
request.relativePath === 'q3/summary.txt' ? new Error('Sim refused the entry') : null
201+
)
194202
const completion = await runner({ imports: { importEntry: sim.importEntry } }).run(
195203
importCall(await reportsFolder()),
196204
new AbortController().signal
@@ -209,18 +217,38 @@ describe('background imports', () => {
209217

210218
it('stores nothing more once the call is stopped', async () => {
211219
const controller = new AbortController()
212-
const sim = recordingSim()
213-
sim.importEntry.mockImplementationOnce(async (request) => {
220+
const sim = recordingSim(() => {
214221
controller.abort()
215-
return { id: 'id-root', name: request.sourceName }
222+
return null
216223
})
217224
const completion = await runner({ imports: { importEntry: sim.importEntry } }).run(
218225
importCall(await reportsFolder()),
219226
controller.signal
220227
)
221228

222229
expect(completion.status).toBe('error')
223-
expect(sim.importEntry).toHaveBeenCalledTimes(1)
230+
expect(sim.stored).toEqual([{ kind: 'directory', relativePath: '' }])
231+
})
232+
233+
it("waits out Sim's rate limit instead of failing the import part way", async () => {
234+
let limited = false
235+
const sim = recordingSim((request) => {
236+
if (request.relativePath !== 'notes.txt' || limited) return null
237+
limited = true
238+
return new DeviceRequestError(429, 'Too many requests', 10)
239+
})
240+
const completion = await runner({ imports: { importEntry: sim.importEntry } }).run(
241+
importCall(await reportsFolder()),
242+
new AbortController().signal
243+
)
244+
245+
expect(completion.status).toBe('success')
246+
expect(sim.stored.map((entry) => entry.relativePath)).toEqual([
247+
'',
248+
'notes.txt',
249+
'q3',
250+
'q3/summary.txt',
251+
])
224252
})
225253

226254
it('fails without storing anything when the source cannot be read', async () => {
@@ -231,7 +259,7 @@ describe('background imports', () => {
231259
)
232260

233261
expect(completion.status).toBe('error')
234-
expect(sim.importEntry).not.toHaveBeenCalled()
262+
expect(sim.requests()).toBe(0)
235263
expect(completion.data).toMatchObject({ workspaceId: 'ws-1', partial: false })
236264
})
237265
})

‎apps/desktop/src/main/desktop-executor/runner.ts‎

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,8 @@ import {
4444
import { getErrorMessage } from '@sim/utils/errors'
4545
import { interruptibleSleep } from '@sim/utils/helpers'
4646
import { isRecordLike } from '@sim/utils/object'
47+
import { backoffWithJitter } from '@sim/utils/retry'
48+
import { DeviceRequestError } from '@/main/desktop-executor/client'
4749
import type { DesktopToolRunner } from '@/main/desktop-executor/executor'
4850
import type {
4951
ClaimedDesktopCall,
@@ -54,6 +56,9 @@ import type {
5456
const logger = createLogger('DesktopExecutorRunner')
5557

5658
const USER_LOCAL_TOOLS: ReadonlySet<string> = new Set(['read', 'grep', 'glob'])
59+
/** A rate-limited import entry is retried this many times; Sim never stored a refused one. */
60+
const IMPORT_RATE_LIMIT_ATTEMPTS = 8
61+
const IMPORT_RATE_LIMIT_MAX_WAIT_MS = 30_000
5762

5863
/** The model learns a call never ran because a surface is switched off on this machine. */
5964
function surfaceOff(surface: string): DesktopToolCompletion {
@@ -241,6 +246,34 @@ export function createDesktopToolRunner(deps: DesktopToolRunnerDeps): DesktopToo
241246
* Imports the call's source into its workspace one entry at a time: each directory as a folder,
242247
* each file read in chunks and checked against the manifest that listed it.
243248
*/
249+
/**
250+
* Sends one entry, waiting out Sim's rate limit: a large tree can outrun it, and a 429 means
251+
* nothing was stored, so sending the entry again cannot duplicate it.
252+
*/
253+
async function importEntry(
254+
request: DesktopImportEntryRequest,
255+
signal: AbortSignal
256+
): Promise<DesktopImportedEntry> {
257+
for (let attempt = 1; ; attempt++) {
258+
try {
259+
return await deps.imports.importEntry(request, signal)
260+
} catch (error) {
261+
if (
262+
!(error instanceof DeviceRequestError) ||
263+
error.status !== 429 ||
264+
attempt >= IMPORT_RATE_LIMIT_ATTEMPTS
265+
) {
266+
throw error
267+
}
268+
await interruptibleSleep(
269+
backoffWithJitter(attempt, error.retryAfterMs, { maxMs: IMPORT_RATE_LIMIT_MAX_WAIT_MS }),
270+
signal
271+
)
272+
signal.throwIfAborted()
273+
}
274+
}
275+
}
276+
244277
async function runImport(
245278
call: ClaimedDesktopCall,
246279
signal: AbortSignal
@@ -260,12 +293,12 @@ export function createDesktopToolRunner(deps: DesktopToolRunnerDeps): DesktopToo
260293
signal.throwIfAborted()
261294
const target = { call, sourceName: manifest.name, relativePath: entry.relativePath }
262295
if (entry.kind === 'directory') {
263-
const folder = await deps.imports.importEntry({ ...target, kind: 'directory' }, signal)
296+
const folder = await importEntry({ ...target, kind: 'directory' }, signal)
264297
folders.push({ id: folder.id, relativePath: entry.relativePath })
265298
continue
266299
}
267300
const parts = await readImportEntry(call.toolCallId, entry, read, signal)
268-
const file = await deps.imports.importEntry(
301+
const file = await importEntry(
269302
{ ...target, kind: 'file', content: new Blob(parts) },
270303
signal
271304
)

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

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,10 +40,16 @@ export const PUT = withRouteHandler(async (request: NextRequest) => {
4040
const parsed = await parseRequest(importDesktopEntryContract, request, {})
4141
if (!parsed.success) return parsed.response
4242
const query = parsed.data.query
43-
const declaredLength = Number(request.headers.get('content-length') ?? 0)
43+
const lengthHeader = request.headers.get('content-length')
44+
const declaredLength = Number(lengthHeader ?? 0)
4445
if (!Number.isFinite(declaredLength) || declaredLength > MAX_DESKTOP_IMPORT_FILE_BYTES) {
4546
return NextResponse.json(withRequestId({ error: TOO_LARGE }), { status: 413 })
4647
}
48+
if (query.kind === 'file' && lengthHeader === null) {
49+
return NextResponse.json(withRequestId({ error: 'A file import must declare its length' }), {
50+
status: 411,
51+
})
52+
}
4753

4854
try {
4955
await admitDesktopImportEntry(principal, query)
@@ -65,6 +71,13 @@ export const PUT = withRouteHandler(async (request: NextRequest) => {
6571
}
6672
throw error
6773
}
74+
// Anything between the device and here that cut the body short must not become a stored file.
75+
if (content.length !== declaredLength) {
76+
return NextResponse.json(
77+
withRequestId({ error: 'The file did not arrive whole; nothing was stored' }),
78+
{ status: 400 }
79+
)
80+
}
6881
}
6982

7083
try {

‎apps/sim/lib/api/contracts/desktop-executor.ts‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -183,6 +183,9 @@ export const completeDesktopToolContract = defineRouteContract({
183183
error: z.object({ error: z.string() }),
184184
})
185185

186+
/** Workspace file names cannot hold a backslash, which a macOS or Linux file name can. */
187+
const BACKSLASH_NAME = 'Sim cannot store a file or folder whose name contains a backslash'
188+
186189
/** One relative path inside an import source, as the device's manifest lists it. */
187190
const desktopImportRelativePathSchema = z
188191
.string()
@@ -193,14 +196,21 @@ const desktopImportRelativePathSchema = z
193196
path.split('/').every((segment) => segment !== '' && segment !== '.' && segment !== '..'),
194197
'Relative path must stay inside the import source'
195198
)
199+
.refine((path) => !path.includes('\\'), BACKSLASH_NAME)
196200

197201
const importDesktopEntryQuerySchema = z.object({
198202
deviceId: desktopDeviceIdSchema,
199203
toolCallId: desktopToolCallIdSchema,
200204
executionToken: z.string().min(1).max(128),
201205
kind: z.enum(['file', 'directory']),
202206
/** The import source's own name: the folder a directory import lands in, or the file. */
203-
sourceName: z.string().trim().min(1, 'Source name is required').max(255),
207+
sourceName: z
208+
.string()
209+
.trim()
210+
.min(1, 'Source name is required')
211+
.max(255)
212+
.refine((name) => !name.includes('/'), 'Source name must be a single name')
213+
.refine((name) => !name.includes('\\'), BACKSLASH_NAME),
204214
relativePath: desktopImportRelativePathSchema,
205215
})
206216

‎apps/sim/lib/desktop/application/import.integration.ts‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ vi.mock('@/lib/uploads/core/setup.server', () => ({
2020
import type { SessionPrincipal } from '@sim/auth/principal'
2121
import { db } from '@sim/db'
2222
import {
23+
auditLog,
2324
copilotAsyncToolCalls,
2425
copilotChats,
2526
copilotRuns,
@@ -49,6 +50,8 @@ describe('desktop imports', () => {
4950

5051
afterAll(async () => {
5152
if (userIds.length) {
53+
// Audit rows outlive their actor (the foreign key sets null), so they go first.
54+
await db.delete(auditLog).where(inArray(auditLog.actorId, userIds))
5255
await db.delete(workspace).where(inArray(workspace.ownerId, userIds))
5356
await db.delete(user).where(inArray(user.id, userIds))
5457
}
@@ -185,6 +188,30 @@ describe('desktop imports', () => {
185188
expect(stored).toBeDefined()
186189
})
187190

191+
it('records the folders an import creates, and only those', async () => {
192+
const claimed = await claimedImport()
193+
194+
const root = await entry(claimed, 'directory', '')
195+
await entry(claimed, 'directory', '')
196+
197+
const auditedRoot = async () =>
198+
(
199+
await db
200+
.select({ resourceId: auditLog.resourceId, action: auditLog.action })
201+
.from(auditLog)
202+
.where(eq(auditLog.workspaceId, claimed.workspaceId))
203+
).filter((row) => row.resourceId === root.id)
204+
await expect.poll(auditedRoot).toEqual([{ resourceId: root.id, action: 'folder.created' }])
205+
})
206+
207+
it('refuses a file entry with no bytes instead of storing an empty file', async () => {
208+
const claimed = await claimedImport()
209+
210+
await expect(entry(claimed, 'file', 'empty.txt')).rejects.toMatchObject({
211+
code: 'validation',
212+
})
213+
})
214+
188215
it('merges into folders that already exist and never overwrites a file', async () => {
189216
const claimed = await claimedImport()
190217
const first = await entry(claimed, 'directory', '')

‎apps/sim/lib/desktop/application/import.ts‎

Lines changed: 33 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,12 @@
1+
import { AuditAction, AuditResourceType } from '@sim/audit'
12
import type { Principal } from '@sim/auth/principal'
23
import { toRecord } from '@sim/utils/object'
34
import { OrchestrationError } from '@/lib/core/orchestration/types'
45
import { DesktopDeviceUnrecognizedError } from '@/lib/desktop/executor/errors'
56
import { getBoundDesktopCall, getBoundDesktopDevice } from '@/lib/desktop/executor/repository'
67
import { ASYNC_TOOL_STATUS } from '@/lib/mothership/async-runs/lifecycle'
78
import { getAsyncToolCall } from '@/lib/mothership/async-runs/repository'
9+
import { notifyWorkspaceFilesChanged } from '@/lib/realtime/notify'
810
import {
911
ensureWorkspaceFileChildFolder,
1012
loadActiveWorkspaceContext,
@@ -105,21 +107,25 @@ export const importDesktopEntry = defineAuthorizedWorkspaceFileUseCase({
105107
const segments = input.relativePath
106108
? [input.sourceName, ...input.relativePath.split('/')]
107109
: [input.sourceName]
108-
const folders = input.kind === 'directory' ? segments : segments.slice(0, -1)
110+
const folderSegments = input.kind === 'directory' ? segments : segments.slice(0, -1)
109111
let folderId = context.rootFolderId
110-
for (const name of folders) {
111-
folderId = await ensureWorkspaceFileChildFolder({
112+
const createdFolders: Array<{ id: string; name: string }> = []
113+
for (const name of folderSegments) {
114+
const folder = await ensureWorkspaceFileChildFolder({
112115
workspaceId: context.workspaceId,
113116
userId: context.importingUserId,
114117
parentId: folderId,
115118
name,
116119
})
120+
if (folder.created) createdFolders.push({ id: folder.id, name: folder.name })
121+
folderId = folder.id
117122
}
118123
const name = segments[segments.length - 1] ?? input.sourceName
119124
if (input.kind === 'directory') {
120125
if (!folderId) throw new OrchestrationError('validation', 'A directory needs a name')
121-
return { kind: 'directory' as const, id: folderId, name }
126+
return { kind: 'directory' as const, id: folderId, name, createdFolders }
122127
}
128+
if (!input.content) throw new OrchestrationError('validation', 'A file import needs its bytes')
123129
const created = await createAuthorizedWorkspaceFile({
124130
principal,
125131
input: {
@@ -129,11 +135,30 @@ export const importDesktopEntry = defineAuthorizedWorkspaceFileUseCase({
129135
folderId,
130136
exactName: false,
131137
},
132-
content: input.content ?? Buffer.alloc(0),
138+
content: input.content,
133139
workspace: context,
134140
})
135-
return { kind: 'file' as const, id: created.file.id, name: created.file.name, created }
141+
return {
142+
kind: 'file' as const,
143+
id: created.file.id,
144+
name: created.file.name,
145+
createdFolders,
146+
created,
147+
}
148+
},
149+
// Folders an import creates are recorded like folders made by hand; the file records itself.
150+
projectAudit: ({ result }) => [
151+
...result.createdFolders.map((folder) => ({
152+
action: AuditAction.FOLDER_CREATED,
153+
resourceType: AuditResourceType.FOLDER,
154+
resourceId: folder.id,
155+
resourceName: folder.name,
156+
description: `Created file folder "${folder.name}"`,
157+
})),
158+
...(result.kind === 'file' ? [projectCreateWorkspaceFileAudit(result.created)] : []),
159+
],
160+
async afterSuccess({ context, result }) {
161+
// A new file announces itself; a new folder is announced here, as folder creation does.
162+
if (result.createdFolders.length > 0) await notifyWorkspaceFilesChanged(context.workspaceId)
136163
},
137-
projectAudit: ({ result }) =>
138-
result.kind === 'file' ? projectCreateWorkspaceFileAudit(result.created) : [],
139164
})

0 commit comments

Comments
 (0)