fix(download): isolate temp files and reject duplicate download targets

src/download/index.test.ts

@@ -133,6 +133,48 @@ describe('download helpers', { retry: process.platform === 'win32' ? 2 : 0 }, ()
     expect(fs.readFileSync(destPath, 'utf8')).toBe('ok');
   });
 
+  it('gives each concurrent download of the same destination its own temp file', async () => {
+    const slowBody = 'slow-body';
+    const fastBody = 'fast-body';
+    let releaseSlowTail = () => {};
+    const slowTailReleased = new Promise<void>((resolve) => { releaseSlowTail = resolve; });
+
+    const baseUrl = await startServer(async (req, res) => {
+      const body = req.url === '/slow' ? slowBody : fastBody;
+      res.writeHead(200, { 'content-length': String(body.length) });
+      if (req.url !== '/slow') {
+        res.end(body);
+        return;
+      }
+      // Hold the slow response open so both downloads are writing at once.
+      res.write(body.slice(0, 1));
+      await slowTailReleased;
+      res.end(body.slice(1));
+    });
+
+    const tempDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'webcmd-dl-'));
+    tempDirs.push(tempDir);
+    const destPath = path.join(tempDir, 'same-name.bin');
+    const tempFiles = () => fs.readdirSync(tempDir).filter((name) => name.endsWith('.tmp'));
+
+    const slow = httpDownload(`${baseUrl}/slow`, destPath);
+    while (tempFiles().length === 0) {
+      await new Promise((resolve) => setTimeout(resolve, 5));
+    }
+
+    // Second attempt at the same destination, while the first still holds a
+    // temp file open. A shared `${destPath}.tmp` would let it truncate and
+    // rename the first attempt's file out from under it.
+    const fast = await httpDownload(`${baseUrl}/fast`, destPath);
+    releaseSlowTail();
+
+    expect(fast).toEqual({ success: true, size: fastBody.length });
+    expect(await slow).toEqual({ success: true, size: slowBody.length });
+    // The later rename wins; the file is one whole body, never a blend.
+    expect(fs.readFileSync(destPath, 'utf8')).toBe(slowBody);
+    expect(tempFiles()).toEqual([]);
+  });
+
   it.skipIf(process.platform === 'win32')('writes the Netscape cookie file with 0o600 owner-only permissions', async () => {
     const tempDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'webcmd-dl-'));
     tempDirs.push(tempDir);

src/download/index.ts

@@ -3,6 +3,7 @@
  */
 
 import { spawn } from 'node:child_process';
+import { randomBytes } from 'node:crypto';
 import * as fs from 'node:fs';
 import * as path from 'node:path';
 import * as os from 'node:os';
@@ -96,6 +97,20 @@ export function requiresYtdlp(url: string): boolean {
   return isVideoPlatformUrl(url);
 }
 
+/**
+ * Build a unique temporary path for one download attempt.
+ *
+ * The temp file stays beside the destination so the final `rename` never
+ * crosses a filesystem boundary. It must not be a plain `${destPath}.tmp`:
+ * the download pipeline step runs several items in parallel and two of them
+ * can resolve to the same destination, in which case a shared temp path lets
+ * both write into one file and lets one attempt's cleanup delete the other's
+ * in-flight data. Mirrors the temp-write naming used in hosted/files.ts.
+ */
+function createTempPath(destPath: string): string {
+  return `${destPath}.${process.pid}.${randomBytes(8).toString('hex')}.tmp`;
+}
+
 /**
  * HTTP download with progress callback.
  */
@@ -116,7 +131,7 @@ export async function httpDownload(
     requestHeaders['Cookie'] = cookies;
   }
 
