Skip to content

Commit d0c6269

Browse files
committed
fix(uploads): idempotent register skips duplicate audit/posthog; add edge-case tests
- registerUploadedWorkspaceFile now returns { file, created } so the route can skip captureServerEvent and recordAudit on idempotent re-register (existing metadata reused). Previously a re-register fired duplicate analytics + audit log entries. - Add tests covering: idempotent re-register skips audit/analytics, isNetworkError matches econnreset/timeout/etc keywords, multipart complete failure fires action=abort cleanup.
1 parent 13fcf40 commit d0c6269

5 files changed

Lines changed: 160 additions & 36 deletions

File tree

apps/sim/app/api/workspaces/[id]/files/register/route.test.ts

Lines changed: 31 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
*/
66
import {
77
auditMock,
8+
auditMockFns,
89
authMockFns,
910
permissionsMock,
1011
permissionsMockFns,
@@ -72,13 +73,16 @@ describe('POST /api/workspaces/[id]/files/register', () => {
7273
return match ? match[1] : null
7374
})
7475
mockRegisterUploadedWorkspaceFile.mockResolvedValue({
75-
id: 'wf_123',
76-
name: 'video.mp4',
77-
size: 10 * 1024 * 1024,
78-
type: 'video/mp4',
79-
url: '/api/files/serve/...',
80-
key: VALID_KEY,
81-
context: 'workspace',
76+
file: {
77+
id: 'wf_123',
78+
name: 'video.mp4',
79+
size: 10 * 1024 * 1024,
80+
type: 'video/mp4',
81+
url: '/api/files/serve/...',
82+
key: VALID_KEY,
83+
context: 'workspace',
84+
},
85+
created: true,
8286
})
8387
})
8488

@@ -134,6 +138,26 @@ describe('POST /api/workspaces/[id]/files/register', () => {
134138
expect(body.isDuplicate).toBe(true)
135139
})
136140

