diff --git a/src/codecs/image.ts b/src/codecs/image.ts index 3891f0d..05f81ca 100644 --- a/src/codecs/image.ts +++ b/src/codecs/image.ts @@ -1,7 +1,8 @@ import sharp from "sharp"; -import { writeFile, mkdir } from "node:fs/promises"; -import { dirname, extname } from "node:path"; +import { writeFile } from "node:fs/promises"; +import { extname } from "node:path"; import { CompressorError } from "../core/errors.js"; +import { withAtomicOutput } from "../core/atomic-output.js"; import { sniffFile } from "../core/sniff.js"; import { isInputOnlyImageFormat } from "../types/image-formats.js"; import { @@ -108,8 +109,10 @@ export async function encodeImage( return { bytes: buffer.byteLength, write: async () => { - await mkdir(dirname(outputPath), { recursive: true }); - await writeFile(outputPath, buffer); + await withAtomicOutput(outputPath, async (temporaryPath) => { + await writeFile(temporaryPath, buffer); + return { value: undefined, replace: true }; + }); }, }; } diff --git a/src/codecs/video.ts b/src/codecs/video.ts index 58374d0..7e2d178 100644 --- a/src/codecs/video.ts +++ b/src/codecs/video.ts @@ -1,5 +1,4 @@ -import { mkdir, stat, rm } from "node:fs/promises"; -import { dirname } from "node:path"; +import { stat } from "node:fs/promises"; import { runFfmpeg, type FfmpegTools } from "./ffmpeg.js"; import { CompressorError } from "../core/errors.js"; import { @@ -388,27 +387,19 @@ export interface EncodeVideoResult { readonly bytes: number; } -/** Run one encode, cleaning up the partial file if anything goes wrong. */ +/** Run one encode into the caller-owned staging path. */ export async function encodeVideo( params: EncodeVideoParams, ): Promise { const { tools, outputPath } = params; - await mkdir(dirname(outputPath), { recursive: true }); - - try { - await runFfmpeg({ - ffmpeg: tools.ffmpeg, - args: params.args, - durationSeconds: params.durationSeconds ?? null, - ...(params.onProgress ? { onProgress: params.onProgress } : {}), - ...(params.signal ? { signal: params.signal } : {}), - }); - } catch (err) { - // A half-written file is worse than none: it looks like a successful run. - await rm(outputPath, { force: true }).catch(() => undefined); - throw err; - } + await runFfmpeg({ + ffmpeg: tools.ffmpeg, + args: params.args, + durationSeconds: params.durationSeconds ?? null, + ...(params.onProgress ? { onProgress: params.onProgress } : {}), + ...(params.signal ? { signal: params.signal } : {}), + }); const info = await stat(outputPath); return { bytes: info.size }; diff --git a/src/core/atomic-output.ts b/src/core/atomic-output.ts new file mode 100644 index 0000000..80120a6 --- /dev/null +++ b/src/core/atomic-output.ts @@ -0,0 +1,37 @@ +import { randomUUID } from "node:crypto"; +import { mkdir, rename, rm } from "node:fs/promises"; +import { basename, dirname, extname, join } from "node:path"; + +export interface AtomicOutputDecision { + readonly value: T; + readonly replace: boolean; +} + +/** + * Produce a file beside its destination and replace the destination atomically. + * + * The leading dot keeps discovery from treating an in-flight encode as input + * during a concurrent or later run. Keeping the real extension last also lets + * tools such as ffmpeg infer the correct output format. + */ +export function temporaryOutputPath(outputPath: string): string { + const extension = extname(outputPath); + const stem = basename(outputPath, extension); + return join(dirname(outputPath), `.${stem}-${randomUUID()}.tmp${extension}`); +} + +export async function withAtomicOutput( + outputPath: string, + produce: (temporaryPath: string) => Promise>, +): Promise { + await mkdir(dirname(outputPath), { recursive: true }); + const temporaryPath = temporaryOutputPath(outputPath); + + try { + const decision = await produce(temporaryPath); + if (decision.replace) await rename(temporaryPath, outputPath); + return decision.value; + } finally { + await rm(temporaryPath, { force: true }).catch(() => undefined); + } +} diff --git a/src/core/compress.ts b/src/core/compress.ts index e41babb..8d59fd6 100644 --- a/src/core/compress.ts +++ b/src/core/compress.ts @@ -1,9 +1,10 @@ -import { access, stat, rm } from "node:fs/promises"; +import { access, stat } from "node:fs/promises"; import { join, relative, resolve, basename, extname, dirname } from "node:path"; import { discoverFiles, type DiscoveredFile } from "./discover.js"; import { mapWithConcurrency, defaultConcurrency } from "./pool.js"; import { CompressorError, toFailure } from "./errors.js"; +import { withAtomicOutput } from "./atomic-output.js"; import { encodeImage, resolveImageTarget } from "../codecs/image.js"; import { encodeVideo, @@ -393,7 +394,8 @@ async function runVideoJob( job: CompressionJob, ctx: ExecuteContext, ): Promise { - if (!ctx.tools) { + const tools = ctx.tools; + if (!tools) { throw new CompressorError( "FFMPEG_NOT_FOUND", "ffmpeg is required to compress video.", @@ -412,30 +414,37 @@ async function runVideoJob( ? planStreams(extension, probe) : { plan: openStreamPlan(probe), dropped: [] as string[] }; - const args = isVideoContainer(extension) - ? curatedArgsFor(extension, job, ctx, probe, plan.plan) - : await openArgsFor(extension, job, ctx, probe, plan.plan); - - const encoded = await encodeVideo({ - tools: ctx.tools, - inputPath: job.inputPath, - outputPath: job.outputPath, - args, - durationSeconds: probe.durationSeconds, - ...(ctx.options.onProgress - ? { - onProgress: (ratio: number) => - ctx.options.onProgress?.({ type: "job-progress", job, ratio }), - } - : {}), - ...(ctx.options.signal ? { signal: ctx.options.signal } : {}), + return withAtomicOutput(job.outputPath, async (temporaryPath) => { + const args = isVideoContainer(extension) + ? curatedArgsFor(extension, job, ctx, probe, plan.plan, temporaryPath) + : await openArgsFor(extension, job, ctx, probe, plan.plan, temporaryPath); + + const encoded = await encodeVideo({ + tools, + inputPath: job.inputPath, + outputPath: temporaryPath, + args, + durationSeconds: probe.durationSeconds, + ...(ctx.options.onProgress + ? { + onProgress: (ratio: number) => + ctx.options.onProgress?.({ type: "job-progress", job, ratio }), + } + : {}), + ...(ctx.options.signal ? { signal: ctx.options.signal } : {}), + }); + + if (ctx.skipLarger && encoded.bytes >= job.inputBytes) { + return { + value: { bytes: null, warnings: plan.dropped }, + replace: false, + }; + } + return { + value: { bytes: encoded.bytes, warnings: plan.dropped }, + replace: true, + }; }); - - if (ctx.skipLarger && encoded.bytes >= job.inputBytes) { - await rm(job.outputPath, { force: true }).catch(() => undefined); - return { bytes: null, warnings: plan.dropped }; - } - return { bytes: encoded.bytes, warnings: plan.dropped }; } const EMPTY_PROBE: MediaProbe = { @@ -523,13 +532,14 @@ function curatedArgsFor( ctx: ExecuteContext, probe: MediaProbe, plan: StreamPlan | null, + outputPath: string, ): string[] { const videoCodec = resolveVideoCodec(container, ctx.options.videoCodec); const audioCodec = resolveAudioCodec(container, ctx.options.audioCodec, probe.audio); return curatedArgs({ inputPath: job.inputPath, - outputPath: job.outputPath, + outputPath, container, // Checked against the container's own matrix by the resolvers above. videoCodec: videoCodec, @@ -557,6 +567,7 @@ async function openArgsFor( ctx: ExecuteContext, probe: MediaProbe, plan: StreamPlan | null, + outputPath: string, ): Promise { const ffmpeg = ctx.tools?.ffmpeg ?? "ffmpeg"; const muxer = extension.slice(1); @@ -586,7 +597,7 @@ async function openArgsFor( return buildOpenVideoArgs({ inputPath: job.inputPath, - outputPath: job.outputPath, + outputPath, extension, // null means "let ffmpeg decide", which is always a legal choice. videoCodec: requested ?? null, diff --git a/src/core/pool.ts b/src/core/pool.ts index dc5658d..2620fd7 100644 --- a/src/core/pool.ts +++ b/src/core/pool.ts @@ -34,7 +34,15 @@ export async function mapWithConcurrency( } } - await Promise.all(Array.from({ length: workers }, () => worker())); + // An aborted sibling may still be unwinding an encoder. Waiting for every + // active worker ensures its output cleanup finishes before the run rejects. + const settled = await Promise.allSettled( + Array.from({ length: workers }, () => worker()), + ); + const rejected = settled.find( + (result): result is PromiseRejectedResult => result.status === "rejected", + ); + if (rejected) throw rejected.reason; return results; } diff --git a/test/atomic-output.test.ts b/test/atomic-output.test.ts new file mode 100644 index 0000000..601ef27 --- /dev/null +++ b/test/atomic-output.test.ts @@ -0,0 +1,86 @@ +import { afterAll, beforeAll, describe, expect, it } from "vitest"; +import { readFile, readdir, writeFile } from "node:fs/promises"; +import { basename, dirname, join } from "node:path"; + +import { tempDir } from "./helpers.js"; +import { temporaryOutputPath, withAtomicOutput } from "../src/core/atomic-output.js"; + +describe("atomic output lifecycle", () => { + let dir: string; + let cleanup: () => Promise; + + beforeAll(async () => { + ({ dir, cleanup } = await tempDir()); + }); + afterAll(() => cleanup()); + + it("uses unique hidden names that keep the destination extension last", () => { + const output = join(dir, "hero.mp4"); + const first = temporaryOutputPath(output); + const second = temporaryOutputPath(output); + + expect(dirname(first)).toBe(dir); + expect(basename(first)).toMatch(/^\.hero-.+\.tmp\.mp4$/); + expect(first).not.toBe(second); + }); + + it("atomically replaces the destination after a successful write", async () => { + const output = join(dir, "success", "hero.webp"); + await withAtomicOutput(output, async (temporaryPath) => { + await writeFile(temporaryPath, "new bytes"); + return { value: undefined, replace: true }; + }); + + expect(await readFile(output, "utf8")).toBe("new bytes"); + expect(await readdir(dirname(output))).toEqual(["hero.webp"]); + }); + + it("preserves the destination and removes the temp file after failure", async () => { + const output = join(dir, "failure", "hero.webp"); + await withAtomicOutput(output, async (temporaryPath) => { + await writeFile(temporaryPath, "old bytes"); + return { value: undefined, replace: true }; + }); + + await expect( + withAtomicOutput(output, async (temporaryPath) => { + await writeFile(temporaryPath, "partial bytes"); + throw new Error("encode failed"); + }), + ).rejects.toThrow("encode failed"); + + expect(await readFile(output, "utf8")).toBe("old bytes"); + expect(await readdir(dirname(output))).toEqual(["hero.webp"]); + }); + + it("preserves the destination when the caller declines replacement", async () => { + const output = join(dir, "skipped", "hero.webp"); + await withAtomicOutput(output, async (temporaryPath) => { + await writeFile(temporaryPath, "old bytes"); + return { value: undefined, replace: true }; + }); + + await withAtomicOutput(output, async (temporaryPath) => { + await writeFile(temporaryPath, "larger bytes"); + return { value: undefined, replace: false }; + }); + + expect(await readFile(output, "utf8")).toBe("old bytes"); + expect(await readdir(dirname(output))).toEqual(["hero.webp"]); + }); + + it("removes the temp file when the operation is aborted", async () => { + const output = join(dir, "aborted", "hero.webp"); + const abortError = new Error("aborted"); + abortError.name = "AbortError"; + + await expect( + withAtomicOutput(output, async (temporaryPath) => { + await writeFile(temporaryPath, "partial bytes"); + throw abortError; + }), + ).rejects.toBe(abortError); + + expect(await readdir(dirname(output))).toEqual([]); + }); +}); diff --git a/test/video.test.ts b/test/video.test.ts index c7e2908..d043c1b 100644 --- a/test/video.test.ts +++ b/test/video.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect, beforeAll, afterAll } from "vitest"; import { join } from "node:path"; import { spawn } from "node:child_process"; +import { mkdir, readFile, readdir, writeFile } from "node:fs/promises"; import { tempDir, makeVideo, hasFfmpeg } from "./helpers.js"; import { compressVideos } from "../src/core/compress.js"; @@ -42,6 +43,12 @@ function probeCodec(file: string, stream: "v" | "a"): Promise { return probeStream(file, "codec_name", stream); } +async function temporaryFiles(dir: string): Promise { + return (await readdir(dir)).filter( + (name) => name.startsWith(".") && name.includes(".tmp."), + ); +} + describe("video argument construction", () => { it("escapes the comma inside a scale expression", () => { // An unescaped comma is read by ffmpeg as an option separator. @@ -153,10 +160,11 @@ describe.skipIf(!(await hasFfmpeg()))("video encoding (requires ffmpeg)", () => it("encodes an MP4 with H.264", async () => { const src = join(dir, "mp4"); + const out = join(dir, "mp4-out"); await makeVideo(join(src, "clip.mp4")); const report = await compressVideos([src], { - outDir: join(dir, "mp4-out"), + outDir: out, quality: toQuality(50), // A 1s clip is mostly audio, so the re-encode can be larger than the // source and skip-larger would decline to write it. This test is about @@ -165,7 +173,76 @@ describe.skipIf(!(await hasFfmpeg()))("video encoding (requires ffmpeg)", () => }); expect(report.summary.failed).toBe(0); - expect(await probeCodec(join(dir, "mp4-out", "clip.mp4"), "v")).toBe("h264"); + expect(await probeCodec(join(out, "clip.mp4"), "v")).toBe("h264"); + expect(await temporaryFiles(out)).toEqual([]); + }, 120_000); + + it("preserves an existing output when ffmpeg fails", async () => { + const src = join(dir, "atomic-failure"); + const out = join(dir, "atomic-failure-out"); + const input = join(src, "clip.mp4"); + const output = join(out, "clip.mp4"); + await mkdir(src, { recursive: true }); + await writeFile(input, "this is not video data"); + await makeVideo(output); + const existing = await readFile(output); + + const report = await compressVideos([src], { + outDir: out, + overwrite: true, + skipLarger: false, + }); + + expect(report.results[0]?.status).toBe("failed"); + expect(await readFile(output)).toEqual(existing); + expect(await temporaryFiles(out)).toEqual([]); + }, 120_000); + + it("preserves an existing output when the new encode is larger", async () => { + const src = join(dir, "atomic-larger"); + const out = join(dir, "atomic-larger-out"); + const input = join(src, "clip.mp4"); + const output = join(out, "clip.mp4"); + await makeVideo(input); + await makeVideo(output); + const existing = await readFile(output); + + const report = await compressVideos([src], { + outDir: out, + overwrite: true, + quality: toQuality(100), + preset: "ultrafast", + }); + + const result = report.results[0]; + expect(result?.status).toBe("skipped"); + expect(result?.status === "skipped" && result.reason).toBe( + "output-larger-than-input", + ); + expect(await readFile(output)).toEqual(existing); + expect(await temporaryFiles(out)).toEqual([]); + }, 120_000); + + it("removes the temporary output when a run is aborted", async () => { + const src = join(dir, "atomic-abort"); + const out = join(dir, "atomic-abort-out"); + await makeVideo(join(src, "first.mp4")); + await makeVideo(join(src, "second.mp4")); + const controller = new AbortController(); + + const run = compressVideos([src], { + outDir: out, + signal: controller.signal, + concurrency: 2, + skipLarger: false, + onProgress: (event) => { + if (event.type === "job-start") controller.abort(); + }, + }); + + await expect(run).rejects.toMatchObject({ name: "AbortError" }); + expect(controller.signal.aborted).toBe(true); + expect(await temporaryFiles(out)).toEqual([]); }, 120_000); it("does not enlarge a small source given a large resize box", async () => {