-  const tempPath = `${destPath}.tmp`;
+  const tempPath = createTempPath(destPath);
 
   const cleanupTempFile = async () => {
     try {

src/pipeline/steps/download.test.ts

@@ -1,4 +1,5 @@
-import { beforeEach, describe, expect, it, vi } from 'vitest';
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
+import * as fs from 'node:fs';
 import * as os from 'node:os';
 import * as path from 'node:path';
 import type { IPage } from '../../types.js';
@@ -50,6 +51,8 @@ function createMockPage(getCookies: IPage['getCookies']): IPage {
 }
 
 describe('stepDownload', () => {
+  const tempDirs: string[] = [];
+
   beforeEach(() => {
     mockHttpDownload.mockReset();
     mockHttpDownload.mockResolvedValue({ success: true, size: 2 });
@@ -58,6 +61,13 @@ describe('stepDownload', () => {
     mockExportCookiesToNetscape.mockReset();
   });
 
+  afterEach(() => {
+    for (const dir of tempDirs) {
+      try { fs.rmSync(dir, { recursive: true, force: true }); } catch { /* ignore */ }
+    }
+    tempDirs.length = 0;
+  });
+
   it('scopes browser cookies to each direct-download target domain', async () => {
     const page = createMockPage(vi.fn().mockImplementation(async (opts?: { domain?: string }) => {
       const domain = opts?.domain ?? 'unknown';
@@ -94,6 +104,38 @@ describe('stepDownload', () => {
     );
   });
 
+  it('reports duplicate destinations instead of racing them onto one path', async () => {
+    const dir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'webcmd-download-dupe-'));
+    tempDirs.push(dir);
+
+    const results = await stepDownload(
+      null,
+      {
+        url: '${{ item.url }}',
+        dir,
+        filename: 'report.pdf',
+        progress: false,
+      },
+      [
+        { url: 'https://a.example/one.pdf' },
+        { url: 'https://b.example/two.pdf' },
+        { url: 'https://c.example/three.pdf' },
+      ],
+      {},
+    ) as Array<{ _download: { status: string; error?: string } }>;
+
+    // Only the first claimant downloads; the rest fail loudly.
+    expect(mockHttpDownload).toHaveBeenCalledTimes(1);
+    expect(mockHttpDownload).toHaveBeenCalledWith(
+      'https://a.example/one.pdf',
+      path.join(dir, 'report.pdf'),
+      expect.anything(),
+    );
+    expect(results.map((row) => row._download.status)).toEqual(['success', 'failed', 'failed']);
+    expect(results[1]._download.error).toContain('Duplicate download target "report.pdf"');
+    expect(results[2]._download.error).toContain('already claimed by item 0');
+  });
+
   it('builds yt-dlp cookies from all target domains instead of only the first item', async () => {
     const getCookies = vi.fn().mockImplementation(async (opts?: { domain?: string }) => {
       const domain = opts?.domain ?? 'unknown';

src/pipeline/steps/download.ts

@@ -5,7 +5,7 @@
  * - Direct HTTP downloads (images, documents)
  * - yt-dlp integration for video platforms
  * - Browser cookie forwarding for authenticated downloads
- * - Filename templating and deduplication
+ * - Filename templating and duplicate-target detection
  */
 
 import * as fs from 'node:fs';
@@ -37,6 +37,19 @@ export interface DownloadResult {
   duration?: number;
 }
 
+/**
+ * A single item's resolved download target.
+ *
+ * `conflictsWith` is the index of the earlier item that claimed the same
+ * destination path, if any.
+ */
+interface DownloadPlan {
+  url: string;
+  filename: string;
+  destPath: string;
+  conflictsWith?: number;
+}
+
 
 
 /**
@@ -154,6 +167,32 @@ export async function stepDownload(
     return [];
   }
 
+  // Resolve every destination up front. Items run in parallel, so two of them
+  // resolving to the same path would otherwise both download and both rename,
+  // leaving whichever finished last on disk with no diagnostic. Claiming paths
+  // in item order makes the winner deterministic and the losers reportable.
+  const plans: DownloadPlan[] = items.map((item, index) => {
+    const url = String(render(urlTemplate, { args, data, item, index }));
+    if (!url) return { url: '', filename: '', destPath: '' };
+    const filename = sanitizeFilename(
+      filenameTemplate
+        ? String(render(filenameTemplate, { args, data, item, index }))
+        : generateFilename(url, index),
+    );
+    return { url, filename, destPath: path.join(dir, filename) };
+  });
+
+  const claimedBy = new Map<string, number>();
+  for (const [index, plan] of plans.entries()) {
+    if (!plan.destPath) continue;
+    const owner = claimedBy.get(plan.destPath);
+    if (owner === undefined) {
+      claimedBy.set(plan.destPath, index);
+    } else {
+      plan.conflictsWith = owner;
+    }
+  }
+
   // Create progress tracker
   const tracker = new DownloadProgressTracker(items.length, showProgress);
 
@@ -163,13 +202,9 @@ export async function stepDownload(
 
   if (page) {
     // For yt-dlp, we need to export cookies to Netscape format
-    if (useYtdlp || items.some((item, index) => {
-      const url = String(render(urlTemplate, { args, data, item, index }));
-      return requiresYtdlp(url);
-    })) {
+    if (useYtdlp || plans.some((plan) => requiresYtdlp(plan.url))) {
       try {
-        const ytdlpDomains = [...new Set(items.flatMap((item, index) => {
-          const url = String(render(urlTemplate, { args, data, item, index }));
+        const ytdlpDomains = [...new Set(plans.flatMap(({ url }) => {
           if (!useYtdlp && !requiresYtdlp(url)) return [];
           try {
             return [new URL(url).hostname];
@@ -198,8 +233,7 @@ export async function stepDownload(
   const results = await mapConcurrent(items, concurrency, async (item, index): Promise<DownloadedItem> => {
     const startTime = Date.now();
 
-    // Render URL
-    const url = String(render(urlTemplate, { args, data, item, index }));
+    const { url, filename, destPath, conflictsWith } = plans[index];
     if (!url) {
       tracker.onFileComplete(false);
       return {
@@ -208,17 +242,6 @@ export async function stepDownload(
       };
     }
 
-    // Render filename
-    let filename: string;
-    if (filenameTemplate) {
-      filename = String(render(filenameTemplate, { args, data, item, index }));
-    } else {
-      filename = generateFilename(url, index);
-    }
-    filename = sanitizeFilename(filename);
-
-    const destPath = path.join(dir, filename);
-
     // Check if file exists and skip_existing is true
     if (skipExisting && fs.existsSync(destPath)) {
       tracker.onFileComplete(true, true);
@@ -232,6 +255,18 @@ export async function stepDownload(
       };
     }
 
+    // Report rather than race: an earlier item already owns this path.
+    if (conflictsWith !== undefined) {
+      tracker.onFileComplete(false);
+      return {
+        ...item,
+        _download: {
+          status: 'failed',
+          error: `Duplicate download target "${filename}" (already claimed by item ${conflictsWith}); give each item a unique filename`,
+        } as DownloadResult,
+      };
+    }
+
     // Create progress bar for this file
     const progressBar = tracker.onFileStart(filename, index);