mirror of
https://github.com/lrnselfreliance/wrolpi.git
synced 2026-10-25 18:40:06 -04:00
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 <noreply@anthropic.com>
This commit is contained in:
Roland
parent
ec534186d1
commit
9f8da18cd0
5 changed files with 135 additions and 25 deletions
|
|
@ -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'];
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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(<TranscodeModal open={true} onClose={jest.fn()} fileGroupId={7} video={mkv}/>);
|
||||
|
||||
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 <JobProgress job={job} onCancel={cancel}/>;
|
||||
}
|
||||
|
||||
|
|
@ -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(<JobHarness jobId='transcode_video-abc' onFinished={onFinished} onLost={onLost}/>);
|
||||
|
||||
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(<JobHarness jobId='transcode_video-abc' onLost={onLost}/>);
|
||||
|
||||
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(
|
||||
<VideoEditMenu videoFile={videoFile} video={video} onRefresh={jest.fn()} onDelete={jest.fn()}
|
||||
onTranscodeComplete={jest.fn()} initialJobId='transcode_video-abc'/>,
|
||||
);
|
||||
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(
|
||||
<VideoEditMenu videoFile={videoFile} video={video} onRefresh={jest.fn()} onDelete={jest.fn()}
|
||||
initialJobId='transcode_video-abc'/>,
|
||||
);
|
||||
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(
|
||||
|
|
|
|||
|
|
@ -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'
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue