Skip to content

Commit a6ebfec

Browse files
fix(files): restore CSV preview cancellation (#6596)
* fix(files): restore CSV preview cancellation * fix
1 parent 0877ecb commit a6ebfec

9 files changed

Lines changed: 198 additions & 2 deletions

File tree

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { authMockFns } from '@sim/testing'
5+
import { NextRequest } from 'next/server'
6+
import { beforeEach, describe, expect, it, vi } from 'vitest'
7+
8+
const mocks = vi.hoisted(() => ({
9+
getSlice: vi.fn(),
10+
readFile: vi.fn(),
11+
}))
12+
13+
vi.mock('@/lib/file-parsers/csv-preview-slice', () => ({
14+
getCsvPreviewSlice: mocks.getSlice,
15+
}))
16+
17+
vi.mock('@/lib/workspace-files/application/read-workspace-file-record', () => ({
18+
readWorkspaceFileContentRecord: {
19+
operation: { id: 'files.read_content', minimumRole: 'read', workspaceApiKey: 'allow' },
20+
execute: mocks.readFile,
21+
},
22+
}))
23+
24+
import { GET } from '@/app/api/workspaces/[id]/files/[fileId]/csv-preview/route'
25+
26+
const WORKSPACE_ID = '7727ef3f-8cf6-4686-b063-2bb006a10785'
27+
const FILE_ID = 'wf_csv'
28+
const KEY = `workspace/${WORKSPACE_ID}/large.csv`
29+
const USER = { id: 'user-1' }
30+
const context = { params: Promise.resolve({ id: WORKSPACE_ID, fileId: FILE_ID }) }
31+
32+
function request(signal?: AbortSignal): NextRequest {
33+
return new NextRequest(
34+
`http://localhost/api/workspaces/${WORKSPACE_ID}/files/${FILE_ID}/csv-preview?key=${encodeURIComponent(KEY)}`,
35+
{ signal }
36+
)
37+
}
38+
39+
describe('GET /api/workspaces/[id]/files/[fileId]/csv-preview', () => {
40+
beforeEach(() => {
41+
vi.clearAllMocks()
42+
authMockFns.mockGetSession.mockResolvedValue({ user: USER, session: { id: 'session-1' } })
43+
mocks.readFile.mockResolvedValue({ file: { id: FILE_ID, key: KEY } })
44+
mocks.getSlice.mockResolvedValue({
45+
headers: ['name'],
46+
rows: [['Ada']],
47+
truncated: false,
48+
})
49+
})
50+
51+
it('propagates client cancellation to the storage preview read', async () => {
52+
const req = request()
53+
const response = await GET(req, context)
54+
55+
expect(response.status).toBe(200)
56+
expect(mocks.getSlice).toHaveBeenCalledWith({
57+
key: KEY,
58+
context: 'workspace',
59+
signal: req.signal,
60+
})
61+
})
62+
63+
it('returns a cancellation response when the client disconnects during the storage read', async () => {
64+
const controller = new AbortController()
65+
const req = request(controller.signal)
66+
mocks.getSlice.mockImplementation(async () => {
67+
controller.abort()
68+
throw Object.assign(new Error('Premature close'), {
69+
code: 'ERR_STREAM_PREMATURE_CLOSE',
70+
})
71+
})
72+
73+
const response = await GET(req, context)
74+
75+
expect(response.status).toBe(499)
76+
await expect(response.json()).resolves.toMatchObject({
77+
error: 'Client cancelled request',
78+
requestId: expect.any(String),
79+
})
80+
})
81+
})

apps/sim/app/api/workspaces/[id]/files/[fileId]/csv-preview/route.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,11 @@ export const GET = defineInternalJsonRoute({
1515
operation: csvPreviewWorkspaceFile.operation,
1616
rateLimit: internalRateLimits.none({ reason: 'Preserve existing internal CSV preview behavior' }),
1717
errorPolicy: internalFileErrorPolicies.default,
18-
mapInput: ({ params, query }) => ({
18+
mapInput: ({ params, query }, { request }) => ({
1919
fileId: params.fileId,
2020
assertedWorkspaceId: params.id,
2121
key: query.key,
22+
signal: request.signal,
2223
}),
2324
useCase: csvPreviewWorkspaceFile,
2425
onSuccess: ({ result }) => {

apps/sim/lib/api/server/routes/internal-json-route.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -365,6 +365,10 @@ export function defineInternalJsonRoute<
365365
}
366366
},
367367
{
368+
clientAbortResponse: ({ requestId }) =>
369+
createJsonErrorResponse(
370+
internalErrorResponse(499, { error: 'Client cancelled request', requestId })
371+
),
368372
typedErrorResponse: ({ error, status, requestId }) =>
369373
NextResponse.json({ error: error.message, requestId }, { status }),
370374
unhandledErrorResponse: () =>

apps/sim/lib/api/server/routes/v2-json-route.test.ts

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,11 +116,12 @@ function createHandler(overrides: HandlerOverrides = {}) {
116116
})
117117
}
118118

119-
function request(body: unknown = { value: 'ok' }): NextRequest {
119+
function request(body: unknown = { value: 'ok' }, signal?: AbortSignal): NextRequest {
120120
return new NextRequest('http://localhost/api/v2/widgets', {
121121
method: 'POST',
122122
headers: { 'content-type': 'application/json', 'x-api-key': 'secret' },
123123
body: JSON.stringify(body),
124+
signal,
124125
})
125126
}
126127

@@ -395,6 +396,25 @@ describe('defineV2JsonRoute', () => {
395396
expect(response.headers.get('X-RateLimit-Remaining')).toBe('99')
396397
})
397398

399+
it('renders a client disconnect through the v2 cancellation envelope', async () => {
400+
const controller = new AbortController()
401+
const response = await createHandler({
402+
execute: async () => {
403+
controller.abort()
404+
throw Object.assign(new Error('Premature close'), {
405+
code: 'ERR_STREAM_PREMATURE_CLOSE',
406+
})
407+
},
408+
})(request(undefined, controller.signal))
409+
410+
expect(response.status).toBe(499)
411+
await expect(response.json()).resolves.toEqual({
412+
error: { code: 'CLIENT_CLOSED_REQUEST', message: 'Client cancelled request' },
413+
})
414+
expect(response.headers.get('Cache-Control')).toBe('private, no-store')
415+
expect(response.headers.get('X-RateLimit-Remaining')).toBe('99')
416+
})
417+
398418
it('validates the presented response before onSuccess', async () => {
399419
const onSuccess = vi.fn()
400420
const response = await createHandler({

apps/sim/lib/api/server/routes/v2-json-route.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -287,6 +287,7 @@ export function defineV2JsonRoute<
287287
}
288288
},
289289
{
290+
clientAbortResponse: () => v2Error('CLIENT_CLOSED_REQUEST', 'Client cancelled request'),
290291
typedErrorResponse: ({ error }) => v2HttpError(error),
291292
unhandledErrorResponse: ({ error }) =>
292293
error instanceof V2RouteInfrastructureError

apps/sim/lib/core/utils/with-route-handler.test.ts

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
* @vitest-environment node
33
*/
44

5+
import { loggerMock } from '@sim/testing'
56
import { NextRequest, NextResponse } from 'next/server'
67
import { describe, expect, it, vi } from 'vitest'
78
import { HttpError } from '@/lib/core/utils/http-error'
@@ -17,6 +18,47 @@ class TestHttpError extends HttpError {
1718
}
1819

1920
describe('withRouteHandler', () => {
21+
it('classifies errors after a client disconnect without using the unhandled fallback', async () => {
22+
const routeHandlerLogger = vi.mocked(loggerMock.createLogger).mock.results[
23+
vi.mocked(loggerMock.createLogger).mock.calls.findIndex(([name]) => name === 'RouteHandler')
24+
]?.value
25+
routeHandlerLogger?.info.mockClear()
26+
routeHandlerLogger?.error.mockClear()
27+
28+
const controller = new AbortController()
29+
const clientAbortResponse = vi.fn(() =>
30+
NextResponse.json({ error: 'Client cancelled request' }, { status: 499 })
31+
)
32+
const unhandledErrorResponse = vi.fn(() =>
33+
NextResponse.json({ error: 'Internal server error' }, { status: 500 })
34+
)
35+
const handler = withRouteHandler(
36+
async () => {
37+
controller.abort()
38+
throw Object.assign(new Error('Premature close'), {
39+
code: 'ERR_STREAM_PREMATURE_CLOSE',
40+
})
41+
},
42+
{ clientAbortResponse, unhandledErrorResponse }
43+
)
44+
45+
const response = await handler(
46+
new NextRequest('http://localhost/api/test', { signal: controller.signal }),
47+
undefined
48+
)
49+
50+
expect(response.status).toBe(499)
51+
await expect(response.json()).resolves.toEqual({ error: 'Client cancelled request' })
52+
expect(clientAbortResponse).toHaveBeenCalledOnce()
53+
expect(unhandledErrorResponse).not.toHaveBeenCalled()
54+
expect(routeHandlerLogger?.error).not.toHaveBeenCalled()
55+
expect(routeHandlerLogger?.info).toHaveBeenCalledWith('Client closed request', {
56+
duration: expect.any(Number),
57+
status: 499,
58+
})
59+
expect(response.headers.get('x-request-id')).toBeTruthy()
60+
})
61+
2062
it('lets a route family render a typed error before its generic fallback', async () => {
2163
const unhandledErrorResponse = vi.fn(() =>
2264
NextResponse.json({ family: 'generic' }, { status: 500 })

apps/sim/lib/core/utils/with-route-handler.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ interface RouteHandlerTypedErrorContext {
2525
}
2626

2727
interface RouteHandlerOptions {
28+
clientAbortResponse?: (context: RouteHandlerErrorContext) => NextResponse | Response
2829
typedErrorResponse?: (context: RouteHandlerTypedErrorContext) => NextResponse | Response
2930
unhandledErrorResponse?: (context: RouteHandlerErrorContext) => NextResponse | Response
3031
}
@@ -78,7 +79,9 @@ function applyResponseHeaders(
7879
* - Generates a unique request ID and stores it in AsyncLocalStorage so every
7980
* logger in the request lifecycle automatically includes it
8081
* - Logs all 4xx and 5xx responses with method, path, status, duration
82+
* - Classifies errors after a client disconnect as a normal 499 cancellation
8183
* - Catches unhandled errors, logs them, and returns a 500 with the request ID
84+
* - Supports a route-family-specific client-abort response envelope
8285
* - Supports a route-family-specific unhandled-error response envelope
8386
* - Attaches `x-request-id`, plus the rate-limit headers when the route
8487
* recorded a snapshot for the request
@@ -101,6 +104,15 @@ export function withRouteHandler<T>(
101104
} catch (error) {
102105
const duration = Date.now() - startTime
103106
const message = getErrorMessage(error, 'Unknown error')
107+
if (request.signal.aborted) {
108+
logger.info('Client closed request', { duration, status: 499 })
109+
response = options.clientAbortResponse
110+
? options.clientAbortResponse({ error, requestId })
111+
: new Response(null, { status: 499 })
112+
applyResponseHeaders(response, request, requestId)
113+
return response
114+
}
115+
104116
const typedError = readTypedError(error)
105117
if (typedError) {
106118
const typedStatus = typedError.statusCode

apps/sim/lib/file-parsers/csv-preview-slice.test.ts

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,4 +100,32 @@ describe('getCsvPreviewSlice', () => {
100100
expect(slice.truncated).toBe(true)
101101
expect(destroySpy).toHaveBeenCalled()
102102
})
103+
104+
it('destroys a source acquired after the request was already aborted', async () => {
105+
const source = streamOf('a,b\n1,2\n')
106+
const destroySpy = vi.spyOn(source, 'destroy')
107+
const controller = new AbortController()
108+
controller.abort()
109+
mockDownloadFileStream.mockResolvedValue(source)
110+
111+
await expect(getCsvPreviewSlice({ ...args, signal: controller.signal })).rejects.toMatchObject({
112+
name: 'AbortError',
113+
})
114+
expect(destroySpy).toHaveBeenCalled()
115+
})
116+
117+
it('destroys an active source when the request is aborted', async () => {
118+
const read = vi.fn()
119+
const source = new Readable({ read })
120+
const destroySpy = vi.spyOn(source, 'destroy')
121+
const controller = new AbortController()
122+
mockDownloadFileStream.mockResolvedValue(source)
123+
124+
const preview = getCsvPreviewSlice({ ...args, signal: controller.signal })
125+
await vi.waitFor(() => expect(read).toHaveBeenCalled())
126+
controller.abort()
127+
128+
await expect(preview).rejects.toMatchObject({ name: 'AbortError' })
129+
expect(destroySpy).toHaveBeenCalled()
130+
})
103131
})

apps/sim/lib/file-parsers/csv-preview-slice.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,10 @@ export async function getCsvPreviewSlice({
5252
signal,
5353
}: CsvPreviewSliceArgs): Promise<CsvPreviewSlice> {
5454
const source = await downloadFileStream({ key, context })
55+
if (signal?.aborted) {
56+
source.destroy()
57+
signal.throwIfAborted()
58+
}
5559
const onAbort = () => source.destroy()
5660
signal?.addEventListener('abort', onAbort, { once: true })
5761

@@ -133,6 +137,9 @@ export async function getCsvPreviewSlice({
133137
piped.destroy()
134138
parser.destroy()
135139
return { headers, rows, truncated }
140+
} catch (error) {
141+
if (signal?.aborted) signal.throwIfAborted()
142+
throw error
136143
} finally {
137144
signal?.removeEventListener('abort', onAbort)
138145
source.destroy()

0 commit comments

Comments
 (0)