Merge pull request #247 from screentinker/thumbnail-backfill

Heal missing thumbnails: boot-time backfill, ffmpeg diagnostics + packaging, phantom-path fix

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).

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

server/lib/content-ingest.js

@@ -48,18 +48,35 @@ 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');
-        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');
@@ -68,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);

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 };

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 };

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

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');
+});

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`);
+});