diff --git a/backend/src/__tests__/stack-files-routes.test.ts b/backend/src/__tests__/stack-files-routes.test.ts index 650d9f35..e5460bfc 100644 --- a/backend/src/__tests__/stack-files-routes.test.ts +++ b/backend/src/__tests__/stack-files-routes.test.ts @@ -391,9 +391,10 @@ describe('GET /api/stacks/:stackName/files/download', () => { }); it('records the download metric exactly once per successful response', async () => { - // The metric recorder is wired to the file stream lifecycle and guarded - // by a flag so a single completion does not double-fire. A regression - // that drops the flag would push successCount to 2 for one download. + // The metric recorder is wired to both res.on("finish") and + // res.on("close"), guarded by a flag so a single completion does not + // double-fire. A regression that drops the flag would push successCount + // to 2 for one download. const { FileExplorerMetricsService } = await import('../services/FileExplorerMetricsService'); FileExplorerMetricsService.resetForTests(); @@ -403,7 +404,7 @@ describe('GET /api/stacks/:stackName/files/download', () => { .set('Cookie', adminCookie); expect(res.status).toBe(200); - // Allow the stream lifecycle tail event to fire after the test's await. + // Allow the res.on('close') tail event to fire after the test's await. await new Promise((r) => setTimeout(r, 50)); const metricsRes = await request(app) diff --git a/backend/src/routes/stacks.ts b/backend/src/routes/stacks.ts index 881f8795..86aa4d44 100644 --- a/backend/src/routes/stacks.ts +++ b/backend/src/routes/stacks.ts @@ -1459,14 +1459,15 @@ stacksRouter.get('/:stackName/files/download', async (req: Request, res: Respons const encodedFilename = encodeURIComponent(result.filename); const safeFilename = result.filename.replace(/[\\"]/g, ''); res.setHeader('Content-Disposition', `attachment; filename="${safeFilename}"; filename*=UTF-8''${encodedFilename}`); - // Track download completion from the file stream, but treat a close after - // all bytes were read as success even if the response flags have not - // settled yet under the in-process supertest transport. + // Track download completion off the file stream's lifecycle, not the + // response's. Node's `res.on('finish')` and `res.on('close')` fire in + // platform-dependent order under the in-process supertest transport + // (close occasionally precedes finish, which made the success-vs-error + // recorder racy in CI). The file stream's `end`, `error`, and `close` + // events ARE deterministic: a clean read emits `end` then `close`; a + // destroy emits `close` without `end`; a disk error emits `error` then + // `close`. Hanging the recorder off these signals removes the race. let downloadRecorded = false; - const streamWithBytes = result.stream as typeof result.stream & { bytesRead?: number }; - const hasReadFullFile = (): boolean => ( - typeof streamWithBytes.bytesRead === 'number' && streamWithBytes.bytesRead >= result.size - ); const recordDownloadOnce = (ok: boolean): void => { if (downloadRecorded) return; downloadRecorded = true; @@ -1480,11 +1481,11 @@ stacksRouter.get('/:stackName/files/download', async (req: Request, res: Respons }); result.stream.on('end', () => recordDownloadOnce(true)); result.stream.on('close', () => { - if (res.writableEnded || hasReadFullFile()) recordDownloadOnce(true); + if (res.writableEnded) recordDownloadOnce(true); else recordDownloadOnce(false); }); - res.on('close', () => { - if (!res.writableEnded && !hasReadFullFile()) result.stream.destroy(); + req.on('close', () => { + if (!res.writableEnded) result.stream.destroy(); }); logFileDiag('download stream opened', { stackName, relPath, nodeId: req.nodeId, size: result.size, elapsedMs: Date.now() - startedAt }); result.stream.pipe(res);