Skip to content

fix: record every output file when a converter splits its input #646

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tizius79 wants to merge 1 commit into
base: main
Choose a base branch
from
+172 −30
Open

fix: record every output file when a converter splits its input #646

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
202 changes: 172 additions & 30 deletions src/converters/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -153,6 +155,82 @@ function chunks<T>(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<string>, 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,
Expand All @@ -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<string>[] = [];
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<string>();

@cubic-dev-ai cubic-dev-ai Bot Sep 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: When two conversions for the same job overlap or a job is retried, each handleConvert call has its own claimed set, so the later rename can overwrite the earlier conversion's output and both rows point to the same content. Share destination claims across calls or reserve names atomically in the output directory.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At src/converters/main.ts, line 272:

<comment>When two conversions for the same job overlap or a job is retried, each `handleConvert` call has its own `claimed` set, so the later rename can overwrite the earlier conversion's output and both rows point to the same content. Share destination claims across calls or reserve names atomically in the output directory.</comment>

<file context>
@@ -241,6 +268,9 @@ export async function handleConvert(
   });
 
+  // Output names handed out so far, so conversions never overwrite each other.
+  const claimed = new Set<string>();
+
   for (const chunk of chunks(conversions, MAX_CONVERT_PROCESS)) {
</file context>

@tizius79 tizius79 Sep 12, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Real, but it predates this PR and this change does not widen it. Converters used to write straight into the output directory, so a job converted a second time overwrote its earlier files in exactly the same way, with the older rows already pointing at the newer content.

Reserving names on disk across calls would change that behaviour rather than restore it: a job converted twice would start accumulating name (2).jpg, name (3).jpg, which seems worse than the problem it solves and well outside a fix for broken downloads.

For genuinely concurrent conversions this branch is already an improvement. Each conversion writes into a directory of its own, so two overlapping runs can no longer interleave writes to the same path; the loser of a rename is a whole file rather than a half-written one.

The comment above claimed did promise more than the code delivers, though, so I have corrected it to say the guarantee holds for the conversions started by that call.


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);
}
}
Expand Down