fix: un-stick federated video posters — emit asset-arrived for the regenerated thumbnail and name it from the history row

.changelog/next/fixed-issue-4162.md

@@ -0,0 +1 @@
+- Federated video posters now un-stick from the "Syncing" placeholder: a pulled video's regenerated thumbnail emits its own asset-arrived event and is named from the video-history row's thumbnail field instead of the mp4 basename (which was wrong for stitched timeline finals)

server/services/sharing/peerSyncAssets.js

@@ -407,20 +407,48 @@ export async function buildProjectAssetManifest(project) {
 }
 
 // `data/video-history.json` is a FLAT array of video-generation rows
-// (`{ id, filename, ... }`). The same store dataSync's `videoHistory` category
-// federates as metadata; here we read it only to resolve a scene's
-// `videoHistoryId` to its on-disk basename under PATHS.videos. Mirrors
-// dataSync's direct readJSONFile (no videoGen import — that drags in
-// ffmpeg/spawn machinery we don't need on the manifest path).
-async function videoHistoryFilenamesById() {
+// (`{ id, filename, thumbnail, ... }`). The same store dataSync's `videoHistory`
+// category federates as metadata; the two lookups below read it to map between a
+// row's id, its on-disk basename under PATHS.videos, and its poster basename
+// under PATHS.videoThumbnails. Mirrors dataSync's direct readJSONFile (no
+// videoGen import — that drags in ffmpeg/spawn machinery we don't need here).
+async function readVideoHistoryRows() {
   const raw = await readJSONFile(join(PATHS.data, 'video-history.json'), []);
+  return Array.isArray(raw) ? raw : [];
+}
+
+// Resolve a scene's `videoHistoryId` to its on-disk basename.
+async function videoHistoryFilenamesById() {
   const map = new Map();
-  for (const row of Array.isArray(raw) ? raw : []) {
+  for (const row of await readVideoHistoryRows()) {
     if (isStr(row?.id) && isStr(row?.filename)) map.set(row.id, row.filename);
   }
   return map;
 }
 
+// The reverse lookup (#4162): given an on-disk video basename, what THUMBNAIL
+// basename does its history row declare? Returns `null` when no row carries the
+// filename or its `thumbnail` isn't a safe basename.
+//
+// A history id is NOT the video filename stem. `videoGen/local.js` names a clip
+// `<jobId>.mp4` beside `thumbnail: '<jobId>.jpg'`, so stem-derivation happens to
+// land on the right name there — but `videoTimeline/local.js` mints an
+// independent `randomUUID()` id for a stitched final whose file is
+// `timeline-<projectId-slice>-<ts>.mp4`. Every poster URL the UI builds is
+// `/data/video-thumbnails/<row.id>.jpg`, so a stem-derived regeneration writes a
+// name nothing ever requests and the card stays on MediaImage's "Syncing"
+// placeholder forever.
+//
+// `row.thumbnail` rode the wire from a peer, so it goes through
+// `sanitizeAssetFilename` before it can become a path segment.
+async function videoThumbnailNameForVideo(filename) {
+  for (const row of await readVideoHistoryRows()) {
+    if (row?.filename !== filename) continue;
+    return isStr(row?.thumbnail) ? sanitizeAssetFilename(row.thumbnail) : null;
+  }
+  return null;
+}
+
 /**
  * Build a Music Video project's asset manifest (#1772). Unlike the Creative
  * Director store, a music video project has NO auto-linked media collection, so
@@ -919,16 +947,31 @@ async function doPullOneAsset(peer, base, entry, urlPrefix, localDir, safeName)
   }
   // After a video pull, regenerate the thumbnail LOCALLY rather than pulling it
   // as a sibling asset. Cheaper end-to-end: no new asset kind / URL-prefix /
-  // manifest-diff plumbing, and the thumbnail filename is deterministic
-  // (`<jobId>.jpg`, where jobId === the video filename minus `.mp4`). The
-  // synced video-history row already carries `thumbnail: '<jobId>.jpg'`, so
-  // once this file exists on disk `normalizeVideo` renders the collection
-  // tile. Best-effort: if ffmpeg is missing the row still syncs (the item
-  // stops being filtered as "missing"); the tile just falls back to no
-  // preview. Mirrors generateThumbnail's null-on-failure contract.
+  // manifest-diff plumbing. The NAME comes from the synced video-history row's
+  // `thumbnail` field (#4162) — NOT the mp4 stem, which is only coincidentally
+  // right for videoGen clips and flatly wrong for a stitched timeline final (see
+  // `videoThumbnailNameForVideo`). The stem stays the fallback for the window
+  // where the bytes beat the `videoHistory` metadata category across.
+  //
+  // The regenerated thumbnail then gets its OWN `asset-arrived` emit: the video
+  // emit above names the `.mp4`, and `MediaImage` matches on filename alone, so
+  // without this a poster `<img>` that already 404'd sits on the "Syncing"
+  // placeholder until a remount even though the bytes are on disk.
+  //
+  // Best-effort throughout: if ffmpeg is missing the row still syncs (the item
+  // stops being filtered as "missing"); the tile just falls back to no preview.
+  // Mirrors generateThumbnail's null-on-failure contract.
   if (entry.kind === 'video') {
-    const jobId = safeName.replace(/\.[a-z0-9]+$/i, '');
+    const rowThumbnail = await videoThumbnailNameForVideo(safeName).catch(() => null);
+    const jobId = (rowThumbnail || safeName).replace(/\.[a-z0-9]+$/i, '');
     const videoPath = join(localDir, safeName);
-    await generateThumbnail(videoPath, jobId).catch(() => null);
+    const thumbFilename = await generateThumbnail(videoPath, jobId).catch(() => null);
+    if (thumbFilename) {
+      peerSyncEvents.emit('asset-arrived', {
+        filename: thumbFilename,
+        kind: 'video-thumbnail',
+        peerId: peer.instanceId,
+      });
+    }
   }
 }

server/services/sharing/peerSyncAssets.videoThumbnail.test.js

@@ -0,0 +1,160 @@
+/**
+ * #4162 — a federated video poster must un-stick from MediaImage's "Syncing"
+ * placeholder on its own.
+ *
+ * Two halves to the bug, both covered here:
+ *   1. the regenerated thumbnail got NO `asset-arrived` emit (only the `.mp4`
+ *      did), and MediaImage matches on filename, so a poster that already 404'd
+ *      stayed on the placeholder until a remount;
+ *   2. the thumbnail's NAME was derived from the mp4 basename, which is only
+ *      coincidentally right for a videoGen clip (`<jobId>.mp4`) and flatly wrong
+ *      for a stitched timeline final (`timeline-<slice>-<ts>.mp4` beside an
+ *      independent `randomUUID()` history id).
+ */
+import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
+import { mkdtempSync, rmSync, mkdirSync, writeFileSync } from 'fs';
+import { tmpdir } from 'os';
+import { join } from 'path';
+import { createHash } from 'crypto';
+import { makePathsProxy } from '../../lib/mockPathsDataRoot.js';
+
+let tempRoot = mkdtempSync(join(tmpdir(), 'portos-vthumb-boot-'));
+
+vi.mock('../../lib/fileUtils.js', async () => {
+  const actual = await vi.importActual('../../lib/fileUtils.js');
+  return makePathsProxy(actual, { dataRoot: () => tempRoot });
+});
+
+const peers = [];
+vi.mock('../instances.js', () => ({
+  getPeers: vi.fn(async () => peers),
+}));
+
+vi.mock('../../lib/peerHttpClient.js', () => ({ peerFetch: vi.fn() }));
+
+// Stub only the thumbnail generator — the rest of ffmpeg.js stays real so any
+// other consumer in the import graph behaves normally. The real one shells out
+// to ffmpeg, which CI has no business requiring.
+vi.mock('../../lib/ffmpeg.js', async () => {
+  const actual = await vi.importActual('../../lib/ffmpeg.js');
+  return { ...actual, generateThumbnail: vi.fn(async (_videoPath, jobId) => `${jobId}.jpg`) };
+});
+
+const { peerFetch } = await import('../../lib/peerHttpClient.js');
+const { generateThumbnail } = await import('../../lib/ffmpeg.js');
+const { pullMissingAssetsFromPeer, assetWriteQueue } = await import('./peerSyncAssets.js');
+const { peerSyncEvents } = await import('./peerSyncShared.js');
+
+const sha = (buf) => createHash('sha256').update(buf).digest('hex');
+
+const mkRes = (buffer) => ({
+  ok: true,
+  headers: {
+    has: (h) => h === 'content-length',
+    get: (h) => (h === 'content-length' ? String(buffer.length) : null),
+  },
+  arrayBuffer: async () => buffer,
+});
+
+function writeVideoHistory(rows) {
+  writeFileSync(join(tempRoot, 'video-history.json'), JSON.stringify(rows));
+}
+
+/** Collect every `asset-arrived` payload emitted during `run()`. */
+async function captureArrivals(run) {
+  const seen = [];
+  const handler = (payload) => seen.push(payload);
+  peerSyncEvents.on('asset-arrived', handler);
+  try {
+    await run();
+  } finally {
+    peerSyncEvents.off('asset-arrived', handler);
+  }
+  return seen;
+}
+
+async function pullVideo(filename, bytes) {
+  vi.mocked(peerFetch).mockResolvedValue(mkRes(bytes));
+  return captureArrivals(() => pullMissingAssetsFromPeer('peer-a', [
+    { filename, kind: 'video', sha256: sha(bytes) },
+  ]));
+}
+
+describe('#4162 — pulled-video thumbnail naming + arrival event', () => {
+  beforeEach(() => {
+    tempRoot = mkdtempSync(join(tmpdir(), 'portos-vthumb-'));
+    mkdirSync(join(tempRoot, 'videos'), { recursive: true });
+    assetWriteQueue.clear();
+    vi.mocked(peerFetch).mockReset();
+    vi.mocked(generateThumbnail).mockClear();
+    peers.length = 0;
+    peers.push({ instanceId: 'peer-a', name: 'peer-a', address: '192.0.2.10', port: 5555, fullSync: true });
+  });
+  afterEach(() => {
+    if (tempRoot) rmSync(tempRoot, { recursive: true, force: true });
+  });
+
+  it('names a stitched timeline final\'s thumbnail from the history row, not the mp4 stem', async () => {
+    // The shape videoTimeline/local.js persists: an independent randomUUID id
+    // (and `<id>.jpg` poster) beside a `timeline-…` filename.
+    writeVideoHistory([{
+      id: '11111111-2222-3333-4444-555555555555',
+      filename: 'timeline-abcd1234-1700000000000.mp4',
+      thumbnail: '11111111-2222-3333-4444-555555555555.jpg',
+    }]);
+    const arrivals = await pullVideo('timeline-abcd1234-1700000000000.mp4', Buffer.from('mp4-bytes'));
+
+    expect(vi.mocked(generateThumbnail)).toHaveBeenCalledTimes(1);
+    const [videoPath, jobId] = vi.mocked(generateThumbnail).mock.calls[0];
+    expect(videoPath).toBe(join(tempRoot, 'videos', 'timeline-abcd1234-1700000000000.mp4'));
+    // The bug: this used to be 'timeline-abcd1234-1700000000000'.
+    expect(jobId).toBe('11111111-2222-3333-4444-555555555555');
+
+    expect(arrivals.map((a) => a.filename)).toEqual([
+      'timeline-abcd1234-1700000000000.mp4',
+      '11111111-2222-3333-4444-555555555555.jpg',
+    ]);
+    const thumbArrival = arrivals[1];
+    expect(thumbArrival.kind).toBe('video-thumbnail');
+    expect(thumbArrival.peerId).toBe('peer-a');
+  });
+
+  it('emits an arrival for a videoGen clip thumbnail too (name already matched the stem)', async () => {
+    writeVideoHistory([{
+      id: 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee',
+      filename: 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee.mp4',
+      thumbnail: 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee.jpg',
+    }]);
+    const arrivals = await pullVideo('aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee.mp4', Buffer.from('clip-bytes'));
+
+    expect(vi.mocked(generateThumbnail).mock.calls[0][1]).toBe('aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee');
+    expect(arrivals.map((a) => a.filename)).toContain('aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee.jpg');
+  });
+
+  it('falls back to the mp4 stem when the history row has not synced yet', async () => {
+    // No video-history.json at all — the bytes beat the `videoHistory` metadata
+    // category across. Pre-fix behavior is the fallback, not a regression.
+    const arrivals = await pullVideo('ffffffff-0000-1111-2222-333333333333.mp4', Buffer.from('orphan-bytes'));
+
+    expect(vi.mocked(generateThumbnail).mock.calls[0][1]).toBe('ffffffff-0000-1111-2222-333333333333');
+    expect(arrivals.map((a) => a.filename)).toContain('ffffffff-0000-1111-2222-333333333333.jpg');
+  });
+
+  it('ignores a traversal-shaped `thumbnail` from a peer row and falls back to the stem', async () => {
+    writeVideoHistory([{
+      id: 'evil',
+      filename: 'timeline-deadbeef-1700000000001.mp4',
+      thumbnail: '../../escape.jpg',
+    }]);
+    await pullVideo('timeline-deadbeef-1700000000001.mp4', Buffer.from('evil-bytes'));
+
+    expect(vi.mocked(generateThumbnail).mock.calls[0][1]).toBe('timeline-deadbeef-1700000000001');
+  });
+
+  it('emits no thumbnail arrival when regeneration fails (no ffmpeg)', async () => {
+    vi.mocked(generateThumbnail).mockResolvedValueOnce(null);
+    const arrivals = await pullVideo('timeline-cafebabe-1700000000002.mp4', Buffer.from('no-ffmpeg-bytes'));
+
+    expect(arrivals.map((a) => a.filename)).toEqual(['timeline-cafebabe-1700000000002.mp4']);
+  });
+});