From 66bf0db0fcc6b03781bb36abb3b420b444ef5994 Mon Sep 17 00:00:00 2001 From: sudacode Date: Sun, 16 Aug 2026 16:55:21 -0700 Subject: [PATCH] fix(anki): prevent media timing review hangs and invalid ranges - Reject stale or invalid timing actions - Fall back to original timing when the renderer stops responding --- .../services/media-timing-preview.test.ts | 49 +++++++++- src/core/services/media-timing-preview.ts | 2 + src/main/runtime/media-timing-review-open.ts | 4 +- src/main/runtime/media-timing-review.test.ts | 93 +++++++++++++++++++ src/main/runtime/media-timing-review.ts | 50 +++++----- src/preload.ts | 11 ++- src/renderer/handlers/keyboard.test.ts | 1 + src/renderer/handlers/keyboard.ts | 4 +- 8 files changed, 185 insertions(+), 29 deletions(-) diff --git a/src/core/services/media-timing-preview.test.ts b/src/core/services/media-timing-preview.test.ts index 02569d12..b972a8df 100644 --- a/src/core/services/media-timing-preview.test.ts +++ b/src/core/services/media-timing-preview.test.ts @@ -22,6 +22,23 @@ describe('buildMediaTimingPreviewArgs', () => { assert.equal(args.at(-2), '--'); assert.equal(args.at(-1), '/video/show.mkv'); }); + + test('separates an option-like media path without adding optional audio arguments', () => { + const args = buildMediaTimingPreviewArgs('/tmp/review.sock', { + mediaPath: '--fullscreen', + }); + + assert.equal(args.at(-2), '--'); + assert.equal(args.at(-1), '--fullscreen'); + assert.equal( + args.some((arg) => arg.startsWith('--aid=')), + false, + ); + assert.equal( + args.some((arg) => arg.startsWith('--volume=')), + false, + ); + }); }); test('preview session handles socket errors after connecting', async () => { @@ -45,6 +62,31 @@ test('preview session handles socket errors after connecting', async () => { session.dispose(); }); +test('preview session keeps failed connection errors handled through destruction', async () => { + const socket = new EventEmitter() as EventEmitter & { + destroy: () => void; + }; + socket.destroy = () => { + socket.emit('error', new Error('socket failed again while closing')); + }; + const child = new EventEmitter() as EventEmitter & { kill: () => boolean }; + child.kill = () => true; + const times = [0, 0, 0, 6_000]; + const session = new MediaTimingPreviewSession({ + platform: 'linux', + spawnProcess: () => child as never, + connectSocket: () => { + queueMicrotask(() => socket.emit('error', new Error('connection failed'))); + return socket as never; + }, + now: () => times.shift() ?? 6_000, + removeSocketFile: () => undefined, + createSocketPath: () => '/tmp/review.sock', + }); + + await assert.rejects(session.start({ mediaPath: '/video/show.mkv' }), /Timed out starting/); +}); + test('preview session rejects a connection that finishes after disposal', async () => { const socket = new net.Socket(); const child = new EventEmitter() as EventEmitter & { kill: () => boolean }; @@ -126,7 +168,12 @@ test('preview session bounds a connection attempt that never settles', async () spawnProcess: () => child as never, connectSocket: () => { connectAttempts += 1; - return new net.Socket(); + const socket = new net.Socket(); + socket.destroy = (() => { + socket.emit('error', new Error('socket failed while timing out')); + return socket; + }) as typeof socket.destroy; + return socket; }, now: () => { const current = nowMs; diff --git a/src/core/services/media-timing-preview.ts b/src/core/services/media-timing-preview.ts index 8563a956..f7c5ab9c 100644 --- a/src/core/services/media-timing-preview.ts +++ b/src/core/services/media-timing-preview.ts @@ -290,6 +290,7 @@ export class MediaTimingPreviewSession { settled = true; clearAttemptTimeout(); socket.off('connect', onConnect); + socket.on('error', () => {}); socket.destroy(); reject(error); }; @@ -301,6 +302,7 @@ export class MediaTimingPreviewSession { timeout = null; socket.off('connect', onConnect); socket.off('error', onError); + socket.on('error', () => {}); socket.destroy(); reject(new Error('Timed out connecting to the hidden mpv preview player')); }, timeoutMs); diff --git a/src/main/runtime/media-timing-review-open.ts b/src/main/runtime/media-timing-review-open.ts index 9307a527..e45dfb9f 100644 --- a/src/main/runtime/media-timing-review-open.ts +++ b/src/main/runtime/media-timing-review-open.ts @@ -1,4 +1,4 @@ -import type { OverlayHostedModal } from '../../shared/ipc/contracts'; +import { IPC_CHANNELS, type OverlayHostedModal } from '../../shared/ipc/contracts'; import type { MediaTimingReviewOpenPayload } from '../../types/anki'; import { openOverlayHostedModal, retryOverlayModalOpen } from './overlay-hosted-modal-open'; @@ -30,7 +30,7 @@ export async function openMediaTimingReviewModal( 'Media timing review did not acknowledge modal open; retrying the dedicated modal window.', sendOpen: () => openOverlayHostedModal(deps, { - channel: 'media-timing-review:open', + channel: IPC_CHANNELS.event.mediaTimingReviewOpen, modal: MODAL, payload, preferModalWindow: true, diff --git a/src/main/runtime/media-timing-review.test.ts b/src/main/runtime/media-timing-review.test.ts index 60049de5..43e30993 100644 --- a/src/main/runtime/media-timing-review.test.ts +++ b/src/main/runtime/media-timing-review.test.ts @@ -1,5 +1,6 @@ import assert from 'node:assert/strict'; import { describe, test } from 'node:test'; +import type { MediaTimingReviewOpenPayload } from '../../types/anki'; import { buildMediaTimingReviewPayload, createMediaTimingReviewRuntime, @@ -63,6 +64,54 @@ describe('buildMediaTimingReviewPayload', () => { }); }); +async function startActiveMediaTimingReview( + options: { + maxMediaDuration?: number; + decisionTimeoutMs?: number; + } = {}, +) { + const previewCalls: Array<[number, number]> = []; + let publishPayload!: (payload: MediaTimingReviewOpenPayload) => void; + const openedPayload = new Promise((resolve) => { + publishPayload = resolve; + }); + const runtime = createMediaTimingReviewRuntime({ + getMpvClient: () => ({ + connected: true, + currentVideoPath: '/video/show.mkv', + requestProperty: async (name) => (name === 'duration' ? 100 : name === 'pause' ? true : null), + send: () => undefined, + }), + getCurrentMediaPath: () => '/video/show.mkv', + getMpvExecutablePath: () => 'mpv', + generateWaveform: async () => [], + decisionTimeoutMs: options.decisionTimeoutMs, + createPreviewSession: () => ({ + start: async () => undefined, + play: async (startTime, endTime) => { + previewCalls.push([startTime, endTime]); + }, + stop: async () => undefined, + dispose: () => undefined, + }), + openModal: async (payload) => { + publishPayload(payload); + return true; + }, + showStatus: () => undefined, + }); + const pendingDecision = runtime.requestReview({ + kind: 'sentence', + text: '字幕', + startTime: 10, + endTime: 12, + audioPadding: 0, + maxMediaDuration: options.maxMediaDuration ?? 30, + }); + + return { runtime, payload: await openedPayload, pendingDecision, previewCalls }; +} + test('media timing review pauses playback, resolves exact timing, and restores playing state', async () => { const commands: Array> = []; const previewCalls: Array<[number, number]> = []; @@ -189,6 +238,50 @@ test('media timing review analyzes the visible range on the selected audio strea ]); }); +test('media timing review rejects stale and out-of-range actions before allowing discard', async () => { + const { runtime, payload, pendingDecision, previewCalls } = await startActiveMediaTimingReview({ + maxMediaDuration: 3, + }); + + assert.deepEqual( + await runtime.previewRange({ reviewId: 'stale-review', startTime: 10, endTime: 12 }), + { ok: false, message: 'This timing review is no longer active.' }, + ); + assert.deepEqual( + runtime.resolveReview({ + reviewId: 'stale-review', + decision: { action: 'confirm', startTime: 10, endTime: 12 }, + }), + { ok: false, message: 'This timing review is no longer active.' }, + ); + assert.deepEqual( + runtime.resolveReview({ + reviewId: payload.reviewId, + decision: { action: 'confirm', startTime: 10, endTime: 14 }, + }), + { ok: false, message: 'The selected timing range is invalid.' }, + ); + assert.deepEqual( + runtime.resolveReview({ + reviewId: payload.reviewId, + decision: { action: 'confirm', startTime: 99, endTime: 100.5 }, + }), + { ok: false, message: 'The selected timing range is invalid.' }, + ); + assert.deepEqual( + runtime.resolveReview({ reviewId: payload.reviewId, decision: { action: 'discard' } }), + { ok: true }, + ); + assert.deepEqual(await pendingDecision, { action: 'discard' }); + assert.deepEqual(previewCalls, []); +}); + +test('media timing review watchdog falls back when the renderer stops responding', async () => { + const { pendingDecision } = await startActiveMediaTimingReview({ decisionTimeoutMs: 0 }); + + assert.deepEqual(await pendingDecision, { action: 'use-original' }); +}); + test('media timing review does not resume playback when the prior state is unavailable', async () => { const commands: Array> = []; let runtime: ReturnType; diff --git a/src/main/runtime/media-timing-review.ts b/src/main/runtime/media-timing-review.ts index 1eda8e6c..183353fa 100644 --- a/src/main/runtime/media-timing-review.ts +++ b/src/main/runtime/media-timing-review.ts @@ -12,6 +12,7 @@ import type { import type { SpeechWaveformOptions } from '../../core/services/media-timing-waveform'; const INITIAL_TIMELINE_MARGIN_SECONDS = 2; +const REVIEW_DECISION_TIMEOUT_MS = 5 * 60_000; interface ReviewMpvClient { connected: boolean; @@ -49,6 +50,7 @@ export interface MediaTimingReviewRuntimeDeps { getMpvExecutablePath: () => string; createPreviewSession: () => PreviewSession; generateWaveform: (options: SpeechWaveformOptions) => Promise; + decisionTimeoutMs?: number; openModal: (payload: MediaTimingReviewOpenPayload) => Promise; showStatus: (message: string) => void; } @@ -64,6 +66,21 @@ function booleanProperty(value: unknown): boolean | null { return null; } +function isValidMediaTimingRange( + payload: MediaTimingReviewOpenPayload, + startTime: number, + endTime: number, +): boolean { + return ( + Number.isFinite(startTime) && + Number.isFinite(endTime) && + startTime >= 0 && + endTime > startTime && + (payload.maxMediaDuration <= 0 || endTime - startTime <= payload.maxMediaDuration + 0.001) && + (payload.mediaDuration === undefined || endTime <= payload.mediaDuration + 0.001) + ); +} + export function buildMediaTimingReviewPayload( request: MediaTimingReviewRequest, options: { reviewId: string; mediaDuration?: number }, @@ -178,7 +195,16 @@ export function createMediaTimingReviewRuntime(deps: MediaTimingReviewRuntimeDep return { action: 'use-original' }; } - const decision = await decisionPromise; + const decisionWatchdog = setTimeout( + () => resolveDecision({ action: 'use-original' }), + Math.max(0, deps.decisionTimeoutMs ?? REVIEW_DECISION_TIMEOUT_MS), + ); + let decision: MediaTimingReviewDecision; + try { + decision = await decisionPromise; + } finally { + clearTimeout(decisionWatchdog); + } await cleanupActiveReview(); return decision; } @@ -210,16 +236,7 @@ export function createMediaTimingReviewRuntime(deps: MediaTimingReviewRuntimeDep if (!current || request.reviewId !== current.payload.reviewId) { return { ok: false, message: 'This timing review is no longer active.' }; } - if ( - !Number.isFinite(request.startTime) || - !Number.isFinite(request.endTime) || - request.startTime < 0 || - request.endTime <= request.startTime || - (current.payload.maxMediaDuration > 0 && - request.endTime - request.startTime > current.payload.maxMediaDuration + 0.001) || - (current.payload.mediaDuration !== undefined && - request.endTime > current.payload.mediaDuration + 0.001) - ) { + if (!isValidMediaTimingRange(current.payload, request.startTime, request.endTime)) { return { ok: false, message: 'The selected preview range is invalid.' }; } try { @@ -294,16 +311,7 @@ export function createMediaTimingReviewRuntime(deps: MediaTimingReviewRuntimeDep } if (request.decision.action === 'confirm') { const { startTime, endTime } = request.decision; - if ( - !Number.isFinite(startTime) || - !Number.isFinite(endTime) || - startTime < 0 || - endTime <= startTime || - (current.payload.maxMediaDuration > 0 && - endTime - startTime > current.payload.maxMediaDuration + 0.001) || - (current.payload.mediaDuration !== undefined && - endTime > current.payload.mediaDuration + 0.001) - ) { + if (!isValidMediaTimingRange(current.payload, startTime, endTime)) { return { ok: false, message: 'The selected timing range is invalid.' }; } } diff --git a/src/preload.ts b/src/preload.ts index 9615f989..ad6f61a8 100644 --- a/src/preload.ts +++ b/src/preload.ts @@ -69,6 +69,7 @@ import type { OverlayNotificationEventPayload, OverlayNotificationPosition, ChangelogSnapshot, + MediaTimingReviewActionResult, MediaTimingReviewOpenPayload, MediaTimingReviewPreviewRequest, MediaTimingReviewResolveRequest, @@ -468,13 +469,17 @@ const electronAPI: ElectronAPI = { onOpenTsukihime: onOpenTsukihimeEvent, onOpenYoutubeTrackPicker: onOpenYoutubeTrackPickerEvent, onOpenMediaTimingReview: onOpenMediaTimingReviewEvent, - previewMediaTimingReview: (request: MediaTimingReviewPreviewRequest) => + previewMediaTimingReview: ( + request: MediaTimingReviewPreviewRequest, + ): Promise => ipcRenderer.invoke(IPC_CHANNELS.request.mediaTimingReviewPreview, request), getMediaTimingReviewWaveform: (request: MediaTimingReviewWaveformRequest) => ipcRenderer.invoke(IPC_CHANNELS.request.mediaTimingReviewWaveform, request), - stopMediaTimingReviewPreview: (reviewId: string) => + stopMediaTimingReviewPreview: (reviewId: string): Promise => ipcRenderer.invoke(IPC_CHANNELS.request.mediaTimingReviewStopPreview, reviewId), - resolveMediaTimingReview: (request: MediaTimingReviewResolveRequest) => + resolveMediaTimingReview: ( + request: MediaTimingReviewResolveRequest, + ): Promise => ipcRenderer.invoke(IPC_CHANNELS.request.mediaTimingReviewResolve, request), onOpenPlaylistBrowser: onOpenPlaylistBrowserEvent, onOpenCharacterDictionaryManager: onOpenCharacterDictionaryManagerEvent, diff --git a/src/renderer/handlers/keyboard.test.ts b/src/renderer/handlers/keyboard.test.ts index 0e79ed19..a0fce076 100644 --- a/src/renderer/handlers/keyboard.test.ts +++ b/src/renderer/handlers/keyboard.test.ts @@ -494,6 +494,7 @@ function createKeyboardHandlerHarness() { handleKikuKeydown: () => false, handleJimakuKeydown: () => false, handleTsukihimeKeydown: () => false, + handleMediaTimingReviewKeydown: () => false, handleControllerSelectKeydown: () => { controllerSelectKeydownCount += 1; return true; diff --git a/src/renderer/handlers/keyboard.ts b/src/renderer/handlers/keyboard.ts index 95c16d5f..d83708ad 100644 --- a/src/renderer/handlers/keyboard.ts +++ b/src/renderer/handlers/keyboard.ts @@ -18,7 +18,7 @@ export function createKeyboardHandlers( handleJimakuKeydown: (e: KeyboardEvent) => boolean; handleTsukihimeKeydown: (e: KeyboardEvent) => boolean; handleYoutubePickerKeydown: (e: KeyboardEvent) => boolean; - handleMediaTimingReviewKeydown?: (e: KeyboardEvent) => boolean; + handleMediaTimingReviewKeydown: (e: KeyboardEvent) => boolean; handlePlaylistBrowserKeydown: (e: KeyboardEvent) => boolean; handleControllerSelectKeydown: (e: KeyboardEvent) => boolean; handleControllerDebugKeydown: (e: KeyboardEvent) => boolean; @@ -1080,7 +1080,7 @@ export function createKeyboardHandlers( document.addEventListener('keydown', (e: KeyboardEvent) => { if (ctx.state.mediaTimingReviewModalOpen) { - options.handleMediaTimingReviewKeydown?.(e); + options.handleMediaTimingReviewKeydown(e); return; }