From 5c778a9442ffb59c592dcd23968618da2a3b5370 Mon Sep 17 00:00:00 2001 From: sudacode Date: Thu, 6 Aug 2026 22:41:16 -0700 Subject: [PATCH] fix(subsync): decode file:// tracks mpv reports for dropped subtitles - Sync a subtitle dropped onto mpv without hitting "Protocol file: not supported"; decode file:// external-filename/path back to a real path for target, reference, and video - Stream strip proxy: destroy the connection instead of retrying/502 once a response is handed off, and cap buffered playlist bodies - Bridge installer: cap downloaded bundle size to guard against a lying/missing content-length - Anime browser playback: bump generation on dispose so a stale in-flight playEpisode cleans up its own subtitle cache - Fix a flaky sidecar-process test by binding to an OS-assigned port instead of pre-allocating one --- changes/subsync-file-url-tracks.md | 4 + src/anime-bridge/sidecar-process.test.ts | 7 +- src/anime-bridge/stream-strip-proxy.ts | 24 +++++- src/core/services/subsync-extract.ts | 7 +- src/core/services/subsync-remote.test.ts | 95 ++++++++++++++++++++++ src/core/services/subsync.ts | 5 +- src/main/runtime/anime-bridge-installer.ts | 24 +++++- src/main/runtime/anime-browser-playback.ts | 3 + src/subsync/utils.test.ts | 20 ++++- src/subsync/utils.ts | 19 +++++ 10 files changed, 200 insertions(+), 8 deletions(-) create mode 100644 changes/subsync-file-url-tracks.md diff --git a/changes/subsync-file-url-tracks.md b/changes/subsync-file-url-tracks.md new file mode 100644 index 00000000..4a603aaf --- /dev/null +++ b/changes/subsync-file-url-tracks.md @@ -0,0 +1,4 @@ +type: fixed +area: subsync + +- Subsync no longer fails with `Protocol "file:" not supported` on a subtitle that was dropped onto mpv. mpv reports such a track as a percent-encoded `file://` URL, which subsync read as a stream and tried to fetch over HTTP; the URL is now decoded back to its path, so both the retimed target and an alass reference work. A `file://` video path is treated as local too, which restores the video reference and ffsubsync for a dropped file. diff --git a/src/anime-bridge/sidecar-process.test.ts b/src/anime-bridge/sidecar-process.test.ts index b24cd435..cba8365a 100644 --- a/src/anime-bridge/sidecar-process.test.ts +++ b/src/anime-bridge/sidecar-process.test.ts @@ -2,6 +2,7 @@ import test from 'node:test'; import assert from 'node:assert/strict'; import http from 'node:http'; import { EventEmitter } from 'node:events'; +import type { AddressInfo } from 'node:net'; import type { spawn as spawnType, ChildProcess } from 'node:child_process'; import { allocatePort, startSidecar } from './sidecar-process'; import type { BundleBinaries } from './sidecar-bundle'; @@ -92,7 +93,6 @@ test('an early exit is reported with its code rather than waiting out the deadli }); test('onExit reports a death after readiness, including to late subscribers', async () => { - const port = await allocatePort(); // Fake the bridge's capabilities endpoint so startSidecar reports ready. const server = http.createServer((_req, res) => { res.writeHead(200, { 'content-type': 'application/json' }); @@ -100,7 +100,10 @@ test('onExit reports a death after readiness, including to late subscribers', as JSON.stringify({ mangatanMihonBridge: 1, sourceFactory: true, preferenceCallbacks: true }), ); }); - await new Promise((resolve) => server.listen(port, '127.0.0.1', resolve)); + // Bind first and take the port the OS assigned: allocating one up front and + // binding it after leaves a window for another listener to claim it. + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)); + const { port } = server.address() as AddressInfo; const child = fakeChild(); const spawnImpl = (() => child) as unknown as typeof spawnType; diff --git a/src/anime-bridge/stream-strip-proxy.ts b/src/anime-bridge/stream-strip-proxy.ts index 00ab5024..950978ee 100644 --- a/src/anime-bridge/stream-strip-proxy.ts +++ b/src/anime-bridge/stream-strip-proxy.ts @@ -149,6 +149,10 @@ export function startStreamStripProxy( attempt: number, ): void { const mayRetry = req.method === 'GET' && attempt === 0; + // Once the response is handed off, its headers (and often part of its body) + // are already on the wire: a later upstream error can only be reported by + // killing the connection, never by retrying or writing a 502. + let handedOff = false; const retry = (): void => { setTimeout( () => requestUpstream(req, res, upstreamUrl, requestHeaders, attempt + 1), @@ -172,6 +176,7 @@ export function startStreamStripProxy( } log(`[stream-proxy] upstream ${status} for ${upstreamUrl.pathname}`); } + handedOff = true; handleUpstreamResponse(req, res, upstream); }, ); @@ -180,6 +185,11 @@ export function startStreamStripProxy( upstreamRequest.destroy(new Error(`upstream silent for ${UPSTREAM_TIMEOUT_MS}ms`)); }); upstreamRequest.on('error', (error) => { + if (handedOff) { + log(`[stream-proxy] upstream failed mid-response: ${String(error)}`); + res.destroy(); + return; + } if (mayRetry) { log(`[stream-proxy] upstream request failed: ${String(error)}; retrying once`); retry(); @@ -214,7 +224,19 @@ export function startStreamStripProxy( if (isPlaylist) { const chunks: Buffer[] = []; - upstream.on('data', (chunk: Buffer) => chunks.push(chunk)); + let buffered = 0; + upstream.on('data', (chunk: Buffer) => { + buffered += chunk.length; + // A playlist is text and small; anything this large is not one, and it + // has to be held whole in memory to be rewritten. + if (buffered > DECISION_BYTES) { + log(`[stream-proxy] playlist body over ${DECISION_BYTES} bytes; dropping`); + upstream.destroy(); + res.destroy(); + return; + } + chunks.push(chunk); + }); upstream.on('end', () => { const body = rewritePlaylistOrigins( Buffer.concat(chunks).toString('utf8'), diff --git a/src/core/services/subsync-extract.ts b/src/core/services/subsync-extract.ts index 6ec1ce96..c4a528e2 100644 --- a/src/core/services/subsync-extract.ts +++ b/src/core/services/subsync-extract.ts @@ -5,6 +5,7 @@ import { codecToExtension, fileExists, MpvTrack, + resolveLocalMediaPath, runCommand, summarizeCommandFailure, } from '../../subsync/utils'; @@ -148,10 +149,12 @@ export async function extractSubtitleTrackToFile( input: SubtitleExtractionInput, ): Promise { if (input.track.external) { - const externalPath = input.track['external-filename']; - if (typeof externalPath !== 'string' || externalPath.length === 0) { + const externalFilename = input.track['external-filename']; + if (typeof externalFilename !== 'string' || externalFilename.length === 0) { throw new Error('External subtitle track has no file path'); } + // A dropped subtitle arrives as a `file://` URL, which is local, not remote. + const externalPath = resolveLocalMediaPath(externalFilename); if (isRemoteMediaPath(externalPath)) { return downloadRemoteSubtitleTrack(externalPath, input.httpHeaders); } diff --git a/src/core/services/subsync-remote.test.ts b/src/core/services/subsync-remote.test.ts index f6657068..c231cd3f 100644 --- a/src/core/services/subsync-remote.test.ts +++ b/src/core/services/subsync-remote.test.ts @@ -4,6 +4,7 @@ import * as fs from 'fs'; import * as http from 'http'; import * as os from 'os'; import * as path from 'path'; +import { pathToFileURL } from 'url'; import { runSubsyncManual } from './subsync'; import type { TriggerSubsyncFromConfigDeps } from './subsync'; @@ -122,3 +123,97 @@ test('runSubsyncManual syncs stream subtitle tracks served over http', async (t) fs.rmSync(tmpDir, { recursive: true, force: true }); } }); + +/** + * Dropping a subtitle onto mpv hands it a `file://` URL, and mpv reports that + * URL back as the track's `external-filename`. Reading it as a stream made the + * run die with `Protocol "file:" not supported. Expected "http:"`. + */ +test('runSubsyncManual retimes a dropped file:// track against a stream reference', async (t) => { + if (process.platform === 'win32') { + t.skip('stub shell scripts are not executable on Windows'); + return; + } + + const host = await startStubHost(); + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'subsync-file-url-')); + const alassLogPath = path.join(tmpDir, 'alass-args.log'); + const alassPath = path.join(tmpDir, 'alass.sh'); + fs.writeFileSync( + alassPath, + `#!/bin/sh\n: > "${alassLogPath}"\nfor arg in "$@"; do printf '%s\\n' "$arg" >> "${alassLogPath}"; done\ncp "$2" "${tmpDir}/target.copy"\nprintf '%s' "retimed" > "$3"\nexit 0\n`, + { mode: 0o755 }, + ); + + // Percent-encoded on purpose: the space and the CJK are what a naive + // "strip file://" would leave mangled. + const targetPath = path.join(tmpDir, 'ワンピース sdh.srt'); + const targetBody = '1\n00:00:03,000 --> 00:00:04,000\nドロップ\n'; + fs.writeFileSync(targetPath, targetBody); + const referenceUrl = host.url('/subs/en.srt'); + const sentCommands: Array> = []; + + const deps: Pick = { + getMpvClient: () => ({ + connected: true, + currentAudioStreamIndex: null, + send: (payload) => { + sentCommands.push(payload.command); + }, + requestProperty: async (name: string) => { + if (name === 'path') return host.url('/stream/video.m3u8'); + if (name === 'sid') return 2; + if (name === 'secondary-sid') return null; + if (name === 'track-list') { + return [ + { + id: 1, + type: 'sub', + selected: true, + external: true, + lang: 'en', + 'external-filename': referenceUrl, + }, + { + id: 2, + type: 'sub', + selected: true, + external: true, + lang: 'ja', + 'external-filename': pathToFileURL(targetPath).href, + }, + ]; + } + return null; + }, + }), + getResolvedConfig: () => ({ + alassPath, + ffsubsyncPath: '', + ffmpegPath: path.join(tmpDir, 'no-such-ffmpeg'), + replace: true, + }), + }; + + try { + const result = await runSubsyncManual( + { engine: 'alass', referenceTrackId: 1, targetTrackId: 2 }, + deps, + ); + + assert.equal(result.ok, true, result.message); + + const alassArgs = fs.readFileSync(alassLogPath, 'utf8').trim().split('\n'); + // The dropped file went in as-is, decoded, and was retimed in place. + assert.equal(alassArgs[1], targetPath); + assert.equal(fs.readFileSync(path.join(tmpDir, 'target.copy'), 'utf8'), targetBody); + assert.equal(alassArgs[2], targetPath); + assert.equal(fs.readFileSync(targetPath, 'utf8'), 'retimed'); + + const loadCommand = sentCommands.find((command) => command[0] === 'sub-add'); + assert.equal(loadCommand?.[1], targetPath); + } finally { + await host.close(); + fs.rmSync(tmpDir, { recursive: true, force: true }); + } +}); diff --git a/src/core/services/subsync.ts b/src/core/services/subsync.ts index 37971cc6..d3892451 100644 --- a/src/core/services/subsync.ts +++ b/src/core/services/subsync.ts @@ -6,6 +6,7 @@ import { formatTrackLabel, getTrackById, MpvTrack, + resolveLocalMediaPath, runCommand, summarizeCommandFailure, SubsyncContext, @@ -136,7 +137,9 @@ async function gatherSubsyncContext(client: MpvClientLike): Promise { const declared = Number(response.headers.get('content-length') ?? '0'); const reader = response.body?.getReader(); - if (!reader) return new Uint8Array(await response.arrayBuffer()); + if (!reader) { + const buffer = await response.arrayBuffer(); + if (buffer.byteLength > MAX_BUNDLE_BYTES) throw oversizedBundle(); + return new Uint8Array(buffer); + } const chunks: Uint8Array[] = []; let received = 0; @@ -86,6 +102,12 @@ async function downloadWithProgress( const { done, value } = await reader.read(); if (done) break; if (value) { + // Checked against the bytes actually read, not content-length: a lying + // (or absent) header must not let the bundle allocate without bound. + if (received + value.length > MAX_BUNDLE_BYTES) { + await reader.cancel().catch(() => {}); + throw oversizedBundle(); + } chunks.push(value); received += value.length; if (declared > 0) onProgress?.(Math.min(1, received / declared)); diff --git a/src/main/runtime/anime-browser-playback.ts b/src/main/runtime/anime-browser-playback.ts index e3cf862a..bac382ed 100644 --- a/src/main/runtime/anime-browser-playback.ts +++ b/src/main/runtime/anime-browser-playback.ts @@ -183,6 +183,9 @@ export function createAnimeBrowserPlayback(options: AnimeBrowserPlaybackOptions) } async function dispose(): Promise { + // Bumping the generation makes any in-flight playEpisode stale, so a cache + // it is still writing gets removed by that call instead of outliving us. + playbackGeneration += 1; const cacheDir = subtitleCacheDir; subtitleCacheDir = null; await removeSubtitleCache(cacheDir, deps.subtitleCacheIo); diff --git a/src/subsync/utils.test.ts b/src/subsync/utils.test.ts index a0065f37..84aef738 100644 --- a/src/subsync/utils.test.ts +++ b/src/subsync/utils.test.ts @@ -1,6 +1,6 @@ import test from 'node:test'; import assert from 'node:assert/strict'; -import { codecToExtension, getSubsyncConfig } from './utils'; +import { codecToExtension, getSubsyncConfig, resolveLocalMediaPath } from './utils'; test('codecToExtension maps stream/web formats to ffmpeg extractable extensions', () => { assert.equal(codecToExtension('subrip'), 'srt'); @@ -22,3 +22,21 @@ test('getSubsyncConfig respects explicit replace value', () => { assert.equal(getSubsyncConfig({ replace: false }).replace, false); assert.equal(getSubsyncConfig({ replace: true }).replace, true); }); + +test('resolveLocalMediaPath decodes file URLs mpv reports for dropped files', () => { + assert.equal( + resolveLocalMediaPath('file:///home/user/subs/%E3%83%AF%E3%83%B3%20sdh.srt'), + '/home/user/subs/ワン sdh.srt', + ); + assert.equal(resolveLocalMediaPath('FILE:///tmp/ref.srt'), '/tmp/ref.srt'); +}); + +test('resolveLocalMediaPath leaves plain paths and stream URLs alone', () => { + assert.equal(resolveLocalMediaPath('/tmp/ref.srt'), '/tmp/ref.srt'); + assert.equal( + resolveLocalMediaPath('https://jellyfin.example/subs/eng.srt'), + 'https://jellyfin.example/subs/eng.srt', + ); + // A UNC host has no local path; the caller's own error is the useful one. + assert.equal(resolveLocalMediaPath('file://host/share/ref.srt'), 'file://host/share/ref.srt'); +}); diff --git a/src/subsync/utils.ts b/src/subsync/utils.ts index f7edb4a4..03b4d241 100644 --- a/src/subsync/utils.ts +++ b/src/subsync/utils.ts @@ -1,6 +1,7 @@ import * as fs from 'fs'; import * as childProcess from 'child_process'; import * as path from 'path'; +import { fileURLToPath } from 'url'; import { DEFAULT_CONFIG } from '../config'; import { SubsyncConfig } from '../types'; @@ -119,6 +120,24 @@ export function summarizeCommandFailure(command: string, result: CommandResult): return `command failed (${command}) ${parts.join(' | ')}`; } +/** + * Turn a `file://` URL back into a plain path. + * + * mpv echoes back whatever it was given, and a drag-and-drop (or a `file://` + * argument) hands it a percent-encoded URL. Everything downstream wants a real + * path: `fs` cannot stat the URL, alass and ffmpeg cannot open it, and the + * remote-track check reads it as a stream and tries to fetch a local file over + * HTTP — which fails with `Protocol "file:" not supported`. + */ +export function resolveLocalMediaPath(value: string): string { + if (!/^file:\/\//i.test(value)) return value; + try { + return fileURLToPath(new URL(value)); + } catch { + return value; + } +} + export function fileExists(pathOrEmpty: string): boolean { if (!pathOrEmpty) return false; try {