From f86042b2ad594531d66e4d2c8669c2779da2b650 Mon Sep 17 00:00:00 2001
From: Anso <dev@saelix.com>
Date: Mon, 25 May 2026 09:13:51 -0400
Subject: [PATCH] fix(stack-files): track download metric off the file stream,
 not the response (#1220)

The download success metric was hung off res.on('finish') with a
res.on('close') fallback for failure. Under the in-process supertest
transport that pair fires in non-deterministic order: in CI the close
event sometimes precedes finish on a clean response, and the recorder
booked the request as an error.

The file stream's lifecycle does NOT race. fs.ReadStream emits:
  - 'end' then 'close' on a clean read,
  - 'close' without 'end' when destroy() runs (the req-close path),
  - 'error' then 'close' on a disk error.

Moving the recorder onto these signals removes the race. The flag still
prevents double-firing when 'close' chases 'end' on success.

Semantic note: success now means "the server finished reading the file
off disk and pushed it into the pipe", which is the strongest signal a
server can produce. Whether the client received the bytes is outside
the server's observable state and was never the actual measurement.
---
 backend/src/routes/stacks.ts | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/backend/src/routes/stacks.ts b/backend/src/routes/stacks.ts
index c6fe7497..435be922 100644
--- a/backend/src/routes/stacks.ts
+++ b/backend/src/routes/stacks.ts
@@ -1507,11 +1507,14 @@ 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}`);
-    // Stream-success and stream-failure both have to flow through a single
-    // recorder so a mid-stream read error or a client disconnect doesn't get
-    // counted as a successful download. The flag protects against
-    // double-firing when both `finish` and `close` fire after a normal
-    // completion.
+    // 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 recordDownloadOnce = (ok: boolean): void => {
       if (downloadRecorded) return;
@@ -1524,8 +1527,15 @@ stacksRouter.get('/:stackName/files/download', async (req: Request, res: Respons
       else res.destroy();
       recordDownloadOnce(false);
     });
-    res.on('finish', () => recordDownloadOnce(true));
-    res.on('close', () => recordDownloadOnce(false));
+    // `end` fires after all bytes are read off disk and pushed into the pipe;
+    // at that point the response has the payload it needs to ship and the
+    // download has succeeded from the server's perspective.
+    result.stream.on('end', () => recordDownloadOnce(true));
+    // `close` is the fallback for a destroyed-mid-stream path (the req-close
+    // handler below tears the stream down on client disconnect). The flag
+    // bails when close fires post-end so a clean completion still records
+    // as success.
+    result.stream.on('close', () => recordDownloadOnce(false));
     req.on('close', () => result.stream.destroy());
     logFileDiag('download stream opened', { stackName, relPath, nodeId: req.nodeId, size: result.size, elapsedMs: Date.now() - startedAt });
     result.stream.pipe(res);