a photograph may be slow, but it may not take the machine down
ImageMagick sizes its pixel cache against the machine and will hold an entire decoded image in RAM. On a host with room that is right. On a small one it is how resizing a photograph kills whatever else is running — and "whatever else" includes the website this service exists to serve, which is not an acceptable way for an image to fail. So the decode runs under `-limit memory` / `-limit map` / `-limit disk`. Past the caps ImageMagick spills to a temporary file and carries on, which turns an out-of-memory kill of whatever the kernel picks into a conversion that is slower. Nobody minds a slow photograph; everybody minds a site that went down while one was being resized. Measured on fofos. A 12 MP HEIC peaks at 145 MB, so the 256 MB default does not bind and the output is byte-identical — it is a ceiling, not a tax. Forced to bind, on a 36 MP image with a 32 MiB cap, peak RSS is 12 MB and it still exits 0 with the file written. `-limit thread 1` for the same reason: one decode is one image, and each thread carries its own share of the cache the caps exist to bound. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -65,6 +65,21 @@ export const WORK_ROOT = (() => {
|
|||||||
|
|
||||||
export const MAX_UPLOAD_BYTES = num("AV_CLUB_MAX_UPLOAD_BYTES", 512 * 1024 * 1024, 1024);
|
export const MAX_UPLOAD_BYTES = num("AV_CLUB_MAX_UPLOAD_BYTES", 512 * 1024 * 1024, 1024);
|
||||||
|
|
||||||
|
/**
|
||||||
|
* How much RAM ImageMagick may hold while decoding, before it spills to a temporary file.
|
||||||
|
*
|
||||||
|
* Deliberately modest. This is the one operation here that can allocate against the size of the
|
||||||
|
* PICTURE rather than against anything this service chose, so it is the one that can take a small
|
||||||
|
* machine down — and taking a machine down is a worse outcome than being slow, every time, because
|
||||||
|
* the other things on it did not ask to be part of an image conversion.
|
||||||
|
*
|
||||||
|
* 256 MB comfortably holds the working set of a phone photograph. Past it nothing fails: ImageMagick
|
||||||
|
* memory-maps and then falls back to disk, so a genuinely enormous image is slower rather than fatal.
|
||||||
|
*
|
||||||
|
* Raise it on a machine with headroom. See sharp/decodable.ts for how it is applied.
|
||||||
|
*/
|
||||||
|
export const CONVERT_MEMORY_MB = num("AV_CLUB_CONVERT_MEMORY_MB", 256, 32);
|
||||||
|
|
||||||
/** A crafted file can make an encoder run effectively forever. The process is killed at this point. */
|
/** A crafted file can make an encoder run effectively forever. The process is killed at this point. */
|
||||||
export const JOB_TIMEOUT_MS = num("AV_CLUB_JOB_TIMEOUT_MS", 120_000, 1_000);
|
export const JOB_TIMEOUT_MS = num("AV_CLUB_JOB_TIMEOUT_MS", 120_000, 1_000);
|
||||||
|
|
||||||
|
|||||||
@@ -1,7 +1,7 @@
|
|||||||
import { join } from "node:path";
|
import { join } from "node:path";
|
||||||
import { mkdtemp, rm } from "node:fs/promises";
|
import { mkdtemp, rm } from "node:fs/promises";
|
||||||
import { tmpdir } from "node:os";
|
import { tmpdir } from "node:os";
|
||||||
import { JOB_TIMEOUT_MS } from "../config";
|
import { CONVERT_MEMORY_MB, JOB_TIMEOUT_MS } from "../config";
|
||||||
import { unprocessable } from "../errors";
|
import { unprocessable } from "../errors";
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@@ -52,12 +52,41 @@ export async function decodableInput(path: string, format: string): Promise<Deco
|
|||||||
const workDir = await mkdtemp(join(tmpdir(), "avclub-heif-"));
|
const workDir = await mkdtemp(join(tmpdir(), "avclub-heif-"));
|
||||||
const converted = join(workDir, "decoded.png");
|
const converted = join(workDir, "decoded.png");
|
||||||
|
|
||||||
// ImageMagick 6 on Ubuntu 24.04, so `convert` — there is no `magick`. Spawned as an argv array,
|
/*
|
||||||
// so the path is data rather than something a shell parses.
|
ImageMagick 6 on Ubuntu 24.04, so `convert` — there is no `magick`. Spawned as an argv array, so
|
||||||
//
|
the path is data rather than something a shell parses.
|
||||||
// -auto-orient applies the EXIF rotation while the metadata still exists; the PNG carries none, so
|
|
||||||
// this is the last chance to get a phone photograph the right way up.
|
`-auto-orient` applies the EXIF rotation while the metadata still exists; the PNG carries none, so
|
||||||
const proc = Bun.spawn(["convert", path, "-auto-orient", converted], {
|
this is the last chance to get a phone photograph the right way up.
|
||||||
|
|
||||||
|
── THE LIMITS ARE THE POINT, AND THEY GO FIRST ──
|
||||||
|
|
||||||
|
Left alone, ImageMagick sizes its pixel cache against the machine and will happily hold an entire
|
||||||
|
decoded image in RAM. On a box with room that is exactly right. On a small one it is how a
|
||||||
|
photograph takes down everything else on the host — and "everything else" here includes the website
|
||||||
|
this service exists to serve, which is not an acceptable way for an image to fail.
|
||||||
|
|
||||||
|
`-limit memory` caps the in-RAM cache; `-limit map` caps the memory-mapped extension of it. Past
|
||||||
|
both, ImageMagick spills to a temporary FILE and carries on. So the failure mode changes from an
|
||||||
|
out-of-memory kill of whatever the kernel picks to a conversion that is slower than it would have
|
||||||
|
been. That is a trade worth making unconditionally: nobody minds a slow photograph, and everybody
|
||||||
|
minds a website that went down while one was being resized.
|
||||||
|
|
||||||
|
These are DEFAULTS, not a policy — `AV_CLUB_CONVERT_MEMORY_MB` raises them on a machine with the
|
||||||
|
headroom to be quicker.
|
||||||
|
*/
|
||||||
|
const limits = [
|
||||||
|
"-limit", "memory", `${CONVERT_MEMORY_MB}MiB`,
|
||||||
|
"-limit", "map", `${CONVERT_MEMORY_MB * 2}MiB`,
|
||||||
|
// The spill has to land somewhere, and unbounded is a full disk instead of a full memory. Ten
|
||||||
|
// times the memory cap is far more than any real photograph needs and still a number.
|
||||||
|
"-limit", "disk", `${CONVERT_MEMORY_MB * 10}MiB`,
|
||||||
|
// One decode is one image. Threads here buy little and each carries its own share of the cache,
|
||||||
|
// which is the opposite of what the caps above are for.
|
||||||
|
"-limit", "thread", "1",
|
||||||
|
];
|
||||||
|
|
||||||
|
const proc = Bun.spawn(["convert", ...limits, path, "-auto-orient", converted], {
|
||||||
stdout: "pipe",
|
stdout: "pipe",
|
||||||
stderr: "pipe",
|
stderr: "pipe",
|
||||||
stdin: "ignore",
|
stdin: "ignore",
|
||||||
|
|||||||
Reference in New Issue
Block a user