fix(video): throw when neither a real thumbnail nor the placeholder can be produced
processUploadedVideo returned success with thumbnailKey: null when both the real thumbnail AND the SVG placeholder failed -- a total, systemic failure (storage backend down, disk full), not a quirk of one file. On stable, which doesn't have the #845 call-site fallback, this silently completed the video with no thumbnail at all instead of the retryable 'failed' status a throw here produces. On main, the pre-existing #845 fallback already absorbed this exact case (no behavior change there) -- verified against codex's own git-blame check of the pre-PR stable code before applying this. Now throws in that case, restoring the pre-existing "let the caller mark it failed and retryable" behavior for a genuinely unrecoverable video, while keeping every partial-failure case (the vast majority) resolving with whatever succeeded. Found by codex review.
This commit is contained in:
@@ -108,7 +108,7 @@ describe('processUploadedVideo degrades gracefully instead of rejecting the whol
|
|||||||
expect(storage.putFromFile).not.toHaveBeenCalled();
|
expect(storage.putFromFile).not.toHaveBeenCalled();
|
||||||
});
|
});
|
||||||
|
|
||||||
it('still resolves with a null thumbnail when metadata, thumbnail generation, AND the placeholder all fail — never throws, never blocks the upload', async () => {
|
it('throws when metadata, thumbnail generation, AND the placeholder all fail, so the caller surfaces a retryable failure instead of completing with nothing to show (codex review)', async () => {
|
||||||
ffmpeg.ffprobe = jest.fn((videoPath, cb) => cb(new Error('Invalid data found when processing input')));
|
ffmpeg.ffprobe = jest.fn((videoPath, cb) => cb(new Error('Invalid data found when processing input')));
|
||||||
ffmpeg.mockImplementation(() => ({
|
ffmpeg.mockImplementation(() => ({
|
||||||
screenshots() { return this; },
|
screenshots() { return this; },
|
||||||
@@ -119,8 +119,7 @@ describe('processUploadedVideo degrades gracefully instead of rejecting the whol
|
|||||||
}));
|
}));
|
||||||
generateVideoPlaceholder.mockRejectedValue(new Error('sharp render failed'));
|
generateVideoPlaceholder.mockRejectedValue(new Error('sharp render failed'));
|
||||||
|
|
||||||
const result = await processUploadedVideo('/tmp/corrupt.mp4', 'thumbnails/thumb_corrupt.jpg');
|
await expect(processUploadedVideo('/tmp/corrupt.mp4', 'thumbnails/thumb_corrupt.jpg'))
|
||||||
|
.rejects.toThrow('Unable to generate any thumbnail');
|
||||||
expect(result).toEqual({ success: true, metadata: null, thumbnailKey: null });
|
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -145,12 +145,18 @@ async function getVideoDuration(videoPath) {
|
|||||||
* generateVideoThumbnail() would have succeeded on its own — thumbnailing
|
* generateVideoThumbnail() would have succeeded on its own — thumbnailing
|
||||||
* doesn't need valid duration/width/height, it just seeks and grabs a frame.
|
* doesn't need valid duration/width/height, it just seeks and grabs a frame.
|
||||||
* Trying both steps independently means a real thumbnail (and whatever
|
* Trying both steps independently means a real thumbnail (and whatever
|
||||||
* metadata ffprobe *can* read) survives far more often; the callers' throw
|
* metadata ffprobe *can* read) survives far more often. metadata is still
|
||||||
* handling stays as a backstop for anything still unexpected.
|
* allowed to come back null (ffprobe failed) — a video with no thumbnail
|
||||||
|
* would fall back to rendering the raw video as an <img> in the gallery
|
||||||
|
* grid (`photo.thumbnail_url || photo.url`), so this only resolves when a
|
||||||
|
* real thumbnail or the SVG placeholder produced *something*; if both fail
|
||||||
|
* (storage backend down, disk full — not a quirk of one file) it throws
|
||||||
|
* instead, so the caller surfaces a retryable failure rather than silently
|
||||||
|
* completing with nothing to show.
|
||||||
*
|
*
|
||||||
* @param {string} videoPath - Local path to the source video (ffmpeg requires fs).
|
* @param {string} videoPath - Local path to the source video (ffmpeg requires fs).
|
||||||
* @param {string} thumbnailKey - Relative storage key for the thumbnail.
|
* @param {string} thumbnailKey - Relative storage key for the thumbnail.
|
||||||
* @returns {Promise<{success: boolean, metadata: Object|null, thumbnailKey: string|null}>}
|
* @returns {Promise<{success: boolean, metadata: Object|null, thumbnailKey: string}>}
|
||||||
*/
|
*/
|
||||||
async function processUploadedVideo(videoPath, thumbnailKey, options = {}) {
|
async function processUploadedVideo(videoPath, thumbnailKey, options = {}) {
|
||||||
let metadata = null;
|
let metadata = null;
|
||||||
@@ -212,6 +218,18 @@ async function processUploadedVideo(videoPath, thumbnailKey, options = {}) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// A real thumbnail AND the ffmpeg-free SVG placeholder both failing points
|
||||||
|
// at something systemic (storage backend down, disk full) rather than a
|
||||||
|
// quirk of this one file — that's worth surfacing as a retryable failure
|
||||||
|
// rather than silently completing with no thumbnail at all, which would
|
||||||
|
// make the gallery fall back to rendering the raw video as an <img>
|
||||||
|
// (codex review, #1371/#1372). Metadata (if any was extracted) is lost
|
||||||
|
// here, same trade-off the callers' own pre-existing total-failure
|
||||||
|
// handling already makes.
|
||||||
|
if (!generatedThumbnailKey) {
|
||||||
|
throw new Error('Unable to generate any thumbnail (real or placeholder) for this video');
|
||||||
|
}
|
||||||
|
|
||||||
return {
|
return {
|
||||||
success: true,
|
success: true,
|
||||||
metadata,
|
metadata,
|
||||||
|
|||||||
Reference in New Issue
Block a user