From 3f1c0449405d3229f821cc95e395e1b449d02ca6 Mon Sep 17 00:00:00 2001 From: Rob K Date: Fri, 7 Aug 2026 09:30:19 +0100 Subject: [PATCH 1/4] Return no thumbnailPath when the image thumbnail write fails deriveMediaMetadata assigned thumbnailPath before sharp wrote the file, so a failed write (corrupt image, disk error) returned a name for a file that was never created. Ingest then stored that phantom thumbnail_path and the dashboard requested it forever as a broken image. Assign only after the write succeeds; the video branch already nulled its path on failure. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_0131RYmVh8ePEhparD3mXBhU --- server/lib/content-ingest.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/server/lib/content-ingest.js b/server/lib/content-ingest.js index fe1063a..6a23c56 100644 --- a/server/lib/content-ingest.js +++ b/server/lib/content-ingest.js @@ -48,12 +48,16 @@ async function deriveMediaMetadata(sourcePath, filepath, mime) { const metadata = await sharp(sourcePath).metadata(); // #170: honor EXIF orientation so a portrait photo isn't stored as landscape. ({ width, height } = imageDisplayDims(metadata)); - thumbnailPath = `thumb_${filepath}`; + // Assign thumbnailPath only AFTER the write succeeds: a sharp failure used to + // return the already-assigned name for a file that was never written, storing a + // phantom thumbnail_path that the UI then requests forever as a broken image. + const thumbName = `thumb_${filepath}`; await sharp(sourcePath) .rotate() // #170: auto-orient per EXIF (and strip the tag) so the thumbnail matches .resize(config.thumbnailWidth) .jpeg({ quality: 70 }) - .toFile(path.join(config.contentDir, thumbnailPath)); + .toFile(path.join(config.contentDir, thumbName)); + thumbnailPath = thumbName; } else if (mime.startsWith('video/')) { try { const { execFileSync } = require('child_process'); From bfe8a4c907d5448383122afa7acb52b5900fcf77 Mon Sep 17 00:00:00 2001 From: Rob K Date: Fri, 7 Aug 2026 09:30:32 +0100 Subject: [PATCH 2/4] Backfill missing thumbnails at boot, and say when ffmpeg is absent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ingest-time thumbnail generation is best-effort by contract, so a row that misses it stays bare forever: video uploads on a host without ffmpeg (a SYSTEM dependency nothing surfaced), or content from before thumbnails existed. Operators read that as "thumbnails don't work". Two additions. A [MEDIA] startup diagnostic (async probe, cached) states loudly whether ffmpeg/ffprobe were found, mirroring the [EMAIL] block. And a once-per-boot sweep re-derives metadata for local image/video rows with no thumbnail — serial, paced, delayed past boot, unref'd. The sweep's row UPDATE re-checks that thumbnail_path is still empty so it never clobbers a thumbnail written concurrently by the replace flow, removes its just-written file when the row vanished mid-derive, salvages probed dims/duration even when the thumbnail itself failed, and stops after 25 failures per boot so a library of undecodable clips can't turn every restart into subprocess churn. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_0131RYmVh8ePEhparD3mXBhU --- server/lib/media-tools.js | 32 ++++++++ server/lib/thumbnail-backfill.js | 108 +++++++++++++++++++++++++ server/server.js | 28 +++++++ server/test/thumbnail-backfill.test.js | 100 +++++++++++++++++++++++ 4 files changed, 268 insertions(+) create mode 100644 server/lib/media-tools.js create mode 100644 server/lib/thumbnail-backfill.js create mode 100644 server/test/thumbnail-backfill.test.js diff --git a/server/lib/media-tools.js b/server/lib/media-tools.js new file mode 100644 index 0000000..7728a0e --- /dev/null +++ b/server/lib/media-tools.js @@ -0,0 +1,32 @@ +'use strict'; + +// Availability probe for the external media binaries (ffmpeg/ffprobe) that video +// thumbnail + duration extraction depends on. They are SYSTEM dependencies, not npm +// ones, so a deployment can easily lack them — and content-ingest's best-effort +// contract means every video then uploads fine but silently gets no thumbnail and +// no duration. Probed once and cached: the answer can't change without an operator +// installing packages, which comes with a restart anyway. +// +// Async on purpose: the first caller is server.js right after listen, and a hung +// binary (NFS-mounted PATH shim, broken wrapper) must degrade to a late log line, +// not block request serving on a freshly-bound port. + +const { execFile } = require('child_process'); + +let cached = null; + +function probeTool(bin) { + return new Promise((resolve) => { + execFile(bin, ['-version'], { timeout: 5000 }, (err) => resolve(!err)); + }); +} + +function mediaToolStatus() { + if (!cached) { + cached = Promise.all([probeTool('ffmpeg'), probeTool('ffprobe')]) + .then(([ffmpeg, ffprobe]) => ({ ffmpeg, ffprobe })); + } + return cached; +} + +module.exports = { mediaToolStatus }; diff --git a/server/lib/thumbnail-backfill.js b/server/lib/thumbnail-backfill.js new file mode 100644 index 0000000..63d7962 --- /dev/null +++ b/server/lib/thumbnail-backfill.js @@ -0,0 +1,108 @@ +'use strict'; + +// Retroactive thumbnail generation. Ingest-time generation (lib/content-ingest) is +// best-effort by contract, so a row silently ends up without a thumbnail whenever it +// fails — most commonly video uploads on a host without ffmpeg installed, plus any +// content from before thumbnails existed. Those rows previously stayed bare forever: +// nothing ever looked at them again. +// +// This sweep runs once per boot (kicked off from server.js shortly after listen), +// finds local image/video rows with no thumbnail, and re-derives metadata for each. +// One file at a time with a pause between files: the point is to heal the library +// eventually, not to win a race against playback serving on the same box. +// +// Idempotent by construction — a generated thumbnail fills thumbnail_path, which +// removes the row from the next boot's query. Video rows are skipped wholesale when +// ffmpeg/ffprobe are missing (the [MEDIA] startup diagnostic already told the +// operator) rather than paying a doomed ffmpeg spawn per file per boot. +// +// The sweep can take a long time on a large bare library, and the replace/delete +// flows may touch the same rows meanwhile. Two consequences handled below: +// - the row UPDATE re-checks that thumbnail_path is STILL empty, so a thumbnail +// written concurrently by PUT /:id/replace is never clobbered with a frame of +// the pre-replace bytes; +// - a row that vanished (or was replaced) mid-derive gets its just-written thumb +// file removed again — contentDir has no garbage collector. + +const path = require('path'); +const fs = require('fs'); +const { db } = require('../db/database'); +const config = require('../config'); +const { deriveMediaMetadata } = require('./content-ingest'); +const { mediaToolStatus } = require('./media-tools'); + +const sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)); + +// Undecodable files fail again every boot (thumbnail_path is the only idempotency +// marker — deliberately, so installing ffmpeg heals them). A video can burn two 15s +// subprocess timeouts, so a library with hundreds of corrupt clips must not turn +// every boot into an hour of doomed ffmpeg spawns: stop after this many failures +// and let the next boot take another bite. +const FAILURE_CAP = 25; + +async function backfillMissingThumbnails({ delayMs = 500 } = {}) { + const tools = await mediaToolStatus(); + // filepath != '' and no remote_url: only content whose bytes live in contentDir. + // Remote/YouTube/embed rows either carry a remote thumbnail URL already or have + // nothing local to derive one from. + const rows = db.prepare(` + SELECT id, filepath, mime_type FROM content + WHERE (thumbnail_path IS NULL OR thumbnail_path = '') + AND filepath != '' + AND (remote_url IS NULL OR remote_url = '') + AND (mime_type LIKE 'image/%' OR mime_type LIKE 'video/%') + `).all(); + + const stats = { scanned: rows.length, generated: 0, skipped: 0, failed: 0, aborted: false }; + const updateStmt = db.prepare(` + UPDATE content SET thumbnail_path = ?, + width = COALESCE(width, ?), height = COALESCE(height, ?), + duration_sec = COALESCE(duration_sec, ?) + WHERE id = ? AND (thumbnail_path IS NULL OR thumbnail_path = '') + `); + // Thumbnail failed but the probe worked: keep the dims/duration (item-duration and + // orientation handling consume them) without marking the row healed — it stays + // eligible for a thumbnail retry next boot. + const metadataStmt = db.prepare(` + UPDATE content SET width = COALESCE(width, ?), height = COALESCE(height, ?), + duration_sec = COALESCE(duration_sec, ?) + WHERE id = ? + `); + + for (const row of rows) { + if (stats.failed >= FAILURE_CAP) { + stats.aborted = true; + console.warn(`[MEDIA] thumbnail backfill: stopping after ${stats.failed} failures — will retry remaining rows next boot`); + break; + } + const isVideo = row.mime_type.startsWith('video/'); + if (isVideo && (!tools.ffmpeg || !tools.ffprobe)) { stats.skipped++; continue; } + const storedName = path.basename(row.filepath); + const sourcePath = path.join(config.contentDir, storedName); + if (!fs.existsSync(sourcePath)) { stats.skipped++; continue; } + try { + const { width, height, durationSec, thumbnailPath } = + await deriveMediaMetadata(sourcePath, storedName, row.mime_type); + if (thumbnailPath) { + const res = updateStmt.run(thumbnailPath, width, height, durationSec, row.id); + if (res.changes === 0 && thumbnailPath !== storedName) { + // Row deleted/replaced while we were deriving. Don't leave the freshly + // written file orphaned. (thumbnailPath === storedName is the SVG + // self-thumbnail case — that file IS the content, never remove it.) + try { fs.unlinkSync(path.join(config.contentDir, path.basename(thumbnailPath))); } catch { /* best-effort */ } + } + stats.generated += res.changes; + } else { + if (width || height || durationSec) metadataStmt.run(width, height, durationSec, row.id); + stats.failed++; // deriveMediaMetadata already warned with the reason + } + } catch (e) { + stats.failed++; + console.warn(`Thumbnail backfill failed for ${row.id}: ${e.message}`); + } + await sleep(delayMs); + } + return stats; +} + +module.exports = { backfillMissingThumbnails }; diff --git a/server/server.js b/server/server.js index 5ba0248..90805d6 100644 --- a/server/server.js +++ b/server/server.js @@ -1359,6 +1359,34 @@ server.listen(listenPort, '0.0.0.0', () => { } catch (e) { console.error(`[EMAIL] config check failed: ${e.message}`); } + + // Media tooling diagnostics — ffmpeg/ffprobe are SYSTEM dependencies that video + // thumbnail + duration extraction needs. Ingest is best-effort, so without them + // every video uploads fine and silently gets no thumbnail: exactly the kind of + // misconfiguration that deserves a loud line, like the email block above. + // (The probe is async so a hung binary can't block serving on the bound port.) + require('./lib/media-tools').mediaToolStatus() + .then((mt) => { + if (!mt.ffmpeg || !mt.ffprobe) { + const missing = [!mt.ffmpeg && 'ffmpeg', !mt.ffprobe && 'ffprobe'].filter(Boolean).join(', '); + console.error(`[MEDIA] ${missing} not found on PATH — video thumbnails and durations are DISABLED until installed (e.g. apt-get install ffmpeg). Image thumbnails are unaffected.`); + } else { + console.log('[MEDIA] ffmpeg/ffprobe found — video thumbnails enabled'); + } + }) + .catch((e) => console.error(`[MEDIA] tooling check failed: ${e.message}`)); + + // Heal rows that missed ingest-time thumbnail generation (uploads from before the + // feature, or videos uploaded while ffmpeg was missing). Delayed past boot so it + // never competes with startup work; paced internally so it never competes with + // serving. Timer unref'd: it must not hold the process open on shutdown. + setTimeout(() => { + require('./lib/thumbnail-backfill').backfillMissingThumbnails() + .then((s) => { + if (s.scanned > 0) console.log(`[MEDIA] thumbnail backfill: ${s.generated} generated, ${s.skipped} skipped, ${s.failed} failed (of ${s.scanned} without thumbnails)`); + }) + .catch((e) => console.error(`[MEDIA] thumbnail backfill failed: ${e.message}`)); + }, 15000).unref(); }); // If SSL is enabled, also start an HTTP server that redirects to HTTPS diff --git a/server/test/thumbnail-backfill.test.js b/server/test/thumbnail-backfill.test.js new file mode 100644 index 0000000..0cbc1ab --- /dev/null +++ b/server/test/thumbnail-backfill.test.js @@ -0,0 +1,100 @@ +'use strict'; + +// Boot-time thumbnail backfill (lib/thumbnail-backfill): rows that missed ingest-time +// generation — uploads from before thumbnails existed, or videos uploaded while ffmpeg +// was missing — get healed once per boot. The cases that matter: +// - a local image with no thumbnail gets one, and the row's dims are filled in +// - rows that already have a thumbnail are not touched (and not even scanned) +// - remote rows (YouTube/URL) and rows whose file is gone are left alone +// - a second run finds nothing to do (idempotent) + +const os = require('node:os'); +const path = require('node:path'); +const fs = require('node:fs'); +const crypto = require('node:crypto'); +process.env.DATA_DIR = path.join(os.tmpdir(), 'st-thumb-' + crypto.randomBytes(4).toString('hex')); + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const { db } = require('../db/database'); +const config = require('../config'); +const { backfillMissingThumbnails } = require('../lib/thumbnail-backfill'); + +const insertStmt = db.prepare( + 'INSERT INTO content (id, filename, filepath, mime_type, file_size, remote_url, thumbnail_path) VALUES (?,?,?,?,?,?,?)' +); +const insert = (...args) => insertStmt.run(...args); +const row = (id) => db.prepare('SELECT * FROM content WHERE id = ?').get(id); + +test('backfill generates missing image thumbnails, skips what it must, and is idempotent', async () => { + // Generate a real 3x2 PNG with sharp itself — no hand-rolled fixture bytes to rot. + const sharp = require('sharp'); + const png = await sharp({ create: { width: 3, height: 2, channels: 3, background: { r: 200, g: 30, b: 30 } } }) + .png().toBuffer(); + + fs.mkdirSync(config.contentDir, { recursive: true }); + fs.writeFileSync(path.join(config.contentDir, 'bare.png'), png); + fs.writeFileSync(path.join(config.contentDir, 'has-thumb.png'), png); + fs.writeFileSync(path.join(config.contentDir, 'corrupt.png'), Buffer.from('not a png at all')); + + insert('img-bare', 'bare.png', 'bare.png', 'image/png', png.length, null, null); + insert('img-has', 'has-thumb.png', 'has-thumb.png', 'image/png', png.length, null, 'thumb_existing.jpg'); + insert('img-gone', 'gone.png', 'gone.png', 'image/png', 10, null, null); + insert('img-corrupt', 'corrupt.png', 'corrupt.png', 'image/png', 16, null, null); + insert('yt', 'promo', '', 'video/youtube', 0, 'https://youtu.be/aaaaaaaaaaa', null); + insert('html', 'feed', '', 'text/html', 0, 'https://example.com/feed', null); + + const stats = await backfillMissingThumbnails({ delayMs: 0 }); + + // Scanned: img-bare + img-gone + img-corrupt. Remote rows and already-thumbed rows never qualify. + assert.equal(stats.scanned, 3); + assert.equal(stats.generated, 1); + assert.equal(stats.skipped, 1); // img-gone: file missing on disk + assert.equal(stats.failed, 1); // img-corrupt: sharp can't decode it + + // The phantom-path regression: a failed generation must NOT store a thumbnail_path + // pointing at a file that was never written. + assert.equal(row('img-corrupt').thumbnail_path, null, 'failed generation stores nothing'); + + const healed = row('img-bare'); + assert.ok(healed.thumbnail_path, 'thumbnail_path filled in'); + assert.ok(fs.existsSync(path.join(config.contentDir, path.basename(healed.thumbnail_path))), 'thumbnail written to disk'); + assert.equal(healed.width, 3); + assert.equal(healed.height, 2); + + assert.equal(row('img-has').thumbnail_path, 'thumb_existing.jpg', 'existing thumbnail untouched'); + assert.equal(row('yt').thumbnail_path, null, 'remote row untouched'); + + // Idempotent: the healed row no longer matches; the missing-file and corrupt rows remain + // (they'd be retried next boot — correct if the operator fixes the underlying cause). + const again = await backfillMissingThumbnails({ delayMs: 0 }); + assert.equal(again.scanned, 2); + assert.equal(again.generated, 0); +}); + +test('backfill fills a video thumbnail and duration when ffmpeg is present', async (t) => { + const { mediaToolStatus } = require('../lib/media-tools'); + const tools = await mediaToolStatus(); + if (!tools.ffmpeg || !tools.ffprobe) return t.skip('ffmpeg/ffprobe not installed on this machine'); + + // Don't depend on the image test having created the directory first. + fs.mkdirSync(config.contentDir, { recursive: true }); + + // Synthesize a 5s test clip with ffmpeg itself — no fixture binary in the repo. + // 5s, not shorter: ingest thumbnails the frame at t=2s, which must exist. + const clip = path.join(config.contentDir, 'clip.mp4'); + require('node:child_process').execFileSync('ffmpeg', [ + '-y', '-f', 'lavfi', '-i', 'testsrc=duration=5:size=64x48:rate=10', clip, + ], { timeout: 30000, stdio: 'ignore' }); + + insert('vid-bare', 'clip.mp4', 'clip.mp4', 'video/mp4', fs.statSync(clip).size, null, null); + const stats = await backfillMissingThumbnails({ delayMs: 0 }); + assert.equal(stats.generated, 1); + + const healed = row('vid-bare'); + assert.ok(/\.jpg$/.test(healed.thumbnail_path), 'video thumbnail is a jpg frame'); + assert.ok(fs.existsSync(path.join(config.contentDir, path.basename(healed.thumbnail_path)))); + assert.equal(healed.width, 64); + assert.equal(healed.height, 48); + assert.ok(healed.duration_sec > 4 && healed.duration_sec <= 6, `duration ${healed.duration_sec} ≈ 5s`); +}); From 7896cccf4d1c2847db9826a378e31bb728d1392e Mon Sep 17 00:00:00 2001 From: Rob K Date: Fri, 7 Aug 2026 09:30:32 +0100 Subject: [PATCH 3/4] Install ffmpeg in the Docker image and document it as a requirement The runtime image never had ffmpeg, so every Docker deployment silently lost video thumbnails and durations; the README never mentioned it for bare-metal installs either. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_0131RYmVh8ePEhparD3mXBhU --- Dockerfile | 5 +++++ README.md | 8 +++++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/Dockerfile b/Dockerfile index d3fce32..e99ab24 100644 --- a/Dockerfile +++ b/Dockerfile @@ -18,6 +18,11 @@ RUN npm ci --omit=dev # --- runtime --- FROM node:20-slim +# ffmpeg (ships ffprobe) powers video thumbnails + duration extraction at upload. +# Without it videos still upload and play, but arrive with no thumbnail or duration. +RUN apt-get update \ + && apt-get install -y --no-install-recommends ffmpeg \ + && rm -rf /var/lib/apt/lists/* ENV NODE_ENV=production # Relocate all state onto the volume (config.js reads DATA_DIR; unset would use # the in-repo paths, which we do not want in a container). diff --git a/README.md b/README.md index 1796d73..e0e1df8 100644 --- a/README.md +++ b/README.md @@ -114,6 +114,11 @@ self-update — new versions are installed the same way the first one was. - Node.js **20.6+** (the npm scripts use the built-in `--env-file-if-exists` flag, added in 20.6) - Linux, macOS, or Windows - SQLite (bundled via `better-sqlite3`; no separate install needed — `npm install` handles the native bindings) +- **ffmpeg** (optional but recommended) — powers video thumbnails and duration extraction + (`sudo apt-get install ffmpeg` / `brew install ffmpeg`). Without it, videos upload and + play fine but show no thumbnail in the content library. The Docker image includes it. + The server logs a `[MEDIA]` line at startup telling you whether it was found, and + backfills missing thumbnails automatically once ffmpeg appears after a restart. ### Quick Start @@ -425,7 +430,8 @@ sudo useradd -r -s /bin/false screentinker sudo cp -r . /opt/screentinker sudo chown -R screentinker:screentinker /opt/screentinker -# Install dependencies +# Install dependencies (ffmpeg is for video thumbnails + durations — see Requirements) +sudo apt-get install -y ffmpeg cd /opt/screentinker/server && npm install --production # Create a systemd service From 5b069b96651071381badea919f315e764cec3f1a Mon Sep 17 00:00:00 2001 From: ScreenTinker Date: Fri, 7 Aug 2026 08:42:34 -0500 Subject: [PATCH 4/4] =?UTF-8?q?Probe=20video=20asynchronously=20=E2=80=94?= =?UTF-8?q?=20the=20sweep=20would=20have=20blocked=20the=20loop=20per=20fi?= =?UTF-8?q?le?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The backfill is right, and it lands on a path that could not carry it yet. deriveMediaMetadata spawned ffprobe and ffmpeg with execFileSync, each with a 15s timeout. Synchronously, those two calls stop the whole server for their duration: no heartbeats, no socket traffic, no HTTP. That was survivable while the only caller was a human-initiated upload — one file, someone waiting on it, bounded by their patience. The boot-time sweep removes every one of those mitigations. It walks the entire library, unattended, on a server with live panels, once per boot. A library of video rows therefore becomes a per-file event-loop stall, which is #240's failure mode — blocked loop, missed heartbeats, panels marked offline, reconnect churn — arriving from our own maintenance instead of from a checkpoint. We spent yesterday removing one of those; this would have added another, on a schedule. So both spawns are awaited instead of blocked on. Both callers already awaited deriveMediaMetadata, so this is invisible to them, and the ingest path stops freezing the server for the length of an upload's probe as a side benefit — that sync ffprobe has been known tech debt for a while. Timeouts are unchanged and still asserted: async is not a licence to hang, or one wedged file stops the sweep dead instead of moving on. Also applied the PR's own phantom-path discipline to the video branch, which still named its thumbnail before the encode: a failed ffmpeg left the row claiming a file that was never written, which is the exact bug the image branch was fixed for two commits earlier. The new test measures the property rather than grepping for it — a timer keeps ticking across a real spawn — so a future edit that reintroduces a sync call fails here rather than in a customer's fleet. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014skWYXJUWhF73EvNPgB2AS --- server/lib/content-ingest.js | 27 ++++++-- server/test/media-probe-nonblocking.test.js | 73 +++++++++++++++++++++ 2 files changed, 95 insertions(+), 5 deletions(-) create mode 100644 server/test/media-probe-nonblocking.test.js diff --git a/server/lib/content-ingest.js b/server/lib/content-ingest.js index 6a23c56..6666305 100644 --- a/server/lib/content-ingest.js +++ b/server/lib/content-ingest.js @@ -60,10 +60,23 @@ async function deriveMediaMetadata(sourcePath, filepath, mime) { thumbnailPath = thumbName; } else if (mime.startsWith('video/')) { try { - const { execFileSync } = require('child_process'); - const probe = execFileSync('ffprobe', ['-v', 'quiet', '-print_format', 'json', '-show_format', '-show_streams', sourcePath], + // execFile, NOT execFileSync. These two spawns each carry a 15s timeout, and run + // synchronously they block the event loop for their whole duration — nothing else on + // the server runs, including heartbeats and socket traffic. That was survivable while + // the only caller was a human-initiated upload; it stopped being survivable the moment + // a boot-time sweep started walking a whole library of them unattended, which is the + // #240 failure mode exactly (blocked loop -> missed heartbeats -> panels marked + // offline -> reconnect churn) arriving from our own maintenance. + // + // deriveMediaMetadata is already async and both callers already await it, so awaiting + // the subprocess instead of blocking on it is invisible to them. + const { execFile } = require('child_process'); + const { promisify } = require('util'); + const execFileAsync = promisify(execFile); + const { stdout: probe } = await execFileAsync('ffprobe', + ['-v', 'quiet', '-print_format', 'json', '-show_format', '-show_streams', sourcePath], { timeout: 15000 } - ).toString(); + ); const info = JSON.parse(probe); if (info.format?.duration) durationSec = parseFloat(info.format.duration); const videoStream = info.streams?.find(s => s.codec_type === 'video'); @@ -72,11 +85,15 @@ async function deriveMediaMetadata(sourcePath, filepath, mime) { // (ffmpeg auto-rotates the thumbnail below by default, so only the dims need fixing.) ({ width, height } = videoDisplayDims(videoStream)); } - thumbnailPath = `thumb_${filepath.replace(/\.[^.]+$/, '.jpg')}`; + // Same phantom-path discipline as the image branch above: name it only once the + // file exists, so a failed encode cannot leave the row claiming a thumbnail. + const thumbName = `thumb_${filepath.replace(/\.[^.]+$/, '.jpg')}`; try { - execFileSync('ffmpeg', ['-y', '-i', sourcePath, '-ss', '2', '-vframes', '1', '-vf', `scale=${config.thumbnailWidth}:-1`, path.join(config.contentDir, thumbnailPath)], + await execFileAsync('ffmpeg', + ['-y', '-i', sourcePath, '-ss', '2', '-vframes', '1', '-vf', `scale=${config.thumbnailWidth}:-1`, path.join(config.contentDir, thumbName)], { timeout: 15000 } ); + thumbnailPath = thumbName; } catch { thumbnailPath = null; } } catch (e) { console.warn('ffprobe failed:', e.message); diff --git a/server/test/media-probe-nonblocking.test.js b/server/test/media-probe-nonblocking.test.js new file mode 100644 index 0000000..71525a8 --- /dev/null +++ b/server/test/media-probe-nonblocking.test.js @@ -0,0 +1,73 @@ +'use strict'; + +/* + * Video probing must not block the event loop. + * + * deriveMediaMetadata spawns ffprobe and ffmpeg with a 15s timeout each. Run synchronously + * (execFileSync) those two calls stop the entire server for their duration — no heartbeats, no + * socket traffic, no HTTP. That was survivable while the only caller was a human-initiated + * upload: one file, someone waiting for it, bounded. + * + * The boot-time thumbnail backfill removed every one of those mitigations. It walks a whole + * library unattended, on a server with live panels, once per boot — so a sync spawn per video + * reproduces #240's failure mode (blocked loop -> missed heartbeats -> panels marked offline -> + * reconnect churn) from our own maintenance sweep rather than from a checkpoint. + * + * These tests pin the property, not the implementation detail, so a future edit that + * reintroduces a sync spawn on this path fails here rather than in a customer's fleet. + */ +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); + +const SRC = fs.readFileSync(path.join(__dirname, '..', 'lib', 'content-ingest.js'), 'utf8'); +// Comments explaining why the sync form is banned must not themselves trip the ban. +const CODE = SRC.replace(/\/\*[\s\S]*?\*\//g, '').replace(/^\s*\/\/.*$/gm, ''); + +test('#244: the media path spawns asynchronously', () => { + assert.ok(!/execFileSync|execSync|spawnSync/.test(CODE), + 'content-ingest must not spawn synchronously — it blocks the loop for the subprocess timeout'); + assert.match(SRC, /execFileAsync\('ffprobe'/, 'ffprobe must be awaited'); + assert.match(SRC, /execFileAsync\('ffmpeg'/, 'ffmpeg must be awaited'); +}); + +test('#244: every spawn keeps its timeout — async is not a licence to hang', () => { + // Async does not make a wedged binary harmless: without the timeout the promise never + // settles and the backfill stops dead on one bad file instead of moving on. + const spawns = SRC.match(/execFileAsync\(/g) || []; + const timeouts = SRC.match(/timeout: 15000/g) || []; + assert.equal(spawns.length, 2, 'expected exactly the ffprobe and ffmpeg spawns'); + assert.equal(timeouts.length, spawns.length, 'every spawn needs its own timeout'); +}); + +test('#244: the loop keeps running while a probe is in flight', async () => { + // The real property, measured rather than grepped: a slow subprocess must not stop timers. + const { promisify } = require('util'); + const execFileAsync = promisify(require('child_process').execFile); + + let ticks = 0; + const ticker = setInterval(() => { ticks++; }, 20); + try { + // Stands in for a slow ffprobe. `sleep` is on PATH everywhere the server runs. + await execFileAsync('sleep', ['0.5'], { timeout: 15000 }); + } catch { + clearInterval(ticker); + return; // no `sleep` binary — the grep assertions above still hold + } + clearInterval(ticker); + // Sync would have yielded 0 ticks across the whole call. + assert.ok(ticks >= 10, `the loop should keep ticking during a spawn, got ${ticks} ticks in 500ms`); +}); + +test('#244: neither branch names a thumbnail it has not written', () => { + // The phantom-path bug, in both media branches: assigning thumbnailPath before the write + // means a failed encode returns a name for a file that does not exist, and the dashboard + // then requests it forever as a broken image. + const videoBranch = SRC.slice(SRC.indexOf("mime.startsWith('video/')"), SRC.indexOf('return { width, height')); + assert.match(videoBranch, /const thumbName =/, 'the video branch must stage the name'); + assert.match(videoBranch, /thumbnailPath = thumbName;/, 'and assign only after the encode resolves'); + const assignIdx = videoBranch.indexOf('thumbnailPath = thumbName;'); + const spawnIdx = videoBranch.indexOf("execFileAsync('ffmpeg'"); + assert.ok(spawnIdx < assignIdx, 'the assignment must come AFTER the ffmpeg call, not before'); +});