diff --git a/backend/src/__tests__/stack-files-routes.test.ts b/backend/src/__tests__/stack-files-routes.test.ts index 43b09f79..d4d4acd6 100644 --- a/backend/src/__tests__/stack-files-routes.test.ts +++ b/backend/src/__tests__/stack-files-routes.test.ts @@ -502,6 +502,35 @@ describe('GET /api/stacks/:stackName/files/download', () => { } } + class ResponseCloseAfterFullReadStream extends Readable { + public bytesRead = 0; + public destroyCalls = 0; + public emittedResponseClose = false; + + constructor(private readonly payload: Buffer) { + super(); + } + + _read(): void {} + + override destroy(error?: Error): this { + this.destroyCalls += 1; + return super.destroy(error); + } + + override pipe(destination: T): T { + destination.write(this.payload); + this.bytesRead = this.payload.length; + // The response closes after the whole body was received, and the source + // stream never emits its own end/close because the consumer is already + // gone. Mirrors the real fs.ReadStream path under load that previously + // left the download metric unrecorded. + this.emittedResponseClose = destination.emit('close'); + destination.end(); + return destination; + } + } + class ErrorThenCloseStream extends Readable { _read(): void {} @@ -681,6 +710,24 @@ describe('GET /api/stacks/:stackName/files/download', () => { await expectDownloadMetricCounts(1, 1, 0); }); + it('records a success when the response closes after a full read and the source emits no end/close', async () => { + await resetFileExplorerMetrics(); + const payload = Buffer.from('services: {}\n'); + const stream = new ResponseCloseAfterFullReadStream(payload); + await mockDownloadStream(stream, payload.length); + + const res = await request(app) + .get(`/api/stacks/${STACK}/files/download`) + .query({ path: 'compose.yaml' }) + .set('Cookie', adminCookie); + + expect(res.status).toBe(200); + expect(res.text).toBe(payload.toString('utf-8')); + expect(stream.emittedResponseClose).toBe(true); + expect(stream.destroyCalls).toBe(0); + await expectDownloadMetricCounts(1, 1, 0); + }); + it('records stream error followed by close exactly once as a failed download', async () => { await resetFileExplorerMetrics(); const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => undefined); diff --git a/backend/src/routes/stacks.ts b/backend/src/routes/stacks.ts index 1988c942..5388e075 100644 --- a/backend/src/routes/stacks.ts +++ b/backend/src/routes/stacks.ts @@ -1504,12 +1504,12 @@ 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 normal download completion off the file stream's lifecycle, not - // the request lifecycle. Under the in-process supertest transport, request - // close events can race ahead of normal stream completion; response close - // is handled below only as delayed abort cleanup. - // End/error are the durable completion signals, and close can still count - // as success only when the stream reports it read the full file. + // Track download completion off both the file stream's lifecycle and the + // response close. Under the in-process supertest transport, request close + // events can race ahead of normal stream completion. End/error are the + // durable source signals; a close (source or response) counts as success + // only when the stream reports it read the full file, and as an abort + // otherwise. let downloadRecorded = false; const streamWithBytes = result.stream as typeof result.stream & { bytesRead?: number }; const hasReadFullFile = (): boolean => ( @@ -1541,13 +1541,20 @@ stacksRouter.get('/:stackName/files/download', async (req: Request, res: Respons result.stream.on('end', () => recordDownloadOnce(hasReadFullFile())); result.stream.on('close', () => recordDownloadOnce(hasReadFullFile())); res.on('close', () => { - if (downloadRecorded || hasReadFullFile()) return; - // At this point, response close is treated as an abort signal. It can - // beat the source stream's final events in the in-process test transport. - // Give a same-turn clean source completion a chance to win before cleanup. + if (downloadRecorded) return; + // This op measures a server-side file read, so once the source has + // streamed the whole file a response close is a successful completion. + // Record it here rather than waiting on the source stream's end/close, + // which can be dropped once the response consumer is gone, leaving the + // op unrecorded. + if (hasReadFullFile()) { recordDownloadOnce(true); return; } + // Otherwise response close is an abort signal. It can beat the source + // stream's final events in the in-process test transport, so give a + // same-turn clean source completion a chance to win before cleanup. abortCleanupHandle = setImmediate(() => { abortCleanupHandle = null; - if (downloadRecorded || hasReadFullFile()) return; + if (downloadRecorded) return; + if (hasReadFullFile()) { recordDownloadOnce(true); return; } recordDownloadOnce(false); result.stream.destroy(); });