fix(evidence-export): address review feedback on streaming export

Four issues from the PR review, all legitimate:

- Attachment streamer treated every S3 failure as a missing object. Now only
  NoSuchKey / HTTP 404 produce a `_MISSING_*.txt` placeholder. Network,
  permission, and throttling errors rethrow so the archive aborts and the
  user sees a real failure instead of a silently-incomplete bundle.

- `streamOrganizationEvidenceZip` was throwing NotFoundException from the
  async populate task, which fired after headers were already sent — the
  client got a broken stream instead of a 404. Hoisted the empty-content
  check to pre-flight so it becomes a proper HTTP 404.

- Controllers now listen for client disconnect (`req.on('close')`) and abort
  the archive so cancelled downloads stop consuming backend resources
  (S3 fetches, background populate task).

- Org populate no longer buffers all task summaries into an array before
  writing. Each task is streamed into the archive as it's processed, and
  only a lightweight manifest (id / title / counts) is accumulated. Manifest
  is written last.

Tests: 31 passing (was 29) — added AccessDenied rethrow, client-disconnect
abort. Pre-flight 404 test now asserts the throw is synchronous and no
archive is created.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: tofikwest

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

@@ -71,8 +71,12 @@ export function createFilenameTracker(): (rawName: string) => string {
 
 /**
  * Append a single attachment to the archive by streaming its S3 body.
- * If the object is missing (deleted from S3 but DB row still exists), writes a
- * plaintext placeholder so the bundle remains auditable instead of failing.
+ *
+ * - Genuine missing-object errors (`NoSuchKey` / HTTP 404) → write a
+ *   `_MISSING_<name>.txt` placeholder so the bundle stays auditable.
+ * - 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.
  */
 export async function appendAttachmentToArchive(params: {
   archive: Archiver;
@@ -83,14 +87,11 @@ export async function appendAttachmentToArchive(params: {
   const { archive, attachment, folderPath, uniqueName } = params;
 
   if (!s3Client || !BUCKET_NAME) {
-    logger.warn(
-      `S3 client unavailable — attachment ${attachment.id} skipped with placeholder`,
-    );
-    archive.append(
-      buildMissingPlaceholder(attachment, 'S3 client not configured'),
-      { name: `${folderPath}/_MISSING_${uniqueName(attachment.name)}.txt` },
+    // Misconfiguration at process level — fail the whole export, don't silently
+    // produce placeholders for every attachment.
+    throw new Error(
+      'S3 client or bucket not configured; cannot stream attachments',
     );
-    return;
   }
 
   try {
@@ -114,6 +115,15 @@ export async function appendAttachmentToArchive(params: {
       name: `${folderPath}/${uniqueName(attachment.name)}`,
     });
   } catch (error) {
+    if (!isS3MissingObjectError(error)) {
+      logger.error(
+        `Failed to fetch attachment ${attachment.id} (key=${attachment.url}): ${
+          error instanceof Error ? error.message : String(error)
+        }`,
+      );
+      throw error;
+    }
+
     const message = error instanceof Error ? error.message : String(error);
     logger.warn(
       `Missing S3 object for attachment ${attachment.id} (key=${attachment.url}): ${message}`,
@@ -124,6 +134,24 @@ export async function appendAttachmentToArchive(params: {
   }
 }
 
+/**
+ * True only for "the object does not exist" — NoSuchKey or HTTP 404.
+ * Everything else (AccessDenied, SlowDown, NetworkError, timeouts) is treated
+ * as a real failure that should surface, not a silent skip.
+ */
+function isS3MissingObjectError(error: unknown): boolean {
+  if (!error || typeof error !== 'object') return false;
+  const err = error as {
+    name?: string;
+    Code?: string;
+    $metadata?: { httpStatusCode?: number };
+  };
+  if (err.name === 'NoSuchKey' || err.name === 'NotFound') return true;
+  if (err.Code === 'NoSuchKey' || err.Code === 'NotFound') return true;
+  if (err.$metadata?.httpStatusCode === 404) return true;
+  return false;
+}
+
 function buildMissingPlaceholder(
   attachment: TaskAttachment,
   reason: string,

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

@@ -50,20 +50,23 @@ function makeFakeArchive() {
 }
 
 function makeFakeResponse() {
-  const res: {
-    setHeader: jest.Mock;
-    status: jest.Mock;
-    end: jest.Mock;
-    headersSent: boolean;
-  } = {
+  const emitter = new EventEmitter();
+  const res = Object.assign(emitter, {
     setHeader: jest.fn(),
-    status: jest.fn(() => res),
+    status: jest.fn(function (this: unknown) {
+      return res;
+    }),
     end: jest.fn(),
     headersSent: false,
-  };
+    writableEnded: false,
+  });
   return res;
 }
 
+function makeFakeRequest() {
+  return new EventEmitter();
+}
+
 describe('EvidenceExportController', () => {
   let controller: EvidenceExportController;
   let service: jest.Mocked<
@@ -106,12 +109,14 @@ describe('EvidenceExportController', () => {
       archive: archive as unknown as import('archiver').Archiver,
       filename: 'acme_mytask_evidence_2026-04-22.zip',
     });
+    const req = makeFakeRequest();
     const res = makeFakeResponse();
 
     await controller.exportTaskEvidenceZip(
       'org_1',
       'tsk_1',
       'true',
+      req as unknown as import('express').Request,
       res as unknown as import('express').Response,
     );
 
@@ -138,12 +143,14 @@ describe('EvidenceExportController', () => {
       archive: archive as unknown as import('archiver').Archiver,
       filename: 'f.zip',
     });
+    const req = makeFakeRequest();
     const res = makeFakeResponse();
 
     await controller.exportTaskEvidenceZip(
       'org_1',
       'tsk_1',
       undefined as unknown as string,
+      req as unknown as import('express').Request,
       res as unknown as import('express').Response,
     );
 
@@ -153,6 +160,31 @@ describe('EvidenceExportController', () => {
       { includeRawJson: false },
     );
   });
+
+  it('aborts the archive when the client disconnects mid-stream', async () => {
+    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,
+    );
+
+    expect(archive.abort).not.toHaveBeenCalled();
+
+    // Simulate client closing the connection before the stream finished.
+    req.emit('close');
+
+    expect(archive.abort).toHaveBeenCalledTimes(1);
+  });
 });
 
 describe('AuditorEvidenceExportController', () => {
@@ -185,11 +217,13 @@ describe('AuditorEvidenceExportController', () => {
       archive: archive as unknown as import('archiver').Archiver,
       filename: 'acme_all-evidence_2026-04-22.zip',
     });
+    const req = makeFakeRequest();
     const res = makeFakeResponse();
 
     await controller.exportAllEvidence(
       'org_1',
       'true',
+      req as unknown as import('express').Request,
       res as unknown as import('express').Response,
     );
 

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

@@ -3,6 +3,7 @@ import {
   Get,
   Param,
   Query,
+  Req,
   Res,
   UseGuards,
   Logger,
@@ -15,7 +16,8 @@ import {
   ApiSecurity,
   ApiTags,
 } from '@nestjs/swagger';
-import type { Response } from 'express';
+import type { Request, Response } from 'express';
+import type { Archiver } from 'archiver';
 import { AuditRead } from '../../audit/skip-audit-log.decorator';
 import { OrganizationId } from '../../auth/auth-context.decorator';
 import { HybridAuthGuard } from '../../auth/hybrid-auth.guard';
@@ -164,6 +166,7 @@ export class EvidenceExportController {
     @OrganizationId() organizationId: string,
     @Param('taskId') taskId: string,
     @Query('includeJson') includeJson: string,
+    @Req() req: Request,
     @Res() res: Response,
   ) {
     this.logger.log('Export task evidence ZIP', {
@@ -186,16 +189,13 @@ export class EvidenceExportController {
       `attachment; filename="${filename}"`,
     );
 
-    archive.on('error', (err) => {
-      this.logger.error(`Archive stream error for task ${taskId}: ${err.message}`);
-      if (!res.headersSent) {
-        res.status(500).end();
-      } else {
-        res.end();
-      }
+    pipeArchiveToResponse({
+      archive,
+      req,
+      res,
+      logger: this.logger,
+      tag: `task ${taskId}`,
     });
-
-    archive.pipe(res);
   }
 }
 
@@ -242,6 +242,7 @@ export class AuditorEvidenceExportController {
   async exportAllEvidence(
     @OrganizationId() organizationId: string,
     @Query('includeJson') includeJson: string,
+    @Req() req: Request,
     @Res() res: Response,
   ) {
     this.logger.log('Auditor exporting all evidence', {
@@ -261,17 +262,51 @@ export class AuditorEvidenceExportController {
       `attachment; filename="${filename}"`,
     );
 
-    archive.on('error', (err) => {
-      this.logger.error(
-        `Archive stream error for org ${organizationId}: ${err.message}`,
-      );
-      if (!res.headersSent) {
-        res.status(500).end();
-      } else {
-        res.end();
-      }
+    pipeArchiveToResponse({
+      archive,
+      req,
+      res,
+      logger: this.logger,
+      tag: `org ${organizationId}`,
     });
-
-    archive.pipe(res);
   }
 }
+
+/**
+ * Wire an archive to the HTTP response with two concerns:
+ *  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.
+ */
+function pipeArchiveToResponse(params: {
+  archive: Archiver;
+  req: Request;
+  res: Response;
+  logger: Logger;
+  tag: string;
+}): void {
+  const { archive, req, res, logger, tag } = params;
+  let aborted = false;
+
+  const abortIfIncomplete = () => {
+    if (aborted) return;
+    if (res.writableEnded) 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}`);
+    if (!res.headersSent) {
+      res.status(500).end();
+    } else if (!res.writableEnded) {
+      res.end();
+    }
+  });
+
+  archive.pipe(res);
+}

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

@@ -199,7 +199,7 @@ describe('EvidenceExportService — streaming ZIPs', () => {
       );
     });
 
-    it('writes a placeholder when S3 object is missing', async () => {
+    it('writes a placeholder when S3 object is truly missing (NoSuchKey)', async () => {
       const attachments = [
         {
           id: 'att_missing',
@@ -210,9 +210,11 @@ describe('EvidenceExportService — streaming ZIPs', () => {
         },
       ];
 
-      (s3Client!.send as jest.Mock).mockRejectedValue(
-        new Error('NoSuchKey: The specified key does not exist.'),
-      );
+      const noSuchKeyError = Object.assign(new Error('NoSuchKey'), {
+        name: 'NoSuchKey',
+        $metadata: { httpStatusCode: 404 },
+      });
+      (s3Client!.send as jest.Mock).mockRejectedValue(noSuchKeyError);
       primeTaskQueries({ attachments });
 
       const { archive } = await service.streamTaskEvidenceZip(
@@ -230,6 +232,41 @@ describe('EvidenceExportService — streaming ZIPs', () => {
       expect(String(placeholder!.source)).toContain('att_missing');
     });
 
+    it('aborts the archive on transient S3 failures (not a placeholder)', async () => {
+      const attachments = [
+        {
+          id: 'att_err',
+          name: 'file.pdf',
+          url: 'org_1/attachments/task/tsk_123/file.pdf',
+          type: 'document',
+          createdAt: new Date(),
+        },
+      ];
+
+      // AccessDenied — NOT a missing-object error; must surface as a failure.
+      const accessDeniedError = Object.assign(new Error('Access Denied'), {
+        name: 'AccessDenied',
+        $metadata: { httpStatusCode: 403 },
+      });
+      (s3Client!.send as jest.Mock).mockRejectedValue(accessDeniedError);
+      primeTaskQueries({ attachments });
+
+      const { archive } = await service.streamTaskEvidenceZip(
+        'org_1',
+        'tsk_123',
+      );
+      const mock = archive as unknown as MockArchive;
+
+      await expect(mock.finalized).rejects.toThrow('aborted');
+      expect(mock.abort).toHaveBeenCalled();
+
+      // No placeholder text file written for a non-missing error
+      const placeholder = mock.appendCalls.find((c) =>
+        c.options.name.includes('_MISSING_'),
+      );
+      expect(placeholder).toBeUndefined();
+    });
+
     it('disambiguates duplicate filenames within attachments folder', async () => {
       const attachments = [
         {
@@ -296,18 +333,20 @@ describe('EvidenceExportService — streaming ZIPs', () => {
       ).rejects.toBeInstanceOf(NotFoundException);
     });
 
-    it('aborts archive with NotFoundException when no tasks have content', async () => {
+    it('throws NotFoundException synchronously (pre-flight) when no tasks have content', async () => {
+      // Org exists but no tasks with automations and no attachments.
       mockDb.organization.findUnique.mockResolvedValue({ name: 'Acme' });
       mockDb.task.findMany.mockResolvedValue([]);
       mockDb.attachment.findMany.mockResolvedValue([]);
 
-      const { archive } = await service.streamOrganizationEvidenceZip(
-        'org_1',
-      );
-      const mock = archive as unknown as MockArchive;
+      // Must reject synchronously — before an archive is returned — so the
+      // controller can produce a real HTTP 404 instead of a broken streamed ZIP.
+      await expect(
+        service.streamOrganizationEvidenceZip('org_1'),
+      ).rejects.toBeInstanceOf(NotFoundException);
 
-      await expect(mock.finalized).rejects.toThrow('aborted');
-      expect(mock.abort).toHaveBeenCalled();
+      // No archive should have been created at all.
+      expect(archiveInstances).toHaveLength(0);
     });
 
     it('includes a task that has attachments but no automations', async () => {