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())