From 9f8da18cd012472b16cc9461bb457a8f575c329a Mon Sep 17 00:00:00 2001 From: Roland Date: Tue, 15 Sep 2026 14:38:37 -0600 Subject: [PATCH] Transcode: remove the temp output on cancel; tell the user when a closed-modal Job fails or is lost Review fixes. The temp output was only removed on Exception, but a cancelled Job's run_command raises CancelledError (a BaseException), so a cancel left the partial file for a refresh to index. Closing the modal during a Job that then failed showed nothing; it now toasts the error (and a cancellation). Polling gives up on a 404 or five failures in a row (the API restarted and forgot its Jobs), warning the user and unlocking Transcode and Delete instead of reading Transcoding... forever. Co-Authored-By: Claude Fable 5.1 --- app/src/api.js | 4 +- app/src/components/VideoEditMenu.js | 58 +++++++++++++------ app/src/components/VideoEditMenu.test.js | 74 ++++++++++++++++++++++-- modules/videos/test/test_transcode.py | 20 +++++++ modules/videos/transcode.py | 4 +- 5 files changed, 135 insertions(+), 25 deletions(-) diff --git a/app/src/api.js b/app/src/api.js index 6eae64d9..c7894edb 100644 --- a/app/src/api.js +++ b/app/src/api.js @@ -274,7 +274,9 @@ export async function getJobs() { export async function getJob(jobId) { const response = await apiGet(`${API_URI}/jobs/${jobId}`); if (!response.ok) { - throw new Error(await getErrorMessage(response, 'Unable to get the job')); + const error = new Error(await getErrorMessage(response, 'Unable to get the job')); + error.status = response.status; + throw error; } const data = await response.json(); return data['job']; diff --git a/app/src/components/VideoEditMenu.js b/app/src/components/VideoEditMenu.js index 20ec4a66..9af0493e 100644 --- a/app/src/components/VideoEditMenu.js +++ b/app/src/components/VideoEditMenu.js @@ -15,6 +15,8 @@ import { } from './Vars'; const JOB_POLL_MS = 2000; +// Consecutive failed polls before a Job is given up as lost (the API restarted and forgot it). +const JOB_POLL_MAX_FAILURES = 5; export const FINISHED_STATUSES = ['complete', 'failed', 'cancelled']; // ffprobe calls an embedded thumbnail a video stream. const THUMBNAIL_CODECS = ['mjpeg', 'png', 'bmp', 'gif']; @@ -73,16 +75,16 @@ const statusKind = (status) => { } /** - * Poll a Job until it finishes. Lives in whichever component must outlive the UI showing the Job: - * the page must learn the Job finished even when the user closed the modal that started it. - * - * A failed request is retried on the next tick; only unmount, a new `jobId`, or a finished status - * stops the polling. `onFinished` is called once per Job. + * Poll a Job until it finishes; `onFinished(job)` is called once. A failed request is retried; + * a 404 (the API restarted and forgot its Jobs) or `JOB_POLL_MAX_FAILURES` failures in a row stop + * the polling and call `onLost(error)` instead. */ -export function useJob(jobId, onFinished) { +export function useJob(jobId, onFinished, onLost) { const [job, setJob] = useState(null); const onFinishedRef = useRef(onFinished); + const onLostRef = useRef(onLost); onFinishedRef.current = onFinished; + onLostRef.current = onLost; useEffect(() => { setJob(null); @@ -91,13 +93,20 @@ export function useJob(jobId, onFinished) { } let stopped = false; let timer = null; + let failures = 0; const poll = async () => { let latest = null; try { latest = await getJob(jobId); + failures = 0; } catch (e) { console.error(e); + failures += 1; + if (!stopped && (e?.status === 404 || failures >= JOB_POLL_MAX_FAILURES)) { + if (onLostRef.current) onLostRef.current(e); + return; + } } if (stopped) return; if (latest) { @@ -187,11 +196,8 @@ export function JobProgress({job, onCancel}) { } /** - * Choose target codecs for one Video and queue the transcode Job, or show the Job already - * queued for it. The Job itself is owned by the parent (`useJob`), so closing this modal does not - * stop the page from learning when it finishes. - * - * The selects start at the user's preferred codecs from the Videos settings. + * Choose target codecs for one Video and queue the transcode Job (`onQueued(jobId)`), or show the + * given `job`. The selects start at the user's preferred codecs from the Videos settings. */ export function TranscodeModal({ open, onClose, fileGroupId, video, audioOnly = false, job, jobId, onQueued, @@ -274,7 +280,6 @@ export function TranscodeModal({ }; const describe = (codecs) => codecs.length ? codecs.join(', ') : 'unknown'; - // A queued or finished Job replaces the form until the parent forgets it. const showJob = !!jobId; let hint = null; @@ -282,9 +287,11 @@ export function TranscodeModal({ hint = `The video stream is removed; the result is an audio file (.${container}). ` + (audioCodec === TRANSCODE_COPY ? 'The audio is copied without re-encoding.' : `The audio is converted to ${audioCodec}.`); } else if (defaultsLoaded && remuxOnly) { + const kept = audioOnly ? 'The audio is kept' : 'Both streams are kept'; hint = sameContainer - ? 'Both streams are kept: the file is rewritten as-is with fast start (moves the index to the front for quicker playback start).' - : `Both streams are kept: only the container changes to ${container} (a quick remux, no quality loss).`; + ? `${kept}: the file is rewritten as-is` + (container === 'mp4' || container === 'm4a' + ? ' with fast start (moves the index to the front for quicker playback start).' : '.') + : `${kept}: only the container changes to ${container} (a quick remux, no quality loss).`; } const buttonLabel = removeVideo ? 'Extract Audio' : remuxOnly ? 'Remux' : 'Transcode'; @@ -431,10 +438,8 @@ export function EditVideoModal({open, onClose, fileGroupId, videoFile, descripti } /** - * The "Edit" dropdown on the Video page: Edit, Refresh, Transcode, Delete. - * - * Owns the active transcode Job for this video, so the page hears about its completion whether or - * not the modal is open, and a second transcode cannot be queued while one is running. + * The "Edit" dropdown on the Video page: Edit, Refresh, Transcode, Delete. Holds this video's + * active transcode Job; `onTranscodeComplete` fires whether or not the modal is open. */ export function VideoEditMenu({ videoFile, video, description, onRefresh, onTranscodeComplete, onDelete, onSaved, @@ -453,13 +458,28 @@ export function VideoEditMenu({ if (finished.status === 'complete') { toast({type: 'success', title: 'Transcode complete', description: finished.description, time: 5000}); if (onTranscodeComplete) onTranscodeComplete(finished); + } else if (finished.status === 'failed') { + toast({ + type: 'error', title: 'Transcode failed', time: 10000, + description: `${finished.description}: ${finished.error || 'unknown error'}. The original file was kept.`, + }); + } else if (finished.status === 'cancelled') { + toast({type: 'info', title: 'Transcode cancelled', description: finished.description, time: 5000}); } if (!transcodeOpen) { // Nobody is looking at the result; make room for the next transcode. setJobId(null); } }; - const {job, cancel} = useJob(jobId, handleJobFinished); + const handleJobLost = (error) => { + toast({ + type: 'warning', title: 'Lost track of the transcode', time: 10000, + description: 'The job is no longer known (the API may have restarted). Check the file before trying again.', + }); + setJobId(null); + setTranscodeOpen(false); + }; + const {job, cancel} = useJob(jobId, handleJobFinished, handleJobLost); // Another video: its transcodes are its own. const firstVideoId = useRef(videoFile?.id); diff --git a/app/src/components/VideoEditMenu.test.js b/app/src/components/VideoEditMenu.test.js index b4cf8896..6351d634 100644 --- a/app/src/components/VideoEditMenu.test.js +++ b/app/src/components/VideoEditMenu.test.js @@ -20,6 +20,10 @@ jest.mock('../api', () => ({ import {cancelJob, fetchVideoDownloadDefaults, getJob, transcodeVideo, updateVideo} from '../api'; +// The ui index re-exports `toast` from here; a spy lets tests see what the page tells the user. +jest.mock('./ui/toast', () => ({...jest.requireActual('./ui/toast'), toast: jest.fn()})); +import {toast} from './ui/toast'; + const video = { codec_names: ['vp9', 'mjpeg', 'opus'], codec_types: ['video', 'video', 'audio'], @@ -42,6 +46,8 @@ const pendingJob = { }; const runningJob = {...pendingJob, status: 'running', progress: 40, log: ['out_time_us=1']}; const completeJob = {...pendingJob, status: 'complete', progress: 100, log: ['done']}; +const failedJob = {...pendingJob, status: 'failed', progress: 12, error: 'ffmpeg exited with 1', log: ['boom']}; +const notFound = Object.assign(new Error('Job does not exist'), {status: 404}); describe('helpers', () => { test('currentCodecs ignores embedded thumbnail streams', () => { @@ -149,7 +155,9 @@ describe('TranscodeModal', () => { renderWithProviders(); await waitFor(() => expect(selectInput('Container')).toHaveValue('mkv')); - expect(await screen.findByText(/rewritten as-is with fast start/)).toBeInTheDocument(); + // mkv has no fast start; the hint must not promise one. + expect(await screen.findByText(/Both streams are kept: the file is rewritten as-is\./)).toBeInTheDocument(); + expect(screen.queryByText(/fast start/)).not.toBeInTheDocument(); expect(screen.getByRole('button', {name: 'Remux'})).toBeEnabled(); }); @@ -165,7 +173,9 @@ describe('TranscodeModal', () => { // The aac source, copied, belongs in m4a; the file already is one. await waitFor(() => expect(selectInput('Container')).toHaveValue('m4a (aac)')); expect(screen.queryAllByLabelText('Video codec')).toHaveLength(0); - expect(await screen.findByText(/rewritten as-is with fast start/)).toBeInTheDocument(); + expect(await screen.findByText(/The audio is kept: the file is rewritten as-is with fast start/)) + .toBeInTheDocument(); + expect(screen.queryByText(/Both streams/)).not.toBeInTheDocument(); fireEvent.click(screen.getByRole('button', {name: 'Remux'})); // No video codec is sent for an audio file, whatever the video preferences say. @@ -262,8 +272,8 @@ describe('EditVideoModal', () => { }); // A tiny harness so the hook can be driven like the menu drives it. -function JobHarness({jobId, onFinished}) { - const {job, cancel} = useJob(jobId, onFinished); +function JobHarness({jobId, onFinished, onLost}) { + const {job, cancel} = useJob(jobId, onFinished, onLost); return ; } @@ -335,6 +345,31 @@ describe('useJob + JobProgress', () => { expect(onFinished).toHaveBeenCalledTimes(1); }); + test('a 404 means the API forgot the Job: give up at once', async () => { + getJob.mockRejectedValue(notFound); + const onLost = jest.fn(); + const onFinished = jest.fn(); + renderWithProviders(); + + await waitFor(() => expect(onLost).toHaveBeenCalledTimes(1)); + expect(onLost.mock.calls[0][0].status).toBe(404); + await nextPoll(); + expect(getJob).toHaveBeenCalledTimes(1); + expect(onFinished).not.toHaveBeenCalled(); + }); + + test('repeated failures give up after a few polls', async () => { + getJob.mockRejectedValue(new Error('network')); + const onLost = jest.fn(); + renderWithProviders(); + + for (let i = 0; i < 6; i++) { + await nextPoll(); + } + await waitFor(() => expect(onLost).toHaveBeenCalledTimes(1)); + expect(getJob).toHaveBeenCalledTimes(5); + }); + test('Cancel asks the API to cancel the Job', async () => { getJob.mockResolvedValue({...pendingJob, status: 'running', progress: 10}); cancelJob.mockResolvedValue({...pendingJob, status: 'running', progress: 10, cancel_requested: true}); @@ -505,6 +540,37 @@ describe('VideoEditMenu', () => { expect(transcodeVideo).not.toHaveBeenCalled(); }); + test('a Job that fails after the modal was closed still tells the user', async () => { + getJob.mockResolvedValue(failedJob); + renderWithProviders( + , + ); + await waitFor(() => expect(toast).toHaveBeenCalledTimes(1)); + expect(toast.mock.calls[0][0]).toMatchObject({type: 'error', title: 'Transcode failed'}); + expect(toast.mock.calls[0][0].description).toMatch(/ffmpeg exited with 1/); + expect(toast.mock.calls[0][0].description).toMatch(/original file was kept/); + + // Forgotten: the menu is unlocked again. + await openMenu(); + expect(screen.getByRole('menuitem', {name: 'Transcode...'})).toBeEnabled(); + expect(screen.getByRole('menuitem', {name: /Delete/})).toBeEnabled(); + }); + + test('a Job the API no longer knows unlocks the menu with a warning', async () => { + getJob.mockRejectedValue(notFound); + renderWithProviders( + , + ); + await waitFor(() => expect(toast).toHaveBeenCalledTimes(1)); + expect(toast.mock.calls[0][0]).toMatchObject({type: 'warning', title: 'Lost track of the transcode'}); + + await openMenu(); + expect(screen.getByRole('menuitem', {name: 'Transcode...'})).toBeEnabled(); + expect(screen.getByRole('menuitem', {name: /Delete/})).toBeEnabled(); + }); + test('a Job that finished unwatched is forgotten, so the next open offers the form', async () => { getJob.mockResolvedValue(completeJob); renderWithProviders( diff --git a/modules/videos/test/test_transcode.py b/modules/videos/test/test_transcode.py index b6c74310..43a28410 100644 --- a/modules/videos/test/test_transcode.py +++ b/modules/videos/test/test_transcode.py @@ -1,3 +1,4 @@ +import asyncio import json import pathlib from unittest import mock @@ -401,3 +402,22 @@ async def test_transcode_video_file_rejects_undecodable_tail(test_directory, asy assert '-sseof' in cmd and cmd[cmd.index('-sseof') + 1].startswith('-') assert str(cmd[cmd.index('-i') + 1]).endswith('video.transcode.mp4') assert cmd[cmd.index('-f') + 1] == 'null' + + +@pytest.mark.asyncio +async def test_transcode_video_file_cancel_removes_tmp(test_directory, async_client): + """A cancelled encode (a Job's run_command raises CancelledError, a BaseException) must still + remove the temporary output and leave the original.""" + video_path = test_directory / 'video.webm' + video_path.write_bytes(b'fake video data') + + async def fake_run_command(cmd, **kwargs): + pathlib.Path(cmd[-1]).write_bytes(b'partial data') + raise asyncio.CancelledError('killed') + + with mock.patch('modules.videos.transcode.run_command', side_effect=fake_run_command): + with pytest.raises(asyncio.CancelledError): + await transcode_video_file(video_path, target_vcodec='h264') + + assert video_path.read_bytes() == b'fake video data' + assert not (test_directory / 'video.transcode.mp4').exists(), 'The partial output must be removed' diff --git a/modules/videos/transcode.py b/modules/videos/transcode.py index 05d6d0c9..97941daf 100644 --- a/modules/videos/transcode.py +++ b/modules/videos/transcode.py @@ -382,7 +382,9 @@ async def _transcode_video_file(video_path: pathlib.Path, target_vcodec: Optiona output_probe = await probe_for_verify(tmp_path) verify_transcode_output(source_probe, output_probe, target_vcodec, target_acodec, container) await _verify_tail_decodes(tmp_path, runner) - except Exception: + except BaseException: + # BaseException: a cancelled Job's run_command raises CancelledError, which must not leave + # the partial output behind for a refresh to index. tmp_path.unlink(missing_ok=True) raise