From 2c628123b54e7026ae67b97037cd5297004bbeb5 Mon Sep 17 00:00:00 2001 From: tizius79 <51134004+tizius79@users.noreply.github.com> Date: Fri, 11 Sep 2026 23:46:05 +0000 Subject: [PATCH] fix: record every file a conversion produces Not every converter writes the single file it is asked for: ImageMagick turns a multi-page PDF into one numbered file per page (name-0.jpg, name-1.jpg, ...). handleConvert stored the expected name regardless, so the results page linked to a file that was never written and every download failed with ENOENT: no such file or directory, open './data/output/1/4/name .jpg' The conversion itself had succeeded - the log says as much - but its output was unreachable from the UI. The tar button still worked, since /archive/:jobId packs the whole directory and ignores the recorded names. Probably the same root cause as #321. Each conversion now runs in a directory of its own and its results are moved into place afterwards, so the files it produced are known rather than inferred: an input whose name overlaps the numbered pattern can no longer be credited with another input's output. One row is recorded per produced file, and jobs.num_files is raised to match - without that, results.tsx waits forever on a count that can never be reached and leaves the delete and tar buttons disabled. Output names are handed out once per job. Two inputs can genuinely produce the same name - report.pdf split into pages and report-1.pdf both write report-1.jpg - and moving the second over the first would lose a page and leave a row pointing at another conversion's content, so the later arrival is given a name of its own. The move falls back to a copy when rename reports EXDEV, directories a converter leaves behind are not mistaken for output files, and the scratch directory name carries a random component so two conversions of one job cannot share it. Tested against both images by converting a 3-page PDF to JPEG and following the download link the results page actually renders: v0.18.0 answers HTTP 500, this branch serves all three pages. Overlapping names keep every page, each under a distinct name. Single-file conversions are unchanged. prettier, tsc and eslint pass. Co-Authored-By: Claude Opus 5 --- src/converters/main.ts | 202 +++++++++++++++++++++++++++++++++++------ 1 file changed, 172 insertions(+), 30 deletions(-) diff --git a/src/converters/main.ts b/src/converters/main.ts index 6980cc69..1e35da0b 100644 --- a/src/converters/main.ts +++ b/src/converters/main.ts @@ -1,3 +1,5 @@ +import { randomUUID } from "node:crypto"; +import { mkdir, readdir, rename, rm, unlink } from "node:fs/promises"; import { Cookie } from "elysia"; import db from "../db/db"; import { MAX_CONVERT_PROCESS } from "../helpers/env"; @@ -153,6 +155,82 @@ function chunks(arr: T[], size: number): T[][] { ); } +/** + * Not every converter writes the single file it was asked for: ImageMagick + * turns a multi-page PDF into one numbered file per page (`name-0.jpg`, + * `name-1.jpg`, ...). Work out which of the files a conversion produced should + * be offered for download. + */ +function pickOutputNames(produced: string[], expectedName: string): string[] { + if (produced.includes(expectedName)) { + return [expectedName]; + } + + const extIndex = expectedName.lastIndexOf("."); + const base = extIndex === -1 ? expectedName : expectedName.slice(0, extIndex); + const ext = extIndex === -1 ? "" : expectedName.slice(extIndex); + + const numbered = produced + .map((name) => { + if (!name.startsWith(`${base}-`) || !name.endsWith(ext)) { + return null; + } + const suffix = name.slice(base.length + 1, name.length - ext.length); + return /^\d+$/.test(suffix) ? { name, index: Number(suffix) } : null; + }) + .filter((match) => match !== null) + .sort((a, b) => a.index - b.index) + .map((match) => match.name); + + // Nothing recognisable was written, which is what a failed conversion looks + // like. Keep the expected name so those rows read as they always have. + return numbered.length > 0 ? numbered : [expectedName]; +} + +/** + * Uploads and output may sit on different filesystems, where rename fails with + * EXDEV, so fall back to a copy when it does. + */ +async function moveInto(sourceDir: string, targetDir: string, name: string, destination: string) { + try { + await rename(`${sourceDir}${name}`, `${targetDir}${destination}`); + } catch (error) { + if ((error as NodeJS.ErrnoException)?.code !== "EXDEV") { + throw error; + } + + await Bun.write(`${targetDir}${destination}`, Bun.file(`${sourceDir}${name}`)); + await unlink(`${sourceDir}${name}`); + } +} + +/** + * Two inputs of one job can produce the same output name: `report.pdf` split + * into pages and `report-1.pdf` both write `report-1.jpg`. Moving the second + * over the first would lose a page and leave a result row pointing at another + * conversion's content, so hand the later arrival a name of its own. + */ +function claimDestination(claimed: Set, name: string): string { + if (!claimed.has(name)) { + claimed.add(name); + return name; + } + + const extIndex = name.lastIndexOf("."); + const base = extIndex === -1 ? name : name.slice(0, extIndex); + const ext = extIndex === -1 ? "" : name.slice(extIndex); + + let suffix = 2; + let candidate = `${base} (${suffix})${ext}`; + while (claimed.has(candidate)) { + suffix += 1; + candidate = `${base} (${suffix})${ext}`; + } + + claimed.add(candidate); + return candidate; +} + export async function handleConvert( fileNames: string[], userUploadsDir: string, @@ -165,36 +243,100 @@ export async function handleConvert( "INSERT INTO file_names (job_id, file_name, output_file_name, status) VALUES (?1, ?2, ?3, ?4)", ); - for (const chunk of chunks(fileNames, MAX_CONVERT_PROCESS)) { - const toProcess: Promise[] = []; - for (const fileName of chunk) { - const filePath = `${userUploadsDir}${fileName}`; - const fileTypeOrig = fileName.includes(".") ? (fileName.split(".").pop() ?? "") : ""; - const fileType = normalizeFiletype(fileTypeOrig); - const newFileExt = normalizeOutputFiletype(convertTo); - let newFileName: string; - if (fileTypeOrig === "") { - newFileName = `${fileName}.${newFileExt}`; - } else { - newFileName = fileName.replace( - new RegExp(`${fileTypeOrig}(?!.*${fileTypeOrig})`), - newFileExt, - ); - } - const targetPath = `${userOutputDir}${newFileName}`; - toProcess.push( - new Promise((resolve, reject) => { - mainConverter(filePath, fileType, convertTo, targetPath, {}, converterName) - .then((r) => { - if (jobId.value) { - query.run(jobId.value, fileName, newFileName, r); - } - resolve(r); - }) - .catch((c) => reject(c)); - }), - ); - } + const newFileExt = normalizeOutputFiletype(convertTo); + // Should two conversions of the same job ever overlap, this keeps them from + // sharing a scratch directory. + const runId = randomUUID().slice(0, 8); + const conversions = fileNames.map((fileName, index) => { + const fileTypeOrig = fileName.includes(".") ? (fileName.split(".").pop() ?? "") : ""; + const newFileName = + fileTypeOrig === "" + ? `${fileName}.${newFileExt}` + : fileName.replace(new RegExp(`${fileTypeOrig}(?!.*${fileTypeOrig})`), newFileExt); + + return { + fileName, + filePath: `${userUploadsDir}${fileName}`, + fileType: normalizeFiletype(fileTypeOrig), + newFileName, + // Each conversion writes into a directory of its own, so whatever lands + // there was produced by that conversion and by nothing else. Two inputs + // whose names overlap (`report.pdf` and `report-1.pdf`) can no longer be + // confused for one another. It lives under the uploads directory, which + // is neither served nor archived. + scratchDir: `${userUploadsDir}.output-${runId}-${index}/`, + }; + }); + + // Output names handed out so far, so the conversions started here never + // overwrite one another. A job converted a second time still overwrites its + // earlier output, exactly as it did before. + const claimed = new Set(); + + for (const chunk of chunks(conversions, MAX_CONVERT_PROCESS)) { + const toProcess = chunk.map( + async ({ fileName, filePath, fileType, newFileName, scratchDir }) => { + await mkdir(scratchDir, { recursive: true }); + + try { + const status = await mainConverter( + filePath, + fileType, + convertTo, + `${scratchDir}${newFileName}`, + {}, + converterName, + ); + + // Directories are skipped: a converter leaving a working directory + // behind is not an output file. + let produced: string[] = []; + try { + produced = (await readdir(scratchDir, { withFileTypes: true })) + .filter((entry) => entry.isFile()) + .map((entry) => entry.name); + } catch { + produced = []; + } + + // Claimed in one go, before any await, so parallel conversions in the + // same chunk cannot be handed the same destination. + const destinations = new Map( + produced.map((name) => [name, claimDestination(claimed, name)] as const), + ); + + const outputNames = pickOutputNames(produced, newFileName).map( + (name) => destinations.get(name) ?? name, + ); + + for (const [name, destination] of destinations) { + await moveInto(scratchDir, userOutputDir, name, destination); + } + + if (jobId.value) { + for (const outputName of outputNames) { + query.run(jobId.value, fileName, outputName, status); + } + + if (outputNames.length > 1) { + // One input produced several files, so the job now holds more + // files than it was created with. Without this the results page + // waits forever for a count that can never be reached, leaving + // the delete and tar buttons disabled. + db.query("UPDATE jobs SET num_files = num_files + ?1 WHERE id = ?2").run( + outputNames.length - 1, + jobId.value, + ); + } + } + + return status; + } finally { + await rm(scratchDir, { recursive: true, force: true }); + } + }, + ); + await Promise.all(toProcess); } }