From 558e00cb44271ebff25675ed10b319cfc2be432f Mon Sep 17 00:00:00 2001 From: sudacode Date: Sat, 1 Aug 2026 17:35:59 -0700 Subject: [PATCH] fix(subsync): resolve ffmpeg only when a track needs extraction Stream subtitles arrive as URLs and are downloaded, never piped through ffmpeg, so demanding an ffmpeg install up front broke stream syncs (and CI, which has no ffmpeg). Resolution is now lazy, scoped to embedded tracks. --- .../services/__tests__/stats-server.test.ts | 4 +- src/core/services/mpv-http-headers.ts | 5 + src/core/services/subsync-extract.test.ts | 34 ++++-- src/core/services/subsync-extract.ts | 113 +++++++++++------- src/core/services/subsync-remote.test.ts | 5 +- src/core/services/subsync.ts | 6 +- src/subsync/executables.ts | 10 +- src/subsync/utils.ts | 16 ++- 8 files changed, 134 insertions(+), 59 deletions(-) diff --git a/src/core/services/__tests__/stats-server.test.ts b/src/core/services/__tests__/stats-server.test.ts index 1106ee53..36ed69f8 100644 --- a/src/core/services/__tests__/stats-server.test.ts +++ b/src/core/services/__tests__/stats-server.test.ts @@ -1805,7 +1805,7 @@ describe('stats server API routes', () => { Stale English subtitle `; fs.writeFileSync(sourcePath, 'fake media'); - fs.writeFileSync(alassPath, 'fake alass'); + fs.writeFileSync(alassPath, 'fake alass', { mode: 0o755 }); fs.writeFileSync( japanesePath, `1 @@ -1865,7 +1865,7 @@ Aligned English subtitle const englishPath = path.join(dir, 'episode.en.srt'); const alassPath = path.join(dir, 'alass-cli'); fs.writeFileSync(sourcePath, 'fake media'); - fs.writeFileSync(alassPath, 'fake alass'); + fs.writeFileSync(alassPath, 'fake alass', { mode: 0o755 }); fs.writeFileSync( japanesePath, `1 diff --git a/src/core/services/mpv-http-headers.ts b/src/core/services/mpv-http-headers.ts index 092f7ad0..8dcd6b42 100644 --- a/src/core/services/mpv-http-headers.ts +++ b/src/core/services/mpv-http-headers.ts @@ -7,16 +7,21 @@ */ const BLOCKED_HTTP_HEADER_NAMES = new Set(['authorization', 'cookie', 'proxy-authorization']); +// Least specific first: every list is merged in order and later entries +// overwrite earlier ones, so the file-local headers win. const HTTP_HEADER_FIELD_PROPERTY_NAMES = [ 'http-header-fields', 'options/http-header-fields', 'file-local-options/http-header-fields', ] as const; +// Most specific first: these resolve through requestFirstNonEmptyStringProperty, +// which stops at the first match, so the file-local value has to lead. const USER_AGENT_PROPERTY_NAMES = [ 'file-local-options/user-agent', 'options/user-agent', 'user-agent', ] as const; +// Most specific first, for the same first-match reason as the user agent. const REFERRER_PROPERTY_NAMES = [ 'file-local-options/referrer', 'options/referrer', diff --git a/src/core/services/subsync-extract.test.ts b/src/core/services/subsync-extract.test.ts index 5fdd939d..5b1eff05 100644 --- a/src/core/services/subsync-extract.test.ts +++ b/src/core/services/subsync-extract.test.ts @@ -46,9 +46,16 @@ test('extractSubtitleTrackToFile downloads an external track served over http', const server = await startSubtitleServer(); try { const result = await extractSubtitleTrackToFile({ - ffmpegPath: '/unused/ffmpeg', + resolveFfmpegPath: () => { + throw new Error('external tracks must not resolve ffmpeg'); + }, videoPath: server.url('/stream.mp4'), - track: { id: 2, type: 'sub', external: true, 'external-filename': server.url('/subs/ja.srt') }, + track: { + id: 2, + type: 'sub', + external: true, + 'external-filename': server.url('/subs/ja.srt'), + }, httpHeaders: null, }); @@ -72,9 +79,16 @@ test('extractSubtitleTrackToFile forwards mpv request headers to the subtitle ho try { const result = await extractSubtitleTrackToFile({ - ffmpegPath: '/unused/ffmpeg', + resolveFfmpegPath: () => { + throw new Error('external tracks must not resolve ffmpeg'); + }, videoPath: server.url('/stream.mp4'), - track: { id: 2, type: 'sub', external: true, 'external-filename': server.url('/subs/ja.srt') }, + track: { + id: 2, + type: 'sub', + external: true, + 'external-filename': server.url('/subs/ja.srt'), + }, httpHeaders, }); cleanupTemporaryFile(result); @@ -92,7 +106,9 @@ test('extractSubtitleTrackToFile reports the HTTP status when the download fails try { await assert.rejects( extractSubtitleTrackToFile({ - ffmpegPath: '/unused/ffmpeg', + resolveFfmpegPath: () => { + throw new Error('external tracks must not resolve ffmpeg'); + }, videoPath: server.url('/stream.mp4'), track: { id: 2, @@ -116,7 +132,9 @@ test('extractSubtitleTrackToFile still uses a local external track in place', as try { const result = await extractSubtitleTrackToFile({ - ffmpegPath: '/unused/ffmpeg', + resolveFfmpegPath: () => { + throw new Error('external tracks must not resolve ffmpeg'); + }, videoPath: path.join(dir, 'video.mkv'), track: { id: 2, type: 'sub', external: true, 'external-filename': localPath }, httpHeaders: null, @@ -134,7 +152,9 @@ test('extractSubtitleTrackToFile still uses a local external track in place', as test('extractSubtitleTrackToFile rejects a missing local external track', async () => { await assert.rejects( extractSubtitleTrackToFile({ - ffmpegPath: '/unused/ffmpeg', + resolveFfmpegPath: () => { + throw new Error('external tracks must not resolve ffmpeg'); + }, videoPath: '/tmp/video.mkv', track: { id: 2, type: 'sub', external: true, 'external-filename': '/tmp/does-not-exist.srt' }, httpHeaders: null, diff --git a/src/core/services/subsync-extract.ts b/src/core/services/subsync-extract.ts index 31870e50..c4754a3f 100644 --- a/src/core/services/subsync-extract.ts +++ b/src/core/services/subsync-extract.ts @@ -24,7 +24,11 @@ export interface FileExtractionResult { } export interface SubtitleExtractionInput { - ffmpegPath: string; + /** + * Resolved lazily: an external track never shells out to ffmpeg, so a stream + * whose subtitles arrive by URL must not fail just because ffmpeg is absent. + */ + resolveFfmpegPath: () => string; videoPath: string; track: MpvTrack; /** mpv's request context, so stream-hosted tracks stay reachable. */ @@ -41,6 +45,19 @@ function extensionForUrl(url: string, fallback: string): string { } } +/** + * Drop a scratch directory whose extraction never completed. + * + * Removal is recursive here — unlike `cleanupTemporaryFile`, nothing in a failed + * directory is worth keeping, and a half-written download would otherwise leave + * the directory behind for the life of the machine. + */ +function discardTemporaryDirectory(tempDir: string): void { + try { + fs.rmSync(tempDir, { recursive: true, force: true }); + } catch {} +} + /** * Pull a subtitle track mpv loaded from a URL down to disk. * @@ -53,18 +70,25 @@ async function downloadRemoteSubtitleTrack( httpHeaders: ResolvedMpvHttpHeaders | null, ): Promise { const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'subminer-subsync-')); - const outputPath = path.join(tempDir, `remote_track.${extensionForUrl(url, 'srt')}`); + try { + const outputPath = path.join(tempDir, `remote_track.${extensionForUrl(url, 'srt')}`); - const result = await downloadToFile(url, outputPath, toRequestHeaders(httpHeaders)); - if (!result.ok) { - throw new Error(`Failed to download subtitle track: ${result.error?.error ?? 'unknown error'}`); - } - if (!fileExists(outputPath)) { - throw new Error(`Downloaded subtitle track is missing: ${url}`); - } + const result = await downloadToFile(url, outputPath, toRequestHeaders(httpHeaders)); + if (!result.ok) { + throw new Error( + `Failed to download subtitle track: ${result.error?.error ?? 'unknown error'}`, + ); + } + if (!fileExists(outputPath)) { + throw new Error(`Downloaded subtitle track is missing: ${url}`); + } - logger.info(`Downloaded remote subtitle track to ${outputPath}`); - return { path: outputPath, temporary: true }; + logger.info(`Downloaded remote subtitle track to ${outputPath}`); + return { path: outputPath, temporary: true }; + } catch (error) { + discardTemporaryDirectory(tempDir); + throw error; + } } async function extractInternalTrack(input: SubtitleExtractionInput): Promise { @@ -78,40 +102,45 @@ async function extractInternalTrack(input: SubtitleExtractionInput): Promise ({ alassPath, ffsubsyncPath: '', - ffmpegPath: '', + // Points nowhere on purpose: both tracks are external, so resolving ffmpeg + // at all would throw. An empty path would just auto-discover the real + // ffmpeg on a dev machine and prove nothing. + ffmpegPath: path.join(tmpDir, 'no-such-ffmpeg'), replace: true, }), }; diff --git a/src/core/services/subsync.ts b/src/core/services/subsync.ts index 38a92efe..247319aa 100644 --- a/src/core/services/subsync.ts +++ b/src/core/services/subsync.ts @@ -212,9 +212,8 @@ async function subsyncToReference( client: MpvClientLike, httpHeaders: ResolvedMpvHttpHeaders | null, ): Promise { - const ffmpegPath = resolveSubsyncExecutable(resolved.ffmpegPath, 'ffmpeg'); const primaryExtraction = await extractSubtitleTrackToFile({ - ffmpegPath, + resolveFfmpegPath: () => resolveSubsyncExecutable(resolved.ffmpegPath, 'ffmpeg'), videoPath: context.videoPath, track: context.primaryTrack, httpHeaders, @@ -311,11 +310,10 @@ export async function runSubsyncManual( return { ok: false, message: 'Select a subtitle source track for alass' }; } - const ffmpegPath = resolveSubsyncExecutable(resolved.ffmpegPath, 'ffmpeg'); let sourceExtraction: FileExtractionResult | null = null; try { sourceExtraction = await extractSubtitleTrackToFile({ - ffmpegPath, + resolveFfmpegPath: () => resolveSubsyncExecutable(resolved.ffmpegPath, 'ffmpeg'), videoPath: context.videoPath, track: sourceTrack, httpHeaders, diff --git a/src/subsync/executables.ts b/src/subsync/executables.ts index 2f0aaee4..5f45cec3 100644 --- a/src/subsync/executables.ts +++ b/src/subsync/executables.ts @@ -28,9 +28,17 @@ function unique(values: string[]): string[] { return values.filter((value, index) => value.length > 0 && values.indexOf(value) === index); } +/** + * A same-named non-executable file earlier on PATH would otherwise shadow the + * real binary and surface as a spawn EACCES rather than "keep looking". Windows + * has no execute bit, so the regular-file check is all it can offer there. + */ export function isExecutableFile(filePath: string): boolean { try { - return fs.statSync(filePath).isFile(); + if (!fs.statSync(filePath).isFile()) return false; + if (process.platform === 'win32') return true; + fs.accessSync(filePath, fs.constants.X_OK); + return true; } catch { return false; } diff --git a/src/subsync/utils.ts b/src/subsync/utils.ts index ad29369e..c067529b 100644 --- a/src/subsync/utils.ts +++ b/src/subsync/utils.ts @@ -92,11 +92,23 @@ export function getSubsyncConfig(config: SubsyncConfig | undefined): SubsyncReso }; } +/** + * ffsubsync streams a progress bar to stderr, so an unbounded summary reaches + * the OSD (and the error log) as tens of kilobytes of carriage returns. The tail + * is the part that says what went wrong. + */ +const COMMAND_OUTPUT_SUMMARY_LIMIT = 2000; + +function tailForSummary(value: string): string { + if (value.length <= COMMAND_OUTPUT_SUMMARY_LIMIT) return value; + return `…${value.slice(-COMMAND_OUTPUT_SUMMARY_LIMIT)}`; +} + export function summarizeCommandFailure(command: string, result: CommandResult): string { const parts = [ `code=${result.code ?? 'n/a'}`, - result.stderr ? `stderr: ${result.stderr}` : '', - result.stdout ? `stdout: ${result.stdout}` : '', + result.stderr ? `stderr: ${tailForSummary(result.stderr)}` : '', + result.stdout ? `stdout: ${tailForSummary(result.stdout)}` : '', result.error ? `error: ${result.error}` : '', ] .map((value) => value.trim())