From d55b9d8359a57f3db71616fb160bb35324d13551 Mon Sep 17 00:00:00 2001 From: Kaushik Samadder Date: Tue, 18 Aug 2026 09:19:22 +0530 Subject: [PATCH] fix(download): isolate temp files and reject duplicate download targets httpDownload wrote every attempt to `${destPath}.tmp`. The download pipeline step runs items in parallel (3 by default), so two items whose filename template or generateFilename() fallback produced the same name shared one temp file: both write streams truncated and appended to it, both renamed it to the destination, and one attempt's cleanupTempFile() could delete the other's in-flight data. The surviving file was a blend of two downloads, or the second rename failed with ENOENT. Give each attempt its own temp path (pid + random suffix, beside the destination so the rename stays atomic), matching the temp-write naming already used in hosted/files.ts. That stops the corruption but still leaves two items silently racing for one path, so the step now resolves all destinations up front, lets the lowest-index item claim each path, and fails the rest with a named conflict instead of overwriting. Resolving up front also removes the duplicate URL renders in the yt-dlp cookie pre-scan. --- src/download/index.test.ts | 42 ++++++++++++++++ src/download/index.ts | 17 ++++++- src/pipeline/steps/download.test.ts | 44 ++++++++++++++++- src/pipeline/steps/download.ts | 75 +++++++++++++++++++++-------- 4 files changed, 156 insertions(+), 22 deletions(-) diff --git a/src/download/index.test.ts b/src/download/index.test.ts index 6f446ebd..f14cd313 100644 --- a/src/download/index.test.ts +++ b/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((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); diff --git a/src/download/index.ts b/src/download/index.ts index b4635438..51b8b806 100644 --- a/src/download/index.ts +++ b/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 { diff --git a/src/pipeline/steps/download.test.ts b/src/pipeline/steps/download.test.ts index afdd0216..a0fd815e 100644 --- a/src/pipeline/steps/download.test.ts +++ b/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'; diff --git a/src/pipeline/steps/download.ts b/src/pipeline/steps/download.ts index 15f92b02..1bb24bfd 100644 --- a/src/pipeline/steps/download.ts +++ b/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(); + 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 => { 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);