141+
it('skips audit + analytics on idempotent re-register (created=false)', async () => {
142+
mockRegisterUploadedWorkspaceFile.mockResolvedValueOnce({
143+
file: {
144+
id: 'wf_123',
145+
name: 'video.mp4',
146+
size: 10 * 1024 * 1024,
147+
type: 'video/mp4',
148+
url: '/api/files/serve/...',
149+
key: VALID_KEY,
150+
context: 'workspace',
151+
},
152+
created: false,
153+
})
154+
155+
const res = await POST(makeRequest(validBody), params())
156+
expect(res.status).toBe(200)
157+
expect(posthogServerMockFns.mockCaptureServerEvent).not.toHaveBeenCalled()
158+
expect(auditMockFns.mockRecordAudit).not.toHaveBeenCalled()
159+
})
160+
137161
it('finalizes upload, records audit and analytics', async () => {
138162
const res = await POST(makeRequest(validBody), params())
139163
const body = await res.json()

apps/sim/app/api/workspaces/[id]/files/register/route.ts

Lines changed: 25 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ export const POST = withRouteHandler(
5050
}
5151

5252
try {
53-
const userFile = await registerUploadedWorkspaceFile({
53+
const { file: userFile, created } = await registerUploadedWorkspaceFile({
5454
workspaceId,
5555
userId,
5656
key,
@@ -59,28 +59,32 @@ export const POST = withRouteHandler(
5959
size,
6060
})
6161

62-
logger.info(`Registered direct upload ${name} -> ${key}`)
62+
if (created) {
63+
logger.info(`Registered direct upload ${name} -> ${key}`)
6364

64-
captureServerEvent(
65-
userId,
66-
'file_uploaded',
67-
{ workspace_id: workspaceId, file_type: contentType },
68-
{ groups: { workspace: workspaceId } }
69-
)
65+
captureServerEvent(
66+
userId,
67+
'file_uploaded',
68+
{ workspace_id: workspaceId, file_type: contentType },
69+
{ groups: { workspace: workspaceId } }
70+
)
7071

71-
recordAudit({
72-
workspaceId,
73-
actorId: userId,
74-
actorName: session.user.name,
75-
actorEmail: session.user.email,
76-
action: AuditAction.FILE_UPLOADED,
77-
resourceType: AuditResourceType.FILE,
78-
resourceId: userFile.id,
79-
resourceName: name,
80-
description: `Uploaded file "${name}"`,
81-
metadata: { fileSize: size, fileType: contentType },
82-
request,
83-
})
72+
recordAudit({
73+
workspaceId,
74+
actorId: userId,
75+
actorName: session.user.name,
76+
actorEmail: session.user.email,
77+
action: AuditAction.FILE_UPLOADED,
78+
resourceType: AuditResourceType.FILE,
79+
resourceId: userFile.id,
80+
resourceName: name,
81+
description: `Uploaded file "${name}"`,
82+
metadata: { fileSize: size, fileType: contentType },
83+
request,
84+
})
85+
} else {
86+
logger.info(`Idempotent re-register for existing upload ${name} -> ${key}`)
87+
}
8488

8589
return NextResponse.json({ success: true, file: userFile })
8690
} catch (error) {

apps/sim/lib/uploads/client/direct-upload.test.ts

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -175,6 +175,52 @@ describe('runUploadStrategy', () => {
175175
).rejects.toMatchObject({ name: 'DirectUploadError', code: 'ABORTED' })
176176
})
177177

178+
it('fires action=abort when the multipart complete call fails', async () => {
179+
const file = makeFile(LARGE_THRESHOLD + ONE_MB)
180+
const calls: Array<{ url: string }> = []
181+
182+
const fetchMock = vi.fn(async (input: RequestInfo | URL) => {
183+
const url = typeof input === 'string' ? input : input.toString()
184+
calls.push({ url })
185+
186+
if (url.includes('action=initiate')) {
187+
return new Response(
188+
JSON.stringify({ uploadId: 'u1', key: 'workspace/ws-1/big.bin', uploadToken: 't' }),
189+
{ status: 200 }
190+
)
191+
}
192+
if (url.includes('action=get-part-urls')) {
193+
return new Response(
194+
JSON.stringify({
195+
presignedUrls: [
196+
{ partNumber: 1, url: 'https://s3/part1' },
197+
{ partNumber: 2, url: 'https://s3/part2' },
198+
],
199+
}),
200+
{ status: 200 }
201+
)
202+
}
203+
if (url.startsWith('https://s3/part')) {
204+
return new Response(null, { status: 200, headers: { ETag: '"etag-x"' } })
205+
}
206+
if (url.includes('action=complete')) {
207+
return new Response(JSON.stringify({ error: 'kaboom' }), { status: 500 })
208+
}
209+
if (url.includes('action=abort')) {
210+
return new Response(null, { status: 200 })
211+
}
212+
throw new Error(`unexpected url ${url}`)
213+
})
214+
215+
vi.stubGlobal('fetch', fetchMock)
216+
217+
await expect(
218+
runUploadStrategy({ file, workspaceId: 'ws-1', context: 'workspace' })
219+
).rejects.toBeInstanceOf(DirectUploadError)
220+
221+
expect(calls.some((c) => c.url.includes('action=abort'))).toBe(true)
222+
})
223+
178224
it('throws when neither presignedEndpoint nor presignedOverride is supplied', async () => {
179225
const file = makeFile(ONE_MB)
180226
await expect(

apps/sim/lib/uploads/contexts/workspace/workspace-file-manager.ts

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -278,6 +278,12 @@ export async function uploadWorkspaceFile(
278278
* Throws if the object is missing in storage, quota is exceeded, or the
279279
* caller cannot resolve a unique name within the retry budget.
280280
*/
281+
export interface RegisterUploadedWorkspaceFileResult {
282+
file: UserFile
283+
/** True when a new metadata row was inserted; false when an existing row was reused. */
284+
created: boolean
285+
}
286+
281287
export async function registerUploadedWorkspaceFile(params: {
282288
workspaceId: string
283289
userId: string
@@ -286,7 +292,7 @@ export async function registerUploadedWorkspaceFile(params: {
286292
contentType: string
287293
/** Caller-supplied size; verified against storage HEAD when cloud storage is configured. */
288294
size: number
289-
}): Promise<UserFile> {
295+
}): Promise<RegisterUploadedWorkspaceFileResult> {
290296
const { workspaceId, userId, key, originalName, contentType, size } = params
291297

292298
if (parseWorkspaceFileKey(key) !== workspaceId) {
@@ -324,12 +330,14 @@ export async function registerUploadedWorkspaceFile(params: {
324330

325331
let fileId = `wf_${generateShortId()}`
326332
let displayName: string
333+
let created = false
327334
const existing = await getFileMetadataByKey(key, 'workspace')
328335
if (existing) {
329336
fileId = existing.id
330337
displayName = existing.originalName
331338
logger.info(`Using existing metadata record for direct upload: ${key}`)
332339
} else {
340+
created = true
333341
displayName = await allocateUniqueWorkspaceFileName(workspaceId, originalName)
334342
try {
335343
await insertFileMetadata({
@@ -362,13 +370,16 @@ export async function registerUploadedWorkspaceFile(params: {
362370
const serveUrl = `${pathPrefix}${encodeURIComponent(key)}?context=workspace`
363371

364372
return {
365-
id: fileId,
366-
name: displayName,
367-
size: verifiedSize,
368-
type: contentType,
369-
url: serveUrl,
370-
key,
371-
context: 'workspace',
373+
file: {
374+
id: fileId,
375+
name: displayName,
376+
size: verifiedSize,
377+
type: contentType,
378+
url: serveUrl,
379+
key,
380+
context: 'workspace',
381+
},
382+
created,
372383
}
373384
}
374385

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { describe, expect, it } from 'vitest'
5+
import { isAbortError, isNetworkError } from '@/lib/uploads/utils/file-utils'
6+
7+
describe('isAbortError', () => {
8+
it('returns true for AbortError-named errors', () => {
9+
const err = new Error('aborted')
10+
err.name = 'AbortError'
11+
expect(isAbortError(err)).toBe(true)
12+
})
13+
14+
it('returns false for generic Errors', () => {
15+
expect(isAbortError(new Error('boom'))).toBe(false)
16+
expect(isAbortError(null)).toBe(false)
17+
expect(isAbortError('AbortError')).toBe(false)
18+
})
19+
})
20+
21+
describe('isNetworkError', () => {
22+
it.each([
23+
'fetch failed',
24+
'Network request failed',
25+
'connection reset',
26+
'request timeout',
27+
'operation timed out',
28+
'ECONNRESET while reading body',
29+
])('matches transient message %s', (msg) => {
30+
expect(isNetworkError(new Error(msg))).toBe(true)
31+
})
32+
33+
it('does not match deterministic errors', () => {
34+
expect(isNetworkError(new Error('Forbidden'))).toBe(false)
35+
expect(isNetworkError(new Error('Validation failed: name is required'))).toBe(false)
36+
expect(isNetworkError('not an error')).toBe(false)
37+
expect(isNetworkError(null)).toBe(false)
38+
})
39+
})

0 commit comments

Comments
 (0)