From ab928ce9e3010d63ea8a62978be11b7d229117be Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Sat, 15 Aug 2026 18:59:20 +0000 Subject: [PATCH] fix: emit asset-arrived for a pulled video's regenerated thumbnail and name it from the history row (#4162) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit doPullOneAsset regenerated a pulled video's thumbnail but emitted `asset-arrived` only for the .mp4, so MediaImage's filename-keyed listener never fired for the poster and a federated Creative Director card sat on the "Syncing" placeholder until a remount. The name was also derived from the mp4 basename. That is only coincidentally right for a videoGen clip (`.mp4` beside `thumbnail: '.jpg'`) and flatly wrong for a stitched timeline final, which videoTimeline/local.js writes as `timeline--.mp4` beside an independent randomUUID history id — so the regenerated file landed under a name no poster URL ever requests. The name now comes from the synced history row's `thumbnail` field (sanitized, since it rode the wire from a peer), falling back to the mp4 stem for the window where the bytes beat the videoHistory metadata category across. --- .changelog/next/fixed-issue-4162.md | 1 + server/services/sharing/peerSyncAssets.js | 75 ++++++-- .../peerSyncAssets.videoThumbnail.test.js | 160 ++++++++++++++++++ 3 files changed, 220 insertions(+), 16 deletions(-) create mode 100644 .changelog/next/fixed-issue-4162.md create mode 100644 server/services/sharing/peerSyncAssets.videoThumbnail.test.js diff --git a/.changelog/next/fixed-issue-4162.md b/.changelog/next/fixed-issue-4162.md new file mode 100644 index 0000000000..82460f83c1 --- /dev/null +++ b/.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) diff --git a/server/services/sharing/peerSyncAssets.js b/server/services/sharing/peerSyncAssets.js index 1c6d05792f..39eb172c6e 100644 --- a/server/services/sharing/peerSyncAssets.js +++ b/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 +// `.mp4` beside `thumbnail: '.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--.mp4`. Every poster URL the UI builds is +// `/data/video-thumbnails/.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 - // (`.jpg`, where jobId === the video filename minus `.mp4`). The - // synced video-history row already carries `thumbnail: '.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 `` 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, + }); + } } } diff --git a/server/services/sharing/peerSyncAssets.videoThumbnail.test.js b/server/services/sharing/peerSyncAssets.videoThumbnail.test.js new file mode 100644 index 0000000000..f638c0639a --- /dev/null +++ b/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 (`.mp4`) and flatly wrong + * for a stitched timeline final (`timeline--.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 `.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']); + }); +});