fix(evidence-export): address two follow-up review findings from prod deploy PR

apps/api/src/tasks/evidence-export/evidence-attachment-streamer.ts

@@ -77,6 +77,11 @@ export function createFilenameTracker(): (rawName: string) => string {
  * - All other failures (network, permissions, throttling, empty body) → rethrow
  *   so the archive aborts and the user sees a real failure instead of silently
  *   receiving an incomplete export.
+ *
+ * Filename collisions are resolved on the *final* ZIP entry name (including
+ * any `_MISSING_…txt` wrapping), not the raw attachment name — otherwise a
+ * success-path file named `_MISSING_foo.txt` could collide with a failure-path
+ * placeholder for a file named `foo` once the wrapping is applied.
  */
 export async function appendAttachmentToArchive(params: {
   archive: Archiver;
@@ -128,8 +133,13 @@ export async function appendAttachmentToArchive(params: {
     logger.warn(
       `Missing S3 object for attachment ${attachment.id} (key=${attachment.url}): ${message}`,
     );
+    // Feed the FULL final name (including `_MISSING_` prefix and `.txt` suffix)
+    // into the same collision tracker that success paths use, so a legitimate
+    // file uploaded as `_MISSING_foo.txt` can't silently collide with a
+    // placeholder for a different missing attachment named `foo`.
+    const placeholderName = uniqueName(`_MISSING_${attachment.name}.txt`);
     archive.append(buildMissingPlaceholder(attachment, message), {
-      name: `${folderPath}/_MISSING_${uniqueName(attachment.name)}.txt`,
+      name: `${folderPath}/${placeholderName}`,
     });
   }
 }

apps/api/src/tasks/evidence-export/evidence-export.controller.spec.ts

@@ -59,6 +59,7 @@ function makeFakeResponse() {
     end: jest.fn(),
     headersSent: false,
     writableEnded: false,
+    writableFinished: false,
   });
   return res;
 }
@@ -180,11 +181,44 @@ describe('EvidenceExportController', () => {
 
     expect(archive.abort).not.toHaveBeenCalled();
 
-    // Simulate client closing the connection before the stream finished.
-    req.emit('close');
+    // Simulate a client-side disconnect: res emits 'close' before the response
+    // has fully flushed (writableFinished is still false).
+    res.writableFinished = false;
+    res.emit('close');
 
     expect(archive.abort).toHaveBeenCalledTimes(1);
   });
+
+  it('does NOT abort the archive when the response completed normally', async () => {
+    // Regression guard for cubic P1: with req.once('close') this would falsely
+    // abort on Node 16+. Using res.close + writableFinished avoids that.
+    const archive = makeFakeArchive();
+    service.streamTaskEvidenceZip.mockResolvedValue({
+      archive: archive as unknown as import('archiver').Archiver,
+      filename: 'f.zip',
+    });
+    const req = makeFakeRequest();
+    const res = makeFakeResponse();
+
+    await controller.exportTaskEvidenceZip(
+      'org_1',
+      'tsk_1',
+      'false',
+      req as unknown as import('express').Request,
+      res as unknown as import('express').Response,
+    );
+
+    // Successful flow: archiver finishes, writableFinished becomes true before
+    // 'close' fires on the response.
+    res.writableFinished = true;
+    res.emit('close');
+
+    // And even if req 'close' fires too (Node 16+ does this on normal
+    // completion), we ignore it — no listener attached there.
+    req.emit('close');
+
+    expect(archive.abort).not.toHaveBeenCalled();
+  });
 });
 
 describe('AuditorEvidenceExportController', () => {

apps/api/src/tasks/evidence-export/evidence-export.controller.ts

@@ -277,6 +277,12 @@ export class AuditorEvidenceExportController {
  *  1. Archive errors → log and end the response (500 if headers not yet sent).
  *  2. Client disconnect → abort the archive so S3 fetches stop and the
  *     background populate task doesn't keep running for a closed socket.
+ *
+ * We listen on `res.close` only and distinguish normal-completion from
+ * disconnect via `res.writableFinished` (true only after the full response
+ * has been flushed). `req.close` is not used because it fires on normal
+ * request completion in modern Node, which can race with our response flush
+ * and cause false aborts of successful exports.
  */
 function pipeArchiveToResponse(params: {
   archive: Archiver;
@@ -285,19 +291,18 @@ function pipeArchiveToResponse(params: {
   logger: Logger;
   tag: string;
 }): void {
-  const { archive, req, res, logger, tag } = params;
+  const { archive, res, logger, tag } = params;
   let aborted = false;
 
-  const abortIfIncomplete = () => {
+  res.once('close', () => {
     if (aborted) return;
-    if (res.writableEnded) return;
+    // writableFinished becomes true only after the response is fully flushed
+    // and the 'finish' event fires; on a client disconnect this stays false.
+    if (res.writableFinished) return;
     aborted = true;
     logger.warn(`Client disconnected during export (${tag}); aborting archive`);
     archive.abort();
-  };
-
-  req.once('close', abortIfIncomplete);
-  res.once('close', abortIfIncomplete);
+  });
 
   archive.on('error', (err) => {
     logger.error(`Archive stream error (${tag}): ${err.message}`);