gsadmin/sencho
1
0
Fork 0
mirror of https://github.com/Studio-Saelix/sencho.git synced 2026-07-26 11:49:16 +00:00
Code Issues Packages Projects Releases Wiki Activity

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

Browse Source 
* 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.
This commit is contained in:
Anso
2026-05-25 15:28:00 -04:00
committed by GitHub
parent b5709ee801
commit a4a8abb5f8
1 changed files with 7 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/src/routes/stacks.ts
+7 -9
View File
@@ -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;