From a4a8abb5f803d49aaf401bcce90c2286ccdfd686 Mon Sep 17 00:00:00 2001
From: Anso <dev@saelix.com>
Date: Mon, 25 May 2026 15:28:00 -0400
Subject: [PATCH] fix(stack-files): guard download stream destroy against the
 supertest in-process close race (#1227)

* fix(stack-files): guard download stream destroy against the supertest in-process close race

The download metric was still booking the occasional request as a
failure even after PR #1220 moved tracking off res.{finish,close} and
onto stream.{end,close}. The remaining race lives one level up: under
the in-process supertest transport, req.on("close") can fire as soon
as the test consumes the response, before the readable's own end/close
pair has been dispatched.

The unconditional req.on("close", () => result.stream.destroy()) line
forced the readable into a close-without-end state every time that
happened. The close handler then booked a failure even though the pipe
had delivered every byte.

Two layers of defense:
  - req.on("close") now only destroys the stream while the response is
    still in flight. If res.writableEnded is already true the pipe has
    finished and the readable will end on its own; there is nothing to
    clean up and no synthetic close to provoke.
  - stream.on("close") falls back to recording success when the
    response was already fully written. Real disconnects keep recording
    a failure because res.writableEnded stays false in that case.

The semantic stays unchanged: success means the server finished reading
the file off disk and pushed it into the pipe. The fix removes the
spurious failure path without weakening the disconnect-detection.

* chore(stack-files): temporary download diagnostic for the metric race

Logs the event order and response state at each handler so the next
CI run shows whether req-close fires before stream-end, what
res.writable / writableEnded / writableFinished evaluate to at that
moment, and which path actually records the failure. Will be removed
in the follow-up commit once the race is understood.

* chore(stack-files): remove temporary download diagnostic

Diagnostic captured the happy-path event order in the previous CI
run; removing it now to isolate whether the fix alone is sufficient.
---
 backend/src/routes/stacks.ts | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/backend/src/routes/stacks.ts b/backend/src/routes/stacks.ts
index 435be922..b5a4be77 100644
--- a/backend/src/routes/stacks.ts
+++ b/backend/src/routes/stacks.ts
@@ -1527,16 +1527,14 @@ stacksRouter.get('/:stackName/files/download', async (req: Request, res: Respons
       else res.destroy();
       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());
+    result.stream.on('close', () => {
+      if (res.writableEnded) recordDownloadOnce(true);
+      else recordDownloadOnce(false);
+    });
+    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);
     return;