diff --git a/server/lib/image-ops-core.js b/server/lib/image-ops-core.js new file mode 100644 index 0000000..6489c97 --- /dev/null +++ b/server/lib/image-ops-core.js @@ -0,0 +1,111 @@ +'use strict'; + +/* + * Pure-JavaScript image operations — the two things the ingest path ever asked sharp for: + * measure an image, and write a thumbnail. + * + * THIS FILE IS THE WORK, NOT THE ENTRY POINT. Callers use ./image-ops, which runs these on a + * worker thread; everything here is CPU-bound pure JS that would otherwise stall the event loop + * for ~1s per 12MP photo. Requiring this module directly is only correct inside the worker (or in + * image-ops' inline fallback). See ./image-ops for why. + * + * WHY NOT SHARP: sharp is a native module wrapping libvips. That costs us a prebuilt binary per + * platform/ABI, and when there isn't one (or Node moves ABI) the failure is + * ERR_DLOPEN_FAILED/NODE_MODULE_VERSION at require time — the same class of breakage + * lib/preflight-deps.js exists to explain for better-sqlite3. Nothing in here is native, so the + * server runs anywhere Node runs, including the embedded targets that have no toolchain. + * + * FORMAT COVERAGE vs the sharp it replaces: + * jpeg png gif tiff bmp Jimp, natively + * webp avif @jsquash/* — WebAssembly, bundled, no network (see wasmDecode below) + * svg never reaches here; callers thumbnail an SVG with itself + * heic unsupported — and it already was. sharp lists `heif`, but its + * prebuilt libvips has AV1 only and refuses HEVC ("Unsupported + * compression"), so .heic uploads have never produced a thumbnail. + * + * ORIENTATION (#170): Jimp applies EXIF orientation when it decodes and rewrites the tag to 1, + * so what comes back is already DISPLAY dimensions — the rotation sharp needed an explicit + * .rotate() for. metadata() therefore reports orientation 1 and lets imageDisplayDims() run as a + * no-op rather than swapping W/H a second time. Report the tag honestly and that helper stays + * correct for any future decoder that does NOT auto-orient. + */ + +const path = require('path'); +const fs = require('fs'); +const { sniffMime } = require('./upload-sniff'); + +// Jimp is ESM-first but ships a CJS entry; require() is fine and keeps this file loadable from +// the CommonJS server. Deferred so a caller that never touches an image never pays for it. +let _jimp = null; +function jimp() { + if (!_jimp) _jimp = require('jimp'); + return _jimp; +} + +/* + * @jsquash's decoders are browser-first: they locate their .wasm with + * `fetch(new URL('...wasm', import.meta.url))`. Under Node that URL is a file:// one and Node's + * fetch does not implement file://, so the bundled binary never loads and the only symptom is a + * bare "fetch failed". The binary IS on disk in the package — read and compile it ourselves, then + * hand the Module to init(). No network, at install time or after. + */ +const WASM_CODECS = { + 'image/webp': { pkg: '@jsquash/webp', wasm: '@jsquash/webp/codec/dec/webp_dec.wasm' }, + 'image/avif': { pkg: '@jsquash/avif', wasm: '@jsquash/avif/codec/dec/avif_dec.wasm' }, +}; +const decoderCache = new Map(); + +async function wasmDecode(mime, buf) { + const spec = WASM_CODECS[mime]; + if (!spec) return null; + if (!decoderCache.has(mime)) { + decoderCache.set(mime, (async () => { + const mod = await import(`${spec.pkg}/decode.js`); + await mod.init(await WebAssembly.compile(fs.readFileSync(require.resolve(spec.wasm)))); + return mod.default; + })()); + } + const decode = await decoderCache.get(mime); + return decode(buf); // -> ImageData-ish { data, width, height } +} + +/* + * Decode to a Jimp image whatever the format. Reuses sniffMime rather than carrying a second copy + * of the magic-byte table — routes/media.js already duplicating it once is noted there as a smell. + * Throws on anything undecodable, which is the contract callers already handle (a failure yields + * null metadata and no thumbnail, never a lost upload). + */ +async function readImage(src) { + const buf = await fs.promises.readFile(src); + const mime = sniffMime(buf); + + if (WASM_CODECS[mime]) { + const raw = await wasmDecode(mime, buf); + if (!raw) throw new Error(`no decoder for ${mime}`); + return jimp().Jimp.fromBitmap({ data: Buffer.from(raw.data), width: raw.width, height: raw.height }); + } + return jimp().Jimp.read(buf); +} + +/* + * Display dimensions, shaped like the sharp metadata the callers already destructure. + * orientation is 1 because the decode above already applied it — see ORIENTATION note at the top. + */ +async function metadata(src) { + const img = await readImage(src); + return { width: img.bitmap.width, height: img.bitmap.height, orientation: 1 }; +} + +/* + * Write a JPEG thumbnail `width` px wide, aspect preserved — sharp's + * .rotate().resize(width).jpeg({quality}).toFile(). Rotation is implicit in the decode. + * Never upscales: sharp's resize() would enlarge a small source, but a thumbnail bigger than its + * original is pure waste, and the callers only ever shrink. + */ +async function writeThumbnail(src, destPath, width, quality = 70) { + const img = await readImage(src); + if (img.bitmap.width > width) img.resize({ w: width }); + await fs.promises.writeFile(destPath, await img.getBuffer('image/jpeg', { quality })); +} + +module.exports = { metadata, writeThumbnail, readImage }; diff --git a/server/lib/image-ops-worker.js b/server/lib/image-ops-worker.js new file mode 100644 index 0000000..4964f24 --- /dev/null +++ b/server/lib/image-ops-worker.js @@ -0,0 +1,29 @@ +'use strict'; + +/* + * Worker-thread host for image-ops-core. One job per message, one reply per job, keyed by id. + * + * Deliberately thin: every decision (queueing, lifecycle, fallback) lives in ../lib/image-ops so + * there is one place to reason about them. This end only does the work and reports what happened. + * + * Errors come back as a message rather than a thrown exception, so one undecodable upload does + * not tear down the worker and take the queued jobs of unrelated callers with it. + */ + +const { parentPort } = require('worker_threads'); +const core = require('./image-ops-core'); + +const OPS = { + metadata: (job) => core.metadata(job.src), + writeThumbnail: (job) => core.writeThumbnail(job.src, job.dest, job.width, job.quality), +}; + +parentPort.on('message', async (job) => { + try { + const op = OPS[job.op]; + if (!op) throw new Error(`unknown image op: ${job.op}`); + parentPort.postMessage({ id: job.id, ok: true, result: await op(job) }); + } catch (err) { + parentPort.postMessage({ id: job.id, ok: false, error: err && err.message ? err.message : String(err) }); + } +}); diff --git a/server/lib/image-ops.js b/server/lib/image-ops.js index f9918e3..13f65a3 100644 --- a/server/lib/image-ops.js +++ b/server/lib/image-ops.js @@ -1,106 +1,138 @@ 'use strict'; /* - * Pure-JavaScript image operations — the two things the ingest path ever asked sharp for: - * measure an image, and write a thumbnail. + * Image operations, off the main thread. * - * WHY NOT SHARP: sharp is a native module wrapping libvips. That costs us a prebuilt binary per - * platform/ABI, and when there isn't one (or Node moves ABI) the failure is - * ERR_DLOPEN_FAILED/NODE_MODULE_VERSION at require time — the same class of breakage - * lib/preflight-deps.js exists to explain for better-sqlite3. Nothing in here is native, so the - * server runs anywhere Node runs, including the embedded targets that have no toolchain. + * WHY THIS EXISTS: image-ops-core is pure JavaScript, so unlike the native sharp it replaced — + * which handed work to a libvips threadpool — its CPU cost lands on whatever thread calls it. A + * 12MP photo measures at ~1.0s of solid, uninterruptible main-thread work. That is not a slow + * upload, it is a stalled event loop: no heartbeats, no socket traffic, nothing. lib/thumbnail- + * backfill.js walks an entire content library at boot, so in-process it reproduces #240 exactly + * (blocked loop -> missed heartbeats -> panels marked offline -> reconnect churn), arriving from + * our own maintenance. The same reasoning already moved this file's video branch from + * execFileSync to execFile; this is that fix for the image branch. * - * FORMAT COVERAGE vs the sharp it replaces: - * jpeg png gif tiff bmp Jimp, natively - * webp avif @jsquash/* — WebAssembly, bundled, no network (see wasmDecode below) - * svg never reaches here; callers thumbnail an SVG with itself - * heic unsupported — and it already was. sharp lists `heif`, but its - * prebuilt libvips has AV1 only and refuses HEVC ("Unsupported - * compression"), so .heic uploads have never produced a thumbnail. + * The work is therefore hosted on a worker thread and this module is the only entry point. * - * ORIENTATION (#170): Jimp applies EXIF orientation when it decodes and rewrites the tag to 1, - * so what comes back is already DISPLAY dimensions — the rotation sharp needed an explicit - * .rotate() for. metadata() therefore reports orientation 1 and lets imageDisplayDims() run as a - * no-op rather than swapping W/H a second time. Report the tag honestly and that helper stays - * correct for any future decoder that does NOT auto-orient. + * ONE JOB AT A TIME, deliberately. Decoding holds a full RGBA bitmap — a 12MP photo is ~48MB — so + * letting jobs overlap multiplies peak memory by the queue depth, which is exactly the wrong + * failure on the small targets this whole change is meant to reach. Serialized, the ceiling is one + * image regardless of how many uploads land at once. It also costs nothing in throughput: the work + * is CPU-bound, and a single busy worker already saturates the core it runs on. + * + * The worker is unref'd while idle so it never holds the process open — scripts/backfill-rotation- + * dims.js is a CLI that must exit, and `node --test` would otherwise hang forever — and ref'd only + * while a job is in flight, so an in-progress thumbnail cannot be cut short by the process exiting. */ const path = require('path'); -const fs = require('fs'); -const { sniffMime } = require('./upload-sniff'); -// Jimp is ESM-first but ships a CJS entry; require() is fine and keeps this file loadable from -// the CommonJS server. Deferred so a caller that never touches an image never pays for it. -let _jimp = null; -function jimp() { - if (!_jimp) _jimp = require('jimp'); - return _jimp; +const WORKER_PATH = path.join(__dirname, 'image-ops-worker.js'); +const IDLE_SHUTDOWN_MS = 60_000; // release the decoder heap (jimp + the WASM codecs) when quiet + +let worker = null; +let idleTimer = null; +let inFlight = null; // { id, resolve, reject } — at most one, by design +let inlineOnly = false; // set if a worker cannot be created at all; see runInline +let nextId = 1; +const queue = []; + +function clearIdleTimer() { + if (idleTimer) { clearTimeout(idleTimer); idleTimer = null; } } -/* - * @jsquash's decoders are browser-first: they locate their .wasm with - * `fetch(new URL('...wasm', import.meta.url))`. Under Node that URL is a file:// one and Node's - * fetch does not implement file://, so the bundled binary never loads and the only symptom is a - * bare "fetch failed". The binary IS on disk in the package — read and compile it ourselves, then - * hand the Module to init(). No network, at install time or after. - */ -const WASM_CODECS = { - 'image/webp': { pkg: '@jsquash/webp', wasm: '@jsquash/webp/codec/dec/webp_dec.wasm' }, - 'image/avif': { pkg: '@jsquash/avif', wasm: '@jsquash/avif/codec/dec/avif_dec.wasm' }, -}; -const decoderCache = new Map(); - -async function wasmDecode(mime, buf) { - const spec = WASM_CODECS[mime]; - if (!spec) return null; - if (!decoderCache.has(mime)) { - decoderCache.set(mime, (async () => { - const mod = await import(`${spec.pkg}/decode.js`); - await mod.init(await WebAssembly.compile(fs.readFileSync(require.resolve(spec.wasm)))); - return mod.default; - })()); - } - const decode = await decoderCache.get(mime); - return decode(buf); // -> ImageData-ish { data, width, height } +function scheduleIdleShutdown() { + clearIdleTimer(); + if (!worker || inFlight || queue.length) return; + idleTimer = setTimeout(() => { + idleTimer = null; + if (worker && !inFlight && !queue.length) { const w = worker; worker = null; w.terminate(); } + }, IDLE_SHUTDOWN_MS); + idleTimer.unref?.(); } -/* - * Decode to a Jimp image whatever the format. Reuses sniffMime rather than carrying a second copy - * of the magic-byte table — routes/media.js already duplicating it once is noted there as a smell. - * Throws on anything undecodable, which is the contract callers already handle (a failure yields - * null metadata and no thumbnail, never a lost upload). - */ -async function readImage(src) { - const buf = await fs.promises.readFile(src); - const mime = sniffMime(buf); - - if (WASM_CODECS[mime]) { - const raw = await wasmDecode(mime, buf); - if (!raw) throw new Error(`no decoder for ${mime}`); - return jimp().Jimp.fromBitmap({ data: Buffer.from(raw.data), width: raw.width, height: raw.height }); - } - return jimp().Jimp.read(buf); +// Reject everything outstanding. Called when the worker dies underneath us — a crash means OOM or +// a bug, not a bad image (image-ops-worker catches decode failures and replies normally), so there +// is nothing to usefully retry and callers already treat a rejection as "no metadata". +function failAll(reason) { + const dead = [inFlight, ...queue].filter(Boolean); + inFlight = null; + queue.length = 0; + for (const job of dead) job.reject(new Error(reason)); } -/* - * Display dimensions, shaped like the sharp metadata the callers already destructure. - * orientation is 1 because the decode above already applied it — see ORIENTATION note at the top. - */ -async function metadata(src) { - const img = await readImage(src); - return { width: img.bitmap.width, height: img.bitmap.height, orientation: 1 }; +function ensureWorker() { + if (worker) return worker; + const { Worker } = require('worker_threads'); + worker = new Worker(WORKER_PATH); + worker.unref(); + worker.on('message', (msg) => { + const job = inFlight; + if (!job || job.id !== msg.id) return; // a reply from a terminated generation; ignore + inFlight = null; + if (msg.ok) job.resolve(msg.result); else job.reject(new Error(msg.error)); + pump(); + }); + worker.on('error', (err) => { worker = null; failAll(`image worker failed: ${err.message}`); }); + worker.on('exit', (code) => { + worker = null; + if (inFlight || queue.length) failAll(`image worker exited (code ${code})`); + }); + return worker; } -/* - * Write a JPEG thumbnail `width` px wide, aspect preserved — sharp's - * .rotate().resize(width).jpeg({quality}).toFile(). Rotation is implicit in the decode. - * Never upscales: sharp's resize() would enlarge a small source, but a thumbnail bigger than its - * original is pure waste, and the callers only ever shrink. - */ -async function writeThumbnail(src, destPath, width, quality = 70) { - const img = await readImage(src); - if (img.bitmap.width > width) img.resize({ w: width }); - await fs.promises.writeFile(destPath, await img.getBuffer('image/jpeg', { quality })); +function pump() { + if (inFlight) return; + if (!queue.length) { worker?.unref(); scheduleIdleShutdown(); return; } + clearIdleTimer(); + inFlight = queue.shift(); + const w = ensureWorker(); + w.ref(); // a job is running: hold the process open until it finishes + w.postMessage(inFlight.job); } -module.exports = { metadata, writeThumbnail, readImage }; +// Last resort: if worker_threads cannot give us a thread at all, do the work in-process rather +// than refuse to thumbnail. Stalls the loop — that is the bug this module exists to avoid — so it +// is announced rather than silent. +async function runInline(job) { + const core = require('./image-ops-core'); + return job.op === 'metadata' + ? core.metadata(job.src) + : core.writeThumbnail(job.src, job.dest, job.width, job.quality); +} + +function submit(job) { + if (inlineOnly) return runInline(job); + job.id = nextId++; + return new Promise((resolve, reject) => { + try { + ensureWorker(); + } catch (err) { + inlineOnly = true; + console.warn(`[image-ops] no worker thread (${err.message}) — decoding in-process, which blocks the event loop`); + return resolve(runInline(job)); + } + queue.push({ id: job.id, job, resolve, reject }); + pump(); + }); +} + +/* Display dimensions, shaped like the sharp metadata callers destructure. See image-ops-core. */ +function metadata(src) { + return submit({ op: 'metadata', src }); +} + +/* Write a JPEG thumbnail `width` px wide, aspect preserved. Rotation is implicit in the decode. */ +function writeThumbnail(src, dest, width, quality = 70) { + return submit({ op: 'writeThumbnail', src, dest, width, quality }); +} + +/* Drop the worker now rather than waiting out the idle timer. For shutdown paths and tests. */ +async function shutdown() { + clearIdleTimer(); + const w = worker; + worker = null; + if (w) await w.terminate(); +} + +module.exports = { metadata, writeThumbnail, shutdown }; diff --git a/server/test/image-ops.test.js b/server/test/image-ops.test.js new file mode 100644 index 0000000..784100b --- /dev/null +++ b/server/test/image-ops.test.js @@ -0,0 +1,90 @@ +'use strict'; + +// Image decoding is pure JavaScript now (no native sharp), so its CPU cost lands on whichever +// thread runs it — ~1s of solid work for a 12MP photo. In-process that is a stalled event loop: +// no heartbeats, no socket traffic, panels marked offline, reconnect churn — #240 arriving from +// our own thumbnail backfill. lib/image-ops therefore hosts the work on a worker thread, and +// these bites pin the properties that makes it safe, none of which a functional test would catch. + +const { test, after } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); +const sharp = require('sharp'); // devDependency: fixture generator only, never shipped +const imageOps = require('../lib/image-ops'); + +const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'image-ops-')); +after(async () => { await imageOps.shutdown(); fs.rmSync(tmp, { recursive: true, force: true }); }); + +// 12MP — a phone photo, and the size the thresholds below are calibrated against. Smaller is +// tempting for test speed but defeats the point: at 4MP the inline path stalls only ~350ms, which +// slips under any threshold loose enough not to be flaky, so the guard stops detecting the very +// regression it exists for. Measured: inline ~1000ms stall / ~2 timers serviced, worker ~0ms / ~90. +async function bigPhoto(name = 'big.jpg') { + const p = path.join(tmp, name); + if (!fs.existsSync(p)) { + // Random pixels, not a flat fill: a solid colour compresses to almost nothing and decodes far + // faster than any real photo, which would quietly defeat the timing assertion below. + const px = Buffer.allocUnsafe(4000 * 3000 * 3); + for (let i = 0; i < px.length; i++) px[i] = (i * 2654435761) & 0xff; + fs.writeFileSync(p, await sharp(px, { raw: { width: 4000, height: 3000, channels: 3 } }).jpeg().toBuffer()); + } + return p; +} + +test('image work does not stall the event loop (#240)', async () => { + const src = await bigPhoto(); + + let ticks = 0, worstGap = 0, last = Date.now(); + const timer = setInterval(() => { ticks++; worstGap = Math.max(worstGap, Date.now() - last - 10); last = Date.now(); }, 10); + const started = Date.now(); + await imageOps.writeThumbnail(src, path.join(tmp, 'thumb.jpg'), 320, 70); + const elapsed = Date.now() - started; + clearInterval(timer); + + // The point is not that it was fast — it is that the loop kept running while it was slow. + // Thresholds sit in the gap between the two behaviours (worker ~90 ticks / ~0ms stall, inline + // ~2 ticks / ~1000ms stall), far enough from both to bite without being flaky. + assert.ok(ticks >= 20, `event loop serviced only ${ticks} timers in ${elapsed}ms — it is being blocked`); + assert.ok(worstGap < 200, `event loop stalled ${worstGap}ms in one go — image work is on the main thread`); + assert.ok(fs.existsSync(path.join(tmp, 'thumb.jpg')), 'thumbnail was still written'); +}); + +test('an undecodable image rejects without killing the worker', async () => { + const bad = path.join(tmp, 'corrupt.jpg'); + fs.writeFileSync(bad, Buffer.from('not an image')); + await assert.rejects(() => imageOps.metadata(bad), 'corrupt input must reject, so ingest records nulls'); + + // Crash isolation: one bad upload must not take out the queued work of unrelated callers. + const ok = path.join(tmp, 'fine.png'); + fs.writeFileSync(ok, await sharp({ create: { width: 40, height: 25, channels: 3, background: '#123456' } }).png().toBuffer()); + assert.deepEqual(await imageOps.metadata(ok), { width: 40, height: 25, orientation: 1 }); +}); + +test('concurrent callers are serialized, and each still gets its own answer', async () => { + // Serialization bounds peak memory to ONE decoded bitmap (a 12MP photo is ~48MB of RGBA); + // overlapping jobs would multiply that by the queue depth on exactly the small targets this + // change exists to reach. Correctness under concurrency is what is asserted here. + const sizes = [[30, 10], [60, 20], [90, 30], [120, 40]]; + const files = await Promise.all(sizes.map(async ([w, h], i) => { + const p = path.join(tmp, `c${i}.png`); + fs.writeFileSync(p, await sharp({ create: { width: w, height: h, channels: 3, background: '#0a0' } }).png().toBuffer()); + return p; + })); + const got = await Promise.all(files.map(f => imageOps.metadata(f))); + assert.deepEqual(got.map(m => [m.width, m.height]), sizes, 'replies must not be crossed between queued jobs'); +}); + +test('#170 EXIF orientation is applied by the decoder, so dimensions are as DISPLAYED', async () => { + // orientation 6 = "rotate 90° CW to display": a 30x100 stored buffer DISPLAYS as 100x30. + const p = path.join(tmp, 'rot6.jpg'); + fs.writeFileSync(p, await sharp({ create: { width: 30, height: 100, channels: 3, background: '#00ff00' } }) + .withMetadata({ orientation: 6 }).jpeg().toBuffer()); + + const meta = await imageOps.metadata(p); + assert.equal(meta.width, 100, 'EXIF-rotated image measures as displayed, not as stored'); + assert.equal(meta.height, 30); + // Reported as 1 because the rotation is already applied — imageDisplayDims() must NOT swap again. + assert.equal(meta.orientation, 1, 'a tag of 6 here would double-rotate downstream'); +});